From 608d0b5c646188f48e5ab8159ad8c77f97e65afc Mon Sep 17 00:00:00 2001 From: Christof Marti Date: Mon, 21 Sep 2026 18:37:08 +0200 Subject: [PATCH 1/2] test: clean up Windows agent-host descendants after shutdown Record the test server's descendants before graceful shutdown and await cleanup of survivors before removing temporary directories. Keep product provider shutdown and timeout budgets unchanged. Add a Windows regression proving that a clean parent exit does not imply descendant exit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../agentHost/test/node/e2e/README.md | 2 + .../node/serverIntegrationTestHelpers.test.ts | 84 +++++++++++++++++++ .../test/node/serverIntegrationTestHelpers.ts | 35 +++++++- 3 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts diff --git a/src/vs/platform/agentHost/test/node/e2e/README.md b/src/vs/platform/agentHost/test/node/e2e/README.md index e82eccd184b635..1ce643aabff94d 100644 --- a/src/vs/platform/agentHost/test/node/e2e/README.md +++ b/src/vs/platform/agentHost/test/node/e2e/README.md @@ -226,6 +226,8 @@ Each test needs an agent host server (a forked subprocess) fronted by a `CapiRep The lease also owns a fresh suite data directory. Every server it starts uses that directory as its home and VS Code user-data directory and prevents provider-specific config overrides from escaping it, so both shared and provider-specific scenarios are isolated from developer-machine configuration. +On Windows, test-server cleanup records descendants before requesting graceful shutdown and terminates any survivors after the server exits, before temporary directories are removed. + - **Per-test** (always while recording) — fork a fresh server + proxy for every test and kill it in teardown. Full isolation: nothing carries over between tests. The cost is that every test re-pays the server fork **and** the provider SDK/CLI cold start (`_ensureClient` spawns and caches the CLI subprocess per server). - **Shared** (the default in replay, for every provider) — reuse a server + proxy across tests, swapping the per-test fixture and reconnecting a fresh client. The lease recycles after 25 model-backed tests or 40 total tests, whichever comes first. The model cap bounds provider-process load; the total cap bounds host-owned terminals, watchers, subscriptions, and other resource accumulation in host-only suites. diff --git a/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts new file mode 100644 index 00000000000000..9abe123fa4d6ed --- /dev/null +++ b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts @@ -0,0 +1,84 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { spawn } from 'child_process'; +import { once } from 'events'; +import { mkdtemp, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { Promises, raceTimeout } from '../../../../base/common/async.js'; +import { getErrorCode } from '../../../../base/common/errors.js'; +import { join } from '../../../../base/common/path.js'; +import { isWindows } from '../../../../base/common/platform.js'; +import { killTree } from '../../../../base/node/processes.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { stopServer } from './serverIntegrationTestHelpers.js'; + +suite('Agent Host test server cleanup', () => { + ensureNoDisposablesAreLeakedInTestSuite(); + + (isWindows ? test : test.skip)('stops owned descendants after the server exits gracefully', async function () { + this.timeout(30_000); + const directory = await mkdtemp(join(tmpdir(), 'vscode-test-server-cleanup-')); + const descendantCode = ` + require('fs').writeFileSync('owned.txt', String(process.pid)); + process.send(process.pid); + process.disconnect(); + setTimeout(() => process.exit(99), 30000); + `; + const server = spawn(process.execPath, ['-e', ` + const { spawn } = require('child_process'); + const holder = spawn(process.execPath, ['-e', ${JSON.stringify(descendantCode)}], { + detached: true, + windowsHide: true, + stdio: ['ignore', 'ignore', 'ignore', 'ipc'], + env: process.env, + }); + holder.once('message', pid => { + holder.unref(); + process.send(pid); + }); + process.stdin.resume(); + process.stdin.once('end', () => process.exit(0)); + setTimeout(() => process.exit(99), 30000); + `], { + cwd: directory, + env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' }, + stdio: ['pipe', 'pipe', 'pipe', 'ipc'], + windowsHide: true, + }); + let stderr = ''; + server.stderr?.on('data', chunk => stderr += chunk.toString()); + let descendantPid: number | undefined; + try { + const ready = await raceTimeout(once(server, 'message'), 5_000); + assert.ok(ready, `Descendant did not start: ${stderr}`); + const message: unknown = ready[0]; + assert.ok(typeof message === 'number'); + descendantPid = message; + await stopServer({ process: server, port: 0 }); + + assert.strictEqual(server.exitCode, 0); + assert.throws(() => process.kill(message, 0), { code: 'ESRCH' }); + await rm(directory, { recursive: true }); + } finally { + await Promises.settled([server.pid, descendantPid].map(async pid => { + if (pid === undefined) { + return; + } + try { + process.kill(pid, 0); + } catch (error) { + if (getErrorCode(error) === 'ESRCH') { + return; + } + throw error; + } + await killTree(pid, true); + })); + await rm(directory, { recursive: true, force: true }); + } + }); +}); diff --git a/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts index 44a2fc6bb5283f..0aac6451f6f6c6 100644 --- a/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts +++ b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts @@ -5,11 +5,13 @@ import { ChildProcess, fork } from 'child_process'; import { cp, lstat, mkdir, readFile, readdir, realpath, rename, rm, stat, writeFile } from 'fs/promises'; -import { raceTimeout } from '../../../../base/common/async.js'; +import { Promises, raceTimeout } from '../../../../base/common/async.js'; +import { getErrorCode } from '../../../../base/common/errors.js'; import { Schemas } from '../../../../base/common/network.js'; import { createRequire } from 'module'; import { mkdirSync } from 'fs'; import { userInfo } from 'os'; +import { promisify } from 'util'; import { fileURLToPath } from 'url'; import { WebSocket } from 'ws'; import { CapiReplayProxy, type CapiReplayMode, type ICapiReplayResponse } from './e2e/harness/capiReplayProxy.js'; @@ -677,6 +679,18 @@ export async function stopServer(server: IServerHandle | undefined): Promise process.pid !== serverProcess.pid).map(process => process.pid); + } + } catch (error) { + snapshotError = new Error('Failed to capture Agent Host test server descendants', { cause: error }); + } serverProcess.stdin?.end(); if (!await raceTimeout(serverExit.then(() => true), SERVER_SHUTDOWN_TIMEOUT_MS)) { try { @@ -694,6 +708,25 @@ export async function stopServer(server: IServerHandle | undefined): Promise { + try { + await killTree(pid, true); + } catch (error) { + try { + process.kill(pid, 0); + } catch (probeError) { + if (getErrorCode(probeError) === 'ESRCH') { + return; // The descendant already exited during graceful shutdown. + } + throw probeError; + } + throw error; + } + })); } /** Forcefully kill an Agent Host test server and its child processes without graceful shutdown. */ From 62e8628b8fede783704645bb7a5f842edc8bca8a Mon Sep 17 00:00:00 2001 From: Christof Marti Date: Mon, 21 Sep 2026 20:53:47 +0200 Subject: [PATCH 2/2] test: bound descendant snapshots by the shutdown deadline Share the existing shutdown budget between descendant enumeration and graceful exit. Preserve EOF and forced cleanup when enumeration stalls, report the timeout afterward, and cover that path with a real-process regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../agentHost/test/node/e2e/README.md | 2 +- .../node/serverIntegrationTestHelpers.test.ts | 42 ++++++++++++++++++- .../test/node/serverIntegrationTestHelpers.ts | 25 +++++++---- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/src/vs/platform/agentHost/test/node/e2e/README.md b/src/vs/platform/agentHost/test/node/e2e/README.md index 1ce643aabff94d..c544af5b40780c 100644 --- a/src/vs/platform/agentHost/test/node/e2e/README.md +++ b/src/vs/platform/agentHost/test/node/e2e/README.md @@ -226,7 +226,7 @@ Each test needs an agent host server (a forked subprocess) fronted by a `CapiRep The lease also owns a fresh suite data directory. Every server it starts uses that directory as its home and VS Code user-data directory and prevents provider-specific config overrides from escaping it, so both shared and provider-specific scenarios are isolated from developer-machine configuration. -On Windows, test-server cleanup records descendants before requesting graceful shutdown and terminates any survivors after the server exits, before temporary directories are removed. +On Windows, test-server cleanup records descendants before requesting graceful shutdown and terminates any survivors after the server exits, before temporary directories are removed. Recording descendants and waiting for graceful exit share the existing shutdown deadline. - **Per-test** (always while recording) — fork a fresh server + proxy for every test and kill it in teardown. Full isolation: nothing carries over between tests. The cost is that every test re-pays the server fork **and** the provider SDK/CLI cold start (`_ensureClient` spawns and caches the CLI subprocess per server). diff --git a/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts index 9abe123fa4d6ed..b63cc2e3bc7e52 100644 --- a/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts +++ b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.test.ts @@ -8,17 +8,55 @@ import { spawn } from 'child_process'; import { once } from 'events'; import { mkdtemp, rm } from 'fs/promises'; import { tmpdir } from 'os'; -import { Promises, raceTimeout } from '../../../../base/common/async.js'; +import { DeferredPromise, Promises, raceTimeout } from '../../../../base/common/async.js'; import { getErrorCode } from '../../../../base/common/errors.js'; import { join } from '../../../../base/common/path.js'; import { isWindows } from '../../../../base/common/platform.js'; import { killTree } from '../../../../base/node/processes.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { stopServer } from './serverIntegrationTestHelpers.js'; +import { killServer, stopServer } from './serverIntegrationTestHelpers.js'; suite('Agent Host test server cleanup', () => { ensureNoDisposablesAreLeakedInTestSuite(); + test('a stalled descendant snapshot still sends EOF and reaches forced shutdown', async function () { + this.timeout(15_000); + const server = spawn(process.execPath, ['-e', ` + process.stdin.resume(); + process.stdout.write('ready'); + setTimeout(() => process.exit(99), 30000); + `], { + env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' }, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true, + }); + const snapshot = new DeferredPromise(); + let stopped: Promise | undefined; + try { + assert.ok(await raceTimeout(once(server.stdout, 'data'), 5_000), 'Server did not start'); + stopped = stopServer({ process: server, port: 0 }, () => snapshot.p, 0).then( + () => undefined, + (error: Error) => error, + ); + const error = await raceTimeout(stopped, 5_000); + assert.deepStrictEqual({ + message: error?.message, + cause: error?.cause instanceof Error ? error.cause.message : undefined, + eof: server.stdin.writableEnded, + exited: server.exitCode !== null || server.signalCode !== null, + }, { + message: 'Failed to capture Agent Host test server descendants', + cause: 'Timed out capturing Agent Host test server descendants', + eof: true, + exited: true, + }); + } finally { + snapshot.complete([]); + await stopped; + await killServer({ process: server, port: 0 }); + } + }); + (isWindows ? test : test.skip)('stops owned descendants after the server exits gracefully', async function () { this.timeout(30_000); const directory = await mkdtemp(join(tmpdir(), 'vscode-test-server-cleanup-')); diff --git a/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts index 0aac6451f6f6c6..6d87b27fbff51a 100644 --- a/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts +++ b/src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts @@ -664,13 +664,23 @@ export interface IServerHandle { const SERVER_SHUTDOWN_TIMEOUT_MS = isCI || isWindows || AGENT_HOST_E2E_COVERAGE ? 30_000 : 5_000; +async function getServerDescendants(pid: number): Promise { + if (!isWindows) { + return []; + } + // Once the parent exits, taskkill /T can no longer discover its descendants. + const { getProcessList } = await import('@vscode/windows-process-tree'); + return (await promisify(getProcessList)(pid)).filter(process => process.pid !== pid).map(process => process.pid); +} + /** Gracefully stop an Agent Host test server, killing it if shutdown stalls. */ -export async function stopServer(server: IServerHandle | undefined): Promise { +export async function stopServer(server: IServerHandle | undefined, getDescendants = getServerDescendants, timeoutMs = SERVER_SHUTDOWN_TIMEOUT_MS): Promise { const serverProcess = server?.process; if (!serverProcess || serverProcess.exitCode !== null || serverProcess.signalCode !== null) { return; } + const deadline = Date.now() + timeoutMs; const serverExit = new Promise(resolve => { const onExit = () => resolve(); serverProcess.once('exit', onExit); @@ -682,17 +692,18 @@ export async function stopServer(server: IServerHandle | undefined): Promise process.pid !== serverProcess.pid).map(process => process.pid); + if (serverProcess.pid !== undefined) { + const snapshot = await raceTimeout(getDescendants(serverProcess.pid), Math.max(0, deadline - Date.now())); + if (snapshot === undefined) { + throw new Error('Timed out capturing Agent Host test server descendants'); + } + descendants = snapshot; } } catch (error) { snapshotError = new Error('Failed to capture Agent Host test server descendants', { cause: error }); } serverProcess.stdin?.end(); - if (!await raceTimeout(serverExit.then(() => true), SERVER_SHUTDOWN_TIMEOUT_MS)) { + if (!await raceTimeout(serverExit.then(() => true), Math.max(0, deadline - Date.now()))) { try { if (serverProcess.exitCode === null && serverProcess.signalCode === null) { const pid = serverProcess.pid;