Retry build when digests of top-level outputs are missing #25448

fmeum · 2025-03-04T13:08:58Z

This is achieved by implementing an ImportantOutputHandler for Bazel and moving the downloading logic that previously lived in CompletionFunction into it. This slightly differs from Blaze since Bazel accounts for runfiles within CompletionFunction's call to the ImportantOutputHandler#processOutputsAndGetLostArtifacts function.

Changes made in #25396 to support build rewinding as a fallback of action rewinding are extracted into ActionRewindStrategy to ensure consistent behavior between lost inputs and lost outputs. This results in a change to the error message shown when lost inputs are encountered but --rewind_lost_inputs is not enabled, which used to not mention the flag (compared to the case of lost outputs, in which it did). New Java tests verify the basic functionality of action rewinding in Bazel.

This change requires a fix to BulkTransferException#add, which previously resulted in infinite recursion when passed a BulkTransferException.

fmeum · 2025-03-04T15:58:39Z

Stacked on #25396

fmeum · 2025-03-07T18:33:42Z

This can be reviewed independently of #25358.

justinhorvitz · 2025-03-07T21:53:17Z

src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java


+    ImmutableMap<String, ActionInput> knownLostOutputs = ImmutableMap.of();
+    try {
+      ensureToplevelArtifacts(env, importantArtifacts, inputMap);


Can the whole ensureTopLevelArtifacts functionality be offloaded to an ImportantOutputHandler? It looks an awful lot like what the blaze ImportantOutputHandler does, and it would clean up the interface awkwardness introduced by passing in known lost inputs.

Thanks for the suggestion, this cleaned up the change quite a bit. I do still need to pass in a new argument (a way to get the generating action for an artifact).

justinhorvitz · 2025-03-07T21:57:30Z

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

+        InputMetadataProvider metadataProvider,
+        ImmutableMap<String, ActionInput> knownLostOutputs) {
+      return new LostArtifacts(
+          knownLostOutputs, new ActionInputDepOwnerMap(knownLostOutputs.values()));


Does the owner map matter at all for full build rewinding or only action rewinding? It looks like you're just creating an empty one and not populating any owners.

It doesn't matter for build rewinding, but as I only noticed after I added more tests, it does for action rewinding for tree artifacts. I am now tracking owners, PTAL.

I am working on some refactoring that will hopefully simplify this ownership tracking. I sent the first change for review today: https://bazel-review.googlesource.com/c/bazel/+/274110. The goal is to eventually end up at a place where the ownership tracking is optional (ActionRewindStrategy fills it in if it's not present).

Thanks! I did wonder why ownership needed to be supplied manually. The CL description helped me understand this better.

fmeum · 2025-03-09T17:25:23Z

@justinhorvitz I added new tests that cover all the kinds of artifacts that Bazel might download via CompletionFunction. Top-level outputs work well, including tree artifacts, but lost runfiles result in this failure in the new remoteCacheEvictBlobs_whenRunfilesRequested_succeedsWithActionRewinding integration test case:

Caused by: java.lang.IllegalStateException: Lost input not a dep of the failed action and can't be associated with such a dep. lostInput: File:[[<execution_root>]bazel-out/darwin_arm64-fastbuild/bin]a/foo.out/file-inside, owners: [File:[[<execution_root>]bazel-out/darwin_arm64-fastbuild/bin]a/foo.out], failedKey: TargetCompletionKey{topLevelArtifactContext=com.google.devtools.build.lib.analysis.TopLevelArtifactContext@90904c3b, actionLookupKey=ConfiguredTargetKey{label=//a:bin, config=BuildConfigurationKey[5ef29e1379ec5c6e584ffec760372c7a27e8ee210a5a61ff33d4bd48eeff889b]}, willTest=false}
	at com.google.devtools.build.lib.skyframe.rewinding.ActionRewindStrategy.getLostInputOwningDirectDeps(ActionRewindStrategy.java:528)
	at com.google.devtools.build.lib.skyframe.rewinding.ActionRewindStrategy.prepareRewindPlan(ActionRewindStrategy.java:258)
	at com.google.devtools.build.lib.skyframe.rewinding.ActionRewindStrategy.prepareRewindPlanForLostTopLevelOutputs(ActionRewindStrategy.java:137)
	at com.google.devtools.build.lib.skyframe.CompletionFunction.informImportantOutputHandler(CompletionFunction.java:474)
	at com.google.devtools.build.lib.skyframe.CompletionFunction.compute(CompletionFunction.java:351)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:471)
	... 7 more

Bazel doesn't contain any callsite of processRunfilesAndGetLostArtifacts, so I am not sure whether I need to go through an entirely different flow here or just add to the failedKeyDeps. Do you have a suggestion for me?

fmeum · 2025-03-09T17:54:27Z

I think that this is not a fundamental problem: I can get the test to pass by passing builtArtifacts into informImportantOutputHandler, not just importantArtifacts - the latter doesn't contain the runfiles tree artifacts. It's rightfully not an important artifact for BES purposes, but with Bazel, it would probably need to be considered one for action rewinding purposes.

@justinhorvitz I pushed a commit that adds runfile tree artifacts to failedDepKeys if present. What do you think of that approach?

justinhorvitz · 2025-03-10T21:14:36Z

I think that this is not a fundamental problem: I can get the test to pass by passing builtArtifacts into informImportantOutputHandler, not just importantArtifacts - the latter doesn't contain the runfiles tree artifacts. It's rightfully not an important artifact for BES purposes, but with Bazel, it would probably need to be considered one for action rewinding purposes.

@justinhorvitz I pushed a commit that adds runfile tree artifacts to failedDepKeys if present. What do you think of that approach?

The reason that blaze does not process runfiles like this in CompletionFunction and instead uses the top-level-aspect approach I mentioned over chat is because for certain builds, that aspect's action also builds a manifest of where the runfiles can be found in remote storage. We need to validate cached storage addresses prior to building that manifest because the addresses of the rewound action's outputs may be different than what was cached.

I think it would be best to make the bazel case work similarly if possible. Do you think it's feasible to pass --aspects=StageRunfilesAspect when runfiles of top-level targets are needed? If so, I'll open-source that aspect and its action.

fmeum · 2025-03-10T22:20:26Z

Bazel has flags such as --remote_download_regex that can request the materialization of individual files from a runfiles tree (or any runfiles tree), but it's impossible to tell from a given regex whether it could match top-level runfiles. Would the aspect approach be able to support this use case? Would Bazel users need to manually pass that flag (and even with a bazel run)?

justinhorvitz · 2025-03-11T01:41:27Z

The aspect-based approach is intended to validate leases and materialize runfiles of top-level targets, not anything else. I would require a flag, unless you make it the default.

I think you could also get away with having CompletionFunction pass in the runfiles to processOutputsAndGetLostArtifacts, and we'll just have the google handler ignore them and continue to rely on the runfiles-specific method.

fmeum · 2025-03-17T16:05:51Z

@justinhorvitz I resolved the merge conflict, PTAL.

justinhorvitz · 2025-03-19T20:01:29Z

src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java

+    var byDigest = byDigestBuilder.build();
+    return new LostArtifacts(
+        byDigest,
+        ActionRewindStrategy.calculateLostInputOwners(byDigest.values(), metadataProvider));


Instead of making this visible, we can make LostArtifacts take an Optional<LostInputOwners>, and pass that into ActionRewindStrategy. Then the lost inputs and outputs cases would become more similar.

src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java

justinhorvitz · 2025-03-19T20:16:36Z

The rewinding part is looking pretty good. Just left a couple comments. I did not review the remote execution parts that I am not familiar with.

fmeum

@justinhorvitz I hope I addressed your comments, please take another look.
@coeuvre This is ready to be reviewed from the remote execution perspective.

fmeum · 2025-03-19T22:24:01Z

src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java

      for (ActionInput lostOutput : lostOutputs.byDigest().values()) {
        builtArtifacts.remove(lostOutput);
-        builtArtifacts.removeAll(lostOutputs.owners().getOwners(lostOutput));
+        lostOutputs


@justinhorvitz Is it okay that Bazel skips this part because it doesn't populate owners? I would think that the answer is no, which would probably require moving this computation into ActionRewindStrategy.prepareRewindPlanForLostTopLevelOutputs?

I added a commit with this change for now.

The removal from builtArtifacts is relevant for errorful cases where builtArtifacts is passed to postFailedEvent. I believe that the _partiallyBuiltTarget_ rewinding test cases are supposed to cover this. The owners would be covered by the tree artifact one.

fmeum · 2025-03-20T09:59:04Z

@bazel-io fork 8.2.0

src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java

coeuvre · 2025-03-26T12:54:54Z

src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java

   */
-  public ImmutableMap<String, ActionInput> getLostInputs(
-      Function<String, ActionInput> actionInputResolver) {
+  public LostArtifacts getLostArtifacts(InputMetadataProvider metadataProvider) {


Why Function<String, ActionInput> is not enough?

It's still enough and I just switched back. InputMetadataProvider is now always a transitive dep of BulkTransferException, which is what I initially wanted to avoid - but LostArtifacts brings that with it.

coeuvre

I will import this.

fmeum · 2025-03-26T14:39:21Z

@bazel-io fork 8.2.0

coeuvre · 2025-04-02T13:40:16Z

I have made some changes on top of this PR during internal reviews, most notably:

I undid the change to interface ImportantOutputHandler#processOutputsAndGetLostArtifacts. Instead, let the new implementation find its own way to get the generating action of an artifact.
I moved the new ImportantOutputHandler implementation into its own target so it can be shared with other modules internally.
Instead of passing builtArtifacts and mutate it inside ActionRewindStrategy#prepareRewindPlanForLostTopLevelOutputs, mutate it directly from the call site.

The review is still undergoing but should be merged soon.

This is achieved by implementing an `ImportantOutputHandler` for Bazel and moving the downloading logic that previously lived in `CompletionFunction` into it. This slightly differs from Blaze since Bazel accounts for runfiles within `CompletionFunction`'s call to the `ImportantOutputHandler#processOutputsAndGetLostArtifacts` function. Changes made in bazelbuild#25396 to support build rewinding as a fallback of action rewinding are extracted into `ActionRewindStrategy` to ensure consistent behavior between lost inputs and lost outputs. This results in a change to the error message shown when lost inputs are encountered but `--rewind_lost_inputs` is not enabled, which used to not mention the flag (compared to the case of lost outputs, in which it did). New Java tests verify the basic functionality of action rewinding in Bazel. This change requires a fix to `BulkTransferException#add`, which previously resulted in infinite recursion when passed a `BulkTransferException`. Closes bazelbuild#25448. PiperOrigin-RevId: 743552575 Change-Id: If7bd85fd737d9af9ef339ee1ad57f9d1c230b5a9

… it is also a top-level runfile of the same target. The bug was introduced by #25448 (28270f1) which added top-level output rewinding for bazel. Blaze does not process or rewind runfiles from `CompletionFunction`, so in such a case, there can be a discrepancy between the metadata in `inputMap` (reflects the unrewound `RunfilesArtifactValue`) and `importantInputMap` (reflects the rewound `FileArtifactValue`). The culprit change switches to `inputMap`, resulting in the bug. As a quick fix, pass two metadata providers to `ImportantOutputHandler` - one containing only important metadata (used by blaze's `ImportantOutputHandler`), and one containing the full metadata including artifacts from hidden output groups (used by bazel's `RemoteImportantOutputHandler`). I think this can be cleaned up more, but I want to fix the bug first. PiperOrigin-RevId: 753201303 Change-Id: I46fc6fd6384541ec002cbd7c74ba83ac9fa45a09

fmeum force-pushed the build-rewinding-top-level-outputs branch 4 times, most recently from 6d74866 to fd731fd Compare March 4, 2025 15:49

fmeum changed the title ~~Build rewinding top level outputs~~ Retry build when digests of top-level outputs are missing Mar 4, 2025

fmeum force-pushed the build-rewinding-top-level-outputs branch 3 times, most recently from b8cefea to 8df7e66 Compare March 7, 2025 18:18

fmeum marked this pull request as ready for review March 7, 2025 18:31

fmeum requested a review from a team as a code owner March 7, 2025 18:31

fmeum requested review from coeuvre and justinhorvitz March 7, 2025 18:32

github-actions bot added team-Performance Issues for Performance teams team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Mar 7, 2025

fmeum removed the request for review from a team March 7, 2025 18:32

justinhorvitz reviewed Mar 7, 2025

View reviewed changes

fmeum force-pushed the build-rewinding-top-level-outputs branch 2 times, most recently from 941690f to d9d761d Compare March 9, 2025 07:43

fmeum force-pushed the build-rewinding-top-level-outputs branch 2 times, most recently from 203ee76 to e329b38 Compare March 9, 2025 21:01

fmeum requested a review from justinhorvitz March 9, 2025 21:26

fmeum mentioned this pull request Mar 10, 2025

website/blog: Added blog post on Bazel TTL bug buildbuddy-io/buildbuddy#8588

Merged

Redo rename

b7966f7

fmeum added 3 commits March 17, 2025 23:02

Merge branch 'master' into build-rewinding-top-level-outputs

2d493ff

Merge branch 'master' into build-rewinding-top-level-outputs

a63ba32

Merge branch 'master' into build-rewinding-top-level-outputs

4fd9ae6

justinhorvitz reviewed Mar 19, 2025

View reviewed changes

fmeum added 2 commits March 19, 2025 22:24

Merge branch 'master' into build-rewinding-top-level-outputs

30423f3

Address comments

851886a

fmeum commented Mar 19, 2025

View reviewed changes

fmeum requested a review from justinhorvitz March 19, 2025 22:24

Move builtArtifacts handling into ActionRewindStrategy

df1eef0

bazel-io mentioned this pull request Mar 20, 2025

[8.2.0] Retry build when digests of top-level outputs are missing #25641

Closed

Merge branch 'master' into build-rewinding-top-level-outputs

b6ca821

coeuvre requested changes Mar 26, 2025

View reviewed changes

fmeum added 2 commits March 26, 2025 15:19

Switch back to function

cd59cdd

Merge branch 'master' into build-rewinding-top-level-outputs

56f5fa2

fmeum requested a review from coeuvre March 26, 2025 14:21

coeuvre approved these changes Mar 26, 2025

View reviewed changes

bazel-io mentioned this pull request Mar 26, 2025

[8.2.0] Retry build when digests of top-level outputs are missing #25704

Closed

coeuvre mentioned this pull request Mar 27, 2025

Attach context string to gRPC downloader requests #25442

Closed

copybara-service bot closed this in 28270f1 Apr 3, 2025

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Apr 3, 2025

fmeum deleted the build-rewinding-top-level-outputs branch April 14, 2025 12:59

Retry build when digests of top-level outputs are missing #25448

Retry build when digests of top-level outputs are missing #25448

Uh oh!

Conversation

fmeum commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Mar 4, 2025

Uh oh!

fmeum commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmeum commented Mar 9, 2025

Uh oh!

fmeum commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinhorvitz commented Mar 10, 2025

Uh oh!

fmeum commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinhorvitz commented Mar 11, 2025

Uh oh!

fmeum commented Mar 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinhorvitz commented Mar 19, 2025

Uh oh!

fmeum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmeum commented Mar 20, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coeuvre left a comment

Choose a reason for hiding this comment

Uh oh!

fmeum commented Mar 26, 2025

Uh oh!

coeuvre commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fmeum commented Mar 4, 2025 •

edited

Loading

fmeum commented Mar 9, 2025 •

edited

Loading

fmeum commented Mar 10, 2025 •

edited

Loading