From 67dda9affe45ed6525bf9b087aa321d1f40dde49 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 17 Sep 2026 02:02:27 -0700 Subject: [PATCH] fix(runtime): hold the session mirror through an unverifiable probe (#20085) * fix(runtime): hold the session mirror through an unverifiable probe Two derivations read the same host state and reached opposite verdicts, and the destructive one won. When a status probe came back unverifiable over a still-ready transport, runtimeHostConnectionStateForEntry called the host 'runtime-unavailable' (connected) while getReachableRuntimeSessionMirrorTargets dropped it, tearing down and cold-rebuilding the session-tab mirror while the host's flows were still delivering. The root cause is that applyRuntimeHostStatusSnapshot nulls entry.status for any non-verified probe while the snapshot retains the runtime identity. The connection-state reader consults the snapshot; the mirror-target reader did not. Give both readers one answer: - lastVerifiedRuntimeStatus() in shared/runtime-host-status.ts is now the single definition of "the last identity the host answered with". runtime-status.ts already had this inline as previousVerifiedStatus and now calls it. - The mirror-target reader asks the shared connection verdict instead of entry.status, gated on isDisconnectedRuntimeHostState -- only the one exit verdict earns a destructive read, per docs/reference/ssh-execution-boundary.md. 'checking' and 'reconnecting' are unverifiable, not evidence of an exit. Holding the target through the outage would strand the mirror on its own: the subscription is installed by the effect in use-web-session-tabs-sync.ts keyed on useRuntimeSessionMirrorEnvironmentKey(), a stream 'end' frame is dropped without resubscribing, and the parking layer retries only a rejected subscribe call. The teardown was the recovery. So regaining contact now advances the connection epoch, giving recovery its own "the host is back" trigger rather than leaving the mirror to be restored as a side effect of having been destroyed. The connection epoch is not the runtime session: a same-runtime return fires no restart hook, no provider session bump, and no toast. * test(runtime): drop the redundant status casts the new casting gate rejects --- .../runtime-session-mirror-targets.test.ts | 18 +- .../src/lib/runtime-session-mirror-targets.ts | 30 ++- ...e-session-mirror-unverifiable-host.test.ts | 135 ++++++++++++++ ...us-reconnect-connection-generation.test.ts | 171 ++++++++++++++++++ .../slices/runtime-status-snapshot.test.ts | 25 ++- .../src/store/slices/runtime-status.ts | 11 +- src/shared/runtime-host-status.ts | 10 + 7 files changed, 386 insertions(+), 14 deletions(-) create mode 100644 src/renderer/src/lib/runtime-session-mirror-unverifiable-host.test.ts create mode 100644 src/renderer/src/store/slices/runtime-status-reconnect-connection-generation.test.ts diff --git a/src/renderer/src/lib/runtime-session-mirror-targets.test.ts b/src/renderer/src/lib/runtime-session-mirror-targets.test.ts index 23f76306a7d..232cc796fd3 100644 --- a/src/renderer/src/lib/runtime-session-mirror-targets.test.ts +++ b/src/renderer/src/lib/runtime-session-mirror-targets.test.ts @@ -1,6 +1,18 @@ import { describe, expect, it } from 'vitest' +import type { RuntimeStatus } from '../../../shared/runtime-types' import { getReachableRuntimeSessionMirrorTargets } from './runtime-session-mirror-targets' +function makeStatus(runtimeId: string): RuntimeStatus { + return { + runtimeId, + rendererGraphEpoch: 0, + graphStatus: 'ready', + authoritativeWindowId: null, + liveTabCount: 0, + liveLeafCount: 0 + } +} + const environments = [ { id: 'online-env', @@ -24,7 +36,7 @@ describe('getReachableRuntimeSessionMirrorTargets', () => { [ 'online-env', { - status: { runtimeId: 'runtime-online' }, + status: makeStatus('runtime-online'), connectionGeneration: 3 } ], @@ -55,7 +67,7 @@ describe('getReachableRuntimeSessionMirrorTargets', () => { settings: { activeRuntimeEnvironmentId: 'missing-env' }, runtimeEnvironments: environments, runtimeStatusByEnvironmentId: new Map([ - ['missing-env', { status: { runtimeId: 'runtime-missing' } }] + ['missing-env', { status: makeStatus('runtime-missing') }] ]) }) ).toEqual([]) @@ -67,7 +79,7 @@ describe('getReachableRuntimeSessionMirrorTargets', () => { settings: { activeRuntimeEnvironmentId: 'offline-env' }, runtimeEnvironments: environments, runtimeStatusByEnvironmentId: new Map([ - ['offline-env', { status: { runtimeId: 'runtime-recovered' } }] + ['offline-env', { status: makeStatus('runtime-recovered') }] ]) }) ).toEqual([ diff --git a/src/renderer/src/lib/runtime-session-mirror-targets.ts b/src/renderer/src/lib/runtime-session-mirror-targets.ts index 8c3c545ff28..a9cf3aa1a96 100644 --- a/src/renderer/src/lib/runtime-session-mirror-targets.ts +++ b/src/renderer/src/lib/runtime-session-mirror-targets.ts @@ -1,8 +1,19 @@ +import { + lastVerifiedRuntimeStatus, + type RuntimeHostStatusSnapshot +} from '../../../shared/runtime-host-status' +import type { RuntimeStatus } from '../../../shared/runtime-types' +import { + isDisconnectedRuntimeHostState, + runtimeHostConnectionStateForEntry +} from '@/runtime/runtime-host-connection-state' import type { WorktreeRuntimeOwnerState } from './worktree-runtime-owner-state' import { getRuntimeSessionMirrorEnvironmentIds } from './runtime-session-mirror-owners' type RuntimeMirrorStatus = { - status: { runtimeId: string } | null + status: RuntimeStatus | null + remoteControl?: RuntimeStatus['remoteControl'] | null + snapshot?: RuntimeHostStatusSnapshot connectionGeneration?: number } @@ -35,8 +46,17 @@ export function getReachableRuntimeSessionMirrorTargets( ) const targets: RuntimeSessionMirrorTarget[] = [] for (const environmentId of getRuntimeSessionMirrorEnvironmentIds(state)) { - const status = state.runtimeStatusByEnvironmentId?.get(environmentId) - if (!status?.status) { + const entry = state.runtimeStatusByEnvironmentId?.get(environmentId) + // Why the shared verdict and not `entry.status`: a still-ready transport whose probe + // came back unverifiable nulls `entry.status` while the host keeps delivering. Reading + // that as "gone" tore the mirror down mid-flow, disagreeing with every host surface. + // Dropping the mirror is destructive, so only the one exit verdict earns it — + // 'checking' and 'reconnecting' are unverifiable (docs/reference/ssh-execution-boundary.md). + if (isDisconnectedRuntimeHostState(runtimeHostConnectionStateForEntry(entry))) { + continue + } + const runtimeId = lastVerifiedRuntimeStatus(entry)?.runtimeId + if (!runtimeId) { continue } const environment = environmentById.get(environmentId) @@ -45,8 +65,8 @@ export function getReachableRuntimeSessionMirrorTargets( } targets.push({ environmentId, - runtimeId: status.status.runtimeId, - connectionGeneration: status.connectionGeneration ?? 0, + runtimeId, + connectionGeneration: entry?.connectionGeneration ?? 0, pairingRevision: environment.pairingRevision ?? environment.createdAt }) } diff --git a/src/renderer/src/lib/runtime-session-mirror-unverifiable-host.test.ts b/src/renderer/src/lib/runtime-session-mirror-unverifiable-host.test.ts new file mode 100644 index 00000000000..ce4e3c06b4b --- /dev/null +++ b/src/renderer/src/lib/runtime-session-mirror-unverifiable-host.test.ts @@ -0,0 +1,135 @@ +import { describe, expect, it } from 'vitest' +import type { RuntimeHostStatusSnapshot } from '../../../shared/runtime-host-status' +import type { RuntimeStatus } from '../../../shared/runtime-types' +import { + isDisconnectedRuntimeHostState, + runtimeHostConnectionStateForEntry +} from '@/runtime/runtime-host-connection-state' +import { getReachableRuntimeSessionMirrorTargets } from './runtime-session-mirror-targets' + +const ENVIRONMENT_ID = 'env-a' + +const environments = [{ id: ENVIRONMENT_ID, createdAt: 100, pairingRevision: 101 }] + +function makeStatus(runtimeId: string): RuntimeStatus { + return { + runtimeId, + rendererGraphEpoch: 0, + graphStatus: 'ready', + authoritativeWindowId: null, + liveTabCount: 0, + liveLeafCount: 0, + runtimeProtocolVersion: 3, + minCompatibleRuntimeClientVersion: 3 + } +} + +function makeSnapshot( + patch: Partial & Pick +): RuntimeHostStatusSnapshot { + return { + environmentId: ENVIRONMENT_ID, + pairingRevision: 101, + sequence: 1, + checkedAt: 1, + status: makeStatus('rt-1'), + transport: 'ready', + ...patch + } +} + +/** The entry shape `applyRuntimeHostStatusSnapshot` writes for a given snapshot. */ +function entryForSnapshot(snapshot: RuntimeHostStatusSnapshot) { + return { + snapshot, + checkedAt: snapshot.checkedAt, + connectionGeneration: 4, + status: snapshot.verification === 'verified' && !snapshot.retired ? snapshot.status : null + } +} + +function mirrorTargets(entry: ReturnType) { + return getReachableRuntimeSessionMirrorTargets({ + settings: { activeRuntimeEnvironmentId: ENVIRONMENT_ID }, + runtimeEnvironments: environments, + runtimeStatusByEnvironmentId: new Map([[ENVIRONMENT_ID, entry]]) + }) +} + +describe('mirror targets and host connection state agree on one host', () => { + // Regression: an unverifiable status probe over a still-ready transport read as + // "connected" on the host surfaces and as "gone" to the mirror, so the session-tab + // mirror was torn down and cold-rebuilt while the host's flows were still delivering. + // docs/reference/ssh-execution-boundary.md: loss of contact is never evidence of exit. + it('holds the mirror target when a ready transport returns an unverifiable probe', () => { + const entry = entryForSnapshot(makeSnapshot({ verification: 'unavailable' })) + + expect(runtimeHostConnectionStateForEntry(entry)).toBe('runtime-unavailable') + expect(mirrorTargets(entry)).toEqual([ + { + environmentId: ENVIRONMENT_ID, + runtimeId: 'rt-1', + connectionGeneration: 4, + pairingRevision: 101 + } + ]) + }) + + it('holds the mirror target while the transport is reconnecting', () => { + // A dropped transport is unverifiable, not an exit verdict, so it must not be + // the trigger for destroying a mirror whose host may still be running the work. + const entry = entryForSnapshot( + makeSnapshot({ verification: 'unavailable', transport: 'disconnected' }) + ) + + expect(runtimeHostConnectionStateForEntry(entry)).toBe('reconnecting') + expect(mirrorTargets(entry)).toHaveLength(1) + }) + + it('keeps the same target across verified -> unverifiable -> verified', () => { + const verified = mirrorTargets(entryForSnapshot(makeSnapshot({ verification: 'verified' }))) + const unverifiable = mirrorTargets( + entryForSnapshot(makeSnapshot({ verification: 'unavailable', sequence: 2 })) + ) + + expect(verified).toHaveLength(1) + expect(unverifiable).toEqual(verified) + }) + + it.each([ + ['a retired host', makeSnapshot({ verification: 'verified', retired: true })], + ['a blocked host', makeSnapshot({ verification: 'blocked' })] + ])('drops the mirror target for %s', (_label, snapshot) => { + const entry = entryForSnapshot(snapshot) + + expect(isDisconnectedRuntimeHostState(runtimeHostConnectionStateForEntry(entry))).toBe(true) + expect(mirrorTargets(entry)).toEqual([]) + }) + + it('drops the mirror target for a host that has never verified', () => { + // Unverifiable, but there is no runtime identity to mirror yet. + const entry = entryForSnapshot(makeSnapshot({ verification: 'checking', status: null })) + + expect(runtimeHostConnectionStateForEntry(entry)).toBe('checking') + expect(mirrorTargets(entry)).toEqual([]) + }) + + it('drops the mirror target when the control channel closed', () => { + const closed: NonNullable = { + state: 'closed', + pendingRequestCount: 0, + subscriptionCount: 0, + reconnectAttempt: 0, + lastConnectedAt: null, + lastClose: null, + lastError: null + } + const entry = { + ...entryForSnapshot(makeSnapshot({ verification: 'verified' })), + remoteControl: closed + } + + expect(runtimeHostConnectionStateForEntry(entry)).toBe('disconnected') + expect(mirrorTargets(entry)).toEqual([]) + }) +}) diff --git a/src/renderer/src/store/slices/runtime-status-reconnect-connection-generation.test.ts b/src/renderer/src/store/slices/runtime-status-reconnect-connection-generation.test.ts new file mode 100644 index 00000000000..e682b2be2f2 --- /dev/null +++ b/src/renderer/src/store/slices/runtime-status-reconnect-connection-generation.test.ts @@ -0,0 +1,171 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { create } from 'zustand' +import type { RuntimeHostStatusSnapshot } from '../../../../shared/runtime-host-status' +import type { RuntimeStatus } from '../../../../shared/runtime-types' +import { buildRuntimeSessionMirrorEnvironmentKey } from '@/runtime/use-runtime-session-mirror-environment-key' +import { + clearRuntimeEnvironmentConnectionGenerationsForTests, + createRuntimeStatusSlice, + getRuntimeEnvironmentConnectionGeneration, + type RuntimeStatusSlice +} from './runtime-status' + +vi.mock('sonner', () => ({ + toast: { warning: vi.fn(), dismiss: vi.fn() } +})) + +const ENVIRONMENT_ID = 'env-a' +const PAIRING_REVISION = 101 + +type Store = ReturnType + +function createSliceStore() { + return create()((...a) => ({ + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the slice creator is declared against the whole AppState; this store holds only its own slice, which is all the code under test reads. + ...createRuntimeStatusSlice(...(a as unknown as Parameters)) + })) +} + +function makeStatus(runtimeId: string): RuntimeStatus { + return { + runtimeId, + rendererGraphEpoch: 0, + graphStatus: 'ready', + authoritativeWindowId: null, + liveTabCount: 0, + liveLeafCount: 0, + runtimeProtocolVersion: 3, + minCompatibleRuntimeClientVersion: 3 + } +} + +function makeSnapshot( + sequence: number, + patch: Partial & Pick +): RuntimeHostStatusSnapshot { + return { + environmentId: ENVIRONMENT_ID, + pairingRevision: PAIRING_REVISION, + sequence, + checkedAt: sequence, + status: makeStatus('rt-1'), + transport: 'ready', + ...patch + } +} + +/** The mirror-subscription effect dependency, rebuilt from the slice's current state. */ +function mirrorKey(store: Store): string { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the builder takes the whole app state; only the fields below reach the mirror-target scan. + return buildRuntimeSessionMirrorEnvironmentKey({ + activeRuntimeEnvironmentId: ENVIRONMENT_ID, + repos: [], + worktreesByRepo: {}, + detectedWorktreesByRepo: {}, + projectGroups: [], + restoredRuntimeHostIdByWorkspaceSessionKey: {}, + runtimeEnvironments: store.getState().runtimeEnvironments, + runtimeStatusByEnvironmentId: store.getState().runtimeStatusByEnvironmentId + } as Parameters[0]) +} + +function seedEnvironment(store: Store): void { + const endpointId = `ws-${ENVIRONMENT_ID}` + store.setState({ + runtimeEnvironments: [ + { + id: ENVIRONMENT_ID, + name: ENVIRONMENT_ID, + createdAt: 100, + updatedAt: 100, + pairingRevision: PAIRING_REVISION, + lastUsedAt: null, + runtimeId: null, + endpoints: [{ id: endpointId, kind: 'websocket', label: 'WebSocket', endpoint: 'ws://x' }], + preferredEndpointId: endpointId + } + ] + }) +} + +beforeEach(() => { + clearRuntimeEnvironmentConnectionGenerationsForTests() + vi.stubGlobal('window', { api: {}, dispatchEvent: vi.fn() }) +}) + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('regaining contact is its own mirror-recovery trigger', () => { + // The mirror subscription is (re)installed by the effect in + // web-session-tabs-sync/use-web-session-tabs-sync.ts, keyed on + // useRuntimeSessionMirrorEnvironmentKey(). Before this change the only thing that + // moved that key across an outage was the target being dropped and re-added — the + // teardown was the recovery. Holding the target through the outage strands the mirror + // unless regaining contact moves the key on its own. + it('advances the connection generation when a host answers again after an outage', () => { + const store = createSliceStore() + seedEnvironment(store) + + store.getState().applyRuntimeHostStatusSnapshot(makeSnapshot(1, { verification: 'verified' })) + const connectedGeneration = getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID) + const connectedKey = mirrorKey(store) + expect(connectedKey).not.toBe('') + + store + .getState() + .applyRuntimeHostStatusSnapshot(makeSnapshot(2, { verification: 'unavailable' })) + expect(getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID)).toBe(connectedGeneration) + expect(mirrorKey(store)).toBe(connectedKey) + + store.getState().applyRuntimeHostStatusSnapshot(makeSnapshot(3, { verification: 'verified' })) + expect(getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID)).toBe(connectedGeneration + 1) + expect(mirrorKey(store)).not.toBe(connectedKey) + }) + + it('does not advance the generation while the host keeps answering', () => { + const store = createSliceStore() + seedEnvironment(store) + + store.getState().applyRuntimeHostStatusSnapshot(makeSnapshot(1, { verification: 'verified' })) + const connectedGeneration = getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID) + const connectedKey = mirrorKey(store) + + store.getState().applyRuntimeHostStatusSnapshot(makeSnapshot(2, { verification: 'verified' })) + + expect(getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID)).toBe(connectedGeneration) + expect(mirrorKey(store)).toBe(connectedKey) + }) + + it('leaves a first publication with no prior entry on its original generation', () => { + // Regression (#19241): a first contact is not a reconnect, or the generation fence + // retires worktree scans already in flight against that same connection. + const store = createSliceStore() + seedEnvironment(store) + const before = getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID) + + store.getState().applyRuntimeHostStatusSnapshot(makeSnapshot(1, { verification: 'verified' })) + + expect(getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID)).toBe(before) + }) + + it('advances once, not twice, when the runtime restarted during the outage', () => { + const store = createSliceStore() + seedEnvironment(store) + + store.getState().applyRuntimeHostStatusSnapshot(makeSnapshot(1, { verification: 'verified' })) + const connectedGeneration = getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID) + + store + .getState() + .applyRuntimeHostStatusSnapshot(makeSnapshot(2, { verification: 'unavailable' })) + store + .getState() + .applyRuntimeHostStatusSnapshot( + makeSnapshot(3, { verification: 'verified', status: makeStatus('rt-2') }) + ) + + expect(getRuntimeEnvironmentConnectionGeneration(ENVIRONMENT_ID)).toBe(connectedGeneration + 1) + }) +}) diff --git a/src/renderer/src/store/slices/runtime-status-snapshot.test.ts b/src/renderer/src/store/slices/runtime-status-snapshot.test.ts index fa437065b28..737e9831205 100644 --- a/src/renderer/src/store/slices/runtime-status-snapshot.test.ts +++ b/src/renderer/src/store/slices/runtime-status-snapshot.test.ts @@ -10,6 +10,7 @@ import type { RuntimeHostStatusSnapshot } from '../../../../shared/runtime-host- import type { RuntimeStatus } from '../../../../shared/runtime-types' import type { PublicKnownRuntimeEnvironment } from '../../../../shared/runtime-environments' import { runtimeHostConnectionStateForEntry } from '@/runtime/runtime-host-connection-state' +import { ensureBrowserClientHostForRestartedRuntime } from '@/runtime/restored-client-hosted-browser-host-attach' vi.mock('sonner', () => ({ toast: { warning: vi.fn(), dismiss: vi.fn() } })) vi.mock('@/runtime/restored-client-hosted-browser-host-attach', () => ({ @@ -77,17 +78,33 @@ it('represents failed verification honestly without manufacturing a session rest expect( runtimeHostConnectionStateForEntry(viewer.getState().runtimeStatusByEnvironmentId.get('env-a')) ).toBe('runtime-unavailable') - viewer.getState().applyRuntimeHostStatusSnapshot(snapshot(3)) expect(viewer.getState().runtimeStatusByEnvironmentId.get('env-a')?.connectionGeneration).toBe( generation ) + viewer.getState().applyRuntimeHostStatusSnapshot(snapshot(3)) + // The connection epoch is not the runtime session: regaining contact opens a new epoch + // (reads issued before the outage were against the lost connection, and the session + // mirror needs this edge to resume), while the runtime session is unchanged — same + // runtime id, no restart hook, no toast. + expect(viewer.getState().runtimeStatusByEnvironmentId.get('env-a')?.connectionGeneration).toBe( + (generation ?? 0) + 1 + ) + expect(viewer.getState().runtimeStatusByEnvironmentId.get('env-a')?.status?.runtimeId).toBe( + 'rt-1' + ) + expect(ensureBrowserClientHostForRestartedRuntime).not.toHaveBeenCalled() expect(toast.warning).not.toHaveBeenCalled() + const reconnectedGeneration = viewer + .getState() + .runtimeStatusByEnvironmentId.get('env-a')?.connectionGeneration viewer .getState() .applyRuntimeHostStatusSnapshot(snapshot(4, { status: { runtimeId: 'rt-2' } as RuntimeStatus })) - expect( - viewer.getState().runtimeStatusByEnvironmentId.get('env-a')?.connectionGeneration - ).toBeGreaterThan(generation ?? 0) + // A replacement runtime id is a restart, and it advances the epoch exactly once more. + expect(viewer.getState().runtimeStatusByEnvironmentId.get('env-a')?.connectionGeneration).toBe( + (reconnectedGeneration ?? 0) + 1 + ) + expect(ensureBrowserClientHostForRestartedRuntime).toHaveBeenCalled() }) it('retains disconnect ordering and rejects publications for removed or replaced pairings', () => { diff --git a/src/renderer/src/store/slices/runtime-status.ts b/src/renderer/src/store/slices/runtime-status.ts index c1d3e88880a..6816baaa8ca 100644 --- a/src/renderer/src/store/slices/runtime-status.ts +++ b/src/renderer/src/store/slices/runtime-status.ts @@ -2,6 +2,7 @@ import type { StateCreator } from 'zustand' import type { AppState } from '../types' import type { RuntimeStatusSlice } from './runtime-status-types' export type { RuntimeEnvironmentStatus, RuntimeStatusSlice } from './runtime-status-types' +import { lastVerifiedRuntimeStatus } from '../../../../shared/runtime-host-status' import { runtimeEnvironmentStatusesEqual } from './runtime-environment-status-equality' import { clearRecentRuntimeCompatibilityFailure, @@ -171,7 +172,7 @@ export const createRuntimeStatusSlice: StateCreator export function runtimeHostStatusFailure(code: string, message: string): RuntimeRpcFailure {