Skip to content

fix(retryer): retry transient network errors with backoff - #510

Open
luojiyin1987 wants to merge 1 commit into
stats-organization:masterfrom
vizrtluo:fix/retryer-transient-retry
Open

fix(retryer): retry transient network errors with backoff#510
luojiyin1987 wants to merge 1 commit into
stats-organization:masterfrom
vizrtluo:fix/retryer-transient-retry

Conversation

@luojiyin1987

@luojiyin1987 luojiyin1987 commented Aug 24, 2026

Copy link
Copy Markdown

Fixes #508

What

retryer() now separates token rotation from transient retry:

  • Each PAT gets an inner retry with exponential backoff (1s, 2s, 4s) plus jitter before rotation moves to the next token.
  • A failure is transient when the error has no response (network level: ECONNRESET, ETIMEDOUT, socket hang up, DNS failures) or when the HTTP status is 502/503/504.
  • Rate-limit responses (HTTP 429, or 403 carrying a rate-limit message) skip quick retries entirely and rotate to the next PAT, avoiding violations of GitHub Retry-After / reset guidance.
  • Permanent failures keep their behavior: bad credentials and suspended accounts rotate to the next PAT; non-axios errors and other HTTP errors are returned or thrown immediately.
  • Existing call sites remain backward-compatible. A new optional options parameter allows retry delays to be overridden for tests.

Final error message

Exhaustion now reports the real cause instead of always claiming rate limiting:

// transient exhaustion (no rate limit observed):
GitHub API request failed after transient retries: connect ECONNRESET 140.82.x.x:443

// genuine rate-limit rotation exhaustion (unchanged):
Downtime due to GitHub API rate limiting

The MAX_RETRY type is kept, so secondaryMessage mapping stays the same.

Tests

  • New cases in packages/core/tests/retryer.test.ts: same-PAT retry after a network error, retry on 502, no retry on 404, immediate throw for non-axios errors, accurate message on transient exhaustion, and no "rate limiting" claim when only transient errors occurred.
  • apps/backend/tests/status.up.test.js: the two networkError tests now use fake timers to advance the backoff schedule.

Full suite: 728 passed. Retryer status handling is covered by a parameterized matrix (502/503/504 quick retry, 429 rotate).

@vercel

vercel Bot commented Aug 24, 2026

Copy link
Copy Markdown

@luojiyin1987 is attempting to deploy a commit to the martin-mfg's projects Team on Vercel.

A member of the Team first needs to authorize it.

@luojiyin1987
luojiyin1987 force-pushed the fix/retryer-transient-retry branch from 2bf2fc0 to 63bd62e Compare August 24, 2026 02:24
@luojiyin1987

Copy link
Copy Markdown
Author

Agreed — reporting "rate limiting" with no rate limit in sight was contradictory. Reworked:

  • Transient exhaustion now throws GitHub API request failed after transient retries: <cause> (your suggested wording).
  • Genuine rate-limit/bad-credential rotation exhaustion keeps the historical Downtime due to GitHub API rate limiting message, so existing tests and consumers are unaffected.
  • MAX_RETRY type unchanged; added a regression test asserting the absence of "rate limiting" when only transient errors occurred.

I kept the reason classification at this two-way level for now: a fuller split (new error type per exhaustion reason) would change the public contract, and secondaryMessage mapping would need entries for each new type. Happy to follow up with that if maintainers want it.

@luojiyin1987

Copy link
Copy Markdown
Author

Both points addressed:

P3: PR description

Fixed. The body now says:

Existing call sites remain backward-compatible. A new optional options parameter allows retry delays to be overridden for tests.

You are right that "signature is unchanged" was wrong — the signature gained an optional fourth parameter, and only call-site compatibility is guaranteed.

P2: mixed-failure attribution

Agreed with keeping this out of the PR. lastTransientError is shared across the rotation and never cleared, so in mixed scenarios (PAT A transient → PAT B rate-limited) the final message reports the transient cause even though the last signal was rate limiting.

I have documented the known limitation and the lastFailureKind idea on #508 as a candidate follow-up, so it does not get lost after merge.

@luojiyin1987
luojiyin1987 force-pushed the fix/retryer-transient-retry branch from 63bd62e to 3963a22 Compare August 24, 2026 03:40
@luojiyin1987

Copy link
Copy Markdown
Author

Two updates, mirroring feedback that came out of reviewing the same code on our deployment fork:

Rate-limit responses no longer get quick retries

HTTP 429 (and 403 answers carrying a rate-limit message) previously sat in the quick-retry set. They now skip the backoff loop entirely and rotate to the next PAT instead, so callers do not violate GitHub Retry-After / reset guidance. Quick backoff stays reserved for network-level errors and 502/503/504.

Covered by a parameterized status matrix (502/503/504 → retry, 429 → rotate) plus a dedicated rate-limit-403 case.

Tests parameterized

The per-status tests collapsed into one it.each matrix; the two exhaustion-message tests merged into a single positive-plus-negative assertion ("reports the cause" and "does not claim rate limiting").

Full suite: 728 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transient network errors are never retried; retry count equals the number of PATs

1 participant