Simplify exception handling in spawn strategies

The main change here is to only catch SpawnExecException in
StandaloneTestStrategy, so all other exceptions simplify propagate up. As a
result, Bazel no longer retries tests that fail with an exception, we only
retry tests that actually ran, had a spawn result, and resulted in a
UserExecException. That is probably what we want.

Also do some cleanup:
- Remove ExecException.timedOut; nobody was calling it (but there's still
  SpawnExecException.timedOut)
- Remove SpawnActionContext.shouldPropagateExecException; all exceptions
  (except SpawnExecException) are now propagated by default
- Remote the SandboxOptions from the SandboxStrategies; all sandboxing options
  are now handled by the underlying SpawnRunner implementations

I'll send a followup CL to remove the UserExecException and
EnvironmentalExecException types; the types don't do anything special, and
there are no catch blocks in production code that catch one of these more
specific types.

This should fix #3322 by removing a bunch of special handling.

PiperOrigin-RevId: 161960919

11 files changed, 5 insertions, 48 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecException.java b/src/main/java/com/google/devtools/build/lib/actions/ExecException.java
index 16f4550195..a217ac259b 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ExecException.java
@@ -98,12 +98,4 @@ public abstract class ExecException extends Exception {
    */
   public abstract ActionExecutionException toActionExecutionException(String messagePrefix,
         boolean verboseFailures, Action action);
-
-
-  /**
-   * Tells if the execution exception was thrown because of the execution timing out.
-   */
-  public boolean hasTimedOut() {
-    return false;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java
index b57d4e255b..e37887ffd8 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java
@@ -22,11 +22,4 @@ public interface SpawnActionContext extends ActionContext {
   /** Executes the given spawn. */
   void exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException;
-
-  /**
-   * If an ExecException should be rethrown by the strategy that executed this.
-   * Currently only works for LinuxSandboxedStrategy:
-   * If true, will throw ExecException and give reproduction instruction for sandbox.
-   */
-  boolean shouldPropagateExecException();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java
index 4c9bd95372..bd9f5d8485 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java
@@ -49,7 +49,6 @@ public class SpawnExecException extends ExecException {
     return result;
   }
 
-  @Override
   public boolean hasTimedOut() {
     return getSpawnResult().status() == Status.TIMEOUT;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index 8ef6049ae8..acbf5c4963 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -325,15 +325,14 @@ public class StandaloneTestStrategy extends TestStrategy {
             .setTestPassed(true)
             .setStatus(BlazeTestStatus.PASSED)
             .setPassedLog(testLogPath.getPathString());
-      } catch (ExecException e) {
-        // Execution failed, which we consider a test failure.
+      } catch (SpawnExecException e) {
+        // If this method returns normally, then the higher level will rerun the test (up to
+        // --flaky_test_attempts times). We don't catch any other ExecException here, so those never
+        // get retried.
         builder
             .setTestPassed(false)
             .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED)
             .addFailedLogs(testLogPath.getPathString());
-        if (spawnActionContext.shouldPropagateExecException()) {
-          throw e;
-        }
       } finally {
         long duration = actionExecutionContext.getClock().currentTimeMillis() - startTime;
         builder.setStartTimeMillisEpoch(startTime);
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
index 87c00ee5c6..44d62b6493 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java
@@ -167,9 +167,4 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
       throw new SpawnExecException(message, result, /*catastrophe=*/ false);
     }
   }
-
-  @Override
-  public boolean shouldPropagateExecException() {
-    return false;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
index 38c8645f2c..29b572fe53 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
@@ -36,7 +36,6 @@ final class DarwinSandboxedStrategy extends SandboxStrategy {
       String productName) throws IOException {
     super(
         verboseFailures,
-        buildRequest.getOptions(SandboxOptions.class),
         new DarwinSandboxedSpawnRunner(cmdEnv, buildRequest, sandboxBase, productName));
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
index 3d556d3bfc..8b7b2b7fdd 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
@@ -38,7 +38,6 @@ public final class LinuxSandboxedStrategy extends SandboxStrategy {
       Path inaccessibleHelperDir) {
     super(
         verboseFailures,
-        buildRequest.getOptions(SandboxOptions.class),
         new LinuxSandboxedSpawnRunner(
             cmdEnv, buildRequest, sandboxBase, inaccessibleHelperFile, inaccessibleHelperDir));
   }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java
index 8c770a8bb2..1e59735a0f 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java
@@ -35,7 +35,6 @@ final class ProcessWrapperSandboxedStrategy extends SandboxStrategy {
       String productName) {
     super(
         verboseFailures,
-        buildRequest.getOptions(SandboxOptions.class),
         new ProcessWrapperSandboxedSpawnRunner(cmdEnv, buildRequest, sandboxBase, productName));
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
index f290b812c1..7b325f61dc 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
@@ -43,16 +43,13 @@ import java.util.concurrent.atomic.AtomicReference;
 /** Abstract common ancestor for sandbox strategies implementing the common parts. */
 abstract class SandboxStrategy implements SandboxedSpawnActionContext {
   private final boolean verboseFailures;
-  private final SandboxOptions sandboxOptions;
   private final SpawnInputExpander spawnInputExpander;
   private final AbstractSandboxSpawnRunner spawnRunner;
 
   public SandboxStrategy(
       boolean verboseFailures,
-      SandboxOptions sandboxOptions,
       AbstractSandboxSpawnRunner spawnRunner) {
     this.verboseFailures = verboseFailures;
-    this.sandboxOptions = sandboxOptions;
     this.spawnInputExpander = new SpawnInputExpander(false);
     this.spawnRunner = spawnRunner;
   }
@@ -141,7 +138,7 @@ abstract class SandboxStrategy implements SandboxedSpawnActionContext {
     if (result.status() != Status.SUCCESS || result.exitCode() != 0) {
       String message =
           CommandFailureUtils.describeCommandFailure(
-              true, spawn.getArguments(), spawn.getEnvironment(), null);
+              verboseFailures, spawn.getArguments(), spawn.getEnvironment(), null);
       throw new SpawnExecException(
           message, result, /*forciblyRunRemotely=*/false, /*catastrophe=*/false);
     }
@@ -151,9 +148,4 @@ abstract class SandboxStrategy implements SandboxedSpawnActionContext {
   public String toString() {
     return "sandboxed";
   }
-
-  @Override
-  public boolean shouldPropagateExecException() {
-    return verboseFailures && sandboxOptions.sandboxDebug;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
index b1e0e9d93b..4a637d0001 100644
--- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java
@@ -154,9 +154,4 @@ public class StandaloneSpawnStrategy implements SpawnActionContext {
   public String toString() {
     return "standalone";
   }
-
-  @Override
-  public boolean shouldPropagateExecException() {
-    return false;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
index 595751c124..5688933b94 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java
@@ -397,9 +397,4 @@ public final class WorkerSpawnStrategy implements SandboxedSpawnActionContext {
   public String toString() {
     return "worker";
   }
-
-  @Override
-  public boolean shouldPropagateExecException() {
-    return false;
-  }
 }