fix(repo_store): use SHA-256 for stable advisory-lock key (#210)#215
fix(repo_store): use SHA-256 for stable advisory-lock key (#210)#215Gravirei wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRepository advisory locks now retain their acquiring PostgreSQL connection, explicitly unlock on a fallback error path, and derive keys deterministically with SHA-256. Tests cover stable and input-sensitive keys, while the changelog documents mixed-version rollout caveats. ChangesRepository advisory-lock handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RepoStore
participant Postgres
participant RepoWriteGuard
RepoStore->>Postgres: Acquire dedicated connection
RepoStore->>Postgres: Try advisory lock
Postgres-->>RepoStore: Return lock result
RepoStore->>RepoWriteGuard: Store connection and lock key
RepoWriteGuard->>Postgres: Unlock through retained connection
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
beardthelion
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The fix is correct and the golden test is load-bearing. I verified the pinned value is the real SHA-256 (recomputed SHA-256("did_key_...:hello")[..8] as LE i64 == -6680856138670956537, and big-endian would differ, so byte order is pinned), then ran a mutation matrix at the head: reverting to DefaultHasher, flipping the separator, flipping to from_be_bytes, and dropping owner_slug from the hash each turn the golden test RED, and it's GREEN restored. The separator is injective because owner_slug is colon-sanitized upstream (owner_did.replace([':','/'], "_")), so there's no wrong-repo aliasing, and the byte-slice path can't panic. This closes #210 the way it asked.
One thing to resolve before it lands, plus a small test note.
Findings
-
[P2] Persist the re-keying rollout caveat where operators will see it after merge
crates/gitlawb-node/src/git/repo_store.rs:407
Switching the algorithm re-keys every node. On a shared-Postgres, multi-node deployment doing a rolling upgrade, an old-binary node (DefaultHasher key) and a new-binary node (SHA-256 key) compute different keys for the same repo and stop mutually excluding for the deploy window, a transient loss of the cross-machine write-exclusion this lock exists to provide (concurrent same-repo pushes could race). That topology is real (the shared-DB compose target, plus purge-spam as a separate process on the same lock namespace). Right now the caveat lives only in the PR body, which disappears on merge. I filed #210 with "a staged rollout OR an accepted brief window" as acceptable, so I'm not asking you to change the swap itself, only to make the guidance durable: a short note onacquire_write(drain writes or cut over single-node during the upgrade) plus a CHANGELOG line. If you'd rather remove the window entirely, the mechanism is a transition release whereacquire_writetakes both the legacy and the new key, dropping the legacy one a release later, but that's optional given the accepted-window path. -
[P3] Strengthen or drop
advisory_lock_key_differs_for_different_inputs
crates/gitlawb-node/src/git/repo_store.rs:594
It varies both inputs at once (owner_a/repo_avsowner_b/repo_b), so it can't isolate a regression that drops one parameter, and I confirmed that by mutation: hashing only repo_name leaves this test green while the golden test goes red. So the golden test already backstops that case and this one adds little. If you want it to pull weight, assert a same-owner/different-repo pair and a different-owner/same-repo pair; otherwise it's fine to drop it and lean on the golden test.
Not a change request: the Cargo.lock version bumps (0.5.0 -> 0.5.1 across five crates, plus a sha2 line under icaptcha-client) aren't from this PR. The manifests are already 0.5.1 on main and gitlawb-node already declares sha2; the lockfile was just stale (that's issue #185), and regenerating it here corrected the drift. Leave them as they are; a one-line mention in the PR description is enough.
Solid, well-scoped fix. Once the rollout note is durable I'm happy to approve.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Retain the lock-owning database session for the write guard
crates/gitlawb-node/src/git/repo_store.rs:164
This change makes the key deterministic, butpg_try_advisory_lockis still executed through&PgPooland the guard later unlocks through that pool again. Those are session-scoped PostgreSQL locks: SQLx returns the acquiring connection afterfetch_one, sorelease()can be assigned another session and silently fail to unlock. Worse, a later writer can be given the idle lock-owning session and reentrantly acquire the same lock. The advertised shared-Postgres exclusion therefore remains unreliable even with the SHA-256 key. Hold aPoolConnection<Postgres>inRepoWriteGuardfrom successful acquisition through unlock (including error cleanup), and add a competing-session test. PR #196 already has this exact dedicated-connection repair, so either make this PR depend on/rebase onto it or bring that repair here before merging. -
[P2] Preserve exclusion while old and new lock keys coexist
crates/gitlawb-node/src/git/repo_store.rs:407
ReplacingDefaultHasherre-keys every repository. In a rolling shared-Postgres upgrade, an old node can hold the legacy key while a new node acquires the SHA-256 key for the same repository; PostgreSQL treats them as independent locks, allowing concurrent receive-pack/issue/pull writes and competing archive uploads. The PR body acknowledges this window, but it disappears on merge and the checked-in documentation only describes the topology. Acquire both keys for a transition release, or add durable deployment guidance requiring writes to be drained or cut over through a single node before versions are mixed.
Address review findings on Gitlawb#210: - RepoWriteGuard now stores PoolConnection<sqlx::Postgres> 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 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/git/repo_store.rs (1)
410-457: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winTask cancellation leaks session advisory locks to the connection pool.
If an async task is cancelled (e.g., due to a client timeout or disconnect) while holding the lock, the
sqlx::pool::PoolConnectionis implicitly dropped and returned to the pool. Because Postgres session-level advisory locks are not cleared automatically when a connection is returned to the pool, any subsequent request drawing this connection will inherit the lock. This can silently lock out legitimate writers or grant unintended access. This hazard applies both to downstream callers holding the guard (e.g., while awaitingreceive_packas seen in snippet 1) and withinacquire_writeitself during thetigris.download.await.
crates/gitlawb-node/src/git/repo_store.rs#L410-L457: Wrapconnin anOptionand implementDropforRepoWriteGuard. If the guard is dropped beforerelease()is explicitly called, invokeconn.detach()to discard the connection entirely, which forces the Postgres session to close and safely clears the lock.crates/gitlawb-node/src/git/repo_store.rs#L231-L266: ConstructRepoWriteGuardimmediately after lock acquisition so its newDropimplementation protects thetigris.download(...).awaitpoint against task cancellations.🔒️ Proposed fix to implement cancellation safety
For
crates/gitlawb-node/src/git/repo_store.rs#L410-L457:pub struct RepoWriteGuard { owner_slug: String, repo_name: String, pub local_path: PathBuf, lock_key: i64, - conn: PoolConnection<sqlx::Postgres>, + conn: Option<PoolConnection<sqlx::Postgres>>, tigris: Option<TigrisClient>, } +impl Drop for RepoWriteGuard { + fn drop(&mut self) { + if let Some(conn) = self.conn.take() { + // Task was cancelled before release() could run. + // Detach to discard the connection and close the Postgres session. + let _ = conn.detach(); + } + } +} + impl RepoWriteGuard { pub fn path(&self) -> &Path { &self.local_path } pub async fn release(mut self, success: bool) { if success { if let Some(ref tigris) = self.tigris { if let Err(e) = tigris .upload(&self.owner_slug, &self.repo_name, &self.local_path) .await { warn!(repo = %self.repo_name, err = %e, "failed to upload repo to tigris after write"); } } } else { warn!(repo = %self.repo_name, "write failed — skipping tigris upload to avoid propagating an inconsistent repo"); } - let _ = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(self.lock_key) - .execute(&mut *self.conn) - .await; + if let Some(mut conn) = self.conn.take() { + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(self.lock_key) + .execute(&mut *conn) + .await; + } } }For
crates/gitlawb-node/src/git/repo_store.rs#L231-L266:- // 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) { - debug!(repo = %repo_name, "write acquire: downloading latest from tigris"); - if let Err(e) = tigris.download(&owner_slug, repo_name, &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() { - 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"); - } - } - } - } - - Ok(RepoWriteGuard { + let mut guard = RepoWriteGuard { owner_slug, repo_name: repo_name.to_string(), local_path, lock_key, - conn, + 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(&guard.owner_slug, &guard.repo_name).await.unwrap_or(false) { + debug!(repo = %guard.repo_name, "write acquire: downloading latest from tigris"); + if let Err(e) = tigris.download(&guard.owner_slug, &guard.repo_name, &guard.local_path).await { + if guard.local_path.exists() { + warn!(repo = %guard.repo_name, err = %e, + "write acquire: tigris download failed — falling back to local copy"); + } else { + // Explicitly release the guard on error so the connection unlocks and returns to the pool cleanly. + guard.release(false).await; + return Err(e).context("downloading repo from tigris for write"); + } + } + } + } + + Ok(guard)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/git/repo_store.rs` around lines 410 - 457, Update crates/gitlawb-node/src/git/repo_store.rs:410-457 so RepoWriteGuard stores its PoolConnection in an Option, takes it during successful release, and implements Drop to call detach() when release() was not completed, preventing cancellation from returning a lock-bearing session to the pool. In crates/gitlawb-node/src/git/repo_store.rs:231-266, construct RepoWriteGuard immediately after advisory-lock acquisition, before awaiting tigris.download, so its Drop implementation protects that cancellation point; preserve explicit release behavior and lock cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 410-457: Update crates/gitlawb-node/src/git/repo_store.rs:410-457
so RepoWriteGuard stores its PoolConnection in an Option, takes it during
successful release, and implements Drop to call detach() when release() was not
completed, preventing cancellation from returning a lock-bearing session to the
pool. In crates/gitlawb-node/src/git/repo_store.rs:231-266, construct
RepoWriteGuard immediately after advisory-lock acquisition, before awaiting
tigris.download, so its Drop implementation protects that cancellation point;
preserve explicit release behavior and lock cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b4619c0-2d72-49f8-b859-8dc4678f0ec7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CHANGELOG.mdcrates/gitlawb-node/src/git/repo_store.rs
…to close on cancellation
Summary
Replace
DefaultHasherwith SHA-256 for the Postgres advisory-lock key so the same(owner_slug, repo_name)produces the samei64across all Rust toolchain versions, OSes, and machines — fixing silent cross-machine write exclusion breakage.Motivation & context
Closes #210
Kind of change
What changed
advisory_lock_key()inrepo_store.rsnow usessha2::Sha256instead ofstd::collections::hash_map::DefaultHasher(not guaranteed stable across Rust releases).acquire_writedoc comment scoped to clarify cross-machine guarantee holds only on shared Postgres.How a reviewer can verify
cargo test -p gitlawb-node -- git::repo_store::tests::advisory_lock_key_is_stableBefore you request review
cargo test --workspacepasses locally (relevant tests pass)cargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfix(...))Protocol & signing impact
N/A — internal implementation detail, no protocol change.
Notes for reviewers
Changing the algorithm re-keys every node. During a mixed-version upgrade window, an in-flight lock held under the old key will not exclude a new-key caller, so this needs a staged rollout (or an accepted brief window), not a hot swap.
Summary by CodeRabbit