Skip to content

fix(opencode): stop CLI test fixtures leaking processes on Windows - #42990

Open
NamedIdentity wants to merge 1 commit into
anomalyco:devfrom
NamedIdentity:fix/cli-fixture-execpath
Open

fix(opencode): stop CLI test fixtures leaking processes on Windows#42990
NamedIdentity wants to merge 1 commit into
anomalyco:devfrom
NamedIdentity:fix/cli-fixture-execpath

Conversation

@NamedIdentity

Copy link
Copy Markdown

Issue for this PR

Closes #42989

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Changes the four places packages/opencode/test/lib/cli-process.ts picks the bun executable, from the string "bun" to process.execPath.

"bun" gets resolved through PATH. Under bunx that finds the node_modules\.bin\bun.exe shim, which starts the real bun as a separate process and waits on it — so the fixture is holding the shim, not the process it thinks it owns. kill() + await exited then completes as soon as the shim dies, while the actual OpenCode process keeps running with no parent. On my machine that had accumulated 35 leftover processes. process.execPath is the bun already running the tests (bun resolves it via selfExePath(), not PATH), so the handle points at the process we actually want to control.

Only serve() and startRun() were leaking. acp() is safe by accident (stdin EOF exits the child first) and the generic spawn() is safe because it goes through CrossSpawnSpawner, which tree-kills on Windows. I changed all four anyway so the fixture never picks a different bun than the test runner.

Two test changes so this can't 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.

How did you verify your code works?

I wrote both tests first and ran them against the unchanged code to confirm they fail:

  • serve: got three HTTP 200s from the server after exited had already resolved
  • SIGINT: run.result never settled, ran to the 30s timeout

Then changed the four selectors and reran the same test files unmodified:

  • serve: passes in 2.1s, all three requests refused at the transport level
  • SIGINT: passes in 2.9s

Full test/cli run afterwards: 398 tests, zero leftover processes. The same run left 3 behind before the change. Full bun test from packages/opencode: 16 failures, all of which were already failing on my machine before I touched anything (symlink tests and one Zed path test). Typecheck clean.

This branch is the change applied to current dev (5a0e07e), and I reran both tests there as well — serve passes in 2.1s, SIGINT in 2.8s, no leftover processes, typecheck clean.

To confirm it yourself on Windows: run any CLI subprocess test through bunx, then look for leftover bun.exe processes whose parent no longer exists.

One limitation worth flagging — this only reproduces where bunx puts the shim on PATH. The Windows CI job installs bun through setup-bun, so no shim is generated there and CI wouldn't catch a regression in this.

Screenshots / recordings

N/A — not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

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.
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

  1. packages/opencode/test/lib/cli-process.ts:109/:288/:331/:402 — process.execPath instead of bare "bun" is the right root-cause fix for Windows leaks (no PATH/PATHEXT resolution, no shim .cmd wrapper swallowing signals) — worth one comment at each call site noting why execPath matters on win32, or the next fixture author will "simplify" it back.

  2. packages/opencode/test/cli/serve/serve-process.test.ts:46 — asserting three consecutive connection Failures after scope close is much stronger than the old exit-code shape check (it actually proves the port stopped serving, catching zombie/orphaned servers on Windows where SIGTERM handling differs) — good upgrade. Minor: Effect.sleep("75 millis") pacing is arbitrary; a short retry loop asserting "still failing for ≥500ms" would be equally strong with less timing sensitivity.

  3. packages/opencode/test/cli/run/run-process.test.ts:321 — wrapping in Effect.timeout("24 seconds") plus asserting durationMs < 24_000 double-bounds the same thing while leaving only ~6s of headroom inside the 30s test timeout for spawn + llm.hang setup on cold CI runners — consider keeping the outer timeout as the guard and dropping the durationMs assertion to a looser bound (e.g. <28s), or measuring only the post-interrupt window.

  4. Nit — packages/opencode/test/lib/cli-process.ts:106 — thanks for documenting the --port 0 ≠ OS-assigned reality; suggest also surfacing the parsed URL in the failure message when stdout never contains "listening on", since that's the current failure mode when 4096 is occupied and parsing silently hangs.

Overall: focused, well-commented fix for real Windows CI pain, with the serve-dead verification being a genuinely better invariant than what it replaces. Nothing blocking. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] CLI test fixtures leaking processes on Windows

2 participants