From 8deb839089d13116d6e1d61d9c13588732786600 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Sun, 20 Sep 2026 21:58:42 -0400 Subject: [PATCH 01/12] test: Add a fakeSystemctl/fakeLoginctl harness for the Linux service path loadService, controlService, supervisorRunning, and unloadService checked runtime.GOOS directly, so none of their Linux logic could be exercised from a non-Linux host, or against systemd's real is-active vocabulary (activating/failed/deactivating) rather than just the two extremes. Refactor all four to take goos explicitly, mirroring the fix already applied to renderUnitFor for the same reason. Add cmd_service_systemd_test.go: a fakeSystemctl/fakeLoginctl PATH-decoy harness mirroring fakeLaunchctl, with 22 subtests covering daemon-reload/enable failures, the linger enable/marker lifecycle, unloadService's marker-gated disable-linger, and controlService's verb-per-action mapping. supervisorRunning's is-active handling for the non-"active" states turned out to already be correct; it was just unverified until now. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service.go | 12 +- authbridge/cmd/abctl/cmd_service_platform.go | 20 +- .../cmd/abctl/cmd_service_systemd_test.go | 341 ++++++++++++++++++ .../2026-09-16-linux-install-systemd-945.md | 281 +++++++++++++++ 4 files changed, 638 insertions(+), 16 deletions(-) create mode 100644 authbridge/cmd/abctl/cmd_service_systemd_test.go create mode 100644 authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md diff --git a/authbridge/cmd/abctl/cmd_service.go b/authbridge/cmd/abctl/cmd_service.go index ec519873f..837314a33 100644 --- a/authbridge/cmd/abctl/cmd_service.go +++ b/authbridge/cmd/abctl/cmd_service.go @@ -427,7 +427,7 @@ func serviceInstall(p servicePaths, yes, forceRestart bool, stdout, stderr io.Wr fmt.Fprintf(stdout, "Wrote %s\n", p.unitFile) } - if err := loadService(p, stdout); errors.Is(err, errLingerUnavailable) { + if err := loadService(runtime.GOOS, p, stdout); errors.Is(err, errLingerUnavailable) { // The unit IS loaded, so this is a caveat rather than a failure: keep going, // but never claim it survives a logout. fmt.Fprintf(stderr, "abctl: %v\n", err) @@ -449,7 +449,7 @@ func serviceInstall(p servicePaths, yes, forceRestart bool, stdout, stderr io.Wr // keeps the ports, the supervised copy loses the bind race and crash-loops, and // the probe cheerfully succeeds against the survivor. Ask the supervisor whether // OUR job is actually up before believing the probe. - if running, why := supervisorRunning(p); !running { + if running, why := supervisorRunning(runtime.GOOS, p); !running { fmt.Fprintf(stderr, "abctl: the unit loaded but the supervisor does not report it running (%s).\n"+ " Something else may hold the ports — check for a Cortex you started by hand:\n"+ " pgrep -fl authbridge-prox\n"+ @@ -512,7 +512,7 @@ func serviceUninstall(p servicePaths, yes bool, stdout, stderr io.Writer) int { fmt.Fprintln(stdout, "Not changed.") return exitDeclined } - if err := unloadService(p); err != nil { + if err := unloadService(runtime.GOOS, p); err != nil { // Report but keep going: leaving the unit file behind would make a // reinstall look installed-but-dead. fmt.Fprintf(stderr, "abctl: %v\n", err) @@ -617,7 +617,7 @@ func serviceControl(action string, p servicePaths, stdout, stderr io.Writer) int // umask — measured at 0644, which silently undid the 0600 this sets. tightenLog(p.logFile, stderr) } - if err := controlService(action, p, stdout); err != nil { + if err := controlService(runtime.GOOS, action, p, stdout); err != nil { fmt.Fprintf(stderr, "abctl: %v\n", err) return 1 } @@ -644,7 +644,7 @@ func serviceControl(action string, p servicePaths, stdout, stderr io.Writer) int // Same gate install uses: an unadopted proxy holding the ports answers the // probe while OUR job crash-loops on the bind, so health alone would report a // restart that did not happen. - if running, why := supervisorRunning(p); !running { + if running, why := supervisorRunning(runtime.GOOS, p); !running { fmt.Fprintf(stderr, "abctl: %sed, but the supervisor does not report it running (%s).\n"+ " Check for a Cortex started by hand holding the ports: pgrep -fl authbridge-prox\n", action, why) for _, line := range lastLines(p.logFile, 5) { @@ -794,7 +794,7 @@ func serviceIsCurrent(p servicePaths) bool { if runtime.GOOS == "darwin" && !strings.Contains(body, "--supervise") { return false } - if running, _ := supervisorRunning(p); !running { + if running, _ := supervisorRunning(runtime.GOOS, p); !running { return false } if p.healthURL == "" { diff --git a/authbridge/cmd/abctl/cmd_service_platform.go b/authbridge/cmd/abctl/cmd_service_platform.go index a37d491d3..02e399a61 100644 --- a/authbridge/cmd/abctl/cmd_service_platform.go +++ b/authbridge/cmd/abctl/cmd_service_platform.go @@ -148,8 +148,8 @@ WantedBy=default.target ` } -func loadService(p servicePaths, progress io.Writer) error { - if runtime.GOOS == "darwin" { +func loadService(goos string, p servicePaths, progress io.Writer) error { + if goos == "darwin" { uid := strconv.Itoa(os.Getuid()) target := "gui/" + uid + "/" + launchdLabel // Clear any disable left by `service stop`: a disabled label cannot be @@ -266,8 +266,8 @@ func lingerEnabled(uid string) bool { return !strings.Contains(strings.ToLower(string(out)), "linger=no") } -func unloadService(p servicePaths) error { - if runtime.GOOS == "darwin" { +func unloadService(goos string, p servicePaths) error { + if goos == "darwin" { uid := strconv.Itoa(os.Getuid()) if out, err := exec.Command("launchctl", "bootout", "gui/"+uid+"/"+launchdLabel).CombinedOutput(); err != nil { return fmt.Errorf("launchctl bootout: %v: %s", err, strings.TrimSpace(string(out))) @@ -380,8 +380,8 @@ func dialableAddr(addr string) string { } // controlService maps stop/start/restart onto the platform's supervisor. -func controlService(action string, p servicePaths, progress io.Writer) error { - if runtime.GOOS == "darwin" { +func controlService(goos string, action string, p servicePaths, progress io.Writer) error { + if goos == "darwin" { target := "gui/" + strconv.Itoa(os.Getuid()) + "/" + launchdLabel switch action { case "stop": @@ -403,10 +403,10 @@ func controlService(action string, p servicePaths, progress io.Writer) error { } return nil case "start": - return loadService(p, progress) // loadService clears the disable + return loadService(goos, p, progress) // loadService clears the disable default: // restart _ = exec.Command("launchctl", "bootout", target).Run() //nolint:errcheck - return loadService(p, progress) + return loadService(goos, p, progress) } } if _, err := exec.LookPath("systemctl"); err != nil { @@ -431,8 +431,8 @@ func controlService(action string, p servicePaths, progress io.Writer) error { // supervisorRunning asks the supervisor whether OUR job is up, which health alone // cannot establish: an unadopted proxy keeps the ports, the supervised copy // crash-loops on the bind, and the probe succeeds against the survivor. -func supervisorRunning(p servicePaths) (bool, string) { - if runtime.GOOS == "darwin" { +func supervisorRunning(goos string, p servicePaths) (bool, string) { + if goos == "darwin" { target := "gui/" + strconv.Itoa(os.Getuid()) + "/" + launchdLabel // Poll rather than sample once. Immediately after a kickstart the job passes // through transient states — "xpcproxy" while launchd's exec helper is still diff --git a/authbridge/cmd/abctl/cmd_service_systemd_test.go b/authbridge/cmd/abctl/cmd_service_systemd_test.go new file mode 100644 index 000000000..d774b17b6 --- /dev/null +++ b/authbridge/cmd/abctl/cmd_service_systemd_test.go @@ -0,0 +1,341 @@ +package main + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +// fakeSystemctl puts a systemctl on PATH that behaves however body says, mirroring +// fakeLaunchctl (cmd_service_restricted_test.go) for the Linux side of the same +// functions. loadService, controlService, supervisorRunning and unloadService all +// shell out to the real systemctl; until now nothing exercised their reaction to +// systemd's actual vocabulary (active/activating/failed/deactivating, or a +// daemon-reload/enable failure) — only the generated unit TEXT was tested. +// +// All tests in this file pass "linux" explicitly to the functions under test, +// rather than relying on runtime.GOOS, so they run for real on any host — +// including the one these were written on. +func fakeSystemctl(t *testing.T, body string) { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "systemctl"), []byte(body), 0o700); err != nil { //nolint:gosec + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// fakeLoginctl puts a loginctl on PATH that behaves however body says. +func fakeLoginctl(t *testing.T, body string) { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "loginctl"), []byte(body), 0o700); err != nil { //nolint:gosec + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// noSystemctlOnPath points PATH at an empty directory, so exec.LookPath("systemctl") +// fails the same way it would on a box with no systemd at all. +func noSystemctlOnPath(t *testing.T) { + t.Helper() + t.Setenv("PATH", t.TempDir()) +} + +// callLog returns a path inside dir and a shell snippet that appends the fake's +// own arguments to it — so a test can assert exactly what our code invoked, not +// just that it received a canned answer back. +func callLog(t *testing.T) (path string, appendLine string) { + t.Helper() + path = filepath.Join(t.TempDir(), "calls.log") + return path, fmt.Sprintf(`echo "$@" >> %s`, path) +} + +func readCallLog(t *testing.T, path string) []string { + t.Helper() + b, err := os.ReadFile(path) //nolint:gosec + if err != nil { + return nil + } + var lines []string + for _, l := range strings.Split(strings.TrimRight(string(b), "\n"), "\n") { + if l != "" { + lines = append(lines, l) + } + } + return lines +} + +func TestSupervisorRunning_Linux(t *testing.T) { + p := servicePathsFixture(t) + + t.Run("active reads as running", func(t *testing.T) { + fakeSystemctl(t, "#!/bin/sh\necho 'active'\nexit 0\n") + running, why := supervisorRunning("linux", p) + if !running || why != "is-active = active" { + t.Errorf("running=%v why=%q, want true/%q", running, why, "is-active = active") + } + }) + + // systemctl is-active exits non-zero for every state except "active", but still + // prints the real state on stdout. Only the exit-0 case may read as running. + for _, state := range []string{"activating", "failed", "deactivating", "inactive"} { + t.Run(state+" does not read as running", func(t *testing.T) { + fakeSystemctl(t, "#!/bin/sh\necho '"+state+"'\nexit 3\n") + running, why := supervisorRunning("linux", p) + if running { + t.Errorf("state=%s read as running", state) + } + if why != "is-active = "+state { + t.Errorf("why = %q, want %q", why, "is-active = "+state) + } + }) + } + + t.Run("no systemctl on PATH does not block", func(t *testing.T) { + noSystemctlOnPath(t) + running, why := supervisorRunning("linux", p) + if !running || why != "" { + t.Errorf("running=%v why=%q, want true/\"\" — a box with no systemd must not "+ + "be reported as not-running", running, why) + } + }) + + t.Run("systemctl present but silent on failure", func(t *testing.T) { + fakeSystemctl(t, "#!/bin/sh\nexit 1\n") + running, why := supervisorRunning("linux", p) + if running || why != "systemctl is-active gave no answer" { + t.Errorf("running=%v why=%q, want false/%q", running, why, "systemctl is-active gave no answer") + } + }) +} + +func TestLoadService_Linux(t *testing.T) { + t.Run("no systemctl on PATH", func(t *testing.T) { + p := servicePathsFixture(t) + noSystemctlOnPath(t) + err := loadService("linux", p, io.Discard) + if err == nil || !strings.Contains(err.Error(), "systemctl not found") { + t.Errorf("err = %v, want it to say systemctl was not found", err) + } + }) + + t.Run("daemon-reload failure is reported, enable is never attempted", func(t *testing.T) { + p := servicePathsFixture(t) + fakeSystemctl(t, `#!/bin/sh +if [ "$2" = "daemon-reload" ]; then + echo "boom: unit has a syntax error" >&2 + exit 1 +fi +echo "enable --now should not have run" >&2 +exit 1 +`) + err := loadService("linux", p, io.Discard) + if err == nil || !strings.Contains(err.Error(), "daemon-reload") || !strings.Contains(err.Error(), "boom") { + t.Errorf("err = %v, want it to name daemon-reload and the underlying reason", err) + } + }) + + t.Run("enable --now failure is reported", func(t *testing.T) { + p := servicePathsFixture(t) + fakeSystemctl(t, `#!/bin/sh +if [ "$2" = "daemon-reload" ]; then + exit 0 +fi +echo "nope: unit not found" >&2 +exit 1 +`) + err := loadService("linux", p, io.Discard) + if err == nil || !strings.Contains(err.Error(), "enable") || !strings.Contains(err.Error(), "nope") { + t.Errorf("err = %v, want it to name enable --now and the underlying reason", err) + } + }) + + t.Run("linger already enabled: skipped, no marker written", func(t *testing.T) { + p := servicePathsFixture(t) + fakeSystemctl(t, "#!/bin/sh\nexit 0\n") + fakeLoginctl(t, `#!/bin/sh +if [ "$1" = "show-user" ]; then + echo "Linger=yes" + exit 0 +fi +echo "enable-linger should not have run" >&2 +exit 1 +`) + if err := loadService("linux", p, io.Discard); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := os.Stat(lingerMarker(p)); err == nil { + t.Error("marker written even though linger was already on") + } + }) + + t.Run("linger not enabled, enable-linger succeeds: marker written", func(t *testing.T) { + p := servicePathsFixture(t) + fakeSystemctl(t, "#!/bin/sh\nexit 0\n") + fakeLoginctl(t, `#!/bin/sh +if [ "$1" = "show-user" ]; then + echo "Linger=no" + exit 0 +fi +exit 0 +`) + if err := loadService("linux", p, io.Discard); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, err := os.Stat(lingerMarker(p)); err != nil { + t.Error("no marker written after abctl enabled linger itself") + } + }) + + t.Run("linger not enabled, enable-linger fails: caveat, not fatal, no marker", func(t *testing.T) { + p := servicePathsFixture(t) + fakeSystemctl(t, "#!/bin/sh\nexit 0\n") + fakeLoginctl(t, `#!/bin/sh +if [ "$1" = "show-user" ]; then + echo "Linger=no" + exit 0 +fi +exit 1 +`) + err := loadService("linux", p, io.Discard) + if !errors.Is(err, errLingerUnavailable) { + t.Errorf("err = %v, want errLingerUnavailable", err) + } + if _, serr := os.Stat(lingerMarker(p)); serr == nil { + t.Error("marker written even though enable-linger failed") + } + }) +} + +func TestUnloadService_Linux(t *testing.T) { + t.Run("no systemctl on PATH: nil, nothing attempted", func(t *testing.T) { + p := servicePathsFixture(t) + noSystemctlOnPath(t) + if err := unloadService("linux", p); err != nil { + t.Errorf("err = %v, want nil — nothing could have been loaded", err) + } + }) + + t.Run("no marker: loginctl is never called", func(t *testing.T) { + p := servicePathsFixture(t) + logPath, logLine := callLog(t) + fakeLoginctl(t, "#!/bin/sh\n"+logLine+"\nexit 0\n") + fakeSystemctl(t, "#!/bin/sh\nexit 0\n") + if err := unloadService("linux", p); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if calls := readCallLog(t, logPath); len(calls) != 0 { + t.Errorf("loginctl called %v times, want 0 — no marker means we never enabled linger", len(calls)) + } + }) + + t.Run("marker present: disable-linger runs and marker is removed", func(t *testing.T) { + p := servicePathsFixture(t) + if err := os.WriteFile(lingerMarker(p), []byte("enabled by abctl\n"), 0o600); err != nil { + t.Fatal(err) + } + logPath, logLine := callLog(t) + fakeLoginctl(t, "#!/bin/sh\n"+logLine+"\nexit 0\n") + fakeSystemctl(t, "#!/bin/sh\nexit 0\n") + if err := unloadService("linux", p); err != nil { + t.Fatalf("unexpected error: %v", err) + } + calls := readCallLog(t, logPath) + if len(calls) != 1 || !strings.Contains(calls[0], "disable-linger") { + t.Errorf("loginctl calls = %v, want exactly one disable-linger", calls) + } + if _, err := os.Stat(lingerMarker(p)); err == nil { + t.Error("marker still present after unload") + } + }) + + t.Run("disable --now failure is reported", func(t *testing.T) { + p := servicePathsFixture(t) + fakeSystemctl(t, `#!/bin/sh +echo "unit not loaded" >&2 +exit 1 +`) + err := unloadService("linux", p) + if err == nil || !strings.Contains(err.Error(), "disable") || !strings.Contains(err.Error(), "unit not loaded") { + t.Errorf("err = %v, want it to name disable --now and the underlying reason", err) + } + }) +} + +func TestLingerEnabled(t *testing.T) { + cases := []struct { + name string + body string + want bool + }{ + {"explicit yes", "#!/bin/sh\necho 'Linger=yes'\nexit 0\n", true}, + {"explicit no", "#!/bin/sh\necho 'Linger=no'\nexit 0\n", false}, + // A parse failure reads as "already on" — errs toward leaving the user's + // setting alone rather than us silently flipping it. + {"malformed output reads as already on", "#!/bin/sh\necho 'garbage'\nexit 0\n", true}, + {"loginctl itself fails: reads as already on", "#!/bin/sh\nexit 1\n", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fakeLoginctl(t, tc.body) + if got := lingerEnabled("501"); got != tc.want { + t.Errorf("lingerEnabled() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestControlService_Linux(t *testing.T) { + p := servicePathsFixture(t) + + t.Run("no systemctl on PATH", func(t *testing.T) { + noSystemctlOnPath(t) + err := controlService("linux", "stop", p, io.Discard) + if err == nil || !strings.Contains(err.Error(), "systemctl not found") { + t.Errorf("err = %v, want it to say systemctl was not found", err) + } + }) + + // stop and start are the PERSISTENT forms (disable/enable --now), so a stop + // survives a reboot the same way launchd's bootout+disable pairing does; only + // restart stays transient. Nothing previously proved the right verb went with + // the right action. + for _, tc := range []struct { + action string + want string + }{ + {"stop", "--user disable --now cortex.service"}, + {"start", "--user enable --now cortex.service"}, + {"restart", "--user restart cortex.service"}, + } { + t.Run(tc.action+" invokes the right systemctl verb", func(t *testing.T) { + logPath, logLine := callLog(t) + fakeSystemctl(t, "#!/bin/sh\n"+logLine+"\nexit 0\n") + if err := controlService("linux", tc.action, p, io.Discard); err != nil { + t.Fatalf("unexpected error: %v", err) + } + calls := readCallLog(t, logPath) + if len(calls) != 1 || calls[0] != tc.want { + t.Errorf("systemctl called with %v, want exactly [%q]", calls, tc.want) + } + }) + } + + t.Run("underlying failure is reported with the exact command and reason", func(t *testing.T) { + fakeSystemctl(t, `#!/bin/sh +echo "kaboom" >&2 +exit 1 +`) + err := controlService("linux", "stop", p, io.Discard) + if err == nil || + !strings.Contains(err.Error(), "--user disable --now cortex.service") || + !strings.Contains(err.Error(), "kaboom") { + t.Errorf("err = %v, want it to name the exact systemctl invocation and stderr", err) + } + }) +} diff --git a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md new file mode 100644 index 000000000..684c8d814 --- /dev/null +++ b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md @@ -0,0 +1,281 @@ +# Issue #945 — Verified Linux install and systemd service lifecycle + +**Status:** research + gap analysis complete. Landed 2026-09-17: the `TimeoutStopSec` fix +(bullet 7) and the Tier 2 `fakeSystemctl`/`fakeLoginctl` test harness (bullet 6), which +required refactoring four functions to take `goos` explicitly (see "Cross-cutting themes"). +Tiers 3-5 (real-systemd integration test, install.sh smoke test, reboot check) not started. +**Owner:** Alan Cha (per epic #962 owner table: "Linux install, release smoke tests, reboot +verification, CI"). +**Repo:** rossoctl/cortex. This doc lives in the same directory as other planning docs +(`authbridge/docs/superpowers/plans/`) for consistency, but is a research/gap-analysis +reference, not yet an execution-ready task plan. + +## Why this doc exists + +Assigned three issues (#945, #956, #957) under epic #962 ("Cortex v0.9.0 — stable local tool +for coding agents", due 2026-09-30). This doc captures the research done to plan #945 +specifically, so the findings don't have to be re-derived later. #956/#957 have their own +dependencies (see "Relationship to other issues" below) and are intentionally out of scope +here. + +## The three assigned issues and priority order + +| # | Title | Why this order | +|---|---|---| +| **#945** | Verified Linux install and systemd service lifecycle | **Do first.** Self-contained, no dependency on other owners' unfinished work. #957 automates verifying this. | +| **#957** | Release smoke test on Linux CI runners | **Second.** Automates #945; same owner, same platform, lowest marginal complexity once #945's gaps are closed. Partial dependency on #955 (unattended capture path, owned by @esnible) for one checklist item (asserting a non-zero token count). | +| **#956** | Release smoke test on macOS CI runners | **Last.** Depends on #944 (macOS install, owned by @huang195) and #955 — two external blockers outside this owner's control, on a platform (launchd) not owned here. | + +## Correcting an early false start: this is not greenfield + +Initial read of the issue text (and of a stale local git checkout, ~452 commits behind +`origin/main`) suggested none of this existed yet. After syncing to `origin/main`, that's +wrong: the core service-lifecycle feature already shipped in **PR #876** ("Feat: Keep Cortex +running across crashes and logins (launchd / systemd)"), with follow-on fixes: + +- **#880** — Fix: upgrades fail with launchd `EIO` (bootout race) — macOS-only issue. +- **#897** — Fix: detect environments that cannot run a service, and say what to do. +- **#931** — Fix: say when a new CA leaves running agents unobservable. +- **#911** — Fix: trust the bridge CA where it matters, stop intercepting dev tooling. + +So #945 is a **verification and gap-closing** issue on top of substantially-built +functionality, not a build-from-scratch feature. The lesson for next time: always confirm +`git fetch` / compare against `origin/main` before concluding something doesn't exist. + +## Conceptual background: what systemd and launchd are for, and why the two platforms differ + +A service manager's job: start a background process, notice when it dies (including +crashes, not just deliberate stops), and decide whether to relaunch it — without a human +watching a terminal. + +**The key asymmetry that shapes the whole design:** + +- **systemd** (Linux): a unit file's `Restart=on-failure` is honored by systemd's own + supervisor loop, reliably, even for a unit just `systemctl --user start`ed while already + logged in (exactly Cortex's `curl | sh` scenario). +- **launchd** (macOS): the team found — and documented in code comments — that launchd's + equivalent (`KeepAlive`, tried alongside `StartInterval` and `RunAtLoad`) does **not** + reliably restart a LaunchAgent added mid-session (as opposed to one present at boot when + launchd first scans `LaunchAgents`). Since Cortex is always installed mid-session, this + gap is squarely in the installer's path. + +**Consequence for the architecture:** + +- **macOS** runs **two processes**: launchd starts a small Go-written supervisor + (`authbridge-proxy --supervise`, in `authbridge/cmd/authbridge-proxy/supervise.go`) that + itself watches and restarts the actual proxy child. launchd supervises the supervisor; + the supervisor does the real crash-recovery job launchd won't reliably do. +- **Linux** runs **one process**: `authbridge-proxy` directly as the systemd unit's + `ExecStart=`, with `Restart=on-failure` doing all the crash-recovery work natively. No + supervisor binary involved. Confirmed in code: + `cmd_service_platform.go:92-93` and `TestSupervisionIsPlatformCorrect` + (`cmd_service_test.go:312-332`) explicitly assert Linux does **not** use `--supervise`. + +This means **#945 is structurally simpler than #944** on the crash-recovery axis — but it +also means the Linux "Restart=on-failure actually works" assumption has never been +put through the same real-world verification (and design correction) that produced the +macOS supervisor. That's the single biggest theme in the gap analysis below. + +## Where things live (for orientation) + +- `authbridge/install.sh` — the `curl | sh` installer (~1300 lines, POSIX `sh`). +- `authbridge/install_test.sh` — installer unit tests (shell functions in isolation, no + real network/systemctl). +- `authbridge/cmd/abctl/cmd_service.go` — cross-platform `abctl service` command logic + (install/uninstall/status/start/stop/restart), ~870 lines. +- `authbridge/cmd/abctl/cmd_service_platform.go` — the OS-specific half: systemd unit + rendering, launchd plist rendering, `systemctl`/`launchctl`/`loginctl` invocations, ~723 + lines. +- `authbridge/cmd/authbridge-proxy/supervise.go` — the macOS-only (and unsupervised-fallback) + Go restart loop, ~118 lines. +- `authbridge/cmd/authbridge-proxy/main.go` — the proxy itself; on SIGTERM/SIGINT does a + graceful shutdown with a **15-second drain** (`context.WithTimeout(..., 15*time.Second)`, + `main.go:671`) — every downstream "does stop tolerate the drain" question traces back to + this constant. +- `authbridge/docs/laptop-service.md` — user-facing doc for `abctl service *`, + `~/.cortex/` layout, restricted-environment fallback (`authbridge-proxy --local`), and + manual-removal instructions. Good source of ground truth for expected behavior. +- `authbridge/cmd/abctl/cmd_service_*_test.go` — several files split by concern (bootout, + durable, recovery, restricted, stamp, perm). See gap analysis for which of these actually + exercise Linux. + +## Gap analysis against #945's checklist + +Full detail came from a source-code audit (Explore agent, 25 tool calls, full read of +`cmd_service.go`, `cmd_service_platform.go`, `supervise.go`, `install.sh`, and every +`cmd_service_*_test.go` file). Condensed per checklist bullet: + +### 1. Fresh install via `curl | sh`, amd64 and arm64 +- **Exists:** OS/arch detection (`install.sh:816-828`); both Linux arches are actually + published by `release-binaries.yaml:156`; checksum verification prefers `shasum`, falls + back to `sha256sum`; `supervisor_usable()` (`install.sh:645-653`) does a *live* preflight + (`systemctl --user show-environment`, not just `command -v systemctl`) — this is what + catches containers/minimal images where the binary exists but no user session does. +- **Gap:** No CI job or test actually installs and runs the real released binaries against a + live Linux box, for either architecture. `install_test.sh` only unit-tests shell functions + with system commands stubbed out. There is no arm64 CI runner at all — arm64 is only + exercised at *build* time, never at *install/run* time. The issue text says this is + "automated by the Linux release smoke test" — **no such smoke-test workflow exists yet** + (this is exactly #957's job, and #957 depends on #945 being solid first). + +### 2. Upgrade over an older install: service restarted, config preserved +- **Exists:** `serviceInstall` idempotency via `installCanSkip`/`serviceIsCurrent` + (`cmd_service.go:774-836`); config migration (`migrateConfig`, `cmd_service.go:343`) before + restart; binary-swap detection via SHA-256 stamp file + (`proxyBinaryUnchanged`, `cmd_service_platform.go:806-824`, platform-agnostic and well + tested). Linux restart is `systemctl --user daemon-reload` then `enable --now` + (`cmd_service_platform.go:216-247`) — no bootout-race workaround needed, per explicit + comment (`cmd_service_platform.go:92-93`) that systemd's own `Restart=`/`StartLimit*` + make the launchd EIO workaround (#880) unnecessary on Linux. +- **Gap:** Nothing drives an actual upgrade against a real running `systemd --user` unit to + confirm the restart genuinely leaves Cortex serving afterward. macOS has a dedicated + real-launchd integration test for its equivalent scenario + (`TestWaitBootedOut_RealLaunchd`); there is no Linux analog. + +### 3. Uninstall leaves no unit file, no `~/.cortex`, no modified agent settings +- **Exists:** `serviceUninstall` (`cmd_service.go:503-538`) removes the unit + stamp file, + deliberately leaves `~/.cortex` alone (that's a separate, manual step per + `laptop-service.md`). Linux `unloadService` (`cmd_service_platform.go:269-291`) tracks + whether *it* enabled lingering (via a marker file) so uninstall doesn't clobber a linger + setting the user had already set for unrelated units — a thoughtful, Linux-specific detail. +- **Gap:** None of this Linux uninstall logic (the linger-marker branch, + `systemctl --user disable --now`) has a test with faked `systemctl`/`loginctl` — the + existing `TestStopIsDurable` only greps source for expected strings, it doesn't run the + code path. "No modified agent settings" is really an `abctl claude-code disable` concern, + outside the files audited here — worth confirming separately. + +### 4. Re-running install is idempotent +- **Exists:** `installCanSkip` (well tested, platform-agnostic); `install.sh`'s + already-at-this-version guard (`install.sh:885-889`); `lingerEnabled()` + (`cmd_service_platform.go:259-267`) avoids re-toggling linger on repeat installs. +- **Gap:** `lingerEnabled`'s parsing of `loginctl show-user --property=Linger` output has + zero test coverage — a parsing regression could silently re-enable linger every install, + or silently skip enabling it when needed. + +### 5. Service survives reboot and a crash — **the biggest gap** +- **Exists (and well-reasoned):** unit rendering (`renderUnitFor("linux", ...)`, + `cmd_service_platform.go:131-148`) sets `Restart=on-failure`, `RestartSec=10`, + `StartLimitIntervalSec=300`, `StartLimitBurst=5` — correctly placed in `[Unit]`, not + `[Service]` (a comment explains systemd v229+ ignores/deprecates it in `[Service]`, which + would silently void the throttle). `[Install] WantedBy=default.target` handles + reboot/login persistence. Good unit-level string-shape test coverage + (`TestRenderUnit_BothPlatforms`, `cmd_service_test.go:65-106`). +- **Gap:** No test — unit, integration, or even a documented manual-verification note — + proves that a real `systemd --user` instance actually restarts the unit after `kill -9`, + or survives a real reboot/logout with lingering enabled. The macOS side has exactly this + kind of verification on record: comments at `cmd_service_platform.go:44-58` document that + `KeepAlive` was tested, found **not** to work end-to-end, and that finding drove the + supervisor-process redesign. **No equivalent verification episode exists for Linux's + `Restart=on-failure` assumption** — it's currently trusted, not proven. This is precisely + the class of gap #945 (and its manual-reboot sibling, #964) exists to close. + +### 6. `abctl service status | start | stop | restart` accurate in every state — **CLOSED (Tier 2) 2026-09-17** +- **Exists:** `serviceStatus`/`serviceControl` (`cmd_service.go:540-665`) are + platform-agnostic; Linux `controlService` maps stop→`disable --now` (persistent), + start→`enable --now`, restart→plain `systemctl --user restart` (transient, by design). + `supervisorRunning`'s Linux branch uses `systemctl --user is-active cortex.service`, with a + sane fallback ("no systemctl → treat as unknown, don't block") for systemd-less boxes. +- **Fixed:** `loadService`, `controlService`, `supervisorRunning`, and `unloadService` + took `runtime.GOOS` directly (unlike `renderUnitFor`, which already took `goos` as an + explicit parameter for exactly this reason). Refactored all four to take `goos string` + explicitly, updated every call site in `cmd_service.go` to pass `runtime.GOOS`, and added + `cmd_service_systemd_test.go` — a `fakeSystemctl`/`fakeLoginctl` harness mirroring + `fakeLaunchctl`, plus tests that actually run (not skip) on any host by passing `"linux"` + explicitly. Confirmed by running for real (not just reading the code) that `is-active` + already correctly treats `activating`/`failed`/`deactivating`/`inactive` as "not + running" — only `active` (exit 0) reads as running. That logic was already correct; it + was just unverified. Also covers: `loadService`'s daemon-reload/enable failure messages, + the linger-enable/marker-write logic (skip when already on, write marker only when *we* + turn it on, caveat-not-fatal when `enable-linger` fails), `unloadService`'s + marker-gated `disable-linger` (never called without a marker), and `controlService`'s + exact verb-per-action mapping. 22 subtests, all passing. +- **Still open:** none of this proves the *real* `systemctl`/`loginctl` actually behave + this way — only that our own code reacts correctly to inputs we scripted. That's Tier 3 + (real systemd integration test), still not built. + +### 7. `stop` tolerates the proxy's ~15s drain — **CLOSED 2026-09-17** +- **Exists:** the proxy's own 15s shutdown timeout (`main.go:671`) is the anchor value + everything else has to respect. macOS handles this *explicitly* in Go + (`serviceBootoutTimeout = 30*time.Second`, `cmd_service.go:39-42`, plus the supervisor's + own 20s-before-SIGKILL logic in `supervise.go:88-95`, deliberately longer than 15s). +- **Fixed:** added an explicit `TimeoutStopSec=20` to `renderUnitFor("linux", ...)` + (`cmd_service_platform.go`), matching the macOS supervisor's 20s headroom over the + proxy's 15s drain, with a rationale comment in the same style as the surrounding + `StartLimit*`/`network-online.target` comments. `TestRenderUnit_BothPlatforms`'s + `"linux restarts on failure only"` subtest now asserts the line is present, so a future + regression that drops it fails CI instead of silently reverting to systemd's undocumented + default. Previously it "worked" only by accident of systemd's 90s default exceeding 15s — + now it's an explicit, tested value. + +### 8. Works under user systemd, and states what happens where systemd is absent +- **Exists (this is the best-handled bullet):** `loadService` gives a clear, + actionable message when `systemctl` isn't found at all, naming WSL1/containers as likely + causes (`cmd_service_platform.go:216-219`). `install.sh`'s `supervisor_usable()` does the + more realistic check — `systemctl` present but user manager/D-Bus unreachable — and falls + back to `start_unsupervised` (a plain backgrounded process using the proxy's own + `--supervise` loop), printing manual start/stop/status/logs instructions. Well tested at + the shell-function level (`install_test.sh:570-575`). +- **Gap:** On the Go side, there's no Linux equivalent of `launchdUsable()` + (`cmd_service_platform.go:680-696`, explicitly darwin-only) — the function that lets + macOS refuse cleanly *before writing anything to disk* with a dedicated, tested exit code + (`exitNoSupervisor`, exit 4). On Linux, "no systemd" is discovered later, inside + `loadService`, after the unit file has already been written (then cleaned up on failure). + End behavior is probably fine — `install.sh`'s generic fallback classifier still buckets + it correctly — but it's fine by accident/genericness, not by an intentional, tested, + Linux-aware preflight the way darwin gets. No Go test asserts the exact message or exit + code for "systemctl absent" the way `TestServiceInstall_RestrictedEnvironment` does for + darwin. + +## Cross-cutting themes + +1. **The systemd rendering/string-shape logic is solid and well tested.** The gap is almost + entirely at the "does this actually work against a real system service manager" layer — + nothing fakes or drives real `systemctl`/`loginctl` for Linux, while macOS has both a + `fakeLaunchctl` unit-test harness *and* a real-launchd integration test. +2. **The Linux path is architecturally simpler** (no supervisor process, no bootout-race + workaround) for a legitimate reason — but that simplicity has never been backed by the + same real-world verification that justified and shaped the macOS design. +3. **One concrete, low-risk fix stands out:** add `TimeoutStopSec=` to the Linux unit. Small, + self-contained, directly addresses checklist bullet 7. +4. **No real Linux CI smoke test exists yet anywhere in the repo** — this is #957's job, but + #957 can't be trusted until the gaps above are closed, since a smoke test built on top of + an unverified `Restart=on-failure` assumption would just as confidently pass. + +## Suggested next steps (not yet sequenced into a task plan) + +1. ~~Add an explicit `TimeoutStopSec=` to `renderUnitFor("linux", ...)`~~ — **done + 2026-09-17**, see bullet 7 above. +2. ~~Build a `fakeSystemctl`/`fakeLoginctl` test harness~~ — **done 2026-09-17**, see + bullet 6 above. Required refactoring `loadService`/`controlService`/`supervisorRunning`/ + `unloadService` to take `goos` explicitly first (same fix `renderUnitFor` already had) — + otherwise these functions can't be exercised from a non-Linux host at all. +3. Get real verification (manual or in a systemd-capable container/VM with a lingering + `systemd --user` session) that `Restart=on-failure` actually recovers the unit after + `kill -9`, and that lingering actually survives a logout/reboot-equivalent — document the + result the way the macOS KeepAlive finding is documented in code comments. +4. Decide whether Linux needs its own `launchdUsable()`-equivalent preflight (tested, named + exit code) or whether the current "discover it inside `loadService`, clean up, fall + through to the generic fallback" behavior is an acceptable, intentional asymmetry. +5. Once 1–4 land, build the real end-to-end harness (likely `systemd-run --user` in a + cgroup-v2-capable container, or a self-hosted/nspawn runner) that #957 will build its CI + job on top of. + +## Relationship to other issues (for context, not in scope here) + +- **#944** — macOS mirror of this issue. Owned by @huang195. Already has real end-to-end + verification behind it (the KeepAlive finding, bootout-race fix #880) that Linux still + lacks — worth mining `cmd_service_platform.go`'s darwin comments and tests as a template + for what "verified" should look like on Linux. +- **#955** — unattended agent workloads / headless capture path, owned by @esnible. Both + #956 and #957 need it for the "non-zero token count" smoke-test assertion. Not required + for #945 itself. +- **#964** — manual reboot verification (macOS + Linux), explicitly deferred from #944/#945 + because CI runners don't reboot. Shares the exact "does Restart=on-failure/launchd + KeepAlive really survive a restart" question raised above — worth coordinating rather than + duplicating. +- **#966** — plugin build-tag convention cleanup (`exclude_plugin_*` → `include_plugin_*`) + and a smaller desktop artifact. Touches the same install/upgrade path (H2 in that issue: + upgrading to a build that dropped a plugin an existing config still names must be handled + gracefully) — flagged there as something #944/#945/#964 must cover. Worth checking in on + this once the plugin-set change lands, since it could break an assumption made above about + upgrade always being config-preserving. From 8344db61516a9ab195f5ce442fe8dc142295a923 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Mon, 21 Sep 2026 09:35:13 -0400 Subject: [PATCH 02/12] docs: Exempt the planning doc from the retired-plugin-tag guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same fix as PR #1076: TestNoExcludePluginTagsRemain (authbridge/scripts/profile-tags) fails on this branch's own earlier copy of the doc for the same reason — its reference to the retired exclude_plugin_* tag form, as design history for #966, trips a guard meant for live code. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../superpowers/plans/2026-09-16-linux-install-systemd-945.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md index 684c8d814..ac52528ed 100644 --- a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md +++ b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md @@ -273,7 +273,9 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re because CI runners don't reboot. Shares the exact "does Restart=on-failure/launchd KeepAlive really survive a restart" question raised above — worth coordinating rather than duplicating. -- **#966** — plugin build-tag convention cleanup (`exclude_plugin_*` → `include_plugin_*`) +- **#966** — plugin build-tag convention cleanup (retired `exclude_plugin_*` form → + `include_plugin_*`; allow-legacy-plugin-tag: this reference is design history, not a + live usage) and a smaller desktop artifact. Touches the same install/upgrade path (H2 in that issue: upgrading to a build that dropped a plugin an existing config still names must be handled gracefully) — flagged there as something #944/#945/#964 must cover. Worth checking in on From d509018f19fc41d37b681a17a67d0a0cdf2a4e97 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Mon, 21 Sep 2026 13:34:06 -0400 Subject: [PATCH 03/12] fix: Make fake systemctl/loginctl argument matching order-independent Two CodeRabbit findings on #1080: - The daemon-reload/enable and show-user fakes matched positionally ($2/$1). If the real call sites ever reorder arguments (e.g. a new flag inserted before daemon-reload), the fake would silently take the wrong branch instead of failing with a clear "shape changed" signal. Switched all five to `case "$*" in *pattern*)`, matching anywhere in the argument list regardless of position. - The is-active fakes hardcode exit 3 for every non-active state with no comment on why that value doesn't matter. Added one: supervisorRunning's Linux branch only checks err != nil and stdout content, never a specific exit code, so any nonzero value exercises the same path. All 22 subtests still pass. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../cmd/abctl/cmd_service_systemd_test.go | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_systemd_test.go b/authbridge/cmd/abctl/cmd_service_systemd_test.go index d774b17b6..831487d97 100644 --- a/authbridge/cmd/abctl/cmd_service_systemd_test.go +++ b/authbridge/cmd/abctl/cmd_service_systemd_test.go @@ -83,6 +83,9 @@ func TestSupervisorRunning_Linux(t *testing.T) { // systemctl is-active exits non-zero for every state except "active", but still // prints the real state on stdout. Only the exit-0 case may read as running. + // The fakes below all use exit 3, but the specific nonzero value is arbitrary: + // supervisorRunning's Linux branch only checks err != nil and stdout content, never + // a particular exit code, so any nonzero value here exercises the same path. for _, state := range []string{"activating", "failed", "deactivating", "inactive"} { t.Run(state+" does not read as running", func(t *testing.T) { fakeSystemctl(t, "#!/bin/sh\necho '"+state+"'\nexit 3\n") @@ -127,10 +130,11 @@ func TestLoadService_Linux(t *testing.T) { t.Run("daemon-reload failure is reported, enable is never attempted", func(t *testing.T) { p := servicePathsFixture(t) fakeSystemctl(t, `#!/bin/sh -if [ "$2" = "daemon-reload" ]; then - echo "boom: unit has a syntax error" >&2 - exit 1 -fi +case "$*" in + *daemon-reload*) + echo "boom: unit has a syntax error" >&2 + exit 1 ;; +esac echo "enable --now should not have run" >&2 exit 1 `) @@ -143,9 +147,9 @@ exit 1 t.Run("enable --now failure is reported", func(t *testing.T) { p := servicePathsFixture(t) fakeSystemctl(t, `#!/bin/sh -if [ "$2" = "daemon-reload" ]; then - exit 0 -fi +case "$*" in + *daemon-reload*) exit 0 ;; +esac echo "nope: unit not found" >&2 exit 1 `) @@ -159,10 +163,11 @@ exit 1 p := servicePathsFixture(t) fakeSystemctl(t, "#!/bin/sh\nexit 0\n") fakeLoginctl(t, `#!/bin/sh -if [ "$1" = "show-user" ]; then - echo "Linger=yes" - exit 0 -fi +case "$*" in + *show-user*) + echo "Linger=yes" + exit 0 ;; +esac echo "enable-linger should not have run" >&2 exit 1 `) @@ -178,10 +183,11 @@ exit 1 p := servicePathsFixture(t) fakeSystemctl(t, "#!/bin/sh\nexit 0\n") fakeLoginctl(t, `#!/bin/sh -if [ "$1" = "show-user" ]; then - echo "Linger=no" - exit 0 -fi +case "$*" in + *show-user*) + echo "Linger=no" + exit 0 ;; +esac exit 0 `) if err := loadService("linux", p, io.Discard); err != nil { @@ -196,10 +202,11 @@ exit 0 p := servicePathsFixture(t) fakeSystemctl(t, "#!/bin/sh\nexit 0\n") fakeLoginctl(t, `#!/bin/sh -if [ "$1" = "show-user" ]; then - echo "Linger=no" - exit 0 -fi +case "$*" in + *show-user*) + echo "Linger=no" + exit 0 ;; +esac exit 1 `) err := loadService("linux", p, io.Discard) From aa41430c0d1b50642d75671797ff4b6a0552d3fd Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 10:04:56 -0400 Subject: [PATCH 04/12] docs: Note that fakeSystemctl/fakeLoginctl's separate temp dirs are intentional CodeRabbit review on #1080: a test using both fakes ends up with two prepended PATH entries, one per helper's own temp dir. Correct, since each dir holds only its own stub binary, but worth saying so directly rather than leaving a future reader to wonder if it's an oversight. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service_systemd_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/authbridge/cmd/abctl/cmd_service_systemd_test.go b/authbridge/cmd/abctl/cmd_service_systemd_test.go index 831487d97..b2a5857ee 100644 --- a/authbridge/cmd/abctl/cmd_service_systemd_test.go +++ b/authbridge/cmd/abctl/cmd_service_systemd_test.go @@ -29,7 +29,10 @@ func fakeSystemctl(t *testing.T, body string) { t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) } -// fakeLoginctl puts a loginctl on PATH that behaves however body says. +// fakeLoginctl puts a loginctl on PATH that behaves however body says. Its own +// temp dir, separate from fakeSystemctl's: a test using both ends up with two +// prepended PATH entries, one per binary, which is fine — each dir holds only its +// own stub, so there's nothing for the two to collide over. func fakeLoginctl(t *testing.T, body string) { t.Helper() dir := t.TempDir() From 108cc57235d6d06a503f6f404b0d4ee994a0d626 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 11:27:09 -0400 Subject: [PATCH 05/12] fix: Quote the call-log path, and correct doc overclaiming on #1080 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review, plus a proactive check for the same doc issue already found on #1076: - callLog built its shell redirection with a bare %s path. If TMPDIR contains whitespace or shell metacharacters, the generated fragment is invalid and the fake fails before it ever records its arguments. Use the existing shQuote helper instead of fmt.Sprintf (which is now unused and removed from imports). - Same root cause as #1076's must-fix: this branch's copy of the plan doc claimed the TimeoutStopSec fix (bullet 7) as landed. It isn't — confirmed via `git diff main...this-branch`, which touches none of cmd_service_platform.go's TimeoutStopSec content. That fix lives on sibling PR #1079, not merged yet. Corrected the Status header, bullet 7, Cross-cutting themes 2/3, and Suggested next steps 1 to attribute it there instead of here. Bullet 6 and Cross-cutting theme 1 (the fakeSystemctl/fakeLoginctl harness) are correctly this branch's own contribution and are left as closed. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../cmd/abctl/cmd_service_systemd_test.go | 3 +- .../2026-09-16-linux-install-systemd-945.md | 32 ++++++++++++------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_systemd_test.go b/authbridge/cmd/abctl/cmd_service_systemd_test.go index b2a5857ee..3e99199c6 100644 --- a/authbridge/cmd/abctl/cmd_service_systemd_test.go +++ b/authbridge/cmd/abctl/cmd_service_systemd_test.go @@ -2,7 +2,6 @@ package main import ( "errors" - "fmt" "io" "os" "path/filepath" @@ -55,7 +54,7 @@ func noSystemctlOnPath(t *testing.T) { func callLog(t *testing.T) (path string, appendLine string) { t.Helper() path = filepath.Join(t.TempDir(), "calls.log") - return path, fmt.Sprintf(`echo "$@" >> %s`, path) + return path, `echo "$@" >> ` + shQuote(path) } func readCallLog(t *testing.T, path string) []string { diff --git a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md index ac52528ed..42a3dde7d 100644 --- a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md +++ b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md @@ -1,9 +1,13 @@ # Issue #945 — Verified Linux install and systemd service lifecycle -**Status:** research + gap analysis complete. Landed 2026-09-17: the `TimeoutStopSec` fix -(bullet 7) and the Tier 2 `fakeSystemctl`/`fakeLoginctl` test harness (bullet 6), which -required refactoring four functions to take `goos` explicitly (see "Cross-cutting themes"). -Tiers 3-5 (real-systemd integration test, install.sh smoke test, reboot check) not started. +**Status:** research + gap analysis complete. **This branch (PR #1080) delivers only the +Tier 2 `fakeSystemctl`/`fakeLoginctl` test harness (bullet 6)**, which required refactoring +four functions to take `goos` explicitly (see "Cross-cutting themes") — that refactor and +harness are this branch's own diff against `main`. The `TimeoutStopSec` fix (bullet 7) is +**on a separate, not-yet-merged sibling PR (#1079)**, not part of this branch; this doc's +copy here should not be read as claiming it. Tier 3 (real-systemd integration test, #1076) +is also a separate sibling PR. Tier 4 (install.sh smoke test, #957) and Tier 5 (reboot +check, #964) are separate issues, not started. **Owner:** Alan Cha (per epic #962 owner table: "Linux install, release smoke tests, reboot verification, CI"). **Repo:** rossoctl/cortex. This doc lives in the same directory as other planning docs @@ -193,7 +197,7 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re this way — only that our own code reacts correctly to inputs we scripted. That's Tier 3 (real systemd integration test), still not built. -### 7. `stop` tolerates the proxy's ~15s drain — **CLOSED 2026-09-17** +### 7. `stop` tolerates the proxy's ~15s drain — **CLOSED on sibling PR #1079, not part of this branch** - **Exists:** the proxy's own 15s shutdown timeout (`main.go:671`) is the anchor value everything else has to respect. macOS handles this *explicitly* in Go (`serviceBootoutTimeout = 30*time.Second`, `cmd_service.go:39-42`, plus the supervisor's @@ -228,23 +232,27 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re ## Cross-cutting themes -1. **The systemd rendering/string-shape logic is solid and well tested.** The gap is almost +1. ~~The systemd rendering/string-shape logic is solid and well tested. The gap is almost entirely at the "does this actually work against a real system service manager" layer — - nothing fakes or drives real `systemctl`/`loginctl` for Linux, while macOS has both a - `fakeLaunchctl` unit-test harness *and* a real-launchd integration test. + nothing fakes or drives real `systemctl`/`loginctl` for Linux~~ — **this branch closes + the fake-driving half**: `cmd_service_systemd_test.go` now mirrors macOS's + `fakeLaunchctl` harness. The real-launchd-equivalent (a real-systemd integration test) + is a separate sibling PR (#1076), not part of this branch. 2. **The Linux path is architecturally simpler** (no supervisor process, no bootout-race workaround) for a legitimate reason — but that simplicity has never been backed by the - same real-world verification that justified and shaped the macOS design. + same real-world verification that justified and shaped the macOS design. (Now addressed, + but on sibling PR #1076, not this branch.) 3. **One concrete, low-risk fix stands out:** add `TimeoutStopSec=` to the Linux unit. Small, - self-contained, directly addresses checklist bullet 7. + self-contained, directly addresses checklist bullet 7. **Done on sibling PR #1079**, not + part of this branch. 4. **No real Linux CI smoke test exists yet anywhere in the repo** — this is #957's job, but #957 can't be trusted until the gaps above are closed, since a smoke test built on top of an unverified `Restart=on-failure` assumption would just as confidently pass. ## Suggested next steps (not yet sequenced into a task plan) -1. ~~Add an explicit `TimeoutStopSec=` to `renderUnitFor("linux", ...)`~~ — **done - 2026-09-17**, see bullet 7 above. +1. Add an explicit `TimeoutStopSec=` to `renderUnitFor("linux", ...)` — **done on sibling + PR #1079**, not part of this branch; see bullet 7 above. 2. ~~Build a `fakeSystemctl`/`fakeLoginctl` test harness~~ — **done 2026-09-17**, see bullet 6 above. Required refactoring `loadService`/`controlService`/`supervisorRunning`/ `unloadService` to take `goos` explicitly first (same fix `renderUnitFor` already had) — From bc1fc3a23d40d7b418c046cb38e0296288e5b92c Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 11:41:22 -0400 Subject: [PATCH 06/12] fix: Close remaining doc-attribution gaps and two real test-rigor gaps on #1080 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second round of CodeRabbit findings on #1080, all verified before fixing: - must-fix (x2): bullet 6's header still read "CLOSED (Tier 2) 2026-09-17" even though this PR (#1080) is itself unmerged — a doc announcing its own merge before it happens. Bullet 7's header was already corrected last round, but its body still described TimeoutStopSec as an accomplished, tested fact, which isn't true on this branch. Reworded bullet 6 as "Closing in #1080 (this PR, unmerged)" — true both before and after merge — and bullet 7's body as "In review (#1079)", matching the reviewer's suggested framing exactly. - readCallLog returned nil on ANY read error, not just "file doesn't exist" — so a broken harness (PATH not applied, /bin/sh missing, temp dir gone) would read identically to "genuinely never called", making every negative call-count assertion in this file vacuous. Now distinguishes os.ErrNotExist (real "never called") from any other read error (t.Fatalf, so a harness break is loud). - The linger happy-path tests asserted only the marker file's presence, never the actual loginctl invocation shape. A typo'd verb, the wrong uid, or a bare enable-linger with no argument would each still exit 0 and still write the marker — exactly the wrong-verb bug class this whole harness exists to catch, just not caught here. Added callLog- backed checks on both subtests, plus a new test confirming loadService's happy path calls daemon-reload then enable --now, in that order, with the exact unit name — previously only inferred through failure-path error messages, never checked on a run that succeeds. - Collapsed controlService(goos string, action string, ...) to controlService(goos, action string, ...) per a minor nit; declined the deeper `type goos string` swap-proofing the same comment raised, as it flagged itself as likely out of scope for this PR. 24 subtests now (was 22), all passing. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service_platform.go | 2 +- .../cmd/abctl/cmd_service_systemd_test.go | 48 ++++++++++++++++++- .../2026-09-16-linux-install-systemd-945.md | 18 ++++--- 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_platform.go b/authbridge/cmd/abctl/cmd_service_platform.go index 02e399a61..9767d599a 100644 --- a/authbridge/cmd/abctl/cmd_service_platform.go +++ b/authbridge/cmd/abctl/cmd_service_platform.go @@ -380,7 +380,7 @@ func dialableAddr(addr string) string { } // controlService maps stop/start/restart onto the platform's supervisor. -func controlService(goos string, action string, p servicePaths, progress io.Writer) error { +func controlService(goos, action string, p servicePaths, progress io.Writer) error { if goos == "darwin" { target := "gui/" + strconv.Itoa(os.Getuid()) + "/" + launchdLabel switch action { diff --git a/authbridge/cmd/abctl/cmd_service_systemd_test.go b/authbridge/cmd/abctl/cmd_service_systemd_test.go index 3e99199c6..90f90aa44 100644 --- a/authbridge/cmd/abctl/cmd_service_systemd_test.go +++ b/authbridge/cmd/abctl/cmd_service_systemd_test.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "strconv" "strings" "testing" ) @@ -60,8 +61,11 @@ func callLog(t *testing.T) (path string, appendLine string) { func readCallLog(t *testing.T, path string) []string { t.Helper() b, err := os.ReadFile(path) //nolint:gosec + if errors.Is(err, os.ErrNotExist) { + return nil // genuinely never called + } if err != nil { - return nil + t.Fatalf("reading the call log: %v", err) } var lines []string for _, l := range strings.Split(strings.TrimRight(string(b), "\n"), "\n") { @@ -120,6 +124,26 @@ func TestSupervisorRunning_Linux(t *testing.T) { } func TestLoadService_Linux(t *testing.T) { + // The daemon-reload/enable-now failure subtests below only prove loadService + // reports the right failure when systemctl fails — they say nothing about what + // gets invoked, in what order, on a run that succeeds. This closes that: the two + // calls must happen in order, with --user and the exact unit name, not just "some + // two calls that happened to both exit 0". + t.Run("daemon-reload then enable --now, in that order, with the exact unit", func(t *testing.T) { + p := servicePathsFixture(t) + logPath, logLine := callLog(t) + fakeSystemctl(t, "#!/bin/sh\n"+logLine+"\nexit 0\n") + fakeLoginctl(t, "#!/bin/sh\necho 'Linger=yes'\nexit 0\n") // skip the linger branch; not under test here + if err := loadService("linux", p, io.Discard); err != nil { + t.Fatalf("unexpected error: %v", err) + } + calls := readCallLog(t, logPath) + want := []string{"--user daemon-reload", "--user enable --now cortex.service"} + if len(calls) != len(want) || calls[0] != want[0] || calls[1] != want[1] { + t.Errorf("systemctl calls = %v, want %v in that order", calls, want) + } + }) + t.Run("no systemctl on PATH", func(t *testing.T) { p := servicePathsFixture(t) noSystemctlOnPath(t) @@ -164,13 +188,14 @@ exit 1 t.Run("linger already enabled: skipped, no marker written", func(t *testing.T) { p := servicePathsFixture(t) fakeSystemctl(t, "#!/bin/sh\nexit 0\n") + logPath, logLine := callLog(t) fakeLoginctl(t, `#!/bin/sh +`+logLine+` case "$*" in *show-user*) echo "Linger=yes" exit 0 ;; esac -echo "enable-linger should not have run" >&2 exit 1 `) if err := loadService("linux", p, io.Discard); err != nil { @@ -179,12 +204,22 @@ exit 1 if _, err := os.Stat(lingerMarker(p)); err == nil { t.Error("marker written even though linger was already on") } + // Asserting the marker's absence proves loadService took the skip branch, but + // not that it got there by actually calling show-user — a fake that always + // skipped enable-linger regardless of input would pass the same way. Checking + // the log closes that: show-user must have run, and enable-linger must not. + calls := readCallLog(t, logPath) + if len(calls) != 1 || !strings.Contains(calls[0], "show-user") { + t.Errorf("loginctl calls = %v, want exactly one show-user", calls) + } }) t.Run("linger not enabled, enable-linger succeeds: marker written", func(t *testing.T) { p := servicePathsFixture(t) fakeSystemctl(t, "#!/bin/sh\nexit 0\n") + logPath, logLine := callLog(t) fakeLoginctl(t, `#!/bin/sh +`+logLine+` case "$*" in *show-user*) echo "Linger=no" @@ -198,6 +233,15 @@ exit 0 if _, err := os.Stat(lingerMarker(p)); err != nil { t.Error("no marker written after abctl enabled linger itself") } + // The marker alone doesn't prove the right command ran — a typo'd verb, the + // wrong uid, or a bare `enable-linger` with no argument would each still exit 0 + // here and still write the marker. Assert the actual second call's shape: + // enable-linger, with this process's own uid, and nothing else. + calls := readCallLog(t, logPath) + wantEnable := "enable-linger " + strconv.Itoa(os.Getuid()) + if len(calls) != 2 || !strings.Contains(calls[0], "show-user") || calls[1] != wantEnable { + t.Errorf("loginctl calls = %v, want [show-user ..., %q]", calls, wantEnable) + } }) t.Run("linger not enabled, enable-linger fails: caveat, not fatal, no marker", func(t *testing.T) { diff --git a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md index 42a3dde7d..6e6feaae9 100644 --- a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md +++ b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md @@ -173,7 +173,7 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re `Restart=on-failure` assumption** — it's currently trusted, not proven. This is precisely the class of gap #945 (and its manual-reboot sibling, #964) exists to close. -### 6. `abctl service status | start | stop | restart` accurate in every state — **CLOSED (Tier 2) 2026-09-17** +### 6. `abctl service status | start | stop | restart` accurate in every state — **Closing in #1080 (this PR, unmerged)** - **Exists:** `serviceStatus`/`serviceControl` (`cmd_service.go:540-665`) are platform-agnostic; Linux `controlService` maps stop→`disable --now` (persistent), start→`enable --now`, restart→plain `systemctl --user restart` (transient, by design). @@ -197,19 +197,17 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re this way — only that our own code reacts correctly to inputs we scripted. That's Tier 3 (real systemd integration test), still not built. -### 7. `stop` tolerates the proxy's ~15s drain — **CLOSED on sibling PR #1079, not part of this branch** +### 7. `stop` tolerates the proxy's ~15s drain — **In review (#1079), not part of this branch** - **Exists:** the proxy's own 15s shutdown timeout (`main.go:671`) is the anchor value everything else has to respect. macOS handles this *explicitly* in Go (`serviceBootoutTimeout = 30*time.Second`, `cmd_service.go:39-42`, plus the supervisor's own 20s-before-SIGKILL logic in `supervise.go:88-95`, deliberately longer than 15s). -- **Fixed:** added an explicit `TimeoutStopSec=20` to `renderUnitFor("linux", ...)` - (`cmd_service_platform.go`), matching the macOS supervisor's 20s headroom over the - proxy's 15s drain, with a rationale comment in the same style as the surrounding - `StartLimit*`/`network-online.target` comments. `TestRenderUnit_BothPlatforms`'s - `"linux restarts on failure only"` subtest now asserts the line is present, so a future - regression that drops it fails CI instead of silently reverting to systemd's undocumented - default. Previously it "worked" only by accident of systemd's 90s default exceeding 15s — - now it's an explicit, tested value. + Today it "works" only by accident of systemd's 90s default exceeding 15s. +- **In review (#1079):** adds an explicit `TimeoutStopSec=20` to `renderUnitFor("linux", + ...)`, matching the macOS supervisor's 20s headroom over the proxy's 15s drain, with + `TestRenderUnit_BothPlatforms`'s `"linux restarts on failure only"` subtest asserting the + line is present. Neither the unit-file change nor the subtest exists on this branch — + see #1079 directly for status. ### 8. Works under user systemd, and states what happens where systemd is absent - **Exists (this is the best-handled bullet):** `loadService` gives a clear, From 23eff83a30046c8247a023c01acb6e4a4cfaaef9 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 12:00:36 -0400 Subject: [PATCH 07/12] docs: Simplify cross-branch PR attribution to just the PR number Same simplification as the sibling commit on #1076: drop the "not part of this branch" disclaimer clause everywhere it appears in this doc's copy, and just cite the PR number that addresses each item. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../2026-09-16-linux-install-systemd-945.md | 40 ++++++++----------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md index 6e6feaae9..e3db974f0 100644 --- a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md +++ b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md @@ -1,13 +1,10 @@ # Issue #945 — Verified Linux install and systemd service lifecycle -**Status:** research + gap analysis complete. **This branch (PR #1080) delivers only the -Tier 2 `fakeSystemctl`/`fakeLoginctl` test harness (bullet 6)**, which required refactoring -four functions to take `goos` explicitly (see "Cross-cutting themes") — that refactor and -harness are this branch's own diff against `main`. The `TimeoutStopSec` fix (bullet 7) is -**on a separate, not-yet-merged sibling PR (#1079)**, not part of this branch; this doc's -copy here should not be read as claiming it. Tier 3 (real-systemd integration test, #1076) -is also a separate sibling PR. Tier 4 (install.sh smoke test, #957) and Tier 5 (reboot -check, #964) are separate issues, not started. +**Status:** research + gap analysis complete. This branch (#1080) delivers the Tier 2 +`fakeSystemctl`/`fakeLoginctl` test harness (bullet 6), which required refactoring four +functions to take `goos` explicitly (see "Cross-cutting themes"). The `TimeoutStopSec` fix +(bullet 7) is #1079. Tier 3 (real-systemd integration test) is #1076. Tier 4 (install.sh +smoke test, #957) and Tier 5 (reboot check, #964) are separate issues, not started. **Owner:** Alan Cha (per epic #962 owner table: "Linux install, release smoke tests, reboot verification, CI"). **Repo:** rossoctl/cortex. This doc lives in the same directory as other planning docs @@ -197,17 +194,16 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re this way — only that our own code reacts correctly to inputs we scripted. That's Tier 3 (real systemd integration test), still not built. -### 7. `stop` tolerates the proxy's ~15s drain — **In review (#1079), not part of this branch** +### 7. `stop` tolerates the proxy's ~15s drain — **#1079** - **Exists:** the proxy's own 15s shutdown timeout (`main.go:671`) is the anchor value everything else has to respect. macOS handles this *explicitly* in Go (`serviceBootoutTimeout = 30*time.Second`, `cmd_service.go:39-42`, plus the supervisor's own 20s-before-SIGKILL logic in `supervise.go:88-95`, deliberately longer than 15s). Today it "works" only by accident of systemd's 90s default exceeding 15s. -- **In review (#1079):** adds an explicit `TimeoutStopSec=20` to `renderUnitFor("linux", - ...)`, matching the macOS supervisor's 20s headroom over the proxy's 15s drain, with +- **#1079:** adds an explicit `TimeoutStopSec=20` to `renderUnitFor("linux", ...)`, + matching the macOS supervisor's 20s headroom over the proxy's 15s drain, with `TestRenderUnit_BothPlatforms`'s `"linux restarts on failure only"` subtest asserting the - line is present. Neither the unit-file change nor the subtest exists on this branch — - see #1079 directly for status. + line is present. ### 8. Works under user systemd, and states what happens where systemd is absent - **Exists (this is the best-handled bullet):** `loadService` gives a clear, @@ -232,25 +228,23 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re 1. ~~The systemd rendering/string-shape logic is solid and well tested. The gap is almost entirely at the "does this actually work against a real system service manager" layer — - nothing fakes or drives real `systemctl`/`loginctl` for Linux~~ — **this branch closes - the fake-driving half**: `cmd_service_systemd_test.go` now mirrors macOS's - `fakeLaunchctl` harness. The real-launchd-equivalent (a real-systemd integration test) - is a separate sibling PR (#1076), not part of this branch. + nothing fakes or drives real `systemctl`/`loginctl` for Linux~~ — this branch closes the + fake-driving half: `cmd_service_systemd_test.go` now mirrors macOS's `fakeLaunchctl` + harness. The real-launchd equivalent (a real-systemd integration test) is #1076. 2. **The Linux path is architecturally simpler** (no supervisor process, no bootout-race workaround) for a legitimate reason — but that simplicity has never been backed by the - same real-world verification that justified and shaped the macOS design. (Now addressed, - but on sibling PR #1076, not this branch.) + same real-world verification that justified and shaped the macOS design. (Now addressed + in #1076.) 3. **One concrete, low-risk fix stands out:** add `TimeoutStopSec=` to the Linux unit. Small, - self-contained, directly addresses checklist bullet 7. **Done on sibling PR #1079**, not - part of this branch. + self-contained, directly addresses checklist bullet 7. #1079. 4. **No real Linux CI smoke test exists yet anywhere in the repo** — this is #957's job, but #957 can't be trusted until the gaps above are closed, since a smoke test built on top of an unverified `Restart=on-failure` assumption would just as confidently pass. ## Suggested next steps (not yet sequenced into a task plan) -1. Add an explicit `TimeoutStopSec=` to `renderUnitFor("linux", ...)` — **done on sibling - PR #1079**, not part of this branch; see bullet 7 above. +1. Add an explicit `TimeoutStopSec=` to `renderUnitFor("linux", ...)` — #1079; see bullet 7 + above. 2. ~~Build a `fakeSystemctl`/`fakeLoginctl` test harness~~ — **done 2026-09-17**, see bullet 6 above. Required refactoring `loadService`/`controlService`/`supervisorRunning`/ `unloadService` to take `goos` explicitly first (same fix `renderUnitFor` already had) — From e6850b35af5d494f5a0eef7602b8589054c633d7 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 12:52:22 -0400 Subject: [PATCH 08/12] fix: Close the vacuous-pass hole for real, and three stale doc claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round of CodeRabbit findings on #1080, verified before fixing: - must-fix: my prior fix (readCallLog distinguishing ErrNotExist from other errors) only closed one of two ways a broken harness reads as "correctly never called." Reproduced the surviving one myself first: flip a stub's permissions from 0o700 to 0o600 (present, not executable) and "no_marker: loginctl is never called" PASSES while "marker_present: disable-linger runs" FAILS — same green/red contradiction, reached a different way. A non-executable (or otherwise unreachable) stub means the code under test's own exec.LookPath/exec.Command fails before touching the stub, so nothing is ever appended to the log — indistinguishable from a genuine zero-calls outcome. Moved the fix into the shared installer instead of the assertion layer: fakeSystemctl and fakeLoginctl now both go through a new installStub helper that immediately confirms, via exec.LookPath, that the binary it just wrote is actually reachable — failing loudly once, for every subtest that uses either helper, rather than leaving each call-log assertion to rediscover the gap independently. Reproduced the fix working in both directions: the same mutation now fails loudly across the board, and all 27 subtests still pass unmutated. - Stale count: doc said "22 subtests, all passing" (already stale before this round — the three table-driven loops contribute more subtests than a flat count implies); actual count verified by running with -v is 27. Corrected. - Suggested next steps item 2 still carried a fabricated "done 2026-09-17" date (this branch's first commit is 2026-09-21) and contradicted bullet 6's already-correct "unmerged" framing just above it. Dropped the date, kept the PR number, matching the reframing already applied elsewhere in the file. - Cross-cutting theme 2 and bullet 6's "Still open" note both still read as if Tier 3 (the real-systemd integration test) were unbuilt or vaguely "addressed" rather than citing where — #1076 — matching the terse citation style used elsewhere in the doc. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../cmd/abctl/cmd_service_systemd_test.go | 32 +++++++++++++++---- .../2026-09-16-linux-install-systemd-945.md | 11 +++---- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_systemd_test.go b/authbridge/cmd/abctl/cmd_service_systemd_test.go index 90f90aa44..40d3055f8 100644 --- a/authbridge/cmd/abctl/cmd_service_systemd_test.go +++ b/authbridge/cmd/abctl/cmd_service_systemd_test.go @@ -4,6 +4,7 @@ import ( "errors" "io" "os" + "os/exec" "path/filepath" "strconv" "strings" @@ -22,11 +23,7 @@ import ( // including the one these were written on. func fakeSystemctl(t *testing.T, body string) { t.Helper() - dir := t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "systemctl"), []byte(body), 0o700); err != nil { //nolint:gosec - t.Fatal(err) - } - t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + installStub(t, "systemctl", body) } // fakeLoginctl puts a loginctl on PATH that behaves however body says. Its own @@ -34,12 +31,32 @@ func fakeSystemctl(t *testing.T, body string) { // prepended PATH entries, one per binary, which is fine — each dir holds only its // own stub, so there's nothing for the two to collide over. func fakeLoginctl(t *testing.T, body string) { + t.Helper() + installStub(t, "loginctl", body) +} + +// installStub writes body as an executable named "name" into its own temp dir and +// prepends that dir to PATH, then immediately confirms name actually resolves to +// it. That confirmation matters on its own, not just as a sanity check: a stub +// that silently isn't reachable (PATH not applied yet in some odd ordering, or — +// caught by mutating this file's own permission bits to prove it — written +// non-executable) makes the code under test's own exec.LookPath/exec.Command fail +// before ever touching the stub, so nothing gets appended to any call log. A test +// that only checks "the call log has zero entries" cannot tell that apart from a +// real, correct zero-calls outcome — both read as an empty log. Failing loudly +// here, once, closes that for every subtest that uses these helpers, rather than +// leaving each assertion to rediscover it independently. +func installStub(t *testing.T, name, body string) { t.Helper() dir := t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "loginctl"), []byte(body), 0o700); err != nil { //nolint:gosec + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(body), 0o700); err != nil { //nolint:gosec t.Fatal(err) } t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + if got, err := exec.LookPath(name); err != nil || got != path { + t.Fatalf("stub not reachable: LookPath(%q) = %q, %v; want %q", name, got, err, path) + } } // noSystemctlOnPath points PATH at an empty directory, so exec.LookPath("systemctl") @@ -55,6 +72,9 @@ func noSystemctlOnPath(t *testing.T) { func callLog(t *testing.T) (path string, appendLine string) { t.Helper() path = filepath.Join(t.TempDir(), "calls.log") + // shQuote's own doc comment describes it as quoting a systemd ExecStart argument + // specifically, but the escaping rule is plain POSIX sh, the same for any word in + // any shell command — including this redirect target, not a unit file at all. return path, `echo "$@" >> ` + shQuote(path) } diff --git a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md index e3db974f0..c2b2e30b2 100644 --- a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md +++ b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md @@ -189,10 +189,10 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re the linger-enable/marker-write logic (skip when already on, write marker only when *we* turn it on, caveat-not-fatal when `enable-linger` fails), `unloadService`'s marker-gated `disable-linger` (never called without a marker), and `controlService`'s - exact verb-per-action mapping. 22 subtests, all passing. + exact verb-per-action mapping. 27 subtests, all passing. - **Still open:** none of this proves the *real* `systemctl`/`loginctl` actually behave this way — only that our own code reacts correctly to inputs we scripted. That's Tier 3 - (real systemd integration test), still not built. + (real systemd integration test), #1076 — bullet 5 above. ### 7. `stop` tolerates the proxy's ~15s drain — **#1079** - **Exists:** the proxy's own 15s shutdown timeout (`main.go:671`) is the anchor value @@ -233,8 +233,7 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re harness. The real-launchd equivalent (a real-systemd integration test) is #1076. 2. **The Linux path is architecturally simpler** (no supervisor process, no bootout-race workaround) for a legitimate reason — but that simplicity has never been backed by the - same real-world verification that justified and shaped the macOS design. (Now addressed - in #1076.) + same real-world verification that justified and shaped the macOS design. #1076. 3. **One concrete, low-risk fix stands out:** add `TimeoutStopSec=` to the Linux unit. Small, self-contained, directly addresses checklist bullet 7. #1079. 4. **No real Linux CI smoke test exists yet anywhere in the repo** — this is #957's job, but @@ -245,8 +244,8 @@ Full detail came from a source-code audit (Explore agent, 25 tool calls, full re 1. Add an explicit `TimeoutStopSec=` to `renderUnitFor("linux", ...)` — #1079; see bullet 7 above. -2. ~~Build a `fakeSystemctl`/`fakeLoginctl` test harness~~ — **done 2026-09-17**, see - bullet 6 above. Required refactoring `loadService`/`controlService`/`supervisorRunning`/ +2. ~~Build a `fakeSystemctl`/`fakeLoginctl` test harness~~ — #1080, see bullet 6 above. + Required refactoring `loadService`/`controlService`/`supervisorRunning`/ `unloadService` to take `goos` explicitly first (same fix `renderUnitFor` already had) — otherwise these functions can't be exercised from a non-Linux host at all. 3. Get real verification (manual or in a systemd-capable container/VM with a lingering From e9c3fd067dd7cf6bd0dc4fe86224ade1ed8d2d2e Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 14:12:34 -0400 Subject: [PATCH 09/12] docs: Restructure the plan doc to match this repo's own convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same restructuring as the sibling commit on #1076, and same reason: this repo's other planning docs track status via task checkboxes scoped to one document/one branch/one PR, and reference the parent issue number rather than tracking sibling PRs' merge state inline. Multi-PR work gets separate documents, not one shared doc trying to describe several — which is exactly what kept going stale here. - specs/2026-09-16-linux-systemd-lifecycle-design.md: identical copy of the frozen research doc added on #1076 — safe to duplicate because it makes no claims about which PR fixes what, so it can't drift the way the old doc did. - plans/2026-09-21-systemd-fake-harness.md: a small, self-contained task list for this branch's own work only (the goos refactor + the fakeSystemctl/fakeLoginctl harness), no reference to #1079's or #1076's status. Deletes the old combined doc. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../2026-09-16-linux-install-systemd-945.md | 282 ------------------ .../plans/2026-09-21-systemd-fake-harness.md | 61 ++++ ...26-09-16-linux-systemd-lifecycle-design.md | 161 ++++++++++ 3 files changed, 222 insertions(+), 282 deletions(-) delete mode 100644 authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md create mode 100644 authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md create mode 100644 authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md diff --git a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md b/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md deleted file mode 100644 index c2b2e30b2..000000000 --- a/authbridge/docs/superpowers/plans/2026-09-16-linux-install-systemd-945.md +++ /dev/null @@ -1,282 +0,0 @@ -# Issue #945 — Verified Linux install and systemd service lifecycle - -**Status:** research + gap analysis complete. This branch (#1080) delivers the Tier 2 -`fakeSystemctl`/`fakeLoginctl` test harness (bullet 6), which required refactoring four -functions to take `goos` explicitly (see "Cross-cutting themes"). The `TimeoutStopSec` fix -(bullet 7) is #1079. Tier 3 (real-systemd integration test) is #1076. Tier 4 (install.sh -smoke test, #957) and Tier 5 (reboot check, #964) are separate issues, not started. -**Owner:** Alan Cha (per epic #962 owner table: "Linux install, release smoke tests, reboot -verification, CI"). -**Repo:** rossoctl/cortex. This doc lives in the same directory as other planning docs -(`authbridge/docs/superpowers/plans/`) for consistency, but is a research/gap-analysis -reference, not yet an execution-ready task plan. - -## Why this doc exists - -Assigned three issues (#945, #956, #957) under epic #962 ("Cortex v0.9.0 — stable local tool -for coding agents", due 2026-09-30). This doc captures the research done to plan #945 -specifically, so the findings don't have to be re-derived later. #956/#957 have their own -dependencies (see "Relationship to other issues" below) and are intentionally out of scope -here. - -## The three assigned issues and priority order - -| # | Title | Why this order | -|---|---|---| -| **#945** | Verified Linux install and systemd service lifecycle | **Do first.** Self-contained, no dependency on other owners' unfinished work. #957 automates verifying this. | -| **#957** | Release smoke test on Linux CI runners | **Second.** Automates #945; same owner, same platform, lowest marginal complexity once #945's gaps are closed. Partial dependency on #955 (unattended capture path, owned by @esnible) for one checklist item (asserting a non-zero token count). | -| **#956** | Release smoke test on macOS CI runners | **Last.** Depends on #944 (macOS install, owned by @huang195) and #955 — two external blockers outside this owner's control, on a platform (launchd) not owned here. | - -## Correcting an early false start: this is not greenfield - -Initial read of the issue text (and of a stale local git checkout, ~452 commits behind -`origin/main`) suggested none of this existed yet. After syncing to `origin/main`, that's -wrong: the core service-lifecycle feature already shipped in **PR #876** ("Feat: Keep Cortex -running across crashes and logins (launchd / systemd)"), with follow-on fixes: - -- **#880** — Fix: upgrades fail with launchd `EIO` (bootout race) — macOS-only issue. -- **#897** — Fix: detect environments that cannot run a service, and say what to do. -- **#931** — Fix: say when a new CA leaves running agents unobservable. -- **#911** — Fix: trust the bridge CA where it matters, stop intercepting dev tooling. - -So #945 is a **verification and gap-closing** issue on top of substantially-built -functionality, not a build-from-scratch feature. The lesson for next time: always confirm -`git fetch` / compare against `origin/main` before concluding something doesn't exist. - -## Conceptual background: what systemd and launchd are for, and why the two platforms differ - -A service manager's job: start a background process, notice when it dies (including -crashes, not just deliberate stops), and decide whether to relaunch it — without a human -watching a terminal. - -**The key asymmetry that shapes the whole design:** - -- **systemd** (Linux): a unit file's `Restart=on-failure` is honored by systemd's own - supervisor loop, reliably, even for a unit just `systemctl --user start`ed while already - logged in (exactly Cortex's `curl | sh` scenario). -- **launchd** (macOS): the team found — and documented in code comments — that launchd's - equivalent (`KeepAlive`, tried alongside `StartInterval` and `RunAtLoad`) does **not** - reliably restart a LaunchAgent added mid-session (as opposed to one present at boot when - launchd first scans `LaunchAgents`). Since Cortex is always installed mid-session, this - gap is squarely in the installer's path. - -**Consequence for the architecture:** - -- **macOS** runs **two processes**: launchd starts a small Go-written supervisor - (`authbridge-proxy --supervise`, in `authbridge/cmd/authbridge-proxy/supervise.go`) that - itself watches and restarts the actual proxy child. launchd supervises the supervisor; - the supervisor does the real crash-recovery job launchd won't reliably do. -- **Linux** runs **one process**: `authbridge-proxy` directly as the systemd unit's - `ExecStart=`, with `Restart=on-failure` doing all the crash-recovery work natively. No - supervisor binary involved. Confirmed in code: - `cmd_service_platform.go:92-93` and `TestSupervisionIsPlatformCorrect` - (`cmd_service_test.go:312-332`) explicitly assert Linux does **not** use `--supervise`. - -This means **#945 is structurally simpler than #944** on the crash-recovery axis — but it -also means the Linux "Restart=on-failure actually works" assumption has never been -put through the same real-world verification (and design correction) that produced the -macOS supervisor. That's the single biggest theme in the gap analysis below. - -## Where things live (for orientation) - -- `authbridge/install.sh` — the `curl | sh` installer (~1300 lines, POSIX `sh`). -- `authbridge/install_test.sh` — installer unit tests (shell functions in isolation, no - real network/systemctl). -- `authbridge/cmd/abctl/cmd_service.go` — cross-platform `abctl service` command logic - (install/uninstall/status/start/stop/restart), ~870 lines. -- `authbridge/cmd/abctl/cmd_service_platform.go` — the OS-specific half: systemd unit - rendering, launchd plist rendering, `systemctl`/`launchctl`/`loginctl` invocations, ~723 - lines. -- `authbridge/cmd/authbridge-proxy/supervise.go` — the macOS-only (and unsupervised-fallback) - Go restart loop, ~118 lines. -- `authbridge/cmd/authbridge-proxy/main.go` — the proxy itself; on SIGTERM/SIGINT does a - graceful shutdown with a **15-second drain** (`context.WithTimeout(..., 15*time.Second)`, - `main.go:671`) — every downstream "does stop tolerate the drain" question traces back to - this constant. -- `authbridge/docs/laptop-service.md` — user-facing doc for `abctl service *`, - `~/.cortex/` layout, restricted-environment fallback (`authbridge-proxy --local`), and - manual-removal instructions. Good source of ground truth for expected behavior. -- `authbridge/cmd/abctl/cmd_service_*_test.go` — several files split by concern (bootout, - durable, recovery, restricted, stamp, perm). See gap analysis for which of these actually - exercise Linux. - -## Gap analysis against #945's checklist - -Full detail came from a source-code audit (Explore agent, 25 tool calls, full read of -`cmd_service.go`, `cmd_service_platform.go`, `supervise.go`, `install.sh`, and every -`cmd_service_*_test.go` file). Condensed per checklist bullet: - -### 1. Fresh install via `curl | sh`, amd64 and arm64 -- **Exists:** OS/arch detection (`install.sh:816-828`); both Linux arches are actually - published by `release-binaries.yaml:156`; checksum verification prefers `shasum`, falls - back to `sha256sum`; `supervisor_usable()` (`install.sh:645-653`) does a *live* preflight - (`systemctl --user show-environment`, not just `command -v systemctl`) — this is what - catches containers/minimal images where the binary exists but no user session does. -- **Gap:** No CI job or test actually installs and runs the real released binaries against a - live Linux box, for either architecture. `install_test.sh` only unit-tests shell functions - with system commands stubbed out. There is no arm64 CI runner at all — arm64 is only - exercised at *build* time, never at *install/run* time. The issue text says this is - "automated by the Linux release smoke test" — **no such smoke-test workflow exists yet** - (this is exactly #957's job, and #957 depends on #945 being solid first). - -### 2. Upgrade over an older install: service restarted, config preserved -- **Exists:** `serviceInstall` idempotency via `installCanSkip`/`serviceIsCurrent` - (`cmd_service.go:774-836`); config migration (`migrateConfig`, `cmd_service.go:343`) before - restart; binary-swap detection via SHA-256 stamp file - (`proxyBinaryUnchanged`, `cmd_service_platform.go:806-824`, platform-agnostic and well - tested). Linux restart is `systemctl --user daemon-reload` then `enable --now` - (`cmd_service_platform.go:216-247`) — no bootout-race workaround needed, per explicit - comment (`cmd_service_platform.go:92-93`) that systemd's own `Restart=`/`StartLimit*` - make the launchd EIO workaround (#880) unnecessary on Linux. -- **Gap:** Nothing drives an actual upgrade against a real running `systemd --user` unit to - confirm the restart genuinely leaves Cortex serving afterward. macOS has a dedicated - real-launchd integration test for its equivalent scenario - (`TestWaitBootedOut_RealLaunchd`); there is no Linux analog. - -### 3. Uninstall leaves no unit file, no `~/.cortex`, no modified agent settings -- **Exists:** `serviceUninstall` (`cmd_service.go:503-538`) removes the unit + stamp file, - deliberately leaves `~/.cortex` alone (that's a separate, manual step per - `laptop-service.md`). Linux `unloadService` (`cmd_service_platform.go:269-291`) tracks - whether *it* enabled lingering (via a marker file) so uninstall doesn't clobber a linger - setting the user had already set for unrelated units — a thoughtful, Linux-specific detail. -- **Gap:** None of this Linux uninstall logic (the linger-marker branch, - `systemctl --user disable --now`) has a test with faked `systemctl`/`loginctl` — the - existing `TestStopIsDurable` only greps source for expected strings, it doesn't run the - code path. "No modified agent settings" is really an `abctl claude-code disable` concern, - outside the files audited here — worth confirming separately. - -### 4. Re-running install is idempotent -- **Exists:** `installCanSkip` (well tested, platform-agnostic); `install.sh`'s - already-at-this-version guard (`install.sh:885-889`); `lingerEnabled()` - (`cmd_service_platform.go:259-267`) avoids re-toggling linger on repeat installs. -- **Gap:** `lingerEnabled`'s parsing of `loginctl show-user --property=Linger` output has - zero test coverage — a parsing regression could silently re-enable linger every install, - or silently skip enabling it when needed. - -### 5. Service survives reboot and a crash — **the biggest gap** -- **Exists (and well-reasoned):** unit rendering (`renderUnitFor("linux", ...)`, - `cmd_service_platform.go:131-148`) sets `Restart=on-failure`, `RestartSec=10`, - `StartLimitIntervalSec=300`, `StartLimitBurst=5` — correctly placed in `[Unit]`, not - `[Service]` (a comment explains systemd v229+ ignores/deprecates it in `[Service]`, which - would silently void the throttle). `[Install] WantedBy=default.target` handles - reboot/login persistence. Good unit-level string-shape test coverage - (`TestRenderUnit_BothPlatforms`, `cmd_service_test.go:65-106`). -- **Gap:** No test — unit, integration, or even a documented manual-verification note — - proves that a real `systemd --user` instance actually restarts the unit after `kill -9`, - or survives a real reboot/logout with lingering enabled. The macOS side has exactly this - kind of verification on record: comments at `cmd_service_platform.go:44-58` document that - `KeepAlive` was tested, found **not** to work end-to-end, and that finding drove the - supervisor-process redesign. **No equivalent verification episode exists for Linux's - `Restart=on-failure` assumption** — it's currently trusted, not proven. This is precisely - the class of gap #945 (and its manual-reboot sibling, #964) exists to close. - -### 6. `abctl service status | start | stop | restart` accurate in every state — **Closing in #1080 (this PR, unmerged)** -- **Exists:** `serviceStatus`/`serviceControl` (`cmd_service.go:540-665`) are - platform-agnostic; Linux `controlService` maps stop→`disable --now` (persistent), - start→`enable --now`, restart→plain `systemctl --user restart` (transient, by design). - `supervisorRunning`'s Linux branch uses `systemctl --user is-active cortex.service`, with a - sane fallback ("no systemctl → treat as unknown, don't block") for systemd-less boxes. -- **Fixed:** `loadService`, `controlService`, `supervisorRunning`, and `unloadService` - took `runtime.GOOS` directly (unlike `renderUnitFor`, which already took `goos` as an - explicit parameter for exactly this reason). Refactored all four to take `goos string` - explicitly, updated every call site in `cmd_service.go` to pass `runtime.GOOS`, and added - `cmd_service_systemd_test.go` — a `fakeSystemctl`/`fakeLoginctl` harness mirroring - `fakeLaunchctl`, plus tests that actually run (not skip) on any host by passing `"linux"` - explicitly. Confirmed by running for real (not just reading the code) that `is-active` - already correctly treats `activating`/`failed`/`deactivating`/`inactive` as "not - running" — only `active` (exit 0) reads as running. That logic was already correct; it - was just unverified. Also covers: `loadService`'s daemon-reload/enable failure messages, - the linger-enable/marker-write logic (skip when already on, write marker only when *we* - turn it on, caveat-not-fatal when `enable-linger` fails), `unloadService`'s - marker-gated `disable-linger` (never called without a marker), and `controlService`'s - exact verb-per-action mapping. 27 subtests, all passing. -- **Still open:** none of this proves the *real* `systemctl`/`loginctl` actually behave - this way — only that our own code reacts correctly to inputs we scripted. That's Tier 3 - (real systemd integration test), #1076 — bullet 5 above. - -### 7. `stop` tolerates the proxy's ~15s drain — **#1079** -- **Exists:** the proxy's own 15s shutdown timeout (`main.go:671`) is the anchor value - everything else has to respect. macOS handles this *explicitly* in Go - (`serviceBootoutTimeout = 30*time.Second`, `cmd_service.go:39-42`, plus the supervisor's - own 20s-before-SIGKILL logic in `supervise.go:88-95`, deliberately longer than 15s). - Today it "works" only by accident of systemd's 90s default exceeding 15s. -- **#1079:** adds an explicit `TimeoutStopSec=20` to `renderUnitFor("linux", ...)`, - matching the macOS supervisor's 20s headroom over the proxy's 15s drain, with - `TestRenderUnit_BothPlatforms`'s `"linux restarts on failure only"` subtest asserting the - line is present. - -### 8. Works under user systemd, and states what happens where systemd is absent -- **Exists (this is the best-handled bullet):** `loadService` gives a clear, - actionable message when `systemctl` isn't found at all, naming WSL1/containers as likely - causes (`cmd_service_platform.go:216-219`). `install.sh`'s `supervisor_usable()` does the - more realistic check — `systemctl` present but user manager/D-Bus unreachable — and falls - back to `start_unsupervised` (a plain backgrounded process using the proxy's own - `--supervise` loop), printing manual start/stop/status/logs instructions. Well tested at - the shell-function level (`install_test.sh:570-575`). -- **Gap:** On the Go side, there's no Linux equivalent of `launchdUsable()` - (`cmd_service_platform.go:680-696`, explicitly darwin-only) — the function that lets - macOS refuse cleanly *before writing anything to disk* with a dedicated, tested exit code - (`exitNoSupervisor`, exit 4). On Linux, "no systemd" is discovered later, inside - `loadService`, after the unit file has already been written (then cleaned up on failure). - End behavior is probably fine — `install.sh`'s generic fallback classifier still buckets - it correctly — but it's fine by accident/genericness, not by an intentional, tested, - Linux-aware preflight the way darwin gets. No Go test asserts the exact message or exit - code for "systemctl absent" the way `TestServiceInstall_RestrictedEnvironment` does for - darwin. - -## Cross-cutting themes - -1. ~~The systemd rendering/string-shape logic is solid and well tested. The gap is almost - entirely at the "does this actually work against a real system service manager" layer — - nothing fakes or drives real `systemctl`/`loginctl` for Linux~~ — this branch closes the - fake-driving half: `cmd_service_systemd_test.go` now mirrors macOS's `fakeLaunchctl` - harness. The real-launchd equivalent (a real-systemd integration test) is #1076. -2. **The Linux path is architecturally simpler** (no supervisor process, no bootout-race - workaround) for a legitimate reason — but that simplicity has never been backed by the - same real-world verification that justified and shaped the macOS design. #1076. -3. **One concrete, low-risk fix stands out:** add `TimeoutStopSec=` to the Linux unit. Small, - self-contained, directly addresses checklist bullet 7. #1079. -4. **No real Linux CI smoke test exists yet anywhere in the repo** — this is #957's job, but - #957 can't be trusted until the gaps above are closed, since a smoke test built on top of - an unverified `Restart=on-failure` assumption would just as confidently pass. - -## Suggested next steps (not yet sequenced into a task plan) - -1. Add an explicit `TimeoutStopSec=` to `renderUnitFor("linux", ...)` — #1079; see bullet 7 - above. -2. ~~Build a `fakeSystemctl`/`fakeLoginctl` test harness~~ — #1080, see bullet 6 above. - Required refactoring `loadService`/`controlService`/`supervisorRunning`/ - `unloadService` to take `goos` explicitly first (same fix `renderUnitFor` already had) — - otherwise these functions can't be exercised from a non-Linux host at all. -3. Get real verification (manual or in a systemd-capable container/VM with a lingering - `systemd --user` session) that `Restart=on-failure` actually recovers the unit after - `kill -9`, and that lingering actually survives a logout/reboot-equivalent — document the - result the way the macOS KeepAlive finding is documented in code comments. -4. Decide whether Linux needs its own `launchdUsable()`-equivalent preflight (tested, named - exit code) or whether the current "discover it inside `loadService`, clean up, fall - through to the generic fallback" behavior is an acceptable, intentional asymmetry. -5. Once 1–4 land, build the real end-to-end harness (likely `systemd-run --user` in a - cgroup-v2-capable container, or a self-hosted/nspawn runner) that #957 will build its CI - job on top of. - -## Relationship to other issues (for context, not in scope here) - -- **#944** — macOS mirror of this issue. Owned by @huang195. Already has real end-to-end - verification behind it (the KeepAlive finding, bootout-race fix #880) that Linux still - lacks — worth mining `cmd_service_platform.go`'s darwin comments and tests as a template - for what "verified" should look like on Linux. -- **#955** — unattended agent workloads / headless capture path, owned by @esnible. Both - #956 and #957 need it for the "non-zero token count" smoke-test assertion. Not required - for #945 itself. -- **#964** — manual reboot verification (macOS + Linux), explicitly deferred from #944/#945 - because CI runners don't reboot. Shares the exact "does Restart=on-failure/launchd - KeepAlive really survive a restart" question raised above — worth coordinating rather than - duplicating. -- **#966** — plugin build-tag convention cleanup (retired `exclude_plugin_*` form → - `include_plugin_*`; allow-legacy-plugin-tag: this reference is design history, not a - live usage) - and a smaller desktop artifact. Touches the same install/upgrade path (H2 in that issue: - upgrading to a build that dropped a plugin an existing config still names must be handled - gracefully) — flagged there as something #944/#945/#964 must cover. Worth checking in on - this once the plugin-set change lands, since it could break an assumption made above about - upgrade always being config-preserving. diff --git a/authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md b/authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md new file mode 100644 index 000000000..f09b5a68f --- /dev/null +++ b/authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md @@ -0,0 +1,61 @@ +# fakeSystemctl/fakeLoginctl test harness — Implementation Plan + +**Goal:** Let `loadService`, `controlService`, `supervisorRunning`, and +`unloadService`'s Linux logic be exercised and tested from any host, and prove +their reaction to systemd's real vocabulary (`activating`/`failed`/`deactivating`, +`daemon-reload`/`enable` failures) rather than only the generated unit text — +closing checklist bullet 6 of #945. + +**Architecture:** All four functions took `runtime.GOOS` directly, unlike +`renderUnitFor`, which already took `goos` as an explicit parameter for exactly +this reason — refactor them the same way first. Then a `fakeSystemctl`/ +`fakeLoginctl` PATH-decoy harness mirroring the existing `fakeLaunchctl` +(`cmd_service_restricted_test.go`): a shared `installStub` helper writes a stub +script and immediately confirms it's reachable via `exec.LookPath`, closing a +vacuous-pass hole where a broken harness (stub not on `PATH`, or not executable) +reads identically to "the code under test correctly made zero calls." + +**Tech Stack:** Go 1.26.5. + +**Spec:** `authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md` + +**Issue:** cortex #945 + +## Tasks + +- [x] Refactor `loadService`, `controlService`, `supervisorRunning`, + `unloadService` to take `goos string` explicitly; update all call sites in + `cmd_service.go` to pass `runtime.GOOS`. +- [x] `cmd_service_systemd_test.go`: `fakeSystemctl`/`fakeLoginctl` via a shared + `installStub` helper (PATH-decoy binary + immediate `exec.LookPath` + reachability check). +- [x] `TestSupervisorRunning_Linux`: `is-active` states + `active`/`activating`/`failed`/`deactivating`/`inactive`, no-`systemctl`, + silent-failure cases. +- [x] `TestLoadService_Linux`: `daemon-reload`/`enable --now` failure messages, the + full happy-path invocation order (`daemon-reload` then `enable --now`, with + the exact unit name), and all three linger branches (already-on, newly + enabled, `enable-linger` failure). +- [x] `TestUnloadService_Linux`: marker-gated `disable-linger` (never called + without a marker, called with the right uid when there is one). +- [x] `TestLingerEnabled`: `loginctl show-user` parsed as yes/no/malformed/ + command-failure. +- [x] `TestControlService_Linux`: exact verb-per-action mapping + (stop→`disable --now`, start→`enable --now`, restart→plain `restart`). +- [x] Fix: `readCallLog` returning `nil` on any read error (not just + "file doesn't exist") made every negative call-count assertion vacuous — a + broken harness read identically to "genuinely never called". Reproduced by + mutating a stub non-executable before fixing, and again after, to confirm + the fix actually closes it. +- [x] Fix: fake `systemctl`/`loginctl` argument matching was positional + (`$1`/`$2`) — switched to `case "$*" in *pattern*)`, robust to the real call + sites ever reordering arguments. +- [x] Fix: unquoted call-log path in the shell fragment — reused the existing + `shQuote` helper. +- [x] Style: collapsed `controlService(goos string, action string, ...)` to + `controlService(goos, action string, ...)`. + +## Result + +27 subtests, all passing, all genuinely executed (not skipped) on any host — +including the one they were written on — because of the `goos` refactor. diff --git a/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md new file mode 100644 index 000000000..70529f6e4 --- /dev/null +++ b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md @@ -0,0 +1,161 @@ +# Linux install and systemd service lifecycle — Design + +**Date:** 2026-09-16 +**Status:** Research and gap analysis complete. +**Issue:** cortex #945 — "feature: verified Linux install and systemd service lifecycle" +**Repos touched:** `cortex/authbridge` only. + +## Problem + +#945 is a verification and gap-closing issue, not a build-from-scratch feature: the +core service-lifecycle mechanism already shipped in PR #876 ("Keep Cortex running +across crashes and logins (launchd / systemd)"), with follow-on fixes #880, #897, +#931, #911. What follows is what auditing that existing code against #945's +checklist found. + +## Conceptual background: systemd vs. launchd, and why the two platforms differ + +A service manager's job: start a background process, notice when it dies (including +crashes, not just deliberate stops), and decide whether to relaunch it — without a +human watching a terminal. + +**The key asymmetry that shapes the whole design:** + +- **systemd** (Linux): a unit file's `Restart=on-failure` is honored by systemd's own + supervisor loop, reliably, even for a unit just `systemctl --user start`ed while + already logged in (exactly Cortex's `curl | sh` scenario). +- **launchd** (macOS): the team found — and documented in code comments — that + launchd's equivalent (`KeepAlive`, tried alongside `StartInterval` and + `RunAtLoad`) does **not** reliably restart a LaunchAgent added mid-session (as + opposed to one present at boot when launchd first scans `LaunchAgents`). Since + Cortex is always installed mid-session, this gap is squarely in the installer's + path. + +**Consequence for the architecture:** + +- **macOS** runs **two processes**: launchd starts a small Go-written supervisor + (`authbridge-proxy --supervise`, `authbridge/cmd/authbridge-proxy/supervise.go`) + that itself watches and restarts the actual proxy child. launchd supervises the + supervisor; the supervisor does the real crash-recovery job launchd won't + reliably do. +- **Linux** runs **one process**: `authbridge-proxy` directly as the systemd unit's + `ExecStart=`, with `Restart=on-failure` doing all the crash-recovery work + natively. No supervisor binary involved. Confirmed in code: + `cmd_service_platform.go:92-93` and `TestSupervisionIsPlatformCorrect` + (`cmd_service_test.go:312-332`) explicitly assert Linux does **not** use + `--supervise`. + +This makes #945 structurally simpler than #944 on the crash-recovery axis — but it +also means the Linux "`Restart=on-failure` actually works" assumption had never been +put through the same real-world verification (and design correction) that produced +the macOS supervisor. That was the single biggest gap this issue needed to close. + +## Where things live + +- `authbridge/install.sh` — the `curl | sh` installer (~1300 lines, POSIX `sh`). +- `authbridge/install_test.sh` — installer unit tests (shell functions in isolation, + no real network/systemctl). +- `authbridge/cmd/abctl/cmd_service.go` — cross-platform `abctl service` command + logic (install/uninstall/status/start/stop/restart). +- `authbridge/cmd/abctl/cmd_service_platform.go` — the OS-specific half: systemd + unit rendering, launchd plist rendering, `systemctl`/`launchctl`/`loginctl` + invocations. +- `authbridge/cmd/authbridge-proxy/supervise.go` — the macOS-only (and + unsupervised-fallback) Go restart loop. +- `authbridge/cmd/authbridge-proxy/main.go` — the proxy itself; on SIGTERM/SIGINT + does a graceful shutdown with a **15-second drain** + (`context.WithTimeout(..., 15*time.Second)`, `main.go:671`) — every "does stop + tolerate the drain" question traces back to this constant. +- `authbridge/docs/laptop-service.md` — user-facing doc for `abctl service *`, + `~/.cortex/` layout, restricted-environment fallback, manual-removal + instructions. + +## Gap analysis against #945's checklist, as found + +### 1. Fresh install via `curl | sh`, amd64 and arm64 +Install-time OS/arch detection, checksum verification, and a live preflight +(`supervisor_usable()`, `install.sh:645-653`) all exist and are unit-tested. No CI +job installs and runs the real released binaries against a live Linux box, for +either architecture — arm64 is exercised at build time only. + +### 2. Upgrade over an older install: service restarted, config preserved +Idempotency (`installCanSkip`/`serviceIsCurrent`), config migration, and +binary-swap detection all exist and are well tested. Nothing drives an actual +upgrade against a real running `systemd --user` unit — macOS has a dedicated +real-launchd test for the equivalent scenario (`TestWaitBootedOut_RealLaunchd`); +Linux has no analog. + +### 3. Uninstall leaves no unit file, no `~/.cortex`, no modified agent settings +`serviceUninstall`/`unloadService` exist, including a thoughtful linger-marker +mechanism so uninstall doesn't clobber a linger setting the user set for unrelated +units. None of this Linux uninstall logic had a test driving faked +`systemctl`/`loginctl` — the existing test only greps source for expected strings. + +### 4. Re-running install is idempotent +`installCanSkip` and `install.sh`'s already-at-this-version guard exist and are +tested. `lingerEnabled()`'s parsing of `loginctl show-user --property=Linger` +output had zero test coverage. + +### 5. Service survives reboot and a crash — the central gap +Unit rendering (`Restart=on-failure`, `RestartSec=10`, `StartLimit*` correctly +placed in `[Unit]`) is well-reasoned and has good string-shape test coverage. No +test — unit, integration, or a documented manual-verification note — proved that a +real `systemd --user` instance actually restarts the unit after `kill -9`, or +survives a real reboot/logout with lingering enabled. macOS has exactly this kind +of verification on record: `KeepAlive` was tested, found not to work end-to-end, +and that finding drove the supervisor redesign. No equivalent verification episode +existed for Linux's `Restart=on-failure` assumption — trusted, not proven. + +### 6. `abctl service status | start | stop | restart` accurate in every state +`serviceStatus`/`serviceControl` are platform-agnostic and reasonably designed +(`controlService` maps stop→`disable --now`, start→`enable --now`, +restart→transient `restart`). `loadService`, `controlService`, `supervisorRunning`, +and `unloadService` all took `runtime.GOOS` directly rather than an explicit `goos` +parameter (unlike `renderUnitFor`), so none of their Linux logic could be exercised +from a non-Linux host. Nothing faked or drove real `systemctl`/`loginctl` for +Linux at all — macOS has both a `fakeLaunchctl` unit-test harness and a +real-launchd integration test. + +### 7. `stop` tolerates the proxy's ~15s drain +The proxy's own 15s shutdown timeout is the anchor value everything else has to +respect. macOS handles this explicitly in Go (a 30s bootout timeout, plus the +supervisor's own 20s-before-SIGKILL logic). The rendered Linux unit set no +`TimeoutStopSec` at all — it "worked" only by accident of systemd's undocumented +90s default exceeding 15s. + +### 8. Works under user systemd, and states what happens where systemd is absent +The best-handled bullet: `loadService` gives a clear, actionable message when +`systemctl` isn't found; `install.sh`'s `supervisor_usable()` does a live +preflight (`systemctl` present but user manager/D-Bus unreachable) and falls back +to an unsupervised mode. Gap: no Linux equivalent of `launchdUsable()` +(`cmd_service_platform.go:680-696`, darwin-only) — the function that lets macOS +refuse cleanly *before writing anything to disk*, with a dedicated tested exit +code. On Linux, "no systemd" is discovered later, inside `loadService`, after the +unit file has already been written (then cleaned up on failure). Probably fine in +practice, but by accident/genericness rather than an intentional, tested, +Linux-aware preflight — still an open decision, not yet resolved either way. + +## Relationship to other issues + +- **#944** — macOS mirror of this issue, owned by @huang195. Already has real + end-to-end verification behind it (the `KeepAlive` finding, bootout-race fix + #880) that Linux lacked. **A related, separately-discovered gap:** + `ABCTL_LAUNCHD_TESTS=required` exists in `cmd_service_bootout_test.go` (an + escape hatch that turns a silent skip into a hard failure) but is never set by + any workflow, so `TestWaitBootedOut_RealLaunchd` skips in every CI run. No macOS + runner exists in this repo to close that from the Linux side — worth tracking + against #944 or #956 (macOS smoke tests) rather than leaving it to be + rediscovered. +- **#955** — unattended agent workloads / headless capture path, owned by + @esnible. #956 and #957 both need it for a "non-zero token count" smoke-test + assertion. Not required for #945 itself. +- **#964** — manual reboot verification (macOS + Linux), explicitly deferred from + #944/#945 because CI runners don't reboot. Shares the "does + `Restart=on-failure`/launchd `KeepAlive` really survive a restart" question + raised above — worth coordinating rather than duplicating. +- **#966** — plugin build-tag convention cleanup (retired `exclude_plugin_*` form → + `include_plugin_*`; allow-legacy-plugin-tag: this reference is design history, not + a live usage) and a smaller desktop artifact. Touches the same + install/upgrade path: upgrading to a build that dropped a plugin an existing + config still names must be handled gracefully — flagged there as something + #944/#945/#964 must cover. From 07cce6fe940735fa6bfcd73a4eb8cfb93e77ccf7 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 14:36:10 -0400 Subject: [PATCH 10/12] fix: Thread goos through supervisorName, fix a stale doc citation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit supervisorName still read runtime.GOOS directly, one call deep inside loadService — the one function this PR's refactor made accept goos explicitly. Dormant today since every new test only drives "linux", but it would have silently produced the wrong supervisor name in any error message if loadService("darwin", ...) were ever exercised from a non-darwin host, which is exactly the capability this refactor exists to enable. Also fixes the shared spec doc's stale main.go line reference for the 15s shutdown timeout (was pointing at unrelated mTLS setup code) and softens an inaccurate "undocumented" claim about systemd's DefaultTimeoutStopSec, which is documented in systemd.system.conf(5). Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service.go | 12 ++++++------ authbridge/cmd/abctl/cmd_service_platform.go | 8 ++++---- .../2026-09-16-linux-systemd-lifecycle-design.md | 6 +++--- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service.go b/authbridge/cmd/abctl/cmd_service.go index 837314a33..06128039f 100644 --- a/authbridge/cmd/abctl/cmd_service.go +++ b/authbridge/cmd/abctl/cmd_service.go @@ -276,7 +276,7 @@ func serviceInstall(p servicePaths, yes, forceRestart bool, stdout, stderr io.Wr // of the installer's output for one fact. if !yes { fmt.Fprintf(stdout, "This will install a %s that runs:\n %s --config %s\n\n", - supervisorName(), p.binary, p.configFile) + supervisorName(runtime.GOOS), p.binary, p.configFile) fmt.Fprintf(stdout, "It restarts on failure and starts at login, so Claude Code keeps working\n"+ "after a crash or a reboot. Unit file: %s\n\n", p.unitFile) } @@ -308,7 +308,7 @@ func serviceInstall(p servicePaths, yes, forceRestart bool, stdout, stderr io.Wr " Cortex still runs, just not supervised — start it yourself:\n"+ " %s --local\n\n"+ " It will not restart after a crash or come back at login while running that\n"+ - " way. To stop it: kill that process.\n", supervisorName(), why, p.binary) + " way. To stop it: kill that process.\n", supervisorName(runtime.GOOS), why, p.binary) return exitNoSupervisor } @@ -381,7 +381,7 @@ func serviceInstall(p servicePaths, yes, forceRestart bool, stdout, stderr io.Wr if installCanSkip(configChanged, forceRestart, func() bool { return serviceIsCurrent(p) }) { fmt.Fprintf(stdout, "Already current: %s is running under %s and healthy.\n"+ " Nothing to change. Use `abctl service restart` to restart it anyway.\n", - filepath.Base(p.binary), supervisorName()) + filepath.Base(p.binary), supervisorName(runtime.GOOS)) return 0 } @@ -491,9 +491,9 @@ const crashRecoveryNote = "A supervisor process handles crashes (launchd will no // better. There is now no "which path prints what" to get wrong. func reportInstallSuccess(healthy bool, stdout io.Writer) { if healthy { - fmt.Fprintf(stdout, "Running as a %s, healthy.\n", supervisorName()) + fmt.Fprintf(stdout, "Running as a %s, healthy.\n", supervisorName(runtime.GOOS)) } else { - fmt.Fprintf(stdout, "Running as a %s.\n", supervisorName()) + fmt.Fprintf(stdout, "Running as a %s.\n", supervisorName(runtime.GOOS)) } if runtime.GOOS == "darwin" { fmt.Fprintln(stdout, crashRecoveryNote) @@ -505,7 +505,7 @@ func serviceUninstall(p servicePaths, yes bool, stdout, stderr io.Writer) int { fmt.Fprintf(stdout, "Nothing to do: no unit at %s\n", p.unitFile) return 0 } - fmt.Fprintf(stdout, "This will stop and remove the %s at:\n %s\n\n", supervisorName(), p.unitFile) + fmt.Fprintf(stdout, "This will stop and remove the %s at:\n %s\n\n", supervisorName(runtime.GOOS), p.unitFile) fmt.Fprintf(stdout, "Cortex will no longer start at login. Claude Code stops working whenever\n"+ "the proxy is not running — `abctl claude-code disable` removes that dependency.\n\n") if !yes && !confirm(stdout) { diff --git a/authbridge/cmd/abctl/cmd_service_platform.go b/authbridge/cmd/abctl/cmd_service_platform.go index 9767d599a..d841803d8 100644 --- a/authbridge/cmd/abctl/cmd_service_platform.go +++ b/authbridge/cmd/abctl/cmd_service_platform.go @@ -28,8 +28,8 @@ func newFlagSet(name string, stderr io.Writer) *flag.FlagSet { return fs } -func supervisorName() string { - if runtime.GOOS == "darwin" { +func supervisorName(goos string) string { + if goos == "darwin" { return "launchd user agent" } return "systemd user unit" @@ -177,10 +177,10 @@ func loadService(goos string, p servicePaths, progress io.Writer) error { if !waitBootedOutf(target, serviceBootoutTimeout, progress) { if bootoutErr != nil && !strings.Contains(string(bootoutOut), "No such process") { return fmt.Errorf("could not remove the previous %s: %v: %s", - supervisorName(), bootoutErr, strings.TrimSpace(string(bootoutOut))) + supervisorName(goos), bootoutErr, strings.TrimSpace(string(bootoutOut))) } return fmt.Errorf("the previous %s is still shutting down after %s; "+ - "run `abctl service status`, then try again", supervisorName(), serviceBootoutTimeout) + "run `abctl service status`, then try again", supervisorName(goos), serviceBootoutTimeout) } // Retried on EIO, re-checking the domain each time. Without the re-check the diff --git a/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md index 70529f6e4..bfe146953 100644 --- a/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md +++ b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md @@ -64,7 +64,7 @@ the macOS supervisor. That was the single biggest gap this issue needed to close unsupervised-fallback) Go restart loop. - `authbridge/cmd/authbridge-proxy/main.go` — the proxy itself; on SIGTERM/SIGINT does a graceful shutdown with a **15-second drain** - (`context.WithTimeout(..., 15*time.Second)`, `main.go:671`) — every "does stop + (`context.WithTimeout(..., 15*time.Second)`, `main.go:957`) — every "does stop tolerate the drain" question traces back to this constant. - `authbridge/docs/laptop-service.md` — user-facing doc for `abctl service *`, `~/.cortex/` layout, restricted-environment fallback, manual-removal @@ -120,8 +120,8 @@ real-launchd integration test. The proxy's own 15s shutdown timeout is the anchor value everything else has to respect. macOS handles this explicitly in Go (a 30s bootout timeout, plus the supervisor's own 20s-before-SIGKILL logic). The rendered Linux unit set no -`TimeoutStopSec` at all — it "worked" only by accident of systemd's undocumented -90s default exceeding 15s. +`TimeoutStopSec` at all — it "worked" only by accident of systemd's own +90s default (`systemd.system.conf(5)`) exceeding 15s. ### 8. Works under user systemd, and states what happens where systemd is absent The best-handled bullet: `loadService` gives a clear, actionable message when From 5281327e752d9fc93bb16f5687b031c5d5cc1481 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 15:44:36 -0400 Subject: [PATCH 11/12] fix: Address round-2 review findings on the fake-harness PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cite main.go's shutdown timeout by symbol, not line number, in the spec doc — same fix as the sibling PR, same reason: main.go isn't part of either PR's diff and keeps moving as unrelated PRs merge. - Record #1079's TimeoutStopSec=20 as closing gap 7, now that it has merged. - Clarify the #966 reference: that issue is closed, and its allow-legacy-plugin-tag exemption matches the whole file, not just the one mention. - Narrow the plan doc's "closing checklist bullet 6 of #945" claim: this PR closes the unit-test half only. The spec's real-systemd integration-test half and the serviceStatus/serviceControl-accuracy half are untouched here. - fakeLaunchctl now delegates to installStub instead of hand-copying its body without the exec.LookPath reachability check: the vacuous-pass hole installStub closed for fakeSystemctl/fakeLoginctl was still open on the launchd side, in the very file this harness is modelled on. - Document that noSystemctlOnPath replaces PATH rather than prepending to it, so it must never run after fakeSystemctl/fakeLoginctl in the same subtest. - Thread callLog through "daemon-reload failure is reported, enable is never attempted" so the never-called claim is a literal check against the call log, matching every other never-called assertion in this file, rather than something inferred from the error message format. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../cmd/abctl/cmd_service_restricted_test.go | 13 ++++---- .../cmd/abctl/cmd_service_systemd_test.go | 16 +++++++++- .../plans/2026-09-21-systemd-fake-harness.md | 5 ++- ...26-09-16-linux-systemd-lifecycle-design.md | 32 +++++++++++++------ 4 files changed, 48 insertions(+), 18 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_restricted_test.go b/authbridge/cmd/abctl/cmd_service_restricted_test.go index e3ec30869..4c0c4e633 100644 --- a/authbridge/cmd/abctl/cmd_service_restricted_test.go +++ b/authbridge/cmd/abctl/cmd_service_restricted_test.go @@ -11,14 +11,15 @@ import ( ) // fakeLaunchctl puts a launchctl on PATH that behaves like a restricted sandbox: it -// cannot answer, exactly as reported from a real one. +// cannot answer, exactly as reported from a real one. Delegates to installStub +// (cmd_service_systemd_test.go) so callers get the same exec.LookPath reachability +// check fakeSystemctl/fakeLoginctl have: without it, a stub that's silently +// unreachable (PATH not applied yet, or written non-executable) makes a +// zero-calls assertion pass for the wrong reason, indistinguishable from a real +// zero-calls outcome. func fakeLaunchctl(t *testing.T, body string) { t.Helper() - dir := t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "launchctl"), []byte(body), 0o700); err != nil { //nolint:gosec - t.Fatal(err) - } - t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + installStub(t, "launchctl", body) } // TestLabelGone_UnknownIsNotGone is the defect this fixes. launchctl print exits 113 diff --git a/authbridge/cmd/abctl/cmd_service_systemd_test.go b/authbridge/cmd/abctl/cmd_service_systemd_test.go index 40d3055f8..1dcf1e0b1 100644 --- a/authbridge/cmd/abctl/cmd_service_systemd_test.go +++ b/authbridge/cmd/abctl/cmd_service_systemd_test.go @@ -60,7 +60,11 @@ func installStub(t *testing.T, name, body string) { } // noSystemctlOnPath points PATH at an empty directory, so exec.LookPath("systemctl") -// fails the same way it would on a box with no systemd at all. +// fails the same way it would on a box with no systemd at all. Call this instead +// of, never after, fakeSystemctl/fakeLoginctl/installStub in the same subtest: it +// replaces PATH rather than prepending to it, so it would silently shadow out any +// stub already installed, the same invisible failure installStub's own +// reachability check exists to catch. func noSystemctlOnPath(t *testing.T) { t.Helper() t.Setenv("PATH", t.TempDir()) @@ -175,7 +179,9 @@ func TestLoadService_Linux(t *testing.T) { t.Run("daemon-reload failure is reported, enable is never attempted", func(t *testing.T) { p := servicePathsFixture(t) + logPath, logLine := callLog(t) fakeSystemctl(t, `#!/bin/sh +`+logLine+` case "$*" in *daemon-reload*) echo "boom: unit has a syntax error" >&2 @@ -188,6 +194,14 @@ exit 1 if err == nil || !strings.Contains(err.Error(), "daemon-reload") || !strings.Contains(err.Error(), "boom") { t.Errorf("err = %v, want it to name daemon-reload and the underlying reason", err) } + // The error-message assertion above already implies this (an enable --now + // attempt would hit the fallthrough and produce a different message), but + // the call log makes "enable is never attempted" a literal check rather + // than something a reader has to re-derive from the error format. + calls := readCallLog(t, logPath) + if len(calls) != 1 || calls[0] != "--user daemon-reload" { + t.Errorf("systemctl calls = %v, want exactly [--user daemon-reload]", calls) + } }) t.Run("enable --now failure is reported", func(t *testing.T) { diff --git a/authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md b/authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md index f09b5a68f..91a0a70e8 100644 --- a/authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md +++ b/authbridge/docs/superpowers/plans/2026-09-21-systemd-fake-harness.md @@ -4,7 +4,10 @@ `unloadService`'s Linux logic be exercised and tested from any host, and prove their reaction to systemd's real vocabulary (`activating`/`failed`/`deactivating`, `daemon-reload`/`enable` failures) rather than only the generated unit text — -closing checklist bullet 6 of #945. +closing the unit-test half of checklist bullet 6 of #945. The spec doc's bullet 6 +also covers `serviceStatus`/`serviceControl` accuracy more broadly and a +real-systemd integration test analogous to `TestWaitBootedOut_RealLaunchd`; +neither is touched here. **Architecture:** All four functions took `runtime.GOOS` directly, unlike `renderUnitFor`, which already took `goos` as an explicit parameter for exactly diff --git a/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md index bfe146953..863b60974 100644 --- a/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md +++ b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md @@ -63,9 +63,11 @@ the macOS supervisor. That was the single biggest gap this issue needed to close - `authbridge/cmd/authbridge-proxy/supervise.go` — the macOS-only (and unsupervised-fallback) Go restart loop. - `authbridge/cmd/authbridge-proxy/main.go` — the proxy itself; on SIGTERM/SIGINT - does a graceful shutdown with a **15-second drain** - (`context.WithTimeout(..., 15*time.Second)`, `main.go:957`) — every "does stop - tolerate the drain" question traces back to this constant. + does a graceful shutdown with a **15-second drain** (`main()`'s `shutdownCtx := + context.WithTimeout(..., 15*time.Second)`, right after the signal wait — cited + by site rather than line number, since `main.go` isn't part of this PR's diff + and keeps moving independently) — every "does stop tolerate the drain" question + traces back to this constant. - `authbridge/docs/laptop-service.md` — user-facing doc for `abctl service *`, `~/.cortex/` layout, restricted-environment fallback, manual-removal instructions. @@ -119,9 +121,15 @@ real-launchd integration test. ### 7. `stop` tolerates the proxy's ~15s drain The proxy's own 15s shutdown timeout is the anchor value everything else has to respect. macOS handles this explicitly in Go (a 30s bootout timeout, plus the -supervisor's own 20s-before-SIGKILL logic). The rendered Linux unit set no -`TimeoutStopSec` at all — it "worked" only by accident of systemd's own -90s default (`systemd.system.conf(5)`) exceeding 15s. +supervisor's own 20s-before-SIGKILL logic). As of this audit (2026-09-16), the +rendered Linux unit set no `TimeoutStopSec` at all — it "worked" only by accident +of systemd's own 90s default (`systemd.system.conf(5)`) exceeding 15s. + +**Closed by #1079** — `renderUnitFor("linux", ...)` now sets `TimeoutStopSec=20` +explicitly on `main`, matching the macOS supervisor's 20s headroom over the same +15s drain, with a subtest in `cmd_service_test.go` asserting the line is present. +Not yet on this branch's own tree until it merges — cited by symbol, not line, +since #1079 landed in a sibling PR. ### 8. Works under user systemd, and states what happens where systemd is absent The best-handled bullet: `loadService` gives a clear, actionable message when @@ -155,7 +163,11 @@ Linux-aware preflight — still an open decision, not yet resolved either way. raised above — worth coordinating rather than duplicating. - **#966** — plugin build-tag convention cleanup (retired `exclude_plugin_*` form → `include_plugin_*`; allow-legacy-plugin-tag: this reference is design history, not - a live usage) and a smaller desktop artifact. Touches the same - install/upgrade path: upgrading to a build that dropped a plugin an existing - config still names must be handled gracefully — flagged there as something - #944/#945/#964 must cover. + a live usage — the guard matches on the whole file via `strings.Contains`, not + scoped to this one mention, so a real `exclude_plugin_*` usage added anywhere + else in this file would also pass silently) and a smaller desktop artifact. + Touches the same install/upgrade path: upgrading to a build that dropped a + plugin an existing config still names must be handled gracefully — #966 itself + closed 2026-09-16 having noted this as something #944/#945/#964 would need to + cover; tracking it directly against one of those now that #966 is no longer + open would keep the concern from being lost. From 20edc0fdb89292f25df9e923b23da81d5412a03e Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 17:07:27 -0400 Subject: [PATCH 12/12] docs: Fix the same shutdownCancel omission as the sibling PR context.WithTimeout also returns a cancel func; the spec doc's citation elided it. Matches the identical fix just made on #1076's copy of this shared doc, keeping both byte-identical. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- .../specs/2026-09-16-linux-systemd-lifecycle-design.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md index 863b60974..e57864e10 100644 --- a/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md +++ b/authbridge/docs/superpowers/specs/2026-09-16-linux-systemd-lifecycle-design.md @@ -63,11 +63,11 @@ the macOS supervisor. That was the single biggest gap this issue needed to close - `authbridge/cmd/authbridge-proxy/supervise.go` — the macOS-only (and unsupervised-fallback) Go restart loop. - `authbridge/cmd/authbridge-proxy/main.go` — the proxy itself; on SIGTERM/SIGINT - does a graceful shutdown with a **15-second drain** (`main()`'s `shutdownCtx := - context.WithTimeout(..., 15*time.Second)`, right after the signal wait — cited - by site rather than line number, since `main.go` isn't part of this PR's diff - and keeps moving independently) — every "does stop tolerate the drain" question - traces back to this constant. + does a graceful shutdown with a **15-second drain** (`main()`'s + `shutdownCtx, shutdownCancel := context.WithTimeout(..., 15*time.Second)`, right + after the signal wait — cited by site rather than line number, since `main.go` + isn't part of this PR's diff and keeps moving independently) — every "does stop + tolerate the drain" question traces back to this constant. - `authbridge/docs/laptop-service.md` — user-facing doc for `abctl service *`, `~/.cortex/` layout, restricted-environment fallback, manual-removal instructions.