[AI-7115] Add a request retry strategy to the async GitHub client

ddev/changelog.d/24963.added

@@ -0,0 +1 @@
+Add a retry strategy to the async GitHub client for failures that are not rate limiting.

ddev/src/ddev/cli/ci/tests/dispatcher_config.py

@@ -10,6 +10,7 @@
 from pydantic import BaseModel, ConfigDict, Field
 
 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
@@ -26,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."""
 
@@ -67,6 +111,9 @@ class DispatcherConfig(BaseModel):
     github_rate_limits: RateLimiterFactoryConfig = RateLimiterFactoryConfig()
     """Rate limiter tiers shared by every task, from `[dispatcher.github_rate_limits]`."""
 
+    github_retries: GitHubRetryConfig = GitHubRetryConfig()
+    """How hard a failed GitHub request is retried, from `[dispatcher.github_retries]`."""
+
     @classmethod
     def from_repo_config(cls, repo_config: RepositoryConfig) -> DispatcherConfig:
         """Build a DispatcherConfig from the `/dispatcher` table of `.ddev/config.toml`."""

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

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',
 }