Check every statement in the webhook deletion transaction (closes #262) #273

clawbot · 2026-08-24T02:16:37+02:00

clawbot commented

2026-08-24 02:16:37 +02:00

Closes #262

The defect

deleteWebhookResources issued three deletes without checking any of them. A failing delete sets .Error on the returned session but leaves the transaction usable, so the handler committed whatever succeeded, redirected to /sources as though deletion worked, and then hard-deleted the per-webhook event database anyway (its own error logged and swallowed). The result was a webhook whose entrypoints or targets survived in the main database while its entire event history was permanently gone — reported to the operator as success.

The change

The transaction moves into commitWebhookDeletion, which checks each statement and tx.Rollback()s on any failure, matching commitWebhook twenty lines above rather than inventing a new idiom.
deleteWebhookResources reports the error with h.serverError — the failure convention already used throughout this file — and returns without touching the event database.

The ordering decision (config commit vs. `DeleteDB`)

No transaction spans SQLite and the filesystem, so one side has to go first and a window exists either way. The configuration commit goes first, and the event database is only removed after it succeeds.

Failure before the commit: nothing is lost. Configuration rolls back whole, the event database is untouched, operator gets a 500.
Failure of DeleteDB after the commit: the webhook is gone and an unreferenced event database file is left on disk.

That is the side chosen to fail on because a leftover file can be removed by hand while deleted history cannot be recovered. It is not reported as success: DeleteDB failure now returns h.serverError instead of the previous redirect, and the error names the file path (DeleteDB wraps it as deleting webhook database file %s: %w), so the operator has what they need to clean up. A retry of the delete returns 404, since the configuration really is gone — the logged error is the record of what is left behind.

Audit of the same pattern

The repo has exactly three Begin() sites (grep -rn '\.Begin()' internal/), and this was the only defective one:

commitWebhook (internal/handlers/source_management.go) — checks each statement, rolls back. Known good.
createAndFanOut (internal/handlers/webhook.go) — checks tx.Create(event) and buildDeliveryTasks (which checks each tx.Create(dlv)), rolls back on either. Safe.

Also checked every other write in internal/handlers (.Create/.Save/.Updates/.Delete): all single-statement, all with .Error checked, including deleteChildResource, which is the target/entrypoint deletion path. Nothing else to fix.

Verification

make check green with GOFLAGS=-count=1 — all packages ran (no (cached) lines), lint executed in Docker for 47s and reported 0 issues.

Two new tests in internal/handlers/source_delete_test.go:

TestHandleSourceDelete_FailedDeleteKeepsEverything — injects a failure into the targets delete via a GORM before-delete callback, which reproduces the exact condition in the report: .Error set on the statement with the surrounding transaction still usable. Asserts 500 with no Location header, and that the webhook, the entrypoint (deleted by the statement before the failing one, so this is the rollback proof), the target and the event database file all survive.
TestHandleSourceDelete_RemovesConfigAndEventDatabase — positive control: an ordinary deletion still returns 303 to /sources and removes the webhook, entrypoint, target and event database file.

Fail-first, with only the handler reverted, the failure test fails on every assertion — including unable to find file .../events-<id>.db, which is the data loss itself:

--- FAIL: TestHandleSourceDelete_FailedDeleteKeepsEverything
    a failed deletion must be reported, not redirected
    Should be empty, but was /sources
    the webhook must survive a failed deletion
    the entrypoint delete must be rolled back
    unable to find file ".../events-8204c8ea-....db"

End-to-end against a running instance (PORT=19140, own DATA_DIR), normal deletion still works:

before: eventdb=1 listed=1
delete=303 loc=http://127.0.0.1:19140/sources
after:  eventdb=0 listed=0 detail=404

with the webhook, its default entrypoint and a database target created through the web UI first, and the log showing webhook deleted followed by deleted per-webhook database.

Closes https://git.eeqj.de/sneak/webhooker/issues/262 ## The defect `deleteWebhookResources` issued three deletes without checking any of them. A failing delete sets `.Error` on the returned session but leaves the transaction usable, so the handler committed whatever succeeded, redirected to `/sources` as though deletion worked, and then hard-deleted the per-webhook event database anyway (its own error logged and swallowed). The result was a webhook whose entrypoints or targets survived in the main database while its entire event history was permanently gone — reported to the operator as success. ## The change - The transaction moves into `commitWebhookDeletion`, which checks each statement and `tx.Rollback()`s on any failure, matching `commitWebhook` twenty lines above rather than inventing a new idiom. - `deleteWebhookResources` reports the error with `h.serverError` — the failure convention already used throughout this file — and returns without touching the event database. ## The ordering decision (config commit vs. `DeleteDB`) No transaction spans SQLite and the filesystem, so one side has to go first and a window exists either way. **The configuration commit goes first, and the event database is only removed after it succeeds.** - Failure before the commit: nothing is lost. Configuration rolls back whole, the event database is untouched, operator gets a 500. - Failure of `DeleteDB` after the commit: the webhook is gone and an unreferenced event database file is left on disk. That is the side chosen to fail on because a leftover file can be removed by hand while deleted history cannot be recovered. It is **not** reported as success: `DeleteDB` failure now returns `h.serverError` instead of the previous redirect, and the error names the file path (`DeleteDB` wraps it as `deleting webhook database file %s: %w`), so the operator has what they need to clean up. A retry of the delete returns 404, since the configuration really is gone — the logged error is the record of what is left behind. ## Audit of the same pattern The repo has exactly three `Begin()` sites (`grep -rn '\.Begin()' internal/`), and this was the only defective one: - `commitWebhook` (`internal/handlers/source_management.go`) — checks each statement, rolls back. Known good. - `createAndFanOut` (`internal/handlers/webhook.go`) — checks `tx.Create(event)` and `buildDeliveryTasks` (which checks each `tx.Create(dlv)`), rolls back on either. Safe. Also checked every other write in `internal/handlers` (`.Create/.Save/.Updates/.Delete`): all single-statement, all with `.Error` checked, including `deleteChildResource`, which is the target/entrypoint deletion path. Nothing else to fix. ## Verification `make check` green with `GOFLAGS=-count=1` — all packages ran (no `(cached)` lines), lint executed in Docker for 47s and reported `0 issues.` Two new tests in `internal/handlers/source_delete_test.go`: - `TestHandleSourceDelete_FailedDeleteKeepsEverything` — injects a failure into the `targets` delete via a GORM before-delete callback, which reproduces the exact condition in the report: `.Error` set on the statement with the surrounding transaction still usable. Asserts 500 with no `Location` header, and that the webhook, the entrypoint (deleted by the statement *before* the failing one, so this is the rollback proof), the target and the event database file all survive. - `TestHandleSourceDelete_RemovesConfigAndEventDatabase` — positive control: an ordinary deletion still returns 303 to `/sources` and removes the webhook, entrypoint, target and event database file. Fail-first, with only the handler reverted, the failure test fails on every assertion — including `unable to find file .../events-<id>.db`, which is the data loss itself: ``` --- FAIL: TestHandleSourceDelete_FailedDeleteKeepsEverything a failed deletion must be reported, not redirected Should be empty, but was /sources the webhook must survive a failed deletion the entrypoint delete must be rolled back unable to find file ".../events-8204c8ea-....db" ``` End-to-end against a running instance (`PORT=19140`, own `DATA_DIR`), normal deletion still works: ``` before: eventdb=1 listed=1 delete=303 loc=http://127.0.0.1:19140/sources after: eventdb=0 listed=0 detail=404 ``` with the webhook, its default entrypoint and a `database` target created through the web UI first, and the log showing `webhook deleted` followed by `deleted per-webhook database`.

clawbot added 1 commit 2026-08-24 02:16:37 +02:00

Check every statement in the webhook deletion transaction (closes #262 )

check / check (push) Successful in 3m9s

Details

27d4f1c576

deleteWebhookResources issued three deletes without checking any of
them. A failing delete sets .Error on the returned session but leaves
the transaction usable, so the handler committed whatever succeeded,
redirected to /sources as though deletion had worked, and then
hard-deleted the per-webhook event database anyway. A webhook could
end up with its entrypoints or targets still in the main database and
its entire event history permanently gone, reported as a success.

The transaction moves into commitWebhookDeletion, which checks each
statement and rolls back on any failure, matching commitWebhook in the
same file. deleteWebhookResources reports the failure with
h.serverError and leaves the event database alone.

The configuration commit deliberately precedes DeleteDB: no
transaction spans SQLite and the filesystem, and a failure after the
commit leaves an unreferenced event database file, which the operator
can remove, rather than destroying history for a webhook that still
exists. That failure is reported with h.serverError too instead of a
success redirect.

clawbot added the needs-review label 2026-08-24 02:16:41 +02:00

clawbot self-assigned this 2026-08-24 02:16:41 +02:00

clawbot referenced this pull request

2026-08-24 02:16:49 +02:00

Webhook deletion commits a partial delete and reports success when a delete statement fails #262

clawbot commented

2026-08-24 02:27:46 +02:00

PASS — verified by execution with an independent harness, not by reading the author's tests.

Injection at each of the three deletes independently (entrypoints, targets, webhooks): on the parent commit all three reproduce the loss (303 to /sources, partial config committed, events-<id>.db destroyed); on this head all three give 500, no Location, and webhook + entrypoint + target + event database all intact. Positive control (normal delete) unchanged. DeleteDB failure after the commit returns 500 with the full file path in the logged error. Ordering decision ruled correct: the post-commit window leaves only a recoverable file, retry is a 404 as documented, and a stale event database can never be adopted by a new webhook (UUIDv4 IDs and the soft-deleted webhooks row keeps the primary key occupied). Audit reconfirmed independently: three Begin() sites, no .Transaction( sites, every other write in internal/handlers single-statement with .Error checked. make check green from a clean clone with GOFLAGS=-count=1 in 95s (20 packages, zero (cached); lint in Docker 61.7s, 0 issues.); CI green on 27d4f1c; merges cleanly into next.

Disclosure: the per-injection-point probes were run as scoped go test -run in throwaway copies of the tree rather than through make; the reviewed tree itself was only ever exercised via make bootstrap / make check / make fmt-check and is unmodified.

**PASS** — verified by execution with an independent harness, not by reading the author's tests. Injection at each of the three deletes independently (`entrypoints`, `targets`, `webhooks`): on the parent commit all three reproduce the loss (303 to `/sources`, partial config committed, `events-<id>.db` destroyed); on this head all three give 500, no `Location`, and webhook + entrypoint + target + event database all intact. Positive control (normal delete) unchanged. `DeleteDB` failure after the commit returns 500 with the full file path in the logged error. Ordering decision ruled **correct**: the post-commit window leaves only a recoverable file, retry is a 404 as documented, and a stale event database can never be adopted by a new webhook (UUIDv4 IDs and the soft-deleted `webhooks` row keeps the primary key occupied). Audit reconfirmed independently: three `Begin()` sites, no `.Transaction(` sites, every other write in `internal/handlers` single-statement with `.Error` checked. `make check` green from a clean clone with `GOFLAGS=-count=1` in 95s (20 packages, zero `(cached)`; lint in Docker 61.7s, `0 issues.`); CI green on `27d4f1c`; merges cleanly into `next`. Disclosure: the per-injection-point probes were run as scoped `go test -run` in throwaway copies of the tree rather than through `make`; the reviewed tree itself was only ever exercised via `make bootstrap` / `make check` / `make fmt-check` and is unmodified.

clawbot referenced this pull request

2026-08-24 02:28:48 +02:00

DeleteDB returns on the first failing suffix, so it can report a leftover file when the history is already destroyed #275

clawbot merged commit 65ace2d856 into next

2026-08-24 03:01:34 +02:00

clawbot deleted branch issue-262-delete-transaction

2026-08-24 03:01:34 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#273