From b44ca1abe5908dc2a1213efcd772efce3c0fa1e5 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 19:04:26 -0700 Subject: [PATCH] fix(ssh): preserve generation-qualified teardown state --- src/main/ipc/pty.test.ts | 16 ++++++---- src/main/ipc/pty.ts | 12 ++------ src/main/ipc/ssh.test.ts | 26 +++++++++++----- src/main/ipc/ssh.ts | 5 ++-- src/main/persistence.test.ts | 57 ++++++++++++++++++++++++++++++++++++ src/main/persistence.ts | 6 +++- 6 files changed, 96 insertions(+), 26 deletions(-) diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index b212aad318e..29fdf631ba7 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -2469,11 +2469,15 @@ describe('registerPtyHandlers', () => { }) ).rejects.toThrow('SSH_SESSION_EXPIRED: remote-pty') - expect(store.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'remote-pty', 'expired') + expect(store.markSshRemotePtyLease).toHaveBeenCalledWith( + 'ssh-1', + 'ssh:ssh-1@@remote-pty', + 'expired' + ) }) - it('marks a scoped SSH session expired using the raw relay lease id', async () => { - const scopedPtyId = 'ssh:ssh-1@@remote-pty' + it('marks a generation-scoped SSH session expired using its exact lease identity', async () => { + const scopedPtyId = 'ssh:ssh-1@@boot-a@@remote-pty' const sshSpawn = vi.fn(async () => { throw new Error('SSH_SESSION_EXPIRED: remote-pty') }) @@ -2527,7 +2531,7 @@ describe('registerPtyHandlers', () => { deletePtyOwnership(scopedPtyId) } - expect(store.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'remote-pty', 'expired') + expect(store.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', scopedPtyId, 'expired') expect(openCodeClearPtyMock).toHaveBeenCalledWith(scopedPtyId) expect(piClearPtyMock).toHaveBeenCalledWith(scopedPtyId) }) @@ -6677,7 +6681,7 @@ describe('registerPtyHandlers', () => { persistHostSessionBinding?: boolean }): Promise<{ id: string }> } - const appPtyId = 'ssh:ssh-expired-runtime@@relay-pty' + const appPtyId = 'ssh:ssh-expired-runtime@@boot-a@@relay-pty' const remoteWrite = vi.fn() registerSshPtyProvider('ssh-expired-runtime', { spawn: vi.fn(async () => { @@ -6751,7 +6755,7 @@ describe('registerPtyHandlers', () => { expect(store.markSshRemotePtyLease).toHaveBeenCalledWith( 'ssh-expired-runtime', - 'relay-pty', + appPtyId, 'expired' ) expect(store.upsertSshRemotePtyLease).not.toHaveBeenCalled() diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 7680edd4deb..d9df1131236 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -3755,9 +3755,7 @@ export function registerPtyHandlers( if (effectiveSessionAppId !== undefined && !isIdentityMismatch) { clearProviderPtyState(effectiveSessionAppId) deletePtyOwnership(effectiveSessionAppId) - } - if (!isIdentityMismatch) { - store?.markSshRemotePtyLease(args.connectionId, effectiveSessionRelayId, 'expired') + store?.markSshRemotePtyLease(args.connectionId, effectiveSessionAppId, 'expired') } } if (isMintedSessionId && sessionId !== undefined) { @@ -4689,13 +4687,7 @@ export function registerPtyHandlers( if (effectiveSessionAppId !== undefined && !isIdentityMismatch) { clearProviderPtyState(effectiveSessionAppId) deletePtyOwnership(effectiveSessionAppId) - } - if (!isIdentityMismatch) { - store?.markSshRemotePtyLease( - args.connectionId, - effectiveSessionRelayId, - 'expired' - ) + store?.markSshRemotePtyLease(args.connectionId, effectiveSessionAppId, 'expired') } } // Why: if buildPtyHostEnv materialized provider state for this minted diff --git a/src/main/ipc/ssh.test.ts b/src/main/ipc/ssh.test.ts index bb7a9192d88..82996961ac1 100644 --- a/src/main/ipc/ssh.test.ts +++ b/src/main/ipc/ssh.test.ts @@ -1332,7 +1332,11 @@ describe('SSH IPC handlers', () => { expect(mockConnectionManager.connect).toHaveBeenCalledWith(target) expect(mockForceStopRelayForTarget).toHaveBeenCalledWith(conn, 'ssh-1') - expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'pty-1', 'expired') + expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + 'ssh-1', + 'ssh:ssh-1@@pty-1', + 'expired' + ) expect(mockStore.markSshRemotePtyLease).not.toHaveBeenCalledWith( 'ssh-1', 'pty-expired', @@ -1341,7 +1345,7 @@ describe('SSH IPC handlers', () => { expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') }) - it('ssh:resetRelay clears scoped live PTYs while expiring raw leases', async () => { + it('ssh:resetRelay clears scoped live PTYs and exact generation-qualified leases', async () => { const target: SshTarget = { id: 'ssh-1', label: 'Server', @@ -1354,17 +1358,21 @@ describe('SSH IPC handlers', () => { mockConnectionManager.connect.mockResolvedValue(conn) mockConnectionManager.getConnection.mockReturnValue(undefined) mockStore.getSshRemotePtyLeases.mockReturnValue([ - { targetId: 'ssh-1', ptyId: 'pty-lease', state: 'detached' } + { targetId: 'ssh-1', ptyId: 'pty-lease', relayInstanceId: 'boot-a', state: 'detached' } ]) vi.mocked(getPtyIdsForConnection).mockReturnValue(['ssh:ssh-1@@pty-live']) await handlers.get('ssh:resetRelay')!(null, { targetId: 'ssh-1' }) - expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'pty-lease', 'expired') + expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + 'ssh-1', + 'ssh:ssh-1@@boot-a@@pty-lease', + 'expired' + ) expect(clearProviderPtyState).toHaveBeenCalledWith('ssh:ssh-1@@pty-live') - expect(clearProviderPtyState).toHaveBeenCalledWith('ssh:ssh-1@@pty-lease') + expect(clearProviderPtyState).toHaveBeenCalledWith('ssh:ssh-1@@boot-a@@pty-lease') expect(deletePtyOwnership).toHaveBeenCalledWith('ssh:ssh-1@@pty-live') - expect(deletePtyOwnership).toHaveBeenCalledWith('ssh:ssh-1@@pty-lease') + expect(deletePtyOwnership).toHaveBeenCalledWith('ssh:ssh-1@@boot-a@@pty-lease') expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') }) @@ -1667,7 +1675,11 @@ describe('SSH IPC handlers', () => { expect(mockStore.markSshRemotePtyLeases).not.toHaveBeenCalledWith('ssh-1', 'terminated') expect(mockStore.markSshRemotePtyLeases).toHaveBeenCalledWith('ssh-1', 'detached') - expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('ssh-1', 'pty-1', 'expired') + expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith( + 'ssh-1', + 'ssh:ssh-1@@pty-1', + 'expired' + ) expect(mockForceStopRelayForTarget).toHaveBeenCalledWith(conn, 'ssh-1') }) diff --git a/src/main/ipc/ssh.ts b/src/main/ipc/ssh.ts index 2ae1127678d..9f5a7963613 100644 --- a/src/main/ipc/ssh.ts +++ b/src/main/ipc/ssh.ts @@ -1106,8 +1106,9 @@ export function registerSshHandlers( const ptyIds = new Set(getPtyIdsForConnection(targetId)) for (const lease of persistedStore!.getSshRemotePtyLeases(targetId)) { if (lease.state !== 'terminated' && lease.state !== 'expired') { - ptyIds.add(lease.ptyId) - persistedStore!.markSshRemotePtyLease(targetId, lease.ptyId, 'expired') + const appPtyId = toAppSshPtyId(targetId, lease.ptyId, lease.relayInstanceId) + ptyIds.add(appPtyId) + persistedStore!.markSshRemotePtyLease(targetId, appPtyId, 'expired') } } // Why: reset force-kills the remote relay daemon, so every local PTY diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index a8a4ff913e9..0c08b72c9f2 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -14,6 +14,7 @@ import { statSync, symlinkSync } from 'node:fs' +import type * as NodeFsPromises from 'node:fs/promises' import { join } from 'node:path' import { tmpdir } from 'node:os' import type { @@ -8872,6 +8873,62 @@ describe('Store', () => { ) }) + it('keeps an SSH generation migration journaled behind an in-flight stale save', async () => { + vi.useFakeTimers() + const actualFs = await vi.importActual('node:fs/promises') + let releaseWrite!: () => void + let markWriteStarted!: () => void + const writeCanFinish = new Promise((resolve) => { + releaseWrite = resolve + }) + const writeStarted = new Promise((resolve) => { + markWriteStarted = resolve + }) + let blockNextDataWrite = true + vi.doMock('node:fs/promises', () => ({ + ...actualFs, + writeFile: async (...args: unknown[]) => { + const path = String(args[0]) + if (blockNextDataWrite && path.includes('orca-data.json.') && path.endsWith('.tmp')) { + blockNextDataWrite = false + markWriteStarted() + await writeCanFinish + } + return Reflect.apply(actualFs.writeFile, actualFs, args) + } + })) + try { + const store = await createStore() + store.upsertSshRemotePtyLease({ + targetId: 'ssh-1', + ptyId: 'remote-pty', + state: 'detached' + }) + store.updateUI({ sidebarWidth: 401 }) + vi.advanceTimersByTime(1_000) + await writeStarted + + expect(store.migrateLegacySshRemotePtyLeaseGeneration('ssh-1', 'remote-pty', 'boot-a')).toBe( + true + ) + releaseWrite() + await store.waitForPendingWrite() + + expect(readTerminalTeardownIntentJournal()).toContainEqual( + expect.objectContaining({ kind: 'ssh-migrate-generation', relayInstanceId: 'boot-a' }) + ) + const reloaded = await createStore() + expect(reloaded.getSshRemotePtyLeases('ssh-1')).toEqual([ + expect.objectContaining({ ptyId: 'remote-pty', relayInstanceId: 'boot-a' }) + ]) + } finally { + vi.clearAllTimers() + vi.doUnmock('node:fs/promises') + vi.resetModules() + vi.useRealTimers() + } + }) + it('preserves exact local and runtime close intent across app restart', async () => { const store = await createStore() store.upsertPendingLocalPtyShutdown({ diff --git a/src/main/persistence.ts b/src/main/persistence.ts index b6ea074ead2..bfe657d59fb 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -3793,7 +3793,11 @@ export class Store { // is not what the file holds. if (this.writeGeneration === gen) { this.lastWrittenStateHash = stateHash - this.compactTerminalTeardownIntentJournal() + // Why: a teardown mutation can append while this stale payload is in + // flight. Keep its journal record until that newer state reaches disk. + if (this.computeStateHash() === stateHash) { + this.compactTerminalTeardownIntentJournal() + } } } finally { if (!renamed) {