fix(tools): keep ix stdout when the command exits non-zero - #20
Merged
Conversation
`callRuntime` and `getRuntime` cleared their abort timer only after a successful fetch. When the runtime is unreachable the fetch rejects, so the timer stayed pending for its full 5s -- and a pending timer keeps the event loop alive, so the host process hung for five seconds at exit on every call. That is the normal case, not an edge case: the tools have a CLI fallback precisely because most machines do not run the Core Runtime, and every one of those calls paid the five seconds. `isRuntimeAvailable` never captured its timer at all, so it leaked HEALTH_TIMEOUT_MS on every path including success. Move all three into `finally`. Measured against an unreachable runtime: the call returns in ~20ms and the process now exits at 28ms instead of 5010ms. Invisible in-process -- only the exit is delayed -- so the regression test measures how long a child takes to exit after the call resolves.
Bun's `$` throws on a non-zero exit and `.text()` discards stdout along with
it, so `safeRun` returned null for any `ix` command that failed. Several `ix`
commands already exit 1 to mean "you asked for something that does not exist"
while still printing a useful JSON body, and `locate` is about to join them
(Ix#539) -- at which point ix-docs-tool would lose the diagnostics ix supplied
and fall back to a generic "Not found in graph".
`.nothrow()` keeps stdout. It returns "" both for a missing binary and for a
failure with no output, and both still map to null, so the ix-unavailable path
is unchanged.
This is step 1 of Ix#539's sequence: the plugins must tolerate a non-zero exit
before the CLI starts producing one.
The tests run in a child process. Bun's shell resolves binaries from the real
process PATH -- neither mutating `process.env.PATH` nor `$.env({PATH})` nor
`.env({PATH})` redirects it, so an in-process stub is silently ignored and the
test would run the developer's real `ix` against their real graph. A child with
its own PATH is the only way to stub it, and it covers the real spawn path.
Refs ix-infrastructure/Ix#539
KageBinary
added a commit
that referenced
this pull request
Sep 1, 2026
#20 landed first and appended its own `NonZeroExitDiagnostics` block to the end of `tests/tools.test.ts`; this branch appends `LiveIxCli` to the same place. Git saw one conflict region covering both additions. They are independent -- `LiveIxCli` is opt-in behind `IX_LIVE_TESTS` and drives the tools against a real `ix`, while `NonZeroExitDiagnostics` runs a stubbed `ix` in a child process -- so both are kept, and both sets of imports survived the merge. bun test: 119 pass, 3 skip, 0 fail.
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.
Step 1 of the sequence ix-infrastructure/Ix#539 lays out: the plugins must tolerate a non-zero exit before the CLI starts producing one. That issue names this plugin explicitly as a reason
ix locatecannot simply be fixed.Two commits — the second is what #539 asks for; the first is a bug I hit while testing it and could not leave.
1.
fix(runtime): abort timers leaked on the failure pathcallRuntimeandgetRuntimecleared their abort timer only after a successful fetch. When the runtime is unreachable the fetch rejects, so the timer stayed pending for its full 5s — and a pending timer keeps the event loop alive, so the host process hung for five seconds at exit on every call.isRuntimeAvailablenever captured its timer at all, leaking on every path including success.This is the normal case, not an edge case: these tools have a CLI fallback precisely because most machines do not run the Core Runtime.
Measured against an unreachable runtime — the call itself returns in ~20ms, so only the exit is delayed:
Found because it made the tests below time out at bun's 5s default.
2.
fix(tools): keep stdout when ix exits non-zeroBun's
$throws on a non-zero exit and.text()discards stdout with it, sosafeRunreturned null for any failed command — and ix-docs-tool fell back to a generic "Not found in graph", discarding the diagnostics ix had supplied..nothrow()keeps stdout. It returns""both for a missing binary and for a failure with no output, and both still map to null, so the ix-unavailable path is unchanged.The tests run in a child process, deliberately
Bun's shell resolves binaries from the real process PATH. Neither mutating
process.env.PATHnor$.env({PATH})nor.env({PATH})redirects it — I verified all three. An in-process stub is silently ignored, and the test then runs the developer's realixagainst their real graph (which is exactly what happened on my first attempt). A child process with its own PATH is the only way to stub it, and it covers the real spawn path as a bonus.Verification
Each new test was checked to fail without its fix, not just pass with it:
safeRunreverted,keeps the JSON body when ix exits non-zerofails on its assertion in 54ms — showing exactly the #539 symptom rather than a timeout.Full suite 95 passed / 0 failed (91 existing + 4 new), 649ms.