From d600eba35b5d17717e24dfa1b90ffa06f9c920a5 Mon Sep 17 00:00:00 2001 From: dmlanday <85572596+dmlanday@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:13:49 -0400 Subject: [PATCH] fix(cli): preflight the port before serving so a second instance cannot de-register the first (#12485) * fix(cli): preflight the port before serving so a second instance cannot de-register the first Starting `omniroute serve` against a port another OmniRoute already owns produced three identical raw Node stack traces and no explanation: Error: listen EADDRINUSE: address already in use 0.0.0.0:20128 The conflict was handed to the child process, so it surfaced only after the child had been spawned and retried twice on the supervisor's restart budget, and never named the process holding the port. The damage was worse than the noise. Both spawns happen after writePidFile("supervisor") and the failed child's cleanupPidFile("server"), so a doomed second instance overwrites the pid files of the healthy instance that owns the port: supervisor/.pid ends up pointing at the dead starter and server/.pid is deleted, de-registering a server that is up and serving. Observed live: healthy server 19348 under supervisor 11108, while supervisor/.pid read 21440 (dead) and server/.pid was gone. `omniroute stop` still worked, but only by falling through to its killByPort port fallback. serve now resolves the port owner before spawning anything or touching a pid file, and exits with a message naming the owning PID and the two ways out (`omniroute stop`, or `serve --port `). Discovery lives in findListeningPids() in bin/cli/utils/pid.mjs (netstat on win32, lsof elsewhere); it mirrors killByPort()'s discovery in stop.mjs, which is worth consolidating next time that file is touched. A discovery failure reports the port as free, since a false "busy" would block a legitimate start. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CJf2dxEpiwZqyZujWk57T2 * chore(changelog): link the port-preflight fix to PR 12485 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CJf2dxEpiwZqyZujWk57T2 --------- Co-authored-by: Claude Opus 5 Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Co-authored-by: dmlanday --- bin/cli/commands/serve.mjs | 26 ++++ bin/cli/utils/pid.mjs | 53 ++++++++ .../12485-cli-serve-port-in-use-preflight.md | 1 + .../cli-serve-port-in-use-preflight.test.mjs | 125 ++++++++++++++++++ 4 files changed, 205 insertions(+) create mode 100644 changelog.d/fixes/12485-cli-serve-port-in-use-preflight.md create mode 100644 tests/unit/cli-serve-port-in-use-preflight.test.mjs diff --git a/bin/cli/commands/serve.mjs b/bin/cli/commands/serve.mjs index ce674be340..ec71cbd170 100644 --- a/bin/cli/commands/serve.mjs +++ b/bin/cli/commands/serve.mjs @@ -8,6 +8,7 @@ import { writePidFile, cleanupPidFile, waitForServer, + findListeningPids, resolveReadyTimeoutMs, } from "../utils/pid.mjs"; import { @@ -240,6 +241,16 @@ export async function runServe(opts = {}) { process.exit(1); } + // Refuse to start a second instance on a port something else already owns, + // BEFORE any pid file is written or any child is spawned. Otherwise the + // doomed child's EADDRINUSE arrives only after this process has rewritten + // the pid files of the healthy instance that actually owns the port. + const busyPids = await findListeningPids(dashboardPort); + if (busyPids.length > 0) { + reportPortInUse(dashboardPort, busyPids); + process.exit(1); + } + console.log(` \x1b[2m⏳ Starting server...\x1b[0m\n`); // #5172/#5160/#5152: default the V8 heap to ~35% of physical RAM (clamped @@ -318,6 +329,21 @@ export async function runServe(opts = {}) { ); } +/** + * Explain a port conflict in terms the operator can act on: who owns the port, + * and the two ways out. Exported for unit tests. + */ +export function reportPortInUse(port, pids = []) { + const owner = pids.length === 1 ? `PID ${pids[0]}` : `PIDs ${pids.join(", ")}`; + console.error(`\n\x1b[31m✖ Port ${port} is already in use by ${owner}.\x1b[0m`); + console.error( + ` Another OmniRoute is most likely already serving there, so open` + + ` ${urlScheme}://localhost:${port} before starting a second one.` + ); + console.error(` To replace it: \x1b[36momniroute stop\x1b[0m, then start again`); + console.error(` To run alongside: \x1b[36momniroute serve --port \x1b[0m\n`); +} + function runDaemon(serverJs, env, memoryLimit, dashboardPort, apiPort) { // #5238: skip the explicit CLI --max-old-space-size when the user pinned the // heap via NODE_OPTIONS (a CLI arg would shadow/override their value). diff --git a/bin/cli/utils/pid.mjs b/bin/cli/utils/pid.mjs index e12f2ada30..8b1d607ea9 100644 --- a/bin/cli/utils/pid.mjs +++ b/bin/cli/utils/pid.mjs @@ -59,6 +59,59 @@ export function isPidRunning(pid) { } } +// A port that is already owned must be reported, not spawned into. `omniroute +// serve` used to hand the conflict to the child, which died with EADDRINUSE +// twice on the supervisor's restart budget and printed three raw Node stack +// traces without ever saying another instance owned the port. It did that +// AFTER writing the pid files, so the doomed second instance de-registered the +// healthy running one (supervisor/.pid left pointing at the dead starter, +// server/.pid deleted outright). +// +// Discovery mirrors killByPort() in bin/cli/commands/stop.mjs (netstat on +// win32, lsof elsewhere); the two are worth consolidating next time stop.mjs +// is touched. +export async function findListeningPids(port, deps = {}) { + const platform = deps.platform || process.platform; + let exec = deps.execFileAsync; + if (!exec) { + const { execFile } = await import("node:child_process"); + const { promisify } = await import("node:util"); + exec = promisify(execFile); + } + try { + if (platform === "win32") { + const { stdout } = await exec("netstat", ["-ano"]); + return parseNetstatListeningPids(stdout, port); + } + const { stdout } = await exec("lsof", ["-ti", `:${port}`]); + return stdout + .trim() + .split("\n") + .map((entry) => parseInt(entry, 10)) + .filter((entry) => Number.isFinite(entry) && entry > 0); + } catch { + // No netstat/lsof available, or simply no listener. Report "free": a false + // "busy" would block a legitimate start, the worse failure of the two. + return []; + } +} + +function parseNetstatListeningPids(stdout, port) { + const portCol = `:${port}`; + const pids = []; + for (const line of stdout.split(/\r?\n/)) { + const cols = line.trim().split(/\s+/); + // Proto LocalAddress ForeignAddress State PID + if (cols.length < 5) continue; + if (cols[0] !== "TCP" && cols[0] !== "TCPv6") continue; + if (!(cols[1] || "").endsWith(portCol)) continue; + if ((cols[cols.length - 2] || "").toUpperCase() !== "LISTENING") continue; + const pid = parseInt(cols[cols.length - 1], 10); + if (Number.isFinite(pid) && pid > 0 && !pids.includes(pid)) pids.push(pid); + } + return pids; +} + export function sleep(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } diff --git a/changelog.d/fixes/12485-cli-serve-port-in-use-preflight.md b/changelog.d/fixes/12485-cli-serve-port-in-use-preflight.md new file mode 100644 index 0000000000..831e8710d5 --- /dev/null +++ b/changelog.d/fixes/12485-cli-serve-port-in-use-preflight.md @@ -0,0 +1 @@ +- **fix(cli):** `omniroute serve` now checks whether the port is already owned before spawning anything, and reports the conflict with the owning PID plus the two ways out (`omniroute stop`, or `--port`). Previously it handed the conflict to the child process, which died with `EADDRINUSE` and was retried twice on the supervisor's restart budget, printing three identical raw Node stack traces without ever saying that another instance held the port. Because that happened after the pid files were written, the doomed second instance also de-registered the healthy running one, leaving `supervisor/.pid` pointing at the dead starter and `server/.pid` deleted. ([#12485](https://github.com/diegosouzapw/OmniRoute/pull/12485)) diff --git a/tests/unit/cli-serve-port-in-use-preflight.test.mjs b/tests/unit/cli-serve-port-in-use-preflight.test.mjs new file mode 100644 index 0000000000..ca7f614b76 --- /dev/null +++ b/tests/unit/cli-serve-port-in-use-preflight.test.mjs @@ -0,0 +1,125 @@ +// `omniroute serve` against a port another OmniRoute already owns produced a +// confusing, self-inflicted mess: it spawned a child that died with +// EADDRINUSE, retried it twice on the supervisor's restart budget, and printed +// three identical raw Node stack traces before giving up — never once saying +// that another instance already owns the port. +// +// Worse, it did that AFTER writePidFile("supervisor") and the failed child's +// cleanupPidFile("server"), so the doomed second instance overwrote the pid +// files of the healthy running one: supervisor/.pid pointed at the dead +// starter and server/.pid was deleted outright, de-registering a server that +// was up and serving. (Observed live: healthy server 19348 under supervisor +// 11108, while supervisor/.pid read 21440 — dead — and server/.pid was gone.) +// +// Fix: preflight the port before spawning anything. Report who owns it and +// exit, touching no pid files. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import net from "node:net"; +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { findListeningPids } from "../../bin/cli/utils/pid.mjs"; + +const execFileAsync = promisify(execFile); + +// findListeningPids() has no discovery mechanism on POSIX besides `lsof` +// (see pid.mjs) — a runner without it (this devbox, some minimal CI images) +// makes the real preflight silently report "port free" by design (a false +// "busy" would block a legitimate `serve`, the worse failure of the two), so +// the end-to-end assertion below cannot pass there. Skip rather than fail: an +// absent discovery tool is an environment gap, not a regression in this PR. +async function hasPosixPortDiscovery() { + try { + await execFileAsync("lsof", ["-v"]); + return true; + } catch (err) { + return err?.code !== "ENOENT"; + } +} + +const canDiscoverListeningPorts = process.platform === "win32" || (await hasPosixPortDiscovery()); + +const WIN_NETSTAT = [ + "Active Connections", + "", + " Proto Local Address Foreign Address State PID", + " TCP 0.0.0.0:20128 0.0.0.0:0 LISTENING 19348", + " TCP 127.0.0.1:20128 127.0.0.1:60894 TIME_WAIT 0", + " TCP 0.0.0.0:445 0.0.0.0:0 LISTENING 4", +].join("\r\n"); + +test("findListeningPids reports the PID holding the port (win32 netstat)", async () => { + const pids = await findListeningPids(20128, { + platform: "win32", + execFileAsync: async () => ({ stdout: WIN_NETSTAT }), + }); + assert.deepEqual(pids, [19348], "must return only the LISTENING pid for that exact port"); +}); + +test("findListeningPids ignores TIME_WAIT and other ports", async () => { + const pids = await findListeningPids(445, { + platform: "win32", + execFileAsync: async () => ({ stdout: WIN_NETSTAT }), + }); + assert.deepEqual(pids, [4]); +}); + +test("findListeningPids reports the PID holding the port (posix lsof)", async () => { + const pids = await findListeningPids(20128, { + platform: "linux", + execFileAsync: async () => ({ stdout: "4242\n4243\n" }), + }); + assert.deepEqual(pids, [4242, 4243]); +}); + +test("findListeningPids returns empty when nothing is listening", async () => { + const pids = await findListeningPids(20128, { + platform: "win32", + execFileAsync: async () => { + throw new Error("netstat unavailable"); + }, + }); + assert.deepEqual(pids, [], "a discovery failure must not be reported as a busy port"); +}); + +test( + "findListeningPids finds a real listening socket (end-to-end)", + { + skip: !canDiscoverListeningPorts && "no lsof/netstat available on this runner", + }, + async () => { + const server = net.createServer(); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", resolve); + }); + const { port } = server.address(); + try { + const pids = await findListeningPids(port); + assert.ok( + pids.includes(process.pid), + `expected the preflight to find this process (${process.pid}) holding port ${port}, got ${JSON.stringify(pids)}` + ); + } finally { + await new Promise((r) => server.close(r)); + } + } +); + +test("reportPortInUse names the port, the owning pid, and how to resolve it", async () => { + const { reportPortInUse } = await import("../../bin/cli/commands/serve.mjs"); + const lines = []; + const origErr = console.error.bind(console); + console.error = (...args) => lines.push(args.join(" ")); + try { + reportPortInUse(20128, [19348]); + } finally { + console.error = origErr; + } + const out = lines.join("\n"); + assert.match(out, /20128/, "must name the port"); + assert.match(out, /19348/, "must name the process already holding it"); + assert.match(out, /omniroute stop/, "must tell the user how to free the port"); + assert.match(out, /--port/, "must offer running on a different port"); +});