Guard ActionTemplate outputs by the key of the template - #30654
Open
fmeum wants to merge 1 commit into
Open
Conversation
fmeum
force-pushed
the
rewound-action-synchronizer-action-template
branch
6 times, most recently
from
August 11, 2026 09:22
fab692f to
0f4ddc0
Compare
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`.
fmeum
force-pushed
the
rewound-action-synchronizer-action-template
branch
from
August 11, 2026 09:47
0f4ddc0 to
e03701d
Compare
fmeum
marked this pull request as ready for review
August 11, 2026 09:51
github-actions
Bot
added
team-Core
Collaborator
Author
|
@justinhorvitz for the new test case in |
Collaborator
Author
|
@bazel-io fork 9.3.0 |
coeuvre
approved these changes
Aug 11, 2026
fmeum
commented
Aug 11, 2026
| * <p>The more of them are reading it when the expansion is rewound, the more likely it is that a | ||
| * missing mutual exclusion is detected. | ||
| */ | ||
| private static final int TREE_CONSUMER_COUNT = 8; |
Collaborator
Author
There was a problem hiding this comment.
This could probably be reduced to 4 or 3 if it causes issues otherwise.
justinhorvitz
approved these changes
Aug 11, 2026
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
3 participants
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
RemoteRewoundActionSynchronizerkeeps a rewound action from preparing for its re-execution while other actions are still consuming its outputs. Both keys were derived from the generating action key of the artifact itself, which is wrong for actions generated by anActionTemplate: their outputs areTreeFileArtifacts owned by the expansion, whereas consumers depend on the parent tree artifact, which is owned by the template. A rewound expansion action thus acquired a write lock that no consumer would ever lock.Both sides now derive the key from the outermost tree artifact containing the artifact, so the outputs of an expansion are guarded by the key of the template, which is shared by all tree artifacts a template declares.
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. The locks aren't reentrant, so a rewound action now skips the read lock of that key.
Also keys
outputUploadTasksbyActionLookupDatainstead of by theActioninstance, which isn't stable across rewinding for actions generated by anActionTemplate.Motivation
Without this, an action that consumes a tree artifact populated by an
ActionTemplatecan read its files while the expansion action that generated them is being rewound.Build API Changes
No
Checklist
Release Notes
RELNOTES: None