Simplify SpawnRunner interface

It turns out that the SUCCESS status is often misunderstood to mean "zero exit",
even though this is clearly documented. I've decided to add another status for
non-zero exit, and use success only for zero exit to avoid this pitfall.

Also, many of the status codes are set, but never used. I decided to reduce the
number of status codes to only those that are actually relevant, which
simplifies further processing. Instead, we should add a string message for the
error case when we need one - we're not using it right now, so I decided not to
add that yet.

PiperOrigin-RevId: 177129441

1 files changed, 41 insertions, 56 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
index 8040946774..c130de86c2 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
@@ -33,12 +33,12 @@ import javax.annotation.Nullable;
 public interface SpawnResult {
   /** The status of the attempted Spawn execution. */
   public enum Status {
-    /**
-     * Subprocess executed successfully, but may have returned a non-zero exit code. See
-     * {@link #exitCode} for the actual exit code.
-     */
+    /** Subprocess executed successfully, and returned a zero exit code. */
     SUCCESS(true),
 
+    /** Subprocess executed successfully, but returned a non-zero exit code. */
+    NON_ZERO_EXIT(true),
+
     /** Subprocess execution timed out. */
     TIMEOUT(true),
 
@@ -49,46 +49,22 @@ public interface SpawnResult {
     OUT_OF_MEMORY(true),
 
     /**
-     * Subprocess did not execute for an unknown reason - only use this if none of the more specific
-     * status codes apply.
+     * Subprocess did not execute. This error is not catastrophic - Bazel will try to continue the
+     * build if keep_going is enabled, possibly attempt to rerun the same spawn, and attempt to run
+     * other actions.
      */
     EXECUTION_FAILED,
 
-    /** The attempted subprocess was disallowed by a user setting. */
-    LOCAL_ACTION_NOT_ALLOWED(true),
-
-    /** The Spawn referred to an non-existent absolute or relative path. */
-    COMMAND_NOT_FOUND,
-
     /**
-     * One of the Spawn inputs was a directory. For backwards compatibility, some
-     * {@link SpawnRunner} implementations may attempt to run the subprocess anyway. Note that this
-     * leads to incremental correctness issues, as Bazel does not track dependencies on directories.
+     * Subprocess did not execute. Do not retry, and exit immediately even if keep_going is enabled.
      */
-    DIRECTORY_AS_INPUT_DISALLOWED(true),
+    EXECUTION_FAILED_CATASTROPHICALLY,
 
     /**
-     * Too many input files - remote execution systems may refuse to execute subprocesses with an
-     * excessive number of input files.
+     * Subprocess did not execute in a way that indicates that the user can fix it. For example, a
+     * remote system may have denied the execution due to too many inputs or too large inputs.
      */
-    TOO_MANY_INPUT_FILES(true),
-
-    /**
-     * Total size of inputs is too large - remote execution systems may refuse to execute
-     * subprocesses if the total size of all inputs exceeds a limit.
-     */
-    INPUTS_TOO_LARGE(true),
-
-    /**
-     * One of the input files to the Spawn was modified during the build - some {@link SpawnRunner}
-     * implementations cache checksums and may detect such modifications on a best effort basis.
-     */
-    FILE_MODIFIED_DURING_BUILD(true),
-
-    /**
-     * The {@link SpawnRunner} was unable to establish a required network connection.
-     */
-    CONNECTION_FAILED,
+    EXECUTION_DENIED(true),
 
     /**
      * The remote execution system is overloaded and had to refuse execution for this Spawn.
@@ -99,18 +75,7 @@ public interface SpawnResult {
      * The result of the remotely executed Spawn could not be retrieved due to errors in the remote
      * caching layer.
      */
-    REMOTE_CACHE_FAILED,
-
-    /**
-     * The remote execution system did not allow the request due to missing authorization or
-     * authentication.
-     */
-    NOT_AUTHORIZED(true),
-
-    /**
-     * The Spawn was malformed.
-     */
-    INVALID_ARGUMENT;
+    REMOTE_CACHE_FAILED;
 
     private final boolean isUserError;
 
@@ -128,21 +93,30 @@ public interface SpawnResult {
   }
 
   /**
-   * Returns whether the spawn was actually run, regardless of the exit code. I.e., returns if
-   * status == SUCCESS || status == TIMEOUT || status == OUT_OF_MEMORY. Returns false if there were
-   * errors that prevented the spawn from being run, such as network errors, missing local files,
-   * errors setting up sandboxing, etc.
+   * Returns whether the spawn was actually run, regardless of the exit code. I.e., returns
+   * {@code true} if {@link #status} is any of {@link Status#SUCCESS}, {@link Status#NON_ZERO_EXIT},
+   * {@link Status#TIMEOUT} or {@link Status#OUT_OF_MEMORY}.
+   *
+   * <p>Returns false if there were errors that prevented the spawn from being run, such as network
+   * errors, missing local files, errors setting up sandboxing, etc.
    */
   boolean setupSuccess();
 
+  /** Returns true if the status was {@link Status#EXECUTION_FAILED_CATASTROPHICALLY}. */
   boolean isCatastrophe();
 
   /** The status of the attempted Spawn execution. */
   Status status();
 
   /**
-   * The exit code of the subprocess if the subprocess was executed. Check {@link #status} for
-   * {@link Status#SUCCESS} before calling this method.
+   * The exit code of the subprocess if the subprocess was executed. Should only be called if
+   * {@link #status} returns {@link Status#SUCCESS} or {@link Status#NON_ZERO_EXIT}.
+   *
+   * <p>This method must return a non-zero exit code if the status is {@link Status#TIMEOUT} or
+   * {@link Status#OUT_OF_MEMORY}. It is recommended to return 128 + 14 when the status is
+   * {@link Status#TIMEOUT}, which corresponds to the Unix signal SIGALRM.
+   *
+   * <p>This method may throw {@link IllegalStateException} if called for any other status.
    */
   int exitCode();
 
@@ -208,12 +182,15 @@ public interface SpawnResult {
 
     @Override
     public boolean setupSuccess() {
-      return status == Status.SUCCESS || status == Status.TIMEOUT || status == Status.OUT_OF_MEMORY;
+      return status == Status.SUCCESS
+          || status == Status.NON_ZERO_EXIT
+          || status == Status.TIMEOUT
+          || status == Status.OUT_OF_MEMORY;
     }
 
     @Override
     public boolean isCatastrophe() {
-      return false;
+      return status == Status.EXECUTION_FAILED_CATASTROPHICALLY;
     }
 
     @Override
@@ -295,6 +272,14 @@ public interface SpawnResult {
     private boolean cacheHit;
 
     public SpawnResult build() {
+      if (status == Status.SUCCESS) {
+        Preconditions.checkArgument(exitCode == 0);
+      }
+      if (status == Status.NON_ZERO_EXIT
+          || status == Status.TIMEOUT
+          || status == Status.OUT_OF_MEMORY) {
+        Preconditions.checkArgument(exitCode != 0);
+      }
       return new SimpleSpawnResult(this);
     }