Fix rewinding bugs with --remote_cache_async - #30675
Draft
fmeum wants to merge 3 commits into
Draft
Conversation
This guards against interrupts and Errors causing locks to be held indefinitely. Along the way add more profiler spans.
Actions generated by an `ActionTemplate` have `TreeFileArtifact`s as outputs, whose generating action key is that of the expansion action. Consumers, however, depend on the parent tree artifact and thus lock the key of the *template*, so a rewound expansion action acquired a write lock that no consumer ever contended on and could prepare for its re-execution while its outputs were still being read. Both the outputs of a rewound action and the inputs of any action are now mapped to the generating action key of the outermost tree artifact containing them, which is the template for the outputs of an expansion. This key is shared by all output trees of a template and is thus still a single lock per action - the proof of deadlock freedom relies on this. Since the actions of a single expansion may form a chain, an action can have an input that is guarded by the very key it holds the write lock of. As the locks aren't reentrant, a rewound action skips the read lock of that key. Also keys `outputUploadTasks` by `ActionLookupData` instead of by the `Action` instance, which isn't stable across rewinding for actions generated by an `ActionTemplate`.
Fixes two issues in the synchronization of rewound actions with the consumers of their outputs: * Output upload tasks were only ever removed from `outputUploadTasks` when the corresponding action was rewound, so they accumulated (and retained the action as well as its spawn result) for the duration of the build. Since `registerOutputUploadTask` threw on a second registration for the same action, an action uploading its outputs twice would also have failed the build. Tasks are now unregistered on completion and multiple concurrent tasks per action are permitted. * Cancelling an output upload before the background task started running blocked forever: the body of a task submitted to an `ExecutorService` is skipped entirely if its future is cancelled before it starts, so neither the latch nor the completion callback (which closes the in-flight execution in `RemoteSpawnCache`) ever ran. The upload is now started via `execute` and cancelled by interrupting its thread, which always runs the body and thus the completion callback.
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
Output upload tasks were never unregistered:
outputUploadTasksentries were only removed when the corresponding action was rewound, so every asynchronously uploaded spawn left behind an entry retaining the action and the upload's future for the rest of the build. BecauseregisterOutputUploadTaskthrew on a second registration for the same action, an action uploading outputs twice would also have failed the build. Today this could only happen for remote include scanning since other multi-spawn actions all happen to returntruefrommayModifySpawnOutputsAfterExecution(), but that isn't necessarily going to remain the case. Tasks are now unregistered on completion, and multiple concurrent tasks per action are permitted.Cancelling an upload before it started blocked forever: Guava's
InterruptibleTask#runskips the task body entirely if the future is already done, so acancel(true)landing betweensubmit()returning and the virtual thread entering the body meant neitheruploadDone.countDown()noronUploadCompleteever ran. The rewinding thread then waited on the latch forever while holding the write lock, and the completion callback was never invoked, stranding any spawn deduplicating against that execution. The upload is now guarded by a claim that either the background task or the canceller wins, and the winner runs the completion callback.Motivation
Build API Changes
No
Checklist
Release Notes
RELNOTES: None