fix(#6044): make grafel stop actually stop grafel — service-aware, persistent, honestly confirmed - #6049
Merged
Merged
Conversation
`grafel stop` only sent a Stop RPC and reported success unconditionally; when the daemon runs as an OS service (launchd/systemd/schtasks), the service manager's own keep-alive/restart respawned it before the caller could observe anything, and launchd's unconditional KeepAlive made this happen almost instantly. `start`/`restart` already routed through the service manager when one was installed for this root — `stop` did not. - internal/daemon/service: add Stop/stopService/stopConverge. It Unloads (no re-Load) and confirms via Status() that the daemon is actually down before returning success, returning a genuine error otherwise (same defect class as #5991's `grafel reset`). - Semantics chosen: stop is PERSISTENT — it survives the next login/reboot, matching what systemd's `disable --now` and schtasks task deletion already do in Unload(). macOS's bootout alone only clears the current session (RunAtLoad still fires at next login), so launchd_darwin.go now pairs bootout with `launchctl disable`, and Load() unconditionally `enable`s before bootstrap so the ordinary install/start/restart Unload;Load cycle is unaffected. `grafel start` documents itself as the way back; help text on `stop` states this explicitly. - internal/cli/watcher_ctl.go: runDaemonStop now checks serviceInstalledForThisRoot() and routes to serviceStopForThisRoot (mirrors serviceRestartForThisRoot) when installed. The RPC-only path (manually-started foreground daemon) now polls for the socket to actually disappear before reporting success, instead of printing "stop requested" and returning 0 regardless. runDaemonRestart tolerates the new errStopNotConfirmed sentinel exactly like ErrDaemonNotRunning, since it performs its own more patient pid-based wait/SIGKILL escalation right after. - internal/daemon/supervise_unix.go / supervise.go: the engine child's drain signals (SIGTERM, and the SIGKILL escalation) are now group-directed (kill(-pid)), following the #5999 precedent in sched/nice_unix.go. Setpgid alone isolates the child from serve's own signal group but a single-pid signal still leaves any grandchildren the engine child forks running as orphans after drain — group-directed closes that gap (issue #6044 item 4). Mutation-tested: reverting each behavioural change individually (the service-aware gate, the RPC confirmation, stopConverge's Status check, and the group-directed signal) produces a real test failure, not a build failure, in every case. go build ./... && go vet ./... && gofmt -l . all clean. rtk proxy go test ./internal/cli/... ./internal/daemon/... ./cmd/grafel/... exit=0.
…persistence, fix pid-reuse gap Fixes for all 7 findings from the independent review of fix/6044-service-aware-stop: 1. HIGH (blocking) — stopConverge confirmed on the wrong axis. On darwin, Status().Running came from `launchctl list`'s exit code — "is the job loaded in the domain", not "is the daemon alive". A daemon started manually (never loaded in the domain) made bootout a no-op and Status() say "not running" while the daemon was still live, so `grafel stop` would have printed "daemon stopped" over it. stopConverge now polls Probe() (the actual socket) via a new waitStopped — the mirror image of the existing waitReady — before declaring success. 2/3. Persistent-stop decision had zero test coverage, and the CLI's "survives reboot" claim was unconditional even though only a discarded-error `launchctl disable` call made it true. launchctl bootout/bootstrap/disable/enable are now package vars (launchctlBootout/Bootstrap/Disable/Enable) so Unload/Load's full behavior is testable without ever touching the real gui/$UID/com.grafel.daemon. Unload records the disable outcome on launchdManager.lastDisableErr; stopService reads it and returns a genuine failure (not a lying success message) when persistence could not be confirmed. 4. SIGKILL escalation's group-directedness was unpinned — the drain test's helper died on the first SIGTERM, so it never exercised signalKill at all. Added a helper that ignores SIGTERM (forcing the drain timeout and the SIGKILL escalation to actually fire) plus a narrow TestSignalKill_IsGroupDirected unit test. Also fixed a latent flaw in the original grandchild fixture: a plain `sleep` grandchild died to the (already-correct) group-directed SIGTERM before the SIGKILL branch was ever reached, so the escalation test passed regardless of signalKill's correctness. The grandchild now also traps SIGTERM. 5. Group-kill pid-reuse race: terminateChild's SIGKILL escalation races cmd.Wait() on a separate goroutine — unlike the #5999 precedent, whose Cancel hook is ordered strictly before Wait frees the pid. A blind kill(-pid) could hit a just-reused pid's unrelated process group. Added a Getpgid(pid) == pid check before negating; falls back to a single-pid signal otherwise. 6. Two uncovered callers of the new persistence: `grafel uninstall` now re-enables the label after teardown (a full uninstall shouldn't leave a disabled-override with no plist to explain it), and install.sh's restart_daemon now detects "plist present but not loaded" (exactly what `grafel stop` leaves behind) and calls `launchctl enable` before `bootstrap`, instead of falling through to the generic "nothing registered" case and silently giving up. Both pinned with runtime tests (fake launchctl on PATH sourcing install.sh as a library). 7. LOW — the service-path stop failure message now names a next step (check the log / run `grafel status`), matching the RPC path. Mutation-tested end to end: every one of the above — including the two findings that previously survived (deleting disable/enable, mutating only signalKill) — now produces a real test failure when reverted individually, confirmed by reverting each change, running the specific test, observing the failure, and restoring. go build ./... && go vet ./... && gofmt -l . all clean (both before and after; tree-sitter/CGO cross-compile noise on GOOS=linux/windows for internal/cli is pre-existing and unrelated — internal/daemon/service and internal/daemon build clean under both). rtk proxy go test ./internal/cli/... ./internal/daemon/... ./cmd/grafel/... ./internal/install/... exit=0.
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.
grafel stopdid not stop grafel. It sent the Stop RPC, the daemon obeyed, and launchd immediately respawned it — the LaunchAgent declares an unconditionalKeepAlive. The command printedstop requestedand returned 0 either way, so the user saw the service still running and reasonably concluded the command was broken.Reported by a user, reproduced four times on macOS. The asymmetry is the root cause:
grafel startis service-aware (OS service detected for this daemon; restarting via the OS service manager);stopwas not.startwent through launchd,stopwent around it.Three
stopcalls inside the same second returnedstop requested/daemon not running/stop requested— launchd resurrecting the daemon between consecutive calls. That flapping is what a user experiences as "sometimes it stops it, and it never stays down".stopis now persistent, and that decision is deliberateThe issue asked for this to be chosen rather than defaulted into.
bootoutstops the job now but the plist still loads at login, so macOS pairs it withlaunchctl disable;Load()unconditionallyenables beforebootstrapso the ordinary install/start/restart cycle is unaffected. systemd'sUnload()was alreadydisable --nowand schtasks's already deletes the registration, so all three platforms now agree: stop means stay down;grafel startis the way back.grafel stop --helpsays so explicitly.Confirmation is on the socket, not the job registry
The first implementation confirmed via
Status(), which on darwin derivesRunningpurely fromlaunchctl list <label>'s exit code — is the job loaded in the domain, not is the process alive. Afterbootoutthe job is gone by definition, making the check very nearly a tautology, and blind to a manually-started daemon outside the domain entirely.Concretely: a plist on disk plus a hand-started daemon routes to the service path (
serviceInstalledForThisRootonly stats the plist).Unload()gives up after 3s with errors ignored,bootoutno-ops on an unloaded job with its exit status discarded, andstopprinted "daemon stopped" over a live daemon.stopConvergenow pollsProbe()— the real socket — viawaitStopped, mirroring the existingwaitReady. Pinned byTestStopConverge_TrustsProbeNotStatus, whose fixture composesprobeAlwaysUpwithloaded:falseso it genuinely models "Status says down, socket says up", and which asserts its own fixture precondition before the real assertion.The persistence decision is now falsifiable
launchctlBootout/Bootstrap/Disable/Enableare package vars. Before that seam existed, deleting both thedisableandenablecalls outright left the entire suite green — the diff's most consequential decision was unpinned.Unloadrecords the outcome onlastDisableErrandstopServicereads it, so the CLI no longer prints "even across reboot" over adisablewhose error it discarded.Two callers of the new persistence that the first pass missed:
uninstallnow re-enables the label after teardown rather than leaving a launchd override with no plist to explain it, andinstall.sh'srestart_daemondetects "plist present but not loaded" and callsenablebeforebootstrapinstead of silently giving up on any machine wherestophad been used.The orphaned-engine window
A hard exit of
serveorphaned itsenginechild —ppidflips to 1 — while launchd spawned a replacement alongside it: two process trees against one state directory, the shape that produces torn writes and doubled memory. Drain signals are now group-directed.groupOrSingleSignalchecksGetpgid(pid) == pidbefore negating. The #5999 precedent (sched/nice_unix.go) was verified rather than cited: it is safe there becauseCancelruns strictly beforeWaitfrees the pid. This supervisor's drain races a separate reaping goroutine and has no such ordering, so without the leadership check a lost race would signal whatever group inherited the reused pid. A microsecond TOCTOU window remains and is documented as such — a reuse race is not constructible in a test, and the comment says so rather than implying coverage.defaultEngineChildCommand'scmd.Env = os.Environ()and its load-bearing comment are byte-identical;git diff -- internal/daemon/supervise.gois empty.Verification
go build ./... && go vet ./... && gofmt -l .clean. 30 packages green at raw exit 0 acrossinternal/cli,internal/daemon,cmd/grafel,internal/install.-raceclean on the service and supervisor packages.Mutation battery rebuilt against the final tree; every behavioural change dies individually. Two mutations that survived the first round now die, each naming its hole: reverting
stopConvergetoStatus()-only, and deleting thedisable/enablecalls.A false-pass fixture was found, disclosed by the author, and independently reproduced. The original SIGKILL test used a plain
sleepgrandchild, which died to the group SIGTERM before the drain timeout ever elapsed — so it passed regardless of whethersignalKillwas group-directed. The grandchild now doestrap '' TERM; exec sleep 300(SIG_IGNsurvivesexec), and the escalation branch is genuinely reached: 0.59s blind versus 6.63s biting. Reverting the trap alone, withsignalKillstill broken, restores the false pass — which is how the fixture's load-bearing element was confirmed rather than assumed.signalTerminateandsignalKillnow have independent pins; mutating either fails only its own tests.Known and disclosed
Probe()proves socket-death, not process-death.listener.Close()precedes the remaining graceful tail, sowaitStoppedcan return before the process is gone — the issue's own worst case, a stalled RPC mid-index. Bounded bydefaultShutdownWatchdog = 5s, which force-exits the whole tail, so the window is ≤~5s. The definitive axis (ReadPIDFile+pidStillAlive, already used byrunDaemonRestart) is in-tree and unused here. Follow-up, not a blocker.list-unit-filesand stillrestarts, so an upgrade brings the daemon back running-but-still-disabled-at-boot, while macOS now fully reverts the stop. Neither tells the user their deliberate stop was undone. Follow-up.waitStopped's budget is 60s against the RPC path's 5s. Success is fast (the first probe short-circuits); a stubborn daemon now blocks for a minute before reporting. Correct-over-fast, but the asymmetry is unexplained.Independently adversarially reviewed across two rounds; five of six findings verified closed by execution.
Closes #6044