fix(terminal): fork the Resize auth check onto the per-subscription queue#95
Conversation
…ueue The Resize handler awaited subscription.canInput() (a DB authorization round-trip) inline on the shared per-connection message queue, so one session's resize auth check head-of-line-blocked Input/Subscribe/Stop for every other multiplexed session on the connection. Fork the canInput() check and the resize onto the per-subscription inputQueue, exactly as the Input path already does, so the shared queue returns immediately. Includes the same stale-subscription identity guard the Input path uses.
|
Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 8:29 PM ET / July 18, 2026, 00:29 UTC. Summary Reproducibility: yes. at source level: the added real-TerminalHub harness deterministically holds Resize authorization pending, sends Ping on the same connection queue, and observes whether Pong arrives before authorization is released. The PR body provides matching before/after live output for that path. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Next step before merge
Security Review detailsBest possible solution: Land the narrow per-subscription Resize queueing change after a human reviewer confirms it preserves the TerminalHub ordering and lifecycle contract alongside the existing Input path. Do we have a high-confidence way to reproduce the issue? Yes, at source level: the added real-TerminalHub harness deterministically holds Resize authorization pending, sends Ping on the same connection queue, and observes whether Pong arrives before authorization is released. The PR body provides matching before/after live output for that path. Is this the best way to solve the issue? Yes. Reusing the established per-subscription inputQueue pattern is the narrowest maintainable solution because it removes the shared-queue await while preserving per-subscription ordering and the stale-subscription identity guard. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model internal, reasoning high; reviewed against 2cb0c9ac0892. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles) |
|
The previous review could not inspect the branch — its sandbox failed to initialize ( @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
The fresh review started (23:14 UTC) but its lease expired ~20 min ago without a verdict landing — the run appears to have stalled. Requesting another pass. @clawsweeper re-review |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(terminal): fork the Resize auth check onto the per-subscription queue This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
What Problem This Solves
Terminal frames from a client are processed on a single shared per-connection
queue(allmultiplexed sessions on that WebSocket serialize through it). The
Resizehandlerawaitedsubscription.canInput()— an authorization DB round-trip — inline on that shared queue. Sowhile one session's resize auth check was in flight, every other session's
Input/Subscribe/Stop/Pingframe was head-of-line-blocked behind it.The
Inputpath already avoids exactly this: it forks its owncanInput()check onto aper-subscription queue and returns from the shared queue immediately.
Resizedid not.The change
Fork the
canInput()check and the resize work ontosubscription.inputQueue(the sameper-subscription queue
Inputuses), so the shared connection queue returns immediately. The forkedwork re-checks subscription identity before touching the upstream (the same stale-subscription guard
the
Inputpath uses).Real-behaviour proof (before → after, real
TerminalHub)Captured by driving the real
TerminalHub(the production class, unmocked): subscribe a session,send a
ResizewhosecanInput()authorization is held pending (standing in for a slow authround-trip), then a
Pingon the shared per-connection queue. The timestamped transcripts below arethe actual harness output on each revision — the load-bearing line is when the
Pongarrivesrelative to the
Resizeauthorization being released:BASE (
origin/main@f4ef10b— ResizeawaitscanInput()inline on the shared queue)HEAD (this PR @
3b35686— Resize forked onto the per-subscriptioninputQueue)The one-line difference: on base the
Pongis emitted after the resize's auth round-trip(t+16.447, past the release); on HEAD it is emitted before it (t+13.694, while still pending) — and
the resize itself still applies correctly on release (upstream
resize+ clientresizeevent) onboth. Same interleaving, driven through the production
TerminalHubon each revision.A live WebSocket can't inject latency into the real authorization check, so this deterministic
real-
TerminalHubreproduction is the strongest feasible proof for a head-of-line stall; it iscommitted as the regression test below so it runs in CI.
Testing
pnpm check,pnpm build,pnpm exec wrangler deploy --dry-run,pnpm testall green (953 tests).tests/terminal-hub.test.tsadds the two cases behind the transcript above: a slow Resize does notblock a following Ping (the fix — pins pongs-before-release = 1); and an authorized Resize applies the
new size and notifies both client and upstream (pins the resize still lands). Mutation of the changed
range scores 76%. A concurrency lint of the diff is clean.
Related: FINDING 4 of #88.