From bfbf416393533dd3a9d58f9930ffdd7d297e8088 Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 20 Aug 2026 04:13:50 +0000 Subject: [PATCH] Exit non-zero when the HTTP listener fails (closes #200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fx OnStart hook returned as soon as the serving goroutine was spawned, so a failed listen was discovered only inside that goroutine. It logged the error and cancelled the server's own context, which nothing outside the goroutine observes: fx reported RUNNING and the process stayed alive with nothing bound. The service was down and looked up, so systemd Restart=on-failure and Docker restart policies never fired. The Server now takes fx.Shutdowner and, on a listen failure, asks it to stop the app with ListenFailureExitCode. Shutdown runs through fx's normal stop sequence, so every OnStop hook — including the HTTP drain and the database close — still executes; the clean-shutdown path is untouched. Server.exitCode goes with it. Its only writer set it to zero during cleanShutdown and its only reader was serve()'s return value, which Run discards; the process status is fx's to decide. Left in place it would have been a genuine data race, since the new path makes cleanShutdown and serve()'s return run concurrently under -race. Verified by TestListenFailure_ShutsDownTheApp, which occupies a port, starts the wired app on it, and requires a non-zero shutdown signal within two seconds. Reverting the fix fails it with "listen failure left the app running". --- internal/server/http.go | 5 +- internal/server/listen_failure_test.go | 95 ++++++++++++++++++++++++++ internal/server/server.go | 44 ++++++++++-- 3 files changed, 134 insertions(+), 10 deletions(-) create mode 100644 internal/server/listen_failure_test.go diff --git a/internal/server/http.go b/internal/server/http.go index 4c99ba5..5413dd9 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -43,10 +43,7 @@ func (s *Server) serveUntilShutdown() { err := s.httpServer.ListenAndServe() if err != nil && !errors.Is(err, http.ErrServerClosed) { s.log.Error("listen error", "error", err) - - if s.cancelFunc != nil { - s.cancelFunc() - } + s.shutdownOnListenFailure() } } diff --git a/internal/server/listen_failure_test.go b/internal/server/listen_failure_test.go new file mode 100644 index 0000000..6b71185 --- /dev/null +++ b/internal/server/listen_failure_test.go @@ -0,0 +1,95 @@ +package server_test + +import ( + "context" + "net" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/globals" + "sneak.berlin/go/webhooker/internal/server" +) + +// listenFailureDeadline is how long the app gets to give up after a +// listen it cannot satisfy. The defect this pins left the process +// reporting RUNNING for 183 seconds with nothing bound; a bind error +// is known instantly, so anything past a moment here is that defect +// back. +const listenFailureDeadline = 2 * time.Second + +// lifecycleTimeout bounds the app's start and stop sequences so a +// wedged hook fails the test instead of hanging it. +const lifecycleTimeout = 15 * time.Second + +// TestListenFailure_ShutsDownTheApp pins that a listener the server +// cannot bind terminates the application with a non-zero status. +// +// The fx OnStart hook returns as soon as the serving goroutine is +// spawned, so a bind failure is discovered after fx has already +// reported RUNNING. Nothing else in the graph observes it, and the +// process used to stay alive with no listener: down, but indis- +// tinguishable from healthy to systemd's Restart=on-failure and to +// Docker's restart policies, which is the state this test exists to +// keep from returning. +// +// The port is occupied by a listener this test holds open, on a +// kernel-chosen port, so the failure is the real EADDRINUSE the +// operator hits when a second instance starts. Loopback is enough to +// collide with the server's wildcard bind: a listening socket on a +// specific address blocks the wildcard from claiming the same port. +func TestListenFailure_ShutsDownTheApp(t *testing.T) { + t.Parallel() + + var listenCfg net.ListenConfig + + occupied, err := listenCfg.Listen( + t.Context(), "tcp", "127.0.0.1:0", + ) + require.NoError(t, err) + + t.Cleanup(func() { _ = occupied.Close() }) + + addr, ok := occupied.Addr().(*net.TCPAddr) + require.True(t, ok, "listener is not TCP") + + // The collaborators come from the wired graph rather than stubs, + // so the Server under test is the one that ships. Only the port + // is test-specific. + env := newTestEnv(t) + env.cfg.Port = addr.Port + + app := fx.New( + fx.NopLogger, + fx.Supply(env.log, env.cfg, env.mw, env.hnd), + fx.Provide(globals.New, server.New), + fx.Invoke(func(*server.Server) {}), + ) + + startCtx, cancelStart := context.WithTimeout( + context.Background(), lifecycleTimeout, + ) + defer cancelStart() + + require.NoError(t, app.Start(startCtx)) + + select { + case sig := <-app.Wait(): + require.Equal( + t, server.ListenFailureExitCode, sig.ExitCode, + "listen failure must exit non-zero", + ) + case <-time.After(listenFailureDeadline): + t.Fatal("listen failure left the app running") + } + + // The stop sequence still has to complete: the fix must reach + // shutdown through fx rather than around it. + stopCtx, cancelStop := context.WithTimeout( + context.Background(), lifecycleTimeout, + ) + defer cancelStop() + + require.NoError(t, app.Stop(stopCtx)) +} diff --git a/internal/server/server.go b/internal/server/server.go index 0bc2f4f..721df5b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -50,6 +50,13 @@ const ( minSentryFlush = 250 * time.Millisecond ) +// ListenFailureExitCode is the status the process exits with when the +// HTTP listener cannot be established, or dies for a reason other +// than a requested shutdown. It must stay non-zero: systemd +// `Restart=on-failure` and Docker's restart policies key off it, and a +// zero exit would read as a deliberate stop. +const ListenFailureExitCode = 1 + // SentryFlushBudget reports how long the Sentry flush may run when // remaining is the time left on the fx stop context after the HTTP // drain. sentry.Flush takes a bare duration and honours no context, @@ -75,13 +82,13 @@ type ServerParams struct { Config *config.Config Middleware *middleware.Middleware Handlers *handlers.Handlers + Shutdowner fx.Shutdowner } // Server is the main HTTP server that wires up routes and manages // graceful shutdown. type Server struct { startupTime time.Time - exitCode int sentryEnabled bool log *slog.Logger cancelFunc context.CancelFunc @@ -159,7 +166,12 @@ func (s *Server) enableSentry() { s.sentryEnabled = true } -func (s *Server) serve() int { +// serve installs the signal watcher, starts the listener and blocks +// until the server's context is cancelled. The process exit status is +// fx's to decide — from a signal, or from the code +// shutdownOnListenFailure hands the Shutdowner — so this reports +// nothing back to its caller. +func (s *Server) serve() { ctx, cancelFunc := context.WithCancel(context.Background()) s.cancelFunc = cancelFunc @@ -185,7 +197,30 @@ func (s *Server) serve() int { <-ctx.Done() // Shutdown is handled by the fx OnStop hook (cleanShutdown). // Do not call cleanShutdown() here to avoid double invocation. - return s.exitCode +} + +// shutdownOnListenFailure ends the application after the HTTP +// listener failed. The fx OnStart hook returns as soon as the serving +// goroutine is spawned, so nothing downstream of it ever learns that +// the listen failed: fx reports RUNNING and the process sits alive +// with nothing bound, which is invisible to systemd and Docker +// restart policies. Asking the Shutdowner to stop the app with a +// non-zero code is what turns that into a visible failure. +// +// The context cancel that follows only unwinds serve()'s own wait. +// The shutdown itself runs through fx's normal stop sequence, so the +// clean-shutdown drain in cleanShutdown is reached unchanged. +func (s *Server) shutdownOnListenFailure() { + err := s.params.Shutdowner.Shutdown( + fx.ExitCode(ListenFailureExitCode), + ) + if err != nil { + s.log.Error("shutdown request failed", "error", err) + } + + if s.cancelFunc != nil { + s.cancelFunc() + } } func (s *Server) cleanupForExit() { @@ -193,9 +228,6 @@ func (s *Server) cleanupForExit() { } func (s *Server) cleanShutdown(ctx context.Context) { - // initiate clean shutdown - s.exitCode = 0 - ctxShutdown, shutdownCancel := context.WithTimeout( ctx, ShutdownTimeout, ) -- 2.49.1