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

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

[jatmn]

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

Merge readiness

• [P1] Resolve the failing CodeQL security gate
  crates/gitlawb-node/src/git/repo_store.rs:135
  The current d1ce2e3 head has a failing CodeQL check with four high-severity path-expression alerts (repo_store.rs:135 and tigris.rs:440-443). The production call trace appears to route the affected paths through RepoStore::local_path and then validated_repo_disk_path, whose allowlist, rooted join, and component walk reject traversal, so I do not currently have evidence that these annotations are exploitable. The check is nevertheless red and cannot be treated as cleared merely because the Rust analysis job itself passed. Please either make the validation barrier recognizable to the analyzer or triage/dismiss each alert with the complete source-to-sink call-path evidence. Do not suppress the query broadly: the same helper protects exists, remove_dir_all, and rename, so a future unvalidated caller must still be caught.

Findings

• [P1] Keep an abandoned extraction from publishing after its lock is released
  crates/gitlawb-node/src/git/repo_store.rs:477
  The under-lock refresh timeout drops the async download and then drops the guard, but download_to's spawn_blocking extraction cannot be cancelled. The blocking task retains an unconditional right to call decompress_repo, take publish_lock, remove local_path, and rename its extracted tree even though the advisory-lock ownership that authorized that refresh no longer exists.

  The destructive ordering is:

  1. Writer A takes the advisory lock and starts the under-lock refresh.
  2. The refresh exceeds its bound. A returns RepoUnavailable; dropping the guard closes the session and frees the lock.
  3. Writer B acquires the same advisory lock and starts mutating local_path.
  4. A's detached extraction finishes. publish_lock only serializes extraction swaps; B does not hold it.
  5. A removes and replaces B's live directory. B now works in an unlinked tree and can fail unpredictably or report an accepted write whose objects are no longer the live repository.

  The root cause is split ownership: cancellation revokes A's database lock but does not revoke the blocking task's authority to publish into the live path. Fix that ownership boundary rather than only changing timeout values. For example, make extraction two-phase and require a still-valid generation/cancellation token immediately before the swap, or register the pending publisher so a successor cannot enter until it has completed or been disarmed. Keep both existing requirements: the request must remain bounded, and a cancelled task must not retain permission to mutate local_path.

  Add a deterministic regression with a seam immediately before the swap: park A there, expire/drop A and prove B acquires, let B mutate, then release A and assert A cannot replace B's tree. Include the positive control that a non-cancelled extraction still publishes and cleans up normally. A timeout-only assertion is insufficient because it does not observe the late filesystem effect.

• [P1] Do not treat a missing release outcome as confirmed durability
  crates/gitlawb-node/src/api/repos.rs:2539
  A disconnect during release leaves publish_durability as None, but after lock_held_transfer_timeout_secs + 5 this helper returns true. At that point the upload may never have been sent, may have failed, may have timed out with unknowable durability, or may have been refused by the conditional store. The detached tail then proceeds to Pinata mappings, gossip, GraphQL broadcasts, ref-update Arweave anchoring, and peer notifications for a ref whose shared archive was never confirmed.

  The root cause is that one absence value represents two incompatible states: “the legacy/direct test caller has no durability channel” and “the production handler had a channel but disappeared before publishing an outcome.” The outer Option already covers the first case; an inner None is pending/abandoned, not success. The timer currently converts uncertainty into durability to preserve detached-tail liveness, which defeats the new gate's stated safety contract.

  Preserve INV22's required spawn point above release; moving the task below release would reopen the disconnect-loss defect this ordering fixed. Instead, give the detached task an explicit completion signal (Released, known non-durable, or producer-dropped/unknown) and fail closed for every state except Released. If local recovery work intentionally has different semantics from public announcements, model those as separate phases rather than using one boolean to admit the whole remaining tail.

  Add handler-level fault tests for disconnect during the upload and during unlock, plus definite UploadFailed, UploadUnknowable, and Fenced outcomes. Assert that no Pinata mapping, gossip event, GraphQL broadcast, Arweave ref anchor, or peer notification occurs. Pair them with a successful-release positive control proving each effect still occurs exactly once; otherwise a fix that simply disables the tail would pass.

• [P2] Roll back a fork archive when the DB row cannot be created
  crates/gitlawb-node/src/api/repos.rs:3264
  The handler publishes the fork with If-None-Match: * before inserting its DB row. If that PUT succeeds and create_repo then fails, times out, or is cancelled, the archive key remains while no repository row exists. A retry passes the DB precheck, loses the create-only PUT to its own orphan, and maps that 409/412 to RepoExists; because archive deletion has no production caller, the user cannot reuse the name without manual object-store repair.

  The root cause is a non-transactional two-resource commit with no recovery identity. The new create-only fence correctly protects an unrelated archive, but the handler cannot distinguish “someone else owns this key” from “my earlier partial attempt uploaded these exact bytes and failed before DB persistence.” Treating both as RepoExists turns an ordinary transient DB failure into a permanent namespace tombstone.

  Address the partial-commit protocol rather than weakening If-None-Match. Viable outcomes include reserving the DB name before publishing and finalizing it afterward, uploading under an attempt/staging key and promoting only after DB persistence, compensating a successful PUT when the row insert fails, or attaching enough idempotency/generation identity to recognize and resume the same fork safely. Preserve the protection against genuinely unrelated existing archives.

  Add a deterministic fault immediately after the successful PUT and before create_repo: force the DB insert to fail, retry the same fork, and prove the retry can complete without manual cleanup. Also prove that an unrelated pre-existing archive remains protected and that compensation never deletes bytes it cannot attribute to the failed attempt.

• [P3] Recompute the acquire remainder before sleeping
  crates/gitlawb-node/src/git/repo_store.rs:430
left is measured before pg_try_advisory_lock and reused after that query returns false. A query that consumes nearly the entire remaining budget can therefore be followed by a sleep using the stale value, overshooting the advertised wall-clock acquire cap by up to one second. The root cause is treating one budget snapshot as valid across another await; every await can consume the remainder. Recalculate deadline.checked_duration_since(now) immediately before backoff and skip the sleep once it has expired. Add a paused-time or controllable-query regression that returns false just before the deadline and asserts there is no full extra backoff, with an ordinary-contention positive control proving retries still sleep instead of spinning.

Review guidance

This PR has accumulated repeated review rounds because the change is not only a session-affinity repair. It creates a distributed write protocol spanning five authorities that fail independently:

1. PostgreSQL decides who currently holds the advisory lock and whether that ownership survives cancellation.
2. The local filesystem holds the tree being read or mutated, including blocking extraction tasks that outlive their async caller.
3. Tigris holds the cross-node archive generation and can answer success, definite precondition loss, definite failure, or an unknowable timeout.
4. PostgreSQL metadata records repository/issue/PR state separately from object storage.
5. Detached replication publishes secondary effects to IPFS, Pinata, gossip, GraphQL, Arweave, and peers after the request may no longer exist.

The recurring failure mode is that a repair makes one transition locally correct but does not carry its new outcome through every later consumer. Pinning the lock connection made transfer cancellation load-bearing; bounding extraction let filesystem work outlive lock ownership; conditional writes introduced partial-persistence and topology assumptions; typed non-durable outcomes required every detached effect to distinguish success from absence. Tests around the changed function can all pass while the next lifecycle edge remains inconsistent.

Please do one final state-machine pass rather than patching the four visible lines independently:

• Define the authoritative states explicitly: lock not attempted, probe in flight, locked, refresh pending, local mutation landed, publish pending, publish confirmed, publish refused, publish unknowable, unlock pending, released, and abandoned. Avoid booleans or None where those states have different safety rules.
• For every cancellation boundary, identify which capability is revoked and which background work survives. A surviving task must not retain permission to mutate or announce state after the guard that authorized it disappears.
• For every cross-system write, define the partial-success recovery path. In particular, cover object accepted / DB failed, DB accepted / object failed, response lost after success, retry after ambiguity, and cleanup that must not delete another writer's state.
• State deployment assumptions as enforced invariants, not comments. If all writers sharing a Tigris key must also share one PostgreSQL advisory-lock namespace, validate or document that topology and test the failure at its boundary.
• Keep one authoritative mapping for each typed outcome and enumerate every producer and consumer. RepoBusy, RepoUnavailable, RepoWriteFenced, and every ReleaseOutcome should have a table covering HTTP status/body, local-cache state, object-store state, DB effects, replication effects, logging, retry behavior, and cleanup.
• Test production entry points, not only helpers. Drive push, issue create, issue close, PR merge, and fork through contention, HEAD/GET failure, upload failure, upload timeout, first and second precondition loss, unlock failure/cancellation, request disconnect, and DB failure after object publication.
• Make each negative test load-bearing with a positive control. Assert both the refused response and the absence of irreversible effects; then assert the same effects occur exactly once on confirmed success. This prevents a broad fail-closed patch from looking correct merely because it disables work.
• After each repair, search every sibling caller and every stage after the repaired line. The prior rounds repeatedly fixed the named route while a sibling handler, detached tail, local cache, or persistence step retained the old assumption.

A useful final regression matrix would record, for each injected failure and each write route: client response, advisory-lock/session state, live local tree, Tigris generation, DB rows, webhooks/trust/certificates, IPFS/Pinata state, gossip/GraphQL/peer events, retry result, and cleanup. The goal is not more tests by count; it is one end-to-end source of truth showing that every terminal outcome leaves all five authorities in an intentional, recoverable state. That contract-driven pass is the best way to avoid another round where fixing one surfaced edge reveals the next one.

Needs maintainer decision

• Confirm whether separate-Postgres writers can share one Tigris bucket in a supported deployment. If they can, the supersede retry at repo_store.rs:1101 is unsafe: after another node publishes E1, a writer based on E0 can lose If-Match(E0), HEAD E1, and overwrite it under If-Match(E1). If shared-bucket writers always share the same PostgreSQL advisory lock, that sequence is outside the supported topology and should not be reported as a defect.

• Confirm that the deployed PostgreSQL connection budget can support the new defaults: each node can now allocate 48 application connections plus 32 lock connections, multiplied by node count and added to admin/migration usage. The repository does not contain the fleet's max_connections, so this cannot be verified from the PR.

• Run the credential-gated tigris_honors_conditional_writes probe against the same bucket class used in production, or otherwise confirm the backend's current conditional-write guarantees. The fence is now load-bearing, but the repository contains only the mock and an unrun real-provider test, not production evidence.

[beardthelion]

Head 19112915 addresses the d1ce2e3 round.

Abandoned extraction (P1): refresh_swap_authority is revoked on refresh timeout and on guard drop; swap_extracted_into_validated_repo re-checks that token under publish_lock before remove/rename. cargo test -p gitlawb-node --bin gitlawb-node publish_swap (revoked vs non-revoked).

Durability gate (P1): publish_durability_confirmed is fail-closed when the release channel never records an outcome; only ReleaseOutcome::Released admits the Pinata/gossip tail. cargo test -p gitlawb-node --bin gitlawb-node publish_durability plus receive_pack_tail_survives_a_disconnect_during_release.

CodeQL path alerts (P1): sinks route through validated_repo_disk_path / swap_extracted_into_validated_repo; waiting on CodeQL on this head.

Fork partial commit (P2): compensate_fork_archive runs when create_repo fails after a successful create-only PUT; release_after_write now propagates plain upload errors so fork creation does not insert a DB row without an archive.

Acquire backoff (P3): sleep uses deadline.checked_duration_since(now) immediately before backoff.

Also on this head: close_issue rate limiting runs after the read gate (close_issue_rate_limit_runs_after_the_read_gate); create_issue rolls back the local ref via bounded delete_issue_ref when publish refuses (create_issue_rolls_back_local_ref_when_publish_refuses); lock-pool config help points at GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS.

Declined: acquire_write_app_error drift on issues/pulls/receive-pack was already fixed on the prior head; release-side fenced publish uses into_result()? so RepoWriteFenced maps to the fixed 503 body (fenced_publish test).

Maintainer-decision items (shared Postgres/Tigris topology, fleet connection budget, conditional-write probe) are unchanged and still called out in the PR body.

[jatmn]

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

Overall guidance

These findings look separate at their individual call sites, but they come from the same underlying design problem: ownership and commit state are represented locally for only part of each asynchronous operation. The code has several boundaries where work continues after the future that initiated it can time out, disconnect, or return an error—blocking extraction, object-store publication, database insertion, and cleanup. The current fixes generally handle one observed error arm at the caller, but the underlying resource or state has already crossed into another owner by then.

Before another point-fix round, I recommend defining one explicit state machine for each write attempt and making every durable or externally observable artifact belong to that attempt until it reaches a terminal state. At minimum:

1. A repository write should move through staged locally -> object-store outcome resolved -> promoted to the readable local path -> acknowledged. The mutable staging tree should not also be the unlocked read cache. A definite publish refusal should discard or refresh the staging state; an unknowable publish should quarantine it until the object-store generation is resolved.
2. Every background worker must carry an ownership token that remains valid for the exact destructive action it is about to perform. Revocation and commit must be mutually exclusive operations, not an atomic flag checked once before several filesystem mutations.
3. Fork creation is a distributed transaction across local disk, object storage, and PostgreSQL. Give each attempt a durable identity and resumable state, or use an outbox/saga protocol. Do not infer "the insert did not commit" from a transport-level database error, and do not make best-effort deletion the only record that cleanup remains due.
4. Temporary paths should be owned by an RAII guard from creation through the final commit point. Error-specific cleanup arms are too easy to miss; disarm the guard only after the operation has transferred ownership to the database-backed repository or snapshot object.
5. Add deterministic interleaving tests at the ownership boundaries, not only tests for the final enum/status mapping. Each test should pause immediately before the destructive or durable step, trigger timeout/cancellation/failure, admit a retry or successor, and then resume the old work. That is the pattern needed to prove that an abandoned attempt cannot mutate, delete, publish, or leak state afterward.

Addressing those invariants centrally should close the findings below together and reduce the risk of another review round exposing the same class at a different caller.

Findings

• [P1] Synchronize swap revocation with the destructive swap
  crates/gitlawb-node/src/git/repo_store.rs:793
  Failure sequence: the extraction worker takes publish_lock and loads swap_authority == true. Before it reaches remove_dir_all or rename, the outer bounded transfer expires. The timeout arm stores false, returns from acquire_write, and drops the guard, which releases the PostgreSQL advisory lock. A successor can now acquire that repository while the abandoned worker continues deleting and replacing the same live directory. Taking publish_lock only around the worker side does not close this ordering because revocation does not participate in that lock.

  Root cause: authority is modeled as a one-time observation rather than exclusive ownership of the commit operation. The atomic answers whether authority existed at one instant; it does not prevent authority from being revoked between the check and the destructive filesystem steps.

  Guidance: make revocation and commit mutually exclusive under the same synchronization primitive or use a terminal state transition such as Active -> Committing -> Committed versus Active -> Revoked, where only one transition can win. Do not release the advisory lock until revocation has either prevented the commit or observed that the commit already completed. Add a deterministic test that pauses after the worker has observed authority, times out and admits a successor, then resumes the old worker and proves it cannot replace the successor's tree.

• [P1] Keep refused writes out of the unlocked read cache
  crates/gitlawb-node/src/git/repo_store.rs:1260
  Failure sequence: receive-pack, issue close, or PR merge mutates local_path, after which publication returns Fenced or UploadFailed. release converts that outcome to a retryable error but unlocks without restoring or invalidating the directory. RepoStore::acquire returns an existing local path without refreshing Tigris, so same-node reads can observe a write that returned 503 and is definitely absent from the durable archive. For a new archive, the lazy-migration path can later upload that retained tree. UploadUnknowable is a related but distinct state: the PUT may land later, so blindly deleting or serving the tree is unsafe until its generation is resolved. The create-issue-specific rollback is not a general solution because it runs after unlock and can race a successor writer.

  Root cause: local_path is serving as both the transaction's mutable workspace and the shared read cache. Once mutation happens in place, an object-store refusal cannot be represented without leaving contradictory state visible somewhere.

  Guidance: perform writes in an attempt-scoped staging repository and promote it to the readable cache only after publication is confirmed. If an in-place design is retained, preserve a pre-write snapshot and restore or invalidate it under the same write exclusion for definite failures. Quarantine UploadUnknowable state and resolve it with HEAD/generation comparison before either serving it or rolling it back. Centralize this in RepoWriteGuard rather than adding rollback code to each handler. Add tests for push, close, merge, and create that force each release outcome, immediately read from the same node, and then admit a successor write; no refused attempt should be observable or able to overwrite the successor.

• [P1] Resolve ambiguous DB outcomes before deleting a fork archive
  crates/gitlawb-node/src/api/repos.rs:3264
  Failure sequence: the create-only PUT succeeds, PostgreSQL commits the standalone INSERT, and the connection drops before SQLx receives the completion response. create_repo returns Err, so this branch treats the insert as aborted and calls compensate_fork_archive. The archive is deleted even though the repository row exists. Subsequent requests stop at the database name-conflict check, while remote nodes have no durable repository to fetch.

  Root cause: this is a dual write without a transaction protocol. A transport error is being treated as a definitive database outcome, and compensation has no attempt identifier or generation check proving that the archive being deleted belongs to an aborted operation.

  Guidance: give fork creation a durable attempt ID and explicit states such as pending_upload, uploaded, and ready, or write an outbox row before performing the object-store operation. On an ambiguous insert result, query by the attempt/repository ID on a fresh connection before deciding whether to compensate. Preserve the successful PUT generation and condition any deletion on that exact generation so cleanup cannot remove storage owned by a committed or successor state. Test the response-loss case by committing the row and then surfacing a client-side error; recovery must converge to a ready repository rather than deleting its archive.

• [P2] Persist failed fork cleanup instead of tombstoning the name
  crates/gitlawb-node/src/git/repo_store.rs:728
  Failure sequence: the archive upload succeeds, the database insert definitely fails, and this compensating DELETE encounters a transient object-store failure. The error is logged and forgotten, and the only local clone is removed. A retry sees no database row and clones successfully, but its create-only PUT loses to the orphan archive and returns RepoExists. The requested name is then unusable until an operator manually deletes the object.

  Root cause: cleanup is best effort and has no durable owner, retry record, or recovery path. The retry cannot distinguish an unrelated existing repository from the orphan created by its own prior attempt.

  Guidance: persist cleanup work before returning, retry it asynchronously until confirmed, and retain the attempt ID plus uploaded generation. Alternatively, allow a retry carrying the same attempt identity to adopt or conditionally replace its own orphan. A precondition loss should become RepoExists only after proving the object belongs to a different successful attempt. Add a test where DELETE fails once and the same-name retry recovers without operator intervention.

• [P2] Keep snapshot cleanup ownership across the async handoff
  crates/gitlawb-node/src/git/tigris.rs:323
  Failure sequence: the blocking task finishes unpacking, calls mem::forget on SnapshotCleanup, and returns a plain PathBuf. Before the JoinHandle result is polled and wrapped in RepoSnapshot, the request future is cancelled. Dropping the join handle detaches the completed or completing blocking task, and dropping its PathBuf does not remove the directory. No cleanup owner remains for the expanded .tmp-snapshot.* tree, and no startup scavenger removes it.

  Root cause: cleanup ownership ends in the producer before ownership has been accepted by the async consumer. The successful value crossing the task boundary does not itself own the filesystem resource it names.

  Guidance: have the blocking task return an armed cleanup wrapper rather than a PathBuf; the task runtime will then drop and clean that wrapper if the join result is abandoned. Convert or disarm it only when RepoSnapshot takes ownership. If crash recovery is also required, scavenge attempt-namespaced snapshot directories at startup. Add a test that pauses after successful extraction but before the outer future resumes, aborts the outer task, and asserts that the snapshot directory disappears.

• [P2] Make the fork clone attempt-scoped cleanup, not error-arm cleanup
  crates/gitlawb-node/src/api/repos.rs:3246
  Failure sequence: git clone --mirror completes and release_after_write returns UploadError::Other, such as a transient Tigris transport or 5xx failure. Only the PreconditionLost arm removes disk_path; this arm returns while the clone remains and no database row exists. The next request passes the database name check but git clone --mirror fails because the destination already exists.

  Root cause: ownership of the temporary mirror is implicit, and cleanup is attached to one enumerated error variant. Any early return added now or later can strand the same path.

  Guidance: create an RAII attempt guard immediately after choosing disk_path. It should remove the clone on every exit and be disarmed only after the database row has committed and responsibility for the path has transferred to the repository. Keep archive compensation separate because it has a different durable owner and failure protocol. Add a test for an ordinary upload failure followed by an immediate same-name retry, and assert both that the first clone is removed and that the second attempt reaches the upload step.

[beardthelion]

Pushed three follow-up commits on 487687c5 addressing the 2026-08-29 round.

Swap authority and late extraction. try_claim_swap_commit is a CAS on the refresh swap token; revoke_swap_authority runs on timeout and on guard drop. swap_extracted_into_validated_repo refuses when the token is already false. cargo test -p gitlawb-node revoked_publish_swap_cannot_replace_the_live_tree ok.

Definite publish refusal and read cache. RepoWriteGuard::release calls invalidate_local_write_cache on Fenced and UploadFailed, not on UploadUnknowable. a_second_consecutive_loss_refuses_and_never_attempts_a_third ok.

Fork compensation. Ambiguous create_repo errors re-query get_repo and treat an existing row as success. retry_fork_archive_delete retries object-store DELETE with backoff. ForkCloneGuard removes the mirror on every upload failure path (fork_clone_guard_removes_mirror_on_drop ok). Snapshot reads use TempSnapshotDir RAII.

Post-receive tail and publish durability. The tail still spawns above release on push success. It now waits for a recorded release outcome: Released or UploadUnknowable proceed; Fenced and UploadFailed skip all side effects (post_receive_tail_skips_all_work_when_publish_fenced ok). PublishDurabilitySlot writes UploadUnknowable when the handler drops mid-release so a disconnect during upload still runs the tail (receive_pack_tail_survives_a_disconnect_during_release ok).

close_issue rate limit. Separate close_issue_rate_limiter from the push bucket (close_issue_rate_limit_does_not_drain_push_bucket ok).

CodeQL path-injection. ValidatedRepoDiskPath is only constructible from validated_repo_disk_path (inline join plus component walk, matching main). Publish-path sinks take that type.

CI should be running on the new head. Re-requesting review.

[jatmn]

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

Findings

• [P2] Do not skip fork recovery when the confirmation lookup is unavailable
  crates/gitlawb-node/src/api/repos.rs:3342
  Failure sequence: the create-only Tigris upload succeeds, then create_repo returns an error (for example because the connection is lost before its result is received). The recovery lookup is meant to determine whether the insert actually committed, but an unavailable database makes get_repo(...).await? return immediately. That bypasses compensate_fork_archive; ForkCloneGuard removes only the local clone, leaving the uploaded archive under the requested key. Once the database is reachable, a retry sees no row, clones successfully, and then its IfAbsent upload is refused by that orphan, permanently reporting RepoExists until manual cleanup.

  Root cause: the outcome of a cross-system create is inferred from a second fallible database read, while cleanup is attached only to the branch where that read succeeds. The archive has no durable attempt/recovery owner across the upload → database-result-resolution boundary.

  Please make every terminal result of the confirmation step converge: retry or persist the lookup/cleanup work when the DB is unavailable, and only discard the archive after establishing that this attempt did not create a durable row. Keep the successful-response-loss recovery path intact; the goal is recovery from ambiguity, not treating all insert errors as failures.

• [P3] Make the tail outcome handoff cancellation-safe
  crates/gitlawb-node/src/api/repos.rs:2549
  Failure sequence: after a successful receive-pack, the detached replication tail starts polling the shared slot. record() first sets recorded = true, then awaits the mutex before storing the actual ReleaseOutcome. If the request is cancelled in that await, Drop sees recorded and returns without installing UploadUnknowable; likewise, its one-shot try_lock cannot repair the state if the tail owns the mutex at that instant. The slot remains None, so the tail waits the complete upload bound plus five seconds — 305 seconds by default — before it can continue with pinning and announcements.

  Root cause: “a result is being recorded” is represented as a terminal state before the result is durably visible to the consumer. Cancellation can therefore strand the producer/consumer handoff between those two state transitions.

  Please make installation of exactly one terminal outcome cancellation-safe, and wake the consumer from that same state transition. An atomic terminal-state primitive, a sender that is completed before cancellation can intervene, or a drop guard that can reliably publish the fallback outcome would all satisfy the contract. Preserve the existing bounded-upload behavior; this change should remove only the accidental full-timeout delay.

• [P2] Reserve lock-pool capacity for non-push writers
  crates/gitlawb-node/src/config.rs:771
  Failure sequence: with the shipped defaults, 32 distinct git-receive-pack requests are admitted and each pins one of the 32 dedicated lock-pool connections for its refresh, Git work, and publish. create_issue, close_issue, and merge_pr do not consume the push admission permits, but they call the same acquire_write path. They consequently time out acquiring a lock-pool connection and return 503 until a push releases one, despite operating on unrelated repositories.

  Root cause: the new pool’s capacity invariant accounts only for the subset of writers governed by max_concurrent_git_pushes, while the pool is shared by additional write routes. Moving locks out of the main database pool removed the old eight-connection headroom without replacing its admission or reservation policy.

  Please make the capacity model cover every lock-pool consumer: reserve capacity for non-push mutations, put those mutations behind a shared writer budget, or validate the configured pool against the push cap plus the documented headroom. Keep the dedicated lock pool — the required outcome is that a saturated push budget cannot turn ordinary issue and merge mutations into avoidable pool-exhaustion failures.

[beardthelion]

Head 85c6f48b (two commits on top of 487687c5).

[P2] Fork recovery when confirmation lookup is unavailable (repos.rs:3342)
get_repo(...).await? no longer short-circuits past compensation. confirm_fork_repo_row retries the lookup five times with backoff. If the row still cannot be read, we schedule schedule_fork_create_recovery in the background and return AppError::RepoUnavailable (503) instead of propagating the DB error. The successful-response-loss path is unchanged.

[P3] Cancellation-safe tail outcome handoff (repos.rs:2549)
PublishDurabilitySlot::record now writes the mutex slot before setting recorded. Drop on an unrecorded slot still installs UploadUnknowable. Tests: publish_durability_slot_drop_installs_unknowable_when_never_recorded, publish_durability_confirmed_proceeds_quickly_after_unrecorded_slot_drop.

[P2] Lock-pool headroom for non-push writers (config.rs)
Config::validate requires db_lock_pool_max_connections >= max_concurrent_git_pushes + 8. Default lock pool is 40 (was 32). Tests: db_pool_must_clear_the_git_push_cap, lock_pool_size_defaults_to_40_and_rejects_zero.

Also on this head (self-review pass):

• Tail durability gate fails closed when the bounded wait ends with no recorded outcome; only Released or UploadUnknowable admits the tail (publish_durability_confirmed_fails_closed_when_release_never_records). Direction: UploadUnknowable after disconnect/timeout is still deliberate so local-disk tail work can proceed while the handler returns 503.
• pg_advisory_unlock runs under bounded_transfer with lock_held_transfer_timeout, same budget as upload.
• close_issue pre-lock snapshot is bounded by git_acquire_timeout_secs (30s default), not lock_held_transfer_timeout_secs.

Ready for another look.

[jatmn]

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

Findings

• [P1] Do not roll back a created issue after releasing its write guard
  crates/gitlawb-node/src/api/issues.rs:73
release() consumes the guard and releases its advisory-lock session before this failure branch runs. On UploadUnknowable, the PUT may still land and release() deliberately retains the live local tree; a second writer can therefore acquire the same repository before this request executes delete_issue_ref. The older request then runs an unguarded git update-ref -d against the successor's working tree, defeating the new write-serialization contract and potentially deleting a ref from newer state. It also treats an ambiguous upload as a definite failure: the client receives 503 and this node removes the ref locally, while the late conditional PUT can still publish that issue remotely.

  The root cause is attempting per-handler compensation only after the write guard has ended, even though the publication outcome is no longer definitive. Keep any repository mutation serialized with the write attempt, and make the guard-level outcome handling reconcile the local tree with a confirmed object-store result before either rolling it back or exposing a retryable failure. The fix should cover the outcome contract centrally rather than adding another route-specific post-release git mutation.

• [P3] Make the cancellation fallback for the replication tail reliable
  crates/gitlawb-node/src/api/repos.rs:2561
  The detached tail starts polling inner as soon as it is spawned. If the request is cancelled while that polling task owns the mutex, PublishDurabilitySlot::drop's single try_lock() fails and silently returns without installing UploadUnknowable. Nothing retries the installation, so the tail sees None until lock_held_transfer_timeout_secs + 5 elapses, then skips the pinning and announcement work for an otherwise successful receive-pack. The existing test only drops the slot while the mutex is uncontended, so it cannot observe this interleaving.

  The root cause is a producer/consumer terminal-state handoff that is both cancellation-sensitive and best-effort: the producer can disappear between release and the outcome becoming observable. Install exactly one terminal outcome through a cancellation-safe primitive or a drop-safe mechanism that cannot be defeated by transient mutex contention, and notify the consumer from that same transition. Preserve the existing bounded-upload and fail-closed behavior; the required outcome is that a cancelled handler cannot turn an admitted push into a lost detached tail.

[beardthelion]

Addressed the two items on 85c6f48b in 3a82ccb8.

P1 (issues.rs:73) — Issue-ref rollback now runs inside release_compensating while the advisory lock is still held. The handler no longer calls delete_issue_ref after guard.release(). Compensation runs only on definite publish refusal (Fenced / UploadFailed); UploadUnknowable keeps the local tree and returns 503 without mutating git state. create_issue_rolls_back_local_ref_when_publish_refuses covers the definite-refusal path.

P3 (repos.rs:2561) — PublishDurabilitySlot uses a std::sync::Mutex so Drop blocks until it can install the cancellation fallback instead of a one-shot try_lock. mark_release_started() gates synthetic UploadUnknowable: the slot stays empty until release begins, so a tail spawned before release cannot treat pre-release cancellation as unknowable durability. publish_durability_slot_drop_waits_for_contended_mutex, publish_durability_slot_drop_leaves_empty_before_release_starts, and publish_durability_slot_drop_installs_unknowable_when_release_started_but_unrecorded exercise both directions.

Adversarial follow-ups on the same head — Under-lock GET failure now refuses even when a local copy exists (no stale-tree publish on a failed refresh). Same test: acquire_write_refuses_when_the_download_fails_with_a_local_copy.

Checks run locally before push: full cargo test -p gitlawb-node --bin gitlawb-node (1156 passed, 1 ignored), inv22_gates (7/7), pre-push fmt + clippy clean.

[jatmn]

I found four lifecycle issues that need to be addressed before this is ready. They share a common theme: several resources are tracked only by a coarse state such as “release started,” “path exists,” or “row with this name exists,” when correctness depends on the identity and publication stage of one specific write attempt.

Findings

• [P1] Do not treat cancellation before PUT dispatch as publish durability

  crates/gitlawb-node/src/api/repos.rs:2565

  git_receive_pack calls mark_release_started() immediately before awaiting RepoWriteGuard::release(). Release then enters TigrisClient::upload, which first awaits spawn_blocking(compress_repo) and does not construct or send the conditional PUT until compression returns. If the handler is cancelled while that blocking compression is still running, dropping the upload future prevents execution from ever reaching req.send(), so this is a definite “no publication was attempted” state—not an ambiguous in-flight PUT.

  PublishDurabilitySlot::drop nevertheless records every cancellation after mark_release_started() as UploadUnknowable. publish_durability_confirmed accepts that value and lets the detached post-receive tail perform IPFS/Pinata work, P2P announcement, GraphQL publication, Arweave work, and peer notification for refs that exist only in the rejected local write. Because the release future was dropped, the write guard also releases its advisory lock before those effects run.

  The root cause is that the slot records only whether release entered, while the safety decision depends on a later boundary: whether this attempt's PUT was actually dispatched and whether its durable generation was confirmed. Please represent those stages explicitly. A robust design could keep the release operation independently owned until it reports its real outcome, or distinguish at least PreparingArchive, PutDispatched, Published, Refused, and Ambiguous. The replication tail should require Published; an ambiguous dispatch should first be reconciled to this attempt's expected generation rather than being treated as confirmation.

  Please add a regression test that blocks compression, cancels the handler after mark_release_started(), and verifies that no PUT is observed and none of the post-receive effects execute. The existing cancellation test parks after upload, so it cannot exercise this boundary.

• [P1] Quarantine an unknowable generation instead of serving it as normal cache state

  crates/gitlawb-node/src/git/repo_store.rs:1407

  When the bounded release upload expires, release_maybe_compensate correctly avoids deleting the local tree because the PUT may already have landed. However, it leaves that modified tree at the ordinary live path and returns UploadUnknowable, which becomes a 503. Later read requests call RepoStore::acquire; its existing-path fast path returns the live path without checking which object-store generation it represents. If the timed-out PUT was actually rejected, fenced, or never completed, the node can therefore serve the refused refs indefinitely even though durable storage still contains the previous generation. A later write refreshes the path, but ordinary reads do not.

  The root cause is that cache validity is inferred solely from filesystem existence. The live path has no provenance tying it to a confirmed object-store generation, and there is no quarantined state for an attempt whose PUT outcome is unresolved. Please make generation state part of the cache contract. For example, mutate an attempt-specific tree and promote it to the readable live path only after publication is confirmed, or mark the live tree quarantined with its expected generation and force reads to reconcile it with HEAD/object metadata before serving it. Immediate deletion is not a safe fix because it could remove the only local copy of a PUT that did land.

  A focused test should let a PUT consume the request and then remain unresolved or fail, assert that the writer receives 503, and then issue a same-node read. The read must either serve the last confirmed generation or return a retryable refusal; it must not expose the uncertain refs as an ordinary successful read.

• [P1] Bind fork confirmation and cleanup to the exact creation attempt

  crates/gitlawb-node/src/api/repos.rs:3201

  Fork creation uploads the archive and then inserts a freshly generated record.id. If create_repo returns an error, confirm_fork_repo_row looks up only owner/name and treats any matching row as proof that this insert committed. A concurrent ordinary create, mirror registration, or retry can insert a different row under that logical name, after which this request returns 201 using the other attempt's row even though its own uploaded archive, disk path, and fork provenance do not belong to that row.

  The background recovery path has the inverse race. It observes get_repo(owner, name) == None once and then calls compensate_fork_archive, which unconditionally deletes the shared object key and local path. Another creation can commit after the None result but before those deletions, so recovery for the failed fork can erase the succeeding attempt's repository after that attempt has already returned success. Retried object deletion makes the ownership gap persist beyond the initial race window.

  The root cause is using the public owner/name as both lookup key and cleanup authority. It identifies a namespace, not the attempt that owns a DB row, object generation, or filesystem tree. Please carry a durable attempt identity through the whole workflow: confirm the exact record.id, associate the uploaded generation/checksum and disk path with that ID, and make every cleanup conditional on those resources still belonging to the failed attempt. Object deletion needs an If-Match-style generation guard or an equivalent attempt-owned staging/manifest design; a second name lookup immediately before an unconditional delete would only move the race.

  Please exercise both interleavings with barriers: one where another row commits before confirmation, and one where recovery reads None before a successor commits but resumes cleanup afterward. The first request must not claim the successor's row, and the failed attempt must be unable to delete the successor's object or path.

• [P2] Preserve response-loss ambiguity instead of compensating as definite failure

  crates/gitlawb-node/src/git/tigris.rs:185

  TigrisClient::upload recognizes 409/412 precondition loss, but maps every other AWS SDK error to UploadError::Other. The callers then treat Other as proof that publication failed: guarded writes produce UploadFailed and invalidate/compensate local state, while fork creation drops its local clone and skips the DB insert. That classification is not valid for all SdkError variants. Smithy's timeout and dispatch errors allow that the request may have been sent, and a response error means a response was received but could not be parsed. The server can therefore commit the conditional PUT and lose or corrupt the response before the client observes success.

  Fork creation demonstrates the durable failure mode: its create-only PUT can commit, req.send() can return a response-loss error, and ForkCloneGuard then removes the local clone without inserting a DB row. Every retry sends If-None-Match: *, sees the orphan object, and returns RepoExists; the fork name remains unusable until operator cleanup. Guarded issue/push writes also take definite-failure cache and compensation paths despite not knowing whether their generation landed.

  The root cause is that UploadError encodes an HTTP outcome but not the client's knowledge of request dispatch or commit. Please split definite pre-dispatch failures and explicit conditional refusals from possibly-dispatched/response-loss failures. Destructive compensation is safe only for outcomes that prove this attempt did not publish. For ambiguous create-only uploads, reconcile ownership using an attempt identifier plus a stored checksum/generation (or publish through an attempt-owned staging key and atomically claim the logical name) before deciding whether to insert the row or remove the object.

  Please test with a server that accepts the complete PUT and closes or corrupts the response before the SDK can return success. The implementation must recover the committed attempt or leave it safely reconcilable; it must not delete its only local state and permanently fence the logical fork name.