Skip to content

fix: retry transient github api failures on cached get requests - #248

Merged
vigneshrajsb merged 3 commits into
mainfrom
fix-github-transient-retry
Sep 21, 2026
Merged

vigneshrajsb merged 3 commits into
mainfrom
fix-github-transient-retry

Conversation

@vigneshrajsb

@vigneshrajsb vigneshrajsb commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Description

  • A transient GitHub API failure — typically a keep-alive socket the remote closes, which octokit reports as an error with status 500 — was treated by cacheRequest as a hard failure with no retry.
  • Callers that read repository config through it could not distinguish that from a genuinely missing config file. A pull request opened with autoDeploy enabled was therefore handled as a non-autoDeploy repo: its environment never received the deploy label, and because config is only read when the PR opens, no later push corrected it.
  • A cached GET that fails this way is now retried once, 10s later. Writes are excluded, so a retried POST cannot duplicate a deployment, and the wait is short enough that a retry cannot outlive a worker's shutdown grace.

Verifying Changes

  • jest full suite and eslint --ext .ts src — both green.
  • New unit tests cover a 5xx GET retried then succeeding, a non-GET never retried, a 304 refetch that hits the transient failure, and the single retry being exhausted.

Notes

  • Retries only trigger on status >= 500. A 429 or any 4xx still fails immediately, as before — backing off blindly on a rate limit would be worse than failing fast.
  • One retry, not a backoff ladder: the failure this targets is an instantly-closed socket, which a single spaced retry clears. Anything that outlives 10s is an outage rather than a blip.
  • So this lowers the odds of the stuck state rather than removing it. The durable fix is to re-read config on later pushes when auto-deploy has not been applied, which is worth a follow-up.
  • New GitHub: cache request retrying log line, with the endpoint and status, for anyone watching this in production.

A dropped keep-alive socket surfaces as an octokit RequestError with status
500, which cacheRequest reported as a hard failure with no retry. Callers that
read repository config through it then could not tell a network blip from a
missing config file: a pull request opened with autoDeploy enabled was treated
as a non-autoDeploy repo, so its environment never received the deploy label,
and nothing later re-read the config to correct it.

Retry idempotent GET requests twice, at 5s and 15s. Writes are excluded so a
retried POST cannot duplicate a deployment, and the budget stays under the job
queue's 30s lock so a retry cannot outlive a worker restart.
The cached-body refetch and the transient retry are two separate recursive
paths through cacheRequest. This pins the attempt budget being preserved
across the first and consumed only by the second.

Also corrects the retry comment: the delays are bounded by a worker's
shutdown grace, not by the job lock, which is renewed while the process runs.
@vigneshrajsb
vigneshrajsb marked this pull request as ready for review September 21, 2026 18:54
@vigneshrajsb
vigneshrajsb requested a review from a team as a code owner September 21, 2026 18:54
Replaces the two-entry delay schedule and its attempt counter with one
constant and a boolean. The retried request is the same idempotent GET, so
the only thing the counter bought was a second wait, and a 10s gap already
clears the dropped socket that motivated the retry. A failure that outlives
it is an outage rather than a blip, and wants a different fix.
@vigneshrajsb
vigneshrajsb merged commit 9b24e50 into main Sep 21, 2026
5 checks 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.

1 participant