Rewind repo fetches for files lost outside action execution - #30465
Draft
fmeum wants to merge 23 commits into
Draft
Rewind repo fetches for files lost outside action execution#30465fmeum wants to merge 23 commits into
fmeum wants to merge 23 commits into
Conversation
The remote repo contents cache injects a cached repo's metadata without verifying that every leaf blob referenced by its Tree is still available in the CAS. If a blob had been evicted by the time another repo rule triggered full materialization through rctx.path() or rctx.read(), the build failed with "Failed to materialize remote repo ...: Missing digest ..." and never reran the repo rule, not even with the default whole-command remote cache eviction retries. Full materialization now handles an all-CacheNotFoundException prefetch failure like the existing on-demand read path: the repo is marked as having lost files and a transient REMOTE_FILE_EVICTED error is raised. The failure detail is preserved through repo rule evaluation (as the EvalException cause) and PackageLookupFunction so that the command exits with REMOTE_CACHE_EVICTED and is automatically retried. Since the stale ActionResult and Tree survive in the remote cache, retried commands skip the cache lookup for such repos until their contents, including the lost blobs, have been fetched and uploaded again, which heals the cache entry. Fixes bazelbuild#30218. RELNOTES: The remote repo contents cache now recovers from cached repository contents that are no longer fully available in the remote cache by refetching the repository.
…nally Repos whose cached contents referenced lost remote files must be refetched and thus have no use for their stale in-memory contents, even if no materialization was attempted for them. This also simplifies the eviction filter for materialized repos.
A module extension that triggers the full materialization of a cached repo via module_ctx.path() previously converted the transient REMOTE_FILE_EVICTED failure into a persistent EXTENSION_EVAL_ERROR, so the build failed without a retry. ExternalDepsException now derives its exit code and transience from a DetailedIOException cause so that the failure detail is preserved through module extension evaluation and the build is retried, refetching the repo.
testLostRemoteFile_sourceDirectoryMaterialization covers a cached repo whose only lost CAS blob lies below a source directory input consumed by a local action: the materialization of just that subtree discovers the loss and must trigger the transparent refetch that heals the cache entry.
A repo that is only reachable through a dependency edge is fetched during the analysis of the target that depends on it. If such a fetch fails because a file is no longer available in the remote cache, the resulting REMOTE_CACHE_EVICTED exit code was replaced with a generic one and the build was not retried, in both of the ways such an error can be reported: With --keep_going, the error is aggregated into an ErrorProcessingResult and BuildView#createAnalysisFailureDetail turns it into a generic loading or analysis phase failure. The detailed exit code of an analysis root cause that can be resolved by a retry is now preserved and takes precedence over these generic failure details. With --nokeep_going, the error is thrown as a ViewCreationFailedException whose failure detail does describe the lost file, but BuildTool unconditionally paired it with the PARSING_FAILURE exit code. The exit code implied by the failure detail is now kept if it allows a retry.
The remote worker used by the tests serves an action result only after checking that all blobs it references are still present in the CAS. Real remote caches commonly don't do this, which is precisely what allows a repo's cached Tree to outlive the contents of its files and is the situation the remote repo contents cache has to recover from. Run the tests against a worker with --noaction_cache_integrity_check so that a repo whose blobs have been deleted is served from the cache rather than silently turning into a cache miss.
A repo that is only reachable through an implicit attribute of a top-level aspect is fetched during the analysis of that aspect. If the fetch fails because a file is no longer available in the remote cache, the resulting REMOTE_CACHE_EVICTED exit code was replaced with a generic one with --keep_going and the build was not retried. Unlike a ConfiguredValueCreationException, whose root causes carry the detailed exit code all the way into the ErrorProcessingResult, the root causes of an AspectCreationException were dropped and the aspect's IndividualErrorProcessingResult was always reported with an empty set of analysis root causes. BuildView#createAnalysisFailureDetail then fell back to NOT_ALL_TARGETS_ANALYZED. Keep the causes of an AspectCreationException, just like those of a ConfiguredValueCreationException, which both restores the retry and populates the AnalysisFailureEvent posted for a top-level aspect.
A file lost from the remote cache is never specific to a single repo, but it is only noticed when that particular file is read. Repos with lost files were thus discovered one at a time, each costing one of the bounded number of build retries: a module extension that materializes one cached repo after another aborts at the first failure and only gets to the next repo once the previous one has been refetched. With enough affected repos, --experimental_remote_cache_eviction_retries is exhausted before the build can recover. Once a lost file has been observed, discard all repos whose contents were restored from the cache, not just those in which a lost file was found, and have the next command verify a repo's cached contents against the remote cache before restoring them. A cached Tree that references a blob the cache no longer has is treated as a miss, so the repo is fetched and its contents are uploaded anew right away. All affected repos therefore recover in the same retry. Builds that don't encounter a lost file are unaffected: no additional FindMissingBlobs requests are sent and no repos are discarded.
--nobuild turns off the merged analysis and execution phase, and so do cquery and aquery, which imply it. Analysis errors are then reported through AnalysisPhaseRunner and SkyframeErrorProcessor#processErrors without the execution phase rather than through AnalysisAndExecutionPhaseRunner. Run the tests for the retry after a lost remote file discovered during analysis with --nobuild as well. The aspect variant with --keep_going fails without the preceding commit.
With --keep_going and a second, healthy target, the analysis error of a repo that could not be fetched is reported together with the results of actions that did execute. Release branches that still have --experimental_skyframe_error_handling_refactor handle that case in a separate code path that derives the exit code without looking at analysis root causes, so the lost remote file has to survive it for the build to be retried.
…ontents cache When an action input is a source file in an external repository served from the remote repo contents cache and the remote cache loses the file's contents, the action fails with a lost inputs exception. Source artifacts have no generating action to rewind, so this previously crashed action rewinding (or fell back to restarting the build when invocation retries are enabled). With --rewind_lost_inputs, such lost source files are now recovered within the build by rewinding the Skyframe chain that produced the file's metadata: the source artifact, its file and file state nodes, and the fetch of the repository containing it. The repo is marked as having lost files in the RemoteExternalOverlayFileSystem, which makes the next remote repo contents cache lookup report a miss so that the repo rule is executed again. The refetch materializes the repo on the local file system and re-uploads its contents to the remote cache, after which the failed action is retried. (cherry picked from commit 27dbb60)
SequencedSkyframeProgressReceiver assumed that FileKey nodes can only be dirtied during loading and analysis, but rewinding a lost source file served from the remote repo contents cache dirties them during the execution phase, when the incremental build monitor is null. (cherry picked from commit 8534b72)
Covers two genrules concurrently losing source file inputs served from external repositories, both for distinct repositories and for the same repository. The lost inputs are recovered by concurrently rewinding the source artifacts' metadata nodes together with the repository fetches, which re-execute the repo rules. The test lives in a separate class from RewindingTest because that test disables external repositories to preserve its action graph structure between blaze and bazel. (cherry picked from commit 2a798a0)
Adds two test cases to RepoRewindingTest: * An action that is already executing when the repository providing one of its inputs is refetched due to another action's lost input still completes successfully and is not rewound itself. * A repo that materializes another repo (by reading one of its files by label during its fetch) is unaffected by that repo being refetched to recover a lost input later in the build and picks up the refetched contents through its recorded inputs in the next build. (cherry picked from commit afdbf34)
A file of a cached repo can be read at any point at which Bazel fetches a repo or evaluates a module extension, none of which is an action that ActionRewindStrategy could rewind. Such a read previously failed the build with an exit code that made it start over from scratch, and since each attempt only ever discovers the first lost file, recovering N affected repos cost N of the bounded number of retries. While rewinding is enabled, the overlay file system now reports a lost file as a LostRepoFileException naming the repo that has to be fetched again. The failing node returns a Reset that rewinds that repo's fetch and is re-evaluated once the repo rule has run again, all within the same build. A module extension that reads files of several such repos aborts at the first one, so it simply rewinds once per repo instead of restarting the build once per repo. Rewinding the repo fetch alone is not enough: reading a file by label resolves its package first, and a package lookup that stays valid hands out the very repo root whose contents are stale, so the rewound fetch is never consulted. The lost file therefore records the label it was read through and the package lookup is rewound in between. Materializing a repo moves to the RewindableRepoFileSystem interface so that it no longer depends on the concrete remote file system, which also lets tests exercise the path on which the loss surfaces in production. Hooked up for module extension evaluation and repo fetching.
A BUILD file of a cached repo is read while loading its package, which is not an action that ActionRewindStrategy could rewind, so losing it from the remote cache still restarted the whole build. PackageFunction now recovers such a file the same way repo rules and module extensions already do: rewind the fetch of the repo containing it and re-evaluate once the repo rule has run again. It knows the package it read the file for, so the package lookup is rewound in between without having to plumb a label through. A file read on behalf of a different package, such as a .bzl file that a BUILD file loads from another repo, is left alone: recovering it needs the load in between rewound as well, and an incomplete rewind graph leaves the package missing rather than merely unrecovered. Those still fall back to restarting the build. Note that a lost file does not surface when its metadata is read: the overlay file system has that injected, so stat never reaches the cache and only reading the contents fails. FileStateFunction is therefore not involved.
Runs the remote repo contents cache tests with --rewind_lost_inputs, so that every way in which the cache can lose a file is recovered within the build rather than by restarting it. None of them expects a retry any more, and the lost file is no longer reported as an error since the build recovers from it. Two gaps had to be closed for that: A failed materialization was memoized in the map of in-flight materializations, so a repo rule that ran again after its repo had been rewound was handed the previous failure instead of materializing the repo anew. Failures are no longer remembered. The contents of a source directory input are materialized while prefetching an action's inputs, where a lost file surfaced as a plain I/O error that failed the action outright. It is now reported as a lost input naming the source directory, which lets the existing action rewinding recover it by refetching the repo, just like a lost regular file input.
Rewinding recovers every way in which the remote cache can lose a file of a cached repo within the build, so the machinery that instead restarted the whole invocation is no longer needed. Gone are the propagation of a retryable exit code out of analysis, both for configured targets and for aspects, and the recovery that discarded every cached repo and verified the remaining ones against the cache on the following attempt so that a single retry sufficed. Recovering from a lost repo file now requires --rewind_lost_inputs.
Rewinding refetches a repo whose file the remote cache has lost while the build that needs it is still running, so remembering across commands that a repo has to be fetched rather than restored from the cache no longer serves a purpose.
Rewinding dirties file nodes to recover files that the remote repo contents cache has lost, which is not a change to a source file and thus must not be accrued by the incremental build monitor. The monitor happens to be null at that point, since the changed files are drained before the execution phase, but a null check says nothing about why reporting would be wrong and would silently start reporting again if the monitor ever outlived the loading phase. Skip rewound nodes explicitly instead. Also restores the .gemini configuration dropped by an earlier rebase.
A cache lookup cleared the record that the remote cache had lost files of a repo, on the assumption that the lookup's cache miss would be followed by a fetch that uploads the repo anew. A lookup is not that promise: the fetch is skipped with --nofetch and restarted when the repo rule's state object is dropped under memory pressure. The record is now kept until the contents have actually been uploaded, which is also when the previously lost files become available again. This restores the behavior that the retry-based recovery had, whose reasons for it apply just as much to recovery by rewinding. Not covered by a test: with rewinding enabled, restoring stale contents is not observable in the build's result, since reading the lost file merely rewinds the repo's fetch a second time.
Lost repo files are recovered by rewinding, so none of the machinery that made them surface as the exit code triggering a whole-build retry is used any more. Revert it to its state before this branch: the causal chain walk in `PackageLookupFunction`, `RepositoryFetchException`'s `DetailedExitCode` constructor, `ExternalDepsException`'s transience, and `DetailedIOException` becomes final again. A lost `.bzl` file can't reach loading in the first place: `.bzl` and `REPO.bazel` files are prefetched when a repo is injected and a lost blob among them turns the injection into a plain cache miss. Drop the label plumbing through `BzlLoadFunction` that existed to rewind such a file, and with it `RepoRewinding`'s fallback to the whole-build retry when the label resolves in another package. `vfs/LostRepoFileException` and `LostRepoFileWhileReadingLabelException` collapse into a single `LostRemoteRepoFileException` next to `RepoRewinding`, holding the repo to refetch and, once `getPathFromLabel` has attached it, the label the file was read through.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Recovers files that the remote repo contents cache has lost through Skyframe rewinding instead of restarting the whole build.
--rewind_lost_inputsalready recovers a lost source file that an action consumes (#29818). A file of a cached repo is also read at points that are not actions at all: while a repo rule runs, while a module extension is evaluated, and while a package is loaded. Those still failed the build withREMOTE_CACHE_EVICTEDand were left to the bounded retry of the whole invocation.While rewinding is enabled, the overlay file system now reports a lost file as a
LostRepoFileExceptionnaming the repo that has to be fetched again. The failing Skyframe node returns aResetthat rewinds that repo's fetch and is re-evaluated once the repo rule has run again, all within the same build.Rewinding the repo fetch alone is not enough. Reading a file by label resolves its package first, and a package lookup that stays valid hands out the very repo root whose contents are stale, so the rewound fetch is never consulted. The lost file therefore records the label it was read through, and the package lookup is rewound in between.
Two smaller pieces come along: materializing a repo moves onto the
RewindableRepoFileSysteminterface so that it no longer depends on the concrete remote file system, and a file read on behalf of a different package (a BUILD file loading a.bzlfile from another repo) deliberately falls back to the bounded retry rather than building an incomplete rewind graph..bzlfiles need no handling of their own: they are prefetched when a repo is injected, so a lost one is already reported as a cache miss at that point, and the prefetched contents are written to the local file system, so a later read never reaches the remote cache.Stacked on #30226 and #29818, whose commits this branch currently carries. Only the last two commits are new.
Motivation
With the remote repo contents cache enabled, a cache that serves action results without checking that the blobs they reference still exist can hand out a repo whose
Treeoutlived its file contents. Recovering by restarting the build costs one of the bounded--experimental_remote_cache_eviction_retriesper affected repo, because each attempt only ever discovers the first lost file. A module extension that materializes one cached repo after another aborts at the first failure and only reaches the next repo once the previous one has recovered.Rewinding makes that cost disappear: each affected repo is simply rewound in place, so a workspace with many stale repos recovers in a single invocation.
Build API Changes
No
Checklist
Release Notes
RELNOTES: None