From a5addabac1fd7095a11df0249d1623a89a109807 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 19:07:59 +0000 Subject: [PATCH 1/3] Revert the migration advisory lock The lock guarded against two processes running AutoMigrate at once. That cannot happen here. db.Connect has one non-test caller, main.go:48. docker-compose gives the server container_name: quotient_server, a fixed name that pins the service to one instance, and sets no replica count. The five replicas belong to the runner, which works off Redis and never calls db.Connect. restart: always restarts sequentially. So the condition was created only by go test running two package binaries against one database. -p 1 on the integration job already fixes that: 8 of 8 runs clean with the lock reverted, while the same two binaries started concurrently still collide 3 of 6 times. The lock also made startup able to block forever, since pg_advisory_lock has no timeout and nothing logged while waiting, and it implied that concurrent migration is supported when AutoMigrate at startup would be the wrong mechanism for it anyway. engine/db/db.go returns byte for byte to its state before b1a6335. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NqKNZiXr2BPn1ztnmMmiXS --- engine/db/db.go | 42 ------------------------------------------ 1 file changed, 42 deletions(-) diff --git a/engine/db/db.go b/engine/db/db.go index 7eeba9a..bd8678d 100644 --- a/engine/db/db.go +++ b/engine/db/db.go @@ -1,7 +1,6 @@ package db import ( - "context" "errors" "fmt" "log" @@ -40,47 +39,6 @@ func Connect(connectURL string) { slog.Info("Connected to DB") - migrate() -} - -// migrationLockID identifies the advisory lock that serializes schema -// migration. Any value works as long as every process agrees on it. -const migrationLockID int64 = 0x71756F74 - -// migrate applies the schema under an advisory lock. -// -// AutoMigrate and CREATE ... IF NOT EXISTS both read the catalog and then -// write, so two processes connecting at once can each decide a table is -// missing and issue CREATE TABLE. The loser gets "relation already exists" or -// a unique violation on pg_type. The lock makes the read-then-write pair -// exclusive; the second process runs its migration afterwards and finds -// nothing to do. -func migrate() { - sqlDB, err := db.DB() - if err != nil { - log.Fatalln("Failed to access database handle:", err) - } - - // The lock is session scoped, so it has to be held on one pinned - // connection rather than borrowed from the pool per statement. - ctx := context.Background() - conn, err := sqlDB.Conn(ctx) - if err != nil { - log.Fatalln("Failed to acquire connection for migration lock:", err) - } - defer func() { - if _, err := conn.ExecContext(ctx, "SELECT pg_advisory_unlock($1)", migrationLockID); err != nil { - slog.Error("failed to release migration lock", "error", err) - } - if err := conn.Close(); err != nil { - slog.Error("failed to close migration lock connection", "error", err) - } - }() - - if _, err := conn.ExecContext(ctx, "SELECT pg_advisory_lock($1)", migrationLockID); err != nil { - log.Fatalln("Failed to acquire migration lock:", err) - } - err = db.AutoMigrate(&AnnouncementSchema{}, &TeamSchema{}, &RoundSchema{}, &ServiceCheckSchema{}, &SLASchema{}, &ManualAdjustmentSchema{}, &InjectSchema{}, &SubmissionSchema{}, &TeamServiceCheckSchema{}, From 5032b3b1184240e507ee72155522aa47bd33ec95 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 19:31:04 +0000 Subject: [PATCH 2/3] Say why -p 1 is needed, and record that Connect migrates The -p 1 comment listed the Redis queues and the truncated rows as the reasons the two packages cannot run together. It did not mention AutoMigrate. That was true while the advisory lock existed; after reverting it, -p 1 is the only thing keeping two AutoMigrate runs off one catalog, so a reader who isolated the queues and the rows would drop the flag and get log.Fatalln at db.Connect. Name AutoMigrate, name the two packages rather than "these packages" (the glob matches five, and engine/checks, engine/config and engine/db hold no shared state), and point at the Makefile, which has passed -p 1 since eb53301 for the same reason. Connect gains a doc comment for the property that makes the flag necessary. It states what the code does rather than that one process happens to call it, which is a deployment fact that would rot. Verified: -p 1 clean 5 of 5 against a fresh database per run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NqKNZiXr2BPn1ztnmMmiXS --- .github/workflows/test.yml | 9 ++++----- engine/db/db.go | 3 +++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3dc225f..730698c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -79,11 +79,10 @@ jobs: run: go mod download - name: Run integration tests - # -p 1 runs one package binary at a time. These packages share a single - # Postgres database and a single Redis instance, and they use fixed - # names for both (the `tasks` and `results` lists, one schema). Running - # them concurrently lets one package consume the other's queued tasks - # and reset its rows. + # -p 1: the engine and tests/integration packages share one Postgres + # database and one Redis instance under fixed names, so in parallel they + # race in AutoMigrate, drain each other's `tasks`/`results` lists, and + # truncate each other's rows. The Makefile targets pass -p 1 too. run: go test -v -race -p 1 ./tests/integration/... ./engine/... -timeout 10m env: REDIS_HOST: localhost diff --git a/engine/db/db.go b/engine/db/db.go index bd8678d..d5cd2d6 100644 --- a/engine/db/db.go +++ b/engine/db/db.go @@ -19,6 +19,9 @@ var ( db *gorm.DB ) +// Connect opens the connection pool and runs AutoMigrate. AutoMigrate reads the +// catalog and then creates, so it is not safe to run from two processes against +// the same database at once. func Connect(connectURL string) { var err error From 92e43bd588fda7715b8a143a65f34fc1ac27f548 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 01:37:44 +0000 Subject: [PATCH 3/3] Redo the db.go merge resolution The merge interleaved two versions of Connect instead of picking one. It kept #150's DB struct and its d.createCumulativeScoresView() call, but took the old signature and the old AutoMigrate line, so the result had a function with no receiver calling d, an assignment to the package-level db that #150 deleted, and an unused gormDB. Three compile errors, and Connect no longer returned *DB, which engine.go:67 and testutil.go:69 both need. Rebuild db.go from upstream and remove only the lock. #150 had already moved migrate() onto the struct, so that method stays and loses its advisory lock mechanics; Connect keeps upstream's shape and its *DB return. The doc comment moves to Connect, the exported entry point. Against upstream this is now 4 insertions and 39 deletions in one file. Verified: build, vet and golangci-lint clean, and the integration suite 4 of 4 with a fresh database per run against the service configuration docker-compose.base.yml specifies. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NqKNZiXr2BPn1ztnmMmiXS --- engine/db/db.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/engine/db/db.go b/engine/db/db.go index 7817c1a..2acc61f 100644 --- a/engine/db/db.go +++ b/engine/db/db.go @@ -19,10 +19,10 @@ type DB struct { db *gorm.DB } -// Connect opens the connection pool and runs AutoMigrate. AutoMigrate reads the -// catalog and then creates, so it is not safe to run from two processes against -// the same database at once. -func Connect(connectURL string) { +// Connect opens the connection pool and migrates. AutoMigrate reads the catalog +// and then creates, so it is not safe to run from two processes against the same +// database at once. +func Connect(connectURL string) *DB { var err error newLogger := logger.New( @@ -42,7 +42,15 @@ func Connect(connectURL string) { slog.Info("Connected to DB") - err = db.AutoMigrate(&AnnouncementSchema{}, + db := &DB{db: gormDB} + + db.migrate() + + return db +} + +func (d *DB) migrate() { + err := d.db.AutoMigrate(&AnnouncementSchema{}, &TeamSchema{}, &RoundSchema{}, &ServiceCheckSchema{}, &SLASchema{}, &ManualAdjustmentSchema{}, &InjectSchema{}, &SubmissionSchema{}, &TeamServiceCheckSchema{}, // box schema must come first for automigrate to work