Harden withRetry for transient fetch failures - #61439
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
🟡 Changes recommended
Delay validation permits timer overflow, and unrelated action downgrades should be removed or justified.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens JavaScript retry handling for transient HTTP failures and server-directed backoff.
Changes:
- Adds status-based transient failure detection and broader
Retry-Aftersupport. - Validates retry configuration and caps jittered delays.
- Updates tests and Docker action pins.
File summaries
| File | Description |
|---|---|
actions/setup/js/error_recovery.cjs |
Implements retry hardening. |
actions/setup/js/error_recovery.test.cjs |
Tests new retry behavior. |
actions/setup/js/create_issue.test.cjs |
Updates delay-cap expectations. |
.github/aw/actions-lock.json |
Downgrades two Docker action pins. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
| for (const key of ["maxRetries", "initialDelayMs", "maxDelayMs", "jitterMs"]) { | ||
| const value = config[key]; | ||
| if (!Number.isSafeInteger(value) || value < 0) { | ||
| throw new RangeError(`Retry configuration ${key} must be a non-negative safe integer`); | ||
| } | ||
| } |
| "docker/build-push-action@v7.3.0": { | ||
| "repo": "docker/build-push-action", | ||
| "version": "v7.4.0", | ||
| "sha": "c3c9e263c25d99ce0380d002d59b67737d91b0dc" | ||
| "version": "v7.3.0", | ||
| "sha": "53b7df96c91f9c12dcc8a07bcb9ccacbed38856a" |
| "docker/setup-buildx-action@v4.3.0": { | ||
| "repo": "docker/setup-buildx-action", | ||
| "version": "v4.4.0", | ||
| "sha": "594f3bf4285d9ea8dc53c9a0c9c4092420091003" | ||
| "version": "v4.3.0", | ||
| "sha": "37fe631027851001ddb9b187196cc803df7f5f0e" |
There was a problem hiding this comment.
Reviewed the withRetry hardening changes for transient fetch failures using the impeccable harden + audit lenses (bug-fix/error-state change).
Logic review (error_recovery.cjs):
- Status-based transient detection (408/425/429/500/502/503/504), Fetch
Headers-aware header reads,Retry-Aftersupport extended to secondary-rate-limit 403 and 503, andmaxDelayMscapping after jitter all check out against the accompanying unit tests and manual verification. validateRetryConfigcorrectly rejects malformed retry configs before executing the operation.
One blocking issue found: .github/aw/actions-lock.json unintentionally reverts the docker action pin bump from #61359 (merged just before this branch) — see inline comment. This file is generated by make sync-action-pins, so it's very likely a stale-branch artifact, but as committed it downgrades docker/build-push-action and docker/setup-buildx-action pins.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
codeload.github.comgithub.laiyagushi.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "codeload.github.com"
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 155.5 AIC · ⌖ 13.7 AIC · ⊞ 8.4K
| "sha": "22d081ff2d3a40755e97629de92e3bcbfa7cf2ed" | ||
| }, | ||
| "docker/build-push-action@v7.4.0": { | ||
| "docker/build-push-action@v7.3.0": { |
There was a problem hiding this comment.
This PR reverts the docker action pin bump from #61359 (merged just before this branch): docker/build-push-action v7.4.0→v7.3.0 and docker/setup-buildx-action v4.4.0→v4.3.0, including their SHAs. This file is machine-generated (make sync-action-pins), so it's likely just a stale branch/merge artifact rather than an intentional change, but merging as-is will silently downgrade these pinned actions.
Please rebase onto latest main (or regenerate via gh aw compile) so this file reflects the current pins instead of reverting them.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design (classified by pr-triage as bug_fix) — requesting changes on the widened 403 rate-limit heuristic and an unrelated lock-file downgrade.
📋 Key Themes & Highlights
Key Themes
- Widened 403 rate-limit heuristic in
getRetryAfterMs/isRateLimitError: treating "nox-ratelimit-remainingheader +retry-afterpresent" as a secondary rate limit is a behavioral change with no authoritative signal behind it and no dedicated regression test for the false-positive case. - Duplicated classification logic between
getRetryAfterMsandisRateLimitError— same heuristic copy-pasted, risk of drift on future edits. - Unrelated lock-file downgrade in
.github/aw/actions-lock.json(docker action versions) — looks like a stale-branch artifact unrelated to thewithRetryhardening.
Positive Highlights
- ✅ New
TRANSIENT_HTTP_STATUSESset cleanly replaces brittle message-text matching for standard HTTP codes. - ✅
getHeader()correctly abstracts FetchHeadersvs. plain objects, with solid test coverage (it("should read case-insensitive headers from a Fetch Headers instance")). - ✅
validateRetryConfigand the jitter cap (Math.min(delay + jitter, maxDelayMs)) are well tested with explicit edge cases (Number.NaN,Infinity, non-integer, negative). - ✅ Existing
create_issue.test.cjswas correctly updated to reflect the new jitter-cap behavior.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 168.9 AIC · ⌖ 15.3 AIC · ⊞ 10.4K
Comment /matt to run again
| const remainingHeader = getHeader(headers, "x-ratelimit-remaining"); | ||
| const retryAfter = getHeader(headers, "retry-after"); | ||
| const remainingExhausted = remainingHeader != null && parseInt(remainingHeader, 10) === 0; | ||
| const isRateLimitStatus = status === 429 || (status === 403 && (remainingExhausted || (remainingHeader == null && retryAfter != null))); |
There was a problem hiding this comment.
[/diagnosing-bugs] Widening the 403 secondary-rate-limit heuristic to also match "no x-ratelimit-remaining header + retry-after present" risks misclassifying ordinary permission/auth 403s (e.g. from a proxy or WAF) that happen to send a retry-after header as GitHub rate-limit errors — triggering the E010 RATE_LIMIT_EXCEEDED code and honoring server-directed delay for a non-rate-limit failure.
💡 Why this matters & suggested fix
The original code only trusted x-ratelimit-remaining: 0 as the secondary-rate-limit signal for 403s, which is authoritative (GitHub always sets it to 0 on secondary rate limits). The new fallback branch (remainingHeader == null && retryAfter != null) has no such guarantee — any 403 with a retry-after header (e.g. from an intermediary) now gets treated as a rate limit.
This logic is duplicated between getRetryAfterMs and isRateLimitError (lines 177 and 223), so the same risk applies to both the delay calculation and the E010 classification.
Consider requiring the error message to also contain a rate-limit indicator (hasRateLimitIndicator) when the x-ratelimit-remaining header is absent, or add a regression test with a 403 + retry-after that is not a rate limit (e.g. an auth failure) to confirm it isn't misclassified.
@copilot please address this.
| const remainingHeader = getHeader(headers, "x-ratelimit-remaining"); | ||
| const retryAfterHeader = getHeader(headers, "retry-after"); | ||
| const remainingExhausted = remainingHeader != null && parseInt(remainingHeader, 10) === 0; | ||
| const hasRateLimitHeaders = status === 403 && (remainingExhausted || (remainingHeader == null && retryAfterHeader != null)); |
There was a problem hiding this comment.
[/codebase-design] The secondary-rate-limit-detection heuristic (remainingExhausted || (remainingHeader == null && retryAfter != null)) is copy-pasted between getRetryAfterMs (line 177) and isRateLimitError (line 223). Extract it into one shared helper (e.g. isSecondaryRateLimit403(status, headers)) so both call sites can't drift out of sync.
💡 Suggested refactor
function isSecondaryRateLimit403(status, headers) {
if (status !== 403) return false;
const remainingHeader = getHeader(headers, "x-ratelimit-remaining");
const retryAfter = getHeader(headers, "retry-after");
const remainingExhausted = remainingHeader != null && parseInt(remainingHeader, 10) === 0;
return remainingExhausted || (remainingHeader == null && retryAfter != null);
}Then both getRetryAfterMs and isRateLimitError call this single source of truth, eliminating the risk of the two implementations diverging on a future edit.
@copilot please address this.
| "sha": "22d081ff2d3a40755e97629de92e3bcbfa7cf2ed" | ||
| }, | ||
| "docker/build-push-action@v7.4.0": { | ||
| "docker/build-push-action@v7.3.0": { |
There was a problem hiding this comment.
[/diagnosing-bugs] This PR downgrades docker/build-push-action (v7.4.0→v7.3.0) and docker/setup-buildx-action (v4.4.0→v4.3.0), unrelated to the stated withRetry hardening. This looks like a stale branch/merge artifact rather than an intentional change.
💡 Suggested fix
Rebase onto latest main (or re-run the lock-file generator) so this file only reflects intentional changes, or drop this hunk from the PR if it isn't needed.
@copilot please address this.
|
Failed checks:
Latest blocking review points:
Run URL: https://github.com/github/gh-aw/actions/runs/35164374706 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 62b4dd5: restored the unrelated generated pin/lock changes, limited timer delays to 2,147,483,647 ms, centralized and tightened 403 rate-limit classification, and updated the affected test. Local pre-PR validation passes; the listed CI failures were cache misses, while the shard regression was corrected. |
|
🎉 This pull request is included in a new release. Release: |
withRetryrelied on error-message matching and could miss transient HTTP failures or mishandle server-directed delays. This change makes retries predictable across Fetch and Octokit error shapes.Changes
Transient failures
408,425,429,500,502,503, and504by status.Server-directed backoff
Retry-Afterfor429, GitHub secondary-rate-limit403, and503.Headersand plain header objects.x-ratelimit-resetspecific to rate-limit responses.Retry safety
maxDelayMs.pr-sous-chefRun: https://github.com/github/gh-aw/actions/runs/35164374706
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.