diff --git a/build-plugins/plain-node-entry-guard.ts b/build-plugins/plain-node-entry-guard.ts index a7ad171a402..13499118b65 100644 --- a/build-plugins/plain-node-entry-guard.ts +++ b/build-plugins/plain-node-entry-guard.ts @@ -26,6 +26,23 @@ const PLAIN_NODE_ENTRY_NAMES = [ 'codex/codex-app-server-grant-entry' ] as const +// Entries executed as worker threads of the main process. Electron's module is +// not registered on worker threads, so require("electron") throws +// "Cannot find module 'electron'" there too (verified on Electron 43) and kills +// the worker at startup. These carry hand-written "must stay electron-free" +// comments, which is convention, not enforcement — and the port-scan worker in +// particular sits one import away from a client module that deliberately does +// require electron. +const WORKER_THREAD_ENTRY_NAMES = [ + 'stt-worker', + 'warp-theme-parser-worker', + 'session-scanner-opencode-sqlite-worker-entry', + 'main-thread-hang-watchdog-entry', + 'port-scan-command-worker-entry' +] as const + +type EntryRuntime = 'plain-Node process' | 'worker thread' + const ELECTRON_REQUIRE_RE = /require\(\s*["']electron["']\s*\)/ function collectReachableChunks( @@ -56,13 +73,14 @@ function collectReachableChunks( function assertNoElectronRequire( entryName: string, entry: OutputChunk, - byFileName: Map + byFileName: Map, + runtime: EntryRuntime = 'plain-Node process' ): void { for (const chunk of collectReachableChunks(entry, byFileName)) { if (ELECTRON_REQUIRE_RE.test(chunk.code)) { throw new Error( `[plain-node-entry-guard] "${entryName}" reaches chunk "${chunk.fileName}" that ` + - `requires electron. "${entryName}" runs as a plain-Node process, where ` + + `requires electron. "${entryName}" runs as a ${runtime}, where ` + `require("electron") throws MODULE_NOT_FOUND and kills it at startup (the ` + `v1.4.129-rc.1 daemon outage). Keep electron imports out of its module graph.` ) @@ -125,7 +143,14 @@ export function createPlainNodeEntryGuardPlugin(): Plugin { for (const entryName of PLAIN_NODE_ENTRY_NAMES) { const entry = entryByName.get(entryName) if (entry) { - assertNoElectronRequire(entryName, entry, byFileName) + assertNoElectronRequire(entryName, entry, byFileName, 'plain-Node process') + } + } + + for (const entryName of WORKER_THREAD_ENTRY_NAMES) { + const entry = entryByName.get(entryName) + if (entry) { + assertNoElectronRequire(entryName, entry, byFileName, 'worker thread') } } diff --git a/config/knip.json b/config/knip.json index a48793b7902..cb929266ff3 100644 --- a/config/knip.json +++ b/config/knip.json @@ -10,6 +10,7 @@ "src/main/speech/stt-worker.ts", "src/main/warp-themes/warp-theme-parser-worker.ts", "src/main/ai-vault/session-scanner-opencode-sqlite-worker-entry.ts", + "src/main/ports/port-scan-command-worker-entry.ts", "src/main/ipc/parcel-watcher-process-entry.ts", "src/main/hang-watchdog/main-thread-hang-watchdog-entry.ts", "src/main/codex/codex-app-server-grant-entry.ts", diff --git a/config/scripts/build-windows-cli-launcher.test.mjs b/config/scripts/build-windows-cli-launcher.test.mjs index 6e4803ed7ba..4bb219a5687 100644 --- a/config/scripts/build-windows-cli-launcher.test.mjs +++ b/config/scripts/build-windows-cli-launcher.test.mjs @@ -14,6 +14,21 @@ import { describe, expect, it } from 'vitest' const itCrossHost = process.platform === 'win32' ? it.skip : it const projectRoot = resolve(import.meta.dirname, '../..') +const WINDOWS_LOCK_CODES = ['EBUSY', 'ENOTEMPTY', 'EPERM'] + +// Why: Windows releases the image handle on a just-executed exe (and finishes the +// AV scan of the freshly compiled one) after the process exits, so tearing down the +// fixture races those locks. Retry, then leave the temp tree rather than reporting a +// teardown lock as a launcher failure. +function removeFixtureTree(path) { + try { + rmSync(path, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }) + } catch (error) { + if (process.platform !== 'win32' || !WINDOWS_LOCK_CODES.includes(error?.code)) { + throw error + } + } +} // Why: cold csc.exe startup exceeds Vitest's 5s unit budget on hosted Windows; // keep the larger allowance scoped to the real compiler integration test. function itWindows(name, test) { @@ -35,7 +50,7 @@ describe('Windows CLI launcher', () => { expect(result.stderr).toContain('Windows CLI launcher') expect(result.stderr).toContain('Windows host') } finally { - rmSync(outputRoot, { recursive: true, force: true }) + removeFixtureTree(outputRoot) } }) @@ -108,7 +123,7 @@ describe('Windows CLI launcher', () => { orcaNodeOptions: '--no-warnings' }) } finally { - rmSync(appRoot, { recursive: true, force: true }) + removeFixtureTree(appRoot) } }) @@ -161,7 +176,7 @@ describe('Windows CLI launcher', () => { pathKeys: ['PATH', 'Path'] }) } finally { - rmSync(appRoot, { recursive: true, force: true }) + removeFixtureTree(appRoot) } }) }) diff --git a/config/scripts/plain-node-entry-guard.test.ts b/config/scripts/plain-node-entry-guard.test.ts index a7583e1d470..b9601a67d41 100644 --- a/config/scripts/plain-node-entry-guard.test.ts +++ b/config/scripts/plain-node-entry-guard.test.ts @@ -85,3 +85,91 @@ describe('plain Node entry guard', () => { ) }) }) + +// Why (#11161): Electron's module is not registered on worker threads either — +// require("electron") throws "Cannot find module 'electron'" inside a +// main-process worker and kills it at startup. The worker entries carried only +// hand-written "must stay electron-free" comments, and the port-scan worker sits +// one import away from a client that deliberately does require electron. +describe('worker thread entry guard', () => { + function runWorkerWriteBundle(plugin: Plugin, bundle: Rollup.OutputBundle): void { + const hook = plugin.writeBundle + if (typeof hook !== 'function') { + throw new Error('Expected writeBundle hook') + } + hook.call( + { meta: { watchMode: false } } as never, + { dir: createOutputDir() } as Rollup.NormalizedOutputOptions, + bundle + ) + } + + function workerChunk(name: string, code: string, imports: string[] = []): Rollup.OutputChunk { + return { + type: 'chunk', + code, + dynamicImports: [], + fileName: `${name}.js`, + imports, + isEntry: true, + name + } as Rollup.OutputChunk + } + + it('rejects an Electron require reachable from a worker entry', () => { + const plugin = createPlainNodeEntryGuardPlugin() + const bundle = { + 'port-scan-command-worker-entry.js': workerChunk( + 'port-scan-command-worker-entry', + 'require("electron")' + ) + } as Rollup.OutputBundle + + expect(() => runWorkerWriteBundle(plugin, bundle)).toThrow('requires electron') + }) + + it('names the worker-thread runtime so the failure is actionable', () => { + const plugin = createPlainNodeEntryGuardPlugin() + const bundle = { + 'stt-worker.js': workerChunk('stt-worker', 'require("electron")') + } as Rollup.OutputBundle + + expect(() => runWorkerWriteBundle(plugin, bundle)).toThrow('runs as a worker thread') + }) + + // The real risk is transitive: a worker entry importing a shared chunk that + // reaches the electron-requiring client, not a direct import anyone would spot. + it('follows shared chunks out of a worker entry', () => { + const plugin = createPlainNodeEntryGuardPlugin() + const bundle = { + 'session-scanner-opencode-sqlite-worker-entry.js': workerChunk( + 'session-scanner-opencode-sqlite-worker-entry', + 'require("./chunks/shared.js")', + ['chunks/shared.js'] + ), + 'chunks/shared.js': { + type: 'chunk', + code: 'require("electron")', + dynamicImports: [], + fileName: 'chunks/shared.js', + imports: [], + isEntry: false, + name: 'shared' + } as Rollup.OutputChunk + } as Rollup.OutputBundle + + expect(() => runWorkerWriteBundle(plugin, bundle)).toThrow('chunks/shared.js') + }) + + it('passes a clean worker entry', () => { + const plugin = createPlainNodeEntryGuardPlugin() + const bundle = { + 'warp-theme-parser-worker.js': workerChunk( + 'warp-theme-parser-worker', + 'require("node:worker_threads")' + ) + } as Rollup.OutputBundle + + expect(() => runWorkerWriteBundle(plugin, bundle)).not.toThrow() + }) +}) diff --git a/electron.vite.config.ts b/electron.vite.config.ts index 39f923cb215..a6e656ac9c6 100644 --- a/electron.vite.config.ts +++ b/electron.vite.config.ts @@ -216,6 +216,11 @@ export const electronViteConfig: UserConfig = { 'session-scanner-opencode-sqlite-worker-entry': resolve( 'src/main/ai-vault/session-scanner-opencode-sqlite-worker-entry.ts' ), + // Why: libuv spawns processes inline on the calling loop, so the port + // scan's probe commands run on a worker thread instead of the UI one. + 'port-scan-command-worker-entry': resolve( + 'src/main/ports/port-scan-command-worker-entry.ts' + ), // Why: forked with ELECTRON_RUN_AS_NODE so @parcel/watcher faults // can't take down the main process (issue #7547). 'parcel-watcher-process-entry': resolve('src/main/ipc/parcel-watcher-process-entry.ts'), diff --git a/src/main/index.ts b/src/main/index.ts index 91bb5aea817..6e769cd39d6 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -253,7 +253,8 @@ import { buildHeadlessAutomationWorktreeCreateArgs } from './automations/headles import { AgentAwakeService } from './agent-awake-service' import { registerSystemResumeBroadcast } from './system-resume-broadcast' import { settleTeardownWithinDeadline } from './quit-teardown-deadline' -import { QuitTeardownStartGate } from './quit-teardown-start-gate' +import { quitTeardownStartGate } from './quit-teardown-start-gate' +import { detachAllSshSessionsForShutdown } from './ipc/ssh' import { PluginService } from './plugins/plugin-service' import { PluginKillListService } from './plugins/plugin-kill-list-service' import { getPluginsDataDir } from './plugins/plugin-discovery' @@ -2978,7 +2979,6 @@ app.on('before-quit', () => { // Why: will-quit fires twice — first pass preventDefaults and runs teardown; second pass exits. let daemonDisconnectDone = false -const quitTeardownStartGate = new QuitTeardownStartGate() app.on('will-quit', (e) => { // Why return instead of re-running teardown: the second pass is Electron re-firing after // our own app.quit(), so every step below already ran and every durable write already @@ -3030,6 +3030,7 @@ app.on('will-quit', (e) => { runtime?.getOffscreenBrowserBackend()?.destroyAll?.() browserManager.setBrowserGuestStateChangedListener(null) const emulatorShutdown = runtime?.getEmulatorBridge()?.destroyAllSessions() ?? Promise.resolve() + const sshShutdown = detachAllSshSessionsForShutdown() killAllPty() const watcherShutdown = shutdownWatchersOnce() const storeFlush = store?.flushAsync() ?? Promise.resolve() @@ -3072,6 +3073,7 @@ app.on('will-quit', (e) => { { name: 'runtime-rpc', promise: rpcStopAndClear }, { name: 'watchers', promise: watcherShutdown }, { name: 'emulator', promise: emulatorShutdown }, + { name: 'ssh', promise: sshShutdown }, { name: 'plugin-hosts', promise: pluginHostShutdown }, { name: 'usage-cache', promise: usageCacheFlush }, { name: 'stats', promise: statsFlush }, diff --git a/src/main/ipc/localhost-worktree-labels.ts b/src/main/ipc/localhost-worktree-labels.ts index 382072f6c05..77c3c3d6503 100644 --- a/src/main/ipc/localhost-worktree-labels.ts +++ b/src/main/ipc/localhost-worktree-labels.ts @@ -42,7 +42,11 @@ async function assertAllowedTarget(store: Store, targetUrl: string): Promise { if (String(port.port) !== targetPort) { return false diff --git a/src/main/ipc/ssh.test.ts b/src/main/ipc/ssh.test.ts index 7a7b01563e1..6609899dce1 100644 --- a/src/main/ipc/ssh.test.ts +++ b/src/main/ipc/ssh.test.ts @@ -45,6 +45,7 @@ const { mockConnectionManager: { connect: vi.fn(), disconnect: vi.fn(), + disconnectConnection: vi.fn(), reconnect: vi.fn(), getConnection: vi.fn(), getState: vi.fn(), @@ -257,7 +258,13 @@ vi.mock('../ssh/ssh-port-scanner', () => ({ } })) -import { getSshConnectionManager, registerSshHandlers, resetSshHandlerStateForTests } from './ssh' +import { + detachAllSshSessionsForShutdown, + getActiveMultiplexer, + getSshConnectionManager, + registerSshHandlers, + resetSshHandlerStateForTests +} from './ssh' import { RelayVersionMismatchError } from '../ssh/ssh-relay-version-mismatch-error' import { SSH_RELAY_CONFIGURE_GRACE_TIME_METHOD, @@ -276,6 +283,7 @@ import { } from './pty' import { assertSshMutationExpectation } from '../ssh/ssh-connection-generation' import { getSshPtyConsumerRecovery } from '../ssh/ssh-pty-consumer-recovery' +import { quitTeardownStartGate } from '../quit-teardown-start-gate' describe('SSH IPC handlers', () => { const relayBuildId = '0.1.0+ipc-test' @@ -312,6 +320,7 @@ describe('SSH IPC handlers', () => { const createConnectionManagerMock = () => ({ connect: vi.fn(), disconnect: vi.fn(), + disconnectConnection: vi.fn(), reconnect: vi.fn(), getConnection: vi.fn(), getState: vi.fn(), @@ -381,6 +390,7 @@ describe('SSH IPC handlers', () => { mockConnectionManager.connect.mockReset() mockConnectionManager.disconnect.mockReset() + mockConnectionManager.disconnectConnection.mockReset().mockResolvedValue(undefined) mockConnectionManager.reconnect.mockReset() mockConnectionManager.getConnection.mockReset() mockConnectionManager.getState.mockReset() @@ -557,6 +567,145 @@ describe('SSH IPC handlers', () => { expect(mockSshStore.removeTarget).toHaveBeenCalledWith('ssh-1') }) + it('detaches active SSH sessions during app shutdown without terminating recovery', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockResolvedValue({}) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' }) + mockMux.dispose.mockClear() + mockConnectionManager.disconnectAll.mockClear().mockResolvedValue(undefined) + + await detachAllSshSessionsForShutdown() + + expect(mockPortForwardManager.removeAllForwards).toHaveBeenCalledWith('ssh-1') + expect(mockMux.dispose).toHaveBeenCalledWith('connection_lost') + expect(mockStore.markSshRemotePtyLeasesAsync).toHaveBeenCalledWith('ssh-1', 'detached') + expect(mockStore.removeSshPtyConsumerRecovery).not.toHaveBeenCalled() + expect(mockConnectionManager.disconnectAll).toHaveBeenCalled() + }) + + it('refuses the replacement session a paused connect would publish after shutdown began', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockResolvedValue({}) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + mockConnectionManager.disconnectAll.mockResolvedValue(undefined) + await handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' }) + + // Why here: the old session's lease flush is the one window where doConnect resumes straight into + // publishing its replacement session without re-checking authority. + let releaseDetach = (): void => {} + const detachFlush = new Promise((resolve) => { + releaseDetach = resolve + }) + let signalEnteredDetach = (): void => {} + const enteredDetach = new Promise((resolve) => { + signalEnteredDetach = resolve + }) + mockStore.markSshRemotePtyLeasesAsync.mockImplementationOnce(() => { + signalEnteredDetach() + return detachFlush + }) + + const replacement = handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' }) + // Why await the flush entry and not ticks: shutdown must not snapshot what to drain until the + // replacement connect is parked in the lease flush. + await enteredDetach + expect(mockStore.markSshRemotePtyLeasesAsync).toHaveBeenCalledWith('ssh-1', 'detached') + + mockConnectionManager.disconnectAll.mockClear() + // Why latch the gate here: the committed quit path owns it, so the drain alone must not fence. + quitTeardownStartGate.tryStart({ preventDefault() {} }) + const shutdown = detachAllSshSessionsForShutdown() + releaseDetach() + + await expect(replacement).rejects.toThrow('closed for app shutdown') + await shutdown + + // Why one detach: the fence sits at the publication point, so the resumed connect never registers a + // replacement session — only the old session it had already torn down was detached. + const detaches = mockStore.markSshRemotePtyLeasesAsync.mock.calls.filter( + (call) => call[1] === 'detached' + ) + expect(detaches).toHaveLength(1) + // Why twice: once for the drain's snapshot, once after joining the connect that was still in flight. + expect(mockConnectionManager.disconnectAll).toHaveBeenCalledTimes(2) + expect(getActiveMultiplexer('ssh-1')).toBeUndefined() + await expect(handlers.get('ssh:connect')!(null, { targetId: 'ssh-1' })).rejects.toThrow( + 'closed for app shutdown' + ) + }) + + it('joins an in-flight test-connection probe before the final shutdown disconnect', async () => { + const target: SshTarget = { + id: 'ssh-probe', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-probe', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + mockConnectionManager.disconnectAll.mockResolvedValue(undefined) + mockConnectionManager.disconnect.mockClear().mockResolvedValue(undefined) + + // Why: a probe holds a transport no session owns, so shutdown has to wait for it to hand it back. + const probeState = { + targetId: 'ssh-probe', + status: 'connected', + error: null, + reconnectAttempt: 0 + } + let openProbeTransport = (): void => {} + mockConnectionManager.connect.mockImplementationOnce( + () => + new Promise((resolve) => { + openProbeTransport = () => resolve({ getState: () => probeState }) + }) + ) + const probe = handlers.get('ssh:testConnection')!(null, { targetId: 'ssh-probe' }) + for (let tick = 0; tick < 5; tick++) { + await Promise.resolve() + } + expect(mockConnectionManager.connect).toHaveBeenCalledWith(target) + + quitTeardownStartGate.tryStart({ preventDefault() {} }) + const shutdown = detachAllSshSessionsForShutdown() + openProbeTransport() + await shutdown + + expect(await probe).toMatchObject({ success: true }) + expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-probe') + }) + it('ssh:importConfig returns imported targets', async () => { const imported: SshTarget[] = [ { id: 'ssh-imp', label: 'staging', host: 'staging.com', port: 22, username: '' } @@ -1886,6 +2035,106 @@ describe('SSH IPC handlers', () => { expect(mockDeployAndLaunchRelay).toHaveBeenCalledTimes(1) }) + it('closes the transport a cancelled connect opened after the disconnect finished', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + const lateConn = { id: 'late-transport' } + let resolveStaleConnect!: (connection: unknown) => void + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.getConnection.mockReturnValue(undefined) + mockConnectionManager.connect.mockReturnValueOnce( + new Promise((resolve) => { + resolveStaleConnect = resolve + }) + ) + mockConnectionManager.disconnect.mockResolvedValue(undefined) + + const staleConnect = handlers.get('ssh:connect')!(null, { + targetId: 'ssh-1' + }) as Promise + await vi.waitFor(() => expect(mockConnectionManager.connect).toHaveBeenCalledTimes(1)) + // Why await the whole disconnect: the leak only exists once its teardown has already run, so + // nothing else is left to close the transport this attempt opens afterwards. + await handlers.get('ssh:disconnect')!(null, { targetId: 'ssh-1' }) + + resolveStaleConnect(lateConn) + + await expect(staleConnect).rejects.toThrow('SSH connection attempt was cancelled') + expect(mockConnectionManager.disconnectConnection).toHaveBeenCalledWith('ssh-1', lateConn) + expect(getActiveMultiplexer('ssh-1')).toBeUndefined() + }) + + it('closes the transport when establish resumes after the connect was invalidated', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + const conn = { id: 'establishing-transport' } + let releaseRelayLaunch = (): void => {} + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.getConnection.mockReturnValue(undefined) + mockConnectionManager.connect.mockResolvedValue(conn) + mockConnectionManager.disconnect.mockResolvedValue(undefined) + mockDeployAndLaunchRelay.mockImplementationOnce( + () => + new Promise((resolve) => { + releaseRelayLaunch = () => resolve(createRelayLaunchResult()) + }) + ) + + const connect = handlers.get('ssh:connect')!(null, { + targetId: 'ssh-1' + }) as Promise + await vi.waitFor(() => expect(mockDeployAndLaunchRelay).toHaveBeenCalledTimes(1)) + await handlers.get('ssh:disconnect')!(null, { targetId: 'ssh-1' }) + + releaseRelayLaunch() + + await expect(connect).rejects.toThrow('SSH connection attempt was cancelled') + expect(mockConnectionManager.disconnectConnection).toHaveBeenCalledWith('ssh-1', conn) + expect(getActiveMultiplexer('ssh-1')).toBeUndefined() + }) + + it('leaves a reused transport to its replacement when the connect is cancelled', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + // Why: connect() hands back the already-open transport, so this attempt never owned it. + const sharedConn = { id: 'shared-transport' } + let resolveStaleConnect!: (connection: unknown) => void + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.getConnection.mockReturnValue(sharedConn) + mockConnectionManager.connect.mockReturnValueOnce( + new Promise((resolve) => { + resolveStaleConnect = resolve + }) + ) + mockConnectionManager.disconnect.mockResolvedValue(undefined) + + const staleConnect = handlers.get('ssh:connect')!(null, { + targetId: 'ssh-1' + }) as Promise + await vi.waitFor(() => expect(mockConnectionManager.connect).toHaveBeenCalledTimes(1)) + await handlers.get('ssh:disconnect')!(null, { targetId: 'ssh-1' }) + + resolveStaleConnect(sharedConn) + + await expect(staleConnect).rejects.toThrow('SSH connection attempt was cancelled') + expect(mockConnectionManager.disconnectConnection).not.toHaveBeenCalled() + }) + it('keeps reconnect behind transport disconnect when forward teardown fails', async () => { const target: SshTarget = { id: 'ssh-1', @@ -2374,6 +2623,51 @@ describe('SSH IPC handlers', () => { expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') }) + it('ssh:resetRelay does not open a transport when shutdown starts while it waits for a connect', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + let failConnect!: (error: unknown) => void + const connectResult = new Promise((_resolve, reject) => { + failConnect = reject + }) + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockReturnValue(connectResult) + // Why undefined: reset must fall through to opening its own transport, which is the call under test. + mockConnectionManager.getConnection.mockReturnValue(undefined) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + + const connectPromise = handlers.get('ssh:connect')!(null, { + targetId: 'ssh-1' + }) as Promise + await vi.waitFor(() => expect(mockConnectionManager.connect).toHaveBeenCalledTimes(1)) + + // Why admitted first: the gate latches only after reset is already parked behind the connect, so + // the entry fence cannot be what stops it. + const resetPromise = handlers.get('ssh:resetRelay')!(null, { + targetId: 'ssh-1' + }) as Promise + await Promise.resolve() + + quitTeardownStartGate.tryStart({ preventDefault() {} }) + failConnect(new Error('transport lost')) + await expect(connectPromise).rejects.toThrow('transport lost') + + await expect(resetPromise).rejects.toThrow('closed for app shutdown') + // Why once: the resumed reset must not open a second transport that would outlive the drain. + expect(mockConnectionManager.connect).toHaveBeenCalledTimes(1) + expect(mockForceStopRelayForTarget).not.toHaveBeenCalled() + }) + it('ssh:connect waits for an in-flight reset before starting a new connection', async () => { const target: SshTarget = { id: 'ssh-1', diff --git a/src/main/ipc/ssh.ts b/src/main/ipc/ssh.ts index 0dd32ce534e..3b362aa7ef4 100644 --- a/src/main/ipc/ssh.ts +++ b/src/main/ipc/ssh.ts @@ -7,7 +7,7 @@ import { listUserSshConfigHostSummaries, resolveUserSshConfigHost } from '../ssh/ssh-config-host-picker' -import type { SshConnectionCallbacks } from '../ssh/ssh-connection' +import type { SshConnection, SshConnectionCallbacks } from '../ssh/ssh-connection' import { SshConnectionManager } from '../ssh/ssh-connection-manager' import type { SshChannelMultiplexer } from '../ssh/ssh-channel-multiplexer' import { SshRelaySession, type SshRelayAiVaultHostInfo } from '../ssh/ssh-relay-session' @@ -25,6 +25,7 @@ import type { DirectSshAuthority } from '../../shared/ssh-types' import { SSH_TERMINATE_RECONNECT_REQUIRED } from '../../shared/constants' +import { quitTeardownStartGate } from '../quit-teardown-start-gate' import { isRuntimeOwnedSshTargetId } from '../../shared/execution-host' import { isAuthError } from '../ssh/ssh-connection-utils' import { createCancelledConnectAttemptError } from '../ssh/ssh-connect-attempt-cancellation' @@ -292,6 +293,33 @@ async function abandonFailedSshSession(targetId: string, session: SshRelaySessio } } +// Why: a connect cancelled after its transport already opened still owns that transport and its +// unpublished session — nothing else will reach them, so it has to close them itself. Why only the +// connection it minted, and by identity: disconnecting by target id (or closing a transport it merely +// reused) would tear down the replacement connect's live transport. +async function abandonCancelledConnectAttempt( + targetId: string, + session: SshRelaySession, + mintedConnection: SshConnection | null +): Promise { + // Why the identity guard: every path that removes a session from activeSessions detaches it first, + // so re-detaching a superseded session would only clobber the replacement's lease state. + if (activeSessions.get(targetId) === session) { + await abandonFailedSshSession(targetId, session) + } + if (!mintedConnection) { + return + } + try { + await connectionManager!.disconnectConnection(targetId, mintedConnection) + } catch (error) { + // Why: the caller is about to throw the cancellation; a teardown throw must not replace it. + console.warn( + `[ssh] Failed to disconnect cancelled connect transport for ${targetId}: ${error instanceof Error ? error.message : String(error)}` + ) + } +} + function relayGracePeriodForTarget(target: SshTarget | null | undefined): number | undefined { return target?.relayGracePeriodSeconds } @@ -306,6 +334,15 @@ type ConnectAttempt = { const connectInFlight = new Map() const pendingTransportReconnects = new Set() +// Why the quit gate rather than a local latch: "the committed quit has begun" already has an owner, +// and a private copy could be set by something that is not actually quitting — leaving SSH connects +// refused for the rest of the process lifetime. +function assertSshConnectsNotFenced(): void { + if (quitTeardownStartGate.hasStarted()) { + throw new Error('SSH connects are closed for app shutdown') + } +} + function invalidateConnectAttempt(targetId: string): void { rotateSshProviderAuthority(targetId) pendingTransportReconnects.delete(targetId) @@ -322,6 +359,7 @@ const resetRelayInFlight = new Map>() // Why: ssh:testConnection connects then disconnects; suppressing broadcasts during the test avoids worktree cards flashing connected → disconnected. const testingTargets = new Set() +const testConnectionProbes = new Set>() // Why: without backoff, a relay channel that keeps dying reconnects as fast as the network allows, hammering local + remote sshd; track attempts and back off to end the loop recoverably. type RelayLostBackoffState = { @@ -1042,6 +1080,9 @@ export function registerSshHandlers( if (!isCurrentSshProviderAuthority(observedAuthority)) { throw createCancelledConnectAttemptError() } + // Why: the shutdown drain fences and snapshots synchronously, so a connect either registers in + // connectInFlight below (and gets joined) or fails here — it can never slip between the two. + assertSshConnectsNotFenced() pendingTransportReconnects.delete(targetId) const promise = doConnect(targetId, replacePendingTransport) @@ -1130,6 +1171,11 @@ export function registerSshHandlers( } } + // Why here and not only at entry: this is the publication point, and it is the last statement + // before the transport opens. Checking it in the same synchronous block as activeSessions.set + // means a connect either registers before the shutdown drain snapshots, or registers never and + // owns nothing to clean up. + assertSshConnectsNotFenced() // Why: create the session early so onStateChange sees it in 'deploying' and skips reconnect logic. const session = new SshRelaySession( targetId, @@ -1144,6 +1190,12 @@ export function registerSshHandlers( const ownsSession = (): boolean => isCurrentConnectAttempt(targetId, authority) && activeSessions.get(targetId) === session + // Why captured here and not with existingState: connect() reuses an already-connected transport, + // and only a transport this attempt opened is this attempt's to close when it loses the race. + const priorConnection = connectionManager!.getConnection(targetId) + const mintedConnection = (): SshConnection | null => + conn && conn !== priorConnection ? conn : null + try { conn = await connectionManager!.connect(target) if (!ownsSession()) { @@ -1154,6 +1206,7 @@ export function registerSshHandlers( const errObj = err instanceof Error ? err : new Error(String(err)) const status: SshConnectionStatus = isAuthError(errObj) ? 'auth-failed' : 'error' if (!ownsSession()) { + await abandonCancelledConnectAttempt(targetId, session, mintedConnection()) throw createCancelledConnectAttemptError() } // Why: clear this failed connect's flag so a later non-prompting connect isn't deferred. @@ -1194,6 +1247,7 @@ export function registerSshHandlers( }) } catch (err) { if (!ownsSession()) { + await abandonCancelledConnectAttempt(targetId, session, mintedConnection()) throw createCancelledConnectAttemptError() } await abandonFailedSshSession(targetId, session) @@ -1300,7 +1354,13 @@ export function registerSshHandlers( } const existingConn = connectionManager!.getConnection(targetId) - const conn = existingConn ?? (await connectionManager!.connect(target)) + let conn = existingConn + if (!conn) { + // Why re-check: admission fenced this reset before it parked on the in-flight connect, so shutdown + // may have started (and drained) while we waited — opening a transport now would outlive the drain. + assertSshConnectsNotFenced() + conn = await connectionManager!.connect(target) + } try { await forceStopRelayForTarget(conn, targetId) } finally { @@ -1333,6 +1393,8 @@ export function registerSshHandlers( if (!target) { throw new Error(`SSH target "${args.targetId}" not found`) } + // Why: reset opens its own transport, so it must be fenced by shutdown the same way connect is. + assertSshConnectsNotFenced() let resetPromise: Promise resetPromise = runTargetLifecycle(args.targetId, () => @@ -1392,17 +1454,26 @@ export function registerSshHandlers( } testingTargets.add(args.targetId) - try { + // Why a tracked promise and not just the id: a probe holds a real transport that no session owns, + // so shutdown has to be able to join it before the final drain disconnects what is left. + const probe = (async () => { + // Why: a probe transport opened after the shutdown drain would outlive orderly teardown. + assertSshConnectsNotFenced() const conn = await connectionManager!.connect(target) const state = conn.getState() await connectionManager!.disconnect(args.targetId) - return { success: true, state } + return state + })() + testConnectionProbes.add(probe) + try { + return { success: true, state: await probe } } catch (err) { return { success: false, error: err instanceof Error ? err.message : String(err) } } finally { + testConnectionProbes.delete(probe) testingTargets.delete(args.targetId) // Why: clear so a test's credential prompt doesn't leave lastRequiredPassphrase=true and defer this target at startup. credentialRequestedForTarget.delete(args.targetId) @@ -1510,6 +1581,61 @@ export function getSshConnectionManager(): SshConnectionManager | null { return connectionManager } +// Why: an invalidated connect only observes its cancellation at the next checkpoint, and one blocked +// in the transport handshake can sit there for the whole SSH timeout. Bound the join so the final +// drain always runs well inside the quit deadline instead of racing it. +const SSH_SHUTDOWN_INFLIGHT_JOIN_MS = 2_000 + +async function settleWithinMs(work: readonly Promise[], timeoutMs: number): Promise { + if (work.length === 0) { + return + } + await Promise.race([ + Promise.allSettled(work), + new Promise((resolve) => { + const timer = setTimeout(resolve, timeoutMs) + timer.unref?.() + }) + ]) +} + +// Why no fence latch here: the committed quit path sets it before calling this, so it is already on +// for the snapshot below. Called without that gate (tests), this degrades to a plain drain. +export async function detachAllSshSessionsForShutdown(): Promise { + const inFlightWork: Promise[] = [ + ...[...connectInFlight.values()].map((attempt) => attempt.promise), + ...resetRelayInFlight.values(), + ...testConnectionProbes + ] + for (const targetId of Array.from(connectInFlight.keys())) { + invalidateConnectAttempt(targetId) + } + + const errors: unknown[] = [] + const drain = async (): Promise => { + const results = await Promise.allSettled([ + ...[...activeSessions.keys()].map((targetId) => + teardownActiveSshSession(targetId, (session) => session.detachAndPersist()) + ), + connectionManager?.disconnectAll() ?? Promise.resolve() + ]) + for (const result of results) { + if (result.status === 'rejected') { + errors.push(result.reason as unknown) + } + } + } + + await drain() + // Why: a connect paused in old-session teardown still publishes its replacement session and opens a + // transport before it reaches the cancellation checkpoint, so the first drain can miss both. + await settleWithinMs(inFlightWork, SSH_SHUTDOWN_INFLIGHT_JOIN_MS) + await drain() + if (errors.length > 0) { + throw new AggregateError(errors, 'SSH shutdown detach failed') + } +} + export async function resetSshHandlerStateForTests(): Promise { advertisedUrlWatcherUnsubscribe?.() advertisedUrlWatcherUnsubscribe = null @@ -1536,7 +1662,9 @@ export async function resetSshHandlerStateForTests(): Promise { resetSshProviderAuthorities() resetRelayInFlight.clear() testingTargets.clear() + testConnectionProbes.clear() credentialRequestedForTarget.clear() + quitTeardownStartGate.resetForTests() await connectionManager?.disconnectAll() portForwardManager?.dispose() diff --git a/src/main/ipc/workspace-ports.test.ts b/src/main/ipc/workspace-ports.test.ts index 841c046092f..f5afe03628b 100644 --- a/src/main/ipc/workspace-ports.test.ts +++ b/src/main/ipc/workspace-ports.test.ts @@ -102,14 +102,18 @@ describe('registerWorkspacePortHandlers', () => { worktrees: [{ id: 'attacker', path: '/tmp/not-authorized', repoId: 'local-repo' }] }) - expect(scanWorkspacePortsMock).toHaveBeenCalledWith([ - { - id: 'local-repo::/workspace/repo', - repoId: 'local-repo', - displayName: 'Primary', - path: '/workspace/repo' - } - ]) + expect(scanWorkspacePortsMock).toHaveBeenCalledWith( + [ + { + id: 'local-repo::/workspace/repo', + repoId: 'local-repo', + displayName: 'Primary', + path: '/workspace/repo' + } + ], + undefined, + undefined + ) }) it('deduplicates concurrent scans for the same store-derived probe set', async () => { @@ -139,20 +143,24 @@ describe('registerWorkspacePortHandlers', () => { await handlers.get('workspacePorts:scan')?.(null, undefined) - expect(scanWorkspacePortsMock).toHaveBeenCalledWith([ - { - id: 'local-repo::/workspace/repo', - repoId: 'local-repo', - displayName: 'Primary', - path: '/workspace/repo' - }, - { - id: 'other-repo::/workspace/other', - repoId: 'other-repo', - displayName: 'Other', - path: '/workspace/other' - } - ]) + expect(scanWorkspacePortsMock).toHaveBeenCalledWith( + [ + { + id: 'local-repo::/workspace/repo', + repoId: 'local-repo', + displayName: 'Primary', + path: '/workspace/repo' + }, + { + id: 'other-repo::/workspace/other', + repoId: 'other-repo', + displayName: 'Other', + path: '/workspace/other' + } + ], + undefined, + undefined + ) }) it('stops a process only after the current scan proves the pid owns a workspace port', async () => { diff --git a/src/main/ports/local-workspace-port-scanner.test.ts b/src/main/ports/local-workspace-port-scanner.test.ts index a6eeb21e5ee..132eba1950e 100644 --- a/src/main/ports/local-workspace-port-scanner.test.ts +++ b/src/main/ports/local-workspace-port-scanner.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi, type Mock } from 'vitest' import path from 'node:path' import { attributePortToWorkspace, @@ -9,13 +9,21 @@ import { resetWorkspacePortScanTimeoutBackoffForTests, scanWorkspacePorts } from './local-workspace-port-scanner' +import { PortScanCommandTimeoutError } from './port-scan-command-protocol' -const execFileMock = vi.hoisted(() => vi.fn()) +const runPortScanCommandMock = vi.hoisted(() => vi.fn()) -vi.mock('child_process', () => ({ - execFile: execFileMock +vi.mock('./port-scan-command-client', () => ({ + runPortScanCommand: runPortScanCommandMock, + isPortScanWorkerUnavailableError: () => false })) +const LSOF_LISTEN_OUTPUT = ['p123', 'cnode', 'n127.0.0.1:5173'].join('\n') + +function urlWatcherStub(): { lookup: () => undefined; reconcileScan: Mock } { + return { lookup: () => undefined, reconcileScan: vi.fn() } +} + const worktrees = [ { id: 'repo::/repo', @@ -177,52 +185,42 @@ describe('scanWorkspacePorts attribution work', () => { afterEach(() => { resetWorkspacePortScanTimeoutBackoffForTests() vi.restoreAllMocks() - execFileMock.mockReset() + runPortScanCommandMock.mockReset() }) it('normalizes worktree paths once per scan instead of once per port phase', async () => { vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') const win32ResolveSpy = vi.spyOn(path.win32, 'resolve') const posixResolveSpy = vi.spyOn(path.posix, 'resolve') - const invokeCallback = (callback: unknown, stdout: string): void => { - if (typeof callback !== 'function') { - throw new Error('missing execFile callback') - } - const execCallback = callback as (error: Error | null, stdout: string) => void - execCallback(null, stdout) - } - execFileMock.mockImplementation( - (command: string, args: string[], _options: unknown, callback: unknown) => { - if (command === 'lsof' && args.includes('-iTCP')) { - invokeCallback( - callback, - ['p123', 'cnode', 'n127.0.0.1:3000', 'p124', 'cnode', 'n127.0.0.1:3001'].join('\n') - ) - } else if (command === 'lsof') { - invokeCallback( - callback, - ['p123', 'n/repo/service', 'p124', 'n/repo/worktrees/feature/app'].join('\n') - ) - } else if (command === 'ps') { - invokeCallback( - callback, - [ - '123 node /repo/service/server.js', - '124 node /repo/worktrees/feature/app/server.js' - ].join('\n') - ) - } else { - invokeCallback(callback, '') + runPortScanCommandMock.mockImplementation(async (command: string, args: string[]) => { + if (command === 'lsof' && args.includes('-iTCP')) { + return { + stdout: ['p123', 'cnode', 'n127.0.0.1:3000', 'p124', 'cnode', 'n127.0.0.1:3001'].join( + '\n' + ), + spawnMs: 5 } - return { kill: vi.fn() } } - ) - - const scan = await scanWorkspacePorts(worktrees, { - lookup: () => undefined, - reconcileScan: vi.fn() + if (command === 'lsof') { + return { + stdout: ['p123', 'n/repo/service', 'p124', 'n/repo/worktrees/feature/app'].join('\n'), + spawnMs: 5 + } + } + if (command === 'ps') { + return { + stdout: [ + '123 node /repo/service/server.js', + '124 node /repo/worktrees/feature/app/server.js' + ].join('\n'), + spawnMs: 5 + } + } + return { stdout: '', spawnMs: 5 } }) + const scan = await scanWorkspacePorts(worktrees, urlWatcherStub()) + expect(scan.ports.filter((port) => port.kind === 'workspace')).toHaveLength(2) const win32WorktreePathResolveCalls = win32ResolveSpy.mock.calls.filter( ([input]) => input === '/repo' || input === '/repo/worktrees/feature' @@ -240,62 +238,39 @@ describe('scanWorkspacePorts command timeout', () => { vi.useRealTimers() resetWorkspacePortScanTimeoutBackoffForTests() vi.restoreAllMocks() - execFileMock.mockReset() + runPortScanCommandMock.mockReset() }) it('returns an unavailable scan when lsof never reports completion', async () => { - vi.useFakeTimers() vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') - const killMock = vi.fn() - execFileMock.mockImplementation(() => ({ kill: killMock })) + runPortScanCommandMock.mockRejectedValue( + new PortScanCommandTimeoutError('lsof timed out after 4000ms') + ) - let settled = false - const scanPromise = scanWorkspacePorts([], { - lookup: () => undefined, - reconcileScan: vi.fn() - }).then((scan) => { - settled = true - return scan - }) - - await vi.advanceTimersByTimeAsync(4_000) - - expect(settled).toBe(true) - await expect(scanPromise).resolves.toMatchObject({ + await expect(scanWorkspacePorts([], urlWatcherStub())).resolves.toMatchObject({ platform: 'darwin', ports: [], unavailableReason: 'Port scanning is unavailable on darwin.' }) - expect(killMock).toHaveBeenCalled() }) it('backs off after a command timeout instead of launching lsof on every scan tick', async () => { vi.useFakeTimers() vi.setSystemTime(1_000) vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') - const killMock = vi.fn() - execFileMock.mockImplementation(() => ({ kill: killMock })) + runPortScanCommandMock.mockRejectedValue( + new PortScanCommandTimeoutError('lsof timed out after 4000ms') + ) - const firstScanPromise = scanWorkspacePorts([], { - lookup: () => undefined, - reconcileScan: vi.fn() - }) - - await vi.advanceTimersByTimeAsync(4_000) - await expect(firstScanPromise).resolves.toMatchObject({ + await expect(scanWorkspacePorts([], urlWatcherStub())).resolves.toMatchObject({ platform: 'darwin', ports: [], unavailableReason: 'Port scanning is unavailable on darwin.' }) - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(runPortScanCommandMock).toHaveBeenCalledTimes(1) const cooldownScans = await Promise.all( - Array.from({ length: 10 }, () => - scanWorkspacePorts([], { - lookup: () => undefined, - reconcileScan: vi.fn() - }) - ) + Array.from({ length: 10 }, () => scanWorkspacePorts([], urlWatcherStub())) ) expect(cooldownScans).toHaveLength(10) @@ -306,25 +281,172 @@ describe('scanWorkspacePorts command timeout', () => { expect( cooldownScans.every((scan) => scan.unavailableReason?.includes('temporarily paused')) ).toBe(true) - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(runPortScanCommandMock).toHaveBeenCalledTimes(1) vi.setSystemTime(65_001) await vi.advanceTimersByTimeAsync(0) - execFileMock.mockImplementation( - (_command: string, args: string[], _options: unknown, callback: unknown) => { - const execCallback = callback as (error: Error | null, stdout: string) => void - const output = args.includes('-iTCP') ? 'p123\ncnode\nn127.0.0.1:3000' : '' - execCallback(null, output) - return { kill: vi.fn() } - } - ) + runPortScanCommandMock.mockImplementation(async (_command: string, args: string[]) => ({ + stdout: args.includes('-iTCP') ? 'p123\ncnode\nn127.0.0.1:3000' : '', + spawnMs: 5 + })) - const recoveredScan = await scanWorkspacePorts([], { - lookup: () => undefined, - reconcileScan: vi.fn() - }) + const recoveredScan = await scanWorkspacePorts([], urlWatcherStub()) expect(recoveredScan.unavailableReason).toBeUndefined() - expect(execFileMock).toHaveBeenCalledTimes(4) + expect(runPortScanCommandMock).toHaveBeenCalledTimes(4) }) }) + +describe('scanWorkspacePorts with delayed process creation', () => { + afterEach(() => { + resetWorkspacePortScanTimeoutBackoffForTests() + vi.restoreAllMocks() + runPortScanCommandMock.mockReset() + }) + + // Regression for #11161: an endpoint-security hook makes CreateProcessW take + // seconds, so the command's own budget must not be charged for the spawn. + it('does not report a command timeout when only process creation was delayed', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + runPortScanCommandMock.mockResolvedValue({ stdout: LSOF_LISTEN_OUTPUT, spawnMs: 4_200 }) + + const first = await scanWorkspacePorts([], urlWatcherStub()) + const second = await scanWorkspacePorts([], urlWatcherStub()) + + expect(first.unavailableReason).toBeUndefined() + expect(second.unavailableReason).toBeUndefined() + expect(first.ports).toHaveLength(1) + }) + + it('skips the optional metadata commands for one cycle after a stalled spawn', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + runPortScanCommandMock.mockResolvedValue({ stdout: LSOF_LISTEN_OUTPUT, spawnMs: 4_200 }) + + const scan = await scanWorkspacePorts([], urlWatcherStub()) + + expect(runPortScanCommandMock).toHaveBeenCalledTimes(1) + expect(scan.ports).toHaveLength(1) + }) + + // Regression for #11161 review: a metadata-less scan must not be reconciled as + // "the listener vanished" — that evicts advertised URLs only a PTY can restore. + it('does not reconcile advertised URLs for a scan that skipped metadata', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + runPortScanCommandMock.mockResolvedValue({ stdout: LSOF_LISTEN_OUTPUT, spawnMs: 4_200 }) + const watcher = urlWatcherStub() + + await scanWorkspacePorts(worktrees, watcher) + + expect(watcher.reconcileScan).not.toHaveBeenCalled() + }) + + it('re-probes metadata on the scan after a skip instead of degrading forever', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + runPortScanCommandMock.mockImplementation(async (command: string, args: string[]) => { + if (command === 'lsof' && args.includes('-iTCP')) { + return { stdout: LSOF_LISTEN_OUTPUT, spawnMs: 4_200 } + } + if (command === 'lsof') { + return { stdout: ['p123', 'n/repo'].join('\n'), spawnMs: 4_200 } + } + return { stdout: '123 node /repo/server.js', spawnMs: 4_200 } + }) + const watcher = urlWatcherStub() + + const skipped = await scanWorkspacePorts(worktrees, watcher) + const recovered = await scanWorkspacePorts(worktrees, watcher) + + expect(skipped.ports[0]?.kind).toBe('external') + expect(recovered.ports[0]).toMatchObject({ kind: 'workspace' }) + expect(runPortScanCommandMock).toHaveBeenCalledTimes(4) + expect(watcher.reconcileScan).toHaveBeenCalledTimes(worktrees.length) + }) + + it('still collects process metadata when process creation was fast', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + runPortScanCommandMock.mockImplementation(async (_command: string, args: string[]) => ({ + stdout: args.includes('-iTCP') ? LSOF_LISTEN_OUTPUT : '', + spawnMs: 5 + })) + + await scanWorkspacePorts([], urlWatcherStub()) + + expect(runPortScanCommandMock).toHaveBeenCalledTimes(3) + }) + + // Regression for #11161 review: the skip parity is driven by the 30s poller, + // so a one-shot user action would otherwise land on a random parity. + it('keeps probing metadata for callers that require attribution', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + mockStalledDarwinScan() + + const scan = await scanWorkspacePorts(worktrees, urlWatcherStub(), { requireMetadata: true }) + + expect(scan.ports[0]).toMatchObject({ kind: 'workspace' }) + expect(runPortScanCommandMock).toHaveBeenCalledTimes(3) + }) + + it('does not let a required-metadata scan reset the background skip parity', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + mockStalledDarwinScan() + + await scanWorkspacePorts(worktrees, urlWatcherStub()) + await scanWorkspacePorts(worktrees, urlWatcherStub(), { requireMetadata: true }) + await scanWorkspacePorts(worktrees, urlWatcherStub()) + + // 1 skipped + 3 required + 3 recovered; a reset parity would skip twice. + expect(runPortScanCommandMock).toHaveBeenCalledTimes(7) + }) + + // Regression for #11161 review: without carry-forward the panel moves every + // workspace port into External on each skipped cycle. + it('carries the previous cycle attribution through a skipped scan', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + let listenSpawnMs = 5 + runPortScanCommandMock.mockImplementation(async (command: string, args: string[]) => { + if (command === 'lsof' && args.includes('-iTCP')) { + return { stdout: LSOF_LISTEN_OUTPUT, spawnMs: listenSpawnMs } + } + return { stdout: command === 'lsof' ? ['p123', 'n/repo'].join('\n') : '', spawnMs: 5 } + }) + + const full = await scanWorkspacePorts(worktrees, urlWatcherStub()) + listenSpawnMs = 4_200 + const skipped = await scanWorkspacePorts(worktrees, urlWatcherStub()) + + expect(full.ports[0]).toMatchObject({ kind: 'workspace' }) + expect(skipped.ports[0]).toMatchObject({ kind: 'workspace', processName: 'node' }) + }) + + it('does not hand carried-forward metadata to a different listener', async () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + let listenOutput = LSOF_LISTEN_OUTPUT + let listenSpawnMs = 5 + runPortScanCommandMock.mockImplementation(async (command: string, args: string[]) => { + if (command === 'lsof' && args.includes('-iTCP')) { + return { stdout: listenOutput, spawnMs: listenSpawnMs } + } + return { stdout: command === 'lsof' ? ['p123', 'n/repo'].join('\n') : '', spawnMs: 5 } + }) + + await scanWorkspacePorts(worktrees, urlWatcherStub()) + listenOutput = ['p123', 'cnode', 'n127.0.0.1:9999'].join('\n') + listenSpawnMs = 4_200 + const skipped = await scanWorkspacePorts(worktrees, urlWatcherStub()) + + expect(skipped.ports[0]?.kind).toBe('external') + }) +}) + +/** Darwin scan where every spawn stalls past the metadata-skip threshold. */ +function mockStalledDarwinScan(): void { + runPortScanCommandMock.mockImplementation(async (command: string, args: string[]) => { + if (command === 'lsof' && args.includes('-iTCP')) { + return { stdout: LSOF_LISTEN_OUTPUT, spawnMs: 4_200 } + } + if (command === 'lsof') { + return { stdout: ['p123', 'n/repo'].join('\n'), spawnMs: 4_200 } + } + return { stdout: '123 node /repo/server.js', spawnMs: 4_200 } + }) +} diff --git a/src/main/ports/local-workspace-port-scanner.ts b/src/main/ports/local-workspace-port-scanner.ts index 1d8f4dbf891..ce2b7b7683c 100644 --- a/src/main/ports/local-workspace-port-scanner.ts +++ b/src/main/ports/local-workspace-port-scanner.ts @@ -1,6 +1,5 @@ /* eslint-disable max-lines -- Why: the platform-specific scan paths share parsing, attribution, and normalization rules that must stay in lockstep. */ -import { execFile } from 'node:child_process' import { readFile, readdir, readlink } from 'node:fs/promises' import path from 'node:path' import type { @@ -11,14 +10,36 @@ import type { } from '../../shared/workspace-ports' import { getProcessOutputFields } from '../../shared/process-output-field-scanner' import { advertisedUrlWatcher, type AdvertisedUrlWatcher } from './advertised-url-watcher' +import { isPortScanWorkerUnavailableError, runPortScanCommand } from './port-scan-command-client' +import { PortScanCommandTimeoutError } from './port-scan-command-protocol' import { WorkspacePortScanTimeoutBackoff } from './workspace-port-scan-timeout-backoff' -const COMMAND_TIMEOUT_MS = 4_000 +// Why (#11161): on an EDR-hooked host process creation alone can take seconds. +// Past this, skip the scan's optional metadata commands for one cycle so a scan +// costs roughly one stall instead of three. +const SLOW_SPAWN_SKIP_METADATA_MS = 2_000 const MAX_PORTS = 200 const HTTP_PORTS = new Set([80, 3000, 3001, 4200, 5000, 5173, 5174, 8000, 8080, 8888]) const HTTPS_PORTS = new Set([443, 8443]) const commandTimeoutBackoff = new WorkspacePortScanTimeoutBackoff() +let loggedWorkerUnavailable = false +// Why (#11161): a hooked host stalls every spawn, so gating only on the current +// scan would drop metadata forever. Never skip twice running, so attribution — +// and the Stop action and advertised-URL matching that ride on it — recovers on +// the next tick. +let skippedMetadataOnLastScan = false +// Why (#11161): a skipped cycle would otherwise report every listener as +// external, so the panel flip-flops and the Stop action loses its owner. Carry +// the previous cycle's metadata forward, keyed tightly enough that a recycled +// pid cannot inherit it. +let lastListenerMetadata = new Map() + +export type WorkspacePortScanOptions = { + /** Set by attribution-dependent callers (Stop, the localhost-label allowlist) + * that must never trade owner metadata for scan latency. */ + requireMetadata?: boolean +} type RawListeningPort = { host: string @@ -40,9 +61,16 @@ type NormalizedWorkspacePortProbe = { normalizedPath: string } +type PlatformListeningPortScan = { + ports: RawListeningPort[] + /** False when a stalled spawn made the scan skip its cwd/command-line probes. */ + metadataAvailable: boolean +} + export async function scanWorkspacePorts( worktrees: WorkspacePortProbe[], - urlWatcher: Pick = advertisedUrlWatcher + urlWatcher: Pick = advertisedUrlWatcher, + options: WorkspacePortScanOptions = {} ): Promise { const cooldown = commandTimeoutBackoff.snapshot() if (cooldown.isCoolingDown) { @@ -54,10 +82,15 @@ export async function scanWorkspacePorts( } try { - const rawPorts = await scanPlatformListeningPorts() + const { ports: rawPorts, metadataAvailable } = await scanPlatformListeningPorts(options) commandTimeoutBackoff.recordSuccess() const normalizedWorktrees = normalizeWorkspacePortProbes(worktrees) - reconcileAdvertisedUrls(rawPorts, normalizedWorktrees, urlWatcher) + // Why (#11161): without cwd/command-line every port looks unattributed, and + // reconciling that would read as "the listener vanished" and evict cached + // advertised URLs that only live PTY output can ever restore. + if (metadataAvailable) { + reconcileAdvertisedUrls(rawPorts, normalizedWorktrees, urlWatcher) + } const ports = rawPorts .map((port) => enrichPort(port, normalizedWorktrees, urlWatcher)) .sort(compareWorkspacePorts) @@ -67,13 +100,66 @@ export async function scanWorkspacePorts( if (isCommandTimeoutError(error)) { commandTimeoutBackoff.recordTimeout() } - console.warn('[workspace-ports] scan failed', error) + warnScanFailure(error) return makeUnavailableScan(`Port scanning is unavailable on ${process.platform}.`) } } export function resetWorkspacePortScanTimeoutBackoffForTests(): void { commandTimeoutBackoff.reset() + loggedWorkerUnavailable = false + skippedMetadataOnLastScan = false + lastListenerMetadata = new Map() +} + +function shouldSkipMetadataCommands(spawnMs: number, opts: WorkspacePortScanOptions): boolean { + if (opts.requireMetadata) { + // Leave the skip parity alone: it belongs to the background scan cadence, + // and a one-shot user action must not shift which tick degrades. + return false + } + const skip = spawnMs > SLOW_SPAWN_SKIP_METADATA_MS && !skippedMetadataOnLastScan + skippedMetadataOnLastScan = skip + return skip +} + +/** Identity tight enough that a recycled pid cannot inherit stale metadata. */ +function listenerMetadataKey(port: RawListeningPort): string { + return `${port.pid ?? 'unknown'}:${port.host}:${port.port}` +} + +function rememberListenerMetadata(ports: readonly RawListeningPort[]): void { + lastListenerMetadata = new Map( + ports.map((port) => [ + listenerMetadataKey(port), + { processName: port.processName, commandLine: port.commandLine, cwd: port.cwd } + ]) + ) +} + +function recallListenerMetadata(port: RawListeningPort): RawListeningPort { + const remembered = lastListenerMetadata.get(listenerMetadataKey(port)) + if (!remembered) { + return port + } + return { + ...port, + processName: port.processName ?? remembered.processName, + commandLine: port.commandLine ?? remembered.commandLine, + cwd: port.cwd ?? remembered.cwd + } +} + +// Why: a mispackaged probe worker fails identically forever, so logging it on +// every 30s scan tick is pure noise. +function warnScanFailure(error: unknown): void { + if (isPortScanWorkerUnavailableError(error)) { + if (loggedWorkerUnavailable) { + return + } + loggedWorkerUnavailable = true + } + console.warn('[workspace-ports] scan failed', error) } function makeUnavailableScan(reason: string): WorkspacePortScanResult { @@ -194,38 +280,73 @@ export function parseProcNetTcp(content: string): { host: string; port: number; return results } -async function scanPlatformListeningPorts(): Promise { +async function scanPlatformListeningPorts( + options: WorkspacePortScanOptions +): Promise { + const scan = await dispatchPlatformListeningPortScan(options) + if (scan.metadataAvailable) { + rememberListenerMetadata(scan.ports) + return scan + } + return { ...scan, ports: scan.ports.map(recallListenerMetadata) } +} + +async function dispatchPlatformListeningPortScan( + options: WorkspacePortScanOptions +): Promise { if (process.platform === 'linux') { return scanLinuxProcPorts() } if (process.platform === 'darwin') { - return scanDarwinLsofPorts() + return scanDarwinLsofPorts(options) } if (process.platform === 'win32') { - return scanWindowsNetstatPorts() + return scanWindowsNetstatPorts(options) } throw new Error(`Port scanning is not supported on ${process.platform}`) } -async function scanDarwinLsofPorts(): Promise { - const { stdout } = await runCommand('lsof', ['-nP', '-iTCP', '-sTCP:LISTEN', '-F', 'pcn']) +async function scanDarwinLsofPorts( + options: WorkspacePortScanOptions +): Promise { + const { stdout, spawnMs } = await runPortScanCommand('lsof', [ + '-nP', + '-iTCP', + '-sTCP:LISTEN', + '-F', + 'pcn' + ]) const ports = parseLsofListeningOutput(stdout) + if (shouldSkipMetadataCommands(spawnMs, options)) { + return { ports, metadataAvailable: false } + } const metadata = await loadDarwinProcessMetadata( new Set(ports.flatMap((p) => (p.pid ? [p.pid] : []))) ) - return ports.map((port) => ({ ...metadata.get(port.pid ?? -1), ...port })) + return { + ports: ports.map((port) => ({ ...metadata.get(port.pid ?? -1), ...port })), + metadataAvailable: true + } } -async function scanWindowsNetstatPorts(): Promise { - const { stdout } = await runCommand('netstat', ['-ano', '-p', 'tcp']) +async function scanWindowsNetstatPorts( + options: WorkspacePortScanOptions +): Promise { + const { stdout, spawnMs } = await runPortScanCommand('netstat', ['-ano', '-p', 'tcp']) const ports = parseNetstatListeningOutput(stdout) + if (shouldSkipMetadataCommands(spawnMs, options)) { + return { ports, metadataAvailable: false } + } const metadata = await loadWindowsProcessMetadata( new Set(ports.flatMap((p) => (p.pid ? [p.pid] : []))) ) - return ports.map((port) => ({ ...metadata.get(port.pid ?? -1), ...port })) + return { + ports: ports.map((port) => ({ ...metadata.get(port.pid ?? -1), ...port })), + metadataAvailable: true + } } -async function scanLinuxProcPorts(): Promise { +async function scanLinuxProcPorts(): Promise { const [tcp4, tcp6] = await Promise.all([ readProcNet('/proc/net/tcp'), readProcNet('/proc/net/tcp6') @@ -248,7 +369,7 @@ async function scanLinuxProcPorts(): Promise { }) } - return dedupeRawPorts(rawPorts) + return { ports: dedupeRawPorts(rawPorts), metadataAvailable: true } } async function readProcNet( @@ -321,10 +442,24 @@ async function loadDarwinProcessMetadata(pids: Set): Promise null), - runCommand('ps', ['-p', pidList, '-o', 'pid=', '-o', 'command=']).catch(() => null) - ]) + // Why (#11161): sequential, not Promise.all — the probe worker dispatches one + // command at a time, so issuing both at once would only queue the second. + const cwdOutput = await runPortScanCommand('lsof', [ + '-a', + '-p', + pidList, + '-d', + 'cwd', + '-Fn' + ]).catch(() => null) + const commandOutput = await runPortScanCommand('ps', [ + '-p', + pidList, + '-o', + 'pid=', + '-o', + 'command=' + ]).catch(() => null) let currentPid: number | null = null for (const line of cwdOutput?.stdout.split('\n') ?? []) { @@ -360,7 +495,7 @@ async function loadWindowsProcessMetadata( .filter(Number.isFinite) .map((pid) => `ProcessId=${pid}`) .join(' OR ') - const { stdout } = await runCommand('powershell.exe', [ + const { stdout } = await runPortScanCommand('powershell.exe', [ '-NoProfile', '-Command', `Get-CimInstance Win32_Process -Filter "${pidFilter}" | Select-Object ProcessId,Name,CommandLine | ConvertTo-Json -Compress` @@ -382,62 +517,8 @@ async function loadWindowsProcessMetadata( return result } -async function runCommand(command: string, args: string[]): Promise<{ stdout: string }> { - return await new Promise((resolve, reject) => { - let settled = false - let child: ReturnType | undefined - const timer = setTimeout(() => { - if (settled) { - return - } - settled = true - child?.kill() - reject(new CommandTimeoutError(command, COMMAND_TIMEOUT_MS)) - }, COMMAND_TIMEOUT_MS) - - const settle = (callback: () => void): void => { - if (settled) { - return - } - settled = true - clearTimeout(timer) - callback() - } - - // Why: Node's execFile timeout only signals the child; if the callback - // never arrives, the workspace port scan would otherwise hang forever. - try { - child = execFile( - command, - args, - { - timeout: COMMAND_TIMEOUT_MS, - maxBuffer: 2 * 1024 * 1024, - windowsHide: true - }, - (error, stdout) => { - if (error) { - settle(() => reject(error)) - return - } - settle(() => resolve({ stdout: String(stdout) })) - } - ) - } catch (error) { - settle(() => reject(error)) - } - }) -} - -class CommandTimeoutError extends Error { - constructor(command: string, timeoutMs: number) { - super(`${command} timed out after ${timeoutMs}ms`) - this.name = 'CommandTimeoutError' - } -} - function isCommandTimeoutError(error: unknown): boolean { - return error instanceof CommandTimeoutError + return error instanceof PortScanCommandTimeoutError } async function readTextIfAvailable(filePath: string): Promise { diff --git a/src/main/ports/port-scan-command-client.test.ts b/src/main/ports/port-scan-command-client.test.ts new file mode 100644 index 00000000000..d1a15d29774 --- /dev/null +++ b/src/main/ports/port-scan-command-client.test.ts @@ -0,0 +1,300 @@ +import { readFileSync } from 'node:fs' +import { join, sep } from 'node:path' +import { Worker } from 'node:worker_threads' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + CALL_DEADLINE_MS, + MAX_QUEUED_CALLS, + PortScanCommandClient, + isPortScanWorkerUnavailableError, + resolveWorkerEntryPath +} from './port-scan-command-client' +import { + PortScanCommandTimeoutError, + type PortScanCommandRequest +} from './port-scan-command-protocol' + +type PortScanCommandResponseBody = + | { ok: true; stdout: string; spawnMs: number } + | { ok: false; timedOut: boolean; error: string } + +const execFileMock = vi.hoisted(() => vi.fn()) + +vi.mock('child_process', () => ({ + execFile: execFileMock +})) + +// A worker_threads stand-in the tests drive directly: it records posted requests +// and lets a test emit message/error/exit without a built worker bundle. +class FakeWorker { + postedRequests: PortScanCommandRequest[] = [] + terminated = false + private listeners = new Map void>>() + + on(event: string, listener: (arg?: unknown) => void): this { + const set = this.listeners.get(event) ?? new Set() + set.add(listener) + this.listeners.set(event, set) + return this + } + + off(event: string, listener: (arg?: unknown) => void): this { + this.listeners.get(event)?.delete(listener) + return this + } + + removeAllListeners(): void { + this.listeners.clear() + } + + unref(): void {} + + async terminate(): Promise { + this.terminated = true + return 1 + } + + postMessage(request: PortScanCommandRequest): void { + this.postedRequests.push(request) + } + + emit(event: string, arg?: unknown): void { + // Copy first: the client removes its listeners synchronously during a fault. + for (const listener of Array.from(this.listeners.get(event) ?? [])) { + listener(arg) + } + } + + respond(body: PortScanCommandResponseBody): void { + const last = this.postedRequests.at(-1) + if (!last) { + throw new Error('no request posted to fake worker') + } + this.emit('message', { id: last.id, ...body }) + } +} + +function makeFactory(workers: FakeWorker[]): () => Worker { + return () => { + const worker = new FakeWorker() + workers.push(worker) + return worker as unknown as Worker + } +} + +function makeClient(workers: FakeWorker[]): PortScanCommandClient { + return new PortScanCommandClient({ workerFactory: makeFactory(workers), log() {} }) +} + +describe('PortScanCommandClient', () => { + afterEach(() => { + vi.useRealTimers() + execFileMock.mockReset() + }) + + it('dispatches one command at a time so a stalled spawn cannot fan out', async () => { + const workers: FakeWorker[] = [] + const client = makeClient(workers) + + const first = client.run('lsof', ['-nP']) + const second = client.run('ps', ['-p', '1']) + await Promise.resolve() + + // Why (#11161): a second in-flight request would have its deadline armed + // while the worker's loop is still blocked inside the first uv_spawn. + expect(workers[0].postedRequests.map((request) => request.command)).toEqual(['lsof']) + + workers[0].respond({ ok: true, stdout: 'lsof-out', spawnMs: 12 }) + await expect(first).resolves.toEqual({ stdout: 'lsof-out', spawnMs: 12 }) + expect(workers[0].postedRequests.map((request) => request.command)).toEqual(['lsof', 'ps']) + + workers[0].respond({ ok: true, stdout: 'ps-out', spawnMs: 9 }) + await expect(second).resolves.toEqual({ stdout: 'ps-out', spawnMs: 9 }) + }) + + it('ignores responses that do not correlate with the active request', async () => { + const workers: FakeWorker[] = [] + const client = makeClient(workers) + + const pending = client.run('lsof', []) + await Promise.resolve() + workers[0].emit('message', { id: 999, ok: true, stdout: 'stale', spawnMs: 1 }) + workers[0].respond({ ok: true, stdout: 'fresh', spawnMs: 3 }) + + await expect(pending).resolves.toMatchObject({ stdout: 'fresh' }) + }) + + it('rehydrates a worker-side command timeout so the scan can back off', async () => { + const workers: FakeWorker[] = [] + const client = makeClient(workers) + + const pending = client.run('lsof', []) + await Promise.resolve() + workers[0].respond({ ok: false, timedOut: true, error: 'lsof timed out after 4000ms' }) + + await expect(pending).rejects.toBeInstanceOf(PortScanCommandTimeoutError) + }) + + it('reports a worker crash as a non-timeout error and respawns for the next call', async () => { + const workers: FakeWorker[] = [] + const client = makeClient(workers) + + const pending = client.run('lsof', []) + await Promise.resolve() + workers[0].emit('error', new Error('worker blew up')) + + const error = await pending.catch((err: unknown) => err) + expect(error).toBeInstanceOf(Error) + expect(error).not.toBeInstanceOf(PortScanCommandTimeoutError) + expect(workers[0].terminated).toBe(true) + + const next = client.run('ps', []) + await Promise.resolve() + expect(workers).toHaveLength(2) + workers[1].respond({ ok: true, stdout: 'ps-out', spawnMs: 2 }) + await expect(next).resolves.toMatchObject({ stdout: 'ps-out' }) + }) + + it('terminates a silent worker at the call deadline without arming the backoff', async () => { + vi.useFakeTimers() + const workers: FakeWorker[] = [] + const client = makeClient(workers) + + const pending = client.run('netstat', ['-ano']) + const settled = pending.catch((err: unknown) => err) + await vi.advanceTimersByTimeAsync(CALL_DEADLINE_MS) + + const error = await settled + expect(error).toBeInstanceOf(Error) + expect(error).not.toBeInstanceOf(PortScanCommandTimeoutError) + expect(workers[0].terminated).toBe(true) + }) + + it('rejects overflow instead of growing the queue without bound', async () => { + const workers: FakeWorker[] = [] + const client = makeClient(workers) + + const accepted = Array.from({ length: MAX_QUEUED_CALLS + 1 }, () => client.run('lsof', [])) + const overflow = client.run('lsof', []) + + const error = await overflow.catch((err: unknown) => err) + expect(error).toBeInstanceOf(Error) + expect(error).not.toBeInstanceOf(PortScanCommandTimeoutError) + + for (let i = 0; i < accepted.length; i++) { + workers[0].respond({ ok: true, stdout: 'drained', spawnMs: 1 }) + } + await expect(Promise.all(accepted)).resolves.toHaveLength(MAX_QUEUED_CALLS + 1) + }) + + it('fails closed instead of spawning the command on this thread', async () => { + const client = new PortScanCommandClient({ + workerFactory: () => { + throw new Error('worker entry not found') + }, + log() {} + }) + + const error = await client.run('lsof', []).catch((err: unknown) => err) + + expect(isPortScanWorkerUnavailableError(error)).toBe(true) + expect(execFileMock).not.toHaveBeenCalled() + }) +}) + +// Why: the packaged branch never runs in dev or e2e (both take the __dirname +// path), so it is pinned here at the path-construction level. Whether Electron's +// asar shim can load a Worker entry from inside app.asar is not testable here — +// that still needs a packaged smoke test. +describe('resolveWorkerEntryPath', () => { + const WORKER_ENTRY_FILENAME = 'port-scan-command-worker-entry.js' + + it('resolves a packaged build under resourcesPath/app.asar/out/main', () => { + const resourcesPath = join(sep, 'Applications', 'Orca.app', 'Contents', 'Resources') + + const resolved = resolveWorkerEntryPath({ + isPackaged: true, + resourcesPath, + moduleDir: join(sep, 'unpackaged', 'out', 'main') + }) + + expect(resolved.startsWith(`${resourcesPath}${sep}`)).toBe(true) + expect(resolved.slice(resourcesPath.length + 1).split(sep)).toEqual([ + 'app.asar', + 'out', + 'main', + WORKER_ENTRY_FILENAME + ]) + }) + + it('ignores resourcesPath when the app is not packaged', () => { + const moduleDir = join(sep, 'repo', 'out', 'main') + + const resolved = resolveWorkerEntryPath({ + isPackaged: false, + resourcesPath: join(sep, 'Applications', 'Orca.app', 'Contents', 'Resources'), + moduleDir + }) + + expect(resolved).toBe(join(moduleDir, WORKER_ENTRY_FILENAME)) + expect(resolved).not.toContain('app.asar') + }) + + // A rename in the build config would leave both branches pointing at a file + // that is never emitted, and only the packaged one fails silently. + it('names the entry the main build actually emits', () => { + const config = readFileSync( + join(import.meta.dirname, '..', '..', '..', 'electron.vite.config.ts'), + 'utf8' + ) + + expect(config).toContain("'port-scan-command-worker-entry': resolve(") + }) +}) + +const REAL_WORKER_BLOCK_MS = 800 + +// Mirrors the worker entry's protocol but blocks its own thread first, standing +// in for the endpoint-security hook that makes CreateProcessW take seconds. +const BLOCKING_WORKER_SCRIPT = ` +const { parentPort } = require('node:worker_threads') +const { execFile } = require('node:child_process') +parentPort.on('message', (request) => { + const startedAt = Date.now() + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ${REAL_WORKER_BLOCK_MS}) + execFile(process.execPath, ['-e', ''], () => { + parentPort.postMessage({ + id: request.id, + ok: true, + stdout: 'ok', + spawnMs: Date.now() - startedAt + }) + }) +}) +` + +describe('PortScanCommandClient on a real worker thread', () => { + it('keeps the calling event loop responsive while a spawn stalls', async () => { + const client = new PortScanCommandClient({ + workerFactory: () => new Worker(BLOCKING_WORKER_SCRIPT, { eval: true }), + log() {} + }) + + let last = Date.now() + let maxStallMs = 0 + const probe = setInterval(() => { + const now = Date.now() + maxStallMs = Math.max(maxStallMs, now - last - 10) + last = now + }, 10) + try { + const results = await Promise.all([client.run('lsof', []), client.run('ps', [])]) + + expect(results.map((result) => result.stdout)).toEqual(['ok', 'ok']) + expect(results[0].spawnMs).toBeGreaterThanOrEqual(REAL_WORKER_BLOCK_MS - 100) + expect(maxStallMs).toBeLessThan(400) + } finally { + clearInterval(probe) + } + }, 30_000) +}) diff --git a/src/main/ports/port-scan-command-client.ts b/src/main/ports/port-scan-command-client.ts new file mode 100644 index 00000000000..9748a8cfc2b --- /dev/null +++ b/src/main/ports/port-scan-command-client.ts @@ -0,0 +1,364 @@ +import { existsSync } from 'node:fs' +import { join } from 'node:path' +import { Worker } from 'node:worker_threads' +import { + PORT_SCAN_COMMAND_TIMEOUT_MS, + PortScanCommandTimeoutError, + type PortScanCommandRequest, + type PortScanCommandResponse +} from './port-scan-command-protocol' + +// Why (#11161): a lazily-spawned, unref'd worker runs the port scan's probe +// spawns off the Electron main-process event loop, because libuv performs +// process creation inline on the calling thread. Lifecycle (FIFO one-at-a-time +// dispatch, per-call deadlines, respawn-on-fault, idle teardown, fail-closed) +// mirrors src/main/ai-vault/session-scanner-opencode-sqlite-worker-client.ts; +// the duplicated ~150 lines are cheaper than a premature shared abstraction, so +// a third adopter should extract one. +// +// This module contains the literal text require('electron'), so it must never +// become reachable from a plain-Node fork entry (build-plugins/ +// plain-node-entry-guard.ts fails the build on that text, try/catch or not). + +// Why: the worker's own loop absorbs the spawn stall, so the client only needs +// a backstop for a wedged thread. Kept at 30s because a scan sits on the +// user-blocking localhost-label allowlist path (src/main/ipc/ +// localhost-worktree-labels.ts). +export const WORKER_STALL_GRACE_MS = 26_000 +export const CALL_DEADLINE_MS = PORT_SCAN_COMMAND_TIMEOUT_MS + WORKER_STALL_GRACE_MS +// Deliberately far longer than the 30s scan cadence so a visible window does not +// re-create the worker every tick; the renderer stops the interval when hidden, +// so this is effectively the hidden-window teardown. +export const IDLE_TEARDOWN_MS = 5 * 60_000 +export const MAX_CONSECUTIVE_DEATHS = 3 +// One scan issues at most three commands; anything beyond this is pile-up. +export const MAX_QUEUED_CALLS = 8 + +export type PortScanCommandResult = { stdout: string; spawnMs: number } +export type PortScanWorkerFactory = () => Worker + +// Distinguishes "no worker at all" from a timeout or crash so the scanner can +// log it once and callers never mistake it for a command timeout. +class PortScanWorkerUnavailableError extends Error {} + +/** True when a scan failed because the probe worker could not be started. */ +export function isPortScanWorkerUnavailableError(error: unknown): boolean { + return error instanceof PortScanWorkerUnavailableError +} + +type PendingCall = { + request: PortScanCommandRequest + resolve: (value: PortScanCommandResult) => void + reject: (error: Error) => void + timer: NodeJS.Timeout | null +} + +/** + * Main-thread bridge that runs port-scan probe commands on a persistent worker + * thread. Dispatches one command at a time (FIFO), times each call out from + * dispatch, respawns after faults (capped by `MAX_CONSECUTIVE_DEATHS`), tears + * the worker down after `IDLE_TEARDOWN_MS`, and fails closed when no worker can + * be spawned rather than moving process creation back onto the main thread. + */ +export class PortScanCommandClient { + private worker: Worker | null = null + private active: PendingCall | null = null + private queue: PendingCall[] = [] + private idleTimer: NodeJS.Timeout | null = null + private consecutiveDeaths = 0 + private nextId = 1 + private loggedWorkerUnavailable = false + private cleanupWorkerListeners: (() => void) | null = null + private readonly workerFactory: PortScanWorkerFactory + private readonly log: (message: string) => void + + constructor(options: { workerFactory: PortScanWorkerFactory; log?: (message: string) => void }) { + this.workerFactory = options.workerFactory + this.log = options.log ?? ((message) => console.warn(message)) + } + + /** + * Run one probe command on the worker. + * @param command - Executable name (lsof, ps, netstat, powershell.exe). + * @param args - Argument vector passed verbatim to execFile. + * @returns The command's stdout plus its measured process-creation latency. + */ + run(command: string, args: string[]): Promise { + return new Promise((resolve, reject) => { + if (this.queue.length >= MAX_QUEUED_CALLS) { + reject(new Error(`Port scan command queue is full; dropped ${command}.`)) + return + } + // A fresh burst from full idle starts a new scan: clear any death count + // carried from a prior scan so the respawn cap can't drain this scan early. + if (!this.active && this.queue.length === 0) { + this.consecutiveDeaths = 0 + } + this.queue.push({ + request: { id: this.nextId++, command, args }, + resolve, + reject, + timer: null + }) + this.pump() + }) + } + + private pump(): void { + if (this.active || this.queue.length === 0) { + return + } + const worker = this.ensureWorker() + if (!worker) { + this.failQueuedAsUnavailable() + return + } + const call = this.queue.shift() + if (!call) { + return + } + this.active = call + this.clearIdleTimer() + // Why (#11161): one at a time. uv_spawn blocks the worker's own loop, so a + // second concurrent request would have its deadline armed while the first + // spawn is still stalling the thread, producing a false timeout. + call.timer = setTimeout(() => this.onDeadline(call), CALL_DEADLINE_MS) + call.timer.unref?.() + worker.postMessage(call.request) + } + + private ensureWorker(): Worker | null { + if (this.worker) { + return this.worker + } + try { + const worker = this.workerFactory() + const onMessage = (response: PortScanCommandResponse): void => this.onMessage(response) + const onError = (error: Error): void => this.onWorkerFault(error) + const onExit = (code: number): void => this.onWorkerExit(code) + worker.on('message', onMessage) + worker.on('error', onError) + worker.on('exit', onExit) + this.cleanupWorkerListeners = () => { + worker.off('message', onMessage) + worker.off('error', onError) + worker.off('exit', onExit) + } + // Never keep the app alive for a port scan. + worker.unref?.() + this.worker = worker + return worker + } catch (err) { + // Why (#11161): never fall back to in-process execFile here; a missing + // bundle must report port scanning as unavailable rather than reintroduce + // the main-thread freeze this worker boundary exists to prevent. + if (!this.loggedWorkerUnavailable) { + this.loggedWorkerUnavailable = true + this.log(`[workspace-ports] probe worker unavailable. ${errorMessage(err)}`) + } + return null + } + } + + private onMessage(response: PortScanCommandResponse): void { + const call = this.active + if (!call || call.request.id !== response.id) { + return + } + this.consecutiveDeaths = 0 + if (response.ok) { + this.settle(call, () => call.resolve({ stdout: response.stdout, spawnMs: response.spawnMs })) + } else { + const error = response.timedOut + ? new PortScanCommandTimeoutError(response.error) + : new Error(response.error) + this.settle(call, () => call.reject(error)) + } + this.afterSettle() + } + + private onDeadline(call: PendingCall): void { + if (this.active !== call) { + return + } + // Plain Error on purpose: a wedged worker is not a command timeout and must + // never feed the scanner's timeout backoff. + this.onWorkerFault(new Error(`Port scan probe worker stalled after ${CALL_DEADLINE_MS}ms`)) + } + + private onWorkerExit(code: number): void { + // A clean self-exit is not a death, but the stale handle must be dropped or + // the next dispatch would post into a dead worker and stall to its deadline. + if (code === 0 && !this.active && this.queue.length === 0) { + this.destroyWorker() + return + } + this.onWorkerFault(new Error(`Port scan probe worker exited with code ${code}`)) + } + + private onWorkerFault(error: Error): void { + const failed = this.active + this.destroyWorker() + this.consecutiveDeaths++ + if (failed) { + this.settle(failed, () => failed.reject(error)) + } + if (this.consecutiveDeaths >= MAX_CONSECUTIVE_DEATHS) { + this.drainQueueAfterCrashLoop(error) + return + } + if (this.queue.length > 0) { + this.pump() + } + } + + private drainQueueAfterCrashLoop(error: Error): void { + const pending = this.queue + this.queue = [] + this.consecutiveDeaths = 0 + const drainError = new Error(`Port scan probe worker crashed repeatedly (${error.message})`) + for (const call of pending) { + this.settle(call, () => call.reject(drainError)) + } + } + + private failQueuedAsUnavailable(): void { + const pending = this.queue + this.queue = [] + for (const call of pending) { + this.settle(call, () => + call.reject(new PortScanWorkerUnavailableError('port scan probe worker spawn failed')) + ) + } + } + + private settle(call: PendingCall, run: () => void): void { + if (call.timer) { + clearTimeout(call.timer) + call.timer = null + } + if (this.active === call) { + this.active = null + } + run() + } + + private afterSettle(): void { + if (this.queue.length > 0) { + this.pump() + } else { + this.scheduleIdleTeardown() + } + } + + private scheduleIdleTeardown(): void { + this.clearIdleTimer() + if (!this.worker) { + return + } + this.idleTimer = setTimeout(() => this.teardownIfIdle(), IDLE_TEARDOWN_MS) + this.idleTimer.unref?.() + } + + private teardownIfIdle(): void { + this.idleTimer = null + // Only tear down with nothing active AND nothing queued: a request arriving + // as the timer fires must never be lost to a self-exiting worker. + if (this.active || this.queue.length > 0) { + return + } + this.destroyWorker() + } + + private clearIdleTimer(): void { + if (this.idleTimer) { + clearTimeout(this.idleTimer) + this.idleTimer = null + } + } + + private destroyWorker(): void { + this.clearIdleTimer() + const worker = this.worker + this.worker = null + if (!worker) { + return + } + this.cleanupWorkerListeners?.() + this.cleanupWorkerListeners = null + worker.removeAllListeners() + // Terminating can orphan a probe child mid-spawn; the worker reaps what it + // can on exit, and every probe here is short-lived. + void worker.terminate().catch(() => undefined) + } +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} + +const WORKER_ENTRY_FILENAME = 'port-scan-command-worker-entry.js' + +/** Where the built worker entry can live: packaged resources or the build dir. */ +export type WorkerEntryLayout = { + isPackaged: boolean + resourcesPath: string + moduleDir: string +} + +/** + * Resolve the built worker entry for one runtime layout. + * @param layout - Packaged flag plus both candidate roots. + * @returns Path passed to `new Worker()`. + */ +export function resolveWorkerEntryPath(layout: WorkerEntryLayout): string { + // Packaged builds leave this entry inside app.asar — only forked child + // processes are asarUnpack'd — so it resolves off resourcesPath rather than + // the bundler's __dirname, matching the shipped stt/warp/opencode workers. + // Split out from the electron read so the packaged branch is testable without + // a packaged build. + if (layout.isPackaged) { + return join(layout.resourcesPath, 'app.asar', 'out', 'main', WORKER_ENTRY_FILENAME) + } + return join(layout.moduleDir, WORKER_ENTRY_FILENAME) +} + +function currentWorkerEntryLayout(): WorkerEntryLayout { + let app: { isPackaged: boolean } | null = null + try { + app = require('electron').app ?? null + } catch { + app = null + } + return { + isPackaged: app?.isPackaged === true, + resourcesPath: process.resourcesPath, + moduleDir: __dirname + } +} + +function defaultWorkerFactory(): Worker { + const workerPath = resolveWorkerEntryPath(currentWorkerEntryLayout()) + // Why: a missing built entry must throw synchronously so the client can fail + // closed before it waits on a worker that can never post a result. + if (!existsSync(workerPath)) { + throw new Error(`Port scan command worker entry not found: ${workerPath}`) + } + return new Worker(workerPath) +} + +let sharedClient: PortScanCommandClient | null = null + +/** + * Run a port-scan probe command through the process-wide worker client. + * @param command - Executable name (lsof, ps, netstat, powershell.exe). + * @param args - Argument vector passed verbatim to execFile. + * @returns The command's stdout plus its measured process-creation latency. + */ +export function runPortScanCommand( + command: string, + args: string[] +): Promise { + sharedClient ??= new PortScanCommandClient({ workerFactory: defaultWorkerFactory }) + return sharedClient.run(command, args) +} diff --git a/src/main/ports/port-scan-command-execution.test.ts b/src/main/ports/port-scan-command-execution.test.ts new file mode 100644 index 00000000000..15e97d4a3a9 --- /dev/null +++ b/src/main/ports/port-scan-command-execution.test.ts @@ -0,0 +1,97 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { runPortScanCommandInProcess } from './port-scan-command-execution' +import { + PORT_SCAN_COMMAND_TIMEOUT_MS, + PortScanCommandTimeoutError, + WATCHDOG_GRACE_MS +} from './port-scan-command-protocol' + +const execFileMock = vi.hoisted(() => vi.fn()) + +vi.mock('child_process', () => ({ + execFile: execFileMock +})) + +// Why (#11161): must outlast the whole watchdog budget, otherwise a watchdog +// armed before execFile still survives the stall and the ordering goes unpinned. +const SPAWN_STALL_MS = PORT_SCAN_COMMAND_TIMEOUT_MS + WATCHDOG_GRACE_MS + 200 +const LSOF_OUTPUT = ['p123', 'cnode', 'n127.0.0.1:5173'].join('\n') + +/** Emulates a hooked CreateProcessW: blocks the calling thread inside uv_spawn. */ +function blockCallingThread(ms: number): void { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) +} + +describe('runPortScanCommandInProcess', () => { + afterEach(() => { + vi.useRealTimers() + execFileMock.mockReset() + }) + + it('arms the watchdog only after process creation returns', async () => { + execFileMock.mockImplementation( + (_command: string, _args: string[], _options: unknown, callback: unknown) => { + blockCallingThread(SPAWN_STALL_MS) + // The command itself is healthy once it finally starts. + setTimeout(() => (callback as (e: null, out: string) => void)(null, LSOF_OUTPUT), 5) + return { kill: vi.fn() } + } + ) + + const result = await runPortScanCommandInProcess('lsof', ['-nP', '-iTCP']) + + expect(result.stdout).toBe(LSOF_OUTPUT) + expect(result.spawnMs).toBeGreaterThanOrEqual(SPAWN_STALL_MS - 100) + }) + + it('kills the child and times out when the callback never arrives', async () => { + vi.useFakeTimers() + const killMock = vi.fn() + execFileMock.mockImplementation(() => ({ kill: killMock })) + + let settled = false + const promise = runPortScanCommandInProcess('lsof', []).catch((error: unknown) => { + settled = true + return error + }) + + await vi.advanceTimersByTimeAsync(PORT_SCAN_COMMAND_TIMEOUT_MS) + expect(settled).toBe(false) + + await vi.advanceTimersByTimeAsync(WATCHDOG_GRACE_MS) + + expect(await promise).toBeInstanceOf(PortScanCommandTimeoutError) + expect(killMock).toHaveBeenCalled() + }) + + it("classifies Node's own execFile timeout kill as a command timeout", async () => { + execFileMock.mockImplementation( + (_command: string, _args: string[], _options: unknown, callback: unknown) => { + const killed = Object.assign(new Error('Command failed: lsof'), { + killed: true, + signal: 'SIGTERM' + }) + setTimeout(() => (callback as (e: Error) => void)(killed), 0) + return { kill: vi.fn() } + } + ) + + await expect(runPortScanCommandInProcess('lsof', [])).rejects.toBeInstanceOf( + PortScanCommandTimeoutError + ) + }) + + it('leaves a genuine command failure unclassified so the scan does not back off', async () => { + execFileMock.mockImplementation( + (_command: string, _args: string[], _options: unknown, callback: unknown) => { + setTimeout(() => (callback as (e: Error) => void)(new Error('spawn ENOENT')), 0) + return { kill: vi.fn() } + } + ) + + const error = await runPortScanCommandInProcess('lsof', []).catch((err: unknown) => err) + + expect(error).toBeInstanceOf(Error) + expect(error).not.toBeInstanceOf(PortScanCommandTimeoutError) + }) +}) diff --git a/src/main/ports/port-scan-command-execution.ts b/src/main/ports/port-scan-command-execution.ts new file mode 100644 index 00000000000..9c63fc9aaa3 --- /dev/null +++ b/src/main/ports/port-scan-command-execution.ts @@ -0,0 +1,115 @@ +import { execFile, type ChildProcess } from 'node:child_process' +import { + PORT_SCAN_COMMAND_TIMEOUT_MS, + PortScanCommandTimeoutError, + WATCHDOG_GRACE_MS, + portScanCommandTimeoutMessage +} from './port-scan-command-protocol' + +// Why (#11161): libuv runs uv_spawn inline on whichever event loop calls it, so +// an endpoint-security hook on CreateProcessW stalls that thread for the whole +// spawn. This module is only ever entered from the worker thread +// (port-scan-command-worker-entry.ts); nothing on the main thread may import it. + +const WATCHDOG_TIMEOUT_MS = PORT_SCAN_COMMAND_TIMEOUT_MS + WATCHDOG_GRACE_MS + +const activeChildren = new Set() + +/** + * Run a port-scan probe command and report how long process creation took. + * @param command - Executable name resolved against the worker's PATH. + * @param args - Argument vector passed verbatim, never shell-interpolated. + * @returns The command's stdout plus `spawnMs`, the measured process-creation + * latency callers use to skip optional follow-up commands on a stalled host. + * @throws PortScanCommandTimeoutError when the command itself outran its budget. + */ +export async function runPortScanCommandInProcess( + command: string, + args: string[] +): Promise<{ stdout: string; spawnMs: number }> { + return await new Promise((resolve, reject) => { + let settled = false + let timer: NodeJS.Timeout | null = null + let spawnMs = 0 + let child: ChildProcess | undefined + + const settle = (callback: () => void): void => { + if (settled) { + return + } + settled = true + if (timer) { + clearTimeout(timer) + } + if (child) { + activeChildren.delete(child) + } + callback() + } + + const startedAt = Date.now() + try { + child = execFile( + command, + args, + { + timeout: PORT_SCAN_COMMAND_TIMEOUT_MS, + maxBuffer: 2 * 1024 * 1024, + windowsHide: true + }, + (error, stdout) => { + if (error) { + // Node kills its own execFile timeout with a signal, which surfaces + // as killed:true — the only way to tell a timeout from a real error. + const timedOut = (error as { killed?: boolean }).killed === true + settle(() => + reject( + timedOut + ? new PortScanCommandTimeoutError( + portScanCommandTimeoutMessage(command, PORT_SCAN_COMMAND_TIMEOUT_MS) + ) + : error + ) + ) + return + } + settle(() => resolve({ stdout: String(stdout), spawnMs })) + } + ) + } catch (error) { + settle(() => reject(error)) + return + } + + // Why (#11161): measured after execFile returns, because that call blocks + // for the whole of process creation. Arming the watchdog earlier would + // charge the spawn stall against the command's budget and fire immediately. + spawnMs = Date.now() - startedAt + if (settled || !child) { + return + } + activeChildren.add(child) + timer = setTimeout(() => { + settle(() => { + child?.kill() + reject( + new PortScanCommandTimeoutError( + portScanCommandTimeoutMessage(command, WATCHDOG_TIMEOUT_MS) + ) + ) + }) + }, WATCHDOG_TIMEOUT_MS) + }) +} + +/** Best-effort reap so a terminated worker does not orphan an in-flight probe. */ +export function killActivePortScanCommands(): void { + for (const child of activeChildren) { + try { + child.kill() + } catch { + // Already exited; nothing to reap. + } + } + activeChildren.clear() +} diff --git a/src/main/ports/port-scan-command-import-boundary.test.ts b/src/main/ports/port-scan-command-import-boundary.test.ts new file mode 100644 index 00000000000..bd9b9d05fb8 --- /dev/null +++ b/src/main/ports/port-scan-command-import-boundary.test.ts @@ -0,0 +1,18 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +// Why (#11161): libuv runs process creation inline on the calling event loop, +// so the port scan only stays off CrBrowserMain while these main-thread modules +// spawn nothing themselves. Spawning belongs to port-scan-command-execution.ts, +// which only the worker entry imports. +const MAIN_THREAD_MODULES = ['local-workspace-port-scanner.ts', 'port-scan-command-client.ts'] + +describe('port scan main-thread spawn boundary', () => { + it.each(MAIN_THREAD_MODULES)('keeps %s free of child_process', (fileName) => { + const source = readFileSync(join(import.meta.dirname, fileName), 'utf8') + + expect(source).not.toMatch(/from\s+['"](node:)?child_process['"]/) + expect(source).not.toMatch(/require\(\s*['"](node:)?child_process['"]\s*\)/) + }) +}) diff --git a/src/main/ports/port-scan-command-protocol.ts b/src/main/ports/port-scan-command-protocol.ts new file mode 100644 index 00000000000..02beeb982a8 --- /dev/null +++ b/src/main/ports/port-scan-command-protocol.ts @@ -0,0 +1,36 @@ +// Why (#11161): request/response shapes plus the timeout error shared by the +// port-scan worker entry and its main-thread client. Kept free of Electron, +// node:worker_threads and node:child_process so importing it from either side +// can never drag the other side's dependencies across the boundary. + +export const PORT_SCAN_COMMAND_TIMEOUT_MS = 4_000 +// Node's own execFile timeout is the primary killer; the manual watchdog only +// covers "the callback never arrived", so it must fire strictly later. +export const WATCHDOG_GRACE_MS = 1_000 + +export type PortScanCommandRequest = { + id: number + command: string + args: string[] +} + +export type PortScanCommandResponse = + | { id: number; ok: true; stdout: string; spawnMs: number } + | { id: number; ok: false; timedOut: boolean; error: string } + +/** + * Raised when a port-scan command was killed after exceeding its budget. Errors + * do not survive structured clone as subclasses, so the worker reports a + * `timedOut` flag and the client reconstructs this type from it. + */ +export class PortScanCommandTimeoutError extends Error { + constructor(message: string) { + super(message) + this.name = 'PortScanCommandTimeoutError' + } +} + +/** Shared wording so worker-side and client-side timeouts read identically. */ +export function portScanCommandTimeoutMessage(command: string, timeoutMs: number): string { + return `${command} timed out after ${timeoutMs}ms` +} diff --git a/src/main/ports/port-scan-command-worker-entry.ts b/src/main/ports/port-scan-command-worker-entry.ts new file mode 100644 index 00000000000..b5cb5ad5a3a --- /dev/null +++ b/src/main/ports/port-scan-command-worker-entry.ts @@ -0,0 +1,53 @@ +import { parentPort } from 'node:worker_threads' +import { + killActivePortScanCommands, + runPortScanCommandInProcess +} from './port-scan-command-execution' +import { + PortScanCommandTimeoutError, + type PortScanCommandRequest, + type PortScanCommandResponse +} from './port-scan-command-protocol' + +// Why (#11161): process creation blocks the event loop that issues it. Running +// the port scan's probe commands on this worker thread keeps an EDR-hooked +// CreateProcessW off CrBrowserMain. The client dispatches one request at a +// time, so this loop stays serial; imports must remain electron-free. + +if (!parentPort) { + throw new Error('Port scan command worker must run with a parent port.') +} +const port = parentPort + +process.once('exit', killActivePortScanCommands) + +async function handleRequest(request: PortScanCommandRequest): Promise { + try { + const { stdout, spawnMs } = await runPortScanCommandInProcess(request.command, request.args) + return { id: request.id, ok: true, stdout, spawnMs } + } catch (err) { + return { + id: request.id, + ok: false, + timedOut: err instanceof PortScanCommandTimeoutError, + error: err instanceof Error ? err.message : String(err) + } + } +} + +port.on('message', (request: PortScanCommandRequest) => { + void handleRequest(request).then((response) => { + try { + port.postMessage(response) + } catch { + // A non-cloneable result would otherwise post nothing and leave the client + // waiting out its deadline; fail that request fast instead. + port.postMessage({ + id: request.id, + ok: false, + timedOut: false, + error: 'Port scan command result could not be serialized.' + }) + } + }) +}) diff --git a/src/main/ports/workspace-port-ownership.test.ts b/src/main/ports/workspace-port-ownership.test.ts new file mode 100644 index 00000000000..b367fcbf29c --- /dev/null +++ b/src/main/ports/workspace-port-ownership.test.ts @@ -0,0 +1,57 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { killWorkspacePort } from './workspace-port-ownership' + +const scanWorkspacePortsMock = vi.hoisted(() => vi.fn()) + +vi.mock('./local-workspace-port-scanner', () => ({ + scanWorkspacePorts: scanWorkspacePortsMock +})) + +const worktrees = [{ id: 'repo::/repo', repoId: 'repo', displayName: 'main', path: '/repo' }] + +describe('killWorkspacePort', () => { + afterEach(() => { + scanWorkspacePortsMock.mockReset() + vi.restoreAllMocks() + }) + + // Regression for #11161 review: on an EDR-hooked host the background poller + // alternates the metadata skip, so an unscoped skip would fail Stop with + // "Only workspace-owned local processes can be stopped here" every other try. + it('requires owner metadata from the authorizing re-scan', async () => { + scanWorkspacePortsMock.mockResolvedValue({ platform: 'darwin', scannedAt: 0, ports: [] }) + + await killWorkspacePort(worktrees, { pid: 123, port: 5173 }) + + expect(scanWorkspacePortsMock).toHaveBeenCalledWith(worktrees, undefined, { + requireMetadata: true + }) + }) + + it('refuses a pid the re-scan does not attribute to a workspace', async () => { + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + scanWorkspacePortsMock.mockResolvedValue({ + platform: 'darwin', + scannedAt: 0, + ports: [ + { + id: '127.0.0.1:5173:123', + bindHost: '127.0.0.1', + connectHost: '127.0.0.1', + port: 5173, + pid: 123, + protocol: 'http', + kind: 'external' + } + ] + }) + + const result = await killWorkspacePort(worktrees, { pid: 123, port: 5173 }) + + expect(result).toEqual({ + ok: false, + reason: 'Only workspace-owned local processes can be stopped here.' + }) + expect(killSpy).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ports/workspace-port-ownership.ts b/src/main/ports/workspace-port-ownership.ts index 9ae5bf6582a..9f7df2465cb 100644 --- a/src/main/ports/workspace-port-ownership.ts +++ b/src/main/ports/workspace-port-ownership.ts @@ -8,7 +8,7 @@ import type { WorkspacePortProbe, WorkspacePortScanResult } from '../../shared/workspace-ports' -import { scanWorkspacePorts } from './local-workspace-port-scanner' +import { scanWorkspacePorts, type WorkspacePortScanOptions } from './local-workspace-port-scanner' export type WorkspacePortProbeInput = WorkspacePortProbe & { connectionId?: string | null @@ -82,7 +82,9 @@ export async function killWorkspacePort( return { ok: false, reason: 'Invalid process or port.' } } - const scan = await scanWorkspacePorts([...worktrees]) + // Why (#11161): this re-scan is the authorization check for SIGTERM, so it + // must never land on a cycle that skipped the owner-attribution metadata. + const scan = await scanWorkspacePorts([...worktrees], undefined, { requireMetadata: true }) const port = scan.ports.find( (candidate) => candidate.pid === args.pid && candidate.port === args.port ) @@ -113,7 +115,8 @@ export async function killWorkspacePort( } export async function scanWorkspacePortProbes( - worktrees: readonly WorkspacePortProbe[] + worktrees: readonly WorkspacePortProbe[], + options?: WorkspacePortScanOptions ): Promise { - return scanWorkspacePorts([...worktrees]) + return scanWorkspacePorts([...worktrees], undefined, options) } diff --git a/src/main/quit-teardown-start-gate.ts b/src/main/quit-teardown-start-gate.ts index be970495a4b..a96b7b3f38b 100644 --- a/src/main/quit-teardown-start-gate.ts +++ b/src/main/quit-teardown-start-gate.ts @@ -9,4 +9,16 @@ export class QuitTeardownStartGate { this.started = true return true } + + hasStarted(): boolean { + return this.started + } + + resetForTests(): void { + this.started = false + } } + +// Why shared, not per-caller: subsystems fence new work on "the committed quit has begun", and a +// second instance would answer no while the real teardown is already draining what it snapshotted. +export const quitTeardownStartGate = new QuitTeardownStartGate() diff --git a/src/main/runtime/relay/desktop-relay-service.test.ts b/src/main/runtime/relay/desktop-relay-service.test.ts index a69c5a74fa6..508b3fb64d7 100644 --- a/src/main/runtime/relay/desktop-relay-service.test.ts +++ b/src/main/runtime/relay/desktop-relay-service.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import type { MobilePairingConnectionContext } from '../runtime-rpc' import { DesktopRelayService, pairingAuthorizationForContext } from './desktop-relay-service' @@ -61,6 +61,47 @@ describe('pairingAuthorizationForContext', () => { }) }) +describe('liveness safety net lifecycle', () => { + afterEach(() => vi.useRealTimers()) + + it('fences hard: the tick stops on fence and re-arms on the next auth mutation', () => { + // Why: a tick surviving the pre-sign-out fence could catch the window + // before the profile wipe and briefly resurrect a broker. + vi.useFakeTimers() + const coordinator = { + reconcile: vi.fn(), + ensureLive: vi.fn(), + fenceAndCloseNow: vi.fn(), + stop: vi.fn() + } + const service = Object.create(DesktopRelayService.prototype) as DesktopRelayService + Object.assign(service, { + coordinator, + demandLedger: { nextPendingExpiry: () => null }, + stopped: false, + livenessTimer: null, + demandExpiryTimer: null + }) + + service.start() + vi.advanceTimersByTime(5 * 60_000) + expect(coordinator.ensureLive).toHaveBeenCalledTimes(1) + + service.fenceAndCloseNow() + expect(coordinator.fenceAndCloseNow).toHaveBeenCalledOnce() + vi.advanceTimersByTime(30 * 60_000) + expect(coordinator.ensureLive).toHaveBeenCalledTimes(1) + + service.authMutated() + vi.advanceTimersByTime(5 * 60_000) + expect(coordinator.ensureLive).toHaveBeenCalledTimes(2) + + service.stop() + vi.advanceTimersByTime(30 * 60_000) + expect(coordinator.ensureLive).toHaveBeenCalledTimes(2) + }) +}) + describe('local-only mobile pairing', () => { it('refuses endpoint discovery and provisioning without opening Relay demand', async () => { const registry = { diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index 04236fea2b7..575cbe23bbc 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -96,9 +96,6 @@ export class DesktopRelayService { start(): void { this.refreshDemand() - if (!this.livenessTimer) { - this.livenessTimer = setInterval(() => this.ensureLive(), RELAY_LIVENESS_INTERVAL_MS) - } } // Safe to call from any wake signal (power resume, network change). @@ -113,6 +110,13 @@ export class DesktopRelayService { } fenceAndCloseNow(): void { + // Why: a fence must be hard — a surviving liveness tick could catch the + // window between the pre-sign-out fence and the profile wipe and briefly + // resurrect a broker. The next auth mutation re-arms via refreshDemand. + if (this.livenessTimer) { + clearInterval(this.livenessTimer) + this.livenessTimer = null + } this.coordinator.fenceAndCloseNow() } @@ -307,6 +311,9 @@ export class DesktopRelayService { if (this.stopped) { return } + if (!this.livenessTimer) { + this.livenessTimer = setInterval(() => this.ensureLive(), RELAY_LIVENESS_INTERVAL_MS) + } if (this.demandExpiryTimer) { clearTimeout(this.demandExpiryTimer) this.demandExpiryTimer = null diff --git a/src/main/ssh/ssh-connection-manager.test.ts b/src/main/ssh/ssh-connection-manager.test.ts index 3a7a73cbcbf..b2de497a006 100644 --- a/src/main/ssh/ssh-connection-manager.test.ts +++ b/src/main/ssh/ssh-connection-manager.test.ts @@ -94,4 +94,36 @@ describe('SshConnectionManager', () => { expect(await manager.connect(target)).toBe(replacement) expect(manager.getConnection(target.id)).toBe(replacement) }) + + it('closes a late-resolved connection without evicting the replacement', async () => { + let resolveFirst!: () => void + mockState.connectResults.push( + new Promise((resolve) => { + resolveFirst = resolve + }), + Promise.resolve() + ) + const manager = new SshConnectionManager({ onStateChange: vi.fn() }) + + const firstConnect = manager.connect(target) + await manager.disconnect(target.id) + const replacement = await manager.connect(target) + resolveFirst() + const late = await firstConnect + + await manager.disconnectConnection(target.id, late) + + expect(mockState.instances[0].disconnect).toHaveBeenCalledTimes(2) + expect(manager.getConnection(target.id)).toBe(replacement) + }) + + it('clears the pool entry when the closed connection is still the registered one', async () => { + const manager = new SshConnectionManager({ onStateChange: vi.fn() }) + const conn = await manager.connect(target) + + await manager.disconnectConnection(target.id, conn) + + expect(mockState.instances[0].disconnect).toHaveBeenCalledOnce() + expect(manager.getConnection(target.id)).toBeUndefined() + }) }) diff --git a/src/main/ssh/ssh-connection-manager.ts b/src/main/ssh/ssh-connection-manager.ts index 8bc17d06b70..e553bf906b5 100644 --- a/src/main/ssh/ssh-connection-manager.ts +++ b/src/main/ssh/ssh-connection-manager.ts @@ -76,6 +76,18 @@ export class SshConnectionManager { } } + /** + * Close one specific connection, clearing the pool entry only when it is still the registered one. + * Why: a cancelled connect whose transport opened late owns that exact connection — disconnecting + * by target id would tear down the replacement's live transport instead. + */ + async disconnectConnection(targetId: string, conn: SshConnection): Promise { + await conn.disconnect() + if (this.connections.get(targetId) === conn) { + this.connections.delete(targetId) + } + } + async reconnect(targetId: string): Promise { const conn = this.connections.get(targetId) if (!conn) { diff --git a/src/main/ssh/ssh-owner-recovery-retry.test.ts b/src/main/ssh/ssh-owner-recovery-retry.test.ts new file mode 100644 index 00000000000..9c8f4bd6a14 --- /dev/null +++ b/src/main/ssh/ssh-owner-recovery-retry.test.ts @@ -0,0 +1,106 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR, + PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR +} from '../../shared/pty-consumer-session' +import { retrySshOwnerRecoveryWhileBlocked } from './ssh-owner-recovery-retry' + +function publicationPendingError(): Error & { code: number } { + return Object.assign(new Error('Owner grant publication is still pending'), { + code: PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR + }) +} + +function supersededError(): Error & { code: number } { + return Object.assign(new Error('Owner recovery generation was superseded'), { + code: PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR + }) +} + +function openGate() { + return { + isCurrent: () => true, + onClosed: () => () => {} + } +} + +describe('SSH owner recovery retry', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('recovers once the incumbent grant publication settles', async () => { + vi.useFakeTimers() + const attempt = vi + .fn<() => Promise>() + .mockRejectedValueOnce(publicationPendingError()) + .mockRejectedValueOnce(publicationPendingError()) + .mockResolvedValue('recovered') + + const recovery = retrySshOwnerRecoveryWhileBlocked(attempt, openGate()) + await vi.advanceTimersByTimeAsync(75) + + await expect(recovery).resolves.toBe('recovered') + expect(attempt).toHaveBeenCalledTimes(3) + }) + + it('stops at the publication-settlement deadline', async () => { + vi.useFakeTimers() + const error = publicationPendingError() + const attempt = vi.fn<() => Promise>().mockRejectedValue(error) + + const recovery = retrySshOwnerRecoveryWhileBlocked(attempt, openGate(), 60) + const rejection = expect(recovery).rejects.toBe(error) + await vi.advanceTimersByTimeAsync(60) + + await rejection + expect(attempt).toHaveBeenCalledTimes(3) + }) + + it('does not retry unrelated failures', async () => { + const error = Object.assign(new Error('stale owner'), { code: -32041 }) + const attempt = vi.fn<() => Promise>().mockRejectedValue(error) + + await expect(retrySshOwnerRecoveryWhileBlocked(attempt, openGate())).rejects.toBe(error) + expect(attempt).toHaveBeenCalledOnce() + }) + + it('retries while a superseded transport is closing', async () => { + vi.useFakeTimers() + const attempt = vi + .fn<() => Promise>() + .mockRejectedValueOnce(supersededError()) + .mockResolvedValue('recovered') + + const recovery = retrySshOwnerRecoveryWhileBlocked(attempt, openGate()) + await vi.advanceTimersByTimeAsync(25) + + await expect(recovery).resolves.toBe('recovered') + expect(attempt).toHaveBeenCalledTimes(2) + }) + + it('stops waiting when the relay channel closes', async () => { + vi.useFakeTimers() + let current = true + let close: (() => void) | undefined + const error = publicationPendingError() + const attempt = vi.fn<() => Promise>().mockRejectedValue(error) + const recovery = retrySshOwnerRecoveryWhileBlocked(attempt, { + isCurrent: () => current, + onClosed: (listener) => { + close = listener + return () => { + close = undefined + } + } + }) + await vi.advanceTimersByTimeAsync(0) + expect(close).toBeTypeOf('function') + + current = false + close?.() + + await expect(recovery).rejects.toBe(error) + expect(attempt).toHaveBeenCalledOnce() + }) +}) diff --git a/src/main/ssh/ssh-owner-recovery-retry.ts b/src/main/ssh/ssh-owner-recovery-retry.ts new file mode 100644 index 00000000000..e5e78d9a4f2 --- /dev/null +++ b/src/main/ssh/ssh-owner-recovery-retry.ts @@ -0,0 +1,69 @@ +import { + PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR, + PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR +} from '../../shared/pty-consumer-session' + +// Why: bound polling when publication is settling or a superseded attempt is closing its transport. +export const SSH_OWNER_RECOVERY_WAIT_MS = 3_000 +const SSH_OWNER_RECOVERY_INITIAL_DELAY_MS = 25 +const SSH_OWNER_RECOVERY_MAX_DELAY_MS = 250 + +type SshOwnerRecoveryRetryGate = { + isCurrent: () => boolean + onClosed: (listener: () => void) => () => void +} + +function waitForRetry(delayMs: number, gate: SshOwnerRecoveryRetryGate): Promise { + return new Promise((resolve) => { + let settled = false + let timer: ReturnType | undefined + let unsubscribe = (): void => {} + const finish = (): void => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + unsubscribe() + resolve() + } + unsubscribe = gate.onClosed(finish) + timer = setTimeout(finish, delayMs) + timer.unref?.() + if (!gate.isCurrent()) { + finish() + } + }) +} + +export async function retrySshOwnerRecoveryWhileBlocked( + attempt: () => Promise, + gate: SshOwnerRecoveryRetryGate, + waitMs: number = SSH_OWNER_RECOVERY_WAIT_MS +): Promise { + const deadline = Date.now() + waitMs + let delayMs = SSH_OWNER_RECOVERY_INITIAL_DELAY_MS + while (true) { + try { + return await attempt() + } catch (error) { + const code = (error as { code?: unknown } | null | undefined)?.code + if ( + (code !== PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR && + code !== PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR) || + !gate.isCurrent() + ) { + throw error + } + const remainingMs = deadline - Date.now() + if (remainingMs <= 0) { + throw error + } + await waitForRetry(Math.min(delayMs, remainingMs), gate) + if (!gate.isCurrent()) { + throw error + } + delayMs = Math.min(delayMs * 2, SSH_OWNER_RECOVERY_MAX_DELAY_MS) + } + } +} diff --git a/src/main/ssh/ssh-relay-session-recovery-durability.test.ts b/src/main/ssh/ssh-relay-session-recovery-durability.test.ts index df7bce635ec..6e1fe81c5a5 100644 --- a/src/main/ssh/ssh-relay-session-recovery-durability.test.ts +++ b/src/main/ssh/ssh-relay-session-recovery-durability.test.ts @@ -1,4 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR, + PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR +} from '../../shared/pty-consumer-session' import { SshRelaySession } from './ssh-relay-session' import { createMismatchedOwnerRecoveryError, @@ -154,6 +158,82 @@ describe('SshRelaySession consumer recovery durability', () => { session.dispose() }) + it('retries a pending incumbent publication before recovering its persisted lease', async () => { + vi.useFakeTimers() + try { + const targetId = 'target-owner-publication-pending' + const deps = createMockDeps() + vi.mocked(deps.mockStore.getSshPtyConsumerRecovery).mockReturnValue({ + targetId, + clientInstanceId: 'persisted-client', + serverBuildId: 'test-relay-build', + clientGeneration: 1, + ownerGeneration: 1, + ownerLease: 'persisted-owner' + }) + openConsumerSessionMock.mockRejectedValueOnce( + Object.assign(new Error('Owner grant publication is still pending'), { + code: PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR + }) + ) + const session = new SshRelaySession( + targetId, + deps.getMainWindow, + deps.mockStore, + deps.mockPortForward + ) + + const establishing = session.establish(deps.mockConn) + await vi.advanceTimersByTimeAsync(25) + await establishing + + expect(openConsumerSessionMock).toHaveBeenCalledTimes(2) + expect(openConsumerSessionMock.mock.calls[0]?.[1]).toMatchObject({ + clientInstanceId: 'persisted-client', + resume: { ownerGeneration: 1, ownerLease: 'persisted-owner' } + }) + session.dispose() + } finally { + vi.useRealTimers() + } + }) + + it('preserves persisted recovery while a superseding transport is still live', async () => { + vi.useFakeTimers() + try { + const targetId = 'target-owner-generation-superseded' + const deps = createMockDeps() + vi.mocked(deps.mockStore.getSshPtyConsumerRecovery).mockReturnValue({ + targetId, + clientInstanceId: 'persisted-client', + serverBuildId: 'test-relay-build', + clientGeneration: 1, + ownerGeneration: 1, + ownerLease: 'persisted-owner' + }) + const superseded = Object.assign(new Error('Owner recovery generation was superseded'), { + code: PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR + }) + openConsumerSessionMock.mockRejectedValue(superseded) + const session = new SshRelaySession( + targetId, + deps.getMainWindow, + deps.mockStore, + deps.mockPortForward + ) + + const failed = expect(session.establish(deps.mockConn)).rejects.toBe(superseded) + await vi.advanceTimersByTimeAsync(3_000) + await failed + + expect(openConsumerSessionMock.mock.calls.length).toBeGreaterThan(1) + expect(deps.mockStore.removeSshPtyConsumerRecovery).not.toHaveBeenCalled() + session.dispose() + } finally { + vi.useRealTimers() + } + }) + it('keeps destructive disposal pending until consumer recovery is removed', async () => { const { mockStore, mockPortForward, getMainWindow } = createMockDeps() vi.mocked(mockStore.getSshPtyConsumerRecovery).mockReturnValue({ @@ -232,6 +312,46 @@ describe('SshRelaySession consumer recovery durability', () => { expect(openConsumerSessionMock).toHaveBeenCalledTimes(1) }) + it('leaves recovery state alone when a newer owner already claimed the target record', async () => { + const targetId = 'target-stale-owner-loser' + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + vi.mocked(mockStore.getSshPtyConsumerRecovery).mockReturnValue({ + targetId, + clientInstanceId: 'persisted-client', + serverBuildId: 'test-relay-build', + clientGeneration: 1, + ownerGeneration: 1, + ownerLease: 'stale-owner' + }) + const winner = { + mode: 'negotiated' as const, + clientInstanceId: 'persisted-client', + clientGeneration: 2, + ownerGeneration: 5, + ownerLease: 'winner-owner' + } + openConsumerSessionMock.mockImplementationOnce(() => { + // Why inside the rejection: the record is target-scoped, so the winner can land while this + // attempt is still unwinding its own resume. + const record = getSshPtyConsumerRecovery(targetId)! + record.owner = winner + record.checkpointsByAppPtyId.set('pty-1', { + id: 'pty-1' + } as unknown as never) + return Promise.reject(createMismatchedOwnerRecoveryError()) + }) + openConsumerSessionMock.mockRejectedValueOnce(new Error('fresh open failed')) + const session = new SshRelaySession(targetId, getMainWindow, mockStore, mockPortForward) + + await expect(session.establish(mockConn)).rejects.toThrow('fresh open failed') + + const record = getSshPtyConsumerRecovery(targetId)! + expect(record.owner).toBe(winner) + expect(record.checkpointsByAppPtyId.has('pty-1')).toBe(true) + expect(mockStore.removeSshPtyConsumerRecovery).not.toHaveBeenCalled() + session.dispose() + }) + it('does not remember a consumer opened after establish was disposed', async () => { const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() let signalOpenStarted!: () => void diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index c8440f6c239..3f1c5105d29 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -81,6 +81,7 @@ import type { Store } from '../persistence' import type { OrcaRuntimeService } from '../runtime/orca-runtime' import { DEFAULT_PTY_SOURCE_WINDOW_SU } from '../../shared/pty-source-credit-contract' import { PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR } from '../../shared/pty-consumer-session' +import { retrySshOwnerRecoveryWhileBlocked } from './ssh-owner-recovery-retry' import { runRemoteOrcaCli } from './ssh-remote-orca-cli' import { acknowledgeRemoteOrcaCliPostOutput, @@ -1027,17 +1028,24 @@ export class SshRelaySession { outputFlowControl: { requestedWindowSu: DEFAULT_PTY_SOURCE_WINDOW_SU } } try { - return await openSshPtyConsumerSession(mux, { - ...options, - ...(previousOwner - ? { - resume: { - ownerGeneration: previousOwner.ownerGeneration, - ownerLease: previousOwner.ownerLease - } - } - : {}) - }) + return await retrySshOwnerRecoveryWhileBlocked( + () => + openSshPtyConsumerSession(mux, { + ...options, + ...(previousOwner + ? { + resume: { + ownerGeneration: previousOwner.ownerGeneration, + ownerLease: previousOwner.ownerLease + } + } + : {}) + }), + { + isCurrent: () => ownsAttempt() && !mux.isDisposed(), + onClosed: (listener) => mux.onDispose(listener) + } + ) } catch (error) { if ( !previousOwner || @@ -1046,26 +1054,36 @@ export class SshRelaySession { throw error } const recovery = getSshPtyConsumerRecovery(this.targetId) - if (recovery) { - delete recovery.owner - recovery.checkpointsByAppPtyId.clear() - for (const [ptyId, migration] of recovery.modelMigrationsByAppPtyId) { - recovery.modelMigrationsByAppPtyId.set( - ptyId, - migration.then(() => - Object.freeze({ - status: 'checkpoint-unavailable' as const, - reason: 'completion-failed' as const - }) + // Why identity-guarded: the record is target-scoped and its clientInstanceId is shared by every + // session for that target, so only a record still describing the owner this attempt tried to + // resume is ours to drop — otherwise a loser wipes the winner's checkpoints. + const ownsRecoveryRecord = + ownsAttempt() && + (!recovery?.owner || + (recovery.owner.ownerGeneration === previousOwner.ownerGeneration && + recovery.owner.ownerLease === previousOwner.ownerLease)) + if (ownsRecoveryRecord) { + if (recovery) { + delete recovery.owner + recovery.checkpointsByAppPtyId.clear() + for (const [ptyId, migration] of recovery.modelMigrationsByAppPtyId) { + recovery.modelMigrationsByAppPtyId.set( + ptyId, + migration.then(() => + Object.freeze({ + status: 'checkpoint-unavailable' as const, + reason: 'completion-failed' as const + }) + ) ) - ) + } } + await removeSshPtyConsumerOwnerRecovery( + this.targetId, + this.ptyConsumerClientInstanceId, + this.store + ) } - await removeSshPtyConsumerOwnerRecovery( - this.targetId, - this.ptyConsumerClientInstanceId, - this.store - ) if (!ownsAttempt()) { throw new Error('Session disposed during establish') } diff --git a/src/main/updater.install-failure-cause.test.ts b/src/main/updater.install-failure-cause.test.ts new file mode 100644 index 00000000000..5e61ac19554 --- /dev/null +++ b/src/main/updater.install-failure-cause.test.ts @@ -0,0 +1,333 @@ +import os from 'node:os' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as TracerModule from './observability/tracer' +import type * as UpdaterModule from './updater' + +const { + appMock, + browserWindowMock, + nativeUpdaterMock, + autoUpdaterMock, + isMock, + killAllPtyMock, + recordUpdaterLifecycleMock +} = vi.hoisted(() => { + const appEventHandlers = new Map void)[]>() + const eventHandlers = new Map void)[]>() + + const appOn = vi.fn((event: string, handler: (...args: unknown[]) => void) => { + const handlers = appEventHandlers.get(event) ?? [] + handlers.push(handler) + appEventHandlers.set(event, handlers) + return appMock + }) + + const on = vi.fn((event: string, handler: (...args: unknown[]) => void) => { + const handlers = eventHandlers.get(event) ?? [] + handlers.push(handler) + eventHandlers.set(event, handlers) + return autoUpdaterMock + }) + + const emit = (event: string, ...args: unknown[]) => { + for (const handler of eventHandlers.get(event) ?? []) { + handler(...args) + } + } + + const reset = () => { + appEventHandlers.clear() + appOn.mockClear() + eventHandlers.clear() + on.mockClear() + autoUpdaterMock.checkForUpdates.mockReset() + autoUpdaterMock.downloadUpdate.mockReset() + autoUpdaterMock.quitAndInstall.mockReset() + autoUpdaterMock.setFeedURL.mockClear() + } + + const autoUpdaterMock = { + autoDownload: false, + autoInstallOnAppQuit: false, + on, + checkForUpdates: vi.fn(), + downloadUpdate: vi.fn(), + quitAndInstall: vi.fn(), + setFeedURL: vi.fn(), + emit, + reset + } + + return { + appMock: { + isPackaged: true, + getVersion: vi.fn(() => '1.4.162'), + on: appOn, + quit: vi.fn(), + exit: vi.fn() + }, + browserWindowMock: { getAllWindows: vi.fn(() => []) }, + nativeUpdaterMock: { on: vi.fn() }, + autoUpdaterMock, + isMock: { dev: false }, + killAllPtyMock: vi.fn(), + recordUpdaterLifecycleMock: vi.fn() + } +}) + +vi.mock('electron', () => ({ + app: appMock, + BrowserWindow: browserWindowMock, + autoUpdater: nativeUpdaterMock, + powerMonitor: { on: vi.fn() }, + shell: { openExternal: vi.fn() }, + net: { fetch: vi.fn() } +})) + +vi.mock('electron-updater', () => ({ autoUpdater: autoUpdaterMock })) +vi.mock('./electron-updater-loader', () => ({ + loadElectronAutoUpdater: () => autoUpdaterMock +})) +vi.mock('@electron-toolkit/utils', () => ({ is: isMock })) +vi.mock('./ipc/pty', () => ({ killAllPty: killAllPtyMock })) +vi.mock('./updater-changelog', () => ({ + fetchChangelog: vi.fn().mockResolvedValue(null) +})) +vi.mock('./updater-nudge', () => ({ + fetchNudge: vi.fn().mockResolvedValue(null), + shouldApplyNudge: vi.fn().mockReturnValue(false) +})) +vi.mock('./updater-lifecycle-diagnostics', () => ({ + recordUpdaterLifecycle: recordUpdaterLifecycleMock +})) + +// The real electron-updater DebUpdater failure text when elevation is impossible. +const DEB_ELEVATION_ERROR = + 'Error: Command failed: /usr/bin/pkexec --disable-internal-agent "/bin/bash" "-c" "dpkg -i \'/home/u/.cache/orca-updater/pending/orca-ide_1.4.163_amd64.deb\'"\npkexec must be setuid root' + +// electron-updater's ERR_UPDATER_INVALID_SIGNATURE text, which drives its own card in UpdateCard. +const WINDOWS_SIGNATURE_MISMATCH_ERROR = + 'New version 1.4.163 is not signed by the application owner: publisherNames: Orca, Inc.' + +type CapturedSpan = { + readonly name: string + readonly exit: { readonly _tag: string; readonly cause?: string } +} + +const originalPlatform = process.platform + +let spans: CapturedSpan[] +let tracer: typeof TracerModule | null = null + +function capturingSink(): TracerModule.TracerSink { + return { + push(record) { + spans.push(record as CapturedSpan) + }, + flush() { + /* no-op */ + }, + close() { + /* no-op */ + } + } +} + +function installSpan(): CapturedSpan | undefined { + return spans.find((span) => span.name === 'updater.install') +} + +/** Drives the updater to `downloaded`, the only state `quitAndInstall` acts on. */ +async function reachDownloaded(): Promise { + const mainWindow = { webContents: { send: vi.fn() } } + autoUpdaterMock.checkForUpdates.mockResolvedValue(undefined) + // Why: resetModules gives each test a fresh module graph, so the sink must be installed on the + // same tracer instance updater.ts will import. + tracer = await import('./observability/tracer') + tracer.setActiveSink(capturingSink()) + const updater = await import('./updater') + + updater.setupAutoUpdater(mainWindow as never) + await vi.waitFor(() => { + expect(autoUpdaterMock.checkForUpdates).toHaveBeenCalledTimes(1) + }) + autoUpdaterMock.emit('checking-for-update') + autoUpdaterMock.emit('update-available', { version: '1.4.163' }) + await new Promise((resolve) => setTimeout(resolve, 0)) + autoUpdaterMock.emit('update-downloaded', { version: '1.4.163' }) + expect(updater.getUpdateStatus().state).toBe('downloaded') + return updater +} + +/** + * On a `.deb` Linux host electron-updater's `install()` catches the failed elevation and + * re-dispatches it through the 'error' event *synchronously* inside `quitAndInstall()`. Orca + * recovers the app state, so the payload has to survive on the status and the span has to exit + * Failure — otherwise the only record of why the install never ran is destroyed (#11906). + */ +describe('quitAndInstall failure carries the updater cause', () => { + beforeEach(() => { + vi.resetModules() + autoUpdaterMock.reset() + nativeUpdaterMock.on.mockReset() + browserWindowMock.getAllWindows.mockReset() + browserWindowMock.getAllWindows.mockReturnValue([]) + appMock.getVersion.mockReset() + appMock.getVersion.mockReturnValue('1.4.162') + appMock.quit.mockReset() + appMock.isPackaged = true + isMock.dev = false + killAllPtyMock.mockReset() + recordUpdaterLifecycleMock.mockReset() + Object.defineProperty(process, 'platform', { + value: 'linux', + configurable: true + }) + vi.useRealTimers() + spans = [] + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { + value: originalPlatform, + configurable: true + }) + tracer?._resetTracerForTests() + tracer = null + }) + + it('reports the underlying elevation failure rather than a generic restart message', async () => { + const { quitAndInstall, getUpdateStatus } = await reachDownloaded() + + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + autoUpdaterMock.emit('error', new Error(DEB_ELEVATION_ERROR)) + }) + + quitAndInstall() + await vi.waitFor(() => { + expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledTimes(1) + }) + + const status = getUpdateStatus() + expect(status.state).toBe('error') + // The only place the real cause exists is this event payload; dropping it leaves nothing + // in the logs and nothing actionable in the client's remote-server update dialog. + const message = status.state === 'error' ? status.message : '' + expect(message).toContain('pkexec') + expect(message).toContain('Could not start the update installer.') + }) + + it('keeps the durable breadcrumb to classification only', async () => { + const { quitAndInstall } = await reachDownloaded() + + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + autoUpdaterMock.emit('error', new Error(DEB_ELEVATION_ERROR)) + }) + + quitAndInstall() + await vi.waitFor(() => { + expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledTimes(1) + }) + + expect(recordUpdaterLifecycleMock).toHaveBeenCalledWith( + 'quit_and_install_failed_via_event', + { errorType: 'Error' }, + expect.anything() + ) + }) + + it('exits the install span Failure with the cause', async () => { + const { quitAndInstall } = await reachDownloaded() + + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + autoUpdaterMock.emit('error', new Error(DEB_ELEVATION_ERROR)) + }) + + quitAndInstall() + await vi.waitFor(() => { + expect(installSpan()).toBeDefined() + }) + + expect(installSpan()?.exit._tag).toBe('Failure') + expect(installSpan()?.exit.cause).toContain('pkexec') + }) + + it('exits the install span Success when the installer takes over', async () => { + const { quitAndInstall } = await reachDownloaded() + + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + // The installer runs and the old process is left to exit; no 'error' comes back. + }) + + quitAndInstall() + await vi.waitFor(() => { + expect(installSpan()).toBeDefined() + }) + + expect(installSpan()?.exit._tag).toBe('Success') + }) + + it('keeps a Windows signature verdict unprefixed so its own card still renders', async () => { + Object.defineProperty(process, 'platform', { + value: 'win32', + configurable: true + }) + const { quitAndInstall, getUpdateStatus } = await reachDownloaded() + + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + autoUpdaterMock.emit('error', new Error(WINDOWS_SIGNATURE_MISMATCH_ERROR)) + }) + + quitAndInstall() + await vi.waitFor(() => { + expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledTimes(1) + }) + + const status = getUpdateStatus() + // Prefixing this would put two contradictory instructions on one card. + expect(status.state === 'error' ? status.message : '').toBe(WINDOWS_SIGNATURE_MISMATCH_ERROR) + }) + + it('redacts the cause the same way the retained-package card does', async () => { + const { quitAndInstall, getUpdateStatus } = await reachDownloaded() + const home = os.homedir() + const escape = String.fromCharCode(27) + + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + autoUpdaterMock.emit( + 'error', + new Error(`${escape}[31mdpkg -i '${home}/.cache/orca-updater/pending/orca.deb'${escape}[0m`) + ) + }) + + quitAndInstall() + await vi.waitFor(() => { + expect(autoUpdaterMock.quitAndInstall).toHaveBeenCalledTimes(1) + }) + + const status = getUpdateStatus() + const message = status.state === 'error' ? status.message : '' + expect(message).not.toContain(home) + expect(message).not.toContain(escape) + expect(message).toContain("/.cache/orca-updater/pending/orca.deb'") + }) + + it('carries the cause when quitAndInstall throws instead of dispatching an error event', async () => { + const { quitAndInstall, getUpdateStatus } = await reachDownloaded() + + // Squirrel/NSIS can reject the request by throwing out of the native call; the event path never runs. + autoUpdaterMock.quitAndInstall.mockImplementation(() => { + throw new Error('Squirrel.framework is missing from the app bundle') + }) + + quitAndInstall() + await vi.waitFor(() => { + expect(getUpdateStatus().state).toBe('error') + }) + + const status = getUpdateStatus() + const message = status.state === 'error' ? status.message : '' + expect(message).toContain('Squirrel.framework is missing from the app bundle') + expect(message).toContain('Could not start the update installer.') + }) +}) diff --git a/src/main/updater.test.ts b/src/main/updater.test.ts index a0ab479343d..dce8b39a7b6 100644 --- a/src/main/updater.test.ts +++ b/src/main/updater.test.ts @@ -159,6 +159,12 @@ const { getLinuxRootPackageTypeMock, recordUpdaterLifecycleMock } = vi.hoisted(( recordUpdaterLifecycleMock: vi.fn() })) +// Why: macOS keeps the restart advice because quitting does re-stage a Squirrel update. +const PRE_COMMIT_INSTALL_FAILURE = + process.platform === 'darwin' + ? 'Could not restart to install the update. Quit and reopen Orca, then try again.' + : 'Could not start the update installer. Orca remains open.' + // Why: only the marker resolver is faked so the real artifact capture/redaction path stays under test. vi.mock('./linux-update-package-type', () => ({ getLinuxRootPackageType: getLinuxRootPackageTypeMock @@ -1829,10 +1835,8 @@ describe('updater', () => { state: 'error', // Why: a pre-commit install failure is not fixed by restarting, so the copy must not // suggest it — except on macOS, where quitting does re-stage a Squirrel update. - message: - process.platform === 'darwin' - ? 'Could not restart to install the update. Quit and reopen Orca, then try again.' - : 'Could not start the update installer. Orca remains open.' + // The updater's own text is appended because it is the only record of why the install never ran. + message: `${PRE_COMMIT_INSTALL_FAILURE} (No update filepath provided, can't quit and install)` }) ) }) @@ -3911,11 +3915,7 @@ describe('updater', () => { const lastStatus = (send: ReturnType): UpdateStatus | undefined => send.mock.calls.findLast(([channel]) => channel === 'updater:status')?.[1] - // Why: macOS keeps the restart advice because quitting does re-stage a Squirrel update. - const PRE_COMMIT_FAILURE_MESSAGE = - process.platform === 'darwin' - ? 'Could not restart to install the update. Quit and reopen Orca, then try again.' - : 'Could not start the update installer. Orca remains open.' + const PRE_COMMIT_FAILURE_MESSAGE = PRE_COMMIT_INSTALL_FAILURE const AGENT_STDERR = 'pkexec: Error executing command as another user: No authentication agent found.' @@ -4081,7 +4081,7 @@ describe('updater', () => { expect(send).toHaveBeenCalledWith('updater:status', { state: 'error', - message: PRE_COMMIT_FAILURE_MESSAGE + message: `${PRE_COMMIT_FAILURE_MESSAGE} (${EXIT_127})` }) expect(recordUpdaterLifecycleMock).not.toHaveBeenCalledWith( 'linux_package_install_failed', diff --git a/src/main/updater.ts b/src/main/updater.ts index 302591286f3..96bf163e54e 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -13,7 +13,10 @@ import type { RemoteServerUpdaterSnapshot, RemoteServerUpdateSupport } from '../shared/remote-server-update' -import { isWindowsSignatureCheckUnavailableFailure } from '../shared/updater-windows-signature-check' +import { + isWindowsSignatureCheckUnavailableFailure, + isWindowsSignatureMismatchFailure +} from '../shared/updater-windows-signature-check' import { killAllPty } from './ipc/pty' import { withUpdaterSpan } from './observability/instrumentation' import { loadElectronAutoUpdater, type ElectronAutoUpdater } from './electron-updater-loader' @@ -763,6 +766,8 @@ async function performQuitAndInstall(): Promise { true ) resetQuitForUpdateState() + // Why: a bare return would exit this span Success and hide the aborted install from tracing. + span.fail('Could not persist the supervised serve update handoff') return } @@ -790,6 +795,12 @@ async function performQuitAndInstall(): Promise { // Why: quitAndInstall can synchronously clear quitAndInstallInProgress via recovery (Win/Linux dispatchError); skip destructive prep if it already ran. if (!quitAndInstallInProgress) { + // Why: recovery already wrote the reason to currentStatus; a bare return would exit this span Success. + span.fail( + currentStatus.state === 'error' + ? currentStatus.message + : 'quitAndInstall returned without invoking the installer' + ) return } @@ -853,8 +864,9 @@ async function performQuitAndInstall(): Promise { recoveryStatus ?? { state: 'error', // Why: past the native invoke this is the same pre-commit failure the event path reports, so it gets the same copy; only a pre-native exception can be helped by a restart. + // A synchronous throw out of quitAndInstall carries the same installer text the 'error' event would have. message: quitAndInstallNativeInvokedBeforeReset - ? getPreCommitInstallFailureMessage() + ? withInstallFailureCause(getPreCommitInstallFailureMessage(), error) : 'Could not restart to install the update. Quit and reopen Orca, then try again.' } ) @@ -890,6 +902,33 @@ function sendInstallFailureStatus(status: UpdateStatus): void { sendStatus(status, { force: true }) } +const INSTALL_FAILURE_CAUSE_MAX_LENGTH = 200 + +/** + * Appends the updater's own text to the generic install-failure copy. Without it the only record of + * why the install never started is destroyed — on Linux that text carries the exact `dpkg -i ` + * command the user has to run by hand, and remote clients get nothing but "it didn't come back". + */ +function withInstallFailureCause(baseMessage: string, error: unknown): string { + const raw = error instanceof Error ? error.message : typeof error === 'string' ? error : '' + // Why: the retained-package card runs its text through this same sanitizer, so a home directory, + // user name, or terminal escape must not reach the card merely because no artifact was tracked. + const redacted = + redactLinuxPackageInstallText(raw, getTrackedLinuxPackageArtifact()?.path ?? null) ?? '' + const cause = redacted.slice(0, INSTALL_FAILURE_CAUSE_MAX_LENGTH) + if (!cause || cause === 'Unknown error') { + return baseMessage + } + // Why: UpdateCard picks the whole card off this string, so a signature verdict must not be prefixed by contradictory restart advice. + if ( + isWindowsSignatureCheckUnavailableFailure(cause) || + isWindowsSignatureMismatchFailure(cause) + ) { + return cause + } + return `${baseMessage} (${cause})` +} + /** * The recovery status for a failed `.deb`/`.rpm` install, or null when no retained package can * recover it. Must run before `resetQuitForUpdateState()` clears the attempt diagnostic. @@ -945,14 +984,19 @@ function handleQuitAndInstallFailure(error?: unknown): boolean { const recoveryStatus = buildLinuxPackageInstallFailureStatus(error) failServeUpdateHandoff('The native updater rejected the install request.') resetQuitForUpdateState() - recordUpdaterLifecycle('quit_and_install_failed_via_event', undefined, { - level: 'warn', - message: 'Update install could not start; recovered app state' - }) + // Durable data carries classification only — the cause text stays on the status the user can read. + recordUpdaterLifecycle( + 'quit_and_install_failed_via_event', + { errorType: error instanceof Error ? error.name : typeof error }, + { + level: 'warn', + message: 'Update install could not start; recovered app state' + } + ) sendInstallFailureStatus( recoveryStatus ?? { state: 'error', - message: getPreCommitInstallFailureMessage() + message: withInstallFailureCause(getPreCommitInstallFailureMessage(), error) } ) return true diff --git a/src/relay/dispatcher.ts b/src/relay/dispatcher.ts index 98e8959547f..d89d0795ed9 100644 --- a/src/relay/dispatcher.ts +++ b/src/relay/dispatcher.ts @@ -161,6 +161,16 @@ export class RelayDispatcher { this.closeClient(client, new Error('Relay client detached'), true) } + // Why: a displaced owner must lose its transport whichever client holds it, and the launch channel is + // often the half-open one. The primary keeps its id for setWrite() revival, so invalidate it instead. + releaseDisplacedClient(clientId: number): void { + if (clientId === this.primaryClient.id) { + this.invalidateClient() + return + } + this.detachClient(clientId) + } + feedClient(clientId: number, data: Buffer): void { const client = this.clients.get(clientId) if (!client) { diff --git a/src/relay/relay-pty-consumer-owner-displacement.test.ts b/src/relay/relay-pty-consumer-owner-displacement.test.ts new file mode 100644 index 00000000000..89059119f5f --- /dev/null +++ b/src/relay/relay-pty-consumer-owner-displacement.test.ts @@ -0,0 +1,368 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + RelayDispatcher, + type RelayClientSessionIdentity, + type SinkWriteSettlement +} from './dispatcher' +import { encodeJsonRpcFrame, MessageType } from './protocol' +import { SshPtyConsumerSessionAdapter } from './ssh-pty-consumer-session-adapter' +import { + PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR, + PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR +} from '../shared/pty-consumer-session' + +const endpointIdentity: RelayClientSessionIdentity = { + principal: 'endpoint-principal', + authenticated: true, + allowSessionOwner: true, + authenticationKind: 'endpoint-credential' +} + +function requestFrame(id: number, method: string, params: Record): Buffer { + return encodeJsonRpcFrame({ jsonrpc: '2.0', id, method, params }, id, 0) +} + +function message(buffer: Buffer): Record | null { + if (buffer[0] !== MessageType.Regular) { + return null + } + const length = buffer.readUInt32BE(9) + return JSON.parse(buffer.subarray(13, 13 + length).toString('utf8')) +} + +function response(writes: readonly Buffer[], id: number): Record | undefined { + return writes.map(message).find((entry) => entry?.id === id) as + | Record + | undefined +} + +function ownerHelloParams(resume?: Record): Record { + return { + protocolVersion: 1, + clientInstanceId: 'client-1', + requestedRole: 'session-owner', + capabilities: { outputFlowControl: { versions: [1], requestedWindowSu: 8 } }, + ...(resume ? { resume } : {}) + } +} + +async function flushRequests(): Promise { + await new Promise((resolve) => setImmediate(resolve)) +} + +describe('relay PTY consumer owner displacement', () => { + let dispatcher: RelayDispatcher | null = null + + afterEach(() => { + dispatcher?.dispose() + dispatcher = null + }) + + it('displaces a half-open owner and retains its deliveries for the reconnected client', async () => { + // Why no settle deferral: a half-open peer is indistinguishable from a live one at the sink — writes + // still "succeed" locally, so the relay never learns the owner is gone. + const staleWrites: Buffer[] = [] + const closeStaleTransport = vi.fn() + dispatcher = new RelayDispatcher( + (data, settle) => { + staleWrites.push(Buffer.from(data)) + settle({ ok: true }) + return true + }, + { supportsWriteCallback: true, close: closeStaleTransport }, + endpointIdentity + ) + const detached: number[] = [] + dispatcher.onClientDetached((clientId) => detached.push(clientId)) + const adapter = new SshPtyConsumerSessionAdapter(dispatcher, 'build-a') + + dispatcher.feed(requestFrame(1, 'pty.openClient', ownerHelloParams())) + await flushRequests() + const staleGrant = response(staleWrites, 1)!.result as Record + expect(adapter.deliveryMode(1)).toBe('source-owner') + + const staleDelivery = adapter.openDelivery(1, 'pty-1', 'incarnation-1')! + adapter.appendSource(staleDelivery, { + spanId: 'span-1', + data: 'tail', + displayStart: 0, + displayEnd: 4, + splittable: true, + transform: { transformed: false, rawLengthSu: 4, scalarSafe: true } + }) + + const reconnectWrites: Buffer[] = [] + const reconnectClientId = dispatcher.attachClient( + (data, settle) => { + reconnectWrites.push(Buffer.from(data)) + settle({ ok: true }) + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + dispatcher.feedClient( + reconnectClientId, + requestFrame( + 2, + 'pty.openClient', + ownerHelloParams({ + ownerGeneration: staleGrant.ownerGeneration, + ownerLease: staleGrant.ownerLease + }) + ) + ) + await flushRequests() + + expect(response(reconnectWrites, 2)!.result).toMatchObject({ + role: 'session-owner', + ownerGeneration: 2 + }) + expect(adapter.deliveryMode(reconnectClientId)).toBe('source-owner') + // Why: revocation is what bounds the takeover — the stale owner cannot open or drive deliveries again + // even though its socket never closed. + expect(adapter.deliveryMode(1)).toBe('subscriber') + expect(adapter.openDelivery(1, 'pty-2', 'incarnation-1')).toBeNull() + expect(detached).toContain(1) + expect(closeStaleTransport).toHaveBeenCalledOnce() + + // Why: retained, not closed — the reconnected owner still has to rotate the stale delivery forward. + expect(adapter.getDebugSnapshot()).toMatchObject({ deliveryTokens: 1, graceTimers: 1 }) + const rotation = adapter.rotateDelivery(staleDelivery, reconnectClientId, 0) + expect(rotation.identity).toMatchObject({ ownerGeneration: 2, clientGeneration: 2 }) + expect(rotation.recovery.map((span) => span.data)).toEqual(['tail']) + }) + + it('retains the incumbent delivery once when its socket closes mid-publication', async () => { + const incumbentWrites: Buffer[] = [] + dispatcher = new RelayDispatcher( + (data, settle) => { + incumbentWrites.push(Buffer.from(data)) + settle({ ok: true }) + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + const adapter = new SshPtyConsumerSessionAdapter(dispatcher, 'build-a') + + dispatcher.feed(requestFrame(1, 'pty.openClient', ownerHelloParams())) + await flushRequests() + const incumbentGrant = response(incumbentWrites, 1)!.result as Record + const incumbentDelivery = adapter.openDelivery(1, 'pty-1', 'incarnation-1')! + adapter.appendSource(incumbentDelivery, { + spanId: 'span-1', + data: 'tail', + displayStart: 0, + displayEnd: 4, + splittable: true, + transform: { transformed: false, rawLengthSu: 4, scalarSafe: true } + }) + + const reconnectWrites: Buffer[] = [] + let grantSettlement: ((result: SinkWriteSettlement) => void) | undefined + const reconnectClientId = dispatcher.attachClient( + (data, settle) => { + reconnectWrites.push(Buffer.from(data)) + grantSettlement = settle + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + dispatcher.feedClient( + reconnectClientId, + requestFrame( + 2, + 'pty.openClient', + ownerHelloParams({ + ownerGeneration: incumbentGrant.ownerGeneration, + ownerLease: incumbentGrant.ownerLease + }) + ) + ) + await flushRequests() + + // Why this ordering: the incumbent's socket close and the replacement's grant write are independent + // events, so detach-then-commit has to leave exactly one retention, not two competing ones. + dispatcher.invalidateClient() + grantSettlement!({ ok: true }) + + expect(adapter.deliveryMode(reconnectClientId)).toBe('source-owner') + expect(adapter.getDebugSnapshot()).toMatchObject({ deliveryTokens: 1, graceTimers: 1 }) + const rotation = adapter.rotateDelivery(incumbentDelivery, reconnectClientId, 0) + expect(rotation.identity).toMatchObject({ ownerGeneration: 2, clientGeneration: 2 }) + expect(adapter.getDebugSnapshot()).toMatchObject({ graceTimers: 0 }) + }) + + it('keeps the incumbent owner when the replacement grant fails to publish', async () => { + const incumbentWrites: Buffer[] = [] + dispatcher = new RelayDispatcher( + (data, settle) => { + incumbentWrites.push(Buffer.from(data)) + settle({ ok: true }) + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + const detached: number[] = [] + dispatcher.onClientDetached((clientId) => detached.push(clientId)) + const adapter = new SshPtyConsumerSessionAdapter(dispatcher, 'build-a') + + dispatcher.feed(requestFrame(1, 'pty.openClient', ownerHelloParams())) + await flushRequests() + const incumbentGrant = response(incumbentWrites, 1)!.result as Record + + const reconnectWrites: Buffer[] = [] + let grantSettlement: ((result: SinkWriteSettlement) => void) | undefined + const reconnectClientId = dispatcher.attachClient( + (data, settle) => { + reconnectWrites.push(Buffer.from(data)) + // Why: the takeover must not commit until the replacement grant is on the wire. + grantSettlement = settle + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + dispatcher.feedClient( + reconnectClientId, + requestFrame( + 2, + 'pty.openClient', + ownerHelloParams({ + ownerGeneration: incumbentGrant.ownerGeneration, + ownerLease: incumbentGrant.ownerLease + }) + ) + ) + await flushRequests() + + expect(adapter.deliveryMode(1)).toBe('source-owner') + expect(detached).not.toContain(1) + + grantSettlement!({ ok: false, error: new Error('reconnect socket closed mid-response') }) + expect(adapter.deliveryMode(1)).toBe('source-owner') + expect(adapter.deliveryMode(reconnectClientId)).toBe('subscriber') + + // Why: the rolled-back attempt must leave the incumbent's lease reclaimable by the next reconnect. + const retryWrites: Buffer[] = [] + const retryClientId = dispatcher.attachClient( + (data, settle) => { + retryWrites.push(Buffer.from(data)) + settle({ ok: true }) + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + dispatcher.feedClient( + retryClientId, + requestFrame( + 3, + 'pty.openClient', + ownerHelloParams({ + ownerGeneration: incumbentGrant.ownerGeneration, + ownerLease: incumbentGrant.ownerLease + }) + ) + ) + await flushRequests() + + // Why generation 3: the rolled-back attempt consumed generation 2, but not the incumbent's lease. + expect(response(retryWrites, 3)!.result).toMatchObject({ + role: 'session-owner', + ownerGeneration: 3 + }) + expect(adapter.deliveryMode(1)).toBe('subscriber') + expect(detached).toContain(1) + }) + + it('fences an old proof while its replacement is live', async () => { + const incumbentWrites: Buffer[] = [] + dispatcher = new RelayDispatcher( + (data, settle) => { + incumbentWrites.push(Buffer.from(data)) + settle({ ok: true }) + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + const adapter = new SshPtyConsumerSessionAdapter(dispatcher, 'build-a') + + dispatcher.feed(requestFrame(1, 'pty.openClient', ownerHelloParams())) + await flushRequests() + const incumbentGrant = response(incumbentWrites, 1)!.result as Record + const resume = { + ownerGeneration: incumbentGrant.ownerGeneration, + ownerLease: incumbentGrant.ownerLease + } + + let firstReconnectSettlement: ((result: SinkWriteSettlement) => void) | undefined + const firstReconnectWrites: Buffer[] = [] + const firstReconnectClientId = dispatcher.attachClient( + (data, settle) => { + firstReconnectWrites.push(Buffer.from(data)) + // Why ??=: the test settles the grant response, not whatever frame the dispatcher wrote last. + firstReconnectSettlement ??= settle + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + dispatcher.feedClient( + firstReconnectClientId, + requestFrame(2, 'pty.openClient', ownerHelloParams(resume)) + ) + await flushRequests() + + const retryWrites: Buffer[] = [] + const retryClientId = dispatcher.attachClient( + (data, settle) => { + retryWrites.push(Buffer.from(data)) + settle({ ok: true }) + return true + }, + { supportsWriteCallback: true }, + endpointIdentity + ) + dispatcher.feedClient( + retryClientId, + requestFrame(3, 'pty.openClient', ownerHelloParams(resume)) + ) + await flushRequests() + + expect(response(retryWrites, 3)!.error).toMatchObject({ + code: PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR + }) + + firstReconnectSettlement!({ ok: true }) + dispatcher.feedClient( + retryClientId, + requestFrame(4, 'pty.openClient', ownerHelloParams(resume)) + ) + await flushRequests() + + expect(response(retryWrites, 4)!.error).toMatchObject({ + code: PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR + }) + expect(adapter.deliveryMode(firstReconnectClientId)).toBe('source-owner') + expect(adapter.deliveryMode(retryClientId)).toBe('subscriber') + + dispatcher.detachClient(firstReconnectClientId) + dispatcher.feedClient( + retryClientId, + requestFrame(5, 'pty.openClient', ownerHelloParams(resume)) + ) + await flushRequests() + + expect(response(retryWrites, 5)!.result).toMatchObject({ + role: 'session-owner', + ownerGeneration: 3, + ownerLease: incumbentGrant.ownerLease + }) + expect(adapter.deliveryMode(retryClientId)).toBe('source-owner') + }) +}) diff --git a/src/relay/relay.ts b/src/relay/relay.ts index d5003bdf649..4169c4fc449 100644 --- a/src/relay/relay.ts +++ b/src/relay/relay.ts @@ -10,7 +10,15 @@ import { createServer, createConnection, type Socket, type Server } from 'node:net' import { join } from 'node:path' -import { unlinkSync, existsSync, statSync, readFileSync, chmodSync } from 'node:fs' +import { + unlinkSync, + existsSync, + statSync, + readFileSync, + chmodSync, + closeSync, + openSync +} from 'node:fs' import { RELAY_SENTINEL, FrameDecoder, @@ -597,6 +605,31 @@ async function main(): Promise { } stdoutDrainWaiters.add(cb) return () => stdoutDrainWaiters.delete(cb) + }, + close: () => { + stdoutAlive = false + flushStdoutDrainWaiters() + // Why close then re-pin: the SSH peer must see EOF, but a long-lived daemon that + // frees fds 0/1 lets accept()/open() recycle them while Node still treats + // process.stdin/stdout as those numbers — corrupting socket clients and shutdown. + for (const fd of [process.stdin.fd, process.stdout.fd]) { + try { + closeSync(fd) + } catch { + // Already closed by the peer. + } + } + const devNull = process.platform === 'win32' ? 'NUL' : '/dev/null' + try { + openSync(devNull, 'r') + } catch { + /* best-effort pin of the lowest free fd (normally 0) */ + } + try { + openSync(devNull, 'w') + } catch { + /* best-effort pin of the next free fd (normally 1) */ + } } }, undefined, diff --git a/src/relay/ssh-pty-consumer-session-adapter.ts b/src/relay/ssh-pty-consumer-session-adapter.ts index 1617fdb09dd..b28082aa7bc 100644 --- a/src/relay/ssh-pty-consumer-session-adapter.ts +++ b/src/relay/ssh-pty-consumer-session-adapter.ts @@ -1,6 +1,7 @@ import { PTY_CONSUMER_SESSION_PROTOCOL_VERSION, PtyConsumerSession, + type PtyConsumerSessionAdmission, type PtyConsumerSessionGrant, type PtyConsumerSessionHello } from '../shared/pty-consumer-session' @@ -74,7 +75,7 @@ export class SshPtyConsumerSessionAdapter { private readonly pausedDeliveryByPty = new Map() constructor( - dispatcher: RelayDispatcher, + private readonly dispatcher: RelayDispatcher, serverBuildId: string, private readonly setDeliveryPaused?: (id: string, paused: boolean) => void, onSourceCreditAvailable?: (id: string) => void @@ -268,15 +269,30 @@ export class SshPtyConsumerSessionAdapter { throw new Error('SSH PTY consumer response publication fence is unavailable') } context.onResponseSettled((result) => { - if (result.ok) { - admission.commitPublication() - } else { + if (!result.ok) { admission.rollbackPublication() + return } + admission.commitPublication() + this.closeDisplacedOwner(admission.displacedOwner) }) return admission.grant } + // Why: only after the replacement grant is published — until then the admission can still roll back + // onto the incumbent. Its deliveries are retained for the new owner to rotate, exactly as on detach. + private closeDisplacedOwner(displaced: PtyConsumerSessionAdmission['displacedOwner']): void { + if (!displaced) { + return + } + this.clearPausedForGrant(displaced.grant) + this.sourceCredit.retainOrCloseOnDetach(displaced.grant) + const clientId = Number(displaced.connectionId) + if (Number.isSafeInteger(clientId)) { + this.dispatcher.releaseDisplacedClient(clientId) + } + } + private clearPausedIdentity(identity: PtySourceDeliveryIdentity): void { if (this.pausedDeliveryByPty.get(identity.id) === identity) { this.pausedDeliveryByPty.delete(identity.id) diff --git a/src/renderer/src/components/UpdateCard.error-card.test.tsx b/src/renderer/src/components/UpdateCard.error-card.test.tsx index 0e41c46f07b..4e4aec4d2b5 100644 --- a/src/renderer/src/components/UpdateCard.error-card.test.tsx +++ b/src/renderer/src/components/UpdateCard.error-card.test.tsx @@ -127,6 +127,19 @@ describe('UpdateCard Windows signature failures', () => { 'true' ) }) + + // An install failure now carries the updater's own text, so it can reach these branches too. + it('routes a signature verdict raised during install to the security-stop card', () => { + const message = + 'New version 1.4.200 is not signed by the application owner: publisherNames: Orca' + renderAfterAvailableStatus() + + act(() => useAppStore.getState().setUpdateStatus({ state: 'error', message })) + + expect(screen.getByText("Update Wasn't Installed")).toBeTruthy() + // The generic restart advice must not be prefixed onto a security stop. + expect(screen.queryByText(/Quit and reopen Orca/)).toBeNull() + }) }) describe('UpdateCard hourly builds', () => { @@ -260,6 +273,18 @@ describe('UpdateCard Linux package-install recovery', () => { expect(download).toHaveBeenCalledTimes(1) }) + it('shows the appended install cause behind the generic card details', () => { + const message = + 'Could not start the update installer. Orca remains open. (Command failed: pkexec must be setuid root)' + renderAfterAvailableStatus() + + act(() => useAppStore.getState().setUpdateStatus({ state: 'error', message })) + + expect(screen.getByText('Update Error')).toBeTruthy() + fireEvent.click(screen.getByRole('button', { name: 'Show details' })) + expect(screen.getByText(message)).toBeTruthy() + }) + it('leaves the HTTP/1.1 compatibility branch untouched', () => { renderAfterAvailableStatus() diff --git a/src/renderer/src/components/settings/RemoteServerUpdateDialog.tsx b/src/renderer/src/components/settings/RemoteServerUpdateDialog.tsx index 918eec3344a..473caf85f00 100644 --- a/src/renderer/src/components/settings/RemoteServerUpdateDialog.tsx +++ b/src/renderer/src/components/settings/RemoteServerUpdateDialog.tsx @@ -93,7 +93,9 @@ function ServerUpdateRow({ {help ? (

{help} diff --git a/src/renderer/src/runtime/remote-server-install-failure-probe.test.ts b/src/renderer/src/runtime/remote-server-install-failure-probe.test.ts new file mode 100644 index 00000000000..1c840aecbac --- /dev/null +++ b/src/renderer/src/runtime/remote-server-install-failure-probe.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it, vi } from 'vitest' +import type { + RemoteServerUpdateInstallResult, + RemoteServerUpdaterSnapshot +} from '../../../shared/remote-server-update' +import type { RuntimeStatus } from '../../../shared/runtime-types' +import { readRemoteServerInstallFailure } from './remote-server-install-failure-probe' + +const support = { + installMode: 'supervised-headless-serve', + automatic: true, + reason: 'available' +} as const + +const install: RemoteServerUpdateInstallResult = { + accepted: true, + fromVersion: '1.4.0', + targetVersion: '1.5.0', + runtimeId: 'runtime-old' +} + +function runtime(runtimeId = 'runtime-old'): RuntimeStatus { + return { + runtimeId, + rendererGraphEpoch: 0, + graphStatus: 'ready', + authoritativeWindowId: null, + liveTabCount: 2, + liveLeafCount: 1, + capabilities: ['updater.remote-control.v1'], + appVersion: '1.4.0', + remoteUpdateSupport: support + } +} + +function snapshot( + status: RemoteServerUpdaterSnapshot['status'], + runtimeId = 'runtime-old' +): RemoteServerUpdaterSnapshot { + return { appVersion: '1.4.0', runtimeId, support, status } +} + +describe('readRemoteServerInstallFailure', () => { + it('returns the message the original process is publishing', async () => { + const getUpdaterStatus = vi.fn(async () => + snapshot({ state: 'error', message: 'pkexec must be setuid root' }) + ) + + await expect( + readRemoteServerInstallFailure('server-1', { getUpdaterStatus }, install, runtime()) + ).resolves.toBe('pkexec must be setuid root') + }) + + it('bounds the status call by the deadline the caller passes', async () => { + const getUpdaterStatus = vi.fn(async () => + snapshot({ state: 'error', message: 'pkexec must be setuid root' }) + ) + + await readRemoteServerInstallFailure( + 'server-1', + { getUpdaterStatus }, + install, + runtime(), + 4_000 + ) + + // Without this the call inherits the RPC client's 15s default and can outlast the reconnect budget. + expect(getUpdaterStatus).toHaveBeenCalledWith('server-1', 4_000) + }) + + it('ignores an error published by a different process', async () => { + const getUpdaterStatus = vi.fn(async () => + snapshot({ state: 'error', message: 'pkexec must be setuid root' }, 'runtime-new') + ) + + await expect( + readRemoteServerInstallFailure('server-1', { getUpdaterStatus }, install, runtime()) + ).resolves.toBeNull() + }) + + it('ignores a snapshot that is not an error', async () => { + const getUpdaterStatus = vi.fn(async () => snapshot({ state: 'downloaded', version: '1.5.0' })) + + await expect( + readRemoteServerInstallFailure('server-1', { getUpdaterStatus }, install, runtime()) + ).resolves.toBeNull() + }) + + it('costs no round trip once the replacement runtime answers', async () => { + const getUpdaterStatus = vi.fn(async () => + snapshot({ state: 'error', message: 'pkexec must be setuid root' }) + ) + + await expect( + readRemoteServerInstallFailure( + 'server-1', + { getUpdaterStatus }, + install, + runtime('runtime-new') + ) + ).resolves.toBeNull() + expect(getUpdaterStatus).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/runtime/remote-server-install-failure-probe.ts b/src/renderer/src/runtime/remote-server-install-failure-probe.ts new file mode 100644 index 00000000000..7f37fd38453 --- /dev/null +++ b/src/renderer/src/runtime/remote-server-install-failure-probe.ts @@ -0,0 +1,36 @@ +import type { + RemoteServerUpdateInstallResult, + RemoteServerUpdaterSnapshot +} from '../../../shared/remote-server-update' +import type { RuntimeStatus } from '../../../shared/runtime-types' + +type InstallFailureProbeTransport = { + getUpdaterStatus: ( + environmentId: string, + timeoutMs?: number + ) => Promise +} + +/** + * The install RPC is accepted before the installer runs, so a server that failed to restart keeps + * publishing the reason through `updater.getStatus`. Reading it turns a three-minute reconnect + * timeout into the actual cause. Both runtime-id guards matter: a snapshot from a process that is + * not the one we asked to install describes some other attempt. + */ +export async function readRemoteServerInstallFailure( + environmentId: string, + transport: InstallFailureProbeTransport, + install: RemoteServerUpdateInstallResult, + runtime: RuntimeStatus, + timeoutMs?: number +): Promise { + if (runtime.runtimeId !== install.runtimeId) { + return null + } + // Why: without an explicit deadline this inherits the RPC client's 15s default and can outlast the caller's own wait. + const snapshot = await transport.getUpdaterStatus(environmentId, timeoutMs) + if (snapshot.runtimeId !== install.runtimeId || snapshot.status.state !== 'error') { + return null + } + return snapshot.status.message +} diff --git a/src/renderer/src/runtime/remote-server-restart-wait.test.ts b/src/renderer/src/runtime/remote-server-restart-wait.test.ts new file mode 100644 index 00000000000..76567556d92 --- /dev/null +++ b/src/renderer/src/runtime/remote-server-restart-wait.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, it, vi } from 'vitest' +import type { + RemoteServerUpdateInstallResult, + RemoteServerUpdaterSnapshot +} from '../../../shared/remote-server-update' +import type { RuntimeStatus } from '../../../shared/runtime-types' +import { waitForReplacementRuntime } from './remote-server-restart-wait' + +const support = { + installMode: 'supervised-headless-serve', + automatic: true, + reason: 'available' +} as const + +const install: RemoteServerUpdateInstallResult = { + accepted: true, + fromVersion: '1.4.0', + targetVersion: '1.5.0', + runtimeId: 'runtime-old' +} + +const idleSnapshot: RemoteServerUpdaterSnapshot = { + appVersion: '1.4.0', + runtimeId: 'runtime-old', + support, + status: { state: 'downloaded', version: '1.5.0' } +} + +function runtime(version: string, runtimeId: string): RuntimeStatus { + return { + runtimeId, + rendererGraphEpoch: 0, + graphStatus: 'ready', + authoritativeWindowId: null, + liveTabCount: 2, + liveLeafCount: 1, + capabilities: ['updater.remote-control.v1'], + appVersion: version, + remoteUpdateSupport: support + } +} + +describe('waitForReplacementRuntime', () => { + it('returns the replacement process once it answers on the target version', async () => { + let clock = 0 + let ticks = 0 + + await expect( + waitForReplacementRuntime( + 'server-1', + { + now: () => clock, + wait: async (milliseconds) => { + clock += milliseconds + }, + getUpdaterStatus: async () => idleSnapshot, + getRuntimeStatus: async () => { + ticks += 1 + return ticks > 1 ? runtime('1.5.0', 'runtime-new') : runtime('1.4.0', 'runtime-old') + } + }, + install, + { reconnectTimeoutMs: 1_000, pollIntervalMs: 10 } + ) + ).resolves.toMatchObject({ runtimeId: 'runtime-new', appVersion: '1.5.0' }) + }) + + it('never lets a poll RPC outlive the reconnect deadline', async () => { + let clock = 0 + const reconnectTimeoutMs = 25_000 + const pollIntervalMs = 500 + + await expect( + waitForReplacementRuntime( + 'server-1', + { + now: () => clock, + wait: async (milliseconds) => { + clock += milliseconds + }, + getUpdaterStatus: async () => idleSnapshot, + // A wedged SSH link burns the whole allowance it is given before rejecting. + getRuntimeStatus: async (_environmentId, timeoutMs) => { + clock += timeoutMs ?? 0 + throw new Error('runtime status timed out') + } + }, + install, + { reconnectTimeoutMs, pollIntervalMs } + ) + ).rejects.toThrow('remote_update_reconnect_timeout') + + // A tick allowed its full 10s past the deadline would push this well beyond the budget. + expect(clock).toBeLessThanOrEqual(reconnectTimeoutMs) + }) + + it('keeps the total wait inside the budget when the status RPC eats it before the probe', async () => { + let clock = 0 + const reconnectTimeoutMs = 20_000 + + await expect( + waitForReplacementRuntime( + 'server-1', + { + now: () => clock, + wait: async (milliseconds) => { + clock += milliseconds + }, + // Both RPCs burn everything they are handed, so a stale budget doubles the tick. + getUpdaterStatus: async (_environmentId, timeoutMs) => { + clock += timeoutMs ?? 0 + return idleSnapshot + }, + getRuntimeStatus: async (_environmentId, timeoutMs) => { + clock += timeoutMs ?? 0 + return runtime('1.4.0', 'runtime-old') + } + }, + install, + { reconnectTimeoutMs, pollIntervalMs: 500 } + ) + ).rejects.toThrow('remote_update_reconnect_timeout') + + expect(clock).toBeLessThanOrEqual(reconnectTimeoutMs) + }) + + it('charges the failure probe only the budget left after the status RPC', async () => { + let clock = 0 + const getUpdaterStatus = vi.fn(async () => idleSnapshot) + + await expect( + waitForReplacementRuntime( + 'server-1', + { + now: () => clock, + wait: async (milliseconds) => { + clock += milliseconds + }, + getUpdaterStatus, + getRuntimeStatus: async () => { + clock += 2_000 + return runtime('1.4.0', 'runtime-old') + } + }, + install, + { reconnectTimeoutMs: 9_000, pollIntervalMs: 1_000 } + ) + ).rejects.toThrow('remote_update_reconnect_timeout') + + // First probe runs on the second tick: it starts at 3s, the status RPC spends 2s more, + // so 4s of the 9s budget is left — not the 6s the tick began with. + expect(getUpdaterStatus).toHaveBeenNthCalledWith(1, 'server-1', 4_000) + }) + + it('bounds the failure probe by the same remaining budget', async () => { + let clock = 0 + const getUpdaterStatus = vi.fn(async () => idleSnapshot) + + await expect( + waitForReplacementRuntime( + 'server-1', + { + now: () => clock, + wait: async (milliseconds) => { + clock += milliseconds + }, + getUpdaterStatus, + getRuntimeStatus: async () => runtime('1.4.0', 'runtime-old') + }, + install, + { reconnectTimeoutMs: 4_000, pollIntervalMs: 1_000 } + ) + ).rejects.toThrow('remote_update_reconnect_timeout') + + // First tick is the probe's warm-up; the second runs with 3s of the 4s budget left. + expect(getUpdaterStatus).toHaveBeenCalledWith('server-1', 3_000) + }) +}) diff --git a/src/renderer/src/runtime/remote-server-restart-wait.ts b/src/renderer/src/runtime/remote-server-restart-wait.ts new file mode 100644 index 00000000000..d78b2170f1d --- /dev/null +++ b/src/renderer/src/runtime/remote-server-restart-wait.ts @@ -0,0 +1,81 @@ +import { hasReachedAppVersion } from '../../../shared/app-version' +import type { + RemoteServerUpdateInstallResult, + RemoteServerUpdaterSnapshot +} from '../../../shared/remote-server-update' +import type { RuntimeStatus } from '../../../shared/runtime-types' +import { readRemoteServerInstallFailure } from './remote-server-install-failure-probe' + +type RestartWaitTransport = { + getRuntimeStatus: (environmentId: string, timeoutMs?: number) => Promise + getUpdaterStatus: ( + environmentId: string, + timeoutMs?: number + ) => Promise + wait: (milliseconds: number) => Promise + now?: () => number +} + +type RestartWaitTiming = { + reconnectTimeoutMs: number + pollIntervalMs: number +} + +/** Per-call ceiling for the two poll RPCs; each tick also clamps to the time left. */ +const RESTART_WAIT_RPC_TIMEOUT_MS = 10_000 + +/** + * Waits for the replacement process to answer on the target version and returns its runtime status. + * Throws the server's own install-failure text as soon as the *original* process publishes one — + * without that, a server that could not restart is indistinguishable from a slow one until the + * reconnect budget runs out, and the user is told the wrong thing. + */ +export async function waitForReplacementRuntime( + environmentId: string, + transport: RestartWaitTransport, + install: RemoteServerUpdateInstallResult, + timing: RestartWaitTiming +): Promise { + const now = transport.now ?? Date.now + const deadline = now() + timing.reconnectTimeoutMs + // Why: the install RPC returns before the installer fires, so an error on the first tick belongs to an earlier attempt. + let probed = false + while (now() < deadline) { + // Why: an RPC allowed to outlive the deadline drags the whole wait past the budget the caller set. + const rpcTimeoutMs = Math.min(RESTART_WAIT_RPC_TIMEOUT_MS, deadline - now()) + let installFailure: string | null = null + try { + const status = await transport.getRuntimeStatus(environmentId, rpcTimeoutMs) + const version = status.appVersion?.trim() ?? '' + const reachedTarget = hasReachedAppVersion(version, install.targetVersion) + if (status.runtimeId !== install.runtimeId && reachedTarget) { + return status + } + // Why: the status RPC already spent part of rpcTimeoutMs, so the probe re-reads what is left + // rather than starting a second full budget of its own. + const probeTimeoutMs = Math.min(RESTART_WAIT_RPC_TIMEOUT_MS, deadline - now()) + installFailure = + probed && probeTimeoutMs > 0 + ? await readRemoteServerInstallFailure( + environmentId, + transport, + install, + status, + probeTimeoutMs + ) + : null + probed = true + } catch { + // A refused connection is expected while the server process is being replaced. + } + // Why: thrown outside the try so the loop's own catch cannot swallow the reason we came for. + if (installFailure !== null) { + throw new Error(installFailure) + } + const waitMs = Math.min(timing.pollIntervalMs, deadline - now()) + if (waitMs > 0) { + await transport.wait(waitMs) + } + } + throw new Error('remote_update_reconnect_timeout') +} diff --git a/src/renderer/src/runtime/remote-server-update-coordinator.test.ts b/src/renderer/src/runtime/remote-server-update-coordinator.test.ts index 824fdf6fbdd..66cb646f711 100644 --- a/src/renderer/src/runtime/remote-server-update-coordinator.test.ts +++ b/src/renderer/src/runtime/remote-server-update-coordinator.test.ts @@ -44,6 +44,10 @@ function status(version: string, runtimeId = 'runtime-old', automatic = true): R } } +// The main process's own copy once quitAndInstall fails; it must survive to the client verbatim. +const INSTALL_FAILURE = + "Could not start the update installer. Orca remains open. (Command failed: pkexec dpkg -i '/home/u/.cache/orca-updater/pending/orca-ide_1.5.0_amd64.deb' pkexec must be setuid root)" + const availableSnapshot: RemoteServerUpdaterSnapshot = { appVersion: '1.4.0', runtimeId: 'runtime-old', @@ -275,6 +279,91 @@ describe('remote server update execution', () => { }) }) + it('repeats the install failure a still-running server is publishing', async () => { + const snapshots = [ + availableSnapshot, + { ...availableSnapshot, status: { state: 'downloaded', version: '1.5.0' } } + ] satisfies RemoteServerUpdaterSnapshot[] + let installed = false + const result = await runRemoteServerUpdate( + availableEntry(), + transport({ + getUpdaterStatus: async () => + installed + ? { ...availableSnapshot, status: { state: 'error', message: INSTALL_FAILURE } } + : (snapshots.shift() ?? availableSnapshot), + install: async (): Promise => { + installed = true + return { + accepted: true, + fromVersion: '1.4.0', + targetVersion: '1.5.0', + runtimeId: 'runtime-old' + } + }, + // The server never restarts: same runtimeId, same version, still perfectly reachable. + getRuntimeStatus: async () => status('1.4.0', 'runtime-old') + }), + () => undefined, + { timing: { operationTimeoutMs: 10, reconnectTimeoutMs: 60, pollIntervalMs: 1 } } + ) + expect(result).toMatchObject({ phase: 'failed', error: INSTALL_FAILURE }) + }) + + it('ignores an updater error that predates the install request', async () => { + const snapshots = [ + availableSnapshot, + { ...availableSnapshot, status: { state: 'downloaded', version: '1.5.0' } } + ] satisfies RemoteServerUpdaterSnapshot[] + let reconnectTicks = 0 + const result = await runRemoteServerUpdate( + availableEntry(), + transport({ + // A background check that errored inside the 100ms before the installer fires. + getUpdaterStatus: async () => + snapshots.shift() ?? { + ...availableSnapshot, + status: { state: 'error', message: 'a background check failed' } + }, + getRuntimeStatus: async () => { + reconnectTicks += 1 + return reconnectTicks > 1 + ? status('1.5.0', 'runtime-new') + : status('1.4.0', 'runtime-old') + } + }), + () => undefined, + { timing: { operationTimeoutMs: 10, reconnectTimeoutMs: 60, pollIntervalMs: 1 } } + ) + expect(result).toMatchObject({ phase: 'updated', currentVersion: '1.5.0' }) + }) + + it('keeps waiting when the failure probe itself cannot be reached', async () => { + const snapshots = [ + availableSnapshot, + { ...availableSnapshot, status: { state: 'downloaded', version: '1.5.0' } } + ] satisfies RemoteServerUpdaterSnapshot[] + const result = await runRemoteServerUpdate( + availableEntry(), + transport({ + getUpdaterStatus: async () => { + const next = snapshots.shift() + if (!next) { + throw new Error('connection refused') + } + return next + }, + getRuntimeStatus: async () => status('1.4.0', 'runtime-old') + }), + () => undefined, + { timing: { operationTimeoutMs: 10, reconnectTimeoutMs: 6, pollIntervalMs: 1 } } + ) + expect(result).toMatchObject({ + phase: 'failed', + error: 'The server did not reconnect on the updated version.' + }) + }) + it('turns a capability race into manual update guidance', async () => { const result = await runRemoteServerUpdate( availableEntry(), diff --git a/src/renderer/src/runtime/remote-server-update-coordinator.ts b/src/renderer/src/runtime/remote-server-update-coordinator.ts index 77b1b6f2cbd..196699b6eed 100644 --- a/src/renderer/src/runtime/remote-server-update-coordinator.ts +++ b/src/renderer/src/runtime/remote-server-update-coordinator.ts @@ -1,5 +1,6 @@ import { compareAppVersions, + hasReachedAppVersion, isPerfPrereleaseAppVersion, isPrereleaseAppVersion, isValidAppVersion @@ -13,6 +14,7 @@ import type { import type { PublicKnownRuntimeEnvironment } from '../../../shared/runtime-environments' import type { RuntimeStatus } from '../../../shared/runtime-types' import type { UpdateCheckOptions } from '../../../shared/types' +import { waitForReplacementRuntime } from './remote-server-restart-wait' import { remoteServerUpdateErrorMessage } from './remote-server-update-errors' import { pollRemoteServerUpdater } from './remote-server-updater-polling' @@ -45,7 +47,10 @@ export type RemoteServerUpdateEntry = { export type RemoteServerUpdateTransport = { getRuntimeStatus: (environmentId: string, timeoutMs?: number) => Promise - getUpdaterStatus: (environmentId: string) => Promise + getUpdaterStatus: ( + environmentId: string, + timeoutMs?: number + ) => Promise check: ( environmentId: string, options: UpdateCheckOptions @@ -213,12 +218,7 @@ export async function runRemoteServerUpdate( if (available.status.state === 'not-available') { const status = await transport.getRuntimeStatus(entry.environmentId, 10_000) const currentVersion = status.appVersion?.trim() ?? '' - const reachedTarget = - entry.targetVersion !== null && - isValidAppVersion(currentVersion) && - isValidAppVersion(entry.targetVersion) && - compareAppVersions(currentVersion, entry.targetVersion) >= 0 - if (!reachedTarget) { + if (!hasReachedAppVersion(currentVersion, entry.targetVersion)) { throw new Error('remote_update_requested_version_unavailable') } next = { @@ -267,34 +267,22 @@ export async function runRemoteServerUpdate( } onProgress(next) - const now = transport.now ?? Date.now - const reconnectDeadline = now() + timing.reconnectTimeoutMs - while (now() < reconnectDeadline) { - try { - const status = await transport.getRuntimeStatus(entry.environmentId, 10_000) - const version = status.appVersion?.trim() ?? '' - const reachedTarget = - isValidAppVersion(version) && - isValidAppVersion(install.targetVersion) && - compareAppVersions(version, install.targetVersion) >= 0 - if (status.runtimeId !== install.runtimeId && reachedTarget) { - next = { - ...next, - phase: 'updated', - currentVersion: version, - runtimeId: status.runtimeId, - liveTabCount: status.liveTabCount, - liveLeafCount: status.liveLeafCount - } - onProgress(next) - return next - } - } catch { - // A refused connection is expected while the server process is being replaced. - } - await transport.wait(timing.pollIntervalMs) + const replacement = await waitForReplacementRuntime( + entry.environmentId, + transport, + install, + timing + ) + next = { + ...next, + phase: 'updated', + currentVersion: replacement.appVersion?.trim() ?? '', + runtimeId: replacement.runtimeId, + liveTabCount: replacement.liveTabCount, + liveLeafCount: replacement.liveLeafCount } - throw new Error('remote_update_reconnect_timeout') + onProgress(next) + return next } catch (error) { next = { ...next, diff --git a/src/renderer/src/store/slices/remote-server-updates.ts b/src/renderer/src/store/slices/remote-server-updates.ts index 909c05e3190..80b8a7370c1 100644 --- a/src/renderer/src/store/slices/remote-server-updates.ts +++ b/src/renderer/src/store/slices/remote-server-updates.ts @@ -33,8 +33,13 @@ const transport: RemoteServerUpdateTransport = { window.api.runtimeEnvironments .getStatus({ selector: environmentId, timeoutMs }) .then((response) => unwrapRuntimeRpcResult(response)), - getUpdaterStatus: (environmentId) => - callRemoteUpdater(environmentId, 'updater.getStatus'), + getUpdaterStatus: (environmentId, timeoutMs) => + callRemoteUpdater( + environmentId, + 'updater.getStatus', + undefined, + timeoutMs + ), check: (environmentId, options) => callRemoteUpdater(environmentId, 'updater.check', options), download: (environmentId) => diff --git a/src/shared/app-version.ts b/src/shared/app-version.ts index a904b4990c3..e3c537f4534 100644 --- a/src/shared/app-version.ts +++ b/src/shared/app-version.ts @@ -94,3 +94,13 @@ export function compareAppVersions(left: string, right: string): number { return 0 } + +/** True when `current` is at or past `target`. Unparseable or absent versions never qualify. */ +export function hasReachedAppVersion(current: string, target: string | null): boolean { + return ( + target !== null && + isValidAppVersion(current) && + isValidAppVersion(target) && + compareAppVersions(current, target) >= 0 + ) +} diff --git a/src/shared/pty-consumer-owner-recovery.ts b/src/shared/pty-consumer-owner-recovery.ts new file mode 100644 index 00000000000..dfac94d7e9c --- /dev/null +++ b/src/shared/pty-consumer-owner-recovery.ts @@ -0,0 +1,63 @@ +import { + PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR, + PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR, + PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR, + type PtyConsumerAuthentication, + type PtyConsumerSessionHello +} from './pty-consumer-session-contract' + +type IncumbentOwner = { + principal: string + clientInstanceId: string + generation: number + lease: string + state: 'pending' | 'active' | 'disconnected' + replaces?: { generation: number } +} + +function throwRecoveryError(message: string, code: number): never { + throw Object.assign(new Error(message), { code }) +} + +export function assertPtyConsumerOwnerRecovery( + hello: PtyConsumerSessionHello, + authentication: PtyConsumerAuthentication, + current: IncumbentOwner +): void { + const resume = hello.resume + if (!resume) { + throw new Error('Owner recovery proof is required') + } + if ( + resume.ownerLease !== current.lease || + hello.clientInstanceId !== current.clientInstanceId || + authentication.principal !== current.principal + ) { + throwRecoveryError( + 'Owner recovery lease is stale or belongs to another principal', + PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR + ) + } + if (current.state === 'active' && resume.ownerGeneration < current.generation) { + throwRecoveryError( + 'Owner recovery generation was superseded', + PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR + ) + } + const generationMatches = + resume.ownerGeneration === current.generation || + (current.state === 'pending' && resume.ownerGeneration === current.replaces?.generation) || + (current.state === 'disconnected' && resume.ownerGeneration < current.generation) + if (!generationMatches) { + throwRecoveryError( + 'Owner recovery generation is stale', + PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR + ) + } + if (current.state === 'pending') { + throwRecoveryError( + 'Owner grant publication is still pending', + PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR + ) + } +} diff --git a/src/shared/pty-consumer-session-contract.ts b/src/shared/pty-consumer-session-contract.ts index 661138896cd..1e359a11f53 100644 --- a/src/shared/pty-consumer-session-contract.ts +++ b/src/shared/pty-consumer-session-contract.ts @@ -1,6 +1,10 @@ export const PTY_CONSUMER_SESSION_PROTOCOL_VERSION = 1 export const PTY_CONSUMER_OWNER_GRACE_MS = 30_000 export const PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR = -32041 +// Why: recovery is blocked only while the incumbent owner's grant publication is still settling — a +// window bounded by one response write, so the client may retry within a short budget. +export const PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR = -32042 +export const PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR = -32043 export type PtyConsumerRole = 'session-owner' | 'subscriber' @@ -41,8 +45,16 @@ export type PtyConsumerAuthentication = { allowSessionOwner: boolean } +export type PtyConsumerDisplacedOwner = { + connectionId: string + grant: Readonly +} + export type PtyConsumerSessionAdmission = { grant: Readonly + // Why: set when this admission takes over a still-attached owner. The transport layer owns closing + // that connection and releasing its deliveries — do it only once the new grant has been published. + displacedOwner?: Readonly commitPublication: () => void rollbackPublication: () => void } diff --git a/src/shared/pty-consumer-session-hello.ts b/src/shared/pty-consumer-session-hello.ts new file mode 100644 index 00000000000..c8119da88ee --- /dev/null +++ b/src/shared/pty-consumer-session-hello.ts @@ -0,0 +1,52 @@ +import type { PtyConsumerSessionHello } from './pty-consumer-session-contract' + +export const MAX_CAPABILITY_VERSIONS = 8 + +export function assertNonEmptyString(value: unknown, name: string): asserts value is string { + if (typeof value !== 'string' || value.length === 0 || value.length > 512) { + throw new Error(`${name} must be a non-empty string of at most 512 characters`) + } +} + +export function validateHello(hello: PtyConsumerSessionHello): void { + assertNonEmptyString(hello.clientInstanceId, 'clientInstanceId') + if (hello.requestedRole !== 'session-owner' && hello.requestedRole !== 'subscriber') { + throw new Error('requestedRole must be session-owner or subscriber') + } + if (hello.resume) { + if (!Number.isSafeInteger(hello.resume.ownerGeneration) || hello.resume.ownerGeneration <= 0) { + throw new Error('resume.ownerGeneration must be a positive safe integer') + } + assertNonEmptyString(hello.resume.ownerLease, 'resume.ownerLease') + } + const flow = hello.capabilities?.outputFlowControl + if (!flow) { + return + } + if ( + !Array.isArray(flow.versions) || + flow.versions.length > MAX_CAPABILITY_VERSIONS || + flow.versions.some((version) => !Number.isSafeInteger(version) || version <= 0) + ) { + throw new Error('outputFlowControl.versions must contain positive safe integers') + } + if (!Number.isSafeInteger(flow.requestedWindowSu) || flow.requestedWindowSu <= 0) { + throw new Error('outputFlowControl.requestedWindowSu must be a positive safe integer') + } +} + +// Why: a duplicate open on one connection is only idempotent if it asks for exactly the same thing. +export function helloFingerprint(hello: PtyConsumerSessionHello): string { + const flow = hello.capabilities?.outputFlowControl + return JSON.stringify({ + clientInstanceId: hello.clientInstanceId, + requestedRole: hello.requestedRole, + resume: hello.resume, + outputFlowControl: flow + ? { + versions: [...flow.versions].sort((a, b) => a - b), + requestedWindowSu: flow.requestedWindowSu + } + : undefined + }) +} diff --git a/src/shared/pty-consumer-session.test.ts b/src/shared/pty-consumer-session.test.ts index 07f9f7f9bf9..ce6282e8664 100644 --- a/src/shared/pty-consumer-session.test.ts +++ b/src/shared/pty-consumer-session.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it } from 'vitest' import { + PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR, + PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR, PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR, PtyConsumerSession, type PtyConsumerAuthentication, @@ -121,7 +123,7 @@ describe('PtyConsumerSession', () => { ).toThrow('authentication required') }) - it('rotates the lease and increments owner generation on valid recovery', () => { + it('keeps the lease stable and increments owner generation on valid recovery', () => { const session = createSession() const first = session.admit(ownerHello(), auth('connection-1')) first.commitPublication() @@ -141,15 +143,97 @@ describe('PtyConsumerSession', () => { expect(recovered.grant).toMatchObject({ role: 'session-owner', ownerGeneration: 2, - ownerLease: 'lease-2' + ownerLease: 'lease-1' }) }) - it('rejects recovery while the current owner is still active', () => { + it('displaces a still-attached owner that a matching resume proof reclaims', () => { const session = createSession() const first = session.admit(ownerHello(), auth('connection-1')) first.commitPublication() + const recovered = session.admit( + ownerHello({ + resume: { + ownerGeneration: first.grant.ownerGeneration!, + ownerLease: first.grant.ownerLease! + } + }), + auth('connection-2') + ) + + expect(recovered.displacedOwner).toEqual({ + connectionId: 'connection-1', + grant: first.grant + }) + // Why: the incumbent keeps authority until the replacement grant is actually published. + expect(session.activeGrant('connection-1')).toBe(first.grant) + + recovered.commitPublication() + expect(recovered.grant).toMatchObject({ role: 'session-owner', ownerGeneration: 2 }) + expect(session.activeGrant('connection-1')).toBeNull() + expect(session.activeGrant('connection-2')).toBe(recovered.grant) + }) + + it('restores the displaced owner when the replacement publication rolls back', () => { + const session = createSession() + const first = session.admit(ownerHello(), auth('connection-1')) + first.commitPublication() + + const recovered = session.admit( + ownerHello({ + resume: { + ownerGeneration: first.grant.ownerGeneration!, + ownerLease: first.grant.ownerLease! + } + }), + auth('connection-2') + ) + recovered.rollbackPublication() + + expect(session.activeGrant('connection-1')).toBe(first.grant) + expect(session.activeGrant('connection-2')).toBeNull() + // Why: the restored incumbent must still hold the lease it was admitted with. + const reclaimed = session.admit( + ownerHello({ + resume: { + ownerGeneration: first.grant.ownerGeneration!, + ownerLease: first.grant.ownerLease! + } + }), + auth('connection-3') + ) + expect(reclaimed.displacedOwner?.connectionId).toBe('connection-1') + }) + + it('expires a displaced owner restored after its connection already closed', () => { + let now = 10 + const session = createSession({ now: () => now }) + const first = session.admit(ownerHello(), auth('connection-1')) + first.commitPublication() + + const recovered = session.admit( + ownerHello({ + resume: { + ownerGeneration: first.grant.ownerGeneration!, + ownerLease: first.grant.ownerLease! + } + }), + auth('connection-2') + ) + session.close('connection-1') + recovered.rollbackPublication() + + now += 30_000 + session.sweepExpired() + const fresh = session.admit(ownerHello(), auth('connection-4')) + expect(fresh.grant.role).toBe('session-owner') + }) + + it('refuses recovery while the incumbent grant publication is still settling', () => { + const session = createSession() + const first = session.admit(ownerHello(), auth('connection-1')) + expect(() => session.admit( ownerHello({ @@ -160,7 +244,99 @@ describe('PtyConsumerSession', () => { }), auth('connection-2') ) - ).toThrow('Active owner') + ).toThrow( + expect.objectContaining({ + code: PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR, + message: expect.stringContaining('still pending') + }) + ) + }) + + it('fences an old recovery generation after its replacement commits', () => { + const session = createSession() + const first = session.admit(ownerHello(), auth('connection-1')) + first.commitPublication() + const resume = { + ownerGeneration: first.grant.ownerGeneration!, + ownerLease: first.grant.ownerLease! + } + const replacement = session.admit(ownerHello({ resume }), auth('connection-2')) + + expect(() => session.admit(ownerHello({ resume }), auth('connection-3'))).toThrow( + expect.objectContaining({ code: PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR }) + ) + + replacement.commitPublication() + expect(() => session.admit(ownerHello({ resume }), auth('connection-3'))).toThrow( + expect.objectContaining({ code: PTY_CONSUMER_OWNER_RECOVERY_SUPERSEDED_ERROR }) + ) + + const retry = session.admit( + ownerHello({ + resume: { + ownerGeneration: replacement.grant.ownerGeneration!, + ownerLease: replacement.grant.ownerLease! + } + }), + auth('connection-3') + ) + + expect(retry.grant).toMatchObject({ + role: 'session-owner', + ownerGeneration: 3, + ownerLease: first.grant.ownerLease + }) + expect(retry.displacedOwner?.connectionId).toBe('connection-2') + // Why: the committed replacement already retired the incumbent, so only connection-2 is left to displace. + expect(session.activeGrant('connection-1')).toBeNull() + }) + + it('accepts an older stable proof after its replacement disconnects', () => { + const session = createSession() + const first = session.admit(ownerHello(), auth('connection-1')) + first.commitPublication() + const resume = { + ownerGeneration: first.grant.ownerGeneration!, + ownerLease: first.grant.ownerLease! + } + const replacement = session.admit(ownerHello({ resume }), auth('connection-2')) + replacement.commitPublication() + session.close('connection-2') + + const retry = session.admit(ownerHello({ resume }), auth('connection-3')) + + expect(retry.grant).toMatchObject({ + role: 'session-owner', + ownerGeneration: 3, + ownerLease: first.grant.ownerLease + }) + }) + + it('retries overlapping recovery against the incumbent after publication rolls back', () => { + const session = createSession() + const first = session.admit(ownerHello(), auth('connection-1')) + first.commitPublication() + const resume = { + ownerGeneration: first.grant.ownerGeneration!, + ownerLease: first.grant.ownerLease! + } + const replacement = session.admit(ownerHello({ resume }), auth('connection-2')) + + expect(() => session.admit(ownerHello({ resume }), auth('connection-3'))).toThrow( + expect.objectContaining({ code: PTY_CONSUMER_OWNER_RECOVERY_PENDING_ERROR }) + ) + + replacement.rollbackPublication() + const retry = session.admit(ownerHello({ resume }), auth('connection-3')) + + expect(retry.grant).toMatchObject({ + role: 'session-owner', + ownerGeneration: 3, + ownerLease: first.grant.ownerLease + }) + expect(retry.displacedOwner?.connectionId).toBe('connection-1') + // Why: the rolled-back replacement never published, so it holds no grant the retry could displace. + expect(session.activeGrant('connection-2')).toBeNull() }) it('types mismatched recovery without disturbing principal or lease ownership', () => { @@ -201,7 +377,7 @@ describe('PtyConsumerSession', () => { expect(recovered.grant).toMatchObject({ role: 'session-owner', ownerGeneration: 2, - ownerLease: 'lease-2' + ownerLease: 'lease-1' }) }) diff --git a/src/shared/pty-consumer-session.ts b/src/shared/pty-consumer-session.ts index 953b77c3a8d..3baf502acd8 100644 --- a/src/shared/pty-consumer-session.ts +++ b/src/shared/pty-consumer-session.ts @@ -4,22 +4,28 @@ import { PTY_CONSUMER_SESSION_PROTOCOL_VERSION, PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR, type PtyConsumerAuthentication, + type PtyConsumerDisplacedOwner, type PtyConsumerSessionAdmission, type PtyConsumerSessionGrant, type PtyConsumerSessionHello, type PtyConsumerSessionOptions } from './pty-consumer-session-contract' +import { + assertNonEmptyString, + helloFingerprint, + MAX_CAPABILITY_VERSIONS, + validateHello +} from './pty-consumer-session-hello' +import { assertPtyConsumerOwnerRecovery } from './pty-consumer-owner-recovery' export * from './pty-consumer-session-contract' -const MAX_CAPABILITY_VERSIONS = 8 - type ClientRecord = { fingerprint: string principal: string clientInstanceId: string grant: Readonly - state: 'pending' | 'active' + state: 'pending' | 'active' | 'displaced' publicationState: 'pending' | 'committed' | 'rolled-back' } @@ -34,54 +40,6 @@ type OwnerRecord = { replaces?: OwnerRecord } -function assertNonEmptyString(value: unknown, name: string): asserts value is string { - if (typeof value !== 'string' || value.length === 0 || value.length > 512) { - throw new Error(`${name} must be a non-empty string of at most 512 characters`) - } -} - -function validateHello(hello: PtyConsumerSessionHello): void { - assertNonEmptyString(hello.clientInstanceId, 'clientInstanceId') - if (hello.requestedRole !== 'session-owner' && hello.requestedRole !== 'subscriber') { - throw new Error('requestedRole must be session-owner or subscriber') - } - if (hello.resume) { - if (!Number.isSafeInteger(hello.resume.ownerGeneration) || hello.resume.ownerGeneration <= 0) { - throw new Error('resume.ownerGeneration must be a positive safe integer') - } - assertNonEmptyString(hello.resume.ownerLease, 'resume.ownerLease') - } - const flow = hello.capabilities?.outputFlowControl - if (!flow) { - return - } - if ( - !Array.isArray(flow.versions) || - flow.versions.length > MAX_CAPABILITY_VERSIONS || - flow.versions.some((version) => !Number.isSafeInteger(version) || version <= 0) - ) { - throw new Error('outputFlowControl.versions must contain positive safe integers') - } - if (!Number.isSafeInteger(flow.requestedWindowSu) || flow.requestedWindowSu <= 0) { - throw new Error('outputFlowControl.requestedWindowSu must be a positive safe integer') - } -} - -function helloFingerprint(hello: PtyConsumerSessionHello): string { - const flow = hello.capabilities?.outputFlowControl - return JSON.stringify({ - clientInstanceId: hello.clientInstanceId, - requestedRole: hello.requestedRole, - resume: hello.resume, - outputFlowControl: flow - ? { - versions: [...flow.versions].sort((a, b) => a - b), - requestedWindowSu: flow.requestedWindowSu - } - : undefined - }) -} - export class PtyConsumerSession { private readonly clients = new Map() private readonly now: () => number @@ -160,7 +118,7 @@ export class PtyConsumerSession { if (owner) { this.owner = owner } - return this.admissionFor(client) + return this.admissionFor(client, this.displacedOwnerFor(owner)) } close(connectionId: string): void { @@ -170,6 +128,17 @@ export class PtyConsumerSession { } this.clients.delete(connectionId) if (this.owner?.connectionId !== connectionId) { + // Why: a pending replacement can still roll back onto the owner it is displacing; restoring an + // 'active' record whose connection has since closed would wedge an owner that can never expire. + if ( + this.owner?.replaces?.connectionId === connectionId && + this.owner.replaces.state === 'active' + ) { + this.owner = { + ...this.owner, + replaces: { ...this.owner.replaces, state: 'disconnected', disconnectedAt: this.now() } + } + } return } if (this.owner.state === 'pending') { @@ -192,9 +161,13 @@ export class PtyConsumerSession { return client?.state === 'active' ? client.grant : null } - private admissionFor(client: ClientRecord): PtyConsumerSessionAdmission { + private admissionFor( + client: ClientRecord, + displacedOwner?: Readonly + ): PtyConsumerSessionAdmission { return { grant: client.grant, + ...(displacedOwner ? { displacedOwner } : {}), commitPublication: () => { if (client.publicationState !== 'pending') { return @@ -206,6 +179,7 @@ export class PtyConsumerSession { client.state = 'active' const owner = this.owner if (owner?.connectionId === this.connectionIdFor(client) && owner.state === 'pending') { + this.retireDisplacedOwner(owner.replaces) this.owner = { ...owner, state: 'active', replaces: undefined } } }, @@ -254,32 +228,46 @@ export class PtyConsumerSession { if (!hello.resume) { return null } - const recoveryMatches = - hello.resume.ownerGeneration === current.generation && - hello.resume.ownerLease === current.lease && - hello.clientInstanceId === current.clientInstanceId && - authentication.principal === current.principal - if (!recoveryMatches) { - throw Object.assign( - new Error('Owner recovery lease is stale or belongs to another principal'), - { code: PTY_CONSUMER_STALE_OWNER_RECOVERY_ERROR } - ) - } - if (current.state === 'pending') { - throw new Error('Owner grant publication is still pending') - } - if (current.state !== 'disconnected') { - throw new Error('Active owner cannot be replaced by recovery') - } + assertPtyConsumerOwnerRecovery(hello, authentication, current) + // Why an active owner is displaced rather than refused: the resume proof matched this owner's + // generation, lease, client instance, and principal on a *different* transport, so the requester is + // the same logical owner reconnecting. Waiting for the incumbent's socket to close is unbounded — + // a half-open connection after sleep/resume or NAT loss never gets there. return this.newOwner(hello, authentication, current) } + private displacedOwnerFor( + owner: OwnerRecord | null + ): Readonly | undefined { + const replaced = owner?.replaces + if (replaced?.state !== 'active') { + return undefined + } + const client = this.clients.get(replaced.connectionId) + if (client?.state !== 'active') { + return undefined + } + return Object.freeze({ connectionId: replaced.connectionId, grant: client.grant }) + } + + // Why: the displaced connection may still be writable (half-open), so revoke its grant the moment the + // replacement is published — a stale owner must not keep driving deliveries under the old generation. + private retireDisplacedOwner(replaced: OwnerRecord | undefined): void { + if (replaced?.state !== 'active') { + return + } + const client = this.clients.get(replaced.connectionId) + if (client?.state === 'active') { + client.state = 'displaced' + } + } + private newOwner( hello: PtyConsumerSessionHello, authentication: PtyConsumerAuthentication, replaces: OwnerRecord | null ): OwnerRecord { - const lease = this.createLease() + const lease = replaces?.lease ?? this.createLease() assertNonEmptyString(lease, 'ownerLease') return { connectionId: authentication.connectionId,