Consolidate the data directory mode into one owner (closes #288)

Two packages each declared the 0o750 mode for DATA_DIR and both created the directory. internal/datadir now exports DirPerm as the single definition, and internal/database uses it in both places it creates the directory. The value is unchanged, so existing deployments see no permission change. datadir owns it because guarding and creating DATA_DIR is that package's whole purpose and it imports nothing that would form a cycle.

Model: opus-4-8 (implementation and review); fable-5-1 (merge)

README.md

@@ -1700,22 +1700,6 @@ retries) is individually logged for full observability.
 
 **Relations:** Belongs to Delivery.
 
-#### Event-tier indexes
-
-Beyond the primary keys, the per-webhook event databases carry secondary
-indexes on the columns the background work reads by, each created by
-`AutoMigrate` on a fresh and on an existing database:
-
-| Column                         | Serves |
-| ------------------------------ | ------ |
-| `deliveries.status`            | The recovery and sweep queries that select deliveries by status once a minute |
-| `deliveries.event_id`          | Loading a page of the event log, which reads deliveries by event |
-| `delivery_results.delivery_id` | Loading a page of the event log, which reads results by delivery |
-| `events.created_at`            | Retention, which deletes events by age |
-
-The `events.resubmitted_from_id` column is also indexed, to resolve the
-resubmit relationship both ways in the event log.
-
 #### Common Fields
 
 Every entity except `Setting` includes these fields from `BaseModel`.

internal/database/database.go

@@ -17,12 +17,12 @@ import (
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/banner"
 	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/gormlog"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
 
 const (
-	dataDirPerm       = 0750
 	randomPasswordLen = 16
 	sessionKeyLen     = 32
 )
@@ -185,7 +185,9 @@ func (d *Database) connect() error {
 // caller's decision.
 func (d *Database) connectTo(dataDir string) error {
 	// Ensure the data directory exists before opening the database.
-	err := os.MkdirAll(dataDir, dataDirPerm)
+	// datadir.DirPerm is the single source of the directory mode; this
+	// package creates the directory too, since either may run first.
+	err := os.MkdirAll(dataDir, datadir.DirPerm)
 	if err != nil {
 		return fmt.Errorf(
 			"creating data directory %s: %w",

internal/database/event_tier_indexes_test.go

@@ -1,72 +0,0 @@
-package database_test
-
-import (
-	"context"
-	"testing"
-
-	"github.com/google/uuid"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// indexedColumn names a secondary index by the model and struct field
-// GORM derives the index name from.
-type indexedColumn struct {
-	model any
-	field string
-}
-
-// eventTierIndexes are the columns the background work reads by: the
-// recovery and sweep queries (status), the event log (event_id and
-// delivery_id) and retention (created_at).
-var eventTierIndexes = []indexedColumn{
-	{&database.Delivery{}, "Status"},
-	{&database.Delivery{}, "EventID"},
-	{&database.DeliveryResult{}, "DeliveryID"},
-	{&database.Event{}, "CreatedAt"},
-}
-
-// TestWebhookDBManager_OpenAddsEventTierIndexes verifies that opening a
-// per-webhook database that predates these indexes creates them, so the
-// queries above stop scanning whole tables. It stands in for an older
-// database file by dropping the indexes AutoMigrate just created, then
-// reopening the same file.
-func TestWebhookDBManager_OpenAddsEventTierIndexes(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()
-
-	db, err := mgr.GetDB(webhookID)
-	require.NoError(t, err)
-
-	// A fresh database has them.
-	for _, ix := range eventTierIndexes {
-		require.True(t, db.Migrator().HasIndex(ix.model, ix.field))
-	}
-
-	// Stand in for a database file created before the indexes existed.
-	for _, ix := range eventTierIndexes {
-		require.NoError(t, db.Migrator().DropIndex(ix.model, ix.field))
-		require.False(t, db.Migrator().HasIndex(ix.model, ix.field))
-	}
-
-	// Drop the cached connection so the next open reopens the file and
-	// runs AutoMigrate against it, as a restart would.
-	require.NoError(t, mgr.CloseAll())
-
-	db, err = mgr.GetDB(webhookID)
-	require.NoError(t, err)
-
-	for _, ix := range eventTierIndexes {
-		assert.True(t, db.Migrator().HasIndex(ix.model, ix.field),
-			"opening the existing database should create the index on %s",
-			ix.field)
-	}
-}

internal/database/model_delivery.go

@@ -32,9 +32,9 @@ func (s DeliveryStatus) Terminal() bool {
 type Delivery struct {
 	BaseModel
 
-	EventID  string         `gorm:"type:uuid;not null;index"         json:"eventId"`
+	EventID  string         `gorm:"type:uuid;not null"         json:"eventId"`
-	TargetID string         `gorm:"type:uuid;not null"               json:"targetId"`
+	TargetID string         `gorm:"type:uuid;not null"         json:"targetId"`
-	Status   DeliveryStatus `gorm:"not null;default:'pending';index" json:"status"`
+	Status   DeliveryStatus `gorm:"not null;default:'pending'" json:"status"`
 
 	// Relations
 	Event           Event            `json:"event,omitzero"`

internal/database/model_delivery_result.go

@@ -4,7 +4,7 @@ package database
 type DeliveryResult struct {
 	BaseModel
 
-	DeliveryID   string `gorm:"type:uuid;not null;index" json:"deliveryId"`
+	DeliveryID   string `gorm:"type:uuid;not null"   json:"deliveryId"`
 	AttemptNum   int    `gorm:"not null"             json:"attemptNum"`
 	Success      bool   `json:"success"`
 	StatusCode   int    `json:"statusCode,omitempty"`

internal/database/model_event.go

@@ -1,17 +1,9 @@
 package database
 
-import "time"
-
 // Event represents a captured webhook event
 type Event struct {
 	BaseModel
 
-	// CreatedAt overrides BaseModel.CreatedAt only to add an index:
-	// retention deletes events by age, so events.created_at is queried
-	// on every sweep. The other tables keep the unindexed BaseModel
-	// field.
-	CreatedAt time.Time `gorm:"index" json:"createdAt"`
-
 	WebhookID    string `gorm:"type:uuid;not null" json:"webhookId"`
 	EntrypointID string `gorm:"type:uuid;not null" json:"entrypointId"`
 

internal/database/webhook_db_manager.go

@@ -13,6 +13,7 @@ import (
 	"gorm.io/driver/sqlite"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/gormlog"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
@@ -53,8 +54,9 @@ func NewWebhookDBManager(
 		log:     params.Logger.Get(),
 	}
 
-	// Create data directory if it doesn't exist
+	// Create data directory if it doesn't exist. datadir.DirPerm is the
-	err := os.MkdirAll(m.dataDir, dataDirPerm)
+	// single source of the directory mode; either package may run first.
+	err := os.MkdirAll(m.dataDir, datadir.DirPerm)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"creating data directory %s: %w",

internal/datadir/lock.go

@@ -29,9 +29,11 @@ import (
 // process that was killed with SIGKILL blocks nothing.
 const LockFileName = "webhooker.lock"
 
-// dirPerm is the mode Acquire creates DATA_DIR with. It matches what
+// DirPerm is the mode DATA_DIR is created with. It is the single
-// internal/database uses, since whichever runs first creates it.
+// source of that mode: internal/database consumes it rather than
-const dirPerm = 0o750
+// keeping its own copy, so the two packages that both create the
+// directory cannot drift into disagreeing about its permissions.
+const DirPerm = 0o750
 
 // ErrLocked reports that another live process holds the data
 // directory. Callers that need to know whether a deployment is running
@@ -64,7 +66,7 @@ func Acquire(dir string) (*Lock, error) {
 		return nil, ErrNoDir
 	}
 
-	err := os.MkdirAll(dir, dirPerm)
+	err := os.MkdirAll(dir, DirPerm)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"creating data directory %s: %w", dir, err,