Repository navigation
fix(cli): resolve serve preflight busy pids to an array when discovery returns null - #15021
Closed
krzemienski wants to merge 1 commit into
Closed
krzemienski wants to merge 1 commit into
krzemienski wants to merge 1 commit into
Conversation
…y returns null diegosouzapw#14538 made findListeningPids() return null when discovery cannot answer and bind-probed the port instead, but only reassigned busyPids when the probe found the port held. On a free port busyPids stayed null and the following busyPids.length threw, so `omniroute serve` crashed on every start with "Cannot read properties of null (reading 'length')". On macOS this hits every normal start: `lsof -ti :PORT` exits 1 when nothing listens, which execFile surfaces as an error and findListeningPids() maps to null. launchd/KeepAlive installs crash-loop after upgrading. Extract the decision into resolveBusyPortPids() in bin/cli/utils/pid.mjs, which always returns an array: discovered pids, [] when the bind probe finds the port free, or [null] when the probe proves it held without a known owner. serve.mjs uses it in place of the inline branches.
Owner
|
Thanks, @krzemienski. The null-discovery crash in the serve preflight was a real issue. It is already fixed on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After #14538,
omniroute servecrashes on every start where the target port is free and pid discovery can't answer:findListeningPids()returnsnullwhen discovery throws, and the bind-probe fallback only reassignsbusyPidswhen the probe finds the port held. On a free portbusyPidsstaysnull, andbusyPids.lengththrows.On macOS this is the normal path, not an edge case:
lsof -ti :PORTexits 1 when nothing is listening,execFilerejects on that, andfindListeningPids()maps it tonull. A launchdKeepAliveinstall that upgrades to currentrelease/v3.8.51crash-loops. Reproduced on macOS arm64, Node 22.22.3.Fix
resolveBusyPortPids(port, deps)inbin/cli/utils/pid.mjsalways returns an array:[]when the bind probe finds the port free[null]when the probe proves the port is held but no owner could be named (keeps the fix(cli): port-in-use guard silently passes when lsof is unavailable (Termux/slim containers) — user gets an EADDRINUSE restart loop instead of the clear message #14518 guard)serve.mjsuses it in place of the inline branches.reportPortInUse()and the rest of the preflight are unchanged.Tests
4 cases appended to
tests/unit/cli-serve-port-in-use-preflight.test.mjs: null discovery with a free port gives[], null discovery with a held port gives[null], discovered pids skip the probe, and a real free port with the reallsofgives[]. All pass.Heads-up, unrelated to this change: on macOS, tests 7 and 9 in that file (
probePortFree is false while a socket holds the port…and the #14518 end-to-end test) also fail on unmodifiedrelease/v3.8.51.probePortFree()binds the dual-stack wildcard, which succeeds while the test's holder is bound to127.0.0.1only.Verified live
Built
release/v3.8.51@ d4bf564, installed it as the global package under launchd, and confirmed the crash loop. With this patch,servestarts cleanly, and/api/healthplus chat completions answer 200 locally and through a Cloudflare tunnel.