From 767688a9d179749ccdc20f3b8bc468cc63e76f9b Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 14:08:48 +0200 Subject: [PATCH 1/8] Add a request retry strategy to the async GitHub client - 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]. --- .../ddev/cli/ci/tests/dispatcher_config.py | 2 + ddev/src/ddev/cli/ci/tests/github_retries.py | 56 ++++ ddev/src/ddev/utils/github_async/AGENTS.md | 41 +++ ddev/src/ddev/utils/github_async/__init__.py | 12 + ddev/src/ddev/utils/github_async/client.py | 295 ++++++++++++++--- ddev/src/ddev/utils/github_async/retry.py | 219 +++++++++++++ ddev/src/ddev/utils/github_errors.py | 23 ++ .../cli/ci/tests/test_dispatcher_config.py | 8 + .../tests/cli/ci/tests/test_github_retries.py | 40 +++ ddev/tests/utils/github_async/conftest.py | 17 + ddev/tests/utils/github_async/helpers.py | 24 +- .../github_async/test_download_artifact.py | 56 +++- .../utils/github_async/test_rate_limiting.py | 10 +- ddev/tests/utils/github_async/test_retry.py | 300 ++++++++++++++++++ 14 files changed, 1051 insertions(+), 52 deletions(-) create mode 100644 ddev/src/ddev/cli/ci/tests/github_retries.py create mode 100644 ddev/src/ddev/utils/github_async/retry.py create mode 100644 ddev/tests/cli/ci/tests/test_github_retries.py create mode 100644 ddev/tests/utils/github_async/conftest.py create mode 100644 ddev/tests/utils/github_async/test_retry.py diff --git a/ddev/src/ddev/cli/ci/tests/dispatcher_config.py b/ddev/src/ddev/cli/ci/tests/dispatcher_config.py index fc9d45cf33f10..9e8f8234d07df 100644 --- a/ddev/src/ddev/cli/ci/tests/dispatcher_config.py +++ b/ddev/src/ddev/cli/ci/tests/dispatcher_config.py @@ -9,6 +9,7 @@ from pydantic import BaseModel, ConfigDict, Field +from ddev.cli.ci.tests.github_retries import GitHubRetryConfig from ddev.cli.ci.tests.rate_limiting import RateLimiterFactoryConfig if TYPE_CHECKING: @@ -36,6 +37,7 @@ class DispatcherConfig(BaseModel): default_python_version: str = Field(default="3.13", pattern=r"^\d+\.\d+$") batching: BatchingConfig = BatchingConfig() github_rate_limits: RateLimiterFactoryConfig = RateLimiterFactoryConfig() + github_retries: GitHubRetryConfig = GitHubRetryConfig() @classmethod def from_repo_config(cls, repo_config: RepositoryConfig) -> DispatcherConfig: diff --git a/ddev/src/ddev/cli/ci/tests/github_retries.py b/ddev/src/ddev/cli/ci/tests/github_retries.py new file mode 100644 index 0000000000000..f22c2def199d7 --- /dev/null +++ b/ddev/src/ddev/cli/ci/tests/github_retries.py @@ -0,0 +1,56 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Dispatcher-facing configuration for the GitHub client's retry strategy. + +Only *how hard* to try is configurable. *What* may be retried stays in +``ddev.utils.github_async.retry``, because it follows from whether an endpoint can be replayed +safely: a config file that could widen it would turn a duplicate workflow run into a setting. +""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, Field + +from ddev.utils.github_async.retry import MUTATION_RETRY, SAFE_RETRY, RetryPolicies, RetryPolicy + + +class RetryLimitsConfig(BaseModel): + """Attempt and backoff limits for one class of request.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + attempts: int = Field(default=3, ge=1) + # Bounds the whole ladder including backoff. None leaves attempts as the only stop condition. + timeout_seconds: float | None = Field(default=60.0, gt=0) + wait_initial_seconds: float = Field(default=0.5, gt=0) + wait_max_seconds: float = Field(default=10.0, gt=0) + wait_jitter_seconds: float = Field(default=1.0, ge=0) + + def apply_to(self, policy: RetryPolicy) -> RetryPolicy: + """*policy* with these limits, keeping the conditions it retries on.""" + return policy.replace( + attempts=self.attempts, + timeout=self.timeout_seconds, + wait_initial=self.wait_initial_seconds, + wait_max=self.wait_max_seconds, + wait_jitter=self.wait_jitter_seconds, + ) + + +class GitHubRetryConfig(BaseModel): + """Retry limits for the Dispatcher's GitHub client, read from ``[dispatcher.github_retries]``.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + safe: RetryLimitsConfig = RetryLimitsConfig() + # Fewer attempts by default: a mutation only retries when the request provably never left, which + # a second attempt either fixes at once or is unlikely to fix at all. + mutating: RetryLimitsConfig = RetryLimitsConfig(attempts=2) + + def to_policies(self) -> RetryPolicies: + """Build the client's policies, these limits over the built-in conditions.""" + return RetryPolicies( + safe=self.safe.apply_to(SAFE_RETRY), + mutating=self.mutating.apply_to(MUTATION_RETRY), + ) diff --git a/ddev/src/ddev/utils/github_async/AGENTS.md b/ddev/src/ddev/utils/github_async/AGENTS.md index 3d839f2fc69d0..b7f04b1341add 100644 --- a/ddev/src/ddev/utils/github_async/AGENTS.md +++ b/ddev/src/ddev/utils/github_async/AGENTS.md @@ -103,6 +103,47 @@ Follow the block with an `Args:` section that describes each argument, and a `Returns:` section describing the wrapped response. This keeps every method traceable to its source contract and makes the public surface self-explanatory. +## Two retry layers, and which one owns a failure + +Failures are retried in two places, and adding a retry to the wrong one is the mistake this section +exists to prevent. + +- **Rate limiting** (`_rate_limited_request`, with `ddev.utils.rate_limiting`) owns 403 and 429 + responses that the headers confirm are rate limiting. Re-acquiring the limiter *is* the backoff, so + there is no sleeping or arithmetic there. Never add a sleep or a wait to this layer. +- **The retry strategy** (`_request`, with `retry.py`) owns everything else: transport failures and + 5xx. stamina executes it, so there is no backoff arithmetic here either. + +The strategy wraps the rate-limit layer, never the reverse, so every attempt re-acquires the limiter +and waits out any pause the governor holds. + +When adding an endpoint method, decide its default by whether the request can be **replayed**, not by +its verb: + +- A GET takes the default, `retry` forwarded straight through. +- A mutation that is idempotent (sets given fields to given values, or is a no-op when repeated) + passes `retry=retry if retry is not None else self._retry_policies.safe` and says why in the + docstring. `update_check_run`, `update_issue_comment` and `add_labels_to_issue` are the examples. +- Any other mutation takes the default, which only retries failures that prove the request never + reached GitHub. Widening one of these is how you get a duplicate workflow run or a second comment. + +What may be retried is not configurable; `[dispatcher.github_retries]` tunes attempts and backoff +only. A config file that could widen the conditions would make a duplicate side effect a setting. + +The client reports its own retries through the logger passed to it, and stays quiet when there is +none. Separately, stamina installs a process-wide `on_retry` hook that logs every scheduled retry to +the `stamina` logger, so retries are visible even with no logger injected. That hook is global, and +disabling it would also silence the unrelated `stamina.retry` in `ddev/e2e/agent/docker.py`, so it is +left alone here rather than reached into from this package. + +## Never follow a redirect + +The client does not follow redirects, because the `Authorization` header would travel to whatever +host `Location` names. An unexpected 3xx raises `GitHubUnexpectedRedirectError` naming the endpoint, +and no policy may retry one. `download_artifact` is the single endpoint whose contract is a redirect; +it opts in with `expect_redirect=True` and reads `Location` itself. Do not enable `follow_redirects` +to make a failing request work. + ## One method per API endpoint Always keep exactly one method per API endpoint. Do not create convenience diff --git a/ddev/src/ddev/utils/github_async/__init__.py b/ddev/src/ddev/utils/github_async/__init__.py index edc0007d4a7d6..72f3674161e53 100644 --- a/ddev/src/ddev/utils/github_async/__init__.py +++ b/ddev/src/ddev/utils/github_async/__init__.py @@ -31,6 +31,12 @@ from .client import GitHubResponse as GitHubResponse from .client import PaginationData as PaginationData from .client import async_github_client as async_github_client + from .retry import DEFAULT_RETRY_POLICIES as DEFAULT_RETRY_POLICIES + from .retry import MUTATION_RETRY as MUTATION_RETRY + from .retry import NO_RETRY as NO_RETRY + from .retry import SAFE_RETRY as SAFE_RETRY + from .retry import RetryPolicies as RetryPolicies + from .retry import RetryPolicy as RetryPolicy # Map of exported name -> submodule (relative to this package) that defines it. MODULE_BY_NAME: dict[str, str] = { @@ -41,6 +47,12 @@ 'COMMENT_BODY_LIMIT': 'client', 'GitHubResponse': 'client', 'PaginationData': 'client', + 'RetryPolicy': 'retry', + 'RetryPolicies': 'retry', + 'NO_RETRY': 'retry', + 'SAFE_RETRY': 'retry', + 'MUTATION_RETRY': 'retry', + 'DEFAULT_RETRY_POLICIES': 'retry', } diff --git a/ddev/src/ddev/utils/github_async/client.py b/ddev/src/ddev/utils/github_async/client.py index aac236ac31809..acc91527b65f7 100644 --- a/ddev/src/ddev/utils/github_async/client.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -6,6 +6,7 @@ from __future__ import annotations import io +import logging import re import zipfile from collections.abc import AsyncIterator, Callable @@ -15,18 +16,20 @@ from typing import Any, Literal, Self, overload import httpx +import stamina from pydantic import BaseModel, ConfigDict, Field from ddev.utils.github_errors import ( GITHUB_AUTHENTICATION_STATUS_CODES, GitHubAuthenticationError, GitHubBodyTooLongError, + GitHubUnexpectedRedirectError, github_body_too_long_message, github_secondary_rate_limit_wait, ) -from ddev.utils.rate_limiting import NULL_SNAPSHOT, BudgetSnapshot, InstrumentedAsyncLimiter +from ddev.utils.rate_limiting import NULL_SNAPSHOT, BudgetSnapshot, InstrumentedAsyncLimiter, RateLimitEvent -from .defaults import default_github_rate_limiter +from .defaults import default_github_rate_limiter, log_rate_limit_events from .models import ( ArtifactsList, CheckRun, @@ -39,6 +42,17 @@ WorkflowJobsList, WorkflowRun, ) +from .retry import ( + DEFAULT_RETRY_POLICIES, + NO_RETRY, + RetryPolicies, + RetryPolicy, + RetryTracker, + is_redirect_status, + on_status, + refuses_retry, + retry_attempts, +) GITHUB_API_VERSION = "2022-11-28" DEFAULT_BASE_URL = "https://api.github.com" @@ -131,8 +145,12 @@ class AsyncGitHubClient: Rate-limit protection is on by default: requests are paced and, when GitHub signals a rate-limit rejection, retried in reaction to the response headers. The governor supplies the - backoff, so there is no sleeping or backoff arithmetic in this client. The default protection - logs through the ``ddev.utils.github_async.defaults`` logger. + backoff, so there is no sleeping or backoff arithmetic in this client. + + Failures that are *not* rate limiting, a dropped connection or a 502, are handled separately by + the retry strategy in ``retry.py``. The two layers answer different questions, "GitHub told us to + wait" against "that request did not land, ask again", and each endpoint method takes a ``retry`` + argument to override its default for a single call. Args: token: GitHub token; must be non-empty. @@ -150,9 +168,18 @@ class AsyncGitHubClient: max_rate_limit_retries: Extra attempts for a header-confirmed rate-limit response (403/429). Each retry is a full fresh acquisition (governor wait plus bucket token); the default of 2 covers the common "hit a secondary limit once, wait, succeed" case plus one repeat. - Only rate-limit responses are retried: transport errors and non-rate-limit statuses - propagate immediately, and RateLimitWaitAbandoned (the governor's ``max_wait_seconds`` - killswitch) propagates to the caller. + Only rate-limit responses are retried here; every other failure belongs to the retry + strategy, and RateLimitWaitAbandoned (the governor's ``max_wait_seconds`` killswitch) + reaches the caller untouched by either layer. + retry_policies: Overrides the defaults for failures that are not rate limiting. None uses + ``DEFAULT_RETRY_POLICIES``. *What* may be retried is a correctness property of each + endpoint rather than a preference, so policies supplied here are expected to change how + hard to try, not to widen the conditions. + logger: Where this client reports its retries. None silences its own reporting; stamina's + built-in retry instrumentation is global and independent of this (see AGENTS.md). A logger + given here also receives the events of the default rate limiter, while a caller-supplied + ``rate_limiter`` keeps whatever logging it was built with, since that choice belongs to + whoever built it. transport: Optional custom HTTPX transport (useful for testing with MockTransport). """ @@ -163,18 +190,30 @@ def __init__( rate_limiter: InstrumentedAsyncLimiter | None = None, default_timeout: float = 30.0, max_rate_limit_retries: int = 2, + retry_policies: RetryPolicies | None = None, + logger: logging.Logger | None = None, transport: httpx.AsyncBaseTransport | None = None, ) -> None: if not token: raise ValueError("GitHub token must not be empty.") + self._logger = logger # A None limiter means "use the default protection," not "no protection." The local bucket # is deliberately permissive because the governor is the protection; with a healthy budget # and no secondary limits the governor adds zero wait, so this default is invisible to # well-behaved callers and engages only once GitHub has already signaled backpressure. - self._rate_limiter = rate_limiter if rate_limiter is not None else default_github_rate_limiter() + self._rate_limiter = ( + rate_limiter if rate_limiter is not None else default_github_rate_limiter(on_event=self._limiter_events()) + ) self._default_timeout = default_timeout self._max_rate_limit_retries = max_rate_limit_retries + self._retry_policies = retry_policies if retry_policies is not None else DEFAULT_RETRY_POLICIES + self._refuses_retry = refuses_retry(self._is_rate_limit_response) + # An expired signed URL comes back from the storage host as a 403, and the cure is resolving + # the redirect again rather than refetching a dead URL, so the pair is retried on one. A 403 + # from GitHub itself arrives as GitHubAuthenticationError, which the guard refuses, so this + # cannot turn a real permission denial into a retry loop. + self._artifact_retry = self._retry_policies.safe.also_on(on_status(403)) self._headers = { "Authorization": f"Bearer {token}", "X-GitHub-Api-Version": GITHUB_API_VERSION, @@ -195,9 +234,25 @@ async def aclose(self) -> None: # Internal helpers # ------------------------------------------------------------------ + def _limiter_events(self) -> Callable[[RateLimitEvent], None] | None: + """Handler for the default rate limiter's events, routed to our logger when there is one.""" + return log_rate_limit_events(self._logger) if self._logger is not None else None + def _effective_timeout(self, timeout: float | None) -> float: return timeout if timeout is not None else self._default_timeout + def _log_retry(self, description: str, tracker: RetryTracker, attempt: stamina.Attempt) -> None: + """Report a retry that is about to run. Silent on the first attempt, and with no logger.""" + if attempt.num == 1 or self._logger is None: + return + self._logger.warning( + "Retrying %s after %r (attempt %s)", + description, + tracker.last_error, + attempt.num, + extra={"attempt": attempt.num, "error": repr(tracker.last_error)}, + ) + @staticmethod def _is_rate_limit_response(response: httpx.Response) -> bool: """Whether *response* is a retryable rate-limit rejection, by GitHub's own discrimination rule. @@ -218,6 +273,8 @@ async def _execute_request( method: str, endpoint: str, timeout: float, + *, + expect_redirect: bool = False, **kwargs: Any, ) -> httpx.Response: try: @@ -234,14 +291,24 @@ async def _execute_request( snapshot = replace(snapshot or NULL_SNAPSHOT, retry_after=secondary_rate_limit_wait) if snapshot is not None: self._rate_limiter.observe(snapshot) + if is_redirect_status(response.status_code): + # raise_for_status would turn a redirect into an HTTP error indistinguishable from any + # other, so both cases are named here: the one endpoint that expects a redirect gets the + # response back to read Location from, and everywhere else says what happened instead of + # leaving a caller to work out why an ordinary-looking request failed. + if expect_redirect: + return response + raise GitHubUnexpectedRedirectError.from_response(method, endpoint, response) response.raise_for_status() return response - async def _request( + async def _rate_limited_request( self, method: str, endpoint: str, timeout: float | None = None, + *, + expect_redirect: bool = False, **kwargs: Any, ) -> httpx.Response: effective_timeout = self._effective_timeout(timeout) @@ -258,7 +325,9 @@ async def _request( for attempt in range(self._max_rate_limit_retries + 1): async with self._rate_limiter: try: - return await self._execute_request(method, endpoint, effective_timeout, **kwargs) + return await self._execute_request( + method, endpoint, effective_timeout, expect_redirect=expect_redirect, **kwargs + ) except httpx.HTTPStatusError as exc: # A rate-limit 403/429 is safe to retry for every endpoint, including # non-idempotent POSTs, precisely because GitHub rejected it without performing @@ -273,6 +342,32 @@ async def _request( if exc.response.status_code in GITHUB_AUTHENTICATION_STATUS_CODES: raise GitHubAuthenticationError.from_http_status_error(exc) from exc raise + raise RuntimeError("unreachable: the rate-limit loop always returns or raises") # pragma: no cover + + async def _request( + self, + method: str, + endpoint: str, + timeout: float | None = None, + *, + retry: RetryPolicy | None = None, + expect_redirect: bool = False, + **kwargs: Any, + ) -> httpx.Response: + """Send one request, retrying the failures its policy accepts. + + Wraps the rate-limit layer instead of living inside it, so every attempt re-acquires the + limiter and therefore waits out any pause the governor holds. Retrying inside it would keep + the acquisition and hammer GitHub through its own backoff. + """ + policy = retry if retry is not None else self._retry_policies.for_method(method) + tracker = RetryTracker(policy, self._refuses_retry) + async for attempt in retry_attempts(policy, tracker): + with attempt: + self._log_retry(f"{method} {endpoint}", tracker, attempt) + return await self._rate_limited_request( + method, endpoint, timeout, expect_redirect=expect_redirect, **kwargs + ) raise RuntimeError("unreachable: the retry loop always returns or raises") # pragma: no cover async def _paginated_request( @@ -280,18 +375,24 @@ async def _paginated_request( method: str, endpoint: str, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, **kwargs: Any, ) -> AsyncIterator[httpx.Response]: - """Yield one httpx.Response per page, following Link headers.""" + """Yield one httpx.Response per page, following Link headers. + + The policy applies per page, so a flaky page is retried on its own without refetching the + pages already yielded. + """ url: str | None = endpoint first = True while url is not None: if first: - response = await self._request(method, url, timeout=timeout, **kwargs) + response = await self._request(method, url, timeout=timeout, retry=retry, **kwargs) first = False else: # Subsequent pages: use the absolute next URL, no extra kwargs - response = await self._request(method, url, timeout=timeout) + response = await self._request(method, url, timeout=timeout, retry=retry) yield response pagination = PaginationData.from_header(response.headers.get("link")) url = pagination.next @@ -317,6 +418,7 @@ async def create_workflow_dispatch( inputs: dict[str, str] | None = None, timeout: float | None = None, *, + retry: RetryPolicy | None = None, return_run_details: Literal[True], ) -> GitHubResponse[WorkflowDispatchResult]: ... @@ -330,6 +432,7 @@ async def create_workflow_dispatch( inputs: dict[str, str] | None = None, timeout: float | None = None, *, + retry: RetryPolicy | None = None, return_run_details: Literal[False] = False, ) -> GitHubResponse[None]: ... @@ -342,6 +445,7 @@ async def create_workflow_dispatch( inputs: dict[str, str] | None = None, timeout: float | None = None, *, + retry: RetryPolicy | None = None, return_run_details: bool = False, ) -> GitHubResponse[WorkflowDispatchResult] | GitHubResponse[None]: """ @@ -357,6 +461,8 @@ async def create_workflow_dispatch( ref: Branch or tag name to run the workflow on. inputs: Optional key/value inputs forwarded to the workflow. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's mutating policy, which will + not replay a dispatch that may have landed: doing so would start a duplicate run. return_run_details: When True, requests a 200 response with the new run's metadata (workflow_run_id, run_url, html_url) instead of the default 204 No Content. See https://github.blog/changelog/2026-02-19-workflow-dispatch-api-now-returns-run-ids/. @@ -375,6 +481,7 @@ async def create_workflow_dispatch( "POST", f"/repos/{owner}/{repo}/actions/workflows/{workflow_id}/dispatches", timeout=timeout, + retry=retry, json=body, ) if return_run_details: @@ -387,6 +494,8 @@ async def get_workflow_run( repo: str, run_id: int, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[WorkflowRun]: """ Calls the GitHub API to get a single workflow run. @@ -399,11 +508,14 @@ async def get_workflow_run( repo: Repository name. run_id: Numeric ID of the workflow run. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's policy for replayable requests. Returns: GitHubResponse[WorkflowRun]: The validated workflow run data and headers. """ - response = await self._request("GET", f"/repos/{owner}/{repo}/actions/runs/{run_id}", timeout=timeout) + response = await self._request( + "GET", f"/repos/{owner}/{repo}/actions/runs/{run_id}", timeout=timeout, retry=retry + ) return self._parse_response(response, WorkflowRun) async def list_workflow_run_artifacts( @@ -413,6 +525,8 @@ async def list_workflow_run_artifacts( run_id: int, per_page: int = 30, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> AsyncIterator[GitHubResponse[ArtifactsList]]: """ Calls the GitHub API to list artifacts for a workflow run (paginated). @@ -426,12 +540,15 @@ async def list_workflow_run_artifacts( run_id: Numeric ID of the workflow run. per_page: Number of artifacts per page (default 30, max 100). timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for each page. Defaults to the client's policy for replayable requests. Returns: AsyncIterator[GitHubResponse[ArtifactsList]]: One page of artifacts per iteration. """ endpoint = f"/repos/{owner}/{repo}/actions/runs/{run_id}/artifacts" - async for response in self._paginated_request("GET", endpoint, timeout=timeout, params={"per_page": per_page}): + async for response in self._paginated_request( + "GET", endpoint, timeout=timeout, retry=retry, params={"per_page": per_page} + ): yield self._parse_response(response, ArtifactsList) async def list_workflow_jobs( @@ -441,6 +558,8 @@ async def list_workflow_jobs( run_id: int, per_page: int = 30, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> AsyncIterator[GitHubResponse[WorkflowJobsList]]: """ Calls the GitHub API to list jobs for a workflow run (paginated). @@ -454,12 +573,15 @@ async def list_workflow_jobs( run_id: Numeric ID of the workflow run. per_page: Number of jobs per page (default 30, max 100). timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for each page. Defaults to the client's policy for replayable requests. Returns: AsyncIterator[GitHubResponse[WorkflowJobsList]]: One page of jobs per iteration. """ endpoint = f"/repos/{owner}/{repo}/actions/runs/{run_id}/jobs" - async for response in self._paginated_request("GET", endpoint, timeout=timeout, params={"per_page": per_page}): + async for response in self._paginated_request( + "GET", endpoint, timeout=timeout, retry=retry, params={"per_page": per_page} + ): yield self._parse_response(response, WorkflowJobsList) async def create_issue_comment( @@ -469,6 +591,8 @@ async def create_issue_comment( issue_number: int, body: str, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[IssueComment]: """ Calls the GitHub API to create a comment on an issue or pull request. @@ -482,6 +606,8 @@ async def create_issue_comment( issue_number: Issue or pull request number. body: Markdown body text of the comment. At most `COMMENT_BODY_LIMIT` UTF-8 bytes. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's mutating policy, since a + replayed create leaves a second comment behind. Returns: GitHubResponse[IssueComment]: The validated comment data and headers. @@ -490,7 +616,7 @@ async def create_issue_comment( GitHubBodyTooLongError: If `body` is too long, measured here or refused by GitHub. """ response = await self._comment_request( - "POST", f"/repos/{owner}/{repo}/issues/{issue_number}/comments", body=body, timeout=timeout + "POST", f"/repos/{owner}/{repo}/issues/{issue_number}/comments", body=body, timeout=timeout, retry=retry ) return self._parse_response(response, IssueComment) @@ -501,6 +627,8 @@ async def update_issue_comment( comment_id: int, body: str, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[IssueComment]: """ Calls the GitHub API to update an existing comment on an issue or pull request. @@ -514,6 +642,9 @@ async def update_issue_comment( comment_id: Numeric ID of the comment to update. body: New markdown body text of the comment. At most `COMMENT_BODY_LIMIT` UTF-8 bytes. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's policy for replayable + requests: this mutates, but it sets one comment to one body, so replaying it lands + the same result rather than a second comment. Returns: GitHubResponse[IssueComment]: The validated comment data and headers. @@ -522,11 +653,23 @@ async def update_issue_comment( GitHubBodyTooLongError: If `body` is too long, measured here or refused by GitHub. """ response = await self._comment_request( - "PATCH", f"/repos/{owner}/{repo}/issues/comments/{comment_id}", body=body, timeout=timeout + "PATCH", + f"/repos/{owner}/{repo}/issues/comments/{comment_id}", + body=body, + timeout=timeout, + retry=retry if retry is not None else self._retry_policies.safe, ) return self._parse_response(response, IssueComment) - async def _comment_request(self, method: str, endpoint: str, *, body: str, timeout: float | None) -> httpx.Response: + async def _comment_request( + self, + method: str, + endpoint: str, + *, + body: str, + timeout: float | None, + retry: RetryPolicy | None = None, + ) -> httpx.Response: """Send a comment *body*, enforcing the length limit from both sides. Scoped to the comment endpoints rather than `_request`, because a 422 elsewhere has nothing to do @@ -534,7 +677,7 @@ async def _comment_request(self, method: str, endpoint: str, *, body: str, timeo """ _ensure_body_fits(body) try: - return await self._request(method, endpoint, timeout=timeout, json={"body": body}) + return await self._request(method, endpoint, timeout=timeout, retry=retry, json={"body": body}) except httpx.HTTPStatusError as exc: if (message := github_body_too_long_message(exc.response)) is not None: raise GitHubBodyTooLongError.from_response(message, limit=COMMENT_BODY_LIMIT) from exc @@ -547,6 +690,8 @@ async def list_issue_comments( issue_number: int, per_page: int = 100, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> AsyncIterator[GitHubResponse[list[IssueComment]]]: """ Calls the GitHub API to list comments on an issue or pull request (paginated). @@ -560,6 +705,7 @@ async def list_issue_comments( issue_number: Issue or pull request number. per_page: Number of comments per page (default 100, GitHub's maximum). timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for each page. Defaults to the client's policy for replayable requests. Returns: AsyncIterator[GitHubResponse[list[IssueComment]]]: One page of comments per iteration, @@ -568,7 +714,9 @@ async def list_issue_comments( # The response body is a bare JSON array, so there is no wrapper model to validate against # (unlike ``WorkflowJobsList``); each item is validated individually. endpoint = f"/repos/{owner}/{repo}/issues/{issue_number}/comments" - async for response in self._paginated_request("GET", endpoint, timeout=timeout, params={"per_page": per_page}): + async for response in self._paginated_request( + "GET", endpoint, timeout=timeout, retry=retry, params={"per_page": per_page} + ): comments = [IssueComment.model_validate(item) for item in response.json()] yield GitHubResponse[list[IssueComment]].model_validate( {"data": comments, "headers": dict(response.headers)} @@ -580,6 +728,8 @@ async def get_pull_request( repo: str, pull_number: int, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[PullRequest]: """ Calls the GitHub API to get a single pull request. @@ -592,11 +742,15 @@ async def get_pull_request( repo: Repository name. pull_number: Pull request number. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's policy for replayable + requests, which does not retry the 404 that means "no such pull request". Returns: GitHubResponse[PullRequest]: The validated pull request data and headers. """ - response = await self._request("GET", f"/repos/{owner}/{repo}/pulls/{pull_number}", timeout=timeout) + response = await self._request( + "GET", f"/repos/{owner}/{repo}/pulls/{pull_number}", timeout=timeout, retry=retry + ) return self._parse_response(response, PullRequest) async def list_pull_requests( @@ -608,6 +762,8 @@ async def list_pull_requests( base: str | None = None, per_page: int = 100, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[list[PullRequest]]: """ Calls the GitHub API to list pull requests in a repository. @@ -625,6 +781,7 @@ async def list_pull_requests( base: Filter by base branch name. per_page: Number of results per page (max 100). Only the first page is fetched. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's policy for replayable requests. Returns: GitHubResponse[list[PullRequest]]: The validated pull requests on the first result page. @@ -634,7 +791,9 @@ async def list_pull_requests( params["head"] = head if base is not None: params["base"] = base - response = await self._request("GET", f"/repos/{owner}/{repo}/pulls", timeout=timeout, params=params) + response = await self._request( + "GET", f"/repos/{owner}/{repo}/pulls", timeout=timeout, retry=retry, params=params + ) pulls = [PullRequest.model_validate(item) for item in response.json()] return GitHubResponse[list[PullRequest]].model_validate({"data": pulls, "headers": dict(response.headers)}) @@ -648,6 +807,8 @@ async def create_pull_request( body: str = "", draft: bool = False, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[PullRequest]: """ Calls the GitHub API to create a pull request. @@ -664,6 +825,8 @@ async def create_pull_request( body: Pull request body. draft: Whether to open the pull request as a draft. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's mutating policy, since a + replayed create opens a second pull request. Returns: GitHubResponse[PullRequest]: The validated pull request data and headers. @@ -672,6 +835,7 @@ async def create_pull_request( "POST", f"/repos/{owner}/{repo}/pulls", timeout=timeout, + retry=retry, json={"title": title, "head": head, "base": base, "body": body, "draft": draft}, ) return self._parse_response(response, PullRequest) @@ -683,6 +847,8 @@ async def add_labels_to_issue( issue_number: int, labels: list[str], timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[list[Label]]: """ Calls the GitHub API to add one or more labels to an issue or pull request. @@ -696,6 +862,9 @@ async def add_labels_to_issue( issue_number: Issue or pull request number. labels: Labels to add. Existing labels on the issue are preserved. timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's policy for replayable + requests: this mutates, but adding a label the issue already carries is a no-op, so + replaying it cannot compound. Returns: GitHubResponse[list[Label]]: The full label list resulting from the operation (preserves @@ -705,6 +874,7 @@ async def add_labels_to_issue( "POST", f"/repos/{owner}/{repo}/issues/{issue_number}/labels", timeout=timeout, + retry=retry if retry is not None else self._retry_policies.safe, json={"labels": labels}, ) labels_out = [Label.model_validate(item) for item in response.json()] @@ -722,6 +892,8 @@ async def create_pr_review_comment( line: int | None = None, side: Literal["LEFT", "RIGHT"] | None = None, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[PullRequestReviewComment]: """ Calls the GitHub API to create an inline review comment on a pull request diff. @@ -740,6 +912,8 @@ async def create_pr_review_comment( line: Line number in the file (newer style, paired with side). side: 'LEFT' or 'RIGHT' (newer style, paired with line). timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's mutating policy, since a + replayed create leaves a second review comment on the diff. Returns: GitHubResponse[PullRequestReviewComment]: The validated comment data and headers. @@ -759,6 +933,7 @@ async def create_pr_review_comment( "POST", f"/repos/{owner}/{repo}/pulls/{pull_number}/comments", timeout=timeout, + retry=retry, json=payload, ) return self._parse_response(response, PullRequestReviewComment) @@ -773,6 +948,8 @@ async def create_check_run( details_url: str | None = None, output: dict[str, Any] | None = None, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[CheckRun]: """ Calls the GitHub API to create a check run on a commit. @@ -789,6 +966,8 @@ async def create_check_run( details_url: Optional URL the check title links to. output: Optional structured output (title, summary, ...). timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's mutating policy, since a + replayed create leaves a second check run on the commit. Returns: GitHubResponse[CheckRun]: The validated check run data and headers. @@ -802,6 +981,7 @@ async def create_check_run( "POST", f"/repos/{owner}/{repo}/check-runs", timeout=timeout, + retry=retry, json=payload, ) return self._parse_response(response, CheckRun) @@ -816,6 +996,8 @@ async def update_check_run( details_url: str | None = None, output: dict[str, Any] | None = None, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> GitHubResponse[CheckRun]: """ Calls the GitHub API to update an existing check run. @@ -832,6 +1014,9 @@ async def update_check_run( details_url: Optional URL the check title links to. output: Optional structured output (title, summary, ...). timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + retry: Retry strategy for this call. Defaults to the client's policy for replayable + requests: this mutates, but it sets the given fields to the given values, so replaying + it lands the same check run rather than another one. Returns: GitHubResponse[CheckRun]: The validated check run data and headers. @@ -851,6 +1036,7 @@ async def update_check_run( "PATCH", f"/repos/{owner}/{repo}/check-runs/{check_run_id}", timeout=timeout, + retry=retry if retry is not None else self._retry_policies.safe, json=payload, ) return self._parse_response(response, CheckRun) @@ -859,23 +1045,23 @@ async def _resolve_artifact_redirect( self, archive_download_url: str, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> str: - """Authenticated GET; return the unauthenticated signed URL from the 302 Location header.""" - try: - redirect_response = await self._request( - "GET", archive_download_url, timeout=timeout, follow_redirects=False - ) - except GitHubAuthenticationError: - raise - except httpx.HTTPStatusError as exc: - # httpx.raise_for_status() treats the expected 302 as an error since it isn't a 2xx; - # recover the response from the exception so the redirect can still be inspected below. - # The retry layer in _request is deliberately transparent here: a 302 is not a rate-limit - # response by _is_rate_limit_response, so it is never retried and surfaces on the first - # attempt exactly as before. - redirect_response = exc.response + """Authenticated GET; return the unauthenticated signed URL from the 302 Location header. + + The one endpoint whose contract is a redirect, so it asks for the response instead of having + it reported as an unexpected one. + """ + redirect_response = await self._request( + "GET", + archive_download_url, + timeout=timeout, + retry=retry, + expect_redirect=True, + follow_redirects=False, + ) if redirect_response.status_code != 302: - redirect_response.raise_for_status() raise httpx.HTTPError( f"Expected 302 redirect from {archive_download_url}, got {redirect_response.status_code}" ) @@ -913,6 +1099,8 @@ async def download_artifact( archive_download_url: str, dest_path: Path, timeout: float | None = None, + *, + retry: RetryPolicy | None = None, ) -> None: """ Downloads and extracts a workflow run artifact zip into ``dest_path``. @@ -927,16 +1115,26 @@ async def download_artifact( bearer token is not leaked to the redirect target. Each zip member is validated against ``dest_path`` before extraction (zip-slip protection). - This performs a single attempt with no retries; any failure propagates to the - caller. + Both requests are retried as one unit, so a retry resolves a fresh signed URL rather than + refetching one that may have expired in the meantime. Nothing is written until the whole zip + is in memory, so a retry cannot leave a half-extracted directory behind. Args: archive_download_url: The artifact's ``archive_download_url`` (absolute or relative to the API base). dest_path: Directory where the zip contents will be extracted. Created if missing. timeout: Optional timeout for both HTTP requests. + retry: Retry strategy for the pair. Defaults to the client's policy for replayable + requests, plus the 403 an expired signed URL produces. """ - location = await self._resolve_artifact_redirect(archive_download_url, timeout) - await self._download_and_extract_zip(location, dest_path, timeout) + policy = retry if retry is not None else self._artifact_retry + tracker = RetryTracker(policy, self._refuses_retry) + async for attempt in retry_attempts(policy, tracker): + with attempt: + self._log_retry(f"artifact download {archive_download_url}", tracker, attempt) + # NO_RETRY on the inner call: this loop is the only ladder, or the two would multiply. + location = await self._resolve_artifact_redirect(archive_download_url, timeout, retry=NO_RETRY) + await self._download_and_extract_zip(location, dest_path, timeout) + return # --------------------------------------------------------------------------- @@ -951,16 +1149,17 @@ async def async_github_client( rate_limiter: InstrumentedAsyncLimiter | None = None, default_timeout: float = 30.0, max_rate_limit_retries: int = 2, + retry_policies: RetryPolicies | None = None, + logger: logging.Logger | None = None, transport: httpx.AsyncBaseTransport | None = None, ) -> AsyncIterator[AsyncGitHubClient]: """ Async context manager that creates an AsyncGitHubClient and ensures it is closed on exit. Rate-limit protection is on by default; the governor paces requests and supplies the backoff for - retries. Header-confirmed rate-limit responses (403/429) are retried, transport errors and - non-rate-limit statuses are not, and RateLimitWaitAbandoned propagates to the caller when the - governor is configured with a wait budget. The default protection logs through the - ``ddev.utils.github_async.defaults`` logger. + those retries. Header-confirmed rate-limit responses (403/429) are retried there, and + RateLimitWaitAbandoned propagates to the caller when the governor is configured with a wait + budget. Other failures are handled by the retry strategy in ``retry.py``. Args: token: GitHub personal access token or app token. @@ -970,6 +1169,8 @@ async def async_github_client( default_timeout: Default per-request HTTP timeout in seconds. Bounds individual HTTP requests only, not governor waits. max_rate_limit_retries: Extra attempts for a header-confirmed rate-limit response. + retry_policies: Overrides the per-endpoint defaults for failures that are not rate limiting. + logger: Where retries are reported; None keeps the client silent. transport: Optional custom HTTPX transport (useful for testing with MockTransport). Yields: @@ -980,6 +1181,8 @@ async def async_github_client( rate_limiter=rate_limiter, default_timeout=default_timeout, max_rate_limit_retries=max_rate_limit_retries, + retry_policies=retry_policies, + logger=logger, transport=transport, ) try: diff --git a/ddev/src/ddev/utils/github_async/retry.py b/ddev/src/ddev/utils/github_async/retry.py new file mode 100644 index 0000000000000..570946bebea94 --- /dev/null +++ b/ddev/src/ddev/utils/github_async/retry.py @@ -0,0 +1,219 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Retry strategy for the async GitHub client. + +Separate from rate-limit handling, which lives in ``ddev.utils.rate_limiting`` and reacts to GitHub +telling us to slow down: there the backoff *is* the limiter, so re-acquiring it is the whole retry. +This layer covers the failures that carry no such instruction, a dropped connection or a 502, where +the only useful response is to wait a little and ask again. + +A ``RetryPolicy`` only describes what to do. stamina executes it, so no backoff arithmetic, sleeping +or attempt counting lives here. +""" + +from __future__ import annotations + +import dataclasses +from dataclasses import dataclass +from typing import TYPE_CHECKING, Any + +import httpx +import stamina + +from ddev.utils.github_errors import GitHubAuthenticationError +from ddev.utils.rate_limiting import RateLimitWaitAbandoned + +if TYPE_CHECKING: + from collections.abc import AsyncIterator, Callable + +type RetryPredicate = Callable[[Exception], bool] + +# Transport failures raised before any byte of the request reached GitHub, so replaying them cannot +# repeat a side effect. Read and write failures are deliberately absent: once the request is on the +# wire we cannot know whether the server acted on it. +PRE_SEND_TRANSPORT_ERRORS = (httpx.ConnectError, httpx.ConnectTimeout, httpx.PoolTimeout) + +# Server-side failures an identical later request may well survive. +RETRYABLE_SERVER_STATUSES = frozenset((500, 502, 503, 504)) + +# Verbs whose requests can be replayed without changing anything server-side. +REPLAYABLE_HTTP_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) + + +def is_redirect_status(status_code: int) -> bool: + """Whether *status_code* is a redirect, Location header or not.""" + return 300 <= status_code < 400 + + +# --------------------------------------------------------------------------- +# Predicates +# --------------------------------------------------------------------------- + + +def never(exc: Exception) -> bool: + """Refuse everything. The condition of a policy that does not retry.""" + return False + + +def on_transport_error(exc: Exception) -> bool: + """Any transport-level failure, whether or not the request reached GitHub.""" + return isinstance(exc, httpx.TransportError) + + +def on_pre_send_transport_error(exc: Exception) -> bool: + """Only the transport failures that prove the request never left.""" + return isinstance(exc, PRE_SEND_TRANSPORT_ERRORS) + + +def on_status(*status_codes: int) -> RetryPredicate: + """Responses whose status is one of *status_codes*.""" + wanted = frozenset(status_codes) + + def matches(exc: Exception) -> bool: + return isinstance(exc, httpx.HTTPStatusError) and exc.response.status_code in wanted + + return matches + + +def any_of(*predicates: RetryPredicate) -> RetryPredicate: + """Accept what any of *predicates* accepts.""" + + def matches(exc: Exception) -> bool: + return any(predicate(exc) for predicate in predicates) + + return matches + + +on_server_error = on_status(*RETRYABLE_SERVER_STATUSES) + + +# --------------------------------------------------------------------------- +# Policies +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class RetryPolicy: + """How hard to try, and what to try again on, for one request. + + ``timeout`` bounds the whole ladder including backoff, not one request; ``None`` removes that + bound and leaves ``attempts`` as the only stop condition. + """ + + should_retry: RetryPredicate = never + attempts: int = 3 + timeout: float | None = 60.0 + wait_initial: float = 0.5 + wait_max: float = 10.0 + wait_jitter: float = 1.0 + + def __post_init__(self) -> None: + if self.attempts < 1: + raise ValueError(f"attempts must be at least 1, got {self.attempts}") + if self.timeout is not None and self.timeout <= 0: + # stamina reads timeout=0 as "no retries", which is a confusing way to spell attempts=1. + raise ValueError(f"timeout must be positive or None, got {self.timeout}") + + def replace(self, **overrides: Any) -> RetryPolicy: + """A copy with *overrides* applied.""" + return dataclasses.replace(self, **overrides) + + def also_on(self, predicate: RetryPredicate) -> RetryPolicy: + """A copy that retries what *predicate* accepts as well.""" + return dataclasses.replace(self, should_retry=any_of(self.should_retry, predicate)) + + def unless(self, predicate: RetryPredicate) -> RetryPolicy: + """A copy that refuses what *predicate* accepts, whatever this policy accepted before.""" + accepted = self.should_retry + + def narrowed(exc: Exception) -> bool: + return accepted(exc) and not predicate(exc) + + return dataclasses.replace(self, should_retry=narrowed) + + +NO_RETRY = RetryPolicy(attempts=1) +SAFE_RETRY = RetryPolicy(should_retry=any_of(on_transport_error, on_server_error)) +MUTATION_RETRY = RetryPolicy(should_retry=on_pre_send_transport_error, attempts=2) + + +@dataclass(frozen=True) +class RetryPolicies: + """The client's defaults, one per replay-safety class. + + A verb is the proxy the client uses to pick between them, but idempotence is the real question, + so an endpoint that is idempotent despite mutating (setting a comment body, closing a check run) + asks for ``safe`` explicitly. + """ + + safe: RetryPolicy = SAFE_RETRY + mutating: RetryPolicy = MUTATION_RETRY + + def for_method(self, method: str) -> RetryPolicy: + """The default for *method*, by whether the verb is replayable.""" + return self.safe if method.upper() in REPLAYABLE_HTTP_METHODS else self.mutating + + +DEFAULT_RETRY_POLICIES = RetryPolicies() + + +# --------------------------------------------------------------------------- +# Execution +# --------------------------------------------------------------------------- + + +def refuses_retry(is_rate_limit_response: Callable[[httpx.Response], bool]) -> RetryPredicate: + """Build the exclusions the client applies whatever policy a caller supplies. + + Retrying any of these is useless or harmful, so no policy may opt in: + + - authentication failures, which no amount of waiting fixes; + - rate-limit responses, owned by the limiter, whose pause is the correct backoff; + - ``RateLimitWaitAbandoned``, the caller's killswitch for that pause; + - redirects, which are an answer rather than a failure. + """ + + def refuses(exc: Exception) -> bool: + if isinstance(exc, (GitHubAuthenticationError, RateLimitWaitAbandoned)): + return True + if isinstance(exc, httpx.HTTPStatusError): + return is_rate_limit_response(exc.response) or is_redirect_status(exc.response.status_code) + return False + + return refuses + + +class RetryTracker: + """Decides retries for one operation and keeps the failure that caused the most recent one. + + stamina asks whether to retry where the exception is known, and the caller logs where the attempt + number is known. This carries the exception between the two. + """ + + def __init__(self, policy: RetryPolicy, refuses: RetryPredicate) -> None: + self._policy = policy + self._refuses = refuses + self.last_error: Exception | None = None + + def __call__(self, exc: Exception) -> bool: + if self._refuses(exc) or not self._policy.should_retry(exc): + return False + self.last_error = exc + return True + + +def retry_attempts(policy: RetryPolicy, should_retry: RetryPredicate) -> AsyncIterator[stamina.Attempt]: + """Yield one stamina attempt per try of *policy*, retrying what *should_retry* accepts. + + The caller runs its work inside ``with attempt:``; that context manager is what swallows a + retryable exception, waits the backoff and lets the loop turn again. + """ + return stamina.retry_context( + on=should_retry, + attempts=policy.attempts, + timeout=policy.timeout, + wait_initial=policy.wait_initial, + wait_max=policy.wait_max, + wait_jitter=policy.wait_jitter, + ) diff --git a/ddev/src/ddev/utils/github_errors.py b/ddev/src/ddev/utils/github_errors.py index 09e2c39390bd5..b48699f81977d 100644 --- a/ddev/src/ddev/utils/github_errors.py +++ b/ddev/src/ddev/utils/github_errors.py @@ -4,6 +4,7 @@ from __future__ import annotations import math +from typing import Self import httpx @@ -42,6 +43,28 @@ def github_secondary_rate_limit_wait(response: httpx.Response) -> float | None: return None +class GitHubUnexpectedRedirectError(httpx.HTTPStatusError): + """A GitHub endpoint answered with a redirect that is not part of its contract. + + The client never follows redirects, because the ``Authorization`` header would travel to whatever + host ``Location`` names. One endpoint, the artifact download, does expect a redirect and asks for + it; anywhere else a redirect means our assumption about the endpoint is wrong, so it is surfaced + rather than followed or retried. + """ + + @classmethod + def from_response(cls, method: str, endpoint: str, response: httpx.Response) -> Self: + """Build the error for an unexpected redirect returned by *method* *endpoint*.""" + location = response.headers.get('location') or '' + return cls( + f'{method} {endpoint} returned an unexpected redirect (HTTP {response.status_code}) to ' + f'{location}. This endpoint is not expected to redirect, so the client did not follow it ' + f'and the GitHub token was not sent to the target.', + request=response.request, + response=response, + ) + + class GitHubAuthenticationError(httpx.HTTPStatusError): """A GitHub HTTP failure caused by invalid authentication or insufficient permissions.""" diff --git a/ddev/tests/cli/ci/tests/test_dispatcher_config.py b/ddev/tests/cli/ci/tests/test_dispatcher_config.py index 5d32e5f65e7b9..e0e57d0743e0f 100644 --- a/ddev/tests/cli/ci/tests/test_dispatcher_config.py +++ b/ddev/tests/cli/ci/tests/test_dispatcher_config.py @@ -47,11 +47,19 @@ def test_from_repo_config_reads_full_dispatcher_table(repo_config: RepoConfigBui [dispatcher.github_rate_limits.slow] max_rate = 120 + + [dispatcher.github_retries.safe] + attempts = 5 + + [dispatcher.github_retries.mutating] + attempts = 1 """ ) result = DispatcherConfig.from_repo_config(config) + assert result.github_retries.safe.attempts == 5 + assert result.github_retries.mutating.attempts == 1 assert result.batching.max_jobs_per_batch == 120 assert result.batching.allow_integration_splitting is True assert result.global_timeout_seconds == 3600.0 diff --git a/ddev/tests/cli/ci/tests/test_github_retries.py b/ddev/tests/cli/ci/tests/test_github_retries.py new file mode 100644 index 0000000000000..5d1502efa1720 --- /dev/null +++ b/ddev/tests/cli/ci/tests/test_github_retries.py @@ -0,0 +1,40 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Tests for turning the Dispatcher's retry configuration into client policies.""" + +from __future__ import annotations + +import httpx + +from ddev.cli.ci.tests.github_retries import GitHubRetryConfig, RetryLimitsConfig + + +def test_configured_limits_reach_the_policies(): + """The config exists to tune the ladder, so its numbers have to arrive on the policy.""" + config = GitHubRetryConfig( + safe=RetryLimitsConfig(attempts=7, timeout_seconds=120.0, wait_max_seconds=30.0), + mutating=RetryLimitsConfig(attempts=1), + ) + + policies = config.to_policies() + + assert policies.safe.attempts == 7 + assert policies.safe.timeout == 120.0 + assert policies.safe.wait_max == 30.0 + assert policies.mutating.attempts == 1 + + +def test_configuring_the_limits_does_not_change_what_is_retried(): + """Widening the conditions from a config file would make a duplicate workflow run a setting. + + Only the ladder is configurable, so each tier has to keep the conditions it was built with: the + safe tier still replays a 502, and the mutating tier still refuses one. + """ + policies = GitHubRetryConfig(safe=RetryLimitsConfig(attempts=9)).to_policies() + request = httpx.Request("GET", "https://api.github.com/x") + server_error = httpx.HTTPStatusError("boom", request=request, response=httpx.Response(502, request=request)) + + assert policies.safe.should_retry(server_error) + assert not policies.mutating.should_retry(server_error) + assert policies.mutating.should_retry(httpx.ConnectError("refused")) diff --git a/ddev/tests/utils/github_async/conftest.py b/ddev/tests/utils/github_async/conftest.py new file mode 100644 index 0000000000000..7ecbe17c94e48 --- /dev/null +++ b/ddev/tests/utils/github_async/conftest.py @@ -0,0 +1,17 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +from __future__ import annotations + +import pytest + +from tests.helpers.clock import FakeClock, advance_clock_on_sleep + + +@pytest.fixture +def instant_backoff(monkeypatch: pytest.MonkeyPatch) -> None: + """Remove the wait between retries, so a test that exhausts a policy costs no wall-clock time. + + stamina sleeps through ``asyncio.sleep``, which this replaces with a fake clock advance. + """ + advance_clock_on_sleep(FakeClock(), monkeypatch) diff --git a/ddev/tests/utils/github_async/helpers.py b/ddev/tests/utils/github_async/helpers.py index bb6e6acea17f3..c6e54625de7cd 100644 --- a/ddev/tests/utils/github_async/helpers.py +++ b/ddev/tests/utils/github_async/helpers.py @@ -10,7 +10,7 @@ import io import zipfile from collections.abc import AsyncIterator, Awaitable, Callable -from typing import Any +from typing import Any, Literal import httpx from aiolimiter import AsyncLimiter @@ -109,6 +109,10 @@ class EndpointCase: id: str call: Callable[[AsyncGitHubClient], Awaitable[GitHubResponse[Any]]] ok_response: Callable[[], httpx.Response] + # Which retry default the endpoint is expected to use. Stated per endpoint rather than derived + # from the verb, because whether a request can be safely replayed is the whole question, and + # three of the mutating endpoints can be. + default_retry: Literal["safe", "mutating"] ENDPOINT_CALLS = [ @@ -118,68 +122,84 @@ class EndpointCase: lambda: json_response( {"workflow_run_id": 1, "run_url": "https://api.github.com/x", "html_url": "https://github.com/x"} ), + default_retry="mutating", ), EndpointCase( - "get_workflow_run", lambda c: c.get_workflow_run("o", "r", 42), lambda: json_response(workflow_run_payload()) + "get_workflow_run", + lambda c: c.get_workflow_run("o", "r", 42), + lambda: json_response(workflow_run_payload()), + default_retry="safe", ), EndpointCase( "list_workflow_run_artifacts", lambda c: first_page(c.list_workflow_run_artifacts("o", "r", 1)), lambda: json_response({"total_count": 1, "artifacts": [artifact(1)]}), + default_retry="safe", ), EndpointCase( "list_workflow_jobs", lambda c: first_page(c.list_workflow_jobs("o", "r", 42)), lambda: json_response({"total_count": 1, "jobs": [workflow_job(1)]}), + default_retry="safe", ), EndpointCase( "create_issue_comment", lambda c: c.create_issue_comment("o", "r", 1, "body"), lambda: json_response(issue_comment_payload()), + default_retry="mutating", ), EndpointCase( "update_issue_comment", lambda c: c.update_issue_comment("o", "r", 1, "body"), lambda: json_response(issue_comment_payload()), + default_retry="safe", ), EndpointCase( "list_issue_comments", lambda c: first_page(c.list_issue_comments("o", "r", 1)), lambda: json_response([issue_comment_payload()]), + default_retry="safe", ), EndpointCase( "get_pull_request", lambda c: c.get_pull_request("o", "r", 5), lambda: json_response(full_pull_request_payload(number=5)), + default_retry="safe", ), EndpointCase( "list_pull_requests", lambda c: c.list_pull_requests("o", "r"), lambda: json_response([pull_request_payload(number=1)]), + default_retry="safe", ), EndpointCase( "create_pull_request", lambda c: c.create_pull_request("o", "r", "t", "h", "b"), lambda: json_response(pull_request_payload(number=1), status_code=201), + default_retry="mutating", ), EndpointCase( "add_labels_to_issue", lambda c: c.add_labels_to_issue("o", "r", 1, ["bug"]), lambda: json_response([{"id": 1, "name": "bug"}]), + default_retry="safe", ), EndpointCase( "create_pr_review_comment", lambda c: c.create_pr_review_comment("o", "r", 1, "body", "sha", "path", position=1), lambda: json_response(pr_review_comment_payload()), + default_retry="mutating", ), EndpointCase( "create_check_run", lambda c: c.create_check_run("o", "r", "ck", "abc", "in_progress"), lambda: json_response(check_run_payload()), + default_retry="mutating", ), EndpointCase( "update_check_run", lambda c: c.update_check_run("o", "r", 77, status="in_progress"), lambda: json_response(check_run_payload(id=77)), + default_retry="safe", ), ] diff --git a/ddev/tests/utils/github_async/test_download_artifact.py b/ddev/tests/utils/github_async/test_download_artifact.py index c190ba1f7ddb0..014126c837622 100644 --- a/ddev/tests/utils/github_async/test_download_artifact.py +++ b/ddev/tests/utils/github_async/test_download_artifact.py @@ -9,6 +9,8 @@ from ddev.utils.github_errors import GitHubAuthenticationError from tests.utils.github_async.helpers import TOKEN, make_client, make_zip, patch_signed_download +pytestmark = pytest.mark.usefixtures("instant_backoff") + async def test_download_artifact_token_not_leaked_to_redirect_target(monkeypatch, tmp_path) -> None: captured_signed_headers: dict[str, str] = {} @@ -50,7 +52,7 @@ async def test_download_artifact_authentication_error_remains_actionable(tmp_pat async def test_download_artifact_signed_url_error_propagates( monkeypatch: pytest.MonkeyPatch, tmp_path, status_code: int ) -> None: - """A failed signed-URL download propagates as httpx.HTTPStatusError (no retries).""" + """A signed-URL download that keeps failing reaches the caller once the retries are spent.""" def github_handler(request: httpx.Request) -> httpx.Response: return httpx.Response(302, headers={"location": "https://signed.example/zip"}) @@ -96,3 +98,55 @@ def signed_handler(request: httpx.Request) -> httpx.Response: # Nothing was extracted before the guard fired. assert list(dest.rglob("*")) == [] + + +async def test_an_expired_signed_url_is_resolved_again_rather_than_refetched(monkeypatch, tmp_path) -> None: + """The signed URL is short-lived, so the retry has to start from the redirect. + + An expired URL comes back from the storage host as a 403. Retrying only the download would + refetch the same dead URL and fail identically, so the pair is retried together and the second + attempt asks GitHub for a fresh one. + """ + signed_urls = ["https://signed.example/expired", "https://signed.example/fresh"] + github_calls: list[httpx.Request] = [] + signed_calls: list[str] = [] + + def github_handler(request: httpx.Request) -> httpx.Response: + github_calls.append(request) + return httpx.Response(302, headers={"location": signed_urls[min(len(github_calls) - 1, 1)]}) + + def signed_handler(request: httpx.Request) -> httpx.Response: + url = str(request.url) + signed_calls.append(url) + if url.endswith("/expired"): + return httpx.Response(403, content=b"AccessDenied") + return httpx.Response(200, content=make_zip({"hello.txt": b"hi"})) + + patch_signed_download(monkeypatch, signed_handler) + client = AsyncGitHubClient(token=TOKEN, transport=httpx.MockTransport(github_handler)) + + await client.download_artifact("/repos/o/r/actions/artifacts/1/zip", tmp_path / "out") + + assert len(github_calls) == 2 + assert signed_calls == ["https://signed.example/expired", "https://signed.example/fresh"] + assert (tmp_path / "out" / "hello.txt").read_bytes() == b"hi" + + +async def test_a_denial_from_github_itself_is_not_retried_as_an_expired_url(tmp_path) -> None: + """The 403 the artifact policy retries is the storage host's, not GitHub's. + + GitHub answers a real permission problem with its own 403, which arrives as an authentication + error; retrying that would spend the whole ladder on a failure no wait can fix. + """ + calls: list[httpx.Request] = [] + + def github_handler(request: httpx.Request) -> httpx.Response: + calls.append(request) + return httpx.Response(403) + + client = AsyncGitHubClient(token=TOKEN, transport=httpx.MockTransport(github_handler)) + + with pytest.raises(GitHubAuthenticationError): + await client.download_artifact("/repos/o/r/actions/artifacts/1/zip", tmp_path / "out") + + assert len(calls) == 1 diff --git a/ddev/tests/utils/github_async/test_rate_limiting.py b/ddev/tests/utils/github_async/test_rate_limiting.py index ddffc121e16e5..b97710ea1f2d3 100644 --- a/ddev/tests/utils/github_async/test_rate_limiting.py +++ b/ddev/tests/utils/github_async/test_rate_limiting.py @@ -207,13 +207,17 @@ async def test_authentication_error_is_actionable_and_not_retried(status_code: i assert "ddev config set github.token" in str(exc_info.value) -async def test_no_retry_on_transport_error() -> None: - """A transport error is never retried (the action may have executed); it propagates immediately.""" +async def test_the_rate_limit_layer_does_not_retry_a_transport_error() -> None: + """A transport error is not a rate-limit signal, so this layer must leave it alone. + + Retrying belongs to the retry strategy, which decides by whether the request can be replayed; + treating one as a rate-limit event here would retry it for every endpoint, dispatches included. + """ transport, calls = recording_transport([httpx.ConnectError("boom")]) client = AsyncGitHubClient(token=TOKEN, transport=transport) with pytest.raises(httpx.ConnectError): - await client._request("GET", "/x") + await client._rate_limited_request("GET", "/x") assert len(calls) == 1 diff --git a/ddev/tests/utils/github_async/test_retry.py b/ddev/tests/utils/github_async/test_retry.py new file mode 100644 index 0000000000000..7f7f99f052791 --- /dev/null +++ b/ddev/tests/utils/github_async/test_retry.py @@ -0,0 +1,300 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Tests for retrying the failures that are not rate limiting. + +The question every test here answers is which failures a given endpoint replays, and the reason it +matters is that replaying the wrong one duplicates a side effect: a second workflow run, a second +comment. Rate-limit retries are a different layer and live in ``test_rate_limiting.py``. +""" + +from __future__ import annotations + +import logging + +import httpx +import pytest + +from ddev.utils.github_async import AsyncGitHubClient +from ddev.utils.github_async.retry import ( + MUTATION_RETRY, + NO_RETRY, + SAFE_RETRY, + RetryPolicies, + RetryPolicy, + never, + on_status, + on_transport_error, +) +from ddev.utils.github_errors import GitHubUnexpectedRedirectError +from tests.utils.github_async.helpers import ENDPOINT_CALLS, TOKEN, json_response, recording_transport +from tests.utils.github_async.payloads import ( + full_pull_request_payload, + issue_comment_payload, + workflow_job, + workflow_run_payload, +) + +pytestmark = pytest.mark.usefixtures("instant_backoff") + +# A policy that would retry anything, to prove the client's exclusions win regardless. +RETRY_EVERYTHING = RetryPolicy(should_retry=lambda exc: True, attempts=3) + + +def policy_for(kind: str) -> RetryPolicy: + return SAFE_RETRY if kind == "safe" else MUTATION_RETRY + + +# --------------------------------------------------------------------------- +# Per-endpoint defaults +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("case", ENDPOINT_CALLS, ids=lambda case: case.id) +async def test_a_server_error_is_replayed_only_where_replaying_is_safe(case) -> None: + """A 503 says nothing about whether the request landed, so only replayable endpoints try again. + + Catches the mistake that matters most here: widening the default of a create endpoint, where a + replay leaves a duplicate workflow run, comment or check run behind. + """ + transport, calls = recording_transport([httpx.Response(503), case.ok_response()]) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + if case.default_retry == "safe": + await case.call(client) + assert len(calls) == 2 + else: + with pytest.raises(httpx.HTTPStatusError): + await case.call(client) + assert len(calls) == 1 + + +@pytest.mark.parametrize("case", ENDPOINT_CALLS, ids=lambda case: case.id) +async def test_a_request_that_never_left_is_replayed_by_every_endpoint(case) -> None: + """A refused connection proves GitHub never saw the request, so even a create can safely repeat. + + Without this, a mutating endpoint would give up on a blip that cost it nothing. + """ + transport, calls = recording_transport([httpx.ConnectError("refused")]) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + with pytest.raises(httpx.ConnectError): + await case.call(client) + + assert len(calls) == policy_for(case.default_retry).attempts + + +@pytest.mark.parametrize( + "error", + [ + pytest.param(httpx.ReadTimeout("timed out"), id="read_timeout"), + pytest.param(httpx.RemoteProtocolError("disconnected"), id="server_disconnected"), + ], +) +async def test_a_mutation_does_not_replay_a_request_that_may_have_landed(error: Exception) -> None: + """Once the request is on the wire, a failure cannot tell us whether GitHub acted on it. + + A dispatch replayed in that state starts a second batch of test jobs, so this is the case where + giving up is the cheaper mistake. + """ + transport, calls = recording_transport([error]) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + with pytest.raises(type(error)): + await client.create_workflow_dispatch("o", "r", "wf.yml", "main") + + assert len(calls) == 1 + + +# --------------------------------------------------------------------------- +# Caller overrides +# --------------------------------------------------------------------------- + + +async def test_a_caller_can_turn_retrying_off_for_one_call() -> None: + transport, calls = recording_transport([httpx.Response(503), json_response(workflow_run_payload())]) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + with pytest.raises(httpx.HTTPStatusError): + await client.get_workflow_run("o", "r", 42, retry=NO_RETRY) + + assert len(calls) == 1 + + +async def test_a_caller_can_widen_a_mutation_that_it_knows_is_safe_to_repeat() -> None: + """The default is deliberately cautious, so a caller that can absorb a duplicate may opt in.""" + transport, calls = recording_transport([httpx.Response(503), json_response(issue_comment_payload())]) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + await client.create_issue_comment("o", "r", 1, "body", retry=SAFE_RETRY) + + assert len(calls) == 2 + + +async def test_the_client_defaults_can_be_replaced_wholesale() -> None: + """Dispatcher builds these from config, so the constructor argument has to reach the requests.""" + transport, calls = recording_transport([httpx.ConnectError("refused")]) + policies = RetryPolicies(safe=SAFE_RETRY.replace(attempts=5), mutating=MUTATION_RETRY) + client = AsyncGitHubClient(token=TOKEN, transport=transport, retry_policies=policies) + + with pytest.raises(httpx.ConnectError): + await client.get_workflow_run("o", "r", 42) + + assert len(calls) == 5 + + +# --------------------------------------------------------------------------- +# What no policy may retry +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "response", + [ + pytest.param(httpx.Response(401), id="unauthenticated"), + pytest.param(httpx.Response(403), id="permission_denied"), + pytest.param(httpx.Response(403, headers={"retry-after": "5", "x-ratelimit-remaining": "0"}), id="rate_limit"), + pytest.param(httpx.Response(302, headers={"location": "https://elsewhere.example"}), id="redirect"), + ], +) +async def test_the_client_refuses_to_replay_what_replaying_cannot_fix(response: httpx.Response) -> None: + """Even asked to retry everything, these stay single attempts. + + Credentials do not improve by asking again, a rate-limit response belongs to the limiter whose + pause is the correct wait, and a redirect is an answer rather than a failure. A policy that could + opt into these would turn each one into a slower version of the same outcome. + """ + transport, calls = recording_transport([response]) + client = AsyncGitHubClient(token=TOKEN, transport=transport, max_rate_limit_retries=0) + + with pytest.raises(httpx.HTTPStatusError): + await client.get_workflow_run("o", "r", 42, retry=RETRY_EVERYTHING) + + assert len(calls) == 1 + + +async def test_a_missing_resource_is_an_answer_rather_than_a_failure_to_retry() -> None: + """``get_pull_request`` returning 404 means there is no pull request for that number. + + Dispatcher relies on that answer to fall through to commit resolution, so retrying it would only + delay a decision GitHub has already given. It stays out of the defaults rather than out of every + policy, since a caller waiting for a freshly created resource to appear may opt in. + """ + transport, calls = recording_transport( + [httpx.Response(404), httpx.Response(404), json_response(full_pull_request_payload(number=5))] + ) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + with pytest.raises(httpx.HTTPStatusError): + await client.get_pull_request("o", "r", 5) + + assert len(calls) == 1 + + # The same 404, with a caller that is waiting for the resource to appear: retried, then found. + await client.get_pull_request("o", "r", 5, retry=SAFE_RETRY.also_on(on_status(404))) + assert len(calls) == 3 + + +# --------------------------------------------------------------------------- +# Redirects +# --------------------------------------------------------------------------- + + +async def test_an_unexpected_redirect_names_the_endpoint_and_is_not_followed() -> None: + """Following a redirect would send the token to whoever the Location names. + + The client never follows one, so the risk is a caller misreading the generic HTTP error that used + to surface and "fixing" it by enabling redirects. + """ + transport, calls = recording_transport([httpx.Response(302, headers={"location": "https://evil.example/steal"})]) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + with pytest.raises(GitHubUnexpectedRedirectError) as exc_info: + await client.get_workflow_run("o", "r", 42) + + message = str(exc_info.value) + assert "/repos/o/r/actions/runs/42" in message + assert "https://evil.example/steal" in message + assert len(calls) == 1 + assert calls[0].url.host == "api.github.com" + + +# --------------------------------------------------------------------------- +# Pagination +# --------------------------------------------------------------------------- + + +async def test_a_failing_page_is_retried_without_refetching_the_pages_already_read() -> None: + """Pagination is a sequence of requests, so a blip on page two must not restart page one.""" + page_one = json_response( + {"total_count": 2, "jobs": [workflow_job(1)]}, + headers={"link": '; rel="next"'}, + ) + page_two = json_response({"total_count": 2, "jobs": [workflow_job(2)]}) + transport, calls = recording_transport([page_one, httpx.Response(503), page_two]) + client = AsyncGitHubClient(token=TOKEN, transport=transport) + + pages = [page async for page in client.list_workflow_jobs("o", "r", 42)] + + assert len(calls) == 3 + assert [job.id for page in pages for job in page.data.jobs] == [1, 2] + + +# --------------------------------------------------------------------------- +# Reporting +# --------------------------------------------------------------------------- + + +async def test_a_retry_is_reported_with_its_cause(caplog: pytest.LogCaptureFixture) -> None: + """A silent retry hides a degraded GitHub behind a slow run, so the cause has to reach the log.""" + transport, _ = recording_transport([httpx.Response(503), json_response(workflow_run_payload())]) + logger = logging.getLogger("test-github-client") + client = AsyncGitHubClient(token=TOKEN, transport=transport, logger=logger) + + with caplog.at_level(logging.WARNING, logger="test-github-client"): + await client.get_workflow_run("o", "r", 42) + + records = [record for record in caplog.records if record.name == "test-github-client"] + assert len(records) == 1 + assert "/repos/o/r/actions/runs/42" in records[0].getMessage() + assert "503" in records[0].getMessage() + assert records[0].attempt == 2 + + +# --------------------------------------------------------------------------- +# Composing policies +# --------------------------------------------------------------------------- + + +def test_also_on_keeps_what_the_policy_already_retried() -> None: + """The point of composing is starting from a default, so widening must not drop its conditions.""" + policy = SAFE_RETRY.also_on(on_status(404)) + + assert policy.should_retry(_status_error(404)) + assert policy.should_retry(_status_error(503)) + assert policy.should_retry(httpx.ConnectError("refused")) + + +def test_unless_removes_a_condition_the_policy_would_otherwise_retry() -> None: + policy = SAFE_RETRY.unless(on_status(503)) + + assert not policy.should_retry(_status_error(503)) + assert policy.should_retry(_status_error(502)) + + +def test_a_policy_that_cannot_retry_is_rejected_rather_than_silently_useless() -> None: + """``attempts=0`` would make every request fail without being sent.""" + with pytest.raises(ValueError, match="attempts must be at least 1"): + RetryPolicy(should_retry=on_transport_error, attempts=0) + + +def test_no_retry_refuses_everything() -> None: + assert NO_RETRY.should_retry is never + assert NO_RETRY.attempts == 1 + + +def _status_error(status_code: int) -> httpx.HTTPStatusError: + request = httpx.Request("GET", "https://api.github.com/x") + return httpx.HTTPStatusError( + f"HTTP {status_code}", request=request, response=httpx.Response(status_code, request=request) + ) From c86bea94d0c762abb97961a3cf29fe0360ef44f1 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 14:09:36 +0200 Subject: [PATCH 2/8] Add changelog entry --- ddev/changelog.d/24963.added | 1 + 1 file changed, 1 insertion(+) create mode 100644 ddev/changelog.d/24963.added diff --git a/ddev/changelog.d/24963.added b/ddev/changelog.d/24963.added new file mode 100644 index 0000000000000..6a5ed6b2a7567 --- /dev/null +++ b/ddev/changelog.d/24963.added @@ -0,0 +1 @@ +Add a retry strategy to the async GitHub client for failures that are not rate limiting. From f3b5071d144b271d4b1fc192df197d33e5d3a460 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 15:55:54 +0200 Subject: [PATCH 3/8] Address review feedback on the retry strategy - 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. --- .../ddev/cli/ci/tests/dispatcher_config.py | 45 +++- ddev/src/ddev/cli/ci/tests/github_retries.py | 56 ----- ddev/src/ddev/utils/github_async/client.py | 122 +++++++---- ddev/src/ddev/utils/github_async/retry.py | 197 ++++++++---------- ddev/src/ddev/utils/github_errors.py | 4 +- .../cli/ci/tests/test_dispatcher_config.py | 29 ++- .../tests/cli/ci/tests/test_github_retries.py | 40 ---- ddev/tests/utils/github_async/conftest.py | 2 +- ddev/tests/utils/github_async/test_retry.py | 31 +-- 9 files changed, 266 insertions(+), 260 deletions(-) delete mode 100644 ddev/src/ddev/cli/ci/tests/github_retries.py delete mode 100644 ddev/tests/cli/ci/tests/test_github_retries.py diff --git a/ddev/src/ddev/cli/ci/tests/dispatcher_config.py b/ddev/src/ddev/cli/ci/tests/dispatcher_config.py index 9e8f8234d07df..175956bf6d102 100644 --- a/ddev/src/ddev/cli/ci/tests/dispatcher_config.py +++ b/ddev/src/ddev/cli/ci/tests/dispatcher_config.py @@ -9,8 +9,8 @@ from pydantic import BaseModel, ConfigDict, Field -from ddev.cli.ci.tests.github_retries import GitHubRetryConfig from ddev.cli.ci.tests.rate_limiting import RateLimiterFactoryConfig +from ddev.utils.github_async.retry import MUTATION_RETRY, SAFE_RETRY, RetryPolicies, RetryPolicy if TYPE_CHECKING: from ddev.repo.config import RepositoryConfig @@ -27,6 +27,49 @@ class BatchingConfig(BaseModel): allow_integration_splitting: bool = False +class RetryLimitsConfig(BaseModel): + """Attempt and backoff limits for one class of GitHub request.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + attempts: int = Field(default=3, ge=1) + # Bounds the whole ladder including backoff. None leaves attempts as the only stop condition. + timeout_seconds: float | None = Field(default=60.0, gt=0) + wait_initial_seconds: float = Field(default=0.5, gt=0) + wait_max_seconds: float = Field(default=10.0, gt=0) + wait_jitter_seconds: float = Field(default=1.0, ge=0) + + def apply_to(self, policy: RetryPolicy) -> RetryPolicy: + """`policy` with these limits, keeping the conditions it retries on.""" + return policy.replace( + attempts=self.attempts, + timeout=self.timeout_seconds, + wait_initial=self.wait_initial_seconds, + wait_max=self.wait_max_seconds, + wait_jitter=self.wait_jitter_seconds, + ) + + +class GitHubRetryConfig(BaseModel): + """Retry limits for the GitHub client, read from `[dispatcher.github_retries]`. + + Only how hard to try is configurable. What may be retried follows from whether an endpoint can be + replayed, so widening it from a config file would make a duplicate side effect a setting. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + safe: RetryLimitsConfig = RetryLimitsConfig() + mutating: RetryLimitsConfig = RetryLimitsConfig(attempts=2) + + def to_policies(self) -> RetryPolicies: + """Build the client's policies: these limits over the built-in conditions.""" + return RetryPolicies( + safe=self.safe.apply_to(SAFE_RETRY), + mutating=self.mutating.apply_to(MUTATION_RETRY), + ) + + class DispatcherConfig(BaseModel): """Per-repository Dispatcher configuration.""" diff --git a/ddev/src/ddev/cli/ci/tests/github_retries.py b/ddev/src/ddev/cli/ci/tests/github_retries.py deleted file mode 100644 index f22c2def199d7..0000000000000 --- a/ddev/src/ddev/cli/ci/tests/github_retries.py +++ /dev/null @@ -1,56 +0,0 @@ -# (C) Datadog, Inc. 2026-present -# All rights reserved -# Licensed under a 3-clause BSD style license (see LICENSE) -"""Dispatcher-facing configuration for the GitHub client's retry strategy. - -Only *how hard* to try is configurable. *What* may be retried stays in -``ddev.utils.github_async.retry``, because it follows from whether an endpoint can be replayed -safely: a config file that could widen it would turn a duplicate workflow run into a setting. -""" - -from __future__ import annotations - -from pydantic import BaseModel, ConfigDict, Field - -from ddev.utils.github_async.retry import MUTATION_RETRY, SAFE_RETRY, RetryPolicies, RetryPolicy - - -class RetryLimitsConfig(BaseModel): - """Attempt and backoff limits for one class of request.""" - - model_config = ConfigDict(frozen=True, extra="forbid") - - attempts: int = Field(default=3, ge=1) - # Bounds the whole ladder including backoff. None leaves attempts as the only stop condition. - timeout_seconds: float | None = Field(default=60.0, gt=0) - wait_initial_seconds: float = Field(default=0.5, gt=0) - wait_max_seconds: float = Field(default=10.0, gt=0) - wait_jitter_seconds: float = Field(default=1.0, ge=0) - - def apply_to(self, policy: RetryPolicy) -> RetryPolicy: - """*policy* with these limits, keeping the conditions it retries on.""" - return policy.replace( - attempts=self.attempts, - timeout=self.timeout_seconds, - wait_initial=self.wait_initial_seconds, - wait_max=self.wait_max_seconds, - wait_jitter=self.wait_jitter_seconds, - ) - - -class GitHubRetryConfig(BaseModel): - """Retry limits for the Dispatcher's GitHub client, read from ``[dispatcher.github_retries]``.""" - - model_config = ConfigDict(frozen=True, extra="forbid") - - safe: RetryLimitsConfig = RetryLimitsConfig() - # Fewer attempts by default: a mutation only retries when the request provably never left, which - # a second attempt either fixes at once or is unlikely to fix at all. - mutating: RetryLimitsConfig = RetryLimitsConfig(attempts=2) - - def to_policies(self) -> RetryPolicies: - """Build the client's policies, these limits over the built-in conditions.""" - return RetryPolicies( - safe=self.safe.apply_to(SAFE_RETRY), - mutating=self.mutating.apply_to(MUTATION_RETRY), - ) diff --git a/ddev/src/ddev/utils/github_async/client.py b/ddev/src/ddev/utils/github_async/client.py index acc91527b65f7..68eca6c4c806a 100644 --- a/ddev/src/ddev/utils/github_async/client.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -27,7 +27,12 @@ github_body_too_long_message, github_secondary_rate_limit_wait, ) -from ddev.utils.rate_limiting import NULL_SNAPSHOT, BudgetSnapshot, InstrumentedAsyncLimiter, RateLimitEvent +from ddev.utils.rate_limiting import ( + NULL_SNAPSHOT, + BudgetSnapshot, + InstrumentedAsyncLimiter, + RateLimitWaitAbandoned, +) from .defaults import default_github_rate_limiter, log_rate_limit_events from .models import ( @@ -47,10 +52,8 @@ NO_RETRY, RetryPolicies, RetryPolicy, - RetryTracker, - is_redirect_status, + RetryPredicate, on_status, - refuses_retry, retry_attempts, ) @@ -62,6 +65,11 @@ # never below the character count GitHub means, so it errs only towards refusing a body it might take. COMMENT_BODY_LIMIT = 65_536 +# How an expired signed URL presents from the artifact storage host. +SIGNED_URL_EXPIRED_STATUS = 403 + +REDIRECT_STATUS_RANGE = range(300, 400) + _LINK_RE = re.compile(r'<([^>]+)>;\s*rel="([^"]+)"') @@ -125,6 +133,34 @@ def parse_header[T](headers: httpx.Headers, key: str, cast: Callable[[str], T]) return None +def is_redirect_status(status_code: int) -> bool: + """Whether `status_code` is a redirect, Location header or not.""" + return status_code in REDIRECT_STATUS_RANGE + + +def loggable_url(url: str) -> str: + """`url` without its query string, which is where a signed URL keeps its signature.""" + return url.split("?", 1)[0] + + +class RetryCause: + """Carries the failure from the predicate, which sees it, to the log line, which counts attempts. + + stamina exposes the attempt number but not what went wrong, and the predicate is the only place + the exception is available, so a retry cannot otherwise say why it happened. + """ + + def __init__(self, should_retry: RetryPredicate) -> None: + self._should_retry = should_retry + self.error: Exception | None = None + + def __call__(self, exc: Exception) -> bool: + if not self._should_retry(exc): + return False + self.error = exc + return True + + def github_rate_limit_snapshot(headers: httpx.Headers) -> BudgetSnapshot | None: """Parse GitHub's `x-ratelimit-*` / `retry-after` response headers into a BudgetSnapshot.""" snapshot = BudgetSnapshot( @@ -148,8 +184,8 @@ class AsyncGitHubClient: backoff, so there is no sleeping or backoff arithmetic in this client. Failures that are *not* rate limiting, a dropped connection or a 502, are handled separately by - the retry strategy in ``retry.py``. The two layers answer different questions, "GitHub told us to - wait" against "that request did not land, ask again", and each endpoint method takes a ``retry`` + the retry strategy in `retry.py`. The two layers answer different questions, "GitHub told us to + wait" against "that request did not land, ask again", and each endpoint method takes a `retry` argument to override its default for a single call. Args: @@ -169,17 +205,13 @@ class AsyncGitHubClient: Each retry is a full fresh acquisition (governor wait plus bucket token); the default of 2 covers the common "hit a secondary limit once, wait, succeed" case plus one repeat. Only rate-limit responses are retried here; every other failure belongs to the retry - strategy, and RateLimitWaitAbandoned (the governor's ``max_wait_seconds`` killswitch) + strategy, and RateLimitWaitAbandoned (the governor's `max_wait_seconds` killswitch) reaches the caller untouched by either layer. - retry_policies: Overrides the defaults for failures that are not rate limiting. None uses - ``DEFAULT_RETRY_POLICIES``. *What* may be retried is a correctness property of each - endpoint rather than a preference, so policies supplied here are expected to change how - hard to try, not to widen the conditions. - logger: Where this client reports its retries. None silences its own reporting; stamina's - built-in retry instrumentation is global and independent of this (see AGENTS.md). A logger - given here also receives the events of the default rate limiter, while a caller-supplied - ``rate_limiter`` keeps whatever logging it was built with, since that choice belongs to - whoever built it. + retry_policies: The retry strategies to use for failures that are not rate limiting, one per + replay-safety class. Defaults to `DEFAULT_RETRY_POLICIES`. + logger: Logger this client writes to. None keeps it silent. It also receives the events of the + default rate limiter, while a caller-supplied `rate_limiter` keeps whatever logging it was + built with, since that choice belongs to whoever built it. transport: Optional custom HTTPX transport (useful for testing with MockTransport). """ @@ -203,17 +235,16 @@ def __init__( # and no secondary limits the governor adds zero wait, so this default is invisible to # well-behaved callers and engages only once GitHub has already signaled backpressure. self._rate_limiter = ( - rate_limiter if rate_limiter is not None else default_github_rate_limiter(on_event=self._limiter_events()) + rate_limiter + if rate_limiter is not None + else default_github_rate_limiter(on_event=log_rate_limit_events(logger) if logger is not None else None) ) self._default_timeout = default_timeout self._max_rate_limit_retries = max_rate_limit_retries self._retry_policies = retry_policies if retry_policies is not None else DEFAULT_RETRY_POLICIES - self._refuses_retry = refuses_retry(self._is_rate_limit_response) - # An expired signed URL comes back from the storage host as a 403, and the cure is resolving - # the redirect again rather than refetching a dead URL, so the pair is retried on one. A 403 - # from GitHub itself arrives as GitHubAuthenticationError, which the guard refuses, so this - # cannot turn a real permission denial into a retry loop. - self._artifact_retry = self._retry_policies.safe.also_on(on_status(403)) + # A 403 from GitHub itself arrives as GitHubAuthenticationError, which the guard refuses, so + # adding this one cannot turn a permission denial into a retry loop. + self._artifact_retry = self._retry_policies.safe.also_on(on_status(SIGNED_URL_EXPIRED_STATUS)) self._headers = { "Authorization": f"Bearer {token}", "X-GitHub-Api-Version": GITHUB_API_VERSION, @@ -234,23 +265,40 @@ async def aclose(self) -> None: # Internal helpers # ------------------------------------------------------------------ - def _limiter_events(self) -> Callable[[RateLimitEvent], None] | None: - """Handler for the default rate limiter's events, routed to our logger when there is one.""" - return log_rate_limit_events(self._logger) if self._logger is not None else None - def _effective_timeout(self, timeout: float | None) -> float: return timeout if timeout is not None else self._default_timeout - def _log_retry(self, description: str, tracker: RetryTracker, attempt: stamina.Attempt) -> None: + def _retry_cause(self, policy: RetryPolicy) -> RetryCause: + """The predicate for one operation: what `policy` accepts, minus what this client refuses.""" + + def should_retry(exc: Exception) -> bool: + return not self._refuses_retry(exc) and policy.should_retry(exc) + + return RetryCause(should_retry) + + def _refuses_retry(self, exc: Exception) -> bool: + """Failures no policy may retry. + + Authentication does not improve by asking again; a rate-limit response belongs to the limiter, + whose pause is the correct wait, and `RateLimitWaitAbandoned` is the caller's killswitch for + it; a redirect is an answer rather than a failure. + """ + if isinstance(exc, (GitHubAuthenticationError, RateLimitWaitAbandoned)): + return True + if isinstance(exc, httpx.HTTPStatusError): + return self._is_rate_limit_response(exc.response) or is_redirect_status(exc.response.status_code) + return False + + def _log_retry(self, description: str, cause: RetryCause, attempt: stamina.Attempt) -> None: """Report a retry that is about to run. Silent on the first attempt, and with no logger.""" if attempt.num == 1 or self._logger is None: return self._logger.warning( "Retrying %s after %r (attempt %s)", description, - tracker.last_error, + cause.error, attempt.num, - extra={"attempt": attempt.num, "error": repr(tracker.last_error)}, + extra={"attempt": attempt.num, "error": repr(cause.error)}, ) @staticmethod @@ -361,10 +409,10 @@ async def _request( the acquisition and hammer GitHub through its own backoff. """ policy = retry if retry is not None else self._retry_policies.for_method(method) - tracker = RetryTracker(policy, self._refuses_retry) - async for attempt in retry_attempts(policy, tracker): + cause = self._retry_cause(policy) + async for attempt in retry_attempts(policy, cause): with attempt: - self._log_retry(f"{method} {endpoint}", tracker, attempt) + self._log_retry(f"{method} {endpoint}", cause, attempt) return await self._rate_limited_request( method, endpoint, timeout, expect_redirect=expect_redirect, **kwargs ) @@ -1127,10 +1175,10 @@ async def download_artifact( requests, plus the 403 an expired signed URL produces. """ policy = retry if retry is not None else self._artifact_retry - tracker = RetryTracker(policy, self._refuses_retry) - async for attempt in retry_attempts(policy, tracker): + cause = self._retry_cause(policy) + async for attempt in retry_attempts(policy, cause): with attempt: - self._log_retry(f"artifact download {archive_download_url}", tracker, attempt) + self._log_retry(f"artifact download {loggable_url(archive_download_url)}", cause, attempt) # NO_RETRY on the inner call: this loop is the only ladder, or the two would multiply. location = await self._resolve_artifact_redirect(archive_download_url, timeout, retry=NO_RETRY) await self._download_and_extract_zip(location, dest_path, timeout) @@ -1159,7 +1207,7 @@ async def async_github_client( Rate-limit protection is on by default; the governor paces requests and supplies the backoff for those retries. Header-confirmed rate-limit responses (403/429) are retried there, and RateLimitWaitAbandoned propagates to the caller when the governor is configured with a wait - budget. Other failures are handled by the retry strategy in ``retry.py``. + budget. Other failures are handled by the retry strategy in `retry.py`. Args: token: GitHub personal access token or app token. diff --git a/ddev/src/ddev/utils/github_async/retry.py b/ddev/src/ddev/utils/github_async/retry.py index 570946bebea94..335a0ca61e36a 100644 --- a/ddev/src/ddev/utils/github_async/retry.py +++ b/ddev/src/ddev/utils/github_async/retry.py @@ -1,49 +1,53 @@ # (C) Datadog, Inc. 2026-present # All rights reserved # Licensed under a 3-clause BSD style license (see LICENSE) -"""Retry strategy for the async GitHub client. +"""Retry strategy for requests that failed for a reason other than rate limiting. -Separate from rate-limit handling, which lives in ``ddev.utils.rate_limiting`` and reacts to GitHub -telling us to slow down: there the backoff *is* the limiter, so re-acquiring it is the whole retry. -This layer covers the failures that carry no such instruction, a dropped connection or a 502, where -the only useful response is to wait a little and ask again. - -A ``RetryPolicy`` only describes what to do. stamina executes it, so no backoff arithmetic, sleeping -or attempt counting lives here. +Rate limiting is handled elsewhere (`ddev.utils.rate_limiting`), where re-acquiring the limiter is +itself the backoff. This module covers the rest: a policy says what to retry and how hard to try, +and stamina executes it. """ from __future__ import annotations -import dataclasses from dataclasses import dataclass -from typing import TYPE_CHECKING, Any +from enum import Enum, auto +from typing import TYPE_CHECKING import httpx import stamina -from ddev.utils.github_errors import GitHubAuthenticationError -from ddev.utils.rate_limiting import RateLimitWaitAbandoned - if TYPE_CHECKING: from collections.abc import AsyncIterator, Callable type RetryPredicate = Callable[[Exception], bool] -# Transport failures raised before any byte of the request reached GitHub, so replaying them cannot -# repeat a side effect. Read and write failures are deliberately absent: once the request is on the -# wire we cannot know whether the server acted on it. +# Transport failures raised before any byte of the request reached the server, so a replay cannot +# repeat a side effect. Read and write failures are absent on purpose. PRE_SEND_TRANSPORT_ERRORS = (httpx.ConnectError, httpx.ConnectTimeout, httpx.PoolTimeout) -# Server-side failures an identical later request may well survive. RETRYABLE_SERVER_STATUSES = frozenset((500, 502, 503, 504)) -# Verbs whose requests can be replayed without changing anything server-side. REPLAYABLE_HTTP_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) +DEFAULT_ATTEMPTS = 3 +DEFAULT_TIMEOUT_SECONDS = 60.0 +DEFAULT_WAIT_INITIAL_SECONDS = 0.5 +DEFAULT_WAIT_MAX_SECONDS = 10.0 +DEFAULT_WAIT_JITTER_SECONDS = 1.0 + +# A mutation retries only what provably never left, which a second attempt either fixes at once or +# is unlikely to fix at all. +MUTATION_ATTEMPTS = 2 + -def is_redirect_status(status_code: int) -> bool: - """Whether *status_code* is a redirect, Location header or not.""" - return 300 <= status_code < 400 +class Unset(Enum): + """Sentinel for `RetryPolicy.replace`, where `timeout=None` means no timeout, not unchanged.""" + + TOKEN = auto() + + +UNSET = Unset.TOKEN # --------------------------------------------------------------------------- @@ -52,12 +56,12 @@ def is_redirect_status(status_code: int) -> bool: def never(exc: Exception) -> bool: - """Refuse everything. The condition of a policy that does not retry.""" + """Refuse everything.""" return False def on_transport_error(exc: Exception) -> bool: - """Any transport-level failure, whether or not the request reached GitHub.""" + """Any transport failure, whether or not the request reached the server.""" return isinstance(exc, httpx.TransportError) @@ -67,7 +71,7 @@ def on_pre_send_transport_error(exc: Exception) -> bool: def on_status(*status_codes: int) -> RetryPredicate: - """Responses whose status is one of *status_codes*.""" + """Responses whose status is one of `status_codes`.""" wanted = frozenset(status_codes) def matches(exc: Exception) -> bool: @@ -77,7 +81,7 @@ def matches(exc: Exception) -> bool: def any_of(*predicates: RetryPredicate) -> RetryPredicate: - """Accept what any of *predicates* accepts.""" + """Accept what any of `predicates` accepts.""" def matches(exc: Exception) -> bool: return any(predicate(exc) for predicate in predicates) @@ -85,129 +89,106 @@ def matches(exc: Exception) -> bool: return matches -on_server_error = on_status(*RETRYABLE_SERVER_STATUSES) - - # --------------------------------------------------------------------------- # Policies # --------------------------------------------------------------------------- -@dataclass(frozen=True) class RetryPolicy: - """How hard to try, and what to try again on, for one request. + """What to retry, and how hard to try. - ``timeout`` bounds the whole ladder including backoff, not one request; ``None`` removes that - bound and leaves ``attempts`` as the only stop condition. + Treat instances as values: `replace`, `also_on` and `unless` return new policies rather than + mutating this one, which matters because the defaults below are shared. """ - should_retry: RetryPredicate = never - attempts: int = 3 - timeout: float | None = 60.0 - wait_initial: float = 0.5 - wait_max: float = 10.0 - wait_jitter: float = 1.0 - - def __post_init__(self) -> None: - if self.attempts < 1: - raise ValueError(f"attempts must be at least 1, got {self.attempts}") - if self.timeout is not None and self.timeout <= 0: - # stamina reads timeout=0 as "no retries", which is a confusing way to spell attempts=1. - raise ValueError(f"timeout must be positive or None, got {self.timeout}") - - def replace(self, **overrides: Any) -> RetryPolicy: - """A copy with *overrides* applied.""" - return dataclasses.replace(self, **overrides) + __slots__ = ("attempts", "should_retry", "timeout", "wait_initial", "wait_jitter", "wait_max") + + def __init__( + self, + should_retry: RetryPredicate = never, + attempts: int = DEFAULT_ATTEMPTS, + timeout: float | None = DEFAULT_TIMEOUT_SECONDS, + wait_initial: float = DEFAULT_WAIT_INITIAL_SECONDS, + wait_max: float = DEFAULT_WAIT_MAX_SECONDS, + wait_jitter: float = DEFAULT_WAIT_JITTER_SECONDS, + ) -> None: + """`timeout` bounds the whole ladder including backoff; None leaves `attempts` as the only stop.""" + if attempts < 1: + raise ValueError(f"attempts must be at least 1, got {attempts}") + if timeout is not None and timeout <= 0: + # stamina reads timeout=0 as "no retries", a confusing way to spell attempts=1. + raise ValueError(f"timeout must be positive or None, got {timeout}") + self.should_retry = should_retry + self.attempts = attempts + self.timeout = timeout + self.wait_initial = wait_initial + self.wait_max = wait_max + self.wait_jitter = wait_jitter + + def replace( + self, + *, + should_retry: RetryPredicate | None = None, + attempts: int | None = None, + timeout: float | None | Unset = UNSET, + wait_initial: float | None = None, + wait_max: float | None = None, + wait_jitter: float | None = None, + ) -> RetryPolicy: + """A copy with the given fields changed.""" + return RetryPolicy( + should_retry=self.should_retry if should_retry is None else should_retry, + attempts=self.attempts if attempts is None else attempts, + timeout=self.timeout if isinstance(timeout, Unset) else timeout, + wait_initial=self.wait_initial if wait_initial is None else wait_initial, + wait_max=self.wait_max if wait_max is None else wait_max, + wait_jitter=self.wait_jitter if wait_jitter is None else wait_jitter, + ) def also_on(self, predicate: RetryPredicate) -> RetryPolicy: - """A copy that retries what *predicate* accepts as well.""" - return dataclasses.replace(self, should_retry=any_of(self.should_retry, predicate)) + """A copy that retries what `predicate` accepts as well.""" + return self.replace(should_retry=any_of(self.should_retry, predicate)) def unless(self, predicate: RetryPredicate) -> RetryPolicy: - """A copy that refuses what *predicate* accepts, whatever this policy accepted before.""" + """A copy that refuses what `predicate` accepts.""" accepted = self.should_retry def narrowed(exc: Exception) -> bool: return accepted(exc) and not predicate(exc) - return dataclasses.replace(self, should_retry=narrowed) + return self.replace(should_retry=narrowed) +# Policy constants. They follow the class because they are instances of it. NO_RETRY = RetryPolicy(attempts=1) -SAFE_RETRY = RetryPolicy(should_retry=any_of(on_transport_error, on_server_error)) -MUTATION_RETRY = RetryPolicy(should_retry=on_pre_send_transport_error, attempts=2) +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) @dataclass(frozen=True) class RetryPolicies: - """The client's defaults, one per replay-safety class. + """The defaults a client picks from, one per replay-safety class. - A verb is the proxy the client uses to pick between them, but idempotence is the real question, - so an endpoint that is idempotent despite mutating (setting a comment body, closing a check run) - asks for ``safe`` explicitly. + The verb is only a proxy: idempotence is the real question, so an endpoint that mutates but is + idempotent asks for `safe` explicitly. """ safe: RetryPolicy = SAFE_RETRY mutating: RetryPolicy = MUTATION_RETRY def for_method(self, method: str) -> RetryPolicy: - """The default for *method*, by whether the verb is replayable.""" + """The default for `method`, by whether the verb is replayable.""" return self.safe if method.upper() in REPLAYABLE_HTTP_METHODS else self.mutating DEFAULT_RETRY_POLICIES = RetryPolicies() -# --------------------------------------------------------------------------- -# Execution -# --------------------------------------------------------------------------- - - -def refuses_retry(is_rate_limit_response: Callable[[httpx.Response], bool]) -> RetryPredicate: - """Build the exclusions the client applies whatever policy a caller supplies. - - Retrying any of these is useless or harmful, so no policy may opt in: - - - authentication failures, which no amount of waiting fixes; - - rate-limit responses, owned by the limiter, whose pause is the correct backoff; - - ``RateLimitWaitAbandoned``, the caller's killswitch for that pause; - - redirects, which are an answer rather than a failure. - """ - - def refuses(exc: Exception) -> bool: - if isinstance(exc, (GitHubAuthenticationError, RateLimitWaitAbandoned)): - return True - if isinstance(exc, httpx.HTTPStatusError): - return is_rate_limit_response(exc.response) or is_redirect_status(exc.response.status_code) - return False - - return refuses - - -class RetryTracker: - """Decides retries for one operation and keeps the failure that caused the most recent one. - - stamina asks whether to retry where the exception is known, and the caller logs where the attempt - number is known. This carries the exception between the two. - """ - - def __init__(self, policy: RetryPolicy, refuses: RetryPredicate) -> None: - self._policy = policy - self._refuses = refuses - self.last_error: Exception | None = None - - def __call__(self, exc: Exception) -> bool: - if self._refuses(exc) or not self._policy.should_retry(exc): - return False - self.last_error = exc - return True - - def retry_attempts(policy: RetryPolicy, should_retry: RetryPredicate) -> AsyncIterator[stamina.Attempt]: - """Yield one stamina attempt per try of *policy*, retrying what *should_retry* accepts. + """Yield one attempt per try of `policy`, retrying what `should_retry` accepts. - The caller runs its work inside ``with attempt:``; that context manager is what swallows a - retryable exception, waits the backoff and lets the loop turn again. + The caller runs its work inside `with attempt:`, which is what swallows a retryable exception, + waits the backoff and lets the loop turn again. """ return stamina.retry_context( on=should_retry, diff --git a/ddev/src/ddev/utils/github_errors.py b/ddev/src/ddev/utils/github_errors.py index b48699f81977d..ba1e03659bb2e 100644 --- a/ddev/src/ddev/utils/github_errors.py +++ b/ddev/src/ddev/utils/github_errors.py @@ -46,8 +46,8 @@ def github_secondary_rate_limit_wait(response: httpx.Response) -> float | None: class GitHubUnexpectedRedirectError(httpx.HTTPStatusError): """A GitHub endpoint answered with a redirect that is not part of its contract. - The client never follows redirects, because the ``Authorization`` header would travel to whatever - host ``Location`` names. One endpoint, the artifact download, does expect a redirect and asks for + The client never follows redirects, because the `Authorization` header would travel to whatever + host `Location` names. One endpoint, the artifact download, does expect a redirect and asks for it; anywhere else a redirect means our assumption about the endpoint is wrong, so it is surfaced rather than followed or retried. """ diff --git a/ddev/tests/cli/ci/tests/test_dispatcher_config.py b/ddev/tests/cli/ci/tests/test_dispatcher_config.py index e0e57d0743e0f..57eb5ae4facc7 100644 --- a/ddev/tests/cli/ci/tests/test_dispatcher_config.py +++ b/ddev/tests/cli/ci/tests/test_dispatcher_config.py @@ -7,10 +7,16 @@ from collections.abc import Callable +import httpx import pytest from pydantic import ValidationError -from ddev.cli.ci.tests.dispatcher_config import BatchingConfig, DispatcherConfig +from ddev.cli.ci.tests.dispatcher_config import ( + BatchingConfig, + DispatcherConfig, + GitHubRetryConfig, + RetryLimitsConfig, +) from ddev.cli.ci.tests.rate_limiting import RateLimiterFactoryConfig from ddev.repo.config import RepositoryConfig from ddev.utils.fs import Path @@ -135,3 +141,24 @@ def test_batching_rejects_out_of_range_max_jobs_per_batch(repo_config: RepoConfi with pytest.raises(ValidationError): DispatcherConfig.from_repo_config(config) + + +def test_configured_retry_limits_reach_the_policies_without_changing_what_is_retried(): + """The config tunes the ladder; widening it would make a duplicate side effect a setting. + + Catches an `apply_to` that rebuilt the policy from scratch and silently dropped its conditions, + which would let a workflow dispatch be replayed after a read timeout. + """ + config = GitHubRetryConfig( + safe=RetryLimitsConfig(attempts=7, timeout_seconds=120.0), + mutating=RetryLimitsConfig(attempts=1), + ) + + policies = config.to_policies() + request = httpx.Request("GET", "https://api.github.com/x") + server_error = httpx.HTTPStatusError("boom", request=request, response=httpx.Response(502, request=request)) + + assert (policies.safe.attempts, policies.safe.timeout) == (7, 120.0) + assert policies.mutating.attempts == 1 + assert policies.safe.should_retry(server_error) + assert not policies.mutating.should_retry(server_error) diff --git a/ddev/tests/cli/ci/tests/test_github_retries.py b/ddev/tests/cli/ci/tests/test_github_retries.py deleted file mode 100644 index 5d1502efa1720..0000000000000 --- a/ddev/tests/cli/ci/tests/test_github_retries.py +++ /dev/null @@ -1,40 +0,0 @@ -# (C) Datadog, Inc. 2026-present -# All rights reserved -# Licensed under a 3-clause BSD style license (see LICENSE) -"""Tests for turning the Dispatcher's retry configuration into client policies.""" - -from __future__ import annotations - -import httpx - -from ddev.cli.ci.tests.github_retries import GitHubRetryConfig, RetryLimitsConfig - - -def test_configured_limits_reach_the_policies(): - """The config exists to tune the ladder, so its numbers have to arrive on the policy.""" - config = GitHubRetryConfig( - safe=RetryLimitsConfig(attempts=7, timeout_seconds=120.0, wait_max_seconds=30.0), - mutating=RetryLimitsConfig(attempts=1), - ) - - policies = config.to_policies() - - assert policies.safe.attempts == 7 - assert policies.safe.timeout == 120.0 - assert policies.safe.wait_max == 30.0 - assert policies.mutating.attempts == 1 - - -def test_configuring_the_limits_does_not_change_what_is_retried(): - """Widening the conditions from a config file would make a duplicate workflow run a setting. - - Only the ladder is configurable, so each tier has to keep the conditions it was built with: the - safe tier still replays a 502, and the mutating tier still refuses one. - """ - policies = GitHubRetryConfig(safe=RetryLimitsConfig(attempts=9)).to_policies() - request = httpx.Request("GET", "https://api.github.com/x") - server_error = httpx.HTTPStatusError("boom", request=request, response=httpx.Response(502, request=request)) - - assert policies.safe.should_retry(server_error) - assert not policies.mutating.should_retry(server_error) - assert policies.mutating.should_retry(httpx.ConnectError("refused")) diff --git a/ddev/tests/utils/github_async/conftest.py b/ddev/tests/utils/github_async/conftest.py index 7ecbe17c94e48..b365b60b420c1 100644 --- a/ddev/tests/utils/github_async/conftest.py +++ b/ddev/tests/utils/github_async/conftest.py @@ -12,6 +12,6 @@ def instant_backoff(monkeypatch: pytest.MonkeyPatch) -> None: """Remove the wait between retries, so a test that exhausts a policy costs no wall-clock time. - stamina sleeps through ``asyncio.sleep``, which this replaces with a fake clock advance. + stamina sleeps through `asyncio.sleep`, which this replaces with a fake clock advance. """ advance_clock_on_sleep(FakeClock(), monkeypatch) diff --git a/ddev/tests/utils/github_async/test_retry.py b/ddev/tests/utils/github_async/test_retry.py index 7f7f99f052791..721dacbeb9ed5 100644 --- a/ddev/tests/utils/github_async/test_retry.py +++ b/ddev/tests/utils/github_async/test_retry.py @@ -3,9 +3,8 @@ # Licensed under a 3-clause BSD style license (see LICENSE) """Tests for retrying the failures that are not rate limiting. -The question every test here answers is which failures a given endpoint replays, and the reason it -matters is that replaying the wrong one duplicates a side effect: a second workflow run, a second -comment. Rate-limit retries are a different layer and live in ``test_rate_limiting.py``. +Which failures an endpoint replays, and why that matters: replaying the wrong one duplicates a side +effect. Rate-limit retries are a different layer, covered in `test_rate_limiting.py`. """ from __future__ import annotations @@ -22,7 +21,6 @@ SAFE_RETRY, RetryPolicies, RetryPolicy, - never, on_status, on_transport_error, ) @@ -132,7 +130,7 @@ async def test_a_caller_can_widen_a_mutation_that_it_knows_is_safe_to_repeat() - async def test_the_client_defaults_can_be_replaced_wholesale() -> None: - """Dispatcher builds these from config, so the constructor argument has to reach the requests.""" + """The limits are configurable, so what the constructor is given has to reach the requests.""" transport, calls = recording_transport([httpx.ConnectError("refused")]) policies = RetryPolicies(safe=SAFE_RETRY.replace(attempts=5), mutating=MUTATION_RETRY) client = AsyncGitHubClient(token=TOKEN, transport=transport, retry_policies=policies) @@ -174,7 +172,7 @@ async def test_the_client_refuses_to_replay_what_replaying_cannot_fix(response: async def test_a_missing_resource_is_an_answer_rather_than_a_failure_to_retry() -> None: - """``get_pull_request`` returning 404 means there is no pull request for that number. + """`get_pull_request` returning 404 means there is no pull request for that number. Dispatcher relies on that answer to fall through to commit resolution, so retrying it would only delay a decision GitHub has already given. It stays out of the defaults rather than out of every @@ -282,15 +280,20 @@ def test_unless_removes_a_condition_the_policy_would_otherwise_retry() -> None: assert policy.should_retry(_status_error(502)) -def test_a_policy_that_cannot_retry_is_rejected_rather_than_silently_useless() -> None: - """``attempts=0`` would make every request fail without being sent.""" - with pytest.raises(ValueError, match="attempts must be at least 1"): - RetryPolicy(should_retry=on_transport_error, attempts=0) - +@pytest.mark.parametrize( + ("limits", "message"), + [ + pytest.param({"attempts": 0}, "attempts must be at least 1", id="no_attempts"), + pytest.param({"timeout": 0}, "timeout must be positive", id="zero_timeout"), + ], +) +def test_a_policy_that_could_never_send_a_request_is_rejected(limits: dict[str, int], message: str) -> None: + """Both spellings mean "no attempts at all", which would fail every call without sending it. -def test_no_retry_refuses_everything() -> None: - assert NO_RETRY.should_retry is never - assert NO_RETRY.attempts == 1 + stamina reads `timeout=0` that way rather than as an error, so it has to be caught here. + """ + with pytest.raises(ValueError, match=message): + RetryPolicy(should_retry=on_transport_error, **limits) def _status_error(status_code: int) -> httpx.HTTPStatusError: From 2ab07c16be6fd4faef68c4eb843a68c9d0fd94e0 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 18:12:44 +0200 Subject: [PATCH 4/8] Keep the signed artifact URL out of retry logs 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. --- ddev/src/ddev/utils/github_async/client.py | 24 ++++++++++-- .../github_async/test_download_artifact.py | 37 +++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/ddev/src/ddev/utils/github_async/client.py b/ddev/src/ddev/utils/github_async/client.py index 68eca6c4c806a..f39eeab4ad04f 100644 --- a/ddev/src/ddev/utils/github_async/client.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -1124,11 +1124,29 @@ async def _download_and_extract_zip( dest_path: Path, timeout: float | None = None, ) -> None: - """Anonymous fetch (no bearer token to S3) + zip-slip-validated extractall.""" + """Anonymous fetch (no bearer token to S3) + zip-slip-validated extractall. + + Failures are re-raised without the URL. The signature lives in its query string, and a failure + here is retryable, so the exception reaches both this client's log line and stamina's retry + hook, each of which renders it. + """ effective_timeout = self._effective_timeout(timeout) + safe_url = loggable_url(signed_url) async with httpx.AsyncClient(timeout=effective_timeout) as anonymous_client: - download_response = await anonymous_client.get(signed_url) - download_response.raise_for_status() + try: + download_response = await anonymous_client.get(signed_url) + except httpx.TransportError as exc: + # Chained: a transport error reports an OS-level reason and keeps the URL in its + # request, not its message. + raise type(exc)(f"artifact download from {safe_url}: {exc}") from exc + if download_response.is_error or is_redirect_status(download_response.status_code): + # Built here rather than by raise_for_status, whose message embeds the full URL. Not + # chained, so the original message cannot resurface in a traceback. + raise httpx.HTTPStatusError( + f"artifact download from {safe_url} failed with HTTP {download_response.status_code}", + request=download_response.request, + response=download_response, + ) dest_path.mkdir(parents=True, exist_ok=True) with zipfile.ZipFile(io.BytesIO(download_response.content)) as zf: diff --git a/ddev/tests/utils/github_async/test_download_artifact.py b/ddev/tests/utils/github_async/test_download_artifact.py index 014126c837622..9f4c42f251393 100644 --- a/ddev/tests/utils/github_async/test_download_artifact.py +++ b/ddev/tests/utils/github_async/test_download_artifact.py @@ -2,6 +2,8 @@ from __future__ import annotations +import logging + import httpx import pytest @@ -150,3 +152,38 @@ def github_handler(request: httpx.Request) -> httpx.Response: await client.download_artifact("/repos/o/r/actions/artifacts/1/zip", tmp_path / "out") assert len(calls) == 1 + + +async def test_the_signed_url_credentials_never_reach_the_log(monkeypatch, tmp_path, caplog) -> None: + """A presigned URL carries its signature in the query string, so a retry must not log the URL. + + The retried exception reaches this client's log line and stamina's retry hook, and httpx builds + its message from the full URL, so a leak here would write usable artifact credentials into CI + logs that outlive the run. + """ + signature = "X-Amz-Signature=b1acc0dedb1acc0de" + github_calls: list[httpx.Request] = [] + + def github_handler(request: httpx.Request) -> httpx.Response: + github_calls.append(request) + return httpx.Response(302, headers={"location": f"https://signed.example/zip?{signature}"}) + + def signed_handler(request: httpx.Request) -> httpx.Response: + if len(github_calls) == 1: + return httpx.Response(403, content=b"AccessDenied") + return httpx.Response(200, content=make_zip({"hello.txt": b"hi"})) + + patch_signed_download(monkeypatch, signed_handler) + client = AsyncGitHubClient( + token=TOKEN, transport=httpx.MockTransport(github_handler), logger=logging.getLogger("test-client") + ) + + with caplog.at_level(logging.WARNING): + await client.download_artifact("/repos/o/r/actions/artifacts/1/zip", tmp_path / "out") + + # The expired URL was retried, so there is a retry to have logged something. + assert len(github_calls) == 2 + assert caplog.records + # Messages and every structured field, since the signature can hide in either. + logged = "\n".join(f"{record.getMessage()} {record.__dict__}" for record in caplog.records) + assert signature not in logged From 7de91ae802c170d59fb1bd5f6036708b93ac2ace Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 18:15:57 +0200 Subject: [PATCH 5/8] Stop depending on httpx message format to keep the signed URL out of 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. --- ddev/src/ddev/utils/github_async/client.py | 25 +++++--- .../github_async/test_download_artifact.py | 61 +++++++++++++++---- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/ddev/src/ddev/utils/github_async/client.py b/ddev/src/ddev/utils/github_async/client.py index f39eeab4ad04f..b5b668ded2a01 100644 --- a/ddev/src/ddev/utils/github_async/client.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -143,6 +143,16 @@ def loggable_url(url: str) -> str: return url.split("?", 1)[0] +def without_query_of(text: str, url: str) -> str: + """`text` with `url`'s query string removed. + + For messages that quote a failure reason produced by someone else: whether a URL we must not log + ends up in one is not our decision to depend on. + """ + _, _, query = url.partition("?") + return text.replace(query, "") if query else text + + class RetryCause: """Carries the failure from the predicate, which sees it, to the log line, which counts attempts. @@ -1126,9 +1136,9 @@ async def _download_and_extract_zip( ) -> None: """Anonymous fetch (no bearer token to S3) + zip-slip-validated extractall. - Failures are re-raised without the URL. The signature lives in its query string, and a failure - here is retryable, so the exception reaches both this client's log line and stamina's retry - hook, each of which renders it. + Failures are re-raised without the URL and without a cause. The signature lives in its query + string, a failure here is retryable, and the exception reaches this client's log line, stamina's + retry hook and any traceback, all of which render it or its chain. """ effective_timeout = self._effective_timeout(timeout) safe_url = loggable_url(signed_url) @@ -1136,12 +1146,11 @@ async def _download_and_extract_zip( try: download_response = await anonymous_client.get(signed_url) except httpx.TransportError as exc: - # Chained: a transport error reports an OS-level reason and keeps the URL in its - # request, not its message. - raise type(exc)(f"artifact download from {safe_url}: {exc}") from exc + raise type(exc)( + f"artifact download from {safe_url}: {without_query_of(str(exc), signed_url)}" + ) from None if download_response.is_error or is_redirect_status(download_response.status_code): - # Built here rather than by raise_for_status, whose message embeds the full URL. Not - # chained, so the original message cannot resurface in a traceback. + # Built here rather than by raise_for_status, whose message embeds the full URL. raise httpx.HTTPStatusError( f"artifact download from {safe_url} failed with HTTP {download_response.status_code}", request=download_response.request, diff --git a/ddev/tests/utils/github_async/test_download_artifact.py b/ddev/tests/utils/github_async/test_download_artifact.py index 9f4c42f251393..8df7bdaa9c5ff 100644 --- a/ddev/tests/utils/github_async/test_download_artifact.py +++ b/ddev/tests/utils/github_async/test_download_artifact.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import traceback import httpx import pytest @@ -154,25 +155,44 @@ def github_handler(request: httpx.Request) -> httpx.Response: assert len(calls) == 1 -async def test_the_signed_url_credentials_never_reach_the_log(monkeypatch, tmp_path, caplog) -> None: - """A presigned URL carries its signature in the query string, so a retry must not log the URL. +# A signed URL keeps its signature in the query string. Both cases below assume the worst about what +# a failure quotes: a status whose httpx message is built from the full URL, and a transport error +# whose message happens to contain it. Neither is ours to control, so neither is relied upon. +SIGNATURE = "X-Amz-Signature=b1acc0dedb1acc0de" +SIGNED_URL = f"https://signed.example/zip?{SIGNATURE}" - The retried exception reaches this client's log line and stamina's retry hook, and httpx builds - its message from the full URL, so a leak here would write usable artifact credentials into CI - logs that outlive the run. - """ - signature = "X-Amz-Signature=b1acc0dedb1acc0de" +SIGNED_DOWNLOAD_FAILURES = [ + pytest.param(httpx.Response(403, content=b"AccessDenied"), id="expired-signature"), + pytest.param(httpx.ConnectError(f"connection to {SIGNED_URL} refused"), id="transport-error"), +] + + +def _signed_download(failure: httpx.Response | Exception, *, fail_every_attempt: bool): + """GitHub handler and signed handler where the signed download fails at least once.""" github_calls: list[httpx.Request] = [] def github_handler(request: httpx.Request) -> httpx.Response: github_calls.append(request) - return httpx.Response(302, headers={"location": f"https://signed.example/zip?{signature}"}) + return httpx.Response(302, headers={"location": SIGNED_URL}) def signed_handler(request: httpx.Request) -> httpx.Response: - if len(github_calls) == 1: - return httpx.Response(403, content=b"AccessDenied") + if fail_every_attempt or len(github_calls) == 1: + if isinstance(failure, Exception): + raise failure + return failure return httpx.Response(200, content=make_zip({"hello.txt": b"hi"})) + return github_handler, signed_handler, github_calls + + +@pytest.mark.parametrize("failure", SIGNED_DOWNLOAD_FAILURES) +async def test_the_signed_url_credentials_never_reach_the_log(monkeypatch, tmp_path, caplog, failure) -> None: + """A retry of the signed download must not write usable artifact credentials into CI logs. + + The retried exception reaches this client's log line and stamina's retry hook, both of which + render it, and CI logs outlive the signature's validity. + """ + github_handler, signed_handler, github_calls = _signed_download(failure, fail_every_attempt=False) patch_signed_download(monkeypatch, signed_handler) client = AsyncGitHubClient( token=TOKEN, transport=httpx.MockTransport(github_handler), logger=logging.getLogger("test-client") @@ -181,9 +201,26 @@ def signed_handler(request: httpx.Request) -> httpx.Response: with caplog.at_level(logging.WARNING): await client.download_artifact("/repos/o/r/actions/artifacts/1/zip", tmp_path / "out") - # The expired URL was retried, so there is a retry to have logged something. + # The failure was retried, so there is a retry to have logged something. assert len(github_calls) == 2 assert caplog.records # Messages and every structured field, since the signature can hide in either. logged = "\n".join(f"{record.getMessage()} {record.__dict__}" for record in caplog.records) - assert signature not in logged + assert SIGNATURE not in logged + + +@pytest.mark.parametrize("failure", SIGNED_DOWNLOAD_FAILURES) +async def test_the_signed_url_credentials_never_reach_the_error_that_escapes(monkeypatch, tmp_path, failure) -> None: + """Once the retries are spent the failure reaches the caller, whose handler may log it. + + Python prints a chained cause in full, so the chain has to be as clean as the message. + """ + github_handler, signed_handler, _ = _signed_download(failure, fail_every_attempt=True) + patch_signed_download(monkeypatch, signed_handler) + client = AsyncGitHubClient(token=TOKEN, transport=httpx.MockTransport(github_handler)) + + with pytest.raises(httpx.HTTPError) as exc_info: + await client.download_artifact("/repos/o/r/actions/artifacts/1/zip", tmp_path / "out") + + reported = "".join(traceback.format_exception(exc_info.value)) + assert SIGNATURE not in reported From 8710c631c50f923f3e0740cf6c1039937d5b0eef Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 18:23:25 +0200 Subject: [PATCH 6/8] Mask signed URL query values instead of dropping the query 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. --- ddev/src/ddev/utils/github_async/client.py | 61 +++++++++++-------- .../utils/github_async/test_client_core.py | 38 ++++++++++++ .../github_async/test_download_artifact.py | 4 +- 3 files changed, 77 insertions(+), 26 deletions(-) diff --git a/ddev/src/ddev/utils/github_async/client.py b/ddev/src/ddev/utils/github_async/client.py index b5b668ded2a01..1eae6828650c8 100644 --- a/ddev/src/ddev/utils/github_async/client.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -70,6 +70,9 @@ REDIRECT_STATUS_RANGE = range(300, 400) +# Stands in for a query parameter value that must not be logged. +QUERY_VALUE_MASK = "***" # noqa: S105 + _LINK_RE = re.compile(r'<([^>]+)>;\s*rel="([^"]+)"') @@ -138,19 +141,30 @@ def is_redirect_status(status_code: int) -> bool: return status_code in REDIRECT_STATUS_RANGE -def loggable_url(url: str) -> str: - """`url` without its query string, which is where a signed URL keeps its signature.""" - return url.split("?", 1)[0] +def url_without_query(url: str) -> str: + """`url` up to its query string, which is where a signed URL keeps its signature.""" + return url.partition("?")[0] -def without_query_of(text: str, url: str) -> str: - """`text` with `url`'s query string removed. +def masked_query(query: str) -> str: + """`query` with every parameter value masked. - For messages that quote a failure reason produced by someone else: whether a URL we must not log - ends up in one is not our decision to depend on. + Names are kept because they identify the signing scheme and are useful in a log. Values are all + masked rather than only the ones known to be secret, because which parameter carries the signature + depends on the host a download redirects to (`X-Amz-Signature` on S3, `sig` on Azure Blob), so an + allowlist would leak the first time that changes. """ - _, _, query = url.partition("?") - return text.replace(query, "") if query else text + masked = [] + for parameter in query.split("&"): + name, separator, _ = parameter.partition("=") + masked.append(f"{name}={QUERY_VALUE_MASK}" if separator else name) + return "&".join(masked) + + +def with_query_masked(text: str, url: str) -> str: + """`text` with the query of `url` masked, for a message someone else built out of that URL.""" + query = url.partition("?")[2] + return text.replace(query, masked_query(query)) if query else text class RetryCause: @@ -1136,26 +1150,25 @@ async def _download_and_extract_zip( ) -> None: """Anonymous fetch (no bearer token to S3) + zip-slip-validated extractall. - Failures are re-raised without the URL and without a cause. The signature lives in its query - string, a failure here is retryable, and the exception reaches this client's log line, stamina's - retry hook and any traceback, all of which render it or its chain. + A failure here reports without the URL. The signature lives in its query string, the failure is + retryable, and the exception reaches this client's log line, stamina's retry hook and any + traceback, each of which renders its message. """ effective_timeout = self._effective_timeout(timeout) - safe_url = loggable_url(signed_url) async with httpx.AsyncClient(timeout=effective_timeout) as anonymous_client: try: download_response = await anonymous_client.get(signed_url) - except httpx.TransportError as exc: - raise type(exc)( - f"artifact download from {safe_url}: {without_query_of(str(exc), signed_url)}" - ) from None - if download_response.is_error or is_redirect_status(download_response.status_code): - # Built here rather than by raise_for_status, whose message embeds the full URL. - raise httpx.HTTPStatusError( - f"artifact download from {safe_url} failed with HTTP {download_response.status_code}", - request=download_response.request, - response=download_response, + download_response.raise_for_status() + except httpx.HTTPError as exc: + # Rewritten in place rather than replaced by a copy, so the type, the request and the + # frames survive and only the reason changes. httpx builds that reason from the full + # URL for a bad status, and whether it does so for a transport error is not ours to + # depend on. + exc.args = ( + f"artifact download from {url_without_query(signed_url)}: " + f"{with_query_masked(str(exc), signed_url)}", ) + raise dest_path.mkdir(parents=True, exist_ok=True) with zipfile.ZipFile(io.BytesIO(download_response.content)) as zf: @@ -1205,7 +1218,7 @@ async def download_artifact( cause = self._retry_cause(policy) async for attempt in retry_attempts(policy, cause): with attempt: - self._log_retry(f"artifact download {loggable_url(archive_download_url)}", cause, attempt) + self._log_retry(f"artifact download {url_without_query(archive_download_url)}", cause, attempt) # NO_RETRY on the inner call: this loop is the only ladder, or the two would multiply. location = await self._resolve_artifact_redirect(archive_download_url, timeout, retry=NO_RETRY) await self._download_and_extract_zip(location, dest_path, timeout) diff --git a/ddev/tests/utils/github_async/test_client_core.py b/ddev/tests/utils/github_async/test_client_core.py index cba077dfc9a23..d488806099108 100644 --- a/ddev/tests/utils/github_async/test_client_core.py +++ b/ddev/tests/utils/github_async/test_client_core.py @@ -9,6 +9,7 @@ import pytest from ddev.utils.github_async import GITHUB_API_VERSION, AsyncGitHubClient, PaginationData, async_github_client +from ddev.utils.github_async.client import QUERY_VALUE_MASK, masked_query, with_query_masked from tests.utils.github_async.helpers import TOKEN, json_response, make_client from tests.utils.github_async.payloads import artifact, workflow_run_payload @@ -122,3 +123,40 @@ def handler(request: httpx.Request) -> httpx.Response: assert len(pages) == 2 assert pages[0].data.artifacts[0].id == 1 assert pages[1].data.artifacts[0].id == 2 + + +@pytest.mark.parametrize( + ("query", "secret", "expected_names"), + [ + pytest.param( + "X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Expires=900&X-Amz-Signature=abc123SECRET", + "abc123SECRET", + ["X-Amz-Algorithm", "X-Amz-Expires", "X-Amz-Signature"], + id="s3", + ), + pytest.param( + "se=2026-08-24T13%3A00%3A00Z&sig=SECRETSAS%2Bxyz&sp=r", "SECRETSAS", ["se", "sig", "sp"], id="azure" + ), + ], +) +def test_masking_a_query_hides_every_value_and_keeps_every_name( + query: str, secret: str, expected_names: list[str] +) -> None: + """Which parameter holds the signature depends on the storage host, so all values are masked. + + Keeping the names is what makes a masked URL still worth logging: they say which signing scheme + was in play. Masking only the names we recognise would leak the first time a download redirects + somewhere new. + """ + masked = masked_query(query) + + assert secret not in masked + assert [parameter.partition("=")[0] for parameter in masked.split("&")] == expected_names + assert {parameter.partition("=")[2] for parameter in masked.split("&")} == {QUERY_VALUE_MASK} + + +def test_masking_leaves_a_message_that_quotes_no_url_alone() -> None: + """A transport error reports an OS-level reason, and rewriting one would only obscure it.""" + assert with_query_masked("[Errno 61] Connection refused", "https://signed.example/zip") == ( + "[Errno 61] Connection refused" + ) diff --git a/ddev/tests/utils/github_async/test_download_artifact.py b/ddev/tests/utils/github_async/test_download_artifact.py index 8df7bdaa9c5ff..15d84bf4f23af 100644 --- a/ddev/tests/utils/github_async/test_download_artifact.py +++ b/ddev/tests/utils/github_async/test_download_artifact.py @@ -158,8 +158,8 @@ def github_handler(request: httpx.Request) -> httpx.Response: # A signed URL keeps its signature in the query string. Both cases below assume the worst about what # a failure quotes: a status whose httpx message is built from the full URL, and a transport error # whose message happens to contain it. Neither is ours to control, so neither is relied upon. -SIGNATURE = "X-Amz-Signature=b1acc0dedb1acc0de" -SIGNED_URL = f"https://signed.example/zip?{SIGNATURE}" +SIGNATURE = "b1acc0dedb1acc0de" +SIGNED_URL = f"https://signed.example/zip?X-Amz-Expires=900&X-Amz-Signature={SIGNATURE}" SIGNED_DOWNLOAD_FAILURES = [ pytest.param(httpx.Response(403, content=b"AccessDenied"), id="expired-signature"), From 97f6e0ef83b2de905b2eeac2c67cd37e1be774ac Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 18:27:40 +0200 Subject: [PATCH 7/8] Keep the request attached when wrapping a transport error 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. --- ddev/src/ddev/utils/github_async/client.py | 5 ++++- .../tests/utils/github_async/test_client_core.py | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/ddev/src/ddev/utils/github_async/client.py b/ddev/src/ddev/utils/github_async/client.py index 1eae6828650c8..6a0c33826060d 100644 --- a/ddev/src/ddev/utils/github_async/client.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -352,7 +352,10 @@ async def _execute_request( try: response = await self._client.request(method, endpoint, timeout=timeout, **kwargs) except httpx.TransportError as exc: - raise type(exc)(f"{method} {endpoint}: {exc}") from exc + # Rewritten in place rather than replaced by a copy, which would drop the request httpx + # attached and leave `exc.request` raising RuntimeError for the caller. + exc.args = (f"{method} {endpoint}: {exc}",) + raise # Observe before raise_for_status, never after: learning must not be gated on success. A # failed response's rate-limit headers arm the shared pause even if the caller swallows the # exception, so one request's 403 protects every other in-flight and future request in this diff --git a/ddev/tests/utils/github_async/test_client_core.py b/ddev/tests/utils/github_async/test_client_core.py index d488806099108..a35c913be3944 100644 --- a/ddev/tests/utils/github_async/test_client_core.py +++ b/ddev/tests/utils/github_async/test_client_core.py @@ -10,6 +10,7 @@ from ddev.utils.github_async import GITHUB_API_VERSION, AsyncGitHubClient, PaginationData, async_github_client from ddev.utils.github_async.client import QUERY_VALUE_MASK, masked_query, with_query_masked +from ddev.utils.github_async.retry import NO_RETRY from tests.utils.github_async.helpers import TOKEN, json_response, make_client from tests.utils.github_async.payloads import artifact, workflow_run_payload @@ -160,3 +161,18 @@ def test_masking_leaves_a_message_that_quotes_no_url_alone() -> None: assert with_query_masked("[Errno 61] Connection refused", "https://signed.example/zip") == ( "[Errno 61] Connection refused" ) + + +async def test_a_transport_failure_still_carries_the_request_it_failed_on() -> None: + """The client adds context to a transport failure without discarding what httpx attached. + + A caller reaching for `exc.request` after a dropped connection would otherwise get + `RuntimeError: The .request property has not been set` instead of the request. + """ + client = make_client(httpx.MockTransport(lambda request: (_ for _ in ()).throw(httpx.ConnectError("refused")))) + + with pytest.raises(httpx.ConnectError) as exc_info: + await client.get_workflow_run("o", "r", 42, retry=NO_RETRY) + + assert exc_info.value.request.url.path == "/repos/o/r/actions/runs/42" + assert "GET /repos/o/r/actions/runs/42" in str(exc_info.value) From d96c71003830abe43e8cda52be78e2e653c3f032 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Mon, 24 Aug 2026 18:44:42 +0200 Subject: [PATCH 8/8] Make the retry policies frozen again 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. --- ddev/src/ddev/utils/github_async/retry.py | 44 +++++++++------------ ddev/tests/utils/github_async/test_retry.py | 17 ++++++++ 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/ddev/src/ddev/utils/github_async/retry.py b/ddev/src/ddev/utils/github_async/retry.py index 335a0ca61e36a..67427d8379416 100644 --- a/ddev/src/ddev/utils/github_async/retry.py +++ b/ddev/src/ddev/utils/github_async/retry.py @@ -94,36 +94,30 @@ def matches(exc: Exception) -> bool: # --------------------------------------------------------------------------- +@dataclass(frozen=True, slots=True) class RetryPolicy: """What to retry, and how hard to try. - Treat instances as values: `replace`, `also_on` and `unless` return new policies rather than - mutating this one, which matters because the defaults below are shared. - """ + Frozen because the defaults below are shared for the life of the process: tuning one in place would + change the behaviour of every client that took it. `replace`, `also_on` and `unless` return new + policies instead. - __slots__ = ("attempts", "should_retry", "timeout", "wait_initial", "wait_jitter", "wait_max") + `timeout` bounds the whole ladder including backoff; None leaves `attempts` as the only stop. + """ - def __init__( - self, - should_retry: RetryPredicate = never, - attempts: int = DEFAULT_ATTEMPTS, - timeout: float | None = DEFAULT_TIMEOUT_SECONDS, - wait_initial: float = DEFAULT_WAIT_INITIAL_SECONDS, - wait_max: float = DEFAULT_WAIT_MAX_SECONDS, - wait_jitter: float = DEFAULT_WAIT_JITTER_SECONDS, - ) -> None: - """`timeout` bounds the whole ladder including backoff; None leaves `attempts` as the only stop.""" - if attempts < 1: - raise ValueError(f"attempts must be at least 1, got {attempts}") - if timeout is not None and timeout <= 0: + should_retry: RetryPredicate = never + attempts: int = DEFAULT_ATTEMPTS + timeout: float | None = DEFAULT_TIMEOUT_SECONDS + wait_initial: float = DEFAULT_WAIT_INITIAL_SECONDS + wait_max: float = DEFAULT_WAIT_MAX_SECONDS + wait_jitter: float = DEFAULT_WAIT_JITTER_SECONDS + + def __post_init__(self) -> None: + if self.attempts < 1: + raise ValueError(f"attempts must be at least 1, got {self.attempts}") + if self.timeout is not None and self.timeout <= 0: # stamina reads timeout=0 as "no retries", a confusing way to spell attempts=1. - raise ValueError(f"timeout must be positive or None, got {timeout}") - self.should_retry = should_retry - self.attempts = attempts - self.timeout = timeout - self.wait_initial = wait_initial - self.wait_max = wait_max - self.wait_jitter = wait_jitter + raise ValueError(f"timeout must be positive or None, got {self.timeout}") def replace( self, @@ -165,7 +159,7 @@ def narrowed(exc: Exception) -> bool: MUTATION_RETRY = RetryPolicy(should_retry=on_pre_send_transport_error, attempts=MUTATION_ATTEMPTS) -@dataclass(frozen=True) +@dataclass(frozen=True, slots=True) class RetryPolicies: """The defaults a client picks from, one per replay-safety class. diff --git a/ddev/tests/utils/github_async/test_retry.py b/ddev/tests/utils/github_async/test_retry.py index 721dacbeb9ed5..af1f04c4e6cf2 100644 --- a/ddev/tests/utils/github_async/test_retry.py +++ b/ddev/tests/utils/github_async/test_retry.py @@ -10,12 +10,14 @@ from __future__ import annotations import logging +from dataclasses import FrozenInstanceError import httpx import pytest from ddev.utils.github_async import AsyncGitHubClient from ddev.utils.github_async.retry import ( + DEFAULT_ATTEMPTS, MUTATION_RETRY, NO_RETRY, SAFE_RETRY, @@ -280,6 +282,21 @@ def test_unless_removes_a_condition_the_policy_would_otherwise_retry() -> None: assert policy.should_retry(_status_error(502)) +def test_a_shared_default_cannot_be_retuned_in_place() -> None: + """The defaults are shared for the life of the process, so one caller must not retune them. + + `SAFE_RETRY.attempts = 1` would otherwise disable retries for every client already holding it, + including ones on other tasks, which is a change nobody could trace back to its cause. + """ + with pytest.raises(FrozenInstanceError): + SAFE_RETRY.attempts = 1 # type: ignore[misc] + + assert SAFE_RETRY.attempts == DEFAULT_ATTEMPTS + # Tuning goes through replace, which leaves the shared default alone. + assert SAFE_RETRY.replace(attempts=1).attempts == 1 + assert SAFE_RETRY.attempts == DEFAULT_ATTEMPTS + + @pytest.mark.parametrize( ("limits", "message"), [