Skip to content

Migrations use a session-scoped advisory lock that breaks under transaction pooling #164

Description

@Hazzng

Summary

runMigrations serializes multi-replica boot with a session-scoped pg_advisory_lock held across separate transactions. That is unsafe under transaction-mode connection pooling, which is the only way we connect in production (Azure Postgres built-in PgBouncer; no direct connection is used).

DEVELOPER.md:190 already states the rule this violates:

The session-scoped pg_advisory_lock breaks silently under pgbouncer / Neon transaction-mode pooling.

What actually happens (measured)

Verified against a real PgBouncer (edoburu/pgbouncer:1.25.2, transaction mode) in front of Postgres:

Direct Through PgBouncer
Second booter waits 515 ms (mutual exclusion holds) acquires in ~10 ms
pg_advisory_unlock true false
Postgres log clean WARNING: you don't own a lock of type ExclusiveLock
Aftermath lock leaked onto a pooled server connection; a later direct client blocked ~50 s until PgBouncer recycled it

Why max: 1 does not save it

src/api/migrations.ts:45-47 reasons:

// max:1 so the session-level advisory lock below is held on the one
// connection that runs every migration.
const sql = postgres(url, { prepare: false, max: 1 });

That assumption is exactly what a transaction pooler invalidates: PgBouncer re-assigns the server connection per transaction, so the pg_advisory_lock statement, each sql.begin() migration, and the pg_advisory_unlock can all land on different backends. Client-side pool size is irrelevant.

Impact

  • Two replicas booting together run the same DDL concurrently with no mutual exclusion — the exact scenario the lock was added for (comment cites "Audit M4").
  • A leaked advisory lock persists on a pooled backend and blocks unrelated clients until recycled.
  • Silent: no error is raised at the time.

Recommended fix

All DDL in src/sql-fs/migrations/postgres/ is transaction-safe (no CREATE INDEX CONCURRENTLY, no VACUUM — verified), so the whole run can be one transaction:

  1. Wrap every migration file in a single sql.begin(...).
  2. Take pg_advisory_xact_lock(MIGRATION_LOCK_KEY) as the first statement inside it.
  3. Delete the explicit pg_advisory_unlock calls — commit/rollback releases it.

This is pooler-safe by construction and needs no direct connection. It also makes the whole migration run atomic, which the current per-file loop is not.

Provenance

Found during pre-merge load testing of #162. Not introduced by that PR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:distributedDistributed locking / caching / coherencebugSomething isn't workingseverity:highHigh severity

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions