mirror of
https://github.com/stablyai/orca.git
synced 2026-09-26 16:02:43 +00:00
fix(runtime): release a handle-gap pane once per store write
`releaseDueWaiters` snapshotted the due KEYS and then re-looked-up each one. A replay earlier in the loop writes to the store — the sweep reaches `createTab` and `clearSleepingAgentSession` — and zustand notifies re-entrantly with no queue, so the nested pass can release and re-park a pane still queued in the outer loop. The outer `releaseWaiter(key)` then found the re-park, cleared its brand-new deadline and replayed it a second time off one store write, handing that pane another full budget. That is the extension `parkUntilHostMirrorHandleLands` already refuses to grant a re-park, arriving through a different door. The direction is conservative (hold longer, never resume early), which is why no outcome assertion could see it; only the replay count separates the two implementations. Snapshot the waiter alongside its key and release only while the map still holds that same waiter. Also folds host-mirror-handle-gap-replay-containment.test.ts into the drain suite, since the guard it pins is the one this commit extends. It was the same fix imported twice: its deadline case is a strict subset of the drain suite's, its store-write case differs only by also asserting that later store listeners still run, and one mutation — rethrowing from the replay catch — killed all five cases across both files. Its fixture also seeded no layout bindings and used tab ids `isWebTerminalSurfaceTabId` rejects, so those panes could not have reached the park path it claimed to exercise. The unique assertion moves across; the file goes. Killed by `releases a pane once per store write even when an earlier replay re-enters the drain`: 2 replay calls instead of 1 without the identity guard.
This commit is contained in:
@@ -15,6 +15,11 @@ import {
|
||||
// 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.
|
||||
//
|
||||
// Both release paths are here on purpose: the store-write drain and the deadline both funnel into
|
||||
// `releaseWaiter`, and a guard added to one is easy to forget on the other. One mutation —
|
||||
// rethrowing from that catch — kills both cases below, which is the point: they are the two entry
|
||||
// points, not two behaviours.
|
||||
|
||||
const ENVIRONMENT_ID = 'env-handle-gap-drain'
|
||||
const WORKTREE_ID = 'repo-1::/workspace/repo'
|
||||
@@ -57,56 +62,55 @@ function seedRows(): void {
|
||||
function publishBothHandles(): void {
|
||||
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the seeded slice names only the store fields this suite drives; the rest of AppState keeps its defaults.
|
||||
useAppStore.setState({
|
||||
ptyIdsByTabId: { [FIRST_TAB_ID]: ['pty-1'], [SECOND_TAB_ID]: ['pty-2'] }
|
||||
ptyIdsByTabId: {
|
||||
[FIRST_TAB_ID]: [`remote:${ENVIRONMENT_ID}@@term_1`],
|
||||
[SECOND_TAB_ID]: [`remote:${ENVIRONMENT_ID}@@term_2`]
|
||||
}
|
||||
} as never)
|
||||
}
|
||||
|
||||
describe('host-mirror handle-gap drain', () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
// The replays below throw on purpose; the module logs and swallows, which is the behaviour
|
||||
// under test, so the log itself is noise.
|
||||
vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
resetHostMirrorHandleGapWaitsForTests()
|
||||
clearRuntimeEnvironmentConnectionGenerationsForTests()
|
||||
seedRows()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
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 pane’s replay throw out of the store write that released it', () => {
|
||||
// The drain runs inside `useAppStore.subscribe`, and zustand notifies listeners in a plain loop
|
||||
// with no queue, so an unguarded throw from one pane's replay reaches three strangers at once:
|
||||
// the `setState` that published the handle (the mirror apply, which has nothing to do with this
|
||||
// pane), every sibling pane the same frame made due, and every listener registered after this
|
||||
// module's. `resumeSleepingAgentSessionsForWorktree` reaches `state.createTab` with no guard of
|
||||
// its own, so the throw is reachable.
|
||||
it('does not let one pane’s replay throw reach the store write, its siblings, or later listeners', () => {
|
||||
const siblingReplay = vi.fn()
|
||||
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 pane’s 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)
|
||||
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, SECOND_TAB_ID, siblingReplay)
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(2)
|
||||
|
||||
try {
|
||||
publishBothHandles()
|
||||
} catch {
|
||||
// The assertion is about the second pane, not about who swallowed the throw.
|
||||
}
|
||||
// Registered after this module's subscription, so it is notified after the drain.
|
||||
const laterListener = vi.fn()
|
||||
const unsubscribe = useAppStore.subscribe(laterListener)
|
||||
|
||||
expect(secondReplay).toHaveBeenCalledTimes(1)
|
||||
expect(() => publishBothHandles()).not.toThrow()
|
||||
unsubscribe()
|
||||
|
||||
expect(siblingReplay).toHaveBeenCalledTimes(1)
|
||||
expect(laterListener).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)
|
||||
@@ -115,8 +119,33 @@ describe('host-mirror handle-gap drain', () => {
|
||||
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.
|
||||
// The drain is re-entrant: a replay writes to the store, zustand notifies with no queue, and the
|
||||
// nested pass drains the same map the outer loop is still walking. One store write must still
|
||||
// mean one release per pane — a second release clears the re-park's fresh deadline and replays
|
||||
// it again, granting a budget extension the re-park path deliberately refuses.
|
||||
it('releases a pane once per store write even when an earlier replay re-enters the drain', () => {
|
||||
const siblingReplay = vi.fn(() => {
|
||||
// What the real replay does when the sweep still finds the pane undecided.
|
||||
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, SECOND_TAB_ID, siblingReplay)
|
||||
})
|
||||
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, FIRST_TAB_ID, () => {
|
||||
// `resumeSleepingAgentSessionsForWorktree` reaches `createTab` and
|
||||
// `clearSleepingAgentSession`, so a replay writing mid-drain is the ordinary case.
|
||||
useAppStore.setState({ tabsByWorktree: { ...useAppStore.getState().tabsByWorktree } })
|
||||
})
|
||||
parkUntilHostMirrorHandleLands(ENVIRONMENT_ID, WORKTREE_ID, SECOND_TAB_ID, siblingReplay)
|
||||
|
||||
publishBothHandles()
|
||||
|
||||
expect(siblingReplay).toHaveBeenCalledTimes(1)
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(1)
|
||||
// And the re-park it made owns exactly one timer, not one per spurious release.
|
||||
expect(vi.getTimerCount()).toBe(1)
|
||||
})
|
||||
|
||||
// The other entry into `releaseWaiter`. Here the throw would escape the timer callback instead of
|
||||
// the store write, and the verdict must still be recorded — a pane whose replay failed has still
|
||||
// used up its budget, and dropping the verdict re-parks it on a fresh one forever.
|
||||
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')
|
||||
|
||||
@@ -1,83 +0,0 @@
|
||||
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,
|
||||
parkUntilHostMirrorHandleLands,
|
||||
resetHostMirrorHandleGapWaitsForTests
|
||||
} from './host-mirror-handle-gap-wait'
|
||||
|
||||
// What this pins: the parked replay is `resumeSleepingAgentSessionsForWorktree`, a large
|
||||
// synchronous sweep, and it is released from inside `useAppStore.subscribe`. Zustand notifies
|
||||
// listeners in a plain loop, so a replay that throws escapes the `setState` that triggered it:
|
||||
// the listeners registered after this module never see the write, and every sibling pane the
|
||||
// same mirror frame made due is left parked. The waiter's own state is torn down before `run`,
|
||||
// so containing the throw holds nothing back.
|
||||
|
||||
const initialAppStoreState = useAppStore.getState()
|
||||
const ENV_ID = 'env-gap-containment'
|
||||
|
||||
function seedTwoMirroredPanes(): void {
|
||||
useAppStore.setState({
|
||||
tabsByWorktree: {
|
||||
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the fixture carries the fields this suite drives; the cast only supplies the rest of the declared shape.
|
||||
wt: [
|
||||
{ id: 'tab-a', title: 'a', ptyId: null },
|
||||
{ id: 'tab-b', title: 'b', ptyId: null }
|
||||
] as never
|
||||
},
|
||||
ptyIdsByTabId: {}
|
||||
})
|
||||
}
|
||||
|
||||
describe('host-mirror handle-gap replay containment', () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
useAppStore.setState(initialAppStoreState, true)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
resetHostMirrorHandleGapWaitsForTests()
|
||||
clearRuntimeEnvironmentConnectionGenerationsForTests()
|
||||
useAppStore.setState(initialAppStoreState, true)
|
||||
vi.useRealTimers()
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it('a replay that throws neither aborts the store write nor strands its sibling panes', () => {
|
||||
vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
seedTwoMirroredPanes()
|
||||
const siblingReplay = vi.fn()
|
||||
parkUntilHostMirrorHandleLands(ENV_ID, 'wt', 'tab-a', () => {
|
||||
throw new Error('replay blew up')
|
||||
})
|
||||
parkUntilHostMirrorHandleLands(ENV_ID, 'wt', 'tab-b', siblingReplay)
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(2)
|
||||
|
||||
// A store subscriber registered after this module's, so it is notified after the drain.
|
||||
const laterSubscriber = vi.fn()
|
||||
const unsubscribe = useAppStore.subscribe(laterSubscriber)
|
||||
|
||||
// One mirror frame lands both handles, making both waiters due on a single store write.
|
||||
expect(() =>
|
||||
useAppStore.setState({ ptyIdsByTabId: { 'tab-a': ['pty-a'], 'tab-b': ['pty-b'] } })
|
||||
).not.toThrow()
|
||||
unsubscribe()
|
||||
|
||||
expect(siblingReplay).toHaveBeenCalledTimes(1)
|
||||
expect(laterSubscriber).toHaveBeenCalledTimes(1)
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
|
||||
})
|
||||
|
||||
it('a replay that throws on the deadline path does not escape the timer', () => {
|
||||
vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
seedTwoMirroredPanes()
|
||||
parkUntilHostMirrorHandleLands(ENV_ID, 'wt', 'tab-a', () => {
|
||||
throw new Error('replay blew up')
|
||||
})
|
||||
|
||||
expect(() => vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS)).not.toThrow()
|
||||
expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0)
|
||||
})
|
||||
})
|
||||
@@ -77,6 +77,17 @@ const waitersByPane = new Map<string, HandleGapWaiter>()
|
||||
* about its predecessor. Pinned as class D in host-mirror-handle-gap-verdict-union.test.ts; do not
|
||||
* delete that case.
|
||||
*
|
||||
* KNOWN LEAK, deliberately not drained: a verdict whose row the host retracts for good on an
|
||||
* environment that stays paired and never records again. The generation has not moved, teardown
|
||||
* never fires, the retracted row can never publish a handle, and the tab-death rule only runs from
|
||||
* inside a later recording. That entry outlives the session, and because
|
||||
* `stopStoreSubscriptionIfIdle` counts verdicts, so does the store subscription — a no-op rescan on
|
||||
* every write to the two slices below. It cannot answer (a retracted row reads `paneBinding: ''`,
|
||||
* which the read below refuses), so it costs work, not correctness. The obvious drain — drop a
|
||||
* verdict whose binding no longer matches — is NOT safe: it would break the genuine reattach, where
|
||||
* the binding goes away and comes back and the verdict must still answer
|
||||
* (host-mirror-handle-gap-verdict-union.test.ts, "answers for a genuine reattach").
|
||||
*
|
||||
* The PUBLISHED HANDLE drain does not close that class and must not be read as closing it: it
|
||||
* needs the row to stay published throughout, and that class needs the row to go away. Read-time
|
||||
* identity separates two panes behind one tab id; the drain separates two gaps on one pane. They
|
||||
@@ -248,14 +259,22 @@ function waiterIsReleased(waiter: HandleGapWaiter, state: HandleGapStoreState):
|
||||
function releaseDueWaiters(state: HandleGapStoreState): void {
|
||||
// Why: drain from a snapshot — a replay can re-park the pane, and that new
|
||||
// waiter belongs to the next store write, not this one.
|
||||
const dueKeys: string[] = []
|
||||
const due: [string, HandleGapWaiter][] = []
|
||||
for (const [key, waiter] of waitersByPane) {
|
||||
if (waiterIsReleased(waiter, state)) {
|
||||
dueKeys.push(key)
|
||||
due.push([key, waiter])
|
||||
}
|
||||
}
|
||||
for (const key of dueKeys) {
|
||||
releaseWaiter(key)
|
||||
for (const [key, waiter] of due) {
|
||||
// Why the snapshot holds the WAITER and not just its key: a replay earlier in this loop writes
|
||||
// to the store (the sweep reaches `createTab` and `clearSleepingAgentSession`), and zustand
|
||||
// notifies re-entrantly with no queue, so the nested pass can release and re-park a pane still
|
||||
// queued here. Releasing by key would then find the re-park, clear its brand-new deadline and
|
||||
// replay it a second time off one store write — handing that pane another full budget, which
|
||||
// is exactly the extension `parkUntilHostMirrorHandleLands` refuses to grant a re-park.
|
||||
if (waitersByPane.get(key) === waiter) {
|
||||
releaseWaiter(key)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user