From 291e7bc40b34d477a3115e5b8d7ca3c21bdd5d35 Mon Sep 17 00:00:00 2001 From: SSU-WEI HUANG Date: Wed, 12 Aug 2026 09:29:59 +0800 Subject: [PATCH] fix(test): stop runner integration suite from leaking detached process trees (#1515) (#1521) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(test): stop runner integration suite from leaking detached process trees (#1515) The default CLI test run included runner.integration.test.ts, which spawns real detached runner/session process trees. A failing, timed-out, or interrupted test (or a plain runner stop) left those trees alive under PID 1 — on the Mac this accumulated ~600 Node/Bun/agent processes and several GiB of RSS over repeated runs. Test harness changes only; production runner session-preservation semantics are untouched: - Exclude runner.integration.test.ts from the default parallel unit-test suite; move it into a dedicated serial integration project (vitest.integration.config.ts, 'bun run test:integration'). The 20-session stress test is opt-in via HAPI_RUN_STRESS_TESTS=true. - Add a test-owned process/session registry (processRegistry.ts): every runner, runner-spawned session, and terminal-style child is registered immediately after spawn; afterEach/afterAll run two-stage cleanup (logical stopRunnerSession first, then bounded process-tree kill), followed by a marker sweep for agent grandchildren reparented to PID 1. - Add a per-run HAPI_TEST_MARKER env stamp + identity/secret env neutralization for test children (integrationEnv.ts) so outer HAPI/pi session variables never leak into test processes and the final audit can recognize test-owned processes by env alone. - Final suite audit in globalSetup teardown: reap anything still carrying the run marker and fail with PID/command diagnostics if anything cannot be reaped, before removing the temp home. - Regression coverage: a deliberately failing test registers a detached child and the follow-up audit must find zero test-owned processes. - CI: replace the dead .env.integration-test step with a dedicated integration job running the serial project. * refactor(test): drop unused killByChildProcess import and child field from registry * chore(test): raise integration hookTimeout to 60s for slow teardown hosts * fix(test): fail loudly when the process-table audit cannot scan; assert regression child death Bot review #1521 findings: - A failed `ps` scan (unsupported flags, buffer exhaustion, permissions) previously returned [] and silently disabled both teardown audit layers. It now throws; globalSetup teardown catches the scan error into the audit error (temp home is still removed) so the run fails visibly. - The regression audit test cleaned the leak with the reaper before asserting, and force-killed the fresh marked runner. The failing test's direct child PID is now asserted dead in afterEach right after registry cleanup (before the marker sweep), and the audit test stops its own runner gracefully before reaping. * fix(test): bound the logical cleanup phase so a hung runner cannot stall the hook Bot review #1521: stopRunnerSession carries the worker's 60s HTTP timeout (setup.ts raises HAPI_RUNNER_HTTP_TIMEOUT for the stress test), and the integration hook timeout is also 60s — N sequential stops could exhaust the hook budget before the process-tree fallback and marker sweep ran, recreating the very leak this change prevents. Logical shutdown is now parallel (Promise.allSettled over all tracked sessions) and the whole phase (stops + PID resolution) races against a 15s budget, so stage-2 tree-kill and the marker sweep always get their share of the hook window. * fix(test): bound graceful runner stop in hooks; keep credentials out of audit diagnostics Bot review #1521 (follow-up): - stopRunner()'s HTTP stop can burn the worker-wide 60s timeout on a hung-but-live runner, starving the marker sweep within the hook budget. afterEach/afterAll now race the graceful stop against a 10s bound; a runner that does not stop in time is force-reaped by the sweep (it carries the run marker) and the next beforeEach's alive-PID guard ignores any stale state file. - The env-bearing ps scan (ps eww) was also used for diagnostics, so the first 500 chars of a short-command process could print inherited credentials (CLI_API_TOKEN etc.) into teardown error logs. The scan now only identifies marked PIDs; command lines are fetched separately without 'e', falling back to '(command unavailable)' instead of the env dump. * fix(test): reap runner model-probe orphans before the zero-survivor inspection Bot review #1521 (Minor): inspect-before-reap. Applying it exposed a real race: each test's runner legitimately spawns marker-carrying children at startup (agent acp + agent --list-models model-catalog probes). Stopping the runner orphans them (ppid 1) with the run marker, so the audit test's OWN runner polluted the pure inspection with fresh probes spawned after the failing test's sweep window. - reapTestOwnedProcesses now re-kills every re-scan iteration instead of killing once and only re-scanning, so a process that survived its first SIGKILL (mid-exec) or spawned mid-kill is not given a free pass. - The regression audit test stops its runner, reaps (clearing its own legitimate orphan probes), then inspects: anything still marked is a genuine survivor the bounded reaper could not remove and fails the suite. Killable leaks from the failing test are already asserted dead in afterEach before the sweep runs. * fix(test): strictly bound the marker reaper; make per-test sweep unconditional and verified Bot review #1521 (follow-up): - The 10s reap deadline did not bound the awaited per-tree kills: each killProcessTreeByPid can wait up to 2s per PID, so several stuck processes could still exceed the 60s hook budget. Every process in a test-owned tree carries the marker (env is inherited), so tree-walking is unnecessary: the reaper now SIGKILLs every marked PID found by each scan, fire-and-forget, and re-scans every 250ms — the deadline strictly bounds the function. - The per-test sweep was skipped when the direct-child assertion failed first, and its survivors were ignored. afterEach now snapshots the regression-child state BEFORE the unconditional sweep, then verifies both the registry result and the sweep leftovers. * fix(test): replace it.fails regression with a direct assertion test Bot review #1521 (Minor): Vitest applies the it.fails expected-failure inversion after afterEach, so a broken registry assertion inside the hook would be masked as an expected failure, and the marker sweep would erase the evidence before the follow-up audit ran. The regression is now a normal test that registers a detached child at spawn time, deliberately performs NO per-test teardown, runs only the spawn-time registered cleanup, and asserts the child PID is dead. The afterEach no longer carries the registry-leak assertion (moved into the test body where it cannot be inverted); the per-test sweep assertion and the final audit test are unchanged. * fix(test): bound registry stage-2 tree-kills; require live regression fixture Bot review #1521 (follow-up): - Stage-2 killProcessTreeByPid awaits per descendant serially and can consume the whole 60s hook for a large/stuck tree. Signals are all delivered synchronously (children first) before any waiting, so racing the awaits against a 5s budget bounds the phase without skipping any kill; waitForAllDead still verifies the outcome. - The regression test could pass vacuously if its fixture exited during the startup delay (the registry exit listener would remove it before cleanup). It now asserts the child is alive before running cleanup. * fix(test): kill registered roots with bare synchronous SIGKILL, no pgrep walk Bot review #1521 (follow-up): racing the mapped killProcessTreeByPid calls against a timer does not bound the phase — evaluating the map invokes each call immediately, and each runs the recursive synchronous pgrep walk before its first await, which can consume the hook before the timer, runner stop, or marker sweep run. Stage-2 now SIGKILLs registered roots directly (fire-and-forget, no tree walk, no per-PID waits) and waits a bounded 5s for death. Descendants are reaped by the unconditional marker sweep immediately afterward — every descendant inherits the run marker, so tree-walking is unnecessary. * fix(test): drop duplicate process-death wait in registry cleanup Bot review #1521 (Minor): the duplicated waitForAllDead delayed the authoritative marker sweep by another 5s under the exact stuck-process condition the harness must handle. Keep the single bounded wait; the afterEach marker sweep remains the guarantee. --- .github/workflows/test.yml | 22 ++- cli/package.json | 2 + cli/src/runner/runner.integration.test.ts | 213 ++++++++++++++++++---- cli/src/test/auditTestProcesses.ts | 128 +++++++++++++ cli/src/test/globalSetup.ts | 29 +++ cli/src/test/integrationEnv.ts | 87 +++++++++ cli/src/test/processRegistry.ts | 165 +++++++++++++++++ cli/vitest.config.ts | 8 + cli/vitest.integration.config.ts | 51 ++++++ package.json | 1 + 10 files changed, 657 insertions(+), 49 deletions(-) create mode 100644 cli/src/test/auditTestProcesses.ts create mode 100644 cli/src/test/integrationEnv.ts create mode 100644 cli/src/test/processRegistry.ts create mode 100644 cli/vitest.integration.config.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b91a3a8f..1ec7f0e1 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -16,13 +16,17 @@ jobs: - run: bun typecheck - run: bunx playwright install --with-deps chromium - run: bun run test:e2e -- terminal-wrap-fidelity.spec.ts - - name: Create integration test env - run: | - { - echo "HAPI_HOME=~/.hapi-dev-test" - echo "HAPI_API_URL=http://localhost:3006" - echo "CLI_API_TOKEN=${CLI_API_TOKEN:-dev-test-token}" - echo "HAPI_DAEMON_HTTP_TIMEOUT=60000" - echo "HAPI_DAEMON_HEARTBEAT_INTERVAL=30000" - } > cli/.env.integration-test - run: bun run test + + # Serial runner-integration suite: starts real detached runner/session + # process trees against an isolated temp hub, so it runs in its own job + # (never in the parallel unit-test path). See cli/vitest.integration.config.ts. + integration: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: oven-sh/setup-bun@v2 + with: + bun-version: 1.3.14 + - run: bun install + - run: bun run test:cli:integration diff --git a/cli/package.json b/cli/package.json index c36d96d8..6f50374e 100644 --- a/cli/package.json +++ b/cli/package.json @@ -44,6 +44,8 @@ "tools:unpack": "bun run scripts/unpack-tools.ts", "update-homebrew-formula": "bun run scripts/update-homebrew-formula.ts", "test": "bun run tools:unpack && vitest run", + "test:integration": "bun run tools:unpack && vitest run --config vitest.integration.config.ts", + "test:integration:stress": "bun run tools:unpack && HAPI_RUN_STRESS_TESTS=true vitest run --config vitest.integration.config.ts", "test:win": "vitest run", "dev": "bun src/index.ts", "dev:local-server": "bun --env-file .env.dev-local-server src/index.ts", diff --git a/cli/src/runner/runner.integration.test.ts b/cli/src/runner/runner.integration.test.ts index 730c9c89..f0941a40 100644 --- a/cli/src/runner/runner.integration.test.ts +++ b/cli/src/runner/runner.integration.test.ts @@ -3,19 +3,24 @@ * * Tests the full flow of runner startup, session tracking, and shutdown * - * IMPORTANT: These tests MUST be run with the integration test environment: - * yarn test:integration-test-env + * IMPORTANT: These tests spawn real detached runner/session process trees + * and MUST be run through the dedicated serial integration project: * - * DO NOT run with regular 'npm test' or 'yarn test' - it will use the wrong environment - * and the runner will not work properly! + * bun run test:integration (runner lifecycle coverage) + * bun run test:integration:stress (+ the 20-session stress test) * - * The integration test environment uses .env.integration-test which sets: - * - HAPI_HOME=~/.hapi-dev-test (DIFFERENT from dev's ~/.hapi-dev!) - * - HAPI_API_URL=http://localhost:3006 (local hapi-hub) - * - CLI_API_TOKEN=... (must match the hub) + * They are EXCLUDED from the default parallel unit-test suite + * (`bun run test` / `vitest run`) — see vitest.config.ts and + * vitest.integration.config.ts. Every process the suite spawns is registered + * with testProcessRegistry immediately after spawn, so even a failing or + * interrupted test still reaps its children (two-stage: logical stop first, + * then bounded process-tree termination). The final audit lives in + * globalSetup teardown and fails the run if any test-owned process survives. + * + * The 20-session stress test is opt-in via HAPI_RUN_STRESS_TESTS=true. */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, afterAll } from 'vitest'; import { spawn } from 'child_process'; import { existsSync, unlinkSync, readFileSync, writeFileSync, readdirSync } from 'fs'; import path, { join } from 'path'; @@ -33,6 +38,9 @@ import { Metadata } from '@/api/types'; import { spawnHappyCLI } from '@/utils/spawnHappyCLI'; import { getLatestRunnerLog } from '@/ui/logger'; import { isProcessAlive, isWindows, killProcess, killProcessByChildProcess } from '@/utils/process'; +import { buildTestChildEnv, testOwnedMarker } from '@/test/integrationEnv'; +import { trackChildProcess, trackRunnerPid, trackSession, cleanupAllRegisteredProcesses } from '@/test/processRegistry'; +import { findTestOwnedProcesses, reapTestOwnedProcesses } from '@/test/auditTestProcesses'; // Utility to wait for condition async function waitFor( @@ -81,20 +89,38 @@ async function isServerHealthy(): Promise { describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: 20_000 }, () => { let runnerPid: number; + /** Spawn a runner session and register it in the test-owned registry immediately. */ + async function spawnTrackedSession(directory: string, sessionId?: string): Promise { + const response = await spawnRunnerSession(directory, sessionId); + if (response?.sessionId) { + trackSession(response.sessionId, `runner-session:${response.sessionId}`); + } + return response; + } + beforeEach(async () => { // First ensure no runner is running by checking PID in metadata file await stopRunner() // Start fresh runner for this test // This will return and start a background process - we don't need to wait for it - void spawnHappyCLI(['runner', 'start'], { - stdio: 'ignore' + const runnerLauncher = spawnHappyCLI(['runner', 'start'], { + stdio: 'ignore', + // Test-scoped env: neutralizes any outer HAPI/pi session identity and + // stamps every child with the run's unique test marker. + env: buildTestChildEnv() }); + // Register immediately after spawn so cleanup runs even if this test fails + // before reaching its happy-path teardown. + trackChildProcess(runnerLauncher, 'runner-launcher'); // Wait for runner to write its state file (it needs to auth, setup, and start server) + // Also require the PID to actually be alive: a SIGKILLed runner (or a + // crashed one) can leave a stale runner.state.json behind, and reading it + // as "started" would make every control call in the test body fail. await waitFor(async () => { const state = await readRunnerState(); - return state !== null; + return state !== null && isProcessAlive(state.pid); }, 10_000, 250); // Wait up to 10 seconds, checking every 250ms const runnerState = await readRunnerState(); @@ -102,13 +128,45 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: throw new Error('Runner failed to start within timeout'); } runnerPid = runnerState.pid; + trackRunnerPid(runnerPid, 'runner'); console.log(`[TEST] Runner started for test: PID=${runnerPid}`); console.log(`[TEST] Runner log file: ${runnerState?.runnerLogPath}`); }); + /** Bounded wrapper around the graceful runner stop. */ + async function stopRunnerBounded(): Promise { + // stopRunner()'s HTTP stop can burn the worker-wide 60s timeout + // (HAPI_RUNNER_HTTP_TIMEOUT) on a hung-but-live runner, which would + // exhaust the hook budget before the marker sweep runs. Bound it: if the + // runner does not stop in time, the sweep below force-reaps it (it + // carries the run marker) and the next beforeEach's alive-PID guard + // ignores any stale state file. + await Promise.race([stopRunner(), new Promise((resolve) => setTimeout(resolve, 10_000))]) + } + afterEach(async () => { - await stopRunner() + // Two-stage cleanup must run BEFORE stopRunner() so the runner is still + // alive to logically stop tracked sessions and report their PIDs. + await cleanupAllRegisteredProcesses() + // Graceful stop (the runner removes its own state file), bounded so a + // hung runner cannot starve the sweep below. + await stopRunnerBounded() + // Marker sweep: sessions/agents can reparent to PID 1 before the registry + // tree-kill runs, so reap anything still carrying the run's test marker + // (same audit globalSetup teardown performs at the end of the run). This + // runs unconditionally and its survivors are verified, never ignored. + const leftovers = await reapTestOwnedProcesses(testOwnedMarker()) + expect( + leftovers, + 'test-owned processes survived the marker sweep' + ).toEqual([]); + }); + + afterAll(async () => { + await cleanupAllRegisteredProcesses() + await stopRunnerBounded() + await reapTestOwnedProcesses(testOwnedMarker()) }); it('should list sessions (initially empty)', async () => { @@ -143,7 +201,7 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: }); it('should spawn & stop a session via HTTP (not testing RPC route, but similar enough)', async () => { - const response = await spawnRunnerSession('/tmp', 'spawned-test-456'); + const response = await spawnTrackedSession('/tmp', 'spawned-test-456'); expect(response).toHaveProperty('success', true); expect(response).toHaveProperty('sessionId'); @@ -164,28 +222,32 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: expect(await stopRunnerSession('unknown-session-id')).toBe('still_alive'); }); - it('stress test: spawn / stop', { timeout: 60_000 }, async () => { - const promises = []; - const sessionCount = 20; - for (let i = 0; i < sessionCount; i++) { - promises.push(spawnRunnerSession('/tmp')); + it.skipIf(process.env.HAPI_RUN_STRESS_TESTS !== 'true')( + 'stress test: spawn / stop (opt-in via HAPI_RUN_STRESS_TESTS=true)', + { timeout: 60_000 }, + async () => { + const promises = []; + const sessionCount = 20; + for (let i = 0; i < sessionCount; i++) { + promises.push(spawnTrackedSession('/tmp')); + } + + // Wait for all sessions to be spawned + const results = await Promise.all(promises); + const sessionIds = results.map(r => r.sessionId); + + const sessions = await listRunnerSessions(); + expect(sessions).toHaveLength(sessionCount); + + // Stop all sessions + const stopResults = await Promise.all(sessionIds.map(sessionId => stopRunnerSession(sessionId))); + expect(stopResults.every(r => r === 'stopped' || r === 'already_gone'), 'Not all sessions reported stopped').toBe(true); + + // Verify all sessions are stopped + const emptySessions = await listRunnerSessions(); + expect(emptySessions).toHaveLength(0); } - - // Wait for all sessions to be spawned - const results = await Promise.all(promises); - const sessionIds = results.map(r => r.sessionId); - - const sessions = await listRunnerSessions(); - expect(sessions).toHaveLength(sessionCount); - - // Stop all sessions - const stopResults = await Promise.all(sessionIds.map(sessionId => stopRunnerSession(sessionId))); - expect(stopResults.every(r => r === 'stopped' || r === 'already_gone'), 'Not all sessions reported stopped').toBe(true); - - // Verify all sessions are stopped - const emptySessions = await listRunnerSessions(); - expect(emptySessions).toHaveLength(0); - }); + ); it('should handle runner stop request gracefully', async () => { await stopRunnerHttp(); @@ -202,8 +264,11 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: ], { cwd: '/tmp', detached: true, - stdio: 'ignore' + stdio: 'ignore', + env: buildTestChildEnv() }); + // Register immediately after spawn so cleanup runs even on failure. + trackChildProcess(terminalHappyProcess, 'terminal-session'); if (!terminalHappyProcess || !terminalHappyProcess.pid) { throw new Error('Failed to spawn terminal hapi process'); } @@ -211,7 +276,7 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: await new Promise(resolve => setTimeout(resolve, 5_000)); // Spawn a runner session - const spawnResponse = await spawnRunnerSession('/tmp', 'runner-session-bbb'); + const spawnResponse = await spawnTrackedSession('/tmp', 'runner-session-bbb'); // List all sessions const sessions = await listRunnerSessions(); @@ -245,7 +310,7 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: it('should update session metadata when webhook is called', async () => { // Spawn a session - const spawnResponse = await spawnRunnerSession('/tmp'); + const spawnResponse = await spawnTrackedSession('/tmp'); // Verify webhook was processed (session ID updated) const sessions = await listRunnerSessions(); @@ -261,9 +326,11 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: // Try to start another runner const secondChild = spawn('bun', ['src/index.ts', 'runner', 'start-sync'], { cwd: process.cwd(), - env: process.env, + env: buildTestChildEnv(), stdio: ['ignore', 'pipe', 'pipe'] }); + // Register immediately so the registry can reap it if the test fails. + trackChildProcess(secondChild, 'second-runner'); let output = ''; secondChild.stdout?.on('data', (data) => { @@ -287,7 +354,7 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: const promises = []; for (let i = 0; i < 3; i++) { promises.push( - spawnRunnerSession('/tmp') + spawnTrackedSession('/tmp') ); } @@ -463,6 +530,72 @@ describe.skipIf(!await isServerHealthy())('Runner Integration Tests', { timeout: } }); + /** + * Regression coverage for issue #1515: a test that never reaches its own + * happy-path cleanup must not leak the detached children it registered at + * spawn time. The child is tracked immediately after spawn; the test body + * then runs ONLY the spawn-time registered cleanup and asserts the child is + * gone. + * + * A deliberately-failing (`it.fails`) variant would be weaker here: Vitest + * applies the expected-failure inversion after afterEach, so a broken + * registry assertion inside the hook would be masked as "expected". A + * normal test asserts directly. + */ + it('regression: registered detached child is reaped even when the test body never reaches its own cleanup', async () => { + const child = spawnHappyCLI([ + '--hapi-starting-mode', 'remote', + '--started-by', 'terminal' + ], { + cwd: '/tmp', + detached: true, + stdio: 'ignore', + env: buildTestChildEnv() + }); + // Register immediately after spawn — cleanup must run even though this + // test deliberately performs no per-test teardown of its own. + trackChildProcess(child, 'regression-terminal'); + if (!child.pid) { + throw new Error('Failed to spawn regression terminal hapi process'); + } + + // Give the detached child time to fully start (including its agent + // probes), so a leak would be real and observable. Require the fixture to + // actually be alive: if it exited on its own, the registry exit listener + // would remove it and the dead assertion below would pass vacuously. + await new Promise(resolve => setTimeout(resolve, 2_000)); + expect( + isProcessAlive(child.pid), + 'regression fixture exited before cleanup — test is vacuous' + ).toBe(true); + + // Simulate the failure path: only the spawn-time registered cleanup runs. + await cleanupAllRegisteredProcesses() + expect( + isProcessAlive(child.pid), + 'registered detached child survived registry cleanup' + ).toBe(false); + }); + + it('regression: final audit finds zero test-owned processes after the reaping regression test', async () => { + // The runner from this test's beforeEach legitimately spawns model-catalog + // probe children (agent --list-models / agent acp) at startup; stopping + // it orphans them with the run marker. Reap first to clear that noise, + // then INSPECT: anything still marked at this point is a genuine survivor + // the reaper could not remove within its bounded window and must fail the + // suite (same audit globalSetup teardown performs at the end of the run). + // Killable leaks from the reaping regression test are already gone here: + // its direct child is asserted dead in the test body, and its afterEach + // sweep re-kills for its full bounded window. + await stopRunnerBounded() + await reapTestOwnedProcesses(testOwnedMarker()) + const leftovers = findTestOwnedProcesses(testOwnedMarker()); + expect( + leftovers, + 'test-owned processes survived the bounded reaper — cleanup guarantee broken' + ).toEqual([]); + }); + // TODO: Add a test to see if a corrupted file will work // TODO: Test npm uninstall scenario - runner should gracefully handle when hapi is uninstalled diff --git a/cli/src/test/auditTestProcesses.ts b/cli/src/test/auditTestProcesses.ts new file mode 100644 index 00000000..a0aaa497 --- /dev/null +++ b/cli/src/test/auditTestProcesses.ts @@ -0,0 +1,128 @@ +/** + * Final audit for test-owned processes. + * + * The runner integration suite spawns real detached process trees. Even with + * the per-test registry (see `processRegistry.ts`), an orphan whose runner was + * already killed, or a child that escaped a crashing test, can survive the + * suite. This module is the last-resort backstop: it scans the live process + * table for the run's unique marker (`HAPI_TEST_MARKER=`, injected by + * `integrationEnv.ts` into every test child) and force-reaps whatever remains. + * + * The marker lives in the process environment, which survives reparenting to + * PID 1, so orphaned grandchildren are still recognized. Production processes + * never carry the marker and are never touched. + */ + +import { execFileSync } from 'node:child_process' + +export interface TestOwnedProcess { + pid: number + ppid: number + rssKb: number + command: string +} + +/** + * Scans for live processes whose environment dump contains `marker`. + * Returns an empty array on platforms without `ps eww` (Windows); THROWS on + * scan failure (unsupported flags, buffer exhaustion, permission errors) so + * the audit can never silently report "zero survivors" while detached + * test-owned processes remain alive. + * + * The environment-bearing scan is used ONLY to identify marked PIDs. + * Diagnostics (the `command` field) are fetched with a separate `ps` call + * WITHOUT `e`, so inherited credentials in the env dump never reach logs. + */ +export function findTestOwnedProcesses(marker: string): TestOwnedProcess[] { + if (process.platform === 'win32') return [] + + let output: string + try { + // `-eo` (not `-axo`): procps-ng 4.x rejects `-x` with "must set + // personality" on some Linux builds. `e` shows the environment after + // the command; `ww` removes width truncation so the env dump is not + // cut off. + output = execFileSync('ps', ['eww', '-eo', 'pid=,ppid=,rss=,command='], { + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + stdio: ['ignore', 'pipe', 'pipe'], + }) + } catch (error) { + throw new Error( + `[test process audit] failed to inspect process table: ${error instanceof Error ? error.message : String(error)}` + ) + } + + const matchedPids: number[] = [] + const ppidByPid = new Map() + const rssByPid = new Map() + for (const line of output.split('\n')) { + const match = line.match(/^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/) + if (!match) continue + if (match[4].includes(marker)) { + const pid = Number(match[1]) + matchedPids.push(pid) + ppidByPid.set(pid, Number(match[2])) + rssByPid.set(pid, Number(match[3])) + } + } + if (matchedPids.length === 0) return [] + + // Fetch clean command lines (no environment) for diagnostics. + const commandByPid = new Map() + try { + const clean = execFileSync( + 'ps', + ['-p', matchedPids.join(','), '-o', 'pid=,command='], + { + encoding: 'utf8', + maxBuffer: 16 * 1024 * 1024, + stdio: ['ignore', 'pipe', 'pipe'], + } + ) + for (const line of clean.split('\n')) { + const match = line.match(/^\s*(\d+)\s+(.*)$/) + if (match) { + commandByPid.set(Number(match[1]), match[2].trim()) + } + } + } catch { + // Diagnostics are best-effort; never fall back to the env dump. + } + + return matchedPids.map((pid) => ({ + pid, + ppid: ppidByPid.get(pid) ?? 0, + rssKb: rssByPid.get(pid) ?? 0, + command: (commandByPid.get(pid) ?? '(command unavailable)').slice(0, 500), + })) +} + +/** + * Force-reaps every process carrying `marker`, waiting a bounded window for + * them to disappear, and returns whatever still remains. + * + * Every process in a test-owned tree carries the marker (env is inherited), + * so there is no need to tree-walk: each scan finds the whole marked set and + * SIGKILLs it directly. Kills are fire-and-forget — no per-PID wait — so the + * 10s deadline strictly bounds this function even with many stuck processes. + * The loop re-runs on every re-scan so a process that survived its first + * SIGKILL (e.g. mid-exec, D-state) or spawned after the previous scan is + * never given a free pass. + */ +export async function reapTestOwnedProcesses(marker: string): Promise { + const deadline = Date.now() + 10_000 + let found = findTestOwnedProcesses(marker) + while (found.length > 0 && Date.now() < deadline) { + for (const { pid } of found) { + try { + process.kill(pid, 'SIGKILL') + } catch { + // Already dead or racing exit; re-scan below decides. + } + } + await new Promise((resolve) => setTimeout(resolve, 250)) + found = findTestOwnedProcesses(marker) + } + return findTestOwnedProcesses(marker) +} diff --git a/cli/src/test/globalSetup.ts b/cli/src/test/globalSetup.ts index ed117b86..7bee0d46 100644 --- a/cli/src/test/globalSetup.ts +++ b/cli/src/test/globalSetup.ts @@ -6,6 +6,8 @@ import { fileURLToPath } from 'node:url' import net from 'node:net' import { spawn, execSync } from 'node:child_process' import type { ChildProcess } from 'node:child_process' +import { reapTestOwnedProcesses } from './auditTestProcesses' +import { TEST_OWNED_MARKER_KEY } from './integrationEnv' // Workers can't inherit process.env from globalSetup, so we write config to a file // and let setupFile.ts read it in each worker. @@ -110,7 +112,34 @@ async function stopHubProcess(): Promise { export async function teardown() { await stopHubProcess() try { rmSync(TEST_CONFIG_FILE) } catch {} + + // Final audit: test children carry `HAPI_TEST_MARKER=` in their + // environment (see integrationEnv.ts). Anything still alive after the + // suites ran is a test-owned leak — reap it, then fail the run with + // PID/command diagnostics if something could not be reaped. The temp home + // is always removed so a leak cannot also accumulate DB rows on disk. + let auditError: Error | null = null + if (tmpHome && process.platform !== 'win32') { + try { + const leftovers = await reapTestOwnedProcesses(`${TEST_OWNED_MARKER_KEY}=${tmpHome}`) + if (leftovers.length > 0) { + const detail = leftovers + .map((p) => ` pid=${p.pid} ppid=${p.ppid} rss=${p.rssKb}KB ${p.command}`) + .join('\n') + auditError = new Error( + `[globalSetup] ${leftovers.length} test-owned process(es) survived teardown:\n${detail}` + ) + } + } catch (error) { + // A failed process-table scan must fail the run, never pass as a + // "clean" audit. + auditError = error instanceof Error ? error : new Error(String(error)) + } + } if (tmpHome) { rmSync(tmpHome, { recursive: true, force: true }) } + if (auditError) { + throw auditError + } } diff --git a/cli/src/test/integrationEnv.ts b/cli/src/test/integrationEnv.ts new file mode 100644 index 00000000..f71213f0 --- /dev/null +++ b/cli/src/test/integrationEnv.ts @@ -0,0 +1,87 @@ +/** + * Test-child environment builder for the runner integration suite. + * + * Every real CLI child the suite spawns must run with this environment so + * that: + * + * 1. Identity variables of the outer HAPI/pi session (PI_SESSION_ID, + * HAPI_SESSION_ID, PM2 metadata, ...) never leak into test children. + * These are blanked rather than dropped because `spawnHappyCLI` merges + * `{ ...process.env, ...options.env }` — a blank value still wins over the + * inherited one, while a missing key would let the parent value through. + * 2. Every child carries a unique per-run marker (`HAPI_TEST_MARKER=`) + * that the final audit (see `auditTestProcesses.ts`) can use to recognize + * test-owned processes even after they have been orphaned/reparented to + * PID 1. + * + * The worker env already points at the isolated temporary hub (see + * `setup.ts`), so the hub credentials stay intact while session identity and + * well-known secrets are neutralized. + */ + +import { join } from 'node:path' +import { tmpdir } from 'node:os' + +const TEST_CONFIG_FILE = join(tmpdir(), 'hapi-test-config.json') + +/** Marker env key injected into every test child. Value is the run's tmpHome. */ +export const TEST_OWNED_MARKER_KEY = 'HAPI_TEST_MARKER' + +/** Keys/prefixes that identify the outer session and must never reach children. */ +const IDENTITY_ENV_PATTERNS: RegExp[] = [ + /^PI_/i, + /^HAPI_SESSION_/i, + /^PM2_/i, + /^pm_/, + /^PM_/, + /^HAPI_CLI_EXECUTABLE$/, +] + +/** Well-known secrets that must not leak from the dev environment into children. */ +const SECRET_ENV_PATTERNS: RegExp[] = [ + /^DB_PATH$/, + /^TELEGRAM_BOT_TOKEN$/, + /^SERVERCHAN_/i, + /^ELEVENLABS_/i, +] + +function isNeutralizedKey(key: string): boolean { + return ( + IDENTITY_ENV_PATTERNS.some((pattern) => pattern.test(key)) || + SECRET_ENV_PATTERNS.some((pattern) => pattern.test(key)) + ) +} + +/** + * Builds the environment for a test-spawned CLI child. + * + * Starts from `baseEnv` (defaults to the worker env, which already carries the + * isolated hub credentials injected by `setup.ts`), blanks identity/secret + * keys, and injects the per-run test marker. + */ +export function buildTestChildEnv(baseEnv: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = {} + for (const [key, value] of Object.entries(baseEnv)) { + if (value === undefined) continue + env[key] = isNeutralizedKey(key) ? '' : value + } + + const tmpHome = baseEnv.HAPI_HOME + if (!tmpHome) { + throw new Error('[test env] Missing HAPI_HOME — setup.ts must point the worker at the temp hub home first') + } + env[TEST_OWNED_MARKER_KEY] = tmpHome + return env +} + +/** + * The audit marker string for this run: `HAPI_TEST_MARKER=`. + * Matches the env dump produced by `ps eww`, which the final audit greps. + */ +export function testOwnedMarker(tmpHome?: string): string { + const home = tmpHome ?? process.env.HAPI_HOME + if (!home) { + throw new Error('[test env] Missing HAPI_HOME — cannot build test-owned marker') + } + return `${TEST_OWNED_MARKER_KEY}=${home}` +} diff --git a/cli/src/test/processRegistry.ts b/cli/src/test/processRegistry.ts new file mode 100644 index 00000000..9dd812fe --- /dev/null +++ b/cli/src/test/processRegistry.ts @@ -0,0 +1,165 @@ +/** + * Test-owned process/session registry for the runner integration suite. + * + * The production runner intentionally starts sessions with `detached: true` + * so they survive runner restarts — that means stopping the runner (or a + * failing test) never reaps its session children by itself. This registry is + * the suite's ownership record: every runner, runner-spawned session, and + * terminal-style process created by a test must be registered **immediately + * after spawn** (not after happy-path assertions), so cleanup runs even when + * the test body fails, times out, or is interrupted. + * + * Cleanup is two-stage: + * 1. Logical shutdown — `stopRunnerSession` per tracked session id, which + * asks the runner to terminate the session's process tree. + * 2. Bounded fallback — any tracked process (or session pid still reported + * by the runner) that is still alive is force tree-killed. + * + * The runner itself is NOT killed here — the suite stops it gracefully via + * `stopRunner()` (which also removes its state file). The final safety net + * lives in `auditTestProcesses.ts`: the suite hooks sweep every process + * carrying the run's unique marker after each test, and the globalSetup + * teardown audit reaps anything that still escaped (e.g. an agent tree that + * reparented to PID 1 before the registry tree-kill ran). + */ + +import type { ChildProcess } from 'node:child_process' +import { isProcessAlive } from '../utils/process' +import { listRunnerSessions, stopRunnerSession } from '../runner/controlClient' + +export interface RegisteredProcess { + /** Human-readable label for diagnostics, e.g. `runner-launcher`, `terminal-session`. */ + label: string + kind: 'child' | 'runner' | 'session' + pid?: number + sessionId?: string +} + +const registered: RegisteredProcess[] = [] + +/** Registers a ChildProcess immediately after spawn; auto-removes on exit. */ +export function trackChildProcess(child: ChildProcess, label: string): ChildProcess { + if (!child.pid) return child + const entry: RegisteredProcess = { label, kind: 'child', pid: child.pid } + registered.push(entry) + child.once('exit', () => { + const index = registered.indexOf(entry) + if (index >= 0) registered.splice(index, 1) + }) + return child +} + +/** Registers a runner PID (from runner.state.json) for tree-cleanup. */ +export function trackRunnerPid(pid: number, label: string): void { + if (Number.isFinite(pid) && pid > 0) { + registered.push({ label, kind: 'runner', pid }) + } +} + +/** + * Registers a runner-spawned session by its HAPI session id, immediately when + * the spawn response arrives (the child PID is only known to the runner). + */ +export function trackSession(sessionId: string, label: string): void { + if (sessionId) { + registered.push({ label, kind: 'session', sessionId }) + } +} + +export function trackedEntries(): readonly RegisteredProcess[] { + return registered +} + +function waitForAllDead(pids: number[], timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs + return new Promise((resolve) => { + const poll = () => { + const alive = pids.filter((pid) => isProcessAlive(pid)) + if (alive.length === 0 || Date.now() >= deadline) { + resolve() + return + } + setTimeout(poll, 100) + } + poll() + }) +} + +/** + * Two-stage cleanup of every registered resource. Safe to call repeatedly + * (afterEach + afterAll) — already-dead entries are skipped and pruned. + */ +export async function cleanupAllRegisteredProcesses(): Promise { + // The runner control API carries a long HTTP timeout (setup.ts raises + // HAPI_RUNNER_HTTP_TIMEOUT for the stress test), so the whole logical + // phase is bounded: a hung-but-live runner must not exhaust the hook + // budget before the process-tree fallback and marker sweep run. + const LOGICAL_PHASE_BUDGET_MS = 15_000 + + // Stage 1: logical shutdown through the runner control API (all sessions + // in parallel) + resolve any surviving session PIDs from the runner's + // own tracking for the fallback below. + const sessionEntries = registered.filter((entry) => entry.kind === 'session' && entry.sessionId) + const pidsToKill = new Set() + await Promise.race([ + (async () => { + await Promise.allSettled( + sessionEntries.map((entry) => stopRunnerSession(entry.sessionId!)) + ) + + const trackedSessionIds = new Set(sessionEntries.map((entry) => entry.sessionId)) + try { + const sessions = await listRunnerSessions() + for (const session of sessions) { + if ( + session?.happySessionId && + trackedSessionIds.has(session.happySessionId) && + typeof session.pid === 'number' && + isProcessAlive(session.pid) + ) { + pidsToKill.add(session.pid) + } + } + } catch { + // Runner unreachable — orphaned sessions are caught by the + // final audit. + } + })(), + new Promise((resolve) => setTimeout(resolve, LOGICAL_PHASE_BUDGET_MS)), + ]) + + // Stage 2: bounded termination for anything still alive. The runner + // itself is deliberately NOT killed here: it is always stopped + // via `stopRunner()` (graceful HTTP stop, which also removes its state + // file). SIGKILLing the runner would leave a stale runner.state.json that + // the next test's beforeEach can mistake for a live runner. + for (const entry of registered) { + if (entry.kind === 'session' || entry.kind === 'runner') continue + if (entry.pid && isProcessAlive(entry.pid)) { + pidsToKill.add(entry.pid) + } + } + // Kill registered roots with a bare synchronous SIGKILL: no recursive + // pgrep tree walk (unbounded under a large tree, and it would run before + // any await could race it) and no per-PID waits. Descendants are reaped + // by the unconditional marker sweep the suite runs right after this + // cleanup — every descendant inherits the run marker. + for (const pid of pidsToKill) { + try { + process.kill(pid, 'SIGKILL') + } catch { + // Already dead or racing exit; the wait below re-checks. + } + } + + await waitForAllDead([...pidsToKill], 5_000) + + // Prune dead entries so the registry does not grow across tests. + for (let i = registered.length - 1; i >= 0; i--) { + const entry = registered[i] + const pid = entry.pid + if (!pid || !isProcessAlive(pid)) { + registered.splice(i, 1) + } + } +} diff --git a/cli/vitest.config.ts b/cli/vitest.config.ts index 70469ff5..89c9432a 100644 --- a/cli/vitest.config.ts +++ b/cli/vitest.config.ts @@ -6,6 +6,14 @@ export default defineConfig({ globals: false, environment: 'node', include: ['src/**/*.test.ts'], + exclude: [ + // Runner integration tests spawn real detached runner/session + // process trees and must run serially through the dedicated + // integration project (`bun run test:integration`, see + // vitest.integration.config.ts), not inside the parallel + // unit-test suite. + '**/runner.integration.test.ts', + ], globalSetup: './src/test/globalSetup.ts', setupFiles: './src/test/setup.ts', coverage: { diff --git a/cli/vitest.integration.config.ts b/cli/vitest.integration.config.ts new file mode 100644 index 00000000..59806a38 --- /dev/null +++ b/cli/vitest.integration.config.ts @@ -0,0 +1,51 @@ +/** + * Dedicated, serial project for the runner integration suite. + * + * `runner.integration.test.ts` starts real detached runner/session process + * trees against the isolated temporary hub. It must never run inside the + * default parallel unit-test suite (see the exclude in `vitest.config.ts`); + * run it explicitly with: + * + * bun run test:integration # serial runner lifecycle coverage + * bun run test:integration:stress # + the 20-session stress test + * + * The whole file runs in a single worker (`fileParallelism: false`) so + * resource ownership stays unambiguous and the suite-level registry cleanup + * (see `src/test/processRegistry.ts`) is authoritative. + */ +import { defineConfig } from 'vitest/config' +import { resolve } from 'node:path' + +export default defineConfig({ + test: { + globals: false, + environment: 'node', + include: ['src/runner/runner.integration.test.ts'], + globalSetup: './src/test/globalSetup.ts', + setupFiles: './src/test/setup.ts', + // Real detached process trees: never parallelize this suite. + fileParallelism: false, + // beforeEach starts a real runner (state-file wait can exceed the + // default 5s hook budget on slow machines); afterEach runs the + // two-stage cleanup + marker sweep, which can take longer on hosts + // with slow process teardown. + testTimeout: 20_000, + hookTimeout: 60_000, + coverage: { + provider: 'v8', + reporter: ['text', 'json', 'html'], + exclude: [ + 'node_modules/**', + 'dist/**', + '**/*.d.ts', + '**/*.config.*', + '**/mockData/**', + ], + }, + }, + resolve: { + alias: { + '@': resolve('./src'), + }, + }, +}) diff --git a/package.json b/package.json index 0b1bfc5f..3b269a04 100644 --- a/package.json +++ b/package.json @@ -27,6 +27,7 @@ "typecheck:web": "cd web && bun run typecheck", "test": "bun run test:cli && bun run test:hub && bun run test:web && bun run test:shared", "test:cli": "cd cli && bun run test", + "test:cli:integration": "cd cli && bun run test:integration", "test:hub": "cd hub && bun run test", "test:web": "cd web && bun run test", "test:shared": "cd shared && bun run test",