Repository navigation
test: stabilize CLI adapter timing fixtures - #2431
Conversation
Co-authored-by: CI Doctor <WyJjaS1kb2N0b3IiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: codex/gpt-6-sol/medium
lilyshen0722
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
Summary
Stabilize two CLI adapter tests that have intermittently failed merge-group checks.
.warm.lockused by the neighboring fixtures.prepareMcpSpawnstarted a real detached warmer fromnode:child_process; that child could write intomcp-homewhilewithTempremoved it, producing the observedENOTEMPTYat both the 422 and 409 assertions. Marking the fixture warm prevents that unrelated background work.SIGTERM, preserving the timeout and kill assertions without a competing wall-clock timer.Proof
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.adapters.opencode.test.mjsis excluded (69 suites passed). The affected adapter files are clean under--detectOpenHandles; this PR fixes the observedENOTEMPTYrace, not that independent full-suite warning.Test & Coverage, and legacy control job all passed; every PR check is green.--runInBand --detectOpenHandles: 83 tests passed, no open-handle report. No detachedopencode-adapter-testwarmer 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: PASSata8462eb27ceb6de21016aea5da285e769ddaf3b1. 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.