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 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 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
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] Keep advisory-lock sessions out of the shared application pool
crates/gitlawb-node/src/git/repo_store.rs:207
acquire_writenow checks out a connection from the sameDbpool used by every handler and holds it for the full retry loop, Tigris I/O, and write operation. With the default 20-connection pool, one lock holder plus 19 requests contending for that repository consume every connection while the contenders sleep and retry for up to 60 seconds; distinct slow writes can do the same for up to the 600-second git-service timeout. Unrelated database-backed requests then fail their five-second acquire timeout. Use a separate bounded pool for advisory-lock sessions, or otherwise bound lock waiters/write guards while reserving capacity for the application pool. -
[P1] Do not return a session to the pool until its advisory unlock is known to have completed
crates/gitlawb-node/src/git/repo_store.rs:469
releaseremoves the lock-owning connection fromself.connbefore awaitingpg_advisory_unlock. If that future is cancelled, the guard'sDropseesNone, so SQLx returns the session to the pool even though PostgreSQL may still hold its session-scoped advisory lock. A later borrower of that session can reentrantly acquire the lock while other sessions remain blocked, defeating the write exclusion. The Tigris-download error cleanup has the same unsafe outcome when its ignored unlock query fails. Keep the connection in the guard through a successful unlock, and close it rather than pool it on cancellation or unlock failure; add a regression test that aborts during unlock.
Superseded: re-reviewed the current head 26c4dfe.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 26c4dfe8. Both points from my last round landed properly. The rolling-upgrade caveat is now in the acquire_write docstring, and the differs-test rewrite pulls real weight: I ran the mutation matrix and dropping either parameter from the hash turns it RED (key must depend on repo_name / key must depend on owner_slug), where the old both-vary-at-once shape stayed green. The golden constant still catches an algorithm swap. The SHA-256 change itself remains correct.
Holding the lock-owning connection in the guard is the right move and the new Drop works: I confirmed that dropping a guard without calling release() frees the lock to an independent session, because close_on_drop really does end the session in sqlx 0.8.6. What is not right yet is everything that happens once the connection leaves the guard, plus a collision with #79 that will bite whichever of the two merges second.
Findings
-
[P1] Settle one lock-key derivation with #79 before either merges
crates/gitlawb-node/src/git/repo_store.rs:496
#79 also claims to fix #210, and it hashesformat!("{owner}/{repo}")folded big-endian while this PR hashesowner+":"+repofolded little-endian. Same repo, different key. Whichever lands second re-keys every repository a second time, breaks the other's golden constant, and makes operators pay the exclusion-gap window twice; #79's legacy fallback is main'sDefaultHasher, so its dual-key transition would not cover a fleet already running this key either. My call as lead: this PR owns the lock layer, since it is the focused change that closes #210 and it got here first. I am asking #79 to drop its lock work and rebase onto this, so nothing here needs to change except knowing the decision is settled. -
[P1] Stop pinning a pool connection for the whole 60-second acquire wait
crates/gitlawb-node/src/git/repo_store.rs:207
The connection is now checked out before the retry loop, so a writer that waits out a contended repo holds one of the pool's 20 connections (config.rs:192) for up to 60 seconds without doing any work. On the base commit the loop usedfetch_one(&self.pool)and returned its connection between probes, so this is a regression the fix introduces rather than an existing gap. What makes it more than a tuning question is who can reach it:create_issuesits behindauthorize_repo_readand nothing stronger (api/issues.rs:46) before it callsacquire_writeat:66, so any authenticated identity with read access to a repo can occupy a connection for the length of a write. Only the winning attempt needs session affinity, so acquire inside the loop and drop before each sleep; I compiled that change. The other half, every in-flight write pinning a connection for its whole duration, is what jatmn's dedicated-pool ask is really about, and that one still stands. -
[P1] Keep the connection inside the guard across the unlock await, and check the unlock result
crates/gitlawb-node/src/git/repo_store.rs:469
self.conn.take()moves the connection out before the await, which disarms theDropyou just added: from that instant the lock is held by a connection nothing will close. Cancellation there, or an unlock that does not actually unlock, returns that connection to the pool with the lock still held. This is worse than a one-off leak because advisory locks are counted per session. I checked it against Postgres directly: twopg_try_advisory_lockcalls on one session both return true, and after a singlepg_advisory_unlockthe session still holds the lock. So the next writer for that repo, handed the same pooled connection, re-acquires reentrantly and its ownrelease()only decrements the count back to one. The lock then never clears until sqlx recycles the connection, which on the 0.8.6 defaults is 10 minutes idle or 30 minutes lifetime, and every writer on every node fails the 60-second retry in the meantime. Note also thatpg_advisory_unlockreports "you did not hold this" as afalsereturn with a WARNING, not an error, solet _ = ... .execute(...)cannot see it at either this site or:267. Your own error path at:266already uses the correct borrow shape, so the two paths just need to agree. Unlocking throughif let Some(ref mut conn) = self.conn, taking the connection only once the unlock is confirmed, and otherwise warning and leaving it forDropto close, compiles and keeps the existing 20 repo_store tests green. -
[P2] Move the operator caveat out of CHANGELOG.md
CHANGELOG.md:3
My last round asked for a CHANGELOG line and that was the wrong placement, so this one is on me. That file is generated by release-please (release-please-config.json, and every commit touching it is achore(main): release), and main is already at 0.7.0, so a hand-written## Unreleasedsection will sit above the next generated release forever. Keep the docstring, which is the durable half I actually wanted, and put the operator guidance in the operational checklist indocs/RUN-A-NODE.md, next to where the signed-peer rolling-upgrade posture is already documented. -
[P3] Drop the Cargo.lock hunk when you rebase
Cargo.lock:3303
It was legitimate drift correction when you wrote it, but main now carries both halves already: the crates are at 0.5.1 there andicaptcha-clientalready listssha2. After a rebase this hunk is empty churn. -
[P2] Close the same gap in the acquire loop
crates/gitlawb-node/src/git/repo_store.rs:216
The probe query is the one await where the connection is a bare local with no guard around it. If the future is dropped while that query is in flight, the request may still have reached Postgres and taken the lock, and the connection then returns to the pool holding it, with the same counted-lock consequence as above. If you take the acquire-inside-the-loop change this shrinks to the single winning query, and wrapping that connection so itsDropcloses the session until the guard takes ownership closes it entirely.
Also still open from jatmn's last round: the two regression tests, a competing-session case proving a second Postgres session is excluded while the guard is held, and one that exercises cancellation during the unlock. The second one is worth writing RED against the current take() shape so it proves the fix rather than tracking it.
… conn on unlock cancellation
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I rechecked this at 30e0dec. The dedicated lock_pool, acquire-inside-the-retry-loop change, and keeping the lock-owning connection across the unlock .await all landed correctly since my last round. The SHA-256 key and golden test still look right. What remains is the unlock/return-to-pool contract, lock-pool sizing, and the regression tests reviewers asked for.
Findings
- [P1] Verify
pg_advisory_unlockactually released the lock before returning the session tolock_pool
crates/gitlawb-node/src/git/repo_store.rs:506
release()still treats a successfulexecute()as proof the session is clean and callsself.conn.take(), returning the connection tolock_pool. PostgreSQL reports “you did not hold this lock” as afalsereturn value, not a query error, soexecute()cannot distinguish that case. Worse, session-level advisory locks are counted: if this session already held the key (for example because a prior holder returned a lock-bearing connection to the pool and this writer re-acquired reentrantly), a singlepg_advisory_unlockcan returntruewhile the session still holds the lock. The next borrower of that pooled connection inherits it, and exclusion can stay broken until sqlx recycles the session. beardthelion called this out on the prior head; it is still present on both the happy path here and the Tigris error cleanup at:288. Please unlock throughquery_asinto(bool,), treatfalselike a failed unlock (leave the connection forDrop/close_on_drop), and add a regression test that aborts during unlock so this cannot regress again. - [P1] Hard-coding
lock_poolto two connections caps the whole node at two in-flight writes
crates/gitlawb-node/src/main.rs:283
EachRepoWriteGuardnow pins alock_poolconnection for the full write path (Tigris download, git receive-pack/merge/issue work, Tigris upload, unlock) — up togit_service_timeout_secs(600s by default). Withmax_connections(2), a third concurrentacquire_write()anywhere on the node blocks onlock_pool.acquire()and, after the 5sdb_acquire_timeout_secs, fails withacquiring connection for advisory lockinstead of entering the 60×1s advisory-lock retry loop. That is a new node-wide write denial under modest concurrency, including cross-repo traffic (repos.rs,pulls.rs,issues.rsall callacquire_write). Worse, each iteration of the contended-repo retry loop callslock_pool.acquire().await?(:226), so a same-repo waiter that is merely waiting on an advisory lock can abort after ~5s when two unrelated writes occupy both slots — the"could not acquire advisory lock after 60s"path is unreachable under pool saturation. Please size the pool for expected concurrent writers (config/env), retry pool-acquire failures inside the lock loop, or stop holding a pooled connection for the entire git/Tigris phase once the lock is acquired. - [P2] The competing-session and cancellation regression tests still do not protect CI
crates/gitlawb-node/src/git/repo_store.rs:757
beardthelion and I both asked for load-bearing Postgres tests here. The newcompeting_session_is_excluded_while_guard_heldandcancelled_guard_releases_lock_when_droppedcases are marked#[ignore]andmake_store_for_live()wireslock_poolwithconnect_lazy("postgres://invalid")while the#[sqlx::test]fixture provisionspoolagainst the real test database, so even a manual run cannot pass. The ignore reason (requires DATABASE_URL) is also misleading — CI already provisions Postgres inpr-checks.yml. Even after fixing the fixture,cancelled_guard_releases_lock_when_droppedprobes lock availability immediately afterhandle.awaitwhileDroponly callsclose_on_drop(), which spawns an async session teardown in sqlx 0.8.6, so the test can pass or fail depending on scheduler timing rather than proving the lock is gone. Please pointlock_poolat the same database as the testpool, drop the#[ignore], wait for the close future (or use synchronous teardown) before probing, and keep the cases in the defaultcargo test -p gitlawb-nodepath. - [P2] A cancelled winning lock probe can still poison
lock_pool
crates/gitlawb-node/src/git/repo_store.rs:232
Failed probes drop their connection without holding a lock, but the one successfulpg_try_advisory_lockruns on a barePoolConnectionuntilRepoWriteGuardis constructed immediately afterward. If the task is cancelled during thatfetch_one().awaitafter Postgres took the lock, the connection is returned tolock_poolwithoutclose_on_drop, recreating the counted-lock/reentrancy failure mode the newDropis meant to prevent. Please wrap the winning probe connection in the same “close on drop unless explicitly handed to the guard” RAII used for cancellation safety. - [P3]
lock_poolstartup uses a singleconnect()with no retry
crates/gitlawb-node/src/main.rs:287
The main application pool goes throughconnect_db_with_retry, but the advisory-lock pool is created with oneconnect().await?immediately afterward. A transient connection failure or briefmax_connectionspressure after the main pool comes up aborts node startup even though migrations already succeeded. Please reuse the same retry/backoff pattern (or share the established pool with a dedicated advisory-lock semaphore) so startup is not flakier than the rest of the DB bootstrap. - [P3] Operator connection sizing ignores the two extra
lock_poolsessions
crates/gitlawb-node/src/main.rs:282
Each node now opens a second pool to the samedatabase_urlwithmax_connections(2)on top ofGITLAWB_DB_MAX_CONNECTIONS(default 20, documented in.env.example). Fleet sizing that budgets only the main pool can hit Postgresmax_connectionsearlier than expected and contribute to the acquire failures above. Please document the extra headroom inRUN-A-NODE.md/.env.example, or make the lock-pool size configurable alongsidedb_max_connections.
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