sneak/webhooker
1
0
Fork 0
Code Issues 62 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Archive writer lifecycle: evict writers on webhook deletion and sweep idle archives #89

New Issue
Closed
opened 2026-08-07 19:11:43 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-07 19:11:43 +02:00
Collaborator
Copy Link

Tracking issue for two non-blocking findings carried through the PR #84 (issue #43) review rounds, so they stop being untracked.

Problems

  1. The per-webhook archiveWriter registry (writers map in internal/delivery) is never evicted when a webhook is deleted: the writer (and any open archive DB handle within its debounce window) lingers for the process lifetime.
  2. Expiry pruning runs only on archive (re)open, and reopens happen only on writes — an archive for a webhook that stops receiving events is never swept, so expired rows persist indefinitely.

Definition of done

  • Deleting a webhook closes and removes its archiveWriter from the registry (and the reaper/deletion path is covered by a test).
  • Idle archives with a configured expiry get pruned without requiring a new write (periodic sweep, or prune scheduled off the debounce timer) — with a test proving rows older than expiry disappear from an idle archive.
  • No behavior change for archives with expiry "never".
Tracking issue for two non-blocking findings carried through the PR #84 (issue #43) review rounds, so they stop being untracked. ## Problems 1. The per-webhook `archiveWriter` registry (`writers` map in `internal/delivery`) is never evicted when a webhook is deleted: the writer (and any open archive DB handle within its debounce window) lingers for the process lifetime. 2. Expiry pruning runs only on archive (re)open, and reopens happen only on writes — an archive for a webhook that stops receiving events is never swept, so expired rows persist indefinitely. ## Definition of done - Deleting a webhook closes and removes its `archiveWriter` from the registry (and the reaper/deletion path is covered by a test). - Idle archives with a configured expiry get pruned without requiring a new write (periodic sweep, or prune scheduled off the debounce timer) — with a test proving rows older than expiry disappear from an idle archive. - No behavior change for archives with expiry "never".
clawbot commented 2026-08-09 04:05:21 +02:00
Author
Collaborator
Copy Link

Implementation requirements

Baseline: main @ 4f5ecb1 (#84 merged as ee7c626, so the archiving target is in place).

Current state, confirmed by reading

  • The registry is databaseTarget.writers map[string]*archiveWriter guarded by databaseTarget.mu (internal/delivery/target_database.go:22-27), populated lazily by writerFor and never deleted from.
  • archiveWriter (internal/delivery/target_database_archive.go:156) holds mu, db, lastReopen, debounce, and already has a close() method that nils the handle. Pruning happens only inside open(), which is only reached from reopen(), which is only reached from write().
  • Webhook deletion runs through Handlers.deleteWebhookResources (internal/handlers/source_management.go:492), which soft-deletes entrypoints/targets/webhook and hard-deletes the event DB via WebhookDBManager.DeleteDB. Nothing in that path knows the delivery engine exists.
  • WebhookDBManager.DeleteDB removes events-{id}.db plus its -wal/-shm siblings. It does not touch archive-{id}.db.

1. Evict the writer on webhook deletion

Add an explicit eviction entry point on the delivery side that closes the writer's handle under the writer's own mutex (so it cannot race an in-flight write) and removes it from the writers map under databaseTarget.mu. Call it from deleteWebhookResources.

Plumbing: HandlersParams currently injects Notifier delivery.Notifier. Do not widen Notifier — archiving lifecycle is not notification. Add a separate narrow interface (e.g. delivery.WebhookEvictor with a single EvictWebhook(webhookID string) method), provide it from the delivery fx module, and inject it into HandlersParams. A one-method interface keeps the handler package from depending on the engine's internals and keeps it trivially fakeable in tests.

Eviction must be idempotent and must not error when no writer exists for that webhook (the common case — a webhook with no database target never creates one).

Also consider HandleTargetDelete: deleting the last database target for a webhook leaves a live writer behind. Either evict there too or state in the PR body why you did not (a subsequent webhook deletion will collect it, and a re-added target would just recreate the writer). Make it a deliberate choice, not an omission.

2. Do NOT delete the archive file

Eviction closes the handle and drops the map entry. It must not delete archive-{webhookID}.db. The archive is explicitly long-term storage that an operator may move away for offline retention; destroying it as a side effect of deleting a webhook would be surprising and unrecoverable. Document this in the README next to the existing database-target documentation: deleting a webhook releases the archive but leaves the file on disk for the operator to handle.

@sneak — this is the one judgement call in this issue and it is easy to reverse. I chose "keep the data" because silent, unrecoverable data destruction is never the right default. Say the word if you would rather webhook deletion remove the archive file too.

3. Sweep idle archives

Follow the RetentionReaper pattern in internal/database/retention.go exactly: an fx-managed component with OnStart/OnStop hooks, a cancellable context, a sync.WaitGroup, and a ticker loop that exits cleanly on cancellation.

Reuse the existing Config.RetentionSweepInterval. Do not add a new environment variable. Both are retention sweeps, the semantics match, and — importantly — PR #92 (#80) is concurrently rewriting every helper in internal/config. Adding a config key here would create a pointless merge conflict. If you believe a separate interval is genuinely required, say so in the PR body rather than adding one unilaterally.

Sweep behaviour:

  • Enumerate webhooks that have a database target whose config carries a positive expiry. Skip missing/empty/never expiry entirely — the issue requires no behaviour change for those.
  • Never create an archive file that does not already exist. open() uses mode=rwc, so a naive reuse of it will conjure empty archive-*.db files for every webhook that has a database target but has never received an event. Check for file existence first, or open read-write-without-create. Call out in the PR body which approach you took, and add a test that a webhook with a database target and no archive file has no file created by a sweep.
  • Serialise against writes. Do not open the archive file behind the writer's back. Route the prune through the per-webhook archiveWriter so its mu orders the sweep against concurrent write calls. Do not leave a handle open after an idle sweep — an idle archive should end the sweep closed, so the operator's move-the-file-away workflow keeps working.
  • A prune failure for one webhook must be logged and must not abort the sweep for the others, matching how prune already treats errors as non-fatal.

4. Tests

  • Deleting a webhook evicts its writer: assert the map entry is gone and the handle closed. Exercise it through the deletion path, not just by calling the evictor directly.
  • Eviction of a webhook with no writer is a no-op and does not panic.
  • An idle archive with a positive expiry has rows older than the expiry removed by a sweep with no intervening write — this is the core regression test, so make it fail without the fix.
  • An archive with expiry never is untouched by a sweep.
  • A webhook with a database target but no existing archive file gets no file created by a sweep.
  • The sweep loop stops cleanly on OnStop (no goroutine leak); the repo runs tests with -race, so concurrent write-plus-sweep coverage is worth having.

5. Docs

  • README: idle-sweep behaviour and the retained-file-on-deletion semantics from item 2.
  • TODO.md updated in the same commit as the code.

Definition of done

Everything in the issue's own Definition of done, plus: no new config key, no archive file deleted, no archive file created by a sweep, make check green via the repo's own entrypoints only, .golangci.yml untouched, single commit whose title ends with (closes #89), no attribution trailers.

## Implementation requirements Baseline: `main` @ `4f5ecb1` (#84 merged as `ee7c626`, so the archiving target is in place). ### Current state, confirmed by reading - The registry is `databaseTarget.writers map[string]*archiveWriter` guarded by `databaseTarget.mu` (`internal/delivery/target_database.go:22-27`), populated lazily by `writerFor` and **never** deleted from. - `archiveWriter` (`internal/delivery/target_database_archive.go:156`) holds `mu`, `db`, `lastReopen`, `debounce`, and already has a `close()` method that nils the handle. Pruning happens only inside `open()`, which is only reached from `reopen()`, which is only reached from `write()`. - Webhook deletion runs through `Handlers.deleteWebhookResources` (`internal/handlers/source_management.go:492`), which soft-deletes entrypoints/targets/webhook and hard-deletes the event DB via `WebhookDBManager.DeleteDB`. Nothing in that path knows the delivery engine exists. - `WebhookDBManager.DeleteDB` removes `events-{id}.db` plus its `-wal`/`-shm` siblings. It does not touch `archive-{id}.db`. ### 1. Evict the writer on webhook deletion Add an explicit eviction entry point on the delivery side that closes the writer's handle **under the writer's own mutex** (so it cannot race an in-flight `write`) and removes it from the `writers` map under `databaseTarget.mu`. Call it from `deleteWebhookResources`. Plumbing: `HandlersParams` currently injects `Notifier delivery.Notifier`. Do **not** widen `Notifier` — archiving lifecycle is not notification. Add a separate narrow interface (e.g. `delivery.WebhookEvictor` with a single `EvictWebhook(webhookID string)` method), provide it from the delivery fx module, and inject it into `HandlersParams`. A one-method interface keeps the handler package from depending on the engine's internals and keeps it trivially fakeable in tests. Eviction must be idempotent and must not error when no writer exists for that webhook (the common case — a webhook with no database target never creates one). Also consider `HandleTargetDelete`: deleting the last `database` target for a webhook leaves a live writer behind. Either evict there too or state in the PR body why you did not (a subsequent webhook deletion will collect it, and a re-added target would just recreate the writer). Make it a deliberate choice, not an omission. ### 2. Do NOT delete the archive file Eviction closes the handle and drops the map entry. It must **not** delete `archive-{webhookID}.db`. The archive is explicitly long-term storage that an operator may move away for offline retention; destroying it as a side effect of deleting a webhook would be surprising and unrecoverable. Document this in the README next to the existing database-target documentation: deleting a webhook releases the archive but leaves the file on disk for the operator to handle. @sneak — this is the one judgement call in this issue and it is easy to reverse. I chose "keep the data" because silent, unrecoverable data destruction is never the right default. Say the word if you would rather webhook deletion remove the archive file too. ### 3. Sweep idle archives Follow the `RetentionReaper` pattern in `internal/database/retention.go` exactly: an fx-managed component with `OnStart`/`OnStop` hooks, a cancellable context, a `sync.WaitGroup`, and a ticker loop that exits cleanly on cancellation. **Reuse the existing `Config.RetentionSweepInterval`. Do not add a new environment variable.** Both are retention sweeps, the semantics match, and — importantly — PR #92 (#80) is concurrently rewriting every helper in `internal/config`. Adding a config key here would create a pointless merge conflict. If you believe a separate interval is genuinely required, say so in the PR body rather than adding one unilaterally. Sweep behaviour: - Enumerate webhooks that have a `database` target whose config carries a **positive** expiry. Skip missing/empty/`never` expiry entirely — the issue requires no behaviour change for those. - **Never create an archive file that does not already exist.** `open()` uses `mode=rwc`, so a naive reuse of it will conjure empty `archive-*.db` files for every webhook that has a database target but has never received an event. Check for file existence first, or open read-write-without-create. Call out in the PR body which approach you took, and add a test that a webhook with a database target and no archive file has no file created by a sweep. - **Serialise against writes.** Do not open the archive file behind the writer's back. Route the prune through the per-webhook `archiveWriter` so its `mu` orders the sweep against concurrent `write` calls. Do not leave a handle open after an idle sweep — an idle archive should end the sweep closed, so the operator's move-the-file-away workflow keeps working. - A prune failure for one webhook must be logged and must not abort the sweep for the others, matching how `prune` already treats errors as non-fatal. ### 4. Tests - Deleting a webhook evicts its writer: assert the map entry is gone and the handle closed. Exercise it through the deletion path, not just by calling the evictor directly. - Eviction of a webhook with no writer is a no-op and does not panic. - An **idle** archive with a positive expiry has rows older than the expiry removed by a sweep with no intervening write — this is the core regression test, so make it fail without the fix. - An archive with expiry `never` is untouched by a sweep. - A webhook with a database target but no existing archive file gets no file created by a sweep. - The sweep loop stops cleanly on `OnStop` (no goroutine leak); the repo runs tests with `-race`, so concurrent write-plus-sweep coverage is worth having. ### 5. Docs - README: idle-sweep behaviour and the retained-file-on-deletion semantics from item 2. - `TODO.md` updated in the **same commit** as the code. ### Definition of done Everything in the issue's own Definition of done, plus: no new config key, no archive file deleted, no archive file created by a sweep, `make check` green via the repo's own entrypoints only, `.golangci.yml` untouched, single commit whose title ends with ` (closes #89)`, no attribution trailers.
clawbot commented 2026-08-09 04:09:27 +02:00
Author
Collaborator
Copy Link

Implementation plan

Branch issue-89-archive-lifecycle off main @ 4f5ecb1. Single commit ending in (closes #89).

1. Eviction plumbing

  • internal/delivery/engine.go: new one-method interface

    type WebhookEvictor interface {
    EvictWebhook(webhookID string)
}

    Notifier stays untouched — archiving lifecycle is not notification. Engine gains EvictWebhook, and initTargets retains the *databaseTarget in a field (same pattern as the existing httpTarget field) so the engine can reach the registry.

  • internal/delivery/target_database.go: (*databaseTarget).evict(webhookID) removes the map entry under databaseTarget.mu, releases that lock, then closes the handle under the writer's own mu so it cannot race an in-flight write. Idempotent: unknown webhook id is a no-op.

  • archiveWriter gains an evicted flag set under mu at eviction. A write that was already blocked on mu when eviction happened completes, but a subsequent write on the detached writer returns an error instead of reopening the file behind the registry's back (the handle would otherwise leak on an object nobody holds).

  • cmd/webhooker/main.go provides *delivery.Engine as delivery.WebhookEvictor; HandlersParams gains Evictor delivery.WebhookEvictor.

  • deleteWebhookResources calls h.evictor.EvictWebhook(webhook.ID) after the config-deletion transaction commits. No archive file is deleted.

2. HandleTargetDelete

I will evict there too, via the shared deleteChildResource helper gaining an optional after-delete hook. The hook counts the webhook's remaining (non-soft-deleted) database targets and evicts only when the count reaches zero. That is correct for both child types without inspecting what was deleted: deleting a non-database target while a database target remains leaves the writer alone, and deleting a non-database target on a webhook that has no database target evicts nothing because no writer exists. Rationale goes in the PR body.

3. Idle sweep

New internal/delivery/archive_sweeper.go, modelled directly on internal/database/retention.go: fx params struct, OnStart/OnStop, cancellable context, sync.WaitGroup, ticker loop. Interval is the existing Config.RetentionSweepInterval — no new config key.

Per tick: list non-deleted database targets from the main DB; parseArchiveExpiry each config; skip anything non-positive (so never/empty/missing behave exactly as today); then, per webhook:

  1. Compute archive-{id}.db and stat it first — a missing file is skipped before any writer or handle exists.
  2. Fetch the per-webhook archiveWriter from the registry and call w.sweepExpired(expiry), which takes the writer's mu for the whole operation, so the sweep is ordered against concurrent write calls.
  3. Inside the lock: re-check existence under the lock, close any live handle, reopen with mode=rw (not rwc) so SQLite cannot create the file, prune, and close again. The archive ends the sweep closed, preserving the operator's move-the-file-away workflow. open is refactored into a mode-parameterised helper; the write path keeps mode=rwc.

Per-webhook failures are logged and the loop continues, matching prune's existing non-fatal treatment.

main.go provides and invokes the sweeper.

4. Tests

  • Webhook deletion evicts: driven through HandleSourceDelete with a recording fake evictor (deletion path, not a direct call).
  • EvictWebhook closes the handle and drops the map entry; eviction of an unknown webhook is a no-op.
  • Core regression: an idle archive with {"expiry":"1h"} and a row stamped older than the expiry loses that row to a sweep with no intervening write; a fresh row survives. Fails without the fix.
  • {"expiry":"never"} archive is untouched.
  • Database target with no archive file: no archive-*.db (nor -wal/-shm) created by a sweep.
  • Sweep leaves the handle closed.
  • Concurrent write-plus-sweep (-race) and clean OnStop with no leaked goroutine.

5. Docs

README archive section: idle sweep semantics, and that deleting a webhook releases the archive handle but deliberately leaves archive-{id}.db on disk for the operator. TODO.md in the same commit.

Verification: make fmt then script/cibuild.

## Implementation plan Branch `issue-89-archive-lifecycle` off `main` @ `4f5ecb1`. Single commit ending in ` (closes #89)`. ### 1. Eviction plumbing - `internal/delivery/engine.go`: new one-method interface ``` type WebhookEvictor interface { EvictWebhook(webhookID string) } ``` `Notifier` stays untouched — archiving lifecycle is not notification. `Engine` gains `EvictWebhook`, and `initTargets` retains the `*databaseTarget` in a field (same pattern as the existing `httpTarget` field) so the engine can reach the registry. - `internal/delivery/target_database.go`: `(*databaseTarget).evict(webhookID)` removes the map entry under `databaseTarget.mu`, releases that lock, then closes the handle under the writer's own `mu` so it cannot race an in-flight `write`. Idempotent: unknown webhook id is a no-op. - `archiveWriter` gains an `evicted` flag set under `mu` at eviction. A `write` that was already blocked on `mu` when eviction happened completes, but a subsequent `write` on the detached writer returns an error instead of reopening the file behind the registry's back (the handle would otherwise leak on an object nobody holds). - `cmd/webhooker/main.go` provides `*delivery.Engine` as `delivery.WebhookEvictor`; `HandlersParams` gains `Evictor delivery.WebhookEvictor`. - `deleteWebhookResources` calls `h.evictor.EvictWebhook(webhook.ID)` after the config-deletion transaction commits. No archive file is deleted. ### 2. `HandleTargetDelete` I will evict there too, via the shared `deleteChildResource` helper gaining an optional after-delete hook. The hook counts the webhook's remaining (non-soft-deleted) `database` targets and evicts only when the count reaches zero. That is correct for both child types without inspecting what was deleted: deleting a non-database target while a database target remains leaves the writer alone, and deleting a non-database target on a webhook that has no database target evicts nothing because no writer exists. Rationale goes in the PR body. ### 3. Idle sweep New `internal/delivery/archive_sweeper.go`, modelled directly on `internal/database/retention.go`: fx params struct, `OnStart`/`OnStop`, cancellable context, `sync.WaitGroup`, ticker loop. Interval is the existing `Config.RetentionSweepInterval` — no new config key. Per tick: list non-deleted `database` targets from the main DB; `parseArchiveExpiry` each config; skip anything non-positive (so `never`/empty/missing behave exactly as today); then, per webhook: 1. Compute `archive-{id}.db` and **stat it first** — a missing file is skipped before any writer or handle exists. 2. Fetch the per-webhook `archiveWriter` from the registry and call `w.sweepExpired(expiry)`, which takes the writer's `mu` for the whole operation, so the sweep is ordered against concurrent `write` calls. 3. Inside the lock: re-check existence under the lock, close any live handle, reopen with `mode=rw` (**not** `rwc`) so SQLite cannot create the file, prune, and close again. The archive ends the sweep **closed**, preserving the operator's move-the-file-away workflow. `open` is refactored into a mode-parameterised helper; the write path keeps `mode=rwc`. Per-webhook failures are logged and the loop continues, matching `prune`'s existing non-fatal treatment. `main.go` provides and invokes the sweeper. ### 4. Tests - Webhook deletion evicts: driven through `HandleSourceDelete` with a recording fake evictor (deletion path, not a direct call). - `EvictWebhook` closes the handle and drops the map entry; eviction of an unknown webhook is a no-op. - Core regression: an **idle** archive with `{"expiry":"1h"}` and a row stamped older than the expiry loses that row to a sweep with no intervening write; a fresh row survives. Fails without the fix. - `{"expiry":"never"}` archive is untouched. - Database target with no archive file: no `archive-*.db` (nor `-wal`/`-shm`) created by a sweep. - Sweep leaves the handle closed. - Concurrent write-plus-sweep (`-race`) and clean `OnStop` with no leaked goroutine. ### 5. Docs README archive section: idle sweep semantics, and that deleting a webhook releases the archive handle but deliberately leaves `archive-{id}.db` on disk for the operator. `TODO.md` in the same commit. Verification: `make fmt` then `script/cibuild`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#89
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?