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] 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 a7ad171a4..13499118b 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 a48793b79..cb929266f 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 6e4803ed7..4bb219a56 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 a7583e1d4..b9601a67d 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 39f923cb2..a6e656ac9 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 382072f6c..77c3c3d65 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 841c04609..f5afe0362 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 a6eeb21e5..132eba195 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 1d8f4dbf8..ce2b7b768 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 000000000..d1a15d297 --- /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 000000000..9748a8cfc --- /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 000000000..15e97d4a3 --- /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 000000000..9c63fc9aa --- /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 000000000..bd9b9d05f --- /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 000000000..02beeb982 --- /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 000000000..b5cb5ad5a --- /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 000000000..b367fcbf2 --- /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 9ae5bf658..9f7df2465 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) }