SIGTERM during startup panics on a nil httpServer, and cleanShutdown reads two fields with no happens-before edge #226

New Issue

clawbot · 2026-08-20T06:42:47+02:00

clawbot commented

2026-08-20 06:42:47 +02:00

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.httpServer is assigned inside the serveUntilShutdown goroutine (internal/server/http.go:29), but cleanShutdown calls s.httpServer.Shutdown(ctxShutdown) unguarded (internal/server/server.go:236). A SIGTERM arriving between the OnStart hook returning and that assignment panics on a nil *http.Server.

The window is not theoretical: it spans configure() and enableSentry(), and the latter performs a network sentry.Init when SENTRY_DSN is set. A container killed early in a crash-loop or a fast redeploy lands in it.

2. No happens-before edge on s.httpServer and s.sentryEnabled. On the SIGTERM path, and on a direct app.Stop(), those fields are written by the serve goroutine and read by cleanShutdown with no synchronisation. The race detector has never flagged it because the existing internal/server tests never call serve(). The path added by #218 IS race-free — the Shutdowner channel supplies the edge — which is precisely why -race stayed 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:

cleanShutdown reads httpServer and sentryEnabled under a mutex, or behind a "server ready" handshake
a test calls app.Stop() immediately after app.Start() and passes under -race
no change to the clean-shutdown drain budget: the stopTimeout / ShutdownTimeout / TailHookReserve arithmetic pinned by #134 and #102 stays intact

Separately, 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. Rewriting main as Start/Done/Stop would regress #200 silently with that test still green. Worth an exec-based assertion on the built binary.

Found during the review of https://git.eeqj.de/sneak/webhooker/pulls/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.httpServer` is assigned inside the `serveUntilShutdown` goroutine (`internal/server/http.go:29`), but `cleanShutdown` calls `s.httpServer.Shutdown(ctxShutdown)` unguarded (`internal/server/server.go:236`). A SIGTERM arriving between the `OnStart` hook returning and that assignment panics on a nil `*http.Server`. The window is not theoretical: it spans `configure()` and `enableSentry()`, and the latter performs a network `sentry.Init` when `SENTRY_DSN` is set. A container killed early in a crash-loop or a fast redeploy lands in it. **2. No happens-before edge on `s.httpServer` and `s.sentryEnabled`.** On the SIGTERM path, and on a direct `app.Stop()`, those fields are written by the serve goroutine and read by `cleanShutdown` with no synchronisation. The race detector has never flagged it because the existing `internal/server` tests never call `serve()`. The path added by https://git.eeqj.de/sneak/webhooker/pulls/218 IS race-free — the Shutdowner channel supplies the edge — which is precisely why `-race` stayed 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: - `cleanShutdown` reads `httpServer` and `sentryEnabled` under a mutex, or behind a "server ready" handshake - a test calls `app.Stop()` immediately after `app.Start()` and passes under `-race` - no change to the clean-shutdown drain budget: the `stopTimeout` / `ShutdownTimeout` / `TailHookReserve` arithmetic pinned by https://git.eeqj.de/sneak/webhooker/issues/134 and https://git.eeqj.de/sneak/webhooker/issues/102 stays intact Separately, and cheaply: nothing in the repo tests that the process exit STATUS is non-zero on listen failure — the test added by https://git.eeqj.de/sneak/webhooker/pulls/218 asserts fx's `ShutdownSignal.ExitCode`, not the process's. Rewriting `main` as `Start`/`Done`/`Stop` would regress https://git.eeqj.de/sneak/webhooker/issues/200 silently with that test still green. Worth an exec-based assertion on the built binary.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#226