Repository navigation
fix(openchamber): single-instance guard for the socket-activated server - #2465
Conversation
The user unit is socket-activated with KillMode=process, so a stop terminates only the tracked main process. A server that is no longer the tracked main (a self-handoff predecessor) survives in the cgroup still holding the listen socket systemd handed it; the next start passes the same socket to a new server and both accept() on it, so /api/version answers alternate between instances (2026-10-29: pids 272664 + 1668515 on 127.0.0.1:3000, alternating startedAt). - openchamber-serve now reaps leftover servers (SIGTERM, up to 10 s drain, then SIGKILL) before exec; the matcher requires a node/ openchamber exe plus the serve cmdline so diagnostic shells whose argv merely contains the pattern text are never matched. The managed OpenCode (different port) is untouched and stays up for adoption, per the KillMode=process design. - ExecStartPost assertion fails the start loudly unless exactly one server is on port 3000 afterwards. Tracking issue: dezren39/darwin-nix#1117 (cross-repo set; the systemd model lives here, the launchd port in dezren39/nix disables self-handoff for the same reason).
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc6044f7a3
ℹ️ 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".
| # The pattern text itself rides in the argv of anything inspecting | ||
| # the fleet (a grep, a diagnostic shell), so a cmdline match alone | ||
| # is not proof: the process must actually be the node server. | ||
| exe="$(readlink "$proc/exe" 2>/dev/null)" || continue |
There was a problem hiding this comment.
The OpenChamber unit explicitly sets PATH to only the mcpx, git, and openssh bin directories, but both new scripts invoke unqualified coreutils commands. In particular, readlink is unavailable, and its suppressed error causes every process to be skipped: cleanup does nothing and the post-start assertion always reports zero servers, failing startup and triggering repeated restarts. Running the extracted assertion against a matching Node process reproduced this failure when coreutils was excluded from PATH. Add pkgs.coreutils to the service PATH or use absolute executable paths in both scripts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29d77ef — thank you, this one would have broken the service on deploy: the guard would have silently matched nothing (and the assertion failed every start). is now in the unit's PATH via the same list, with a comment naming the guard's dependency on it (readlink/cat/tr/seq/sleep).
| ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID"; | ||
| # Fail the start loudly if the start script's single-instance | ||
| # guard left anything but exactly one server on the port. | ||
| ExecStartPost = "${openchamberSingleInstanceCheck}"; |
There was a problem hiding this comment.
This unit leaves Type unset and therefore uses simple; the systemd documentation specifies that post-start commands run once “the process has been started for Type=simple,” not after a startup shell reaches its final exec. Consequently, this assertion races the wrapper's process scan and up-to-ten-second drain. During duplicate recovery it can count both old servers before cleanup finishes, or zero after they exit but before the replacement starts, causing startup to fail and systemd to terminate the wrapper instead of completing recovery. Synchronize the assertion with cleanup completion and actual server startup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29d77ef by removing the assertion rather than synchronizing it: the script and unit line are gone. The start script now verifies synchronously after the reap loop (SIGTERM → ≤10 s drain → SIGKILL → re-check) and exits 1 — 'refusing to start a second server on port 3000' — if a leftover survives, which is race-free by construction since the same process owns the whole sequence before exec. The loud-failure property is preserved (failed start + journal message + Restart=on-failure retries the reap).
| *) continue ;; | ||
| esac | ||
| case "$cmd" in | ||
| *"--port 3000"*) leftovers="$leftovers $pid" ;; |
There was a problem hiding this comment.
If another OpenChamber instance runs on port 30000–30009, this substring matcher also selects it as a port-3000 leftover, so starting or restarting this service sends SIGTERM and potentially SIGKILL to an unrelated instance that cannot share the intended listener. Applying the exact matcher to a live Node process with cli.js serve --port 30001 reproduced the false match. The assertion duplicates this prefix match and also counts different-port instances. Require an argument boundary after 3000 in both matchers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29d77ef: the matcher now requires an argument boundary — patterns are --port 3000 and --port=3000 (cmdline words are space-separated after the NUL translation, so the trailing word is covered). Verified on sample strings: 3000 matches in both forms and both positions; 30001 and 30009 do not match in either form. The duplicate assertion matcher is gone with the ExecStartPost removal above, so this is now the only matcher.
…uard
- P1 (PATH): the unit PATH held only mcpx/git/openssh, so the guard's
coreutils (readlink/cat/tr/seq/sleep) resolved to nothing and the
scan silently matched no processes. Add pkgs.coreutils to the
service PATH.
- P1 (race): ExecStartPost with Type=simple fires after fork, racing
the guard's up-to-10s drain -- it could count 0 or 2 and fail a
recovery start. Remove the assertion script and ExecStartPost; the
start script now verifies synchronously after reaping and exits 1
(refusing to start a second server) if a leftover survives.
- P2 (prefix match): "--port 3000" substring also matched ports
30001-30009 of unrelated instances. Require an argument boundary
("--port 3000 " / "--port=3000 ") in the matcher.
Matcher re-verified live (only the running server matches) and on
sample strings for 3000/30001/30009 in both flag forms.
Problem
Tracking issue: dezren39/darwin-nix#1117 (cross-repo set — the defect was filed there; the systemd model it names lives in this repo,
home/common/default.nix. The launchd port in dezren39/nix disables self-handoff for the same failure class.)The
openchamberuser unit is socket-activated (openchamber.socketowns127.0.0.1:3000) withKillMode=process, so a stop terminates only the tracked main process. On 2026-10-29 the 12:36:40 CDT stop left the actual web server (pid 272664, started 05:22 CDT) running in the cgroup — systemd's own journal:Unit process 272664 (MainThread) remains running after unit stopped, with the tracked main an adopted processnot our child. Socket activation started a new server (pid 1668515) at 12:38:44 and handed it the same listen socket (both processes hold fd 3 → socket inode 11096). Bothaccept()on it, so/api/versionanswers alternatestartedAt10:22:55Z / 17:38:45Z under one serverId and/api/session-activityflips between divergent session sets. Fleet dispatch/capacity reads over port 3000 are unreliable while this lasts.Fix
openchamber-serve(the start script) now reaps leftover servers beforeexec: SIGTERM, up to 10 s drain (OpenChamber's documented SIGTERM drain), then SIGKILL. Because the start script's store path is the unit's deploy key, this change also triggers the activation restart that deploys it.node/openchamberexe plus theserve --port 3000cmdline — verified live: it matches exactly pids 272664 + 1668515, does not match the managed OpenCode children (ports 42573/39327, which must stay up for adoption), and does not match a diagnostic shell whose argv merely contains the pattern text.ExecStartPostassertion fails the start loudly unless exactly one server is on port 3000 afterwards. Verified against the live duplicate state:openchamber: expected exactly 1 server on port 3000 after start, found 2, exit 1.Verification
bash -nas Nix emits them;nixfmtclean.🤖 Queue lane A (repo-queue-worker-shift-nix), task 3677.
Need help on this PR? Tag
@codesmith-botwith what you need.