WIP: root background loops at context.Background() (refs #97)

Incomplete: tests pass but three lint findings remain (funcorder on
registerHooks, unparam in engine_integration_test.go). Committed to
preserve work in progress; to be amended into a single clean commit.

internal/database/export_test.go

@@ -5,6 +5,8 @@ import (
 	"log/slog"
 	"os"
 	"time"
+
+	"go.uber.org/fx"
 )
 
 // NewTestRetentionReaper builds a RetentionReaper backed by the given
@@ -29,3 +31,26 @@ func NewTestRetentionReaper(
 func (r *RetentionReaper) ExportSweep(ctx context.Context) {
 	r.sweep(ctx)
 }
+
+// ExportRegisterHooks registers the reaper's real fx lifecycle hooks
+// on a lifecycle supplied by a test, so a test can drive the exact
+// OnStart/OnStop functions the application runs and hand OnStart the
+// kind of context fx actually supplies.
+func (r *RetentionReaper) ExportRegisterHooks(lc fx.Lifecycle) {
+	r.registerHooks(lc)
+}
+
+// ExportStart starts the reaper's background loop for tests.
+func (r *RetentionReaper) ExportStart() {
+	r.start()
+}
+
+// ExportStop stops the reaper's background loop for tests.
+func (r *RetentionReaper) ExportStop() {
+	r.stop()
+}
+
+// ExportSetInterval overrides the sweep interval for tests.
+func (r *RetentionReaper) ExportSetInterval(d time.Duration) {
+	r.interval = d
+}

internal/database/retention.go

@@ -56,9 +56,20 @@ func NewRetentionReaper(
 		interval:  params.Config.RetentionSweepInterval,
 	}
 
+	r.registerHooks(lc)
+
+	return r
+}
+
+// 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.
+func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
 	lc.Append(fx.Hook{
-		OnStart: func(ctx context.Context) error {
-			r.start(ctx)
+		//nolint:contextcheck // Not inheriting the hook context is
+		// the point: see start.
+		OnStart: func(_ context.Context) error {
+			r.start()
 
 			return nil
 		},
@@ -68,12 +79,20 @@ func NewRetentionReaper(
 			return nil
 		},
 	})
-
-	return r
 }
 
-func (r *RetentionReaper) start(ctx context.Context) {
-	ctx, cancel := context.WithCancel(ctx)
+// start launches the background sweep loop.
+//
+// The loop's 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 a loop derived from it dies 45 minutes before its
+// first tick under the default one-hour sweep interval, leaving a
+// reaper that never reaps. 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 (r *RetentionReaper) start() {
+	ctx, cancel := context.WithCancel(context.Background())
 	r.cancel = cancel
 
 	r.wg.Add(1)

internal/database/retention_lifecycle_test.go

@@ -0,0 +1,209 @@
+package database_test
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx"
+	"gorm.io/gorm"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+const (
+	// reaperTestInterval is the sweep interval a lifecycle test
+	// runs the reaper at, so a loop that survives startup produces
+	// an observable sweep quickly.
+	reaperTestInterval = 10 * time.Millisecond
+
+	// reaperStopTimeout bounds how long a lifecycle test waits for
+	// the reaper's OnStop hook to return before declaring the
+	// shutdown hung.
+	reaperStopTimeout = 10 * time.Second
+
+	// reaperTestRetentionDays is the retention policy the lifecycle
+	// tests give their webhook.
+	reaperTestRetentionDays = 30
+)
+
+// 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)
+}
+
+// startReaperViaHook drives the genuine fx hooks the application
+// registers for the reaper, handing OnStart a context that is
+// already done. It returns the recorded lifecycle so the caller
+// can drive OnStop too.
+func startReaperViaHook(
+	t *testing.T, r *database.RetentionReaper,
+) *recordingLifecycle {
+	t.Helper()
+
+	lc := &recordingLifecycle{}
+	r.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))
+
+	return lc
+}
+
+// eventGone reports whether an event row has been removed. It
+// takes no *testing.T because it is polled from an
+// assert.Eventually condition, which runs off the test goroutine
+// where testify assertions must not be used.
+func eventGone(db *gorm.DB, eventID string) bool {
+	var n int64
+
+	err := db.Unscoped().Model(&database.Event{}).
+		Where("id = ?", eventID).Count(&n).Error
+	if err != nil {
+		return false
+	}
+
+	return n == 0
+}
+
+// seedExpiredWebhook creates a webhook with a finite retention
+// policy plus one long-expired event chain, and returns the
+// webhook's database and the chain's event ID.
+func seedExpiredWebhook(
+	t *testing.T, env *retentionTestEnv,
+) (*gorm.DB, string) {
+	t.Helper()
+
+	webhookID := createWebhook(
+		t, env.mainDB.DB(), reaperTestRetentionDays,
+	)
+
+	db, err := env.mgr.GetDB(webhookID)
+	require.NoError(t, err)
+
+	chain := seedEventChain(
+		t, db, webhookID,
+		time.Now().Add(-365*24*time.Hour),
+	)
+
+	return db, chain.eventID
+}
+
+// TestRetentionReaper_LoopOutlivesStartHookContext is the
+// regression test for a reaper that never reaped. 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
+// sweep loop rooted in it is dead three quarters of an hour
+// before its first tick under the default one-hour interval, and
+// per-webhook event databases grow without bound exactly as they
+// did before retention existed.
+//
+// Driving OnStart with an already-cancelled context is that
+// defect taken to its limit: a loop that inherits the hook
+// context never ticks once, while a correctly rooted loop keeps
+// sweeping for as long as the process lives.
+func TestRetentionReaper_LoopOutlivesStartHookContext(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupRetentionTest(t)
+
+	db, eventID := seedExpiredWebhook(t, env)
+
+	env.reaper.ExportSetInterval(reaperTestInterval)
+
+	lc := startReaperViaHook(t, env.reaper)
+	t.Cleanup(func() {
+		_ = lc.hooks[0].OnStop(context.Background())
+	})
+
+	assert.Eventually(
+		t,
+		func() bool { return eventGone(db, eventID) },
+		5*time.Second,
+		reaperTestInterval,
+		"the sweep loop must keep running after the start "+
+			"hook's context is done; it reaped nothing, so it "+
+			"inherited the hook context and died",
+	)
+}
+
+// TestRetentionReaper_StopHookStopsLoop proves the fix did not
+// trade a startup bug for a shutdown hang: now that the sweep
+// loop 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 loop stopped.
+func TestRetentionReaper_StopHookStopsLoop(t *testing.T) {
+	t.Parallel()
+
+	env := setupRetentionTest(t)
+
+	db, eventID := seedExpiredWebhook(t, env)
+
+	env.reaper.ExportSetInterval(reaperTestInterval)
+
+	lc := startReaperViaHook(t, env.reaper)
+
+	// Let the loop prove it is running before stopping it, so a
+	// fast OnStop cannot pass by stopping something already dead.
+	require.Eventually(
+		t,
+		func() bool { return eventGone(db, eventID) },
+		5*time.Second,
+		reaperTestInterval,
+	)
+
+	var stopErr error
+
+	stopped := make(chan struct{})
+
+	go func() {
+		defer close(stopped)
+
+		// stop blocks on the loop's WaitGroup, so returning at all
+		// proves the goroutine observed the cancellation.
+		stopErr = lc.hooks[0].OnStop(context.Background())
+	}()
+
+	select {
+	case <-stopped:
+	case <-time.After(reaperStopTimeout):
+		t.Fatal(
+			"OnStop did not return: the retention reaper's " +
+				"WaitGroup is still waiting on a loop that never " +
+				"observed cancellation",
+		)
+	}
+
+	require.NoError(t, stopErr)
+
+	// With the loop gone, a newly expired chain must survive.
+	survivor := seedEventChain(
+		t, db, "stopped-webhook",
+		time.Now().Add(-365*24*time.Hour),
+	)
+
+	time.Sleep(20 * reaperTestInterval)
+
+	assert.False(
+		t,
+		eventGone(db, survivor.eventID),
+		"a stopped reaper must not sweep anything",
+	)
+}