From 65ace2d8561b7444051aaa928ace0fd4767f1090 Mon Sep 17 00:00:00 2001 From: clawbot Date: Mon, 24 Aug 2026 03:01:33 +0200 Subject: [PATCH] Roll back a failed webhook deletion instead of committing it (closes #262) --- internal/handlers/source_delete_test.go | 226 ++++++++++++++++++++++++ internal/handlers/source_management.go | 109 +++++++----- 2 files changed, 293 insertions(+), 42 deletions(-) diff --git a/internal/handlers/source_delete_test.go b/internal/handlers/source_delete_test.go index 2b493bd..5441139 100644 --- a/internal/handlers/source_delete_test.go +++ b/internal/handlers/source_delete_test.go @@ -2,6 +2,7 @@ package handlers_test import ( "context" + "errors" "net/http" "net/http/httptest" "os" @@ -11,6 +12,7 @@ import ( "github.com/go-chi/chi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gorm.io/gorm" "gorm.io/gorm/clause" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/handlers" @@ -73,6 +75,77 @@ func seedTarget( return tgt } +// errInjectedDelete is the failure failDeleteOnTable reports +// from a delete statement. +var errInjectedDelete = errors.New("injected delete failure") + +// seedEntrypoint inserts an entrypoint for a webhook. +func seedEntrypoint( + t *testing.T, + db *database.Database, + webhookID string, +) { + t.Helper() + + ep := &database.Entrypoint{ + WebhookID: webhookID, + Path: "ep-" + webhookID, + Active: true, + } + + require.NoError( + t, + db.DB().Omit(clause.Associations).Create(ep).Error, + ) +} + +// countRows counts the live (not soft-deleted) rows of a model +// matching column = value. +func countRows( + t *testing.T, + db *database.Database, + model any, + column, value string, +) int64 { + t.Helper() + + var n int64 + + require.NoError( + t, + db.DB().Model(model). + Where(column+" = ?", value). + Count(&n).Error, + ) + + return n +} + +// failDeleteOnTable makes every delete against the named table +// fail the way a database-level error does: the statement +// reports an error but leaves the surrounding transaction +// usable, so a caller that does not check it can go on to +// commit the statements that did succeed. +func failDeleteOnTable( + t *testing.T, + db *database.Database, + table string, +) { + t.Helper() + + require.NoError(t, db.DB().Callback().Delete(). + Before("gorm:delete"). + Register( + "test:fail_delete_"+table, + func(tx *gorm.DB) { + if tx.Statement.Table == table { + _ = tx.AddError(errInjectedDelete) + } + }, + ), + ) +} + // archivePathFor returns the archive database path the // delivery engine would use for a webhook: beside the webhook's // event database in the data directory. @@ -209,6 +282,159 @@ func TestHandleSourceDelete_KeepsArchiveFile(t *testing.T) { ) } +// TestHandleSourceDelete_FailedDeleteKeepsEverything proves +// that a failing delete statement loses nothing: the +// configuration is rolled back whole, the event database +// survives, and the operator is told the deletion failed +// instead of being redirected as though it worked. +func TestHandleSourceDelete_FailedDeleteKeepsEverything( + t *testing.T, +) { + t.Parallel() + + var ( + h *handlers.Handlers + sess *session.Session + db *database.Database + mgr *database.WebhookDBManager + ) + + app := newTestApp(t, &h, &sess, &db, &mgr) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + wh := seedWebhook(t, db) + seedEntrypoint(t, db, wh.ID) + seedTarget(t, db, wh.ID, database.TargetTypeDatabase) + + require.NoError(t, mgr.CreateDB(wh.ID)) + + eventDBPath := mgr.DBPath(wh.ID) + require.FileExists(t, eventDBPath) + + // The entrypoint delete runs first and succeeds; the target + // delete then fails, which is what the whole transaction has + // to be rolled back over. + failDeleteOnTable(t, db, "targets") + + cookies := authenticatedCookies( + t, sess, deleteTestUserID, deleteTestUsername, + ) + + req := postRequest( + "/source/"+wh.ID+"/delete", + cookies, + map[string]string{paramSourceID: wh.ID}, + ) + w := httptest.NewRecorder() + + h.HandleSourceDelete().ServeHTTP(w, req) + + assert.Equal( + t, http.StatusInternalServerError, w.Code, + "a failed deletion must be reported, not redirected", + ) + assert.Empty( + t, w.Header().Get("Location"), + "a failed deletion must not redirect to /sources", + ) + + assert.Equal( + t, int64(1), + countRows(t, db, &database.Webhook{}, "id", wh.ID), + "the webhook must survive a failed deletion", + ) + assert.Equal( + t, int64(1), + countRows( + t, db, &database.Entrypoint{}, "webhook_id", wh.ID, + ), + "the entrypoint delete must be rolled back", + ) + assert.Equal( + t, int64(1), + countRows( + t, db, &database.Target{}, "webhook_id", wh.ID, + ), + "the target must survive a failed deletion", + ) + + assert.FileExists( + t, eventDBPath, + "event history must not be destroyed when the "+ + "configuration delete did not commit", + ) +} + +// TestHandleSourceDelete_RemovesConfigAndEventDatabase is the +// positive control for the rollback above: an ordinary deletion +// still removes the webhook, its children and its event +// database. +func TestHandleSourceDelete_RemovesConfigAndEventDatabase( + t *testing.T, +) { + t.Parallel() + + var ( + h *handlers.Handlers + sess *session.Session + db *database.Database + mgr *database.WebhookDBManager + ) + + app := newTestApp(t, &h, &sess, &db, &mgr) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + wh := seedWebhook(t, db) + seedEntrypoint(t, db, wh.ID) + seedTarget(t, db, wh.ID, database.TargetTypeDatabase) + + require.NoError(t, mgr.CreateDB(wh.ID)) + + eventDBPath := mgr.DBPath(wh.ID) + require.FileExists(t, eventDBPath) + + cookies := authenticatedCookies( + t, sess, deleteTestUserID, deleteTestUsername, + ) + + req := postRequest( + "/source/"+wh.ID+"/delete", + cookies, + map[string]string{paramSourceID: wh.ID}, + ) + w := httptest.NewRecorder() + + h.HandleSourceDelete().ServeHTTP(w, req) + + require.Equal(t, http.StatusSeeOther, w.Code) + assert.Equal(t, "/sources", w.Header().Get("Location")) + + assert.Equal( + t, int64(0), + countRows(t, db, &database.Webhook{}, "id", wh.ID), + ) + assert.Equal( + t, int64(0), + countRows( + t, db, &database.Entrypoint{}, "webhook_id", wh.ID, + ), + ) + assert.Equal( + t, int64(0), + countRows( + t, db, &database.Target{}, "webhook_id", wh.ID, + ), + ) + assert.NoFileExists( + t, eventDBPath, + "a successful deletion removes the event database", + ) +} + // TestHandleTargetDelete_EvictsWhenLastDatabaseTargetGone // proves that removing the last database target releases the // archive writer. diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index e375f97..e01adf9 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -625,43 +625,27 @@ func (h *Handlers) deleteWebhookResources( webhook database.Webhook, userID string, ) { - tx := h.db.DB().Begin() - if tx.Error != nil { - h.log.Error( - "failed to begin transaction", - "error", tx.Error, - ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) - - return - } - - tx.Where( - "webhook_id = ?", webhook.ID, - ).Delete(&database.Entrypoint{}) - - tx.Where( - "webhook_id = ?", webhook.ID, - ).Delete(&database.Target{}) - - tx.Delete(&webhook) - - err := tx.Commit().Error + // The configuration delete commits before the event database + // is touched. No transaction spans the main database and the + // filesystem, so one side has to go first: committing the + // configuration first means a later failure leaves an unused + // event database file on disk, while removing the event + // database first would mean a failed commit destroys the + // history of a webhook that still exists. A leftover file can + // be removed by hand; deleted history cannot be recovered. + err := h.commitWebhookDeletion(&webhook) if err != nil { - h.log.Error( - "failed to commit deletion", "error", err, - ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, "failed to delete webhook", err) return } + h.log.Info( + "webhook deleted", + "webhook_id", webhook.ID, + "user_id", userID, + ) + // Release the delivery engine's per-webhook archiving state // so a deleted webhook's archive writer (and any handle open // within its debounce window) does not linger for the @@ -671,22 +655,63 @@ func (h *Handlers) deleteWebhookResources( err = h.dbMgr.DeleteDB(webhook.ID) if err != nil { - h.log.Error( - "failed to delete webhook event database", - "webhook_id", webhook.ID, - "error", err, + // The configuration is committed, so the webhook is gone, + // but its event database file is still on disk with + // nothing referencing it. Report the failure rather than + // redirecting as though everything succeeded: the file + // needs removing by hand, and the logged error names it. + h.serverError( + w, "failed to delete webhook event database", err, ) + + return } - h.log.Info( - "webhook deleted", - "webhook_id", webhook.ID, - "user_id", userID, - ) - http.Redirect(w, r, "/sources", http.StatusSeeOther) } +// commitWebhookDeletion soft-deletes a webhook's entrypoints, +// targets and the webhook row in one transaction. Every +// statement is checked and any failure rolls the whole +// transaction back, so a caller that gets an error knows the +// configuration is untouched and the event database must be +// left alone. +func (h *Handlers) commitWebhookDeletion( + webhook *database.Webhook, +) error { + tx := h.db.DB().Begin() + if tx.Error != nil { + return tx.Error + } + + err := tx.Where( + "webhook_id = ?", webhook.ID, + ).Delete(&database.Entrypoint{}).Error + if err != nil { + tx.Rollback() + + return err + } + + err = tx.Where( + "webhook_id = ?", webhook.ID, + ).Delete(&database.Target{}).Error + if err != nil { + tx.Rollback() + + return err + } + + err = tx.Delete(webhook).Error + if err != nil { + tx.Rollback() + + return err + } + + return tx.Commit().Error +} + // evictArchiveWriter asks the delivery engine to drop its // cached archive writer for a webhook, closing the archive file // handle.