/ready has failed from the moment shutdown begins since the readiness probe was
added, and the order it does that in is right: reversed, the state would be
reported after the connections were already cut. But order alone does not
produce a window. Nothing waited between the flip and Shutdown, so the two were
microseconds apart, and a poller on a multi-second interval never saw the 503 -
it saw a refused connection, which is the thing the probe was supposed to
avoid. Polling a container through a SIGTERM on the demo host recorded exactly
that: 200, then connection refused, and no 503 in between.
extend.shutdown.drain is that wait. The process keeps serving normally for it -
answering requests, not refusing them, because refusing them would move the
outage earlier rather than avoid it - and only then closes the listener.
It is zero by default, so nothing changes for a deployment that does not ask
for it. That is not timidity: the budgets are spent one after another, and a
non-zero default would push every existing shutdown closer to the orchestrator's
grace period, where being cut off part-way through the cleanup callbacks is
worse than never draining at all.
Keep-alive is switched off with the flip. The server keeps connections alive
until Shutdown sets shuttingDown() itself, so without this the pooled
connections a balancer holds would sit untouched for the whole window and be
cut at the end of it anyway - the cost of the window without its benefit. This
is the switch Shutdown flips, moved earlier by the window's length.
The signal disposition is restored after the window rather than on the first
signal. Before there was a window, the interval where a second signal killed
the process outright was only reachable while a cleanup callback hung; putting
a multi-second wait inside it would have made every ordinary shutdown
interruptible for the length of the drain. A second signal during the window is
taken by the channel and ends the window early instead - somebody sending
another kill wants this over with sooner - and the escape hatch comes back the
moment the window does.
What the window is worth depends on who removes this instance. A balancer that
polls /ready acts on the 503 and needs the window to cover its check interval
times its failure threshold; a Kubernetes Service withdraws the endpoint when
the Pod is deleted, concurrently with SIGTERM and regardless of what the probe
returns, and there the window covers the delay in that removal reaching every
node. The three comments that used to say a balancer "has a chance to" take the
instance out said it without either qualification, which is how a claim comes to
be repeated after a live test has refuted it.
The subprocess test polls the real probes on a connection it opens after the
signal - a reused one can be served after the listener is closed, which would
let this pass against a shutdown that had already broken it - and asserts on the
draining answer in the body, not on the status code. With no database the status
is 503 from start-up, so a status-code assertion would hold even with
BeginDraining deleted. Two window lengths, because one proves only that
something takes that long.
The child process built a server, restored its own signal disposition and
called shutdownServer and runShutdownHooks itself, in an order it chose. It
never called anything run() calls. So the assertions were about a copy of the
sequence: move a step in the real one, or drop it, and every test here stays
green. The acceptance criteria these back are worth exactly as much as that.
The child now calls gracefulShutdown and asserts on what comes out of it. The
budget it spends is defaultBudget with one field shortened where a test needs a
deterministic timeout, which is also how the two waits stop being wired by
hand.
The stuck-shutdown case changes shape as a result. It used to sleep inside the
child, between the steps it had copied; there is no "between" to sleep in any
more, so it registers a BeforeExit callback that never returns and gives it a
budget long enough to hang on. That is where a shutdown actually hangs, and it
now runs through the same function - which means this test also pins where the
signal disposition is restored, rather than just asserting that the child dies.
It signals repeatedly rather than once. The marker is printed immediately
before gracefulShutdown is entered, so a single signal sent on seeing it can
still arrive before the disposition is restored, land in the buffered channel
and be dropped. Which signal does the killing is not the assertion; that one of
them can is.
A module can now register cleanup and have it happen. Until this commit the
process stopped serving and returned; anything a module had set up went down
with the process rather than being taken down.
BeginShutdown is said first, before anything is dismantled. Without it a
configuration reload arriving in this window re-runs AfterResource - rebuilding
the pool and the queue adapter and re-registering consumers - on top of cleanup
that has already run.
The cleanup runs whether or not Shutdown reported an error, which is the whole
reason that error stopped being fatal in the first place: Shutdown fails exactly
when connections were still in flight, and that is when there is most left to
take down.
The two budgets are spent one after the other, so what has to fit inside the
orchestrator's grace period is their sum. `docker stop` allows 10s by default
before SIGKILL; 5+3 leaves room to finish returning. Raising one without
lowering the other buys nothing.
Both halves are tested through the existing subprocess child, which now
registers a BeforeExit callback of its own:
- after a Shutdown that timed out, the callback still runs. Moving the call
into the success branch reports "the BeforeExit callback did not run after
a failed Shutdown".
- a callback that outlasts its budget is abandoned, not awaited. It sleeps
two seconds against a 300ms budget; RunShutdown reports the deadline, the
process exits cleanly inside one second, and the callback's own marker
never appears. Widening the budget to five seconds makes the test time out
waiting for the exit, which is what "awaited" looks like.
Both counter-proofs compile and fail.
Moving the dial to just before the shutdown removed one flake and introduced
another: Shutdown only waits for connections the server has already accepted,
so calling it in the gap between the dial and the accept finds nothing to
wait for and returns cleanly. The test then fails on its own "this proves
nothing" guard - which it did, after passing once.
A ConnState hook closes both gaps deterministically. The connection is opened
late enough not to age past the five seconds net/http stops counting it at,
and the child does not proceed until the server has taken it off the
listener.
Ran five times in a row rather than once, because a single green run is what
made the previous version look fixed.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
net/http stops counting a StateNew connection against Shutdown once it is
more than five seconds old. The connection was opened when the child started
and the parent then waited for readiness before signalling, so on a slow run
the connection could age past that mark and Shutdown would succeed - and the
test would fail on its own "this proves nothing" guard rather than on the
behaviour it is there to pin.
Opening it immediately before the shutdown keeps the timeout deterministic.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The previous commit split arming from waiting so a caller could arm first,
wrote a comment saying a signal landing in between reaches the default
handler and kills the process, used it that way in the subprocess test - and
then left run() calling the combined helper after the whole readiness banner.
The window it warned about was still there in the one place that ships.
The signals are now armed before the server starts serving, and the wait
happens where it did. The disposition is restored right after the first
signal rather than deferred, so a shutdown that hangs can still be
interrupted by a second one.
waitForStopSignal goes away: run() was its only caller, and what was worth
keeping from its comment is now on armStopSignals.
Note that no test covers this ordering. The subprocess test drives
armStopSignals directly, which is what makes it a test of the mechanism
rather than of run(); moving the call back below the banner leaves it green.
Verified by reading the sequence in run(), not by a failing test.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Three defects on one path, none of which could be seen from the code alone.
**SIGTERM was never registered.** signal.Notify listened for os.Interrupt
only, and Go terminates the process outright for a signal nothing handles.
`docker stop`, a Kubernetes pod deletion and `systemctl stop` all send
SIGTERM, so every line of the graceful shutdown below the wait was dead code
outside a terminal: measured on a real binary, SIGINT printed "Shutdown
Server ..." and "Server exiting" and SIGTERM printed neither.
**A stuck shutdown could not be interrupted.** quit is buffered and
signal.Notify stays armed after the first delivery, so further signals only
refill the buffer. That was harmless while SIGTERM went to the default
handler - it was the escape hatch. Registering it removes the hatch, so the
disposition is now restored once the first signal is taken, and a second one
kills the process the default way. Arming is split from waiting so a caller
can arm before it announces readiness; a signal in between reaches the
default handler, which is the very failure being fixed.
**A failed Shutdown skipped everything after it.** log.Fatal is an
unconditional os.Exit(1), and Shutdown reports an error precisely when
connections were still in flight - the moment the cleanup that follows
matters most. It is an error now, and the process carries on.
That failure is closer than it looks. net/http only treats a StateNew
connection as idle once it is over five seconds old, so a connection opened
shortly before the signal that has sent nothing holds the whole budget: with
the shipped settings.yml (readtimeout 1) the server closes it first and
shutdown takes 5ms, but with settings.demo.yml (readtimeout 10000) the same
connection made shutdown take 5.04s and exit 1, printing no "Server
exiting". The default configuration is what has been hiding this.
The wait and the shutdown are extracted so the subprocess tests can drive the
real functions against an empty http.Server: CI has no database, and none of
this needs one.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
zhangwenjian