From cbd2a1babace8ab9a0f5145069463e370920a991 Mon Sep 17 00:00:00 2001 From: Pritom Mazumdar Date: Mon, 22 Jun 2026 13:14:46 +0530 Subject: [PATCH] fix(desktop): attach to a serving daemon instead of spawning a doomed child (#373) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(desktop): attach to a serving daemon instead of spawning a doomed child Launching the Electron app while a standalone `ao daemon` already owns the port made the Electron-spawned child daemon log "daemon already running … refusing to start" and exit 1, instead of attaching to the running daemon. `inspectExistingDaemon` only attaches when ~/.ao/running.json agrees with a live daemon, so any run-file divergence (missing/stale/unparseable file, dead PID, or a /healthz pid mismatch) made it return null — and there was no independent port probe before spawn(), so Electron spawned into an occupied port and the Go bind guard correctly refused. Add a defensive direct probe of http://127.0.0.1:/healthz in startDaemonInner, after the run-file check and before spawn(): if a genuine daemon answers, attach to it (the same "ready" DaemonStatus shape the run-file path returns) instead of spawning. The expected port is resolved the same way startup does (AO_PORT or the default). The attach-or-spawn decision is extracted into a pure, dependency-injected module (shared/daemon-attach.ts) so it can be exercised directly; main.ts keeps ownership of fs reads, process signals, fetch, and the path identity check. Covered by 27 tests, including real loopback-server cases that reproduce the issue scenario end to end. Fixes #367 Co-Authored-By: Claude Opus 4.8 * chore: format with prettier [skip ci] * fix(desktop): enforce readiness + identity checks on the port-probe attach path Address review feedback on #367: the new direct port probe attached to any service-matching daemon as soon as /healthz returned ok, without the /readyz and foreign-binary identity checks the run-file path enforces. That reopened the mismatch daemonIdentityError was built to prevent — Electron could silently drive a different/older AO build serving the port — and could mark a still-starting daemon "ready". Extract the shared post-handshake tail (readinessStatus) and run it from both paths, anchoring on the PID /healthz reports for the port probe. A serving daemon that is not ready, or whose binary the identity check refuses, now yields the same "error" DaemonStatus instead of attaching — strictly safer than spawning, which would only collide on the occupied port and die. resolveDaemonFromPort now takes the same identityError dependency resolveDaemonFromRunFile does; main.ts passes daemonIdentityError(launch, …). Adds port-path tests for not-ready, foreign-binary identity (unit), and a real-server foreign-binary scenario (e2e). 30 tests in daemon-attach. Co-Authored-By: Claude Opus 4.8 * chore: format with prettier [skip ci] * docs(desktop): note why the port probe uses the expected (not hardcoded) port Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 Co-authored-by: github-actions[bot] --- frontend/src/main.ts | 104 ++--- frontend/src/shared/daemon-attach.test.ts | 494 ++++++++++++++++++++++ frontend/src/shared/daemon-attach.ts | 172 ++++++++ 3 files changed, 708 insertions(+), 62 deletions(-) create mode 100644 frontend/src/shared/daemon-attach.test.ts create mode 100644 frontend/src/shared/daemon-attach.ts diff --git a/frontend/src/main.ts b/frontend/src/main.ts index 4c2aeaf59..e513fb463 100644 --- a/frontend/src/main.ts +++ b/frontend/src/main.ts @@ -21,6 +21,13 @@ import { pathToFileURL } from "node:url"; import { type DaemonLaunchSpec, resolveDaemonLaunch } from "./shared/daemon-launch"; import { createListenPortScanner, defaultRunFilePath, parseRunFile } from "./shared/daemon-discovery"; import type { DaemonStatus } from "./shared/daemon-status"; +import { + type DaemonProbe, + expectedDaemonPort, + parseDaemonProbe, + resolveDaemonFromPort, + resolveDaemonFromRunFile, +} from "./shared/daemon-attach"; import { DEFAULT_POSTHOG_HOST, DEFAULT_POSTHOG_PROJECT_KEY } from "./shared/posthog-config"; import { buildTelemetryBootstrap } from "./shared/telemetry"; import { createBrowserViewHost, type BrowserViewHost } from "./main/browser-view-host"; @@ -196,15 +203,6 @@ const RUN_FILE_POLL_MS = 300; // clock reading and ours race within normal scheduling jitter. const RUN_FILE_FRESHNESS_SKEW_MS = 2_000; const DAEMON_PROBE_TIMEOUT_MS = 2_000; -const DAEMON_SERVICE_NAME = "agent-orchestrator-daemon"; - -type DaemonProbe = { - status: string; - service: string; - pid: number; - executablePath?: string; - workingDirectory?: string; -}; function runFilePath(): string | null { if (process.env.AO_RUN_FILE) return process.env.AO_RUN_FILE; @@ -252,17 +250,7 @@ async function readDaemonProbe(port: number, endpoint: "healthz" | "readyz"): Pr try { const response = await net.fetch(`http://127.0.0.1:${port}/${endpoint}`, { signal: controller.signal }); if (!response.ok) return null; - const body = (await response.json()) as Partial; - if (body.status !== (endpoint === "healthz" ? "ok" : "ready")) return null; - if (body.service !== DAEMON_SERVICE_NAME) return null; - if (typeof body.pid !== "number" || !Number.isInteger(body.pid)) return null; - return { - status: body.status, - service: body.service, - pid: body.pid, - executablePath: typeof body.executablePath === "string" ? body.executablePath : undefined, - workingDirectory: typeof body.workingDirectory === "string" ? body.workingDirectory : undefined, - }; + return parseDaemonProbe(endpoint, await response.json()); } catch { return null; } finally { @@ -297,49 +285,20 @@ function daemonIdentityError(launch: DaemonLaunchSpec, probe: DaemonProbe): stri async function inspectExistingDaemon(launch: DaemonLaunchSpec): Promise { const handshakePath = runFilePath(); - if (!handshakePath) return null; - let contents: string; - try { - contents = await readFile(handshakePath, "utf8"); - } catch { - return null; + let runFileContents: string | null = null; + if (handshakePath) { + try { + runFileContents = await readFile(handshakePath, "utf8"); + } catch { + runFileContents = null; + } } - const info = parseRunFile(contents); - if (!info || !processAlive(info.pid)) return null; - - const health = await readDaemonProbe(info.port, "healthz"); - if (!health || health.pid !== info.pid) return null; - const ready = await readDaemonProbe(info.port, "readyz"); - if (!ready || ready.pid !== info.pid) { - return { - state: "error", - port: info.port, - pid: info.pid, - executablePath: health.executablePath, - workingDirectory: health.workingDirectory, - message: "An AO daemon is already running, but it is not ready yet.", - }; - } - - const identityError = daemonIdentityError(launch, ready); - if (identityError) { - return { - state: "error", - port: info.port, - pid: info.pid, - executablePath: ready.executablePath, - workingDirectory: ready.workingDirectory, - message: identityError, - }; - } - - return { - state: "ready", - port: info.port, - pid: info.pid, - executablePath: ready.executablePath, - workingDirectory: ready.workingDirectory, - }; + return resolveDaemonFromRunFile({ + runFileContents, + isProcessAlive: processAlive, + probe: readDaemonProbe, + identityError: (probe) => daemonIdentityError(launch, probe), + }); } async function refreshDaemonStatus(): Promise { @@ -412,6 +371,27 @@ async function startDaemonInner(startEpoch: number): Promise { return daemonStatus; } + // Defensive: inspectExistingDaemon only attaches when the run-file agrees with + // a live daemon. Any divergence (missing/stale/unparseable run-file, dead PID, + // health.pid mismatch) makes it return null — yet a daemon may still be serving + // the port. Spawning then would just make the Go child refuse and exit 1. Probe + // the expected port directly, independent of the run-file, and attach if a + // daemon answers. The expected port (AO_PORT or the default) is exactly the + // port the Go child would bind and collide on — probing a hardcoded 3001 would + // miss an AO_PORT override. + const directDaemon = await resolveDaemonFromPort({ + expectedPort: expectedDaemonPort(process.env), + probe: readDaemonProbe, + identityError: (probe) => daemonIdentityError(launch, probe), + }); + if (startEpoch !== daemonStartEpoch) { + return daemonStatus; + } + if (directDaemon) { + setDaemonStatus(directDaemon); + return daemonStatus; + } + if (launch.source === "bundled" && !existsSync(launch.command)) { setDaemonStatus({ state: "error", diff --git a/frontend/src/shared/daemon-attach.test.ts b/frontend/src/shared/daemon-attach.test.ts new file mode 100644 index 000000000..12dba66fd --- /dev/null +++ b/frontend/src/shared/daemon-attach.test.ts @@ -0,0 +1,494 @@ +import { createServer, type Server } from "node:http"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + DAEMON_SERVICE_NAME, + DEFAULT_DAEMON_PORT, + type DaemonProbe, + type DaemonProber, + expectedDaemonPort, + parseDaemonProbe, + resolveDaemonFromPort, + resolveDaemonFromRunFile, +} from "./daemon-attach"; + +// A run-file the daemon would write: pid+port+timestamp, as JSON. +function runFile(pid: number, port: number): string { + return JSON.stringify({ pid, port, startedAt: "2026-06-10T16:15:04Z" }); +} + +// A probe map keyed by `${port}:${endpoint}` → fake an HTTP probe deterministically. +function fakeProbe(responses: Record): DaemonProber { + return (port, endpoint) => Promise.resolve(responses[`${port}:${endpoint}`] ?? null); +} + +const ALIVE = () => true; +const DEAD = () => false; +const NO_IDENTITY_ERROR = () => null; + +describe("expectedDaemonPort", () => { + it("defaults to 3001 when AO_PORT is unset or empty", () => { + expect(expectedDaemonPort({})).toBe(3001); + expect(expectedDaemonPort({ AO_PORT: "" })).toBe(3001); + expect(DEFAULT_DAEMON_PORT).toBe(3001); + }); + + it("honors a valid AO_PORT override", () => { + expect(expectedDaemonPort({ AO_PORT: "3037" })).toBe(3037); + expect(expectedDaemonPort({ AO_PORT: "1" })).toBe(1); + expect(expectedDaemonPort({ AO_PORT: "65535" })).toBe(65535); + }); + + it("falls back to the default for an out-of-range or non-integer AO_PORT", () => { + expect(expectedDaemonPort({ AO_PORT: "0" })).toBe(3001); + expect(expectedDaemonPort({ AO_PORT: "70000" })).toBe(3001); + expect(expectedDaemonPort({ AO_PORT: "3001.5" })).toBe(3001); + expect(expectedDaemonPort({ AO_PORT: "not-a-number" })).toBe(3001); + expect(expectedDaemonPort({ AO_PORT: "-1" })).toBe(3001); + }); +}); + +describe("parseDaemonProbe", () => { + const healthBody = { status: "ok", service: DAEMON_SERVICE_NAME, pid: 4242 }; + const readyBody = { status: "ready", service: DAEMON_SERVICE_NAME, pid: 4242 }; + + it("accepts a well-formed healthz body", () => { + expect(parseDaemonProbe("healthz", healthBody)).toEqual({ + status: "ok", + service: DAEMON_SERVICE_NAME, + pid: 4242, + executablePath: undefined, + workingDirectory: undefined, + }); + }); + + it("accepts a well-formed readyz body and carries identity fields", () => { + expect(parseDaemonProbe("readyz", { ...readyBody, executablePath: "/bin/ao", workingDirectory: "/work" })).toEqual({ + status: "ready", + service: DAEMON_SERVICE_NAME, + pid: 4242, + executablePath: "/bin/ao", + workingDirectory: "/work", + }); + }); + + it("rejects a status that does not match the endpoint", () => { + expect(parseDaemonProbe("healthz", readyBody)).toBeNull(); + expect(parseDaemonProbe("readyz", healthBody)).toBeNull(); + }); + + it("rejects a foreign or missing service", () => { + expect(parseDaemonProbe("healthz", { ...healthBody, service: "something-else" })).toBeNull(); + expect(parseDaemonProbe("healthz", { status: "ok", pid: 1 })).toBeNull(); + }); + + it("rejects a missing or non-integer pid", () => { + expect(parseDaemonProbe("healthz", { status: "ok", service: DAEMON_SERVICE_NAME })).toBeNull(); + expect(parseDaemonProbe("healthz", { ...healthBody, pid: 1.5 })).toBeNull(); + expect(parseDaemonProbe("healthz", { ...healthBody, pid: "4242" })).toBeNull(); + }); + + it("rejects non-object bodies", () => { + expect(parseDaemonProbe("healthz", null)).toBeNull(); + expect(parseDaemonProbe("healthz", "ok")).toBeNull(); + expect(parseDaemonProbe("healthz", 200)).toBeNull(); + }); + + it("drops identity fields that are not strings", () => { + const probe = parseDaemonProbe("healthz", { ...healthBody, executablePath: 5, workingDirectory: {} }); + expect(probe?.executablePath).toBeUndefined(); + expect(probe?.workingDirectory).toBeUndefined(); + }); +}); + +describe("resolveDaemonFromRunFile", () => { + it("returns null when there is no run-file (caller falls through to port probe)", async () => { + const probe = vi.fn(); + const result = await resolveDaemonFromRunFile({ + runFileContents: null, + isProcessAlive: ALIVE, + probe, + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + expect(probe).not.toHaveBeenCalled(); // never probes without a parseable run-file + }); + + it("returns null for an unparseable run-file", async () => { + const result = await resolveDaemonFromRunFile({ + runFileContents: "{not json", + isProcessAlive: ALIVE, + probe: fakeProbe({}), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + }); + + it("returns null when the recorded pid is not alive (#367 divergence)", async () => { + const result = await resolveDaemonFromRunFile({ + runFileContents: runFile(4242, 3001), + isProcessAlive: DEAD, + probe: fakeProbe({ + "3001:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 4242 }, + }), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + }); + + it("returns null when the health probe fails (#367 divergence)", async () => { + const result = await resolveDaemonFromRunFile({ + runFileContents: runFile(4242, 3001), + isProcessAlive: ALIVE, + probe: fakeProbe({ "3001:healthz": null }), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + }); + + it("returns null when health.pid disagrees with the run-file pid (#367 divergence)", async () => { + const result = await resolveDaemonFromRunFile({ + runFileContents: runFile(4242, 3001), + isProcessAlive: ALIVE, + probe: fakeProbe({ + "3001:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 9999 }, + }), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + }); + + it("reports an error (not a spawn) when the daemon is up but not ready", async () => { + const result = await resolveDaemonFromRunFile({ + runFileContents: runFile(4242, 3001), + isProcessAlive: ALIVE, + probe: fakeProbe({ + "3001:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 4242 }, + "3001:readyz": null, + }), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toEqual({ + state: "error", + port: 3001, + pid: 4242, + executablePath: undefined, + workingDirectory: undefined, + message: "An AO daemon is already running, but it is not ready yet.", + }); + }); + + it("surfaces a foreign-daemon identity error", async () => { + const result = await resolveDaemonFromRunFile({ + runFileContents: runFile(4242, 3001), + isProcessAlive: ALIVE, + probe: fakeProbe({ + "3001:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 4242 }, + "3001:readyz": { status: "ready", service: DAEMON_SERVICE_NAME, pid: 4242, workingDirectory: "/other" }, + }), + identityError: () => "Another AO daemon is already running from /other.", + }); + expect(result).toMatchObject({ + state: "error", + pid: 4242, + port: 3001, + message: "Another AO daemon is already running from /other.", + }); + }); + + it("attaches (ready) when the run-file, liveness, health, readiness, and identity all agree", async () => { + const result = await resolveDaemonFromRunFile({ + runFileContents: runFile(4242, 3037), + isProcessAlive: ALIVE, + probe: fakeProbe({ + "3037:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 4242 }, + "3037:readyz": { + status: "ready", + service: DAEMON_SERVICE_NAME, + pid: 4242, + executablePath: "/bin/ao", + workingDirectory: "/work/backend", + }, + }), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toEqual({ + state: "ready", + port: 3037, + pid: 4242, + executablePath: "/bin/ao", + workingDirectory: "/work/backend", + }); + }); +}); + +describe("resolveDaemonFromPort", () => { + it("returns null when nothing valid answers the port (caller spawns)", async () => { + const result = await resolveDaemonFromPort({ + expectedPort: 3001, + probe: fakeProbe({}), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + }); + + it("attaches (ready) when a daemon answers /healthz and /readyz on the expected port", async () => { + const result = await resolveDaemonFromPort({ + expectedPort: 3001, + probe: fakeProbe({ + "3001:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 777 }, + "3001:readyz": { + status: "ready", + service: DAEMON_SERVICE_NAME, + pid: 777, + executablePath: "/bin/ao", + workingDirectory: "/work", + }, + }), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toEqual({ + state: "ready", + port: 3001, + pid: 777, + executablePath: "/bin/ao", + workingDirectory: "/work", + }); + }); + + it("reports an error (not a spawn) when the serving daemon is not ready yet", async () => { + const result = await resolveDaemonFromPort({ + expectedPort: 3001, + probe: fakeProbe({ + "3001:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 777 }, + "3001:readyz": null, + }), + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toEqual({ + state: "error", + port: 3001, + pid: 777, + executablePath: undefined, + workingDirectory: undefined, + message: "An AO daemon is already running, but it is not ready yet.", + }); + }); + + it("reports an identity error (not a silent attach) for a foreign daemon binary on the port", async () => { + const result = await resolveDaemonFromPort({ + expectedPort: 3001, + probe: fakeProbe({ + "3001:healthz": { status: "ok", service: DAEMON_SERVICE_NAME, pid: 777 }, + "3001:readyz": { status: "ready", service: DAEMON_SERVICE_NAME, pid: 777, executablePath: "/old/ao" }, + }), + identityError: (probe) => + probe.executablePath === "/new/ao" + ? null + : `Another AO daemon is already running from ${probe.executablePath}.`, + }); + expect(result).toMatchObject({ + state: "error", + port: 3001, + pid: 777, + message: "Another AO daemon is already running from /old/ao.", + }); + }); + + it("probes exactly the expected port", async () => { + const probe = vi.fn().mockResolvedValue(null); + await resolveDaemonFromPort({ expectedPort: 4317, probe, identityError: NO_IDENTITY_ERROR }); + expect(probe).toHaveBeenCalledWith(4317, "healthz"); + }); +}); + +// End-to-end against a REAL loopback HTTP server, exercising the actual network +// probe (Node's global fetch, mirroring main.ts's readDaemonProbe) and the real +// startup decision order: run-file first, then the #367 port-probe backstop. +describe("end-to-end against a real daemon server", () => { + const servers: Server[] = []; + + afterEach(async () => { + await Promise.all(servers.splice(0).map((s) => new Promise((r) => s.close(() => r())))); + }); + + // Stand up a server on an ephemeral port. `service` lets us simulate a foreign + // (non-AO) server squatting on the port. + function startServer(opts: { + pid: number; + service?: string; + executablePath?: string; + workingDirectory?: string; + }): Promise { + const service = opts.service ?? DAEMON_SERVICE_NAME; + const server = createServer((req, res) => { + const url = req.url ?? ""; + const base = { + service, + pid: opts.pid, + executablePath: opts.executablePath, + workingDirectory: opts.workingDirectory, + }; + if (url === "/healthz") { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ status: "ok", ...base })); + return; + } + if (url === "/readyz") { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ status: "ready", ...base })); + return; + } + res.writeHead(404); + res.end(); + }); + servers.push(server); + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const addr = server.address(); + resolve(typeof addr === "object" && addr ? addr.port : 0); + }); + }); + } + + // Faithful copy of main.ts's readDaemonProbe, but with global fetch in place of + // electron's net.fetch — so this test exercises the real HTTP round-trip + parse. + const realProbe: DaemonProber = async (port, endpoint) => { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), 2_000); + try { + const response = await fetch(`http://127.0.0.1:${port}/${endpoint}`, { signal: controller.signal }); + if (!response.ok) return null; + return parseDaemonProbe(endpoint, await response.json()); + } catch { + return null; + } finally { + clearTimeout(timer); + } + }; + + // The real startup decision: attach via run-file if possible, else fall back to + // the direct port probe (the #367 fix), else null → caller spawns. + async function startupDecision(opts: { + runFileContents: string | null; + isProcessAlive: (pid: number) => boolean; + expectedPort: number; + identityError?: (probe: DaemonProbe) => string | null; + }) { + const identityError = opts.identityError ?? NO_IDENTITY_ERROR; + const fromRunFile = await resolveDaemonFromRunFile({ + runFileContents: opts.runFileContents, + isProcessAlive: opts.isProcessAlive, + probe: realProbe, + identityError, + }); + if (fromRunFile) return fromRunFile; + return resolveDaemonFromPort({ expectedPort: opts.expectedPort, probe: realProbe, identityError }); + } + + it("attaches to a genuinely serving daemon via the direct port probe", async () => { + const port = await startServer({ pid: 555 }); + const result = await resolveDaemonFromPort({ + expectedPort: port, + probe: realProbe, + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toEqual({ + state: "ready", + port, + pid: 555, + executablePath: undefined, + workingDirectory: undefined, + }); + }); + + it("returns null when the port is closed (caller spawns)", async () => { + const port = await startServer({ pid: 1 }); + // Close the only server so the port is now refused. + await Promise.all(servers.splice(0).map((s) => new Promise((r) => s.close(() => r())))); + const result = await resolveDaemonFromPort({ + expectedPort: port, + probe: realProbe, + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + }); + + it("does NOT attach to a foreign (non-AO) server squatting on the port", async () => { + const port = await startServer({ pid: 1, service: "some-other-service" }); + const result = await resolveDaemonFromPort({ + expectedPort: port, + probe: realProbe, + identityError: NO_IDENTITY_ERROR, + }); + expect(result).toBeNull(); + }); + + // A foreign AO daemon (correct service, wrong binary) serving the port. The + // identity check must surface an error rather than silently attach — the same + // guard the run-file path enforces, now enforced on the port-probe path too. + it("surfaces an identity error for a foreign AO binary serving the port (does not silently attach)", async () => { + const port = await startServer({ pid: 909, executablePath: "/old/build/ao", workingDirectory: "/old/build" }); + const result = await startupDecision({ + runFileContents: null, // run-file diverged, so we reach the port probe + isProcessAlive: ALIVE, + expectedPort: port, + identityError: (probe) => + probe.executablePath === "/expected/ao" + ? null + : `Another AO daemon is already running from ${probe.executablePath}.`, + }); + expect(result).toMatchObject({ + state: "error", + port, + pid: 909, + message: "Another AO daemon is already running from /old/build/ao.", + }); + }); + + // THE #367 SCENARIO: a standalone `ao daemon` is serving the port, but the + // run-file diverges — here it names a DEAD pid (e.g. a stale handshake from a + // crashed launch). Pre-fix this fell through to spawn() and the Go child + // refused with exit 1. Post-fix the port probe attaches instead. + it("attaches when a daemon serves the port but the run-file names a dead pid", async () => { + const port = await startServer({ pid: 6060, executablePath: "/bin/ao" }); + const result = await startupDecision({ + runFileContents: runFile(4242, port), // stale pid 4242 ... + isProcessAlive: DEAD, // ... which is no longer alive + expectedPort: port, + }); + expect(result).toEqual({ + state: "ready", + port, + pid: 6060, // attached to the daemon actually serving the port + executablePath: "/bin/ao", + workingDirectory: undefined, + }); + }); + + // #367 VARIANT: run-file pid is alive, but /healthz reports a different pid + // (run-file points at the wrong process). Old code returned null → spawn. + it("attaches when the run-file pid disagrees with the daemon's reported pid", async () => { + const port = await startServer({ pid: 8080 }); + const result = await startupDecision({ + runFileContents: runFile(4242, port), + isProcessAlive: ALIVE, + expectedPort: port, + }); + expect(result).toMatchObject({ state: "ready", port, pid: 8080 }); + }); + + it("still attaches via the run-file path when everything agrees (no regression)", async () => { + const port = await startServer({ + pid: 4242, + executablePath: "/work/backend/ao", + workingDirectory: "/work/backend", + }); + const result = await startupDecision({ + runFileContents: runFile(4242, port), + isProcessAlive: ALIVE, + expectedPort: port, + identityError: (probe) => (probe.pid === 4242 ? null : "wrong daemon"), + }); + expect(result).toMatchObject({ state: "ready", port, pid: 4242 }); + }); +}); diff --git a/frontend/src/shared/daemon-attach.ts b/frontend/src/shared/daemon-attach.ts new file mode 100644 index 000000000..31bc702fd --- /dev/null +++ b/frontend/src/shared/daemon-attach.ts @@ -0,0 +1,172 @@ +// Deciding whether to ATTACH to an already-running daemon or SPAWN a fresh one. +// +// Two independent signals are consulted, in order: +// 1. the running.json handshake file (whatever the last daemon wrote), and +// 2. a direct probe of the expected port, independent of the run-file. +// +// (2) is the defensive backstop for issue #367: a standalone `ao daemon` may be +// serving the port while running.json is missing, stale, unparseable, names a +// dead PID, or reports a PID that disagrees with /healthz. In every one of those +// cases the run-file check yields null; without the port probe the supervisor +// would spawn a child daemon that the Go bind guard then refuses ("daemon +// already running … refusing to start") and exits 1. +// +// These functions are kept side-effect free and dependency-injected (no node:* +// or electron imports — the vite-plugin-electron-renderer polyfill breaks node:* +// under vitest, see daemon-discovery.ts) so they can be exercised directly; the +// Electron main process owns the real fs reads, process signals, fetch, and +// path identity check. + +import type { DaemonStatus } from "./daemon-status"; +import { parseRunFile } from "./daemon-discovery"; + +// The daemon's default bind port (backend/internal/config). AO_PORT overrides it. +export const DEFAULT_DAEMON_PORT = 3001; +// The `service` field every genuine AO daemon stamps on its health payloads. +export const DAEMON_SERVICE_NAME = "agent-orchestrator-daemon"; + +export type DaemonProbe = { + status: string; + service: string; + pid: number; + executablePath?: string; + workingDirectory?: string; +}; + +/** A /healthz|/readyz probe of a loopback port; resolves null when nothing valid answers. */ +export type DaemonProber = (port: number, endpoint: "healthz" | "readyz") => Promise; + +/** + * The port a freshly spawned daemon is expected to bind: AO_PORT when set and + * valid, otherwise the daemon's default. Used to probe for an already-serving + * daemon before spawning a child that would only refuse and exit. + */ +export function expectedDaemonPort(env: Record): number { + const configured = env.AO_PORT ? Number(env.AO_PORT) : NaN; + return Number.isInteger(configured) && configured >= 1 && configured <= 65535 ? configured : DEFAULT_DAEMON_PORT; +} + +/** + * Validate a /healthz or /readyz JSON body against the daemon contract. Returns + * the typed probe, or null when the body is the wrong shape, status, or service + * (e.g. some unrelated server happens to occupy the port). + */ +export function parseDaemonProbe(endpoint: "healthz" | "readyz", body: unknown): DaemonProbe | null { + if (typeof body !== "object" || body === null) return null; + const candidate = body as Partial; + if (candidate.status !== (endpoint === "healthz" ? "ok" : "ready")) return null; + if (candidate.service !== DAEMON_SERVICE_NAME) return null; + if (typeof candidate.pid !== "number" || !Number.isInteger(candidate.pid)) return null; + return { + status: candidate.status, + service: candidate.service, + pid: candidate.pid, + executablePath: typeof candidate.executablePath === "string" ? candidate.executablePath : undefined, + workingDirectory: typeof candidate.workingDirectory === "string" ? candidate.workingDirectory : undefined, + }; +} + +export type RunFileResolveDeps = { + /** running.json contents, or null when the file has no path or could not be read. */ + runFileContents: string | null; + isProcessAlive: (pid: number) => boolean; + probe: DaemonProber; + /** Foreign-daemon check (dev/bundled identity); returns a message, or null when it is ours. */ + identityError: (probe: DaemonProbe) => string | null; +}; + +/** + * Attach decision driven by the running.json handshake. Returns: + * - a "ready" status → attach to the recorded daemon, + * - an "error" status → a daemon is up but unusable (not ready / foreign); + * surface it rather than spawn, + * - null → the run-file is absent/stale/inconsistent; the caller + * should fall through to {@link resolveDaemonFromPort}. + */ +export async function resolveDaemonFromRunFile(deps: RunFileResolveDeps): Promise { + const { runFileContents, isProcessAlive, probe, identityError } = deps; + if (runFileContents === null) return null; + const info = parseRunFile(runFileContents); + if (!info || !isProcessAlive(info.pid)) return null; + + const health = await probe(info.port, "healthz"); + // The recorded PID must match the live daemon; otherwise the run-file points + // at the wrong process — return null so the caller falls through to the port + // probe rather than trusting a stale handshake. + if (!health || health.pid !== info.pid) return null; + return readinessStatus(info.port, info.pid, health, probe, identityError); +} + +export type PortProbeResolveDeps = { + expectedPort: number; + probe: DaemonProber; + /** Foreign-daemon check (dev/bundled identity); returns a message, or null when it is ours. */ + identityError: (probe: DaemonProbe) => string | null; +}; + +/** + * Attach decision driven by a direct /healthz probe of the expected port, + * independent of the run-file (issue #367 backstop). Returns: + * - a "ready" status → attach to the daemon serving the port, + * - an "error" status → a daemon serves the port but is unusable (not ready, or + * a foreign binary the identity check refuses); surface it + * rather than spawn (spawning would only collide and die), + * - null → nothing genuine answers the port; the caller should spawn. + * + * This mirrors {@link resolveDaemonFromRunFile}'s post-handshake validation + * (/readyz + identity), anchoring on the PID /healthz reports instead of the + * run-file's, so attaching via the port is no laxer than attaching via the file. + */ +export async function resolveDaemonFromPort(deps: PortProbeResolveDeps): Promise { + const { expectedPort, probe, identityError } = deps; + const health = await probe(expectedPort, "healthz"); + if (!health) return null; + return readinessStatus(expectedPort, health.pid, health, probe, identityError); +} + +/** + * Shared tail of both attach paths: given a daemon confirmed serving /healthz on + * `port` with PID `pid`, confirm it is ready and is the daemon we expect, and + * build the resulting DaemonStatus. Returns an "error" status (never null) — by + * here a daemon is definitely occupying the port, so spawning is never the right + * move. + */ +async function readinessStatus( + port: number, + pid: number, + health: DaemonProbe, + probe: DaemonProber, + identityError: (probe: DaemonProbe) => string | null, +): Promise { + const ready = await probe(port, "readyz"); + if (!ready || ready.pid !== pid) { + return { + state: "error", + port, + pid, + executablePath: health.executablePath, + workingDirectory: health.workingDirectory, + message: "An AO daemon is already running, but it is not ready yet.", + }; + } + + const message = identityError(ready); + if (message) { + return { + state: "error", + port, + pid, + executablePath: ready.executablePath, + workingDirectory: ready.workingDirectory, + message, + }; + } + + return { + state: "ready", + port, + pid, + executablePath: ready.executablePath, + workingDirectory: ready.workingDirectory, + }; +}