diff --git a/tests/helpers.mjs b/tests/helpers.mjs index d6981197a..93cd6b351 100644 --- a/tests/helpers.mjs +++ b/tests/helpers.mjs @@ -4,8 +4,89 @@ import path from "node:path"; import process from "node:process"; import { spawnSync } from "node:child_process"; +import { + clearBrokerSession, + loadBrokerSession, + teardownBrokerSession +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; + +/** + * Workspaces handed out by makeTempDir, so cleanup can find the brokers that + * were started for them. + */ +const tempDirs = new Set(); + export function makeTempDir(prefix = "codex-plugin-test-") { - return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + tempDirs.add(dir); + return dir; +} + +/** + * Stop the broker registered for a workspace, if one is still running, and + * clear its session so nothing later reads a dead PID. + * + * The broker is deliberately long-lived so it can be reused across companion + * invocations, which means nothing in a test run ever shuts it down. Each test + * that runs the companion therefore leaves a broker and its app-server child + * behind for the lifetime of the machine. + * + * The session file has to go too. It records a bare PID, so leaving it behind + * for a stable workspace such as the repository root means a later run can read + * it and signal whatever process has since inherited that PID. + */ +export function stopBrokerFor(cwd) { + let session = null; + try { + session = loadBrokerSession(cwd); + } catch { + return; + } + + if (!session) { + return; + } + + try { + teardownBrokerSession({ + endpoint: session.endpoint ?? null, + pidFile: session.pidFile ?? null, + logFile: session.logFile ?? null, + sessionDir: session.sessionDir ?? null, + pid: Number.isFinite(session.pid) ? session.pid : null, + killProcess: (pid) => terminateProcessTree(pid) + }); + } catch { + // Best effort: teardown must never fail the suite. + } + + try { + clearBrokerSession(cwd); + } catch { + // Best effort. + } +} + +/** + * Tear down every workspace created by makeTempDir: stop its broker, then + * remove the directory. Extra workspaces (for example the repository root, + * which some tests use as the companion cwd) can be passed in. + */ +export function cleanupTempWorkspaces(extraWorkspaces = []) { + for (const cwd of [...tempDirs, ...extraWorkspaces]) { + stopBrokerFor(cwd); + } + + for (const dir of tempDirs) { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + // Best effort: a leftover temp dir is not worth failing the suite over. + } + } + + tempDirs.clear(); } export function writeExecutable(filePath, source) { diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..611d65f5e 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -1,12 +1,12 @@ import fs from "node:fs"; import path from "node:path"; -import test from "node:test"; +import test, { after } from "node:test"; import assert from "node:assert/strict"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; -import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; +import { cleanupTempWorkspaces, initGitRepo, makeTempDir, run } from "./helpers.mjs"; import { loadBrokerSession, saveBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; @@ -16,6 +16,31 @@ const SCRIPT = path.join(PLUGIN_ROOT, "scripts", "codex-companion.mjs"); const STOP_HOOK = path.join(PLUGIN_ROOT, "scripts", "stop-review-gate-hook.mjs"); const SESSION_HOOK = path.join(PLUGIN_ROOT, "scripts", "session-lifecycle-hook.mjs"); +// Companion invocations start a broker that is deliberately long-lived so it +// can be reused. Nothing else shuts it down, so without this the suite leaves a +// broker plus its app-server child behind for every workspace it touches. +// +// ROOT is a stable workspace a developer may already have a live companion +// session on, so it is only torn down when this run is the thing that started +// it: the session is sampled before any test and stopped only if it changed. +const rootBrokerPidBeforeRun = readRootBrokerPid(); + +function readRootBrokerPid() { + try { + return loadBrokerSession(ROOT)?.pid ?? null; + } catch { + return null; + } +} + +after(() => { + const rootBrokerPidAfterRun = readRootBrokerPid(); + const startedRootBroker = + rootBrokerPidAfterRun !== null && rootBrokerPidAfterRun !== rootBrokerPidBeforeRun; + + cleanupTempWorkspaces(startedRootBroker ? [ROOT] : []); +}); + async function waitFor(predicate, { timeoutMs = 5000, intervalMs = 50 } = {}) { const start = Date.now(); while (Date.now() - start < timeoutMs) {