Repository navigation
fix(services): resolve and record a real pid when adopting a service - #8218
Conversation
ServiceSupervisor.start(), when probeBeforeSpawn detects an already-healthy instance on the target port, "adopts" it (marks the service running without spawning a duplicate that would die with EADDRINUSE). This adopt path calls setToolStatus(tool, "running") with no pid argument at all, so this.pid stays at its constructor default of null for the rest of that supervisor's life -- only the spawn path (a genuinely new child process) ever sets a real pid. In production this "adopt" path is common, not an edge case: any embedded sidecar service (cliproxy, 9router, bifrost, mux) whose child process survives a `systemctl --user restart omniroute.service` gets adopted by the new supervisor instance on the next start(), and its pid is lost from that point on -- even though the service is genuinely healthy and running. The observed symptom: a service shows state "running" but pid null, and something downstream that keys liveness tracking off pid eventually treats it as untrustworthy/stale despite nothing actually being wrong. Fix: resolve the real pid of the process holding the port (via `lsof -ti :<port>`, best-effort -- a lookup failure leaves pid null rather than blocking adoption) and record it the same way the spawn path does. An existing test asserted pid === null on adopt. That was accurate for the old behavior but reflected a missing resolution, not a deliberate "adopted services never get a pid" design choice -- updated its assertion to match the corrected behavior and added a dedicated regression test proving the resolved pid matches the real process actually bound to the port.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a48e134e7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| export async function resolvePortPid(port: number): Promise<number | null> { | ||
| return new Promise((resolve) => { | ||
| const proc = spawn("lsof", ["-ti", `:${port}`]); |
There was a problem hiding this comment.
In Windows/Electron or the official Docker runner image, this spawn("lsof", ...) cannot produce a PID: Windows has no lsof, and the committed Dockerfile installs runtime packages like libsecret-1-0/ca-certificates and later git/docker extras but never lsof. Because the error handler returns null, an adopted healthy service in those supported deployments is still persisted as running with pid: null through the adopt branch, so the restart/adoption fix remains ineffective there; please add a platform/procfs fallback or include the resolver where services run.
Useful? React with 👍 / 👎.
| */ | ||
| export async function resolvePortPid(port: number): Promise<number | null> { | ||
| return new Promise((resolve) => { | ||
| const proc = spawn("lsof", ["-ti", `:${port}`]); |
There was a problem hiding this comment.
When lsof is available, -i :${port} matches any socket using that port, not just the process that owns the listener; the lsof manual says -i selects matching Internet addresses and gives -iTCP -sTCP:LISTEN as the way to list listening TCP sockets. After the health probe (or while clients are connected), this can include client-side ESTABLISHED sockets or UDP users of the same numeric port, so taking the first PID can record the supervisor/client PID instead of the adopted service and make later liveness checks trust the wrong process.
Useful? React with 👍 / 👎.
|
Thanks, @seanford! 🙏 Merged — services now resolve and record a real pid when adopting a running service. ServiceSupervisor tests green. |
337373f
into
cheaper-inference:release/v3.8.49
…heaper-inference#8218) ServiceSupervisor.start(), when probeBeforeSpawn detects an already-healthy instance on the target port, "adopts" it (marks the service running without spawning a duplicate that would die with EADDRINUSE). This adopt path calls setToolStatus(tool, "running") with no pid argument at all, so this.pid stays at its constructor default of null for the rest of that supervisor's life -- only the spawn path (a genuinely new child process) ever sets a real pid. In production this "adopt" path is common, not an edge case: any embedded sidecar service (cliproxy, 9router, bifrost, mux) whose child process survives a `systemctl --user restart omniroute.service` gets adopted by the new supervisor instance on the next start(), and its pid is lost from that point on -- even though the service is genuinely healthy and running. The observed symptom: a service shows state "running" but pid null, and something downstream that keys liveness tracking off pid eventually treats it as untrustworthy/stale despite nothing actually being wrong. Fix: resolve the real pid of the process holding the port (via `lsof -ti :<port>`, best-effort -- a lookup failure leaves pid null rather than blocking adoption) and record it the same way the spawn path does. An existing test asserted pid === null on adopt. That was accurate for the old behavior but reflected a missing resolution, not a deliberate "adopted services never get a pid" design choice -- updated its assertion to match the corrected behavior and added a dedicated regression test proving the resolved pid matches the real process actually bound to the port.
…heaper-inference#8218) ServiceSupervisor.start(), when probeBeforeSpawn detects an already-healthy instance on the target port, "adopts" it (marks the service running without spawning a duplicate that would die with EADDRINUSE). This adopt path calls setToolStatus(tool, "running") with no pid argument at all, so this.pid stays at its constructor default of null for the rest of that supervisor's life -- only the spawn path (a genuinely new child process) ever sets a real pid. In production this "adopt" path is common, not an edge case: any embedded sidecar service (cliproxy, 9router, bifrost, mux) whose child process survives a `systemctl --user restart omniroute.service` gets adopted by the new supervisor instance on the next start(), and its pid is lost from that point on -- even though the service is genuinely healthy and running. The observed symptom: a service shows state "running" but pid null, and something downstream that keys liveness tracking off pid eventually treats it as untrustworthy/stale despite nothing actually being wrong. Fix: resolve the real pid of the process holding the port (via `lsof -ti :<port>`, best-effort -- a lookup failure leaves pid null rather than blocking adoption) and record it the same way the spawn path does. An existing test asserted pid === null on adopt. That was accurate for the old behavior but reflected a missing resolution, not a deliberate "adopted services never get a pid" design choice -- updated its assertion to match the corrected behavior and added a dedicated regression test proving the resolved pid matches the real process actually bound to the port.
What
ServiceSupervisor.start()'s "adopt" path (taken whenprobeBeforeSpawnfinds an already-healthy instance serving the port, so a duplicate isn't spawned) marks the service"running"but never resolves or records a realpid--setToolStatus(tool, "running")is called with no pid argument, andthis.pidstays at its constructor default ofnullfor the rest of that supervisor instance's life. Only the spawn path (a genuinely new child process) ever gets a real pid.Why it matters
This isn't an edge case in practice. Any embedded sidecar service (CLIProxyAPI, 9Router, Bifrost, Mux) whose child process survives a
systemctl --user restart omniroute.service-- which is common, since the sidecar isn't necessarily killed when the parent Node process restarts -- gets adopted by the new supervisor instance on its nextstart(), not spawned fresh. From that point on the service showsstate: "running"butpid: nullvia/api/services/{tool}/status, even though it's genuinely healthy. Anything downstream that keys liveness/trust off pid has no way to distinguish "really running, just adopted" from "stale tracking," and in the reported case appeared to eventually mark the servicestoppeddespite nothing being wrong.Fix
Added
resolvePortPid(port)toportProbe.ts-- resolves the pid of whatever process is actually listening on a port vialsof -ti :<port>, best-effort (a lookup failure or timeout leavespid: nullrather than blocking adoption, so this can't turn a working adoption into a failure). The adopt branch now calls it and records the result the same way the spawn branch already does.Test changes
tests/unit/services/ServiceSupervisor.test.ts's existing#6205test explicitly assertedpid === nullon adopt. That was accurate for the old behavior, but reflects a missing resolution rather than a deliberate "adopted services never get a pid" design choice -- there's no comment or reasoning anywhere suggesting it was intentional, and the adopt/spawn asymmetry it encodes is exactly the bug. Updated that assertion topid !== nulland added a dedicated regression test (adopted service resolves and records the real pid of the process holding the port) that spins up an inline health server (no child process spawned for it) and asserts the resolved pid matchesprocess.pid-- i.e. the pid actually bound to the port, not a stale/fabricated value.Testing
Ran the full suite on a clean checkout (
npm run test:unit:cimain pass + dashboard pass + serial pass, ~25,000 tests total). Result: all of the changed-file tests pass, including the new regression test. 10 pre-existing failures + 27 cancelled in the main pass, all in files unrelated toServiceSupervisor.ts/portProbe.ts(e.g. an antigravity OAuth postExchange timeout-race test failing withPromise resolution is still pending but the event loop has already resolved, and a repo-doc-sync consistency gate that appears sensitive to a shallow clone's missing git history) -- did not attempt to fix these, out of scope for this change and they reproduce identically with this branch's changes reverted.