Stop manually overriding failure detail's coarse exit code in SpawnExecException#toDetailedExitCode. Instead, trust it. This requires changing some failure codes' associated coarse exit codes so that they are properly categorized: a code like Spawn#EXEC_FAILED that is associated with a "system" error status needs to have an exit code of 34 to reflect that.
#11151
PiperOrigin-RevId: 335648755
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 47935b4..da5ea0f 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
@@ -15,10 +15,13 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.shell.TerminationStatus;
+import com.google.devtools.build.lib.util.DetailedExitCode;
+import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.vfs.Path;
import com.google.protobuf.ByteString;
import java.io.InputStream;
@@ -459,18 +462,38 @@
public SpawnResult build() {
Preconditions.checkArgument(!runnerName.isEmpty());
- if (status == Status.SUCCESS) {
- Preconditions.checkArgument(exitCode == 0, exitCode);
- } else if (status == Status.TIMEOUT) {
- Preconditions.checkArgument(exitCode == POSIX_TIMEOUT_EXIT_CODE, exitCode);
- } else if (status == Status.NON_ZERO_EXIT || status == Status.OUT_OF_MEMORY) {
- Preconditions.checkArgument(exitCode != 0, exitCode);
+ switch (status) {
+ case SUCCESS:
+ Preconditions.checkArgument(exitCode == 0, exitCode);
+ Preconditions.checkArgument(failureDetail == null, failureDetail);
+ break;
+ case TIMEOUT:
+ Preconditions.checkArgument(exitCode == POSIX_TIMEOUT_EXIT_CODE, exitCode);
+ // Fall through.
+ default:
+ Preconditions.checkArgument(
+ exitCode != 0,
+ "Failed spawn with status %s had exit code 0 (%s %s)",
+ status,
+ failureMessage,
+ failureDetail);
+ Preconditions.checkArgument(
+ failureDetail != null,
+ "Failed spawn with status %s and exit code %s had no failure detail (%s)",
+ status,
+ exitCode,
+ failureMessage);
+ if (!status.isConsideredUserError()
+ && ExitCode.BUILD_FAILURE.equals(DetailedExitCode.getExitCode(failureDetail))) {
+ BugReport.sendBugReport(
+ new IllegalStateException(
+ String.format(
+ "System error %s should not have failure detail %s with 'build failure'"
+ + " exit code (%s)",
+ status, failureDetail, failureMessage)));
+ }
}
- // TODO(mschaller): Once SimpleSpawnResult.Builder's uses have picked up FailureDetails for
- // unsuccessful spawns, add a precondition that asserts failureDetail's nullity is the same
- // as whether status is SUCCESS.
-
return new SimpleSpawnResult(this);
}
@@ -499,11 +522,6 @@
return this;
}
- public Builder setRunnerSubtype(String runnerSubtype) {
- this.runnerSubtype = runnerSubtype;
- return this;
- }
-
public Builder setSpawnMetrics(SpawnMetrics spawnMetrics) {
this.spawnMetrics = spawnMetrics;
return this;
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 9760ff8..8e255ba 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
@@ -22,11 +22,7 @@
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
-import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
-import com.google.devtools.build.lib.server.FailureDetails.Spawn;
-import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code;
import com.google.devtools.build.lib.util.DetailedExitCode;
-import com.google.devtools.build.lib.util.ExitCode;
/**
* A specialization of {@link ExecException} that indicates something went wrong when trying to
@@ -71,21 +67,6 @@
String message =
result.getDetailMessage(messagePrefix, getMessage(), isCatastrophic(), forciblyRunRemotely);
return new ActionExecutionException(
- message, this, action, isCatastrophic(), getDetailedExitCode());
- }
-
- /** Return detailed exit code depending on the spawn result. */
- private DetailedExitCode getDetailedExitCode() {
- ExitCode exitCode =
- result.status().isConsideredUserError() ? ExitCode.BUILD_FAILURE : ExitCode.REMOTE_ERROR;
- if (result.failureDetail() == null) {
- return DetailedExitCode.of(
- exitCode,
- FailureDetail.newBuilder()
- .setMessage("spawn failed")
- .setSpawn(Spawn.newBuilder().setCode(Code.UNSPECIFIED_EXECUTION_FAILURE))
- .build());
- }
- return DetailedExitCode.of(exitCode, result.failureDetail());
+ message, this, action, isCatastrophic(), DetailedExitCode.of(result.failureDetail()));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/util/DetailedExitCode.java b/src/main/java/com/google/devtools/build/lib/util/DetailedExitCode.java
index 847694a..136669d 100644
--- a/src/main/java/com/google/devtools/build/lib/util/DetailedExitCode.java
+++ b/src/main/java/com/google/devtools/build/lib/util/DetailedExitCode.java
@@ -42,7 +42,7 @@
}
/** Returns the registered {@link ExitCode} associated with a {@link FailureDetail} message. */
- private static ExitCode getExitCode(FailureDetail failureDetail) {
+ public static ExitCode getExitCode(FailureDetail failureDetail) {
// TODO(mschaller): Consider specializing for unregistered exit codes here, if absolutely
// necessary.
int numericExitCode = getNumericExitCode(failureDetail);
@@ -123,7 +123,7 @@
* Returns the numeric exit code associated with a {@link FailureDetail} submessage's subcategory
* enum value.
*/
- private static int getNumericExitCode(EnumValueDescriptor subcategoryDescriptor) {
+ public static int getNumericExitCode(EnumValueDescriptor subcategoryDescriptor) {
checkArgument(
subcategoryDescriptor.getOptions().hasExtension(FailureDetails.metadata),
"Enum value %s has no FailureDetails.metadata",
diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto
index 82537cd..b854f12 100644
--- a/src/main/protobuf/failure_details.proto
+++ b/src/main/protobuf/failure_details.proto
@@ -203,7 +203,7 @@
// Note: Spawn OUT_OF_MEMORY leads to a BUILD_FAILURE exit_code because the
// build tool itself did not run out of memory.
OUT_OF_MEMORY = 3 [(metadata) = { exit_code: 1 }];
- EXECUTION_FAILED = 4 [(metadata) = { exit_code: 1 }];
+ EXECUTION_FAILED = 4 [(metadata) = { exit_code: 34 }];
EXECUTION_DENIED = 5 [(metadata) = { exit_code: 1 }];
REMOTE_CACHE_FAILED = 6 [(metadata) = { exit_code: 34 }];
COMMAND_LINE_EXPANSION_FAILURE = 7 [(metadata) = { exit_code: 1 }];
diff --git a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
index 108bf66..fee0304 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
@@ -18,6 +18,9 @@
import com.google.devtools.build.lib.actions.SpawnResult.MetadataLog;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
+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 com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.util.FileSystems;
import com.google.protobuf.ByteString;
@@ -39,6 +42,10 @@
.setStatus(SpawnResult.Status.TIMEOUT)
.setWallTime(Duration.ofSeconds(5))
.setExitCode(SpawnResult.POSIX_TIMEOUT_EXIT_CODE)
+ .setFailureDetail(
+ FailureDetail.newBuilder()
+ .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.TIMEOUT))
+ .build())
.setRunnerName("test")
.build();
assertThat(r.getDetailMessage("", "", false, false))
@@ -51,6 +58,10 @@
new SpawnResult.Builder()
.setStatus(SpawnResult.Status.TIMEOUT)
.setExitCode(SpawnResult.POSIX_TIMEOUT_EXIT_CODE)
+ .setFailureDetail(
+ FailureDetail.newBuilder()
+ .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.TIMEOUT))
+ .build())
.setRunnerName("test")
.build();
assertThat(r.getDetailMessage("", "", false, false)).contains("(failed due to timeout.)");
@@ -75,7 +86,7 @@
}
@Test
- public void getSpawnResultLogs() throws Exception {
+ public void getSpawnResultLogs() {
SpawnResult.Builder builder =
new SpawnResult.Builder().setStatus(Status.SUCCESS).setExitCode(0).setRunnerName("test");
diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
index 1e503ab..6901136 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
@@ -42,6 +42,9 @@
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.exec.util.SpawnBuilder;
import com.google.devtools.build.lib.remote.options.RemoteOptions;
+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 com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.testutil.Suite;
import com.google.devtools.build.lib.testutil.TestSpec;
@@ -64,6 +67,11 @@
@RunWith(JUnit4.class)
@TestSpec(size = Suite.SMALL_TESTS)
public class AbstractSpawnStrategyTest {
+ private static final FailureDetail NON_ZERO_EXIT_DETAILS =
+ FailureDetail.newBuilder()
+ .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NON_ZERO_EXIT))
+ .build();
+
private static class TestedSpawnStrategy extends AbstractSpawnStrategy {
public TestedSpawnStrategy(Path execRoot, SpawnRunner spawnRunner) {
super(execRoot, spawnRunner, /*verboseFailures=*/ true);
@@ -116,6 +124,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(1)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setRunnerName("test")
.build();
when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class)))
@@ -188,6 +197,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(1)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setRunnerName("test")
.build();
when(spawnRunner.execAsync(any(Spawn.class), any(SpawnExecutionContext.class)))
@@ -383,6 +393,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(23)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setRunnerName("runner")
.build()));
when(actionExecutionContext.getMetadataProvider()).thenReturn(mock(MetadataProvider.class));
diff --git a/src/test/java/com/google/devtools/build/lib/exec/BUILD b/src/test/java/com/google/devtools/build/lib/exec/BUILD
index 1ef8109..4319a88 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/exec/BUILD
@@ -65,6 +65,7 @@
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
"//src/main/java/com/google/devtools/common/options",
+ "//src/main/protobuf:failure_details_java_proto",
"//src/main/protobuf:spawn_java_proto",
"//src/main/protobuf:test_status_java_proto",
"//src/test/java/com/google/devtools/build/lib/actions/util",
diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
index b8cb751..be2f78c 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java
@@ -59,6 +59,9 @@
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.exec.StandaloneTestStrategy.StandaloneFailedAttemptResult;
import com.google.devtools.build.lib.exec.util.TestExecutorBuilder;
+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 com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.io.FileOutErr;
@@ -85,6 +88,10 @@
/** Unit tests for {@link StandaloneTestStrategy}. */
@RunWith(JUnit4.class)
public final class StandaloneTestStrategyTest extends BuildViewTestCase {
+ private static final FailureDetail NON_ZERO_EXIT_DETAILS =
+ FailureDetail.newBuilder()
+ .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NON_ZERO_EXIT))
+ .build();
private static class TestedStandaloneTestStrategy extends StandaloneTestStrategy {
TestResult postedResult = null;
@@ -327,6 +334,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(1)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setWallTime(Duration.ofMillis(10))
.setRunnerName("test")
.build();
@@ -522,6 +530,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(1)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setRunnerName("test")
.build();
when(spawnStrategy.beginExecution(any(), any()))
@@ -614,6 +623,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(1)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setRunnerName("test")
.build();
SpawnResult xmlGeneratorSpawnResult =
@@ -893,6 +903,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(1)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setRunnerName("test")
.build();
when(spawnStrategy.beginExecution(any(), any()))
@@ -983,6 +994,7 @@
new SpawnResult.Builder()
.setStatus(Status.NON_ZERO_EXIT)
.setExitCode(1)
+ .setFailureDetail(NON_ZERO_EXIT_DETAILS)
.setRunnerName("test")
.build();
when(spawnStrategy.beginExecution(any(), any()))
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD
index 6e854c1..d8b77f6 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -82,6 +82,7 @@
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
"//src/main/java/com/google/devtools/common/options",
+ "//src/main/protobuf:failure_details_java_proto",
"//src/main/protobuf:remote_execution_log_java_proto",
"//src/test/java/com/google/devtools/build/lib:test_runner",
"//src/test/java/com/google/devtools/build/lib/actions/util",
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 3ee5392..c385e45 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -36,7 +36,6 @@
import com.google.devtools.build.lib.actions.ActionContext;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputHelper;
-import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.MetadataProvider;
@@ -64,6 +63,9 @@
import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
+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 com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
@@ -93,12 +95,7 @@
@RunWith(JUnit4.class)
public class RemoteSpawnCacheTest {
private static final ArtifactExpander SIMPLE_ARTIFACT_EXPANDER =
- new ArtifactExpander() {
- @Override
- public void expand(Artifact artifact, Collection<? super Artifact> output) {
- output.add(artifact);
- }
- };
+ (artifact, output) -> output.add(artifact);
private FileSystem fs;
private DigestUtil digestUtil;
@@ -125,7 +122,7 @@
public void prefetchInputs() {}
@Override
- public void lockOutputFiles() throws InterruptedException {
+ public void lockOutputFiles() {
throw new UnsupportedOperationException();
}
@@ -453,6 +450,10 @@
new SpawnResult.Builder()
.setExitCode(1)
.setStatus(Status.NON_ZERO_EXIT)
+ .setFailureDetail(
+ FailureDetail.newBuilder()
+ .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.NON_ZERO_EXIT))
+ .build())
.setRunnerName("test")
.build();
ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
