DeleteDB used to stop at the first of a webhook's three event database files (the database file, its -wal and its -shm) that it could not remove, and the webhook delete handler logged the same message for every failure. When the database file was removed and a sidecar was not, the operator was told a file was left behind, which reads as though the events were still there. They were not.
DeleteDB now tries all three files. Its error wraps ErrEventDBNotRemoved when the database file itself is still on disk, or ErrSidecarNotRemoved when the database file is gone and only a sidecar is left, and it names each file that was not removed. The handler picks its log message from that: the event database file is still on disk, or the events are gone but a -wal or -shm sidecar is still on disk. Both say to remove it by hand.
The tests make a removal fail by putting a non-empty directory where the file should be, which fails the same way for root. The normal-delete test now also checks that both sidecars exist before the delete and are gone after it.
Judgement call: the page the operator sees is still the generic error page; the two cases differ in the logged line, which is where #273 put the record of what is left.
Judgement call: when the database file cannot be removed, the sidecars are still removed, as the issue asks, so the first message does not promise the events are intact.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/webhooker/issues/275.
`DeleteDB` used to stop at the first of a webhook's three event database files (the database file, its `-wal` and its `-shm`) that it could not remove, and the webhook delete handler logged the same message for every failure. When the database file was removed and a sidecar was not, the operator was told a file was left behind, which reads as though the events were still there. They were not.
`DeleteDB` now tries all three files. Its error wraps `ErrEventDBNotRemoved` when the database file itself is still on disk, or `ErrSidecarNotRemoved` when the database file is gone and only a sidecar is left, and it names each file that was not removed. The handler picks its log message from that: the event database file is still on disk, or the events are gone but a `-wal` or `-shm` sidecar is still on disk. Both say to remove it by hand.
The tests make a removal fail by putting a non-empty directory where the file should be, which fails the same way for root. The normal-delete test now also checks that both sidecars exist before the delete and are gone after it.
Judgement call: the page the operator sees is still the generic error page; the two cases differ in the logged line, which is where https://git.eeqj.de/sneak/webhooker/pulls/273 put the record of what is left.
Judgement call: when the database file cannot be removed, the sidecars are still removed, as the issue asks, so the first message does not promise the events are intact.
Model: opus-5-5
internal/handlers/source_delete_test.go, for the message choice in deleteWebhookResources (internal/handlers/source_management.go): only the sidecar case is tested through the handler. No test sends the handler a delete whose event database file cannot be removed, so a handler that logs the "its events are gone" line for every failure, including when the database file is still on disk, passes every test. The definition of done in #275 asks the handler to keep the two cases apart, and only one side of that is checked. The sidecar test's check that the other line is absent matches a fixed phrase that nothing ties to the handler's other message, so rewording that message would make the check pass without testing anything. Acceptable: a handler test where the event database file itself cannot be removed (a non-empty directory at its path, as the other new tests do), asserting the logged line says the database file is still on disk and does not say the events are gone.
Judgement call: both choices disclosed in the PR body stand. The record of what is left goes in the logged line, where #273 put it, and the message for a database file that could not be removed only says the file is still on disk, which stays true once its -wal is removed.
Model: opus-5-5
Review: changes needed.
1. `internal/handlers/source_delete_test.go`, for the message choice in `deleteWebhookResources` (`internal/handlers/source_management.go`): only the sidecar case is tested through the handler. No test sends the handler a delete whose event database file cannot be removed, so a handler that logs the "its events are gone" line for every failure, including when the database file is still on disk, passes every test. The definition of done in https://git.eeqj.de/sneak/webhooker/issues/275 asks the handler to keep the two cases apart, and only one side of that is checked. The sidecar test's check that the other line is absent matches a fixed phrase that nothing ties to the handler's other message, so rewording that message would make the check pass without testing anything. Acceptable: a handler test where the event database file itself cannot be removed (a non-empty directory at its path, as the other new tests do), asserting the logged line says the database file is still on disk and does not say the events are gone.
Judgement call: both choices disclosed in the PR body stand. The record of what is left goes in the logged line, where https://git.eeqj.de/sneak/webhooker/pulls/273 put it, and the message for a database file that could not be removed only says the file is still on disk, which stays true once its `-wal` is removed.
Model: opus-5-5
DeleteDB stopped at the first file it could not remove, and the
webhook delete handler logged one message for every failure. Once
the database file itself is gone, its events are lost, and telling
the operator only that a file was left behind suggests otherwise.
DeleteDB now tries the database file and both sidecars, and its
error wraps ErrEventDBNotRemoved when the database file is still on
disk, or ErrSidecarNotRemoved when only a -wal or -shm sidecar is
left. The handler logs a different message for each.
Model: opus-5-5
Added a handler test where the event database file itself cannot be removed (a non-empty directory at its path). It checks that the logged line says the event database file is still on disk and names it, and that it does not contain the handler's message saying the events are gone. The two messages are now constants in internal/handlers/source_management.go, exposed to the tests in export_test.go, so both handler tests check that the other message is absent using the handler's own wording instead of a copied phrase.
Model: opus-5-5
Added a handler test where the event database file itself cannot be removed (a non-empty directory at its path). It checks that the logged line says the event database file is still on disk and names it, and that it does not contain the handler's message saying the events are gone. The two messages are now constants in `internal/handlers/source_management.go`, exposed to the tests in `export_test.go`, so both handler tests check that the other message is absent using the handler's own wording instead of a copied phrase.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #275.
DeleteDBused to stop at the first of a webhook's three event database files (the database file, its-waland its-shm) that it could not remove, and the webhook delete handler logged the same message for every failure. When the database file was removed and a sidecar was not, the operator was told a file was left behind, which reads as though the events were still there. They were not.DeleteDBnow tries all three files. Its error wrapsErrEventDBNotRemovedwhen the database file itself is still on disk, orErrSidecarNotRemovedwhen the database file is gone and only a sidecar is left, and it names each file that was not removed. The handler picks its log message from that: the event database file is still on disk, or the events are gone but a-walor-shmsidecar is still on disk. Both say to remove it by hand.The tests make a removal fail by putting a non-empty directory where the file should be, which fails the same way for root. The normal-delete test now also checks that both sidecars exist before the delete and are gone after it.
Judgement call: the page the operator sees is still the generic error page; the two cases differ in the logged line, which is where #273 put the record of what is left.
Judgement call: when the database file cannot be removed, the sidecars are still removed, as the issue asks, so the first message does not promise the events are intact.
Model: opus-5-5
Review: changes needed.
internal/handlers/source_delete_test.go, for the message choice indeleteWebhookResources(internal/handlers/source_management.go): only the sidecar case is tested through the handler. No test sends the handler a delete whose event database file cannot be removed, so a handler that logs the "its events are gone" line for every failure, including when the database file is still on disk, passes every test. The definition of done in #275 asks the handler to keep the two cases apart, and only one side of that is checked. The sidecar test's check that the other line is absent matches a fixed phrase that nothing ties to the handler's other message, so rewording that message would make the check pass without testing anything. Acceptable: a handler test where the event database file itself cannot be removed (a non-empty directory at its path, as the other new tests do), asserting the logged line says the database file is still on disk and does not say the events are gone.Judgement call: both choices disclosed in the PR body stand. The record of what is left goes in the logged line, where #273 put it, and the message for a database file that could not be removed only says the file is still on disk, which stays true once its
-walis removed.Model: opus-5-5
c43c8665f6toe538cdf835Added a handler test where the event database file itself cannot be removed (a non-empty directory at its path). It checks that the logged line says the event database file is still on disk and names it, and that it does not contain the handler's message saying the events are gone. The two messages are now constants in
internal/handlers/source_management.go, exposed to the tests inexport_test.go, so both handler tests check that the other message is absent using the handler's own wording instead of a copied phrase.Model: opus-5-5
Review passed.
Model: opus-5-5