fix(upgrade): finish the restart on hosts with no service manager#62
fix(upgrade): finish the restart on hosts with no service manager#62jiunbae wants to merge 6 commits into
Conversation
`muxa upgrade` builds the new binaries and then asks the OS service manager to restart the daemon. When there isn't one that works, it gave up with "manual restart required" — so the upgrade silently didn't take effect until someone remembered to restart muxad by hand. A container where systemd is installed but not running hits this on *every* upgrade, and `which systemctl` succeeding makes it look supported right up until the call fails with "Failed to connect to bus". Fall back to replacing the daemon ourselves. Ask the daemon on the socket this upgrade is already managing to drain, wait for it to let go, then start the new one detached. The distinction from the `pkill` fallback that was removed earlier is the whole point: this addresses one daemon *through its own control channel*, identified by the socket the user pointed us at. A muxad belonging to another socket, checkout, or user is never a candidate. Verified: the production daemon on a different socket kept serving untouched across the whole exercise. Adds `shutdown` to the IPC protocol to make that possible. It routes to the same broadcast SIGTERM already fires, so the drain path is unchanged and in-flight handlers still finish. It's opt-in — a Server without a trigger answers with an error rather than exiting, so a caller can tell "declined" from "old daemon that never knew the method". The socket is 0600, so anyone who can call it is already the user who owns the daemon: the same boundary that lets `ingest` forge events. The detached spawn reuses init's shell-based `nohup … & disown` rather than a new `pre_exec`/`setsid` — the workspace forbids unsafe, and `setsid(1)` isn't on macOS. Extracted it to `init::util` so init and upgrade can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Self-review found the fallback added in the previous commit panics the moment it runs. `request_shutdown` built its own tokio runtime and `block_on`'d it from inside the CLI's `#[tokio::main]` — "Cannot start a runtime from within a runtime" — so `muxa upgrade` on the exact host this targets died with a backtrace *after* pulling and rebuilding. My earlier "end-to-end" check missed it because it re-created the steps in a shell instead of calling the code, which tested nothing about the code. The chain is async now, and a test drives `request_shutdown` from inside a runtime; reintroducing the nested runtime fails it. Four more things the same review surfaced, each of which made the fallback claim success it hadn't earned: - The replacement was spawned as a bare `muxad`, which re-resolves its own default socket. `muxa upgrade --socket /tmp/x.sock` therefore stopped the daemon on /tmp/x.sock and started one on the default path instead — the daemon the user asked about simply gone. It now inherits the socket we retired. - `nohup … &` makes the shell exit 0 having only *started* the fork, so a muxad that isn't on `$PATH` reported `Restarted`. Since we just stopped a working daemon, "probably fine" isn't good enough: `replace_daemon` waits for the replacement to answer and reports failure if it doesn't. - Any non-zero `systemctl` was treated as "no service manager" and triggered the fallback, quietly demoting a supervised daemon to an unsupervised `nohup` child. Present-but-no-bus is now distinguished from present-and-refused; only the former falls back. - Support was inferred from error text (`RuntimeError::Json` meant "old daemon"), which also matched a refusal and a transport hiccup. It reads the `shutdown` capability from `hello` instead — the tag added for this in the previous commit was going unused. The socket-release wait still can't see the old process finish flushing its final snapshot. Documented rather than fixed: that window is muxad's shutdown ordering, and the manual recipe this replaces (`pkill muxad`, then start it) has the identical overlap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 2 of self-review. The Unusable/Failed split added last commit asked
the wrong question — "can I reach the service manager" instead of "does
the service manager own this daemon" — so it disabled the fallback on the
biggest class of hosts it was written for: a live systemd user bus with no
muxad unit, which is what you get from `cargo install` plus a shellrc
autostart. `systemctl --user restart muxad` fails there with "Unit not
found", got classified `Failed` ("its daemon stays its business"), and the
user was told to restart by hand — with nothing supervising the daemon at
all. macOS had the mirror bug via a launchctl short-circuit.
Both now hinge on one read-only probe — `systemctl --user cat muxad`,
`launchctl print gui/<uid>/…` — which fails both when the manager can't be
reached and when it never heard of muxad. That also retires the
`"Failed to connect to bus"` stderr match: modern systemd words it
differently, so the gate would have silently stopped firing on exactly the
hosts that need it.
Two more, same shape as before — claiming success we hadn't earned:
- `muxa upgrade` could exit 0 with the daemon *stopped*. The fallback
retires the old daemon before starting the new, so a failed restart no
longer means "nothing changed" the way it did when the command only ever
asked a service manager. `muxa upgrade && muxa status` would sail past a
dead daemon. ManualRequired is now an error, with the command to run.
- `Client::capabilities` skipped the `ok` check every other checked call
has, so a *rejected* hello read back as an empty capability list. That is
the upgrade case exactly: a new CLI whose PROTOCOL_VERSION just bumped
talks to the old daemon, gets turned away, and reports "the daemon
predates IPC shutdown" — the refusal-vs-old-daemon conflation from round
1, reintroduced one layer down, and recurring on every protocol bump.
The round-1 regression test was decorative: a dead socket short-circuits at
the capability probe, so a nested runtime in `Client::shutdown` sailed
through it. It runs against a live server now. Verified by mutation —
nesting a runtime in `shutdown`, dropping the `shutdown` tag, removing the
`ok` check, and restoring warn-and-exit-0 each fail the suite; before this,
three of the four passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rounds 1-3 of self-review kept finding the same shape of bug in this fallback, because the fallback kept guessing. To replace a daemon from the outside you need its argv, its socket, whether a supervisor owns it, and the moment it finished writing — and `muxa upgrade` can know none of them. Each round I approximated one and the approximation became the next round's defect: a bare `muxad` that re-resolved its own socket, a spawn whose shell always exits 0, an ownership probe that asked about reachability, an exit code that failed for daemons nothing had touched. Hand the job to the process that already has all four. `restart` over IPC now makes muxad drain exactly as it would on SIGTERM and then `exec` its own argv[0]. Same pid, same argv, same environment, same socket. Nothing to guess, nothing to overlap: there is only ever one process. That deletes, rather than fixes, every defect class from those rounds: - two daemons on one data dir — impossible; the compaction/rename race that could punch a hole in prompts.ndjson has no second process to race - lost argv (`--dashboard`, `--allow-public` have no env fallback) — it is the daemon's own argv - socket rediscovery — it rebinds its own - "does a service manager own this?" — irrelevant; a supervisor watching the pid sees nothing happen, so self-restart is correct either way And it restores the invariant that made this command safe to begin with: **nothing stops a daemon it cannot bring back.** A daemon too old to know the request, or a host with neither self-restart nor a service manager, is left running and the user is told — which is exit 0, honestly, because a daemon is still serving. Only one path can end with no daemon (it accepted, tore down, and the exec failed) and only that path fails the command. `argv[0]`, not `current_exe()`: `cargo install` renames a new binary over the path, so the running image is unlinked and `/proc/self/exe` reads "… (deleted)". Verified end to end — swapped a binary under a live daemon, sent `restart`, and watched the same pid come back on the same socket with its exe pointing at the replacement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 4. The redesign held, but its liveness check was fiction: the probe was `UnixStream::connect(socket).is_ok()`, and an AF_UNIX connect lands in the listen backlog and succeeds with nobody accepting — which is exactly the state of a daemon that has agreed to restart and is draining. So the first poll after an accepted restart answered "it's back" within ~20ms, against the daemon we had just retired. `RestartOutcome::Failed` — the branch the whole exit-code contract rests on — could essentially never fire, and an exec that failed would have exited 0 saying "muxad responsive". My own live check missed it because it inspected `/proc/PID/exe`, which the code never does. Verified now with a socket that is bound and silent: the old probe calls it up, the new one doesn't. Liveness is a round-trip: only a process that is accepting can answer, so it reads false across the handover and true once the new image serves. Three more from the same round: - The verify step could only warn. It is the step that *proves* nothing is answering, so it now downgrades the outcome to `Failed` and the command fails, which is what the contract claimed all along. - A restart whose reply we never read was classified "nothing happened, try the service manager". But the daemon arms the re-exec and starts draining before it writes that reply, so acceptance is already irrevocable: the fallback would have raced a daemon that was coming back (losing the bind and dying on SocketInUse), or told the user their old daemon was still serving mid-handover. A lost reply now means "may have committed" — wait and report what we see. - `restart` was advertised by every server, including one with no hook that would refuse it. A client that believes the tag stops looking for another way to restart the daemon, so the tag is now conditional on the hook. Also: a signal now disarms the re-exec flag. SIGTERM means stop, and it has to keep meaning that even if an IPC restart armed the flag a moment earlier — an operator or a service manager stopping the daemon must not get one that comes back. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Marking draft. Five review rounds in, this is not converging, and I'd rather say so than keep patching. What's solidThe daemon-side design is right and demonstrably works: That design deleted whole defect classes rather than fixing them — no two-daemon overlap (one process), no lost argv, no socket rediscovery, no "does a supervisor own this?". What isn'tThe CLI cannot tell whether the daemon came back. Every "is it up?" check so far has been unable to distinguish the old daemon from the new one:
The real problem underneath both: the probe has no identity. And re-exec preserves the pid, so a pid can't supply one either. Without knowing which daemon answered, every verdict built on it is a guess — which is why round 4's fix produced round 5's defects (a 3s verify that now fails the command on a daemon that starts healthily in 4s). The fix, for whoever picks this upGive the daemon a generation: an env var incremented across the Round-5 findings worth keeping regardless:
Not merging this as-is. The daemon half is worth salvaging; the CLI half needs the generation counter before it can honestly claim anything about the outcome. |
Round 5 found the liveness check still couldn't tell the old daemon from the new one — and traced it to the fact that my round-4 justification was simply false. A round-trip is *not* only answerable by the process that came back: `Server::run`'s `select!` is random-order, so the accept loop can take one more connection after the shutdown broadcast, and the dying daemon answers too. The probe had no identity, and `exec` keeps the pid, so nothing the CLI could observe distinguished "it restarted" from "it never left". Every timeout built on that (3s verify vs 15s wait vs the fallback's zero) was guessing, and they disagreed. Give the daemon an identity that survives the exec: a generation counter, carried in `MUXA_RESTART_GENERATION`, incremented on each re-exec, and surfaced in `hello`. "Did it come back" stops being a timing question and becomes "did the generation advance" — the one thing that actually answers it. `restart_daemon` reads the generation before asking, then waits for a *higher* one; the old daemon answering at the same generation no longer counts, however healthily it does so. The mutually inconsistent timeouts collapse into that single wait. Verified live: swapped a binary under a running daemon and sent `restart` twice — same pid throughout, generation 0 → 1 → 2, exe going from "… (deleted)" to the replacement each time. Also from round 5: - `ManualRequired` printed "muxad is still running the previous build" even on the `NotRunning` path, where nothing was running. The message no longer asserts daemon state — the remediation is the same either way. - PROTOCOL.md had the drain/reply ordering backwards and didn't mention `generation`; both corrected, and the `restart` doc now points at it as the confirmation signal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Why
muxa upgradebuilds the new binaries, then asks the OS service manager to restart the daemon. When there isn't one that works, it stops at "manual restart required" — the new code is installed but the running daemon keeps serving the old build until a human remembers to restart it.A container with systemd installed but not running hits this on every upgrade.
which systemctlsucceeds (the binary is right there in/usr/bin), so the path looks supported right up until the call fails:This bit me twice today on my own box — both times it presented as "muxad died", which is a misdiagnosis the upgrade flow shouldn't be inviting.
What
Fall back to replacing the daemon ourselves: ask the daemon on the socket this upgrade is already managing to drain, wait for it to actually let go, then start the new one detached.
This is not the
pkillfallback that was removed in 68cfba4, and the difference is the whole design. That one hunted for processes namedmuxadand signalled whatever it found. This one addresses exactly one daemon through its own control channel, identified by the socket the user pointed us at. A muxad belonging to another socket, checkout, or user is never a candidate.To make that possible,
shutdownjoins the IPC protocol:SIGTERMalready fires, so the drain path is unchanged and in-flight handlers still finish.Serverwithout a trigger answers with an error instead of exiting, so a caller can distinguish "declined" from "old daemon that never knew the method".0600, so anyone who can call it is already the user who owns the daemon — the same boundary that already letsingestforge arbitrary agent events. It doesn't widen the trust model.hellocapability, and documented in PROTOCOL.md. Additive, and the protocol is explicitly unstable pre-1.0.Notes
The detached spawn reuses init's shell-based
nohup … & disownrather than a newpre_exec+setsid: the workspace setsunsafe_code = "forbid", andsetsid(1)isn't on macOS anyway.init/apply.rsalready had exactly this logic, so it moved toinit::utiland both callers share it — no copy to drift.Two dry-run tests asserted the string
"manual restart required", which is the behavior this removes; they now pin the meaning instead. Their!contains("pkill")/!contains("SIGUSR1")assertions are deliberately kept — they're the guard against regressing 68cfba4, and this change must not.Verification
{"ok":true}→ exits → releases the socket → logs it.systemctl --user restart muxadfails here, then the socket fallback replaced the daemon (pid 1305768 → 1306054).cargo test --workspace(429 + 11 e2e), workspaceclippy -D warnings,fmt --checkall clean.🤖 Generated with Claude Code