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.
This commit is contained in:
Neil
2026-09-10 06:46:12 -07:00
parent 6e5708448f
commit d02ac377a5
2 changed files with 35 additions and 7 deletions
@@ -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)
@@ -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