fix(server): graceful shutdown drains held requests; bind failures terminate the process#1020
Merged
Merged
Conversation
…nd errors Shutdown called server.Close(), which resets every active connection. The blocking strategy holds requests open up to its configured timeout by design, so every restart cut those requests off mid-wait while the health endpoint was dutifully reporting 503 to drain traffic. The server now uses server.Shutdown with a drain window of max(15s, blocking default timeout + 5s grace), then force-closes whatever remains. server.Start also returned nothing: a ListenAndServe failure (port already in use, bad address) was only logged, and the process kept running all its watchers with no HTTP listener, looking healthy while serving nothing. Start now returns the serve error and sabliercmd terminates with it. The dead 15-second timeout context in sabliercmd.Start (created, deferred, used by nothing - the fossil of an intended drain that was never wired) is replaced by actually waiting for the drain to complete before saving state and exiting. The drain regression test fails against the Close-based implementation (client gets EOF) and passes with the drain; the bind-failure test pins that Start returns instead of hanging.
Test Results✅ All tests passed! | 763 tests in 100.232s |
|
|
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
1 participant
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.
Finding
Three related lifecycle defects in the HTTP server, found during a design review of the wiring layer:
1. "Graceful" shutdown resets live connections. server.go says
// Graceful web server shutdown.and then callsserver.Close()- which is the opposite: it immediately terminates every active connection. Sablier's blocking strategy holds requests open by design (up tostrategy.blocking.default-timeout, 1m by default), so every restart/deploy of Sablier reset the very requests it exists to hold, surfacing as 502s at the reverse proxy. The irony: the health endpoint already flips to 503 on shutdown to drain traffic - infrastructure built for draining, defeated byClose().2. Port-conflict zombie.
ListenAndServeerrors were only logged (server failed to start) and swallowed.sablier starton a busy port kept running all its watchers with no HTTP listener - the process looks healthy indocker pswhile serving nothing, forever.3. Dead code confirming the intent.
sabliercmd.Startcreates a 15-second timeout context on shutdown (start.go:194) that is used by nothing - the fossil of a graceful drain that was never wired.Discovery
The drain regression test holds a blocking request inside a mocked session call, cancels the server context mid-request, and asserts the response completes. Against the
Close()-based implementation:The client gets a connection reset exactly as a reverse-proxy plugin would during a deploy.
Fix
server.Startnow returns an error and owns the full lifecycle:ListenAndServefailures return immediately (killing the zombie mode), and on ctx cancellation it drains withserver.Shutdownusing a window ofmax(15s, blocking default timeout + 5s grace)- derived from existing config, no new option - then force-closes whatever remains.sabliercmd.Startwaits for the drain to complete before saving state and exiting, and propagates a serve error as a non-zero exit. The dead 15s context is gone.StartHttp(only used by the old path) is removed.Tests
TestStartDrainsInFlightRequestsOnShutdown- deterministic (the request is provably in flight when shutdown starts); fails onClose(), passes on drain.TestStartReturnsOnServeError- bind conflict returns an error instead of hanging.TestDrainTimeout- pins the drain-window policy.internal/server,internal/api,pkg/sabliercmdtests andgolangci-lintpass.Docs
No config surface changed (the drain window derives from the existing blocking timeout). There is currently no docs page describing server lifecycle/shutdown behavior; the policy is documented in code. Happy to add an ops note to the docs in a follow-up if wanted.