fix(storage): refuse PostgreSQL storage whose advisory locks lose their session

[aparajon]

SchemaBot's cross-instance exclusion on PostgreSQL is entirely session-scoped advisory locks: the startup bootstrap lock, the apply target lock behind OW-5, and reaper election. Behind a transaction-mode connection pooler a client connection stops mapping to one backend, so the lock is granted on a session the pod no longer reaches. A second pod reaching that backend takes the same lock (advisory locks are re-entrant per session), and pg_advisory_unlock later answers false without erroring. Both callers are told they hold the lock, and nothing in the system reports otherwise.

Hosted PostgreSQL platforms hand out the pooled connection string by default, so an operator following the platform's own setup docs lands on this configuration.

  direct connection                     transaction-mode pooler
  -----------------                     -----------------------
  pod A ── conn ── session S1           pod A ── conn ─┐
                     └── lock held                     ├── S1 ── lock held
  pod B ── conn ── session S2           pod B ── conn ─┘
                     └── refused                       └── granted (same session)

                                        pod A: release -> false, silently

Startup now proves the session binding before relying on it, and refuses to bootstrap when the proof fails, naming the endpoint that fixes it. A release that reports the lock was not held is surfaced as lost ownership rather than as a routine answer.

Invariants

ID	Effect
OW-9	Established. An advisory lock is exclusion only where the session holds still, so on PostgreSQL the binding is proven at startup rather than assumed, and a failed proof refuses the process.
OW-5	Upholds. The apply target lock is one of the locks this defect dissolves. It is unchanged; the entry now says the exclusion it claims depends on OW-9.
AV-9	Upholds. Unchanged. The refusal runs before the drift scan, so a refused bootstrap executes no DDL at all.
RC-2	Upholds. Reaper election is an efficiency gate rather than a safety one, so it is not weakened by the same pooler. It already reported a false release; that reading is now documented as what it means.

Options considered

pg_advisory_xact_lock (rejected as the fix). The obstacle named for it turns out not to be one: SET LOCAL lock_timeout applies for a whole transaction and can be re-set inside it, so the PostgreSQL convergence could run as one transaction and take a transaction-scoped lock. It still does not fix the mechanism. Reaper election guards a sweep of many independent transactions and is the wrong shape for it. More to the point, converting the lock leaves the operator on the pooled endpoint, where the rest of the storage layer has not been validated: pgx prepares statements by default, which a transaction-mode pooler only survives when the pooler tracks them itself. Making the lock pooler-safe would advertise support for a topology nothing else here has been proven against. It is the right move if SchemaBot ever wants to support transaction pooling, which is its own project.

Direct connection for the bootstrap path only (rejected). The bootstrap is not the only user of these locks. A bootstrap-only direct DSN fixes the startup race and leaves one-apply-per-deployment broken, which is the worse of the two, and it adds config surface. The remediation the refusal names is the same idea applied to the whole storage DSN, and needs no new config.

Detect and fail closed (chosen). Failing closed on ambiguity is the house rule for anything safety-relevant, and the refusal is the only thing that tells an operator their default connection string is unsafe.

The probe

Postgres.VerifySessionAffinity takes a lock on one connection, then makes a second connection hold a transaction open across the check. A transaction-mode pooler pins a backend for a transaction's duration, so the second connection takes the backend the first one's single-statement acquire just released, and the first connection's next statement lands elsewhere. That turns a rebind that would otherwise depend on load into one the probe can observe, and gives three independent readings of the same failure: the second connection takes a lock the first holds, the first no longer appears in pg_locks as its own lock's holder, and the first cannot release what it took.

It has no false positive: each reading is a fact about the pool in front of it rather than a guess about what sits behind it, so a direct connection and a session-mode pooler both pass. It is one-sided in the other direction: an idle transaction-mode pooler with spare backends can still answer every reading the healthy way, so a clean probe is evidence and not proof. The probe key is freshly random per call, so a lock left on an unreachable backend can never block anything later.

Testing

The scenario is reproduced end to end rather than only at the detection boundary: testutil.StartPostgresBehindPgBouncer puts a PgBouncer container in front of a PostgreSQL container on a shared network, in either pool mode.

• TestPostgresAdvisoryLockLosesExclusionBehindTransactionPooling pins the defect itself: two pooled connections both take the same advisory lock.
• TestEnsureSchemaPostgres_RefusesTransactionPooledConnection starts two concurrent bootstrappers over the pooled DSN, asserts neither proceeds, that both errors name the remedy, and that no DDL ran.
• TestEnsureSchemaPostgres_BootstrapsThroughSessionPooledConnection and TestPostgresVerifySessionAffinityAcceptsSessionPooling pin the other side: the refusal is keyed on the property SchemaBot needs, not on the presence of a pooler.

Operator-facing

A refused pod retries the storage boot on the existing 8-minute budget and then fails to start, logging the refusal on each attempt. docs/configuration.md gains a section on which endpoint the storage DSN needs, with the ports the hosted platforms use.

---

This PR was written by an agent (Claude Code, Opus 5).

[Copilot]

🟡 Changes recommended

The added integration test has a loop-variable capture bug and the new cleanup/release paths have concrete context-handling issues that can make tests flaky and skip intended lock-release signaling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a PostgreSQL storage safety guard that detects when the configured DSN is behind a transaction-mode pooler (no stable server session per client connection), where SchemaBot’s session-scoped advisory locks cease to provide cross-instance exclusion. Startup now verifies session affinity before proceeding, refuses unsafe configurations with an actionable message, and treats “unlock returned false” as a lost-ownership signal rather than a routine outcome.

Changes:

• Introduces a PostgreSQL session-affinity probe (Postgres.VerifySessionAffinity) and wires it into the PostgreSQL storage bootstrap path to fail closed on pooled endpoints.
• Adds PgBouncer-based integration test utilities and end-to-end integration tests covering transaction vs session pooling behavior.
• Documents the new invariant (OW-9) and operator guidance for using a session-per-connection storage endpoint.

EnsureSchema (Postgres)
┌──────────────────────────────────────────────┐
│ VerifySessionAffinity(storage DSN)           │
│   ├─ pass  → proceed with drift check/lock   │
│   └─ fail  → refuse bootstrap (actionable)   │
└──────────────────────────────────────────────┘

File summaries

File	Description
pkg/testutil/pgbouncer.go	Adds a test helper to run Postgres behind PgBouncer in different pool modes.
pkg/namedlock/postgres.go	Tightens semantics around advisory lock release and refactors discard behavior.
pkg/namedlock/postgres_test.go	Adds unit coverage for advisory-lock key splitting/round-tripping with pg_locks.
pkg/namedlock/postgres_affinity.go	Implements the session-affinity probe and connection discard helper.
pkg/namedlock/postgres_affinity_integration_test.go	Integration tests proving transaction pooling breaks exclusion and session pooling/direct works.
pkg/api/ensure_schema.go	Adds lock-release reporting to detect “unlock false” ownership loss on EnsureSchema locks.
pkg/api/ensure_schema_postgres.go	Refuses PostgreSQL bootstrap when session affinity can’t be proven; adds operator-facing refusal text.
pkg/api/ensure_schema_postgres_test.go	Adds unit test for refusing lockers that can’t verify session affinity.
pkg/api/ensure_schema_postgres_pooling_integration_test.go	Integration tests for refusing transaction-pooled storage DSNs and allowing session pooling.
docs/postgresql.md	Documents that storage DSN must be session-per-connection and links to configuration guidance.
docs/invariants.md	Adds OW-9 and updates OW-5’s advisory-lock scope assumptions.
docs/configuration.md	Adds a configuration section explaining required Postgres storage endpoint characteristics.

Review details

Suppressed comments (1)

pkg/testutil/pgbouncer.go:56

• The docker network cleanup uses t.Context(), but testing cancels that context before running t.Cleanup callbacks. That makes nw.Remove(ctx) likely to fail with context cancellation and can leak networks/containers across the integration suite.

	ctx := t.Context()

	nw, err := network.New(ctx)
	require.NoError(t, err, "failed to create docker network")
	t.Cleanup(func() {
		if err := nw.Remove(ctx); err != nil {
			t.Logf("failed to remove docker network: %v", err)
		}
	})

• Files reviewed: 12/12 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.