Enforces each webhook's RetentionDays so per-webhook SQLite files no longer grow without bound.
Reaper
New RetentionReaper in internal/database/retention.go. A background ticker runs each sweep: it lists all webhooks from the main DB and, for each webhook with a positive RetentionDays, opens its per-webhook DB via WebhookDBManager.GetDB and deletes every Event (and its dependent Delivery and DeliveryResult rows) whose CreatedAt is older than RetentionDays days.
Deletions run in foreign-key-safe order: delivery results, then deliveries, then events.
Deletes are unscoped (hard deletes) so rows are physically removed and disk is reclaimed, rather than GORM soft-deleting them.
RetentionDays <= 0 means retain forever; those webhooks are skipped.
Webhooks whose per-webhook DB does not yet exist are skipped.
Config
internal/config/config.go gains RetentionSweepInterval (env RETENTION_SWEEP_INTERVAL, parsed as a Go duration, default 1h) via a new envDuration helper, following the existing env-helper conventions.
Wiring
cmd/webhooker/main.go registers database.NewRetentionReaper as an fx provider and forces its construction in fx.Invoke. The reaper starts its sweep loop on an fx OnStart hook and stops cleanly on OnStop via context cancellation, matching the existing lifecycle components.
Test
internal/database/retention_test.go seeds an old event chain (event + delivery + result, 40 days old) and a recent one (1 day old) in a real per-webhook DB and asserts a single sweep removes only the expired chain while keeping the recent one. A second test forces a non-positive RetentionDays and asserts an ancient event is retained.
Note: the Webhook.RetentionDays column carries gorm:"default:30", so a 0 passed to a GORM Create is replaced by the default; the test forces the value with an explicit column update to exercise the retain-forever path. No model changes were made.
Enforces each webhook's `RetentionDays` so per-webhook SQLite files no longer grow without bound.
## Reaper
New `RetentionReaper` in `internal/database/retention.go`. A background ticker runs each sweep: it lists all webhooks from the main DB and, for each webhook with a positive `RetentionDays`, opens its per-webhook DB via `WebhookDBManager.GetDB` and deletes every `Event` (and its dependent `Delivery` and `DeliveryResult` rows) whose `CreatedAt` is older than `RetentionDays` days.
- Deletions run in foreign-key-safe order: delivery results, then deliveries, then events.
- Deletes are unscoped (hard deletes) so rows are physically removed and disk is reclaimed, rather than GORM soft-deleting them.
- `RetentionDays <= 0` means retain forever; those webhooks are skipped.
- Webhooks whose per-webhook DB does not yet exist are skipped.
## Config
`internal/config/config.go` gains `RetentionSweepInterval` (env `RETENTION_SWEEP_INTERVAL`, parsed as a Go duration, default `1h`) via a new `envDuration` helper, following the existing env-helper conventions.
## Wiring
`cmd/webhooker/main.go` registers `database.NewRetentionReaper` as an fx provider and forces its construction in `fx.Invoke`. The reaper starts its sweep loop on an fx `OnStart` hook and stops cleanly on `OnStop` via context cancellation, matching the existing lifecycle components.
## Test
`internal/database/retention_test.go` seeds an old event chain (event + delivery + result, 40 days old) and a recent one (1 day old) in a real per-webhook DB and asserts a single sweep removes only the expired chain while keeping the recent one. A second test forces a non-positive `RetentionDays` and asserts an ancient event is retained.
Note: the `Webhook.RetentionDays` column carries `gorm:"default:30"`, so a `0` passed to a GORM `Create` is replaced by the default; the test forces the value with an explicit column update to exercise the retain-forever path. No model changes were made.
Validated with `docker build .` (fmt-check, lint, test, build) exit 0.
Closes #63
Added a per-webhook event retention reaper that enforces RetentionDays.
Files changed (5, +604/-5):
internal/database/retention.go (new): RetentionReaper with fx OnStart/OnStop hooks, a ticker-driven run/sweep, and reapExpired which hard-deletes (unscoped) delivery results, then deliveries, then events older than the cutoff, in FK-safe order. Skips webhooks with RetentionDays <= 0 and those with no per-webhook DB yet.
internal/config/config.go: new RetentionSweepInterval field (env RETENTION_SWEEP_INTERVAL, default 1h) plus an envDuration helper and log-summary line.
cmd/webhooker/main.go: registers database.NewRetentionReaper as a provider and constructs it via fx.Invoke.
internal/database/retention_test.go (new): seeds old (40d) and recent (1d) event+delivery+result chains and asserts only the expired chain is removed; a second test forces a non-positive RetentionDays and asserts retain-forever.
internal/database/export_test.go (new): test-only bridge exposing a reaper constructor and a synchronous single-sweep trigger.
None of routes.go, http.go, engine.go, source_management.go, or middleware.go were touched.
Validation
docker build . (fmt-check, lint, test, build):
docker build exit code: 0
### What changed
Added a per-webhook event retention reaper that enforces `RetentionDays`.
Files changed (5, +604/-5):
- `internal/database/retention.go` (new): `RetentionReaper` with fx `OnStart`/`OnStop` hooks, a ticker-driven `run`/`sweep`, and `reapExpired` which hard-deletes (unscoped) delivery results, then deliveries, then events older than the cutoff, in FK-safe order. Skips webhooks with `RetentionDays <= 0` and those with no per-webhook DB yet.
- `internal/config/config.go`: new `RetentionSweepInterval` field (env `RETENTION_SWEEP_INTERVAL`, default `1h`) plus an `envDuration` helper and log-summary line.
- `cmd/webhooker/main.go`: registers `database.NewRetentionReaper` as a provider and constructs it via `fx.Invoke`.
- `internal/database/retention_test.go` (new): seeds old (40d) and recent (1d) event+delivery+result chains and asserts only the expired chain is removed; a second test forces a non-positive `RetentionDays` and asserts retain-forever.
- `internal/database/export_test.go` (new): test-only bridge exposing a reaper constructor and a synchronous single-sweep trigger.
None of `routes.go`, `http.go`, `engine.go`, `source_management.go`, or `middleware.go` were touched.
### Validation
`docker build .` (fmt-check, lint, test, build):
```
docker build exit code: 0
```
Adversarial review against the issue spec and repo policies.
internal/database/retention.go: RetentionReaper runs a ticker sweep (configurable RetentionSweepInterval, env RETENTION_SWEEP_INTERVAL, default 1h) started on fx OnStart and stopped cleanly on OnStop via context cancel plus a WaitGroup. Each sweep lists webhooks, skips a RetentionDays of 0 or less (retain forever) and webhooks with no per-webhook DB yet, and reaps the rest; it also checks context cancellation between webhooks.
reapExpired hard-deletes (Unscoped) in foreign-key-safe order — delivery results, then deliveries, then events — for events older than the cutoff, building fresh subquery builders per statement. Good catch on the soft-delete trap: BaseModel has a DeletedAt, so a plain Delete would only soft-delete and never reclaim disk; Unscoped physically removes the rows.
Wiring: the NewRetentionReaper fx provider is registered and constructed in fx.Invoke (cmd/webhooker/main.go); the new envDuration config helper follows the existing env-helper conventions.
Tests: seed a 40-day-old and a 1-day-old event/delivery/result chain under a 30-day policy and assert only the expired chain is reaped; a second test asserts retain-forever for a non-positive RetentionDays (forcing the value past GORM's default-of-30 with an explicit column update). docker build . green; no AI/tooling references; commit closes the issue.
Scope respected: none of the in-flight files (engine.go, routes.go, etc.) were touched.
One follow-up, not blocking this PR: a RetentionDays of 0 is unreachable through a normal create because the column default is 30, so "retain forever" cannot currently be expressed via the create path. Filed separately.
Verdict: meets the bar. Marking merge-ready and handing to @sneak for final review.
## Independent review — PASS (merge-ready)
Adversarial review against the issue spec and repo policies.
- `internal/database/retention.go`: `RetentionReaper` runs a ticker sweep (configurable `RetentionSweepInterval`, env `RETENTION_SWEEP_INTERVAL`, default 1h) started on fx `OnStart` and stopped cleanly on `OnStop` via context cancel plus a `WaitGroup`. Each sweep lists webhooks, skips a `RetentionDays` of 0 or less (retain forever) and webhooks with no per-webhook DB yet, and reaps the rest; it also checks context cancellation between webhooks.
- `reapExpired` hard-deletes (`Unscoped`) in foreign-key-safe order — delivery results, then deliveries, then events — for events older than the cutoff, building fresh subquery builders per statement. Good catch on the soft-delete trap: `BaseModel` has a `DeletedAt`, so a plain `Delete` would only soft-delete and never reclaim disk; `Unscoped` physically removes the rows.
- Wiring: the `NewRetentionReaper` fx provider is registered and constructed in `fx.Invoke` (`cmd/webhooker/main.go`); the new `envDuration` config helper follows the existing env-helper conventions.
- Tests: seed a 40-day-old and a 1-day-old event/delivery/result chain under a 30-day policy and assert only the expired chain is reaped; a second test asserts retain-forever for a non-positive `RetentionDays` (forcing the value past GORM's default-of-30 with an explicit column update). `docker build .` green; no AI/tooling references; commit closes the issue.
- Scope respected: none of the in-flight files (`engine.go`, `routes.go`, etc.) were touched.
One follow-up, not blocking this PR: a `RetentionDays` of 0 is unreachable through a normal create because the column default is 30, so "retain forever" cannot currently be expressed via the create path. Filed separately.
Verdict: meets the bar. Marking merge-ready and handing to @sneak for final review.
sneak
was assigned by clawbot2026-08-07 15:26:39 +02:00
You're right, and I should have caught this in review — my miss. envDuration silently falls back to the default when RETENTION_SWEEP_INTERVAL is set but unparseable; a bad config value must fail loudly and prevent startup, not be silently ignored.
Sending it back for rework: envDuration will return an error on a set-but-unparseable value, and config.New will propagate it so startup aborts. An unset value still uses the default. Reassigning to clawbot — off your plate until it is fixed and re-reviewed.
(Also filing the same silent-default pattern in the other env helpers as a follow-up.)
You're right, and I should have caught this in review — my miss. `envDuration` silently falls back to the default when `RETENTION_SWEEP_INTERVAL` is set but unparseable; a bad config value must fail loudly and prevent startup, not be silently ignored.
Sending it back for rework: `envDuration` will return an error on a set-but-unparseable value, and `config.New` will propagate it so startup aborts. An unset value still uses the default. Reassigning to `clawbot` — off your plate until it is fixed and re-reviewed.
(Also filing the same silent-default pattern in the other env helpers as a follow-up.)
sneak
was unassigned by clawbot2026-08-07 15:29:18 +02:00
clawbot
self-assigned this 2026-08-07 15:29:18 +02:00
Reworked per review: a set-but-unparseable RETENTION_SWEEP_INTERVAL now fails startup loudly instead of silently falling back to the default.
Changes (in internal/config/):
envDuration(key, defaultValue) now returns (time.Duration, error). If the env var is unset it returns (defaultValue, nil); if set and valid, (parsed, nil); if set but time.ParseDuration fails, it returns a wrapped error naming the key and the bad value (invalid duration for %s: %q: %w).
config.New calls the new envDuration for RETENTION_SWEEP_INTERVAL and returns any error, so fx aborts startup rather than swallowing it.
envInt/envString are deliberately left unchanged (separate follow-up).
Added TestRetentionSweepInterval covering: unset uses the 1h default, a valid 15m is parsed, and an unparseable not-a-duration makes config.New fail (asserted via app.Err()). Env vars are set/unset hermetically with t.Setenv / os.Unsetenv.
#32 writing image sha256:64505e6a4b6bdc50c64973babdbafe9dbba4d0e4292b25715fdbd812946b81cf done
#32 naming to docker.io/library/webhooker-issue63-rework done
Pushed to issue-63-retention-reaper as d0e73c7.
Reworked per review: a set-but-unparseable `RETENTION_SWEEP_INTERVAL` now fails startup loudly instead of silently falling back to the default.
Changes (in `internal/config/`):
- `envDuration(key, defaultValue)` now returns `(time.Duration, error)`. If the env var is unset it returns `(defaultValue, nil)`; if set and valid, `(parsed, nil)`; if set but `time.ParseDuration` fails, it returns a wrapped error naming the key and the bad value (`invalid duration for %s: %q: %w`).
- `config.New` calls the new `envDuration` for `RETENTION_SWEEP_INTERVAL` and returns any error, so fx aborts startup rather than swallowing it.
- `envInt`/`envString` are deliberately left unchanged (separate follow-up).
- Added `TestRetentionSweepInterval` covering: unset uses the `1h` default, a valid `15m` is parsed, and an unparseable `not-a-duration` makes `config.New` fail (asserted via `app.Err()`). Env vars are set/unset hermetically with `t.Setenv` / `os.Unsetenv`.
Validation: `docker build` (fmt-check, lint, test, build) exits 0:
```
#32 writing image sha256:64505e6a4b6bdc50c64973babdbafe9dbba4d0e4292b25715fdbd812946b81cf done
#32 naming to docker.io/library/webhooker-issue63-rework done
```
Pushed to `issue-63-retention-reaper` as `d0e73c7`.
Verified the fail-loud fix on internal/config/config.go:
envDuration now returns (time.Duration, error): an unset variable returns the default with no error; a set-and-valid value is parsed; a set-but-unparseable value returns a wrapped error naming the key and the bad value (invalid duration for %s: %q).
config.New calls it for RETENTION_SWEEP_INTERVAL and returns the error before constructing the config, so fx aborts startup instead of silently falling back to the default.
The test covers all three cases — unset to default, valid parsed, and unparseable causing config.New to error — set hermetically via t.Setenv. docker build . green.
The remaining silently-defaulting helpers (envInt, envBool) are tracked separately in #80, out of scope for this PR.
Verdict: the flagged defect is resolved. Re-assigning to @sneak for final review.
## Re-review after rework — PASS (merge-ready)
Verified the fail-loud fix on `internal/config/config.go`:
- `envDuration` now returns `(time.Duration, error)`: an unset variable returns the default with no error; a set-and-valid value is parsed; a set-but-unparseable value returns a wrapped error naming the key and the bad value (`invalid duration for %s: %q`).
- `config.New` calls it for `RETENTION_SWEEP_INTERVAL` and returns the error before constructing the config, so fx aborts startup instead of silently falling back to the default.
- The test covers all three cases — unset to default, valid parsed, and unparseable causing `config.New` to error — set hermetically via `t.Setenv`. `docker build .` green.
The remaining silently-defaulting helpers (`envInt`, `envBool`) are tracked separately in #80, out of scope for this PR.
Verdict: the flagged defect is resolved. Re-assigning to @sneak for final review.
clawbot
removed their assignment 2026-08-07 15:46:53 +02:00
sneak
was assigned by clawbot2026-08-07 15:46:53 +02:00
sneak
merged commit f6b929f2d7 into main2026-08-07 16:15:14 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Enforces each webhook's
RetentionDaysso per-webhook SQLite files no longer grow without bound.Reaper
New
RetentionReaperininternal/database/retention.go. A background ticker runs each sweep: it lists all webhooks from the main DB and, for each webhook with a positiveRetentionDays, opens its per-webhook DB viaWebhookDBManager.GetDBand deletes everyEvent(and its dependentDeliveryandDeliveryResultrows) whoseCreatedAtis older thanRetentionDaysdays.RetentionDays <= 0means retain forever; those webhooks are skipped.Config
internal/config/config.gogainsRetentionSweepInterval(envRETENTION_SWEEP_INTERVAL, parsed as a Go duration, default1h) via a newenvDurationhelper, following the existing env-helper conventions.Wiring
cmd/webhooker/main.goregistersdatabase.NewRetentionReaperas an fx provider and forces its construction infx.Invoke. The reaper starts its sweep loop on an fxOnStarthook and stops cleanly onOnStopvia context cancellation, matching the existing lifecycle components.Test
internal/database/retention_test.goseeds an old event chain (event + delivery + result, 40 days old) and a recent one (1 day old) in a real per-webhook DB and asserts a single sweep removes only the expired chain while keeping the recent one. A second test forces a non-positiveRetentionDaysand asserts an ancient event is retained.Note: the
Webhook.RetentionDayscolumn carriesgorm:"default:30", so a0passed to a GORMCreateis replaced by the default; the test forces the value with an explicit column update to exercise the retain-forever path. No model changes were made.Validated with
docker build .(fmt-check, lint, test, build) exit 0.Closes #63
What changed
Added a per-webhook event retention reaper that enforces
RetentionDays.Files changed (5, +604/-5):
internal/database/retention.go(new):RetentionReaperwith fxOnStart/OnStophooks, a ticker-drivenrun/sweep, andreapExpiredwhich hard-deletes (unscoped) delivery results, then deliveries, then events older than the cutoff, in FK-safe order. Skips webhooks withRetentionDays <= 0and those with no per-webhook DB yet.internal/config/config.go: newRetentionSweepIntervalfield (envRETENTION_SWEEP_INTERVAL, default1h) plus anenvDurationhelper and log-summary line.cmd/webhooker/main.go: registersdatabase.NewRetentionReaperas a provider and constructs it viafx.Invoke.internal/database/retention_test.go(new): seeds old (40d) and recent (1d) event+delivery+result chains and asserts only the expired chain is removed; a second test forces a non-positiveRetentionDaysand asserts retain-forever.internal/database/export_test.go(new): test-only bridge exposing a reaper constructor and a synchronous single-sweep trigger.None of
routes.go,http.go,engine.go,source_management.go, ormiddleware.gowere touched.Validation
docker build .(fmt-check, lint, test, build):this should fail loudly and prevent server startup if it can't parse the duration string in the env var. your implementation silently fails. bad.
Independent review — PASS (merge-ready)
Adversarial review against the issue spec and repo policies.
internal/database/retention.go:RetentionReaperruns a ticker sweep (configurableRetentionSweepInterval, envRETENTION_SWEEP_INTERVAL, default 1h) started on fxOnStartand stopped cleanly onOnStopvia context cancel plus aWaitGroup. Each sweep lists webhooks, skips aRetentionDaysof 0 or less (retain forever) and webhooks with no per-webhook DB yet, and reaps the rest; it also checks context cancellation between webhooks.reapExpiredhard-deletes (Unscoped) in foreign-key-safe order — delivery results, then deliveries, then events — for events older than the cutoff, building fresh subquery builders per statement. Good catch on the soft-delete trap:BaseModelhas aDeletedAt, so a plainDeletewould only soft-delete and never reclaim disk;Unscopedphysically removes the rows.NewRetentionReaperfx provider is registered and constructed infx.Invoke(cmd/webhooker/main.go); the newenvDurationconfig helper follows the existing env-helper conventions.RetentionDays(forcing the value past GORM's default-of-30 with an explicit column update).docker build .green; no AI/tooling references; commit closes the issue.engine.go,routes.go, etc.) were touched.One follow-up, not blocking this PR: a
RetentionDaysof 0 is unreachable through a normal create because the column default is 30, so "retain forever" cannot currently be expressed via the create path. Filed separately.Verdict: meets the bar. Marking merge-ready and handing to @sneak for final review.
You're right, and I should have caught this in review — my miss.
envDurationsilently falls back to the default whenRETENTION_SWEEP_INTERVALis set but unparseable; a bad config value must fail loudly and prevent startup, not be silently ignored.Sending it back for rework:
envDurationwill return an error on a set-but-unparseable value, andconfig.Newwill propagate it so startup aborts. An unset value still uses the default. Reassigning toclawbot— off your plate until it is fixed and re-reviewed.(Also filing the same silent-default pattern in the other env helpers as a follow-up.)
Reworked per review: a set-but-unparseable
RETENTION_SWEEP_INTERVALnow fails startup loudly instead of silently falling back to the default.Changes (in
internal/config/):envDuration(key, defaultValue)now returns(time.Duration, error). If the env var is unset it returns(defaultValue, nil); if set and valid,(parsed, nil); if set buttime.ParseDurationfails, it returns a wrapped error naming the key and the bad value (invalid duration for %s: %q: %w).config.Newcalls the newenvDurationforRETENTION_SWEEP_INTERVALand returns any error, so fx aborts startup rather than swallowing it.envInt/envStringare deliberately left unchanged (separate follow-up).TestRetentionSweepIntervalcovering: unset uses the1hdefault, a valid15mis parsed, and an unparseablenot-a-durationmakesconfig.Newfail (asserted viaapp.Err()). Env vars are set/unset hermetically witht.Setenv/os.Unsetenv.Validation:
docker build(fmt-check, lint, test, build) exits 0:Pushed to
issue-63-retention-reaperasd0e73c7.Re-review after rework — PASS (merge-ready)
Verified the fail-loud fix on
internal/config/config.go:envDurationnow returns(time.Duration, error): an unset variable returns the default with no error; a set-and-valid value is parsed; a set-but-unparseable value returns a wrapped error naming the key and the bad value (invalid duration for %s: %q).config.Newcalls it forRETENTION_SWEEP_INTERVALand returns the error before constructing the config, so fx aborts startup instead of silently falling back to the default.config.Newto error — set hermetically viat.Setenv.docker build .green.The remaining silently-defaulting helpers (
envInt,envBool) are tracked separately in #80, out of scope for this PR.Verdict: the flagged defect is resolved. Re-assigning to @sneak for final review.