From c3f790025b219a32ebf86c7d5dea403c20b7e583 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:32:35 -0700 Subject: [PATCH] perf(windows): dedupe per-pane process-table scans in agent inspection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows agent foreground-process inspection forks a whole-process-table PowerShell/CIM scan per pane on the same 750ms/2000ms cadence the POSIX path uses. The POSIX side routes through getProcessTableSnapshot (500ms TTL + single in-flight, #6288/#6667), collapsing N concurrent panes to ~2 scans/sec. The Windows path (queryWindowsProcessDescendants) had no such dedup: K concurrent agent panes forked K powershell.exe cold-starts, each enumerating the ENTIRE process table then filtering per-pid in JS — ~10-40x heavier than `ps` (a powershell cold start is ~150-400ms CPU + tens of MB RSS). The degraded/local PTY provider path calls it with no per-pane throttle at all. This is the Windows analogue of the idle-CPU churn #6288 fixed for POSIX. Generalize the existing createProcessTableSnapshotReader factory to be generic over its scan result (default T = string, so the POSIX path and its test are byte-identical) and add a Windows singleton reader that caches parsed WindowsProcessRow[]. queryWindowsProcessDescendants now reads the shared snapshot and runs its own descendant walk; runWindowsProcessRows throws on total enumeration failure so the miss is not cached and the prior null-fallback contract (callers fall through to node-pty's name) is preserved. Windows scan-volume regression test (mirrors the POSIX #6288 guard) drives PANE_COUNT concurrent panes over the cadence window and asserts powershell.exe spawns are bounded by ticks, not pane count, while every pane still resolves its descendant. Reverting the dedup fails both cases. POSIX snapshot + volume tests unchanged and green; node/web/cli typecheck clean. Co-authored-by: Orca --- ...ent-foreground-process-scan-volume.test.ts | 127 ++++++++++++++++++ .../windows-foreground-process-rows.ts | 43 +++++- src/shared/process-table-snapshot.ts | 34 ++--- 3 files changed, 186 insertions(+), 18 deletions(-) create mode 100644 src/main/providers/windows-agent-foreground-process-scan-volume.test.ts diff --git a/src/main/providers/windows-agent-foreground-process-scan-volume.test.ts b/src/main/providers/windows-agent-foreground-process-scan-volume.test.ts new file mode 100644 index 00000000000..28a97aa5437 --- /dev/null +++ b/src/main/providers/windows-agent-foreground-process-scan-volume.test.ts @@ -0,0 +1,127 @@ +// Regression guard: bound the volume of full-process-table PowerShell/CIM scans +// driven by Windows agent foreground-process inspection — the Windows analogue of +// issue #6288 (POSIX `ps`). +// +// Drives queryWindowsProcessDescendants across several concurrently-inspecting +// agent panes on the agent-completion cadence (ACTIVE_POLL_INTERVAL_MS = 750ms) +// and counts how many powershell.exe process-table scans actually spawn. Pre-fix +// the call site forked one powershell.exe per pane per tick; with the shared +// snapshot cache the scans collapse to ~one per tick regardless of pane count, +// while each pane still resolves the same descendant set. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { execFileMock, powershellScanCount } = vi.hoisted(() => ({ + execFileMock: vi.fn(), + powershellScanCount: { value: 0 } +})) + +vi.mock('child_process', () => ({ execFile: execFileMock })) + +import { + queryWindowsProcessDescendants, + resetWindowsProcessRowsSnapshotForTests +} from './windows-foreground-process-rows' + +const ACTIVE_POLL_INTERVAL_MS = 750 +const PANE_COUNT = 6 +const WINDOW_SECONDS = 30 +const TICKS = Math.floor((WINDOW_SECONDS * 1000) / ACTIVE_POLL_INTERVAL_MS) + +const shellPid = (pane: number): number => 100 + pane * 1000 + +// A real CIM query returns the whole system, so one shared snapshot must contain +// every pane's shell + foreground node/codex child. Each pane resolves its own +// descendant from the single scan. +const PROCESS_TABLE_JSON = JSON.stringify( + Array.from({ length: PANE_COUNT }, (_, pane) => { + const shell = shellPid(pane) + return [ + { + ProcessId: shell, + ParentProcessId: 99, + Name: 'cmd.exe', + CommandLine: 'cmd.exe', + ExecutablePath: 'C:/Windows/System32/cmd.exe' + }, + { + ProcessId: shell + 1, + ParentProcessId: shell, + Name: 'node.exe', + CommandLine: 'node C:/Users/dev/AppData/codex/bin/codex.js', + ExecutablePath: 'C:/Program Files/nodejs/node.exe' + } + ] + }).flat() +) + +function installCountingPowerShellMock(): void { + execFileMock.mockImplementation((cmd: string, _args: unknown, _opts: unknown, cb: unknown) => { + const callback = cb as (err: unknown, result: { stdout: string; stderr: string }) => void + if (cmd === 'powershell.exe') { + powershellScanCount.value += 1 + } + callback(null, { stdout: PROCESS_TABLE_JSON, stderr: '' }) + }) +} + +describe('windows agent foreground inspection powershell-scan volume', () => { + let platform: PropertyDescriptor | undefined + + beforeEach(() => { + execFileMock.mockReset() + resetWindowsProcessRowsSnapshotForTests() + powershellScanCount.value = 0 + platform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + vi.useFakeTimers({ toFake: ['Date'] }) + vi.setSystemTime(0) + }) + + afterEach(() => { + vi.useRealTimers() + if (platform) { + Object.defineProperty(process, 'platform', platform) + } + }) + + it('bounds powershell scans by poll ticks, not by pane count, while resolving every pane', async () => { + installCountingPowerShellMock() + + for (let tick = 0; tick < TICKS; tick++) { + vi.setSystemTime(tick * ACTIVE_POLL_INTERVAL_MS) + // All panes inspect concurrently within the tick (worst case). + const resolved = await Promise.all( + Array.from({ length: PANE_COUNT }, (_, pane) => + queryWindowsProcessDescendants(shellPid(pane)) + ) + ) + // Caching must not change the answer: every pane still finds its foreground + // node child as the sole descendant of its shell. + for (let pane = 0; pane < PANE_COUNT; pane++) { + const candidates = resolved[pane] + expect(candidates).not.toBeNull() + expect(candidates).toHaveLength(1) + expect(candidates?.[0]?.pid).toBe(shellPid(pane) + 1) + } + } + + const totalInspections = PANE_COUNT * TICKS + // Pre-fix this equals totalInspections (one powershell.exe per inspection). + // With the shared cache, concurrent panes within a tick share one scan and + // the 500ms TTL forces a fresh scan each new 750ms tick -> ~one per tick. + expect(powershellScanCount.value).toBeLessThanOrEqual(TICKS + 1) + expect(powershellScanCount.value).toBeLessThan(totalInspections / 2) + }) + + it('collapses a burst of concurrent panes into a single scan', async () => { + installCountingPowerShellMock() + + await Promise.all( + Array.from({ length: PANE_COUNT }, (_, pane) => + queryWindowsProcessDescendants(shellPid(pane)) + ) + ) + + expect(powershellScanCount.value).toBe(1) + }) +}) diff --git a/src/main/providers/windows-foreground-process-rows.ts b/src/main/providers/windows-foreground-process-rows.ts index d19d6bf3ebd..d26067122a5 100644 --- a/src/main/providers/windows-foreground-process-rows.ts +++ b/src/main/providers/windows-foreground-process-rows.ts @@ -1,5 +1,6 @@ import { execFile } from 'node:child_process' import { promisify } from 'node:util' +import { createProcessTableSnapshotReader } from '../../shared/process-table-snapshot' const execFileAsync = promisify(execFile) const WINDOWS_PROCESS_QUERY_TIMEOUT_MS = 3_000 @@ -22,12 +23,48 @@ export type WindowsProcessRow = { export type WindowsProcessCandidate = WindowsProcessRow & { depth: number } +// Why: agent foreground inspection forks a whole-process-table PowerShell/CIM +// scan per pane on the same 750ms/2000ms cadence as the POSIX `ps` path. Without +// dedup, K concurrent agent panes fork K powershell.exe cold-starts, each ~10-40x +// heavier than `ps` — the Windows analogue of the idle-CPU churn #6288/#6667 fixed +// for POSIX. Reuse the same TTL + single-in-flight reader, caching parsed rows so +// a burst of panes collapses to ~2 scans/sec; every caller runs its own descendant +// walk over the shared snapshot. +async function runWindowsProcessRows(): Promise { + const rows = + (await queryWindowsProcessesWithPowerShell()) ?? (await queryWindowsProcessesWithWmic()) + if (!rows) { + // Reject so the reader does not cache the miss; callers fall through to + // node-pty's process name (the prior null-return contract is preserved by + // queryWindowsProcessDescendants catching this). + throw new Error('windows process enumeration unavailable') + } + return rows +} + +const windowsProcessRowsReader = createProcessTableSnapshotReader({ + runPs: runWindowsProcessRows, + now: () => Date.now() +}) + export async function queryWindowsProcessDescendants( rootPid: number ): Promise { - const rows = - (await queryWindowsProcessesWithPowerShell()) ?? (await queryWindowsProcessesWithWmic()) - return rows ? collectDescendants(rows, rootPid).sort((a, b) => b.depth - a.depth) : null + let rows: WindowsProcessRow[] + try { + rows = await windowsProcessRowsReader.getSnapshot() + } catch { + return null + } + return collectDescendants(rows, rootPid).sort((a, b) => b.depth - a.depth) +} + +/** + * Test-only: clear the shared Windows process-table snapshot so suites that mock + * execFile between cases don't get one case's rows served to the next within TTL. + */ +export function resetWindowsProcessRowsSnapshotForTests(): void { + windowsProcessRowsReader.reset() } function parseWindowsProcessValueRows(stdout: string): WindowsProcessRow[] { diff --git a/src/shared/process-table-snapshot.ts b/src/shared/process-table-snapshot.ts index 7234cd84333..358cb39a650 100644 --- a/src/shared/process-table-snapshot.ts +++ b/src/shared/process-table-snapshot.ts @@ -21,31 +21,35 @@ const PS_TIMEOUT_MS = 3000 // identically to a fresh fork. const DEFAULT_SNAPSHOT_TTL_MS = 500 -type Snapshot = { stdout: string; capturedAtMs: number } +type Snapshot = { value: T; capturedAtMs: number } -type ProcessTableSnapshotReaderDeps = { - runPs: () => Promise +type ProcessTableSnapshotReaderDeps = { + runPs: () => Promise now: () => number ttlMs?: number } /** * Build a process-table snapshot reader that deduplicates concurrent and - * near-simultaneous `ps` scans behind a single in-flight promise + short TTL. + * near-simultaneous scans behind a single in-flight promise + short TTL. * Exposed as a factory so tests can inject the scan and clock; production code - * uses the shared `getProcessTableSnapshot` instance below. + * uses the shared `getProcessTableSnapshot` instance below. Generic over the + * scan result so the Windows path can cache parsed rows while POSIX caches the + * raw `ps` stdout string (the default). */ -export function createProcessTableSnapshotReader(deps: ProcessTableSnapshotReaderDeps): { - getSnapshot: () => Promise +export function createProcessTableSnapshotReader( + deps: ProcessTableSnapshotReaderDeps +): { + getSnapshot: () => Promise reset: () => void } { const ttlMs = deps.ttlMs ?? DEFAULT_SNAPSHOT_TTL_MS - let cached: Snapshot | null = null - let inFlight: Promise | null = null + let cached: Snapshot | null = null + let inFlight: Promise | null = null - async function getSnapshot(): Promise { + async function getSnapshot(): Promise { if (cached && deps.now() - cached.capturedAtMs < ttlMs) { - return cached.stdout + return cached.value } if (inFlight) { return inFlight @@ -53,11 +57,11 @@ export function createProcessTableSnapshotReader(deps: ProcessTableSnapshotReade const promise = deps.runPs() inFlight = promise try { - const stdout = await promise - // Why: stamp capture time AFTER the scan returns so a slow `ps` can't + const value = await promise + // Why: stamp capture time AFTER the scan returns so a slow scan can't // hand back a snapshot that is already older than its TTL. - cached = { stdout, capturedAtMs: deps.now() } - return stdout + cached = { value, capturedAtMs: deps.now() } + return value } finally { // Clear in-flight on success and failure so a transient `ps` error // (timeout, nonzero exit) retries on the next call instead of being