Root background loops at context.Background() (closes #97)
All checks were successful
check / check (push) Successful in 3m3s
All checks were successful
check / check (push) Successful in 3m3s
The context fx hands an OnStart hook is derived with context.WithTimeout(ctx, StartTimeout) — 15 seconds by default — and is cancelled once the start phase ends. It is a start-phase context, not an application-lifetime one. Two components derived their long-lived loops from it and so stopped running roughly fifteen seconds after boot. Engine.start rooted the entire worker pool, restart recovery, and the retry sweep in it. Every worker returned on ctx.Done() shortly after startup, so the process kept receiving and persisting inbound events while nothing at all forwarded them: deliveryCh filled up and started logging "delivery channel full" with no consumer left. That is the whole purpose of the application. RetentionReaper.start had the same defect. With the default one-hour RETENTION_SWEEP_INTERVAL the loop was cancelled forty-five minutes before its first tick, so the reaper never ran a single sweep and per-webhook event databases grew without bound. Both now derive their loop context from context.Background(). Their lifetime is bounded by OnStop, which already cancels and waits on the WaitGroup, so shutdown is unchanged. Each hook registration moves into a registerHooks method, the OnStart parameter is named _ so the trap cannot be reintroduced by silencing an unused-parameter warning, and a comment at each start explains why the hook context must not be used. This matches the shape of the same fix applied to the archive sweeper. The new lifecycle tests drive the genuine registered hooks with an already-cancelled OnStart context and assert the loops still do work afterwards — a task delivered, an expired event reaped. Reverting either fix makes its pair of tests fail. Each component also gets a shutdown test asserting OnStop cancels the loop and wg.Wait() returns inside a bounded timeout, so the fix does not trade a startup bug for a shutdown hang. iWaitForStatus becomes iWaitForDelivered: every call site waits for the delivered status, and the two added call sites pushed it past unparam's threshold for reporting an always-identical argument.
This commit is contained in:
@@ -149,18 +149,7 @@ func New(
|
||||
Transport: NewSSRFSafeTransport(),
|
||||
})
|
||||
|
||||
lc.Append(fx.Hook{
|
||||
OnStart: func(ctx context.Context) error {
|
||||
e.start(ctx)
|
||||
|
||||
return nil
|
||||
},
|
||||
OnStop: func(_ context.Context) error {
|
||||
e.stop()
|
||||
|
||||
return nil
|
||||
},
|
||||
})
|
||||
e.registerHooks(lc)
|
||||
|
||||
return e
|
||||
}
|
||||
@@ -210,8 +199,40 @@ func (e *Engine) ScheduleRetry(
|
||||
})
|
||||
}
|
||||
|
||||
func (e *Engine) start(ctx context.Context) {
|
||||
ctx, cancel := context.WithCancel(ctx)
|
||||
// registerHooks wires the engine's start and stop into the fx
|
||||
// lifecycle. The start hook's context is deliberately ignored:
|
||||
// see start for why the worker pool must not inherit it.
|
||||
func (e *Engine) registerHooks(lc fx.Lifecycle) {
|
||||
lc.Append(fx.Hook{
|
||||
//nolint:contextcheck // Not inheriting the hook context
|
||||
// is the point: see start.
|
||||
OnStart: func(_ context.Context) error {
|
||||
e.start()
|
||||
|
||||
return nil
|
||||
},
|
||||
OnStop: func(_ context.Context) error {
|
||||
e.stop()
|
||||
|
||||
return nil
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
// start launches the worker pool, restart recovery, and the
|
||||
// periodic retry sweep.
|
||||
//
|
||||
// Their context is derived from context.Background(), NOT from
|
||||
// the fx OnStart hook context. The hook context carries fx's
|
||||
// start timeout (15s by default) and is cancelled once the start
|
||||
// phase completes, so goroutines derived from it stop a few
|
||||
// seconds into the process: every worker would return and the
|
||||
// engine would silently stop delivering webhooks entirely. A
|
||||
// long-lived goroutine must outlive the startup phase, so its
|
||||
// lifetime is bounded by OnStop instead: stop cancels this
|
||||
// context and waits on the WaitGroup.
|
||||
func (e *Engine) start() {
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
e.cancel = cancel
|
||||
|
||||
for range e.workers {
|
||||
|
||||
Reference in New Issue
Block a user