diff --git a/cmd/agentop/cmd_service_platform.go b/cmd/agentop/cmd_service_platform.go index 8b0fa1c2c..b16a0a3c6 100644 --- a/cmd/agentop/cmd_service_platform.go +++ b/cmd/agentop/cmd_service_platform.go @@ -604,6 +604,14 @@ func controlService(goos, action string, p servicePaths, progress io.Writer) err if out, err := exec.Command("systemctl", args...).CombinedOutput(); err != nil { return fmt.Errorf("systemctl %s: %v: %s", strings.Join(args, " "), err, strings.TrimSpace(string(out))) } + // A stop of a unit systemd had already given up on is a no-op for systemd, so + // the unit stays failed, only now disabled, and status read the deliberate stop + // as a crash and advised a restart. Clearing it leaves inactive and disabled, + // which status reports as stopped. After the stop, never before: nothing is + // left running for the start limit to protect. + if action == "stop" { + clearStartLimit() + } return nil } @@ -615,10 +623,11 @@ func controlService(goos, action string, p servicePaths, progress io.Writer) err // tells the user to run failed with "start of the service was attempted too often", // and so did `service start` and a reinstall. // -// Only before a start someone asked for, never on systemd's own restarts, so the -// limit still stops a crash loop by itself. reset-failed on a unit that is not -// failed changes nothing; its error, for a unit systemd has not loaded, is not -// ours to report. +// Only around a start or stop someone asked for, never on systemd's own restarts, +// so the limit still stops a crash loop by itself. controlService also calls it +// after a stop, for the state that leaves; see there. reset-failed on a unit that +// is not failed changes nothing; its error, for a unit systemd has not loaded, is +// not ours to report. func clearStartLimit() { _ = exec.Command("systemctl", "--user", "reset-failed", systemdUnit).Run() //nolint:errcheck } @@ -637,8 +646,8 @@ type unitState struct { // activating/auto-restart between a crash and its restart, failed/failed once // the restarts run out. active, sub string - // UnitFileState: enabled after `service start`, disabled after `service stop`, - // which is what tells our stop from someone else's `systemctl --user stop`. + // UnitFileState: enabled after `service start`; disabled after `service stop`, + // or for a unit file that was never enabled, which systemd cannot tell apart. enabled string // Result, as systemd words why the unit last stopped: success, exit-code, // signal, start-limit-hit. diff --git a/cmd/agentop/cmd_service_systemd_integration_test.go b/cmd/agentop/cmd_service_systemd_integration_test.go index 386dcabdc..1690f1b25 100644 --- a/cmd/agentop/cmd_service_systemd_integration_test.go +++ b/cmd/agentop/cmd_service_systemd_integration_test.go @@ -551,6 +551,27 @@ func TestServiceLifecycle_RealSystemd(t *testing.T) { code, out = status() expect("status after recovering", code, out, 0, "Cortex is healthy according to") + // A stop of a unit systemd has given up on. systemd's own stop is a no-op on a + // failed unit, so before stop cleared it the unit stayed failed, only disabled, + // and status called the deliberate stop a crash and advised a restart. + if code, out := control("stop"); code != 0 { + t.Fatalf("stop before second crash loop: exit %d:\n%s", code, out) + } + if err := os.WriteFile(bin, []byte("#!/bin/sh\nexit 3\n"), 0o700); err != nil { //nolint:gosec + t.Fatal(err) + } + _, _ = control("start") + if !waitUntil(limitWait, func() bool { return unit().active == "failed" }) { + t.Fatalf("the second crash loop did not hit the start limit within %s; systemd says %+v", limitWait, unit()) + } + code, out = control("stop") + expect("stop of a unit systemd gave up on", code, out, 0) + if s := unit(); s.active != "inactive" || s.enabled != "disabled" { + t.Errorf("after stopping a failed unit systemd says %+v, want inactive and disabled", s) + } + code, out = status() + expect("status after stopping a failed unit", code, out, 0, "Cortex is stopped, and stays stopped") + code, out = run("uninstall", func(o, e io.Writer) int { return serviceUninstall(p, true, o, e) }) expect("uninstall", code, out, 0, "Removed.") code, out = status() diff --git a/cmd/agentop/cmd_service_systemd_test.go b/cmd/agentop/cmd_service_systemd_test.go index 622a78f3b..a8f154953 100644 --- a/cmd/agentop/cmd_service_systemd_test.go +++ b/cmd/agentop/cmd_service_systemd_test.go @@ -689,13 +689,16 @@ func TestControlService_Linux(t *testing.T) { // the right action. // // start and restart clear systemd's start limit first, so they can bring back a - // unit that crash-looped into start-limit-hit. stop does not: it has no start to - // unblock. + // unit that crash-looped into start-limit-hit. stop clears it after instead: it + // has no start to unblock, but it must not leave a deliberate stop reading as a + // crash. for _, tc := range []struct { action string want []string }{ - {"stop", []string{"--user disable --now cortex.service"}}, + // stop clears the failed state AFTER it, so a unit systemd had given up on + // ends inactive and disabled, which status reports as stopped, not crashed. + {"stop", []string{"--user disable --now cortex.service", "--user reset-failed cortex.service"}}, {"start", []string{"--user reset-failed cortex.service", "--user enable --now cortex.service"}}, {"restart", []string{"--user reset-failed cortex.service", "--user restart cortex.service"}}, } { diff --git a/docs/laptop-service.md b/docs/laptop-service.md index 064962599..ed5e95b69 100644 --- a/docs/laptop-service.md +++ b/docs/laptop-service.md @@ -611,7 +611,8 @@ exited 1. On macOS it still does. `service start`, `service restart` and `service install` clear systemd's record of the crashes first, so they can start a unit systemd had given up on without waiting out -its five-minute window. +its five-minute window. `service stop` clears it after, so stopping a unit systemd had +given up on reads as stopped, not as a crash. If it says `config: … will not load`, an edit to `~/.cortex/config.yaml` broke it (`config: no config at …` means the file is gone). A proxy that was already running