Archive sweep repeats open/prune/close once per database target instead of once per webhook #101

New Issue

clawbot · 2026-08-09T07:26:46+02:00

clawbot commented

2026-08-09 07:26:46 +02:00

Surfaced during the independent review of PR #95 (#89) and confirmed by the implementer. Out of scope for #89, so filing it here — it is currently recorded only in that PR's body, which disappears on merge.

Problem

ArchiveSweeper.sweep iterates targets, not webhooks. A webhook has exactly one archive file (archive-{webhookID}.db), but it may have more than one database target, and each target carries its own expiry in its config JSON.

So a webhook with two positive-expiry database targets has its single archive file opened, pruned, and closed twice per tick — once per target. With N such targets it is N times per tick. The work is redundant: the second pass prunes an archive the first pass just pruned.

Why it is not urgent

Correctness is unaffected. Pruning is idempotent, and the per-writer mutex serialises the passes, so there is no race and no data loss.
Multiple database targets on one webhook is an unusual configuration.
The cost is bounded by the tick interval (RETENTION_SWEEP_INTERVAL, default one hour).

It is still wasted file I/O on a path whose whole design goal is to bound file churn, and it makes the close/reopen debounce accounting harder to reason about than it should be.

Second-order oddity worth deciding on

Because the sweep is per-target, two database targets on the same webhook with different expiry values both prune the same file. The shorter expiry effectively wins, since whichever pass runs with the tighter cutoff deletes the most rows. Nothing documents that, and it is not obviously the intended semantics — arguably a webhook's archive should have one retention policy, not one per target.

Definition of done

The sweep visits each webhook's archive at most once per tick.
A deliberate, documented rule for what expiry applies when a webhook has multiple database targets with differing expiry values (longest wins, shortest wins, or reject the configuration at target-creation time). Whichever is chosen, state it in the README next to the existing archive documentation.
A test with two positive-expiry database targets on one webhook asserting the archive is opened once per tick, and a test pinning the chosen multi-expiry rule.
make check green via the repo's own entrypoints; .golangci.yml untouched.

Surfaced during the independent review of PR #95 (#89) and confirmed by the implementer. Out of scope for #89, so filing it here — it is currently recorded only in that PR's body, which disappears on merge. ## Problem `ArchiveSweeper.sweep` iterates **targets**, not webhooks. A webhook has exactly one archive file (`archive-{webhookID}.db`), but it may have more than one `database` target, and each target carries its own `expiry` in its config JSON. So a webhook with two positive-expiry `database` targets has its single archive file opened, pruned, and closed **twice per tick** — once per target. With N such targets it is N times per tick. The work is redundant: the second pass prunes an archive the first pass just pruned. ## Why it is not urgent - Correctness is unaffected. Pruning is idempotent, and the per-writer mutex serialises the passes, so there is no race and no data loss. - Multiple `database` targets on one webhook is an unusual configuration. - The cost is bounded by the tick interval (`RETENTION_SWEEP_INTERVAL`, default one hour). It is still wasted file I/O on a path whose whole design goal is to bound file churn, and it makes the close/reopen debounce accounting harder to reason about than it should be. ## Second-order oddity worth deciding on Because the sweep is per-target, two `database` targets on the same webhook with **different** expiry values both prune the same file. The shorter expiry effectively wins, since whichever pass runs with the tighter cutoff deletes the most rows. Nothing documents that, and it is not obviously the intended semantics — arguably a webhook's archive should have one retention policy, not one per target. ## Definition of done - The sweep visits each webhook's archive at most once per tick. - A deliberate, documented rule for what expiry applies when a webhook has multiple `database` targets with differing expiry values (longest wins, shortest wins, or reject the configuration at target-creation time). Whichever is chosen, state it in the README next to the existing archive documentation. - A test with two positive-expiry `database` targets on one webhook asserting the archive is opened once per tick, and a test pinning the chosen multi-expiry rule. - `make check` green via the repo's own entrypoints; `.golangci.yml` untouched.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#101