diff --git a/src/renderer/src/lib/host-mirror-handle-gap-drain.test.ts b/src/renderer/src/lib/host-mirror-handle-gap-drain.test.ts index 246a72147ca..d7864053eb6 100644 --- a/src/renderer/src/lib/host-mirror-handle-gap-drain.test.ts +++ b/src/renderer/src/lib/host-mirror-handle-gap-drain.test.ts @@ -24,6 +24,9 @@ const SECOND_TAB_ID = 'web-terminal-host-tab-2' const initialAppStoreState = useAppStore.getState() function seedRows(): void { + // Layout bindings are seeded because a verdict names the PANE by the environment-minted PTY it + // held at park time. A pane with no binding never reaches the park path in production, and its + // verdict deliberately refuses to answer, so a fixture without one models nothing real. useAppStore.setState({ ptyIdsByTabId: {}, tabsByWorktree: { @@ -31,6 +34,20 @@ function seedRows(): void { { id: FIRST_TAB_ID, title: 'one' }, { id: SECOND_TAB_ID, title: 'two' } ] + }, + terminalLayoutsByTabId: { + [FIRST_TAB_ID]: { + root: { type: 'leaf', leafId: 'leaf-1' }, + activeLeafId: 'leaf-1', + expandedLeafId: null, + ptyIdsByLeafId: { 'leaf-1': `remote:${ENVIRONMENT_ID}@@term_1` } + }, + [SECOND_TAB_ID]: { + root: { type: 'leaf', leafId: 'leaf-2' }, + activeLeafId: 'leaf-2', + expandedLeafId: null, + ptyIdsByLeafId: { 'leaf-2': `remote:${ENVIRONMENT_ID}@@term_2` } + } } } as never) } diff --git a/src/renderer/src/lib/host-mirror-handle-gap-teardown.test.ts b/src/renderer/src/lib/host-mirror-handle-gap-teardown.test.ts index 7076af927f3..c25434f705c 100644 --- a/src/renderer/src/lib/host-mirror-handle-gap-teardown.test.ts +++ b/src/renderer/src/lib/host-mirror-handle-gap-teardown.test.ts @@ -29,11 +29,24 @@ function parkAndExpire(environmentId: string, tabId: string): void { // Rows ACCUMULATE. Replacing them would unpublish the panes parked earlier, and the tab-death // rule would then legitimately sweep their verdicts before teardown was ever reached — this // suite is about a class no recording-driven prune can reach, so every pane here stays live. - const published = useAppStore.getState().tabsByWorktree[WORKTREE_ID] ?? [] + const state = useAppStore.getState() + const published = state.tabsByWorktree[WORKTREE_ID] ?? [] useAppStore.setState({ ptyIdsByTabId: {}, tabsByWorktree: { [WORKTREE_ID]: [...published.filter((tab) => tab.id !== tabId), { id: tabId, title: tabId }] + }, + // A verdict names its PANE by the environment-minted PTY held at park time, so a fixture with + // no layout binding records '' and the verdict refuses to answer. Bind per environment: one + // shared environment id would filter to '' for every other environment's pane. + terminalLayoutsByTabId: { + ...state.terminalLayoutsByTabId, + [tabId]: { + root: { type: 'leaf', leafId: `leaf-${tabId}` }, + activeLeafId: `leaf-${tabId}`, + expandedLeafId: null, + ptyIdsByLeafId: { [`leaf-${tabId}`]: `remote:${environmentId}@@term_${tabId}` } + } } } as never) parkUntilHostMirrorHandleLands(environmentId, WORKTREE_ID, tabId, () => {}) diff --git a/src/renderer/src/lib/host-mirror-handle-gap-verdict-union.test.ts b/src/renderer/src/lib/host-mirror-handle-gap-verdict-union.test.ts index c0d9479a20b..449c8519435 100644 --- a/src/renderer/src/lib/host-mirror-handle-gap-verdict-union.test.ts +++ b/src/renderer/src/lib/host-mirror-handle-gap-verdict-union.test.ts @@ -24,10 +24,15 @@ import { * A tab churn on a LIVE environment tab-death rule, recording environment only * B REMOVED environment clearHostMirrorHandleGapVerdictsForEnvironment * C cross-environment QUIESCENCE generation rule, per key, every environment - * D REUSED tab id NOTHING. Pinned below as a live hazard. + * D REUSED tab id read-time pane identity, NOT a prune * - * Plus the two properties no rule may break: the verdict stays sticky enough to break the - * park/expire/replay loop, and no rule evicts a verdict a live pane still needs. + * D is the one that needed no new trigger: every trigger the other three own fires downstream of + * the moment it needs. The verdict instead carries the PTY binding its pane held AT PARK TIME and + * only answers for a pane that still holds it. + * + * Plus the properties no rule may break: the verdict stays sticky enough to break the + * park/expire/replay loop, a genuine reattach still inherits, and no rule evicts a verdict a live + * pane still needs. */ const ENV_A = 'env-union-a' @@ -36,9 +41,30 @@ const ENV_C = 'env-union-c' const WORKTREE = 'repo-1::wt-union' const initialAppStoreState = useAppStore.getState() -function setLiveTabs(tabIds: string[]): void { +/** Which environment minted each pane's PTY; the binding only counts for its own environment. */ +const ENV_OF_TAB: Record = { + a1: ENV_A, + a2: ENV_A, + reused: ENV_A, + b1: ENV_B, + c1: ENV_C +} + +/** Publishes rows AND the layout PTY binding each pane holds — the binding is the pane's identity. */ +function setLiveTabs(tabIds: string[], ptyByTabId: Record = {}): void { + const layouts: Record = {} + for (const id of tabIds) { + const ptyId = ptyByTabId[id] ?? `remote:${ENV_OF_TAB[id] ?? ENV_A}@@term_${id}` + layouts[id] = { + root: { type: 'leaf', leafId: `leaf-${id}` }, + activeLeafId: `leaf-${id}`, + expandedLeafId: null, + ptyIdsByLeafId: { [`leaf-${id}`]: ptyId } + } + } useAppStore.setState({ tabsByWorktree: { [WORKTREE]: tabIds.map((id) => ({ id, title: id, ptyId: null })) }, + terminalLayoutsByTabId: layouts, ptyIdsByTabId: {} } as unknown as AppState) } @@ -82,8 +108,9 @@ describe('handle-gap verdict map, all rules on one tree', () => { setRuntimeEnvironmentConnectionGenerationForTests(ENV_C, 2) clearHostMirrorHandleGapVerdictsForEnvironment(ENV_B) - // A's tab closes; the reused id is retracted and republished as a DIFFERENT pane. - setLiveTabs(['a2', 'reused']) + // A's tab closes; the reused id is retracted and republished as a DIFFERENT pane, which binds + // a PTY the host newly minted. That new binding is what makes it a different pane, not the id. + setLiveTabs(['a2', 'reused'], { reused: `remote:${ENV_A}@@term_freshly_minted` }) parkAndExpire(ENV_A, 'a2') // A drained: a1's row is gone and env-a recorded again, so the tab-death rule swept it. @@ -93,27 +120,61 @@ describe('handle-gap verdict map, all rules on one tree', () => { // C drained: env-a's expiry retired env-c's superseded row, though env-c never expired again. expect(hasHostMirrorHandleWaitExpired(ENV_C, 'c1')).toBe(false) - // D IS NOT DRAINED, and this assertion pins a LIVE HAZARD rather than a desired behaviour. - // The republished pane inherits the retracted pane's verdict and skips its own wait. - // - // Why this one is different from every other gap argued over on this map: the others DROP a - // verdict, so the pane re-parks and only ever holds longer. This one RETAINS a verdict and - // lets a fresh pane resume on a handle that has not landed — the #19735 direction itself. - // It is therefore the one gap here that is not conservative. - // - // No rule reaches it, and each for its own reason: the tab-death rule's predicate stops - // matching the moment the id is republished, so it is not even eventually consistent; the - // teardown drain fires on environment teardown, not on tab retraction inside a live one; and - // no waiter exists to observe the retraction, because a pane holding a verdict never parks - // (`findUnhydratedHostMirrorForPane` returns null on it). Closing it needs a fourth trigger, - // on row retraction. DO NOT delete this case when a prune for dead tabs lands — "a prune for - // dead tabs shipped" is exactly the plausible assumption that would delete it. - expect(hasHostMirrorHandleWaitExpired(ENV_A, 'reused')).toBe(true) + // D is closed, and NOT by a prune. No trigger any rule above owns fires at the right moment: + // the tab-death predicate stops matching once the id is live again, teardown is the wrong + // event, and no waiter observes the retraction because a pane holding a verdict never parks. + // It is closed at READ time instead — the verdict names the pane it was about, so a pane that + // binds a newly minted PTY does not answer to it and serves its own wait. + expect(hasHostMirrorHandleWaitExpired(ENV_A, 'reused')).toBe(false) // Only the two live verdicts survive: a2's and the stranded reused-id row. expect(countHostMirrorHandleGapVerdictsForTests()).toBe(2) }) + it('answers for a genuine reattach that still holds the same PTY', () => { + // The verdict follows the PTY, not the tab id. A pane that reattaches to the SAME environment + // PTY is the same pane, so it must inherit — otherwise the identity check would have quietly + // removed the loop-breaker for every reattach. + setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1) + setLiveTabs(['a1']) + parkAndExpire(ENV_A, 'a1') + setLiveTabs([]) + setLiveTabs(['a1']) + expect(hasHostMirrorHandleWaitExpired(ENV_A, 'a1')).toBe(true) + }) + + it('records the binding the pane held at PARK time, not at expiry', () => { + // The mutation this kills: reading the binding inside `recordExpiredWait` from the store + // instead of from the waiter. A pane replaced mid-wait leaves the original waiter running to + // term, and an expiry-time read would attribute the verdict to whoever holds the id by then — + // handing the new pane a wait it never served. Three earlier cases all survived that bug; + // only rebinding BETWEEN park and expire distinguishes the two implementations. + setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1) + setLiveTabs(['a1']) + parkUntilHostMirrorHandleLands(ENV_A, WORKTREE, 'a1', () => {}) + setLiveTabs(['a1'], { a1: `remote:${ENV_A}@@term_replacement` }) + vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS + 1) + + // The replacement pane never served this wait, so it must not inherit its verdict. + expect(hasHostMirrorHandleWaitExpired(ENV_A, 'a1')).toBe(false) + }) + + it('refuses to answer on an empty binding, which is a match value and not a null', () => { + // '' is what `paneBindingFor` returns when no leaf holds an environment-minted PTY. Two + // different panes both reading '' would compare EQUAL and inherit, which is the reused-tab-id + // shape again. Measured unreachable through the production park path rather than assumed: the + // only route into `parkUntilHostMirrorHandleLands` is `kind: 'handle'`, which + // `findUnhydratedHostMirrorForPane` reports only when `tabHoldsEnvironmentPtyBinding` + // (host-mirrored-pane-liveness.ts:28-31) finds a match — the SAME `terminalLayoutsByTabId` + // map through the SAME `parseRemoteRuntimePtyId` predicate `paneBindingFor` uses, so a pane + // that would bind '' never parks. It must still refuse rather than match, because that + // coupling is two functions in two files and nothing enforces it. + setRuntimeEnvironmentConnectionGenerationForTests(ENV_A, 1) + setLiveTabs(['a1'], { a1: 'remote:some-other-env@@term_1' }) + parkAndExpire(ENV_A, 'a1') + expect(hasHostMirrorHandleWaitExpired(ENV_A, 'a1')).toBe(false) + }) + it('keeps a verdict sticky enough to break the park/expire/replay loop', () => { // The verdict exists to stop a pane re-parking forever. If any rule evicted it while the pane // is live and its connection current, the wait would rearm on a fresh budget every replay. diff --git a/src/renderer/src/lib/host-mirror-handle-gap-wait.ts b/src/renderer/src/lib/host-mirror-handle-gap-wait.ts index 27be4efd91c..c3cd4af0d4d 100644 --- a/src/renderer/src/lib/host-mirror-handle-gap-wait.ts +++ b/src/renderer/src/lib/host-mirror-handle-gap-wait.ts @@ -1,6 +1,7 @@ import { useAppStore } from '@/store' import { getRuntimeEnvironmentConnectionGeneration } from '@/store/slices/runtime-status' import { WEB_SESSION_TAB_RPC_TIMEOUT_MS } from '@/runtime/web-session-tab-rpc-timeout' +import { parseRemoteRuntimePtyId } from '../../../shared/remote-runtime-pty-id' /** * Per-pane park for the frame between a host's tab rows and its PTY handles. @@ -34,6 +35,8 @@ type HandleGapWaiter = { tabId: string /** Connection generation the wait was armed on; its verdict is void on any other. */ generation: number + /** Which PANE this wait is about, captured at park time; see ExpiredHandleGapVerdict. */ + paneBinding: string deadline: ReturnType run: () => void } @@ -63,18 +66,54 @@ const waitersByPane = new Map() * never parks, so no waiter observes the retraction. Closing it needs a fourth trigger, on row * retraction. Pinned in host-mirror-handle-gap-verdict-union.test.ts; do not delete that case. */ -const expiredGenerationByPane = new Map() +type ExpiredHandleGapVerdict = { + generation: number + /** Sorted environment-minted PTY ids the tab's leaves held AT PARK TIME; '' when none. */ + paneBinding: string +} +const expiredGenerationByPane = new Map() let unsubscribeStore: (() => void) | null = null function paneWaitKey(environmentId: string, tabId: string): string { return `${environmentId}\0${tabId}` } -/** True once the deadline fired for this pane on the current connection. */ +/** + * The environment-minted PTY ids this tab's leaves are bound to, as one comparable string. + * + * Read from the layout, not `ptyIdsByTabId`: during the handle gap the published-handle map is + * empty by definition — that is the gap — while the layout binding is what + * `tabHoldsEnvironmentPtyBinding` already uses to call the pane unverifiable rather than dead. + */ +function paneBindingFor(tabId: string, environmentId: string): string { + const bindings = useAppStore.getState().terminalLayoutsByTabId[tabId]?.ptyIdsByLeafId ?? {} + return Object.values(bindings) + .filter( + (ptyId): ptyId is string => + typeof ptyId === 'string' && parseRemoteRuntimePtyId(ptyId)?.environmentId === environmentId + ) + .sort() + .join('') +} + +/** True once the deadline fired for THIS pane on the current connection. */ export function hasHostMirrorHandleWaitExpired(environmentId: string, tabId: string): boolean { + const verdict = expiredGenerationByPane.get(paneWaitKey(environmentId, tabId)) + if (verdict === undefined || verdict.paneBinding === '') { + // Why '' never answers: it is a MATCH VALUE, not a null. Two different panes that both hold no + // environment-minted PTY compare equal, which is the reused-tab-id inheritance this check + // exists to stop, in a narrower window. Unreachable through the production park path — + // `findUnhydratedHostMirrorForPane` only reports `kind: 'handle'` when + // `tabHoldsEnvironmentPtyBinding` finds a binding, reading the same map through the same + // predicate as `paneBindingFor` — and pinned by the coupling test in + // host-mirror-handle-gap-verdict-union.test.ts. Refusing costs a re-park, which is the + // conservative direction, so the pair stays safe even if those two reads ever drift apart. + return false + } return ( - expiredGenerationByPane.get(paneWaitKey(environmentId, tabId)) === - getRuntimeEnvironmentConnectionGeneration(environmentId) + verdict.generation === getRuntimeEnvironmentConnectionGeneration(environmentId) && + // Why this and not the key alone: the key is a tab id, and the pane behind it can be replaced. + verdict.paneBinding === paneBindingFor(tabId, environmentId) ) } @@ -94,14 +133,14 @@ function recordExpiredWait(environmentId: string, key: string): void { // way round, and both wrong shapes were independently written before this was reconciled. const prefix = `${environmentId}\0` const liveTabs = liveTabIds() - for (const [staleKey, staleGeneration] of expiredGenerationByPane) { + for (const [staleKey, stale] of expiredGenerationByPane) { // GENERATION, judged per key across EVERY environment. `hasHostMirrorHandleWaitExpired` // compares a row against its own environment's CURRENT generation, so a row whose generation // has moved can never return true for anyone. Retiring it cannot cost a reader a verdict, // whoever owns it. Scoped to the recording environment, an environment that reconnects and // then goes quiet strands its rows forever. const staleEnvironmentId = staleKey.slice(0, staleKey.indexOf('\0')) - if (staleGeneration !== getRuntimeEnvironmentConnectionGeneration(staleEnvironmentId)) { + if (stale.generation !== getRuntimeEnvironmentConnectionGeneration(staleEnvironmentId)) { expiredGenerationByPane.delete(staleKey) continue } @@ -116,7 +155,14 @@ function recordExpiredWait(environmentId: string, key: string): void { expiredGenerationByPane.delete(staleKey) } } - expiredGenerationByPane.set(key, generation) + // Why the waiter's park-time binding and not a fresh read: this verdict is about the pane whose + // wait just ran out. Re-reading here would attribute it to whatever holds the id NOW, handing a + // pane that replaced it mid-wait a verdict it never served. The caller must therefore record + // BEFORE `releaseWaiter` deletes the entry; the union suite pins that ordering. + expiredGenerationByPane.set(key, { + generation, + paneBinding: waitersByPane.get(key)?.paneBinding ?? '' + }) } function stopStoreSubscriptionIfIdle(): void { @@ -218,7 +264,14 @@ export function parkUntilHostMirrorHandleLands( } releaseWaiter(key) }, HOST_MIRROR_HANDLE_GAP_DEADLINE_MS) - waitersByPane.set(key, { worktreeId, tabId, generation, deadline, run }) + waitersByPane.set(key, { + worktreeId, + tabId, + generation, + paneBinding: paneBindingFor(tabId, environmentId), + deadline, + run + }) startStoreSubscription() }