Evict archive writers on deletion and sweep idle archives (closes #89)

The per-webhook archiveWriter registry in the database delivery target
was never evicted, so a deleted webhook's writer -- and any archive
file handle open within its debounce window -- lingered for the process
lifetime. Separately, expiry pruning ran only when an archive was
(re)opened, and reopens only happen on writes, so an archive belonging
to a webhook that stopped receiving events kept its expired rows
forever.

Eviction: a new one-method delivery.WebhookEvictor interface (kept
separate from Notifier: archiving lifecycle is not notification) is
implemented by the Engine and injected into the handlers. Deleting a
webhook, or deleting its last database target, drops the writer from
the registry and closes its handle under the writer's own mutex, so
eviction can never race an in-flight write. An evicted writer refuses
further writes rather than reopening a file nothing holds. The archive
file is deliberately left on disk: it is long-term storage an operator
may want to keep or move away, and destroying it as a side effect of
deleting a webhook would be unrecoverable.

Idle sweep: a new ArchiveSweeper, modelled on the event RetentionReaper
(fx lifecycle hooks, cancellable context, WaitGroup, ticker loop),
prunes archives whose database target declares a positive expiry. It
reuses the existing RETENTION_SWEEP_INTERVAL rather than adding a
config key. It never creates an archive -- a missing file is skipped,
and the reopen uses SQLite mode=rw so the file cannot be conjured even
if it disappears mid-sweep -- routes the prune through the per-webhook
writer so its mutex orders the sweep against concurrent writes, and
leaves the archive closed so the move-the-file-away workflow keeps
working. A failure for one webhook is logged and the sweep continues.
Archives with no expiry or the expiry "never" are untouched.

The sweep loop's context is rooted at context.Background(), not at the
fx OnStart hook context. The hook context carries fx's 15 second start
timeout, so a loop derived from it is cancelled three quarters of an
hour before the first tick under the default one-hour interval, giving
a sweeper that never sweeps. OnStop still cancels the loop and waits on
the WaitGroup, so shutdown is unchanged.

The sweep also never leaves a registry entry behind. Reaching the
writer through the ordinary create-and-cache accessor would let a sweep
that raced a webhook deletion re-insert a writer for a webhook that no
longer exists, which nothing would ever evict again -- the very leak
this change closes. An entry the sweep has to create is marked
sweep-owned and released when the prune finishes, unless a delivery
claimed it meanwhile, in which case it belongs to the registry and an
eviction can still reach it. A writer evicted underneath a sweep is an
ordinary interleaving and is logged at debug, not error.

Each of those guards is pinned by a test that fails when the guard is
removed. The adopt-during-sweep window -- a delivery claiming the
sweep's own registry entry while that sweep is still running -- is
driven directly against the registry, because a delivery placed between
two sweeps never reaches the release path at all. The requirement that
an idle archive ends the sweep closed is asserted both on a writer
proven to hold an open handle beforehand and, end to end, on a
delivery-owned entry the sweep keeps, rather than on an entry the sweep
has already released and which therefore reports "not open" either
way. The "never" expiry short circuit is checked against an archive
file that has never been migrated, so any open of it would be
observable as a created table.

TODO.md

@@ -14,10 +14,9 @@ pre-1.0. No git tags exist. main (4f5ecb1) is a working webhook proxy
 with auth, CSRF/SSRF protections, login rate limiting, Slack target,
 event retention (#63), the database archiving target (#43), the admin
 password change flow (#65), policy compliance (#6), and pinned lint
-tooling (#55). Note: TODO.md was
+tooling (#55). Note: TODO.md was deliberately deleted from this repo in
-deliberately deleted from this repo in f9a9569 (2026-03-01, #6); its
+f9a9569 (2026-03-01, #6); its content was folded into the README TODO
-content was folded into the README TODO section, which this draft
+section, which this draft reconstructs as of 2026-07-06.
-reconstructs as of 2026-07-06.
 
 # Next Step
 

internal/delivery/archive_sweeper_test.go

@@ -408,6 +408,91 @@ func TestArchiveSweep_KeepsWriterAdoptedByDelivery(
 	)
 }
 
+// TestArchiveSweep_KeepsWriterAdoptedDuringSweep covers the one
+// interleaving the sweepOwned flag exists for, which
+// TestArchiveSweep_KeepsWriterAdoptedByDelivery cannot reach: a
+// delivery adopting the sweep's own entry WHILE that sweep is
+// still running.
+//
+// The registry operations are driven directly, in the order the
+// sweep and a concurrent delivery perform them, so the window is
+// exercised deterministically rather than hoped for:
+//
+//  1. the sweep finds no cached writer and registers one of its
+//     own, marked sweep-owned;
+//  2. a delivery arrives, is handed that very writer, clears the
+//     flag and opens the archive handle;
+//  3. the sweep finishes and releases what it created.
+//
+// Step 3 must leave the entry alone. Dropping it would detach a
+// writer that is holding an open archive handle inside its
+// debounce window, and no eviction could ever reach it again —
+// exactly the process-lifetime handle leak this change exists to
+// close. The eviction at the end proves the entry is still
+// reachable.
+func TestArchiveSweep_KeepsWriterAdoptedDuringSweep(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSweeperTest(t)
+
+	webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`)
+	env.seedArchiveRows(
+		t, webhookID, time.Now().Add(-48*time.Hour),
+	)
+
+	sweepWriter, created, err := env.eng.ExportSweepWriterFor(
+		webhookID,
+	)
+	require.NoError(t, err)
+	require.True(
+		t, created,
+		"the sweep must have created the registry entry itself",
+	)
+
+	// The delivery lands mid-sweep and adopts the entry.
+	webhookDB := testWebhookDB(t)
+	event := seedEvent(t, webhookDB, `{"n":1}`)
+	event.WebhookID = webhookID
+	d := seedDatabaseTargetDelivery(
+		t, webhookDB, event, `{"expiry":"1h"}`,
+	)
+
+	env.eng.ExportDeliverDatabase(webhookDB, d)
+
+	adopted := env.eng.ExportArchiveWriterFor(webhookID)
+	require.NotNil(t, adopted)
+	require.True(
+		t, sweepWriter.Same(adopted),
+		"the delivery must have adopted the sweep's writer",
+	)
+	require.True(
+		t, env.eng.ExportArchiveHandleOpen(webhookID),
+		"the delivery leaves the archive handle open",
+	)
+
+	// The sweep finishes.
+	env.eng.ExportReleaseSweepWriter(webhookID, sweepWriter)
+
+	require.True(
+		t, env.eng.ExportHasArchiveWriter(webhookID),
+		"a writer adopted by a delivery during a sweep must "+
+			"stay registered, or its open handle is unreachable",
+	)
+
+	env.eng.EvictWebhook(webhookID)
+
+	assert.False(
+		t, env.eng.ExportHasArchiveWriter(webhookID),
+		"the adopted writer must still be evictable",
+	)
+	assert.False(
+		t, sweepWriter.HandleOpen(),
+		"eviction must have closed the adopted writer's handle",
+	)
+}
+
 // TestArchiveSweep_ContinuesAfterPerWebhookFailure proves a
 // failure for one webhook does not abort the sweep for the
 // others: an unparseable expiry and an unreadable archive both
@@ -516,21 +601,87 @@ func TestArchiveSweep_PrunesIdleArchive(t *testing.T) {
 // TestArchiveSweep_LeavesArchiveClosed proves the sweep does
 // not hold the archive open afterwards, so an operator can
 // still move the file away for offline retention.
+//
+// The assertion is made on a writer the test holds a reference
+// to, and the handle is proven OPEN before the sweep runs, so the
+// test observes the sweep closing it rather than a writer that
+// merely never opened anything. Asking the registry instead would
+// be vacuous here: the sweep releases an entry it created, and a
+// missing entry reports "not open" whether or not anything was
+// closed.
 func TestArchiveSweep_LeavesArchiveClosed(t *testing.T) {
 	t.Parallel()
 
 	env := setupSweeperTest(t)
 
+	webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`)
+	path := env.seedArchiveRows(
+		t, webhookID, time.Now().Add(-48*time.Hour),
+	)
+
+	w := delivery.NewExportArchiveWriter(
+		path, archiveTestLogger(), 0,
+	)
+
+	require.NoError(t, w.OpenExisting(time.Hour))
+	require.True(
+		t, w.HandleOpen(),
+		"the writer must hold an open handle before the sweep",
+	)
+
+	require.NoError(t, w.SweepExpired(time.Hour))
+
+	assert.False(
+		t, w.HandleOpen(),
+		"an idle archive must end the sweep closed",
+	)
+}
+
+// TestArchiveSweep_ClosesHandleOfRegisteredWriter states the same
+// guarantee end to end, through the real sweeper and a writer the
+// registry keeps.
+//
+// The delivery leaves the archive handle open inside its debounce
+// window and makes the entry delivery-owned, so the sweep finds a
+// cached writer (created is false, nothing is released) and the
+// registry query afterwards is answered by a writer that really
+// exists. A handle left open here would be doubly wrong: it also
+// blocks the operator's move-the-file-away workflow.
+func TestArchiveSweep_ClosesHandleOfRegisteredWriter(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSweeperTest(t)
+
 	webhookID := env.seedDatabaseTarget(t, `{"expiry":"1h"}`)
 	env.seedArchiveRows(
 		t, webhookID, time.Now().Add(-48*time.Hour),
 	)
 
+	webhookDB := testWebhookDB(t)
+	event := seedEvent(t, webhookDB, `{"n":1}`)
+	event.WebhookID = webhookID
+	d := seedDatabaseTargetDelivery(
+		t, webhookDB, event, `{"expiry":"1h"}`,
+	)
+
+	env.eng.ExportDeliverDatabase(webhookDB, d)
+
+	require.True(
+		t, env.eng.ExportArchiveHandleOpen(webhookID),
+		"the delivery must leave the archive handle open",
+	)
+
 	env.sweeper.ExportSweep(context.Background())
 
+	require.True(
+		t, env.eng.ExportHasArchiveWriter(webhookID),
+		"the delivery's registry entry must survive the sweep",
+	)
 	assert.False(
 		t, env.eng.ExportArchiveHandleOpen(webhookID),
-		"an idle archive must end the sweep closed",
+		"the sweep must leave the archive closed",
 	)
 }
 
@@ -559,9 +710,75 @@ func TestArchiveSweep_NeverExpiryUntouched(t *testing.T) {
 			t, []string{sweepRowOld}, archivedEventIDs(t, path),
 			"config %q must keep rows forever", configJSON,
 		)
+		assert.False(
+			t, env.eng.ExportHasArchiveWriter(webhookID),
+			"config %q must leave no registry entry behind",
+			configJSON,
+		)
 	}
 }
 
+// TestArchiveSweep_NeverExpirySkipsBeforeOpening pins the
+// expiry <= 0 boundary in sweepTarget, which the row assertions
+// above cannot reach: pruning is separately gated on a positive
+// expiry, so a "never" archive keeps its rows even if the sweep
+// does open it.
+//
+// The spec is stronger than that — a "never" archive is skipped
+// before any file is touched — so the archive here exists but has
+// never been migrated. Opening it at all would run AutoMigrate
+// and create the archive table, which is exactly what must not
+// happen.
+func TestArchiveSweep_NeverExpirySkipsBeforeOpening(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSweeperTest(t)
+
+	webhookID := env.seedDatabaseTarget(t, `{"expiry":"never"}`)
+	path := env.archivePath(webhookID)
+
+	seedUnmigratedArchive(t, path)
+	require.False(t, archiveTableExists(t, path))
+
+	env.sweeper.ExportSweep(context.Background())
+
+	assert.False(
+		t, archiveTableExists(t, path),
+		"a never-expiry archive must not be opened at all",
+	)
+}
+
+// seedUnmigratedArchive creates an archive file that exists but
+// carries no archive schema, so any open of it is observable: the
+// archive table appears only if something ran AutoMigrate.
+func seedUnmigratedArchive(t *testing.T, path string) {
+	t.Helper()
+
+	sqlDB, err := sql.Open(
+		"sqlite", fmt.Sprintf("file:%s?mode=rwc", path),
+	)
+	require.NoError(t, err)
+
+	_, err = sqlDB.ExecContext(
+		t.Context(), "CREATE TABLE placeholder (id INTEGER)",
+	)
+	require.NoError(t, err)
+
+	require.NoError(t, sqlDB.Close())
+}
+
+// archiveTableExists reports whether an archive file has had the
+// archive schema migrated into it.
+func archiveTableExists(t *testing.T, path string) bool {
+	t.Helper()
+
+	return openArchiveDBForRead(t, path).
+		Migrator().
+		HasTable(&delivery.ExportArchivedEvent{})
+}
+
 // TestArchiveSweep_DoesNotCreateArchiveFile proves the sweep
 // never conjures an archive: a webhook with a database target
 // that has never received an event must still have no archive

internal/delivery/export_test.go

@@ -370,6 +370,15 @@ func (e *ExportArchiveWriter) HandleOpen() bool {
 	return e.w.db != nil
 }
 
+// Same reports whether both wrappers refer to the very same
+// underlying archive writer, so a test can prove a registry entry
+// is the writer it was handed rather than a replacement.
+func (e *ExportArchiveWriter) Same(
+	other *ExportArchiveWriter,
+) bool {
+	return other != nil && e.w == other.w
+}
+
 // ExportArchiveWriterFor returns the archive writer the registry
 // currently caches for a webhook, or nil when none is cached. It
 // never creates one, so a test can hold a reference to the very
@@ -435,6 +444,29 @@ func (e *Engine) ExportEnsureArchiveWriter(
 	return w.path, nil
 }
 
+// ExportSweepWriterFor takes a webhook's registry writer exactly
+// as the idle sweep does, reporting whether the sweep had to
+// create the entry. It lets a test drive the registry through the
+// sweep's own entry point instead of choreographing goroutines.
+func (e *Engine) ExportSweepWriterFor(
+	webhookID string,
+) (*ExportArchiveWriter, bool, error) {
+	w, created, err := e.dbTarget.sweepWriterFor(webhookID)
+	if err != nil {
+		return nil, false, err
+	}
+
+	return &ExportArchiveWriter{w: w}, created, nil
+}
+
+// ExportReleaseSweepWriter releases a sweep-created registry entry
+// exactly as a finished sweep does.
+func (e *Engine) ExportReleaseSweepWriter(
+	webhookID string, w *ExportArchiveWriter,
+) {
+	e.dbTarget.releaseSweepWriter(webhookID, w.w)
+}
+
 // NewTestArchiveSweeper builds an ArchiveSweeper backed by the
 // given main database and engine, without the fx lifecycle.
 // Intended for tests.

internal/delivery/target_database_archive.go

@@ -393,10 +393,12 @@ func (w *archiveWriter) evict() {
 }
 
 // prune deletes archived rows older than expiry, measured from
-// each row's archived time. It runs on every (re)open, and
+// each row's archived time. It runs on every (re)open, so a
-// because the file is reopened after writes this keeps the
+// steadily written archive is swept by its own write traffic. An
-// archive swept without a separate background sweeper. Failures
+// archive that goes idle receives no further reopens, which is
-// are logged, not fatal: a prune error must not stop archiving.
+// why ArchiveSweeper exists to drive sweepExpired on a timer.
+// Failures are logged, not fatal: a prune error must not stop
+// archiving.
 func (w *archiveWriter) prune(expiry time.Duration) {
 	cutoff := time.Now().Add(-expiry)
 

internal/handlers/source_delete_test.go

@@ -307,10 +307,11 @@ func TestHandleTargetDelete_KeepsWriterWhenDatabaseTargetRemains(
 	)
 }
 
-// TestHandleTargetDelete_KeepsWriterWhileDatabaseTargetRemains
+// TestHandleTargetDelete_KeepsWriterWhenOtherTypeDeleted proves
-// proves that deleting an unrelated target type leaves a
+// that deleting a target of an unrelated type leaves a
-// still-needed archive writer alone.
+// still-needed archive writer alone: the webhook's database
-func TestHandleTargetDelete_KeepsWriterWhileDatabaseTargetRemains(
+// target is untouched, so its writer must stay.
+func TestHandleTargetDelete_KeepsWriterWhenOtherTypeDeleted(
 	t *testing.T,
 ) {
 	t.Parallel()