From 4f2fe9d727e3b42e3d929e4c58df8b2949764842 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Thu, 17 Sep 2026 13:14:43 -0400 Subject: [PATCH 1/5] fix: Set an explicit TimeoutStopSec on the Linux service unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rendered systemd unit set no TimeoutStopSec, so `systemctl --user stop` relied on systemd's undocumented default (90s) to exceed the proxy's own 15s graceful-shutdown deadline. Make that margin explicit at 20s, matching the headroom the macOS supervisor already gives the same proxy shutdown before it insists with a kill. Verified by build/vet/gofmt and an extended TestRenderUnit_BothPlatforms assertion. Not yet verified against a real systemd — no integration test drives a live `systemd --user` session for this repo's Linux path. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service_platform.go | 9 +++++++++ authbridge/cmd/abctl/cmd_service_test.go | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/authbridge/cmd/abctl/cmd_service_platform.go b/authbridge/cmd/abctl/cmd_service_platform.go index a37d491d3..d54f713e0 100644 --- a/authbridge/cmd/abctl/cmd_service_platform.go +++ b/authbridge/cmd/abctl/cmd_service_platform.go @@ -128,6 +128,14 @@ func renderUnitFor(goos string, p servicePaths) string { // No After=network-online.target: that target is not part of a user manager, and // every listener here is loopback, so ordering against the network would be a // dependency that never arrives. + // + // TimeoutStopSec=20: without it, `systemctl --user stop` falls back to systemd's + // own default (90s at the time of writing, undocumented here and not ours to rely + // on). The proxy's own graceful shutdown deadline is 15s + // (cmd/authbridge-proxy/main.go); 20s gives it deliberate headroom to finish that + // drain — mirroring the macOS supervisor's 20s wait for the same proxy shutdown + // before it insists with a kill (supervise.go) — rather than depending on a value + // this file never states. return `[Unit] Description=Cortex local proxy (authbridge-proxy) Documentation=https://github.com/rossoctl/cortex @@ -140,6 +148,7 @@ Type=simple ExecStart=` + shQuote(p.binary) + ` --config ` + shQuote(p.configFile) + ` Restart=on-failure RestartSec=10 +TimeoutStopSec=20 StandardOutput=append:` + p.logFile + ` StandardError=append:` + p.logFile + ` diff --git a/authbridge/cmd/abctl/cmd_service_test.go b/authbridge/cmd/abctl/cmd_service_test.go index fc5e6aec8..420b5cb91 100644 --- a/authbridge/cmd/abctl/cmd_service_test.go +++ b/authbridge/cmd/abctl/cmd_service_test.go @@ -80,6 +80,14 @@ func TestRenderUnit_BothPlatforms(t *testing.T) { t.Errorf("unit missing %q:\n%s", want, u) } } + // TimeoutStopSec must exceed the proxy's own 15s graceful-shutdown deadline + // (cmd/authbridge-proxy/main.go), explicitly — not by accident of whatever + // systemd's own default happens to be. See the rationale comment above + // renderUnitFor's linux branch. + if !strings.Contains(u, "TimeoutStopSec=") { + t.Error("no explicit TimeoutStopSec; stop relies on systemd's undocumented default, " + + "which could end up shorter than the proxy's 15s drain") + } // StartLimit* must sit in [Unit]. systemd moved them there in v229 and // deprecated them in [Service], where they can be ignored outright — // silently voiding the crash-loop throttle. From cfd0e9a257cf70bcde205b033edc0f6c8231ecd3 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Mon, 21 Sep 2026 10:24:58 -0400 Subject: [PATCH 2/5] fix: Assert TimeoutStopSec's actual value, not just its presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #1079: the prior assertion only checked that a TimeoutStopSec key existed, via strings.Contains — a regression to TimeoutStopSec=5 (or any value <=15) would satisfy it while defeating the entire point of setting the value explicitly. Parse the value and require it to exceed 15 (the proxy's own graceful- shutdown deadline, main.go), rather than pinning to the literal 20: that's the actual invariant this constant exists to satisfy, and it won't false-fail if the value is later deliberately tuned. Verified by temporarily setting TimeoutStopSec=5 in renderUnitFor and confirming the test fails with a clear message, then reverting. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service_test.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_test.go b/authbridge/cmd/abctl/cmd_service_test.go index 420b5cb91..67e3e225c 100644 --- a/authbridge/cmd/abctl/cmd_service_test.go +++ b/authbridge/cmd/abctl/cmd_service_test.go @@ -84,10 +84,26 @@ func TestRenderUnit_BothPlatforms(t *testing.T) { // (cmd/authbridge-proxy/main.go), explicitly — not by accident of whatever // systemd's own default happens to be. See the rationale comment above // renderUnitFor's linux branch. - if !strings.Contains(u, "TimeoutStopSec=") { - t.Error("no explicit TimeoutStopSec; stop relies on systemd's undocumented default, " + + // Assert the actual invariant (greater than 15), not merely that the key is + // present: a regression to e.g. TimeoutStopSec=5 would satisfy a bare + // Contains check while defeating the entire point of setting it explicitly. + const timeoutKey = "TimeoutStopSec=" + ti := strings.Index(u, timeoutKey) + if ti < 0 { + t.Fatal("no explicit TimeoutStopSec; stop relies on systemd's undocumented default, " + "which could end up shorter than the proxy's 15s drain") } + rest := u[ti+len(timeoutKey):] + if nl := strings.IndexByte(rest, '\n'); nl >= 0 { + rest = rest[:nl] + } + secs, perr := strconv.Atoi(strings.TrimSpace(rest)) + if perr != nil { + t.Fatalf("TimeoutStopSec value %q is not a plain integer of seconds", rest) + } + if secs <= 15 { + t.Errorf("TimeoutStopSec=%d does not exceed the proxy's 15s drain", secs) + } // StartLimit* must sit in [Unit]. systemd moved them there in v229 and // deprecated them in [Service], where they can be ignored outright — // silently voiding the crash-loop throttle. From c2aed8a2c84e3042737fc50dc4708ff8af7a9404 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Mon, 21 Sep 2026 10:44:41 -0400 Subject: [PATCH 3/5] fix: Simplify the TimeoutStopSec assertion to a plain exact-value check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parser added to address CodeRabbit's finding was more machinery than a one-line production fix warrants: nothing requires this value to be flexible, and if it's ever deliberately changed, updating one test literal is trivial. Testing the literal value directly is simpler and equally effective. Replace it with strings.Contains(u, "TimeoutStopSec=20\n") — the trailing newline avoids matching TimeoutStopSec=200/201 etc. Still verified to fail on a mutated TimeoutStopSec=5 and pass on 20. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service_test.go | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_test.go b/authbridge/cmd/abctl/cmd_service_test.go index 67e3e225c..029e24c1a 100644 --- a/authbridge/cmd/abctl/cmd_service_test.go +++ b/authbridge/cmd/abctl/cmd_service_test.go @@ -84,25 +84,9 @@ func TestRenderUnit_BothPlatforms(t *testing.T) { // (cmd/authbridge-proxy/main.go), explicitly — not by accident of whatever // systemd's own default happens to be. See the rationale comment above // renderUnitFor's linux branch. - // Assert the actual invariant (greater than 15), not merely that the key is - // present: a regression to e.g. TimeoutStopSec=5 would satisfy a bare - // Contains check while defeating the entire point of setting it explicitly. - const timeoutKey = "TimeoutStopSec=" - ti := strings.Index(u, timeoutKey) - if ti < 0 { - t.Fatal("no explicit TimeoutStopSec; stop relies on systemd's undocumented default, " + - "which could end up shorter than the proxy's 15s drain") - } - rest := u[ti+len(timeoutKey):] - if nl := strings.IndexByte(rest, '\n'); nl >= 0 { - rest = rest[:nl] - } - secs, perr := strconv.Atoi(strings.TrimSpace(rest)) - if perr != nil { - t.Fatalf("TimeoutStopSec value %q is not a plain integer of seconds", rest) - } - if secs <= 15 { - t.Errorf("TimeoutStopSec=%d does not exceed the proxy's 15s drain", secs) + if !strings.Contains(u, "TimeoutStopSec=20\n") { + t.Error("TimeoutStopSec is missing or not 20; stop relies on an unasserted " + + "value, which could end up shorter than the proxy's 15s drain") } // StartLimit* must sit in [Unit]. systemd moved them there in v229 and // deprecated them in [Service], where they can be ignored outright — From ed0d41f8681174b7c79aa694684516d09b32e074 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Mon, 21 Sep 2026 13:30:46 -0400 Subject: [PATCH 4/5] docs: Trim the TimeoutStopSec rationale comment to the core claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #1079: 9 lines of comment for one config value. Cut it to the actual claim (20s exceeds the proxy's 15s drain, matches the macOS supervisor's own headroom) and drop the "why not rely on systemd's default" elaboration — that's in the PR description and commit message, not owed to every future reader of this file. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service_platform.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_platform.go b/authbridge/cmd/abctl/cmd_service_platform.go index d54f713e0..7ea215a6f 100644 --- a/authbridge/cmd/abctl/cmd_service_platform.go +++ b/authbridge/cmd/abctl/cmd_service_platform.go @@ -129,13 +129,9 @@ func renderUnitFor(goos string, p servicePaths) string { // every listener here is loopback, so ordering against the network would be a // dependency that never arrives. // - // TimeoutStopSec=20: without it, `systemctl --user stop` falls back to systemd's - // own default (90s at the time of writing, undocumented here and not ours to rely - // on). The proxy's own graceful shutdown deadline is 15s - // (cmd/authbridge-proxy/main.go); 20s gives it deliberate headroom to finish that - // drain — mirroring the macOS supervisor's 20s wait for the same proxy shutdown - // before it insists with a kill (supervise.go) — rather than depending on a value - // this file never states. + // TimeoutStopSec=20: exceeds the proxy's own 15s graceful-shutdown deadline + // (cmd/authbridge-proxy/main.go), matching the macOS supervisor's 20s wait for + // the same shutdown (supervise.go). return `[Unit] Description=Cortex local proxy (authbridge-proxy) Documentation=https://github.com/rossoctl/cortex From fc6d1439c7c125bf468eb9496e6fb072960688a2 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 22 Sep 2026 11:22:10 -0400 Subject: [PATCH 5/5] docs: Correct the TimeoutStopSec comment's framing on #1079 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review: two things the comment got wrong, both about framing rather than the value itself (which the reviewer confirms is fine). - systemd's own DefaultTimeoutStopSec is 90s in both system and user managers, per systemd-user.conf(5). Setting 20 narrows that window down toward the proxy's 15s deadline; it doesn't add headroom on top of an undocumented default, since there was already more than enough margin. The real hazard the explicit value guards against is a future *lower* default, not a shorter one. - 15s bounds the HTTP servers and both pipelines specifically (shutdownCtx in main.go), not the process's total stop time. costLedger.Close() and sessions.Close() run after that deadline with no context governing them — confirmed against origin/main, which has moved since this branch forked and now contains exactly that. The 5s of margin over 15 is covering that unbounded tail, not padding an already-bounded number. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- authbridge/cmd/abctl/cmd_service_platform.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/authbridge/cmd/abctl/cmd_service_platform.go b/authbridge/cmd/abctl/cmd_service_platform.go index 7ea215a6f..778e3aa83 100644 --- a/authbridge/cmd/abctl/cmd_service_platform.go +++ b/authbridge/cmd/abctl/cmd_service_platform.go @@ -129,9 +129,12 @@ func renderUnitFor(goos string, p servicePaths) string { // every listener here is loopback, so ordering against the network would be a // dependency that never arrives. // - // TimeoutStopSec=20: exceeds the proxy's own 15s graceful-shutdown deadline - // (cmd/authbridge-proxy/main.go), matching the macOS supervisor's 20s wait for - // the same shutdown (supervise.go). + // TimeoutStopSec=20: narrows systemd's own default (90s) down toward the proxy's + // 15s shutdown deadline (cmd/authbridge-proxy/main.go), rather than adding headroom + // to nothing — matches the macOS supervisor's 20s wait for the same shutdown + // (supervise.go). 15s bounds the HTTP servers and pipelines specifically; a couple + // of unbounded flushes run after that deadline, which is what the 5s of slack over + // 15 is actually for. return `[Unit] Description=Cortex local proxy (authbridge-proxy) Documentation=https://github.com/rossoctl/cortex