Name each database target's archive for its webhook and target (closes #376)

Each database target now writes its own archive file, archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, named by delivery.ArchiveFileName, in place of one archive per webhook keyed on its UUID. Renaming a webhook or a target renames its archive files (with any -wal and -shm) before the new name is saved, never over an existing file, and moves every one back if a rename or the save fails. The webhook edit, the target edit and target creation share one lock so no two interleave. Deleting a webhook or target leaves its files on disk. Nothing looks for the old archive-WEBHOOKID.db files. The README gives the naming and the recovery steps.

Model: opus-5-5

internal/delivery/target_database_test.go

@@ -4,13 +4,12 @@ import (
 	"database/sql"
 	"fmt"
 	"log/slog"
-	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
-	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gorm.io/driver/sqlite"
@@ -74,25 +73,18 @@ func removeArchiveFiles(t *testing.T, path string) {
 
 // TestDeliverDatabase_ArchivesEvent verifies that delivering to
 // a database target marks the delivery delivered and archives
-// the full event into a separate per-webhook archive file.
+// the full event into the target's own archive file.
 func TestDeliverDatabase_ArchivesEvent(t *testing.T) {
 	t.Parallel()
 
-	dataDir := t.TempDir()
-	dbMgr := database.NewTestWebhookDBManager(dataDir)
-
-	e := delivery.NewTestEngineWithDB(
-		nil, dbMgr,
-		archiveTestLogger(),
-		&http.Client{Timeout: 5 * time.Second},
-		1,
-	)
+	env := setupArchiveTest(t)
+	tgt := env.seedDatabaseTarget(t, "")
 
 	webhookDB := testWebhookDB(t)
 	event := seedEvent(t, webhookDB, `{"archived":true}`)
-	d := seedDatabaseTargetDelivery(t, webhookDB, event, "")
+	d := seedDatabaseTargetDelivery(t, webhookDB, event, tgt)
 
-	e.ExportDeliverDatabase(webhookDB, d)
+	env.eng.ExportDeliverDatabase(webhookDB, d)
 
 	var updated database.Delivery
 
@@ -105,8 +97,7 @@ func TestDeliverDatabase_ArchivesEvent(t *testing.T) {
 	)
 
 	archivePath := filepath.Join(
-		dataDir,
-		fmt.Sprintf("archive-%s.db", event.WebhookID),
+		env.dataDir, "archive-sweep-test-archive-"+tgt.ID+".db",
 	)
 	assert.FileExists(t, archivePath)
 
@@ -288,31 +279,31 @@ func TestParseArchiveExpiry(t *testing.T) {
 	}
 }
 
-// seedDatabaseTargetDelivery seeds a pending delivery for a
-// database target with the given config JSON and returns the
-// in-memory delivery the target handler is invoked with.
+// seedDatabaseTargetDelivery seeds a pending delivery of an event
+// to a database target and returns the in-memory delivery the
+// target handler is invoked with.
 func seedDatabaseTargetDelivery(
 	t *testing.T,
 	webhookDB *gorm.DB,
 	event database.Event,
-	config string,
+	tgt *database.Target,
 ) *database.Delivery {
 	t.Helper()
 
 	dlv := seedDelivery(
-		t, webhookDB, event.ID, uuid.New().String(),
+		t, webhookDB, event.ID, tgt.ID,
 		database.DeliveryStatusPending,
 	)
 
 	d := &database.Delivery{
 		EventID:  event.ID,
-		TargetID: dlv.TargetID,
+		TargetID: tgt.ID,
 		Status:   database.DeliveryStatusPending,
 		Event:    event,
 		Target: database.Target{
-			Name:   "test-db",
+			Name:   tgt.Name,
 			Type:   database.TargetTypeDatabase,
-			Config: config,
+			Config: tgt.Config,
 		},
 	}
 	d.ID = dlv.ID
@@ -330,22 +321,14 @@ func TestDeliverDatabase_ArchiveFailureFailsDelivery(
 ) {
 	t.Parallel()
 
-	dataDir := t.TempDir()
-
-	e := delivery.NewTestEngineWithDB(
-		nil, database.NewTestWebhookDBManager(dataDir),
-		archiveTestLogger(),
-		&http.Client{Timeout: 5 * time.Second},
-		1,
-	)
+	env := setupArchiveTest(t)
+	tgt := env.seedDatabaseTarget(t, `{"expiry":"nonsense"}`)
 
 	webhookDB := testWebhookDB(t)
 	event := seedEvent(t, webhookDB, `{"archived":false}`)
-	d := seedDatabaseTargetDelivery(
-		t, webhookDB, event, `{"expiry":"nonsense"}`,
-	)
+	d := seedDatabaseTargetDelivery(t, webhookDB, event, tgt)
 
-	e.ExportDeliverDatabase(webhookDB, d)
+	env.eng.ExportDeliverDatabase(webhookDB, d)
 
 	var updated database.Delivery
 
@@ -373,10 +356,7 @@ func TestDeliverDatabase_ArchiveFailureFailsDelivery(
 	)
 
 	assert.NoFileExists(t,
-		filepath.Join(
-			dataDir,
-			fmt.Sprintf("archive-%s.db", event.WebhookID),
-		),
+		env.archivePath(tgt),
 		"no archive file should exist for a failed config",
 	)
 }
@@ -400,3 +380,290 @@ func TestValidateArchiveExpiry(t *testing.T) {
 		)
 	}
 }
+
+// TestArchiveFileName pins the archive file name and the rules
+// that make a webhook or target name safe to put in it.
+func TestArchiveFileName(t *testing.T) {
+	t.Parallel()
+
+	const id = "3f2a1c9e-8d4b-4c1a-9e2f-0a1b2c3d4e5f"
+
+	cases := []struct {
+		name    string
+		webhook string
+		target  string
+		want    string
+	}{
+		{
+			"plain names", "orders", "archive",
+			"archive-orders-archive-" + id + ".db",
+		},
+		{
+			"lowercased", "Orders", "Main Archive",
+			"archive-orders-main-archive-" + id + ".db",
+		},
+		{
+			"a run of other characters is one dash",
+			`a  /\..b`, "c__--d",
+			"archive-a-b-c-d-" + id + ".db",
+		},
+		{
+			"no dash at either end", "  --orders!! ", "(archive)",
+			"archive-orders-archive-" + id + ".db",
+		},
+		{
+			"path separators", "../../etc/passwd", "a/b",
+			"archive-etc-passwd-a-b-" + id + ".db",
+		},
+		{
+			"letters outside ASCII are dropped",
+			"Bestellungen Größe", "café",
+			"archive-bestellungen-gr-e-caf-" + id + ".db",
+		},
+		{
+			"nothing left is unnamed", "", "!!!",
+			"archive-unnamed-unnamed-" + id + ".db",
+		},
+		{
+			"cut to 40 characters", strings.Repeat("a", 50), "x",
+			"archive-" + strings.Repeat("a", 40) + "-x-" + id + ".db",
+		},
+		{
+			"no dash left by the cut",
+			strings.Repeat("a", 39) + " b", "x",
+			"archive-" + strings.Repeat("a", 39) + "-x-" + id + ".db",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.Equal(
+				t, tc.want,
+				delivery.ArchiveFileName(tc.webhook, tc.target, id),
+			)
+		})
+	}
+}
+
+// TestDeliverDatabase_EachTargetHasItsOwnArchive proves two
+// database targets of one webhook archive into separate files.
+func TestDeliverDatabase_EachTargetHasItsOwnArchive(t *testing.T) {
+	t.Parallel()
+
+	env := setupArchiveTest(t)
+	first := env.seedDatabaseTarget(t, "")
+	second := env.addDatabaseTarget(t, first.WebhookID, "")
+
+	webhookDB := testWebhookDB(t)
+	event := seedEvent(t, webhookDB, `{"n":1}`)
+
+	for _, tgt := range []*database.Target{first, second} {
+		env.eng.ExportDeliverDatabase(
+			webhookDB,
+			seedDatabaseTargetDelivery(t, webhookDB, event, tgt),
+		)
+	}
+
+	require.NotEqual(
+		t, env.archivePath(first), env.archivePath(second),
+	)
+	assert.Equal(
+		t, []string{event.ID},
+		archivedEventIDs(t, env.archivePath(first)),
+	)
+	assert.Equal(
+		t, []string{event.ID},
+		archivedEventIDs(t, env.archivePath(second)),
+	)
+}
+
+// TestRename_MovesTheFile proves a rename moves the archive, rows
+// and all, and that later writes go to the new name.
+func TestRename_MovesTheFile(t *testing.T) {
+	t.Parallel()
+
+	env := setupArchiveTest(t)
+	tgt := env.seedDatabaseTarget(t, "")
+	oldPath := env.archivePath(tgt)
+
+	webhookDB := testWebhookDB(t)
+	first := seedEvent(t, webhookDB, `{"n":1}`)
+	env.eng.ExportDeliverDatabase(
+		webhookDB, seedDatabaseTargetDelivery(t, webhookDB, first, tgt),
+	)
+	require.FileExists(t, oldPath)
+
+	require.NoError(
+		t, env.eng.Rename(tgt.ID, "Orders", "Long Term"),
+	)
+
+	newPath := filepath.Join(
+		env.dataDir, "archive-orders-long-term-"+tgt.ID+".db",
+	)
+
+	assert.NoFileExists(t, oldPath)
+	assert.Equal(t, []string{first.ID}, archivedEventIDs(t, newPath))
+
+	second := seedEvent(t, webhookDB, `{"n":2}`)
+	env.eng.ExportDeliverDatabase(
+		webhookDB,
+		seedDatabaseTargetDelivery(t, webhookDB, second, tgt),
+	)
+
+	assert.ElementsMatch(
+		t, []string{first.ID, second.ID},
+		archivedEventIDs(t, newPath),
+	)
+	assert.NoFileExists(
+		t, oldPath, "a write after the rename must use the new name",
+	)
+}
+
+// TestRename_NeverReplacesAFile plants a file at the new name, once
+// the .db alone, once a lone -wal and once a lone -shm, and proves
+// each time that the rename is refused, the planted file survives,
+// and the archive keeps its name and its rows.
+func TestRename_NeverReplacesAFile(t *testing.T) {
+	t.Parallel()
+
+	for _, suffix := range archiveFileSuffixes() {
+		t.Run("planted .db"+suffix, func(t *testing.T) {
+			t.Parallel()
+
+			env := setupArchiveTest(t)
+			tgt := env.seedDatabaseTarget(t, "")
+			oldPath := env.archivePath(tgt)
+
+			webhookDB := testWebhookDB(t)
+			first := seedEvent(t, webhookDB, `{"n":1}`)
+			env.eng.ExportDeliverDatabase(
+				webhookDB,
+				seedDatabaseTargetDelivery(t, webhookDB, first, tgt),
+			)
+
+			newPath := filepath.Join(
+				env.dataDir, "archive-orders-long-term-"+tgt.ID+".db",
+			)
+			plantedPath := newPath + suffix
+			require.NoError(
+				t, os.WriteFile(plantedPath, []byte("planted"), 0o600),
+			)
+
+			require.ErrorIs(
+				t, env.eng.Rename(tgt.ID, "Orders", "Long Term"),
+				delivery.ErrArchiveNameTaken,
+			)
+
+			//nolint:gosec // reads the file the test planted under t.TempDir()
+			planted, err := os.ReadFile(plantedPath)
+			require.NoError(t, err)
+			assert.Equal(t, "planted", string(planted))
+
+			second := seedEvent(t, webhookDB, `{"n":2}`)
+			env.eng.ExportDeliverDatabase(
+				webhookDB,
+				seedDatabaseTargetDelivery(t, webhookDB, second, tgt),
+			)
+
+			assert.ElementsMatch(
+				t, []string{first.ID, second.ID},
+				archivedEventIDs(t, oldPath),
+			)
+		})
+	}
+}
+
+// TestRename_BeforeTheNameIsSaved covers the order the handlers
+// use: they rename before they save the new name, so a delivery in
+// between must write under the new name although the main database
+// still has the old one. It also shows that renaming an archive that
+// does not exist yet is not an error.
+func TestRename_BeforeTheNameIsSaved(t *testing.T) {
+	t.Parallel()
+
+	env := setupArchiveTest(t)
+	tgt := env.seedDatabaseTarget(t, "")
+
+	require.NoError(
+		t, env.eng.Rename(tgt.ID, "Orders", "Archive"),
+	)
+
+	webhookDB := testWebhookDB(t)
+	event := seedEvent(t, webhookDB, `{"n":1}`)
+	env.eng.ExportDeliverDatabase(
+		webhookDB, seedDatabaseTargetDelivery(t, webhookDB, event, tgt),
+	)
+
+	assert.FileExists(
+		t,
+		filepath.Join(
+			env.dataDir, "archive-orders-archive-"+tgt.ID+".db",
+		),
+	)
+	assert.NoFileExists(t, env.archivePath(tgt))
+}
+
+// TestArchiveWriter_RenameMovesSidecars proves a rename carries
+// the -wal and -shm a crash can leave beside an archive no handle
+// has opened since. SQLite finds them by name, so a -wal left
+// behind would lose the transactions it holds.
+func TestArchiveWriter_RenameMovesSidecars(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	oldPath := filepath.Join(dir, "archive-old.db")
+	newPath := filepath.Join(dir, "archive-new.db")
+
+	for _, suffix := range archiveFileSuffixes() {
+		require.NoError(
+			t, os.WriteFile(oldPath+suffix, []byte(suffix), 0o600),
+		)
+	}
+
+	w := delivery.NewExportArchiveWriter(
+		oldPath, archiveTestLogger(), 0,
+	)
+
+	require.NoError(t, w.Rename("archive-new.db"))
+
+	for _, suffix := range archiveFileSuffixes() {
+		assert.NoFileExists(t, oldPath+suffix)
+		assert.FileExists(t, newPath+suffix)
+	}
+
+	assert.Equal(t, newPath, w.Path())
+}
+
+// TestArchiveWriter_RenameMovesBackOnFailure makes the -wal fail to
+// move after the .db has moved, and proves the .db is moved back, so
+// the archive is never split across two names. The new name is 255
+// bytes, the longest a file name may be, so the .db can take it but
+// the -wal, four bytes longer, cannot.
+func TestArchiveWriter_RenameMovesBackOnFailure(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	oldPath := filepath.Join(dir, "archive-old.db")
+	newName := strings.Repeat("a", 252) + ".db"
+
+	for _, suffix := range archiveFileSuffixes() {
+		require.NoError(
+			t, os.WriteFile(oldPath+suffix, []byte(suffix), 0o600),
+		)
+	}
+
+	w := delivery.NewExportArchiveWriter(
+		oldPath, archiveTestLogger(), 0,
+	)
+
+	require.Error(t, w.Rename(newName))
+
+	for _, suffix := range archiveFileSuffixes() {
+		assert.FileExists(t, oldPath+suffix)
+	}
+
+	assert.NoFileExists(t, filepath.Join(dir, newName))
+	assert.Equal(t, oldPath, w.Path())
+}