Set fx.StopTimeout inside the container stop grace (closes #134)
All checks were successful
check / check (push) Successful in 2m58s
All checks were successful
check / check (push) Successful in 2m58s
fx defaults the stop timeout to 15s and the Dockerfile sets no STOPSIGNAL or grace override, so Docker's 10s default SIGKILLs the process five seconds before the bound can fire. Everything gated on it — including the "shutdown timed out, goroutines still running" error log that tells an operator a component is wedged — was unreachable in the image this repo produces. Set fx.StopTimeout to 5s: inside the grace with headroom for signal delivery and process exit. The option set moves into newApp() so a test can read (*fx.App).StopTimeout() back and pin it against drift; dropping the option makes that test report fx's 15s default. Lower the HTTP drain budget (server.ShutdownTimeout) from 5s to 3s. fx bounds the whole stop sequence and returns without running its remaining hooks once the stop context expires, so two equal values meant a drain that used its full budget exhausted the sequence budget at that instant and skipped every later hook — the delivery engine, the healthcheck, the webhook DB manager and the database close — in exactly the case where the drain mattered. The tail hooks are microsecond-scale in normal operation, so 2s of remaining budget is ample, and holding the total at 5s keeps a wide margin under Docker's 10s grace. The constant is exported so TestStopTimeout_LeavesHeadroomForTailHooks can pin the relationship and fail on a future edit to either value. This does not make the database close unconditional: the ArchiveSweeper and RetentionReaper hooks run before the server and can still consume the whole budget. Also fix a latent coin flip in the shared stop-hook waiter. It selected on the drained channel against ctx.Done() with no preamble, and select picks uniformly among ready cases, so a component that drained against an already-expired context reported a timeout about half the time. Not reachable through fx, which re-checks ctx.Err() before each hook, but the helper is shared and a direct caller can reach it. waitDone now settles the drained case in a non-blocking preamble first; the test drives it over 1000 passes, so a restored coin flip cannot pass by luck. README records the real stop-hook order (ArchiveSweeper, RetentionReaper, server, delivery.Engine, healthcheck, WebhookDBManager, database close), the two timeouts and their relationship, and the container stop grace: that lowering the grace below the bound puts SIGKILL back in front of it, and that an expired stop context makes fx skip its remaining hooks, so a wedge in the first-stopped component means the database close never runs. Adds the missing internal/lifecycle/ entry to the Package Layout tree.
This commit is contained in:
@@ -2,6 +2,8 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"time"
|
||||
|
||||
"go.uber.org/fx"
|
||||
"sneak.berlin/go/webhooker/internal/config"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
@@ -15,6 +17,29 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/session"
|
||||
)
|
||||
|
||||
// stopTimeout bounds the whole fx stop sequence, not each hook.
|
||||
//
|
||||
// fx defaults to 15s, which is longer than Docker's 10s default
|
||||
// stop grace: the container would be SIGKILLed before the bound
|
||||
// could fire, so nothing bounded by it would ever be observed.
|
||||
// 5s leaves headroom inside that grace for signal delivery and
|
||||
// process exit; the observed wedge case already exits at ~5.3s,
|
||||
// so a larger bound would trade a rare skipped database close for
|
||||
// a more common hard kill.
|
||||
//
|
||||
// It must stay strictly above server.ShutdownTimeout: an HTTP
|
||||
// drain that uses its whole budget would otherwise exhaust the
|
||||
// sequence budget at that instant, and fx would skip every hook
|
||||
// after the server — the delivery engine, the healthcheck, the
|
||||
// webhook DB manager and the database close. Those tail hooks are
|
||||
// microsecond-scale in normal operation, so the 2s difference is
|
||||
// ample. TestStopTimeout_LeavesHeadroomForTailHooks pins it.
|
||||
//
|
||||
// This does not make the database close unconditional: the
|
||||
// ArchiveSweeper and RetentionReaper hooks run before the server
|
||||
// and can still consume the whole budget on their own.
|
||||
const stopTimeout = 5 * time.Second
|
||||
|
||||
// Build-time variables set via -ldflags.
|
||||
//
|
||||
//nolint:gochecknoglobals // Build-time variables injected by the linker.
|
||||
@@ -27,7 +52,14 @@ func main() {
|
||||
globals.Appname = appname
|
||||
globals.Version = version
|
||||
|
||||
fx.New(
|
||||
newApp().Run()
|
||||
}
|
||||
|
||||
// newApp builds the application graph. It is separate from main so
|
||||
// a test can assert the options it carries.
|
||||
func newApp() *fx.App {
|
||||
return fx.New(
|
||||
fx.StopTimeout(stopTimeout),
|
||||
fx.Provide(
|
||||
globals.New,
|
||||
logger.New,
|
||||
@@ -60,5 +92,5 @@ func main() {
|
||||
) {
|
||||
},
|
||||
),
|
||||
).Run()
|
||||
)
|
||||
}
|
||||
|
||||
61
cmd/webhooker/main_test.go
Normal file
61
cmd/webhooker/main_test.go
Normal file
@@ -0,0 +1,61 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/webhooker/internal/server"
|
||||
)
|
||||
|
||||
// dockerStopGrace is Docker's default `docker stop` grace period.
|
||||
// The Dockerfile sets no STOPSIGNAL or grace override, so this is
|
||||
// the deadline the container is actually held to, and the fx stop
|
||||
// timeout has to fit inside it with room for signal delivery and
|
||||
// process exit.
|
||||
const dockerStopGrace = 10 * time.Second
|
||||
|
||||
// TestNewApp_StopTimeout pins the fx stop timeout. Without the
|
||||
// explicit fx.StopTimeout option the app reads fx's 15s
|
||||
// DefaultTimeout, which exceeds dockerStopGrace: the container is
|
||||
// SIGKILLed before the bound fires and every shutdown hook bounded
|
||||
// by it — including the operator-facing timeout log — becomes
|
||||
// unreachable in the image this repo produces.
|
||||
//
|
||||
// fx.New applies options before it executes invokes, so the timeout
|
||||
// is set whether or not the graph itself can be constructed here.
|
||||
func TestNewApp_StopTimeout(t *testing.T) {
|
||||
t.Setenv("DATA_DIR", t.TempDir())
|
||||
|
||||
got := newApp().StopTimeout()
|
||||
|
||||
require.Equal(t, stopTimeout, got)
|
||||
require.Less(t, got, dockerStopGrace)
|
||||
}
|
||||
|
||||
// tailHeadroom is the slack the fx stop budget must keep beyond the
|
||||
// HTTP drain. The hooks that run after the server — the delivery
|
||||
// engine, the healthcheck, the webhook DB manager and the database
|
||||
// close — are microsecond-scale in normal operation, so this is
|
||||
// generous for them.
|
||||
const tailHeadroom = 2 * time.Second
|
||||
|
||||
// TestStopTimeout_LeavesHeadroomForTailHooks pins the relationship
|
||||
// between the HTTP drain budget and the fx stop budget. fx bounds
|
||||
// the whole stop sequence, and returns without running its
|
||||
// remaining hooks once the stop context has expired. If the two
|
||||
// values were equal, an HTTP drain that used its full budget would
|
||||
// exhaust the sequence budget at the instant it finished and every
|
||||
// later hook, the database close included, would be skipped in
|
||||
// exactly the case where the drain mattered.
|
||||
//
|
||||
// Lowering either constant to erase the gap must fail here rather
|
||||
// than silently recreating that.
|
||||
func TestStopTimeout_LeavesHeadroomForTailHooks(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
require.Less(t, server.ShutdownTimeout, stopTimeout)
|
||||
require.GreaterOrEqual(
|
||||
t, stopTimeout-server.ShutdownTimeout, tailHeadroom,
|
||||
)
|
||||
}
|
||||
Reference in New Issue
Block a user