internal/database/webhook_db_manager.go

@@ -33,6 +33,19 @@ var errInvalidCachedDBType = errors.New(
 	"invalid cached database type",
 )
 
+// ErrEventDBNotRemoved is in DeleteDB's error when the event
+// database file itself could not be removed: it is still on disk.
+var ErrEventDBNotRemoved = errors.New(
+	"event database file not removed",
+)
+
+// ErrSidecarNotRemoved is in DeleteDB's error when the event
+// database file was removed, so its events are gone, but its -wal
+// or -shm sidecar could not be.
+var ErrSidecarNotRemoved = errors.New(
+	"event database file removed, but a -wal or -shm sidecar was not",
+)
+
 // WebhookDBManager manages per-webhook SQLite database files
 // for event storage. Each webhook gets its own dedicated
 // database containing Events, Deliveries, DeliveryResults and the
@@ -151,7 +164,10 @@ func (m *WebhookDBManager) DBExists(
 }
 
 // DeleteDB closes the connection and deletes the database file
-// for a webhook. The file is permanently removed.
+// for a webhook, with its -wal and -shm sidecars. The files are
+// permanently removed. Each file is tried even when another could
+// not be removed, and the error wraps ErrEventDBNotRemoved or
+// ErrSidecarNotRemoved to say which was left, naming each file.
 func (m *WebhookDBManager) DeleteDB(
 	webhookID string,
 ) error {
@@ -170,16 +186,23 @@ func (m *WebhookDBManager) DeleteDB(
 		}
 	}
 
-	// Delete the main DB file and WAL/SHM files
 	path := m.dbPath(webhookID)
-	for _, suffix := range []string{"", "-wal", "-shm"} {
-		err := os.Remove(path + suffix)
-		if err != nil && !os.IsNotExist(err) {
-			return fmt.Errorf(
-				"deleting webhook database file %s%s: %w",
-				path, suffix, err,
-			)
-		}
+
+	dbErr := removeFile(path)
+	sidecarErr := errors.Join(
+		removeFile(path+"-wal"),
+		removeFile(path+"-shm"),
+	)
+
+	if dbErr != nil {
+		return fmt.Errorf(
+			"%w: %w",
+			ErrEventDBNotRemoved, errors.Join(dbErr, sidecarErr),
+		)
+	}
+
+	if sidecarErr != nil {
+		return fmt.Errorf("%w: %w", ErrSidecarNotRemoved, sidecarErr)
 	}
 
 	m.log.Info(
@@ -190,6 +213,17 @@ func (m *WebhookDBManager) DeleteDB(
 	return nil
 }
 
+// removeFile removes path. A file that is already gone counts as
+// removed; the error from any other failure names the file.
+func removeFile(path string) error {
+	err := os.Remove(path)
+	if errors.Is(err, os.ErrNotExist) {
+		return nil
+	}
+
+	return err
+}
+
 // CloseAll closes all open per-webhook database connections.
 // Called during application shutdown.
 func (m *WebhookDBManager) CloseAll() error {

internal/database/webhook_db_manager_test.go

@@ -182,17 +182,91 @@ func TestWebhookDBManager_DeleteDB(t *testing.T) {
 	}
 	require.NoError(t, db.Create(event).Error)
 
+	// Under WAL, an open database that has been written to has both
+	// sidecars beside it.
+	dbPath := mgr.DBPath(webhookID)
+	require.FileExists(t, dbPath+"-wal")
+	require.FileExists(t, dbPath+"-shm")
+
 	// Delete the DB
 	require.NoError(t, mgr.DeleteDB(webhookID))
 
 	// File should no longer exist
 	assert.False(t, mgr.DBExists(webhookID))
 
-	// Verify the file is actually gone from disk
+	// Verify the files are actually gone from disk
+	assert.NoFileExists(t, dbPath)
+	assert.NoFileExists(t, dbPath+"-wal")
+	assert.NoFileExists(t, dbPath+"-shm")
+}
+
+// blockRemoval puts a non-empty directory at path, which os.Remove
+// cannot remove whoever runs the test, root included.
+func blockRemoval(t *testing.T, path string) {
+	t.Helper()
+
+	require.NoError(t, os.MkdirAll(filepath.Join(path, "keep"), 0o700))
+}
+
+// TestWebhookDBManager_DeleteDBKeepsDatabaseFile proves that when the
+// event database file cannot be removed, the error says so, and both
+// sidecars are still removed.
+func TestWebhookDBManager_DeleteDBKeepsDatabaseFile(t *testing.T) {
+	t.Parallel()
+
+	mgr, lc := setupTestWebhookDBManager(t)
+	ctx := context.Background()
+	require.NoError(t, lc.Start(ctx))
+
+	defer func() { require.NoError(t, lc.Stop(ctx)) }()
+
+	webhookID := uuid.New().String()
 	dbPath := mgr.DBPath(webhookID)
 
-	_, err = os.Stat(dbPath)
-	assert.True(t, os.IsNotExist(err))
+	blockRemoval(t, dbPath)
+	require.NoError(t, os.WriteFile(dbPath+"-wal", nil, 0o600))
+	require.NoError(t, os.WriteFile(dbPath+"-shm", nil, 0o600))
+
+	err := mgr.DeleteDB(webhookID)
+
+	require.ErrorIs(t, err, database.ErrEventDBNotRemoved)
+	require.NotErrorIs(t, err, database.ErrSidecarNotRemoved)
+	assert.Contains(t, err.Error(), dbPath)
+	assert.NoFileExists(t, dbPath+"-wal")
+	assert.NoFileExists(t, dbPath+"-shm")
+}
+
+// TestWebhookDBManager_DeleteDBKeepsSidecar proves that when the
+// event database file is removed but a sidecar is not, the error
+// says the database file is gone, and the other sidecar is still
+// removed.
+func TestWebhookDBManager_DeleteDBKeepsSidecar(t *testing.T) {
+	t.Parallel()
+
+	mgr, lc := setupTestWebhookDBManager(t)
+	ctx := context.Background()
+	require.NoError(t, lc.Start(ctx))
+
+	defer func() { require.NoError(t, lc.Stop(ctx)) }()
+
+	webhookID := uuid.New().String()
+	dbPath := mgr.DBPath(webhookID)
+
+	require.NoError(t, mgr.CreateDB(webhookID))
+	// Closing removes the sidecars, so the ones below are the only
+	// ones there.
+	require.NoError(t, mgr.CloseAll())
+
+	blockRemoval(t, dbPath+"-wal")
+	require.NoError(t, os.WriteFile(dbPath+"-shm", nil, 0o600))
+
+	err := mgr.DeleteDB(webhookID)
+
+	require.ErrorIs(t, err, database.ErrSidecarNotRemoved)
+	require.NotErrorIs(t, err, database.ErrEventDBNotRemoved)
+	assert.Contains(t, err.Error(), dbPath+"-wal")
+	assert.NoFileExists(t, dbPath)
+	assert.NoFileExists(t, dbPath+"-shm")
 }
 
 func TestWebhookDBManager_LazyCreation(t *testing.T) {

internal/handlers/export_test.go

@@ -36,6 +36,15 @@ const MaxRenderedAttemptsForTest = maxRenderedAttempts
 // the handlers enforce rather than a number copied beside it.
 const MaxTargetRetriesForTest = maxTargetRetries
 
+// EventDBLeftMsgForTest and SidecarLeftMsgForTest expose the two
+// messages the webhook delete handler logs when a file of the event
+// database is left on disk, so a test checking that one is absent
+// checks for the handler's own wording.
+const (
+	EventDBLeftMsgForTest = eventDBLeftMsg
+	SidecarLeftMsgForTest = sidecarLeftMsg
+)
+
 // PageOrFirstForTest exposes pageOrFirst for use in the handlers_test
 // package.
 func PageOrFirstForTest(s string) int {

internal/handlers/source_delete_test.go

@@ -1,8 +1,10 @@
 package handlers_test
 
 import (
+	"bytes"
 	"context"
 	"errors"
+	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -466,6 +468,121 @@ func TestHandleSourceDelete_RemovesConfigAndEventDatabase(
 	)
 }
 
+// TestHandleSourceDelete_LeftoverSidecar proves that when the event
+// database file is removed but a sidecar beside it is not, the
+// operator is told the events are gone, never that the event
+// database file is still there.
+func TestHandleSourceDelete_LeftoverSidecar(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)
+
+	logs := new(bytes.Buffer)
+	h.SetLogForTest(slog.New(slog.NewTextHandler(logs, nil)))
+
+	wh := seedWebhook(t, db)
+
+	require.NoError(t, mgr.CreateDB(wh.ID))
+	// Closing removes the sidecars, so the -wal below is the only
+	// one there.
+	require.NoError(t, mgr.CloseAll())
+
+	// A non-empty directory in the -wal file's place, which
+	// os.Remove cannot remove whoever runs the test.
+	eventDBPath := mgr.DBPath(wh.ID)
+	require.NoError(t, os.MkdirAll(
+		filepath.Join(eventDBPath+"-wal", "keep"), 0o700,
+	))
+
+	cookies := authenticatedCookies(
+		t, sess, deleteTestUserID, deleteTestUsername,
+	)
+
+	req := postRequest(
+		"/hook/"+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)
+	assert.NoFileExists(t, eventDBPath)
+	assert.Contains(t, logs.String(), "its events are gone")
+	assert.Contains(t, logs.String(), eventDBPath+"-wal")
+	assert.NotContains(
+		t, logs.String(), handlers.EventDBLeftMsgForTest,
+		"the events are gone, so the operator must not be told "+
+			"the event database file survived",
+	)
+}
+
+// TestHandleSourceDelete_LeftoverDatabaseFile proves that when the
+// event database file itself cannot be removed, the operator is told
+// it is still on disk, never that its events are gone.
+func TestHandleSourceDelete_LeftoverDatabaseFile(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)
+
+	logs := new(bytes.Buffer)
+	h.SetLogForTest(slog.New(slog.NewTextHandler(logs, nil)))
+
+	wh := seedWebhook(t, db)
+
+	// A non-empty directory in the database file's place, which
+	// os.Remove cannot remove whoever runs the test.
+	eventDBPath := mgr.DBPath(wh.ID)
+	require.NoError(t, os.MkdirAll(
+		filepath.Join(eventDBPath, "keep"), 0o700,
+	))
+
+	cookies := authenticatedCookies(
+		t, sess, deleteTestUserID, deleteTestUsername,
+	)
+
+	req := postRequest(
+		"/hook/"+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)
+	assert.Contains(
+		t, logs.String(), "event database file is still on disk",
+	)
+	assert.Contains(t, logs.String(), eventDBPath)
+	assert.NotContains(
+		t, logs.String(), handlers.SidecarLeftMsgForTest,
+		"the database file is still on disk, so the operator must "+
+			"not be told its events are gone",
+	)
+}
+
 // TestHandleTargetDelete_EvictsThatTarget proves that deleting a
 // database target releases that target's archive writer and no
 // other: the webhook's other database target keeps its own.

internal/handlers/source_management.go

@@ -723,6 +723,17 @@ func (h *Handlers) HandleSourceDelete() http.HandlerFunc {
 	}
 }
 
+// The messages deleteWebhookResources logs when a file of the event
+// database cannot be removed: the database file itself, or only a
+// sidecar once the database file is gone.
+const (
+	eventDBLeftMsg = "webhook deleted, but its event database file is " +
+		"still on disk; remove it by hand"
+	sidecarLeftMsg = "webhook deleted and its events are gone, but a " +
+		"-wal or -shm sidecar of its event database is " +
+		"still on disk; remove it by hand"
+)
+
 // deleteWebhookResources soft-deletes config and hard-deletes
 // the per-webhook event database.
 func (h *Handlers) deleteWebhookResources(
@@ -762,13 +773,18 @@ func (h *Handlers) deleteWebhookResources(
 	err = h.dbMgr.DeleteDB(webhook.ID)
 	if err != nil {
 		// The configuration is committed, so the webhook is gone,
-		// but its event database file is still on disk with
+		// but a file of its event database 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, r, "failed to delete webhook event database", err,
-		)
+		// When only a sidecar is left, the events are already
+		// gone, and the message must not suggest they survive.
+		msg := eventDBLeftMsg
+		if errors.Is(err, database.ErrSidecarNotRemoved) {
+			msg = sidecarLeftMsg
+		}
+
+		h.serverError(w, r, msg, err)
 
 		return
 	}