diff --git a/internal/database/webhook_db_manager.go b/internal/database/webhook_db_manager.go index 380dbe5..adcc3eb 100644 --- a/internal/database/webhook_db_manager.go +++ b/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 { diff --git a/internal/database/webhook_db_manager_test.go b/internal/database/webhook_db_manager_test.go index 29a788c..f70f71e 100644 --- a/internal/database/webhook_db_manager_test.go +++ b/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) { diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index a05ed5d..a86e5bd 100644 --- a/internal/handlers/export_test.go +++ b/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 { diff --git a/internal/handlers/source_delete_test.go b/internal/handlers/source_delete_test.go index 3c822a3..73d9259 100644 --- a/internal/handlers/source_delete_test.go +++ b/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. diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 36a3468..377eac8 100644 --- a/internal/handlers/source_management.go +++ b/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 }