Skip to content

fix(terminal): fork the Resize auth check onto the per-subscription queue#95

Open
anagnorisis2peripeteia wants to merge 1 commit into
openclaw:mainfrom
anagnorisis2peripeteia:fix/terminal-hub-resize-hol
Open

fix(terminal): fork the Resize auth check onto the per-subscription queue#95
anagnorisis2peripeteia wants to merge 1 commit into
openclaw:mainfrom
anagnorisis2peripeteia:fix/terminal-hub-resize-hol

Conversation

@anagnorisis2peripeteia

@anagnorisis2peripeteia anagnorisis2peripeteia commented Jul 17, 2026

Copy link
Copy Markdown

What Problem This Solves

Terminal frames from a client are processed on a single shared per-connection queue (all
multiplexed sessions on that WebSocket serialize through it). The Resize handler awaited
subscription.canInput() — an authorization DB round-tripinline on that shared queue. So
while one session's resize auth check was in flight, every other session's Input / Subscribe /
Stop / Ping frame was head-of-line-blocked behind it.

The Input path already avoids exactly this: it forks its own canInput() check onto a
per-subscription queue and returns from the shared queue immediately. Resize did not.

The change

Fork the canInput() check and the resize work onto subscription.inputQueue (the same
per-subscription queue Input uses), so the shared connection queue returns immediately. The forked
work re-checks subscription identity before touching the upstream (the same stale-subscription guard
the Input path uses).

Real-behaviour proof (before → after, real TerminalHub)

Captured by driving the real TerminalHub (the production class, unmocked): subscribe a session,
send a Resize whose canInput() authorization is held pending (standing in for a slow auth
round-trip), then a Ping on the shared per-connection queue. The timestamped transcripts below are
the actual harness output on each revision — the load-bearing line is when the Pong arrives
relative to the Resize authorization being released
:

BASE (origin/main @f4ef10b — Resize awaits canInput() inline on the shared queue)

t+14.050ms  client→hub   Subscribe(session, 120x34)
t+15.817ms  client→hub   Resize(100x40)  [its canInput() authorization is now PENDING]
t+16.214ms  client→hub   Ping(3 bytes)   [next frame on the SHARED connection queue]
            ---- Resize authorization released here (was pending; pongs so far = 0) ----
t+16.385ms  hub→upstream {"type":"resize","cols":100,"rows":40}
t+16.418ms  hub→client   Event resize cols=100 rows=40
t+16.447ms  hub→client   Pong            <- only AFTER the resize auth resolved
=> HEAD-OF-LINE BLOCKED: pongs received before the resize auth resolved = 0

HEAD (this PR @3b35686 — Resize forked onto the per-subscription inputQueue)

t+12.453ms  client→hub   Subscribe(session, 120x34)
t+13.448ms  client→hub   Resize(100x40)  [its canInput() authorization is now PENDING]
t+13.625ms  client→hub   Ping(3 bytes)   [next frame on the SHARED connection queue]
t+13.694ms  hub→client   Pong            <- answered WHILE the resize auth is still pending
            ---- Resize authorization released here (was pending; pongs so far = 1) ----
t+13.808ms  hub→upstream {"type":"resize","cols":100,"rows":40}
t+13.838ms  hub→client   Event resize cols=100 rows=40
=> NOT BLOCKED: pongs received before the resize auth resolved = 1

The one-line difference: on base the Pong is 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 + client resize event) on
both. Same interleaving, driven through the production TerminalHub on each revision.

A live WebSocket can't inject latency into the real authorization check, so this deterministic
real-TerminalHub reproduction is the strongest feasible proof for a head-of-line stall; it is
committed as the regression test below so it runs in CI.

Testing

pnpm check, pnpm build, pnpm exec wrangler deploy --dry-run, pnpm test all green (953 tests).
tests/terminal-hub.test.ts adds the two cases behind the transcript above: a slow Resize does not
block 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.

…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.
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 17, 2026
@clawsweeper

clawsweeper Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 8:29 PM ET / July 18, 2026, 00:29 UTC.

Summary
Moves Resize authorization and resize work off the shared TerminalHub connection queue onto each subscription’s input queue, with regressions for Ping progress during delayed authorization and successful resize delivery.

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.

  • Patch scope: 2 files affected; 134 added, 17 removed. The change is tightly limited to the TerminalHub Resize handler and its focused regression coverage.
  • Regression coverage: 2 terminal-hub cases added. The tests cover both the intended queue-progress behavior and preservation of successful resize delivery.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #88
Summary: This PR is the focused candidate fix for the Resize head-of-line blocking finding documented in the broader open terminal and lifecycle concurrency audit.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Next step before merge

  • [P2] No mechanical repair is identified; the remaining action is ordinary human review of a correct, proof-backed PR.

Security
Cleared: The diff only changes in-process terminal queueing and tests; it adds no dependency, credential, permission, artifact-download, workflow, or supply-chain surface.

Review details

Best 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 changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes directly inspectable timestamped before/after output from a real TerminalHub harness, showing Pong bypasses the delayed Resize authorization after the change while the resize still completes.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes directly inspectable timestamped before/after output from a real TerminalHub harness, showing Pong bypasses the delayed Resize authorization after the change while the resize still completes.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove merge-risk: 🚨 availability: Current PR review selected no merge-risk labels.

Label justifications:

  • P2: This fixes a bounded multiplexed-terminal responsiveness problem without evidence of data loss, security bypass, or an immediate core-runtime outage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes directly inspectable timestamped before/after output from a real TerminalHub harness, showing Pong bypasses the delayed Resize authorization after the change while the resize still completes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes directly inspectable timestamped before/after output from a real TerminalHub harness, showing Pong bypasses the delayed Resize authorization after the change while the resize still completes.
Evidence reviewed

What I checked:

  • Shared-queue stall fix: The PR changes the Resize branch to append authorization, identity validation, upstream resize, and client resize-event work to subscription.inputQueue, allowing the shared connection queue to advance while authorization is pending. (src/worker/terminal-hub.ts:359, 3b35686d1ed0)
  • Focused behavioral coverage: The branch adds one regression that holds Resize authorization pending while a subsequent Ping is processed, plus one that verifies an authorized Resize updates the upstream and client event path. (tests/terminal-hub.test.ts:2038, 3b35686d1ed0)
  • Inspectable real-behavior proof: The PR body contains timestamped before/after output from a real TerminalHub harness: base emits Pong only after delayed Resize authorization releases, while the PR head emits Pong before release and still applies the resize afterward. (3b35686d1ed0)
  • Related canonical audit: The open concurrency audit identifies this exact shared-queue Resize stall as Finding 4 and links this PR as its focused candidate fix.

Likely related people:

  • unknown: The available review context identifies the affected TerminalHub path but does not expose a reliable current-main blame/log trail; the PR author is intentionally not used as an owner solely for proposing this branch. (role: current-main feature-history owner; confidence: low; files: src/worker/terminal-hub.ts, tests/terminal-hub.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (3 earlier review cycles)
  • reviewed 2026-07-17T23:00:45.870Z sha 3b35686 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-17T23:10:51.480Z sha 3b35686 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T00:06:36.176Z sha 3b35686 :: needs real behavior proof before merge. :: none

@anagnorisis2peripeteia

Copy link
Copy Markdown
Author

The previous review could not inspect the branch — its sandbox failed to initialize (bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted), so it read no source, git history, or the linked proof transcript. Requesting a fresh review on a working runner.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 17, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 17, 2026
@anagnorisis2peripeteia

Copy link
Copy Markdown
Author

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 clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 24, 2026

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant