Clean up bug reports for unexpected exceptions and do some general clean-up of SkyframeErrorProcessor. Severity of most unexpected exceptions is reduced to "logUnexpected". However, places where we encounter undetailed exceptions have been promoted to "sendBugReport" since they indicate a failure to attribute responsibility, which could affect downstream systems.
PiperOrigin-RevId: 438867002
diff --git a/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java b/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java
index 6fbb571..d184f44 100644
--- a/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java
+++ b/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java
@@ -135,6 +135,20 @@
}
}
+ /** See {@link #logUnexpected(String, Object...)}. */
+ @FormatMethod
+ public static void logUnexpected(Exception e, @FormatString String message, Object... args) {
+ if (SHOULD_NOT_SEND_BUG_REPORT_BECAUSE_IN_TEST) {
+ sendBugReport(new IllegalStateException(String.format(message, args), e));
+ } else {
+ logger
+ .atWarning()
+ .atMostEvery(50, MILLISECONDS)
+ .withCause(e)
+ .logVarargs("Unexpected state: " + message, args);
+ }
+ }
+
/**
* Convenience method for {@link #sendBugReport(Throwable)}, sending a bug report with a default
* {@link IllegalStateException}.
diff --git a/src/main/java/com/google/devtools/build/lib/bugreport/BugReporter.java b/src/main/java/com/google/devtools/build/lib/bugreport/BugReporter.java
index a91d10b..cb7e22a 100644
--- a/src/main/java/com/google/devtools/build/lib/bugreport/BugReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/bugreport/BugReporter.java
@@ -37,6 +37,12 @@
BugReport.logUnexpected(message, args);
}
+ /** See {@link BugReport#logUnexpected}. */
+ @FormatMethod
+ default void logUnexpected(Exception e, @FormatString String message, Object... args) {
+ BugReport.logUnexpected(e, message, args);
+ }
+
/** Reports an exception, see {@link BugReport#sendBugReport(String, Object...)}. */
@FormatMethod
default void sendBugReport(@FormatString String message, Object... args) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index e24c049..957a74a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -1302,11 +1302,9 @@
Label actionLabel,
BugReporter bugReporter) {
if (!input.isSourceArtifact()) {
- bugReporter.sendBugReport(
- new IllegalStateException(
- String.format(
- "Unexpected exit code %s for generated artifact %s (%s)",
- detailedExitCode, input, actionLabel)));
+ bugReporter.logUnexpected(
+ "Unexpected exit code %s for generated artifact %s (%s)",
+ detailedExitCode, input, actionLabel);
}
return new LabelCause(
MoreObjects.firstNonNull(input.getOwner(), actionLabel), detailedExitCode);
@@ -1533,9 +1531,8 @@
private void handleSourceArtifactExceptionFromSkykey(SkyKey key, SourceArtifactException e) {
if (!(key instanceof Artifact) || !((Artifact) key).isSourceArtifact()) {
- bugReporter.sendBugReport(
- new IllegalStateException(
- "Unexpected SourceArtifactException for key: " + key + ", " + action, e));
+ bugReporter.logUnexpected(
+ e, "Unexpected SourceArtifactException for key: %s, %s", key, action.prettyPrint());
missingArtifactCauses.add(
new LabelCause(action.getOwner().getLabel(), e.getDetailedExitCode()));
return;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index fd0ef11..28d35b0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -2258,7 +2258,6 @@
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
- "//src/main/java/com/google/devtools/build/lib/util:execution_detailed_exit_code_helper",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/protobuf:failure_details_java_proto",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index 18bc499..95e955e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -245,9 +245,8 @@
}
} catch (SourceArtifactException e) {
if (!input.isSourceArtifact()) {
- bugReporter.sendBugReport(
- new IllegalStateException(
- "Non-source artifact had SourceArtifactException: " + input, e));
+ bugReporter.logUnexpected(
+ e, "Non-source artifact had SourceArtifactException: %s", input);
}
handleSourceFileError(input, e.getDetailedExitCode(), rootCausesBuilder, env, value, key);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
index c86fe05..f1474b2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
@@ -55,7 +55,6 @@
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.TopLevelAspectsKey;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.DetailedExitCode.DetailedExitCodeComparator;
-import com.google.devtools.build.lib.util.ExecutionDetailedExitCodeHelper;
import com.google.devtools.build.skyframe.CycleInfo;
import com.google.devtools.build.skyframe.CyclesReporter;
import com.google.devtools.build.skyframe.ErrorInfo;
@@ -327,9 +326,7 @@
}
if (includeExecutionPhase && detailedExitCode == null) {
- detailedExitCode =
- ExecutionDetailedExitCodeHelper.createDetailedExitCodeForUndetailedExecutionCause(
- result, undetailedCause);
+ detailedExitCode = createDetailedExitCodeForUndetailedExecutionCause(result, undetailedCause);
}
return ErrorProcessingResult.create(
@@ -410,7 +407,7 @@
return;
}
- sendBugReportUnexpectedExceptionOrigin(errorInfo, key, walkableGraph, cause);
+ logUnexpectedExceptionOrigin(errorInfo, key, walkableGraph, cause);
}
private static void assertValidAnalysisOrExecutionException(
@@ -428,14 +425,14 @@
return;
}
- sendBugReportUnexpectedExceptionOrigin(errorInfo, key, walkableGraph, cause);
+ logUnexpectedExceptionOrigin(errorInfo, key, walkableGraph, cause);
}
/**
- * Walk the graph to find a path to the lowest-level node that threw unexpected exception and send
- * a BugReport.
+ * Walk the graph to find a path to the lowest-level node that threw unexpected exception and log
+ * it.
*/
- private static void sendBugReportUnexpectedExceptionOrigin(
+ private static void logUnexpectedExceptionOrigin(
ErrorInfo errorInfo, SkyKey key, WalkableGraph walkableGraph, Throwable cause)
throws InterruptedException {
List<SkyKey> path = new ArrayList<>();
@@ -463,9 +460,7 @@
}
} while (foundDep);
} finally {
- BugReport.sendBugReport(
- new IllegalStateException(
- "Unexpected analysis error: " + key + " -> " + errorInfo + ", (" + path + ")"));
+ BugReport.logUnexpected("Unexpected analysis error: %s -> %s, (%s)", key, errorInfo, path);
}
}
@@ -531,10 +526,7 @@
// during evaluation (otherwise, it wouldn't have bothered to find a cycle). So the best
// we can do is throw a generic build failure exception, since we've already reported the
// cycles above.
- throw new BuildFailedException(
- null,
- ExecutionDetailedExitCodeHelper.createDetailedExecutionExitCode(
- "cycle found during execution", Execution.Code.CYCLE));
+ throw new BuildFailedException(null, CYCLE_CODE);
} else {
rethrow(exception, bugReporter, result);
}
@@ -564,6 +556,10 @@
logger.atWarning().withCause(cause).log(
"Non-action-execution/input-error exception for %s", error);
}
+ } else if (!error.getValue().getCycleInfo().isEmpty()) {
+ detailedExitCode =
+ DetailedExitCodeComparator.chooseMoreImportantWithFirstIfTie(
+ detailedExitCode, CYCLE_CODE);
} else {
undetailedCause = cause;
}
@@ -571,8 +567,7 @@
if (detailedExitCode != null) {
return detailedExitCode;
}
- return ExecutionDetailedExitCodeHelper.createDetailedExitCodeForUndetailedExecutionCause(
- result, undetailedCause);
+ return createDetailedExitCodeForUndetailedExecutionCause(result, undetailedCause);
}
/**
@@ -580,7 +575,7 @@
* At the moment, we don't expect any analysis failure to be catastrophic.
*/
@VisibleForTesting
- public static void rethrow(
+ static void rethrow(
Throwable cause, BugReporter bugReporter, EvaluationResult<?> resultForDebugging)
throws BuildFailedException, TestExecException {
Throwables.throwIfUnchecked(cause);
@@ -607,9 +602,23 @@
if (cause instanceof InputFileErrorException) {
throw (InputFileErrorException) cause;
}
+
// We encountered an exception we don't think we should have encountered. This can indicate
// an exception-processing bug in our code, such as lower level exceptions not being properly
// handled, or in our expectations in this method.
+
+ if (cause instanceof DetailedException) {
+ // The exception escaped Skyframe error bubbling, but its failure detail can still be used.
+ bugReporter.logUnexpected(
+ (Exception) cause,
+ "action terminated with unexpected exception with result %s",
+ resultForDebugging);
+ throw new BuildFailedException(
+ cause.getMessage(), ((DetailedException) cause).getDetailedExitCode());
+ }
+
+ // An undetailed exception means we may incorrectly attribute responsibility for the failure:
+ // we need to fix that.
bugReporter.sendBugReport(
new IllegalStateException(
"action terminated with unexpected exception with result " + resultForDebugging,
@@ -617,11 +626,42 @@
String message =
"Unexpected exception, please file an issue with the Bazel team: " + cause.getMessage();
throw new BuildFailedException(
- message,
- DetailedExitCode.of(
- FailureDetail.newBuilder()
- .setMessage(message)
- .setExecution(Execution.newBuilder().setCode(Execution.Code.UNEXPECTED_EXCEPTION))
- .build()));
+ message, createDetailedExecutionExitCode(message, UNKONW_EXECUTION));
+ }
+
+ private static DetailedExitCode createDetailedExitCodeForUndetailedExecutionCause(
+ EvaluationResult<?> result, Throwable undetailedCause) {
+ // TODO(b/227660368): These warning logs should be a bug report, but tests currently fail.
+ if (undetailedCause == null) {
+ logger.atWarning().log("No exceptions found despite error in %s", result);
+ return createDetailedExecutionExitCode(
+ "keep_going execution failed without an action failure",
+ Execution.Code.NON_ACTION_EXECUTION_FAILURE);
+ }
+ logger.atWarning().withCause(undetailedCause).log("No detailed exception found in %s", result);
+ return createDetailedExecutionExitCode(
+ "keep_going execution failed without an action failure: "
+ + undetailedCause.getMessage()
+ + " ("
+ + undetailedCause.getClass().getSimpleName()
+ + ")",
+ Execution.Code.NON_ACTION_EXECUTION_FAILURE);
+ }
+
+ private static final DetailedExitCode CYCLE_CODE =
+ createDetailedExecutionExitCode("cycle found during execution", Execution.Code.CYCLE);
+ private static final Execution UNKONW_EXECUTION =
+ Execution.newBuilder().setCode(Execution.Code.UNEXPECTED_EXCEPTION).build();
+
+ private static DetailedExitCode createDetailedExecutionExitCode(
+ String message, Execution.Code detailedCode) {
+ return createDetailedExecutionExitCode(
+ message, Execution.newBuilder().setCode(detailedCode).build());
+ }
+
+ private static DetailedExitCode createDetailedExecutionExitCode(
+ String message, Execution execution) {
+ return DetailedExitCode.of(
+ FailureDetail.newBuilder().setMessage(message).setExecution(execution).build());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD
index 6b675e8..fc621a8 100644
--- a/src/main/java/com/google/devtools/build/lib/util/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/util/BUILD
@@ -29,17 +29,6 @@
)
java_library(
- name = "execution_detailed_exit_code_helper",
- srcs = ["ExecutionDetailedExitCodeHelper.java"],
- deps = [
- ":detailed_exit_code",
- "//src/main/java/com/google/devtools/build/skyframe",
- "//src/main/protobuf:failure_details_java_proto",
- "//third_party:flogger",
- ],
-)
-
-java_library(
name = "os",
srcs = ["OS.java"],
)
diff --git a/src/main/java/com/google/devtools/build/lib/util/ExecutionDetailedExitCodeHelper.java b/src/main/java/com/google/devtools/build/lib/util/ExecutionDetailedExitCodeHelper.java
deleted file mode 100644
index f3c107b..0000000
--- a/src/main/java/com/google/devtools/build/lib/util/ExecutionDetailedExitCodeHelper.java
+++ /dev/null
@@ -1,54 +0,0 @@
-// Copyright 2021 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.util;
-
-import com.google.common.flogger.GoogleLogger;
-import com.google.devtools.build.lib.server.FailureDetails.Execution;
-import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
-import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
-import com.google.devtools.build.skyframe.EvaluationResult;
-
-/** A collection of helper methods around execution-related DetailedExitCode. */
-public final class ExecutionDetailedExitCodeHelper {
- private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
-
- public static DetailedExitCode createDetailedExitCodeForUndetailedExecutionCause(
- EvaluationResult<?> result, Throwable undetailedCause) {
- if (undetailedCause == null) {
- logger.atWarning().log("No exceptions found despite error in %s", result);
- return createDetailedExecutionExitCode(
- "keep_going execution failed without an action failure",
- Code.NON_ACTION_EXECUTION_FAILURE);
- }
- logger.atWarning().withCause(undetailedCause).log("No detailed exception found in %s", result);
- return createDetailedExecutionExitCode(
- "keep_going execution failed without an action failure: "
- + undetailedCause.getMessage()
- + " ("
- + undetailedCause.getClass().getSimpleName()
- + ")",
- Code.NON_ACTION_EXECUTION_FAILURE);
- }
-
- public static DetailedExitCode createDetailedExecutionExitCode(
- String message, Code detailedCode) {
- return DetailedExitCode.of(
- FailureDetail.newBuilder()
- .setMessage(message)
- .setExecution(Execution.newBuilder().setCode(detailedCode))
- .build());
- }
-
- private ExecutionDetailedExitCodeHelper() {}
-}
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BUILD b/src/test/java/com/google/devtools/build/lib/buildtool/util/BUILD
index 1e5b735..cca6f3b 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BUILD
@@ -98,6 +98,7 @@
"//src/test/java/com/google/devtools/build/lib/testutil:TestConstants",
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
+ "//third_party:error_prone_annotations",
"//third_party:guava",
"//third_party:guava-testlib",
"//third_party:jsr305",
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
index 071fd93..6bcae4a 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
@@ -120,6 +120,7 @@
import com.google.devtools.build.lib.worker.WorkerModule;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
+import com.google.errorprone.annotations.FormatMethod;
import java.io.IOException;
import java.lang.Thread.UncaughtExceptionHandler;
import java.util.ArrayList;
@@ -993,6 +994,18 @@
exceptions.add(exception);
}
+ @FormatMethod
+ @Override
+ public void logUnexpected(String message, Object... args) {
+ sendBugReport(new IllegalStateException(String.format(message, args)));
+ }
+
+ @FormatMethod
+ @Override
+ public void logUnexpected(Exception e, String message, Object... args) {
+ sendBugReport(new IllegalStateException(String.format(message, args), e));
+ }
+
@Override
public void handleCrash(Crash crash, CrashContext ctx) {
// Unexpected: try to crash JVM.
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java b/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java
index 7b44260..87519c3 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/MoreAsserts.java
@@ -205,6 +205,16 @@
expectedExitCode, exitCode, recordingOutErr.outAsLatin1(), recordingOutErr.errAsLatin1());
}
+ public static void assertEqualWithStdoutAndErr(
+ Object expected, Object actual, String stdout, String stderr) {
+ if (!expected.equals(actual)) {
+ fail(
+ String.format(
+ "expected <%s> but was <%s> and stdout was <%s> and stderr was <%s>",
+ expected, actual, stdout, stderr));
+ }
+ }
+
public static void assertStdoutContainsString(String expected, String stdout, String stderr) {
if (!stdout.contains(expected)) {
fail("expected stdout to contain string <" + expected + "> but stdout was <"
