Skip to content

fix(web): PR panel no longer shows "All fibers interrupted" on open - #54

Merged
lukemaj merged 2 commits into
mainfrom
fix/pr-panel-interrupted
Sep 27, 2026
Merged

lukemaj merged 2 commits into
mainfrom
fix/pr-panel-interrupted

Conversation

@lukemaj

@lukemaj lukemaj commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Closes #52.

Opening a pull request from a link sometimes showed "Could not load pull requests: All fibers interrupted without error" in the right-panel PR tab until Retry.

Cause (from the desktop and server traces of the 19:35 occurrence): when the panel opens, the client starts the detail and activity reads, and its query atoms restart them about 6ms later, cancelling the first ones. On the server, the second detail request joins the first one's in-flight Cache lookup while that lookup is still being torn down by the cancellation (the gh subprocess kill takes a few ms). It gets back the interrupt: its server span is interrupted after 1ms with no GitHub work of its own. The client query atom stored that interrupt-only exit as a Failure, and the panel rendered it as an error. This path is upstream code the fork had not changed; the Issues work does not touch it.

Fix: an atom never observes its own cancellation (it detaches before interrupting), so an interrupt-only failure in an environment query is always the server's. createEnvironmentQueryAtomFamily now reads once more on an interrupt-only failure instead of surfacing it. This covers every environment query and every server version, including servers that are not upgraded. The server-side Cache join race stays as it is (a separate, upstream-shaped change).

The upstream edit is allowlisted and owned in the feature map (query-interrupt-retry).

Proof

  • New test reads again when the server interrupts a query this client did not cancel fails without the fix with All fibers interrupted without error, and passes with it.
  • vp test run packages/client-runtime/src/state/runtime.test.ts packages/client-runtime/src/state/pullRequests.test.ts: 75 passed.
  • client-runtime typecheck, vp lint and vp fmt --check on the changed files: clean.
  • knip: output identical to origin/main (existing findings only).
  • scripts/fork-check.sh: OK.
  • Not verified in a running client. No UI change, so no screenshots.

Done by Claude Opus 5.5 in T3 Code (Claude Code harness).

🤖 Generated with Claude Code

lukemaj and others added 2 commits September 27, 2026 20:00
)

Opening a pull request starts its detail read twice within a few ms. The
server lets the second read join the first read's shared lookup while the
first read's cancellation is tearing it down, and answers it with an
interrupt. The client query atom stored that as a failure, so the panel
showed "All fibers interrupted without error" until Retry.

An atom never observes its own cancellation, so an interrupt-only failure
in an environment query is the server's. The query now reads once more
instead of surfacing it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Sep 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.5 KiB 13.5 KiB +10 B (+0.1%) 15.1 KiB ✅
Codex Thread snapshot wire 7.1 KiB 7.1 KiB +2 B (+0.0%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.4 KiB 6.4 KiB +8 B (+0.1%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 56.2 KiB 56.2 KiB 0 B (0.0%) 66.4 KiB ✅
Codex Live turn messages 9 9 0 (0.0%) 21 ✅
Claude Total thread wire 13.5 KiB 13.5 KiB −7 B (−0.1%) 15.1 KiB ✅
Claude Thread snapshot wire 7.1 KiB 7.1 KiB +4 B (+0.1%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.4 KiB 6.4 KiB −11 B (−0.2%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 57.0 KiB 57.0 KiB 0 B (0.0%) 66.4 KiB ✅
Claude Live turn messages 9 9 0 (0.0%) 21 ✅

Baseline: 8518d0d · PR result: 1385889 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj

lukemaj commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Independent review at 1385889f508f0806c6b5357985c9ed5e1ef0e0a7

Verdict: approve. The fix is correct, bounded and fork-clean. No blocking findings; three small ones below.

What I verified

  • Own cancellation never re-reads. The guarantee holds, but for a different reason than the comment gives. Effect skips failure continuations while an interruptible fiber is interrupted (effect/internal/core.ts exitFailCause: while (fiber.interruptible && fiber._interruptedCause && cont) cont = fiber.getCont(contE)). So when the atom's fiber is cancelled (recompute, refresh, reconnect, unmount), the catchCauseIf handler never runs. I added two temporary tests in a throwaway worktree: a cancel caused by a reconnect (executions === 1, one interrupt), and registry.refresh of an in-flight read (executions === 2, not 3). Both pass with the fix.
  • Bounded to one retry. The handler returns the bare read, not the wrapped one, so a second interrupt surfaces. Each atom run (including withRefresh and refresh-trigger runs) gets at most one extra request.
  • The test discriminates. With runtime.ts at the merge base, reads again when the server interrupts a query this client did not cancel fails with All fibers interrupted without error. With the fix it passes. runtime.test.ts + pullRequests.test.ts: 77 passed, including my 2 temporary tests.
  • Every query family. The change applies to createEnvironmentQueryAtomFamily, so it covers pull requests (summary, stack, activity, detail, preview), review diff patches, server host resources and process history, and asset URLs, on web, desktop and mobile. That is right: for a read-only query, an interrupt-only failure has no useful meaning to show.
  • Fork rules. Both upstream files are allowlisted in scripts/fork-upstream-edits.txt and owned by query-interrupt-retry in docs/fork-features.md. scripts/fork-check.sh: fork-features: OK (15 features), fork-check: OK. CI is green.

Findings

  1. The comment's premise is too strong (runtime.ts, retryInterruptedRead doc). "An interrupt-only failure is the server's" is not always true. The client's own RpcClient resumes every in-flight request with Exit.interrupt when its session scope closes (RpcClient.ts, the finalizer that calls clearEntries(Exit.interrupt(parent.id))), and returns Effect.interrupt once isShutdown is set. A session teardown therefore also reaches the handler. The impact is harmless: the single retry either lands on a new session, fails with EnvironmentRpcUnavailableError, or is interrupted again and surfaces as before, and the connection-state change recomputes the atom anyway. Suggested wording: "The atom's own cancellation never reaches this handler, because Effect skips failure handlers on an interrupted fiber. An interrupt-only failure here comes from the server or from a torn-down RPC session; one more read is right for both."
  2. Missing negative test. The PR's headline risk is "no double request on a normal cancel", and no test covers it. Consider adding the refresh-during-read case: first execution Effect.never with an onInterrupt counter, then registry.refresh, then assert executions === 2 and interrupted === 1.
  3. Nit: title scope. The change is in client-runtime and affects mobile as well, so fix(client-runtime) describes it better than fix(web).

Would a server-side fix be smaller or better?

Not smaller. It would be better as a root-cause fix upstream. The race is in Effect's Cache.get: an entry whose lookup fiber is being interrupted stays in the map until the fiber's exit observer removes it, so a new caller joins it and receives the interrupt (Cache.ts get, the addObserver / exitHasInterrupts branch). A fix inside vendored Effect cannot be made here. The server-side alternative is a retry wrapper at each Cache.get site in PullRequestService: detail, activity, preview, diff, list, viewer, files-viewed and stats, roughly eight sites in a large upstream file. That only protects upgraded servers and only that service. The client fix is one site, covers all query families and servers that are not upgraded, and fits the fork's smallest-upstream-edit rule. It does not help installed mobile builds until they update. Recommended follow-up: report the join-on-interrupting-entry race to effect-smol, or to upstream T3 Code as a PullRequestService hardening. Do not add it to this PR.

Reviewed by Claude Opus 5.5 (Claude Code harness).

@lukemaj
lukemaj merged commit 864722e into main Sep 27, 2026
19 checks passed
@lukemaj
lukemaj deleted the fix/pr-panel-interrupted branch September 27, 2026 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR panel shows "All fibers interrupted without error" when a PR link opens

1 participant