From 00867f06e27a8e3ebf1025fb3b476354877346c4 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:37:25 -0700 Subject: [PATCH 1/4] fix(ssh): handle owner displacement and graceful shutdown (#12367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ssh): handle owner displacement and graceful shutdown SSH connections can reconnect with valid session proof after network loss or device sleep. When the incumbent owner is still half-open, allow the reconnecting client to displace it outright rather than wait for socket closure — a window that may never close. Retain displaced deliveries for the new owner to rotate. During app shutdown, drain SSH sessions without terminating recovery operations, and retry pending owner grants in case a replacement commits mid-drain. * fix(ssh): handle owner displacement and graceful shutdown Make QuitTeardownStartGate a shared singleton so SSH connects use the same shutdown fence as the main quit path. Track test-connection probes to ensure they complete before final teardown. Guard owner displacement to prevent stale owners from clearing recovery state claimed by newer owners. * fix(ssh): fix flaky test sync and add error code safety check Test was using tick-based Promise.resolve() loops which don't guarantee the async operation has started. Replace with signal-based synchronization that waits for the actual lease flush. Also add nullish-coalescing to error code check to prevent crashes if error is null or undefined. * fix(ssh): fence reset transport opens during shutdown * fix(ssh): keep recovery leases stable across reconnects * fix(ssh): close transports owned by cancelled connect attempts When a connect is cancelled after its transport has opened, that cancelled attempt still owns the transport and must close it — otherwise it leaks. Add disconnectConnection() to close by identity (not by target ID) so a cancelled attempt closes only the transport it minted, without tearing down its replacement's live transport. Track priorConnection to detect whether this attempt opened a new transport or reused an existing one, and close only on abandonment if this attempt owns the session. * fix(ssh): fence old owner proofs and close superseded transports When an owner reconnects with a new proof while an old one is still live, the old proof is now fenced with SUPERSEDED_ERROR instead of retrying indefinitely. The relay also closes stale transports to signal that their recovery generation has been overtaken by a newer one. This ensures overlapping reconnect scenarios complete with the newest proof rather than getting blocked by stale recovery attempts. * fix(relay): re-pin stdin/stdout fds after closing to prevent recycling When the relay closes stdin/stdout to signal EOF to the SSH peer, the OS can recycle those fds (0 and 1) for new sockets or files. If Node still treats process.stdin/stdout as those numbers, subsequent operations corrupt socket clients and trigger shutdown errors. Re-pin the fds by opening /dev/null to keep them occupied and prevent recycling. --- src/main/index.ts | 6 +- src/main/ipc/ssh.test.ts | 296 +++++++++++++- src/main/ipc/ssh.ts | 136 ++++++- src/main/quit-teardown-start-gate.ts | 12 + src/main/ssh/ssh-connection-manager.test.ts | 32 ++ src/main/ssh/ssh-connection-manager.ts | 12 + src/main/ssh/ssh-owner-recovery-retry.test.ts | 106 +++++ src/main/ssh/ssh-owner-recovery-retry.ts | 69 ++++ ...-relay-session-recovery-durability.test.ts | 120 ++++++ src/main/ssh/ssh-relay-session.ts | 74 ++-- src/relay/dispatcher.ts | 10 + ...ay-pty-consumer-owner-displacement.test.ts | 368 ++++++++++++++++++ src/relay/relay.ts | 35 +- src/relay/ssh-pty-consumer-session-adapter.ts | 24 +- src/shared/pty-consumer-owner-recovery.ts | 63 +++ src/shared/pty-consumer-session-contract.ts | 12 + src/shared/pty-consumer-session-hello.ts | 52 +++ src/shared/pty-consumer-session.test.ts | 186 ++++++++- src/shared/pty-consumer-session.ts | 130 +++---- 19 files changed, 1627 insertions(+), 116 deletions(-) create mode 100644 src/main/ssh/ssh-owner-recovery-retry.test.ts create mode 100644 src/main/ssh/ssh-owner-recovery-retry.ts create mode 100644 src/relay/relay-pty-consumer-owner-displacement.test.ts create mode 100644 src/shared/pty-consumer-owner-recovery.ts create mode 100644 src/shared/pty-consumer-session-hello.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/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/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/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/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/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, From e9cf106769000b4431feab49af597ee87e49615d Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:38:10 -0700 Subject: [PATCH 2/4] fix(relay): make fenceAndCloseNow stop the liveness safety net (#12482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 5-minute liveness tick from #12432 survived fenceAndCloseNow(), so a tick landing between the pre-sign-out fence and the profile wipe could briefly resurrect a broker (benign but soft — the entitlement check bails afterward). The fence now clears the interval; the next auth mutation re-arms it via refreshDemand, and the safety net otherwise behaves identically. Found in release review of the #12432 cherry-pick. Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../relay/desktop-relay-service.test.ts | 43 ++++++++++++++++++- .../runtime/relay/desktop-relay-service.ts | 13 ++++-- 2 files changed, 52 insertions(+), 4 deletions(-) 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 From e1071f59e9b7c8e5e1d2ece693425a8fefb7c0f7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:02:53 -0700 Subject: [PATCH 3/4] Say why an update install failed instead of stalling for three minutes (#12224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaces the real install-failure cause instead of letting a failed elevation stall silently, and keeps the reconnect wait inside its total budget by recomputing the remaining time after each awaited RPC. Relates to #11906 — this fixes the observability half. The functional half (a .deb/.rpm host cannot elevate and can never self-update) is unchanged, so the issue stays open. --- .../updater.install-failure-cause.test.ts | 333 ++++++++++++++++++ src/main/updater.test.ts | 20 +- src/main/updater.ts | 58 ++- .../components/UpdateCard.error-card.test.tsx | 25 ++ .../settings/RemoteServerUpdateDialog.tsx | 4 +- ...emote-server-install-failure-probe.test.ts | 104 ++++++ .../remote-server-install-failure-probe.ts | 36 ++ .../remote-server-restart-wait.test.ts | 178 ++++++++++ .../src/runtime/remote-server-restart-wait.ts | 81 +++++ .../remote-server-update-coordinator.test.ts | 89 +++++ .../remote-server-update-coordinator.ts | 56 ++- .../src/store/slices/remote-server-updates.ts | 9 +- src/shared/app-version.ts | 10 + 13 files changed, 949 insertions(+), 54 deletions(-) create mode 100644 src/main/updater.install-failure-cause.test.ts create mode 100644 src/renderer/src/runtime/remote-server-install-failure-probe.test.ts create mode 100644 src/renderer/src/runtime/remote-server-install-failure-probe.ts create mode 100644 src/renderer/src/runtime/remote-server-restart-wait.test.ts create mode 100644 src/renderer/src/runtime/remote-server-restart-wait.ts 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/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 + ) +} From 2548b816c05a2fe85c12e7f1cd7958512096fab9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:03:40 -0700 Subject: [PATCH 4/4] Keep the app responsive when security software slows process creation (#12217) * fix(ports): keep the app responsive when security software slows process creation Orca ran the workspace port scan's probe commands (lsof/ps on macOS, netstat + powershell.exe on Windows) directly in the Electron main process. libuv performs process creation inline on the calling event loop, which in the main process is the browser UI thread, so an endpoint-security module hooking CreateProcessW froze the whole window for the length of the spawn. The same stall also produced a false diagnosis: the 4s command watchdog was armed before execFile (local-workspace-port-scanner.ts:389 -> :410), so its deadline had already passed by the time the command started. Every scan on a hooked host reported a command timeout, tripping the 60s -> 5min backoff and the "Port scanning is temporarily paused after a command timeout" banner even though the commands themselves were healthy. Probe commands now run on a lazily created, unref'd worker thread with FIFO one-at-a-time dispatch, and the watchdog is armed after execFile returns so it measures the command rather than the spawn. Node's own execFile timeout kill (killed: true) is classified as a command timeout, keeping the backoff working for genuine hangs. A scan that observes a stalled spawn skips its optional metadata commands for that cycle, capping a hooked-host scan at roughly one stall instead of three. Closes #11161 * fix(ports): keep advertised URLs when a stalled spawn skips port metadata Review follow-up on #11161. The stalled-spawn early return handed scanWorkspacePorts raw ports with no cwd/commandLine, so every port failed attribution and reconcileAdvertisedUrls told the watcher each worktree's listeners had vanished. shouldEvictAfterScan then deleted every cached advertised URL and broadcast a removal event; those URLs are only ever captured from live PTY output, so the dev-server link was gone until the server restarted. The scanners now report metadataAvailable, and reconciliation is skipped for a scan that never gathered attribution evidence. The skip is also no longer self-perpetuating: on an EDR-hooked host every spawn stalls, so gating purely on the current scan's spawnMs made every port permanently external (Stop refused with 'Only workspace-owned local processes can be stopped here.'). Metadata is now re-probed on the scan after a skip, matching what the comment and test name already claimed. Co-authored-by: Orca * test(windows): stop a temp-dir lock from failing the CLI launcher smoke test The native launcher assertions passed on windows-latest, but teardown's rmSync raced Windows' release of the image handle on the exe the test had just executed and threw EPERM, failing the job. Cleanup now retries and, on Windows only, tolerates a residual lock code instead of reporting it as a launcher regression. Co-authored-by: Orca * fix(ports): scope the metadata skip away from attribution-dependent scans The metadata skip was a process-wide parity flag, so Stop and the localhost-label allowlist could land on a degraded cycle and reject a port the panel had just shown as workspace-owned. Give those callers an explicit requireMetadata option, and carry the previous cycle's listener metadata forward so a skipped background scan no longer republishes workspace ports as external. Also pin the watchdog ordering: the stall in the execution test was shorter than the watchdog budget, so a watchdog armed before execFile still passed. * build: guard worker-thread entries against electron imports (#11161) Electron's module is not registered on worker threads, so require("electron") throws "Cannot find module 'electron'" inside a main-process worker and kills it at startup (verified on Electron 43.1.0). plain-node-entry-guard covered only forked plain-Node entries, so the five worker entries relied on hand-written "must stay electron-free" comments. The port-scan probe worker is one import away from port-scan-command-client.ts, which deliberately contains require('electron'). A violation there fails closed at runtime while every unit test still passes, because the client's require is try/caught on the main thread. Covers stt-worker, warp-theme-parser-worker, session-scanner-opencode-sqlite-worker-entry, main-thread-hang-watchdog-entry and port-scan-command-worker-entry. The scan is transitive over the emitted chunk graph, so a shared chunk that reaches electron is caught too. Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> * test(windows): retry teardown for main's duplicate-PATH launcher fixture Main's new csc-compiled harness runs an exe from the temp tree, which is exactly the image-handle/AV lock the merged-in removeFixtureTree retry exists for; its bare rmSync would report a teardown lock as a launcher failure. Co-authored-by: Orca * test(ports): pin the packaged-asar worker entry path resolveWorkerEntryPath's packaged branch never runs in dev or e2e, so the path construction had no coverage. Split the electron read out of it and unit-test both layouts. Co-authored-by: Orca --------- Co-authored-by: Orca Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- build-plugins/plain-node-entry-guard.ts | 31 +- config/knip.json | 1 + .../build-windows-cli-launcher.test.mjs | 21 +- config/scripts/plain-node-entry-guard.test.ts | 88 +++++ electron.vite.config.ts | 5 + src/main/ipc/localhost-worktree-labels.ts | 6 +- src/main/ipc/workspace-ports.test.ts | 52 +-- .../local-workspace-port-scanner.test.ts | 298 +++++++++----- .../ports/local-workspace-port-scanner.ts | 235 +++++++---- .../ports/port-scan-command-client.test.ts | 300 +++++++++++++++ src/main/ports/port-scan-command-client.ts | 364 ++++++++++++++++++ .../ports/port-scan-command-execution.test.ts | 97 +++++ src/main/ports/port-scan-command-execution.ts | 115 ++++++ .../port-scan-command-import-boundary.test.ts | 18 + src/main/ports/port-scan-command-protocol.ts | 36 ++ .../ports/port-scan-command-worker-entry.ts | 53 +++ .../ports/workspace-port-ownership.test.ts | 57 +++ src/main/ports/workspace-port-ownership.ts | 11 +- 18 files changed, 1590 insertions(+), 198 deletions(-) create mode 100644 src/main/ports/port-scan-command-client.test.ts create mode 100644 src/main/ports/port-scan-command-client.ts create mode 100644 src/main/ports/port-scan-command-execution.test.ts create mode 100644 src/main/ports/port-scan-command-execution.ts create mode 100644 src/main/ports/port-scan-command-import-boundary.test.ts create mode 100644 src/main/ports/port-scan-command-protocol.ts create mode 100644 src/main/ports/port-scan-command-worker-entry.ts create mode 100644 src/main/ports/workspace-port-ownership.test.ts 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/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/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) }