Advisory-lock DB migrations against concurrent replica startup

Migrate's check-then-apply loop against schema_migrations had no
locking: two replicas booting at once against a fresh or
partially-migrated database could both pass the "not yet applied"
check for the same file and race applying it, crashing whichever lost
the duplicate-key insert (confirmed: reverting the lock fails the new
test 10/10 on a duplicate-key violation, racing as early as the
CREATE TABLE IF NOT EXISTS schema_migrations statement itself).

Hold a Postgres advisory lock for Migrate's whole run, on a dedicated
connection reserved via db.Conn so lock and unlock happen on the same
session. Blocking (pg_advisory_lock), unlike the archiver/notifier's
pg_try_advisory_lock: on boot there's no later tick to defer to, so a
second replica should wait for the first to finish migrating rather
than skip ahead.

Adds internal/db's first test file, exercising two concurrent Migrate
calls against a fresh schema.

Still open: the new-incident-insert race on a webhook for a brand-new
groupKey, noted in the chart's updated comment. Login rate limiting
staying in-process, diluted across replicas, is an accepted tradeoff.

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Niklas Ye
2026-10-03 11:43:09 +02:00
parent 42180948d1
commit 0050738ca0
3 changed files with 156 additions and 5 deletions
+125
View File
@@ -0,0 +1,125 @@
package db_test
import (
"database/sql"
"fmt"
"net/url"
"os"
"strings"
"sync"
"testing"
"git.ryuvia.com/niklas/terdut-server/internal/db"
_ "github.com/jackc/pgx/v5/stdlib"
)
// TERDUT_TEST_DSN must point at a database the test role may create schemas
// in; see internal/api/testdb_test.go for the fuller rationale this mirrors.
// An unset DSN fails rather than skips, deliberately.
const testDSNEnv = "TERDUT_TEST_DSN"
// TestMigrate_ConcurrentCallersDoNotRace reproduces two replicas starting at
// once against a brand-new, unmigrated schema: both call db.Migrate at the
// same time. Before migrationLockKey, the loser could crash on a
// duplicate-key insert into schema_migrations, or contend with the winner's
// uncommitted DDL; with the advisory lock, one blocks until the other
// finishes and both return cleanly.
func TestMigrate_ConcurrentCallersDoNotRace(t *testing.T) {
dsn := os.Getenv(testDSNEnv)
if dsn == "" {
t.Fatalf("%s is not set: these tests need Postgres.\n"+
"Run `make test-db` for a local one, then\n"+
" export %s=postgres://terdut:terdut@localhost:5432/terdut_test?sslmode=disable",
testDSNEnv, testDSNEnv)
}
schema := fmt.Sprintf("migrate_race_%d", os.Getpid())
admin, err := sql.Open("pgx", dsn)
if err != nil {
t.Fatalf("connect to %s: %v", testDSNEnv, err)
}
defer admin.Close()
if _, err := admin.Exec("CREATE SCHEMA " + schema); err != nil {
t.Fatalf("create schema %s: %v", schema, err)
}
t.Cleanup(func() {
cleanup, err := sql.Open("pgx", dsn)
if err != nil {
return
}
defer cleanup.Close()
if _, err := cleanup.Exec("DROP SCHEMA " + schema + " CASCADE"); err != nil {
t.Logf("drop schema %s: %v", schema, err)
}
})
scoped := withSearchPath(dsn, schema)
const callers = 2
errs := make([]error, callers)
var wg sync.WaitGroup
for i := range callers {
wg.Add(1)
go func(i int) {
defer wg.Done()
database, err := db.Open(scoped)
if err != nil {
errs[i] = fmt.Errorf("open: %w", err)
return
}
defer database.Close()
errs[i] = db.Migrate(database)
}(i)
}
wg.Wait()
for i, err := range errs {
if err != nil {
t.Fatalf("Migrate #%d: %v", i, err)
}
}
entries, err := os.ReadDir("migrations")
if err != nil {
t.Fatalf("read migrations dir: %v", err)
}
var want int
for _, e := range entries {
if !e.IsDir() && strings.HasSuffix(e.Name(), ".sql") {
want++
}
}
check, err := sql.Open("pgx", scoped)
if err != nil {
t.Fatalf("connect for verification: %v", err)
}
defer check.Close()
var got int
if err := check.QueryRow("SELECT COUNT(*) FROM schema_migrations").Scan(&got); err != nil {
t.Fatalf("count schema_migrations: %v", err)
}
if got != want {
t.Fatalf("schema_migrations has %d row(s) after two concurrent Migrate calls, want %d (one per migration file, no duplicates)", got, want)
}
}
// withSearchPath pins a DSN to one schema. Copied from
// internal/api/testdb_test.go rather than shared: that helper lives in the
// api_test package, a separate compiled package this one cannot import.
func withSearchPath(dsn, schema string) string {
opt := "-csearch_path=" + schema
if strings.HasPrefix(dsn, "postgres://") || strings.HasPrefix(dsn, "postgresql://") {
u, err := url.Parse(dsn)
if err == nil {
q := u.Query()
q.Set("options", opt)
u.RawQuery = q.Encode()
return u.String()
}
}
return dsn + " options='" + opt + "'"
}