Skip to content

Fix: Set an explicit TimeoutStopSec on the Linux service unit #1079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Alan-Cha merged 5 commits into from
Sep 22, 2026
+16 −0
Merged

Fix: Set an explicit TimeoutStopSec on the Linux service unit #1079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f2fe9d
fix: Set an explicit TimeoutStopSec on the Linux service unit
Alan-Cha Sep 17, 2026
cfd0e9a
fix: Assert TimeoutStopSec's actual value, not just its presence
Alan-Cha Sep 21, 2026
c2aed8a
fix: Simplify the TimeoutStopSec assertion to a plain exact-value check
Alan-Cha Sep 21, 2026
ed0d41f
docs: Trim the TimeoutStopSec rationale comment to the core claim
Alan-Cha Sep 21, 2026
fc6d143
docs: Correct the TimeoutStopSec comment's framing on #1079
Alan-Cha Sep 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions authbridge/cmd/abctl/cmd_service_platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ 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: 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
Expand All @@ -140,6 +147,7 @@ Type=simple
ExecStart=` + shQuote(p.binary) + ` --config ` + shQuote(p.configFile) + `
Restart=on-failure
RestartSec=10
TimeoutStopSec=20

@huang195 huang195 Sep 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion — explicit over implicit is the right call, but the comment tells only half the story and the direction is worth stating out loud.

The net effect on a stock systemd is a narrower window, not wider. DefaultTimeoutStopSec is 90s in both the system and user managers (systemd-user.conf(5)), so this trades 90s of slack for 20s. That's still the right trade — a manager default is configurable and could just as easily be lowered under 15s, which is the real hazard — but the comment reads as if it's adding headroom when it's mostly pinning it.

And 15s is the drain deadline, not the process's stop time. shutdownCtx bounds the HTTP servers and both pipelines, but costLedger.Close() and sessions.Close() run after that deadline with no context governing them (cmd/authbridge-proxy/main.go:987-995). So the 5s of margin here is covering unbounded post-deadline work — and per the ledger's own comment, that flush is exactly what turns "a restart loses up to 60 seconds of cost" into "an orderly stop loses nothing." A slow disk at that moment is a SIGKILL and a lost open minute.

Either is fine to resolve in a sentence of comment rather than a code change; the value itself is defensible.

StandardOutput=append:` + p.logFile + `
StandardError=append:` + p.logFile + `

Expand Down
8 changes: 8 additions & 0 deletions authbridge/cmd/abctl/cmd_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=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 —
// silently voiding the crash-loop throttle.
Expand Down
Loading