diff --git a/runtime/client.ts b/runtime/client.ts index 9c30845..6d6ae86 100644 --- a/runtime/client.ts +++ b/runtime/client.ts @@ -58,10 +58,15 @@ export async function callRuntime( }; const body = JSON.stringify(scrubPayload(rawEnvelope)); - try { - const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), timeoutMs); + const controller = new AbortController(); + // Cleared in `finally`, not after the await: when the runtime is unreachable + // the fetch rejects, and a timer cleared only on the success path stays + // pending for its full duration. It keeps the event loop alive, so the host + // process hangs for `timeoutMs` at exit on every call that falls back to the + // CLI -- which is every call on a machine without the runtime running. + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { const res = await fetch(`${RUNTIME_BASE}${endpoint}`, { method: "POST", headers: { "Content-Type": "application/json" }, @@ -69,7 +74,6 @@ export async function callRuntime( signal: controller.signal, }); - clearTimeout(timer); if (!res.ok) return null; const json = (await res.json()) as RuntimeResponse; if (typeof json.preview_markdown === "string") { @@ -78,6 +82,8 @@ export async function callRuntime( return json; } catch { return null; + } finally { + clearTimeout(timer); } } @@ -91,19 +97,20 @@ export async function getRuntime( ): Promise { const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS; - try { - const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), timeoutMs); + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { const res = await fetch(`${RUNTIME_BASE}${endpoint}`, { signal: controller.signal, }); - clearTimeout(timer); if (!res.ok) return null; return (await res.json()) as RuntimeResponse; } catch { return null; + } finally { + clearTimeout(timer); } } @@ -112,15 +119,18 @@ export async function getRuntime( * Returns false on any failure. */ export async function isRuntimeAvailable(): Promise { - try { - const controller = new AbortController(); - setTimeout(() => controller.abort(), HEALTH_TIMEOUT_MS); + const controller = new AbortController(); + // This one was never captured at all, so it leaked on every path. + const timer = setTimeout(() => controller.abort(), HEALTH_TIMEOUT_MS); + try { const res = await fetch(`${RUNTIME_BASE}/v2/status`, { signal: controller.signal, }); return res.ok; } catch { return false; + } finally { + clearTimeout(timer); } } diff --git a/tests/tools.test.ts b/tests/tools.test.ts index 3740afe..f3e3342 100644 --- a/tests/tools.test.ts +++ b/tests/tools.test.ts @@ -180,3 +180,128 @@ describe("PluginHookContract", () => { expect(typeof reg["tool.execute.after"]).toBe("function"); }); }); + + +// ─── NonZeroExitDiagnostics (Ix#539) ───────────────────────────────────────── +// +// Several `ix` commands 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. Bun's `$` throws on a non-zero exit and `.text()` discards stdout along +// with it, so those diagnostics vanished and the tool fell back to a generic +// "Not found in graph" — losing the guidance ix had actually supplied. +// +// These run in a CHILD process. Bun's shell resolves binaries from the real +// process PATH: neither mutating `process.env.PATH` nor `$.env({PATH})` / +// `.env({PATH})` redirects it, so an in-process stub is silently ignored and +// the test would exercise the developer's real `ix` against their real graph. +// A child process with its own PATH is the only way to stub it, and it has the +// side benefit of covering the actual spawn path. + +import { mkdtempSync, writeFileSync, chmodSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +const TOOL_PATH = path.resolve(import.meta.dir, "../tools/ix-docs-tool.ts"); + +async function runToolWithStubIx(stubScript: string | null): Promise { + const dir = mkdtempSync(path.join(tmpdir(), "ix-stub-")); + try { + if (stubScript !== null) { + const bin = path.join(dir, "ix"); + writeFileSync(bin, `#!/bin/sh\n${stubScript}\n`); + chmodSync(bin, 0o755); + } + + const runner = path.join(dir, "runner.ts"); + writeFileSync( + runner, + `import * as tool from ${JSON.stringify(TOOL_PATH)};\n` + + `const out = await tool.execute({ target: "SomeSymbol", depth: "brief" }, { directory: ${JSON.stringify(dir)} });\n` + + `process.stdout.write(out);\n`, + ); + + // PATH is exactly the stub dir plus the system dirs `sh` needs, so the real + // `ix` cannot be reached even if one is installed. + // process.execPath, not "bun": the restricted PATH below deliberately does + // not contain bun's own directory. + const proc = Bun.spawn([process.execPath, runner], { + env: { ...process.env, PATH: `${dir}${path.delimiter}/usr/bin${path.delimiter}/bin` }, + stdout: "pipe", + stderr: "pipe", + }); + const output = await new Response(proc.stdout).text(); + await proc.exited; + return output; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +describe("NonZeroExitDiagnostics", () => { + test("keeps the JSON body when ix exits non-zero", async () => { + const output = await runToolWithStubIx(` +case "$1" in + locate) echo '{"resolvedTarget":null,"resolutionMode":"none","diagnostics":["No graph entity found."]}'; exit 1 ;; + overview) echo '{"summary":"Overview body retained."}'; exit 1 ;; + *) echo '{}'; exit 1 ;; +esac`); + + // The generic fallback would mean both payloads were thrown away. + expect(output).not.toContain("**Not found in graph.**"); + expect(output).toContain("Overview body retained."); + }); + + test("still reports not-found when ix exits non-zero with no output", async () => { + const output = await runToolWithStubIx("exit 1"); + expect(output).toContain("**Not found in graph.**"); + }); + + test("still reports not-found when ix is absent entirely", async () => { + const output = await runToolWithStubIx(null); + expect(output).toContain("**Not found in graph.**"); + }); +}); + +// ─── RuntimeClientTimers ───────────────────────────────────────────────────── +// +// `callRuntime` cleared its abort timer only after a successful fetch, so when +// the runtime was unreachable — the normal case on a machine without it, which +// is exactly why the tools have a CLI fallback — the timer stayed pending for +// its full 5s. A pending timer keeps the event loop alive, so the host process +// hung for five seconds at exit on every single call. `isRuntimeAvailable` +// never captured its timer at all. +// +// Invisible in-process (the call itself returns in ~20ms); only the exit is +// delayed. So this measures how long a child takes to *exit* after the call +// resolves. + +test("runtime client does not hold the process open after an unreachable call", async () => { + const dir = mkdtempSync(path.join(tmpdir(), "ix-timer-")); + try { + const runner = path.join(dir, "runner.ts"); + const client = path.resolve(import.meta.dir, "../runtime/client.ts"); + writeFileSync( + runner, + `const client = await import(${JSON.stringify(client)});\n` + + // Port 9 (discard) refuses immediately, so any delay is the leaked timer. + `await client.callRuntime("/v2/ix_query", {}, { dir: ${JSON.stringify(dir)} });\n` + + `await client.isRuntimeAvailable();\n`, + ); + + const started = Date.now(); + const proc = Bun.spawn([process.execPath, runner], { + env: { ...process.env, IX_RUNTIME_URL: "http://127.0.0.1:9" }, + stdout: "ignore", + stderr: "ignore", + }); + await proc.exited; + const elapsed = Date.now() - started; + + // Was ~5000ms (callRuntime) + ~2000ms (isRuntimeAvailable) before the fix. + // A generous ceiling still separates "exits promptly" from "waits out a + // 5s timer", without being flaky on a slow runner. + expect(elapsed).toBeLessThan(3000); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}, 20_000); diff --git a/tools/ix-docs-tool.ts b/tools/ix-docs-tool.ts index 2f08982..602c2c6 100644 --- a/tools/ix-docs-tool.ts +++ b/tools/ix-docs-tool.ts @@ -197,11 +197,26 @@ function formatOverview(overview: { return lines.join("\n") + "\n"; } +/** + * Run an `ix` command, keeping stdout even when it exits non-zero. + * + * Bun's `$` throws on a non-zero exit and `.text()` then discards stdout + * entirely. That is fine while every `ix` command exits 0, but several already + * exit 1 to signal "asked for something that does not exist" while still + * printing a useful JSON body -- and `locate` is about to join them (Ix#539). + * Without `.nothrow()` those diagnostics vanish and the tool falls back to a + * generic "Not found in graph", losing the specific guidance ix supplied. + * + * `.nothrow()` returns "" for a missing binary and for a failure with no + * output, both of which still map to null, so the ix-unavailable path is + * unchanged. + */ async function safeRun( cmd: ReturnType ): Promise { try { - return await cmd.text(); + const out = await cmd.nothrow().text(); + return out.trim() ? out : null; } catch { return null; }