mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
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)
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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<string, Set<string>>()
|
||||
type InitialTerminalBootstrapPhase = 'creating' | 'awaiting-mirror'
|
||||
|
||||
const phaseByWorktreeByEnvironment = new Map<string, Map<string, InitialTerminalBootstrapPhase>>()
|
||||
|
||||
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 {
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user