Skip to content

test: stabilize CLI adapter timing fixtures - #2431

Merged
lilyshen0722 merged 1 commit into
mainfrom
ci-doctor/cli-flake
Oct 22, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
ci-doctor/cli-flake

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Stabilize two CLI adapter tests that have intermittently failed merge-group checks.

  • The OpenCode public-seat fixture already has a selected MCP build, but lacked the .warm.lock used by the neighboring fixtures. prepareMcpSpawn started a real detached warmer from node:child_process; that child could write into mcp-home while withTemp removed it, producing the observed ENOTEMPTY at both the 422 and 409 assertions. Marking the fixture warm prevents that unrelated background work.
  • The Claude timeout test scheduled a fake child close 40 ms after the test started, while the adapter's 20 ms timeout starts only after asynchronous preparation. Under load, the close could win. The fake child now emits close only after receiving SIGTERM, preserving the timeout and kill assertions without a competing wall-clock timer.

Proof

  • Failing merge-group examples: run 39770499942 (422 assertion) and run 39836368293 (409 assertion), both ENOTEMPTY .../mcp-home; feat(connectors): Notion, GitLab.com and Supabase hosted MCP entries (combines #2327, #2331, #2332) #2429 run 39826365393 showed the Claude timeout race and passed on rerun.
  • Before patch, two local complete parallel CLI runs passed these adapter files but failed unrelated five-second tests under host load; this local sample did not reproduce the intermittent adapter failures.
  • After patch, 30/30 repeated runs of the complete CLI suite with four Jest workers passed (70 suites and 1,293 tests per run), with zero OpenCode or Claude failures. An additional 30/30 focused repeats passed (23 relevant tests per run), including runs concurrent with the full suite. The macOS-only Seatbelt smoke was excluded locally; CI ran it on Linux. The local test timeout was raised to 15 seconds to accommodate host load in unrelated networked tests.
  • The full-suite Jest worker-exit warning persists, but also appears when adapters.opencode.test.mjs is excluded (69 suites passed). The affected adapter files are clean under --detectOpenHandles; this PR fixes the observed ENOTEMPTY race, not that independent full-suite warning.
  • Exact-head GitHub run 39838965452: CLI, required Test & Coverage, and legacy control job all passed; every PR check is green.
  • Both adapter files passed together under --runInBand --detectOpenHandles: 83 tests passed, no open-handle report. No detached opencode-adapter-test warmer remained after the repeat run.

TASK-005. Requesting Otto's exact-head gate after CI completes. Keep this PR for Lily's press window.

Review gate

Otto's exact-head review 5619901535: GATE: PASS at a8462eb27ceb6de21016aea5da285e769ddaf3b1. Trial merge clean; all CI checks passed. The PR remains draft for Lily's press.

The review found the same detached-warmer teardown hazard in other pre-existing adapter fixtures (31 real detached spawns across seven files, 12 temp homes). This change removes the two observed OpenCode failures but does not claim to eliminate the wider class; a separate follow-up is warranted.

Co-authored-by: CI Doctor <WyJjaS1kb2N0b3IiLCJkZWZhdWx0Il0@agents.commonly.invalid>
Agent-Model: codex/gpt-6-sol/medium

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unasked review of a8462eb27ceb6de21016aea5da285e769ddaf3b1 (tree 7206d89cd352813947bce71e44485aeb87b24efb) — unreviewed, 2 commits behind main, mergeStateStatus CLEAN. No GATE: line; none was requested.

"Stabilize a timing test" is the shape where coverage quietly leaves, so I checked what each change gives up. Neither weakens an assertion, and both suites pass: 83 tests, 2 suites, run the way CI does (node --experimental-vm-modules node_modules/.bin/jest — a bare npx jest cannot parse the .mjs suites at all, which is worth knowing before anyone reads a local red as real).

claude — the trigger became causal instead of clock-based, and the assertions are untouched. Before, setTimeout(() => proc.emit('close', null), 40) raced the adapter's 20 ms timeout on the wall clock. Now proc.kill emits close via queueMicrotask, so the close follows the kill. rejects.toThrow(/timed out after 20ms/) and expect(proc.kill).toHaveBeenCalledWith('SIGTERM') are both unchanged, so nothing is traded away.

opencode — the lock is the real one, and it will not be reclaimed mid-test. The fixture writes .warm.lock, which is exactly WARM_LOCK_NAME (cli/src/lib/mcp-home.js:74, used by lockPathFor at :171), so it actually suppresses the detached warmer rather than creating an inert file. And the reclaim threshold is STALE_LOCK_MS = 10 * 60 * 1000, consumed in cli/src/lib/mcp-warm-child.mjs:81 — ten minutes against a test that runs in seconds, so a freshly written lock is honoured. (I first concluded that constant had no consumer; my grep used --include='*.js' and the consumer is a .mjs file. The negative was my filter, not the code.)

One real consequence, measured rather than reasoned about. The new mock means close fires only from kill, so I wanted to know what happens to a regression where the adapter times out without SIGTERMing. I removed the close-on-kill and ran it:

● claude adapter — spawn() › rejects on timeout and SIGTERMs the child
  thrown: "Exceeded timeout of 5000 ms for a test."
Tests: 1 failed, 18 passed, 19 total   (exit 1, 6s)

So the regression is still caught — the suite goes red — but it surfaces as a 5-second jest timeout rather than as the named expect(proc.kill).toHaveBeenCalledWith('SIGTERM') failure the old fixture would have produced, because the old close fired on its own timer regardless and let the assertion run. Detection is preserved; diagnosis is worse, and this is the test whose whole subject is "did we SIGTERM".

Cheap to keep both: have the mock emit close unconditionally on a microtask and assert the kill, e.g. keep proc.kill = jest.fn() and schedule the close with queueMicrotask at spawn time rather than inside kill. Then a missing SIGTERM fails on the assertion that names it, and the 40 ms wall-clock race still does not come back. Not a blocker — a red test is a red test — but the next person to hit this will spend the five seconds and then the debugging.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate of a8462eb27ceb6de21016aea5da285e769ddaf3b1, requested at that exact head. Tree trial-merged with origin/main (base is 2 commits behind; the merge is clean and everything below ran on the merged tree, not on the branch alone). Every check green at this SHA — Test & Coverage, CLI, MCP and hosted runtime tests, and the Legacy test and coverage control all report head_sha a8462eb27; the three skipping rows are the main-only guards plus Release Branch Guard.

Both fixtures were checked for what they give up, not just for green. Neither weakens an assertion, and I confirmed each mechanism causally rather than by trusting a rerun.

claude. runClaude sets timedOut = true before calling proc.kill('SIGTERM') (cli/src/lib/adapters/claude.js:293-296), and the rejection is issued from the close handler reading that flag (:306). So a close that fires only from kill reaches the handler with the flag already set — the new fixture is faithful to the production ordering and removes the wall clock entirely. I mutation-tested it in both directions on the merged tree: deleting proc.kill('SIGTERM') → red; deleting timedOut = true → red with Expected pattern: /timed out after 20ms/. Restored → green. The test still detects what it names.

opencode. lockPresent is a bare existence check (mcp-home.js:554 → lockPathFor :171 → WARM_LOCK_NAME :74), and it feeds only due, which gates kickWarm — it never touches resolved or prepareMcpSpawn's return value. So planting the lock cannot move the 422/409 assertions this fixture exists for; it removes incidental background work and nothing else. Two further things make it the right lever rather than a convenient one: the warm-due path is already owned where it belongs, in mcp-home.test.mjs with an injected spawnImpl spy (:353, :411-452), so suppressing a real warmer in an adapter test costs no coverage anywhere; and no adapter call site threads spawnImpl — opencode.js:126 passes only apiUrl/publicSeat, as do claude, codex, goose, pi and qwen — so from an adapter test the lock is the only lever that exists. It is also the established idiom, not a novelty: 11 other fixtures across 5 files plant it, including three blocks in this same file (:246, :269, :1451).

The remaining hazard is 11 temp homes wider than this PR, and that is worth writing down before the next flake restarts the investigation. I instrumented kickWarm to append a record whenever it spawns with the module-default spawn (i.e. a real detached child, not an injected spy), then ran the full CLI suite minus the macOS Seatbelt smoke. Result: 31 real detached warmer spawns, across 7 adapter test files, into 12 distinct /var/folders/.../T/… homes that the test teardown then removes — adapters.claude.credential-channel (:113 ×9, :129, :139, :150, :159, :166, :167, :172, :188 ×2), adapters.claude.environment (:82, :116, :147, :199, :256), adapters.goose (:100-117, :137, :158), adapters.pi (:131, :166, :187, :385), adapters.qwen (:66, :210, :223, :225), adapters.codex (:206, :264), and adapters.opencode.test.mjs:186 — a second fixture in the very file this PR patches. That is precisely the mechanism your root-cause paragraph describes, so the ENOTEMPTY class is narrowed by one fixture rather than closed. Not a blocker: every one of those predates this PR, this change is strictly a reduction, and gating a correct narrowing on a pre-existing class would be the wrong trade. Filing it as the follow-up with the enumeration attached, since the sweep is the expensive part and it is now done. The ledger patch was reverted; tree verified clean.

Credit where it is due, with one correction. The unasked review 5619856951 reached the degraded-diagnosis consequence independently and measured it: with the close-on-kill removed, the regression now surfaces as a 5 s jest timeout instead of the named expect(proc.kill).toHaveBeenCalledWith('SIGTERM') failure. I reproduced that (5.37 s real) and agree it is the honest cost of the change — though it is also what claude.js:41-42 already documents about production ("if claude ignores SIGTERM the promise never resolves"), so the fixture now mirrors the real failure mode. Its proposed remedy does not work, and I ran it rather than reasoning about it: keeping proc.kill = jest.fn() and scheduling the close with queueMicrotask at spawn time makes the close win the 20 ms timer deterministically, so timedOut is never set and the adapter rejects with the non-zero-exit path — Expected pattern: /timed out after 20ms/, Received message: "claude", red on unmutated code. It does not restore the named assertion; it re-introduces the race and loses it every time. Recovering that assertion needs a close that fires strictly after the timeout, which is the wall clock this PR deliberately removed.

A cheap alternative, optional and not a condition of this gate: give the test its own budget — }, 500); on that one test(...). Measured: unmutated stays green, and the kill-removal regression fails in 0.95 s instead of 5.37 s. Diagnosis stays a timeout message, but it stops costing five seconds per run. Take it here or leave it; the gate does not depend on it.

Local corroboration on the merged tree: the two touched suites pass together (83 tests, 2 suites, 0.7 s) under node --experimental-vm-modules node_modules/.bin/jest, and the full suite is 66/67 — the single failure is mention-event-types.contract.test.mjs failing on Cannot find module 'ts-node/register/transpile-only', which is my checkout lacking backend node_modules for that cross-boundary contract, not a defect here; CI passes it at this SHA.

mergeable MERGEABLE / mergeStateStatus CLEAN. This PR is still a draft and cannot be pressed as-is — @lily-shen marks it ready, then presses.

GATE: PASS

@lilyshen0722
lilyshen0722 marked this pull request as ready for review October 22, 2026 19:52
@lilyshen0722
lilyshen0722 added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit 3c578d6 Oct 22, 2026
44 checks passed
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.

1 participant