SIGTERM during startup panics on a nil httpServer, and cleanShutdown reads two fields with no happens-before edge #226
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found during the review of #218. Pre-existing on
next; NOT introduced or widened by that PR, which is why it was not blocking there.1. Nil dereference on an early SIGTERM.
s.httpServeris assigned inside theserveUntilShutdowngoroutine (internal/server/http.go:29), butcleanShutdowncallss.httpServer.Shutdown(ctxShutdown)unguarded (internal/server/server.go:236). A SIGTERM arriving between theOnStarthook returning and that assignment panics on a nil*http.Server.The window is not theoretical: it spans
configure()andenableSentry(), and the latter performs a networksentry.InitwhenSENTRY_DSNis set. A container killed early in a crash-loop or a fast redeploy lands in it.2. No happens-before edge on
s.httpServerands.sentryEnabled. On the SIGTERM path, and on a directapp.Stop(), those fields are written by the serve goroutine and read bycleanShutdownwith no synchronisation. The race detector has never flagged it because the existinginternal/servertests never callserve(). The path added by #218 IS race-free — the Shutdowner channel supplies the edge — which is precisely why-racestayed silent there.Not milestoned to 1.0: the impact is a non-graceful exit of a process that was already terminating, and steady-state operation is unaffected. Raising it here rather than folding it into the lifecycle work so it is not lost.
Definition of done:
cleanShutdownreadshttpServerandsentryEnabledunder a mutex, or behind a "server ready" handshakeapp.Stop()immediately afterapp.Start()and passes under-racestopTimeout/ShutdownTimeout/TailHookReservearithmetic pinned by #134 and #102 stays intactSeparately, and cheaply: nothing in the repo tests that the process exit STATUS is non-zero on listen failure — the test added by #218 asserts fx's
ShutdownSignal.ExitCode, not the process's. RewritingmainasStart/Done/Stopwould regress #200 silently with that test still green. Worth an exec-based assertion on the built binary.Reopening. I closed this prematurely and the defect is still in
next.The fix is real and was independently verified — both halves, by reverting each change separately and reproducing the exact predicted failure 3/3 — but that verification ran against the branch of #277, which has NOT merged. On
nexttoday,internal/server/server.go:236still callss.httpServer.Shutdown(ctxShutdown)unguarded whileinternal/server/http.go:29assignss.httpServerinside theserveUntilShutdowngoroutine, andsentryEnabledis read atserver.go:245with the same absence of an edge.I closed it on a reviewer's confirmation without checking the fix had landed. Caught by the
TODO.mdrefresh unit, which declined to record it as fixed and escalated instead of writing something false.This closes for real when #277 merges. That PR fixes it as a side effect of needing the race gone to write its own tests: it constructs the server in
New— stronger than the mutex this issue's definition of done proposed, since afterNewreturns there is no mutable shared state to guard — and makessentryEnabledanatomic.Bool. It also addsTestEarlyShutdown_NoPanicAndNoRace, which callsapp.Stop()immediately afterapp.Start()with no sleep across 25 cycles, and which fails in each singly-reverted state.Not dispatching this separately; doing so would duplicate work already written and reviewed.
Now genuinely fixed and landed. #277 merged as
62576f6onnext.Verified against
nextitself rather than a branch, which is the step I skipped when I closed this prematurely earlier:internal/server/server.go:118—s.httpServer = s.newHTTPServer()in the constructor.internal/server/http.go:46carries the note that this used to be assigned from the serving goroutine. AfterNewreturns there is no mutable shared state to guard, which is stronger than the mutex this issue's definition of done proposed.internal/server/server.go:100—sentryEnabled atomic.Bool, withStoreat:154/:175andLoadat:254.internal/server/early_shutdown_test.gopresent. It callsapp.Stop()immediately afterapp.Start()with no sleep, across 25 cycles so the window is actually observed, and an independent review confirmed it fails in each singly-reverted state — nil-pointer panic plus 7 races with the constructor change reverted; the exact predicted write/read pair with onlysentryEnabledreverted.make checkonnextruns it with the race detector, 0SKIP, 0DATA RACE.The drain-budget arithmetic from #134 and #102 is untouched, as this issue required.
Not covered, and deliberately left open elsewhere: the exec-based assertion on the built binary's process exit status, raised in this issue under "Separately, and cheaply". That sits outside this definition of done and is not a reason to hold this open.
Closing.