[3/4] artifact: honor Retry-After on 429 by tunc-d · Pull Request #2498 · actions/toolkit

tunc-d · 2026-09-24T13:33:38Z

Artifact retry behavior

When the artifact service rate-limits a request with HTTP 429, it sends Retry-After to indicate when to retry. Ignoring it can exhaust attempts before the service is ready.

This PR reads the header only on 429, before reading/parsing the body, so malformed or non-JSON error payloads do not discard it. After trimming surrounding whitespace, only positive decimal integer seconds are accepted (parseInt(value, 10) plus digits-only validation). Missing, zero, negative, fractional, HTTP-date, and malformed values use exponential backoff; other status codes ignore the header.

The fallback base increases from 3s to 8s, retaining the 1.5 multiplier and default maximum of five attempts. There is no sleep after the final failure. A per-request 120s accumulated-sleep budget rejects the next wait if it would exceed that budget; reaching exactly 120s is allowed. This is not a wall-clock HTTP request timeout.

Current scope

Updated through a normal merge of upstream main at 6cb87687384f971ebb756e43c7a56f62cd80a31d, preserving history and the existing retry implementation/tests. The previously inherited unpublished HTTP-client 4.0.2 release/minimum layer and redundant dependency overrides have been removed. All current-main dependency/audit changes are retained; affected locks were regenerated with npm from main's genuine published manifests.

The residual diff against current main is exactly two files: packages/artifact/src/internal/shared/artifact-twirp-client.ts and packages/artifact/__tests__/artifact-http-client.test.ts. There are no dependency, package version, release-note, or lockfile deltas. No unpublished package or fabricated registry resolution is required. The previous dependent-PR sequencing is superseded and is not a prerequisite for this PR.

Local validation

Real root and affected-package npm ci, repository bootstrap, full TypeScript build, lint, formatting, and git diff --check pass.
Root production audit and all nine repository CI package audits pass with zero vulnerabilities. The CI audit target does not include @actions/attest; its unchanged full-install audit still reports baseline advisories and is not claimed fixed here.
Node 24: artifact/core/glob/tool-cache/HTTP-client regression suites — 31 suites, 498 tests passed.
Node 20: artifact and HTTP-client builds plus retry/HTTP-client/proxy tests — 6 suites, 119 tests passed.

Remote checks

All checks completed on exact head 0285a6794dfa664df68bf70948b9e950d313bdd6: 20 of 21 pass, including Audit, uploads, CodeQL, and the other Node 20/24 platform jobs.

Build (ubuntu-latest, 20.x) fails in the unchanged @actions/exec test Runs exec successfully with arguments split out: ENOENT opening packages/exec/__tests__/_temp/my.log. That job reports 779 passed tests and one failed test; the failure is outside this PR's two-file diff. CI is not represented as fully green. No repair or rerun is performed; the lifetime CI-repair budget remains exhausted at 3/3.

No merge, npm publication, auto-merge, draft-state change, or reviewer-thread replies/resolutions are performed.

Copilot

Copilot review overview

🟡 Changes recommended

Read Retry-After before parsing the response body so valid headers are honored independently of malformed error payloads.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Read Retry-After before parsing response body · New

What changed in this PR

Updates the artifact Twirp client to honor bounded Retry-After delays and use longer default retry backoff intervals.

Changes:

Adds capped Retry-After handling for 429/503 responses.
Changes default backoff to 5s with a multiplier of 2.
Adds comprehensive retry timing tests.

File	Summary
`packages/artifact/src/internal/shared/artifact-twirp-client.ts`	Implements retry timing changes; valid headers are not honored when response bodies are malformed.
`packages/artifact/__tests__/artifact-http-client.test.ts`	Tests header handling, caps, fallbacks, and backoff behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟡 Changes recommended

A 429 UsageError path can bypass the required rate-limit warning.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

429 usage errors bypass the required rate-limit warning · New

Resolved since last review (1)

Read Retry-After before parsing response body

Copilot

Copilot review overview

🟡 Changes recommended

packages/artifact/package.json lacks the required undici override, leaving the lockfile on vulnerable 6.28.0.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Add undici override to artifact lockfile · New

Resolved since last review (1)

429 usage errors bypass the required rate-limit warning

Files not reviewed (7)

packages/artifact/package-lock.json: Generated file
packages/attest/package-lock.json: Generated file
packages/cache/package-lock.json: Generated file
packages/core/package-lock.json: Generated file
packages/glob/package-lock.json: Generated file
packages/http-client/package-lock.json: Generated file
packages/tool-cache/package-lock.json: Generated file

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (1)

Add undici override to artifact lockfile

Files not reviewed (7)

packages/artifact/package-lock.json: Generated file
packages/attest/package-lock.json: Generated file
packages/cache/package-lock.json: Generated file
packages/core/package-lock.json: Generated file
packages/glob/package-lock.json: Generated file
packages/http-client/package-lock.json: Generated file
packages/tool-cache/package-lock.json: Generated file

When the Results service responds with HTTP 429 and a Retry-After header that parses to a positive number of seconds, wait that long before the next attempt instead of using exponential backoff. The header is read before the response body is parsed, so it is honored even when the body is not valid JSON. A missing or invalid header falls back to the existing backoff, and other retryable statuses are unchanged. Add a 120 second retry timeout per request covering both Retry-After and backoff waits: if the next wait would push the total over the limit, fail immediately instead of sleeping. Raise the default backoff base interval from 3s to 8s (multiplier 1.5, 5 attempts), so waits are 8s, 12-18s, 18-27s and 27-40.5s. A retry without Retry-After then always lands in a later minute and the backoff fits within the retry timeout. Release @actions/artifact 6.3.0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Fix the moderate+ npm audit findings that make the Audit check fail on main by changing the package.json declarations that pull in the vulnerable versions, then regenerating the lockfiles with npm: - http-client: raise undici to ^6.28.1 - core, tool-cache: override undici to ^6.28.1 (via the published @actions/http-client) - glob: override undici to ^6.28.1 and brace-expansion to ^5.0.12 (via minimatch) - artifact: override brace-expansion 2.x to ^2.1.7 (via archiver) and 5.x to ^5.0.12 (via typedoc), undici to ^6.28.1 and markdown-it to ^14.3.1 (via typedoc) Overrides are per package because audit-all installs each package separately. No major versions change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

salmanmkc · 2026-10-01T09:43:02Z

+      return undefined
+    }
+
+    const parsed = parseInt(value)


nit, could do parseInt(value, 10) and a digit only check

salmanmkc · 2026-10-01T09:49:27Z

 {
  "name": "@actions/artifact",
-  "version": "6.2.2",
+  "version": "6.3.0",


Personally I'd split the releases all up as well: see #2500 as an example.

salmanmkc · 2026-10-01T10:04:04Z

  "dependencies": {
    "tunnel": "^0.0.6",
-    "undici": "^6.23.0"
+    "undici": "^6.28.1"


Could this go in its own PR? it's on the proxy path. If we release http-client first, the other packages can just raise their http-client minimum instead of needing undici overrides.

salmanmkc · 2026-10-01T10:04:45Z

    "@types/semver": "^7.7.1",
    "nock": "^13.5.1"
+  },
+  "overrides": {


don't need this if you bump undici in http-client and then release that and consume it here

salmanmkc · 2026-10-01T10:09:21Z

+    "brace-expansion@^2": "^2.1.7",
+    "brace-expansion@^5": "^5.0.12",
+    "undici": "^6.28.1",
+    "markdown-it": "^14.3.1"


These are all for the npm audit CI check rather than the retry change, right? Could we split them into a separate PR too? That way it's easy to undo if a dependency bump causes issues, without touching the retry change.

Remove dependency audit and release metadata deltas for separate PRs. Parse trimmed decimal integer seconds with an explicit radix and cover invalid headers, retry limits, and accumulated wait boundaries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

salmanmkc · 2026-10-01T10:26:01Z

Thanks for this, the Retry-After handling looks good 👍
Requesting changes mainly to split the PR up so each piece is easy to review and undo on its own:

Retry change: keep this PR to just the Twirp client change and its tests.
npm audit fixes: move the undici bump in http-client and the new overrides into separate PRs, ideally one per package. Dependency bumps in artifact have caused regressions before (e.g. [bug] V6 Upload progress stalled behind proxy while V5 works upload-artifact#747), so it's nice to be able to revert them without touching the retry fix.
Release prep: split the version bumps and release notes per package too, similar to Cache v6.3.0 release #2500.

I also left a small nit on the parseInt radix. Happy to re-review once it's split!

Restore the artifact version and retry-only release notes alongside its implementation. Keep the separate HTTP-client and audit dependency work out of this PR. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Raise the Undici minimum to 6.28.1 and regenerate the package lockfile at 6.29.0. Prepare only the HTTP-client patch release; leave consumer upgrades and artifact behavior to their own layers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Regenerate the assigned consumer locks with published patch releases while retaining existing HTTP-client major versions and consumer ranges. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Raise dependency floors only for artifact, core and tool-cache. HTTP-client 4.0.2 remains unpublished; preserve real audit-only registry lock entries. The draft is not mergeable until publication and proper consumer lock regeneration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Merge the approved consumer dependency layer without rewriting history. Preserve its genuine registry locks and unpublished HTTP-client minimum intent; move artifact 6.3.0 metadata to the upcoming fourth layer while keeping retry source and tests unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Keep the artifact Retry-After implementation and tests unchanged on current upstream audit fixes. Remove unpublished HTTP-client 4.0.2 release/minimum intent and redundant overrides; regenerate affected locks with npm against main's published manifests. No release or dependency-policy changes remain in the PR diff. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

User-requested fresh CI run with no file changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

tunc-d marked this pull request as ready for review September 24, 2026 14:54

tunc-d requested a review from a team as a code owner September 24, 2026 14:54

Copilot AI lite review requested due to automatic review settings September 24, 2026 14:54

Copilot started reviewing on behalf of tunc-d September 24, 2026 14:55 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes