From 77b193c380835e0d2406bde24523d932bf2c4bc2 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Mon, 17 Aug 2026 00:04:58 -0500 Subject: [PATCH] fix(opencode): stop CLI test fixtures leaking processes on Windows cli-process.ts spawns CLI children with a bare "bun" argv[0]. Under bunx, PATH resolves that to the node_modules/.bin/bun.exe shim, which starts the real bun as a separate process and waits on it. The fixture only ever holds the shim, so kill() + await exited completes as soon as the shim dies while the OpenCode process keeps running with no parent. I had 35 of these alive, the oldest 46 hours, holding ports and file locks. Use process.execPath instead. Bun resolves it via selfExePath() rather than PATH, so the handle is the process the fixture means to control. Only serve() and startRun() were leaking. acp() is safe by accident (stdin EOF exits the child first) and the generic spawn() goes through CrossSpawnSpawner, which tree-kills on Windows. Changed all four anyway so the fixture never picks a different bun than the test runner. Also two test changes so this cannot silently come back: the serve teardown assertion was `typeof code === "number" || code === null`, true for every possible value - it now checks the server actually stops answering. And the SIGINT test body gets a timeout so it fails on its own terms instead of running out the test runner's clock. --- .../opencode/test/cli/run/run-process.test.ts | 12 +++--- .../test/cli/serve/serve-process.test.ts | 41 +++++++++++-------- packages/opencode/test/lib/cli-process.ts | 18 ++++---- 3 files changed, 43 insertions(+), 28 deletions(-) diff --git a/packages/opencode/test/cli/run/run-process.test.ts b/packages/opencode/test/cli/run/run-process.test.ts index bd5847e2723c..dee8d8a614ca 100644 --- a/packages/opencode/test/cli/run/run-process.test.ts +++ b/packages/opencode/test/cli/run/run-process.test.ts @@ -318,13 +318,15 @@ describe("opencode run (non-interactive subprocess)", () => { ({ llm, opencode }) => Effect.gen(function* () { yield* llm.hang - const run = yield* opencode.startRun("wait forever") - yield* llm.wait(1) - run.interrupt() - const result = yield* run.result + const result = yield* Effect.gen(function* () { + const run = yield* opencode.startRun("wait forever") + yield* llm.wait(1) + run.interrupt() + return yield* run.result + }).pipe(Effect.timeout("24 seconds")) expect(result.exitCode).not.toBe(0) - expect(result.durationMs).toBeLessThan(30_000) + expect(result.durationMs).toBeLessThan(24_000) }), 30_000, ) diff --git a/packages/opencode/test/cli/serve/serve-process.test.ts b/packages/opencode/test/cli/serve/serve-process.test.ts index 6dbea372b2cd..9be9e9e28ddf 100644 --- a/packages/opencode/test/cli/serve/serve-process.test.ts +++ b/packages/opencode/test/cli/serve/serve-process.test.ts @@ -3,8 +3,9 @@ // catches bugs spanning argv → server boot → routing → instance loading. // // `serve` is long-lived: the harness returns a handle (url/port/kill/exited) -// and kills the process when the test scope closes. The OS-assigned port is -// parsed off the "listening on http://..." line. +// and kills the process when the test scope closes. The bound port is parsed +// off the "listening on http://..." line — `--port 0` asks for 4096 first and +// falls back to an OS-assigned port only if that bind fails. import { describe, expect } from "bun:test" import { Effect } from "effect" import { HttpClient } from "effect/unstable/http" @@ -24,9 +25,11 @@ describe("opencode serve (subprocess)", () => { const client = yield* HttpClient.HttpClient const res = yield* client.get(`${server.url}/global/health`) expect(res.status).toBe(200) - // GlobalHealth schema is { success: true, ... } | { success: false, error }. + // GlobalHealth returns { healthy: true, version } (handlers/global.ts). // We don't lock in further shape here — any 200 with parseable JSON is - // enough proof the routing + auth-bypass + instance loading is alive. + // enough proof that argv → server boot → routing works. It does NOT + // prove instance loading: `serve` runs with instance: false and the + // global health route loads no instance (src/cli/effect-cmd.ts). const body = yield* res.json expect(body).toBeDefined() }), @@ -40,22 +43,28 @@ describe("opencode serve (subprocess)", () => { "kills the subprocess on scope close", ({ opencode }) => Effect.gen(function* () { - // Inner scope so we can observe `.exited` resolving after it closes. - const exitedPromise = yield* Effect.scoped( + const client = yield* HttpClient.HttpClient + const handle = yield* Effect.scoped( Effect.gen(function* () { const server = yield* opencode.serve() - // Capture the Promise, not the resolved value — scope closes after - // this gen returns, at which point the finalizer kills the child. - return server.exited + const response = yield* client.get(`${server.url}/global/health`) + expect(response.status).toBe(200) + return { url: server.url, exited: server.exited } }), ) - // After scope close: finalizer fired, process must have exited. - const code = yield* Effect.promise(() => exitedPromise) - // Bun reports the exit code; SIGTERM-killed processes return non-null - // (typically 143 on POSIX). We just require resolution within a sane - // window — anything else means the kill didn't take. - expect(typeof code === "number" || code === null).toBe(true) - }), + + yield* Effect.promise(() => handle.exited) + + const request = () => + client.get(`${handle.url}/global/health`).pipe(Effect.exit, Effect.timeout("1 second")) + const first = yield* request() + yield* Effect.sleep("75 millis") + const second = yield* request() + yield* Effect.sleep("75 millis") + const third = yield* request() + + expect([first, second, third].map((result) => result._tag)).toEqual(["Failure", "Failure", "Failure"]) + }).pipe(Effect.timeout("20 seconds")), 60_000, ) }) diff --git a/packages/opencode/test/lib/cli-process.ts b/packages/opencode/test/lib/cli-process.ts index 12e8d9c866a5..f54d35646f85 100644 --- a/packages/opencode/test/lib/cli-process.ts +++ b/packages/opencode/test/lib/cli-process.ts @@ -106,7 +106,9 @@ export type RunOpts = SpawnOpts & { // `opencode serve` is a long-lived process — it never exits on its own. // `serve(opts)` therefore returns a handle inside the caller's Scope: the // subprocess is killed when the scope closes (test end), and the URL the -// server actually bound to (port 0 means OS-assigned) is parsed off stdout. +// server actually bound to is parsed off stdout. Note `--port 0` does not mean +// "OS-assigned": the server asks for 4096 first and only falls back to an +// OS-assigned port if that bind fails (src/server/server.ts). export type ServeOpts = SpawnOpts & { readonly port?: number readonly hostname?: string @@ -211,7 +213,7 @@ export function withCliFixture( // on `Bun.stdin.text()` (see src/cli/cmd/run.ts — non-TTY stdin is // consumed as the prompt). The old Process.run wrapper defaulted to // ignore; ChildProcess.make defaults to pipe, so we set it explicitly. - const command = ChildProcess.make("bun", ["run", "--conditions=browser", cliEntry, ...args], { + const command = ChildProcess.make(process.execPath, ["run", "--conditions=browser", cliEntry, ...args], { cwd: home, env: { ...env, ...opts?.env }, extendEnv: true, @@ -283,7 +285,7 @@ export function withCliFixture( const options = runOpts(opts) const proc = yield* Effect.acquireRelease( Effect.sync(() => - Bun.spawn(["bun", "run", "--conditions=browser", cliEntry, ...runArgs(message, opts)], { + Bun.spawn([process.execPath, "run", "--conditions=browser", cliEntry, ...runArgs(message, opts)], { cwd: home, env: { ...process.env, ...env, ...options?.env }, stdin: "ignore", @@ -313,8 +315,10 @@ export function withCliFixture( const serve = Effect.fn("opencode.serve")(function* (opts?: ServeOpts) { const argv = ["serve"] - // Default port 0 — let the OS pick a free port, parse the actual one - // off stdout. Hard-coded ports flake under parallel tests. + // Default port 0 — the server tries 4096 first and falls back to an + // OS-assigned port only if that bind fails (src/server/server.ts); the + // actual port is parsed off stdout. Hard-coded ports flake under + // parallel tests. argv.push("--port", String(opts?.port ?? 0)) if (opts?.hostname) argv.push("--hostname", opts.hostname) if (opts?.extraArgs) argv.push(...opts.extraArgs) @@ -324,7 +328,7 @@ export function withCliFixture( // as a finalizer error during test teardown. const proc = yield* Effect.acquireRelease( Effect.sync(() => - Bun.spawn(["bun", "run", "--conditions=browser", cliEntry, ...argv], { + Bun.spawn([process.execPath, "run", "--conditions=browser", cliEntry, ...argv], { cwd: home, env: { ...process.env, ...env, ...opts?.env }, stdout: "pipe", @@ -395,7 +399,7 @@ export function withCliFixture( // Either way we await proc.exited so the test scope doesn't leak. const proc = yield* Effect.acquireRelease( Effect.sync(() => - Bun.spawn(["bun", "run", "--conditions=browser", cliEntry, ...argv], { + Bun.spawn([process.execPath, "run", "--conditions=browser", cliEntry, ...argv], { cwd: opts?.cwd ?? home, env: { ...process.env, ...env, ...opts?.env }, stdin: "pipe",