Skip to content

GitHub requests retry transient failures - #331

Merged
moq-bot[bot] merged 8 commits into
mainfrom
quest/m0/github-retry
Nov 1, 2026
Merged

moq-bot[bot] merged 8 commits into
mainfrom
quest/m0/github-retry

Conversation

@kixelated

@kixelated kixelated commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes quest/m0/github-retry.md: one 5xx response or dropped connection no longer fails a quest command or a land step. Evidence: a 502 during the fixer's handback of moq-dev/moq#7323.

Summary

GitHub.request (src/cli/github.ts) now retries a 5xx or a dropped connection up to four times, waiting 1, 2, 4, then 8 seconds, or GitHub's Retry-After when it sends one. After that it fails with the last error. The existing rate-limit handling stays: 429 and secondary 403 limits honour Retry-After and x-ratelimit-reset, or back off from one minute. Any request repeats after a rate limit, since GitHub turned it away without processing it. Every wait goes through the injected sleep/now and is cut short by the client's deadline.

Two further gaps are closed in the same PR:

  • Body reads. request now reads every response's body before returning it, as a fresh Response with the same status and headers. A connection that drops mid-body therefore counts as a dropped connection and follows the same repeat/confirm rules below. A write whose response body was lost goes through its confirm path, never a blind repeat. A refusal whose body was cut off still acts on its status.
  • GraphQL RATE_LIMITED. GitHub can report a GraphQL rate limit as HTTP 200 with errors[].type == "RATE_LIMITED". graphql() now waits that out exactly like a REST limit (Retry-After, else the primary reset, else one minute doubling, six times) within the deadline. Any operation repeats after it, mutations included, since GitHub turned it away unprocessed. The REST and GraphQL waits share GitHub.waitOut.

Whether a request repeats after a transient failure depends on its method and endpoint. That policy lives in one place, GitHub.repeatable and GitHub.mutationRepeat, so no call site changed:

  • Repeat as is: GET, PUT, PATCH and DELETE, which read or set state, and adding labels (a set union). That covers pagination through all()/list(), closing PRs, Review-body PATCHes and label removals.
  • Confirm first, then repeat only if the lost attempt took no effect:
    • PUT /pulls/:n/merge: the PR reads back as merged at the pinned sha. The check returns GitHub's success shape with merge_commit_sha, so mergePinned continues the batch.
    • POST /issues/:n/comments: a comment with the identical body exists since the first attempt (minus 60 s for clock skew) that was not already there. Before the first POST, the client lists the identical comments already on the issue, so an earlier identical comment, such as a requeued PR's previous ejection, is never taken for the lost one.
    • POST /pulls: an open PR from that head branch was created since the first attempt.
    • GraphQL mergePullRequest, enablePullRequestAutoMerge, disablePullRequestAutoMerge and markPullRequestReadyForReview: the PR's node reads back with the effect. A mutation that pins expectedHeadOid is confirmed only at that head.
  • Never repeat: any other POST or mutation, which fails on the first transient error as before.
  • GraphQL queries repeat as is.

No double retry. mergePinned (src/cli/land-serve.ts) retries only GitHub's 405/409 mergeability states, never 5xx. daemonLoop retries whole batches above this layer. The landed checks issue their own reads, which have their own bounded retries.

Tests. New test/github.test.ts covers backoff, the bounds, Retry-After, the deadline, each repeat class, each confirmation, and a mismatched pinned head. It also covers bodies cut off on reads, comments, unconfirmable POSTs and mutations, and GraphQL RATE_LIMITED waits, their bound and deadline. None of it sleeps on the wall clock. Two test/land.test.ts cases change to the new behaviour:

  • A lost merge response is now confirmed and the batch goes on. The restart case remains for a loss that outlasts every retry.

  • A lost enable-auto-merge response is now confirmed and the fixed head requeued, instead of handed back. After merging main, this test keeps main's fixedScript(): it asserts the requeue at the fixed head, its pin and label, the fix recorded in `## Review

  • Reviewer: Codex (quest review --base origin/main), gpt-6.1-sol, medium effort, session 01a198f8-c973-7921-a920-71b0842c9041 (new session after the main merge).

  • Round 4 on head c253a94, against origin/main 1733726: no findings.

  • Round 3 (head c253a94) finding, refuted: a local clock more than a minute ahead of GitHub misses the lost comment and posts it twice. The one-minute bound is unchanged from the reviewed d6048b3 (src/cli/github.ts:297), the failure is a duplicate rather than a lost or false confirmation, and the land daemon cannot authenticate at that skew, since its App JWT sets iat: now - 60 (src/cli/land-serve.ts:60).

  • Round 2 (head 752dfcc) finding, fixed in c253a94: the measured-clock window from 752dfcc still let an identical comment within its 2 s slack pass; replaced by the pre-POST snapshot of identical comments.

  • Round 1 (head 0f55a49, the main merge) finding, fixed in 752dfcc then c253a94: an identical earlier comment, such as a requeued PR's previous ejection, was taken for a lost one.

  • Design checkpoint at round 4: no findings remain; the approach stands.

  • Earlier session 01a193cb-5fc6-7ae2-b503-efbf8d64a7a7: round 3 on d6048b3 reported none; round 1's finding was fixed in 4ff38d4.

Checks

Local checks on c253a94 (merged with origin/main 1733726): just check and env -u CLAUDE_CONFIG_DIR just test pass (795 passed, 2 skipped; Worker 426 passed, including the Cloudflare/workerd suite the land fixer could not run). quest check --base origin/main passes.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 3 commits October 31, 2026 13:54
Quest-Claim: 68e122f3af183d13
GitHub.request retries a 5xx or a dropped connection up to four times
(1, 2, 4, 8 s, or GitHub's Retry-After), then fails with the last error.
Reads, PUT, PATCH, DELETE and label adds repeat; a merge, a comment, a
new pull request, or a pull request mutation is repeated only after a
read-back shows the lost attempt took no effect. Any other POST or
mutation never repeats.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review October 31, 2026 21:14
@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a773262f-57fd-4b8c-bc0a-1330f289318b
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

…id-read

request reads a response's body before returning it, so a connection that drops mid-body goes through the same repeat and confirm rules as one that drops before the response. graphql waits out a 200 whose errors say RATE_LIMITED like a REST rate limit, within the deadline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated enabled auto-merge (squash) November 1, 2026 20:57
@moq-bot
moq-bot Bot disabled auto-merge November 1, 2026 20:57
@moq-bot

moq-bot Bot commented Nov 1, 2026

Copy link
Copy Markdown

quest land removed this pull request from the queue: Conflicts with the base: test/land.test.ts
.

@moq-bot moq-bot Bot removed the quest:merge label Nov 1, 2026
@moq-bot

moq-bot Bot commented Nov 1, 2026

Copy link
Copy Markdown

codex / Codex land fixer: Batch log: "Conflicts with the base: test/land.test.ts"; trial resolution preserved base fixedScript() and PR retry-confirmation assertions, but required validation could not complete: &#96;cd cloudflare . Recommendation: Re·solve test/land.test.ts by retaining base fixedScript() with the PR confirmed-response requeue assertions and fi·xed-result comment assertion; rerun CLI and Cloudflare builds/tests in a working test

…de_modules/@cloudflare/workerd-linux-64/bin/workerd@32c3ca0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2ef6ed0; sentryErrorContext = jsgInternalError; wdErrId = n4393an24l1luo31ro27is5c
uncaught exception; source = Uncaught (in promise); stack = Error: internal error; reference = n4393an24l1luo31ro27is5c
uncaught exception; exception = workerd/jsg/_virtual_includes/iterator/workerd/jsg/value.h:1494: failed: jsg.Error: internal error; reference = n4393an24l1luo31ro27is5c
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@3253b20 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@325478f /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2faf58f
e = kj/compat/tls.c++:269: failed: TLS peer's certificate is not trusted; reason = unable to get local issuer certificate
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb8320 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb88b5 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fc0380 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a75340 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a755f0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a777b7 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581a787 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581b497 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581e4c3 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57f8ff0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57ffc70 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@58172e0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2247470 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@32c3ca0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2ef6ed0; sentryErrorContext = jsgInternalError; wdErrId = 902skn6kc1a3ui2cgjefn4tf
uncaught exception; source = Uncaught (in promise); stack = Error: internal error; reference = 902skn6kc1a3ui2cgjefn4tf
uncaught exception; exception = workerd/jsg/_virtual_includes/iterator/workerd/jsg/value.h:1494: failed: jsg.Error: internal error; reference = 902skn6kc1a3ui2cgjefn4tf
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@3253b20 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@325478f /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2faf58f
e = kj/compat/tls.c++:269: failed: TLS peer's certificate is not trusted; reason = unable to get local issuer certificate
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb8320 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb88b5 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fc0380 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a75340 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a755f0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a777b7 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581a787 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581b497 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581e4c3 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57f8ff0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57ffc70 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@58172e0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2247470 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@32c3ca0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2ef6ed0; sentryErrorContext = jsgInternalError; wdErrId = 5rc034c22n35cdh2jotijg31
uncaught exception; source = Uncaught (in promise); stack = Error: internal error; reference = 5rc034c22n35cdh2jotijg31
uncaught exception; exception = workerd/jsg/_virtual_includes/iterator/workerd/jsg/value.h:1494: failed: jsg.Error: internal error; reference = 5rc034c22n35cdh2jotijg31
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@3253b20 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@325478f /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2faf58f
e = kj/compat/tls.c++:269: failed: TLS peer's certificate is not trusted; reason = unable to get local issuer certificate
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb8320 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb88b5 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fc0380 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a75340 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a755f0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a777b7 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581a787 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581b497 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581e4c3 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57f8ff0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57ffc70 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@58172e0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2247470 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@32c3ca0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2ef6ed0; sentryErrorContext = jsgInternalError; wdErrId = amb21j39bbi952ftdki3b246
uncaught exception; source = Uncaught (in promise); stack = Error: internal error; reference = amb21j39bbi952ftdki3b246
uncaught exception; exception = workerd/jsg/_virtual_includes/iterator/workerd/jsg/value.h:1494: failed: jsg.Error: internal error; reference = amb21j39bbi952ftdki3b246
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@3253b20 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@325478f /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2faf58f
e = kj/compat/tls.c++:269: failed: TLS peer's certificate is not trusted; reason = unable to get local issuer certificate
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb8320 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fb88b5 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2fc0380 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a75340 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a755f0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5a777b7 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581a787 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581b497 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@581e4c3 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57f8ff0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@57ffc70 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@58172e0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2247470 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@32c3ca0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2ef6ed0; sentryErrorContext = jsgInternalError; wdErrId = bg4hrjbpfgniocajulqqn4jc
uncaught exception; source = Uncaught (in promise); stack = Error: internal error; reference = bg4hrjbpfgniocajulqqn4jc
uncaught exception; exception = workerd/jsg/_virtual_includes/iterator/workerd/jsg/value.h:1494: failed: jsg.Error: internal error; reference = bg4hrjbpfgniocajulqqn4jc
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@3253b20 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@325478f /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5adaf68 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@2faf58f
exception = kj/compat/http.c++:4778: disconnected: other end of WebSocketPipe was destroyed
stack: /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@582232a /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5825e11 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@34501d8 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@5ae02e0 /work/node_modules/@cloudflare/workerd-linux-64/bin/workerd@249f7b0
error: script "test" exited with code 130

exec
/bin/bash -lc "rg -n 'fetch\\(|https?://' cloudflare/test/setup.ts cloudflare/test/github.test.ts cloudflare/dev/github.ts | head -20" in /work
 succeeded in 0ms:
cloudflare/dev/github.ts:287:		url: `https://github.com/${repo.owner}/${repo.name}/pull/${pull.number}`,
cloudflare/dev/github.ts:567:		html_url: `https://github.com/${repo.owner}/${repo.name}/pull/${pull.number}`,
cloudflare/dev/github.ts:609:		if (url.origin === "https://spdx.org") return json({ licenseId: "MIT", isOsiApproved: true });
cloudflare/dev/github.ts:610:		if (url.origin === "https://github.com" && path === "/login/oauth/access_token")
cloudflare/dev/github.ts:612:		if (url.origin !== "https://api.github.com") return missing(method, request.url);
cloudflare/test/github.test.ts:248:				: json({ id: 100, html_url: "https://github.com/upstream/project/pull/100" });

codex
Cloudflare test startup repeatedly failed with `TLS peer's certificate is not trusted; reason = unable to get local issuer certificate`. I stopped that run. Since the required tests cannot pass here, PR #331 must be handed back; I’ll restore its branch and record the blocker.

 exited 130 in 108175ms:
$ bun build src/cli/bin.ts --target=bun --banner "#!/usr/bin/env bun" --outfile dist/quest.js && bun scripts/executable.mjs dist/quest.js
Bundled 228 modules in 1919ms

  quest.js  0.95 MB  (entry point)

$ vitest run test/github.test.ts test/land.test.ts

 RUN  v5.0.5 /work


exec
/bin/bash -lc "git merge --abort; python3 - <<'PY'
import json
result=[{'number':331,'action':'handback','diagnosis':'Batch log: \"Conflicts with the base: test/land.test.ts\"; trial resolution preserved base fixedScript() and PR retry-confirmation assertions, but required validation could not complete: "'`cd cloudflare && bun run test` repeatedly emitted "TLS peer'"\\'s certificate is not trusted; reason = unable to get local issuer certificate\" and was stopped; "'`bun run test -- test/github.test.ts test/land.test.ts` also made no progress after startup and was stopped. Builds and `bun run check` passed; trial merge aborted and PR branch restored.'"','recommendation':'Resolve test/land.test.ts by retaining base fixedScript() with the PR confirmed-response requeue assertions and fixed-result comment assertion; rerun CLI and Cloudflare builds/tests in a working test environment before landing.'}]
with open('quest-land-result.json','w') as f: json.dump(result,f,indent=2); f.write('\\n')
PY
 git status --short; git rev-parse pr-331; git rev-parse batch; git diff --check" in /work
 succeeded in 44ms:
?? quest-land-result.json
d6048b36e7ecdc7718d61ce9b81bb94517c8bdb4
d6048b36e7ecdc7718d61ce9b81bb94517c8bdb4

codex
PR #331 handed back: the test conflict is mechanically resolvable, but validation was blocked by Cloudflare TLS certificate errors and stalled CLI tests.

The PR branch is unchanged. Diagnosis and recommendation are recorded in uncommitted [quest-land-result.json](/work/quest-land-result.json).
tokens used
42,188
PR #331 handed back: the test conflict is mechanically resolvable, but validation was blocked by Cloudflare TLS certificate errors and stalled CLI tests.

The PR branch is unchanged. Diagnosis and recommendation are recorded in uncommitted [quest-land-result.json](/work/quest-land-result.json).
fixer result written

(written by codex / Codex)

kixelated and others added 3 commits November 1, 2026 13:14
Keep main's fixedScript() in the lost enable-auto-merge test, which now
confirms the effect and requeues the fixed head without an outcome comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ub's clock

An identical comment posted within the minute before, as when a requeued PR
is ejected again for the same reason, passed for the lost one, so the new
comment was never posted. Measure GitHub's clock from each response's Date
header and accept only comments created since the request started, to its
whole seconds; keep the minute's slack until a response has measured it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…started

Snapshot the identical comments already on the issue before posting, so a
requeued PR's earlier ejection comment is never taken for a lost one, however
soon it came. This replaces the measured clock window, which still let one
through within its slack.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated enabled auto-merge (squash) November 1, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant