From cbf18d46d49fecc7ac1c2752ca28cc1cd9501f0d Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:52:10 -0700 Subject: [PATCH] fix(runtime): key the wake-respawn latch per environment, like its twin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`. 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>`, 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. --- ...eb-runtime-worktree-terminal-after-wake.ts | 4 +- ...-wake-terminal-respawn-environment.test.ts | 62 +++++++++++++++++++ .../web-runtime-wake-terminal-respawn.test.ts | 22 ++++--- .../web-runtime-wake-terminal-respawn.ts | 59 ++++++++++++++---- .../active-session-subscription.ts | 10 ++- .../tracking-lifecycle.ts | 15 ++++- 6 files changed, 141 insertions(+), 31 deletions(-) create mode 100644 src/renderer/src/runtime/web-runtime-wake-terminal-respawn-environment.test.ts diff --git a/src/renderer/src/lib/web-runtime-worktree-terminal-after-wake.ts b/src/renderer/src/lib/web-runtime-worktree-terminal-after-wake.ts index ac3a40bd87f..975c396f656 100644 --- a/src/renderer/src/lib/web-runtime-worktree-terminal-after-wake.ts +++ b/src/renderer/src/lib/web-runtime-worktree-terminal-after-wake.ts @@ -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) }) } diff --git a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn-environment.test.ts b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn-environment.test.ts new file mode 100644 index 00000000000..b842da687b9 --- /dev/null +++ b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn-environment.test.ts @@ -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) + }) +}) diff --git a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts index 6e55e47e66b..ff7d9811b11 100644 --- a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts +++ b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.test.ts @@ -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) }) }) diff --git a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts index 7e1dcc51020..029a1af67df 100644 --- a/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts +++ b/src/renderer/src/runtime/web-runtime-wake-terminal-respawn.ts @@ -1,29 +1,62 @@ -const wakeTerminalRespawnInFlightByWorktree = new Set() +/** + * 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>() -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() } 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 d60727f7639..c199d5e86bd 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 @@ -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()) { diff --git a/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts b/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts index 6a7350e369b..95470efe580 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/tracking-lifecycle.ts @@ -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) }