Exit non-zero when the HTTP listener fails (closes #200)
All checks were successful
check / check (push) Successful in 6m20s
All checks were successful
check / check (push) Successful in 6m20s
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".
This commit is contained in:
@@ -43,10 +43,7 @@ func (s *Server) serveUntilShutdown() {
|
|||||||
err := s.httpServer.ListenAndServe()
|
err := s.httpServer.ListenAndServe()
|
||||||
if err != nil && !errors.Is(err, http.ErrServerClosed) {
|
if err != nil && !errors.Is(err, http.ErrServerClosed) {
|
||||||
s.log.Error("listen error", "error", err)
|
s.log.Error("listen error", "error", err)
|
||||||
|
s.shutdownOnListenFailure()
|
||||||
if s.cancelFunc != nil {
|
|
||||||
s.cancelFunc()
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
95
internal/server/listen_failure_test.go
Normal file
95
internal/server/listen_failure_test.go
Normal file
@@ -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))
|
||||||
|
}
|
||||||
@@ -50,6 +50,13 @@ const (
|
|||||||
minSentryFlush = 250 * time.Millisecond
|
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
|
// SentryFlushBudget reports how long the Sentry flush may run when
|
||||||
// remaining is the time left on the fx stop context after the HTTP
|
// remaining is the time left on the fx stop context after the HTTP
|
||||||
// drain. sentry.Flush takes a bare duration and honours no context,
|
// drain. sentry.Flush takes a bare duration and honours no context,
|
||||||
@@ -75,13 +82,13 @@ type ServerParams struct {
|
|||||||
Config *config.Config
|
Config *config.Config
|
||||||
Middleware *middleware.Middleware
|
Middleware *middleware.Middleware
|
||||||
Handlers *handlers.Handlers
|
Handlers *handlers.Handlers
|
||||||
|
Shutdowner fx.Shutdowner
|
||||||
}
|
}
|
||||||
|
|
||||||
// Server is the main HTTP server that wires up routes and manages
|
// Server is the main HTTP server that wires up routes and manages
|
||||||
// graceful shutdown.
|
// graceful shutdown.
|
||||||
type Server struct {
|
type Server struct {
|
||||||
startupTime time.Time
|
startupTime time.Time
|
||||||
exitCode int
|
|
||||||
sentryEnabled bool
|
sentryEnabled bool
|
||||||
log *slog.Logger
|
log *slog.Logger
|
||||||
cancelFunc context.CancelFunc
|
cancelFunc context.CancelFunc
|
||||||
@@ -159,7 +166,12 @@ func (s *Server) enableSentry() {
|
|||||||
s.sentryEnabled = true
|
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())
|
ctx, cancelFunc := context.WithCancel(context.Background())
|
||||||
s.cancelFunc = cancelFunc
|
s.cancelFunc = cancelFunc
|
||||||
|
|
||||||
@@ -185,7 +197,30 @@ func (s *Server) serve() int {
|
|||||||
<-ctx.Done()
|
<-ctx.Done()
|
||||||
// Shutdown is handled by the fx OnStop hook (cleanShutdown).
|
// Shutdown is handled by the fx OnStop hook (cleanShutdown).
|
||||||
// Do not call cleanShutdown() here to avoid double invocation.
|
// 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() {
|
func (s *Server) cleanupForExit() {
|
||||||
@@ -193,9 +228,6 @@ func (s *Server) cleanupForExit() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (s *Server) cleanShutdown(ctx context.Context) {
|
func (s *Server) cleanShutdown(ctx context.Context) {
|
||||||
// initiate clean shutdown
|
|
||||||
s.exitCode = 0
|
|
||||||
|
|
||||||
ctxShutdown, shutdownCancel := context.WithTimeout(
|
ctxShutdown, shutdownCancel := context.WithTimeout(
|
||||||
ctx, ShutdownTimeout,
|
ctx, ShutdownTimeout,
|
||||||
)
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user