Retry intermediary 403s in artifact-twirp-client#2452
Open
npwolf wants to merge 1 commit into
Open
Conversation
The artifact Results Service's internal Twirp client treats every HTTP
403 as non-retryable, on the assumption it always reflects a genuine
authorization failure. In practice its edge/proxy layer intermittently
returns its own 403 ("Error from intermediary with HTTP status code
403"), which is a transient infrastructure hiccup rather than an auth
decision - the same artifact download succeeds moments later from a
parallel job in the same run.
isRetryableHttpStatusCode now also retries a 403 when the error message
identifies it as coming from an intermediary, while leaving all other
403s (real backend authorization failures) failing fast as before.
Fixes the failure mode reported in actions/download-artifact#464.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the resilience of the internal artifact Twirp HTTP client by adding a targeted retry for a specific transient failure mode: intermediary/proxy-generated HTTP 403 responses that can occur intermittently during artifact operations in CI.
Changes:
- Reorders error construction in
retryableRequest()so the full error message is available when deciding retryability. - Extends retry logic to treat HTTP 403 as retryable only when the error message indicates it came “from an intermediary”.
- Adds unit tests covering both the retryable intermediary-403 case and the fail-fast non-intermediary 403 case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/artifact/src/internal/shared/artifact-twirp-client.ts | Builds the full error message before computing retryability and conditionally retries intermediary-style 403s. |
| packages/artifact/tests/artifact-http-client.test.ts | Adds tests ensuring intermediary 403s retry and non-intermediary 403s remain non-retryable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CI intermittently fails an
actions/download-artifact@v8step with:This happened downloading an artifact that had been uploaded moments earlier in the same workflow run by a sibling job (a Rails CI setup with several parallel matrix shards downloading the same precompiled-assets artifact). Two other parallel shards in the same run downloaded the identical artifact successfully, so the artifact existed and wasn't expired — this was a transient failure on the artifact service's edge/proxy layer, surfaced with no retry.
This matches the currently open, unaddressed report in
actions/download-artifact#464("intermittent 403s with no retry" — 6/96 parallel jobs failing this way in one run, one comment speculating it's "some sort of throttle perhaps getting incorrectly returned as a 403"). Per this repo'sCONTRIBUTING.mdanddownload-artifact's own contributing guide, the retry logic actually lives here in@actions/artifact, not indownload-artifact.Root cause
ArtifactHttpClient.isRetryableHttpStatusCode()inpackages/artifact/src/internal/shared/artifact-twirp-client.tsonly retries502/504/500/503/429. A403always falls into the non-retryable branch ofretryableRequest()and throws immediately (Received non-retryable error: ...), matching the error above exactly.Why not just add 403 to the retryable list
A
403from the Results Service can mean two very different things:Blindly adding
HttpCodes.ForbiddentoretryableStatusCodeswould retry all 403s for up tomaxAttempts(5) with exponential backoff, including genuine permanent failures — masking real auth bugs behind ~30-60s of pointless retrying before still failing.Fix
isRetryableHttpStatusCodenow accepts the constructed error message and retries a403only when that message identifies it as coming from an intermediary (/error from intermediary/i). Every other403— including the existingUsageErrorcase, which is checked earlier and unaffected — continues to fail fast exactly as before.retryableRequestwas reordered slightly so the message (which already includesstatusMessageandbody.msg) is fully built before the retryability decision is made.Testing
Added two tests to
packages/artifact/__tests__/artifact-http-client.test.ts, following the existing mock/assert pattern in that file:postcalls)postcall), matching today's behaviorConfirmed the new "retries" test fails with today's code (reproducing the exact
Received non-retryable error: ...: Error from intermediary...message) and passes with the fix. Confirmed the existing 403 →UsageErrortest is unaffected.Ran
npm run bootstrap,npm run build,npm test -- packages/artifact(133/133 passing),npm run format-check, andnpm run lint— all clean.Scope
Deliberately does not touch
packages/artifact/src/internal/find/retry-options.ts, the separate octokit-based retry path used for the public/cross-repo API (listArtifactsPublic), which explicitly exempts403for good reason in that context (different threat model, different callers).Also relevant:
actions/download-artifact#339, an older, messier thread with several different root causes tangled together — some resolved by fixingpathusage, some by pinning versions — but with multiple commenters reporting the same general category of intermittent, non-path-related download failures.Happy to adjust the message-matching approach if maintainers have a more reliable signal for "this 403 came from the intermediary layer" (e.g., a header) than string-matching the body.