From 1b4a37d2b887357bcde7f2ff77ab4b283d21a2db Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 00:37:33 -0700 Subject: [PATCH] fix(ssh): keep the restored relay session id when a tab has not rebound yet Hydration nulls tab.ptyId, empties ptyIdsByTabId, and never restores lastKnownRelayPtyIdByTabId, so between restore and rebind the persistence layer saw no evidence a relay-backed tab owned a session and dropped both remoteSessionIdsByTabId and activeWorktreeIdsOnShutdown - overwriting the handle the local file and the relay snapshot still held. Losing them is self-reinforcing: the next startup has nothing left to reconnect from. Count the two reconnect maps the orphan sweep and retirement planning already treat as live ownership. The !tab.ptyId sleep guard is unchanged. Fixes #17743 --- .../src/lib/workspace-session-patch.ts | 2 + .../workspace-session-relevant-fields.test.ts | 4 +- src/renderer/src/lib/workspace-session.ts | 21 ++- .../restored-relay-session-identity.test.ts | 143 ++++++++++++++++++ 4 files changed, 166 insertions(+), 4 deletions(-) create mode 100644 src/renderer/src/store/terminals/restored-relay-session-identity.test.ts diff --git a/src/renderer/src/lib/workspace-session-patch.ts b/src/renderer/src/lib/workspace-session-patch.ts index 17fd11a7787..4e56654a8f5 100644 --- a/src/renderer/src/lib/workspace-session-patch.ts +++ b/src/renderer/src/lib/workspace-session-patch.ts @@ -77,6 +77,8 @@ export function buildWorkspaceSessionPatch( 'tabsByWorktree', 'ptyIdsByTabId', 'lastKnownRelayPtyIdByTabId', + 'pendingReconnectPtyIdByTabId', + 'deferredSshSessionIdsByTabId', 'repos', 'worktreesByRepo' ] as const) diff --git a/src/renderer/src/lib/workspace-session-relevant-fields.test.ts b/src/renderer/src/lib/workspace-session-relevant-fields.test.ts index 25c96b3bd2e..2dc1346f534 100644 --- a/src/renderer/src/lib/workspace-session-relevant-fields.test.ts +++ b/src/renderer/src/lib/workspace-session-relevant-fields.test.ts @@ -37,7 +37,9 @@ describe('SESSION_RELEVANT_FIELDS', () => { defaultTerminalTabsAppliedByWorktreeId: true, closedTerminalTabTombstonesByTabId: true, sleepingAgentSessionsByPaneKey: true, - clientHostedBrowserCloseIntentsByEnvironment: true + clientHostedBrowserCloseIntentsByEnvironment: true, + pendingReconnectPtyIdByTabId: true, + deferredSshSessionIdsByTabId: true } it('contains every key of WorkspaceSessionSnapshot', () => { diff --git a/src/renderer/src/lib/workspace-session.ts b/src/renderer/src/lib/workspace-session.ts index 31ddc63704a..330a150b636 100644 --- a/src/renderer/src/lib/workspace-session.ts +++ b/src/renderer/src/lib/workspace-session.ts @@ -61,6 +61,9 @@ export type WorkspaceSessionSnapshot = Pick< activeWorkspaceExecutionHostId?: AppState['activeWorkspaceExecutionHostId'] sleepingAgentSessionsByPaneKey?: AppState['sleepingAgentSessionsByPaneKey'] clientHostedBrowserCloseIntentsByEnvironment?: AppState['clientHostedBrowserCloseIntentsByEnvironment'] + /** Optional so the many partial snapshot fixtures keep type-checking; see buildTerminalSessionData. */ + pendingReconnectPtyIdByTabId?: AppState['pendingReconnectPtyIdByTabId'] + deferredSshSessionIdsByTabId?: AppState['deferredSshSessionIdsByTabId'] } // Why: shallow-equality gate for the debounced session writer; _exhaustive below keeps it in sync with the snapshot type. @@ -97,7 +100,9 @@ export const SESSION_RELEVANT_FIELDS = [ 'defaultTerminalTabsAppliedByWorktreeId', 'closedTerminalTabTombstonesByTabId', 'sleepingAgentSessionsByPaneKey', - 'clientHostedBrowserCloseIntentsByEnvironment' + 'clientHostedBrowserCloseIntentsByEnvironment', + 'pendingReconnectPtyIdByTabId', + 'deferredSshSessionIdsByTabId' ] as const satisfies readonly (keyof WorkspaceSessionSnapshot)[] type _MissingSessionField = Exclude< @@ -222,8 +227,18 @@ export function buildTerminalSessionData( // Why: relay reconnect keeps lastKnown but clears tab.ptyId; the !tab.ptyId guard excludes slept tabs (which keep ptyId as a wake hint). const lastKnown = snapshot.lastKnownRelayPtyIdByTabId + // Why the two reconnect maps (#17743): hydration nulls tab.ptyId, empties ptyIdsByTabId, and + // never restores lastKnown, so on a fresh process they are the ONLY surviving handle for a + // relay-backed tab between restore and rebind. Persisting without them republishes the nulled + // row over the id the file (and the relay snapshot) still held, which is the client's own + // bookkeeping being read as evidence the remote PTY is gone. Both already count as live + // ownership for the orphan sweep (terminal-orphan-helpers) and for retirement planning. + const pendingReconnect = snapshot.pendingReconnectPtyIdByTabId ?? {} + const deferredSshSessions = snapshot.deferredSshSessionIdsByTabId ?? {} + const restoredSessionId = (tabId: string): string | undefined => + lastKnown[tabId] || pendingReconnect[tabId] || deferredSshSessions[tabId] const hasReconnectableSession = (tab: { id: string; ptyId: string | null }): boolean => - hasLivePty(tab.id) || (!tab.ptyId && Boolean(lastKnown[tab.id])) + hasLivePty(tab.id) || (!tab.ptyId && Boolean(restoredSessionId(tab.id))) const activeWorktreeIdsOnShutdown = Object.entries(tabsByWorktree) .filter(([, tabs]) => tabs.some(hasReconnectableSession)) @@ -249,7 +264,7 @@ export function buildTerminalSessionData( if (!hasReconnectableSession(tab)) { continue } - const sessionId = tab.ptyId || lastKnown[tab.id] + const sessionId = tab.ptyId || restoredSessionId(tab.id) if (sessionId) { remoteSessionIdsByTabId[tab.id] = sessionId } diff --git a/src/renderer/src/store/terminals/restored-relay-session-identity.test.ts b/src/renderer/src/store/terminals/restored-relay-session-identity.test.ts new file mode 100644 index 00000000000..e7e2ed15296 --- /dev/null +++ b/src/renderer/src/store/terminals/restored-relay-session-identity.test.ts @@ -0,0 +1,143 @@ +import { describe, expect, it } from 'vitest' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' +import { buildWorkspaceSessionPayload } from '@/lib/workspace-session' +import { getOrphanTerminalIds } from '../slices/terminal-orphan-helpers' +import { createTestStore, makeTab, makeWorktree } from '../slices/store-test-helpers' + +const TARGET_ID = 'target' +const REPO_ID = 'repo-ssh' +const WORKTREE_ID = `${REPO_ID}::/work/demo` +const TAB_ID = 'tab-ssh' +const RELAY_PTY_ID = 'ssh:target@@pty-42' + +function connectedSshState(status: 'connected' | 'disconnected') { + return new Map([ + [TARGET_ID, { targetId: TARGET_ID, status, error: null, reconnectAttempt: 0 }] + ]) as never +} + +function seedStore(status: 'connected' | 'disconnected' = 'connected') { + const store = createTestStore() + store.setState({ + repos: [ + { + id: REPO_ID, + path: '/work/demo', + displayName: 'demo', + badgeColor: '#000', + addedAt: 1, + connectionId: TARGET_ID, + executionHostId: 'ssh:target' + } + ], + worktreesByRepo: { + [REPO_ID]: [ + makeWorktree({ + id: WORKTREE_ID, + repoId: REPO_ID, + path: '/work/demo', + hostId: 'ssh:target' + }) + ] + }, + sshConnectionStates: connectedSshState(status), + sshTargetsHydrated: true, + sshTargetLabels: new Map([[TARGET_ID, 'demo host']]), + hydrationSucceeded: true + }) + return store +} + +/** What the previous run wrote: a live relay session recorded on the row AND in the id map. */ +function persistedSession(): WorkspaceSessionState { + return { + activeRepoId: REPO_ID, + activeWorktreeId: WORKTREE_ID, + activeTabId: TAB_ID, + tabsByWorktree: { + [WORKTREE_ID]: [makeTab({ id: TAB_ID, worktreeId: WORKTREE_ID, ptyId: RELAY_PTY_ID })] + }, + terminalLayoutsByTabId: {}, + activeWorktreeIdsOnShutdown: [WORKTREE_ID], + activeTabIdByWorktree: { [WORKTREE_ID]: TAB_ID }, + remoteSessionIdsByTabId: { [TAB_ID]: RELAY_PTY_ID }, + activeConnectionIdsAtShutdown: [TARGET_ID] + } +} + +describe('restored relay session identity (#17743)', () => { + it('does not republish a hydration-nulled ptyId over the persisted relay session id', () => { + const store = seedStore() + store.getState().hydrateWorkspaceSession(persistedSession()) + + // Hydration deliberately nulls the row; that is the contract, not the bug. + expect(store.getState().tabsByWorktree[WORKTREE_ID][0].ptyId).toBeNull() + expect(store.getState().ptyIdsByTabId[TAB_ID]).toEqual([]) + expect(store.getState().lastKnownRelayPtyIdByTabId[TAB_ID]).toBeUndefined() + + const payload = buildWorkspaceSessionPayload(store.getState()) + + expect(payload.remoteSessionIdsByTabId).toEqual({ [TAB_ID]: RELAY_PTY_ID }) + expect(payload.activeWorktreeIdsOnShutdown).toContain(WORKTREE_ID) + expect(payload.activeConnectionIdsAtShutdown).toEqual([TARGET_ID]) + }) + + it('rebinds the restored tab to its live relay PTY and keeps publishing that id', async () => { + const store = seedStore() + store.getState().hydrateWorkspaceSession(persistedSession()) + + await store.getState().reconnectPersistedTerminals() + + expect(store.getState().tabsByWorktree[WORKTREE_ID][0].ptyId).toBe(RELAY_PTY_ID) + expect(store.getState().ptyIdsByTabId[TAB_ID]).toEqual([RELAY_PTY_ID]) + expect(buildWorkspaceSessionPayload(store.getState()).remoteSessionIdsByTabId).toEqual({ + [TAB_ID]: RELAY_PTY_ID + }) + }) + + it('keeps a deferred relay session id after a disconnect clears the row binding', async () => { + const store = seedStore('disconnected') + store.getState().hydrateWorkspaceSession(persistedSession()) + await store.getState().reconnectPersistedTerminals() + + expect(store.getState().deferredSshSessionIdsByTabId[TAB_ID]).toBe(RELAY_PTY_ID) + + // A relay drop clears the row binding. Loss of contact is not evidence the remote PTY exited, + // so the handle must survive into the next write. + store.getState().clearDirectSshTargetPtyBindings(TARGET_ID) + + expect(store.getState().tabsByWorktree[WORKTREE_ID][0].ptyId).toBeNull() + expect(store.getState().ptyIdsByTabId[TAB_ID]).toEqual([]) + expect(buildWorkspaceSessionPayload(store.getState()).remoteSessionIdsByTabId).toEqual({ + [TAB_ID]: RELAY_PTY_ID + }) + }) + + it('leaves a tab whose relay handle is unverifiable alone instead of retiring it', async () => { + const store = seedStore('disconnected') + store.getState().hydrateWorkspaceSession(persistedSession()) + await store.getState().reconnectPersistedTerminals() + store.getState().clearDirectSshTargetPtyBindings(TARGET_ID) + + // No live PTY, no row binding, host unreachable — `unverifiable`, never `exited`. + expect([...getOrphanTerminalIds(store.getState(), WORKTREE_ID)]).toEqual([]) + expect(store.getState().tabsByWorktree[WORKTREE_ID].map((tab) => tab.id)).toEqual([TAB_ID]) + }) + + it('does not invent a session id for a tab that never had one', () => { + const store = seedStore() + const session = persistedSession() + session.tabsByWorktree[WORKTREE_ID] = [ + makeTab({ id: TAB_ID, worktreeId: WORKTREE_ID, ptyId: null }) + ] + delete session.remoteSessionIdsByTabId + delete session.activeConnectionIdsAtShutdown + session.activeWorktreeIdsOnShutdown = [] + store.getState().hydrateWorkspaceSession(session) + + const payload = buildWorkspaceSessionPayload(store.getState()) + + expect(payload.remoteSessionIdsByTabId).toBeUndefined() + expect(payload.activeWorktreeIdsOnShutdown).toEqual([]) + }) +})