From 8dcd46b9de7d51e20002442c114c465bbfef2acd Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 05:45:15 -0700 Subject: [PATCH] fix(ssh): retain remote sessions across late catalogs, path collisions, and PTY rotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three losses in the "remote session state never reconciled" cluster, one rule: absence from a client-side set, or a stale client-side expectation, is `unverifiable` by construction and can never authorise removal. #12902 / #15484 — a direct-SSH snapshot whose host paths the local worktree catalog cannot place yet leaves the target in `conflict`, which suppresses uploads and holds terminal authority at `unverifiable`. Nothing re-pulled once the catalog landed, so the tabs stayed missing and the host ledger stayed stale until a reconnect. The apply now reports the paths it dropped and target-sync watches the catalog for them, re-pulling a fresh host snapshot when they become placeable. #15484 — exportRemoteWorkspaceSession keys the host projection by worktree path, which drops the repoId, so two local rows for one remote checkout collapsed and the last one won outright. An empty duplicate row published an empty tab list for a workspace with live panes, and the upload is a wholesale replace-session. Union by tab id instead, matching the `Math.max` its sibling recency map already applied to the same collision. #11495 — orphan recovery retired a leaf whenever a `terminal.list` with `requireFreshPtyLiveness: true` named a different PTY behind a handle than the snapshot frame's pending row did. That is the host attesting the handle is live under a replacement PTY, which is what a host relaunch looks like. Rebind instead of remove. Two tests pinned the removing behaviour and are retargeted with the reasoning. --- ...mote-workspace-deferred-placement-retry.ts | 93 +++++++++++++++++++ .../hooks/remote-workspace-snapshot-apply.ts | 10 +- .../remote-workspace-snapshot-placement.ts | 22 ++++- ...ace-snapshot-unplaced-tab-adoption.test.ts | 11 ++- .../remote-workspace-target-sync-types.ts | 45 +++++++++ .../remote-workspace-target-sync.test.ts | 57 ++++++++++++ .../src/hooks/remote-workspace-target-sync.ts | 86 ++++++----------- ...sion-terminal-orphan-recovery-inventory.ts | 10 +- ...rminal-orphan-recovery-regressions.test.ts | 17 +++- ...n-terminal-pending-handle-recovery.test.ts | 21 ++++- ...emote-workspace-session-projection.test.ts | 66 +++++++++++++ .../remote-workspace-session-projection.ts | 27 +++++- 12 files changed, 392 insertions(+), 73 deletions(-) create mode 100644 src/renderer/src/hooks/remote-workspace-deferred-placement-retry.ts create mode 100644 src/renderer/src/hooks/remote-workspace-target-sync-types.ts diff --git a/src/renderer/src/hooks/remote-workspace-deferred-placement-retry.ts b/src/renderer/src/hooks/remote-workspace-deferred-placement-retry.ts new file mode 100644 index 00000000000..904ae273234 --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-deferred-placement-retry.ts @@ -0,0 +1,93 @@ +import type { DirectSshAuthority } from '../../../shared/ssh-types' +import type { RemoteWorkspaceObservedSnapshot } from '../../../shared/remote-workspace-types' +import { directSshAuthoritiesEqual } from './direct-ssh-reconnect-tokens' +import { + waitForSnapshotWorktreePlacement, + type RemoteWorkspaceSnapshotPlacementStore +} from './remote-workspace-snapshot-placement' + +/** How long a conflicted target keeps watching the catalog for the rows it could not place. The + * catalog for a remote host often only fills in when the user opens the worktree, which is minutes + * after connect, and `conflict` has no other exit: it suppresses uploads and holds terminal + * authority at `unverifiable` until a reconnect or an unsolicited host push happens to arrive. */ +const DEFERRED_SNAPSHOT_PLACEMENT_TIMEOUT_MS = 600_000 + +export type DeferredSnapshotPlacementRetryDeps = { + store: RemoteWorkspaceSnapshotPlacementStore + getCurrentAuthority: (targetId: string) => DirectSshAuthority | null + getSnapshot: (targetId: string) => Promise + applySnapshot: (targetId: string, snapshot: RemoteWorkspaceObservedSnapshot) => Promise +} + +export type DeferredSnapshotPlacementRetries = { + /** Empty `worktreePaths` retires the target's outstanding watch instead of arming one. */ + watch: (authority: DirectSshAuthority, worktreePaths: readonly string[]) => void + stop: () => void +} + +/** + * Re-pull once the local catalog can place the host paths an apply had to drop. + * + * Why a fresh pull rather than replaying the snapshot already held: that snapshot is only evidence + * of what the host had when it was taken. The host owns execution state, so the answer acted on has + * to be its current one — see docs/reference/ssh-execution-boundary.md. A pull that comes back + * empty or fails is left alone: it is `unverifiable`, and moving the target off `conflict` on it + * would re-authorise uploads and sleeping-agent resume from a picture known to be incomplete. + */ +export function createDeferredSnapshotPlacementRetries( + deps: DeferredSnapshotPlacementRetryDeps +): DeferredSnapshotPlacementRetries { + const watchers = new Map() + let stopped = false + + const watch = (authority: DirectSshAuthority, worktreePaths: readonly string[]): void => { + // Always retire the previous watch: an apply that placed everything passes no paths, and that + // is exactly when the outstanding watch is obsolete. + watchers.get(authority.targetId)?.abort() + watchers.delete(authority.targetId) + if (stopped || worktreePaths.length === 0) { + return + } + const controller = new AbortController() + watchers.set(authority.targetId, controller) + const isCurrent = (): boolean => + !stopped && + watchers.get(authority.targetId) === controller && + directSshAuthoritiesEqual(deps.getCurrentAuthority(authority.targetId), authority) + void (async () => { + try { + const placed = await waitForSnapshotWorktreePlacement( + deps.store, + authority, + worktreePaths, + isCurrent, + controller.signal, + DEFERRED_SNAPSHOT_PLACEMENT_TIMEOUT_MS + ) + if (!placed || !isCurrent()) { + return + } + const snapshot = await deps.getSnapshot(authority.targetId) + if (!snapshot || snapshot.revision <= 0 || !isCurrent()) { + return + } + await deps.applySnapshot(authority.targetId, snapshot) + } finally { + if (watchers.get(authority.targetId) === controller) { + watchers.delete(authority.targetId) + } + } + })() + } + + return { + watch, + stop: () => { + stopped = true + for (const controller of watchers.values()) { + controller.abort() + } + watchers.clear() + } + } +} diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts b/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts index 024fb3f51c0..af2e845ab51 100644 --- a/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts +++ b/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts @@ -79,6 +79,12 @@ type RemoteWorkspaceSnapshotApplyInput = { isPreparationTokenCurrent: (token: DirectSshPreparationToken) => boolean waitForWorkspaceSessionReady: (signal?: AbortSignal) => Promise finalizeHydratedTerminals: (authority: DirectSshAuthority) => number + /** + * Host paths still carrying terminal tabs when this apply gave up placing them. `unverifiable`, + * never proof the rows are not ours, so the caller owns getting back to a placed picture — this + * apply itself has no way back once the bounded in-apply wait expires. + */ + onUnplacedTabWorktreePaths?: (worktreePaths: readonly string[]) => void } export type RemoteWorkspaceSnapshotApplyResult = 'applied' | 'stale' | 'failed' @@ -115,7 +121,8 @@ export async function applyDirectSshRemoteWorkspaceSnapshot({ isArrivalCurrent, isPreparationTokenCurrent, waitForWorkspaceSessionReady, - finalizeHydratedTerminals + finalizeHydratedTerminals, + onUnplacedTabWorktreePaths }: RemoteWorkspaceSnapshotApplyInput): Promise { const { authority } = token if (!isArrivalCurrent(authority.targetId, arrival)) { @@ -181,6 +188,7 @@ export async function applyDirectSshRemoteWorkspaceSnapshot({ return 'stale' } const hasUnplacedTerminalTabs = unplacedTabWorktreePaths.length > 0 + onUnplacedTabWorktreePaths?.([...unplacedTabWorktreePaths]) snapshotApplyDepth += 1 try { const currentStore = store.getState() diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-placement.ts b/src/renderer/src/hooks/remote-workspace-snapshot-placement.ts index 0d280bc6d9e..29925baa4cb 100644 --- a/src/renderer/src/hooks/remote-workspace-snapshot-placement.ts +++ b/src/renderer/src/hooks/remote-workspace-snapshot-placement.ts @@ -39,6 +39,23 @@ export function resolveDirectSshSnapshotWorktreeIds( return worktreeIds } +/** Only the rows the snapshot is allowed to replace, with no host-qualified widening. */ +export function resolveExactDirectSshTargetWorktreeIds( + state: AppState, + authority: DirectSshAuthority +): Set { + return resolveDirectSshTargetScope({ + targetId: authority.targetId, + catalogRevision: 0, + repos: state.repos, + worktreesByRepo: state.worktreesByRepo, + detectedWorktreesByRepo: state.detectedWorktreesByRepo, + folderWorkspaces: state.folderWorkspaces, + projectGroups: state.projectGroups, + restoredRuntimeHostIdByWorkspaceSessionKey: state.restoredRuntimeHostIdByWorkspaceSessionKey + }).gitWorktreeIds +} + function snapshotPathsArePlaceable( state: AppState, authority: DirectSshAuthority, @@ -85,7 +102,8 @@ export async function waitForSnapshotWorktreePlacement( authority: DirectSshAuthority, worktreePaths: readonly string[], isCurrent: () => boolean, - signal?: AbortSignal + signal?: AbortSignal, + timeoutMs: number = SNAPSHOT_WORKTREE_PLACEMENT_TIMEOUT_MS ): Promise { if (signal?.aborted || !isCurrent()) { return false @@ -117,7 +135,7 @@ export async function waitForSnapshotWorktreePlacement( resolve(placed) } const onAbort = (): void => finish(false) - timer = setTimeout(() => finish(false), SNAPSHOT_WORKTREE_PLACEMENT_TIMEOUT_MS) + timer = setTimeout(() => finish(false), timeoutMs) signal?.addEventListener('abort', onAbort, { once: true }) const subscribedUnsubscribe = store.subscribe((state) => { if (!isCurrent()) { diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-unplaced-tab-adoption.test.ts b/src/renderer/src/hooks/remote-workspace-snapshot-unplaced-tab-adoption.test.ts index 6be1721438a..13daf0c9f20 100644 --- a/src/renderer/src/hooks/remote-workspace-snapshot-unplaced-tab-adoption.test.ts +++ b/src/renderer/src/hooks/remote-workspace-snapshot-unplaced-tab-adoption.test.ts @@ -12,8 +12,10 @@ * * The oracle is the promotion, not the drop. Without a local catalog there is nowhere to put the * rows, and that is fine and recoverable — what is not recoverable is declaring the empty result - * authoritative, because nothing re-pulls after the lineage lands. An unplaceable row is - * `unverifiable`, never `exited` (docs/reference/ssh-execution-boundary.md). + * authoritative. An unplaceable row is `unverifiable`, never `exited` + * (docs/reference/ssh-execution-boundary.md). Getting back to a placed picture is the caller's job: + * this apply has no way back once its bounded wait expires, so it reports the paths it dropped and + * remote-workspace-target-sync.ts re-pulls when the catalog can place them. * * The catalog-present case is pinned alongside it so the gate cannot be satisfied by never * hydrating anything. @@ -275,7 +277,8 @@ describe('a host snapshot whose terminal tabs cannot be placed locally', () => { expect(adoptedTabIds(store), 'no local worktree row exists to hang the host tabs on').toEqual( [] ) - // Not recoverable: promoting that to truth. Nothing re-pulls once the lineage lands. + // Not recoverable: promoting that to truth. This apply never re-pulls on its own; the deferred + // placement watch in remote-workspace-target-sync.ts is what re-pulls once the lineage lands. expect( isHydrated(store), 'the host named 3 terminals and this client placed none of them, so the target is not hydrated' @@ -335,7 +338,7 @@ describe('a host snapshot whose terminal tabs cannot be placed locally', () => { const store = createStore() // First pass: the lineage read was degraded, so nothing places and the target is left - // un-hydrated on purpose. With the retry chain gone, this is the only way back. + // un-hydrated on purpose. A later snapshot is how this apply, on its own, gets back. await applySnapshot(store, snapshot(1)) expect(adoptedTabIds(store)).toEqual([]) expect(isHydrated(store)).toBe(false) diff --git a/src/renderer/src/hooks/remote-workspace-target-sync-types.ts b/src/renderer/src/hooks/remote-workspace-target-sync-types.ts new file mode 100644 index 00000000000..40d5b488c41 --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-target-sync-types.ts @@ -0,0 +1,45 @@ +import type { + RemoteWorkspaceObservedPatchResult, + RemoteWorkspaceObservedSnapshot +} from '../../../shared/remote-workspace-types' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import type { DirectSshAuthority } from '../../../shared/ssh-types' +import type { + DirectSshPreparationInput, + DirectSshPreparationOutcome, + DirectSshPreparationToken +} from './direct-ssh-reconnect-coordinator' +import type { RemoteWorkspaceSnapshotPlacementStore } from './remote-workspace-snapshot-placement' + +export type RemoteWorkspaceApi = { + get: (args: { targetId: string }) => Promise + setForConnectedTargets: (args: { + session?: WorkspaceSessionState + hydratedTargetIds?: string[] + expectedRevisionsByTargetId: Record + expectedHostObservationTokensByTargetId: Record + }) => Promise<{ targetId: string; result: RemoteWorkspaceObservedPatchResult }[]> +} + +export type RemoteWorkspaceTargetSyncDeps = { + store: RemoteWorkspaceSnapshotPlacementStore + remoteWorkspace: RemoteWorkspaceApi + getCurrentAuthority: (targetId: string) => DirectSshAuthority | null + isPreparationTokenCurrent: (token: DirectSshPreparationToken) => boolean + capturePreparationInput: ( + authority: DirectSshAuthority, + reason: 'workspace-snapshot', + snapshotRevision: number + ) => Promise + prepareOnly: (input: DirectSshPreparationInput) => Promise + finalizeHydratedTerminals: (authority: DirectSshAuthority) => number +} + +export type RemoteWorkspaceTargetSync = { + syncAfterConnect: (token: DirectSshPreparationToken) => Promise + applyUnsolicitedSnapshot: ( + targetId: string, + snapshot: RemoteWorkspaceObservedSnapshot + ) => Promise + stop: () => void +} diff --git a/src/renderer/src/hooks/remote-workspace-target-sync.test.ts b/src/renderer/src/hooks/remote-workspace-target-sync.test.ts index b3242077052..bc0082c03ce 100644 --- a/src/renderer/src/hooks/remote-workspace-target-sync.test.ts +++ b/src/renderer/src/hooks/remote-workspace-target-sync.test.ts @@ -457,6 +457,63 @@ describe('createRemoteWorkspaceTargetSync', () => { expect(clearRemoteWorkspaceHydrated).toHaveBeenCalledWith('target-a') }) + it('re-pulls a conflicted target once the catalog can place the host tabs it dropped', async () => { + vi.useFakeTimers() + const hydrateTabsSession = vi.fn() + const markRemoteWorkspaceHydrated = vi.fn() + const state = appState({ + worktreesByRepo: {}, + hydrateTabsSession, + markRemoteWorkspaceHydrated + }) + const incoming = snapshot(12, { + '/remote/work': [ + { + id: 'host-tab', + worktreePath: '/remote/work', + ptyId: 'ssh:target-a@@pty-1' + } as RemoteWorkspaceSnapshot['session']['tabsByWorktreePath'][string][number] + ] + }) + const get = vi.fn(async () => incoming) + const harness = createHarness(state, get) + try { + const pending = harness.sync.applyUnsolicitedSnapshot('target-a', incoming) + await flush() + // The bounded in-apply wait expires with the host's path still unplaceable. + await vi.advanceTimersByTimeAsync(10_000) + await pending + expect( + markRemoteWorkspaceHydrated, + 'adopting none of the host tabs is not the host picture' + ).not.toHaveBeenCalled() + expect( + get, + 'nothing should re-pull while the path is still unplaceable' + ).not.toHaveBeenCalled() + + // Minutes later the user opens the worktree and its catalog row finally lands. + state.worktreesByRepo = appState().worktreesByRepo + harness.publishState() + await vi.advanceTimersByTimeAsync(0) + await flush() + await vi.advanceTimersByTimeAsync(0) + + expect(get).toHaveBeenCalledWith({ targetId: 'target-a' }) + expect( + hydrateTabsSession.mock.calls + .at(-1)?.[0] + .tabsByWorktree['repo-a::/remote/work'].map((tab: { id: string }) => tab.id), + 'the host tab dropped by the cold-catalog pull was never hydrated' + ).toEqual(['host-tab']) + // Hydration is what lifts the upload suppression, so the host ledger stops going stale too. + expect(markRemoteWorkspaceHydrated).toHaveBeenCalledWith('target-a') + } finally { + harness.sync.stop() + vi.useRealTimers() + } + }) + it('keeps only the latest placement waiter and fences a burst to the newest snapshot', async () => { vi.useFakeTimers() const hydrateTabsSession = vi.fn() diff --git a/src/renderer/src/hooks/remote-workspace-target-sync.ts b/src/renderer/src/hooks/remote-workspace-target-sync.ts index a6c2a1a3e77..c3d99224dbc 100644 --- a/src/renderer/src/hooks/remote-workspace-target-sync.ts +++ b/src/renderer/src/hooks/remote-workspace-target-sync.ts @@ -1,78 +1,40 @@ -import type { StoreApi } from 'zustand' -import type { - RemoteWorkspaceObservedPatchResult, - RemoteWorkspaceObservedSnapshot -} from '../../../shared/remote-workspace-types' -import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import type { RemoteWorkspaceObservedSnapshot } from '../../../shared/remote-workspace-types' import type { DirectSshAuthority } from '../../../shared/ssh-types' import { translate } from '@/i18n/i18n' import { buildWorkspaceSessionPayload } from '../lib/workspace-session' -import type { AppState } from '../store/types' import type { - DirectSshPreparationInput, - DirectSshPreparationOutcome, DirectSshPreparationToken, DirectSshSnapshotApplyToken } from './direct-ssh-reconnect-coordinator' import { buildDirectSshSnapshotApplyToken } from './direct-ssh-reconnect-coordinator' -import { resolveDirectSshTargetScope } from '../lib/direct-ssh-target-scope' +import { resolveExactDirectSshTargetWorktreeIds } from './remote-workspace-snapshot-placement' import { applyDirectSshRemoteWorkspaceSnapshot } from './remote-workspace-snapshot-apply' import { createRemoteWorkspaceSnapshotArrivalCoordinator } from './remote-workspace-snapshot-arrival-coordinator' +import { createDeferredSnapshotPlacementRetries } from './remote-workspace-deferred-placement-retry' import { applyRemoteWorkspacePushStatus } from './remote-workspace-push-status' import { waitForRemoteWorkspaceSessionReady } from './remote-workspace-session-readiness' +import type { + RemoteWorkspaceTargetSync, + RemoteWorkspaceTargetSyncDeps +} from './remote-workspace-target-sync-types' + +export type { + RemoteWorkspaceTargetSync, + RemoteWorkspaceTargetSyncDeps +} from './remote-workspace-target-sync-types' const MAX_SNAPSHOT_APPLY_ATTEMPTS = 3 -type RemoteWorkspaceApi = { - get: (args: { targetId: string }) => Promise - setForConnectedTargets: (args: { - session?: WorkspaceSessionState - hydratedTargetIds?: string[] - expectedRevisionsByTargetId: Record - expectedHostObservationTokensByTargetId: Record - }) => Promise<{ targetId: string; result: RemoteWorkspaceObservedPatchResult }[]> -} - -export type RemoteWorkspaceTargetSyncDeps = { - store: Pick, 'getState'> & Partial, 'subscribe'>> - remoteWorkspace: RemoteWorkspaceApi - getCurrentAuthority: (targetId: string) => DirectSshAuthority | null - isPreparationTokenCurrent: (token: DirectSshPreparationToken) => boolean - capturePreparationInput: ( - authority: DirectSshAuthority, - reason: 'workspace-snapshot', - snapshotRevision: number - ) => Promise - prepareOnly: (input: DirectSshPreparationInput) => Promise - finalizeHydratedTerminals: (authority: DirectSshAuthority) => number -} - -export type RemoteWorkspaceTargetSync = { - syncAfterConnect: (token: DirectSshPreparationToken) => Promise - applyUnsolicitedSnapshot: ( - targetId: string, - snapshot: RemoteWorkspaceObservedSnapshot - ) => Promise - stop: () => void -} - -function exactTargetWorktreeIds(state: AppState, authority: DirectSshAuthority): Set { - return resolveDirectSshTargetScope({ - targetId: authority.targetId, - catalogRevision: 0, - repos: state.repos, - worktreesByRepo: state.worktreesByRepo, - detectedWorktreesByRepo: state.detectedWorktreesByRepo, - folderWorkspaces: state.folderWorkspaces, - projectGroups: state.projectGroups, - restoredRuntimeHostIdByWorkspaceSessionKey: state.restoredRuntimeHostIdByWorkspaceSessionKey - }).gitWorktreeIds -} - export function createRemoteWorkspaceTargetSync( deps: RemoteWorkspaceTargetSyncDeps ): RemoteWorkspaceTargetSync { const arrivals = createRemoteWorkspaceSnapshotArrivalCoordinator() + const deferredPlacementRetries = createDeferredSnapshotPlacementRetries({ + store: deps.store, + getCurrentAuthority: deps.getCurrentAuthority, + getSnapshot: (targetId) => deps.remoteWorkspace.get({ targetId }), + applySnapshot: (targetId, snapshot) => applyUnsolicitedSnapshot(targetId, snapshot) + }) const isArrivalCurrent = arrivals.isCurrent @@ -104,6 +66,7 @@ export function createRemoteWorkspaceTargetSync( ): Promise => { let applyToken = initialToken for (let attempt = 0; attempt < MAX_SNAPSHOT_APPLY_ATTEMPTS; attempt += 1) { + let unplacedTabWorktreePaths: readonly string[] = [] const result = await applyDirectSshRemoteWorkspaceSnapshot({ store: deps.store, snapshot, @@ -114,8 +77,14 @@ export function createRemoteWorkspaceTargetSync( isPreparationTokenCurrent: deps.isPreparationTokenCurrent, waitForWorkspaceSessionReady: (signal) => waitForRemoteWorkspaceSessionReady(deps.store, signal), - finalizeHydratedTerminals: deps.finalizeHydratedTerminals + finalizeHydratedTerminals: deps.finalizeHydratedTerminals, + onUnplacedTabWorktreePaths: (worktreePaths) => { + unplacedTabWorktreePaths = worktreePaths + } }) + if (result === 'applied') { + deferredPlacementRetries.watch(authority, unplacedTabWorktreePaths) + } if (result !== 'stale' || !isArrivalCurrent(authority.targetId, arrival)) { return } @@ -172,7 +141,7 @@ export function createRemoteWorkspaceTargetSync( return } const stateBeforeGet = deps.store.getState() - const worktreeIds = exactTargetWorktreeIds(stateBeforeGet, authority) + const worktreeIds = resolveExactDirectSshTargetWorktreeIds(stateBeforeGet, authority) const hasLocalTabs = [...worktreeIds].some( (worktreeId) => (stateBeforeGet.tabsByWorktree[worktreeId] ?? []).length > 0 ) @@ -307,6 +276,7 @@ export function createRemoteWorkspaceTargetSync( syncAfterConnect, applyUnsolicitedSnapshot, stop: () => { + deferredPlacementRetries.stop() arrivals.stop() } } diff --git a/src/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory.ts b/src/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory.ts index f314dfb19d1..422a741aec8 100644 --- a/src/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory.ts +++ b/src/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory.ts @@ -202,7 +202,15 @@ export async function resolveTerminalOrphanInventory(args: { ) { disposition = 'retain' } else if (surface.pending && terminal.ptyId !== surface.expectedPtyId) { - disposition = 'remove' + // Rebind, never retire. `expectedPtyId` is the ptyId of the *snapshot frame's* pending row; + // `terminal.ptyId` is the answer to a `terminal.list` with `requireFreshPtyLiveness: true`, so + // the host has just attested this handle is live under a different PTY. A PTY id changing + // across a host relaunch is the normal case, not evidence of death (#11495). Retaining emits a + // ready row bound to the host's handle, which is the rebind — the handle is the identity, the + // ptyId behind it is the host's business. Removal here needs what the two branches around it + // already require: an explicit `retiredTerminalSurfaces` entry, or two authoritative + // inventories omitting the identity. See docs/reference/ssh-execution-boundary.md. + disposition = 'retain' } else if (!hasStrongOrphanIdentity(terminal, surface, snapshot.worktree)) { disposition = 'retain' } else { diff --git a/src/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.ts b/src/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.ts index 9e995de8776..5ff58f9913a 100644 --- a/src/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.ts +++ b/src/renderer/src/runtime/web-session-terminal-orphan-recovery-regressions.test.ts @@ -23,7 +23,16 @@ describe('web session terminal orphan recovery regressions', () => { const ROOTLESS_SOLE_MAP_LEAF = '22222222-2222-4222-8222-222222222222' const ROOTLESS_OFF_TREE_LEAF = '33333333-3333-4333-8333-333333333333' - it('removes only a PTY-mismatched leaf while retaining an unresolved sibling and other tabs', async () => { + // Retargeted (#11495). This previously asserted the mismatched leaf was REMOVED, and that was + // deliberate — it kept a leaf whose pending row named a ptyId the host no longer reported from + // lingering. But `terminal.list` ran with `requireFreshPtyLiveness: true` and answered with the + // handle live under `pty-replacement`, so the only thing the old assertion proved was that the + // host had relaunched the PTY. Stale-leaf accumulation is already covered by the two host-attested + // branches in the same file (`retiredTerminalSurfaces` proof, and two authoritative inventories + // omitting the identity), which is why this branch is the outlier. Removing on it is destructive: + // shouldReplaceTerminalTab rebuilds the whole mirror from one frame, so a dropped leaf takes + // ptyIdsByTabId and terminalLayoutsByTabId with it — the only surviving record of how to rebind. + it('rebinds a PTY-mismatched leaf to the handle the host still reports live', async () => { const worktree = 'repo::mismatch' const leaves = [ { leafId: 'leaf-bad', handle: 'term-bad' }, @@ -70,6 +79,12 @@ describe('web session terminal orphan recovery regressions', () => { expect(recovered?.tabs).toEqual([ browser, + expect.objectContaining({ + parentTabId: tabId, + leafId: 'leaf-bad', + status: 'ready', + terminal: 'term-bad' + }), expect.objectContaining({ parentTabId: tabId, leafId: 'leaf-hold', diff --git a/src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts b/src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts index 48bd556e958..1411200b14c 100644 --- a/src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts +++ b/src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts @@ -564,7 +564,13 @@ describe('web session pending terminal handle recovery', () => { ) }) - it('quarantines a cached handle that now names a different PTY', async () => { + // Retargeted (#11495): this pinned the quarantine, and the quarantine was the bug. The listing + // ran with `requireFreshPtyLiveness: true` and came back `orphaned: true` under a replacement + // PTY, which is a host attestation that the handle is LIVE — a PTY id rotating across a host + // relaunch is the normal case, not evidence of death. The row stays bound to the handle, which is + // the identity the host answers on; the stale `ptyId` on the carried-over row settles on the next + // frame. See web-session-terminal-orphan-recovery-inventory.ts. + it('rebinds a cached handle the host now serves from a different PTY', async () => { const snapshot = pendingSnapshot() const call = vi.fn(async () => ({ ok: true as const, @@ -589,7 +595,18 @@ describe('web session pending terminal handle recovery', () => { ENVIRONMENT_ID, { call: call as never } ) - ).resolves.toEqual(expect.objectContaining({ tabs: [] })) + ).resolves.toEqual( + expect.objectContaining({ + tabs: [ + expect.objectContaining({ + parentTabId: HOST_TAB_ID, + leafId: LEAF_ID, + status: 'ready', + terminal: TERMINAL_HANDLE + }) + ] + }) + ) expect(call).toHaveBeenCalledOnce() }) diff --git a/src/shared/remote-workspace-session-projection.test.ts b/src/shared/remote-workspace-session-projection.test.ts index a22f4123d25..fdccd75b8e9 100644 --- a/src/shared/remote-workspace-session-projection.test.ts +++ b/src/shared/remote-workspace-session-projection.test.ts @@ -200,6 +200,72 @@ describe('remote workspace session projection', () => { expect(Object.keys(session.tabsByWorktree)).toEqual(['repo-b::/srv/app']) }) + it('unions two local repo rows that collapse onto one host path instead of clobbering', () => { + // Duplicate repo rows for one remote checkout are the normal state while a host catalog + // reconciles. `worktreePathFromId` drops the repoId, so both keys project onto '/srv/app'; a + // freshly created empty row iterating last used to publish an empty tab list for a workspace + // the user had panes open in, and the upload is a wholesale replace-session (#15484). + const session = { + ...getDefaultWorkspaceSession(), + activeWorktreeId: 'repo-old::/srv/app', + activeTabId: 'tab-live', + tabsByWorktree: { + 'repo-old::/srv/app': [ + { + id: 'tab-live', + ptyId: 'pty-live', + worktreeId: 'repo-old::/srv/app', + title: 'Agent', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ], + 'repo-new::/srv/app': [] + }, + activeTabIdByWorktree: { + 'repo-old::/srv/app': 'tab-live', + 'repo-new::/srv/app': null + }, + remoteSessionIdsByTabId: { 'tab-live': 'pty-live' } + } + + const projected = exportRemoteWorkspaceSession(session, { + isTargetWorktree: () => true + }) + + expect( + projected.tabsByWorktreePath['/srv/app']?.map((tab) => tab.id), + 'the empty twin row erased a live pane from the host ledger' + ).toEqual(['tab-live']) + expect(projected.activeTabIdByWorktreePath?.['/srv/app']).toBe('tab-live') + expect(projected.remoteSessionIdsByTabId).toEqual({ 'tab-live': 'pty-live' }) + }) + + it('keeps one row per tab id when colliding local keys share a tab', () => { + const tab = { + id: 'tab-shared', + ptyId: 'pty-shared', + title: 'Agent', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + const session = { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + 'repo-old::/srv/app': [{ ...tab, worktreeId: 'repo-old::/srv/app' }], + 'repo-new::/srv/app': [{ ...tab, worktreeId: 'repo-new::/srv/app' }] + } + } + + const projected = exportRemoteWorkspaceSession(session, { isTargetWorktree: () => true }) + + expect(projected.tabsByWorktreePath['/srv/app']?.map((row) => row.id)).toEqual(['tab-shared']) + }) + it('does not report unplaced tabs when every host path resolves', () => { const unplaced: string[] = [] diff --git a/src/shared/remote-workspace-session-projection.ts b/src/shared/remote-workspace-session-projection.ts index 0b0066c1ed0..7f923050d36 100644 --- a/src/shared/remote-workspace-session-projection.ts +++ b/src/shared/remote-workspace-session-projection.ts @@ -59,10 +59,25 @@ export function exportRemoteWorkspaceSession( if (!worktreePath) { continue } - tabsByWorktreePath[worktreePath] = tabs.map((tab) => { + // Why union rather than assignment: `worktreePathFromId` drops the repoId, so two local keys + // for one host path — duplicate repo rows for the same remote checkout, which is the normal + // state while a host catalog reconciles — collapse onto one entry here. Assignment let + // whichever key came last win outright, and an empty twin published an empty tab list for a + // workspace the user had panes open in (#15484). This projection is uploaded as a wholesale + // replace-session, so a clobbered entry deletes those tabs from the host snapshot. The host has + // one workspace at that path, so the union deduped by tab id is the only lossless answer. Same + // collision the `Math.max` below folds for `lastVisitedAtByWorktreePath`. + const merged = tabsByWorktreePath[worktreePath] ?? [] + const alreadyProjected = new Set(merged.map((tab) => tab.id)) + for (const tab of tabs) { + if (alreadyProjected.has(tab.id)) { + continue + } + alreadyProjected.add(tab.id) terminalTabIds.add(tab.id) - return tabToRemote(tab, worktreePath) - }) + merged.push(tabToRemote(tab, worktreePath)) + } + tabsByWorktreePath[worktreePath] = merged } const activeWorktreePath = @@ -80,7 +95,11 @@ export function exportRemoteWorkspaceSession( } const worktreePath = worktreePathFromId(worktreeId) if (worktreePath) { - activeTabIdByWorktreePath[worktreePath] = tabId && terminalTabIds.has(tabId) ? tabId : null + // Same path collision as the tab lists above: a colliding key's null must not erase the + // active tab the other key named. + const resolved = tabId && terminalTabIds.has(tabId) ? tabId : null + activeTabIdByWorktreePath[worktreePath] = + resolved ?? activeTabIdByWorktreePath[worktreePath] ?? null } }