From 89e10d0ff12dcccae3bd4490548d906f77d76636 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Fri, 17 Jul 2026 22:03:18 +0600 Subject: [PATCH 1/4] fix(repo_store): use SHA-256 for stable advisory-lock key (#210) DefaultHasher is not frozen by the Rust standard and has shifted across toolchain versions, so the same (owner_slug, repo_name) produced a different i64 advisory-lock key on different builds and broke cross-machine write-exclusion for the same repo. Hash with SHA-256 instead and pin the result with a golden-value test, plus a one-axis-at-a-time differential test so a regression that drops either parameter is caught. Document the SHA-256 re-keying caveat on acquire_write and in the RUN-A-NODE.md operator checklist (drain writes or cut over single-node during a rolling upgrade). --- crates/gitlawb-node/src/git/repo_store.rs | 88 +++++++++++++++++++++-- docs/RUN-A-NODE.md | 1 + 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 65058016..9e6a278a 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -152,7 +152,34 @@ 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). + /// + /// # 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); @@ -518,12 +545,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)] @@ -932,4 +966,46 @@ 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() { + // 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); + } } diff --git a/docs/RUN-A-NODE.md b/docs/RUN-A-NODE.md index 5ec5357e..c2dd9192 100644 --- a/docs/RUN-A-NODE.md +++ b/docs/RUN-A-NODE.md @@ -178,6 +178,7 @@ GITLAWB_ENFORCE_OWNER_PUSH=true | Operator key | Dedicated wallet, small ETH balance, not your main treasury | | Monitoring | Watch `lastHeartbeat` on-chain; alert if > 22h since last beat | | Public URL | Must resolve and serve `/health` — peers will ping it | +| Rolling upgrade (advisory-lock algorithm) | The SHA-256 advisory-lock key swap (replacing `DefaultHasher`) re-keys every repo in the first release that carries it. During a shared-Postgres rolling upgrade, old nodes compute a different `i64` key than new nodes for the same repo, so PostgreSQL treats them as independent locks and cross-machine write-exclusion is lost. **Drain in-flight writes** or cut over through a **single node** before bringing new-binary nodes online. | --- From c68e1b07f8db752cff786bdc9c226b49889a40b1 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Mon, 10 Aug 2026 22:09:10 +0600 Subject: [PATCH 2/4] fix(test): match SHA-256 advisory-lock key in receive-pack deadline test The test reproduces acquire_write's advisory-lock key derivation locally so a held session lock can stall the handler. repo_store's advisory_lock_key now uses SHA-256, so the DefaultHasher copy no longer collided and the stall never happened. Update the local copy to the SHA-256 derivation. --- crates/gitlawb-node/src/api/repos.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 2ba4591f..937d543a 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -5691,13 +5691,15 @@ mod tests { // Reproduce acquire_write's session-level advisory-lock key exactly so the // second-connection lock collides with the handler's pg_try_advisory_lock - // (repo_store.rs: advisory_lock_key over owner_slug then repo_name). + // (repo_store.rs: SHA-256 advisory_lock_key over owner_slug, ':', repo_name). 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")) } let owner = "z6acqdead"; From aa68d97a8d18f450141755e072403389d93c881b Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 11 Aug 2026 17:59:33 +0600 Subject: [PATCH 3/4] refactor(repo_store): expose advisory_lock_key pub(crate) and drop test copy The deadline test hand-copied the advisory-lock key derivation, and that copy drifted once the key moved to SHA-256, silently breaking the lock collision the test is built around. Import the production function instead so the key cannot diverge again. --- crates/gitlawb-node/src/api/repos.rs | 14 +------------- crates/gitlawb-node/src/git/repo_store.rs | 3 +-- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 937d543a..b09cb6da 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -5683,25 +5683,13 @@ mod tests { /// stays pinned past the deadline. Restore to return GREEN. #[sqlx::test] async fn receive_pack_acquire_deadline_sheds_and_releases_permit(pool: sqlx::PgPool) { + use crate::git::repo_store::advisory_lock_key; use axum::extract::{Path, State}; use axum::Extension; use std::net::SocketAddr; use std::sync::Arc; use tokio::sync::Semaphore; - // Reproduce acquire_write's session-level advisory-lock key exactly so the - // second-connection lock collides with the handler's pg_try_advisory_lock - // (repo_store.rs: SHA-256 advisory_lock_key over owner_slug, ':', repo_name). - fn advisory_lock_key(owner_slug: &str, repo_name: &str) -> 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")) - } - let owner = "z6acqdead"; let name = "acq1"; // owner_slug as local_path() computes it from the record's owner_did. The diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 556e4470..c6e92b96 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -762,7 +762,7 @@ impl Drop for RepoWriteGuard { /// 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 { +pub(crate) fn advisory_lock_key(owner_slug: &str, repo_name: &str) -> i64 { use sha2::Digest; let mut hasher = sha2::Sha256::new(); hasher.update(owner_slug.as_bytes()); @@ -771,7 +771,6 @@ fn advisory_lock_key(owner_slug: &str, repo_name: &str) -> i64 { let digest = hasher.finalize(); i64::from_le_bytes(digest[..8].try_into().expect("sha256 output is >= 8 bytes")) } - #[cfg(test)] mod tests { use super::*; From b01d17b7f36fc66f4a2cdbb7957105cb1c6e5031 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Wed, 12 Aug 2026 13:07:50 +0600 Subject: [PATCH 4/4] docs(repo_store): pin colon-free owner_slug contract on advisory_lock_key The domain separation owner_slug + ':' + repo_name has no length prefix, so it is injective only while owner_slug cannot contain a colon. State that on the pub(crate) function and guard it with a debug_assert, since a raw DID would collide (('did:key:abc','x') and ('did','key:abc:x') hash the same). --- crates/gitlawb-node/src/git/repo_store.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index c6e92b96..2aef6ff0 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -762,7 +762,16 @@ impl Drop for RepoWriteGuard { /// 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. +/// +/// Domain separation is `owner_slug + ":" + repo_name` with no length prefix, +/// so the mapping is injective only while `owner_slug` contains no `:` (the +/// `did:key:`→`did_key_` slug form `local_path` produces). A raw DID would +/// collide: `("did:key:abc", "x")` and `("did", "key:abc:x")` hash the same. pub(crate) fn advisory_lock_key(owner_slug: &str, repo_name: &str) -> i64 { + debug_assert!( + !owner_slug.contains(':'), + "advisory_lock_key owner_slug must not contain ':' (domain-separation guarantee)" + ); use sha2::Digest; let mut hasher = sha2::Sha256::new(); hasher.update(owner_slug.as_bytes());