Set fx.StopTimeout inside the container stop grace (closes #134)
All checks were successful
check / check (push) Successful in 3m3s
All checks were successful
check / check (push) Successful in 3m3s
fx defaults to a 15s stop timeout and the Dockerfile sets no grace override, so Docker SIGKILLed at 10s and the bounded shutdown #130 built — including the log line that tells an operator a component is wedged — was unreachable in the image this repo produces. Sets fx.StopTimeout to 5s, and lowers the HTTP drain to 3s so a full-length drain no longer exhausts the whole sequence budget and skip every later hook, database close included. The Sentry flush, which runs in the same hook and honours no context, is clamped to the remaining stop budget less a 2s tail reserve, so a stalled flush drops Sentry events rather than the database close. Also fixes a latent coin flip in the shared stop-hook waiter, which reported "shutdown timed out" about half the time for a component that drained cleanly against an already-expired context. Independently reviewed three times. The final reviewer derived a stronger invariant than the implementation claims — the server hook's absolute end is bounded at stopTimeout minus the reserve regardless of drain length or of time consumed by preceding hooks — and confirmed the guard's 10ms sweep cannot step over the maximum, since both breakpoints land on its grid. Both Sentry probe arms, the docker stop demo and every mutation were reproduced independently. Known residual, filed separately: the HTTP drain itself is not clamped by the reserve, so slow preceding hooks can still jointly exhaust the budget. Demonstrated with a 2.2s sweeper delay.
This commit was merged in pull request #159.
This commit is contained in:
@@ -24,15 +24,48 @@ import (
|
||||
)
|
||||
|
||||
const (
|
||||
// shutdownTimeout is the maximum time to wait for the HTTP
|
||||
// ShutdownTimeout is the maximum time to wait for the HTTP
|
||||
// server to finish in-flight requests during shutdown.
|
||||
shutdownTimeout = 5 * time.Second
|
||||
//
|
||||
// It must stay strictly below the fx stop timeout in
|
||||
// cmd/webhooker, which bounds the whole stop sequence: a drain
|
||||
// that used the entire sequence budget would leave nothing for
|
||||
// the hooks that run after the server, including the database
|
||||
// close. It is exported so that relationship can be tested.
|
||||
ShutdownTimeout = 3 * time.Second
|
||||
|
||||
// sentryFlushTimeout is the maximum time to wait for Sentry
|
||||
// to flush pending events during shutdown.
|
||||
// TailHookReserve is the share of the fx stop budget this hook
|
||||
// refuses to spend, leaving it for the hooks that run after the
|
||||
// server: the delivery engine, the healthcheck, the webhook DB
|
||||
// manager and the database close.
|
||||
TailHookReserve = 2 * time.Second
|
||||
|
||||
// sentryFlushTimeout is the longest wait for Sentry to flush
|
||||
// pending events during shutdown, before the remaining stop
|
||||
// budget is taken into account.
|
||||
sentryFlushTimeout = 2 * time.Second
|
||||
|
||||
// minSentryFlush is the shortest flush worth attempting. Below
|
||||
// it the remaining budget goes to the tail hooks instead.
|
||||
minSentryFlush = 250 * time.Millisecond
|
||||
)
|
||||
|
||||
// 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,
|
||||
// so this clamp is the only thing keeping a stalled flush from
|
||||
// spending the tail hooks' share of the budget on top of a
|
||||
// full-length drain. TailHookReserve is held back, and anything
|
||||
// under minSentryFlush is skipped rather than attempted uselessly.
|
||||
func SentryFlushBudget(remaining time.Duration) time.Duration {
|
||||
budget := min(remaining-TailHookReserve, sentryFlushTimeout)
|
||||
if budget < minSentryFlush {
|
||||
return 0
|
||||
}
|
||||
|
||||
return budget
|
||||
}
|
||||
|
||||
//nolint:revive // ServerParams is a standard fx naming convention.
|
||||
type ServerParams struct {
|
||||
fx.In
|
||||
@@ -164,7 +197,7 @@ func (s *Server) cleanShutdown(ctx context.Context) {
|
||||
s.exitCode = 0
|
||||
|
||||
ctxShutdown, shutdownCancel := context.WithTimeout(
|
||||
ctx, shutdownTimeout,
|
||||
ctx, ShutdownTimeout,
|
||||
)
|
||||
defer shutdownCancel()
|
||||
|
||||
@@ -178,10 +211,31 @@ func (s *Server) cleanShutdown(ctx context.Context) {
|
||||
s.cleanupForExit()
|
||||
|
||||
if s.sentryEnabled {
|
||||
sentry.Flush(sentryFlushTimeout)
|
||||
s.flushSentry(ctx)
|
||||
}
|
||||
}
|
||||
|
||||
// flushSentry drains Sentry's queue inside what is left of the fx
|
||||
// stop budget. A context carrying no deadline — a caller outside the
|
||||
// fx lifecycle — gets the full timeout.
|
||||
func (s *Server) flushSentry(ctx context.Context) {
|
||||
flush := sentryFlushTimeout
|
||||
|
||||
if deadline, ok := ctx.Deadline(); ok {
|
||||
flush = SentryFlushBudget(time.Until(deadline))
|
||||
}
|
||||
|
||||
if flush <= 0 {
|
||||
s.log.Warn(
|
||||
"skipping sentry flush, stop budget exhausted",
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
sentry.Flush(flush)
|
||||
}
|
||||
|
||||
func (s *Server) configure() {
|
||||
// identify ourselves in the logs
|
||||
s.params.Logger.Identify()
|
||||
|
||||
59
internal/server/shutdown_test.go
Normal file
59
internal/server/shutdown_test.go
Normal file
@@ -0,0 +1,59 @@
|
||||
package server_test
|
||||
|
||||
import (
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/webhooker/internal/server"
|
||||
)
|
||||
|
||||
// TestSentryFlushBudget covers the clamp that keeps the Sentry flush
|
||||
// from spending the tail hooks' share of the fx stop budget.
|
||||
// sentry.Flush ignores the stop context, so without the clamp a
|
||||
// stalled flush adds its whole timeout on top of the HTTP drain.
|
||||
func TestSentryFlushBudget(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
remaining time.Duration
|
||||
want time.Duration
|
||||
}{
|
||||
{
|
||||
name: "full drain leaves only the reserve",
|
||||
remaining: server.TailHookReserve,
|
||||
want: 0,
|
||||
},
|
||||
{
|
||||
name: "expired budget",
|
||||
remaining: -time.Second,
|
||||
want: 0,
|
||||
},
|
||||
{
|
||||
name: "sliver above the reserve is not worth it",
|
||||
remaining: server.TailHookReserve + 10*time.Millisecond,
|
||||
want: 0,
|
||||
},
|
||||
{
|
||||
name: "partial flush when some room is left",
|
||||
remaining: server.TailHookReserve + time.Second,
|
||||
want: time.Second,
|
||||
},
|
||||
{
|
||||
name: "capped at the nominal timeout",
|
||||
remaining: time.Hour,
|
||||
want: 2 * time.Second,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
require.Equal(
|
||||
t, tt.want, server.SentryFlushBudget(tt.remaining),
|
||||
)
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user