From 89305ef67758a22fbca190f46ce641f051961c6c Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 15:16:16 -0700 Subject: [PATCH 1/5] Fix: allocate integration test-server ports from the OS and attach child output to start-up failures The windows-latest validation leg failed intermittently from two unrelated causes. BaseTestServer guessed its port as 40000 + random(10000) and passed the guess to the spawned CLI as an explicit --port. Nothing checked the guess was bindable, so it could already be held by another concurrent vitest worker or fall inside a TCP range Windows reserves for Hyper-V/WinNAT; a failed bind is fatal to the child, which is what 'CLI exited prematurely with code 1' was. reserveFreePort() now binds port 0, reads the assignment back and releases it before the child is spawned, so the port is one the OS just confirmed free. The rejected Error carried only the exit code: stdout went to a separate console.error and stderr was logged per chunk and dropped. Both streams are now accumulated and embedded in the rejection, along with the terminating signal, so a CI log line is enough to diagnose the failure. The handshake matches the banner against the accumulated buffer rather than a single chunk, rejects on 'close' rather than 'exit' so the capture is complete, and routes every settle path through one settle() that clears the timer and detaches its listeners. stop() now waits for the child's 'close' event with a bounded SIGKILL escalation instead of sleeping a fixed 500 ms while the child may still hold its port. The shell case in unsafe_local_code_executor_test.ts spawns a real PowerShell host on Windows but inherited Vitest's 5000 ms default, which is not a process-start budget. It gets one named per-test constant; no project-level or file-level timeout was raised. --- .../unsafe_local_code_executor_test.ts | 43 ++- .../a2a/ts_go/go_backend/go_server.ts | 2 +- tests/integration/test_case_utils.ts | 221 +++++++++--- tests/integration/test_case_utils_test.ts | 338 ++++++++++++++++++ 4 files changed, 538 insertions(+), 66 deletions(-) create mode 100644 tests/integration/test_case_utils_test.ts diff --git a/core/test/code_executors/unsafe_local_code_executor_test.ts b/core/test/code_executors/unsafe_local_code_executor_test.ts index 82a0d2d01..93d960007 100644 --- a/core/test/code_executors/unsafe_local_code_executor_test.ts +++ b/core/test/code_executors/unsafe_local_code_executor_test.ts @@ -45,6 +45,19 @@ const EXPECTED_POWERSHELL_ARGS = [ expect.stringMatching(/script\.ps1$/), ]; +/** + * `it()` budget (ms) for the cases that spawn a real interpreter rather than + * the mocked `spawn` used by the `spawn arguments` suite below. The `unit:core` + * project sets no `testTimeout`, so these inherit Vitest's 5000 ms default, + * which is not a process-start budget: on Windows the default shell is Windows + * PowerShell, whose cold start under V8 coverage instrumentation on a loaded CI + * runner does not reliably fit. The value is above `UnsafeLocalCodeExecutor`'s + * own 30 s default execution timeout on purpose, so a genuinely stuck + * interpreter fails with the executor's timeout message instead of an opaque + * Vitest timeout. + */ +const REAL_INTERPRETER_TIMEOUT_MS = 40000; + function createMockInvocationContext(): InvocationContext { const agent = new LlmAgent({ name: 'test_agent', @@ -158,21 +171,25 @@ describe('UnsafeLocalCodeExecutor', () => { expect(result.stderr).toBe(''); }); - it('should execute shell code and return stdout', async () => { - const params: ExecuteCodeParams = { - invocationContext, - codeExecutionInput: { - code: 'echo "Hello, Shell!"', - language: CodeExecutionLanguage.SHELL, - inputFiles: [], - }, - }; + it( + 'should execute shell code and return stdout', + async () => { + const params: ExecuteCodeParams = { + invocationContext, + codeExecutionInput: { + code: 'echo "Hello, Shell!"', + language: CodeExecutionLanguage.SHELL, + inputFiles: [], + }, + }; - const result = await executor.executeCode(params); + const result = await executor.executeCode(params); - expect(result.stdout).toContain('Hello, Shell!'); - expect(result.stderr).toBe(''); - }); + expect(result.stdout).toContain('Hello, Shell!'); + expect(result.stderr).toBe(''); + }, + REAL_INTERPRETER_TIMEOUT_MS, + ); it('should return error for unsupported language', async () => { const params: ExecuteCodeParams = { diff --git a/tests/cross_language/a2a/ts_go/go_backend/go_server.ts b/tests/cross_language/a2a/ts_go/go_backend/go_server.ts index cf25a2751..82f07e4f3 100644 --- a/tests/cross_language/a2a/ts_go/go_backend/go_server.ts +++ b/tests/cross_language/a2a/ts_go/go_backend/go_server.ts @@ -58,7 +58,7 @@ export class AdkGoServer extends BaseTestServer { }); }, startMessage: 'A2A Server started on', - successLogMessage: `Test Go Server started at ${this.url}`, + successLogMessage: 'Test Go Server started', serverName: 'Go Server', timeoutMs: this.params.startFailureTimeout || DEFAULT_TIMEOUT, }); diff --git a/tests/integration/test_case_utils.ts b/tests/integration/test_case_utils.ts index ad5088fc0..f9e2766c6 100644 --- a/tests/integration/test_case_utils.ts +++ b/tests/integration/test_case_utils.ts @@ -19,6 +19,8 @@ import { GoogleGenAI, } from '@google/genai'; import {ChildProcessWithoutNullStreams} from 'node:child_process'; +import {once} from 'node:events'; +import {createServer} from 'node:net'; import {expect} from 'vitest'; /** @@ -257,23 +259,82 @@ export async function runTestCase(testCase: TestCase) { } } +/** Maximum characters retained from each captured stream in a failure. */ +const OUTPUT_EXCERPT_CHARS = 4000; + +/** How long `stop()` waits for a clean exit before escalating to SIGKILL. */ +const PROCESS_EXIT_TIMEOUT_MS = 5000; + +/** Matches the loopback URL a test server prints in its start-up banner. */ +const SERVER_URL_REGEX = /http:\/\/localhost:([0-9]+)/i; + +/** The signal that terminated a child, sourced from Node's own typing. */ +type TerminationSignal = ChildProcessWithoutNullStreams['signalCode']; + +/** + * Returns a TCP port on `host` that the OS has just confirmed is free, by + * binding port 0, reading the assignment back and releasing it. + * + * Guessing a port instead is what makes a spawned server die at start-up with + * exit code 1: the guess can already be held by another concurrently-starting + * test worker drawing from the same range, or fall inside one of the TCP ranges + * Windows reserves for Hyper-V/WinNAT, and a failed bind is fatal to the child. + */ +export async function reserveFreePort(host: string): Promise { + const probe = createServer(); + + try { + return await new Promise((resolve, reject) => { + probe.once('error', reject); + probe.listen({host, port: 0}, () => { + const address = probe.address(); + // `address()` is typed `AddressInfo | string | null`; the string form is + // for IPC servers, which this never is. + if (address === null || typeof address === 'string') { + reject( + new Error(`Expected a TCP address on ${host}, got ${address}`), + ); + return; + } + resolve(address.port); + }); + }); + } finally { + await new Promise((resolve) => probe.close(() => resolve())); + } +} + +/** + * Renders one captured stream, keeping only its last + * {@link OUTPUT_EXCERPT_CHARS} characters: a child that dies noisily writes far + * more than is useful, and the bytes it wrote last are the ones explaining why. + */ +function formatStream(label: string, output: string): string { + const excerpt = output.slice(-OUTPUT_EXCERPT_CHARS); + return `\n${label}:\n${excerpt || '(no output captured)'}`; +} + +/** Renders both captured streams for inclusion in a start-up failure. */ +function formatCapturedOutput(stdout: string, stderr: string): string { + return formatStream('stdout', stdout) + formatStream('stderr', stderr); +} + /** * Base class for test servers. */ export abstract class BaseTestServer { host: string; port: number; - url: string; protected serverProcess?: ChildProcessWithoutNullStreams; constructor(host: string, port?: number) { this.host = host; - this.port = port || BaseTestServer.getRandomPort(); - this.url = `http://${this.host}:${this.port}`; + // 0 means "allocate at start"; `startProcess` resolves it before spawning. + this.port = port ?? 0; } - static getRandomPort(): number { - return 40000 + Math.floor(Math.random() * 10000); + get url(): string { + return `http://${this.host}:${this.port}`; } protected async startProcess({ @@ -289,76 +350,132 @@ export abstract class BaseTestServer { serverName: string; timeoutMs: number; }): Promise { - this.serverProcess = spawnProcess(); + // Both subclasses read `this.port` from inside `spawnProcess` -- for + // `--port` and for TEST_API_SERVER_PORT -- so it has to be a real port + // before the child starts, not after its banner is parsed. + if (!this.port) { + this.port = await reserveFreePort(this.host); + } + + const child = spawnProcess(); + this.serverProcess = child; + + // Outlive the handshake: an 'error' event with no listener is rethrown by + // EventEmitter, and a server that dies mid-test is still worth reporting. + child.on('error', (error: Error) => { + console.error(`${serverName} Error: ${error.message}`); + }); + child.on('close', (code: number | null) => { + console.error(`${serverName} exited with code ${code}`); + }); await new Promise((resolve, reject) => { - let started = false; - const stdoutChunks: string[] = []; + let stdout = ''; + let stderr = ''; + // Gates retention only. The 'data' listeners stay attached for the life + // of the child so its pipes keep draining; a chatty server that filled + // the 64 KB pipe buffer would otherwise block. + let capturing = true; + + const settle = (error?: Error) => { + clearTimeout(timer); + capturing = false; + child.off('error', onError); + child.off('close', onClose); + if (error) { + reject(error); + } else { + resolve(); + } + }; - this.serverProcess!.stdout.on('data', (data) => { - const message = data.toString(); - stdoutChunks.push(message); + const onStdout = (data: Buffer) => { + if (!capturing) return; + stdout += data.toString(); - // Find URL like http://localhost:12345 - const urlMatch = message.match(/http:\/\/localhost:([0-9]+)/i); - if (urlMatch && urlMatch[1]) { + // Matched against the accumulated output, not this chunk: a banner + // split across two writes must still complete the handshake. + const urlMatch = stdout.match(SERVER_URL_REGEX); + if (urlMatch) { const parsedPort = parseInt(urlMatch[1], 10); if (parsedPort > 0) { this.port = parsedPort; - this.url = `http://${this.host}:${this.port}`; } } - if (message.includes(startMessage)) { - started = true; - console.log(successLogMessage); - resolve(); + if (stdout.includes(startMessage)) { + settle(); } - }); + }; - this.serverProcess!.stderr.on('data', (data) => { - console.error(`${serverName} Stderr: ${data.toString()}`); - }); + const onStderr = (data: Buffer) => { + if (!capturing) return; + stderr += data.toString(); + }; - this.serverProcess!.on('error', (error) => { - console.error(`${serverName} Error: ${error.message}`); - - reject( + const onError = (error: Error) => { + settle( new Error( - `Failed to start ${serverName.toLowerCase()}: ${error.message}`, + `Failed to start ${serverName.toLowerCase()}: ${error.message}` + + formatCapturedOutput(stdout, stderr), ), ); - }); - - this.serverProcess!.on('exit', (code) => { - console.error(`${serverName} exited with code ${code}`); + }; - if (!started) { - console.error( - `${serverName} Captured stdout before premature exit:\n${stdoutChunks.join('')}`, - ); - reject( - new Error(`${serverName} exited prematurely with code ${code}`), - ); - } - }); + // 'close' rather than 'exit': it fires only once the stdio pipes have + // been drained, so the child's last words -- the actual reason it refused + // to start -- are already captured when the error is built. + const onClose = (code: number | null, signal: TerminationSignal) => { + settle( + new Error( + `${serverName} exited prematurely with code ${code}` + + (signal ? ` (signal ${signal})` : '') + + formatCapturedOutput(stdout, stderr), + ), + ); + }; - setTimeout(() => { - if (!started) { - reject( - new Error( - `Timeout waiting for ${serverName.toLowerCase()} to start.`, - ), - ); - } + const timer = setTimeout(() => { + settle( + new Error( + `Timeout waiting for ${serverName.toLowerCase()} to start.` + + formatCapturedOutput(stdout, stderr), + ), + ); }, timeoutMs); + + child.stdout.on('data', onStdout); + child.stderr.on('data', onStderr); + child.on('error', onError); + child.on('close', onClose); }); + + console.log(successLogMessage); } async stop(): Promise { - if (this.serverProcess) { - this.serverProcess.kill('SIGINT'); - await new Promise((resolve) => setTimeout(resolve, 500)); + const child = this.serverProcess; + if (!child) return; + this.serverProcess = undefined; + + // 'close' never fires again for a child that is already gone, so waiting on + // it would hang until the suite timeout. + if (child.exitCode !== null || child.signalCode !== null) return; + + // Subscribed before the kill so a fast exit cannot be missed. + const exited = once(child, 'close'); + child.kill('SIGINT'); + // Windows emulates SIGINT as unconditional termination and a wedged child + // may ignore it outright, so the wait is bounded rather than open-ended. + const escalation = setTimeout( + () => child.kill('SIGKILL'), + PROCESS_EXIT_TIMEOUT_MS, + ); + + try { + await exited; + } finally { + clearTimeout(escalation); } } } diff --git a/tests/integration/test_case_utils_test.ts b/tests/integration/test_case_utils_test.ts new file mode 100644 index 000000000..477601ebd --- /dev/null +++ b/tests/integration/test_case_utils_test.ts @@ -0,0 +1,338 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type {ChildProcessWithoutNullStreams} from 'node:child_process'; +import {spawn} from 'node:child_process'; +import {createServer, type Server} from 'node:net'; +import {afterEach, describe, expect, it} from 'vitest'; +import {BaseTestServer, reserveFreePort} from './test_case_utils.js'; + +const HOST = 'localhost'; +const START_MESSAGE = 'SCRIPTED SERVER STARTED'; +/** Comfortably longer than a `node -e` child needs to print its banner. */ +const START_TIMEOUT_MS = 15000; +/** Keeps a scripted child alive until the test tears it down. */ +const STAY_ALIVE = 'setInterval(() => {}, 1000);'; + +const spawned: ChildProcessWithoutNullStreams[] = []; +const listeners: Server[] = []; + +/** Spawns a `node -e` child; portable and needs no shell quoting. */ +function nodeScript(script: string): () => ChildProcessWithoutNullStreams { + return () => { + const child = spawn(process.execPath, ['-e', script]); + spawned.push(child); + return child; + }; +} + +/** + * A {@link BaseTestServer} driven by a `node -e` script instead of the built + * ADK CLI, so the harness itself can be tested without a build. + */ +class ScriptedTestServer extends BaseTestServer { + child?: ChildProcessWithoutNullStreams; + /** `this.port` as observed from inside the spawn closure. */ + portAtSpawn = 0; + + constructor( + private readonly spawnChild: () => ChildProcessWithoutNullStreams, + port?: number, + ) { + super(HOST, port); + } + + start(timeoutMs = START_TIMEOUT_MS): Promise { + return this.startProcess({ + spawnProcess: () => { + this.portAtSpawn = this.port; + this.child = this.spawnChild(); + return this.child; + }, + startMessage: START_MESSAGE, + successLogMessage: 'Scripted server started', + serverName: 'Scripted', + timeoutMs, + }); + } +} + +function hasExited(child: ChildProcessWithoutNullStreams): boolean { + return child.exitCode !== null || child.signalCode !== null; +} + +/** Binds `port` for the rest of the test, proving it was bindable. */ +function listenOn(port: number): Promise { + const socket = createServer(); + listeners.push(socket); + + return new Promise((resolve, reject) => { + socket.once('error', reject); + socket.listen({host: HOST, port}, () => resolve()); + }); +} + +afterEach(async () => { + for (const child of spawned.splice(0)) { + child.kill('SIGKILL'); + } + await Promise.all( + listeners + .splice(0) + .map( + (socket) => + new Promise((resolve) => socket.close(() => resolve())), + ), + ); +}); + +describe('reserveFreePort', () => { + it('returns a port that can immediately be bound', async () => { + const port = await reserveFreePort(HOST); + + expect(Number.isInteger(port)).toBe(true); + expect(port).toBeGreaterThan(0); + await expect(listenOn(port)).resolves.toBeUndefined(); + }); + + it('never hands back a port that is currently bound', async () => { + const held = await reserveFreePort(HOST); + await listenOn(held); + + for (let i = 0; i < 20; i++) { + expect(await reserveFreePort(HOST)).not.toBe(held); + } + }); + + it('rejects when the probe socket cannot bind the host', async () => { + await expect(reserveFreePort('256.256.256.256')).rejects.toThrow(); + }); +}); + +describe('BaseTestServer.startProcess', () => { + it('reserves the port before the child is spawned', async () => { + const server = new ScriptedTestServer( + nodeScript(`process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`), + ); + + await server.start(); + + // The child reads `this.port` for --port and TEST_API_SERVER_PORT, so a + // port assigned after the spawn would be silently ignored. + expect(server.portAtSpawn).toBeGreaterThan(0); + expect(server.portAtSpawn).toBe(server.port); + expect(server.url).toBe(`http://${HOST}:${server.port}`); + }); + + it('adopts the port the child announces in its banner', async () => { + const server = new ScriptedTestServer( + nodeScript( + "process.stdout.write('Listening on http://localhost:65535\\n');" + + `process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`, + ), + ); + + await server.start(); + + expect(server.port).toBe(65535); + expect(server.url).toBe(`http://${HOST}:65535`); + }); + + it('keeps the reserved port when the banner announces port 0', async () => { + const server = new ScriptedTestServer( + nodeScript( + "process.stdout.write('Listening on http://localhost:0\\n');" + + `process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`, + ), + ); + + await server.start(); + + expect(server.port).toBe(server.portAtSpawn); + expect(server.port).toBeGreaterThan(0); + }); + + it('completes the handshake when the banner is split across writes', async () => { + const [head, tail] = [START_MESSAGE.slice(0, 9), START_MESSAGE.slice(9)]; + const server = new ScriptedTestServer( + nodeScript( + `process.stdout.write('${head}');` + + `setTimeout(() => process.stdout.write('${tail}\\n'), 50);` + + STAY_ALIVE, + ), + ); + + await expect(server.start()).resolves.toBeUndefined(); + }); + + it('surfaces both captured streams when the child exits prematurely', async () => { + const server = new ScriptedTestServer( + nodeScript( + "process.stderr.write('STDERR-REASON\\n', () => " + + "process.stdout.write('STDOUT-REASON\\n', () => process.exit(1)));", + ), + ); + + const attempt = server.start(); + + await expect(attempt).rejects.toThrow( + 'Scripted exited prematurely with code 1', + ); + await expect(attempt).rejects.toThrow('STDERR-REASON'); + await expect(attempt).rejects.toThrow('STDOUT-REASON'); + }); + + it('reports both streams as empty when the child exits silently', async () => { + const server = new ScriptedTestServer(nodeScript('process.exit(1);')); + + const error = await server.start().catch((e: unknown) => e); + + expect(error).toBeInstanceOf(Error); + if (!(error instanceof Error)) { + expect.fail('expected start() to reject with an Error'); + } + expect(error.message).toContain('stdout:\n(no output captured)'); + expect(error.message).toContain('stderr:\n(no output captured)'); + }); + + it('names the signal when the child is terminated', async () => { + // Self-signalling avoids racing the harness for the child handle, which is + // only assigned once `startProcess` reaches its spawn closure. + const server = new ScriptedTestServer( + nodeScript("process.kill(process.pid, 'SIGKILL');"), + ); + + const attempt = server.start(); + + await expect(attempt).rejects.toThrow('(signal SIGKILL)'); + await expect(attempt).rejects.toThrow('exited prematurely with code null'); + }); + + it('keeps only the tail of a noisy child', async () => { + const server = new ScriptedTestServer( + nodeScript( + "process.stdout.write('HEAD-MARKER' + 'x'.repeat(9000) + " + + "'TAIL-MARKER\\n', () => process.exit(1));", + ), + ); + + const error = await server.start().catch((e: unknown) => e); + + expect(error).toBeInstanceOf(Error); + if (!(error instanceof Error)) { + expect.fail('expected start() to reject with an Error'); + } + expect(error.message).toContain('TAIL-MARKER'); + expect(error.message).not.toContain('HEAD-MARKER'); + }); + + it('reports a spawn failure with the captured output', async () => { + const server = new ScriptedTestServer(() => { + const child = spawn('adk-binary-that-does-not-exist', []); + spawned.push(child); + return child; + }); + + const attempt = server.start(); + + await expect(attempt).rejects.toThrow('Failed to start scripted'); + await expect(attempt).rejects.toThrow('(no output captured)'); + }); + + it('surfaces the captured output on a start-up timeout', async () => { + const server = new ScriptedTestServer( + nodeScript( + `process.stdout.write('NOISE-BEFORE-TIMEOUT\\n');${STAY_ALIVE}`, + ), + ); + + const attempt = server.start(300); + + await expect(attempt).rejects.toThrow('Timeout waiting for scripted'); + await expect(attempt).rejects.toThrow('NOISE-BEFORE-TIMEOUT'); + + await server.stop(); + expect(server.child).toBeDefined(); + expect(hasExited(server.child!)).toBe(true); + }); + + it('keeps draining the pipes after the handshake settles', async () => { + // Far beyond the 64 KB pipe buffer: if the handshake left the streams + // unread, the child would block on write and never reach exit(0). + const server = new ScriptedTestServer( + nodeScript( + `process.stdout.write('${START_MESSAGE}\\n');` + + "setTimeout(() => process.stdout.write('y'.repeat(500000), " + + '() => process.exit(0)), 10);', + ), + ); + + await server.start(); + const child = server.child; + expect(child).toBeDefined(); + + await new Promise((resolve) => child!.once('close', () => resolve())); + expect(child!.exitCode).toBe(0); + }); +}); + +describe('BaseTestServer.stop', () => { + it('returns only once the child has actually exited', async () => { + // The child delays its exit past the 500 ms the harness used to sleep for, + // so a fixed sleep would return while it was still running. + const server = new ScriptedTestServer( + nodeScript( + "process.on('SIGINT', () => setTimeout(() => process.exit(0), 800));" + + `process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`, + ), + ); + await server.start(); + const child = server.child; + expect(child).toBeDefined(); + + await server.stop(); + + expect(hasExited(child!)).toBe(true); + }); + + it('escalates to SIGKILL when the child ignores SIGINT', async () => { + const server = new ScriptedTestServer( + nodeScript( + "process.on('SIGINT', () => {});" + + `process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`, + ), + ); + await server.start(); + const child = server.child; + expect(child).toBeDefined(); + + await server.stop(); + + expect(child!.signalCode).toBe('SIGKILL'); + }, 20000); + + it('is a no-op on a server that was never started', async () => { + const server = new ScriptedTestServer(nodeScript(STAY_ALIVE)); + + await expect(server.stop()).resolves.toBeUndefined(); + }); + + it('is safe to call twice, and on a child that already exited', async () => { + const server = new ScriptedTestServer( + nodeScript( + `process.stdout.write('${START_MESSAGE}\\n');` + + 'setTimeout(() => process.exit(0), 10);', + ), + ); + await server.start(); + const child = server.child; + expect(child).toBeDefined(); + await new Promise((resolve) => child!.once('close', () => resolve())); + + await expect(server.stop()).resolves.toBeUndefined(); + await expect(server.stop()).resolves.toBeUndefined(); + }); +}); From e007b19ac4b1091c9803aac8df9fc337af16c132 Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 15:22:15 -0700 Subject: [PATCH 2/5] Test: cover the explicit-port path and stderr draining in the server harness Adds the two cases the first pass left uncovered: an explicitly requested port must be used verbatim rather than re-reserved, and the post-handshake drain test now floods stderr as well as stdout, so the retention gate is exercised on both streams. --- tests/integration/test_case_utils_test.ts | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_case_utils_test.ts b/tests/integration/test_case_utils_test.ts index 477601ebd..2784fba98 100644 --- a/tests/integration/test_case_utils_test.ts +++ b/tests/integration/test_case_utils_test.ts @@ -127,6 +127,19 @@ describe('BaseTestServer.startProcess', () => { expect(server.url).toBe(`http://${HOST}:${server.port}`); }); + it('honours an explicitly requested port instead of reserving one', async () => { + const requested = await reserveFreePort(HOST); + const server = new ScriptedTestServer( + nodeScript(`process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`), + requested, + ); + + await server.start(); + + expect(server.portAtSpawn).toBe(requested); + expect(server.port).toBe(requested); + }); + it('adopts the port the child announces in its banner', async () => { const server = new ScriptedTestServer( nodeScript( @@ -260,13 +273,14 @@ describe('BaseTestServer.startProcess', () => { }); it('keeps draining the pipes after the handshake settles', async () => { - // Far beyond the 64 KB pipe buffer: if the handshake left the streams - // unread, the child would block on write and never reach exit(0). + // Far beyond the 64 KB pipe buffer on both streams: if the handshake left + // either unread, the child would block on write and never reach exit(0). const server = new ScriptedTestServer( nodeScript( `process.stdout.write('${START_MESSAGE}\\n');` + - "setTimeout(() => process.stdout.write('y'.repeat(500000), " + - '() => process.exit(0)), 10);', + "setTimeout(() => process.stderr.write('e'.repeat(500000), " + + "() => process.stdout.write('y'.repeat(500000), " + + '() => process.exit(0))), 10);', ), ); From 8bc46f8d19ba1075799ddc3624ded4aedb64d1aa Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 15:35:53 -0700 Subject: [PATCH 3/5] Fix: reap the child on 'exit' in stop(), not 'close' The cross-language Go suite spawns `go run .`, which leaves the built binary running as a grandchild holding the inherited stdio pipes. 'close' fires only once those pipes are released, so it can outlive the process stop() is trying to reap: killing the wrapper emitted 'exit' but never 'close', and the afterAll hook hung until its 60s budget expired. 'exit' is the event that means the child is gone, and it is guaranteed after the SIGKILL escalation. The informational listener moves back to 'exit' for the same reason, so a mid-test crash of such a server is still reported. The start-up handshake keeps rejecting on 'close', where waiting for the pipes to drain is the point -- that is what guarantees the captured output in the message is complete. Adds a regression test whose grandchild outlives the assertion window, so it cannot pass by merely outlasting the pipe holder. --- tests/integration/test_case_utils.ts | 9 +++-- tests/integration/test_case_utils_test.ts | 42 +++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_case_utils.ts b/tests/integration/test_case_utils.ts index f9e2766c6..8752d7ef0 100644 --- a/tests/integration/test_case_utils.ts +++ b/tests/integration/test_case_utils.ts @@ -365,7 +365,9 @@ export abstract class BaseTestServer { child.on('error', (error: Error) => { console.error(`${serverName} Error: ${error.message}`); }); - child.on('close', (code: number | null) => { + // 'exit' rather than 'close': a child that leaves a grandchild holding the + // inherited stdio pipes never emits 'close'. + child.on('exit', (code: number | null) => { console.error(`${serverName} exited with code ${code}`); }); @@ -462,8 +464,11 @@ export abstract class BaseTestServer { // it would hang until the suite timeout. if (child.exitCode !== null || child.signalCode !== null) return; + // 'exit' rather than 'close': `go run` leaves a grandchild holding the + // inherited stdio pipes, and 'close' waits for those to be released, so it + // can outlive the process this is trying to reap. // Subscribed before the kill so a fast exit cannot be missed. - const exited = once(child, 'close'); + const exited = once(child, 'exit'); child.kill('SIGINT'); // Windows emulates SIGINT as unconditional termination and a wedged child // may ignore it outright, so the wait is bounded rather than open-ended. diff --git a/tests/integration/test_case_utils_test.ts b/tests/integration/test_case_utils_test.ts index 2784fba98..d23a66936 100644 --- a/tests/integration/test_case_utils_test.ts +++ b/tests/integration/test_case_utils_test.ts @@ -6,7 +6,10 @@ import type {ChildProcessWithoutNullStreams} from 'node:child_process'; import {spawn} from 'node:child_process'; +import {mkdtempSync, readFileSync} from 'node:fs'; import {createServer, type Server} from 'node:net'; +import {tmpdir} from 'node:os'; +import * as path from 'node:path'; import {afterEach, describe, expect, it} from 'vitest'; import {BaseTestServer, reserveFreePort} from './test_case_utils.js'; @@ -16,9 +19,13 @@ const START_MESSAGE = 'SCRIPTED SERVER STARTED'; const START_TIMEOUT_MS = 15000; /** Keeps a scripted child alive until the test tears it down. */ const STAY_ALIVE = 'setInterval(() => {}, 1000);'; +/** Mirrors `PROCESS_EXIT_TIMEOUT_MS` in the harness under test. */ +const PROCESS_EXIT_TIMEOUT_MS = 5000; const spawned: ChildProcessWithoutNullStreams[] = []; const listeners: Server[] = []; +/** Files holding the pid of a grandchild that teardown must reap. */ +const grandchildPidFiles: string[] = []; /** Spawns a `node -e` child; portable and needs no shell quoting. */ function nodeScript(script: string): () => ChildProcessWithoutNullStreams { @@ -79,6 +86,9 @@ afterEach(async () => { for (const child of spawned.splice(0)) { child.kill('SIGKILL'); } + for (const pidFile of grandchildPidFiles.splice(0)) { + process.kill(Number(readFileSync(pidFile, 'utf8')), 'SIGKILL'); + } await Promise.all( listeners .splice(0) @@ -328,6 +338,38 @@ describe('BaseTestServer.stop', () => { expect(child!.signalCode).toBe('SIGKILL'); }, 20000); + it('returns when a grandchild still holds the inherited stdio pipes', async () => { + // `go run` behaves this way: killing the wrapper leaves the built binary + // holding the pipes, so the wrapper emits 'exit' but never 'close'. The + // grandchild here outlives the assertion window on purpose -- teardown + // reaps it -- so waiting on 'close' cannot pass by simply outlasting it. + const pidFile = path.join( + mkdtempSync(path.join(tmpdir(), 'adk-harness-')), + 'grandchild.pid', + ); + grandchildPidFiles.push(pidFile); + const server = new ScriptedTestServer( + nodeScript( + "const gc = require('node:child_process').spawn(process.execPath, " + + "['-e', 'setTimeout(() => {}, 60000)'], {stdio: 'inherit'});" + + // Recorded by the parent, which knows the pid the moment it spawns, + // so teardown cannot race the grandchild's own start-up. + `require('node:fs').writeFileSync(${JSON.stringify(pidFile)}, ` + + 'String(gc.pid));' + + `process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`, + ), + ); + await server.start(); + const child = server.child; + expect(child).toBeDefined(); + + const startedAt = Date.now(); + await server.stop(); + + expect(hasExited(child!)).toBe(true); + expect(Date.now() - startedAt).toBeLessThan(PROCESS_EXIT_TIMEOUT_MS); + }); + it('is a no-op on a server that was never started', async () => { const server = new ScriptedTestServer(nodeScript(STAY_ALIVE)); From 14b2475d85a5bffaae2225295c0d76eaeaf93aaa Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 15:49:33 -0700 Subject: [PATCH 4/5] Test: make the signal assertions platform-correct on Windows Three new cases encoded POSIX signal semantics and failed on windows-latest. Windows reports the signal a child was *asked* to terminate with, but a self-termination (process.kill(process.pid, ...)) surfaces only as exit code 1, so the signal-naming case now kills from the test. It waits for the harness to reach its spawn closure first, since startProcess awaits the port reservation before the child handle exists -- the reason that case self-signalled at all. Windows emulates SIGINT as unconditional termination, so a child cannot ignore it and the SIGKILL escalation never arms; the escalation case now expects the signal each platform actually produces, and still pins that stop() returns with the child reaped. Windows tears the grandchild down with its parent, so teardown reaped a pid that no longer existed. It now tolerates ESRCH specifically -- that is the outcome the test wants -- and rethrows anything else. --- tests/integration/test_case_utils_test.ts | 51 +++++++++++++++++++---- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/tests/integration/test_case_utils_test.ts b/tests/integration/test_case_utils_test.ts index d23a66936..05a091df4 100644 --- a/tests/integration/test_case_utils_test.ts +++ b/tests/integration/test_case_utils_test.ts @@ -8,7 +8,7 @@ import type {ChildProcessWithoutNullStreams} from 'node:child_process'; import {spawn} from 'node:child_process'; import {mkdtempSync, readFileSync} from 'node:fs'; import {createServer, type Server} from 'node:net'; -import {tmpdir} from 'node:os'; +import {platform, tmpdir} from 'node:os'; import * as path from 'node:path'; import {afterEach, describe, expect, it} from 'vitest'; import {BaseTestServer, reserveFreePort} from './test_case_utils.js'; @@ -21,6 +21,7 @@ const START_TIMEOUT_MS = 15000; const STAY_ALIVE = 'setInterval(() => {}, 1000);'; /** Mirrors `PROCESS_EXIT_TIMEOUT_MS` in the harness under test. */ const PROCESS_EXIT_TIMEOUT_MS = 5000; +const IS_WINDOWS = platform() === 'win32'; const spawned: ChildProcessWithoutNullStreams[] = []; const listeners: Server[] = []; @@ -71,6 +72,32 @@ function hasExited(child: ChildProcessWithoutNullStreams): boolean { return child.exitCode !== null || child.signalCode !== null; } +/** + * Waits for the harness to reach its spawn closure. `startProcess` awaits the + * port reservation first, so the child handle is not available synchronously. + */ +async function waitForChild( + server: ScriptedTestServer, +): Promise { + for (let i = 0; i < 500 && !server.child; i++) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + if (!server.child) { + expect.fail('the server never reached its spawn closure'); + } + return server.child; +} + +/** True for the Node error raised when a pid no longer exists. */ +function isNoSuchProcess(error: unknown): boolean { + return ( + error instanceof Error && + 'code' in error && + typeof error.code === 'string' && + error.code === 'ESRCH' + ); +} + /** Binds `port` for the rest of the test, proving it was bindable. */ function listenOn(port: number): Promise { const socket = createServer(); @@ -87,7 +114,13 @@ afterEach(async () => { child.kill('SIGKILL'); } for (const pidFile of grandchildPidFiles.splice(0)) { - process.kill(Number(readFileSync(pidFile, 'utf8')), 'SIGKILL'); + try { + process.kill(Number(readFileSync(pidFile, 'utf8')), 'SIGKILL'); + } catch (error: unknown) { + // Windows tears the grandchild down with its parent, so by here it is + // already gone. Any other failure is real and must surface. + if (!isNoSuchProcess(error)) throw error; + } } await Promise.all( listeners @@ -222,13 +255,13 @@ describe('BaseTestServer.startProcess', () => { }); it('names the signal when the child is terminated', async () => { - // Self-signalling avoids racing the harness for the child handle, which is - // only assigned once `startProcess` reaches its spawn closure. - const server = new ScriptedTestServer( - nodeScript("process.kill(process.pid, 'SIGKILL');"), - ); + // Killed from here rather than by the child itself: Windows reports the + // signal it was asked to terminate with, but a self-termination surfaces + // only as exit code 1. + const server = new ScriptedTestServer(nodeScript(STAY_ALIVE)); const attempt = server.start(); + (await waitForChild(server)).kill('SIGKILL'); await expect(attempt).rejects.toThrow('(signal SIGKILL)'); await expect(attempt).rejects.toThrow('exited prematurely with code null'); @@ -335,7 +368,9 @@ describe('BaseTestServer.stop', () => { await server.stop(); - expect(child!.signalCode).toBe('SIGKILL'); + // Windows emulates SIGINT as unconditional termination, so the child cannot + // ignore it and the escalation never arms; POSIX reaches the SIGKILL path. + expect(child!.signalCode).toBe(IS_WINDOWS ? 'SIGINT' : 'SIGKILL'); }, 20000); it('returns when a grandchild still holds the inherited stdio pipes', async () => { From 2696ca3be145d4db1cebeb88d3008f07b19e7c3f Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 16:18:45 -0700 Subject: [PATCH 5/5] Refactor: drop the unreachable banner-port path and three redundant abstractions Simplifications from the complexity review. The banner-port-adoption path is dead once the port is reserved before the spawn: every subclass hands the child that exact port (--port and TEST_API_SERVER_PORT, or PORT), and neither child can bind anything else -- AdkApiServer rejects on EADDRINUSE rather than rebinding and the Go server log.Fatalf's -- so the parsed port could only ever equal the port we already had. The Go banner could not match the regex at all, since it prints 127.0.0.1 and the pattern requires localhost. The regex, the parse block and the two tests covering that behaviour go with it. reserveFreePort hand-rolled a promise wrapper around listen(); node:events once() already does exactly that and rejects on 'error' by design, and it was imported for stop() already. formatStream/formatCapturedOutput were two functions and two doc comments for one string; excerpt() keeps the part worth naming. successLogMessage restated serverName at every call site and carried nothing the callee lacked. It was also the root cause of the go_server.ts workaround: the argument was evaluated before the port was reserved, so interpolating this.url rendered :0. Logging after the await from serverName + this.url drops the parameter and restores the URL the Go server had lost. TerminationSignal was a one-use alias; its single use now names the derived type inline. Not NodeJS.Signals -- that identifier trips eslint no-undef in this config, which is why the alias existed. --- .../a2a/ts_go/go_backend/go_server.ts | 1 - tests/integration/test_api_server.ts | 1 - tests/integration/test_case_utils.ts | 63 +++++++------------ tests/integration/test_case_utils_test.ts | 29 --------- 4 files changed, 22 insertions(+), 72 deletions(-) diff --git a/tests/cross_language/a2a/ts_go/go_backend/go_server.ts b/tests/cross_language/a2a/ts_go/go_backend/go_server.ts index 82f07e4f3..4e1688c22 100644 --- a/tests/cross_language/a2a/ts_go/go_backend/go_server.ts +++ b/tests/cross_language/a2a/ts_go/go_backend/go_server.ts @@ -58,7 +58,6 @@ export class AdkGoServer extends BaseTestServer { }); }, startMessage: 'A2A Server started on', - successLogMessage: 'Test Go Server started', serverName: 'Go Server', timeoutMs: this.params.startFailureTimeout || DEFAULT_TIMEOUT, }); diff --git a/tests/integration/test_api_server.ts b/tests/integration/test_api_server.ts index 8b6337d40..4ff63bee4 100644 --- a/tests/integration/test_api_server.ts +++ b/tests/integration/test_api_server.ts @@ -47,7 +47,6 @@ export class AdkTsApiServer extends BaseTestServer { }); }, startMessage: 'ADK API Server started', - successLogMessage: `Test ADK API Server started`, serverName: 'CLI', timeoutMs: this.params.startFailureTimeout || DEFAULT_TIMEOUT, }); diff --git a/tests/integration/test_case_utils.ts b/tests/integration/test_case_utils.ts index 8752d7ef0..580ed05df 100644 --- a/tests/integration/test_case_utils.ts +++ b/tests/integration/test_case_utils.ts @@ -265,12 +265,6 @@ const OUTPUT_EXCERPT_CHARS = 4000; /** How long `stop()` waits for a clean exit before escalating to SIGKILL. */ const PROCESS_EXIT_TIMEOUT_MS = 5000; -/** Matches the loopback URL a test server prints in its start-up banner. */ -const SERVER_URL_REGEX = /http:\/\/localhost:([0-9]+)/i; - -/** The signal that terminated a child, sourced from Node's own typing. */ -type TerminationSignal = ChildProcessWithoutNullStreams['signalCode']; - /** * Returns a TCP port on `host` that the OS has just confirmed is free, by * binding port 0, reading the assignment back and releasing it. @@ -284,39 +278,33 @@ export async function reserveFreePort(host: string): Promise { const probe = createServer(); try { - return await new Promise((resolve, reject) => { - probe.once('error', reject); - probe.listen({host, port: 0}, () => { - const address = probe.address(); - // `address()` is typed `AddressInfo | string | null`; the string form is - // for IPC servers, which this never is. - if (address === null || typeof address === 'string') { - reject( - new Error(`Expected a TCP address on ${host}, got ${address}`), - ); - return; - } - resolve(address.port); - }); - }); + const listening = once(probe, 'listening'); + probe.listen({host, port: 0}); + await listening; + + const address = probe.address(); + // `address()` is typed `AddressInfo | string | null`; the string form is + // for IPC servers, which this never is. + if (address === null || typeof address === 'string') { + throw new Error(`Expected a TCP address on ${host}, got ${address}`); + } + return address.port; } finally { await new Promise((resolve) => probe.close(() => resolve())); } } /** - * Renders one captured stream, keeping only its last - * {@link OUTPUT_EXCERPT_CHARS} characters: a child that dies noisily writes far - * more than is useful, and the bytes it wrote last are the ones explaining why. + * Keeps only the last {@link OUTPUT_EXCERPT_CHARS} characters of a captured + * stream: a child that dies noisily writes far more than is useful, and the + * bytes it wrote last are the ones explaining why. */ -function formatStream(label: string, output: string): string { - const excerpt = output.slice(-OUTPUT_EXCERPT_CHARS); - return `\n${label}:\n${excerpt || '(no output captured)'}`; +function excerpt(output: string): string { + return output.slice(-OUTPUT_EXCERPT_CHARS) || '(no output captured)'; } -/** Renders both captured streams for inclusion in a start-up failure. */ function formatCapturedOutput(stdout: string, stderr: string): string { - return formatStream('stdout', stdout) + formatStream('stderr', stderr); + return `\nstdout:\n${excerpt(stdout)}\nstderr:\n${excerpt(stderr)}`; } /** @@ -340,13 +328,11 @@ export abstract class BaseTestServer { protected async startProcess({ spawnProcess, startMessage, - successLogMessage, serverName, timeoutMs, }: { spawnProcess: () => ChildProcessWithoutNullStreams; startMessage: string; - successLogMessage: string; serverName: string; timeoutMs: number; }): Promise { @@ -397,14 +383,6 @@ export abstract class BaseTestServer { // Matched against the accumulated output, not this chunk: a banner // split across two writes must still complete the handshake. - const urlMatch = stdout.match(SERVER_URL_REGEX); - if (urlMatch) { - const parsedPort = parseInt(urlMatch[1], 10); - if (parsedPort > 0) { - this.port = parsedPort; - } - } - if (stdout.includes(startMessage)) { settle(); } @@ -427,7 +405,10 @@ export abstract class BaseTestServer { // 'close' rather than 'exit': it fires only once the stdio pipes have // been drained, so the child's last words -- the actual reason it refused // to start -- are already captured when the error is built. - const onClose = (code: number | null, signal: TerminationSignal) => { + const onClose = ( + code: number | null, + signal: ChildProcessWithoutNullStreams['signalCode'], + ) => { settle( new Error( `${serverName} exited prematurely with code ${code}` + @@ -452,7 +433,7 @@ export abstract class BaseTestServer { child.on('close', onClose); }); - console.log(successLogMessage); + console.log(`${serverName} started at ${this.url}`); } async stop(): Promise { diff --git a/tests/integration/test_case_utils_test.ts b/tests/integration/test_case_utils_test.ts index 05a091df4..c908cd67f 100644 --- a/tests/integration/test_case_utils_test.ts +++ b/tests/integration/test_case_utils_test.ts @@ -61,7 +61,6 @@ class ScriptedTestServer extends BaseTestServer { return this.child; }, startMessage: START_MESSAGE, - successLogMessage: 'Scripted server started', serverName: 'Scripted', timeoutMs, }); @@ -183,34 +182,6 @@ describe('BaseTestServer.startProcess', () => { expect(server.port).toBe(requested); }); - it('adopts the port the child announces in its banner', async () => { - const server = new ScriptedTestServer( - nodeScript( - "process.stdout.write('Listening on http://localhost:65535\\n');" + - `process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`, - ), - ); - - await server.start(); - - expect(server.port).toBe(65535); - expect(server.url).toBe(`http://${HOST}:65535`); - }); - - it('keeps the reserved port when the banner announces port 0', async () => { - const server = new ScriptedTestServer( - nodeScript( - "process.stdout.write('Listening on http://localhost:0\\n');" + - `process.stdout.write('${START_MESSAGE}\\n');${STAY_ALIVE}`, - ), - ); - - await server.start(); - - expect(server.port).toBe(server.portAtSpawn); - expect(server.port).toBeGreaterThan(0); - }); - it('completes the handshake when the banner is split across writes', async () => { const [head, tail] = [START_MESSAGE.slice(0, 9), START_MESSAGE.slice(9)]; const server = new ScriptedTestServer(