Skip to content

Clarify UpdateActionResult requirements #351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −0
Open

Clarify UpdateActionResult requirements #351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions build/bazel/remote/execution/v2/remote_execution.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,11 @@ service ActionCache {
// [Command][build.bazel.remote.execution.v2.Command], into the
// `ContentAddressableStorage`.
//
// The client SHOULD also upload other blobs transitively referenced by the
// [Action][build.bazel.remote.execution.v2.Action] message into the
// `ContentAddressableStorage`. The server MAY return `FAILED_PRECONDITION`
// if this condition has not been met.
Comment on lines +183 to +186
Copy link
Contributor

@mostynb mostynb Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious- why is this recommended?

If the this call is made by a remote execution worker uploading build results then all the CAS blobs required by the Action must already be in the cache. And if the caller is a regular client just using remote caching and not remote execution, then the Action is not technically required to be in the CAS.

Copy link
Collaborator Author

@tjgq tjgq Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should read "The client is not required to" instead of "The client SHOULD"? The discussion happened a while ago, but if I recall correctly, the intent was precisely to document that clients are free not to meet the precondition, and servers are not required to enforce it. (If one of the other maintainers remembers it differently, please chime in.)

Copy link
Collaborator

@fmeum fmeum Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The less strict version sounds good to me.

Copy link
Collaborator

@fmeum fmeum Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The less strict version sounds good to me.

//
// Server implementations MAY modify the
// `UpdateActionResultRequest.action_result` and return an equivalent value.
//
Expand Down