fix(node): release the advisory lock on the session that took it (#279) - #285
Open
beardthelion wants to merge 12 commits into
Open
fix(node): release the advisory lock on the session that took it (#279)#285beardthelion wants to merge 12 commits into
beardthelion wants to merge 12 commits into
Conversation
added 12 commits
July 29, 2026 17:04
…id-acquire A cancelled .await does not cancel an already-sent SQL statement, so a pg_try_advisory_lock whose future is dropped still takes the lock server-side while the caller abandons the result, leaving nothing to release it. The connection then returns to the pool holding the lock and wedges that repo until sqlx recycles the session. Introduce LockProbe, which owns the connection across the in-flight try-lock and closes it in its own Drop if it is still held. close_on_drop is a one-way setter, so the arming lives in Drop rather than being set up front and cleared on success; disarming is Option::take, which is what into_conn does once an acquire is actually observed. This is now the only place that issues pg_try_advisory_lock. The committed gate drops a probe without taking its connection, which is the state a cancellation leaves behind, and polls a standalone observer until the lock frees. Deterministic on purpose: the timing sweep that found this window leaks roughly 1 in 600, which is not something a CI gate can rest on. Observed RED before this change with the lock still held for the full 10s window. Refs #279
Pinning a connection for the lock's lifetime is only safe if those connections come from somewhere other than the pool serving ordinary request handlers, otherwise a push burst starves every other query. Add GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS (default 32) and a Db::lock_pool builder, with the sizing tradeoff documented on the field and in .env.example: every in-flight write pins one connection here, so the value is a hard ceiling on simultaneous writes node-wide. The pool connects lazily on purpose. The main pool must connect eagerly because it runs migrations, which is why it needs connect_db_with_retry's backoff and degraded-server handoff; that function is not a generic retry helper and the lock pool is built well after the db-ready handoff has already resolved. A lazy pool has no startup work, so it adds no new way for the process to fail to boot and needs no second copy of that machinery. If Postgres is unreachable when the first write arrives, that write fails on the pool's own acquire timeout, like any other database-backed request. Pure configuration, so no proof-first cycle: the knob is covered by a parse/default/reject-zero test. Db::lock_pool has no caller until the guard wiring lands, hence the temporary dead_code attribute. Refs #279
Postgres advisory locks are session-scoped: only the backend that took one can release it. acquire_write took the lock through fetch_one(&pool) and release unlocked through execute(&pool), two independent checkouts, so the unlock usually landed on a session that held nothing and returned false. Measured on main: two writers on one node and the same repo BOTH acquired, 50 of 50 sequential cycles leaked, and 100 writes left 100 orphaned advisory locks on the server. The guard now owns the PoolConnection that took the lock, drawn from the dedicated lock pool, and releases on that same session. The retry loop probes through LockProbe so a cancellation mid-acquire cannot strand the lock, and hands the connection back before each backoff so a spinner on a contended repo does not pin a slot while idle. Pool exhaustion is deliberately not retried. It is a different condition from lock contention, and retrying it would spend all 60 attempts on a capacity problem unrelated to this repo while reporting it as someone else holding the lock. Both #279 acceptance tests were observed RED first: the exclusion test admitted the second writer, and the leak test reported 1 lock held where 0 was required. Both GREEN after. Full crate suite 516 passed. Db::pool() is removed because this change was its only caller. Refs #279
…easing A guard can exit without reaching release(): an early ? on the pre-write download, a panic, or an axum handler future cancelled when the client disconnects. Its session still holds the lock, so returning that connection to the pool would block every future write to the repo until sqlx recycles it, which on the 0.8.6 defaults is ten minutes idle or thirty minutes lifetime. Close the session instead and let Postgres free the lock at session end. One hazard found by writing the teardown test rather than by reasoning: PoolConnection::drop spawns onto the runtime for both closing and returning, and panics outright when no runtime handle exists. That panic would fire inside a Drop and abort the process during unwind. It is not introduced by this commit, it comes with owning a PoolConnection at all, but this is where it becomes reachable. So Drop checks for a runtime first and, with none, leaks the handle deliberately rather than panicking: the process is already exiting and socket teardown ends the session, which is what frees the lock at exit anyway. Observed RED before the fix, with the lock still held for the full 10s poll window against a standalone observer on a no-reap pool, and the teardown case panicking in sqlx-core connection.rs:208. Proven load-bearing after: neutering the close_on_drop call turns the drop test RED again. The must-not case (a released guard reuses its backend pid across four writes on a pool sized 1) passes in both states, so the signal is specific to the abandoned-guard path. Full crate suite 519 passed. Refs #279
pg_advisory_unlock reports "you did not hold this lock" as a false RETURN VALUE plus a server WARNING, never an error, so let _ = execute(...) could not distinguish a real release from a no-op. Three blindnesses stacked in those four lines: execute discards the row, the boolean lives in the row, and let _ discarded the Result too. Read it through fetch_one into (bool,). A false means this session's lock state is not what we believe it is, so the connection stays in the guard for Drop to close rather than being handed back to the pool as clean. Only a confirmed unlock returns it. A query error gets the same treatment, since the lock must not outlive a session we can no longer reason about. Note this is the only unlock site: the pre-write download's error path returns through the guard, so Drop covers it and there is no second place to keep in sync. RED before: the connection came back on the same backend pid after an unlock that returned false. GREEN after, and proven load-bearing by treating false as success, which turns it RED again. The must-not case (a normal release still reuses its backend) passes in both states, so this does not over-close the happy path. Full crate suite 520 passed. Refs #279
…e lock Two transfers happen while the per-repo advisory lock is held: the archive download inside acquire_write, which runs after the lock is taken and before the guard exists, and the upload inside release. Both were free before the guard pinned a lock-pool connection, because the lock's connection went back to the pool immediately. Now an unbounded stall holds a lock-pool slot for its whole duration, so enough concurrent stalls deny every write on the node with no reaping path. Add GITLAWB_LOCK_HELD_TRANSFER_TIMEOUT_SECS (default 300) and route both through it. A timed-out upload is UNKNOWABLE rather than failed, so the timeout arm takes no compensating action: the PUT may well have landed. The lock is released either way, which trades a narrow last-writer-wins window for not wedging the repo behind a stalled transfer. Deliberate, and stated here rather than discovered later. Note what is NOT bounded by this knob: acquire_fresh's download. It runs before any lock is taken, so it holds nothing. An earlier revision of this change put the bound there by mistake, because both functions contain an identical download call and the first match won. Two disclosures. Coverage: the committed tests cover the bound mechanism, not the wiring. Driving a genuinely stalled transfer through acquire_write needs either the object-store abstraction (out of scope) or a process-global AWS_ENDPOINT_URL_S3 mutation, which would make the suite order-dependent under the concurrent runner. That a stalled transfer is bounded is therefore verified by reading, not by execution. Behavior: on timeout with a local copy present, the download falls into the pre-existing self-healing fallback and the write proceeds against that local copy. This widens the conditions reaching that path from corrupt-or-unreachable to include merely-slow, so a stale tree could now be written and re-uploaded on a slow link. Kept consistent with the existing failed-download behavior rather than inventing new semantics here; changing it is its own decision. Full crate suite 522 passed. Refs #279
close_issue acquired the per-repo advisory lock, then ran the owner-or-author check, then returned 403 with the lock still held. That was harmless only because the lock excluded nothing. Making it work, as the preceding commits do, turns this ordering into a denial primitive: any caller with repo read access could take the write lock on demand and be refused the write, while a legitimate writer burned its 60-attempt retry budget against a lock held by someone with no write authorization. On a public repo that is every permissionless identity. This series creates the exposure, so it closes it in the same series. The owner check is cheap and moves above the lock outright. The author fallback needs the issue's git-JSON blob, since there is no author column, so it now reads through acquire() rather than acquire_write(): an issue's author is set at creation and never changes, so reading it outside the lock races nothing, and only the mutation needs exclusion. Two deliberate behavior choices. A non-owner whose authorship cannot be established gets 403 rather than 404, so this route does not tell an unauthorized caller whether an issue exists. And the owner's existing 404-for-a-missing-issue path is preserved by re-reading under the guard, which the mutation wants anyway. Swept the other three acquire_write sites: merge_pr owner-gates at :200 before acquiring at :214, the repo write path checks did_matches at :916 before :933, and create_issue's read gate is legitimate because that caller IS authorized for the action it performs. close_issue was the only one with the wrong order. RED before: with an independent session holding the repo's lock, a stranger's request sat in the retry loop until the 3s deadline fired (Elapsed), which is the wedge itself. GREEN after, refused immediately. Proven load-bearing: disabling the pre-lock refusal restores the Elapsed. Full crate suite 523 passed. Refs #279
Seven fixes from a seven-reviewer pass. Eight of the nine substantive
findings were defects introduced by this branch, and two contradicted
claims made in its own earlier commit messages.
P0, the worst of them. The under-lock download's timeout was folded into
the same Err as a corrupt archive, so it fell through to the
use-the-local-copy fallback and the write proceeded. Two ways that
destroys data: we do not know whether we hold the latest tree, so
re-uploading can overwrite another node's newer archive; and the abandoned
download's extraction runs in an uncancellable spawn_blocking that ends in
remove_dir_all + rename over the same path, so git would have been running
against a directory a background task was about to delete. Now the
Option is branched rather than collapsed: a genuine fetch error keeps the
local-copy fallback, a timeout refuses the acquire and lets the guard's
Drop free the lock.
LockProbe closed its connection on ordinary contention, so a 60-attempt
spinner tore down 60 backends -- the exact behavior an earlier commit
claimed it avoided. The test written to guarantee that asserted the
HOLDER's lock count, which cannot observe the probe's own connection, so it
passed throughout. The correct predicate turned out to be narrower than the
first attempt at this fix: not "we saw an answer" but "we positively know
nothing was acquired," because a true answer means the lock IS held and
dropping without handoff leaks it exactly as a cancellation would. The
first attempt reopened U1's leak and U1's gate test caught it.
tigris.exists() ran unbounded inside the lock-held span, so the knob's
promise was not kept and the docs were wrong about the shape. The HEAD and
the download now share one budget, so worst-case occupancy is one budget
per span rather than two.
close_issue's pre-lock author read used acquire(), whose fast path returns
on a cached directory and never contacts object storage, so on a multi-node
deploy an issue author would be refused their own issue. Now acquire_fresh,
which refreshes without taking the lock -- the property the comment
already claimed. That comment also asserted authorship is immutable, which
a reviewer disproved by force-pushing a forged blob at refs/gitlawb/**;
reworded to the real justification (a pre-check whose mutation re-reads
under the guard). The underlying pushability is pre-existing and larger
than this branch.
The retry loop had no wall-clock cap, reaching ~360s against a 120s proxy
idle timeout that was itself lowered from 600s after held connection slots
caused a production outage. Capped at 90s, with the bail message naming the
real bound.
All four acquire_write call sites stringified their error, destroying the
anyhow downcast that error.rs documents explicitly ("without this, every
database outage surfaces as a 500 instead of a 503"). PoolTimedOut is in
that 503 set, so a saturated lock pool told clients not to retry something
transient. Now propagated.
Tests: six that the plan required and the first pass never wrote. The R4
pool-isolation test (named in the plan as proving the pool split, entirely
absent), close_issue's owner and non-owner-author twins (INV-21c, a
two-principal gate with only its deny arm covered), a waiter-does-not-block-
an-unrelated-repo case, and a config test for the transfer knob. The
runtime-teardown test no longer returns green when DATABASE_URL is unset.
Drop's no-runtime branch now detaches via leak() instead of mem::forget:
PgConnection has no Drop impl, so the socket closes synchronously and the
lock frees immediately rather than at process exit.
Evidence. The probe predicate is proven load-bearing in BOTH directions:
forcing always-close reddens the churn regression, forcing never-close
reddens the cancellation gate, and only the correct predicate satisfies
both. The author twin reddens when the fallback is removed.
One honest limit: the author twin does NOT redden when acquire_fresh is
reverted to acquire. With tigris disabled in tests the two calls are
identical, so that fix is correct by reading, not by execution -- the same
seam that leaves the transfer bound's wiring untested.
Full crate suite 529 passed, clippy -D warnings clean.
Refs #279
…iness Completes the shed-in-path half of the availability story. F7 already made a pool timeout a retryable 503 by letting the sqlx downcast through; this adds the operator-facing half, with the pool's own size/idle counters so an incident can distinguish "the pool is full" from "the database is gone" without reproducing it. Deliberately NOT a readiness probe. /ready gates Fly routing with no fail-open, so failing it on a saturated pool would pull this node's READS out of service too and push its write load onto peers carrying the same load. That is the downward spiral AWS's health-check guidance names and the SRE Book documents independently. A lock-pool readiness probe would also add nothing on the reachability axis, because both pools are built from the same database_url, so the existing app-pool ping already answers it. The error is still returned via .context() rather than replaced, because anyhow preserves downcastability through context layers and the 503 mapping depends on it. Cost is bounded by construction: one line per failed acquire, and a failed acquire has already paid a multi-second pool timeout. Refs #279
… contention as 503
Four defects a seven-reviewer pass found in the previous two commits, three of
them cases where one arm of a match contradicted its neighbour.
The under-lock refresh read a failed Tigris HEAD as "no archive"
(`exists().await.unwrap_or(false)`), skipped the refresh, and wrote against a
possibly-stale tree, then re-uploaded over it. That is the outcome the timeout
arm twenty lines below explicitly refuses. A failed HEAD and a failed download
leave us knowing different things, so they no longer share a branch: only a
download failure after a successful HEAD establishes that the local copy is a
sound thing to fall back to and re-upload. A HEAD failure now refuses the write.
Lock contention that ran out the acquire deadline surfaced as a 500 carrying the
owner slug and repo name in the client-visible body, while pool exhaustion
fifteen lines up returned a deliberate 503. Contention is transient and ordinary,
so it now maps through a typed `RepoBusy` to a retryable 503 with a fixed body;
the detail stays in the log at the raise site.
`lock_not_taken` was assigned after the try-lock answered, so an error left a
previous `true`-derived value standing and could return a lock-holding session to
the pool. It is now cleared before the statement is sent.
`leak()`'s no-panic property in the guard's no-runtime Drop branch depended on
`min_connections == 0` without saying so; a future tuning change would have
silently re-armed a panic inside Drop. Made explicit with the reason.
Two tests that did not bind:
- `waiter_on_one_repo_does_not_block_another` proved only that two lock keys do
not collide. It now samples the pool counters across more than two backoff
cycles. RED at 0/50 samples with `drop(probe)` moved after the sleep, the
exact defect it names, which the previous version survived.
- the exhaustion test's `Ok(Err(_)) | Err(_)` was satisfied by its own outer
timeout. It now requires `PoolTimedOut` and asserts the slots are accounted
for. RED with the pool sized N+1.
`contended_acquire_sheds_as_repo_busy_not_internal_error` is new and drives the
deadline path for real; the deadline became a field so it does not wait 90s. RED
at 500-vs-503 with the downcast removed.
The acquire deadline's docstring claimed a total under the 120s proxy idle
timeout, which the 300s under-lock transfer bound in the same function
contradicts. It bounds the wait only, and now says so. The backoff is clamped to
the remaining budget.
…e-checking existence The guarded re-read matched `Ok(Some(_))` and discarded the blob, so the authorization decision rested entirely on the pre-lock read. A comment above claimed the opposite. That is not a narrow window: `acquire_write` re-downloads the archive after locking, so the tree that gets mutated is routinely not the one the author was read from. With owner-push enforcement defaulting to false and branch protection covering only `refs/heads/*`, `refs/gitlawb/issues/*` is pushable, so a forged author blob landing between the two reads was honored. The blob is already in hand under the guard, so re-asserting owner-or-author costs a deserialize. A non-owner whose issue has vanished gets 403 rather than 404, matching the pre-check's existing refusal to reveal existence. `owner_can_still_close_after_the_reorder` seeded the owner as their own issue's author, which made it unable to fail: with the owner check disabled the author fallback granted the close and the test stayed green. It now seeds a third party, so only the owner arm can grant. RED with `is_owner = false`. Also drops the claim that the author twin covers the acquire-vs-acquire_fresh distinction. `RepoStore::for_testing` hardcodes `tigris: None`, so the two calls are identical in every test here and reverting that line leaves the twin green. Separating them needs an object-storage seam. Stating the gap beats asserting coverage that does not exist.
Whitespace only; cargo fmt --check gates the push.
Contributor
📝 WalkthroughWalkthroughThe PR introduces a dedicated advisory-lock pool, cancellation-safe session-pinned repository locks, bounded storage transfers, retryable repository-busy responses, and pre-lock authorization checks for issue closure. ChangesRepository write controls
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHandler
participant RepoStore
participant LockPool
participant Tigris
Client->>APIHandler: Submit repository write
APIHandler->>RepoStore: Acquire write guard
RepoStore->>LockPool: Probe advisory lock
LockPool-->>RepoStore: Pinned lock session
RepoStore->>Tigris: Refresh repository with bounded timeout
RepoStore-->>APIHandler: Return write guard
APIHandler->>RepoStore: Release successful write
RepoStore->>Tigris: Upload with bounded timeout
RepoStore->>LockPool: Unlock pinned session
APIHandler-->>Client: Write response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/gitlawb-node/src/git/repo_store.rs (2)
1363-1372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed 300ms sleep with a poll loop.
PoolConnection::dropspawns the close, so on a loaded CI runner the close may not have completed when the nextacquire()runs — the pool then hands back the same still-open connection and theassert_ne!fails spuriously. Polling until the pid changes (or a generous deadline elapses) makes this deterministic, matching the rationale already used inpoll_until_free.♻️ Poll instead of sleeping a fixed interval
- // Give the spawned close a moment, then see which backend we land on. - tokio::time::sleep(std::time::Duration::from_millis(300)).await; - let pid_after = { - let mut c = lock_pool.acquire().await.unwrap(); - let pid: (i32,) = sqlx::query_as("SELECT pg_backend_pid()") - .fetch_one(&mut *c) - .await - .unwrap(); - pid.0 - }; + // The close is spawned, so poll rather than sleeping a fixed interval. + let started = std::time::Instant::now(); + let mut pid_after = pid_before; + while started.elapsed() < std::time::Duration::from_secs(10) { + let mut c = lock_pool.acquire().await.unwrap(); + let pid: (i32,) = sqlx::query_as("SELECT pg_backend_pid()") + .fetch_one(&mut *c) + .await + .unwrap(); + pid_after = pid.0; + if pid_after != pid_before { + break; + } + drop(c); + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + }🤖 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 1363 - 1372, Replace the fixed 300ms sleep before querying pid_after with a poll loop that repeatedly acquires a connection and checks pg_backend_pid() until it differs from the original pid, or a generous deadline is reached. Reuse the existing poll_until_free approach and preserve the final pid comparison while preventing transient failures on slow runners.
250-258: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider jitter on the retry sleep.
The backoff is a flat 1s with no randomization, so multiple waiters on the same repo tend to synchronize their probes and
pg_try_advisory_lockgives no fairness ordering — a waiter can be starved for the whole 90s deadline while later arrivals win. A small random offset (or a short exponential ramp) spreads the probes.🤖 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 250 - 258, Randomize the retry delay in the probe loop around the existing tokio::time::sleep call so concurrent waiters do not synchronize their pg_try_advisory_lock attempts. Preserve the existing deadline clamp via left and the 1-second maximum, while adding a small jitter or short exponential backoff without changing the retry budget or connection-release behavior.crates/gitlawb-node/src/api/issues.rs (1)
262-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReal errors are silently indistinguishable from "not authorized" here.
Ok(None) | Err(_) => Noneis a reasonable fail-closed default for the client, but a genuinegit_issues::get_issuefailure (disk/git corruption, IO error) is dropped with no log line, and will look identical to an ordinary "not authorized" 403 in the logs. Compare with the post-lock re-check a few lines down (Line 325-328), which does surface/log the equivalent error. Worth atracing::warn!/debug!on theErr(e)arm here too, purely for operator visibility — the client-facing fail-closed behavior would stay exactly the same.♻️ Proposed refactor
- let author_did: Option<String> = match git_issues::get_issue(&disk_path, &issue_id) { - Ok(Some(raw)) => serde_json::from_str::<IssueRecord>(&raw) - .ok() - .and_then(|i| i.author), - // Cannot establish authorship, so fail closed. Deliberately 403 rather - // than 404 for a non-owner: a caller who is not authorized to write - // should not learn from this route whether the issue exists. - Ok(None) | Err(_) => None, - }; + let author_did: Option<String> = match git_issues::get_issue(&disk_path, &issue_id) { + Ok(Some(raw)) => serde_json::from_str::<IssueRecord>(&raw) + .ok() + .and_then(|i| i.author), + // Cannot establish authorship, so fail closed. Deliberately 403 rather + // than 404 for a non-owner: a caller who is not authorized to write + // should not learn from this route whether the issue exists. + Ok(None) => None, + Err(e) => { + tracing::warn!(repo = %repo, issue = %issue_id, err = %e, "pre-lock issue read failed — treating as unauthorized"); + None + } + };🤖 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/api/issues.rs` around lines 262 - 270, Update the author lookup match around git_issues::get_issue to handle Err(e) separately from Ok(None): preserve the existing fail-closed None result, but emit a tracing warn or debug log containing the retrieval error for operator visibility. Keep successful issue parsing and the client-facing authorization behavior unchanged.
🤖 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.
Inline comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 930-941: Update the acquire_write error logging in the repository
write-lock flow to avoid unconditionally logging expected RepoBusy/transient 503
failures at error severity. Preserve propagation through the existing ?
operator, but classify contention consistently with repo_store.rs by using
warning-level logging or suppressing the duplicate log for RepoBusy while
retaining error logging for unexpected failures.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/issues.rs`:
- Around line 262-270: Update the author lookup match around
git_issues::get_issue to handle Err(e) separately from Ok(None): preserve the
existing fail-closed None result, but emit a tracing warn or debug log
containing the retrieval error for operator visibility. Keep successful issue
parsing and the client-facing authorization behavior unchanged.
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 1363-1372: Replace the fixed 300ms sleep before querying pid_after
with a poll loop that repeatedly acquires a connection and checks
pg_backend_pid() until it differs from the original pid, or a generous deadline
is reached. Reuse the existing poll_until_free approach and preserve the final
pid comparison while preventing transient failures on slow runners.
- Around line 250-258: Randomize the retry delay in the probe loop around the
existing tokio::time::sleep call so concurrent waiters do not synchronize their
pg_try_advisory_lock attempts. Preserve the existing deadline clamp via left and
the 1-second maximum, while adding a small jitter or short exponential backoff
without changing the retry budget or connection-release behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c10504fc-f6f3-4c2e-a9a7-789138ba8d9a
📒 Files selected for processing (9)
.env.examplecrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/error.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/main.rs
Comment on lines
930
to
941
| tracing::debug!(repo = %name, "acquiring write lock"); | ||
| // Log, then propagate rather than stringify: AppError's From<anyhow::Error> | ||
| // downcasts to sqlx::Error, so a lock-pool timeout or a database outage | ||
| // surfaces as a retryable 503. Converting to a string here would collapse both | ||
| // into a 500 and tell a client not to retry something that is transient. | ||
| let guard = state | ||
| .repo_store | ||
| .acquire_write(&record.owner_did, &record.name) | ||
| .await | ||
| .map_err(|e| { | ||
| .inspect_err(|e| { | ||
| tracing::error!(repo = %name, err = %e, "acquire_write failed"); | ||
| AppError::Git(e.to_string()) | ||
| })?; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log severity no longer matches the new retryable classification for lock contention.
RepoBusy is now an expected, transient 503 (and repo_store.rs already warn!s at the point it's raised, both for contention and for lock-pool exhaustion). This tracing::error! still fires unconditionally for that same condition, double-logging it at a higher severity than intended and risking alert-fatigue/false pages on ordinary write contention.
♻️ Proposed fix
let guard = state
.repo_store
.acquire_write(&record.owner_did, &record.name)
.await
.inspect_err(|e| {
- tracing::error!(repo = %name, err = %e, "acquire_write failed");
+ if e.downcast_ref::<crate::git::repo_store::RepoBusy>().is_some() {
+ tracing::warn!(repo = %name, "acquire_write: repo busy, shedding as retryable 503");
+ } else {
+ tracing::error!(repo = %name, err = %e, "acquire_write failed");
+ }
})?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tracing::debug!(repo = %name, "acquiring write lock"); | |
| // Log, then propagate rather than stringify: AppError's From<anyhow::Error> | |
| // downcasts to sqlx::Error, so a lock-pool timeout or a database outage | |
| // surfaces as a retryable 503. Converting to a string here would collapse both | |
| // into a 500 and tell a client not to retry something that is transient. | |
| let guard = state | |
| .repo_store | |
| .acquire_write(&record.owner_did, &record.name) | |
| .await | |
| .map_err(|e| { | |
| .inspect_err(|e| { | |
| tracing::error!(repo = %name, err = %e, "acquire_write failed"); | |
| AppError::Git(e.to_string()) | |
| })?; | |
| tracing::debug!(repo = %name, "acquiring write lock"); | |
| // Log, then propagate rather than stringify: AppError's From<anyhow::Error> | |
| // downcasts to sqlx::Error, so a lock-pool timeout or a database outage | |
| // surfaces as a retryable 503. Converting to a string here would collapse both | |
| // into a 500 and tell a client not to retry something that is transient. | |
| let guard = state | |
| .repo_store | |
| .acquire_write(&record.owner_did, &record.name) | |
| .await | |
| .inspect_err(|e| { | |
| if e.downcast_ref::<crate::git::repo_store::RepoBusy>().is_some() { | |
| tracing::warn!(repo = %name, "acquire_write: repo busy, shedding as retryable 503"); | |
| } else { | |
| tracing::error!(repo = %name, err = %e, "acquire_write failed"); | |
| } | |
| })?; |
🤖 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/api/repos.rs` around lines 930 - 941, Update the
acquire_write error logging in the repository write-lock flow to avoid
unconditionally logging expected RepoBusy/transient 503 failures at error
severity. Preserve propagation through the existing ? operator, but classify
contention consistently with repo_store.rs by using warning-level logging or
suppressing the duplicate log for RepoBusy while retaining error logging for
unexpected failures.
jatmn
requested changes
Jul 30, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
The core lock-session fix looks ready; a few gaps in the new error and transfer layer should be closed before merge.
Findings
-
[P2] Align
acquire_freshHEAD failure handling with the under-lock refresh path
crates/gitlawb-node/src/git/repo_store.rs:157-158,crates/gitlawb-node/src/api/issues.rs:258-277
unwrap_or(false)on Tigris HEAD is pre-existing inacquire_fresh, but this PR now routesclose_issue's non-owner author pre-check through it whileacquire_writewas fixed to refuse onRefreshFailure::Unknown. That leaves two freshness paths with different epistemics for the same operation. The author-denial scenario on a HEAD blip is largely the same as onmain(both skipped download and read local), but owners and authors who pass pre-check on stale local can now hit a refusedacquire_write(500) when HEAD fails under the lock — stricter, not looser. Please propagate HEAD errors out ofacquire_freshthe same way the under-lock refresh does, or stop usingacquire_freshfor auth until it does. -
[P2] Map new transient Tigris refusal paths to a retryable 503, not HTTP 500
crates/gitlawb-node/src/git/repo_store.rs:341-351,crates/gitlawb-node/src/git/repo_store.rs:365-369,crates/gitlawb-node/src/error.rs:82-94
The under-lock HEAD failure and refresh-timeout arms are new in this series and return plainanyhowerrors.AppError::from(anyhow::Error)only downcastssqlx::ErrorandRepoBusy, so these surface asinternal_error/ HTTP 500 even though the comments call them retryable refusals. This is not a regression frommain— acquire failures already mapped to 500 viaAppError::Git— but it is a gap in the new error taxonomy you added for contention and pool exhaustion. Please introduce a typed retryable error (or extend theRepoBusypattern) for HEAD failure and under-lock refresh timeout. -
[P2] Keep repo-identifying detail out of client-visible error bodies on the new paths
crates/gitlawb-node/src/git/repo_store.rs:365-368,crates/gitlawb-node/src/error.rs:168-172
The under-lock refresh timeout embeds{owner_slug}/{repo_name}in the error string, whichAppError::Internalreturns verbatim in the JSONmessage. That contradicts the fixed-body policy you added forRepoBusy.mainalready leaked repo names in lock-contention 500s; this is a new instance on the timeout path. Please log operator detail and return a fixed retryable body to callers, consistent withRepoBusy. -
[P3] Log expected
acquire_writecontention at warn, not error
crates/gitlawb-node/src/api/repos.rs:939-940
inspect_errlogs everyacquire_writefailure aterrorseverity. Base already logged acquire failures at error, butRepoBusyis new — expected 503 contention now hitstracing::error!whilerepo_store.rslogs the same condition atwarn. Please downgrade or suppress logging forRepoBusy(and other expected transient 503 paths) while keeping error logging for unexpected failures.
Tracked follow-up (not blocking this PR)
- #283 — orphaned Tigris extraction after transfer timeout
crates/gitlawb-node/src/git/repo_store.rs:353-369,crates/gitlawb-node/src/git/tigris.rs:118-124,crates/gitlawb-node/src/git/tigris.rs:218-223
spawn_blocking(decompress_repo)is not cancelled whenbounded_transfertimes out; a late extract can stillremove_dir_all+renameafter the lock is released. The mechanism is pre-existing; the timeout bound makes it more reachable. Refusing the write on timeout is the right call and is strictly better than the old path. You already track this as #283 — no action required here beyond keeping that follow-up open.
Reviewed and not raised as defects
- Unbounded
acquire_freshonclose_issuepre-check —acquire_freshwithout a transfer bound is a pre-existing pattern (repos.rsgit-receive-pack uses it too). This PR improves the stranger case (instant 403 vs lock wedge). Not a new amplification primitive worth blocking on. - Lock-pool saturation →
db_unavailable— deliberate choice documented inrepo_store.rs:220-241; operators get pool counters in the warn log. Client-code conflation is a tradeoff, not an oversight. - Proxy idle timeout vs composed write budgets — real operational tension, predates this PR; you already note reconciliation is tracked separately.
- Fleet Postgres connection budget (+32 lock pool) — new default is intentional; PR body asks operators to budget. Deployment sizing, not a logic bug.
Maintainer decisions
- Proxy idle timeout vs composed write budgets. Fly
idle_timeout = 120vs defaults of 90s lock wait, two 300s under-lock transfer spans, and up to 600s git service work. Please confirm the intended production limits as a set, or document the accepted failure mode when the edge drops the client first. - Fleet Postgres connection budget. Defaults now open up to 52 connections per node (20 + 32) with no startup validation. Please confirm fleet sizing or adjust the default before a broad rollout.
CodeRabbit follow-ups verified
- Still open:
repos.rs:939-940— expectedRepoBusylogged at error (see P3 above). - Still open:
issues.rs:262-270— pre-lockgit_issues::get_issueI/O errors are silently folded into the unauthorized path with no log line (operability nit; client behavior is intentionally fail-closed). - Still open:
repo_store.rs:1363-1372— fixed 300ms sleep inrelease_that_did_not_hold_the_lock_closes_the_sessioncan flake on slow CI; poll likepoll_until_free. - Still open:
repo_store.rs:250-258— flat 1s backoff with no jitter on lock retry (fairness nit under contention).
What looks good
The core session-affinity fix is sound: the guard owns the lock-holding connection, LockProbe closes on cancellation, unlock results are observed, pool splitting is wired correctly, and the regression tests against pg_locks are thoughtfully constructed. close_issue authorization reorder and under-guard re-authorization close the wedge this series introduced. CI is green on the head commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #279.
Session-scoped Postgres advisory locks were taken with
fetch_one(&pool)and released withexecute(&pool). Those are two independent pool checkouts, so the release almost always landed on a backend that held nothing,pg_advisory_unlockreturned false, and the return value was discarded by alet _. The lock leaked on essentially every write.Measured on
mainat 111cff7 before writing any of this:pg_lockspg_advisory_unlockreports "you did not hold this" as a false return plus a warning, never an error, which is why this was silent.What changes
RepoWriteGuardnow owns the connection that took the lock for its whole lifetime and releases on that same session. Everything else here follows from that pin rather than being bundled with it.Pinning a connection per in-flight write means writes can no longer share the 20-connection application pool, so they get a dedicated pool (
GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS, default 32,connect_lazy). Without it, a push burst starves ordinary reads.Pinning also makes the post-write upload load-bearing. It was unbounded and free before, because nothing was held while it ran; now a stalled transfer holds a lock-pool slot, and enough of them deny every write on the node. Hence
GITLAWB_LOCK_HELD_TRANSFER_TIMEOUT_SECS(default 300) over any object-storage transfer that runs with the lock held.The acquire is
pg_try_advisory_lockwith backoff rather than a blocking acquire, so a stale lock from a crashed connection cannot wedge a repo indefinitely. It is bounded on wall clock as well as attempt count, and a waiter hands its pool slot back before each backoff so spinners cannot starve the pool.A cancelled
.awaitdoes not cancel a SQL statement that has already been sent. That asymmetry is the whole design: cancelling an unlock is harmless because the statement completes server side, but cancelling an acquire strands the lock. Soclose_on_dropis armed before the try-lock goes out, via a wrapper that owns the connection in anOptionand closes it in its ownDropunless the lock was positively not taken.PoolConnection::close_on_dropis a one-way setter, so disarming isOption::takerather than a second call.close_issuetook the write lock and then ran the owner-or-author check, returning 403 with the lock held. Once exclusion actually works, that is a wedge primitive for anyone with read access, so authorization moved above the lock. The author fallback reads the issue's git-JSON blob without the lock as a pre-check, and the authoritative owner-or-author check runs again under the guard, becauseacquire_writere-downloads the archive and the tree that gets mutated is frequently not the one the pre-check read.Two settled calls, stated here rather than left open:
Readiness does not probe the lock pool. A node can report ready while every write fails, which two reviewers flagged. Failing readiness on a saturated pool would pull the node out of routing, take its reads down with it, and push its write load onto peers carrying the same load. Saturation surfaces in the request path instead: a retryable 503 to the caller and a warn line carrying the pool's own counters so an incident can distinguish "the pool is full" from "the database is gone."
Entry concurrency is not bounded here. Bounding it belongs with hold time, not with this pin, and the arithmetic is in #282. A rate limit provably cannot close that one, so it is not #196's either.
advisory_lock_keydeliberately stays onDefaultHasher. #215 owns the change to SHA-256 for #210 and the two need to stay separable.Verification
Every guard here was checked by reverting the exact production line it protects and observing red first. That is not incidental: an earlier round of this work shipped with tests that did not observe what they claimed, including one that seeded a repo owner as their own issue author, which made it pass with the owner check disabled entirely.
The must-not tests observe
pg_locksfrom a standalone connection, never from the lock pool, because pool reuse hands the observer the lock-holding session and reentrantly re-grabs the lock, hiding the leak. Lock-freed assertions poll with a deadline rather than asserting immediately, sincePoolConnection::dropspawns the close.530 tests pass, clippy is clean under
-D warnings.Known gaps
The under-lock refresh timeout and the corrupt-archive fallback are correct by reading and not by execution. Driving either needs a seam to stall an object-storage response, which does not exist yet and is out of scope here. For the same reason the author-path test cannot distinguish
acquirefromacquire_fresh:RepoStore::for_testinghas no object-storage client, so the two calls are identical in every test in the suite. The test says so rather than claiming the coverage.The refresh timeout also leaves a hazard it narrows rather than removes: refusing the acquire frees the lock while an uncancellable extraction is still headed for a directory swap. That is #283.
#284 is the remaining cost lever on
close_issue, which this branch improves onmain(the fetch no longer happens with the lock held) without removing.One open operational question: 20 application connections plus 32 lock connections per node needs to fit the fleet's Postgres
max_connections. If it does not, the default is what should change.Summary by CodeRabbit
New Features
Bug Fixes
Documentation