diff --git a/.changeset/quiet-daemons-report.md b/.changeset/quiet-daemons-report.md new file mode 100644 index 000000000..30ba292dc --- /dev/null +++ b/.changeset/quiet-daemons-report.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Report the underlying health-probe failure when a reachable session daemon port cannot be verified. diff --git a/src/session/agent/commands.daemon.test.ts b/src/session/agent/commands.daemon.test.ts index f5c240a4c..c63051a6a 100644 --- a/src/session/agent/commands.daemon.test.ts +++ b/src/session/agent/commands.daemon.test.ts @@ -87,7 +87,9 @@ describe("resolveDaemonAvailability with a foreign process on the port", () => { action: "list", output: "json", } satisfies SessionCommandInput), - ).rejects.toThrow(/already in use/); + ).rejects.toThrow( + /already in use.*Hunk health probe returned HTTP 404 after \d+ms.*busy Hunk daemon or another process/, + ); } finally { server.stop(true); } diff --git a/src/session/agent/commands.ts b/src/session/agent/commands.ts index e5cfca35f..04452eacd 100644 --- a/src/session/agent/commands.ts +++ b/src/session/agent/commands.ts @@ -6,9 +6,10 @@ import type { import type { SessionLiveCommentSummary, SessionReviewNoteSummary } from "../types"; import { NO_ACTIVE_SESSIONS_MESSAGE } from "./errors"; import { + describeSessionBrokerHealthProbeFailure, ensureSessionBrokerAvailable, - isSessionBrokerHealthy, isLoopbackPortReachable, + probeSessionBrokerHealth, readSessionBrokerHealth, waitForSessionBrokerShutdown, } from "../broker/brokerLauncher"; @@ -151,16 +152,18 @@ async function ensureRequiredAction(action: SessionDaemonAction, selector?: Sess async function resolveDaemonAvailability(action: SessionCommandInput["action"]) { const config = resolveSessionBrokerConfig(); - const healthy = await isSessionBrokerHealthy(config); - if (healthy) { + const healthProbe = await probeSessionBrokerHealth(config); + if (healthProbe.kind === "healthy") { return true; } const portReachable = await isLoopbackPortReachable(config); if (portReachable) { + const diagnostic = describeSessionBrokerHealthProbeFailure(healthProbe); throw new Error( - `Hunk session daemon port ${config.host}:${config.port} is already in use by another process. ` + - `Stop the conflicting process or set HUNK_MCP_PORT to a different loopback port.`, + `Hunk session daemon port ${config.host}:${config.port} is already in use, and the listener's ` + + `Hunk health probe ${diagnostic}. The listener may be a busy Hunk daemon or another process. ` + + `Retry, stop the conflicting process, or set HUNK_MCP_PORT to a different loopback port.`, ); } diff --git a/src/session/broker/brokerLauncher.test.ts b/src/session/broker/brokerLauncher.test.ts index e3b9d6097..3f0f611b2 100644 --- a/src/session/broker/brokerLauncher.test.ts +++ b/src/session/broker/brokerLauncher.test.ts @@ -1,17 +1,21 @@ import { afterEach, describe, expect, test } from "bun:test"; import type { ChildProcess } from "node:child_process"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { platform, tmpdir } from "node:os"; import { join } from "node:path"; import { + describeSessionBrokerHealthProbeFailure, ensureSessionBrokerAvailable, isLoopbackPortReachable, parseSessionBrokerHealth, + probeSessionBrokerHealth, + readSessionBrokerHealth, resolveDaemonLaunchCommand, resolveSessionBrokerRuntimePaths, } from "./brokerLauncher"; const tempDirs: string[] = []; +const timeoutProbeTest = platform() === "win32" ? test.skip : test; const testConfig = { host: "127.0.0.1", port: 47657, @@ -19,6 +23,15 @@ const testConfig = { wsOrigin: "ws://127.0.0.1:47657", }; +function createTestBrokerConfig(port: number) { + return { + host: "127.0.0.1", + port, + httpOrigin: `http://127.0.0.1:${port}`, + wsOrigin: `ws://127.0.0.1:${port}`, + }; +} + function createRuntimeDir() { const dir = mkdtempSync(join(tmpdir(), "hunk-session-daemon-launcher-test-")); tempDirs.push(dir); @@ -53,6 +66,94 @@ describe("session daemon launcher", () => { expect(parseSessionBrokerHealth(value)).toBeNull(); } }); + + test("retains the health payload for a successful probe", async () => { + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + fetch: () => Response.json({ ok: true, pid: 123, sessions: 1 }), + }); + const config = createTestBrokerConfig(server.port!); + + try { + const result = await probeSessionBrokerHealth(config); + expect(result).toMatchObject({ + kind: "healthy", + health: { ok: true, pid: 123, sessions: 1 }, + }); + } finally { + server.stop(true); + } + }); + + test("retains HTTP status while nullable health readers stay compatible", async () => { + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + fetch: () => new Response("unavailable", { status: 503 }), + }); + const config = createTestBrokerConfig(server.port!); + + try { + await expect(probeSessionBrokerHealth(config)).resolves.toMatchObject({ + kind: "http-status", + status: 503, + }); + await expect(readSessionBrokerHealth(config)).resolves.toBeNull(); + } finally { + server.stop(true); + } + }); + + test("distinguishes invalid JSON from incompatible health payloads", async () => { + let response: "json" | "payload" = "json"; + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + fetch: () => + response === "json" + ? new Response("not-json", { headers: { "content-type": "application/json" } }) + : Response.json({ ok: "yes" }), + }); + const config = createTestBrokerConfig(server.port!); + + try { + await expect(probeSessionBrokerHealth(config)).resolves.toMatchObject({ + kind: "invalid-json", + }); + response = "payload"; + await expect(probeSessionBrokerHealth(config)).resolves.toMatchObject({ + kind: "invalid-response", + }); + } finally { + server.stop(true); + } + }); + + timeoutProbeTest("retains timeout budget and observed probe latency", async () => { + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + fetch: async () => { + await Bun.sleep(100); + return Response.json({ ok: true }); + }, + }); + const config = createTestBrokerConfig(server.port!); + + try { + const result = await probeSessionBrokerHealth(config, 10); + expect(result).toMatchObject({ kind: "timeout", timeoutMs: 10 }); + if (result.kind !== "timeout") throw new Error("Expected a timeout health probe result."); + expect(result.elapsedMs).toBeGreaterThanOrEqual(10); + expect(describeSessionBrokerHealthProbeFailure(result)).toMatch( + /^timed out after 10ms \(probe elapsed \d+ms\)$/, + ); + } finally { + server.stop(true); + } + }); + test("reuses the current script entrypoint when Hunk is running from source or a JS wrapper", () => { expect(resolveDaemonLaunchCommand(["bun", "src/main.tsx", "diff"], "/usr/bin/bun")).toEqual({ command: "/usr/bin/bun", diff --git a/src/session/broker/brokerLauncher.ts b/src/session/broker/brokerLauncher.ts index 3976484df..f7cf30afc 100644 --- a/src/session/broker/brokerLauncher.ts +++ b/src/session/broker/brokerLauncher.ts @@ -322,6 +322,49 @@ export interface SessionBrokerHealth { staleSessionTtlMs?: number; } +type SessionBrokerHealthProbeResult = + | { kind: "healthy"; health: SessionBrokerHealth } + | { kind: "http-status"; status: number; elapsedMs: number } + | { kind: "invalid-json"; elapsedMs: number } + | { kind: "invalid-response"; elapsedMs: number } + | { kind: "timeout"; timeoutMs: number; elapsedMs: number } + | { kind: "request-error"; message: string; elapsedMs: number }; + +type SessionBrokerHealthProbeFailure = Exclude; + +/** Round one failed probe duration for stable, human-readable diagnostics. */ +function healthProbeElapsedMs(startedAt: number) { + return Math.max(0, Math.round(performance.now() - startedAt)); +} + +/** Bound one runtime-generated transport error before it reaches a terminal. */ +function healthProbeErrorMessage(error: unknown) { + const raw = error instanceof Error ? error.message || error.name : String(error); + return ( + raw + .replace(/[\u0000-\u001f\u007f-\u009f]/g, " ") + .replace(/\s+/g, " ") + .trim() + .slice(0, 240) || "Unknown request error" + ); +} + +/** Describe one failed health probe for the final user-facing CLI error. */ +export function describeSessionBrokerHealthProbeFailure(failure: SessionBrokerHealthProbeFailure) { + switch (failure.kind) { + case "http-status": + return `returned HTTP ${failure.status} after ${failure.elapsedMs}ms`; + case "invalid-json": + return `returned invalid JSON after ${failure.elapsedMs}ms`; + case "invalid-response": + return `returned an incompatible health payload after ${failure.elapsedMs}ms`; + case "timeout": + return `timed out after ${failure.timeoutMs}ms (probe elapsed ${failure.elapsedMs}ms)`; + case "request-error": + return `failed after ${failure.elapsedMs}ms (${failure.message})`; + } +} + /** Parse the minimal or legacy-rich health response without trusting cross-process JSON. */ export function parseSessionBrokerHealth(value: unknown): SessionBrokerHealth | null { try { @@ -376,13 +419,18 @@ export function parseSessionBrokerHealth(value: unknown): SessionBrokerHealth | } } -/** Read the daemon's health payload when one is reachable on the configured loopback port. */ -export async function readSessionBrokerHealth( +/** Probe daemon health while retaining the failure evidence needed for a terminal CLI error. */ +export async function probeSessionBrokerHealth( config: ResolvedSessionBrokerConfig = resolveSessionBrokerConfig(), timeoutMs = 500, -) { +): Promise { + const startedAt = performance.now(); const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), timeoutMs); + let timedOut = false; + const timeout = setTimeout(() => { + timedOut = true; + controller.abort(); + }, timeoutMs); timeout.unref?.(); try { @@ -390,23 +438,56 @@ export async function readSessionBrokerHealth( signal: controller.signal, }); if (!response.ok) { - return null; + return { + kind: "http-status", + status: response.status, + elapsedMs: healthProbeElapsedMs(startedAt), + }; } - return parseSessionBrokerHealth(await response.json()); - } catch { - return null; + let payload: unknown; + try { + payload = await response.json(); + } catch (error) { + if (timedOut) throw error; + return { + kind: "invalid-json", + elapsedMs: healthProbeElapsedMs(startedAt), + }; + } + + const health = parseSessionBrokerHealth(payload); + return health + ? { kind: "healthy", health } + : { kind: "invalid-response", elapsedMs: healthProbeElapsedMs(startedAt) }; + } catch (error) { + return timedOut + ? { kind: "timeout", timeoutMs, elapsedMs: healthProbeElapsedMs(startedAt) } + : { + kind: "request-error", + message: healthProbeErrorMessage(error), + elapsedMs: healthProbeElapsedMs(startedAt), + }; } finally { clearTimeout(timeout); } } +/** Read the daemon's health payload while preserving the nullable compatibility contract. */ +export async function readSessionBrokerHealth( + config: ResolvedSessionBrokerConfig = resolveSessionBrokerConfig(), + timeoutMs = 500, +) { + const result = await probeSessionBrokerHealth(config, timeoutMs); + return result.kind === "healthy" ? result.health : null; +} + /** Check whether the loopback session broker already answers health probes. */ export async function isSessionBrokerHealthy( config: ResolvedSessionBrokerConfig = resolveSessionBrokerConfig(), timeoutMs = 500, ) { - return (await readSessionBrokerHealth(config, timeoutMs))?.ok === true; + return (await probeSessionBrokerHealth(config, timeoutMs)).kind === "healthy"; } /** Check whether some local process is already accepting TCP connections on the daemon port. */