mirror of
https://github.com/stablyai/orca.git
synced 2026-10-09 08:02:35 +00:00
fix(runtime): key the wake-respawn latch per environment, like its twin
The wake-respawn latch is the initial-terminal bootstrap latch's twin — both stop one focus from issuing two creates for a workspace, both are consulted from the same subscription closure, and they sit one line apart in the same teardown. The bootstrap latch got two fixes this stack; this one got neither, and still carried both defects: 1. NOT KEYED BY ENVIRONMENT. It was a bare `Set<worktreeId>`. A worktree id is `repoId::path` with no host component, so the same id can be live on two paired runtimes at once (STA-4343) — which is exactly why the bootstrap latch was re-keyed. Keyed by worktree alone, one runtime's respawn suppressed the other runtime's, and one runtime's `end` released the other's claim. 2. A CLEAR-EVERYTHING INSIDE A PER-ENVIRONMENT TEARDOWN. `clearWebSessionTabsTrackingForEnvironment` called `clearAllWebRuntimeWakeTerminalRespawn()`, so tearing one environment down freed every other environment's in-flight claim and a fresh closure for the sibling could issue a second respawn while the first create was still running. That is STA-6173, and the bootstrap latch's own docstring names it: "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." The correctly-scoped bootstrap clear is the very next line. Found by reading the teardown function as a list rather than as prose — a clear-everything is a one-line call that looks identical to a scoped one at a glance, which is how it sat next to the fix for its own defect. Both latches now have the same shape: `Map<environmentId, Set<worktreeId>>`, a per-worktree release, and a per-environment clear that touches only its own keys. The existing wake-respawn test is threaded with an environment id rather than rewritten: its contract was correct, only the signature moved. The new file pins the two defects themselves. Also documents `clearWebSessionTabsTrackingForEnvironment` as what it has become — the per-environment teardown registry. It now carries seven module clears, and a new per-environment map belongs in that list rather than behind a trigger of its own. The doc says each clear must be scoped to THIS environment and names the wake-respawn latch as the one that was not, because the next clear-everything will look just as harmless. Mutations: reverting the per-environment clear to a clear-all kills exactly the sibling-claim assertion; making the skip check ignore the environment (the old worktree-only keying) kills exactly the two-environments assertion and the sibling-claim one. No survivors.
This commit is contained in:
@@ -60,7 +60,7 @@ export function ensureWebRuntimeWorktreeTerminalAfterWake(worktreeId: string): v
|
||||
return
|
||||
}
|
||||
|
||||
if (!beginWebRuntimeWakeTerminalRespawn(worktreeId)) {
|
||||
if (!beginWebRuntimeWakeTerminalRespawn(runtimeEnvironmentId, worktreeId)) {
|
||||
return
|
||||
}
|
||||
|
||||
@@ -71,6 +71,6 @@ export function ensureWebRuntimeWorktreeTerminalAfterWake(worktreeId: string): v
|
||||
activate: true,
|
||||
selectWorktree: false
|
||||
}).finally(() => {
|
||||
endWebRuntimeWakeTerminalRespawn(worktreeId)
|
||||
endWebRuntimeWakeTerminalRespawn(runtimeEnvironmentId, worktreeId)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -0,0 +1,62 @@
|
||||
import { beforeEach, describe, expect, it } from 'vitest'
|
||||
import {
|
||||
beginWebRuntimeWakeTerminalRespawn,
|
||||
clearWebRuntimeWakeTerminalRespawnForEnvironment,
|
||||
endWebRuntimeWakeTerminalRespawn,
|
||||
resetWebRuntimeWakeTerminalRespawnForTests,
|
||||
shouldSkipWebRuntimeWakeTerminalRespawn
|
||||
} from './web-runtime-wake-terminal-respawn'
|
||||
|
||||
// The wake-respawn latch is the initial-terminal bootstrap latch's twin: both stop one focus from
|
||||
// issuing two creates for a workspace, and both are consulted from the same subscription closure.
|
||||
// The bootstrap latch was re-keyed per environment for STA-4343 — a worktree id is `repoId::path`
|
||||
// with no host component, so the same id can be live on two paired runtimes at once — and its
|
||||
// teardown narrowed to one environment for STA-6173. This latch never got either change: it was a
|
||||
// bare Set of worktree ids, and `clearWebSessionTabsTrackingForEnvironment` cleared ALL of it, one
|
||||
// line above the bootstrap latch's correctly-scoped clear.
|
||||
|
||||
const ENV_A = 'env-a'
|
||||
const ENV_B = 'env-b'
|
||||
const WORKTREE = 'repo-a::worktree-a'
|
||||
|
||||
describe('wake-terminal respawn latch keyed per environment', () => {
|
||||
beforeEach(resetWebRuntimeWakeTerminalRespawnForTests)
|
||||
|
||||
// STA-4343 shape: two paired runtimes can hold the same worktree id at once, so one runtime's
|
||||
// respawn must not suppress the other's — and one runtime's release must not free the other's.
|
||||
it('lets two environments hold the same worktree id independently', () => {
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)).toBe(true)
|
||||
|
||||
// The sibling runtime's respawn is a different workspace, and must not be blocked by A's.
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV_B, WORKTREE)).toBe(false)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV_B, WORKTREE)).toBe(true)
|
||||
|
||||
// A's create settles. Only A's claim goes.
|
||||
endWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)).toBe(false)
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV_B, WORKTREE)).toBe(true)
|
||||
})
|
||||
|
||||
// STA-6173 shape, the cross-environment door: tearing one environment down freed every other
|
||||
// environment's in-flight claim, so a fresh closure for the sibling could issue a second respawn
|
||||
// while the first create was still running.
|
||||
it('does not release a sibling environment’s claim when one environment is torn down', () => {
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)).toBe(true)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV_B, WORKTREE)).toBe(true)
|
||||
|
||||
clearWebRuntimeWakeTerminalRespawnForEnvironment(ENV_A)
|
||||
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)).toBe(false)
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV_B, WORKTREE)).toBe(true)
|
||||
// A freshly installed sibling closure passes its own flag false, so only the surviving latch
|
||||
// stops it issuing a duplicate respawn.
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV_B, WORKTREE)).toBe(false)
|
||||
})
|
||||
|
||||
it('drains the environment entry once its last worktree is released', () => {
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)).toBe(true)
|
||||
endWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)).toBe(false)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV_A, WORKTREE)).toBe(true)
|
||||
})
|
||||
})
|
||||
@@ -7,24 +7,26 @@ import {
|
||||
shouldSkipWebRuntimeWakeTerminalRespawn
|
||||
} from './web-runtime-wake-terminal-respawn'
|
||||
|
||||
const ENV = 'env-a'
|
||||
|
||||
describe('web-runtime-wake-terminal-respawn', () => {
|
||||
beforeEach(() => {
|
||||
resetWebRuntimeWakeTerminalRespawnForTests()
|
||||
})
|
||||
|
||||
it('dedupes concurrent wake respawn requests for the same worktree', () => {
|
||||
expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true)
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(false)
|
||||
endWebRuntimeWakeTerminalRespawn('wt-1')
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn('wt-1')).toBe(false)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')).toBe(true)
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')).toBe(true)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')).toBe(false)
|
||||
endWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')).toBe(false)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')).toBe(true)
|
||||
})
|
||||
|
||||
it('clears wake respawn tracking for a removed worktree', () => {
|
||||
beginWebRuntimeWakeTerminalRespawn('wt-1')
|
||||
clearWebRuntimeWakeTerminalRespawnForWorktree('wt-1')
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn('wt-1')).toBe(false)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn('wt-1')).toBe(true)
|
||||
beginWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')
|
||||
clearWebRuntimeWakeTerminalRespawnForWorktree(ENV, 'wt-1')
|
||||
expect(shouldSkipWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')).toBe(false)
|
||||
expect(beginWebRuntimeWakeTerminalRespawn(ENV, 'wt-1')).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,29 +1,62 @@
|
||||
const wakeTerminalRespawnInFlightByWorktree = new Set<string>()
|
||||
/**
|
||||
* Which workspaces already have a post-wake terminal respawn in flight.
|
||||
*
|
||||
* Keyed by environment AND worktree, for the same two reasons as the initial-terminal bootstrap
|
||||
* latch next door (see web-runtime-initial-terminal-bootstrap.ts). A worktree id is `repoId::path`
|
||||
* with no host component, so the same id can be live on two paired runtimes at once (STA-4343):
|
||||
* keyed by worktree alone, one runtime's respawn suppressed the other's, and one runtime's release
|
||||
* freed the other's claim. And keying per environment is what lets a per-environment teardown
|
||||
* release only its own in-flight keys — clearing every environment's latch releases a sibling's
|
||||
* pending create and lets a new subscription for it seed a duplicate (STA-6173).
|
||||
*/
|
||||
const wakeTerminalRespawnInFlightByEnvironment = new Map<string, Set<string>>()
|
||||
|
||||
export function shouldSkipWebRuntimeWakeTerminalRespawn(worktreeId: string): boolean {
|
||||
return wakeTerminalRespawnInFlightByWorktree.has(worktreeId)
|
||||
export function shouldSkipWebRuntimeWakeTerminalRespawn(
|
||||
environmentId: string,
|
||||
worktreeId: string
|
||||
): boolean {
|
||||
return wakeTerminalRespawnInFlightByEnvironment.get(environmentId)?.has(worktreeId) ?? false
|
||||
}
|
||||
|
||||
export function beginWebRuntimeWakeTerminalRespawn(worktreeId: string): boolean {
|
||||
if (wakeTerminalRespawnInFlightByWorktree.has(worktreeId)) {
|
||||
/** Claims the respawn for this environment's worktree; false when another closure already holds it. */
|
||||
export function beginWebRuntimeWakeTerminalRespawn(
|
||||
environmentId: string,
|
||||
worktreeId: string
|
||||
): boolean {
|
||||
const inFlight = wakeTerminalRespawnInFlightByEnvironment.get(environmentId)
|
||||
if (inFlight?.has(worktreeId)) {
|
||||
return false
|
||||
}
|
||||
wakeTerminalRespawnInFlightByWorktree.add(worktreeId)
|
||||
if (inFlight) {
|
||||
inFlight.add(worktreeId)
|
||||
} else {
|
||||
wakeTerminalRespawnInFlightByEnvironment.set(environmentId, new Set([worktreeId]))
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
export function endWebRuntimeWakeTerminalRespawn(worktreeId: string): void {
|
||||
wakeTerminalRespawnInFlightByWorktree.delete(worktreeId)
|
||||
export function endWebRuntimeWakeTerminalRespawn(environmentId: string, worktreeId: string): void {
|
||||
const inFlight = wakeTerminalRespawnInFlightByEnvironment.get(environmentId)
|
||||
if (!inFlight) {
|
||||
return
|
||||
}
|
||||
inFlight.delete(worktreeId)
|
||||
if (inFlight.size === 0) {
|
||||
wakeTerminalRespawnInFlightByEnvironment.delete(environmentId)
|
||||
}
|
||||
}
|
||||
|
||||
export function clearWebRuntimeWakeTerminalRespawnForWorktree(worktreeId: string): void {
|
||||
wakeTerminalRespawnInFlightByWorktree.delete(worktreeId)
|
||||
export function clearWebRuntimeWakeTerminalRespawnForWorktree(
|
||||
environmentId: string,
|
||||
worktreeId: string
|
||||
): void {
|
||||
endWebRuntimeWakeTerminalRespawn(environmentId, worktreeId)
|
||||
}
|
||||
|
||||
export function clearAllWebRuntimeWakeTerminalRespawn(): void {
|
||||
wakeTerminalRespawnInFlightByWorktree.clear()
|
||||
export function clearWebRuntimeWakeTerminalRespawnForEnvironment(environmentId: string): void {
|
||||
wakeTerminalRespawnInFlightByEnvironment.delete(environmentId)
|
||||
}
|
||||
|
||||
export function resetWebRuntimeWakeTerminalRespawnForTests(): void {
|
||||
clearAllWebRuntimeWakeTerminalRespawn()
|
||||
wakeTerminalRespawnInFlightByEnvironment.clear()
|
||||
}
|
||||
|
||||
@@ -168,7 +168,7 @@ export function installActiveSessionTabsSubscription({
|
||||
snapshotIsFresh: decision.apply,
|
||||
localTerminalCount,
|
||||
hasLiveLocalPty,
|
||||
skipWakeRespawn: shouldSkipWebRuntimeWakeTerminalRespawn(activeWorktreeId)
|
||||
skipWakeRespawn: shouldSkipWebRuntimeWakeTerminalRespawn(environmentId, activeWorktreeId)
|
||||
})
|
||||
let settle: HostSessionMirrorSettle | null = decision.apply
|
||||
? null
|
||||
@@ -209,14 +209,18 @@ export function installActiveSessionTabsSubscription({
|
||||
if (await dispatchWebRuntimeInitialTerminalBootstrap(environmentId, activeWorktreeId)) {
|
||||
requestedInitialTerminal = true
|
||||
}
|
||||
} else if (isCurrent() && respawn && beginWebRuntimeWakeTerminalRespawn(activeWorktreeId)) {
|
||||
} else if (
|
||||
isCurrent() &&
|
||||
respawn &&
|
||||
beginWebRuntimeWakeTerminalRespawn(environmentId, activeWorktreeId)
|
||||
) {
|
||||
requestedRespawnAfterWake = true
|
||||
await createWebRuntimeSessionTerminal({
|
||||
worktreeId: activeWorktreeId,
|
||||
environmentId,
|
||||
activate: true,
|
||||
selectWorktree: false
|
||||
}).finally(() => endWebRuntimeWakeTerminalRespawn(activeWorktreeId))
|
||||
}).finally(() => endWebRuntimeWakeTerminalRespawn(environmentId, activeWorktreeId))
|
||||
}
|
||||
} catch (error) {
|
||||
if (isCurrent()) {
|
||||
|
||||
@@ -20,7 +20,7 @@ import {
|
||||
} from './state'
|
||||
import {
|
||||
clearWebRuntimeWakeTerminalRespawnForWorktree,
|
||||
clearAllWebRuntimeWakeTerminalRespawn
|
||||
clearWebRuntimeWakeTerminalRespawnForEnvironment
|
||||
} from '../web-runtime-wake-terminal-respawn'
|
||||
import {
|
||||
releaseWebRuntimeInitialTerminalBootstrapOnTeardown,
|
||||
@@ -143,7 +143,7 @@ export function clearWebSessionTabsTrackingForWorktree(
|
||||
removeWebSessionTabsEnvironment(environmentId, worktreeId)
|
||||
lastHostTerminalTabCountByWorktree.delete(key)
|
||||
sessionTabsInventoryOmissionsByWorktree.delete(key)
|
||||
clearWebRuntimeWakeTerminalRespawnForWorktree(worktreeId)
|
||||
clearWebRuntimeWakeTerminalRespawnForWorktree(environmentId, worktreeId)
|
||||
releaseWebRuntimeInitialTerminalBootstrapOnTeardown(environmentId, worktreeId)
|
||||
clearWebSessionReorderIntentsForWorktree({ environmentId }, worktreeId)
|
||||
clearWebSessionCloseIntentsForWorktree({ environmentId }, worktreeId)
|
||||
@@ -153,6 +153,15 @@ export function clearWebSessionTabsTrackingForWorktree(
|
||||
clearWebSessionTerminalPlacementsForWorktree(environmentId, worktreeId)
|
||||
}
|
||||
|
||||
/**
|
||||
* The per-environment teardown registry. Every module that keeps state keyed by environment hangs
|
||||
* its clear here — this is the one place that fires when an environment is re-paired or goes away,
|
||||
* so a new per-environment map belongs in this list rather than behind a trigger of its own.
|
||||
*
|
||||
* Each clear must be scoped to THIS environment. A clear-everything hides in here as a one-line
|
||||
* call and releases a sibling environment's in-flight work, which is STA-6173 through a side door;
|
||||
* that is how the wake-respawn latch above lost its sibling's claim.
|
||||
*/
|
||||
export function clearWebSessionTabsTrackingForEnvironment(environmentId: string): void {
|
||||
const trimmedEnvironmentId = environmentId.trim()
|
||||
if (!trimmedEnvironmentId) {
|
||||
@@ -225,7 +234,7 @@ export function clearWebSessionTabsTrackingForEnvironment(environmentId: string)
|
||||
clearWebSessionTerminalPlacementsForEnvironment(trimmedEnvironmentId)
|
||||
clearHostSessionMirrorHydration(trimmedEnvironmentId)
|
||||
clearHostMirrorHandleGapVerdictsForEnvironment(trimmedEnvironmentId)
|
||||
clearAllWebRuntimeWakeTerminalRespawn()
|
||||
clearWebRuntimeWakeTerminalRespawnForEnvironment(trimmedEnvironmentId)
|
||||
clearWebRuntimeInitialTerminalBootstrapsForEnvironment(trimmedEnvironmentId)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user