fix(node): release the advisory lock on the session that took it (#279)

[beardthelion]

Fixes #279.

Session-scoped Postgres advisory locks were taken with fetch_one(&pool) and released with execute(&pool). Those are two independent pool checkouts, so the release almost always landed on a backend that held nothing, pg_advisory_unlock returned false, and the return value was discarded by a let _. The lock leaked on essentially every write.

Measured on main at 111cff7 before writing any of this:

• two writers on one node against the same repo both acquired
• 50 of 50 sequential acquire/release cycles leaked
• 100 writes left 100 orphaned locks in pg_locks

pg_advisory_unlock reports "you did not hold this" as a false return plus a warning, never an error, which is why this was silent.

What changes

RepoWriteGuard now 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_lock with 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 .await does 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. So close_on_drop is armed before the try-lock goes out, via a wrapper that owns the connection in an Option and closes it in its own Drop unless the lock was positively not taken. PoolConnection::close_on_drop is a one-way setter, so disarming is Option::take rather than a second call.

close_issue took 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, because acquire_write re-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_key deliberately stays on DefaultHasher. #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_locks from 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, since PoolConnection::drop spawns 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 acquire from acquire_fresh: RepoStore::for_testing has 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 on main (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

  • Added configurable limits for repository lock connections and storage transfers.
  • Added clear retryable responses when repository operations are temporarily busy or unavailable.
  • Added safeguards to prevent conflicting repository updates.

• Bug Fixes

  • Unauthorized issue actions are rejected before repository locks are acquired.
  • Improved protection against revealing whether inaccessible issues exist.
  • Repository locks now release safely during cancellations, contention, and transfer failures.
  • Failed lock releases no longer allow incomplete repository or pull request updates.

• Documentation

  • Updated the environment configuration example with the new settings.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR adds a dedicated advisory-lock pool, session-pinned repository locks, bounded Tigris transfers, conditional uploads, typed repository errors, and pre-lock authorization checks for issue closure.

Changes

Repository write controls

Layer / File(s)	Summary
Dedicated lock-pool configuration and wiring .env.example, crates/gitlawb-node/src/config.rs, crates/gitlawb-node/src/db/mod.rs, crates/gitlawb-node/src/git/repo_store.rs, crates/gitlawb-node/src/main.rs	Adds validated lock-pool and transfer-timeout settings. Creates a dedicated lazy advisory-lock pool and passes it to RepoStore.
Conditional storage and snapshot transfers crates/gitlawb-node/src/git/tigris.rs	Adds ETag reads, conditional upload handling, bounded download modes, isolated snapshot extraction, and integration coverage.
Session-pinned locking and bounded transfers crates/gitlawb-node/src/git/repo_store.rs	Pins locks to owning sessions, handles cancellation and deadlines, bounds transfers, checks unlock results, closes unsafe sessions, and tests cleanup and pool isolation.
Typed repository error propagation crates/gitlawb-node/src/error.rs, crates/gitlawb-node/src/api/issues.rs, crates/gitlawb-node/src/api/pulls.rs, crates/gitlawb-node/src/api/repos.rs	Maps transient repository errors to 503 Service Unavailable and preserves acquisition and release errors through API handlers.
Pre-lock issue authorization crates/gitlawb-node/src/api/issues.rs	Checks authorization before locking, revalidates it under the guard, distinguishes missing-issue responses, and adds regression tests.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related issues

• Gitlawb/node issue 282 — Addresses the lock-held transfer timeout used by repository write locking.

Possibly related PRs

• Gitlawb/node#215 — Modifies advisory-lock handling in RepoStore.
• Gitlawb/node#174 — Modifies the Git handlers affected by repository lock behavior.
• Gitlawb/node#304 — Modifies repository write-lock and merge paths.

Suggested labels: subsystem:storage

Suggested reviewers: kevincodex1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR includes substantial close_issue authorization and Tigris conditional-publishing changes beyond the coding requirements stated in linked issue #279.	Move the authorization and conditional-publishing changes to linked issues or separate pull requests, unless their scope is explicitly added to #279.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary advisory-lock session-affinity fix and references issue #279.
Description check	✅ Passed	The description provides detailed motivation, implementation changes, verification results, and known gaps, despite omitting several template sections.
Linked Issues check	✅ Passed	The implementation satisfies #279 by preserving session affinity, preventing concurrent writers, releasing locks, and testing cancellation and pool behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/279-advisory-lock-session-affinity

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

crates/gitlawb-node/src/git/repo_store.rs (2)

1363-1372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the fixed 300ms sleep with a poll loop.

PoolConnection::drop spawns the close, so on a loaded CI runner the close may not have completed when the next acquire() runs — the pool then hands back the same still-open connection and the assert_ne! fails spuriously. Polling until the pid changes (or a generous deadline elapses) makes this deterministic, matching the rationale already used in poll_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 value

Consider 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_lock gives 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 win

Real errors are silently indistinguishable from "not authorized" here.

Ok(None) | Err(_) => None is a reasonable fail-closed default for the client, but a genuine git_issues::get_issue failure (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 a tracing::warn!/debug! on the Err(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

📥 Commits

Reviewing files that changed from the base of the PR and between c83cbc5 and 1cc2c7c.

📒 Files selected for processing (9)

• .env.example
• crates/gitlawb-node/src/api/issues.rs
• crates/gitlawb-node/src/api/pulls.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/error.rs
• crates/gitlawb-node/src/git/repo_store.rs
• crates/gitlawb-node/src/main.rs

[jatmn]

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_fresh HEAD 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 in acquire_fresh, but this PR now routes close_issue's non-owner author pre-check through it while acquire_write was fixed to refuse on RefreshFailure::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 on main (both skipped download and read local), but owners and authors who pass pre-check on stale local can now hit a refused acquire_write (500) when HEAD fails under the lock — stricter, not looser. Please propagate HEAD errors out of acquire_fresh the same way the under-lock refresh does, or stop using acquire_fresh for 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 plain anyhow errors. AppError::from(anyhow::Error) only downcasts sqlx::Error and RepoBusy, so these surface as internal_error / HTTP 500 even though the comments call them retryable refusals. This is not a regression from main — acquire failures already mapped to 500 via AppError::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 the RepoBusy pattern) 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, which AppError::Internal returns verbatim in the JSON message. That contradicts the fixed-body policy you added for RepoBusy. main already 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 with RepoBusy.

• [P3] Log expected acquire_write contention at warn, not error
  crates/gitlawb-node/src/api/repos.rs:939-940
inspect_err logs every acquire_write failure at error severity. Base already logged acquire failures at error, but RepoBusy is new — expected 503 contention now hits tracing::error! while repo_store.rs logs the same condition at warn. Please downgrade or suppress logging for RepoBusy (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 when bounded_transfer times out; a late extract can still remove_dir_all + rename after 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_fresh on close_issue pre-check — acquire_fresh without a transfer bound is a pre-existing pattern (repos.rs git-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 in repo_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 = 120 vs 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 — expected RepoBusy logged at error (see P3 above).
• Still open: issues.rs:262-270 — pre-lock git_issues::get_issue I/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 in release_that_did_not_hold_the_lock_closes_the_session can flake on slow CI; poll like poll_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.

[coderabbitai]

Actionable comments posted: 2

🤖 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/git/repo_store.rs`:
- Around line 391-420: Prevent stale asynchronous extraction from replacing
newer repository data: update decompress_repo and the repository write/acquire
flow to track in-flight extractions per repository and make later writes wait or
fail until extraction completes, or validate a repository generation immediately
before publishing. Ensure the final remove_dir_all and rename cannot overwrite
changes made after the timed-out download.
- Around line 923-943: In the no-runtime branch of the write-guard drop logic,
replace the conn.leak() call with conn.detach() so the pool bookkeeping is
released and capacity remains replenishable. Preserve the existing synchronous
drop behavior for the detached PgConnection and update the nearby comment to
describe detach rather than a permanent leak.

🪄 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: 96e1a177-8f0f-4b1e-b34b-f4afde424e77

📥 Commits

Reviewing files that changed from the base of the PR and between 1cc2c7c and 281f0ee.

📒 Files selected for processing (10)

• .env.example
• crates/gitlawb-node/src/api/issues.rs
• crates/gitlawb-node/src/api/pulls.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/error.rs
• crates/gitlawb-node/src/git/repo_store.rs
• crates/gitlawb-node/src/git/tigris.rs
• crates/gitlawb-node/src/main.rs

🚧 Files skipped from review as they are similar to previous changes (5)

• crates/gitlawb-node/src/main.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/api/pulls.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/api/issues.rs

[beardthelion]

All four findings are addressed, plus two of the three CodeRabbit items. The branch is rebased onto current main and pushed as five follow-up commits, so the reviewed history is unchanged. 16 of 17 checks are green on 281f0ee (CodeRabbit has not reported yet).

P2, acquire_fresh HEAD handling

Took the first of your two remedies: acquire_fresh now propagates the HEAD error instead of collapsing it, so both freshness paths refuse on the same condition (9337300). The second remedy, dropping acquire_fresh from the auth pre-check, would have reintroduced the bug its comment documents, where a stale local copy hides an author's own issue and 403s a legitimate author.

Worth flagging that this helper has two callers, not one. The advertisement path in repos.rs wraps it in a map_err that bypasses the From chain entirely, so the typed error would have been stringified into a 500 git_error there. That call site now lets only the typed error through and leaves every other failure on exactly its previous behavior, because it also serves the read path and rerouting all of it would move the read path's error vocabulary. issues.rs needed no change; its bare ? already routes correctly.

P2, transient refusals mapping to 500

RepoUnavailable follows the RepoBusy pattern exactly: fieldless type, raised with the operator detail in a context string, its own downcast rung, mapped to a 503 (abc3c17). Both new refusal arms route through it.

P2, repo detail in client bodies

Same commit. The 503 body interpolates nothing, and the test asserts the negative directly: the response body contains the error code and does not contain the repo name or the owner DID. The detail stays in the log at the raise site.

P3, contention logged at error

Fixed at both call sites (07e4254). The second one matters: the acquire_fresh change above means the advertisement path now raises the same expected-transient class, so fixing only the acquire_write site would have shipped a new source of error-level noise for a condition this series just classified as ordinary. The classifier follows the startup path's permanent-versus-transient split, and anything it cannot classify still logs at error, so an unknown failure keeps paging.

One deliberate asymmetry: the 300s under-lock timeout logs at error at its raise site, not warn, with a comment saying why. It is not an ordinary blip, it held a lock-pool slot for five minutes, and it needs to keep paging through the handler demotion.

CodeRabbit items

Fixed: the swallowed get_issue error is now logged, with the fail-closed 403 unchanged (d4d2766), and the 300ms sleep is now a poll on a standalone connection (281f0ee).

Declined: jitter on the lock retry backoff. The node crate has no direct rand or fastrand dependency (the only rand in the tree is a libp2p-identity feature flag), so this means adding one for a fairness improvement, on a loop that two open PRs already touch. Happy to revisit if you think the contention case justifies it.

The two decisions you asked for

Connection budget. It fits, and the numbers are measured rather than estimated. Postgres gives 97 usable connections (100 minus the 3 superuser reserve), verified against a running instance, and nothing in the compose file or the Terraform template overrides max_connections. The shipped default topology is one Postgres per node, since use_rds defaults to false and the compose template only points at an external host when an operator opts in, so the node count multiplier is 1 and 52 of 97 leaves 45 spare. It breaks only on a shared external database, which is opt-in.

You are right that the missing piece is boot enforcement rather than the number. That belongs in Config::validate, which does not exist on main; it is in #174. Rather than build a second one here and put two open PRs on config.rs at once, it goes in as a clause on the existing validator once #174 lands. Worth noting the existing validator will need retargeting at the same time: its floor keys on the main pool, which is correct today but becomes the wrong pool once writes move to the dedicated one.

Timeout set. Not raising the Fly idle timeout. The 120 is deliberate and the config comment ties it to the 2026-06-12 outage, where long idle windows let hung clients pin connection slots. Not lowering the transfer bound either, since that is what stops a stalled transfer from pinning a lock-pool slot. The real reconciliation needs a different mechanism, and the code comment that said it was tracked separately was tracking nothing, so it is now #299.

On the test seam, and a correction

The earlier draft of this work recorded the wiring as unprovable without an object-store abstraction. That was wrong. RepoStore::new is public and takes the client, so a test-only constructor pointed at a closed port makes a failed HEAD reachable in process with no new dependency and no trait. Both refusals are now executed rather than read-verified, including the under-lock arm, and both tests run in under a second.

Two things remain read-verified and are recorded rather than implied: the timeout arm needs a hang rather than an error, so a refused connection cannot reach it, and nothing joins the store-layer raise to the handler-layer mapping end to end. That second one is #302, and it is cheaper than it looks because a router harness for the advertisement handler already exists.

Also a correction to something I would otherwise have claimed here. Refusing at the advertisement is not strictly cheaper than uploading a pack first. If a storage blip ends between the advertisement and the POST, the push succeeds today and will not after this change, and that window is the pack-upload duration, so it widens with push size. It is still the right call, because the alternative is advertising refs from a tree the write may not be allowed to use, but it is a real behavior change on a read surface and on the close-issue pre-check, where there is no pack upload to save at all.

Filed rather than fixed

Verification of the surrounding code turned up three things that are not in scope here: #300 (a failed HEAD on a cache miss renders a populated repo as an empty 200 on the read endpoints, which is worse than the 500 I first assumed), #301 (the advertisement leg runs an unbounded git subprocess where the other two legs are bounded), and #302 above.

[coderabbitai]

🧹 Nitpick comments (1)

crates/gitlawb-node/src/git/repo_store.rs (1)

1631-1652: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the typed refusal instead of an outer timeout.

This test uses the default 90-second LOCK_ACQUIRE_DEADLINE and asserts only that the 8-second outer tokio::time::timeout fired. That assertion passes for any reason the future did not finish in 8 seconds, including a lock-pool stall unrelated to advisory-lock exclusion. It also adds 8 seconds to every suite run.

with_lock_acquire_deadline already exists and is used by contended_acquire_sheds_as_repo_busy_not_internal_error. Apply it here and assert the RepoBusy downcast, so the test proves exclusion positively and finishes in well under a second.

♻️ Proposed change

-        let store = write_store(&pool, &opts).await;
+        let store = write_store(&pool, &opts)
+            .await
+            .with_lock_acquire_deadline(std::time::Duration::from_millis(300));
 
         let _first = store
             .acquire_write("did:key:z6MkU3Excl", "same-repo")
             .await
             .expect("first writer acquires");
 
-        let second = tokio::time::timeout(
-            std::time::Duration::from_secs(8),
-            store.acquire_write("did:key:z6MkU3Excl", "same-repo"),
-        )
-        .await;
-
-        assert!(
-            second.is_err(),
-            "second writer must NOT be admitted while the first holds the guard \
-             (it should still be retrying when the deadline hits)"
-        );
+        let err = match store.acquire_write("did:key:z6MkU3Excl", "same-repo").await {
+            Err(e) => e,
+            Ok(second) => {
+                second.release(false).await;
+                panic!("a second writer must NOT be admitted while the first holds the guard");
+            }
+        };
+        assert!(
+            err.downcast_ref::<RepoBusy>().is_some(),
+            "the second writer must be shed as RepoBusy, got {err:#}"
+        );

🤖 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 1631 - 1652, Update
two_writers_on_the_same_repo_are_not_both_admitted to configure a short deadline
via with_lock_acquire_deadline, matching
contended_acquire_sheds_as_repo_busy_not_internal_error. Replace the outer
tokio::time::timeout assertion with an assertion that the second acquire_write
call returns the typed RepoBusy refusal, while preserving the first writer’s
active guard.

🤖 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.

Nitpick comments:
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 1631-1652: Update
two_writers_on_the_same_repo_are_not_both_admitted to configure a short deadline
via with_lock_acquire_deadline, matching
contended_acquire_sheds_as_repo_busy_not_internal_error. Replace the outer
tokio::time::timeout assertion with an assertion that the second acquire_write
call returns the typed RepoBusy refusal, while preserving the first writer’s
active guard.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5ca977d5-6794-4baa-baf0-9349aa9c6653

📥 Commits

Reviewing files that changed from the base of the PR and between 281f0ee and 358dbe9.

📒 Files selected for processing (10)

• .env.example
• crates/gitlawb-node/src/api/issues.rs
• crates/gitlawb-node/src/api/pulls.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/error.rs
• crates/gitlawb-node/src/git/repo_store.rs
• crates/gitlawb-node/src/git/tigris.rs
• crates/gitlawb-node/src/main.rs

🚧 Files skipped from review as they are similar to previous changes (8)

• crates/gitlawb-node/src/error.rs
• crates/gitlawb-node/src/main.rs
• crates/gitlawb-node/src/git/tigris.rs
• crates/gitlawb-node/src/api/pulls.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/api/issues.rs

[jatmn]

Rechecked head 358dbe97 after your follow-up commits. The core session-affinity fix looks ready; the prior P2 items from my earlier review are addressed on this head. One gap remains in the new RepoUnavailable error layer.

Findings

• [P2] Map transient acquire_fresh download failures to RepoUnavailable, not HTTP 500
  crates/gitlawb-node/src/git/repo_store.rs:180-191, crates/gitlawb-node/src/api/repos.rs:579-594, crates/gitlawb-node/src/api/issues.rs:258-261
acquire_fresh now refuses a failed Tigris HEAD as RepoUnavailable (retryable 503), but a failed GET when no local copy exists still returns a plain anyhow error. On git-receive-pack info/refs, the map_err closure only routes RepoUnavailable through AppError::from; every other failure is stringified to AppError::Git → 500. A transient object-storage GET blip during push advertisement therefore returns a non-retryable 500 while a HEAD blip on the same path returns retryable 503 — inconsistent client semantics within one endpoint. close_issue's non-owner pre-check has the same split via bare ?. Please raise download failures that leave storage state unknowable (archive present per HEAD, GET failed, no local fallback) as RepoUnavailable, matching the HEAD arm and the under-lock refresh path.

• [P3] Tighten two_writers_on_the_same_repo_are_not_both_admitted to assert RepoBusy
crates/gitlawb-node/src/git/repo_store.rs:1631-1651
  This acceptance test still wraps the second acquire_write in an 8-second outer tokio::time::timeout and only checks that the future did not finish. That passes for unrelated stalls (lock-pool saturation, slow CI) and adds ~8s to every suite run. CodeRabbit's suggestion still applies: use with_lock_acquire_deadline (as contended_acquire_sheds_as_repo_busy_not_internal_error already does) and assert the typed RepoBusy downcast while the first guard remains held.

Prior review items — verified fixed on this head

• acquire_fresh HEAD failures now propagate as RepoUnavailable instead of unwrap_or(false) (aef72fa).
• Under-lock HEAD/timeout refusals map to retryable 503 via RepoUnavailable with fixed bodies (d4c7af6).
• acquire_write / info_refs contention and expected transient failures log at warn, not error (2cfee3d, repos.rs:579-584, 969-974).
• close_issue pre-check logs get_issue I/O failures while keeping fail-closed 403 (07d98af).
• Release-invariant test polls pg_stat_activity instead of sleeping 300ms (358dbe97).

Maintainer decisions (unchanged)

• Proxy idle timeout vs composed write budgets. Fly idle_timeout = 120 vs defaults of 90s lock wait, 300s under-lock transfer (twice on a full push), and 600s git service work. Please confirm the intended production limit set or document the accepted failure mode when the edge drops first (#299).
• Fleet Postgres connection budget. Defaults now open up to 52 connections per node (20 + 32) with no startup validation. Your measured single-node topology fits; please confirm fleet sizing for shared external Postgres or adjust defaults before broad rollout. Boot-time enforcement deferred to #174 is still the right place.

Tracked follow-up (not blocking this PR)

• #283 — orphaned Tigris extraction after under-lock transfer timeout. Refusing the acquire on timeout is strictly better than proceeding; the uncancellable spawn_blocking swap can still race a later writer. Keep #283 open.
• #300 — acquire() still swallows Tigris HEAD errors via unwrap_or(false) on read paths. Pre-existing; out of scope here but now inconsistent with the freshness paths this series fixed.

What looks good

The advisory-lock leak is fixed correctly: the guard owns the lock-holding session, LockProbe closes on cancellation, unlock results are observed, pool splitting is wired, and the pg_locks regression tests are load-bearing. close_issue authorization reorder and under-guard re-authorization close the wedge this series introduced. All 17 CI checks are green on head.

[beardthelion]

Both findings are fixed on bc00199.

[P2] Fresh download failures now refuse as RepoUnavailable. Raised at the source in acquire_fresh rather than patched at each consumer: From<anyhow::Error> for AppError (error.rs:98) already downcasts RepoUnavailable out of the context chain, so the info/refs closure and close_issue's bare ? both pick up the retryable mapping with no edit. The new test drives HEAD 200 with GET 500, which is the archive-present, GET-failed, no-local-fallback state you named.

I checked that test is load-bearing rather than trusting it green. Reverting the raise back to return Err(e).context("downloading repo from tigris (fresh)") turns it red at repo_store.rs:2244:

the refusal must be typed so the handler layer maps it to a retryable 503,
got downloading repo from tigris (fresh): tigris GET repos/v1/.../freshrepo.tar.zst: service error

That also confirms the downcast survives the .context() wrap, which is the part the single-site fix depends on.

[P3] The contention test asserts the typed refusal. two_writers_on_the_same_repo_are_not_both_admitted now uses with_lock_acquire_deadline(300ms) and asserts the RepoBusy downcast while the first guard is held, matching contended_acquire_sheds_as_repo_busy_not_internal_error. It fails loudly if a second writer is admitted, which the outer timeout could not tell apart from a stall. The three targeted tests finish in 1.14s.

fmt, clippy --locked --workspace --all-targets -D warnings, and deny_harness pass on the pushed head.

Still open on my side and not code: the proxy idle timeout against the composed write budgets, and the fleet Postgres connection budget. Both are decisions rather than fixes, so I'll answer them on their own rather than fold them into a resolution round. #283 and #300 stay open as tracked.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Do not refresh the live repository before acquiring the write guard
  crates/gitlawb-node/src/api/issues.rs:238-261
  The new non-owner pre-check calls acquire_fresh before taking the advisory lock. That call downloads and publishes directly into local_path; its publish step removes the existing repository directory and renames the extracted copy into place (tigris.rs:240-250), while the guard only serializes Postgres writers. Any signed non-owner can trigger that refresh before get_issue rejects them, concurrently with git_receive_pack or another guarded write on the same path. The refresh can therefore delete/swap the directory under an in-flight write. Use a non-mutating snapshot for the authorship pre-check, or coordinate this refresh/publish with the same write exclusion.

• [P1] Do not unlock while a timed-out upload can still publish
  crates/gitlawb-node/src/git/repo_store.rs:845-864
tokio::time::timeout drops the client future, but does not establish that the S3 PUT stopped; the comment correctly notes that it may finish later. The guard then unlocks, letting writer B refresh, modify, and upload the newer archive, after which A's late PUT can overwrite the one object key with A's older archive. A later node refresh then loses B's acknowledged update. Keep serialization until the publication outcome is known, or fence/version/conditionally publish so an abandoned upload cannot become visible after a successor.

• [P2] Enforce the lock-acquire deadline around each database await
  crates/gitlawb-node/src/git/repo_store.rs:254-295
  The remaining budget is checked only before lock_pool.acquire().await; the pool checkout and the subsequent pg_try_advisory_lock query are not bounded by left. A checkout that begins just before the 90-second deadline may wait the full independently configurable DB acquire timeout (or a slow query may complete after the deadline), and a late successful query is accepted. This violates the advertised wall-clock cap and lets saturated/slow DB paths keep write tasks beyond the retry budget. Apply the remaining deadline to both awaits and reject any late acquisition.

[beardthelion]

All three findings are addressed on ea5af98, as seven commits rather than a fixup, because P1b turned out to need a different mechanism than the one I first reached for.

[P1] The author pre-check no longer touches the live directory. RepoStore::read_snapshot downloads to a throwaway temp dir and returns a guard that removes it on drop, so the pre-check still reads fresh data and the publish step never runs against local_path. read_snapshot_is_non_mutating asserts the snapshot path differs from the live path, that the live path is never created, and that the temp dir is gone after drop. The wedge invariant survives: stranger_is_refused_without_waiting_on_the_write_lock still passes, which is what ruled out the simpler fix of moving the check under the lock.

[P2] The deadline now bounds both awaits. The pool checkout and the pg_try_advisory_lock query each run under the remaining budget and shed as RepoBusy, so the advertised wall-clock cap no longer holds only on the fast path.

[P1b] You were right that unlocking is the wrong place to fix this, and my first attempt was wrong too. I initially kept the lock held on the timeout arm. That fences nothing, and I should have proven it before writing it: release takes mut self, so the guard drops the moment it returns and Drop closes the session. Measured with the upload parked for 10s behind a 200ms bound, a successor took the same repo's lock 5ms after release returned while the PUT was still in flight. That mechanism is deleted.

The fence is now on the publish itself, which is the only place that can actually reject a stale write. acquire_write reads the archive's ETag under the lock, the guard carries it, and release publishes conditionally on that generation. An abandoned PUT loses because the generation it was written against is gone. Driven end to end rather than argued: A's release is parked past its bound and returns with the outcome unknowable, B acquires and publishes, then A's captured PUT is replayed and the store answers 412 with B's archive intact. The create-only arm has its own test, and a control case pins that an abandoned PUT whose generation still matches does land, so the headline result is attributable to staleness rather than to replay.

Two consequences worth flagging, since neither was in your findings:

The three background uploads outside the write guard (init, acquire's backfill, release_after_write) were publishing unconditionally, which would have let our own code defeat the fence. init uploads an empty bare repo, so a push landing just before it could have had its archive replaced by that empty one. All three now publish create-only, and a refusal there is logged as the correct outcome rather than a failure.

Because init is now create-only, a first push to a fresh repo can lose the race against it. So a lost precondition gets exactly one supersede-retry: the writer still holds the lock, so it re-reads the ETag and republishes once, and a second loss refuses. At most two PUT attempts, ever. That keeps an ordinary first push working instead of surfacing a 503 on the most common operation there is.

A refused publish is surfaced rather than logged and dropped. release returns a #[must_use] outcome and the four publishing handlers propagate it before any trust bump, webhook, or success body, so a publish the store refused reads as a retryable 503 instead of a 201. The three release(false) sites deliberately do not map it, since they publish nothing and a 503 there would shadow the 403 or 404 the route means to return.

One classification call worth your eye: a 404 on a conditional PUT is treated as permanent, not as a lost precondition. AWS documents 404 for a delete racing a conditional write, but TigrisClient::delete has zero callers on this line, so that race cannot arise from our own code, and a 404 here means a wrong bucket or endpoint. Reporting that as retryable would send clients into a loop against a permanent fault. 409 under create-only is folded in, since that one is a genuine conflict.

Verification: the full suite passes locally, and fmt, clippy --locked, and cargo metadata --locked are clean, so the lockfile will not fail CI. Every guard added here was proven load-bearing by injecting the exact defect it names and confirming the named test goes red, 9 of 9. The uncontended-write test was separately confirmed to stay green under the same mutation that reddens the fence, so it pins the do-not-spuriously-refuse property rather than restating the fix.

Direction, not verified: the fence is checked against vendor documentation and a mock that implements the conditional semantics, not against the real backend. Tigris requires a Single-region or Multi-region bucket for conditional operations; against a Global or Dual-region bucket an ignored If-None-Match returns 200 and a publish that should have been fenced would land silently. There is a credentials-gated probe (tigris_honors_conditional_writes) that checks both arms against the real endpoint and cleans up unconditionally, but it has not been run. Worth settling before this is trusted in production.

#283 stays deferred, and no migration was added.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

crates/gitlawb-node/src/git/tigris.rs (2)

242-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider replacing the publish boolean with an explicit mode.

download_to changes both its mutation behavior and the meaning of its return value based on publish. At a call site, true and false carry no meaning without reading the doc comment. An enum such as ExtractMode::Publish and ExtractMode::Snapshot names both variants at the call site.

🤖 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/tigris.rs` around lines 242 - 250, Replace the
boolean publish parameter in download_to with an explicit extraction mode enum,
defining named variants for publish and snapshot behavior. Update download_to’s
branching, return-value handling, and all call sites to use the corresponding
mode variants while preserving existing behavior.

---

268-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting the shared temp-dir unpack step.

Lines 288-299 repeat decompress_repo lines 378-391 exactly: create a unique temp dir, unpack the archive, and remove the temp dir on failure. Only the directory-name infix and the final swap differ. A shared helper such as unpack_to_temp_dir(data, parent, prefix) -> Result<PathBuf> would let decompress_repo call it and then perform the swap.

Line 306 also logs path = %target.display() in snapshot mode, but the bytes landed in extracted. Log extracted instead so the message names the directory that was populated.

🤖 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/tigris.rs` around lines 268 - 307, The
temporary-directory extraction logic duplicated in the non-publish branch and
decompress_repo should be moved into a shared helper such as unpack_to_temp_dir,
parameterized by archive data, parent directory, and naming prefix; have
decompress_repo reuse it before performing its existing swap. In the download
log, update the path field to use extracted rather than target so snapshot mode
reports the populated directory.

🤖 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/git/tigris.rs`:
- Around line 185-216: Update the status extraction in the request error
handling around UploadPrecondition and RepoWriteGuard::publish to use
SdkError::raw_response() for both service and response error variants. Ensure
unparsable 409 and 412 responses are classified as lost preconditions so the
existing supersede retry remains reachable, while preserving the current
status-based behavior for other errors.

---

Nitpick comments:
In `@crates/gitlawb-node/src/git/tigris.rs`:
- Around line 242-250: Replace the boolean publish parameter in download_to with
an explicit extraction mode enum, defining named variants for publish and
snapshot behavior. Update download_to’s branching, return-value handling, and
all call sites to use the corresponding mode variants while preserving existing
behavior.
- Around line 268-307: The temporary-directory extraction logic duplicated in
the non-publish branch and decompress_repo should be moved into a shared helper
such as unpack_to_temp_dir, parameterized by archive data, parent directory, and
naming prefix; have decompress_repo reuse it before performing its existing
swap. In the download log, update the path field to use extracted rather than
target so snapshot mode reports the populated directory.

🪄 Autofix

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: f932b365-e4b9-442c-bb9a-58a6ed92a923

📥 Commits

Reviewing files that changed from the base of the PR and between bc00199 and ea5af98.

📒 Files selected for processing (6)

• crates/gitlawb-node/src/api/issues.rs
• crates/gitlawb-node/src/api/pulls.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/error.rs
• crates/gitlawb-node/src/git/repo_store.rs
• crates/gitlawb-node/src/git/tigris.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• crates/gitlawb-node/src/api/pulls.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/api/issues.rs

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

aws-sdk-s3 Rust SdkError ResponseError vs ServiceError when error response body cannot be parsed

💡 Result:

In the AWS SDK for Rust, the SdkError enum distinguishes between errors received from the service and errors that occur during the handling of the HTTP response [1][2]. When an error response body cannot be parsed (e.g., malformed XML/JSON, unexpected content, or premature connection closure), the SDK returns SdkError::ResponseError [1][2]. ServiceError vs. ResponseError - SdkError::ServiceError(ServiceError<E, R>): This variant indicates that the service successfully returned a structured error response that the SDK was able to parse [1][2]. The ServiceError contains the modeled error type (E) and the raw HTTP response (R) [3]. - SdkError::ResponseError(ResponseError): This variant indicates that the SDK received a response from the service, but it failed to parse the payload according to the expected protocol (e.g., XML/JSON decoding failure) [1][2]. The ResponseError includes the raw response (R) and the underlying parsing error [3]. If the SDK cannot parse the error response body, it cannot identify the specific service error (like NoSuchKey for S3), so it must return a ResponseError rather than a ServiceError [1][2]. Consequently, you cannot use methods like into_service_error or as_service_error on this variant, as no modeled error (E) exists [2][3]. You can access the raw response via the raw_response method to manually inspect the body or headers for debugging purposes [2][3].

Citations:

• 1: https://docs.rs/aws-sdk-s3/latest/aws_sdk_s3/error/type.SdkError.html
• 2: https://docs.rs/aws-smithy-runtime-api/latest/aws_smithy_runtime_api/client/result/enum.SdkError.html
• 3: https://github.com/smithy-lang/smithy-rs/blob/56f4c8e9479792b8b52cc394d563ed49a01d1dcc/rust-runtime/aws-smithy-runtime-api/src/client/result.rs

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- dependency declarations ---'
rg -n 'aws-sdk-s3|aws-smithy-runtime-api|aws-smithy-types' Cargo.toml '**/Cargo.toml' 2>/dev/null || true
printf '%s\n' '--- classifier and call site ---'
sed -n '175,222p' crates/gitlawb-node/src/git/tigris.rs
sed -n '1040,1125p' crates/gitlawb-node/src/git/repo_store.rs
printf '%s\n' '--- SdkError references ---'
rg -n 'SdkError::(ServiceError|ResponseError)|raw_response|raw\(\)' crates Cargo.toml '**/Cargo.toml' 2>/dev/null || true

Repository: Gitlawb/node

Length of output: 7283

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- manifest and lockfile locations ---'
git ls-files '*Cargo.toml' '*Cargo.lock'
printf '%s\n' '--- relevant dependency entries ---'
rg -n -C 3 'aws-sdk-s3|aws-smithy-runtime-api' --glob 'Cargo.toml' --glob 'Cargo.lock' .
printf '%s\n' '--- surrounding status handling ---'
sed -n '440,490p' crates/gitlawb-node/src/git/tigris.rs
sed -n '3290,3325p' crates/gitlawb-node/src/git/repo_store.rs

Repository: Gitlawb/node

Length of output: 9238

---

🌐 Web query:

aws-smithy-runtime-api 1.11.6 SdkError raw_response ResponseError ServiceError Rust docs

💡 Result:

In the aws-smithy-runtime-api crate (version 1.11.6), SdkError is a core enum used to represent failures during the execution of an SDK request [1][2]. It is defined in the aws_smithy_runtime_api::client::result module [1][3]. The SdkError enum contains several variants, including ResponseError and ServiceError [1][4]: 1. SdkError::ResponseError(ResponseError): This variant occurs when a response is received from the server, but it cannot be parsed according to the expected protocol (e.g., the server closed the connection prematurely) [1][4][5]. It contains the error encountered during parsing and the raw response (R) [6]. 2. SdkError::ServiceError(ServiceError<E, R>): This variant represents an explicit error response returned by the service (e.g., a 404 Not Found or an unauthorized exception from an AWS service) [4][5][7]. It encapsulates the modeled service error (E) and the raw response (R) [6]. The SdkError type is generic over the service error type (E) and the raw response type (R) [1][4]. Most AWS SDK clients re-export this type, typically fixing R to be the appropriate HTTP response type for that specific service [5]. The crate itself is intended for use by the Smithy code generator and library authors, rather than direct use in application code [2].

Citations:

• 1: https://docs.rs/aws-smithy-runtime-api/latest/aws_smithy_runtime_api/client/result/enum.SdkError.html
• 2: https://docs.rs/crate/aws-smithy-runtime-api/1.11.6
• 3: https://docs.rs/aws-smithy-runtime-api/1.11.6/x86_64-unknown-linux-gnu/aws_smithy_runtime_api/client/index.html
• 4: https://github.com/smithy-lang/smithy-rs/blob/56f4c8e9479792b8b52cc394d563ed49a01d1dcc/rust-runtime/aws-smithy-runtime-api/src/client/result.rs
• 5: https://docs.rs/aws-sdk-connect/latest/aws_sdk_connect/error/type.SdkError.html
• 6: https://telematik.prakinf.tu-ilmenau.de/gitlab/pub/smithy-rs/-/blame/e8449bd152ffd6471516a125f565c63dd690c034/rust-runtime/aws-smithy-runtime-api/src/client/result.rs
• 7: https://docs.aws.amazon.com/sdk-for-rust/latest/dg/error-handling.html

---

Classify raw-response errors by HTTP status

SdkError::ResponseError can contain an unparsable 409 or 412 response. Use SdkError::raw_response() to read the status for both response variants; otherwise, RepoWriteGuard::publish skips the supersede retry.

🤖 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/tigris.rs` around lines 185 - 216, Update the
status extraction in the request error handling around UploadPrecondition and
RepoWriteGuard::publish to use SdkError::raw_response() for both service and
response error variants. Ensure unparsable 409 and 412 responses are classified
as lost preconditions so the existing supersede retry remains reachable, while
preserving the current status-based behavior for other errors.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Rebase this branch onto current main before it can be merged
  The current head ea5af98 is not descended from the PR base 241b366 (its merge-base is c926e1e), and GitHub reports the PR as CONFLICTING. A three-way merge conflicts in .env.example, api/repos.rs, error.rs, repo_store.rs, and tigris.rs; the stale head also lacks current-main hardening such as the #174 admission/cleanup path and opaque internal-error handling. Please rebase and resolve these changes, then request a review of the resolved base-to-head diff rather than merging a conflict resolution that can roll those protections back.

• [P1] Do not refresh the live repository outside the write exclusion
  crates/gitlawb-node/src/api/repos.rs:566-572, crates/gitlawb-node/src/git/repo_store.rs:200-227
  The receive-pack advertisement still calls acquire_fresh, which downloads and publishes into local_path. That publish removes and renames the live directory, but it does not take the advisory lock. A second advertisement can therefore replace the directory while a guarded receive-pack, merge, or issue write is using it. In the especially bad ordering where the mutation has finished but release has not compressed the tree, the guarded release uploads the replaced old tree with its still-valid ETag and reports success, losing the accepted write. The new snapshot implementation addresses the close_issue pre-check only; use a non-mutating snapshot for advertisement or coordinate this refresh with the same write exclusion.

• [P1] Bound and authorize the pre-lock issue snapshot
  crates/gitlawb-node/src/api/issues.rs:241-269, crates/gitlawb-node/src/git/tigris.rs:252-304
  Any signed non-owner reaches read_snapshot before the handler establishes authorship or even read access. With Tigris enabled, every such request downloads the entire archive into memory and starts an unbounded blocking extraction into a unique directory; this route has no rate/concurrency limit. Disposable identities can issue parallel close requests for arbitrary issue IDs to exhaust transfer, memory, CPU, and disk. A cancellation while the blocking extraction is running occurs before RepoSnapshot is constructed, so its temp directory is not cleaned up. Require a cheap authorization/author lookup before this work, or explicitly bound and clean up the snapshot work.

• [P1] Classify raw 409/412 responses as a lost conditional write
  crates/gitlawb-node/src/git/tigris.rs:185-215
  The new durability fence extracts a status only from SdkError::ServiceError, but this SDK exposes a raw response for both ServiceError and ResponseError. A Tigris/S3-compatible conditional PUT rejected with an unparsable 409 or 412 is a ResponseError, so this code returns UploadError::Other; RepoWriteGuard::release then only logs it and returns success instead of taking the retry/fenced-503 path. That acknowledges a write whose archive was definitively not published. Use e.raw_response() for the status and cover malformed-body 409/412 responses.

• [P2] Preserve the retryable error for a cold-cache under-lock download failure
  crates/gitlawb-node/src/git/repo_store.rs:513-529
  When the under-lock HEAD succeeds but the GET fails on a node without a local copy, this arm returns the bare download error. The handlers route that through AppError::from, which maps it to a 500, unlike the equivalent acquire_fresh condition that is deliberately wrapped as RepoUnavailable and returned as a retryable 503. Wrap this no-local-fallback error in RepoUnavailable as well.

• [P2] Do not accept a conflicting fork archive as a successful fork
  crates/gitlawb-node/src/git/repo_store.rs:631-650
  The new create-only fork upload treats a lost precondition as success because it assumes a missing DB row proves the object key is absent. Database and object-store writes are not atomic: for example, create_repo initializes and starts its background upload before db.create_repo, so a failed DB insertion can leave a permanent orphan archive. A later fork on a node without that local directory can clone its requested source, lose the If-None-Match upload to the orphan, and still create the DB record; other nodes then fetch the unrelated archive. Surface the conflict/refuse the fork, or make the DB and storage namespace transition coordinated and recoverable.

• [P2] Recompute the lock-acquire remainder before the retry sleep
  crates/gitlawb-node/src/git/repo_store.rs:422-430
left is measured before pg_try_advisory_lock; if that query returns false just before the deadline, the following sleep uses the old remainder and can run a full additional second past the advertised wall-clock acquire cap. Recompute the remaining duration immediately before sleeping, and skip the sleep when it has expired.