From 4f775d68c77486b4c2dbdb46a5d3deff83590904 Mon Sep 17 00:00:00 2001 From: Neil Date: Tue, 8 Sep 2026 03:21:49 -0700 Subject: [PATCH] fix(runtime): release a parked bootstrap on the mirror's next frame The previous commit released the latch on a returned failure, but a create that *succeeded* with no mirrored row yet had no release at all: the row-conditional check was the only exit for the success path, so a host that accepted the tab while the mirror never got a frame held the latch until environment teardown and suppressed every later auto-seed for the worktree. Give the latch two phases. `creating` blocks other closures while the RPC is in flight. A success with no row is parked as `awaiting-mirror` instead of held, and the next frame the mirror accepts for that worktree releases it -- that frame is the mirror's answer either way (a row now exists and the predicate declines on its own, or the host genuinely has no terminal and a retry is right). A create still in flight keeps its claim: releasing it on a frame would reopen the re-armed-closure race the latch exists to close. Also correct the closure-flag comment: `requestedInitialTerminal` is set only after the dispatch resolves, so a thrown create never sets it and a later frame may retry. The flag records that this subscription already owned a create; it never described a failed one. Regressions, each mutation-tested against its own term: - success with no row, then the mirror's empty answer, then a fresh closure -> must create again (fails when success-with-no-row is held instead of parked, and when the subscription does not call the frame release) - a mirror frame releases an awaiting-mirror claim but never a pending create (fails when the release ignores the phase) --- ...ime-initial-terminal-bootstrap-dispatch.ts | 24 +++---- .../web-runtime-initial-terminal-bootstrap.ts | 62 ++++++++++++++----- ...bs-sync-initial-terminal-relaunch.test.tsx | 32 +++++++++- ...ssion-tabs-sync-terminal-bootstrap.test.ts | 22 ++++++- .../active-session-subscription.ts | 19 ++++-- 5 files changed, 126 insertions(+), 33 deletions(-) diff --git a/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts index 9869e0179ca..a435f9bcb87 100644 --- a/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts +++ b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts @@ -3,19 +3,20 @@ import { createWebRuntimeSessionTerminal } from './web-runtime-session' import type { WebRuntimeTerminalCreateOutcome } from './web-runtime-session-types' import { beginWebRuntimeInitialTerminalBootstrap, - endWebRuntimeInitialTerminalBootstrap + endWebRuntimeInitialTerminalBootstrap, + markWebRuntimeInitialTerminalBootstrapAwaitingMirror } from './web-runtime-initial-terminal-bootstrap' /** - * Claim the initial-terminal latch for this environment's worktree, create the terminal, and - * release the latch only once a mirrored row exists (or on failure, for retry). + * Claim the initial-terminal latch for this environment's worktree, create the terminal, and decide + * how the latch is released. * - * Why row-conditional and not a plain `.finally`: the snapshot refresh the create awaits can resolve - * on an empty, unconfirmed frame that leaves no `tabsByWorktree` row. Releasing then would let a - * later effect re-run seed a second terminal even though the first create succeeded. Holding the - * latch until a row exists makes the predicate decline on its own; worktree or environment teardown - * clears it either way. A failed create leaves no terminal, so it releases for a later focus to - * retry. + * A thrown or returned failure releases it at once: nothing was created, so a later focus may retry. + * A success whose mirrored `tabsByWorktree` row already exists releases too. A success with no row + * yet is neither: the refresh the create awaits can resolve on an empty, unconfirmed frame while the + * host does hold the tab, and releasing there let the next empty frame seed a duplicate. That case + * is parked as awaiting-mirror and released by the next frame the mirror accepts for the worktree + * (see web-runtime-initial-terminal-bootstrap.ts). * * Returns true when this call owned the create, so the caller can keep its closure-local flag in * step; false when another closure already held the latch. @@ -35,13 +36,14 @@ export async function dispatchWebRuntimeInitialTerminalBootstrap( throw error } // Why check the outcome: the create reports RPC and network failures as `{ status: 'failed' }` - // rather than throwing, so the catch above never sees them. Holding the latch on a returned - // failure would suppress every later auto-seed for this worktree until teardown. + // rather than throwing, so the catch above never sees them. if ( outcome.status === 'failed' || Object.hasOwn(useAppStore.getState().tabsByWorktree, worktreeId) ) { endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) + } else { + markWebRuntimeInitialTerminalBootstrapAwaitingMirror(environmentId, worktreeId) } return true } diff --git a/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap.ts b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap.ts index a32ad190452..961a96fbdc6 100644 --- a/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap.ts +++ b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap.ts @@ -13,14 +13,24 @@ * per environment lets a per-environment teardown release only its own in-flight keys — clearing * every environment's latch would release a sibling environment's pending create and let a new * subscription for it seed a duplicate, which is this very bug through another door. + * + * `creating` blocks every other closure while the create RPC is in flight. `awaiting-mirror` is a + * create that resolved without the mirror yet holding a row for the worktree: the host may have the + * tab and the frame simply has not landed, so the latch stays held — releasing here is what let the + * next empty frame seed a duplicate. The next frame the mirror accepts for that worktree is its + * answer either way (a row now exists and the predicate declines on its own, or the host genuinely + * has no terminal and a retry is right), so that frame releases it. Without that release a create + * whose frame never lands would suppress every later auto-seed until environment teardown. */ -const inFlightWorktreesByEnvironment = new Map>() +type InitialTerminalBootstrapPhase = 'creating' | 'awaiting-mirror' + +const phaseByWorktreeByEnvironment = new Map>() export function isWebRuntimeInitialTerminalBootstrapInFlight( environmentId: string, worktreeId: string ): boolean { - return inFlightWorktreesByEnvironment.get(environmentId)?.has(worktreeId) ?? false + return phaseByWorktreeByEnvironment.get(environmentId)?.has(worktreeId) ?? false } /** Claims the bootstrap for this environment's worktree; false when another closure already holds it. */ @@ -28,40 +38,64 @@ export function beginWebRuntimeInitialTerminalBootstrap( environmentId: string, worktreeId: string ): boolean { - const worktrees = inFlightWorktreesByEnvironment.get(environmentId) - if (worktrees?.has(worktreeId)) { + const phases = phaseByWorktreeByEnvironment.get(environmentId) + if (phases?.has(worktreeId)) { return false } - if (worktrees) { - worktrees.add(worktreeId) + if (phases) { + phases.set(worktreeId, 'creating') } else { - inFlightWorktreesByEnvironment.set(environmentId, new Set([worktreeId])) + phaseByWorktreeByEnvironment.set(environmentId, new Map([[worktreeId, 'creating']])) } return true } +/** The create resolved but no mirrored row exists yet; hold until the mirror answers. */ +export function markWebRuntimeInitialTerminalBootstrapAwaitingMirror( + environmentId: string, + worktreeId: string +): void { + const phases = phaseByWorktreeByEnvironment.get(environmentId) + if (phases?.has(worktreeId)) { + phases.set(worktreeId, 'awaiting-mirror') + } +} + export function endWebRuntimeInitialTerminalBootstrap( environmentId: string, worktreeId: string ): void { - const worktrees = inFlightWorktreesByEnvironment.get(environmentId) - if (!worktrees) { + const phases = phaseByWorktreeByEnvironment.get(environmentId) + if (!phases) { return } - worktrees.delete(worktreeId) - if (worktrees.size === 0) { - inFlightWorktreesByEnvironment.delete(environmentId) + phases.delete(worktreeId) + if (phases.size === 0) { + phaseByWorktreeByEnvironment.delete(environmentId) + } +} + +/** + * Release a bootstrap that was only waiting on the mirror. A create still in flight keeps its claim: + * releasing it on a frame is exactly the re-armed-closure race this latch exists to close. + */ +export function releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame( + environmentId: string, + worktreeId: string +): void { + if (phaseByWorktreeByEnvironment.get(environmentId)?.get(worktreeId) === 'awaiting-mirror') { + endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) } } export function clearWebRuntimeInitialTerminalBootstrapsForEnvironment( environmentId: string ): void { - inFlightWorktreesByEnvironment.delete(environmentId) + phaseByWorktreeByEnvironment.delete(environmentId) } export function clearAllWebRuntimeInitialTerminalBootstraps(): void { - inFlightWorktreesByEnvironment.clear() + phaseByWorktreeByEnvironment.clear() } export function resetWebRuntimeInitialTerminalBootstrapForTests(): void { diff --git a/src/renderer/src/runtime/web-session-tabs-sync-initial-terminal-relaunch.test.tsx b/src/renderer/src/runtime/web-session-tabs-sync-initial-terminal-relaunch.test.tsx index 30f1be5cc91..52f10c11856 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync-initial-terminal-relaunch.test.tsx +++ b/src/renderer/src/runtime/web-session-tabs-sync-initial-terminal-relaunch.test.tsx @@ -209,8 +209,8 @@ describe('useWebSessionTabsSync initial-terminal bootstrap across an effect re-r // then would let the next effect re-run seed a second terminal even though the first create // succeeded. The latch is held until a row exists, so the re-run declines. it('does not seed again after a create that resolved without mirroring a row', async () => { - // The create resolves but writes no tabsByWorktree row (host has not published the tab yet). - mocks.createTerminal.mockResolvedValue(undefined) + // The create succeeds but writes no tabsByWorktree row (host has not published the tab yet). + mocks.createTerminal.mockResolvedValue({ status: 'created' }) const hook = renderHook(() => useWebSessionTabsSync()) await act(settle) @@ -254,4 +254,32 @@ describe('useWebSessionTabsSync initial-terminal bootstrap across an effect re-r expect(mocks.createTerminal).toHaveBeenCalledTimes(2) hook.unmount() }) + + // Readiness review: the inverse hazard of the test above. A create that succeeds but whose frame + // never lands (host accepted, the mirror never got a row) must not hold the latch until environment + // teardown. The frame right after the settle is the mirror's answer and may not seed (pre-mirror + // window); the one after it decides on real state — no row, host affirms empty — and retries. + it('retries after a successful create that never mirrored a row, once the mirror has answered', async () => { + mocks.createTerminal.mockResolvedValue({ status: 'created' }) + + const hook = renderHook(() => useWebSessionTabsSync()) + await act(settle) + + await publish(findActiveSubscription(0), { type: 'snapshot', ...emptyActiveSnapshot(1) }) + expect(mocks.createTerminal).toHaveBeenCalledTimes(1) + expect(useAppStore.getState().tabsByWorktree[WORKTREE]).toBeUndefined() + + // The mirror's answer: still empty. Releases the parked bootstrap; must not itself seed. + await publish(findActiveSubscription(0), { type: 'snapshot', ...emptyActiveSnapshot(2) }) + expect(mocks.createTerminal).toHaveBeenCalledTimes(1) + + // A later focus installs a fresh closure; with the latch released, its empty frame may retry. + act(() => { + useAppStore.setState({ runtimeStatusByEnvironmentId: runtimeStatusMap(2) }) + }) + await act(settle) + await publish(findActiveSubscription(1), { type: 'snapshot', ...emptyActiveSnapshot(3) }) + expect(mocks.createTerminal).toHaveBeenCalledTimes(2) + hook.unmount() + }) }) diff --git a/src/renderer/src/runtime/web-session-tabs-sync-terminal-bootstrap.test.ts b/src/renderer/src/runtime/web-session-tabs-sync-terminal-bootstrap.test.ts index 1e00dcb975f..07c4007013b 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync-terminal-bootstrap.test.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync-terminal-bootstrap.test.ts @@ -18,7 +18,9 @@ import { import { beginWebRuntimeInitialTerminalBootstrap, endWebRuntimeInitialTerminalBootstrap, - isWebRuntimeInitialTerminalBootstrapInFlight + isWebRuntimeInitialTerminalBootstrapInFlight, + markWebRuntimeInitialTerminalBootstrapAwaitingMirror, + releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame } from './web-runtime-initial-terminal-bootstrap' const OTHER_ENV = 'web-env-2' @@ -203,6 +205,24 @@ describe('applyWebSessionTabsSnapshot', () => { ).toBe(false) }) + // Why: a mirror frame is the host's answer, but only for a create that already resolved. Releasing + // a still-pending create on a frame is the re-armed-closure race this latch exists to close. + it('releases an awaiting-mirror bootstrap on a mirror frame but never a pending create', () => { + expect(beginWebRuntimeInitialTerminalBootstrap(ENV, WT)).toBe(true) + releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame(ENV, WT) + expect(isWebRuntimeInitialTerminalBootstrapInFlight(ENV, WT)).toBe(true) + + markWebRuntimeInitialTerminalBootstrapAwaitingMirror(ENV, WT) + expect(isWebRuntimeInitialTerminalBootstrapInFlight(ENV, WT)).toBe(true) + releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame(ENV, WT) + expect(isWebRuntimeInitialTerminalBootstrapInFlight(ENV, WT)).toBe(false) + }) + + it('does not park a bootstrap that was never claimed', () => { + markWebRuntimeInitialTerminalBootstrapAwaitingMirror(ENV, WT) + expect(isWebRuntimeInitialTerminalBootstrapInFlight(ENV, WT)).toBe(false) + }) + // Why: the second half of STA-6173. One focus re-runs the subscription effect (environment, // connection generation and session-ready all settle during a workspace switch), and the old // closure-local flag re-armed with it, so both closures seeded before either create mirrored. diff --git a/src/renderer/src/runtime/web-session-tabs-sync/active-session-subscription.ts b/src/renderer/src/runtime/web-session-tabs-sync/active-session-subscription.ts index c9d44fbc0bf..78941098c5e 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/active-session-subscription.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/active-session-subscription.ts @@ -36,7 +36,10 @@ import { shouldSkipWebRuntimeWakeTerminalRespawn } from '../web-runtime-wake-terminal-respawn' import { createWebRuntimeSessionTerminal } from '../web-runtime-session' -import { isWebRuntimeInitialTerminalBootstrapInFlight } from '../web-runtime-initial-terminal-bootstrap' +import { + isWebRuntimeInitialTerminalBootstrapInFlight, + releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame +} from '../web-runtime-initial-terminal-bootstrap' import { dispatchWebRuntimeInitialTerminalBootstrap } from '../web-runtime-initial-terminal-bootstrap-dispatch' import { toRuntimeWorktreeSelector } from '../runtime-worktree-selector' import type { SessionTabsStreamEvent } from './state' @@ -145,10 +148,11 @@ export function installActiveSessionTabsSubscription({ const bootstrap = shouldBootstrapInitialWebRuntimeTerminal({ event: recoveredEvent, activeWorktreeId, - // Why both: the closure flag keeps one failed attempt from retrying on every later frame of - // the same subscription, and the shared latch is what survives the effect re-runs a workspace - // switch triggers — without it a second closure seeds a second terminal while the first - // create is still in flight (STA-6173). + // Why both: the closure flag records that this subscription already owned a create, so the + // same closure never seeds twice even once the shared latch is released; a create that threw + // never set it, so a later frame may retry. The shared latch is what survives the effect + // re-runs a workspace switch triggers — without it a second closure seeds a second terminal + // while the first create is still in flight (STA-6173). requestedInitialTerminal: requestedInitialTerminal || isWebRuntimeInitialTerminalBootstrapInFlight(environmentId, activeWorktreeId), @@ -192,6 +196,11 @@ export function installActiveSessionTabsSubscription({ event.type === 'updated' && !replayed ) visibilitySnapshotAccepted.current(environmentId, recovered, receivedFrame, runtimeId) + // Why here and only on an applied frame: this is the mirror's answer about the worktree. A + // bootstrap whose create resolved without a row was parked until that answer arrived; a + // pending create keeps its claim (see the latch module). The predicate above already ran with + // the latch held, so this frame never seeds — the next one decides on real state. + releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame(environmentId, recovered.worktree) } try { if (isCurrent() && bootstrap) {