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

Each database target now has its own archive file,
archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, instead of one
archive-WEBHOOKID.db per webhook. delivery.ArchiveFileName builds the
name.

A change of webhook or target name renames its archive files under the
archive writer's lock, before the new name is saved, and back again if
the save fails. Webhook edits, target edits and target creation run one
at a time, so no two of them interleave. A rename never replaces a
file, and one that fails part way moves back what it moved. Deleting a
target evicts only that target's writer. Archive files are never
deleted, and nothing looks for files under the old name.

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())
+}