Forget a CLI probe killed by its own deadline - #71
Merged
Merged
Conversation
A 5s execFile deadline on 'claude --version' and '--help' cached its own timeout, so one busy turn permanently withheld every version-gated flag, the skill bridge and /btw from the opencode process. That is what made test-side-question's native /btw spec flaky under load. Keep the deadline, drop only the answer that described the machine, and make the specs resolve their probes up front instead of racing them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
test-side-question.ts"provider /btw uses native control response between normal turns on the same CLI process" is not flaky because of anything in/btw. It loses a race againstdetectCliVersion's own five-secondexecFiledeadline, and the damage sticks because the answer is cached.Timeline inside a failing run:
detectCliVersion(cliPath). On a loaded machine thatclaude --versionspawn overruns 5,000 ms,execFilekills it, the probe returnsnulland caches it percliPathfor the life of the process./btwturn readscliSupportsSideQuestion(null) === false,requestSideQuestionthrows "/btw requires Claude Code CLI 2.1.258 or newer", the stream carries anerrorpart and notext-delta, andansweris"".Evidence
Deterministic, by making the fixture CLI burn 5.5 s on
--version:against the reported
5723 msand the same assertion.Natural, which is what makes it a product bug and not a slow fixture: 40 concurrent copies of the file at a load average of 66 to 76 failed 6 of 40, and the plugin log carried exactly 6
failed to detect claude cli versionlines, one per failing run. The slowest surviving turn took 5,874 ms.The other two reports are the same class
Same trick, same shape, both confirmed:
test-skill-bridge.ts"a skill Claude already loads is not bridged..."--helpresolveSkillPluginDirsreturns[], dies ondirs[0]!withERR_INVALID_ARG_TYPEafter 5,484 mstest-startup-diagnostics.ts"detectOpencodeVersion reads the version from the opencode binary"sleep 6beforeecho 1.18.5undefinedafter 5,295 msOne class: a fixed five-second child-process probe whose timeout degrades silently into a wrong value a spec asserts against.
The fix, and why at that layer
Product (
src/cli-version.ts), because the caching really is a bug.detectCliVersionanddetectCliSupportsFlagcached theirnull/falseforever. Every version gate reads that one answer, so a single busy five seconds during a first turn silently and permanently withheld--thinking-display summarized, fast mode,--restricted(the read-only preset's first layer),--permission-prompts,/btwand the skill bridge's--plugin-dirfrom the whole opencode session, with one WARN in a log that is off by default as the only sign.Now a probe our own deadline killed is not cached:
killedByDeadline(err)iskilled === truewith a stringsignaland no stringcode(execFilesetskilledonly when this process killed the child; amaxBufferoverflow carriesERR_CHILD_PROCESS_STDIO_MAXBUFFER, a plain refusal carries a numeric exit code), andforgetDeadlineKilldeletes the entry once the probe settles, only while the entry is still the one it created. Every other failure (missing binary, non-zero exit, unparseable output) describes the binary, repeats, and stays cached, so a broken binary is still one spawn percliPath.The deadline itself is kept. It bounds how long a wedged
claudemay hold opencode's first turn; raising it makes that case worse and still loses on a busy enough machine. Losing a probe now costs one extra spawn on a later turn.Tests, because the deadline is the product's contract. The specs no longer race it:
test-side-question.tsandtest-btw-command.tsresolve the fake CLI's version up front viawarmCliVersion(), which asks again when a probe was killed, and assert it./btwis gated on that answer, so a spec must not discover it mid-turn.test-skill-bridge.tsresolves--plugin-dirsupport insidefakeClifor any fixture meant to advertise it; every later call reads the cache.test-startup-diagnostics.tsresets and re-probes until the script answers.AbortSignal.timeout(5_000)in both/btwfixtures becameTURN_HANG_STOP_MS = 30_000, named and commented as a hang-stop rather than a deadline. A real spawn reached 5,874 ms under load, so the old budget was itself the flake it was meant to catch; the node test timeouts (60 s) are the real backstop.test-cli-probe-cache.ts(new) covers the rule. It was rewritten once for the same sin: its first version asserted spawn counts and, at load 112, the trivialexit 3fixture was itself deadline-killed in 20 of 20 runs. It now asks again until the fixture answered for itself and reads "was this cached" off promise identity, which no load can change.Files changed
src/cli-version.ts:killedByDeadline,forgetDeadlineKill,PROBE_TIMEOUT_MS, both probes evict a deadline kill and logdeadlineKill.test-cli-probe-cache.ts: new, 2 tests.test-side-question.ts,test-btw-command.ts:warmCliVersion(),TURN_HANG_STOP_MS, test timeouts to 60 s.test-skill-bridge.ts:fakeCliis async and resolves the flag probe.test-startup-diagnostics.ts: reset-and-retry arounddetectOpencodeVersion.package.json: new file appended at the end of thetestlist.AGENTS.md,docs/agents-history.md: the rule and its evidence,(h #g181).Checks
Load generation for every "under load" run below: 72
yes > /dev/nullprocesses on 18 cores, killed afterwards.Reproduction, before and after
/btwspecfailed to detect claude cli version/btwspecGate, on a quiet machine (
npm run typecheck && npm test > /tmp/lane-btw.log 2>&1; echo EXIT=$?)Not verified / deliberately left
doStream's prologue is dropped, and this PR does not fix it.detectCliVersionis awaited before theReadableStreamis built, and the abort listener is registered insidestart;addEventListener("abort", ...)on an already-aborted signal never fires. That is why turn 1 above passed at 5,874 ms against a 5,000 ms signal. For/btwit is harmless, but for a normal turn an operator's stop during a slow prologue does not stop the CLI's turn. Fixing it changes abort semantics across the whole turn path and belongs in its own change. Recorded in(h #g181).test-skill-bridge.tsfailures and thetest-startup-diagnostics.tsone were reproduced by injection, not observed naturally on this machine; I did not have their original failure output, only the names and the ~5 s timing.detectOpencodeVersionkeeps caching its own timeout. It is read once per process for a diagnostic line, so there is no second caller to benefit from eviction; only its spec changed.npm run buildwas not run (nothing here touches the build).