From 5c3c3907d553ad33e5f894228fadc20b845885d1 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sat, 5 Sep 2026 16:42:44 +0800 Subject: [PATCH] =?UTF-8?q?fix=F0=9F=90=9B:=20arm=20the=20stop=20signals?= =?UTF-8?q?=20before=20announcing=20readiness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/api/server.go | 30 ++++++++++++++---------------- cmd/api/signal_test.go | 3 ++- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/cmd/api/server.go b/cmd/api/server.go index de35e0ac..646b988b 100644 --- a/cmd/api/server.go +++ b/cmd/api/server.go @@ -115,6 +115,13 @@ func run() error { } } + // Armed before the server starts serving, and well before the readiness + // banner: a signal arriving between "the process is up" and "the process + // is listening for signals" reaches the default handler and kills it + // without any of the shutdown below. That window is the whole reason + // arming is separate from waiting. + quit, disarmStopSignals := armStopSignals() + go func() { // 服务连接 if config.SslConfig.Enable { @@ -137,7 +144,10 @@ func run() error { fmt.Printf("- Network: %s://%s:%d/swagger/admin/index.html \r\n", "http", pkg.GetLocalHost(), config.ApplicationConfig.Port) fmt.Printf("%s Enter Control + C Shutdown Server \r\n", pkg.GetCurrentTimeStr()) - waitForStopSignal() + <-quit + // Restored here, not deferred: from this point a second signal must reach + // the default handler, so a shutdown that hangs can still be interrupted. + disarmStopSignals() log.Info("Shutdown Server ... ") if err := shutdownServer(srv, shutdownTimeout); err != nil { @@ -156,27 +166,15 @@ func run() error { // - `docker stop` allows 10s by default before it sends SIGKILL. const shutdownTimeout = 5 * time.Second -// waitForStopSignal blocks until the process is asked to stop. +// armStopSignals registers for the stop signals and returns the channel they +// arrive on together with the function that restores the default disposition. // // SIGTERM is what actually arrives in production: `docker stop`, a Kubernetes // pod deletion and `systemctl stop` all send it, and Go terminates the process // immediately for a signal nobody listens for. Registering only os.Interrupt -// meant every graceful shutdown below this line was dead code outside a +// meant every graceful shutdown below the wait was dead code outside a // terminal. // -// The disposition is restored before returning, so a second signal kills the -// process the default way. The channel keeps the notification we already took, -// so without this a stuck shutdown would swallow every further signal - once -// SIGTERM is registered there is no escape hatch left but SIGKILL. -func waitForStopSignal() os.Signal { - quit, disarm := armStopSignals() - defer disarm() - return <-quit -} - -// armStopSignals registers for the stop signals and returns the channel they -// arrive on together with the function that restores the default disposition. -// // Registering is separate from waiting so a caller can arm before it announces // that it is ready: a signal that arrives between the two is delivered to the // default handler, which for both of these means the process dies without diff --git a/cmd/api/signal_test.go b/cmd/api/signal_test.go index 604b27c3..9d0f6a3f 100644 --- a/cmd/api/signal_test.go +++ b/cmd/api/signal_test.go @@ -15,7 +15,7 @@ import ( // The signal path cannot be exercised in-process: delivering a signal to the // test binary would race with the test framework, and the disposition changes // are global. So the test re-executes itself as a child, and the child runs the -// same waitForStopSignal / shutdownServer the server does. +// same armStopSignals / shutdownServer the server does. // // The child deliberately serves an empty http.Server rather than the real one: // this repository's CI has no database (.github/workflows/go.yml runs neither @@ -77,6 +77,7 @@ func TestSignalChild(t *testing.T) { timeout := shutdownTimeout if os.Getenv(childHangConn) == "1" { + // A connection that has sent nothing keeps Shutdown busy: net/http // only treats a StateNew connection as idle once it is more than five // seconds old. A short budget makes the timeout deterministic without