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

New Issue

clawbot · 2026-08-24T02:28:48+02:00

clawbot commented

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

Found during the review of #273. Pre-existing, not introduced there.

DeleteDB (internal/database/webhook_db_manager.go) unlinks the suffixes {"", "-wal", "-shm"} in a loop and returns on the FIRST failure. So if the main .db unlink succeeds and the -wal unlink then fails, the caller receives an error and — since #273 — correctly reports it to the operator as "the event database file was left behind, remove it by hand".

That message is wrong in exactly that case. The main database is already gone; the history is destroyed. What is left behind is a -wal fragment, not the data. An operator told to clean up a leftover file will delete it and believe nothing was lost.

Why this is worth filing now rather than later

It is currently close to unreachable, because the databases open in rollback-journal mode and no -wal or -shm sidecars normally exist. #263 switches every handle to WAL, at which point those sidecars exist routinely and this path becomes genuinely reachable.

So the severity of this issue changes the moment that PR lands. Filing it while the connection is visible rather than rediscovering it later.

Still narrow — it needs os.Remove to fail on a sidecar after succeeding on the main file, which means a permissions or filesystem problem — and the consequence is a misleading message rather than additional data loss. Not milestoned.

Definition of done

DeleteDB attempts every suffix rather than returning on the first failure, and reports what actually happened: which files were removed and which were not.
The distinction the caller needs is preserved: "the database itself survived, retry is meaningful" versus "the database is gone, only fragments remain". Those warrant different operator guidance and must not collapse into one error string.
The caller in deleteWebhookResources surfaces that distinction rather than flattening it.
A test covering a failure on a sidecar after the main file was removed, asserting the message does not tell the operator the history is recoverable.

Note the interaction: once WAL lands, verify a normal delete removes all three files cleanly, since that is the common path and it currently has no coverage against sidecars existing at all.

Found during the review of https://git.eeqj.de/sneak/webhooker/pulls/273. Pre-existing, not introduced there. `DeleteDB` (`internal/database/webhook_db_manager.go`) unlinks the suffixes `{"", "-wal", "-shm"}` in a loop and returns on the FIRST failure. So if the main `.db` unlink succeeds and the `-wal` unlink then fails, the caller receives an error and — since https://git.eeqj.de/sneak/webhooker/pulls/273 — correctly reports it to the operator as "the event database file was left behind, remove it by hand". That message is wrong in exactly that case. The main database is already gone; the history is destroyed. What is left behind is a `-wal` fragment, not the data. An operator told to clean up a leftover file will delete it and believe nothing was lost. ## Why this is worth filing now rather than later It is currently close to unreachable, because the databases open in rollback-journal mode and no `-wal` or `-shm` sidecars normally exist. https://git.eeqj.de/sneak/webhooker/pulls/263 switches every handle to WAL, at which point those sidecars exist routinely and this path becomes genuinely reachable. So the severity of this issue changes the moment that PR lands. Filing it while the connection is visible rather than rediscovering it later. Still narrow — it needs `os.Remove` to fail on a sidecar after succeeding on the main file, which means a permissions or filesystem problem — and the consequence is a misleading message rather than additional data loss. Not milestoned. ## Definition of done - `DeleteDB` attempts every suffix rather than returning on the first failure, and reports what actually happened: which files were removed and which were not. - The distinction the caller needs is preserved: "the database itself survived, retry is meaningful" versus "the database is gone, only fragments remain". Those warrant different operator guidance and must not collapse into one error string. - The caller in `deleteWebhookResources` surfaces that distinction rather than flattening it. - A test covering a failure on a sidecar after the main file was removed, asserting the message does not tell the operator the history is recoverable. Note the interaction: once WAL lands, verify a normal delete removes all three files cleanly, since that is the common path and it currently has no coverage against sidecars existing at all.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#275