Graceful shutdown: SIGTERM, in-flight request draining, /ready health semantics, clean pool close - #3
Merged
Merged
Conversation
signal.Notify only registered os.Interrupt, so a raw SIGTERM — the default stop signal of Docker, Kubernetes and systemd — took Go's default disposition: immediate process termination. In-flight requests were cut mid-response and PostgreSQL connections reset on every stop from an orchestrator that was not configured to send SIGINT instead. Register SIGTERM alongside SIGINT so both enter the same graceful shutdown path.
Run() treated http.ErrServerClosed — the sentinel the net/http package returns after a clean Shutdown() — like any other startup error and called os.Exit(1). Supervisors (systemd, Kubernetes, docker) read a non-zero status as a crash, so every clean stop was reported as a failure. Return normally instead, letting main exit with status 0.
The context passed to http.Server.Shutdown was hardcoded to 1 second, while ReadTimeout/WriteTimeout allow requests to run for 60: every restart abandoned any in-flight request slower than ~1s (bulk selects, large upserts, RPC calls), the single biggest obstacle to zero-downtime rolling deploys. Shutdown now waits for in-flight requests indefinitely by default — they are already bounded per-request by ReadTimeout/WriteTimeout, and the supervisor's stop grace period is the final backstop (the same model PostgREST adopted in PostgREST/postgrest#4702). A new GracefulShutdownTimeout config key (seconds, 0 = wait until done) bounds the wait for deployments that want a hard cap below their stop grace period. Since the wait is no longer trivially short, the surrounding paths are hardened too: - a second signal during the graceful wait forces an immediate exit, so a stuck drain never needs SIGKILL; - Shutdown() is idempotent (sync.Once) — the signal path installed by Run and an embedder may both call it without a double-close panic; - an expired graceful window is logged before the remaining connections are force-closed.
…window
/live and /ready were registered to the same unconditional 200 handler,
so they answered the same question. They are different probes: liveness
means "the process is up, don't kill it", readiness means "route
traffic to it" — and readiness must start failing when a shutdown
begins, which is how load balancers and k8s-style rolling updates
deregister an instance before it stops accepting connections.
On shutdown the server now:
1. flips /ready to 503 {"status":"draining"} (via a new IsDraining
flag), while /live keeps answering 200 — a draining process is
alive precisely because it is shutting down gracefully;
2. on SIGTERM, optionally keeps serving for DrainDelay seconds (new
config key, default 0 = disabled), so the balancer can observe the
503 and deroute the instance while it still accepts connections.
An interactive Ctrl-C (SIGINT) skips the delay and a second signal
cuts it short: the window exists for orchestrated stops. Same
semantics as PostgREST's server-shutdown-wait-period
(PostgREST/postgrest#4580), the k8s preStop-sleep pattern
internalized;
3. proceeds with the graceful shutdown, bounded by
GracefulShutdownTimeout if set.
Embedders calling Shutdown() directly get the readiness flip as well.
CI already exports it for make test; the database package was the only test suite ignoring it, hardcoding localhost:5432. This makes the suite runnable against any local instance (e.g. a dedicated docker container on a different port).
Pool closing was commented out ("@@ to be fixed, now blocks"), so the
process exited with every pgx pool open and the listener's dedicated
connection up: Postgres saw abrupt connection resets on each stop, and
connection slots lingered until TCP teardown.
Two things made Close() block:
- pgxpool.Pool.Close() waits for acquired connections, and with the old
1-second shutdown window HTTP.Shutdown regularly gave up while
handlers were still running and holding connections.
- NotificationListener.Stop() only closed a channel that the listen
loop checked between notifications, while the loop normally sits
blocked inside WaitForNotification — the goroutine (and its
connection) survived Stop.
Now the listener's Stop cancels the loop context, which interrupts
WaitForNotification, closes the connection cleanly with a bounded
fresh context (so Postgres receives a Terminate message), and waits
for the goroutine to exit — also when racing another Stop.
Server.Shutdown then closes the engine after the HTTP drain completes
— at that point no handler holds a connection — bounding the wait by
the shutdown context (plus a 5s backstop when it has no deadline) so
an expired drain window cannot hang the process past the supervisor's
grace period.
The shutdown tests live in ./server, which make test (and therefore CI) did not run. The pre-existing TLS valid-cert test is skipped when the untracked dev certificate is absent, so the package passes in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
smoothdb currently doesn't survive a standard rolling deploy (or a plain restart) without dropping work:
ReadTimeout/WriteTimeoutallow requests to run for 60 — so every restart killed any request slower than ~1s./liveand/readywere the same unconditional 200 handler, so a load balancer had no way to deregister an instance before its listener closed.s.DBE.Close()was commented out as "now blocks"), so Postgres saw connection resets on every stop.PostgREST recently addressed the same gaps — shutdown waiting for in-flight requests (PostgREST/postgrest#4689, fixed in #4702) and a pre-shutdown wait for load-balancer deregistration (#4580,
server-shutdown-wait-period). This PR follows the same model, and where they differ, kube-apiserver's--shutdown-delay-duration/ traefik'srequestAcceptGraceTimeoutshape.What changes
One commit per fix/feature, in dependency order:
fix: shut down gracefully on SIGTERM, not only SIGINT— registers SIGTERM alongside SIGINT, synchronously inRun()before the listener starts (so an early signal can't hit the default disposition).fix: exit 0 after a graceful shutdown—http.ErrServerClosedis a clean stop, not an error.feat: wait for in-flight requests on shutdown, with optional bound— shutdown now waits for in-flight requests indefinitely by default (they're already bounded per-request byReadTimeout/WriteTimeout; the supervisor's stop grace period is the final backstop — same model as PostgREST). A newGracefulShutdownTimeoutconfig key (seconds,0= wait until done) bounds the wait. Because the wait is no longer trivially short: a second signal forces an immediate exit,Shutdown()is idempotent (sync.Once), and an expired graceful window is logged.feat: report draining on /ready during shutdown, with optional drain window—/readyflips to503 {"status":"draining"}as soon as shutdown begins while/livekeeps answering 200 (the standard probe contract). A newDrainDelayconfig key (seconds,0= disabled) keeps the listener serving that long after readiness flips, so load balancers can observe the 503 and deregister before connections start being refused — the k8s preStop-sleep pattern internalized. The delay applies to SIGTERM only (orchestrated stops); an interactive Ctrl-C skips it, and a second signal cuts it short. Same semantics as PostgREST'sserver-shutdown-wait-period.test: honor SMOOTHDB_DATABASE_URL in database package tests— CI already exports it; this was the only suite ignoring it.fix: close database pools and notification listener on shutdown— un-comments the pool close and fixes both reasons it used to block: the 1s window abandoned handlers still holding connections, andNotificationListener.Stop()didn't interrupt a pendingWaitForNotification(the goroutine and its dedicated connection survivedStop). The listener now cancels its loop context, closes the connection with a bounded fresh context (Postgres receives a properTerminateinstead of a reset), andStopwaits for the goroutine to exit.Server.Shutdowncloses the engine after the HTTP drain, bounded by the shutdown context plus a 5s backstop.test: run the server package in make test— the new tests live in./server, whichmake test(and CI) didn't run.New config keys
GracefulShutdownTimeout0= wait until done)0DrainDelay/readystarts reporting 503 on a SIGTERM shutdown (0= disabled)0Both default to inert values: with an empty config, behavior only changes in that shutdown is now actually graceful.
Shutdown sequence (with both keys set)
Compatibility notes
api.Helpergained anIsDraining() boolmethod — a compile-time break for external implementers of that interface (in-repo,*server.Serveris the only one).api.HealthHandlerwas replaced byLiveHandlerandReadyHandler(Helper).stopHandlernow takes the signal channel (unexported, no API impact).Testing
server/shutdown_test.go(run with-race): SIGTERM graceful path + pools closed, in-flight request completing across shutdown,/ready503 +/live200 during the drain window, SIGINT skipping the drain, a second signal skipping the drain, and a listenerStop-interrupts-WaitForNotificationtest indatabase/.SMOOTHDB_DATABASE_URL=... make test(CI-equivalent env; verified on Postgres 16).kill -TERM→ drain → "Stopped." → exit 0; second SIGTERM during a 30s drain window → immediate graceful stop.Out of scope / follow-ups
/readyverifying the main connection pool (PostgREST checks pool + schema cache state) — natural next step.MAINTAINprivilege (m), soTestGrantsfails on Postgres 17; andInitDbEnginewrites the package-globaldbe, which races when two engines exist in one process.