[AI-7115] Add a request retry strategy to the async GitHub client - #24963
[AI-7115] Add a request retry strategy to the async GitHub client#24963AAraKKe wants to merge 8 commits into
Conversation
- New retry.py: RetryPolicy plus composable predicates, executed by stamina. - Split the two layers: _request retries, _rate_limited_request handles rate limits. - Per-endpoint defaults by replay safety, overridable per call with retry=. - Never follow or retry an unexpected redirect; report it with the endpoint. - Retry the artifact redirect and signed download as a pair. - Expose the limits through [dispatcher.github_retries].
evalya-impact-summaryevalya impact analysis |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: d96c710 | Docs | View more details | Give us feedback! |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c86bea94d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Move the retry config into dispatcher_config, next to the other config models. - Group module constants at the top of retry.py and trim the comments. - RetryPolicy is a plain class with a typed replace instead of a dataclass. - Move the client-specific guard and the retry cause into the client module. - Redact the query string from the artifact URL before logging it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3b5071d14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
httpx builds its message from the full URL, so a retryable failure from the storage host carried the presigned signature into this client's log line and stamina's retry hook. Raise without the URL instead of redacting at each sink.
There was a problem hiding this comment.
💡 Codex Review
Preserve request context when wrapping transport errors
When any HTTPX transport failure occurs, the original exception has already been associated with the outgoing request, but constructing a replacement with only a message discards that context. Callers using the standard exc.request.url pattern will therefore get RuntimeError: The .request property has not been set, and error reporting loses the request metadata; pass the original request when constructing the replacement or preserve and re-raise the existing exception.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…logs A transport error's reason is quoted into our message and, with a chained cause, printed in full by Python. Both were URL-free only because of how httpx builds that message. Redact the query string and drop the chain instead.
Keeps the parameter names, which say which signing scheme was in play, and masks every value rather than the ones known to be secret: the parameter carrying the signature is X-Amz-Signature on S3 and sig on Azure Blob, so an allowlist would leak the first time a download redirects somewhere new.
Building a replacement exception dropped the request httpx had attached, so exc.request raised RuntimeError for a caller. Rewrite the message in place, as the artifact download already does.
|
On the Worth noting it is not introduced by this PR. The same wrapping sits at Applied the same fix to |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97f6e0ef83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| NO_RETRY = RetryPolicy(attempts=1) | ||
| SAFE_RETRY = RetryPolicy(should_retry=any_of(on_transport_error, on_status(*RETRYABLE_SERVER_STATUSES))) | ||
| MUTATION_RETRY = RetryPolicy(should_retry=on_pre_send_transport_error, attempts=MUTATION_ATTEMPTS) |
There was a problem hiding this comment.
Because these exported constants—and DEFAULT_RETRY_POLICIES—reuse the same mutable RetryPolicy instances, a caller that tunes one in place (for example, SAFE_RETRY.attempts = 1 or by replacing should_retry) silently changes retry behavior for every existing and future default client in the process. This can unexpectedly disable retries or widen mutation retries across concurrent clients; make policy attributes immutable and require callers to use replace().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This has been done on porpuse because we own the code that use it and it is up to us to not do that. The other option, which is the one we went with originally, was to make them frozen dataclasses but then we would need to build a post_init method for avlidation, which seemed kind of overcomplicated for what we wanted to do. But the point about being shared makes sense, so we have moved them back to frozen dataclasses.
The defaults are shared for the life of the process, so tuning one in place changed every client that held it. Frozen dataclasses with __post_init__ validation are the idiomatic way to prevent that; the alternatives for a plain class cost more boilerplate for the same result.
Validation ReportAll 21 validations passed. Show details
|
1 participant
What does this PR do?
Adds a retry strategy to the async GitHub client for the failures that are not rate limiting, and separates it from the rate-limit handling that was already there.
Structure worth knowing before reading the diff:
_request(retries) wraps_rate_limited_request(today's loop, renamed). That order matters: each retry re-acquires the limiter, so it waits out any pause the governor is holding. Rate-limit responses stay owned by the inner layer and are never retried by the outer one.retry.pydescribes, stamina executes.RetryPolicyis data: what to retry on, how many attempts, what backoff. No sleeping or backoff arithmetic of ours.retry=to override, and policies compose.download_artifactretries as a pair. The signed URL expires, so the retry has to re-resolve the redirect rather than refetch a dead URL.[dispatcher.github_retries]). Widening what may be retried would make a duplicate side effect a setting.ddev/src/ddev/utils/github_async/AGENTS.mddocuments the layer boundary so the next change lands in the right one.Motivation
Closes AI-7115.
Dispatcher runs for hours and makes thousands of GitHub calls, and until now any failure that was not rate limiting failed on the first attempt. That gives a single blip more power than it should have.
TaskTestRunnerpollsget_workflow_runfor the whole life of a batch inside atry/finallywith noexcept, so one transient 500 aborts the batch, closes its check run as cancelled and throws away the results of every test in it. Other calls swallow the failure and quietly degrade instead: a failedlist_workflow_jobsreturns an empty job list, so job correlation silently loses data.Both get worse as we scale up: more batches and more polling mean more chances to hit the one blip that costs a whole batch of test results. Retrying is also a precondition for trusting the run report, since a report that is missing jobs because of a dropped connection is worse than one that is late.
No task behaviour changes here. Retries only make those paths less likely to fire, and a failure that outlives the ladder surfaces exactly as it does today.
Notes for review
ExecutionState.RETRYING,BatchProgress.retrying_jobs). This is unrelated and only concerns HTTP requests.test_no_retry_on_transport_errorbecametest_the_rate_limit_layer_does_not_retry_a_transport_errorand now calls_rate_limited_request. The property still holds for that layer, but at client level a GET transport error is now retried on purpose.GitHubAuthenticationError, which the guard refuses, so a real denial still fails immediately. Tested both ways.staminalogger, so retries are visible even with no logger injected. Turning it off is global and would also silence the unrelatedstamina.retryinddev/e2e/agent/docker.py, so I left it and documented it. Say the word if you want the client to be the only voice.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged