fix(terminal): don't let a stale upstream handler evict a live resubscription

[anagnorisis2peripeteia]

What Problem This Solves

The terminal hub keys its live subscriptions by session id. When an upstream connection closes or
errors, the close / error handlers removed the subscription with subscriptions.delete(id) — by
id alone. So an unsubscribe → immediate resubscribe of the same session id could let the old
upstream's deferred close (or error) event fire after the new subscription was already stored,
and evict it: releasing the live subscription's input state and telling the client its session had
closed, while the connection stayed up.

The hub's own input loop already guards against exactly this with an identity re-check
(subscriptions.get(id) !== subscription); the two upstream lifecycle handlers did not.

The change

Re-check subscription identity before acting in the three paths that tear a subscription down — the
close handler, the error handler, and the view-revocation path (revokeView) — matching the
input loop:

if (subscriptions.get(id) !== activeSubscription) return; // a newer subscription owns this id now
subscriptions.delete(id);
this.dependencies.releaseInputState(id);

If a newer subscription has taken over the id, the stale handler stops: it no longer evicts the live
subscription, releases its input state, marks the session detached, or sends the client a closed /
error / share revoked event. Each handler still cleans up its own subscription (pending input
acknowledgements, its view-check interval, its own upstream). revokeView is included because a
canView() from a prior interval tick can still be in flight and resolve after the id was
re-subscribed — clearInterval does not cancel that pending promise.

Real Worker WebSocket proof (before → after)

Driven end-to-end against the real worker under wrangler dev (workerd): a real WebSocket client
connects to /api/terminal/ws (real crabbox_session cookie auth), and the subscription's upstream
is a real ws:// server the harness controls. The production sequence — subscribe → unsubscribe →
immediate same-id resubscribe — opens two real upstream connections; the old upstream's close then
fires deferred (a real WS close handshake) after the resubscription:

BASE (origin/main, no identity guard)
  client 'closed' events for the live resubscription: 1
  => EVICTED: the old upstream's close closed the live resubscription

HEAD (this PR, identity guard on close/error/revokeView)
  client 'closed' events for the live resubscription: 0
  => PRESERVED: the resubscription survived the old upstream's close

Reproducible transcript + setup: https://gist.github.com/anagnorisis2peripeteia/a401c163be272045571337a9419c37a8

Deterministic reproduction (unit)

The same interleaving through the real TerminalHub, which additionally makes the double-release
observable:

BASE — no identity guard
  releaseInputState calls=2, spurious client 'closed' after resubscribe=1  => EVICTED
HEAD — identity guard
  releaseInputState calls=1, spurious client 'closed' after resubscribe=0  => PRESERVED

(The extra releaseInputState on base is the second, wrong release — of the live subscription B's
input state.)

Testing

pnpm check (tsc + oxlint + oxfmt), pnpm build, pnpm exec wrangler deploy --dry-run, pnpm test
all green. tests/terminal-hub.test.ts adds four cases covering the close and error handlers on both
paths: a stale close and a stale error must NOT evict a live resubscription; a normal close and a
normal error still tear down the current subscription and notify the client. Mutation testing of the
close/error guard conditions scores 100%. revokeView gets the identical one-line guard; its stale
trigger (an in-flight canView() resolving after the swap) isn't deterministically unit-testable
against the 5s view-check interval, so it relies on that same proven pattern rather than a flaky
fake-timer test. A concurrency lint of the diff is clean.

Related: FINDING 3 of #88.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 7:16 PM ET / 23:16 UTC.

Summary
The PR prevents stale terminal upstream close, error, and view-revocation callbacks from deleting a newer same-session subscription, and adds close/error regression tests.

Reproducibility: no. high-confidence current-main reproduction was established because the local read-only inspection sandbox failed before current source could be exercised. The supplied branch includes a focused source-level reproduction and linked real-worker before/after transcript for the reported interleaving.

Review metrics: 1 noteworthy metric.

• Affected surface: 2 files affected: 20 production lines added, 210 test lines added, 3 lines removed. The production change is narrowly scoped while the regression suite documents both stale and current lifecycle outcomes.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #88
Summary: This PR is the focused candidate fix for the terminal-hub stale-resubscription finding documented in the broader lifecycle-race report.

Members:

• canonical: Several latent session/workspace lifecycle concurrency bugs (native-VNC credential leak, run-status stomp, terminal-hub races) #88 - The report explicitly lists this terminal-hub race and links this PR as its dedicated candidate fix.

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 ✨ media proof bonus
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:

• Refresh the branch against current main and rerun the focused terminal-hub tests on the merged result.

Risk before merge

• [P1] The patch changes terminal subscription teardown, so it should be refreshed against current main and reviewed for all asynchronous close/error/revocation orderings before merge; the supplied state still reports an unstable mergeability calculation.

Maintainer options:

1. Refresh and verify teardown ordering (recommended)
   Rebase or merge-test against current main, then confirm stale close, error, and revocation callbacks cannot release or notify the newer subscription.
2. Pause for terminal-owner review
   Hold the PR if current-main teardown ownership has materially changed and the identity guard needs to be redesigned around the newer subscription lifecycle.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Refresh the branch against current main and rerun the focused terminal-hub stale/current teardown tests before merging.

Next step before merge

• [P2] No discrete automated repair is established; the remaining action is maintainer review of the current-main terminal teardown ordering and merge result.

Security
Cleared: The supplied diff is limited to terminal lifecycle handling and tests, with no dependency, workflow, secret, permission, or package-resolution change.

Review details

Best possible solution:

Retain the identity-based teardown guard if it cleanly rebases onto current main, then validate stale and current subscription teardown behavior on the merged result before landing.

Do we have a high-confidence way to reproduce the issue?

No high-confidence current-main reproduction was established because the local read-only inspection sandbox failed before current source could be exercised. The supplied branch includes a focused source-level reproduction and linked real-worker before/after transcript for the reported interleaving.

Is this the best way to solve the issue?

Yes, conditionally: comparing the stored subscription object before destructive teardown is the narrowest way to preserve a newer same-id subscription. It should still be validated against the current-main three-way merge because the branch base is behind.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against 2cb0c9ac0892.

Label changes

Label justifications:

• P2: This fixes a bounded terminal session-lifecycle race that can incorrectly close an active resubscription.
• merge-risk: 🚨 session-state: A teardown-path change can release input state or detach the wrong live terminal subscription if ordering is wrong after integration.
• 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 (linked_artifact): The PR supplies a linked real Worker WebSocket before/after transcript for the stale-close interleaving, and the supplied review context marks the proof sufficient; any future proof should continue to redact cookies, endpoints, and other private runtime data.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR supplies a linked real Worker WebSocket before/after transcript for the stale-close interleaving, and the supplied review context marks the proof sufficient; any future proof should continue to redact cookies, endpoints, and other private runtime data.

Evidence reviewed

What I checked:

• Proposed lifecycle guard: The PR head adds an identity comparison before deleting a subscription or releasing input state in the view-revocation and upstream lifecycle teardown paths, preventing an old callback from acting on a newer subscription with the same session id. (src/worker/terminal-hub.ts:586, 23cb41d44279)
• Focused regression coverage: The branch adds stale-close, stale-error, current-close, and current-error tests around same-id resubscription behavior, including the observable closed-event and input-state-release outcomes. (tests/terminal-hub.test.ts:2039, 23cb41d44279)
• Real behavior proof: The PR body and follow-up comment link a real Worker WebSocket transcript comparing the base behavior, which emits a spurious closed event, with the PR behavior, which preserves the new subscription. The item is also marked proof sufficient in the supplied review context. (23cb41d44279)
• Related canonical report: The linked umbrella report explicitly identifies this terminal-hub race as its third finding and records this PR as the focused candidate fix. (23cb41d44279)
• Review-refresh requirement: The supplied GitHub state reports PR base 656c3514fcd72be13fadcedda6285177c3f7a3a8 while current main is newer and mergeability is still unstable; this supports a maintainer refresh rather than an implementation-close decision. (2cb0c9ac0892)

Likely related people:

• unknown: The available read-only shell failed before terminal-hub blame and history could be inspected, and the PR author should not be treated as an owner solely for proposing this patch. (role: current-main terminal 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 (2 earlier review cycles)

• reviewed 2026-07-17T21:57:02.359Z sha 23cb41d :: needs real behavior proof before merge. :: none
• reviewed 2026-07-17T22:34:14.503Z sha 23cb41d :: needs maintainer review before merge. :: none

[anagnorisis2peripeteia]

@clawsweeper re-review

Added real Worker WebSocket proof: driven end-to-end against the real worker under wrangler dev (workerd), a real WS client on /api/terminal/ws (real crabbox_session cookie auth) with a real ws:// upstream. The production sequence subscribe → unsubscribe → immediate same-id resubscribe opens two real upstream connections; the old upstream's deferred close then evicts the live resubscription on origin/main (client receives a spurious closed event) but not on this PR (no closed event). Reproducible transcript + setup in the linked gist.

[clawsweeper]

🦞🧹
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:

• State: Complete
• Detail: The durable review comment was published and its verdict was queued for target-serialized routing.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/29620254962
• Updated: 2026-07-17T23:19:21.214Z