From e4d95e032ddf13ff73be0813ef43ca2d136dbb03 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 25 Aug 2026 23:15:37 -0700 Subject: [PATCH] fix(windows): restore a CIM fallback for relay hosts with no native binding (#16550) Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> --- docs/reference/windows-process-enumeration.md | 31 ++++++ .../relay-native-dependency-coverage.test.ts | 102 ++++++++++++++++++ src/main/ssh/ssh-relay-deploy.ts | 5 +- .../windows-process-table-cim-scan.test.ts | 66 ++++++++++++ .../windows/windows-process-table-cim-scan.ts | 101 +++++++++++++++++ .../windows/windows-process-table.test.ts | 80 +++++++++++++- src/main/windows/windows-process-table.ts | 33 ++++++ 7 files changed, 415 insertions(+), 3 deletions(-) create mode 100644 src/main/ssh/relay-native-dependency-coverage.test.ts create mode 100644 src/main/windows/windows-process-table-cim-scan.test.ts create mode 100644 src/main/windows/windows-process-table-cim-scan.ts diff --git a/docs/reference/windows-process-enumeration.md b/docs/reference/windows-process-enumeration.md index f34940143b5..5bd023acfcc 100644 --- a/docs/reference/windows-process-enumeration.md +++ b/docs/reference/windows-process-enumeration.md @@ -37,6 +37,37 @@ Measured on Windows 11 with 1050 processes (p50 / p95): | + memory + command line | 30.6 ms | 33.7 ms | | `Get-CimInstance` via PowerShell | 706 ms | 723 ms | +## The relay has no binding, and falls back + +Relay deployment installs only `node-pty` and `@parcel/watcher` on the remote +host (`RELAY_NATIVE_DEPS` in `src/main/ssh/ssh-relay-deploy.ts`), so a Windows +machine used as an SSH host has no `@vscode/windows-process-tree` at all. It is +not added there on purpose: the package ships no prebuilds, so installing it +would put a from-source `node-gyp` build — MSVC, the SDK, and the same +Spectre-mitigated libraries described below — on the critical path of every +Windows relay deploy, where today none is needed. pnpm patches also do not cross +SSH, so the remote would get the unpatched 1024-process cap regardless. + +Instead, `windows-process-table.ts` falls back to +`readWindowsProcessRowsWithCim` (`windows-process-table-cim-scan.ts`), the +`Get-CimInstance` scan this module replaced. The gate is deliberately narrow: + +- it engages **only** when the module cannot be required, never when a loaded + module fails, wedges, or returns an unreadable table — a present-but-failing + reader must not silently start forking a shell at the caller's poll rate; +- a fallback that also fails still rejects, so "unavailable" never degrades into + "nothing is running"; +- the scan applies the same self-presence guard as the native path. + +`src/main/ssh/relay-native-dependency-coverage.test.ts` asserts that every +native addon reachable from the relay entry is either installed on relay hosts +or listed there with the reason its absence is safe. That test exists because +#15749 shipped this gap: the relay tests injected a fake module through +`__setWindowsProcessTreeLoaderForTests`, so nothing exercised the real require. + +The native fast path stays unavailable on relay hosts until the toolchain or a +prebuild story is solved. That is a real gap, tracked separately. + ## Why the package is patched `config/patches/@vscode__windows-process-tree@0.8.0.patch` carries two hunks. diff --git a/src/main/ssh/relay-native-dependency-coverage.test.ts b/src/main/ssh/relay-native-dependency-coverage.test.ts new file mode 100644 index 00000000000..26203869d76 --- /dev/null +++ b/src/main/ssh/relay-native-dependency-coverage.test.ts @@ -0,0 +1,102 @@ +import { existsSync, readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { build } from 'esbuild' +import { describe, expect, it } from 'vitest' +import { RELAY_NATIVE_DEPS } from './ssh-relay-deploy' + +/** + * The relay is a bundle deployed to a host that has none of Orca's node_modules. + * Every native addon it reaches therefore has to be installed by + * `installNativeDeps`, or the relay silently loses that capability forever. + * + * This exists because #15749 moved the Windows process table onto + * `@vscode/windows-process-tree` without adding it here. The relay tests all + * passed: they inject a fake module through + * `__setWindowsProcessTreeLoaderForTests`, so nothing ever exercised the real + * require that the remote host would fail. Assert against the real graph. + */ + +const REPO_ROOT = resolve(import.meta.dirname, '..', '..', '..') + +/** + * Native addons the relay imports but deliberately does NOT install, each with + * the reason its absence is safe. Adding an entry is a decision, not a default: + * anything not listed must appear in RELAY_NATIVE_DEPS. + */ +const DEGRADES_WITHOUT_INSTALL: Record = { + '@vscode/windows-process-tree': + 'Windows-only and ships no prebuilds, so installing it would put a from-source ' + + 'node-gyp build (MSVC + SDK + Spectre-mitigated libs) on the critical path of ' + + 'every Windows relay deploy — and pnpm patches do not cross SSH, so the remote ' + + 'would get the unpatched 1024-process cap anyway. windows-process-table.ts ' + + 'falls back to a Get-CimInstance scan when the binding is absent.' +} + +/** Addons are the packages npm has to build or unpack a binary for. */ +function nativeDependencyNames(): string[] { + const pkg = JSON.parse(readFileSync(join(REPO_ROOT, 'package.json'), 'utf8')) as { + dependencies?: Record + optionalDependencies?: Record + } + const declared = new Set([ + ...Object.keys(pkg.dependencies ?? {}), + ...Object.keys(pkg.optionalDependencies ?? {}) + ]) + return [...declared].filter((name) => { + const dir = join(REPO_ROOT, 'node_modules', name) + return existsSync(join(dir, 'binding.gyp')) || existsSync(join(dir, 'prebuilds')) + }) +} + +/** + * Source files reachable from the relay entry. + * + * Why the metafile and not the emitted bundle: the process table resolves its + * addon through `createRequire`, which esbuild cannot see and minification + * rewrites, so the specifier only survives reliably in the sources. + */ +async function relayReachableSources(): Promise { + const result = await build({ + entryPoints: [join(REPO_ROOT, 'src', 'relay', 'relay.ts')], + bundle: true, + platform: 'node', + target: 'node18', + format: 'cjs', + write: false, + metafile: true, + external: ['node-pty', '@parcel/watcher', 'electron'], + define: { 'process.env.NODE_ENV': '"production"' } + }) + return Object.keys(result.metafile.inputs).filter((input) => !input.includes('node_modules')) +} + +describe('relay native dependency coverage', () => { + it('installs every native addon the relay bundle imports', async () => { + const sources = await relayReachableSources() + const text = sources.map((file) => readFileSync(join(REPO_ROOT, file), 'utf8')).join('\n') + + const imported = nativeDependencyNames().filter( + (name) => text.includes(`'${name}'`) || text.includes(`"${name}"`) + ) + expect(imported.length).toBeGreaterThan(0) + + const uncovered = imported.filter( + (name) => !(name in RELAY_NATIVE_DEPS) && !(name in DEGRADES_WITHOUT_INSTALL) + ) + expect(uncovered).toEqual([]) + }, 60_000) + + it('keeps every installed native dep at the version the app depends on', () => { + const pkg = JSON.parse(readFileSync(join(REPO_ROOT, 'package.json'), 'utf8')) as { + dependencies?: Record + optionalDependencies?: Record + } + const declared = { ...pkg.dependencies, ...pkg.optionalDependencies } + for (const [name, version] of Object.entries(RELAY_NATIVE_DEPS)) { + // The relay pins exact versions where the app carries a range, so compare + // the base: a relay on a different version than the app marshals the same + // node-pty/watcher data through a binding nothing else cross-checks. + expect(declared[name]?.replace(/^[\^~]/, ''), `${name} matches the app`).toBe(version) + } + }) +}) diff --git a/src/main/ssh/ssh-relay-deploy.ts b/src/main/ssh/ssh-relay-deploy.ts index 7590e9817f2..c9d82cbd744 100644 --- a/src/main/ssh/ssh-relay-deploy.ts +++ b/src/main/ssh/ssh-relay-deploy.ts @@ -697,7 +697,10 @@ function uploadStageNamespaceIfSupported( const NODE_PTY_VERSION = '1.1.0' const NODE_PTY_CONSOLE_LIST_PATCH_FILENAME = 'node-pty-1.1.0-console-list-agent-patch.cjs' -const RELAY_NATIVE_DEPS = { +// Exported for the relay-native-dependency-coverage test, which asserts every +// native addon the relay bundle imports is either installed here or explicitly +// declared as degrading without it. +export const RELAY_NATIVE_DEPS = { 'node-pty': NODE_PTY_VERSION, '@parcel/watcher': '2.5.6' } as const diff --git a/src/main/windows/windows-process-table-cim-scan.test.ts b/src/main/windows/windows-process-table-cim-scan.test.ts new file mode 100644 index 00000000000..023096016be --- /dev/null +++ b/src/main/windows/windows-process-table-cim-scan.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from 'vitest' +import { parseWindowsCimProcessRows } from './windows-process-table-cim-scan' + +describe('parseWindowsCimProcessRows', () => { + it('reads the array form ConvertTo-Json emits for a real table', () => { + const stdout = JSON.stringify([ + { CommandLine: 'node relay.js', Name: 'node.exe', ParentProcessId: 4, ProcessId: 100 }, + { CommandLine: 'claude --resume', Name: 'claude.exe', ParentProcessId: 100, ProcessId: 200 } + ]) + expect(parseWindowsCimProcessRows(stdout)).toEqual([ + { pid: 100, ppid: 4, name: 'node.exe', command: 'node relay.js' }, + { pid: 200, ppid: 100, name: 'claude.exe', command: 'claude --resume' } + ]) + }) + + it('reads the bare-object form ConvertTo-Json emits for a single row', () => { + const stdout = JSON.stringify({ + CommandLine: 'node relay.js', + Name: 'node.exe', + ParentProcessId: 4, + ProcessId: 100 + }) + expect(parseWindowsCimProcessRows(stdout)).toEqual([ + { pid: 100, ppid: 4, name: 'node.exe', command: 'node relay.js' } + ]) + }) + + it('falls back to the image name when a process denied its command line', () => { + const stdout = JSON.stringify([ + { CommandLine: null, Name: 'lsass.exe', ParentProcessId: 4, ProcessId: 700 } + ]) + expect(parseWindowsCimProcessRows(stdout)).toEqual([ + { pid: 700, ppid: 4, name: 'lsass.exe', command: 'lsass.exe' } + ]) + }) + + it('keeps a command line containing newlines on its own row', () => { + // The reason this reads JSON and not PowerShell's Key=Value list form. + const stdout = JSON.stringify([ + { + CommandLine: 'app.exe "a\nProcessId=9\nb"', + Name: 'app.exe', + ParentProcessId: 4, + ProcessId: 5 + } + ]) + expect(parseWindowsCimProcessRows(stdout)).toEqual([ + { pid: 5, ppid: 4, name: 'app.exe', command: 'app.exe "a\nProcessId=9\nb"' } + ]) + }) + + it('drops rows with no usable pid rather than inventing one', () => { + const stdout = JSON.stringify([ + { Name: 'ghost.exe', ParentProcessId: 4 }, + { Name: 'real.exe', ParentProcessId: 4, ProcessId: 5 } + ]) + expect(parseWindowsCimProcessRows(stdout)).toEqual([ + { pid: 5, ppid: 4, name: 'real.exe', command: 'real.exe' } + ]) + }) + + it('returns null on unparseable output so the caller can reject it', () => { + // A policy banner or an error on stdout must not read as an idle machine. + expect(parseWindowsCimProcessRows('Access is denied.')).toBeNull() + }) +}) diff --git a/src/main/windows/windows-process-table-cim-scan.ts b/src/main/windows/windows-process-table-cim-scan.ts new file mode 100644 index 00000000000..f638955d327 --- /dev/null +++ b/src/main/windows/windows-process-table-cim-scan.ts @@ -0,0 +1,101 @@ +import { runProcess } from '../../shared/child-process/run-process' +import { windowsPowerShellPath } from '../../shared/child-process/windows-system-binary' +import type { WindowsProcessRow } from './windows-process-table' + +/** + * The `Get-CimInstance Win32_Process` scan, kept only for hosts where the + * native Toolhelp32 binding is not installed. + * + * Why this still exists after #15749 retired it: the relay bundle is deployed to + * SSH hosts that only ever receive `node-pty` and `@parcel/watcher`, so + * `@vscode/windows-process-tree` is absent there and every native read rejects. + * Callers read that as "no evidence" and agent panes identify as powershell.exe + * forever. This restores the v1.4.188 answer on exactly those hosts; the local + * app ships the addon and never reaches this path. + */ + +const WINDOWS_CIM_QUERY_TIMEOUT_MS = 3_000 +const WINDOWS_CIM_MAX_OUTPUT_BYTES = 8 * 1024 * 1024 + +// Why JSON and not the `Key=Value` list form: CommandLine can itself contain +// CR/LF, so an argument could otherwise masquerade as another row's field. +const POWERSHELL_PROCESS_QUERY = + '[Console]::OutputEncoding = [System.Text.Encoding]::UTF8; ' + + 'Get-CimInstance -ClassName Win32_Process -Property CommandLine,Name,ParentProcessId,ProcessId | ' + + 'Select-Object CommandLine,Name,ParentProcessId,ProcessId | ' + + 'ConvertTo-Json -Compress' + +type CimProcessRow = { + CommandLine?: unknown + Name?: unknown + ParentProcessId?: unknown + ProcessId?: unknown +} + +function fieldAsString(value: unknown): string { + if (typeof value === 'string') { + return value + } + return value === null || value === undefined ? '' : String(value) +} + +function fieldAsNumber(value: unknown): number { + if (typeof value === 'number') { + return value + } + return typeof value === 'string' ? Number.parseInt(value, 10) : Number.NaN +} + +export function parseWindowsCimProcessRows(stdout: string): WindowsProcessRow[] | null { + const trimmed = stdout.trim() + if (!trimmed) { + return [] + } + let parsed: unknown + try { + parsed = JSON.parse(trimmed) + } catch { + return null + } + const items = Array.isArray(parsed) ? parsed : [parsed] + return items.flatMap((item) => { + if (!item || typeof item !== 'object') { + return [] + } + const row = item as CimProcessRow + const pid = fieldAsNumber(row.ProcessId) + const ppid = fieldAsNumber(row.ParentProcessId) + if (!Number.isFinite(pid) || !Number.isFinite(ppid)) { + return [] + } + const name = fieldAsString(row.Name) + // memoryBytes stays undefined: Win32_Process reports WorkingSetSize, but no + // caller reads it off this table and asking widens an already costly scan. + return [{ pid, ppid, name, command: fieldAsString(row.CommandLine) || name }] + }) +} + +/** + * Read the whole process table through PowerShell. + * + * Throws rather than returning `[]` on any failure: an empty table is a claim + * that nothing is running, and callers act on that by declaring a tree dead. + */ +export async function readWindowsProcessRowsWithCim(): Promise { + const result = await runProcess({ + program: windowsPowerShellPath(), + args: ['-NoProfile', '-NonInteractive', '-Command', POWERSHELL_PROCESS_QUERY], + timeoutMs: WINDOWS_CIM_QUERY_TIMEOUT_MS, + maxOutputBytes: WINDOWS_CIM_MAX_OUTPUT_BYTES + }) + if (result.timedOut || result.code !== 0) { + throw new Error( + `windows process table CIM scan failed (code=${result.code} timedOut=${result.timedOut})` + ) + } + const rows = parseWindowsCimProcessRows(result.stdout) + if (!rows || rows.length === 0) { + throw new Error('windows process table CIM scan returned no rows') + } + return rows +} diff --git a/src/main/windows/windows-process-table.test.ts b/src/main/windows/windows-process-table.test.ts index e22f1dae9d9..64f4aeb2483 100644 --- a/src/main/windows/windows-process-table.test.ts +++ b/src/main/windows/windows-process-table.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { + __setWindowsProcessTableCimScanForTests, __setWindowsProcessTreeLoaderForTests, isWindowsProcessTableAvailable, readWindowsProcessTable, @@ -66,9 +67,13 @@ describe('windows process table', () => { it('rejects rather than reporting an empty machine when the module is absent', async () => { // A caller that reads "no processes" acts on it -- by declaring a tree dead, - // or by concluding a shell has no children. Absence must not look like that. + // or by concluding a shell has no children. Absence must not look like that, + // and neither must a fallback that also fails. __setWindowsProcessTreeLoaderForTests(() => null) - await expect(readWindowsProcessTableFresh()).rejects.toThrow(/unavailable/) + __setWindowsProcessTableCimScanForTests(async () => { + throw new Error('powershell unavailable') + }) + await expect(readWindowsProcessTableFresh()).rejects.toThrow(/powershell unavailable/) expect(isWindowsProcessTableAvailable()).toBe(false) }) @@ -107,6 +112,77 @@ describe('windows process table', () => { }) }) +// Why this path exists: relay deployment installs only node-pty and +// @parcel/watcher, so a Windows SSH host has no native binding and every read +// used to reject -- which agent recognition reads as "no evidence" forever. +describe('PowerShell fallback when the native binding is absent', () => { + let platform: PropertyDescriptor | undefined + const cimScan = vi.fn() + const CIM_ROWS = [ + { pid: process.pid, ppid: 0, name: 'node.exe', command: 'node relay.js' }, + { pid: 200, ppid: process.pid, name: 'claude.exe', command: 'claude --resume' } + ] + + beforeEach(() => { + platform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + cimScan.mockReset() + cimScan.mockResolvedValue(CIM_ROWS) + __setWindowsProcessTableCimScanForTests(cimScan) + }) + + afterEach(() => { + __setWindowsProcessTableCimScanForTests() + __setWindowsProcessTreeLoaderForTests() + if (platform) { + Object.defineProperty(process, 'platform', platform) + } + }) + + it('engages when the module cannot be required', async () => { + __setWindowsProcessTreeLoaderForTests(() => null) + await expect(readWindowsProcessTableFresh()).resolves.toEqual(CIM_ROWS) + expect(cimScan).toHaveBeenCalledTimes(1) + }) + + it('does not engage when the native binding is present', async () => { + const getAllProcesses = vi.fn() + getAllProcesses.mockImplementation((cb: (rows: unknown) => void) => cb(NATIVE)) + __setWindowsProcessTreeLoaderForTests(() => ({ + ProcessDataFlag: { None: 0, Memory: 1, CommandLine: 2 }, + getAllProcesses + })) + await readWindowsProcessTableFresh() + expect(cimScan).not.toHaveBeenCalled() + }) + + it('does not engage when a present binding fails its read', async () => { + // A wedged or blocked reader must not silently start forking a shell at the + // caller's poll rate; only absence is unrecoverable. + const getAllProcesses = vi.fn() + getAllProcesses.mockImplementation((cb: (rows: unknown) => void) => cb([])) + __setWindowsProcessTreeLoaderForTests(() => ({ + ProcessDataFlag: { None: 0, Memory: 1, CommandLine: 2 }, + getAllProcesses + })) + await expect(readWindowsProcessTableFresh()).rejects.toThrow(/unreadable/) + expect(cimScan).not.toHaveBeenCalled() + }) + + it('rejects a scan missing our own pid instead of reporting an idle machine', async () => { + __setWindowsProcessTreeLoaderForTests(() => null) + cimScan.mockResolvedValue([{ pid: 200, ppid: 4, name: 'claude.exe', command: 'claude' }]) + await expect(readWindowsProcessTableFresh()).rejects.toThrow(/unreadable/) + }) + + it('stays off Windows-only: darwin still reports unavailable', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'darwin' }) + __setWindowsProcessTreeLoaderForTests(() => null) + await expect(readWindowsProcessTableFresh()).rejects.toThrow(/unavailable/) + expect(cimScan).not.toHaveBeenCalled() + }) +}) + describe('wedge cooldown', () => { let platform: PropertyDescriptor | undefined diff --git a/src/main/windows/windows-process-table.ts b/src/main/windows/windows-process-table.ts index 6ad8a774ec8..b1bc58d3b29 100644 --- a/src/main/windows/windows-process-table.ts +++ b/src/main/windows/windows-process-table.ts @@ -1,5 +1,6 @@ import { createRequire } from 'node:module' import { createProcessTableSnapshotReader } from '../../shared/process-table-snapshot' +import { readWindowsProcessRowsWithCim } from './windows-process-table-cim-scan' /** * The only place Orca reads the Windows process table. @@ -54,6 +55,7 @@ const requireFromMain = createRequire(__filename) let cachedModule: WindowsProcessTreeModule | null | undefined let moduleLoader: () => WindowsProcessTreeModule | null = loadWindowsProcessTree +let cimScan: () => Promise = readWindowsProcessRowsWithCim /** * Resolve the native module, or null where it cannot be used. @@ -110,6 +112,14 @@ let readGeneration = 0 function readNativeRows(): Promise { const native = moduleLoader() if (!native) { + if (process.platform === 'win32') { + // Why only when the module is absent: a binding that loads is the fast + // path even when a read fails or wedges, so a failing native reader must + // never silently start forking shells at the caller's poll rate. Absence + // is the one condition that can never resolve itself — see + // docs/reference/windows-process-enumeration.md. + return readCimRows() + } // Reject rather than resolve empty: an empty table is a claim that nothing // is running, and callers act on that by force-killing or by declaring a // tree dead. "Unavailable" has to stay distinguishable from "empty". @@ -186,6 +196,21 @@ function readNativeRows(): Promise { }) } +/** + * Whole-table read for hosts with no native binding (the relay). + * + * Applies the same self-presence guard as the native path: a scan that omits + * our own pid is truncated or permission-filtered, not empty, and must reject + * so nothing downstream reads it as proof a process died. + */ +async function readCimRows(): Promise { + const rows = await cimScan() + if (!rows.some((row) => row.pid === process.pid)) { + throw new Error('windows process table is unreadable') + } + return rows +} + // Why still cache: the snapshot is cheap but not free, and a worktree delete // tears down PTYs 32-wide. The shared TTL + single-in-flight reader collapses // that burst into one scan, exactly as the PowerShell path had to. @@ -230,6 +255,14 @@ export function __setWindowsProcessTreeLoaderForTests( snapshotReader.reset() } +/** Test-only: substitute the no-binding PowerShell scan, which spawns a child. */ +export function __setWindowsProcessTableCimScanForTests( + scan?: () => Promise +): void { + cimScan = scan ?? readWindowsProcessRowsWithCim + snapshotReader.reset() +} + /** Test-only: drop the shared snapshot so suites cannot serve each other's rows. */ export function resetWindowsProcessTableForTests(): void { snapshotReader.reset()