Bound shutdown hooks by their stop context (closes #102)
All checks were successful
check / check (push) Successful in 3m45s
All checks were successful
check / check (push) Successful in 3m45s
fx hands OnStop a context carrying the application's stop timeout, and the delivery engine, the retention reaper, and the archive sweeper all discarded it and called wg.Wait() bare. A worker wedged inside a delivery target that never returns, or a sweep blocked on a locked SQLite database, hung the process forever instead of letting it exit when the timeout expired. All three now wait through internal/lifecycle.WaitForShutdown, which selects the drained WaitGroup against the stop context and, on timeout, logs at error naming the component and returns an error rather than reporting a clean stop. Engine.stop also gains the cancel != nil guard its two mirrored components already had.
This commit is contained in:
@@ -46,8 +46,19 @@ func (r *RetentionReaper) ExportStart() {
|
||||
}
|
||||
|
||||
// ExportStop stops the reaper's background loop for tests.
|
||||
func (r *RetentionReaper) ExportStop() {
|
||||
r.stop()
|
||||
func (r *RetentionReaper) ExportStop(ctx context.Context) error {
|
||||
return r.stop(ctx)
|
||||
}
|
||||
|
||||
// ExportWedgeLoop adds a goroutine to the reaper's WaitGroup that
|
||||
// never observes cancellation and returns only when release is
|
||||
// closed. It stands in for a sweep stuck on a locked database.
|
||||
func (r *RetentionReaper) ExportWedgeLoop(
|
||||
release <-chan struct{},
|
||||
) {
|
||||
r.wg.Go(func() {
|
||||
<-release
|
||||
})
|
||||
}
|
||||
|
||||
// ExportSetInterval overrides the sweep interval for tests.
|
||||
|
||||
@@ -10,6 +10,7 @@ import (
|
||||
"go.uber.org/fx"
|
||||
"gorm.io/gorm"
|
||||
"sneak.berlin/go/webhooker/internal/config"
|
||||
"sneak.berlin/go/webhooker/internal/lifecycle"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
)
|
||||
|
||||
@@ -62,8 +63,9 @@ func NewRetentionReaper(
|
||||
}
|
||||
|
||||
// registerHooks wires the reaper's start and stop into the fx
|
||||
// lifecycle. The start hook's context is deliberately ignored: see
|
||||
// start for why the sweep loop must not inherit it.
|
||||
// lifecycle. The start hook's context is deliberately ignored (see
|
||||
// start for why the sweep loop must not inherit it); the stop hook's
|
||||
// context is honoured (see stop).
|
||||
func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
|
||||
lc.Append(fx.Hook{
|
||||
//nolint:contextcheck // Not inheriting the hook context is
|
||||
@@ -73,10 +75,8 @@ func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
|
||||
|
||||
return nil
|
||||
},
|
||||
OnStop: func(_ context.Context) error {
|
||||
r.stop()
|
||||
|
||||
return nil
|
||||
OnStop: func(ctx context.Context) error {
|
||||
return r.stop(ctx)
|
||||
},
|
||||
})
|
||||
}
|
||||
@@ -105,15 +105,27 @@ func (r *RetentionReaper) start() {
|
||||
)
|
||||
}
|
||||
|
||||
func (r *RetentionReaper) stop() {
|
||||
// stop cancels the sweep loop's context and waits for it to
|
||||
// exit, bounded by the stop hook's context: a sweep wedged on a
|
||||
// locked database must not hang the process past fx's stop
|
||||
// timeout.
|
||||
func (r *RetentionReaper) stop(ctx context.Context) error {
|
||||
r.log.Info("retention reaper stopping")
|
||||
|
||||
if r.cancel != nil {
|
||||
r.cancel()
|
||||
}
|
||||
|
||||
r.wg.Wait()
|
||||
err := lifecycle.WaitForShutdown(
|
||||
ctx, r.log, "retention reaper", &r.wg,
|
||||
)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
r.log.Info("retention reaper stopped")
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
func (r *RetentionReaper) run(ctx context.Context) {
|
||||
|
||||
@@ -26,6 +26,13 @@ const (
|
||||
// reaperTestRetentionDays is the retention policy the lifecycle
|
||||
// tests give their webhook.
|
||||
reaperTestRetentionDays = 30
|
||||
|
||||
// reaperWedgeStopTimeout is the stop timeout the wedged-shutdown
|
||||
// test hands OnStop, standing in for fx's StopTimeout. The test
|
||||
// asserts only that the hook returns at all, and allows it
|
||||
// reaperStopTimeout — forty times this budget — to do so, so no
|
||||
// assertion races the wall clock.
|
||||
reaperWedgeStopTimeout = 250 * time.Millisecond
|
||||
)
|
||||
|
||||
// recordingLifecycle is a minimal fx.Lifecycle that records the
|
||||
@@ -207,3 +214,59 @@ func TestRetentionReaper_StopHookStopsLoop(t *testing.T) {
|
||||
"a stopped reaper must not sweep anything",
|
||||
)
|
||||
}
|
||||
|
||||
// TestRetentionReaper_StopHookHonoursStopTimeout is the
|
||||
// regression test for a shutdown that could never complete. fx
|
||||
// hands OnStop a context carrying the application's stop timeout;
|
||||
// an OnStop that discards it and calls wg.Wait() bare hangs the
|
||||
// process forever on a sweep blocked on a locked SQLite database
|
||||
// — precisely when a bounded shutdown matters most.
|
||||
//
|
||||
// The wedged goroutine here never observes cancellation, so the
|
||||
// hook can only return by honouring its context, and it must say
|
||||
// so rather than reporting a clean stop.
|
||||
func TestRetentionReaper_StopHookHonoursStopTimeout(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
env := setupRetentionTest(t)
|
||||
|
||||
env.reaper.ExportSetInterval(reaperTestInterval)
|
||||
|
||||
lc := startReaperViaHook(t, env.reaper)
|
||||
|
||||
release := make(chan struct{})
|
||||
|
||||
t.Cleanup(func() { close(release) })
|
||||
|
||||
env.reaper.ExportWedgeLoop(release)
|
||||
|
||||
stopCtx, cancel := context.WithTimeout(
|
||||
context.Background(), reaperWedgeStopTimeout,
|
||||
)
|
||||
defer cancel()
|
||||
|
||||
var stopErr error
|
||||
|
||||
stopped := make(chan struct{})
|
||||
|
||||
go func() {
|
||||
defer close(stopped)
|
||||
|
||||
stopErr = lc.hooks[0].OnStop(stopCtx)
|
||||
}()
|
||||
|
||||
select {
|
||||
case <-stopped:
|
||||
case <-time.After(reaperStopTimeout):
|
||||
t.Fatal(
|
||||
"OnStop did not return: it discarded the stop " +
|
||||
"context and is waiting on a wedged goroutine " +
|
||||
"that will never observe cancellation",
|
||||
)
|
||||
}
|
||||
|
||||
require.ErrorIs(t, stopErr, context.DeadlineExceeded)
|
||||
require.ErrorContains(t, stopErr, "retention reaper")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user