From 04cb922bc6dfbf6838ecc1c951ffd07761b785ac Mon Sep 17 00:00:00 2001 From: Gravirei Date: Fri, 17 Jul 2026 22:03:18 +0600 Subject: [PATCH 1/3] fix(repo_store): use SHA-256 for stable advisory-lock key (#210) --- Cargo.lock | 11 +++--- crates/gitlawb-node/src/git/repo_store.rs | 46 ++++++++++++++++++++--- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d12daa43..24beb9ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3300,7 +3300,7 @@ dependencies = [ [[package]] name = "git-remote-gitlawb" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "gitlawb-core", @@ -3312,7 +3312,7 @@ dependencies = [ [[package]] name = "gitlawb-attest" -version = "0.5.0" +version = "0.5.1" dependencies = [ "base64", "ed25519-dalek", @@ -3329,7 +3329,7 @@ dependencies = [ [[package]] name = "gitlawb-core" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "base64", @@ -3356,7 +3356,7 @@ dependencies = [ [[package]] name = "gitlawb-node" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", @@ -3412,7 +3412,7 @@ dependencies = [ [[package]] name = "gl" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", @@ -3824,6 +3824,7 @@ dependencies = [ "reqwest", "serde", "serde_json", + "sha2", "thiserror 2.0.18", "tracing", ] diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index a5c367e9..71fb7a73 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -152,7 +152,13 @@ impl RepoStore { } /// Take a write lock (Postgres advisory lock), ensure repo is local, return guard. - /// The lock prevents concurrent writes to the same repo across machines. + /// + /// # Cross-machine guarantee + /// + /// When multiple nodes share a single Postgres database (the shared-Postgres + /// deployment model), this lock prevents concurrent writes to the same repo + /// across machines. The lock has no effect across separate Postgres instances + /// (federated per-node-DB topology). pub async fn acquire_write(&self, owner_did: &str, repo_name: &str) -> Result { let (owner_slug, local_path) = self.local_path(owner_did, repo_name)?; let lock_key = advisory_lock_key(&owner_slug, repo_name); @@ -393,12 +399,19 @@ impl RepoWriteGuard { } /// Compute a stable i64 hash for a Postgres advisory lock key. +/// +/// Uses SHA-256 (not `DefaultHasher`) so the same `(owner_slug, repo_name)` +/// produces the same `i64` key across every Rust toolchain version, operating +/// system, and machine — the algorithm is frozen by the SHA-2 standard rather +/// than by a std-internal implementation detail. fn advisory_lock_key(owner_slug: &str, repo_name: &str) -> i64 { - use std::hash::{Hash, Hasher}; - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - owner_slug.hash(&mut hasher); - repo_name.hash(&mut hasher); - hasher.finish() as i64 + use sha2::Digest; + let mut hasher = sha2::Sha256::new(); + hasher.update(owner_slug.as_bytes()); + hasher.update(b":"); + hasher.update(repo_name.as_bytes()); + let digest = hasher.finalize(); + i64::from_le_bytes(digest[..8].try_into().expect("sha256 output is >= 8 bytes")) } #[cfg(test)] @@ -562,4 +575,25 @@ mod tests { ); } } + + // ── advisory_lock_key stability ───────────────────────────────────────── + + #[test] + fn advisory_lock_key_is_stable() { + // Golden value: SHA-256("did_key_...:")[..8] as i64 little-endian. + // If this test fails, the hashing algorithm has changed — the new key + // must be backward-compatible or the rollout planned accordingly. + let key = advisory_lock_key( + "did_key_z6MkqDnb7Siv3Cwj7pGJq4T5EsUisECqR8KpnDLwcaZq5TPr", + "hello", + ); + assert_eq!(key, -6680856138670956537_i64); + } + + #[test] + fn advisory_lock_key_differs_for_different_inputs() { + let a = advisory_lock_key("owner_a", "repo_a"); + let b = advisory_lock_key("owner_b", "repo_b"); + assert_ne!(a, b); + } } From c27b3559a820baa05a24a8faab654771c32a27d5 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sun, 19 Jul 2026 12:39:55 +0600 Subject: [PATCH 2/3] fix(repo_store): hold lock-owning PoolConnection in RepoWriteGuard Address review findings on #210: - RepoWriteGuard now stores PoolConnection instead of PgPool, so the pg_advisory_unlock on release() runs on the same session that acquired the lock (no silent no-op unlock, no reentrant acquisition by a later writer handed the idle lock-owning connection). Error path also releases the lock on the same connection before returning. - Document the rolling-upgrade re-keying caveat durably on acquire_write (drain writes or cut over single-node during upgrade) and in CHANGELOG's Unreleased "Known caveats" section, since the PR body disappears on merge. - Strengthen advisory_lock_key_differs_for_different_inputs to vary one axis at a time (same-owner/diff-repo, diff-owner/same-repo) so a regression that drops one parameter from the hash is caught, not just one that drops both. Co-Authored-By: Claude Opus 4.6 --- CHANGELOG.md | 6 ++ crates/gitlawb-node/src/git/repo_store.rs | 97 ++++++++++++++++++++--- 2 files changed, 94 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11f2bcc3..6adf0737 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Known caveats + +* **repo_store:** the advisory-lock key for cross-machine write exclusion is now derived via SHA-256 of `(owner_slug, repo_name)` instead of `std::collections::DefaultHasher` (the previous algorithm was not frozen by the Rust standard). During a shared-Postgres rolling upgrade, old-binary nodes holding the legacy `DefaultHasher` key and new-binary nodes holding the SHA-256 key for the same repo compute *different* i64 keys, so PostgreSQL treats them as independent locks and cross-machine write-exclusion is lost for the deploy window. Operators should drain in-flight writes or cut over through a single node before bringing new-binary nodes online. A future transition release may acquire both keys for one cycle. See `RepoStore::acquire_write` and issue #210. + ## [0.5.1](https://github.com/Gitlawb/node/compare/v0.5.0...v0.5.1) (2026-07-10) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 71fb7a73..46f5f3bc 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -13,6 +13,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use anyhow::{Context, Result}; +use sqlx::pool::PoolConnection; use sqlx::PgPool; use tokio::sync::Mutex; use tracing::{debug, info, warn}; @@ -159,17 +160,60 @@ impl RepoStore { /// deployment model), this lock prevents concurrent writes to the same repo /// across machines. The lock has no effect across separate Postgres instances /// (federated per-node-DB topology). + /// + /// # Session affinity + /// + /// PostgreSQL advisory locks are session-scoped: the lock is released by the + /// *same* backend connection that acquired it. If the acquiring connection + /// were returned to the pool, a later writer could be handed it and + /// reentrantly acquire the same lock, while `pg_advisory_unlock` issued on + /// a different session would silently no-op. We therefore hold the + /// `PoolConnection` returned by the acquiring query inside + /// `RepoWriteGuard` and release the lock through that exact connection before + /// returning it to the pool on drop. + /// + /// # Rolling upgrade caveat (SHA-256 re-keying) + /// + /// This function hashes `(owner_slug, repo_name)` with SHA-256 to produce a + /// stable `i64` key. Earlier builds used `std::collections::DefaultHasher`, + /// whose algorithm is not frozen by the Rust standard and has already + /// shifted across toolchain versions. The SHA-256 swap re-keys *every* + /// repo: an old-binary node holding the legacy `DefaultHasher` key and a + /// new-binary node holding the SHA-256 key for the same repo compute + /// *different* i64 keys, so PostgreSQL treats them as independent locks and + /// the cross-machine write-exclusion this lock provides is lost for the + /// duration of a rolling upgrade. + /// + /// The accepted remediation (see issue #210) is operational: during a + /// shared-Postgres rolling upgrade, **drain in-flight writes or cut over + /// through a single node** (e.g. stop receive-pack / issue / pull / archive + /// writers on the old version) before bringing new-binary nodes online. The + /// window is bounded by the operator's rollout cadence. A future + /// transition release could acquire both legacy and new keys for one cycle + /// and drop the legacy one a release later; that's optional given the + /// accepted-window path. pub async fn acquire_write(&self, owner_did: &str, repo_name: &str) -> Result { let (owner_slug, local_path) = self.local_path(owner_did, repo_name)?; let lock_key = advisory_lock_key(&owner_slug, repo_name); // Acquire Postgres advisory lock with retry using pg_try_advisory_lock // to avoid blocking indefinitely on stale locks from crashed connections. + // + // We hold onto the connection that won the lock for the lifetime of the + // guard — see the method docstring on session affinity. A subsequent + // writer cannot be handed this connection and reentrantly grab the + // same lock, and the unlock on release() is guaranteed to run on the + // owning session. + let mut conn: PoolConnection = self + .pool + .acquire() + .await + .context("acquiring connection for advisory lock")?; let mut acquired = false; for attempt in 0..60 { let row: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") .bind(lock_key) - .fetch_one(&self.pool) + .fetch_one(&mut *conn) .await .context("trying advisory lock")?; if row.0 { @@ -197,6 +241,14 @@ impl RepoStore { warn!(repo = %repo_name, err = %e, "write acquire: tigris download failed — falling back to local copy"); } else { + // Drop the lock on the error path before returning; the + // connection is released to the pool by Drop on + // PoolConnection, but the lock must be released explicitly + // so a subsequent writer isn't blocked until idle timeout. + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(lock_key) + .execute(&mut *conn) + .await; return Err(e).context("downloading repo from tigris for write"); } } @@ -208,7 +260,7 @@ impl RepoStore { repo_name: repo_name.to_string(), local_path, lock_key, - pool: self.pool.clone(), + conn, tigris: self.tigris.clone(), }) } @@ -355,12 +407,16 @@ fn validate_repo_name(repo_name: &str) -> Result<()> { /// Guard returned by `acquire_write()`. Holds the Postgres advisory lock and /// uploads to Tigris + releases the lock on `release()`. +/// +/// Holds the owning `PoolConnection` so the lock is released on +/// the same session that acquired it — see `acquire_write` for the session +/// affinity rationale. pub struct RepoWriteGuard { owner_slug: String, repo_name: String, pub local_path: PathBuf, lock_key: i64, - pool: PgPool, + conn: PoolConnection, tigris: Option, } @@ -375,7 +431,7 @@ impl RepoWriteGuard { /// half-applied or otherwise inconsistent repo would propagate corruption to /// Tigris (and to every node that later downloads it). The lock is always /// released regardless, to avoid stale locks blocking future writes. - pub async fn release(self, success: bool) { + pub async fn release(mut self, success: bool) { // Upload to Tigris only on success. if success { if let Some(ref tigris) = self.tigris { @@ -390,10 +446,12 @@ impl RepoWriteGuard { warn!(repo = %self.repo_name, "write failed — skipping tigris upload to avoid propagating an inconsistent repo"); } - // Release advisory lock + // Release advisory lock on the owning session. The connection is then + // returned to the pool by `PoolConnection::Drop` when `self` drops at + // the end of this function. let _ = sqlx::query("SELECT pg_advisory_unlock($1)") .bind(self.lock_key) - .execute(&self.pool) + .execute(&mut *self.conn) .await; } } @@ -592,8 +650,29 @@ mod tests { #[test] fn advisory_lock_key_differs_for_different_inputs() { - let a = advisory_lock_key("owner_a", "repo_a"); - let b = advisory_lock_key("owner_b", "repo_b"); - assert_ne!(a, b); + // Vary one axis at a time so a regression that drops either parameter + // from the hash is caught, not just one that drops both. The golden + // test above backstops a total algorithm swap. + let base = advisory_lock_key("owner_a", "repo_a"); + + // Same owner, different repo: a regression that hashes only owner_slug + // would make these collide. + let same_owner_diff_repo = advisory_lock_key("owner_a", "repo_b"); + assert_ne!( + base, same_owner_diff_repo, + "key must depend on repo_name, not just owner_slug" + ); + + // Same repo, different owner: a regression that hashes only repo_name + // would make these collide. + let diff_owner_same_repo = advisory_lock_key("owner_b", "repo_a"); + assert_ne!( + base, diff_owner_same_repo, + "key must depend on owner_slug, not just repo_name" + ); + + // Sanity: both axes varying at once still differs (the original shape). + let both_differ = advisory_lock_key("owner_b", "repo_b"); + assert_ne!(base, both_differ); } } From 26c4dfe8a935f33bc361e2a007bb10ae136805df Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sun, 19 Jul 2026 15:50:56 +0600 Subject: [PATCH 3/3] fix(repo_store): construct guard before tigris download and add Drop to close on cancellation --- crates/gitlawb-node/src/git/repo_store.rs | 85 ++++++++++++++++------- 1 file changed, 58 insertions(+), 27 deletions(-) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 46f5f3bc..b5555ebc 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -228,41 +228,55 @@ impl RepoStore { anyhow::bail!("could not acquire advisory lock after 60s — possible stale lock for {owner_slug}/{repo_name}"); } + // Construct the guard immediately so its Drop implementation protects + // subsequent cancellation points (tigris download below). + let mut guard = RepoWriteGuard { + owner_slug, + repo_name: repo_name.to_string(), + local_path, + lock_key, + conn: Some(conn), + tigris: self.tigris.clone(), + }; + // Always download the latest from Tigris before writing. // Local disk may be stale if another machine pushed since our last access. if let Some(ref tigris) = self.tigris { - if tigris.exists(&owner_slug, repo_name).await.unwrap_or(false) { + if tigris + .exists(&guard.owner_slug, repo_name) + .await + .unwrap_or(false) + { debug!(repo = %repo_name, "write acquire: downloading latest from tigris"); - if let Err(e) = tigris.download(&owner_slug, repo_name, &local_path).await { + if let Err(e) = tigris + .download(&guard.owner_slug, repo_name, &guard.local_path) + .await + { // Same self-healing fallback as acquire_fresh: a corrupt/unreadable // Tigris archive must not block a write when a valid local copy // exists — release(success) will re-upload a good archive. - if local_path.exists() { + if guard.local_path.exists() { warn!(repo = %repo_name, err = %e, "write acquire: tigris download failed — falling back to local copy"); } else { - // Drop the lock on the error path before returning; the - // connection is released to the pool by Drop on - // PoolConnection, but the lock must be released explicitly - // so a subsequent writer isn't blocked until idle timeout. - let _ = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(lock_key) - .execute(&mut *conn) - .await; + // Drop the lock on the error path before returning. + // Unlock through the guard's connection, then clear conn + // so Drop returns it to the pool (lock-free) instead of + // closing it. + if let Some(ref mut gconn) = guard.conn { + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(guard.lock_key) + .execute(&mut **gconn) + .await; + } + guard.conn = None; return Err(e).context("downloading repo from tigris for write"); } } } } - Ok(RepoWriteGuard { - owner_slug, - repo_name: repo_name.to_string(), - local_path, - lock_key, - conn, - tigris: self.tigris.clone(), - }) + Ok(guard) } /// Initialize a new bare repo on local disk and upload to Tigris. @@ -411,12 +425,16 @@ fn validate_repo_name(repo_name: &str) -> Result<()> { /// Holds the owning `PoolConnection` so the lock is released on /// the same session that acquired it — see `acquire_write` for the session /// affinity rationale. +/// +/// On cancellation (drop without `release()`), the connection is closed instead +/// of returned to the pool, so the advisory lock is released by Postgres when +/// the backend session ends. pub struct RepoWriteGuard { owner_slug: String, repo_name: String, pub local_path: PathBuf, lock_key: i64, - conn: PoolConnection, + conn: Option>, tigris: Option, } @@ -446,13 +464,26 @@ impl RepoWriteGuard { warn!(repo = %self.repo_name, "write failed — skipping tigris upload to avoid propagating an inconsistent repo"); } - // Release advisory lock on the owning session. The connection is then - // returned to the pool by `PoolConnection::Drop` when `self` drops at - // the end of this function. - let _ = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(self.lock_key) - .execute(&mut *self.conn) - .await; + // Release advisory lock on the owning session, then return connection + // to the pool. Take the connection so Drop sees None and does nothing. + if let Some(mut conn) = self.conn.take() { + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(self.lock_key) + .execute(&mut *conn) + .await; + // conn drops here, returned to the pool + } + } +} + +impl Drop for RepoWriteGuard { + fn drop(&mut self) { + // If conn is still Some, release() was never called (e.g. cancellation). + // Close the connection instead of returning it to the pool, so the + // Postgres backend session ends and the advisory lock is released. + if let Some(ref mut conn) = self.conn { + conn.close_on_drop(); + } } }