Pin the pre-reopen close in archiveWriter.sweepExpired (unprotected connection-leak guard) #103

New Issue

clawbot · 2026-08-09T07:44:18+02:00

clawbot commented

2026-08-09 07:44:18 +02:00

Follow-up from the round-3 independent review of PR #95 (#89). Non-blocking there; the shipped behaviour is correct. Filing so the gap does not stay untracked.

The gap

internal/delivery/target_database_archive.go:369 — the w.close() that runs before the reopen in sweepExpired has no test behind it. Deleting it leaves the whole -race suite green.

The reviewer proved empirically what its absence costs, with a scratch probe that retained the pre-sweep *sql.DB and pinged it after the sweep: without that close, one SQLite connection leaks per webhook per tick. At the default one-hour RETENTION_SWEEP_INTERVAL that is slow, but it is unbounded and scales with the number of archived webhooks, which is exactly the class of leak #89 was filed to close.

This is the same category as the two round-2 blockers — a guard no test can distinguish from its own absence — and the review flagged that it considered blocking on it for consistency. It decided not to, on the grounds that the shipped code is correct, #89's definition of done is met, and unlike the round-2 blockers this line was never presented as a mutation-verified guard. I agree with that call, but the test should still exist.

Definition of done

A test in internal/delivery that retains the pre-sweep *sql.DB and asserts Ping() errors after SweepExpired.
Verified by mutation: deleting the w.close() at target_database_archive.go:369 must make that test fail.
make check green via the repo's own entrypoints; .golangci.yml untouched.

Three smaller items from the same review, worth folding in

evictArchiveWriterIfUnused in internal/handlers/source_management.go — the type = database filter in the remaining-target count is unprotected: dropping AND type = ? leaves the suite green. Harm if it were wrong is bounded to behaviour the spec already accepts (an early eviction that a later delivery simply recreates), but the filter is load-bearing for intent and should be pinned.
ArchiveSweeper.sweep issues its GORM query without a context. Matches the surrounding code, so not a regression — but see #102, which covers the broader "hook contexts are treated as decoration" theme in this codebase, and this belongs to the same cleanup.
ArchiveSweeper.cancel is unsynchronised. Safe as currently wired and it mirrors RetentionReaper, so this is consistency-only.

Note on one mutation that survived correctly

The review also found that removing the fileExists stat in sweepWebhook leaves the suite green — but verified this is correct, not a gap: with archiveModeExisting (mode=rw) the stat is genuinely redundant belt-and-braces and its removal causes no behaviour change. No action needed; recorded so nobody re-discovers it and "fixes" it.

Follow-up from the round-3 independent review of PR #95 (#89). Non-blocking there; the shipped behaviour is correct. Filing so the gap does not stay untracked. ## The gap `internal/delivery/target_database_archive.go:369` — the `w.close()` that runs **before** the reopen in `sweepExpired` has no test behind it. Deleting it leaves the whole `-race` suite green. The reviewer proved empirically what its absence costs, with a scratch probe that retained the pre-sweep `*sql.DB` and pinged it after the sweep: without that close, **one SQLite connection leaks per webhook per tick**. At the default one-hour `RETENTION_SWEEP_INTERVAL` that is slow, but it is unbounded and scales with the number of archived webhooks, which is exactly the class of leak #89 was filed to close. This is the same category as the two round-2 blockers — a guard no test can distinguish from its own absence — and the review flagged that it considered blocking on it for consistency. It decided not to, on the grounds that the shipped code is correct, #89's definition of done is met, and unlike the round-2 blockers this line was never presented as a mutation-verified guard. I agree with that call, but the test should still exist. ## Definition of done - A test in `internal/delivery` that retains the pre-sweep `*sql.DB` and asserts `Ping()` errors after `SweepExpired`. - Verified by mutation: deleting the `w.close()` at `target_database_archive.go:369` must make that test fail. - `make check` green via the repo's own entrypoints; `.golangci.yml` untouched. ## Three smaller items from the same review, worth folding in 1. **`evictArchiveWriterIfUnused` in `internal/handlers/source_management.go`** — the `type = database` filter in the remaining-target count is unprotected: dropping `AND type = ?` leaves the suite green. Harm if it were wrong is bounded to behaviour the spec already accepts (an early eviction that a later delivery simply recreates), but the filter is load-bearing for intent and should be pinned. 2. **`ArchiveSweeper.sweep` issues its GORM query without a context.** Matches the surrounding code, so not a regression — but see #102, which covers the broader "hook contexts are treated as decoration" theme in this codebase, and this belongs to the same cleanup. 3. **`ArchiveSweeper.cancel` is unsynchronised.** Safe as currently wired and it mirrors `RetentionReaper`, so this is consistency-only. ## Note on one mutation that survived correctly The review also found that removing the `fileExists` stat in `sweepWebhook` leaves the suite green — but verified this is **correct**, not a gap: with `archiveModeExisting` (`mode=rw`) the stat is genuinely redundant belt-and-braces and its removal causes no behaviour change. No action needed; recorded so nobody re-discovers it and "fixes" it.

clawbot referenced this issue

2026-08-09 07:44:37 +02:00

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#103