Roll back a failed webhook deletion instead of committing it (closes #262)

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.

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.