Stop target credentials leaking into event databases (closes #206) #223

Merged

clawbot merged 1 commits from issue-206-no-target-rows-in-event-dbs into next 2026-08-20 07:55:35 +02:00

Conversation 5 Commits 1 Files Changed 5 +795 -8

clawbot commented 2026-08-20 06:25:44 +02:00

Collaborator

Copy Link

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.go processNewTask, processRetryTask)
so the target implementations have the config to deliver with. The row
lands in the event database through updateDeliveryStatus:

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
  #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.

clawbot added 1 commit 2026-08-20 06:25:45 +02:00

Stop target credentials leaking into event databases (closes #206) ... e0456c7efa

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.

clawbot added the needs-review label 2026-08-20 06:25:52 +02:00

clawbot self-assigned this 2026-08-20 06:25:55 +02:00

clawbot commented 2026-08-20 06:53:46 +02:00

Author

Collaborator

Copy Link

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 #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`.

clawbot added needs-rework and removed needs-review labels 2026-08-20 06:55:28 +02:00

clawbot referenced this pull request 2026-08-20 07:09:12 +02:00

Expose delivery metrics on /metrics (closes #209) #224

clawbot force-pushed issue-206-no-target-rows-in-event-dbs from e0456c7efa to 9193fa4736 2026-08-20 07:10:22 +02:00 Compare

clawbot added needs-review and removed needs-rework labels 2026-08-20 07:10:32 +02:00

clawbot commented 2026-08-20 07:12:17 +02:00

Author

Collaborator

Copy Link

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 #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.

clawbot referenced this pull request 2026-08-20 07:18:25 +02:00

Expose delivery metrics on /metrics (closes #209) #224

clawbot commented 2026-08-20 07:26:30 +02:00

Author

Collaborator

Copy Link

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 #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.

clawbot added needs-rework and removed needs-review labels 2026-08-20 07:27:18 +02:00

clawbot force-pushed issue-206-no-target-rows-in-event-dbs from 9193fa4736 to 186daabe22 2026-08-20 07:39:09 +02:00 Compare

clawbot referenced this pull request 2026-08-20 07:39:19 +02:00

`next` is red: queue_depth.go calls (*gorm.DB).Scan, tripping the scan guard #235

clawbot added needs-review and removed needs-rework labels 2026-08-20 07:39:25 +02:00

clawbot commented 2026-08-20 07:40:25 +02:00

Author

Collaborator

Copy Link

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 #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.

clawbot commented 2026-08-20 07:48:48 +02:00

Author

Collaborator

Copy Link

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.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 #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.

clawbot referenced this pull request 2026-08-20 07:49:37 +02:00

The event-DB sweep marker is monotonic and trusts itself, so a downgrade re-leaks target rows permanently #238

clawbot referenced this pull request 2026-08-20 07:50:13 +02:00

Add per-delivery replay to the event log (closes #203) #240

clawbot merged commit ac782f4c5a into next 2026-08-20 07:55:35 +02:00

clawbot deleted branch issue-206-no-target-rows-in-event-dbs 2026-08-20 07:55:35 +02:00

clawbot referenced this issue from a commit 2026-08-20 07:55:37 +02:00

Stop target credentials leaking into event databases (closes #206) (#223)

clawbot referenced this pull request 2026-08-20 07:57:41 +02:00

Add per-delivery replay to the event log (closes #203) #240

clawbot referenced this pull request 2026-08-20 08:10:35 +02:00

Add per-delivery replay to the event log (closes #203) #240

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#223