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",