Repository navigation
Conversation
`execute_with_progress` retried only the opening of the `Execute` call. Once the stream was open, any error on it, such as a connection reset or a GOAWAY, failed the action even though the operation was still running on the server. Follow Bazel's `GrpcRemoteExecutor`: once the server has named the operation, reattach to it with `WaitExecution` instead of failing or re-queueing. `NOT_FOUND` or `UNIMPLEMENTED` while resuming means the server no longer knows the operation, so start over with `Execute`. A stream that closes cleanly without a result is resumed without spending the retry budget, since REAPI allows an execution to outlive a connection, but a server that keeps resuming without reporting anything is given up on. Retries are bounded per action (5, matching Bazel's `--remote_retries` default) and use the same exponential backoff and jitter as `retry`, now shared between the two. Errors are classified by the existing `is_retryable`; `DEADLINE_EXCEEDED` is additionally retried while resuming, but not on a fresh `Execute`, where REAPI defines it as the action's own timeout.
A finished operation that carries a transient failure, in `Operation.error` or `ExecuteResponse.status`, was surfaced as a terminal error. Only failures of the gRPC call itself were retried, so a worker-side problem the server reported this way failed the build. Treat `UNAVAILABLE`, `ABORTED`, `UNKNOWN` and `INTERNAL` on a finished operation as transient, as Bazel's `GrpcRemoteExecutor` does: forget the operation and start over with a fresh `Execute`, since `WaitExecution` would only return the same failure. Restarts draw on the existing per-action budget (`MAX_EXECUTION_RETRIES`), so an action runs at most one more time than that. The error now names the status, which `REClientError` alone does not print. `DEADLINE_EXCEEDED` (the action's own timeout) and `RESOURCE_EXHAUSTED` (may be the action asking for more than any worker offers) are still not retried, nor are permanent codes. A command that exits non-zero is unaffected: REAPI reports it with an `OK` status.
meta-cla
Bot
added
the
CLA Signed
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D125105000. (Because this pull request was imported automatically, there will not be any future comments.) |
Contributor
|
Seems good to me. But this one needs a rebase |
scottcao
approved these changes
Oct 29, 2026
scottcao
left a comment
scottcao
left a comment
Contributor
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
This branch has not been deployed
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
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.
Goal
When the server finishes an operation with a transient error, in
Operation.errororExecuteResponse.status, the action fails. Only failures of the gRPC call itself are retried (#1569). A worker-side failure that the server reports this way, such as an I/O error on the worker, fails the build even though running the action again would succeed.Change
UNAVAILABLE,ABORTED,UNKNOWNandINTERNALon a finished operation are treated as transient. Bazel'sGrpcRemoteExecutorlikewise retries bothOperation.errorandExecuteResponse.status.Execute.WaitExecutionwould only return the same failure.MAX_EXECUTION_RETRIES, with the same backoff, so an action the server keeps failing runs at most six times.Server failed the execution with status Unavailable: ….REClientErroron its own prints only the server's message.wait_to_retryis split out asback_off, which both paths use. Transport-error classification is unchanged.Deliberately not retried:
OKstatus. The command ran.DEADLINE_EXCEEDEDRESOURCE_EXHAUSTEDINVALID_ARGUMENT,FAILED_PRECONDITION, …)Testing
The two tests that pinned no-retry on
INTERNALkeep their intent but now use inputs that genuinely should not be retried. New tests cover the retried cases, the bound, and the full classification.execution_does_not_retry_a_nonzero_exit_code(new)execution_does_not_retry_a_timed_out_action(wasexecution_does_not_retry_a_failed_action, nowDEADLINE_EXCEEDED)execution_does_not_retry_a_permanent_operation_error(wasexecution_does_not_retry_an_operation_error, nowINVALID_ARGUMENT)execution_does_not_retry_resource_exhaustion(new)execution_restarts_after_a_transient_operation_error(new:UNAVAILABLEinOperation.error, asserts a freshExecute)execution_restarts_after_an_internal_action_status(new:INTERNALinExecuteResponse.status)execution_stops_restarting_once_the_budget_is_spent(new)execution_restarts_only_on_transient_statuses(new: table over all sixteen non-OK codes)Results:
cargo test -p remote_execution --lib: 50 passed.python3 test.py --lint-only remote_execution(clippy with--deny=warnings) andcargo fmt --check: clean.