Closes #206 — the first three
bullets of the definition of done, per the scope set in #206 (comment).
Encryption at rest is not in this change; it stays with #212.
The write path
Delivery declares belongs-to Event and Target, and the engine
fills both in memory (engine.goprocessNewTask, processRetryTask)
so the target implementations have the config to deliver with. The row
lands in the event database through updateDeliveryStatus:
Update puts a map in Statement.Dest, but GORM's gorm:setup_reflect_value callback then resets Statement.ReflectValue
to Statement.Model — the Delivery struct — so gorm:save_before_associations runs against the populated Target and
upserts it, config and all, with an empty webhook_id. Every
delivery reaches that call, success or failure, new or retry.
What this does
internal/database/event_db_isolation.go — omitAssociations
registers a callback on the create and update chains of every
per-webhook connection that appends clause.Associations to Statement.Omits, which is what SaveBeforeAssociations and SaveAfterAssociations check before writing a parent row. This is
done at the connection rather than at the one call site so it covers
every write path into the file — Create, Save, Updates, writes
inside the receive handler's transaction, and paths added later.
Nothing in the event tier depends on the automatic save; every row it
holds is written explicitly.
purgeTargetRows sweeps rows already written. It runs on each open,
before AutoMigrate, and is a no-op on a database with no targets
table (a fresh file at that point has none). Deleting is right: those
rows carry an empty webhook_id and nothing in the file refers to
them — deliveries resolve their target against the main database. modernc.org/sqlite leaves secure_delete at SQLite's default of
off, so the DELETE alone only unlinks the rows and the credential
bytes stay readable in the file's free pages; the sweep therefore
follows it with a VACUUM. A non-zero sweep is logged at warn with
the count.
The sweep is gated on a durable marker, PRAGMA user_version
(nothing else in the tree uses it), stamped only after the VACUUM returns. The marker, not the DELETE, is what records that
a file is done, because a DELETE commits on its own: a sweep that
is interrupted or whose VACUUM fails leaves a file whose rows are
gone but whose credential bytes are still in the free pages, and a
row count cannot tell that apart from a file that never leaked. Both
leave the marker unset, so the next open sweeps again. A failure
fails the open with the marker unset, so a webhook whose file cannot
be swept stays unusable rather than quietly serving from a file that
still holds recoverable credentials.
Cost: a file pays for the rewrite once, on the first open that finds
it unmarked; every open after that is a PRAGMA read. A file this
build created is marked before its targets table exists, so it
never vacuums at all. On upgrade each existing events-*.db is
rewritten once.
README: the backup-secrets bullet added by #210 said "until issue Target credentials leak into the per-webhook event databases via GORM association upsert (#206)
is fixed", which this makes untrue. It now states what the sweep
achieves (bytes removed, not rows unlinked), that a file this version
has opened without error holds no recoverable bytes because the
marker is only written after the vacuum, the one-time rewrite on
upgrade, and the cases that remain: backups from an older build,
backups of a file this version has not yet opened successfully, and
copies already taken, including freed blocks left in filesystem
snapshots.
Behaviour note
Post-fix the status update emits UPDATE deliveries SET status=..., updated_at=.... Pre-fix it also echoed event_id/target_id, because SaveBeforeAssociations wrote the resolved foreign keys into the
update map. The values were unchanged either way, so this is benign,
but the emitted SQL is not byte-identical to before.
Not changed: AutoMigrate still creates the empty targets table in
each event database, because Delivery declares the relation.
Suppressing it is a model-level change, not a migration-list change,
and it would not remove the sweep — AutoMigrate never drops, so every
existing event database keeps its targets table regardless.
Tests
internal/delivery/event_db_isolation_test.go drives real deliveries
through the engine and reads the file back with a separate read-only database/sql connection, outside GORM, so the assertion cannot be
masked by the omit:
TestEventDBHoldsNoTargetRows — a delivery, then a retry.
TestEventDBHoldsNoTargetRowsOnFailedDelivery — the failure path,
which takes a different status update.
internal/database/event_db_isolation_test.go:
TestOpenPurgesLeakedTargetRows — seeds a leaked row into a real
event database file, asserts the next open clears it and marks it,
and that a further open stays clean.
TestOpenPurgeRemovesCredentialBytes — seeds a recognisable
credential, asserts it is present in the raw file bytes, sweeps, and
asserts it is absent from the raw bytes. This is the assertion a row
count cannot make, and it fails without the VACUUM.
TestOpenRevacuumsAfterIncompleteSweep — rows already deleted,
marker unset, credential bytes still in the file: the exact state an
interrupted sweep leaves. Asserts the next open vacuums anyway.
TestOpenSkipsSweptDatabase — the other half of the marker: a marked
file is not swept again, and a file this build created is marked
without ever being vacuumed.
TestOpenSucceedsWithoutTargetsTable — an event database with no targets table opens without error.
TestEventDBCreateOmitsAssociations — the guard itself, on a Delivery carrying Event and Target.
Pre-fix reproduction, with the wiring in openDB reverted and
everything else in place:
event_db_isolation_test.go:96:
Error: Should be zero, but was 1
Test: TestEventDBHoldsNoTargetRows
Messages: per-webhook event database must hold no target rows
Gate
See the rework comment below for the current run, including the
pre-existing internal/gormlog failure on next itself
(#235), which is the only
failing package and is not from this branch.
Closes https://git.eeqj.de/sneak/webhooker/issues/206 — the first three
bullets of the definition of done, per the scope set in
https://git.eeqj.de/sneak/webhooker/issues/206#issuecomment-66713.
Encryption at rest is not in this change; it stays with
https://git.eeqj.de/sneak/webhooker/issues/212.
## The write path
`Delivery` declares belongs-to `Event` and `Target`, and the engine
fills both in memory (`engine.go` `processNewTask`, `processRetryTask`)
so the target implementations have the config to deliver with. The row
lands in the event database through `updateDeliveryStatus`:
```go
err := webhookDB.Model(d).
Update("status", status).Error
```
`Update` puts a map in `Statement.Dest`, but GORM's
`gorm:setup_reflect_value` callback then resets `Statement.ReflectValue`
to `Statement.Model` — the `Delivery` struct — so
`gorm:save_before_associations` runs against the populated `Target` and
upserts it, `config` and all, with an empty `webhook_id`. Every
delivery reaches that call, success or failure, new or retry.
## What this does
- `internal/database/event_db_isolation.go` — `omitAssociations`
registers a callback on the create and update chains of every
per-webhook connection that appends `clause.Associations` to
`Statement.Omits`, which is what `SaveBeforeAssociations` and
`SaveAfterAssociations` check before writing a parent row. This is
done at the connection rather than at the one call site so it covers
every write path into the file — `Create`, `Save`, `Updates`, writes
inside the receive handler's transaction, and paths added later.
Nothing in the event tier depends on the automatic save; every row it
holds is written explicitly.
- `purgeTargetRows` sweeps rows already written. It runs on each open,
before `AutoMigrate`, and is a no-op on a database with no `targets`
table (a fresh file at that point has none). Deleting is right: those
rows carry an empty `webhook_id` and nothing in the file refers to
them — deliveries resolve their target against the main database.
`modernc.org/sqlite` leaves `secure_delete` at SQLite's default of
off, so the `DELETE` alone only unlinks the rows and the credential
bytes stay readable in the file's free pages; the sweep therefore
follows it with a `VACUUM`. A non-zero sweep is logged at warn with
the count.
- The sweep is gated on a durable marker, `PRAGMA user_version`
(nothing else in the tree uses it), stamped **only after** the
`VACUUM` returns. The marker, not the `DELETE`, is what records that
a file is done, because a `DELETE` commits on its own: a sweep that
is interrupted or whose `VACUUM` fails leaves a file whose rows are
gone but whose credential bytes are still in the free pages, and a
row count cannot tell that apart from a file that never leaked. Both
leave the marker unset, so the next open sweeps again. A failure
fails the open with the marker unset, so a webhook whose file cannot
be swept stays unusable rather than quietly serving from a file that
still holds recoverable credentials.
- Cost: a file pays for the rewrite once, on the first open that finds
it unmarked; every open after that is a `PRAGMA` read. A file this
build created is marked before its `targets` table exists, so it
never vacuums at all. On upgrade each existing `events-*.db` is
rewritten once.
- README: the backup-secrets bullet added by
https://git.eeqj.de/sneak/webhooker/issues/210 said "until issue #206
is fixed", which this makes untrue. It now states what the sweep
achieves (bytes removed, not rows unlinked), that a file this version
has opened without error holds no recoverable bytes because the
marker is only written after the vacuum, the one-time rewrite on
upgrade, and the cases that remain: backups from an older build,
backups of a file this version has not yet opened successfully, and
copies already taken, including freed blocks left in filesystem
snapshots.
### Behaviour note
Post-fix the status update emits `UPDATE deliveries SET status=...,
updated_at=...`. Pre-fix it also echoed `event_id`/`target_id`, because
`SaveBeforeAssociations` wrote the resolved foreign keys into the
update map. The values were unchanged either way, so this is benign,
but the emitted SQL is not byte-identical to before.
Not changed: `AutoMigrate` still creates the empty `targets` table in
each event database, because `Delivery` declares the relation.
Suppressing it is a model-level change, not a migration-list change,
and it would not remove the sweep — `AutoMigrate` never drops, so every
existing event database keeps its `targets` table regardless.
## Tests
`internal/delivery/event_db_isolation_test.go` drives real deliveries
through the engine and reads the file back with a separate read-only
`database/sql` connection, outside GORM, so the assertion cannot be
masked by the omit:
- `TestEventDBHoldsNoTargetRows` — a delivery, then a retry.
- `TestEventDBHoldsNoTargetRowsOnFailedDelivery` — the failure path,
which takes a different status update.
`internal/database/event_db_isolation_test.go`:
- `TestOpenPurgesLeakedTargetRows` — seeds a leaked row into a real
event database file, asserts the next open clears it and marks it,
and that a further open stays clean.
- `TestOpenPurgeRemovesCredentialBytes` — seeds a recognisable
credential, asserts it is present in the raw file bytes, sweeps, and
asserts it is absent from the raw bytes. This is the assertion a row
count cannot make, and it fails without the `VACUUM`.
- `TestOpenRevacuumsAfterIncompleteSweep` — rows already deleted,
marker unset, credential bytes still in the file: the exact state an
interrupted sweep leaves. Asserts the next open vacuums anyway.
- `TestOpenSkipsSweptDatabase` — the other half of the marker: a marked
file is not swept again, and a file this build created is marked
without ever being vacuumed.
- `TestOpenSucceedsWithoutTargetsTable` — an event database with no
`targets` table opens without error.
- `TestEventDBCreateOmitsAssociations` — the guard itself, on a
`Delivery` carrying `Event` and `Target`.
Pre-fix reproduction, with the wiring in `openDB` reverted and
everything else in place:
```
event_db_isolation_test.go:96:
Error: Should be zero, but was 1
Test: TestEventDBHoldsNoTargetRows
Messages: per-webhook event database must hold no target rows
```
## Gate
See the rework comment below for the current run, including the
pre-existing `internal/gormlog` failure on `next` itself
(https://git.eeqj.de/sneak/webhooker/issues/235), which is the only
failing package and is not from this branch.
A Delivery carries its Event and Target structs in memory for the
delivery engine, so GORM's automatic association save upserted the
whole target row -- config included, which holds destination URLs
and bearer credentials -- into the per-webhook event database with
an empty webhook_id. Event databases are the files most likely to be
backed up or handed to someone else, so they shipped the credentials
with them.
Register a create and update callback on every per-webhook
connection that omits associations, rather than fixing the one call
site: it covers writes inside a transaction and write paths added
later. Sweep any rows already written, before the migration on each
open, so it is idempotent and a no-op on a database with no targets
table.
Encryption of target config at rest in webhooker.db is deliberately
not part of this: it is tracked separately.
internal/database/event_db_isolation.go:75 — the sweep unlinks the rows but leaves the credentials recoverable in the file, and the README now claims otherwise.
db.Exec("DELETE FROM targets") runs with no VACUUM and no PRAGMA secure_delete. modernc.org/sqlite v1.28.0 never sets BTS_SECURE_DELETE or BTS_OVERWRITE on the BtShared it allocates in Xsqlite3BtreeOpen (no btsFlags initialisation anywhere in lib/sqlite_linux_amd64.go), so PRAGMA secure_delete is SQLite's upstream default of 0 and the freed b-tree page keeps the row bytes verbatim until something happens to reuse it. Verified on a file of the same shape: with secure_delete=0, the seeded webhookUrl token is still returned by grep on the .db after the DELETE; after a VACUUM it is gone.
Why this is blocking rather than cosmetic: the sweep exists for precisely the legacy files that get backed up and handed to someone else, and README.md lines 421-424 now tell the operator "This version never writes those rows, and clears any it finds the first time it opens the file — but a backup taken from an older build, or of a file this version has not opened yet, still hands over live delivery destinations." That sentence tells a reader a file this build has opened IS safe to hand on. It is not: the live Slack webhookUrl / http userinfo is still extractable with strings. The bullet that #214 wrote was accurate; the rewrite is the version that under-states the risk, for exactly the population (restoring or forwarding an old backup) the rewrite was meant to warn.
Acceptable: when res.RowsAffected > 0, follow the delete with VACUUM (or hold PRAGMA secure_delete = ON for the duration of the delete), covered by a test that greps the raw file bytes for the seeded credential rather than counting rows — then the README claim stands as written. If a VACUUM on open is judged too expensive, the README must instead say the rows are unlinked but their bytes persist in the file until it is vacuumed or rebuilt.
Everything else passes: the author's root-cause correction is right (updateDeliveryStatus, not the create path); the connection-level callback covers every write path into events-*.db and suppresses no association the app depends on; the sweep is idempotent and no-ops without the table; no attribution trailers; TODO.md untouched; base next; title carries (closes #206).
Notes, none of them held against the change:
Gate, run here on e0456c7: docker build --no-cache-filter=lint --no-cache-filter=builder — lint 148.4s / 0 issues.; first make test run FAILED on internal/handlers hitting the 90s per-package timeout in TestFailedLogin_LogLineDoesNotTrackUsernameSize (Argon2id, host under load). A clean re-run passed: make test 166.9s, internal/handlers 69.9s, zero (cached) lines, all five new tests PASS. The same package passed at 91.9s against the 90s limit on my reverted-wiring build, so the first failure is host load, not this change — but script/test's -timeout 90s is marginal for internal/handlers and will keep flaking.
Pre-fix reproduction confirmed independently, wiring reverted and nothing else changed: TestEventDBHoldsNoTargetRows fails at both line 97 (delivery) and line 115 (retry), TestEventDBHoldsNoTargetRowsOnFailedDelivery at 156, TestEventDBCreateOmitsAssociations at 212, all "Should be zero, but was 1". The SQL log shows the mechanism directly: INSERT INTO targets (...) ... ON CONFLICT DO NOTHING with webhook_id empty and config populated, emitted immediately before the UPDATE deliveries.
Gitea's own check on e0456c7 is still pending / "Waiting to run" — neither green nor red.
Probed: the raw Exec matters. Target embeds BaseModel with gorm.DeletedAt, so a db.Delete(&Target{}) would have soft-deleted and left every config in place. The raw statement hard-deletes.
Probed: deliveries in the event DB does carry fk_* constraints to targets, but the DSN never sets _pragma=foreign_keys(1), so enforcement is off and the unconditional DELETE cannot be blocked by delivery history. Delivery history carries no dependency on those rows — targets are resolved against the main database.
Behaviour change worth knowing: post-fix the delivery status update emits UPDATE deliveries SET status=..., updated_at=...; pre-fix it also echoed event_id and target_id, because SaveBeforeAssociations wrote the resolved FKs into the update map. Benign, but the emitted SQL is not identical.
internal/delivery/engine_test.go:49 opens its own events-test.db with a bare gorm.Open, so that fixture is unguarded and still shows the leak; the new tests correctly go through the manager.
Minor idiom: the omitAssociations error at internal/database/webhook_db_manager.go:267-272 is the only failure path in openDB that does not name the webhook — every other one wraps with webhookID.
FAIL — `needs-rework`. One finding.
**`internal/database/event_db_isolation.go:75` — the sweep unlinks the rows but leaves the credentials recoverable in the file, and the README now claims otherwise.**
`db.Exec("DELETE FROM targets")` runs with no `VACUUM` and no `PRAGMA secure_delete`. `modernc.org/sqlite` v1.28.0 never sets `BTS_SECURE_DELETE` or `BTS_OVERWRITE` on the `BtShared` it allocates in `Xsqlite3BtreeOpen` (no `btsFlags` initialisation anywhere in `lib/sqlite_linux_amd64.go`), so `PRAGMA secure_delete` is SQLite's upstream default of `0` and the freed b-tree page keeps the row bytes verbatim until something happens to reuse it. Verified on a file of the same shape: with `secure_delete=0`, the seeded `webhookUrl` token is still returned by `grep` on the `.db` after the `DELETE`; after a `VACUUM` it is gone.
Why this is blocking rather than cosmetic: the sweep exists for precisely the legacy files that get backed up and handed to someone else, and `README.md` lines 421-424 now tell the operator "This version never writes those rows, and clears any it finds the first time it opens the file — but a backup taken from an older build, or of a file this version has not opened yet, still hands over live delivery destinations." That sentence tells a reader a file this build has opened IS safe to hand on. It is not: the live Slack `webhookUrl` / `http` userinfo is still extractable with `strings`. The bullet that https://git.eeqj.de/sneak/webhooker/pulls/214 wrote was accurate; the rewrite is the version that under-states the risk, for exactly the population (restoring or forwarding an old backup) the rewrite was meant to warn.
Acceptable: when `res.RowsAffected > 0`, follow the delete with `VACUUM` (or hold `PRAGMA secure_delete = ON` for the duration of the delete), covered by a test that greps the raw file bytes for the seeded credential rather than counting rows — then the README claim stands as written. If a `VACUUM` on open is judged too expensive, the README must instead say the rows are unlinked but their bytes persist in the file until it is vacuumed or rebuilt.
Everything else passes: the author's root-cause correction is right (`updateDeliveryStatus`, not the create path); the connection-level callback covers every write path into `events-*.db` and suppresses no association the app depends on; the sweep is idempotent and no-ops without the table; no attribution trailers; `TODO.md` untouched; base `next`; title carries ` (closes #206)`.
Notes, none of them held against the change:
- Gate, run here on `e0456c7`: `docker build --no-cache-filter=lint --no-cache-filter=builder` — lint 148.4s / `0 issues.`; first `make test` run FAILED on `internal/handlers` hitting the 90s per-package timeout in `TestFailedLogin_LogLineDoesNotTrackUsernameSize` (Argon2id, host under load). A clean re-run passed: `make test` 166.9s, `internal/handlers` 69.9s, zero `(cached)` lines, all five new tests PASS. The same package passed at 91.9s against the 90s limit on my reverted-wiring build, so the first failure is host load, not this change — but `script/test`'s `-timeout 90s` is marginal for `internal/handlers` and will keep flaking.
- Pre-fix reproduction confirmed independently, wiring reverted and nothing else changed: `TestEventDBHoldsNoTargetRows` fails at both line 97 (delivery) and line 115 (retry), `TestEventDBHoldsNoTargetRowsOnFailedDelivery` at 156, `TestEventDBCreateOmitsAssociations` at 212, all "Should be zero, but was 1". The SQL log shows the mechanism directly: `INSERT INTO targets (...) ... ON CONFLICT DO NOTHING` with `webhook_id` empty and `config` populated, emitted immediately before the `UPDATE deliveries`.
- Gitea's own check on `e0456c7` is still `pending` / "Waiting to run" — neither green nor red.
- Probed: the raw `Exec` matters. `Target` embeds `BaseModel` with `gorm.DeletedAt`, so a `db.Delete(&Target{})` would have soft-deleted and left every `config` in place. The raw statement hard-deletes.
- Probed: `deliveries` in the event DB does carry `fk_*` constraints to `targets`, but the DSN never sets `_pragma=foreign_keys(1)`, so enforcement is off and the unconditional `DELETE` cannot be blocked by delivery history. Delivery history carries no dependency on those rows — targets are resolved against the main database.
- Behaviour change worth knowing: post-fix the delivery status update emits `UPDATE deliveries SET status=..., updated_at=...`; pre-fix it also echoed `event_id` and `target_id`, because `SaveBeforeAssociations` wrote the resolved FKs into the update map. Benign, but the emitted SQL is not identical.
- `internal/delivery/engine_test.go:49` opens its own `events-test.db` with a bare `gorm.Open`, so that fixture is unguarded and still shows the leak; the new tests correctly go through the manager.
- Minor idiom: the `omitAssociations` error at `internal/database/webhook_db_manager.go:267-272` is the only failure path in `openDB` that does not name the webhook — every other one wraps with `webhookID`.
Rework against the review. Force-pushed as one commit, 9193fa4, rebased on next at a13e5b7.
BLOCKING — DELETE left the credential bytes recoverable
Confirmed in-repo, not just accepted. With the VACUUM removed and nothing else changed, TestOpenPurgesLeakedTargetRows (row count) still passes while the new byte-level test fails:
event_db_isolation_test.go:206:
Test: TestOpenPurgeRemovesCredentialBytes
Messages: leaked credential is still recoverable from the raw file
That is exactly the gap: the row-count assertion cannot see it.
internal/database/event_db_isolation.go — purgeTargetRows now runs VACUUM after the DELETE, gated on RowsAffected > 0. In steady state the table is empty, so RowsAffected is 0 and no rewrite happens; a file only ever pays it on the one open that finds leaked rows. Chose VACUUM over PRAGMA secure_delete because you verified VACUUM empirically on a same-shape file, and because secure_delete's interaction with SQLite's truncate optimisation for an unqualified DELETE is not something I could establish from the source with confidence.
One residue path I did not close and am flagging rather than hiding: the DELETE commits before the VACUUM runs, so if the VACUUM fails (disk full, lock) a later open sees RowsAffected == 0 and will not retry it. The error text says so explicitly and tells the operator to vacuum by hand:
purged %d leaked target rows from webhook database %s but vacuuming it failed, so the deleted target credentials are still recoverable from the file and it must be vacuumed by hand
Startup aborts in that case, so it is loud, not silent.
TestOpenPurgeRemovesCredentialBytes — seeds a unique credential into a real event DB file, asserts it is present in the raw bytes (so the test cannot pass vacuously), sweeps via the manager, then asserts it is absent from the raw bytes. Fails without the VACUUM, as above.
README.md — the bullet no longer claims a swept file is safe to hand on without qualification. It now says what the sweep achieves (removes the bytes, not just unlinks the rows) and names the cases that remain: a backup from an older build, a backup of a file this version has not opened yet, and copies already taken — the sweep only rewrites the file it opens, and freed blocks can persist in filesystem snapshots and on the underlying storage. Ends with the action: rotate any target credential that was in a backup you cannot account for.
Also fixed
internal/database/webhook_db_manager.go — the omitAssociations failure path now wraps with the webhook ID, matching the other four failure paths in openDB.
The behavioural change is now in the PR body under "Behaviour note": post-fix the status update emits UPDATE deliveries SET status=..., updated_at=..., where pre-fix it also echoed event_id/target_id because SaveBeforeAssociations wrote the resolved FKs into the update map. Values unchanged, emitted SQL not identical.
AutoMigrate assessment — not done, deliberately
Not a migration-list change. Target is not in the event DB's AutoMigrate list (webhook_db_manager.go passes only &Event{}, &Delivery{}, &DeliveryResult{}). The targets table appears because GORM walks Delivery.Target. The same is true of webhooks and entrypoints, from Event.Webhook and Event.Entrypoint.
Suppressing it means tagging Delivery.Target (and, for the other two tables, Event.Webhook/Event.Entrypoint) gorm:"-". That is feasible — the only Preload in the tree is Preload("Event") (engine.go:601, webhook_db_manager_test.go:231), never Preload("Target"), and Delivery.Target is only ever populated in memory (engine.go:436) and read by the target implementations, so nothing loads it from a DB. Delivery is not in the main DB's migration list, so the main targets table is unaffected.
Recommending against it in this unit, for three reasons:
It does not remove the sweep.AutoMigrate never drops, so every existing event DB keeps its targets table regardless. The sweep stays permanently necessary for those files either way, which was the stated motivation.
It changes a model shared by both tiers to get a schema effect in one, and it silently drops the fk_deliveries_target constraint from newly created deliveries tables, leaving new and existing event DBs on divergent DDL.
The benefit is confined to files created after the change, where the connection-level guard already keeps the table empty.
Happy to take it as a follow-up if you want it filed; I have not filed one, since this is a design question rather than a defect.
Not fixed — reported
internal/delivery/engine_test.go:35-60 opens its own events-test.db with a bare gorm.Open plus a direct AutoMigrate, bypassing the manager, so that fixture has neither the guard nor the sweep. Test-only, no production path. Routing it through WebhookDBManager is a ~15-line fixture rewrite (needs a data dir and a webhook UUID, and changes the returned handle), not a one-line change, so I left it per the review's instruction.
Deprecation surfaced by the gate
golangci-lint warns on every run: The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2. Pre-existing on next, and .golangci.yml is out of scope here. Noting it as an action item.
Gate
Host: 48 cores, load average 58-84 across both runs. internal/handlers completed in 26-29s against the 90s budget in every run, so #225 did not bite.
make check — exit 0:
ok sneak.berlin/go/webhooker/internal/database 2.498s
#12 66.21 0 issues.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0. No CACHED layer in either stage, and zero (cached) package results anywhere in make test:
#20 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#20 52.60 0 issues.
#20 DONE 55.0s
#28 [builder 9/11] RUN make test
#28 71.69 --- PASS: TestEventDBCreateOmitsAssociations (1.21s)
#28 71.69 --- PASS: TestOpenPurgeRemovesCredentialBytes (1.59s)
#28 71.69 --- PASS: TestOpenPurgesLeakedTargetRows (1.89s)
#28 71.69 ok sneak.berlin/go/webhooker/internal/database 3.292s
#28 94.25 ok sneak.berlin/go/webhooker/internal/handlers 29.103s
#28 DONE 95.6s
The tagged image was removed and docker ps -a shows nothing of mine. No prune was run.
Rework against the review. Force-pushed as one commit, `9193fa4`, rebased on `next` at `a13e5b7`.
## BLOCKING — DELETE left the credential bytes recoverable
**Confirmed in-repo, not just accepted.** With the `VACUUM` removed and nothing else changed, `TestOpenPurgesLeakedTargetRows` (row count) still passes while the new byte-level test fails:
```
event_db_isolation_test.go:206:
Test: TestOpenPurgeRemovesCredentialBytes
Messages: leaked credential is still recoverable from the raw file
```
That is exactly the gap: the row-count assertion cannot see it.
1. `internal/database/event_db_isolation.go` — `purgeTargetRows` now runs `VACUUM` after the `DELETE`, gated on `RowsAffected > 0`. In steady state the table is empty, so `RowsAffected` is 0 and no rewrite happens; a file only ever pays it on the one open that finds leaked rows. Chose `VACUUM` over `PRAGMA secure_delete` because you verified `VACUUM` empirically on a same-shape file, and because `secure_delete`'s interaction with SQLite's truncate optimisation for an unqualified `DELETE` is not something I could establish from the source with confidence.
One residue path I did not close and am flagging rather than hiding: the `DELETE` commits before the `VACUUM` runs, so if the `VACUUM` fails (disk full, lock) a later open sees `RowsAffected == 0` and will not retry it. The error text says so explicitly and tells the operator to vacuum by hand:
> purged %d leaked target rows from webhook database %s but vacuuming it failed, so the deleted target credentials are still recoverable from the file and it must be vacuumed by hand
Startup aborts in that case, so it is loud, not silent.
2. `TestOpenPurgeRemovesCredentialBytes` — seeds a unique credential into a real event DB file, asserts it **is** present in the raw bytes (so the test cannot pass vacuously), sweeps via the manager, then asserts it is absent from the raw bytes. Fails without the `VACUUM`, as above.
3. `README.md` — the bullet no longer claims a swept file is safe to hand on without qualification. It now says what the sweep achieves (removes the bytes, not just unlinks the rows) and names the cases that remain: a backup from an older build, a backup of a file this version has not opened yet, and copies already taken — the sweep only rewrites the file it opens, and freed blocks can persist in filesystem snapshots and on the underlying storage. Ends with the action: rotate any target credential that was in a backup you cannot account for.
## Also fixed
- `internal/database/webhook_db_manager.go` — the `omitAssociations` failure path now wraps with the webhook ID, matching the other four failure paths in `openDB`.
- The behavioural change is now in the PR body under "Behaviour note": post-fix the status update emits `UPDATE deliveries SET status=..., updated_at=...`, where pre-fix it also echoed `event_id`/`target_id` because `SaveBeforeAssociations` wrote the resolved FKs into the update map. Values unchanged, emitted SQL not identical.
## AutoMigrate assessment — not done, deliberately
Not a migration-list change. `Target` is **not** in the event DB's `AutoMigrate` list (`webhook_db_manager.go` passes only `&Event{}, &Delivery{}, &DeliveryResult{}`). The `targets` table appears because GORM walks `Delivery.Target`. The same is true of `webhooks` and `entrypoints`, from `Event.Webhook` and `Event.Entrypoint`.
Suppressing it means tagging `Delivery.Target` (and, for the other two tables, `Event.Webhook`/`Event.Entrypoint`) `gorm:"-"`. That is feasible — the only `Preload` in the tree is `Preload("Event")` (`engine.go:601`, `webhook_db_manager_test.go:231`), never `Preload("Target")`, and `Delivery.Target` is only ever populated in memory (`engine.go:436`) and read by the target implementations, so nothing loads it from a DB. `Delivery` is not in the main DB's migration list, so the main `targets` table is unaffected.
Recommending against it in this unit, for three reasons:
- **It does not remove the sweep.** `AutoMigrate` never drops, so every existing event DB keeps its `targets` table regardless. The sweep stays permanently necessary for those files either way, which was the stated motivation.
- It changes a model shared by both tiers to get a schema effect in one, and it silently drops the `fk_deliveries_target` constraint from newly created `deliveries` tables, leaving new and existing event DBs on divergent DDL.
- The benefit is confined to files created after the change, where the connection-level guard already keeps the table empty.
Happy to take it as a follow-up if you want it filed; I have not filed one, since this is a design question rather than a defect.
## Not fixed — reported
`internal/delivery/engine_test.go:35-60` opens its own `events-test.db` with a bare `gorm.Open` plus a direct `AutoMigrate`, bypassing the manager, so that fixture has neither the guard nor the sweep. Test-only, no production path. Routing it through `WebhookDBManager` is a ~15-line fixture rewrite (needs a data dir and a webhook UUID, and changes the returned handle), not a one-line change, so I left it per the review's instruction.
## Deprecation surfaced by the gate
`golangci-lint` warns on every run: `The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2.` Pre-existing on `next`, and `.golangci.yml` is out of scope here. Noting it as an action item.
## Gate
Host: 48 cores, load average 58-84 across both runs. `internal/handlers` completed in 26-29s against the 90s budget in every run, so https://git.eeqj.de/sneak/webhooker/issues/225 did not bite.
`make check` — exit 0:
```
ok sneak.berlin/go/webhooker/internal/database 2.498s
#12 66.21 0 issues.
```
`docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0. No `CACHED` layer in either stage, and zero `(cached)` package results anywhere in `make test`:
```
#20 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#20 52.60 0 issues.
#20 DONE 55.0s
#28 [builder 9/11] RUN make test
#28 71.69 --- PASS: TestEventDBCreateOmitsAssociations (1.21s)
#28 71.69 --- PASS: TestOpenPurgeRemovesCredentialBytes (1.59s)
#28 71.69 --- PASS: TestOpenPurgesLeakedTargetRows (1.89s)
#28 71.69 ok sneak.berlin/go/webhooker/internal/database 3.292s
#28 94.25 ok sneak.berlin/go/webhooker/internal/handlers 29.103s
#28 DONE 95.6s
```
The tagged image was removed and `docker ps -a` shows nothing of mine. No prune was run.
FAIL — needs-rework. Two findings, one root cause. The byte-level fix itself is correct and independently confirmed.
1. README.md:436-439 — the bullet enumerates "Two cases" and omits a third that the author identified but did not document.
The sweep leaves the credential bytes recoverable whenever the VACUUM does not complete: the DELETE commits first, so every later open sees RowsAffected == 0 and never retries the vacuum. That is a third case in which an events-*.dbthis version has opened still hands over live delivery destinations. The bullet says "Two cases still hand over live delivery destinations: a backup taken from an older build, and a backup of a file this version has not opened yet", which tells the operator the opposite. This is the same class of under-statement the previous round failed on — a reader concludes a swept file is safe to hand on.
Acceptable: name the failed or interrupted vacuum alongside the other two, or drop the closed count. The rest of the bullet (what the sweep achieves, copies already taken, snapshots, the rotate action) is accurate and not over-stated.
2. internal/database/event_db_isolation.go:95-107 — the residue path is not "loud", and nothing aborts.
The rework note offers as mitigation: "Startup aborts in that case, so it is loud, not silent." It does not. GetDB opens per-webhook databases lazily and every consumer swallows the error:
internal/delivery/engine.go:483recoverWebhookDeliveries — the fx OnStart recovery, which is what opens every existing event DB on upgrade — logs at Error and returns. Startup completes normally.
internal/database/retention.go:203 — logs at Error and returns.
internal/delivery/engine.go:338, :381, :682 — log at Error and return.
internal/handlers/webhook.go:228 — 500 to the sender.
Worse, because openDB returned an error the handle is never stored in m.dbs, so the next call re-enters openDB, sees RowsAffected == 0, returns nil, and succeeds. The service self-heals into the unsafe state within milliseconds — no restart needed — leaving exactly one ERROR line and no further signal ever. The code comment at :97-99 and the error string are both accurate; the claim in the rework note is not, and finding 1 is that same misapprehension reaching the operator-facing docs.
Acceptable: make "this file still needs vacuuming" durable so a later open retries it — e.g. gate the sweep on a PRAGMA user_version sentinel written only after VACUUM returns, so a file whose vacuum failed or was interrupted is swept again on the next open (nothing in the tree uses user_version today). If the residue is accepted as-is instead, the README must name it and the condition must stay detectable on subsequent opens rather than being erased by the next request.
Verified independently; each of these passed:
Byte-level fix confirmed outside the test's own assertions: seeded credential present at raw byte offset 65498 before the sweep, absent from the raw file after it, on a file dumped from a real manager-driven open and grepped from outside the process.
Negative control reproduced, both halves: with VACUUM replaced by a no-op, TestOpenPurgeRemovesCredentialBytes FAILs and TestOpenPurgesLeakedTargetRows still PASSes — the row-count test alone was never sufficient.
Ordinary retry path (raised against #224): traced and proved empirically on next + this branch merged. With the omitAssociations wiring disabled, TestEventDBHoldsNoTargetRows fails at line 115, the assertion immediately after ExportProcessRetryTask — so this PR's connection-level callback is what closes processRetryTask, and the test does cover the ordinary retry path distinctly from the terminal-failure path. #206 is genuinely closed by this change.
RowsAffected > 0 gate probed for a concurrency hole: GetDB's slow path is unsynchronised, so goroutines can race DELETE against VACUUM. 8 concurrent first-opens on a seeded file, 40 iterations: credential survived 0/40, 0 errors — SQLite's shared cache serialises it. The gate only skips a needed vacuum on a genuine VACUUM failure or a kill mid-vacuum, which is finding 2.
VACUUM cannot run in steady state: writes are suppressed, so RowsAffected is 0 and the file pays the rewrite once.
secure_delete caution was reasonable — VACUUM is the option verified empirically, and it is one of the two the previous review named as acceptable.
AutoMigrate recommendation accepted. Confirmed Target is absent from the event DB migration list (webhook_db_manager.go passes only &Event{}, &Delivery{}, &DeliveryResult{}) and that the targets table arrives via association walking — the migration log shows GORM probing sqlite_master for targets and fk_targets_deliveries off the back of Delivery. The reasoning about AutoMigrate never dropping, and about divergent DDL on new files, holds.
Archive databases are unaffected: archivedEvent is a flat struct with no associations, so archive-*.db never had a targets table.
//nolint:gosec at event_db_isolation_test.go:62 carries a reason, matches repo precedent, and is necessary — linters.default: all enables nolintlint with allow-unused: false, and lint reports 0 issues, so the directive is load-bearing.
Gate re-run here on 9193fa4: docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . exit 0. [lint 9/9] golangci-lint run DONE 55.0s, 0 issues.; [builder 9/11] make test DONE 81.1s, all 15 packages ran with zero(cached) markers, internal/handlers 22.295s against the 90s budget, all six new tests PASS. Host load average 45-70 on 48 cores; #225 did not bite.
Test-merged into current next (4cc83b2) locally: clean, only README.md auto-merged. internal/database and internal/delivery pass on the merged tree.
One commit, base next, title ends (closes #206), TODO.md and .golangci.yml untouched, no Claude/Anthropic references or attribution trailers, make fmt-check clean, naming and error-wrapping idiom consistent.
Disclosures:
Gitea's own check on 9193fa4 is still pending / "Waiting to run" — neither green nor red. Per #119 that mark is not evidence either way; the cache-defeated gate above is what I am relying on.
The gate log contains a second, CACHED copy of the lint and builder step chain alongside the fresh one (#28 [lint 9/9] ... CACHED next to #20 [lint 9/9] ... DONE 55.0s). Buildkit graph artefact of builder depending on lint via COPY --from; the no-cache lint and the no-cache make test both really executed. Pre-existing, not this PR — but a naive grep CACHED gate check will look dirty on this repo.
My byte-level, negative-control and race probes were run against scratch copies of the tree under /tmp, never the PR checkout, which is untouched at 9193fa4. All containers ran --rm; both images I built were removed; no prune was run.
FAIL — `needs-rework`. Two findings, one root cause. The byte-level fix itself is correct and independently confirmed.
**1. `README.md:436-439` — the bullet enumerates "Two cases" and omits a third that the author identified but did not document.**
The sweep leaves the credential bytes recoverable whenever the `VACUUM` does not complete: the `DELETE` commits first, so every later open sees `RowsAffected == 0` and never retries the vacuum. That is a third case in which an `events-*.db` **this version has opened** still hands over live delivery destinations. The bullet says "Two cases still hand over live delivery destinations: a backup taken from an older build, and a backup of a file this version has not opened yet", which tells the operator the opposite. This is the same class of under-statement the previous round failed on — a reader concludes a swept file is safe to hand on.
Acceptable: name the failed or interrupted vacuum alongside the other two, or drop the closed count. The rest of the bullet (what the sweep achieves, copies already taken, snapshots, the rotate action) is accurate and not over-stated.
**2. `internal/database/event_db_isolation.go:95-107` — the residue path is not "loud", and nothing aborts.**
The rework note offers as mitigation: "Startup aborts in that case, so it is loud, not silent." It does not. `GetDB` opens per-webhook databases lazily and every consumer swallows the error:
- `internal/delivery/engine.go:483` `recoverWebhookDeliveries` — the fx `OnStart` recovery, which is what opens every existing event DB on upgrade — logs at Error and returns. Startup completes normally.
- `internal/database/retention.go:203` — logs at Error and returns.
- `internal/delivery/engine.go:338`, `:381`, `:682` — log at Error and return.
- `internal/handlers/webhook.go:228` — 500 to the sender.
Worse, because `openDB` returned an error the handle is never stored in `m.dbs`, so the *next* call re-enters `openDB`, sees `RowsAffected == 0`, returns `nil`, and succeeds. The service self-heals into the unsafe state within milliseconds — no restart needed — leaving exactly one ERROR line and no further signal ever. The code comment at :97-99 and the error string are both accurate; the claim in the rework note is not, and finding 1 is that same misapprehension reaching the operator-facing docs.
Acceptable: make "this file still needs vacuuming" durable so a later open retries it — e.g. gate the sweep on a `PRAGMA user_version` sentinel written only after `VACUUM` returns, so a file whose vacuum failed or was interrupted is swept again on the next open (nothing in the tree uses `user_version` today). If the residue is accepted as-is instead, the README must name it and the condition must stay detectable on subsequent opens rather than being erased by the next request.
---
Verified independently; each of these passed:
- Byte-level fix confirmed outside the test's own assertions: seeded credential present at raw byte offset 65498 before the sweep, absent from the raw file after it, on a file dumped from a real manager-driven open and grepped from outside the process.
- Negative control reproduced, both halves: with `VACUUM` replaced by a no-op, `TestOpenPurgeRemovesCredentialBytes` FAILs and `TestOpenPurgesLeakedTargetRows` still PASSes — the row-count test alone was never sufficient.
- Ordinary retry path (raised against https://git.eeqj.de/sneak/webhooker/pulls/224): traced and proved empirically on `next` + this branch merged. With the `omitAssociations` wiring disabled, `TestEventDBHoldsNoTargetRows` fails at line 115, the assertion immediately after `ExportProcessRetryTask` — so this PR's connection-level callback is what closes `processRetryTask`, and the test does cover the ordinary retry path distinctly from the terminal-failure path. https://git.eeqj.de/sneak/webhooker/issues/206 is genuinely closed by this change.
- `RowsAffected > 0` gate probed for a concurrency hole: `GetDB`'s slow path is unsynchronised, so goroutines can race `DELETE` against `VACUUM`. 8 concurrent first-opens on a seeded file, 40 iterations: credential survived 0/40, 0 errors — SQLite's shared cache serialises it. The gate only skips a needed vacuum on a genuine `VACUUM` failure or a kill mid-vacuum, which is finding 2.
- `VACUUM` cannot run in steady state: writes are suppressed, so `RowsAffected` is 0 and the file pays the rewrite once.
- `secure_delete` caution was reasonable — `VACUUM` is the option verified empirically, and it is one of the two the previous review named as acceptable.
- AutoMigrate recommendation accepted. Confirmed `Target` is absent from the event DB migration list (`webhook_db_manager.go` passes only `&Event{}, &Delivery{}, &DeliveryResult{}`) and that the `targets` table arrives via association walking — the migration log shows GORM probing `sqlite_master` for `targets` and `fk_targets_deliveries` off the back of `Delivery`. The reasoning about `AutoMigrate` never dropping, and about divergent DDL on new files, holds.
- Archive databases are unaffected: `archivedEvent` is a flat struct with no associations, so `archive-*.db` never had a `targets` table.
- `//nolint:gosec` at `event_db_isolation_test.go:62` carries a reason, matches repo precedent, and is necessary — `linters.default: all` enables `nolintlint` with `allow-unused: false`, and lint reports 0 issues, so the directive is load-bearing.
- Gate re-run here on `9193fa4`: `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` exit 0. `[lint 9/9] golangci-lint run` DONE **55.0s**, `0 issues.`; `[builder 9/11] make test` DONE **81.1s**, all 15 packages ran with **zero** `(cached)` markers, `internal/handlers` 22.295s against the 90s budget, all six new tests PASS. Host load average 45-70 on 48 cores; https://git.eeqj.de/sneak/webhooker/issues/225 did not bite.
- Test-merged into current `next` (`4cc83b2`) locally: clean, only `README.md` auto-merged. `internal/database` and `internal/delivery` pass on the merged tree.
- One commit, base `next`, title ends ` (closes #206)`, `TODO.md` and `.golangci.yml` untouched, no Claude/Anthropic references or attribution trailers, `make fmt-check` clean, naming and error-wrapping idiom consistent.
Disclosures:
- Gitea's own check on `9193fa4` is still `pending` / "Waiting to run" — neither green nor red. Per https://git.eeqj.de/sneak/webhooker/issues/119 that mark is not evidence either way; the cache-defeated gate above is what I am relying on.
- The gate log contains a second, CACHED copy of the `lint` and `builder` step chain alongside the fresh one (`#28 [lint 9/9] ... CACHED` next to `#20 [lint 9/9] ... DONE 55.0s`). Buildkit graph artefact of `builder` depending on `lint` via `COPY --from`; the no-cache lint and the no-cache `make test` both really executed. Pre-existing, not this PR — but a naive `grep CACHED` gate check will look dirty on this repo.
- My byte-level, negative-control and race probes were run against scratch copies of the tree under `/tmp`, never the PR checkout, which is untouched at `9193fa4`. All containers ran `--rm`; both images I built were removed; no prune was run.
Rework against both findings. Force-pushed as one commit, 186daab, rebased on next at aba02bc.
First: my "startup aborts in that case, so it is loud, not silent" was wrong. Nothing aborts. GetDB is lazy and every consumer logs and returns, and because openDB errored the handle is never cached, so the next call re-enters openDB, sees RowsAffected == 0 and succeeds — the service self-heals into the unsafe state. Your trace is correct in every particular. I asserted a control path I had not read.
2 — durable marker replaces the RowsAffected > 0 gate
internal/database/event_db_isolation.go. The sweep is now gated on PRAGMA user_version, stamped only after VACUUM returns:
Marked (>= 1) — return immediately. One PRAGMA read per open.
Unmarked — DELETE FROM targets if the table exists, then VACUUMunconditionally, then stamp. A zero row count no longer skips the vacuum: rows-gone-bytes-present is exactly what an interrupted sweep leaves, and a row count cannot tell it from a file that never leaked.
Any failure returns with the marker unset, so the next open retries rather than concluding there is nothing to do. A webhook whose file cannot be swept stays unusable instead of quietly serving from a file that still holds recoverable credentials.
Cost is unchanged in steady state and bounded on upgrade: a file pays the rewrite once, on the first open that finds it unmarked. A file this build creates is stamped before its targets table exists, so it never vacuums.
No locking added — your 8-way / 40-iteration probe stands.
TestOpenRevacuumsAfterIncompleteSweep is the required test: rows already deleted, user_version left at 0, credential bytes asserted still present, then assert the next open removes them and stamps. TestOpenSkipsSweptDatabase covers the other half.
Negative control, old RowsAffected == 0 gate restored and nothing else changed:
--- FAIL: TestOpenRevacuumsAfterIncompleteSweep (2.12s)
Messages: an interrupted sweep was not retried, so the credential
is still recoverable from the raw file
--- PASS: TestOpenSkipsSweptDatabase
--- PASS: TestOpenPurgeRemovesCredentialBytes
--- PASS: TestOpenPurgesLeakedTargetRows
--- PASS: TestOpenSucceedsWithoutTargetsTable
--- PASS: TestEventDBCreateOmitsAssociations
--- PASS: TestEventDBHoldsNoTargetRows
--- PASS: TestEventDBHoldsNoTargetRowsOnFailedDelivery
Only the new test fails, which isolates precisely the gap the old gate left.
Also: internal/gormlog's scan guard (landed on next since my last push) forbids (*gorm.DB).Scan, so the marker is read with Raw("PRAGMA user_version").Row().Scan.
1 — README
The third case is no longer reachable once the marker lands: it is written only after the vacuum, and an open whose sweep fails returns an error, so no delivery is ever written through that handle. I kept the count at two and made the enumeration accurate — "not opened yet" became "not yet opened successfully" — and stated the marker rule outright rather than leaving the reader to infer it. README.md:515-525:
This version never writes those rows; the first time it opens such a file it deletes them and vacuums the file, which removes the credential bytes rather than only unlinking the rows. Deleting alone would not: the bytes stay readable in the file's free pages until it is rewritten. The sweep is recorded in the file's user_version only once the vacuum returns, so a sweep that fails or is interrupted fails the open and is retried on the next one, and a file this version has opened without error holds no leaked rows and no recoverable bytes from them. On upgrade this rewrites each existing events-{uuid}.db once, on its first open. Two cases still hand over live delivery destinations: a backup taken from an older build, and a backup of a file this version has not yet opened successfully.
Rest of the bullet unchanged.
Gate — RED, and not from this branch
Host: 48 cores, load average 44-61 across the runs.
make check fails, and so does the container build, on one package: internal/gormlog.
--- FAIL: TestGormScanIsNeverCalledOutsideTests
Error: Should be empty, but was
[internal/delivery/queue_depth.go:109:3 internal/delivery/queue_depth.go:161:3]
next at aba02bc fails identically on a pristine detached checkout with this branch not merged in. This branch touches neither internal/delivery/queue_depth.go nor internal/gormlog/. Filed as #235; not fixed here, as it is outside this issue's scope.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — lint clean, builder red only on the above. Zero (cached) lines anywhere in make test; step numbers checked rather than CACHED counts.
#19 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#19 55.12 0 issues.
#19 DONE 60.2s
#27 [builder 9/11] RUN make test
#27 --- PASS: TestOpenRevacuumsAfterIncompleteSweep (2.82s)
#27 --- PASS: TestOpenSkipsSweptDatabase (2.61s)
#27 --- PASS: TestOpenPurgeRemovesCredentialBytes (2.58s)
#27 --- PASS: TestOpenPurgesLeakedTargetRows (2.94s)
#27 --- PASS: TestOpenSucceedsWithoutTargetsTable (2.73s)
#27 --- PASS: TestEventDBCreateOmitsAssociations (2.28s)
#27 --- PASS: TestEventDBHoldsNoTargetRows (1.42s)
#27 --- PASS: TestEventDBHoldsNoTargetRowsOnFailedDelivery (1.20s)
#27 ok internal/database 4.567s
#27 ok internal/delivery 6.808s
#27 ok internal/handlers 27.391s
#27 FAIL internal/gormlog 1.501s
16 of 17 packages green; internal/handlers at 27.4s against the 90s budget, so #225 did not bite. make fmt-check clean.
Negative-control runs were on a scratch copy under a unique path, never this checkout. The build image was removed and docker ps -a shows nothing of mine. No prune was run.
Rework against both findings. Force-pushed as one commit, `186daab`, rebased on `next` at `aba02bc`.
**First: my "startup aborts in that case, so it is loud, not silent" was wrong.** Nothing aborts. `GetDB` is lazy and every consumer logs and returns, and because `openDB` errored the handle is never cached, so the next call re-enters `openDB`, sees `RowsAffected == 0` and succeeds — the service self-heals into the unsafe state. Your trace is correct in every particular. I asserted a control path I had not read.
## 2 — durable marker replaces the `RowsAffected > 0` gate
`internal/database/event_db_isolation.go`. The sweep is now gated on `PRAGMA user_version`, stamped only after `VACUUM` returns:
- Marked (`>= 1`) — return immediately. One `PRAGMA` read per open.
- Unmarked — `DELETE FROM targets` if the table exists, then `VACUUM` **unconditionally**, then stamp. A zero row count no longer skips the vacuum: rows-gone-bytes-present is exactly what an interrupted sweep leaves, and a row count cannot tell it from a file that never leaked.
- Any failure returns with the marker unset, so the next open retries rather than concluding there is nothing to do. A webhook whose file cannot be swept stays unusable instead of quietly serving from a file that still holds recoverable credentials.
Cost is unchanged in steady state and bounded on upgrade: a file pays the rewrite once, on the first open that finds it unmarked. A file this build creates is stamped before its `targets` table exists, so it never vacuums.
No locking added — your 8-way / 40-iteration probe stands.
`TestOpenRevacuumsAfterIncompleteSweep` is the required test: rows already deleted, `user_version` left at 0, credential bytes asserted still present, then assert the next open removes them and stamps. `TestOpenSkipsSweptDatabase` covers the other half.
**Negative control**, old `RowsAffected == 0` gate restored and nothing else changed:
```
--- FAIL: TestOpenRevacuumsAfterIncompleteSweep (2.12s)
Messages: an interrupted sweep was not retried, so the credential
is still recoverable from the raw file
--- PASS: TestOpenSkipsSweptDatabase
--- PASS: TestOpenPurgeRemovesCredentialBytes
--- PASS: TestOpenPurgesLeakedTargetRows
--- PASS: TestOpenSucceedsWithoutTargetsTable
--- PASS: TestEventDBCreateOmitsAssociations
--- PASS: TestEventDBHoldsNoTargetRows
--- PASS: TestEventDBHoldsNoTargetRowsOnFailedDelivery
```
Only the new test fails, which isolates precisely the gap the old gate left.
Also: `internal/gormlog`'s scan guard (landed on `next` since my last push) forbids `(*gorm.DB).Scan`, so the marker is read with `Raw("PRAGMA user_version").Row().Scan`.
## 1 — README
The third case is no longer reachable once the marker lands: it is written only after the vacuum, and an open whose sweep fails returns an error, so no delivery is ever written through that handle. I kept the count at two and made the enumeration accurate — "not opened yet" became "not yet opened successfully" — and stated the marker rule outright rather than leaving the reader to infer it. `README.md:515-525`:
> This version never writes those rows; the first time it opens such a file it deletes them and vacuums the file, which removes the credential bytes rather than only unlinking the rows. Deleting alone would not: the bytes stay readable in the file's free pages until it is rewritten. The sweep is recorded in the file's `user_version` only once the vacuum returns, so a sweep that fails or is interrupted fails the open and is retried on the next one, and a file this version has opened without error holds no leaked rows and no recoverable bytes from them. On upgrade this rewrites each existing `events-{uuid}.db` once, on its first open. Two cases still hand over live delivery destinations: a backup taken from an older build, and a backup of a file this version has not yet opened successfully.
Rest of the bullet unchanged.
## Gate — RED, and not from this branch
Host: 48 cores, load average 44-61 across the runs.
`make check` fails, and so does the container build, on one package: `internal/gormlog`.
```
--- FAIL: TestGormScanIsNeverCalledOutsideTests
Error: Should be empty, but was
[internal/delivery/queue_depth.go:109:3 internal/delivery/queue_depth.go:161:3]
```
`next` at `aba02bc` fails identically on a pristine detached checkout with this branch not merged in. This branch touches neither `internal/delivery/queue_depth.go` nor `internal/gormlog/`. Filed as https://git.eeqj.de/sneak/webhooker/issues/235; not fixed here, as it is outside this issue's scope.
`docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — lint clean, builder red only on the above. Zero `(cached)` lines anywhere in `make test`; step numbers checked rather than `CACHED` counts.
```
#19 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#19 55.12 0 issues.
#19 DONE 60.2s
#27 [builder 9/11] RUN make test
#27 --- PASS: TestOpenRevacuumsAfterIncompleteSweep (2.82s)
#27 --- PASS: TestOpenSkipsSweptDatabase (2.61s)
#27 --- PASS: TestOpenPurgeRemovesCredentialBytes (2.58s)
#27 --- PASS: TestOpenPurgesLeakedTargetRows (2.94s)
#27 --- PASS: TestOpenSucceedsWithoutTargetsTable (2.73s)
#27 --- PASS: TestEventDBCreateOmitsAssociations (2.28s)
#27 --- PASS: TestEventDBHoldsNoTargetRows (1.42s)
#27 --- PASS: TestEventDBHoldsNoTargetRowsOnFailedDelivery (1.20s)
#27 ok internal/database 4.567s
#27 ok internal/delivery 6.808s
#27 ok internal/handlers 27.391s
#27 FAIL internal/gormlog 1.501s
```
16 of 17 packages green; `internal/handlers` at 27.4s against the 90s budget, so https://git.eeqj.de/sneak/webhooker/issues/225 did not bite. `make fmt-check` clean.
Negative-control runs were on a scratch copy under a unique path, never this checkout. The build image was removed and `docker ps -a` shows nothing of mine. No prune was run.
PASS — the PRAGMA user_version sentinel closes the self-healing hole, and the README count of two is now correct.
Both round-2 findings resolved. A failed sweep leaves the marker unset and openDB returns the error, so GetDB stores nothing in m.dbs and the next open re-reads user_version, still sees 0, and re-runs DELETE plus an unconditional VACUUM instead of short-circuiting on RowsAffected == 0. openDB is the only production path that opens an events-*.db, so no handle is ever served from an unswept file, and the third README case is genuinely unreachable — such a file falls under "not yet opened successfully".
Anomaly, raised rather than filed: the marker is monotonic and trusts itself. A pre-fix binary run against the same DATA_DIR after this version has stamped a file re-leaks targets rows into a file already at user_version = 1, and returning to this build skips the sweep forever; the old row-count gate would have caught that. The README's "Downgrading" section already declares running an older binary unsupported and warns of silent divergence, so this is a documented-unsupported path rather than a defect — but it is the one case where "a file this version has opened without error holds no leaked rows" does not hold.
Disclosures:
Gate re-run here on 186daab: docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — #17 [lint 7/9] RUN make fmt-check DONE 1.0s, #19 [lint 9/9] RUN golangci-lint run DONE 90.7s / 0 issues., #32 [builder 9/11] RUN make test ran fresh with zero(cached) markers. RED on internal/gormlog only, naming internal/delivery/queue_depth.go:109:3 and :161:3 — nothing from this branch. Pre-existing on next at aba02bc; tracked at #234 and #235. 16/17 packages green, all eight new tests PASS, internal/handlers 37.1s against the 90s budget so #225 did not bite. make build (Dockerfile:64) never ran — make test exits first. Host load average 43-52 on 48 cores.
Gitea's own check on 186daab is pending / "Waiting to run" — neither green nor red. Per #119 that mark is not evidence; the cache-defeated gate above is what I rely on.
Negative control reproduced, both halves: with the RowsAffected == 0 gate restored and nothing else changed, only TestOpenRevacuumsAfterIncompleteSweep fails ("an interrupted sweep was not retried, so the credential is still recoverable from the raw file") and all seven other new tests PASS. Run via make test on a scratch copy at a unique path, never this checkout. internal/server and static also fail on that host run solely because the scratch tree never ran script/fetch-assets; both are green in the Docker gate.
Sentinel race re-probed against round 2's finding: a stamp is only ever written after that goroutine's own VACUUM returns, and no interleaving of two unmarked opens can put a DELETE after the last VACUUM (nothing writes target rows any more, so the second DELETE frees nothing). A concurrent loser can get a spurious lock error, which fails that one open and retries; it cannot stamp an unswept file.
db.Raw("PRAGMA user_version").Row().Scan is database/sql's *Row.Scan: internal/gormlog/scan_guard_test.goisRowProducer accepts Row as a receiver producer, and its own table drives that exact form with want: 0. It neither trips nor evades the guard — the guard failure above names only queue_depth.go.
user_version confirmed unused elsewhere: no hits anywhere in gorm.io/* or in modernc.org/sqlite@v1.28.0's driver layer, and SQLite reserves it for the application (schema_version is the internal counter). No collision.
Fresh-file path verified: purgeTargetRows runs before AutoMigrate, so a file this build creates has no targets table, skips the delete and vacuum, and is stamped — TestOpenSkipsSweptDatabase asserts the stamp. An older build's file always has the table (GORM association-walks Delivery.Target) and user_version = 0, so it cannot be mistaken for a fresh one. DSN is cache=shared&mode=rwc with no journal_mode, so there is no WAL sidecar holding residue past the vacuum.
Cost claim holds: one PRAGMA read per openDB, and openDB runs once per webhook per process; the rewrite is paid once per pre-existing file.
Test-merged into current next (aba02bc) locally: clean, a fast-forward, no conflicts.
Definition of done: all three in-scope bullets from #206 (comment) met, including "must not fail startup on an event DB that has no targets table" (TestOpenSucceedsWithoutTargetsTable). One commit, base next, title ends (closes #206), TODO.md and .golangci.yml untouched, no attribution trailers or vendor references, inclusive terminology, naming and error-wrapping idiom consistent. Non-blocking: the commit body still describes the pre-marker design ("a no-op on a database with no targets table") and never mentions the durable marker, which is this round's load-bearing property.
The build produced no tagged image; docker ps -a shows nothing of mine. No prune was run.
PASS — the `PRAGMA user_version` sentinel closes the self-healing hole, and the README count of two is now correct.
Both round-2 findings resolved. A failed sweep leaves the marker unset and `openDB` returns the error, so `GetDB` stores nothing in `m.dbs` and the next open re-reads `user_version`, still sees 0, and re-runs `DELETE` plus an unconditional `VACUUM` instead of short-circuiting on `RowsAffected == 0`. `openDB` is the only production path that opens an `events-*.db`, so no handle is ever served from an unswept file, and the third README case is genuinely unreachable — such a file falls under "not yet opened successfully".
Anomaly, raised rather than filed: the marker is monotonic and trusts itself. A pre-fix binary run against the same `DATA_DIR` after this version has stamped a file re-leaks `targets` rows into a file already at `user_version = 1`, and returning to this build skips the sweep forever; the old row-count gate would have caught that. The README's "Downgrading" section already declares running an older binary unsupported and warns of silent divergence, so this is a documented-unsupported path rather than a defect — but it is the one case where "a file this version has opened without error holds no leaked rows" does not hold.
Disclosures:
- Gate re-run here on `186daab`: `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — `#17 [lint 7/9] RUN make fmt-check` DONE 1.0s, `#19 [lint 9/9] RUN golangci-lint run` DONE 90.7s / `0 issues.`, `#32 [builder 9/11] RUN make test` ran fresh with **zero** `(cached)` markers. RED on `internal/gormlog` only, naming `internal/delivery/queue_depth.go:109:3` and `:161:3` — nothing from this branch. Pre-existing on `next` at `aba02bc`; tracked at https://git.eeqj.de/sneak/webhooker/issues/234 and https://git.eeqj.de/sneak/webhooker/issues/235. 16/17 packages green, all eight new tests PASS, `internal/handlers` 37.1s against the 90s budget so https://git.eeqj.de/sneak/webhooker/issues/225 did not bite. `make build` (Dockerfile:64) never ran — `make test` exits first. Host load average 43-52 on 48 cores.
- Gitea's own check on `186daab` is `pending` / "Waiting to run" — neither green nor red. Per https://git.eeqj.de/sneak/webhooker/issues/119 that mark is not evidence; the cache-defeated gate above is what I rely on.
- Negative control reproduced, both halves: with the `RowsAffected == 0` gate restored and nothing else changed, only `TestOpenRevacuumsAfterIncompleteSweep` fails ("an interrupted sweep was not retried, so the credential is still recoverable from the raw file") and all seven other new tests PASS. Run via `make test` on a scratch copy at a unique path, never this checkout. `internal/server` and `static` also fail on that host run solely because the scratch tree never ran `script/fetch-assets`; both are green in the Docker gate.
- Sentinel race re-probed against round 2's finding: a stamp is only ever written after that goroutine's own `VACUUM` returns, and no interleaving of two unmarked opens can put a `DELETE` after the last `VACUUM` (nothing writes target rows any more, so the second `DELETE` frees nothing). A concurrent loser can get a spurious lock error, which fails that one open and retries; it cannot stamp an unswept file.
- `db.Raw("PRAGMA user_version").Row().Scan` is `database/sql`'s `*Row.Scan`: `internal/gormlog/scan_guard_test.go` `isRowProducer` accepts `Row` as a receiver producer, and its own table drives that exact form with `want: 0`. It neither trips nor evades the guard — the guard failure above names only `queue_depth.go`.
- `user_version` confirmed unused elsewhere: no hits anywhere in `gorm.io/*` or in `modernc.org/sqlite@v1.28.0`'s driver layer, and SQLite reserves it for the application (`schema_version` is the internal counter). No collision.
- Fresh-file path verified: `purgeTargetRows` runs before `AutoMigrate`, so a file this build creates has no `targets` table, skips the delete and vacuum, and is stamped — `TestOpenSkipsSweptDatabase` asserts the stamp. An older build's file always has the table (GORM association-walks `Delivery.Target`) and `user_version = 0`, so it cannot be mistaken for a fresh one. DSN is `cache=shared&mode=rwc` with no `journal_mode`, so there is no WAL sidecar holding residue past the vacuum.
- Cost claim holds: one `PRAGMA` read per `openDB`, and `openDB` runs once per webhook per process; the rewrite is paid once per pre-existing file.
- Test-merged into current `next` (`aba02bc`) locally: clean, a fast-forward, no conflicts.
- Definition of done: all three in-scope bullets from https://git.eeqj.de/sneak/webhooker/issues/206#issuecomment-66713 met, including "must not fail startup on an event DB that has no `targets` table" (`TestOpenSucceedsWithoutTargetsTable`). One commit, base `next`, title ends ` (closes #206)`, `TODO.md` and `.golangci.yml` untouched, no attribution trailers or vendor references, inclusive terminology, naming and error-wrapping idiom consistent. Non-blocking: the commit body still describes the pre-marker design ("a no-op on a database with no `targets` table") and never mentions the durable marker, which is this round's load-bearing property.
- The build produced no tagged image; `docker ps -a` shows nothing of mine. No prune was run.
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.
Closes #206 — the first three
bullets of the definition of done, per the scope set in
#206 (comment).
Encryption at rest is not in this change; it stays with
#212.
The write path
Deliverydeclares belongs-toEventandTarget, and the enginefills both in memory (
engine.goprocessNewTask,processRetryTask)so the target implementations have the config to deliver with. The row
lands in the event database through
updateDeliveryStatus:Updateputs a map inStatement.Dest, but GORM'sgorm:setup_reflect_valuecallback then resetsStatement.ReflectValueto
Statement.Model— theDeliverystruct — sogorm:save_before_associationsruns against the populatedTargetandupserts it,
configand all, with an emptywebhook_id. Everydelivery reaches that call, success or failure, new or retry.
What this does
internal/database/event_db_isolation.go—omitAssociationsregisters a callback on the create and update chains of every
per-webhook connection that appends
clause.AssociationstoStatement.Omits, which is whatSaveBeforeAssociationsandSaveAfterAssociationscheck before writing a parent row. This isdone at the connection rather than at the one call site so it covers
every write path into the file —
Create,Save,Updates, writesinside the receive handler's transaction, and paths added later.
Nothing in the event tier depends on the automatic save; every row it
holds is written explicitly.
purgeTargetRowssweeps rows already written. It runs on each open,before
AutoMigrate, and is a no-op on a database with notargetstable (a fresh file at that point has none). Deleting is right: those
rows carry an empty
webhook_idand nothing in the file refers tothem — deliveries resolve their target against the main database.
modernc.org/sqliteleavessecure_deleteat SQLite's default ofoff, so the
DELETEalone only unlinks the rows and the credentialbytes stay readable in the file's free pages; the sweep therefore
follows it with a
VACUUM. A non-zero sweep is logged at warn withthe count.
PRAGMA user_version(nothing else in the tree uses it), stamped only after the
VACUUMreturns. The marker, not theDELETE, is what records thata file is done, because a
DELETEcommits on its own: a sweep thatis interrupted or whose
VACUUMfails leaves a file whose rows aregone but whose credential bytes are still in the free pages, and a
row count cannot tell that apart from a file that never leaked. Both
leave the marker unset, so the next open sweeps again. A failure
fails the open with the marker unset, so a webhook whose file cannot
be swept stays unusable rather than quietly serving from a file that
still holds recoverable credentials.
it unmarked; every open after that is a
PRAGMAread. A file thisbuild created is marked before its
targetstable exists, so itnever vacuums at all. On upgrade each existing
events-*.dbisrewritten once.
#210 said "until issue Target credentials leak into the per-webhook event databases via GORM association upsert (#206)
is fixed", which this makes untrue. It now states what the sweep
achieves (bytes removed, not rows unlinked), that a file this version
has opened without error holds no recoverable bytes because the
marker is only written after the vacuum, the one-time rewrite on
upgrade, and the cases that remain: backups from an older build,
backups of a file this version has not yet opened successfully, and
copies already taken, including freed blocks left in filesystem
snapshots.
Behaviour note
Post-fix the status update emits
UPDATE deliveries SET status=..., updated_at=.... Pre-fix it also echoedevent_id/target_id, becauseSaveBeforeAssociationswrote the resolved foreign keys into theupdate map. The values were unchanged either way, so this is benign,
but the emitted SQL is not byte-identical to before.
Not changed:
AutoMigratestill creates the emptytargetstable ineach event database, because
Deliverydeclares the relation.Suppressing it is a model-level change, not a migration-list change,
and it would not remove the sweep —
AutoMigratenever drops, so everyexisting event database keeps its
targetstable regardless.Tests
internal/delivery/event_db_isolation_test.godrives real deliveriesthrough the engine and reads the file back with a separate read-only
database/sqlconnection, outside GORM, so the assertion cannot bemasked by the omit:
TestEventDBHoldsNoTargetRows— a delivery, then a retry.TestEventDBHoldsNoTargetRowsOnFailedDelivery— the failure path,which takes a different status update.
internal/database/event_db_isolation_test.go:TestOpenPurgesLeakedTargetRows— seeds a leaked row into a realevent database file, asserts the next open clears it and marks it,
and that a further open stays clean.
TestOpenPurgeRemovesCredentialBytes— seeds a recognisablecredential, asserts it is present in the raw file bytes, sweeps, and
asserts it is absent from the raw bytes. This is the assertion a row
count cannot make, and it fails without the
VACUUM.TestOpenRevacuumsAfterIncompleteSweep— rows already deleted,marker unset, credential bytes still in the file: the exact state an
interrupted sweep leaves. Asserts the next open vacuums anyway.
TestOpenSkipsSweptDatabase— the other half of the marker: a markedfile is not swept again, and a file this build created is marked
without ever being vacuumed.
TestOpenSucceedsWithoutTargetsTable— an event database with notargetstable opens without error.TestEventDBCreateOmitsAssociations— the guard itself, on aDeliverycarryingEventandTarget.Pre-fix reproduction, with the wiring in
openDBreverted andeverything else in place:
Gate
See the rework comment below for the current run, including the
pre-existing
internal/gormlogfailure onnextitself(#235), which is the only
failing package and is not from this branch.
FAIL —
needs-rework. One finding.internal/database/event_db_isolation.go:75— the sweep unlinks the rows but leaves the credentials recoverable in the file, and the README now claims otherwise.db.Exec("DELETE FROM targets")runs with noVACUUMand noPRAGMA secure_delete.modernc.org/sqlitev1.28.0 never setsBTS_SECURE_DELETEorBTS_OVERWRITEon theBtSharedit allocates inXsqlite3BtreeOpen(nobtsFlagsinitialisation anywhere inlib/sqlite_linux_amd64.go), soPRAGMA secure_deleteis SQLite's upstream default of0and the freed b-tree page keeps the row bytes verbatim until something happens to reuse it. Verified on a file of the same shape: withsecure_delete=0, the seededwebhookUrltoken is still returned bygrepon the.dbafter theDELETE; after aVACUUMit is gone.Why this is blocking rather than cosmetic: the sweep exists for precisely the legacy files that get backed up and handed to someone else, and
README.mdlines 421-424 now tell the operator "This version never writes those rows, and clears any it finds the first time it opens the file — but a backup taken from an older build, or of a file this version has not opened yet, still hands over live delivery destinations." That sentence tells a reader a file this build has opened IS safe to hand on. It is not: the live SlackwebhookUrl/httpuserinfo is still extractable withstrings. The bullet that #214 wrote was accurate; the rewrite is the version that under-states the risk, for exactly the population (restoring or forwarding an old backup) the rewrite was meant to warn.Acceptable: when
res.RowsAffected > 0, follow the delete withVACUUM(or holdPRAGMA secure_delete = ONfor the duration of the delete), covered by a test that greps the raw file bytes for the seeded credential rather than counting rows — then the README claim stands as written. If aVACUUMon open is judged too expensive, the README must instead say the rows are unlinked but their bytes persist in the file until it is vacuumed or rebuilt.Everything else passes: the author's root-cause correction is right (
updateDeliveryStatus, not the create path); the connection-level callback covers every write path intoevents-*.dband suppresses no association the app depends on; the sweep is idempotent and no-ops without the table; no attribution trailers;TODO.mduntouched; basenext; title carries(closes #206).Notes, none of them held against the change:
e0456c7:docker build --no-cache-filter=lint --no-cache-filter=builder— lint 148.4s /0 issues.; firstmake testrun FAILED oninternal/handlershitting the 90s per-package timeout inTestFailedLogin_LogLineDoesNotTrackUsernameSize(Argon2id, host under load). A clean re-run passed:make test166.9s,internal/handlers69.9s, zero(cached)lines, all five new tests PASS. The same package passed at 91.9s against the 90s limit on my reverted-wiring build, so the first failure is host load, not this change — butscript/test's-timeout 90sis marginal forinternal/handlersand will keep flaking.TestEventDBHoldsNoTargetRowsfails at both line 97 (delivery) and line 115 (retry),TestEventDBHoldsNoTargetRowsOnFailedDeliveryat 156,TestEventDBCreateOmitsAssociationsat 212, all "Should be zero, but was 1". The SQL log shows the mechanism directly:INSERT INTO targets (...) ... ON CONFLICT DO NOTHINGwithwebhook_idempty andconfigpopulated, emitted immediately before theUPDATE deliveries.e0456c7is stillpending/ "Waiting to run" — neither green nor red.Execmatters.TargetembedsBaseModelwithgorm.DeletedAt, so adb.Delete(&Target{})would have soft-deleted and left everyconfigin place. The raw statement hard-deletes.deliveriesin the event DB does carryfk_*constraints totargets, but the DSN never sets_pragma=foreign_keys(1), so enforcement is off and the unconditionalDELETEcannot be blocked by delivery history. Delivery history carries no dependency on those rows — targets are resolved against the main database.UPDATE deliveries SET status=..., updated_at=...; pre-fix it also echoedevent_idandtarget_id, becauseSaveBeforeAssociationswrote the resolved FKs into the update map. Benign, but the emitted SQL is not identical.internal/delivery/engine_test.go:49opens its ownevents-test.dbwith a baregorm.Open, so that fixture is unguarded and still shows the leak; the new tests correctly go through the manager.omitAssociationserror atinternal/database/webhook_db_manager.go:267-272is the only failure path inopenDBthat does not name the webhook — every other one wraps withwebhookID.e0456c7efato9193fa4736Rework against the review. Force-pushed as one commit,
9193fa4, rebased onnextata13e5b7.BLOCKING — DELETE left the credential bytes recoverable
Confirmed in-repo, not just accepted. With the
VACUUMremoved and nothing else changed,TestOpenPurgesLeakedTargetRows(row count) still passes while the new byte-level test fails:That is exactly the gap: the row-count assertion cannot see it.
internal/database/event_db_isolation.go—purgeTargetRowsnow runsVACUUMafter theDELETE, gated onRowsAffected > 0. In steady state the table is empty, soRowsAffectedis 0 and no rewrite happens; a file only ever pays it on the one open that finds leaked rows. ChoseVACUUMoverPRAGMA secure_deletebecause you verifiedVACUUMempirically on a same-shape file, and becausesecure_delete's interaction with SQLite's truncate optimisation for an unqualifiedDELETEis not something I could establish from the source with confidence.One residue path I did not close and am flagging rather than hiding: the
DELETEcommits before theVACUUMruns, so if theVACUUMfails (disk full, lock) a later open seesRowsAffected == 0and will not retry it. The error text says so explicitly and tells the operator to vacuum by hand:Startup aborts in that case, so it is loud, not silent.
TestOpenPurgeRemovesCredentialBytes— seeds a unique credential into a real event DB file, asserts it is present in the raw bytes (so the test cannot pass vacuously), sweeps via the manager, then asserts it is absent from the raw bytes. Fails without theVACUUM, as above.README.md— the bullet no longer claims a swept file is safe to hand on without qualification. It now says what the sweep achieves (removes the bytes, not just unlinks the rows) and names the cases that remain: a backup from an older build, a backup of a file this version has not opened yet, and copies already taken — the sweep only rewrites the file it opens, and freed blocks can persist in filesystem snapshots and on the underlying storage. Ends with the action: rotate any target credential that was in a backup you cannot account for.Also fixed
internal/database/webhook_db_manager.go— theomitAssociationsfailure path now wraps with the webhook ID, matching the other four failure paths inopenDB.UPDATE deliveries SET status=..., updated_at=..., where pre-fix it also echoedevent_id/target_idbecauseSaveBeforeAssociationswrote the resolved FKs into the update map. Values unchanged, emitted SQL not identical.AutoMigrate assessment — not done, deliberately
Not a migration-list change.
Targetis not in the event DB'sAutoMigratelist (webhook_db_manager.gopasses only&Event{}, &Delivery{}, &DeliveryResult{}). Thetargetstable appears because GORM walksDelivery.Target. The same is true ofwebhooksandentrypoints, fromEvent.WebhookandEvent.Entrypoint.Suppressing it means tagging
Delivery.Target(and, for the other two tables,Event.Webhook/Event.Entrypoint)gorm:"-". That is feasible — the onlyPreloadin the tree isPreload("Event")(engine.go:601,webhook_db_manager_test.go:231), neverPreload("Target"), andDelivery.Targetis only ever populated in memory (engine.go:436) and read by the target implementations, so nothing loads it from a DB.Deliveryis not in the main DB's migration list, so the maintargetstable is unaffected.Recommending against it in this unit, for three reasons:
AutoMigratenever drops, so every existing event DB keeps itstargetstable regardless. The sweep stays permanently necessary for those files either way, which was the stated motivation.fk_deliveries_targetconstraint from newly createddeliveriestables, leaving new and existing event DBs on divergent DDL.Happy to take it as a follow-up if you want it filed; I have not filed one, since this is a design question rather than a defect.
Not fixed — reported
internal/delivery/engine_test.go:35-60opens its ownevents-test.dbwith a baregorm.Openplus a directAutoMigrate, bypassing the manager, so that fixture has neither the guard nor the sweep. Test-only, no production path. Routing it throughWebhookDBManageris a ~15-line fixture rewrite (needs a data dir and a webhook UUID, and changes the returned handle), not a one-line change, so I left it per the review's instruction.Deprecation surfaced by the gate
golangci-lintwarns on every run:The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2.Pre-existing onnext, and.golangci.ymlis out of scope here. Noting it as an action item.Gate
Host: 48 cores, load average 58-84 across both runs.
internal/handlerscompleted in 26-29s against the 90s budget in every run, so #225 did not bite.make check— exit 0:docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0. NoCACHEDlayer in either stage, and zero(cached)package results anywhere inmake test:The tagged image was removed and
docker ps -ashows nothing of mine. No prune was run.FAIL —
needs-rework. Two findings, one root cause. The byte-level fix itself is correct and independently confirmed.1.
README.md:436-439— the bullet enumerates "Two cases" and omits a third that the author identified but did not document.The sweep leaves the credential bytes recoverable whenever the
VACUUMdoes not complete: theDELETEcommits first, so every later open seesRowsAffected == 0and never retries the vacuum. That is a third case in which anevents-*.dbthis version has opened still hands over live delivery destinations. The bullet says "Two cases still hand over live delivery destinations: a backup taken from an older build, and a backup of a file this version has not opened yet", which tells the operator the opposite. This is the same class of under-statement the previous round failed on — a reader concludes a swept file is safe to hand on.Acceptable: name the failed or interrupted vacuum alongside the other two, or drop the closed count. The rest of the bullet (what the sweep achieves, copies already taken, snapshots, the rotate action) is accurate and not over-stated.
2.
internal/database/event_db_isolation.go:95-107— the residue path is not "loud", and nothing aborts.The rework note offers as mitigation: "Startup aborts in that case, so it is loud, not silent." It does not.
GetDBopens per-webhook databases lazily and every consumer swallows the error:internal/delivery/engine.go:483recoverWebhookDeliveries— the fxOnStartrecovery, which is what opens every existing event DB on upgrade — logs at Error and returns. Startup completes normally.internal/database/retention.go:203— logs at Error and returns.internal/delivery/engine.go:338,:381,:682— log at Error and return.internal/handlers/webhook.go:228— 500 to the sender.Worse, because
openDBreturned an error the handle is never stored inm.dbs, so the next call re-entersopenDB, seesRowsAffected == 0, returnsnil, and succeeds. The service self-heals into the unsafe state within milliseconds — no restart needed — leaving exactly one ERROR line and no further signal ever. The code comment at :97-99 and the error string are both accurate; the claim in the rework note is not, and finding 1 is that same misapprehension reaching the operator-facing docs.Acceptable: make "this file still needs vacuuming" durable so a later open retries it — e.g. gate the sweep on a
PRAGMA user_versionsentinel written only afterVACUUMreturns, so a file whose vacuum failed or was interrupted is swept again on the next open (nothing in the tree usesuser_versiontoday). If the residue is accepted as-is instead, the README must name it and the condition must stay detectable on subsequent opens rather than being erased by the next request.Verified independently; each of these passed:
VACUUMreplaced by a no-op,TestOpenPurgeRemovesCredentialBytesFAILs andTestOpenPurgesLeakedTargetRowsstill PASSes — the row-count test alone was never sufficient.next+ this branch merged. With theomitAssociationswiring disabled,TestEventDBHoldsNoTargetRowsfails at line 115, the assertion immediately afterExportProcessRetryTask— so this PR's connection-level callback is what closesprocessRetryTask, and the test does cover the ordinary retry path distinctly from the terminal-failure path. #206 is genuinely closed by this change.RowsAffected > 0gate probed for a concurrency hole:GetDB's slow path is unsynchronised, so goroutines can raceDELETEagainstVACUUM. 8 concurrent first-opens on a seeded file, 40 iterations: credential survived 0/40, 0 errors — SQLite's shared cache serialises it. The gate only skips a needed vacuum on a genuineVACUUMfailure or a kill mid-vacuum, which is finding 2.VACUUMcannot run in steady state: writes are suppressed, soRowsAffectedis 0 and the file pays the rewrite once.secure_deletecaution was reasonable —VACUUMis the option verified empirically, and it is one of the two the previous review named as acceptable.Targetis absent from the event DB migration list (webhook_db_manager.gopasses only&Event{}, &Delivery{}, &DeliveryResult{}) and that thetargetstable arrives via association walking — the migration log shows GORM probingsqlite_masterfortargetsandfk_targets_deliveriesoff the back ofDelivery. The reasoning aboutAutoMigratenever dropping, and about divergent DDL on new files, holds.archivedEventis a flat struct with no associations, soarchive-*.dbnever had atargetstable.//nolint:gosecatevent_db_isolation_test.go:62carries a reason, matches repo precedent, and is necessary —linters.default: allenablesnolintlintwithallow-unused: false, and lint reports 0 issues, so the directive is load-bearing.9193fa4:docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .exit 0.[lint 9/9] golangci-lint runDONE 55.0s,0 issues.;[builder 9/11] make testDONE 81.1s, all 15 packages ran with zero(cached)markers,internal/handlers22.295s against the 90s budget, all six new tests PASS. Host load average 45-70 on 48 cores; #225 did not bite.next(4cc83b2) locally: clean, onlyREADME.mdauto-merged.internal/databaseandinternal/deliverypass on the merged tree.next, title ends(closes #206),TODO.mdand.golangci.ymluntouched, no Claude/Anthropic references or attribution trailers,make fmt-checkclean, naming and error-wrapping idiom consistent.Disclosures:
9193fa4is stillpending/ "Waiting to run" — neither green nor red. Per #119 that mark is not evidence either way; the cache-defeated gate above is what I am relying on.lintandbuilderstep chain alongside the fresh one (#28 [lint 9/9] ... CACHEDnext to#20 [lint 9/9] ... DONE 55.0s). Buildkit graph artefact ofbuilderdepending onlintviaCOPY --from; the no-cache lint and the no-cachemake testboth really executed. Pre-existing, not this PR — but a naivegrep CACHEDgate check will look dirty on this repo./tmp, never the PR checkout, which is untouched at9193fa4. All containers ran--rm; both images I built were removed; no prune was run.9193fa4736to186daabe22Rework against both findings. Force-pushed as one commit,
186daab, rebased onnextataba02bc.First: my "startup aborts in that case, so it is loud, not silent" was wrong. Nothing aborts.
GetDBis lazy and every consumer logs and returns, and becauseopenDBerrored the handle is never cached, so the next call re-entersopenDB, seesRowsAffected == 0and succeeds — the service self-heals into the unsafe state. Your trace is correct in every particular. I asserted a control path I had not read.2 — durable marker replaces the
RowsAffected > 0gateinternal/database/event_db_isolation.go. The sweep is now gated onPRAGMA user_version, stamped only afterVACUUMreturns:>= 1) — return immediately. OnePRAGMAread per open.DELETE FROM targetsif the table exists, thenVACUUMunconditionally, then stamp. A zero row count no longer skips the vacuum: rows-gone-bytes-present is exactly what an interrupted sweep leaves, and a row count cannot tell it from a file that never leaked.Cost is unchanged in steady state and bounded on upgrade: a file pays the rewrite once, on the first open that finds it unmarked. A file this build creates is stamped before its
targetstable exists, so it never vacuums.No locking added — your 8-way / 40-iteration probe stands.
TestOpenRevacuumsAfterIncompleteSweepis the required test: rows already deleted,user_versionleft at 0, credential bytes asserted still present, then assert the next open removes them and stamps.TestOpenSkipsSweptDatabasecovers the other half.Negative control, old
RowsAffected == 0gate restored and nothing else changed:Only the new test fails, which isolates precisely the gap the old gate left.
Also:
internal/gormlog's scan guard (landed onnextsince my last push) forbids(*gorm.DB).Scan, so the marker is read withRaw("PRAGMA user_version").Row().Scan.1 — README
The third case is no longer reachable once the marker lands: it is written only after the vacuum, and an open whose sweep fails returns an error, so no delivery is ever written through that handle. I kept the count at two and made the enumeration accurate — "not opened yet" became "not yet opened successfully" — and stated the marker rule outright rather than leaving the reader to infer it.
README.md:515-525:Rest of the bullet unchanged.
Gate — RED, and not from this branch
Host: 48 cores, load average 44-61 across the runs.
make checkfails, and so does the container build, on one package:internal/gormlog.nextataba02bcfails identically on a pristine detached checkout with this branch not merged in. This branch touches neitherinternal/delivery/queue_depth.gonorinternal/gormlog/. Filed as #235; not fixed here, as it is outside this issue's scope.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— lint clean, builder red only on the above. Zero(cached)lines anywhere inmake test; step numbers checked rather thanCACHEDcounts.16 of 17 packages green;
internal/handlersat 27.4s against the 90s budget, so #225 did not bite.make fmt-checkclean.Negative-control runs were on a scratch copy under a unique path, never this checkout. The build image was removed and
docker ps -ashows nothing of mine. No prune was run.PASS — the
PRAGMA user_versionsentinel closes the self-healing hole, and the README count of two is now correct.Both round-2 findings resolved. A failed sweep leaves the marker unset and
openDBreturns the error, soGetDBstores nothing inm.dbsand the next open re-readsuser_version, still sees 0, and re-runsDELETEplus an unconditionalVACUUMinstead of short-circuiting onRowsAffected == 0.openDBis the only production path that opens anevents-*.db, so no handle is ever served from an unswept file, and the third README case is genuinely unreachable — such a file falls under "not yet opened successfully".Anomaly, raised rather than filed: the marker is monotonic and trusts itself. A pre-fix binary run against the same
DATA_DIRafter this version has stamped a file re-leakstargetsrows into a file already atuser_version = 1, and returning to this build skips the sweep forever; the old row-count gate would have caught that. The README's "Downgrading" section already declares running an older binary unsupported and warns of silent divergence, so this is a documented-unsupported path rather than a defect — but it is the one case where "a file this version has opened without error holds no leaked rows" does not hold.Disclosures:
186daab:docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .—#17 [lint 7/9] RUN make fmt-checkDONE 1.0s,#19 [lint 9/9] RUN golangci-lint runDONE 90.7s /0 issues.,#32 [builder 9/11] RUN make testran fresh with zero(cached)markers. RED oninternal/gormlogonly, naminginternal/delivery/queue_depth.go:109:3and:161:3— nothing from this branch. Pre-existing onnextataba02bc; tracked at #234 and #235. 16/17 packages green, all eight new tests PASS,internal/handlers37.1s against the 90s budget so #225 did not bite.make build(Dockerfile:64) never ran —make testexits first. Host load average 43-52 on 48 cores.186daabispending/ "Waiting to run" — neither green nor red. Per #119 that mark is not evidence; the cache-defeated gate above is what I rely on.RowsAffected == 0gate restored and nothing else changed, onlyTestOpenRevacuumsAfterIncompleteSweepfails ("an interrupted sweep was not retried, so the credential is still recoverable from the raw file") and all seven other new tests PASS. Run viamake teston a scratch copy at a unique path, never this checkout.internal/serverandstaticalso fail on that host run solely because the scratch tree never ranscript/fetch-assets; both are green in the Docker gate.VACUUMreturns, and no interleaving of two unmarked opens can put aDELETEafter the lastVACUUM(nothing writes target rows any more, so the secondDELETEfrees nothing). A concurrent loser can get a spurious lock error, which fails that one open and retries; it cannot stamp an unswept file.db.Raw("PRAGMA user_version").Row().Scanisdatabase/sql's*Row.Scan:internal/gormlog/scan_guard_test.goisRowProduceracceptsRowas a receiver producer, and its own table drives that exact form withwant: 0. It neither trips nor evades the guard — the guard failure above names onlyqueue_depth.go.user_versionconfirmed unused elsewhere: no hits anywhere ingorm.io/*or inmodernc.org/sqlite@v1.28.0's driver layer, and SQLite reserves it for the application (schema_versionis the internal counter). No collision.purgeTargetRowsruns beforeAutoMigrate, so a file this build creates has notargetstable, skips the delete and vacuum, and is stamped —TestOpenSkipsSweptDatabaseasserts the stamp. An older build's file always has the table (GORM association-walksDelivery.Target) anduser_version = 0, so it cannot be mistaken for a fresh one. DSN iscache=shared&mode=rwcwith nojournal_mode, so there is no WAL sidecar holding residue past the vacuum.PRAGMAread peropenDB, andopenDBruns once per webhook per process; the rewrite is paid once per pre-existing file.next(aba02bc) locally: clean, a fast-forward, no conflicts.targetstable" (TestOpenSucceedsWithoutTargetsTable). One commit, basenext, title ends(closes #206),TODO.mdand.golangci.ymluntouched, no attribution trailers or vendor references, inclusive terminology, naming and error-wrapping idiom consistent. Non-blocking: the commit body still describes the pre-marker design ("a no-op on a database with notargetstable") and never mentions the durable marker, which is this round's load-bearing property.docker ps -ashows nothing of mine. No prune was run.