Refactor build rewinding to a fallback strategy for action rewinding #25396

fmeum · 2025-02-26T19:48:51Z

By throwing LostInputExecExceptions in the right locations, build rewinding can be realized as a coarse-grained fallback for the more fine-grained action rewinding. This approach avoids further divergence between Bazel and Blaze logic and ensures that further improvements to build rewinding also simplify future efforts to add action rewinding to Bazel.

This should be a pure refactoring with one exception: if the download of the outputs of a remotely executed action (with caching disabled) fails due cache eviction, the build is no longer retried after the remote execution has already been retried. An RE backend that repeatedly fails to keep the outputs of a freshly excuted action in the cache long enough for them to be downloaded is faulty and additional full build retries are pointless after retries of the action execution have failed repeatedly. This change in behavior is required to get existing tests to pass.

gemini-code-assist · 2025-02-26T19:48:54Z

Important

The terms of service for this installation has not been accepted. Please ask the Organization owners to visit the Gemini Code Assist Admin Console to sign it.

fmeum · 2025-02-27T08:25:42Z

CI failures look unrelated, but persistent

coeuvre

LGTM!

I will wait for @justinhorvitz's opinion on the changes related to action rewinding before I import the PR.

# Conflicts: # src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

justinhorvitz · 2025-02-28T03:32:57Z

src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTest.java

        "--rewind_lost_inputs",
        "--features=cc_include_scanning",
        "--experimental_remote_include_extraction_size_threshold=0",
+        "--experimental_remote_cache_eviction_retries=0",


Does this have an effect only after #25396? I don't know how stacked PRs work.

It's relevant already with this PR, which is the first in the stack. Without it, the lostInputWithRewindingDisabled test doesn't see the expected exit code. Since build rewinding and action rewinding are somewhat competing strategies for handling lost inputs now, I found it safer to disable build rewinding for the entire test suite.

I see now. We're no longer guarding the full build retry on a specific exit code which is only possible in bazel. So now --experimental_remote_cache_eviction_retries is load bearing for blaze, when there's a lost input with --norewind_lost_inputs. How can we ensure that --experimental_remote_cache_eviction_retries continues to be a no-op for blaze?

I don't know how these Bazel/Blaze differences are usually handled. If setting the flag to 0 in the global .blazerc is not sufficient, maybe the flag default could be changed with copybara?

@coeuvre

I can use blazerc if necessary, I was just hoping there was a way to continue keeping the flag completely irrelevant.

I think that's the most natural solution and the one that's been used in the past when something like this came up. The only alternative I see would be to add a bit to the lost input exception types that all Bazel call sites set differently, but that feels worse to me.

I mailed a blazerc change to set this flag to 0. Let's make sure that lands safely before importing this.

Oh no, another issue. We need to modify invocation policy to force the flag to 0 instead of ignoring it. Otherwise the blazerc value will be ignored. I'll update here when that lands. If you don't hear from me within a week, please ping.

src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java

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

src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java

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

fmeum · 2025-03-03T18:22:26Z

@coeuvre This is now ready to be merged.

coeuvre · 2025-03-04T10:10:46Z

Importing now!

Cache evictions encountered during reads of remote files in `RemoteActionFileSystem` now result in the build being retried when `--experimental_remote_cache_eviction_retries` is set to a positive value (the default). This is enabled by implementing the `checkForLostInputs` method on the `RemoteOutputService`, building on the refactoring performed in #25396. Closes #25358. PiperOrigin-RevId: 736064349 Change-Id: Idc70d55138c3366d22ece7f0579b242b3c217da8

Cache evictions encountered during reads of remote files in `RemoteActionFileSystem` now result in the build being retried when `--experimental_remote_cache_eviction_retries` is set to a positive value (the default). This is enabled by implementing the `checkForLostInputs` method on the `RemoteOutputService`, building on the refactoring performed in bazelbuild#25396. Closes bazelbuild#25358. PiperOrigin-RevId: 736064349 Change-Id: Idc70d55138c3366d22ece7f0579b242b3c217da8

fmeum · 2025-03-13T12:47:00Z

@bazel-io fork 8.2.0

…winding By throwing `LostInputExecExceptions` in the right locations, build rewinding can be realized as a coarse-grained fallback for the more fine-grained action rewinding. This approach avoids further divergence between Bazel and Blaze logic and ensures that further improvements to build rewinding also simplify future efforts to add action rewinding to Bazel. This should be a pure refactoring with one exception: if the download of the outputs of a remotely executed action (with caching disabled) fails due cache eviction, the build is no longer retried after the remote execution has already been retried. An RE backend that repeatedly fails to keep the outputs of a freshly excuted action in the cache long enough for them to be downloaded is faulty and additional full build retries are pointless after retries of the action execution have failed repeatedly. This change in behavior is required to get existing tests to pass. Closes bazelbuild#25396. PiperOrigin-RevId: 734550819 Change-Id: Idabee2483beb05721c413c27ab610b8f260412ec (cherry picked from commit e8526d4)

…winding (#25545) By throwing `LostInputExecExceptions` in the right locations, build rewinding can be realized as a coarse-grained fallback for the more fine-grained action rewinding. This approach avoids further divergence between Bazel and Blaze logic and ensures that further improvements to build rewinding also simplify future efforts to add action rewinding to Bazel. This should be a pure refactoring with one exception: if the download of the outputs of a remotely executed action (with caching disabled) fails due cache eviction, the build is no longer retried after the remote execution has already been retried. An RE backend that repeatedly fails to keep the outputs of a freshly excuted action in the cache long enough for them to be downloaded is faulty and additional full build retries are pointless after retries of the action execution have failed repeatedly. This change in behavior is required to get existing tests to pass. Closes #25396. PiperOrigin-RevId: 734550819 Change-Id: Idabee2483beb05721c413c27ab610b8f260412ec (cherry picked from commit e8526d4) Closes #25544

Cache evictions encountered during reads of remote files in `RemoteActionFileSystem` now result in the build being retried when `--experimental_remote_cache_eviction_retries` is set to a positive value (the default). This is enabled by implementing the `checkForLostInputs` method on the `RemoteOutputService`, building on the refactoring performed in bazelbuild#25396. Closes bazelbuild#25358. PiperOrigin-RevId: 736064349 Change-Id: Idc70d55138c3366d22ece7f0579b242b3c217da8

…g digest (#25538) Cache evictions encountered during reads of remote files in `RemoteActionFileSystem` now result in the build being retried when `--experimental_remote_cache_eviction_retries` is set to a positive value (the default). This is enabled by implementing the `checkForLostInputs` method on the `RemoteOutputService`, building on the refactoring performed in #25396. Closes #25358. PiperOrigin-RevId: 736064349 Change-Id: Idc70d55138c3366d22ece7f0579b242b3c217da8 Commit 85d9cc9 Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

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`. Closes #25448. PiperOrigin-RevId: 743552575 Change-Id: If7bd85fd737d9af9ef339ee1ad57f9d1c230b5a9

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

fmeum requested a review from a team as a code owner February 26, 2025 19:48

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

fmeum mentioned this pull request Feb 26, 2025

Retry build when RemoteActionFileSystem encounters a missing digest #25358

Closed

fmeum requested review from coeuvre and justinhorvitz and removed request for a team February 26, 2025 19:51

fmeum force-pushed the build-rewinding-jdeps-3 branch from 8139820 to fa9dfa3 Compare February 26, 2025 21:11

fmeum force-pushed the build-rewinding-jdeps-3 branch from fa9dfa3 to c0d54dd Compare February 27, 2025 08:58

coeuvre approved these changes Feb 27, 2025

View reviewed changes

fmeum force-pushed the build-rewinding-jdeps-3 branch from c0d54dd to 4764750 Compare February 27, 2025 18:17

Refactor build rewinding to a fallback strategy for action rewinding

41d95ba

# Conflicts: # src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

fmeum force-pushed the build-rewinding-jdeps-3 branch from 4764750 to 41d95ba Compare February 27, 2025 21:31

justinhorvitz requested changes Feb 28, 2025

View reviewed changes

Address comments

877ae74

fmeum requested a review from justinhorvitz February 28, 2025 12:38

justinhorvitz approved these changes Feb 28, 2025

View reviewed changes

fmeum requested a review from coeuvre March 3, 2025 18:21

fmeum mentioned this pull request Mar 4, 2025

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

Closed

copybara-service bot closed this in e8526d4 Mar 7, 2025

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

fmeum deleted the build-rewinding-jdeps-3 branch March 12, 2025 12:04

iancha1992 mentioned this pull request Mar 12, 2025

[8.2.0] Retry build when RemoteActionFileSystem encounters a missing digest #25538

Merged

bazel-io mentioned this pull request Mar 13, 2025

[8.2.0] Refactor build rewinding to a fallback strategy for action rewinding #25544

Closed

fmeum mentioned this pull request Mar 13, 2025

[8.2.0] Refactor build rewinding to a fallback strategy for action rewinding #25545

Merged

fmeum mentioned this pull request Mar 14, 2025

Add Bazel support for --rewind_lost_inputs #25477

Open

2 tasks

Refactor build rewinding to a fallback strategy for action rewinding #25396

Refactor build rewinding to a fallback strategy for action rewinding #25396

Uh oh!

Conversation

fmeum commented Feb 26, 2025

Uh oh!

gemini-code-assist bot commented Feb 26, 2025

Uh oh!

fmeum commented Feb 27, 2025

Uh oh!

coeuvre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

coeuvre commented Mar 4, 2025

Uh oh!

fmeum commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coeuvre left a comment •

edited

Loading

fmeum commented Mar 3, 2025 •

edited

Loading