Enable timeout propagation for remote actions

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

@@ -118,7 +118,31 @@ public String parseIfMatches(String tag) throws ValidationException {
   }
 
   /** If specified, the timeout of this action in seconds. Must be decimal integer. */
-  public static final String TIMEOUT = "timeout";
+  public static final ParseableRequirement TIMEOUT =
+      ParseableRequirement.create(
+          "timeout:<int>",
+          Pattern.compile("timeout:(.+)"),
+          s -> {
+            Preconditions.checkNotNull(s);
+
+            int value;
+            try {
+              value = Integer.parseInt(s);
+            } catch (NumberFormatException e) {
+              return "can't be parsed as an integer";
+            }
+
+            // De-and-reserialize & compare to only allow canonical integer formats.
+            if (!Integer.toString(value).equals(s)) {
+              return "must be in canonical format (e.g. '4' instead of '+04')";
+            }
+
+            if (value < 1) {
+              return "can't be zero or negative";
+            }
+
+            return null;
+          });
 
   /** If an action would not successfully run other than on Darwin. */
   public static final String REQUIRES_DARWIN = "requires-darwin";

src/main/java/com/google/devtools/build/lib/actions/Spawns.java

@@ -14,9 +14,12 @@
 
 package com.google.devtools.build.lib.actions;
 
+import com.google.devtools.build.lib.actions.ExecutionRequirements.ParseableRequirement.ValidationException;
+import com.google.devtools.build.lib.actions.ExecutionRequirements.ParseableRequirement;
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
+
 import java.io.IOException;
 import java.time.Duration;
 import java.util.Map;
@@ -152,21 +155,72 @@ public static Duration getTimeout(Spawn spawn) throws ExecException {
    * defaultTimeout, or {@code Duration.ZERO} if that is null.
    */
   public static Duration getTimeout(Spawn spawn, Duration defaultTimeout) throws ExecException {
-    String timeoutStr = spawn.getExecutionInfo().get(ExecutionRequirements.TIMEOUT);
-    if (timeoutStr == null) {
-      return defaultTimeout == null ? Duration.ZERO : defaultTimeout;
+    Duration timeout = null;
+
+    for (Map.Entry<String, String> entry: spawn.getExecutionInfo().entrySet()) {
+        String value = getTimeoutValueFromExecutionInfoKeyOrValue(spawn,entry);
+        if (value != null) {
+            if (timeout != null) {
+                throw new UserExecException(
+                    FailureDetail.newBuilder()
+                        .setMessage("multiple timeout values specified")
+                        .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.DUPLICATE_TIMEOUT_TAGS))
+                        .build());
+            }
+            timeout = parseTimeoutValue(value);
+        }
+    }
+
+    if (timeout == null) {
+        timeout = defaultTimeout == null ? Duration.ZERO : defaultTimeout;
     }
+    return timeout;
+}
+
+/**
+ * Retrieves the timeout value from the execution info key or value.
+ * Timeout may either be specified as a tag on format timeout:<int>
+ * or as a key-value pair when specified as an execution_requirement.
+ */
+private static String getTimeoutValueFromExecutionInfoKeyOrValue(Spawn spawn, Map.Entry<String, String> entry) throws ExecException {
+    ParseableRequirement requirement = ExecutionRequirements.TIMEOUT;
+    String value = null;
+
+    if (entry.getKey().equals("timeout")) {
+        value = entry.getValue();
+    } else {
+        try {
+            value = requirement.parseIfMatches(entry.getKey());
+        } catch (ValidationException e) {
+            throw new UserExecException(
+                e,
+                FailureDetail.newBuilder()
+                    .setMessage(
+                        String.format(
+                            "%s has a '%s' tag, but its value '%s' didn't pass validation: %s",
+                            spawn.getTargetLabel(), requirement.userFriendlyName(), e.getTagValue(), e.getMessage()))
+                    .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT_TAG))
+                    .build());
+        }
+    }
+    return value;
+}
+
+/**
+ * Parses a timeout value from a string and returns it as a {@link Duration}.
+ */
+private static Duration parseTimeoutValue(String value) throws ExecException {
     try {
-      return Duration.ofSeconds(Integer.parseInt(timeoutStr));
+        return Duration.ofSeconds(Integer.parseInt(value));
     } catch (NumberFormatException e) {
-      throw new UserExecException(
-          e,
-          FailureDetail.newBuilder()
-              .setMessage("could not parse timeout")
-              .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT))
-              .build());
+        throw new UserExecException(
+            e,
+            FailureDetail.newBuilder()
+                .setMessage("could not parse timeout")
+                .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT))
+                .build());
     }
-  }
+}
 
   /**
    * Returns whether a local {@link Spawn} runner implementation should prefetch the inputs before

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

@@ -119,7 +119,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
       // for this.
       executionInfo.put(ExecutionRequirements.NO_CACHE, "");
     }
-    executionInfo.put(ExecutionRequirements.TIMEOUT, "" + getTimeout(action).toSeconds());
+    executionInfo.put("timeout", "" + getTimeout(action).toSeconds());
 
     SimpleSpawn.LocalResourcesSupplier localResourcesSupplier =
         () ->

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

@@ -58,6 +58,7 @@ private static boolean legalExecInfoKeys(String tag) {
         || tag.startsWith("disable-")
         || tag.startsWith("cpu:")
         || tag.equals(ExecutionRequirements.LOCAL)
+        || tag.startsWith("timeout")
         || tag.equals(ExecutionRequirements.WORKER_KEY_MNEMONIC)
         || tag.startsWith("resources:");
   }

src/main/protobuf/failure_details.proto

@@ -222,6 +222,8 @@ message Spawn {
     COMMAND_LINE_EXPANSION_FAILURE = 7 [(metadata) = { exit_code: 1 }];
     EXEC_IO_EXCEPTION = 8 [(metadata) = { exit_code: 36 }];
     INVALID_TIMEOUT = 9 [(metadata) = { exit_code: 1 }];
+    DUPLICATE_TIMEOUT_TAGS = 16 [(metadata) = { exit_code: 1 }];
+    INVALID_TIMEOUT_TAG = 17 [(metadata) = { exit_code: 1 }];
     INVALID_REMOTE_EXECUTION_PROPERTIES = 10 [(metadata) = { exit_code: 1 }];
     NO_USABLE_STRATEGY_FOUND = 11 [(metadata) = { exit_code: 1 }];
     // TODO(b/138456686): this code should be deprecated when SpawnResult is

src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java

@@ -223,6 +223,19 @@ public void testExecutionInfo_withPrefixCpu() throws Exception {
     assertThat(execInfo).containsExactly("cpu:123", "");
   }
 
+  @Test
+  public void testExecutionInfo_withPrefixTimeout() throws Exception {
+    scratch.file(
+        "tests/BUILD",
+        "load('//test_defs:foo_binary.bzl', 'foo_binary')",
+        "foo_binary(name = 'with-prefix-timeout', srcs=['sh.sh'], tags=['timeout:123', 'wrong-tag'])");
+
+    Rule withCpuPrefix = (Rule) getTarget("//tests:with-prefix-timeout");
+
+    Map<String, String> execInfo = TargetUtils.getExecutionInfo(withCpuPrefix);
+    assertThat(execInfo).containsExactly("timeout:123", "");
+  }
+
   @Test
   public void testExecutionInfo_withLocalTag() throws Exception {
     scratch.file(

src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java

@@ -416,7 +416,7 @@ public void testCreateSpawnActionEnvAndExecInfo() throws Exception {
         "  inputs = ruleContext.files.srcs,",
         "  outputs = ruleContext.files.srcs,",
         "  env = {'a' : 'b'},",
-        "  execution_requirements = {'timeout' : '10', 'block-network' : 'foo'},",
+        "  execution_requirements = {'block-network' : 'foo'},",
         "  mnemonic = 'DummyMnemonic',",
         "  command = 'dummy_command',",
         "  progress_message = 'dummy_message')");
@@ -425,10 +425,31 @@ public void testCreateSpawnActionEnvAndExecInfo() throws Exception {
             Iterables.getOnlyElement(
                 ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions());
     assertThat(action.getIncompleteEnvironmentForTesting()).containsExactly("a", "b");
-    // We expect "timeout" to be filtered by TargetUtils.
     assertThat(action.getExecutionInfo()).containsExactly("block-network", "foo");
   }
 
+  @Test
+  public void testCreateSpawnActionEnvAndExecInfo_withTimeout() throws Exception {
+    StarlarkRuleContext ruleContext = createRuleContext("//foo:foo");
+    setRuleContext(ruleContext);
+    ev.exec(
+        "ruleContext.actions.run_shell(",
+        "  inputs = ruleContext.files.srcs,",
+        "  outputs = ruleContext.files.srcs,",
+        "  execution_requirements = {",
+        "    'timeout': '42',",
+        "  },",
+        "  mnemonic = 'DummyMnemonic',",
+        "  command = 'dummy_command',",
+        "  progress_message = 'dummy_message')");
+    SpawnAction action =
+        (SpawnAction)
+            Iterables.getOnlyElement(
+                ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions());
+    assertThat(action.getExecutionInfo())
+        .containsExactly("timeout", "42");
+  }
+
   @Test
   public void testCreateSpawnActionEnvAndExecInfo_withWorkerKeyMnemonic() throws Exception {
     StarlarkRuleContext ruleContext = createRuleContext("//foo:foo");