diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 2ba4591f..b09cb6da 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -5683,23 +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: advisory_lock_key over owner_slug then 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 - } - 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 303fa637..2aef6ff0 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -171,7 +171,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); @@ -730,14 +757,29 @@ impl Drop for RepoWriteGuard { } /// Compute a stable i64 hash for a Postgres advisory lock key. -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 +/// +/// 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. +/// +/// 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()); + 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)] mod tests { use super::*; @@ -1145,6 +1187,48 @@ 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); + } + // ── advisory-lock cancellation-safety (#174 F1, RED-before/GREEN-after) ── /// F1 (P1): dropping a `RepoWriteGuard` WITHOUT calling `release()` — the diff --git a/docs/RUN-A-NODE.md b/docs/RUN-A-NODE.md index e5ad1d91..0a8a0f77 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` for liveness and DB-aware `/ready` for peer readiness | +| 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. | ---