fix(runtime): isolate one pane's replay from the handle-gap drain

One store write releases every due pane, and the drain runs synchronously inside
a zustand subscriber. `waiter.run()` was unguarded, so a single pane's replay
reached two things it has no business touching:

  - the throw escapes out of `useAppStore.setState`, meaning the mirror apply
    that published the PTY handle throws at its own call site;
  - every pane queued behind the thrower is stranded — waiter still parked,
    deadline still armed — and then decides on a connection whose evidence
    landed long ago.

The deadline path fans out the same way, so a throwing replay also escaped the
timer callback.

Reachable: `resumeSleepingAgentSessionsForWorktree` reaches `state.createTab`
with no guard of its own. The panes in a drain are strangers to each other and
to the frame that released them; none of them should be able to see another's
failure.

The new tests live in their own file because
host-mirror-handle-gap-resume.test.ts drives the waiter through the real resume
sweep and so cannot choose what a replay DOES. Note for anyone extending that
file: per its header, "did the waiter release" is not an observable here — a
spurious release is re-parked immediately and reads identically one tick later.
These tests assert on timer count and on the deadline instead.

Also records two findings next to the code, so they are not rediscovered:
`expiredGenerationByPane` is never pruned for a removed environment (bounded and
inert, since removal advances the generation, but it does not drain — and a
DIFFERENT leak in that same map is being fixed concurrently, so reconcile rather
than patch around it); and sustained reconnect churn holding a pane parked
indefinitely is CORRECT, not the latch-that-never-releases defect, because under
churn liveness genuinely is unverifiable and ssh-execution-boundary.md forbids
resolving that to `exited`. It has the shape of the defect and will eventually
be "fixed" by someone who does not know that.

Mutation: dropping the guard kills exactly the three new assertions and leaves
all twelve existing waiter tests passing.
This commit is contained in:
Neil
2026-09-10 17:25:48 -07:00
parent bf497512d8
commit 15f3401415
2 changed files with 137 additions and 2 deletions
@@ -0,0 +1,112 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { useAppStore } from '@/store'
import { clearRuntimeEnvironmentConnectionGenerationsForTests } from '@/store/slices/runtime-status'
import {
HOST_MIRROR_HANDLE_GAP_DEADLINE_MS,
countParkedHostMirrorHandleGapPanesForTests,
hasHostMirrorHandleWaitExpired,
parkUntilHostMirrorHandleLands,
resetHostMirrorHandleGapWaitsForTests
} from './host-mirror-handle-gap-wait'
// What this file pins, and why it is separate from host-mirror-handle-gap-resume.test.ts: that file
// drives the waiter through the real resume sweep, so it cannot choose what a replay DOES. These
// tests park with a `run` of their own to exercise the drain itself — the loop that releases every
// due pane from one store write, running synchronously inside a zustand subscriber. The panes in
// that loop are strangers to each other and the store write that triggered it is a stranger to all
// of them, so one pane's replay must not be able to reach either.
const ENVIRONMENT_ID = 'env-handle-gap-drain'
const WORKTREE_ID = 'repo-1::/workspace/repo'
const FIRST_TAB_ID = 'web-terminal-host-tab-1'
const SECOND_TAB_ID = 'web-terminal-host-tab-2'
const initialAppStoreState = useAppStore.getState()
function seedRows(): void {
useAppStore.setState({
ptyIdsByTabId: {},
tabsByWorktree: {
[WORKTREE_ID]: [
{ id: FIRST_TAB_ID, title: 'one' },
{ id: SECOND_TAB_ID, title: 'two' }
]
}
} as never)
}
/** The host publishes both panes' PTY handles on one frame: both waiters come due together. */
function publishBothHandles(): void {
useAppStore.setState({
ptyIdsByTabId: { [FIRST_TAB_ID]: ['pty-1'], [SECOND_TAB_ID]: ['pty-2'] }
} as never)
}
describe('host-mirror handle-gap drain', () => {
beforeEach(() => {
vi.useFakeTimers()
resetHostMirrorHandleGapWaitsForTests()
clearRuntimeEnvironmentConnectionGenerationsForTests()
seedRows()
})
afterEach(() => {
resetHostMirrorHandleGapWaitsForTests()
clearRuntimeEnvironmentConnectionGenerationsForTests()
useAppStore.setState(initialAppStoreState, true)
vi.useRealTimers()
})
// The drain runs inside `useAppStore.subscribe`, so an unguarded throw from one pane's replay
// leaves the store write that published the handle throwing at its own call site — the mirror
// apply path, which has nothing to do with this pane. `resumeSleepingAgentSessionsForWorktree`
// reaches `state.createTab` with no guard of its own, so the throw is reachable.
it('does not let one panes replay throw out of the store write that released it', () => {
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, FIRST_TAB_ID, () => {
throw new Error('replay blew up')
})
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, SECOND_TAB_ID, () => {})
expect(() => publishBothHandles()).not.toThrow()
})
// Same write, the other victim: the panes in a drain are strangers. A replay that throws must not
// strand every pane queued behind it — a stranded pane holds its park until its own deadline and
// then decides on a connection whose evidence has long since landed.
it('releases every other due pane when one panes replay throws', () => {
const secondReplay = vi.fn()
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, FIRST_TAB_ID, () => {
throw new Error('replay blew up')
})
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, SECOND_TAB_ID, secondReplay)
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(2)
try {
publishBothHandles()
} catch {
// The assertion is about the second pane, not about who swallowed the throw.
}
expect(secondReplay).toHaveBeenCalledTimes(1)
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
// Both deadlines are cancelled, so neither pane can record an expiry it did not earn.
expect(vi.getTimerCount()).toBe(0)
vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS * 2)
expect(hasHostMirrorHandleWaitExpired(ENVIRONMENT_ID, FIRST_TAB_ID)).toBe(false)
expect(hasHostMirrorHandleWaitExpired(ENVIRONMENT_ID, SECOND_TAB_ID)).toBe(false)
})
// The deadline path fans out the same way: one expiring pane's replay must not keep another pane
// from recording its own verdict on the same connection.
it('records the expiry of a pane whose replay throws and still frees the pane', () => {
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, FIRST_TAB_ID, () => {
throw new Error('replay blew up')
})
expect(() => vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS)).not.toThrow()
expect(hasHostMirrorHandleWaitExpired(ENVIRONMENT_ID, FIRST_TAB_ID)).toBe(true)
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
expect(vi.getTimerCount()).toBe(0)
})
})
@@ -20,6 +20,12 @@ import { WEB_SESSION_TAB_RPC_TIMEOUT_MS } from '@/runtime/web-session-tab-rpc-ti
* recovery: a resume after a bounded wait is defensible, an indefinite hold
* is the latch-that-never-releases defect. A reconnect bumps the connection
* generation and arms a fresh wait.
*
* Sustained reconnect churn can therefore hold a pane parked indefinitely: each reconnect voids the
* in-flight verdict and grants a fresh full budget. That is CORRECT, not the defect above. Under
* churn the pane's liveness genuinely is unverifiable, and `docs/reference/ssh-execution-boundary.md`
* forbids resolving unverifiable to `exited`. It has the shape of a latch that never releases, so
* do not "fix" it by letting a verdict from one connection decide another — that is #19735.
*/
export const HOST_MIRROR_HANDLE_GAP_DEADLINE_MS = WEB_SESSION_TAB_RPC_TIMEOUT_MS
@@ -38,7 +44,16 @@ type HandleGapStoreState = Pick<
>
const waitersByPane = new Map<string, HandleGapWaiter>()
/** Connection generation whose wait already expired for the pane. */
/**
* Connection generation whose wait already expired for the pane.
*
* KNOWN LEAK, not fixed: entries are pruned only by `recordExpiredWait`, and only for the
* environment doing the recording. An environment that is removed and never expires another pane
* keeps its rows for the life of the session. Bounded by panes x environments and inert — a stale
* row cannot match, because removing an environment advances its connection generation — but it
* does not drain. Another agent has a separate fix in flight for a DIFFERENT leak in this same map
* (pruning on tab death); reconcile with that change rather than patching around it.
*/
const expiredGenerationByPane = new Map<string, number>()
let unsubscribeStore: (() => void) | null = null
@@ -82,7 +97,15 @@ function releaseWaiter(key: string): void {
clearTimeout(waiter.deadline)
waitersByPane.delete(key)
stopStoreSubscriptionIfIdle()
waiter.run()
try {
waiter.run()
} catch (error) {
// Why: one write releases every due pane, and the drain runs inside the store subscriber. The
// panes in it are strangers to each other and to the frame that published the handle, so an
// unguarded replay throw both strands every pane queued behind it and surfaces at the mirror
// apply's own `setState`. The pane is already unparked here; only its replay is lost.
console.warn('[host-mirror-handle-gap] parked resume replay failed:', error)
}
}
function waiterIsReleased(waiter: HandleGapWaiter, state: HandleGapStoreState): boolean {