Files
webhooker/internal/delivery/engine_lifecycle_test.go
clawbot 20a050b49d
All checks were successful
check / check (push) Successful in 3m3s
Root background loops at context.Background() (closes #97)
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.
2026-08-09 05:15:13 +00:00

200 lines
5.9 KiB
Go

package delivery_test
import (
"context"
"testing"
"time"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"go.uber.org/fx"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/delivery"
)
const (
// hookStopTimeout bounds how long a lifecycle test waits for
// the engine's OnStop hook to return before declaring the
// shutdown hung.
hookStopTimeout = 10 * time.Second
// hookSettleDelay is how long startEngineViaHook waits after
// OnStart before the caller may enqueue work. A worker pool
// wrongly rooted in the already-done hook context has nothing
// but ctx.Done() ready in its select, so it is deterministically
// gone by the end of this window. Without the wait, Notify would
// race the pool's very first select, in which a ready ctx.Done()
// and a ready deliveryCh are chosen between at random and a
// doomed pool still delivers.
hookSettleDelay = 250 * time.Millisecond
)
// recordingLifecycle is a minimal fx.Lifecycle that records the
// hooks a component registers, so a test can invoke the real
// OnStart/OnStop functions with a context of its choosing.
type recordingLifecycle struct {
hooks []fx.Hook
}
func (l *recordingLifecycle) Append(h fx.Hook) {
l.hooks = append(l.hooks, h)
}
// startEngineViaHook drives the genuine fx hooks the application
// registers for the engine, handing OnStart a context that is
// already done, and returns only once a pool that inherited that
// context would have exited. It returns the recorded lifecycle so
// the caller can drive OnStop too.
//
// Callers must not seed pending or retrying deliveries before
// calling this: restart recovery enqueues those during startup,
// which would put work in the queue while the pool is still
// racing its first select.
func startEngineViaHook(
t *testing.T, eng *delivery.Engine,
) *recordingLifecycle {
t.Helper()
lc := &recordingLifecycle{}
eng.ExportRegisterHooks(lc)
require.Len(t, lc.hooks, 1)
// fx hands OnStart a context carrying the application start
// timeout, and cancels it when the start phase ends. An
// already-cancelled context is that same defect taken to its
// limit, and unlike a plain context.Background() it actually
// distinguishes a correctly rooted loop from a broken one.
hookCtx, cancel := context.WithCancel(context.Background())
cancel()
require.NoError(t, lc.hooks[0].OnStart(hookCtx))
time.Sleep(hookSettleDelay)
return lc
}
// seedLogTask seeds a pending delivery for a log target and
// returns its ID together with the task that drives it. The log
// target needs no network, so a delivery completing proves only
// that a worker picked the task up.
func seedLogTask(
t *testing.T, s iSetup,
) (string, delivery.Task) {
t.Helper()
event := iSeedEvent(
t, s.WebhookDB, s.WebhookID,
`{"lifecycle":"hook-context"}`,
)
targetID := uuid.New().String()
d := iSeedDelivery(
t, s.WebhookDB, event.ID, targetID,
database.DeliveryStatusPending,
)
bodyStr := event.Body
task := iTask(
d, event, s.WebhookID, targetID,
"hook-context-test", "", 0, 1, &bodyStr,
)
task.TargetType = database.TargetTypeLog
return d.ID, task
}
// TestEngine_WorkersOutliveStartHookContext is the regression
// test for a delivery engine that stopped delivering roughly
// fifteen seconds after boot. fx calls OnStart with a context
// carrying the application's start timeout (15s by default) and
// cancels it when the start phase ends, so a worker pool rooted
// in it exits shortly after startup: the process keeps accepting
// and persisting events while nothing at all forwards them.
//
// Driving OnStart with an already-cancelled context is that
// defect taken to its limit. A pool that inherits the hook
// context is gone before the task is even enqueued; a correctly
// rooted pool keeps working for as long as the process lives.
func TestEngine_WorkersOutliveStartHookContext(t *testing.T) {
t.Parallel()
s := newISetup(t)
lc := startEngineViaHook(t, s.Engine)
t.Cleanup(func() {
_ = lc.hooks[0].OnStop(context.Background())
})
// Seeded only after the pool has settled, so restart recovery
// cannot enqueue it during startup.
deliveryID, task := seedLogTask(t, s)
s.Engine.Notify([]delivery.Task{task})
iWaitForDelivered(t, s.WebhookDB, deliveryID)
}
// TestEngine_StopHookStopsWorkers proves the fix did not trade a
// startup bug for a shutdown hang: now that the worker pool no
// longer observes the start hook's cancellation, OnStop is the
// only thing that can stop it, and it must both return promptly
// and actually leave the pool drained.
func TestEngine_StopHookStopsWorkers(t *testing.T) {
t.Parallel()
s := newISetup(t)
lc := startEngineViaHook(t, s.Engine)
// Let the pool prove it is running before stopping it, so a
// fast OnStop cannot pass by stopping something already dead.
firstID, firstTask := seedLogTask(t, s)
s.Engine.Notify([]delivery.Task{firstTask})
iWaitForDelivered(t, s.WebhookDB, firstID)
var stopErr error
stopped := make(chan struct{})
go func() {
defer close(stopped)
// stop blocks on the workers' WaitGroup, so returning at
// all proves every goroutine observed the cancellation.
stopErr = lc.hooks[0].OnStop(context.Background())
}()
select {
case <-stopped:
case <-time.After(hookStopTimeout):
t.Fatal(
"OnStop did not return: the delivery engine's " +
"WaitGroup is still waiting on a goroutine that " +
"never observed cancellation",
)
}
require.NoError(t, stopErr)
// With every worker gone, a freshly notified task must sit
// untouched in the queue rather than being delivered.
secondID, secondTask := seedLogTask(t, s)
s.Engine.Notify([]delivery.Task{secondTask})
time.Sleep(200 * time.Millisecond)
var after database.Delivery
require.NoError(
t,
s.WebhookDB.First(&after, "id = ?", secondID).Error,
)
require.Equal(
t,
database.DeliveryStatusPending,
after.Status,
"a stopped engine must not deliver anything",
)
}