Repository navigation
fix(resilience): deadlines on three fetches that could hang forever - #73
Merged
Merged
Conversation
Three reproduced defects, all the same shape: a fetch with no deadline
feeding a piece of UI that waits on it. Failure was handled in every
case; SLOWNESS was not, and slowness is the worse case because nothing
ever fires.
1. A shared `/?q=...` link could leave the ENTIRE CATALOG permanently
invisible. `corpusSettled` is set only inside the search-index fetch's
handlers, the gate effect keeps `.catalog-filtered` on <html> until it
settles, and globals.css hides the whole grid under that class. The
comment above the gate says "a failed fetch reveals the grid rather than
hiding it forever" -- true, and exactly why the stall was missed: a fetch
that never settles fires neither handler. Reproduced: the controls read
"8 of 542" with a Clear link above a blank page, still blank at 15s.
2. A hanging /api/saves disabled all 542 save buttons and mislabelled
them. `authenticated` stays null until it settles and save-button.tsx
disables on null, so every button went disabled, cursor-wait, labelled
"Save" instead of offering sign-in. This route is Convex-backed, so
"Convex slow" produces precisely this.
3. live-preview-frame accepted `readyState === "complete"` -- which a
fresh iframe's own `about:blank` already satisfies. So a slow embed route
faded the card IN and the placeholder OUT before the real document
existed: rows of blank boxes, no dot grid, no loading signal.
Fixes: an 8s `AbortSignal.timeout` on 1 and 2, matching the existing
lib/status-checks.ts idiom, so the abort lands in the `.catch` that
already degrades correctly -- the grid reveals unfiltered, the buttons
resolve to signed-out. For 3, also require `doc.URL !== "about:blank"`.
Verified by reproduction, control against treatment on the same
selectors, request stalled indefinitely rather than failed:
search index before: display none, 0 cards, gate still on
after: display grid, 8 cards, gate retired
/api/saves before: 542 disabled, title "Save"
after: 0 disabled, title "Sign in to save this component"
Mutation-proven on the severe one: removing the deadline again returns
gridDisplay "none" with the gate class still set, and restoring it
returns "grid".
Defect 3's check is WEAKER than the other two and I am not claiming
otherwise: with the embed route stalled no iframes mounted at all, so
"0 blank-but-visible" is trivially true rather than evidence. The code
change is right by construction -- about:blank is observably reported as
complete -- but it is not demonstrated here.
Signed-off-by: Nik <84yk8btb9f@privaterelay.appleid.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
nikolas-sapa
added a commit
that referenced
this pull request
Oct 14, 2026
/submit opens a PR as the submitting user, in 9 sequential GitHub calls
with up to 9s of deliberate sleep in the fork poll, with no timeout on
any call and no maxDuration on the route. The branch name is
deterministic (`submit/${slug}`), and createBranch threw on any non-ok
response including 422.
So a function killed mid-flow, after the fork and branch existed, left
the slug unsubmittable FOREVER: every later retry hit 422 "Reference
already exists" and threw. The contributor could not recover without
manually deleting the branch on their own fork. This is the contribution
path for a project with almost no outside contributors, so one person
hitting it probably never came back.
Reproduced on unmodified shipped code, not argued from reading it.
`origin/main:lib/github-submit.ts` was extracted verbatim (sole edit: a
`public status` constructor parameter property rewritten longhand,
because Node's strip-only .ts loader rejects that syntax — diffed to
prove nothing else changed) and the route's exact call sequence replayed
against a fake GitHub at the fetch boundary. Attempt 1 killed at call 6:
branch survived, 0 files written, 0 PRs. Attempts 2, 3 and 4 — each with
a full budget and no kill — then died identically on `Could not create
branch submit/retry-trap-demo (422)` after 5 calls. Zero PRs ever.
THREE traps existed, not the one the audit found:
ensureBranch 422 "Reference already exists"
putFile 422 `"sha" wasn't supplied` (one step later)
openPullRequest 422 "A pull request already exists"
Each now resumes, but only on POSITIVE confirmation — it re-reads the
ref, the file, or the PR list. A 422 that cannot be confirmed is still a
real failure and still throws, rather than being pattern-matched out of
GitHub's prose.
Kept the deterministic branch name instead of uniquifying per attempt. A
unique branch retries just as cleanly but leaves a dead branch on the
contributor's fork and a duplicate upstream PR for every failed attempt:
permanent maintainer cleanup to spare the contributor one click.
Pre-flight delete is worse — it can destroy work.
Clobber protection: when the branch exists AND an open PR is found, the
PR is adopted and nothing is written, so a resubmit cannot rewrite files
a maintainer is mid-review on.
Closed the window as well as the trap: GITHUB_CALL_TIMEOUT_MS of 8s per
call (the precedent set in #73), a 45s whole-submission AbortSignal
budget intersected into every call, a fork poll that will not sleep past
that budget, and maxDuration = 60 on the route.
Security properties verified rather than assumed. The PR-adoption path
cannot cross users: `ensureFork` returns `owner: data.owner.login` read
from the fork response made with the CALLER's own token, never from env
and never from client input; `findOpenPullRequest` filters
`head=<that login>:<branch>` and `openPullRequest` posts
`head: ${forkOwner}:${branch}` from the same value. UPSTREAM_OWNER /
UPSTREAM_REPO only ever name the target repo, never the head. So user B
submitting a slug user A already submitted resolves to
`B:submit/<slug>` on B's own fork and can never adopt A's PR.
SUBMIT_TOKEN_BINDING_SECRET untouched and still fails closed. This
change does touch the route that owns that secret, so "no hits in the
diff" was not good enough: all 14 lines mentioning the secret,
`github_not_configured`, `resolveGithubToken` or 428 are text-identical
to origin/main with only line numbers shifted by the maxDuration comment
block above them, and `resolveGithubToken` itself is byte-identical
(diff -q clean). The missing-secret branch still returns
`github_not_configured` and no unbound token is ever issued. The new
test references neither the secret nor that error string and adds no
credentials.
scripts/test-submit-retry.ts — 17 checks, no framework, driven against a
fake GitHub at the fetch boundary. Mutation-proven: reverting the
ensureBranch 422 resume (the original bug) takes it to exit 1 with 10
failing checks and the message `Could not create branch
submit/retry-trap-demo (422)`; restoring it returns exit 0. Five further
mutants — no AbortSignal, putFile not resuming, openPullRequest not
adopting, the open-PR pre-check dropped, the fork poll ignoring its
budget — each fail with their own named check.
Verified on this rebase onto #76: tsc exit 0, source invariants 0
problems, submit-retry PASS, build exit 0.
Separately established, not a code change: the 3 tsc errors in
convex/auth.ts seen in this worktree are an install artifact.
`@auth/core` is a `peer: true` lockfile entry at 0.41.3, a peer of
@convex-dev/auth and absent from package.json, and the global ~/.npmrc
sets legacy-peer-deps=true, which makes npm skip auto-installing peers.
It was missing from node_modules in this worktree only, present in all
five siblings. Installing it with --no-save --package-lock=false took
tsc to 0 errors with package.json and package-lock.json untouched. CI
has no legacy-peer-deps=true and never saw these.
Signed-off-by: Nik <84yk8btb9f@privaterelay.appleid.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nikolas-sapa
added a commit
that referenced
this pull request
Oct 14, 2026
…ug (#78) /submit opens a PR as the submitting user, in 9 sequential GitHub calls with up to 9s of deliberate sleep in the fork poll, with no timeout on any call and no maxDuration on the route. The branch name is deterministic (`submit/${slug}`), and createBranch threw on any non-ok response including 422. So a function killed mid-flow, after the fork and branch existed, left the slug unsubmittable FOREVER: every later retry hit 422 "Reference already exists" and threw. The contributor could not recover without manually deleting the branch on their own fork. This is the contribution path for a project with almost no outside contributors, so one person hitting it probably never came back. Reproduced on unmodified shipped code, not argued from reading it. `origin/main:lib/github-submit.ts` was extracted verbatim (sole edit: a `public status` constructor parameter property rewritten longhand, because Node's strip-only .ts loader rejects that syntax — diffed to prove nothing else changed) and the route's exact call sequence replayed against a fake GitHub at the fetch boundary. Attempt 1 killed at call 6: branch survived, 0 files written, 0 PRs. Attempts 2, 3 and 4 — each with a full budget and no kill — then died identically on `Could not create branch submit/retry-trap-demo (422)` after 5 calls. Zero PRs ever. THREE traps existed, not the one the audit found: ensureBranch 422 "Reference already exists" putFile 422 `"sha" wasn't supplied` (one step later) openPullRequest 422 "A pull request already exists" Each now resumes, but only on POSITIVE confirmation — it re-reads the ref, the file, or the PR list. A 422 that cannot be confirmed is still a real failure and still throws, rather than being pattern-matched out of GitHub's prose. Kept the deterministic branch name instead of uniquifying per attempt. A unique branch retries just as cleanly but leaves a dead branch on the contributor's fork and a duplicate upstream PR for every failed attempt: permanent maintainer cleanup to spare the contributor one click. Pre-flight delete is worse — it can destroy work. Clobber protection: when the branch exists AND an open PR is found, the PR is adopted and nothing is written, so a resubmit cannot rewrite files a maintainer is mid-review on. Closed the window as well as the trap: GITHUB_CALL_TIMEOUT_MS of 8s per call (the precedent set in #73), a 45s whole-submission AbortSignal budget intersected into every call, a fork poll that will not sleep past that budget, and maxDuration = 60 on the route. Security properties verified rather than assumed. The PR-adoption path cannot cross users: `ensureFork` returns `owner: data.owner.login` read from the fork response made with the CALLER's own token, never from env and never from client input; `findOpenPullRequest` filters `head=<that login>:<branch>` and `openPullRequest` posts `head: ${forkOwner}:${branch}` from the same value. UPSTREAM_OWNER / UPSTREAM_REPO only ever name the target repo, never the head. So user B submitting a slug user A already submitted resolves to `B:submit/<slug>` on B's own fork and can never adopt A's PR. SUBMIT_TOKEN_BINDING_SECRET untouched and still fails closed. This change does touch the route that owns that secret, so "no hits in the diff" was not good enough: all 14 lines mentioning the secret, `github_not_configured`, `resolveGithubToken` or 428 are text-identical to origin/main with only line numbers shifted by the maxDuration comment block above them, and `resolveGithubToken` itself is byte-identical (diff -q clean). The missing-secret branch still returns `github_not_configured` and no unbound token is ever issued. The new test references neither the secret nor that error string and adds no credentials. scripts/test-submit-retry.ts — 17 checks, no framework, driven against a fake GitHub at the fetch boundary. Mutation-proven: reverting the ensureBranch 422 resume (the original bug) takes it to exit 1 with 10 failing checks and the message `Could not create branch submit/retry-trap-demo (422)`; restoring it returns exit 0. Five further mutants — no AbortSignal, putFile not resuming, openPullRequest not adopting, the open-PR pre-check dropped, the fork poll ignoring its budget — each fail with their own named check. Verified on this rebase onto #76: tsc exit 0, source invariants 0 problems, submit-retry PASS, build exit 0. Separately established, not a code change: the 3 tsc errors in convex/auth.ts seen in this worktree are an install artifact. `@auth/core` is a `peer: true` lockfile entry at 0.41.3, a peer of @convex-dev/auth and absent from package.json, and the global ~/.npmrc sets legacy-peer-deps=true, which makes npm skip auto-installing peers. It was missing from node_modules in this worktree only, present in all five siblings. Installing it with --no-save --package-lock=false took tsc to 0 errors with package.json and package-lock.json untouched. CI has no legacy-peer-deps=true and never saw these. Signed-off-by: Nik <84yk8btb9f@privaterelay.appleid.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three reproduced defects, all the same shape: a fetch with no deadline feeding UI that waits on it. Failure was handled in every case. Slowness was not — and slowness is the worse case, because nothing ever fires.
1. HIGH — a shared
/?q=…link could leave the entire catalog permanently invisiblecorpusSettledis set only inside the search-index fetch's handlers. The gate effect keeps.catalog-filteredon<html>until it settles, andglobals.csshides the whole grid under that class.The comment above that gate says "a failed fetch reveals the grid rather than hiding it forever" — which is true, and is exactly why the stall was missed: a fetch that never settles fires neither handler.
Reproduced: controls reading "8 of 542" with a Clear link above a blank page, still blank at 15s.
2. HIGH — a hanging
/api/savesdisabled all 542 save buttons and mislabelled themauthenticatedstaysnulluntil it settles, andsave-button.tsxdisables onnull. So every button went disabled,cursor-wait, labelled "Save" instead of offering sign-in. This route is Convex-backed, so "Convex slow" produces precisely this.3. HIGH — cards faded in empty on a slow embed route
live-preview-frameacceptedreadyState === "complete"— which a fresh iframe's ownabout:blankalready satisfies. So it faded the card in and the placeholder out before the real document existed: rows of blank boxes, no dot grid, no loading signal.Fixes
An 8s
AbortSignal.timeouton 1 and 2, matching the existinglib/status-checks.tsidiom, so the abort lands in the.catchthat already degrades correctly — grid reveals unfiltered, buttons resolve to signed-out. For 3, also requiredoc.URL !== "about:blank".Verified by reproduction
Control vs treatment on identical selectors, request stalled indefinitely rather than failed:
display:none, 0 cards, gate still ondisplay:grid, 8 cards, gate retired/api/savesstalledMutation-proven on the severe one: removing the deadline again returns
gridDisplay:"none"with the gate class set; restoring it returns"grid".One honest caveat
Defect 3's check is weaker than the other two. With the embed route stalled, no iframes mounted at all — so "0 blank-but-visible" is trivially true rather than evidence. The code change is right by construction (
about:blankis observably reported ascomplete), but it is not demonstrated here.Verification
npx tsc --noEmit— exit 0🤖 Generated with Claude Code