[beardthelion]

Closes #283.

The advisory-lock fix this PR opened with is already on main. It landed in 752fd19 and reached main through the PR #173 merge, and #279 is closed, so the framing here has moved on. What is left is the work that grew around that pin, and it addresses a different failure: a write attempt whose future is gone can still land its bytes, and nothing downstream could tell that apart from a committed write.

What changes

A publish attempt has an identity. crates/gitlawb-node/src/git/publish.rs adds PublishAttemptId, PublishStage and PublishStageCell. The id travels in object user metadata and is stamped on the on-disk clone and the database row, so a reader can ask whether what it is looking at is this attempt's work, and how far a cancelled attempt actually got is observable from outside the future performing it.

Object-store writes are fenced on a precondition. Uploads and deletes now carry If-Match against the generation the attempt read, so a late PUT from an abandoned attempt is refused instead of overwriting a newer archive. Neither path carried any precondition before, which is what made "the lock is held, therefore nothing else can be writing" false in the first place.

The local directory swap is fenced on the same authority, which is #283. decompress_repo no longer removes and renames the live tree itself. It goes through swap_extracted_into_validated_repo, which takes the publish lock and then claims a commit token with a true -> false compare-and-exchange. A writer that loses lock ownership revokes that token, both on the explicit refusal path and in RepoWriteGuard::drop, so an extraction already running in an uncancellable spawn_blocking bails and removes its temp directory rather than deleting a tree a later writer is using.

An unrefreshable write sheds instead of writing. A failed archive HEAD on the write paths raises a typed RepoUnavailable that maps to a retryable 503 with a fixed body, rather than reading as "no archive exists" and proceeding.

close_issue authorizes before it takes the lock. It 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. The author fallback reads the issue's git-JSON blob without the lock as a pre-check, and the authoritative check runs again under the guard, because the tree that gets mutated is frequently not the one the pre-check read.

The pin the rest of this follows from

Pinning a connection per in-flight write means writes can no longer share the 20-connection application pool, so they get a dedicated one (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.

Two settled calls

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.

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. The conditional-write path is driven against an S3 mock that actually enforces preconditions, rather than one that records the header.

CI is green on the current head across all eighteen checks.

What this does not close

#282 is the entry-concurrency half, above.

#284 is the remaining cost lever on close_issue, which this branch improves on main without removing: the archive fetch no longer happens with the lock held.

#300 stays open. The two archive-HEAD sites on the write-refresh paths are fixed here, but the cache-miss site that issue actually names still reads a failed HEAD as "no archive", and the read endpoints still fold that into an empty 200.

#302 asks for the test that proves the store-layer refusal and the handler-layer 503 meet. This branch ships each half and not the join.

#205 and #343 are improved rather than closed. Fork attempts now stamp the directory and compensate an orphan archive, and the If-Match work speaks to the missing upload precondition, but neither issue's full scope is covered here.

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

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.

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

• Last reviewed commit: bc00199
• Latest commit, not reviewed: 28f3fc2

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

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 | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly identifies the primary change: fencing repository publication to the write attempt that owns it.
Description check	✅ Passed	The description gives detailed motivation, implementation scope, verification results, known gaps, and linked follow-up issues. It does not use every template heading or checklist, but it is mostly co…
Linked Issues check	✅ Passed	The description includes "Closes #283" and identifies related issues and follow-up work, including #282, #284, #300, and #302.
Out of Scope Changes check	✅ Passed	The changes support the stated repository-write fencing and advisory-lock objectives. The description identifies related work that remains out of scope, and the file summaries show no unrelated change…

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

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[beardthelion]

All four findings are addressed. They shared one root cause, so they got one mechanism rather than four patches: git/publish.rs names the identity and the publication stage of a single write attempt, and every caller decides from it. PublishAttemptId is minted before the request is built and travels with the bytes as object metadata, so "is what the store holds mine?" stays answerable when "did my request succeed?" is not. UploadError now encodes what the client knows about dispatch rather than which status came back, and proves_not_published is a whitelist, so a variant added later defaults to unsafe-to-compensate.

Cancellation before dispatch is a definite refusal. PublishDurabilitySlot::drop maps Idle | PreparingArchive | Refused to UploadFailed and PutDispatched | Ambiguous to UploadUnknowable; publish_durability_confirmed admits only Released, so an unreconciled dispatch no longer licenses the tail. Injecting PreparingArchive => Released fails slot_drop_during_compression_records_a_definite_non_publication and publish_durability_confirmed_refuses_quickly_after_unrecorded_slot_drop, and both pass again on restore. The handler test you asked for, receive_pack_cancelled_during_compression_publishes_nothing_and_runs_no_tail, blocks compression and cancels before the PUT. One caveat: under that mutation it hangs rather than failing cleanly, so I would not call its RED observed.

An unknowable generation is quarantined, not served. release_maybe_compensate writes a .{name}.git.quarantine sidecar next to the tree rather than inside it, and acquire reconciles at the top of the function, ahead of the existing-path fast path. Only the store confirming it holds this attempt's archive lifts it; both "belongs to someone else" and "cannot reach the store" refuse retryably, and the tree is never deleted.

Fork confirmation and cleanup are keyed on the attempt. confirm_fork_repo_row looks up the exact record.id and returns a three-way Ours / Foreign / Absent, so a concurrent row under the same name is Foreign and never claimed. The object is deleted only through delete_if_attempt_matches and the directory only through remove_fork_clone_if_ours, gated on a .{name}.git.fork-attempt sidecar where a missing stamp answers no. Stubbing that ownership check to true fails fork_clone_guard_leaves_a_successors_mirror_alone and fork_recovery_resuming_after_a_successor_deletes_neither_object_nor_path while the confirmation test stays green, which is the blast radius I would expect.

Response loss is no longer compensated as failure. ConstructionFailure is the only pre-dispatch arm; timeout, dispatch and response errors are all ambiguous, as is any future variant. On an ambiguous create-only PUT the fork path asks the store whether the archive is this attempt's, and if it cannot say yes it keeps the clone, keeps the object and refuses retryably instead of dropping the only local copy and fencing the name. a_fork_whose_publish_lost_its_response_is_recovered_not_fenced uses a server that commits the PUT and then loses the response; collapsing the split makes it fail with "a committed publish must not be reported as a failure".

Then I went looking for the same root cause on paths the fix had not reached, and found eight more. All eight are closed here, in three commits on top of the head you reviewed.

Five were the cache contract being applied at sites that each had to be remembered. The swap that replaces a live tree now owns the clear, so a cache-miss download no longer leaves a stale marker fencing the archive it just installed, and the under-lock refresh no longer lifts a quarantine on a HEAD that downloaded nothing. init clears a predecessor's marker. Both of the store's live-path hand-outs go through one confirming helper, so read_snapshot can no longer return an unconfirmed tree as an ordinary snapshot; that gate binds ahead of the lazy-migration spawn, because a quarantined tree backfilled by an IfAbsent PUT would publish exactly the refs the marker exists to withhold. And the marker no longer rests on a single fs::write.

Two were on the fork path. Cleanup claims ownership by renaming the stamp before it touches the tree, so a successor cannot lose its directory to a predecessor's check-then-unlink, and the directory-absent path no longer unlinks a stamp it does not own. A create-only precondition loss whose reconciliation HEAD fails is refused retryably rather than reported as a permanent name conflict, and its clone is released so the retry can re-clone.

The last was the pin sweep reading trees whose publish is unresolved, which is the sidecar marker, distinct from the quarantined row flag.

Two of those eight are worth describing properly, because the first attempt at each was wrong and a cross-family review caught it.

The abandoned tree was settled too late. The settlement ran from RepoWriteGuard::drop, and on a client disconnect that guard is held by the clone riding the admission guard into the detached reaper, so it ran only once the process group had been reaped. Reads take no advisory lock, so for the width of that window a concurrent fetch was served the abandoned refs with no marker beside them. I confirmed it before changing anything: no marker on disk, acquire returning the live path, and the abandoned ref still on the tree it served. Settlement now moves into a token the handler takes before the guard is shared, so the marker lands at the disconnect instant while the reaper still holds the lock.

That fix then allowed two classifiers for one write. release classifies the tree and only then awaits the unlock, while the handler disarmed the token after that await returned, so a cancel in the unlock gap settled a second time. After a definite refusal that is a quarantine marker written beside the tree release had just deleted. Observed by parking at the pre-unlock gate: tree gone, marker present. The arm is now shared between the guard and its token, release clears it above the unlock await, and settle takes it with a compare-exchange, so whichever classifier arrives first is the only one that acts.

Every gap-driving test added here was written and observed failing on its assertion before its fix existed. Each new guard was then checked by injecting the defect it names and confirming the named test goes red for the named reason, including one injection that reverts the settlement split and one that reverts the release-side disarm. Two of those started out proving nothing and were rebuilt rather than accepted: a marker-shadow test that asserted only after the clear that removes the thing it was checking, and tree_settled, whose old proof went slack once the shared arm landed. The reap-window test was also passing for weaker reasons than it claimed, since its fake git took SIGTERM and let the reaper free the lock before the probe read.

The full suite passes on CI.

Four residuals I did not close, so they are not a surprise later:

• A SIGKILL or power loss between the refs landing and the settlement still leaves no marker. Closing that needs eager marking when the tree is first handed out, which is a larger change: an attempt-less marker currently means definite non-publication and would invalidate a tree a live push is writing into, so readers would first have to tell "write in flight" from "write abandoned".
• The in-memory fallback for an unwritable marker is a process-lifetime map keyed by path, evicted only by a clear or a later successful write, and lost on restart while the disk may still be unwritable.
• Discovery may settle a source-less legacy pin row whose only warm holder is publish-unresolved without a retryable signal; the provenance loop sets one, discovery does not. Raised by review, not reproduced by me.
• The source scan that would enumerate every direct reader of the live path is deliberately not here. It is a repo-wide gate rather than one of these gaps, and it belongs in its own change.

[jatmn]

lgtm

[beardthelion]

@kevincodex1 LGTM

[euxarist]

This closes the sharpest blades of the #283 family: the swap no longer self-authorizes (publish lock plus a revocable commit token with CAS, so an abandoned future cannot remove or rename the live tree), and the object-store writes carry If-Match/IfAbsent against the generation the attempt read, tested against an S3 mock that actually enforces preconditions. The timeout-after-dispatch quarantine (never compensating an unknowable outcome) is the right epistemology. Residuals are self-declared and real: #282 entry concurrency stays open, #343/#205 are improved rather than closed, and acquire vs acquire_fresh coverage is not distinguishable in the suite. Merge-order note: this rewrites the receive-pack region and the RepoWriteGuard::release contract that #384 calls, so land it first and make #384 rebase for real.