Repo write exclusion does not work: the advisory lock is unlocked on the wrong session, leaks on every write, and does not exclude a second writer #279

acquire_write takes a session-scoped Postgres advisory lock and RepoWriteGuard::release releases it, but the two run on independent pool checkouts, so the unlock usually lands on a different backend than the one holding the lock. The result is not a subtle degradation: on main today, two writers on the same node and the same repo both acquire, and every completed write leaves a lock behind on the server.

Reproduced at 111cff7 against a real Postgres, in a throwaway worktree at origin/main.

Where it comes from

crates/gitlawb-node/src/git/repo_store.rs:

• the acquire runs sqlx::query_as("SELECT pg_try_advisory_lock($1)").fetch_one(&self.pool) inside the retry loop, which checks a connection out and returns it immediately
• the release runs sqlx::query("SELECT pg_advisory_unlock($1)").execute(&self.pool) at :388-391, an independent checkout

Advisory locks taken with pg_advisory_lock/pg_try_advisory_lock are session-scoped: only the backend that took the lock can release it. pg_advisory_unlock on a session that does not hold the key returns false with a warning rather than erroring, and the call site discards the result with let _ =, so nothing anywhere observes the failure.

Db::connect (crates/gitlawb-node/src/db/mod.rs:287-291) builds the pool with max_connections and acquire_timeout only. There is no after_release hook, so a released connection keeps its advisory locks.

What it costs, each observed by execution

1. Two writers on one node both acquire the same repo. Writer one holds the guard; writer two calls acquire_write for the same owner and repo and acquires immediately, because the pool hands it the very connection holding the lock and pg_try_advisory_lock is reentrant on the owning session. Both then write the same repo, and each release(true) uploads its own copy to Tigris, so the losing writer's tree can propagate to every node that later downloads it.

2. The lock leaks on essentially every write. 50 acquire/release cycles on an otherwise idle pool leaked 50 of 50. The same 50 cycles with concurrent traffic on the pool leaked 49 of 50.

3. The unlock silently no-ops. Instrumenting the two queries with pg_backend_pid() shows the acquire and the unlock landing on different backends, with pg_advisory_unlock returning false on exactly those. Rate is timing dependent: 19 of 50 and 45 of 50 across two runs of the same probe.

4. Locks accumulate on the server. 100 writes took the server from 0 advisory locks to 100, none released.

5. A peer node is the one that actually pays. A leaked lock is invisible to the node that leaked it, since its own pool keeps handing the holding session back for reentrant re-acquisition. Another node has no such shortcut: it sees a lock nobody will ever release, retries for 60s and fails the write. It stays that way until sqlx recycles the session, which on the 0.8.6 defaults is ten minutes idle or thirty minutes lifetime.

Reproduction

Add to mod tests in crates/gitlawb-node/src/git/repo_store.rs and run with a live DATABASE_URL:

#[sqlx::test]
async fn two_writers_same_repo_are_not_excluded(pool: PgPool) {
    let store = RepoStore::for_testing(PathBuf::from("/tmp"), pool.clone());
    let _g1 = store.acquire_write("did:key:z6MkDup", "same-repo").await.unwrap();
    // must not succeed: writer 1 still holds the guard
    let r2 = tokio::time::timeout(
        std::time::Duration::from_secs(10),
        store.acquire_write("did:key:z6MkDup", "same-repo"),
    ).await;
    assert!(r2.is_err(), "second writer must be excluded while the guard is held");
}

#[sqlx::test]
async fn completed_write_releases_its_advisory_lock(pool: PgPool) {
    let store = RepoStore::for_testing(PathBuf::from("/tmp"), pool.clone());
    let g = store.acquire_write("did:key:z6MkSeq", "leak-check").await.unwrap();
    g.release(true).await;
    let key = advisory_lock_key("did_key_z6MkSeq", "leak-check");
    let held: (i64,) = sqlx::query_as(
        "SELECT count(*) FROM pg_locks WHERE locktype='advisory' \
         AND ((classid::bigint<<32)|objid::bigint) = $1")
        .bind(key).fetch_one(&pool).await.unwrap();
    assert_eq!(held.0, 0, "lock must be released after release()");
}

Both fail on main today. Neither needs #[ignore]; CI already provisions Postgres for cargo test --workspace.

Directions, not a prescription

The fix is a design call about how to keep session affinity without making the lock connection a scarce resource, and it should be decided before it is written. PR #215 attempted the obvious shape, pinning the lock-owning PoolConnection inside the guard, and that produced its own defects across three review rounds: a dedicated two-connection pool capped the node at two concurrent writes, and a cancelled probe between the acquire and the guard's construction recycled a lock-bearing session and wedged the key. Both were reproduced there. Whatever lands here should account for them rather than rediscover them.

Options worth weighing:

• Hold the lock-owning connection for the write, sized so it cannot become a node-wide write ceiling, with the acquire arranged so no cancellation window exists between taking the lock and owning the connection.
• Use sqlx's own PgAdvisoryLock/PgAdvisoryLockGuard, whose Drop queues the release on the owning connection.
• Add an after_release hook running DISCARD ALL, which does release advisory locks (verified directly). This bounds the leak without giving correct exclusion, so it is a mitigation rather than a fix, and it costs a round trip per checkout.
• Drop the advisory lock for a different exclusion primitive entirely (a row lock inside a transaction, or a lease row), since the session-scoped lock is the source of the affinity requirement.

Whichever shape wins, the two tests above are the acceptance criteria, and they should run in the default cargo test --workspace path.

Relationship to #210 and PR #215

#210 is a different defect on the same function (the key was derived with DefaultHasher, so it was unstable across toolchains) and PR #215 fixes it. Everything above is orthogonal to the key derivation and predates it; a correct key makes exclusion reproducible across builds, and this issue is why exclusion does not work even with a correct key.