diff --git a/src/lib/services/ServiceSupervisor.ts b/src/lib/services/ServiceSupervisor.ts index 3c4ee4dd51..2924f96675 100644 --- a/src/lib/services/ServiceSupervisor.ts +++ b/src/lib/services/ServiceSupervisor.ts @@ -7,7 +7,7 @@ import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; import { getServiceRow, updateServiceField, setToolStatus } from "@/lib/db/versionManager"; import { RingBuffer } from "./ringBuffer"; import { HealthChecker } from "./healthCheck"; -import { decidePreSpawn, probeBeforeSpawn } from "./portProbe"; +import { decidePreSpawn, probeBeforeSpawn, resolvePortPid } from "./portProbe"; import type { ServiceConfig, ServiceState, ServiceStatus, LogLine, HealthState } from "./types"; const CRASH_FAST_THRESHOLD_MS = 5_000; @@ -93,10 +93,17 @@ export class ServiceSupervisor extends EventEmitter { if (decision.action === "adopt") { // Something healthy already serves this port — treat it as running // rather than spawning a duplicate that would die with EADDRINUSE. + // We didn't spawn it, so there's no ChildProcess handle to read a + // pid from — resolve one from the OS instead. Best-effort: if + // resolution fails, pid stays null rather than blocking adoption, + // but downstream liveness checks that key off pid will only trust + // this instance once a real pid is on record. + const adoptedPid = await resolvePortPid(this.config.port); this.checker.start(); this.startedAt = new Date().toISOString(); + this.pid = adoptedPid; this.setState("running"); - await setToolStatus(this.config.tool, "running"); + await setToolStatus(this.config.tool, "running", adoptedPid ?? undefined); return this.getStatus(); } diff --git a/src/lib/services/portProbe.ts b/src/lib/services/portProbe.ts index 99405462a8..3f0deaaf07 100644 --- a/src/lib/services/portProbe.ts +++ b/src/lib/services/portProbe.ts @@ -14,6 +14,7 @@ */ import { createConnection } from "node:net"; +import { spawn } from "node:child_process"; /** Result of probing the service before spawning. */ export interface PreSpawnProbe { @@ -25,12 +26,11 @@ export interface PreSpawnProbe { /** Outcome of the pre-spawn decision. */ export type PreSpawnDecision = - | { action: "spawn" } - | { action: "adopt" } - | { action: "error"; message: string }; + { action: "spawn" } | { action: "adopt" } | { action: "error"; message: string }; const HEALTH_PROBE_TIMEOUT_MS = 3_000; const PORT_PROBE_TIMEOUT_MS = 1_000; +const PID_RESOLVE_TIMEOUT_MS = 2_000; /** * Decide what to do before spawning, given a probe of the port + health. @@ -102,3 +102,46 @@ export async function probeBeforeSpawn(healthUrl: string, port: number): Promise ]); return { healthy, portInUse }; } + +/** + * Resolve the pid of whatever process is listening on `port`, if any. + * + * Used when adopting an already-healthy instance (see `decidePreSpawn`'s + * "adopt" outcome): the supervisor didn't spawn that process itself, so it + * has no pid from a `ChildProcess` handle, but tracking a real pid is still + * needed for downstream liveness checks to trust an adopted service the same + * way they trust a freshly-spawned one. Returns null if nothing is found or + * the lookup fails/times out (best-effort; never blocks adoption on this). + */ +export async function resolvePortPid(port: number): Promise { + return new Promise((resolve) => { + const proc = spawn("lsof", ["-ti", `:${port}`]); + let output = ""; + let settled = false; + + const finish = (value: number | null) => { + if (settled) return; + settled = true; + clearTimeout(timeout); + resolve(value); + }; + + const timeout = setTimeout(() => { + proc.kill(); + finish(null); + }, PID_RESOLVE_TIMEOUT_MS); + + proc.stdout?.on("data", (chunk: Buffer) => { + output += chunk.toString("utf8"); + }); + proc.on("error", () => finish(null)); + proc.on("close", () => { + const firstLine = output + .split("\n") + .map((line) => line.trim()) + .find((line) => line.length > 0); + const parsed = firstLine ? Number.parseInt(firstLine, 10) : Number.NaN; + finish(Number.isFinite(parsed) ? parsed : null); + }); + }); +} diff --git a/tests/unit/services/ServiceSupervisor.test.ts b/tests/unit/services/ServiceSupervisor.test.ts index db7e218b48..fe0d6d82bc 100644 --- a/tests/unit/services/ServiceSupervisor.test.ts +++ b/tests/unit/services/ServiceSupervisor.test.ts @@ -220,7 +220,12 @@ test("#6205: probeBeforeSpawn adopts a healthy existing instance (no spawn)", as try { const status = await sup.start(); assert.equal(status.state, "running", "adopted instance is marked running"); - assert.equal(status.pid, null, "no child process is spawned when adopting"); + // No child process handle exists on adoption (nothing was spawned), but a + // real pid is still resolved from the OS — see the dedicated pid-tracking + // test below. Historically this asserted pid === null, which just + // reflected the missing resolution rather than a deliberate "no pid for + // adopted services" design choice. + assert.ok(status.pid !== null, "adopted instance still gets a tracked pid"); // No child means no captured stdout ticks. await new Promise((r) => setTimeout(r, 300)); assert.equal(sup.getRingBuffer().snapshot().length, 0, "no logs — nothing was spawned"); @@ -229,3 +234,31 @@ test("#6205: probeBeforeSpawn adopts a healthy existing instance (no spawn)", as healthServer.close(); } }); + +// Regression test: the adopt branch used to call setToolStatus(tool, "running") +// with no pid argument at all, leaving `pid` permanently null for any service +// adopted on a supervisor restart (common in production — e.g. after +// `systemctl --user restart omniroute.service`, sidecar child processes can +// outlive the restart and get adopted rather than spawned fresh). Downstream +// consumers that key liveness tracking off pid would then treat a genuinely +// healthy, running service as untrustworthy/stale. This asserts the resolved +// pid on adoption matches the real process actually holding the port. +test("adopted service resolves and records the real pid of the process holding the port", async () => { + const healthServer = startHealthServer(29996); + const cfg = { ...tickConfig("test-adopt", 29996), probeBeforeSpawn: true }; + const sup = new ServiceSupervisor(cfg); + + try { + const status = await sup.start(); + assert.equal(status.state, "running"); + // The health server above runs inline in this test process (no child + // process spawned for it), so the pid actually bound to port 29996 is + // this test process's own pid — that's exactly what resolvePortPid() + // should find and what the supervisor should record. + assert.equal(status.pid, process.pid, "resolved pid matches the process holding the port"); + assert.equal(sup.getStatus().pid, process.pid, "in-memory supervisor state also has the pid"); + } finally { + await sup.stop(); + healthServer.close(); + } +});