From 39920d73a1b97fff332eb7506b96a2cb6f63c691 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 9 Aug 2026 23:55:51 +0000 Subject: [PATCH] test(launch): move broker idle-shutdown regression into collected test tree The coding-agent CI runner collects test files only from packages/coding-agent/test (scripts/ci-test-ts.ts), so the regression at src/launch/broker.test.ts was skipped by bun run test and CI. Relocated to test/launch/broker-idle-shutdown.test.ts and rewrote it to the in-process broker pattern used by the sibling broker tests: it awaits the broker's own run() promise (resolves on idle shutdown) instead of watching the PID file, so shutdown is an awaited signal rather than a poll. Fixes #8110 --- .../coding-agent/src/launch/broker.test.ts | 95 ------------------- .../test/launch/broker-idle-shutdown.test.ts | 77 +++++++++++++++ 2 files changed, 77 insertions(+), 95 deletions(-) delete mode 100644 packages/coding-agent/src/launch/broker.test.ts create mode 100644 packages/coding-agent/test/launch/broker-idle-shutdown.test.ts diff --git a/packages/coding-agent/src/launch/broker.test.ts b/packages/coding-agent/src/launch/broker.test.ts deleted file mode 100644 index 880e9ef03..000000000 --- a/packages/coding-agent/src/launch/broker.test.ts +++ /dev/null @@ -1,95 +0,0 @@ -import { afterEach, expect, test } from "bun:test"; -import * as fs from "node:fs/promises"; -import * as os from "node:os"; -import * as path from "node:path"; -import { createDaemonBrokerClient, type DaemonBrokerClient } from "./client"; - -// Broker PID lease file (internal to broker.ts); its removal is the observable -// signal that the broker process ran its shutdown path and released the lease. -const PID_FILE = "broker.pid"; - -const tempDirs: string[] = []; -let openClient: DaemonBrokerClient | undefined; - -afterEach(async () => { - openClient?.close(); - openClient = undefined; - // A regression leaves the broker running; kill it so it does not outlive the - // suite and hold the temp runtime dir. - for (const dir of tempDirs) { - try { - const raw: unknown = await Bun.file(path.join(dir, PID_FILE)).json(); - if (typeof raw === "object" && raw !== null && "pid" in raw && typeof raw.pid === "number") { - try { - process.kill(raw.pid, "SIGKILL"); - } catch { - // Already gone — the expected outcome. - } - } - } catch { - // No PID file — broker already exited. - } - } - await Promise.all(tempDirs.map(dir => fs.rm(dir, { recursive: true, force: true }))); - tempDirs.length = 0; -}); - -test("broker shuts down after its last persistent daemon exits with no clients", async () => { - const projectDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-broker-idle-project-")); - const runtimeDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-broker-idle-run-")); - tempDirs.push(projectDir, runtimeDir); - - const client = await createDaemonBrokerClient(projectDir, { runtimeDir, idleGraceMs: 200 }); - openClient = client; - - // A persistent daemon that outlives the first idle-shutdown timer (200ms) and - // then self-exits (~700ms). The first idle check must observe it live and - // return; the broker must rearm idle shutdown once it settles. - const started = await client.request({ - op: "start", - spec: { - name: "persistent-temp", - application: process.execPath, - args: ["-e", "setTimeout(() => {}, 700)"], - env: {}, - cwd: projectDir, - pty: false, - restart: "no", - persist: true, - detached: false, - }, - }); - expect(started.op).toBe("start"); - - const pidPath = path.join(runtimeDir, PID_FILE); - expect(await Bun.file(pidPath).exists()).toBe(true); - - // Disconnect the final client. The broker keeps the persistent daemon alive. - client.close(); - openClient = undefined; - - // After the daemon self-exits, terminal settlement must rearm idle shutdown so - // the broker releases its lease (removes the PID file). Await that filesystem - // signal directly rather than polling. Integration test: the broker's idle - // timer runs in a separate process against the real clock, so fake timers - // cannot drive it — the removal event is the observable shutdown proof. - let brokerExited = !(await Bun.file(pidPath).exists()); - if (!brokerExited) { - const watcher = fs.watch(runtimeDir, { signal: AbortSignal.timeout(15_000) }); - try { - for await (const event of watcher) { - if (event.filename === PID_FILE && !(await Bun.file(pidPath).exists())) { - brokerExited = true; - break; - } - } - } catch (error) { - // AbortSignal.timeout aborts the watch with AbortError/TimeoutError once - // the window elapses; that just means the broker never exited, so let the - // assertion below report it. Rethrow anything else. - const name = error instanceof Error ? error.name : ""; - if (name !== "AbortError" && name !== "TimeoutError") throw error; - } - } - expect(brokerExited).toBe(true); -}, 20_000); diff --git a/packages/coding-agent/test/launch/broker-idle-shutdown.test.ts b/packages/coding-agent/test/launch/broker-idle-shutdown.test.ts new file mode 100644 index 000000000..9302ce260 --- /dev/null +++ b/packages/coding-agent/test/launch/broker-idle-shutdown.test.ts @@ -0,0 +1,77 @@ +// Integration test — real timers are required (ts-no-test-timers exception): this drives the actual +// cross-process daemon broker running a real child process, and the bug is a missing idle-shutdown +// rearm in #settle. Fake timers cannot control the OS process-exit promise or the unix-socket RPC, +// and shutdown is observed by awaiting the broker's own run() promise — its resolution IS the signal +// (no polling, no fixed sleep). A regression leaves the broker alive, so the test's own timeout +// surfaces the failure. +import { describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as path from "node:path"; +import { TempDir } from "@oh-my-pi/pi-utils"; +import { startDaemonBrokerFromEnvironment } from "../../src/launch/broker"; +import { createDaemonBrokerClient } from "../../src/launch/client"; +import { DAEMON_IDLE_GRACE_ENV, DAEMON_PROJECT_DIR_ENV, DAEMON_RUNTIME_DIR_ENV } from "../../src/launch/protocol"; + +function restoreEnv(name: string, value: string | undefined): void { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; +} + +function startBroker(projectDir: string, runtimeDir: string, idleGraceMs: number): Promise { + const previousProjectDir = process.env[DAEMON_PROJECT_DIR_ENV]; + const previousRuntimeDir = process.env[DAEMON_RUNTIME_DIR_ENV]; + const previousGrace = process.env[DAEMON_IDLE_GRACE_ENV]; + process.env[DAEMON_PROJECT_DIR_ENV] = projectDir; + process.env[DAEMON_RUNTIME_DIR_ENV] = runtimeDir; + process.env[DAEMON_IDLE_GRACE_ENV] = String(idleGraceMs); + const broker = startDaemonBrokerFromEnvironment(); + restoreEnv(DAEMON_PROJECT_DIR_ENV, previousProjectDir); + restoreEnv(DAEMON_RUNTIME_DIR_ENV, previousRuntimeDir); + restoreEnv(DAEMON_IDLE_GRACE_ENV, previousGrace); + return broker; +} + +describe("daemon broker idle shutdown", () => { + it("shuts down after its last persistent daemon exits with no clients", async () => { + using tempDir = TempDir.createSync("@omp-launch-idle-"); + const projectDir = path.join(tempDir.path(), "project"); + const runtimeDir = path.join(tempDir.path(), "runtime"); + await fs.mkdir(projectDir); + + const previousTitle = process.title; + // Create the client (writes broker.token) before starting the broker, which reads that token. + const client = await createDaemonBrokerClient(projectDir, { runtimeDir, idleGraceMs: 200 }); + const broker = startBroker(projectDir, runtimeDir, 200); + try { + // A persistent daemon that outlives the first idle-shutdown timer (200ms) and then + // self-exits (~700ms). restart:"no" so its exit is terminal. + const started = await client.request({ + op: "start", + spec: { + name: "persistent-temp", + application: process.execPath, + args: ["-e", "setTimeout(() => {}, 700)"], + env: {}, + cwd: projectDir, + pty: false, + restart: "no", + persist: true, + detached: false, + }, + }); + expect(started.op).toBe("start"); + + // Disconnect the final client. The broker keeps the persistent daemon alive, so the + // idle timer this arms fires while the daemon is still live and returns without rearming. + client.close(); + + // When the daemon self-exits, terminal settlement must rearm idle shutdown; the broker + // then releases its lease and run() resolves. Awaiting the broker promise IS the shutdown + // signal. Before the fix nothing rearmed, so this await never resolved and the test timed + // out — the regression this guards. + await broker; + } finally { + process.title = previousTitle; + } + }, 30_000); +});