From 1002cbca930bf5da55c241caa68311e1485ce81f Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Mon, 21 Sep 2026 18:07:51 -0400 Subject: [PATCH] fix(schema): refresh library_type_name on library change; make trigger block re-runnable schema.sql is re-executed in full on every application startup, so every statement in it must be idempotent. Two related problems introduced with the cross-library move feature, plus their fix: Problem 1: media_items.library_type_name was only populated by a BEFORE INSERT trigger. When the scanner repoints an existing row to a different library (cross-library move detection), the denormalized library_type_name went stale, mislabeling the item's type for validation and display. Fix: add a second trigger, set_library_type_name_on_library_change, that fires BEFORE UPDATE OF library_id and re-runs the same population function. Problem 2 (outage): the new trigger EXECUTE FUNCTIONs set_library_type_name(), but the pre-existing idempotency dance drops that function on every startup before recreating it. On the second and later boots, DROP FUNCTION failed with SQLSTATE 2BP01 (function still depended on by the trigger created by the previous boot), aborting the whole schema transaction and crash-looping the container. Fix: drop BOTH triggers before dropping the function, and create both after it. The ordering now survives any number of restarts on any database state. Adds TestSchemaInitializationIsIdempotent, which replays the production database.Initialize twice against the same database so this class of "second startup" regression fails in CI instead of in production. Note the test uses a dedicated pgxpool: Initialize holds the advisory lock on one connection while executing the schema on another, which deadlocks against the shared test pool's MaxConns=1. --- cmd/server/tests/schema_idempotency_test.go | 53 +++++++++++++++++++++ database/schema/schema.sql | 13 ++++- 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 cmd/server/tests/schema_idempotency_test.go diff --git a/cmd/server/tests/schema_idempotency_test.go b/cmd/server/tests/schema_idempotency_test.go new file mode 100644 index 0000000..aadcfc3 --- /dev/null +++ b/cmd/server/tests/schema_idempotency_test.go @@ -0,0 +1,53 @@ +package main + +import ( + "context" + "testing" + + "bookhoard/internal/config" + "bookhoard/internal/database" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/require" +) + +// TestSchemaInitializationIsIdempotent replays the application's production +// schema initializer twice against the same database. schema.sql runs on +// EVERY app startup, so it must apply cleanly to a database where a previous +// startup already applied it. A regression here crash-loops the production +// container on its second boot (e.g. SQLSTATE 2BP01: DROP FUNCTION refused +// while a trigger created by the previous run still references it). +// +// Uses a dedicated pool: production Initialize holds the advisory lock on one +// connection while executing the schema on another, but the shared test pool +// is capped at MaxConns=1 and would deadlock. +func TestSchemaInitializationIsIdempotent(t *testing.T) { + if testing.Short() { + t.Skip("skipping schema re-initialization in -short mode") + } + + // Bring up the standard test environment (seeds users, starts server). + setup := setupTestServer(t) + defer func() { _ = setup.Close() }() + + // Dedicated pool with default connection limits for the initializer. + poolCfg, err := pgxpool.ParseConfig(config.LoadConfig().DatabaseURL()) + require.NoError(t, err, "failed to parse database URL") + pool, err := pgxpool.NewWithConfig(context.Background(), poolCfg) + require.NoError(t, err, "failed to connect for schema initialization test") + defer func() { pool.Close() }() + + ctx := context.Background() + + // First boot: applies the schema to the (possibly fresh) database. + if err := database.Initialize(ctx, pool); err != nil { + t.Fatalf("first schema initialization failed: %v", err) + } + + // Second boot against the now-initialized database: this is exactly + // where non-idempotent statements (bare CREATE TRIGGER, DROP FUNCTION + // with dependent objects, etc.) blow up. + if err := database.Initialize(ctx, pool); err != nil { + t.Fatalf("second schema initialization failed: %v — schema.sql must stay idempotent across restarts", err) + } +} diff --git a/database/schema/schema.sql b/database/schema/schema.sql index bdfccd0..57c180c 100644 --- a/database/schema/schema.sql +++ b/database/schema/schema.sql @@ -259,8 +259,11 @@ CREATE INDEX IF NOT EXISTS idx_media_items_archived ON media_items (archived_at) CREATE INDEX IF NOT EXISTS idx_media_items_tags_search ON media_items USING GIN (tags_search); CREATE INDEX IF NOT EXISTS idx_media_items_contributors_search ON media_items USING GIN (contributors_search); --- Drop existing trigger if exists +-- Drop existing triggers if exists. Both must be dropped BEFORE the function: +-- dropping a function that triggers still reference fails (SQLSTATE 2BP01), +-- which would abort the schema transaction on every startup after the first. DROP TRIGGER IF EXISTS set_library_type_name_on_insert ON media_items; +DROP TRIGGER IF EXISTS set_library_type_name_on_library_change ON media_items; -- Drop existing function if exists DROP FUNCTION IF EXISTS set_library_type_name(); @@ -279,6 +282,14 @@ CREATE TRIGGER set_library_type_name_on_insert FOR EACH ROW EXECUTE FUNCTION set_library_type_name(); +-- Keep library_type_name truthful when an item is repointed to another +-- library (cross-library move detection reuses the same function; the row +-- keeps its ID while its library changes). +CREATE TRIGGER set_library_type_name_on_library_change + BEFORE UPDATE OF library_id ON media_items + FOR EACH ROW + EXECUTE FUNCTION set_library_type_name(); + -- Add GIN indexes for pg_trgm fuzzy search performance CREATE INDEX IF NOT EXISTS idx_media_items_author_trgm ON media_items USING GIN (author gin_trgm_ops);