From d02ac377a5c8a4d3bc76489168df5ab9bfab8353 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 06:46:01 -0700 Subject: [PATCH] fix(runtime): let a failed create retry inside its own subscription The returned-failure release freed the module latch but the caller still latched its closure-local requestedInitialTerminal whenever the dispatch reported it owned the create, including a create that returned { status: 'failed' }. A thrown failure never set it and retried on the next frame; since every RPC and network failure is reported as a return, the live path was the one that suppressed the whole subscription. The dispatch now reports false for a failure whichever way it arrives. --- ...ime-initial-terminal-bootstrap-dispatch.ts | 17 +++++++------ ...bs-sync-initial-terminal-relaunch.test.tsx | 25 +++++++++++++++++++ 2 files changed, 35 insertions(+), 7 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 a435f9bcb87..d4f097a2a68 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 @@ -18,8 +18,10 @@ import { * 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. + * Returns true only when this call owned a create that was not reported as failed, because the + * caller latches a closure-local flag on it. A failure must report false whichever way it arrives: + * releasing the shared latch alone still leaves the subscription that issued the failed create + * unable to retry for as long as its closure lives, which is the same suppression one level up. */ export async function dispatchWebRuntimeInitialTerminalBootstrap( environmentId: string, @@ -36,11 +38,12 @@ 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. - if ( - outcome.status === 'failed' || - Object.hasOwn(useAppStore.getState().tabsByWorktree, worktreeId) - ) { + // rather than throwing, so the catch above never sees them. Both arms report the same way. + if (outcome.status === 'failed') { + endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) + return false + } + if (Object.hasOwn(useAppStore.getState().tabsByWorktree, worktreeId)) { endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) } else { markWebRuntimeInitialTerminalBootstrapAwaitingMirror(environmentId, worktreeId) 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 52f10c11856..fb7808e472d 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 @@ -255,6 +255,31 @@ describe('useWebSessionTabsSync initial-terminal bootstrap across an effect re-r hook.unmount() }) + // Third suppression path (review): the returned-failure release above frees the *module* latch, + // but the caller sets its closure-local `requestedInitialTerminal` whenever the dispatch reports + // it owned the create — including when that create returned `{ status: 'failed' }`. A thrown + // failure never sets it and retries on the very next frame; since the create reports every RPC + // and network failure as a return rather than a throw, the live path was the one that suppressed + // the whole subscription. A failed create must leave the closure free to retry, exactly like a + // thrown one. + it('retries on the next frame of the same subscription after a create that returned failed', async () => { + mocks.createTerminal.mockResolvedValue({ status: 'failed', message: 'host unreachable' }) + + const hook = renderHook(() => useWebSessionTabsSync()) + await act(settle) + + const subscription = findActiveSubscription(0) + await publish(subscription, { type: 'snapshot', ...emptyActiveSnapshot(1) }) + expect(mocks.createTerminal).toHaveBeenCalledTimes(1) + expect(useAppStore.getState().tabsByWorktree[WORKTREE]).toBeUndefined() + + // Same closure, next accepted frame: nothing was created, so the workspace is still + // never-initialized and the retry is the only thing that will ever give it a terminal. + await publish(subscription, { type: 'snapshot', ...emptyActiveSnapshot(2) }) + 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