diff --git a/.env.example b/.env.example index b70d1117..6cdb407e 100644 --- a/.env.example +++ b/.env.example @@ -24,11 +24,26 @@ DATABASE_URL=postgresql://gitlawb:changeme@localhost:5432/gitlawb # ── Database pool & startup resilience ──────────────────────────────────── # Maximum connections in the PostgreSQL pool. A cap, not a floor — # connections open lazily. Size against the DB server's max_connections, -# remembering admin tooling opens its own pool. Each concurrent write pins one -# connection for its whole duration (the connection-affine advisory lock), so the -# node REJECTS at boot any value below GITLAWB_MAX_CONCURRENT_GIT_PUSHES + 8 -# headroom — keep this comfortably above that (default 48 for pushes 32). -GITLAWB_DB_MAX_CONNECTIONS=48 +# remembering admin tooling opens its own pool. +GITLAWB_DB_MAX_CONNECTIONS=20 +# Maximum connections in the DEDICATED advisory-lock pool, separate from the +# pool above. Every in-flight repo write pins one connection here for its whole +# duration, so this is a hard ceiling on simultaneous writes node-wide: size it +# to expected peak concurrent writers, not small. Keeping it separate is what +# stops a push burst from starving ordinary request handlers. Budget +# (GITLAWB_DB_MAX_CONNECTIONS + this) per node against the server's +# max_connections, times node count, plus admin tooling. +GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS=32 +# Upper bound, in seconds, on any object-storage transfer that runs while a +# per-repo write lock is HELD (the archive download inside acquire_write and the +# upload inside release). These were free before the lock's connection was +# pinned to the guard; now an unbounded stall holds a lock-pool slot, and enough +# stalls deny every write on the node. The bound applies PER SPAN and there are +# two (the acquire-side refresh, which covers the existence check and download +# together, and the release-side upload), so worst-case slot occupancy is about +# twice this value plus the git work between them. Read it together with the pool +# size above and with GITLAWB_GIT_SERVICE_TIMEOUT_SECS. +GITLAWB_LOCK_HELD_TRANSFER_TIMEOUT_SECS=300 # Seconds a request waits for a pool connection before failing with 503. GITLAWB_DB_ACQUIRE_TIMEOUT_SECS=5 # Upper bound on each startup connect+migrate attempt, in seconds. Keep it diff --git a/crates/gitlawb-node/src/api/ipfs.rs b/crates/gitlawb-node/src/api/ipfs.rs index df7a42db..4edec3b6 100644 --- a/crates/gitlawb-node/src/api/ipfs.rs +++ b/crates/gitlawb-node/src/api/ipfs.rs @@ -1179,9 +1179,13 @@ mod tests { // deterministically. let endpoint = crate::test_support::silent_http_endpoint().await; let tigris = - crate::git::tigris::TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint) - .await; - state.repo_store = crate::git::repo_store::RepoStore::new(repos_dir, Some(tigris), pool); + crate::git::tigris::TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + state.repo_store = crate::git::repo_store::RepoStore::new( + repos_dir, + Some(tigris), + pool, + std::time::Duration::from_secs(300), + ); state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; let mut cfg = (*state.config).clone(); cfg.git_acquire_timeout_secs = 1; @@ -1243,9 +1247,13 @@ mod tests { // 1s timeout (endpoint-pinned test client, no AWS_* env reads). let endpoint = crate::test_support::silent_http_endpoint().await; let tigris = - crate::git::tigris::TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint) - .await; - state.repo_store = crate::git::repo_store::RepoStore::new(repos_dir, Some(tigris), pool); + crate::git::tigris::TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + state.repo_store = crate::git::repo_store::RepoStore::new( + repos_dir, + Some(tigris), + pool, + std::time::Duration::from_secs(300), + ); state .db .upsert_mirror_repo("z6f2acqcont", "ghost", "/unused-ghost", None, false) @@ -1768,9 +1776,13 @@ mod tests { // the budget (endpoint-pinned test client, no AWS_* env reads). let endpoint = crate::test_support::silent_http_endpoint().await; let tigris = - crate::git::tigris::TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint) - .await; - state.repo_store = crate::git::repo_store::RepoStore::new(repos_dir, Some(tigris), pool); + crate::git::tigris::TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + state.repo_store = crate::git::repo_store::RepoStore::new( + repos_dir, + Some(tigris), + pool, + std::time::Duration::from_secs(300), + ); state .db .upsert_mirror_repo("z6f3budget", "ghost", "/unused-ghost", None, false) diff --git a/crates/gitlawb-node/src/api/issues.rs b/crates/gitlawb-node/src/api/issues.rs index 17acf9af..59421555 100644 --- a/crates/gitlawb-node/src/api/issues.rs +++ b/crates/gitlawb-node/src/api/issues.rs @@ -64,14 +64,17 @@ pub async fn create_issue( let guard = state .repo_store .acquire_write(&record.owner_did, &record.name) - .await - .map_err(|e| AppError::Git(e.to_string()))?; + .await?; let disk_path = guard.path().to_path_buf(); let create_result = git_issues::create_issue(&disk_path, &issue_id, &json_str); // Always release the advisory lock — even on error; upload to Tigris only on success. - guard.release(create_result.is_ok()).await; + // A refused publish short-circuits here, before the trust bump and before + // the 201: the issue is on local disk but not in object storage, so no + // other node can read it and the client must retry rather than be told it + // was filed. + guard.release(create_result.is_ok()).await.into_result()?; create_result.map_err(|e| AppError::Git(e.to_string()))?; @@ -222,6 +225,8 @@ pub async fn close_issue( State(state): State, Extension(auth): Extension, Path((owner, repo, issue_id)): Path<(String, String, String)>, + headers: axum::http::HeaderMap, + crate::rate_limit::PeerAddr(peer): crate::rate_limit::PeerAddr, ) -> Result> { let record = state .db @@ -229,44 +234,194 @@ pub async fn close_issue( .await? .ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?; + // Per-IP flood brake, layered on the same shared limiter and trusted-proxy + // policy as the push advertisement. The pre-lock snapshot downloads the + // whole archive and runs a blocking extraction, so an unlimited route would + // let disposable identities drive unbounded transfer/CPU/disk with parallel + // close requests for arbitrary issue ids. Applied before the snapshot work + // so a rejected request does none of it. + if let Some(key) = crate::rate_limit::client_key(&headers, peer, state.push_limiter_trust) { + if !state.push_rate_limiter.check(&key).await { + tracing::warn!(repo = %repo, key = %key, "close_issue rate limited"); + return Err(AppError::TooManyRequests( + "rate limit exceeded — try again later".into(), + )); + } + } + + // READ-GATE before any snapshot work. The author fallback below needs the + // issue blob, which needs the repo tree, so authorship cannot be established + // without a download; but a caller who cannot even READ the repo must be + // stopped here, cheaply, before any Tigris transfer or extraction happens. + // Without this, any signed non-owner could issue parallel close requests for + // arbitrary issue ids and drive unbounded downloads and blocking extraction + // (a disposable-identity DoS), because the route has no other pre-authorization. + // + // mirror-rows-handled: a repo row synced from a peer is stored public and + // carries none of the owner's visibility rules, so for such a row this check + // can only return allow. That is deliberate rather than overlooked, and it is + // the same verdict every other read gate in this API reaches for one. Refusing + // instead would deny the repo's real owner and the issue's real author on any + // node whose only copy of the repo is a synced one, which is an ordinary state + // here, and it would not buy the protection it appears to, because a synced + // row's recorded owner comes from the peer that sent it. The expensive work + // this check guards is bounded ahead of it by the per-IP limiter above, and the + // authoritative owner-or-author decision still runs below and again under the + // write lock. + { + let rules = state.db.list_visibility_rules(&record.id).await?; + let caller = auth.0.as_str(); + if crate::visibility::visibility_check( + &rules, + record.is_public, + &record.owner_did, + Some(caller), + "/", + ) == crate::visibility::Decision::Deny + { + return Err(AppError::RepoNotFound(format!("{owner}/{repo}"))); + } + } + + // AUTHORIZE BEFORE ACQUIRING. The per-repo advisory lock genuinely excludes + // now, so taking it first would hand any caller with read access a way to hold + // that lock on demand and be refused afterwards, while a legitimate writer + // burned its retry budget against it. On a public repo that is every + // permissionless identity. The lock must not be reachable by a caller who is + // about to be refused the write. + let is_owner = crate::api::require_repo_owner(&record, &auth.0).is_ok(); + if !is_owner { + // Not the owner, so the author fallback decides it, and the author lives in + // the issue's git-JSON blob rather than a DB column. + // + // Read it WITHOUT the write lock, from a NON-MUTATING SNAPSHOT. The + // justification is NOT that authorship is immutable — it is not: + // `refs/gitlawb/**` is pushable, so a forged author blob can be pushed + // (tracked separately; it is what makes this fallback only as trustworthy + // as push authorization). The justification is that this read is only a + // PRE-CHECK, deciding whether to take the lock at all. It is NOT the + // authorization decision: `acquire_write` re-downloads the archive after + // locking, so the tree that gets mutated is routinely not this one, and the + // authoritative owner-or-author check runs again under the guard below. + // Refusing here early just keeps a caller who is already visibly + // unauthorized from reaching the lock. + // + // `read_snapshot`, not `acquire_fresh`: acquire's fast path returns as soon + // as the directory exists and never contacts object storage, so on a node + // with a stale copy the author's own issue would be invisible and the + // cannot-establish-authorship arm below would 403 a legitimate author. + // read_snapshot refreshes the same way, but unpacks into a throwaway temp + // dir instead of publishing into the live repo path — an unlocked + // pre-check must not delete or swap the directory under a concurrent + // guarded write on the same path. + let snapshot = tokio::time::timeout( + std::time::Duration::from_secs(state.config.lock_held_transfer_timeout_secs), + state + .repo_store + .read_snapshot(&record.owner_did, &record.name), + ) + .await + .map_err(|_elapsed| { + tracing::warn!( + repo = %repo, + bound_secs = state.config.lock_held_transfer_timeout_secs, + "close_issue snapshot exceeded the transfer bound — shedding as a retryable refusal" + ); + AppError::RepoUnavailable + })??; + let snapshot_path = snapshot.path().to_path_buf(); + + let author_did: Option = match git_issues::get_issue(&snapshot_path, &issue_id) { + Ok(Some(raw)) => serde_json::from_str::(&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. Both arms + // below return None; they are split only so a read failure is visible + // to operators, since a genuinely absent issue and an unreadable one + // are the same answer to the client but not the same event. + Ok(None) => None, + Err(e) => { + tracing::warn!( + repo = %repo, + issue = %issue_id, + err = %e, + "get_issue failed during close_issue authorship pre-check" + ); + None + } + }; + let is_author = author_did + .as_deref() + .is_some_and(|a| crate::api::did_matches(&auth.0, a)); + if !is_author { + return Err(AppError::Forbidden( + "only the repo owner or the issue author can close this issue".into(), + )); + } + } + + // Authorized. Only now is the lock taken. + // Propagate rather than stringify: AppError's From downcasts to + // sqlx::Error so a pool timeout or a database outage surfaces as a retryable + // 503. Calling .to_string() first destroys that and reports both as a 500. let guard = state .repo_store .acquire_write(&record.owner_did, &record.name) - .await - .map_err(|e| AppError::Git(e.to_string()))?; + .await?; let disk_path = guard.path().to_path_buf(); - // Owner OR issue author may close. The author lives in the issue's git-JSON - // blob (not a DB column); a None author (legacy issues) falls back to - // owner-only. Read it under the write guard, before mutating. - let author_did: Option = match git_issues::get_issue(&disk_path, &issue_id) { - Ok(Some(raw)) => serde_json::from_str::(&raw) - .ok() - .and_then(|i| i.author), + // Re-read under the guard and RE-AUTHORIZE against what we read, rather than + // only confirming the issue still exists. The pre-lock read decided whether to + // take the lock; it cannot be the authorization decision, because acquire_write + // re-downloads the archive after locking, so this is frequently a different tree + // than the one the author was read from. Checking existence alone would leave the + // whole decision resting on the earlier read of a tree we are no longer looking + // at. The blob is already in hand here, so this costs a deserialize. + match git_issues::get_issue(&disk_path, &issue_id) { + Ok(Some(raw)) => { + let author_now: Option = serde_json::from_str::(&raw) + .ok() + .and_then(|i| i.author); + let is_author_now = author_now + .as_deref() + .is_some_and(|a| crate::api::did_matches(&auth.0, a)); + if !is_owner && !is_author_now { + // Consumed, NOT propagated, and that is deliberate at all three + // `release(false)` sites below. These release without + // publishing, so there is nothing for the store to refuse, and + // mapping the outcome here would let a 503 shadow the + // authorization answer this route exists to give. + let _ = guard.release(false).await; + return Err(AppError::Forbidden( + "only the repo owner or the issue author can close this issue".into(), + )); + } + } Ok(None) => { - guard.release(false).await; - return Err(AppError::NotFound(format!("issue {issue_id} not found"))); + let _ = guard.release(false).await; + // The owner keeps the informative 404; a non-owner must not learn from + // this route whether the issue exists, matching the pre-check above. + return Err(if is_owner { + AppError::NotFound(format!("issue {issue_id} not found")) + } else { + AppError::Forbidden( + "only the repo owner or the issue author can close this issue".into(), + ) + }); } Err(e) => { - guard.release(false).await; + let _ = guard.release(false).await; return Err(AppError::Git(e.to_string())); } - }; - let is_owner = crate::api::require_repo_owner(&record, &auth.0).is_ok(); - let is_author = author_did - .as_deref() - .is_some_and(|a| crate::api::did_matches(&auth.0, a)); - if !is_owner && !is_author { - guard.release(false).await; - return Err(AppError::Forbidden( - "only the repo owner or the issue author can close this issue".into(), - )); } let close_result = git_issues::close_issue(&disk_path, &issue_id); // Always release the advisory lock — even on error; upload to Tigris only on success. - guard.release(close_result.is_ok()).await; + // Same short-circuit as create_issue, and before the 200 body below. + guard.release(close_result.is_ok()).await.into_result()?; let updated = close_result .map_err(|e| AppError::Git(e.to_string()))? @@ -279,3 +434,339 @@ pub async fn close_issue( Ok(Json(issue)) } + +#[cfg(test)] +mod tests { + use super::*; + use sqlx::PgPool; + + /// U7: once the advisory lock actually excludes, taking it BEFORE authorizing + /// turns close_issue into a wedge primitive. Any caller with repo read access + /// (on a public repo, any permissionless identity) could take the per-repo + /// write lock on demand and be refused the write afterwards, while the owner's + /// push burned its retry budget against a lock held by someone with no write + /// authorization. + /// + /// The observable: hold the lock from an independent session, then call the + /// handler as a stranger. If it authorizes first it refuses immediately; if it + /// acquires first it sits in the 60-attempt retry loop and the deadline fires. + #[sqlx::test] + async fn stranger_is_refused_without_waiting_on_the_write_lock(pool: PgPool) { + use sqlx::Connection; + let opts = (*pool.connect_options()).clone(); + let state = crate::test_support::test_state(pool.clone()).await; + + let owner = "did:key:z6MkU7Owner"; + state + .db + .upsert_mirror_repo("z6MkU7Owner", "u7repo", "/tmp/u7repo", None, true) + .await + .expect("seed repo"); + let record = state + .db + .get_repo("z6MkU7Owner", "u7repo") + .await + .expect("get_repo") + .expect("repo exists"); + + // An independent session holds the repo's write lock for the whole call. + let key = crate::git::repo_store::advisory_lock_key_for_test( + &record.owner_did.replace([':', '/'], "_"), + &record.name, + ); + let mut holder = sqlx::PgConnection::connect_with(&opts).await.unwrap(); + let held: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") + .bind(key) + .fetch_one(&mut holder) + .await + .unwrap(); + assert!( + held.0, + "the test must hold the lock for this to mean anything" + ); + let _ = owner; + + let stranger = crate::auth::AuthenticatedDid("did:key:z6MkU7Stranger".to_string()); + let outcome = tokio::time::timeout( + std::time::Duration::from_secs(3), + close_issue( + axum::extract::State(state.clone()), + axum::Extension(stranger), + axum::extract::Path(( + "z6MkU7Owner".to_string(), + "u7repo".to_string(), + "1".to_string(), + )), + axum::http::HeaderMap::new(), + crate::rate_limit::PeerAddr(Some("203.0.113.64:5000".parse().unwrap())), + ), + ) + .await; + + let refused = outcome.expect( + "a caller with no write authorization must be refused WITHOUT waiting on the \ + write lock; hitting this deadline means the handler tried to acquire first, \ + which is the wedge primitive", + ); + assert!( + matches!(refused, Err(AppError::Forbidden(_))), + "expected 403 Forbidden for a stranger, got {:?}", + refused.err().map(|e| format!("{e:?}")) + ); + } + + /// The read-gate's blind spot, pinned rather than left to be rediscovered. + /// + /// A repo row synced from a peer is stored public and carries none of the + /// owner's visibility rules, so the gate's own inputs can only produce allow + /// for it. The gate above therefore does not carry this class of row, and the + /// test that does cover it (`non_reader_is_refused_before_the_snapshot`) seeds + /// a locally created repo, which cannot observe this: a passing test there is + /// not coverage here. + /// + /// Two things are asserted, and the second is why the first is acceptable. + /// The gate's verdict for such a row is allow for an arbitrary caller, and the + /// handler still refuses that caller afterwards, because the decision that + /// matters is the owner-or-author check rather than this one. If a later change + /// makes the gate the load-bearing decision for this route, the first assertion + /// breaks and this comment is where to start. + #[sqlx::test] + async fn a_synced_row_is_not_gated_by_its_own_visibility(pool: PgPool) { + let state = crate::test_support::test_state(pool.clone()).await; + + // Only a synced row exists for this repo, with no locally created twin. + state + .db + .upsert_mirror_repo("z6MkSyncOwner", "syncrepo", "/tmp/syncrepo", None, false) + .await + .expect("seed synced repo"); // false = not quarantined, the ordinary case + let record = state + .db + .get_repo("z6MkSyncOwner", "syncrepo") + .await + .expect("get_repo") + .expect("repo exists") + .clone(); + assert!( + record.id.contains('/'), + "this test is only meaningful against a synced row; got id {}", + record.id + ); + + // The gate's two inputs, and what they force. + let rules = state + .db + .list_visibility_rules(&record.id) + .await + .expect("list rules"); + assert!(rules.is_empty(), "a synced row carries no rules of its own"); + assert!(record.is_public, "a synced row is stored public"); + assert_eq!( + crate::visibility::visibility_check( + &rules, + record.is_public, + &record.owner_did, + Some("did:key:z6MkSyncStranger"), + "/", + ), + crate::visibility::Decision::Allow, + "the gate can only allow for a synced row, which is the property the \ + handler's mirror-rows-handled note records", + ); + + // So the refusal has to come from the decision that is actually load-bearing. + let stranger = crate::auth::AuthenticatedDid("did:key:z6MkSyncStranger".to_string()); + let outcome = close_issue( + axum::extract::State(state.clone()), + axum::Extension(stranger), + axum::extract::Path(( + "z6MkSyncOwner".to_string(), + "syncrepo".to_string(), + "1".to_string(), + )), + axum::http::HeaderMap::new(), + crate::rate_limit::PeerAddr(Some("203.0.113.99:5000".parse().unwrap())), + ) + .await; + assert!( + outcome.is_err(), + "a stranger must still be refused on a synced row, gate or no gate", + ); + let body = format!("{:?}", outcome.err().unwrap()); + assert!( + !body.contains("syncrepo/") && !body.to_lowercase().contains("issue body"), + "the refusal must not leak repo contents: {body}", + ); + } + + /// The read-gate added for the pre-lock snapshot: a caller who cannot READ + /// the repo (private repo, no rule granting them access) must be refused with + /// a not-found BEFORE any snapshot download or extraction happens. The + /// observable is the refusal itself; the cheaper part (no Tigris work) is + /// structural (the gate precedes the snapshot call in the handler). + #[sqlx::test] + async fn non_reader_is_refused_before_the_snapshot(pool: PgPool) { + let state = crate::test_support::test_state(pool.clone()).await; + let now = chrono::Utc::now(); + state + .db + .create_repo(&crate::db::RepoRecord { + id: uuid::Uuid::new_v4().to_string(), + name: "priv-close".to_string(), + owner_did: "z6MkT3Owner".to_string(), + description: None, + is_public: false, + default_branch: "main".to_string(), + created_at: now, + updated_at: now, + disk_path: "/tmp/priv-close".to_string(), + forked_from: None, + machine_id: None, + }) + .await + .expect("seed private repo"); + + let stranger = crate::auth::AuthenticatedDid("did:key:z6MkT3Stranger".to_string()); + let res = close_issue( + axum::extract::State(state.clone()), + axum::Extension(stranger), + axum::extract::Path(( + "z6MkT3Owner".to_string(), + "priv-close".to_string(), + "1".to_string(), + )), + axum::http::HeaderMap::new(), + crate::rate_limit::PeerAddr(Some("203.0.113.67:5000".parse().unwrap())), + ) + .await; + assert!( + matches!(res, Err(AppError::RepoNotFound(_))), + "a non-reader must be refused as not-found, got {:?}", + res.err().map(|e| format!("{e:?}")) + ); + } + + async fn seed_repo_with_issue( + state: &crate::state::AppState, + owner_slug: &str, + owner_did: &str, + repo: &str, + issue_id: &str, + author_did: &str, + ) -> std::path::PathBuf { + state + .db + .upsert_mirror_repo(owner_slug, repo, "/unused", None, true) + .await + .expect("seed repo row"); + // Seed at the path the HANDLER will resolve. upsert_mirror_repo stores the + // bare slug in owner_did, and close_issue resolves from record.owner_did, so + // seeding from the full did:key would create the repo in a different + // directory and the handler would find nothing. + let record = state + .db + .get_repo(owner_slug, repo) + .await + .expect("get_repo") + .expect("seeded repo exists"); + let _ = owner_did; + let path = state + .repo_store + .acquire(&record.owner_did, &record.name) + .await + .expect("resolve disk path"); + let _ = std::fs::remove_dir_all(&path); + crate::git::store::init_bare(&path).expect("init bare repo"); + // Must deserialize as a real IssueRecord: `created_at` and `status` are + // required, and a parse failure would silently drop the author (the + // `.ok()` on from_str), which reads as a 403 rather than as a broken fixture. + let json = serde_json::to_string(&IssueRecord { + id: issue_id.to_string(), + title: "seeded".to_string(), + body: Some(String::new()), + author: Some(author_did.to_string()), + created_at: chrono::Utc::now().to_rfc3339(), + status: "open".to_string(), + signed_payload: None, + }) + .expect("serialize seeded issue"); + crate::git::issues::create_issue(&path, issue_id, &json).expect("seed issue blob"); + path + } + + /// INV-21(c) positive twin 1: the OWNER can still close. The reorder moved the + /// owner check above the lock, so this is the arm most likely to have broken, + /// and the deny test alone could not see it. + /// + /// The issue is seeded with a THIRD party as its author, deliberately. Seeding + /// the owner as their own author made this test unable to fail: with the owner + /// check disabled, the author fallback granted the close anyway and the test + /// stayed green. Only the owner arm can grant here now. + #[sqlx::test] + async fn owner_can_still_close_after_the_reorder(pool: PgPool) { + let state = crate::test_support::test_state(pool.clone()).await; + let owner_did = "did:key:z6MkT1Owner"; + seed_repo_with_issue( + &state, + "z6MkT1Owner", + owner_did, + "t1repo", + "1", + "did:key:z6MkT1Stranger", + ) + .await; + + let res = close_issue( + axum::extract::State(state.clone()), + axum::Extension(crate::auth::AuthenticatedDid(owner_did.to_string())), + axum::extract::Path(( + "z6MkT1Owner".to_string(), + "t1repo".to_string(), + "1".to_string(), + )), + axum::http::HeaderMap::new(), + crate::rate_limit::PeerAddr(Some("203.0.113.65:5000".parse().unwrap())), + ) + .await; + assert!( + res.is_ok(), + "the owner must still be able to close: {:?}", + res.err().map(|e| format!("{e:?}")) + ); + } + + /// INV-21(c) positive twin 2: the non-owner AUTHOR can still close, through both + /// the pre-lock check and the re-assertion under the guard. + /// + /// It does NOT cover the acquire-vs-acquire_fresh distinction, despite that being + /// the reason the call changed. `RepoStore::for_testing` hardcodes `tigris: None`, + /// which makes `acquire` and `acquire_fresh` identical in every test here, so + /// reverting that line leaves this green. Separating them needs an object-storage + /// seam, which is out of scope for this change and tracked separately. Claiming + /// the coverage here would be worse than admitting the gap. + #[sqlx::test] + async fn issue_author_who_is_not_the_owner_can_still_close(pool: PgPool) { + let state = crate::test_support::test_state(pool.clone()).await; + let owner_did = "did:key:z6MkT2Owner"; + let author_did = "did:key:z6MkT2Author"; + seed_repo_with_issue(&state, "z6MkT2Owner", owner_did, "t2repo", "1", author_did).await; + + let res = close_issue( + axum::extract::State(state.clone()), + axum::Extension(crate::auth::AuthenticatedDid(author_did.to_string())), + axum::extract::Path(( + "z6MkT2Owner".to_string(), + "t2repo".to_string(), + "1".to_string(), + )), + axum::http::HeaderMap::new(), + crate::rate_limit::PeerAddr(Some("203.0.113.66:5000".parse().unwrap())), + ) + .await; + assert!( + res.is_ok(), + "the issue author, who is NOT the repo owner, must still be able to close: {:?}", + res.err().map(|e| format!("{e:?}")) + ); + } +} diff --git a/crates/gitlawb-node/src/api/pulls.rs b/crates/gitlawb-node/src/api/pulls.rs index 26be6109..1ec8fc84 100644 --- a/crates/gitlawb-node/src/api/pulls.rs +++ b/crates/gitlawb-node/src/api/pulls.rs @@ -212,8 +212,7 @@ pub async fn merge_pr( let guard = state .repo_store .acquire_write(&record.owner_did, &record.name) - .await - .map_err(|e| AppError::Git(e.to_string()))?; + .await?; let disk_path = guard.path().to_path_buf(); let merger_did = auth.0; let merge_result = store::merge_branch( @@ -225,7 +224,10 @@ pub async fn merge_pr( ); // Always release the advisory lock — even on error; upload to Tigris only on success. - guard.release(merge_result.is_ok()).await; + // Short-circuit on a refused publish before the PR is marked merged and + // before the webhook fires. Both are irreversible announcements of a merge + // commit that only exists on this node's disk. + guard.release(merge_result.is_ok()).await.into_result()?; let merge_sha = merge_result.map_err(|e| AppError::Git(e.to_string()))?; diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index b09cb6da..187ce7c9 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -692,8 +692,17 @@ pub async fn git_info_refs( git_permit(&state.git_read_semaphore)? }; - // For receive-pack (push), download the latest from Tigris so the client - // sees the same refs that acquire_write() will operate on. + // For receive-pack (push), read the latest from Tigris so the client sees + // the same refs that acquire_write() will operate on. A NON-MUTATING + // snapshot, not `acquire_fresh`: the advertisement runs WITHOUT the advisory + // lock, and acquire_fresh downloads and publishes into the live repo path + // (removing the existing directory and renaming the extract into place), so + // an unlocked advertisement could delete or swap the directory under a + // concurrent guarded write. In the worst ordering the guarded write has + // finished but `release` has not compressed the tree, and the guarded + // release uploads the replaced old tree with its still-valid ETag and + // reports success, losing the accepted write. The snapshot unpacks into a + // throwaway temp dir that is served from and then removed. // // Bound the acquire under `git_acquire_timeout_secs`: the concurrency permit is // already held above, and `git_service_timeout_secs` only starts once git spawns, @@ -703,28 +712,46 @@ pub async fn git_info_refs( // so the shed frees the slot; return a bounded 503. let acquire_deadline = std::time::Duration::from_secs(state.config.git_acquire_timeout_secs); let acquire_fut = async { - if service == "git-receive-pack" { + let res = if service == "git-receive-pack" { state .repo_store - .acquire_fresh(&record.owner_did, &record.name) + .read_snapshot(&record.owner_did, &record.name) .await + .map(|s| (s.path().to_path_buf(), Some(s))) } else { state .repo_store .acquire(&record.owner_did, &record.name) .await - } + .map(|p| (p, None)) + }; + res.map_err(|e| { + if is_expected_transient_acquire_failure(&e) { + tracing::warn!(repo = %name, service = %service, err = %e, "repo acquire failed"); + } else { + tracing::error!(repo = %name, service = %service, err = %e, "repo acquire failed"); + } + // This closure bypasses the `From` chain, so a typed + // refusal would otherwise be stringified into a 500 `git_error`. Route + // just that one case through `From` and leave every other failure on + // exactly today's behavior: this call site also serves the read path via + // `acquire`, whose error vocabulary is out of scope here. + if e.is::() { + AppError::from(e) + } else { + AppError::Git(e.to_string()) + } + }) }; - let disk_path = tokio::time::timeout(acquire_deadline, acquire_fut) + // The snapshot (if any) is kept alive for the whole handler scope below: its + // Drop removes the temp dir it was unpacked into, so dropping it here would + // delete the directory `info_refs` is about to serve from. + let (disk_path, _snapshot_keepalive) = tokio::time::timeout(acquire_deadline, acquire_fut) .await .map_err(|_elapsed| { tracing::warn!(repo = %name, service = %service, "repo acquire timed out; shedding with 503"); AppError::Overloaded("git service acquisition timed out, retry shortly".into()) - })? - .map_err(|e| { - tracing::error!(repo = %name, service = %service, err = %e, "repo acquire failed"); - AppError::Git(e.to_string()) - })?; + })??; // Move the admission permits into the guard so they release only after the spawned // git process group is confirmed reaped, on complete/timeout/disconnect — not the @@ -1273,6 +1300,23 @@ async fn pin_and_encrypt_objects( /// [`smart_http::GitServiceTimeout`] to 504, a malformed client request to 400, /// anything else to a 500 git error. Pure (no logging) so it is unit-testable; /// callers add their own tracing. +/// Acquire failures that are ordinary and transient: lock contention +/// ([`RepoBusy`]) and an under-lock refresh that could not reach object storage +/// ([`RepoUnavailable`]). Both already log at their raise site and both map to a +/// retryable 503, so the handler layer logs them at warn rather than paging. +/// Best-effort, like the database startup classifier: anything this cannot +/// recognize counts as NOT transient and keeps its error-level log. +/// +/// [`RepoBusy`]: crate::git::repo_store::RepoBusy +/// [`RepoUnavailable`]: crate::git::repo_store::RepoUnavailable +fn is_expected_transient_acquire_failure(err: &anyhow::Error) -> bool { + err.downcast_ref::() + .is_some() + || err + .downcast_ref::() + .is_some() +} + fn git_service_app_error(err: &anyhow::Error) -> AppError { if err .downcast_ref::() @@ -1941,7 +1985,11 @@ pub async fn git_receive_pack( AppError::Overloaded("git service acquisition timed out, retry shortly".into()) })? .map_err(|e| { - tracing::error!(repo = %name, err = %e, "acquire_write failed"); + if is_expected_transient_acquire_failure(&e) { + tracing::warn!(repo = %name, err = %e, "acquire_write failed"); + } else { + tracing::error!(repo = %name, err = %e, "acquire_write failed"); + } AppError::Git(e.to_string()) })?; let disk_path = guard.path().to_path_buf(); @@ -2025,7 +2073,12 @@ pub async fn git_receive_pack( // Always release the advisory lock — even on error — to prevent stale locks // from blocking subsequent pushes. Only upload to Tigris when the push // succeeded; uploading a half-applied repo would propagate corruption. - guard.release(push_succeeded).await; + // Short-circuit on a refused publish BEFORE anything downstream observes + // the push. The pack is on local disk but not in object storage, so + // touching the repo, recording the push, bumping trust, issuing ref + // certificates or answering 200 would all be reporting a write no other + // node can read. + guard.release(receive_result.is_ok()).await.into_result()?; // Clean path: clone (a) already dropped inside run_git_service when the receive-pack // group was reaped; clone (b) held here spanned the success-only Tigris upload that // ran inside release() above. Drop it now so a second same-repo push proceeds the @@ -2787,11 +2840,28 @@ pub async fn fork_repo( ))); } - // Upload fork to Tigris + // Upload fork to Tigris. Create-only: a refused precondition means an orphan + // archive already sits under this key (a failed create_repo or another + // writer), and proceeding would create a DB record whose archive is shadowed + // by bytes that are not this fork. Refuse rather than accept a fork other + // nodes would fetch as unrelated content. The local clone is cleaned up on + // this path by the caller's error return dropping the handler state. state .repo_store .release_after_write(&forker_did, &fork_name) - .await; + .await + .map_err(|e| match e { + crate::git::tigris::UploadError::PreconditionLost { status } => { + tracing::warn!( + forker = %forker_did, + fork = %fork_name, + status, + "fork refused: an archive already exists under the fork's key" + ); + AppError::RepoExists(fork_name.clone()) + } + other => AppError::Git(format!("fork upload failed: {other}")), + })?; let now = Utc::now(); let record = crate::db::RepoRecord { @@ -3228,6 +3298,26 @@ mod tests { assert!(git_permit(&sem).is_ok()); } + #[test] + fn is_expected_transient_matches_both_typed_refusals() { + // The real raise shape wraps the marker in a `.context()` layer naming the + // owner slug and repo, so the downcast has to survive that wrapping. + let busy = anyhow::Error::new(crate::git::repo_store::RepoBusy) + .context("another write is in progress for alice/demo"); + assert!(is_expected_transient_acquire_failure(&busy)); + + let unavailable = anyhow::Error::new(crate::git::repo_store::RepoUnavailable) + .context("could not read the archive HEAD for alice/demo"); + assert!(is_expected_transient_acquire_failure(&unavailable)); + } + + #[test] + fn is_expected_transient_rejects_unrelated_failures() { + // Anything the classifier cannot recognize keeps paging at error level. + let other = anyhow::anyhow!("disk on fire"); + assert!(!is_expected_transient_acquire_failure(&other)); + } + fn repo_owned_by(owner_did: &str) -> crate::db::RepoRecord { let now = chrono::Utc::now(); crate::db::RepoRecord { diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index fefa063e..da6bec38 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -249,6 +249,46 @@ pub struct Config { )] pub db_max_connections: u32, + /// Maximum connections in the dedicated advisory-lock pool, which is separate + /// from the main pool above. + /// + /// Size this against the expected peak number of concurrent distinct-repo + /// writers, NOT small. Every in-flight repo write pins one connection here for + /// its whole duration (the write, its metadata tail, and the bounded archive + /// upload), so this value is a hard ceiling on simultaneous writes node-wide. + /// Keeping it separate from GITLAWB_DB_MAX_CONNECTIONS is what stops a push + /// burst from starving ordinary request handlers; the cost is that + /// (main pool + lock pool) must fit inside the database server's + /// max_connections, times the number of nodes, plus admin tooling. + #[arg( + long, + env = "GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS", + default_value_t = 32, + value_parser = clap::value_parser!(u32).range(1..) + )] + pub db_lock_pool_max_connections: u32, + + /// Upper bound, in seconds, on any single object-storage transfer that runs + /// while the per-repo advisory lock is HELD. + /// + /// Two bounded spans exist, and the bound applies per span. The acquire-side + /// refresh in `acquire_write` covers the existence HEAD and the download + /// together under ONE budget (it runs after the lock is taken and before the + /// guard is constructed), and the archive upload in `release` gets its own. + /// Worst-case slot occupancy is therefore about twice this value plus the git + /// work between them, not one times this value. Both used to be free, because the lock's + /// connection was returned to the pool immediately; now that a write guard + /// pins a lock-pool connection for its whole lifetime, an unbounded transfer + /// holds that slot, and enough stalled transfers deny every write on the node. + /// This is the bound that keeps a stall from becoming an outage. + #[arg( + long, + env = "GITLAWB_LOCK_HELD_TRANSFER_TIMEOUT_SECS", + default_value_t = 300, + value_parser = clap::value_parser!(u64).range(1..) + )] + pub lock_held_transfer_timeout_secs: u64, + /// Maximum time a request waits for a pool connection before failing with /// 503, in seconds. Bounds queueing when the database is slow or down. #[arg( @@ -591,6 +631,25 @@ impl Config { mod tests { use super::*; + #[test] + fn lock_pool_size_defaults_to_32_and_rejects_zero() { + assert_eq!( + Config::parse_from(["gitlawb-node"]).db_lock_pool_max_connections, + 32 + ); + assert_eq!( + Config::parse_from(["gitlawb-node", "--db-lock-pool-max-connections", "8"]) + .db_lock_pool_max_connections, + 8 + ); + // A zero-sized lock pool would deny every write, so clap must reject it + // rather than let a node boot into a state where no repo can be written. + assert!( + Config::try_parse_from(["gitlawb-node", "--db-lock-pool-max-connections", "0"]) + .is_err() + ); + } + #[test] fn git_service_timeout_defaults_to_600_and_rejects_zero() { assert_eq!( diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 50c3bdda..4fbf7e98 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -250,11 +250,6 @@ pub struct Db { } impl Db { - /// Access the underlying Postgres connection pool. - pub fn pool(&self) -> &PgPool { - &self.pool - } - #[cfg(test)] pub fn for_testing(pool: PgPool) -> Self { Self { pool } @@ -295,6 +290,46 @@ impl Db { Ok(db) } + /// Build the dedicated pool that advisory-lock connections come from. + /// + /// Deliberately **lazy**: connections open on first use rather than at boot. + /// The main pool has to connect eagerly because it runs migrations, which is + /// why it needs `connect_db_with_retry`'s backoff and degraded-server + /// handoff. This pool has no startup work at all, so an eager connect would + /// only add a new way for the process to fail to boot, and would need a + /// second copy of that retry machinery to be safe. Being lazy removes the + /// failure mode instead of handling it: if Postgres is unreachable when the + /// first write arrives, that write fails on the pool's own acquire timeout, + /// the same way any other database-backed request already does. + /// + /// Kept separate from the main pool so a burst of lock-holding connections + /// cannot starve ordinary request handlers. See + /// `GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS` for the sizing tradeoff. + pub fn lock_pool( + database_url: &str, + max_connections: u32, + acquire_timeout: Duration, + ) -> Result { + info!( + max_connections, + acquire_timeout_secs = acquire_timeout.as_secs(), + "creating dedicated advisory-lock pool (lazy)" + ); + PgPoolOptions::new() + .max_connections(max_connections) + // Explicit, and load-bearing rather than cosmetic: `RepoWriteGuard::Drop` + // has a no-runtime branch that calls `PoolConnection::leak()` and relies + // on the husk's own drop doing nothing. With `min_connections > 0` that + // drop still spawns a pool-replenish task, and spawning without a runtime + // panics inside Drop, which aborts the process during unwind. It is 0 by + // default, so this line exists to keep a future tuning change from + // silently re-arming that panic. + .min_connections(0) + .acquire_timeout(acquire_timeout) + .connect_lazy(database_url) + .context("creating advisory-lock pool") + } + /// Cheap liveness probe against the pool, for readiness checks: one /// `SELECT 1` that fails fast when the database is unreachable. pub async fn ping(&self) -> Result<()> { diff --git a/crates/gitlawb-node/src/error.rs b/crates/gitlawb-node/src/error.rs index f5e14df1..2ffb8861 100644 --- a/crates/gitlawb-node/src/error.rs +++ b/crates/gitlawb-node/src/error.rs @@ -59,6 +59,15 @@ pub enum AppError { #[error("server overloaded: {0}")] Overloaded(String), + #[error("repository is busy")] + RepoBusy, + + #[error("repository is temporarily unavailable")] + RepoUnavailable, + + #[error("repository write was fenced by a concurrent publish")] + RepoWriteFenced, + #[error("database error: {0}")] Db(#[from] sqlx::Error), @@ -100,7 +109,27 @@ impl From for AppError { fn from(err: anyhow::Error) -> Self { match err.downcast::() { Ok(sql) => AppError::Db(sql), - Err(err) => AppError::Internal(err), + // Lock contention is transient and ordinary, so it must not land as a + // 500. The internal message names the owner slug and repo, so the + // variant carries nothing: the detail stays in the log at the raise + // site and the client gets a fixed retryable body. + Err(err) => match err.downcast::() { + Ok(_) => AppError::RepoBusy, + // Same reasoning one rung down: a refused under-lock refresh is a + // transient storage condition, and its internal message names the + // owner slug and repo, so the variant carries nothing. + Err(err) => match err.downcast::() { + Ok(_) => AppError::RepoUnavailable, + // And one more rung: a publish the store refused twice is + // transient in the same way, and the retry is the client's + // to make. The variant carries nothing for the same reason + // as the two above. + Err(err) => match err.downcast::() { + Ok(_) => AppError::RepoWriteFenced, + Err(err) => AppError::Internal(err), + }, + }, + }, } } } @@ -165,6 +194,29 @@ impl IntoResponse for AppError { // 504, distinct from the 500 git_error and from the read-gate's 404 / // the auth 401, so the client can tell a deadline from a failure. AppError::Timeout(msg) => (StatusCode::GATEWAY_TIMEOUT, "git_timeout", msg.clone()), + // 503 with a FIXED body: the caller should retry, and must not be told + // which repo is contended or for how long. + AppError::RepoBusy => ( + StatusCode::SERVICE_UNAVAILABLE, + "repo_busy", + "repository is busy — retry".into(), + ), + // 503 with a FIXED body for the same reason: the caller should retry, and + // must not be told which repo could not be refreshed or why. + AppError::RepoUnavailable => ( + StatusCode::SERVICE_UNAVAILABLE, + "repo_unavailable", + "repository is temporarily unavailable, retry".into(), + ), + // 503 with a FIXED body again, and its own code: the caller should + // retry, but the condition is not contention, so a client that + // distinguishes them should be able to. The body must not say which + // repo lost its publish or to whom. + AppError::RepoWriteFenced => ( + StatusCode::SERVICE_UNAVAILABLE, + "repo_write_fenced", + "repository changed underneath this write, retry".into(), + ), AppError::Db(e) if db_unavailable(e) => ( StatusCode::SERVICE_UNAVAILABLE, DB_UNAVAILABLE_CODE, diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 2aef6ff0..6c5ee9e3 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -11,23 +11,32 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; use std::sync::Arc; +use std::time::Duration; use anyhow::{Context, Result}; -use sqlx::pool::PoolConnection; -use sqlx::{PgPool, Postgres}; +use sqlx::PgPool; use tokio::sync::Mutex; use tracing::{debug, info, warn}; use super::store; -use super::tigris::TigrisClient; +use super::tigris::{TigrisClient, UploadError, UploadPrecondition}; /// Centralized repo storage: local disk cache + optional Tigris backend. #[derive(Clone)] pub struct RepoStore { repos_dir: PathBuf, tigris: Option, - /// Shared Postgres pool for advisory locks. - pool: PgPool, + /// Dedicated Postgres pool that advisory-lock connections come from, kept + /// separate from the pool serving ordinary request handlers. Each write guard + /// pins one connection here for its whole lifetime, so a push burst consumes + /// this pool rather than starving application queries. Sized by + /// `GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS`. + lock_pool: PgPool, + /// Bound on any object-storage transfer that runs while the lock is HELD. + lock_held_transfer_timeout: Duration, + /// Wall-clock cap on WAITING for the lock. A field rather than a bare const so + /// the busy path can be driven in a test without a 90s wait. + lock_acquire_deadline: Duration, /// Tracks repos already confirmed to exist in Tigris — avoids redundant /// HEAD checks and background uploads for repos we've already migrated. migrated: Arc>>, @@ -41,16 +50,37 @@ pub struct RepoStore { impl RepoStore { #[cfg(test)] - pub fn for_testing(repos_dir: PathBuf, pool: PgPool) -> Self { + pub fn for_testing(repos_dir: PathBuf, lock_pool: PgPool) -> Self { Self { repos_dir, tigris: None, - pool, + lock_pool, + lock_held_transfer_timeout: Duration::from_secs(300), + lock_acquire_deadline: LOCK_ACQUIRE_DEADLINE, migrated: Arc::new(tokio::sync::Mutex::new(std::collections::HashSet::new())), pre_unlock_gate: None, } } + /// Same as [`RepoStore::for_testing`] but with Tigris enabled, so the paths + /// that only run when a backend is configured are reachable in a test. + #[cfg(test)] + pub fn for_testing_with_tigris( + repos_dir: PathBuf, + lock_pool: PgPool, + tigris: TigrisClient, + ) -> Self { + Self::new(repos_dir, Some(tigris), lock_pool, Duration::from_secs(300)) + } + + /// Shorten the lock-acquire deadline so the busy path is reachable in a test + /// without waiting out the production default. + #[cfg(test)] + pub fn with_lock_acquire_deadline(mut self, deadline: Duration) -> Self { + self.lock_acquire_deadline = deadline; + self + } + /// Test-only: every guard from this store parks in `release` right before the /// `pg_advisory_unlock` await, until `gate` is notified. Dropping the future /// while it is parked reproduces a client disconnect inside `release`. @@ -60,11 +90,18 @@ impl RepoStore { self } - pub fn new(repos_dir: PathBuf, tigris: Option, pool: PgPool) -> Self { + pub fn new( + repos_dir: PathBuf, + tigris: Option, + lock_pool: PgPool, + lock_held_transfer_timeout: Duration, + ) -> Self { Self { repos_dir, tigris, - pool, + lock_pool, + lock_held_transfer_timeout, + lock_acquire_deadline: LOCK_ACQUIRE_DEADLINE, migrated: Arc::new(Mutex::new(HashSet::new())), #[cfg(test)] pre_unlock_gate: None, @@ -99,11 +136,34 @@ impl RepoStore { } Ok(false) => { info!(repo = %name, "migrating local repo to tigris"); - if let Err(e) = tigris.upload(&slug, &name, &path).await { - warn!(repo = %name, err = %e, "lazy migration to tigris failed"); - return; + // Create-only. This backfill was decided on a + // negative existence check that is already + // stale, so a refusal means someone else + // published this key in between and dropping + // our bytes is the correct outcome. An + // unconditional PUT here would overwrite their + // archive, which is the exact bug this fence + // exists to close. + match tigris + .upload(&slug, &name, &path, UploadPrecondition::IfAbsent) + .await + { + Ok(()) => { + info!(repo = %name, "lazy migration to tigris complete"); + } + // Logged apart from the warn arm below so a + // refusal, which is the fence working, does + // not read as a storage failure. The key is + // populated either way, so this still + // counts as migrated. + Err(UploadError::PreconditionLost { status }) => { + info!(repo = %name, status, "lazy migration dropped: another writer already published this repo"); + } + Err(e) => { + warn!(repo = %name, err = %e, "lazy migration to tigris failed"); + return; + } } - info!(repo = %name, "lazy migration to tigris complete"); } Err(e) => { warn!(repo = %name, err = %e, "tigris existence check failed"); @@ -139,35 +199,63 @@ impl RepoStore { Ok(local_path) } - /// Ensure a repo is available on local disk with the **latest** Tigris state. - /// Use this for operations that precede a write (e.g. `info/refs` for - /// `git-receive-pack`) so the client sees the same refs that `acquire_write()` - /// will operate on. - pub async fn acquire_fresh(&self, owner_did: &str, repo_name: &str) -> Result { + /// Non-mutating snapshot of a repo's **latest** Tigris state, for reads that + /// must see fresh data but must NOT write into the live repo path. + /// + /// The fresh-acquire form this replaced (`acquire_fresh`) downloaded and + /// PUBLISHED into the live directory (removing the existing dir and renaming + /// the extract into place); this unpacks into a throwaway temp dir and + /// returns it. The live path is never touched, so an unlocked caller cannot + /// delete or swap the directory under a concurrent guarded write. + /// + /// The returned snapshot owns its temp dir and removes it on drop; when + /// there is no Tigris backend (or no archive), the snapshot borrows the live + /// local path and owns nothing. A HEAD failure refuses rather than guessing, + /// matching the under-lock refresh path: a transient storage blip must be a + /// retryable refusal (`RepoUnavailable`), not a 500 or a silently stale read. + pub async fn read_snapshot(&self, owner_did: &str, repo_name: &str) -> Result { let (owner_slug, local_path) = self.local_path(owner_did, repo_name)?; if let Some(ref tigris) = self.tigris { - if tigris.exists(&owner_slug, repo_name).await.unwrap_or(false) { - debug!(repo = %repo_name, "acquire_fresh: downloading latest from tigris"); - if let Err(e) = tigris.download(&owner_slug, repo_name, &local_path).await { - // The Tigris archive is present (HEAD ok) but unreadable — a - // corrupt/partial upload, or a transient GET failure. If we have a - // valid local copy, proceed with it rather than blocking the write; - // the post-write upload re-syncs (self-heals) Tigris. Only hard-fail - // when there is no local copy to fall back to. - if local_path.exists() { - warn!(repo = %repo_name, err = %e, - "acquire_fresh: tigris download failed — falling back to local copy"); - return Ok(local_path); - } - return Err(e).context("downloading repo from tigris (fresh)"); + match tigris.exists(&owner_slug, repo_name).await { + Ok(true) => { + // Snapshot form: unpack into a temp dir, never the live path. + // + // Cancellation cleanup: the extraction runs in a + // `spawn_blocking` that cannot be aborted, so a dropped + // future (client disconnect, a bounded-transfer timeout) + // still leaves the temp dir on disk. The cleanup has to live + // in the ASYNC layer, armed for the whole download await and + // disarmed only when `RepoSnapshot` takes ownership. + let snapshot = tigris + .download_to(&owner_slug, repo_name, &local_path, false) + .await + .map_err(|e| { + anyhow::Error::new(RepoUnavailable).context(format!( + "tigris snapshot download failed during read_snapshot for {owner_slug}/{repo_name}: {e:#}" + )) + })?; + return Ok(RepoSnapshot { + path: snapshot.clone(), + owned: true, + }); + } + Ok(false) => {} + Err(e) => { + warn!(repo = %repo_name, err = %e, + "read_snapshot: tigris HEAD failed — refusing rather than guessing the archive is absent"); + return Err(anyhow::Error::new(RepoUnavailable).context(format!( + "tigris HEAD failed during read_snapshot for {owner_slug}/{repo_name}" + ))); } - return Ok(local_path); } } - // Tigris disabled or repo not in Tigris — fall back to local - Ok(local_path) + // Tigris disabled or repo not in Tigris — fall back to local. + Ok(RepoSnapshot { + path: local_path, + owned: false, + }) } /// Take a write lock (Postgres advisory lock), ensure repo is local, return guard. @@ -203,76 +291,284 @@ impl RepoStore { let (owner_slug, local_path) = self.local_path(owner_did, repo_name)?; let lock_key = advisory_lock_key(&owner_slug, repo_name); - // Pin a dedicated pooled connection and build the guard holding it BEFORE - // issuing the lock query. Session-level pg advisory locks are - // connection-affine (they can only be released on the session that took - // them), so the guard must own the locking connection; and building the - // guard first means any cancellation after the lock is taken — a - // `tokio::time::timeout` firing during the Tigris download below — drops a - // guard that CAN release, closing the leak the outer timeout otherwise - // opened (#174 F1). - let conn = self - .pool - .acquire() - .await - .context("acquiring db connection for the write advisory lock")?; + // Take the lock on a connection this guard will own for its whole + // lifetime, so the release runs on the same session. `pg_try_advisory_lock` + // with retry rather than a blocking acquire, so a stale lock from a crashed + // connection cannot wedge us indefinitely. + // + // Each attempt checks a connection out and, on failure, returns it BEFORE + // sleeping: a writer spinning on a contended repo must not pin a lock-pool + // slot through its backoff, or a handful of spinners would starve the pool + // for everyone else. + // + // Pool exhaustion is a DIFFERENT condition from "someone else holds the + // lock" and is not retried here. Retrying it would burn all 60 attempts + // against a pool that is full for reasons unrelated to this repo, and would + // report a capacity problem as lock contention. It surfaces immediately with + // its own message instead. + // Cap the WALL CLOCK of the WAIT, not just the attempt count. 60 attempts + // each pay a pool acquire (up to db_acquire_timeout_secs) plus a 1s sleep, + // so an attempt-only bound reaches ~360s. This bounds the wait only; the + // under-lock refresh below carries its own separate bound, so do not read + // this as a total for `acquire_write` (see LOCK_ACQUIRE_DEADLINE). + let deadline_budget = self.lock_acquire_deadline; + let deadline = std::time::Instant::now() + deadline_budget; + let mut lock_conn = None; + for attempt in 0..60 { + // The advertised cap is WALL CLOCK, so the remaining budget must bound + // every await in the loop, not just the sleep between attempts. A pool + // checkout or a slow advisory query that starts just before the deadline + // and lands after it would otherwise hold the write task past the budget + // it was promised, which is exactly what the deadline exists to prevent. + let left = match deadline.checked_duration_since(std::time::Instant::now()) { + Some(left) if !left.is_zero() => left, + _ => break, + }; + // Bound the pool checkout by the remaining budget. A checkout that + // would outlive the deadline is not worth starting: it either waits out + // the full DB acquire timeout and fails anyway, or lands a connection + // with no budget left to use it. + let conn = match tokio::time::timeout(left, self.lock_pool.acquire()).await { + Ok(Ok(c)) => c, + Ok(Err(e)) => { + // Saturation is surfaced HERE, in the request path, and + // deliberately not through /ready. Failing readiness on a full + // pool would pull this node out of routing, taking its reads + // with it and pushing its write load onto peers carrying the + // same load — the documented downward spiral. So the signals + // are: a retryable 503 to the caller (via the sqlx downcast on + // this error) and this log line for the operator. + // + // Logged at warn with the pool's own counters so an incident can + // tell "the pool is full" from "the database is gone" without + // reproducing it. Once per failed acquire, and a failed acquire + // already costs a multi-second timeout, so this cannot itself + // become a log flood. + warn!( + repo = %repo_name, + owner = %owner_slug, + pool_size = self.lock_pool.size(), + pool_idle = self.lock_pool.num_idle(), + err = %e, + "advisory-lock pool acquire failed — writes are being shed; \ + raise GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS or investigate long-held write locks" + ); + return Err(e).context("advisory-lock pool exhausted or unreachable"); + } + Err(_) => { + // The pool checkout itself outlived the remaining budget. Same + // refusal as running out of attempts: the wall-clock cap is what + // is advertised, so a checkout that blows past it is contention + // the caller was promised would not happen. + warn!( + repo = %repo_name, + owner = %owner_slug, + waited_secs = deadline_budget.as_secs(), + "advisory-lock pool checkout exceeded the acquire deadline — shedding the write as busy" + ); + return Err(anyhow::Error::new(RepoBusy).context(format!( + "advisory-lock pool checkout exceeded the {}s deadline for {owner_slug}/{repo_name}", + deadline_budget.as_secs() + ))); + } + }; + // Bound the advisory query by the remaining budget too: a query that + // starts with budget left but answers after the deadline must not be + // accepted, or the cap is only as good as the fast path. + let left = match deadline.checked_duration_since(std::time::Instant::now()) { + Some(left) if !left.is_zero() => left, + _ => break, + }; + let mut probe = LockProbe::new(conn); + let acquired = match tokio::time::timeout(left, probe.try_lock(lock_key)).await { + Ok(Ok(acquired)) => acquired, + Ok(Err(e)) => return Err(e).context("trying advisory lock"), + Err(_) => { + // The query outlived the remaining budget. The probe's Drop + // closes its session, which cannot hold the lock it never + // confirmed taking, so this is a plain shed. + warn!( + repo = %repo_name, + owner = %owner_slug, + waited_secs = deadline_budget.as_secs(), + "advisory-lock query exceeded the acquire deadline — shedding the write as busy" + ); + return Err(anyhow::Error::new(RepoBusy).context(format!( + "advisory-lock query exceeded the {}s deadline for {owner_slug}/{repo_name}", + deadline_budget.as_secs() + ))); + } + }; + if acquired { + lock_conn = probe.take_conn(); + break; + } + // Not acquired, and nothing is locked, so hand the connection back + // before the backoff rather than holding a slot while idle. + drop(probe); + // Clamp the backoff to what is left of the budget: sleeping a full + // second past the deadline would turn a short deadline into a longer + // wait than the caller was promised. + if attempt < 59 { + tokio::time::sleep(left.min(std::time::Duration::from_secs(1))).await; + } + } + let Some(lock_conn) = lock_conn else { + // Contention is transient, so this must NOT land as a 500. The detail + // (which repo, which key, how long) goes to the log; the client gets a + // retryable 503 with a fixed body via the `RepoBusy` downcast. + warn!( + repo = %repo_name, + owner = %owner_slug, + lock_key, + waited_secs = deadline_budget.as_secs(), + "advisory lock not acquired within the deadline — shedding the write as busy" + ); + return Err(anyhow::Error::new(RepoBusy).context(format!( + "could not acquire advisory lock within {}s for {owner_slug}/{repo_name}", + deadline_budget.as_secs() + ))); + }; + // From here the lock is HELD. Any early return must not simply drop the + // connection back into the pool, so it is handed to the guard immediately + // below and every exit after this point goes through the guard. let mut guard = RepoWriteGuard { owner_slug: owner_slug.clone(), repo_name: repo_name.to_string(), local_path: local_path.clone(), lock_key, - conn: Some(conn), - locked: false, - released: false, + conn: Some(lock_conn), tigris: self.tigris.clone(), + lock_held_transfer_timeout: self.lock_held_transfer_timeout, + // Overwritten by the refresh below with the generation actually + // observed under the lock. Only reachable unset when no backend is + // configured, in which case `release` publishes nothing at all. + publish_fence: UploadPrecondition::Unconditional, #[cfg(test)] test_pre_unlock_gate: self.pre_unlock_gate.clone(), }; - // Acquire the advisory lock with retry, through the guard's OWN connection, - // so the matching unlock (in release, or the Drop backstop) runs on the same - // session — pg_advisory_unlock on a different pooled connection is a no-op. - let mut acquired = false; - for attempt in 0..60 { - let c = guard - .conn - .as_deref_mut() - .expect("write guard holds its connection during acquisition"); - let row: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") - .bind(lock_key) - .fetch_one(&mut *c) - .await - .context("trying advisory lock")?; - if row.0 { - acquired = true; - break; - } - if attempt < 59 { - tokio::time::sleep(std::time::Duration::from_secs(1)).await; - } - } - if !acquired { - anyhow::bail!("could not acquire advisory lock after 60s — possible stale lock for {owner_slug}/{repo_name}"); - } - guard.locked = true; - // Always download the latest from Tigris before writing. Local disk may be // stale if another machine pushed since our last access. The guard already // owns the lock + its connection, so a cancellation here drops through Drop. if let Some(ref tigris) = self.tigris { - if tigris.exists(&owner_slug, repo_name).await.unwrap_or(false) { - debug!(repo = %repo_name, "write acquire: downloading latest from tigris"); - if let Err(e) = tigris.download(&owner_slug, repo_name, &local_path).await { - // Same self-healing fallback as acquire_fresh: a corrupt/unreadable - // Tigris archive must not block a write when a valid local copy - // exists — release(success) will re-upload a good archive. + // ONE budget for the whole refresh, covering the HEAD and the download + // together. Both run with the lock held and a lock-pool slot pinned, so + // bounding only the download would leave a mute endpoint able to hold + // both indefinitely on the HEAD, and bounding them separately would make + // worst-case occupancy two budgets instead of one. + let refreshed = bounded_transfer( + "acquire-refresh", + repo_name, + self.lock_held_transfer_timeout, + async { + // The HEAD and the download fail for epistemically DIFFERENT + // reasons, so they are kept apart rather than collapsed into one + // `Result`. A failed HEAD leaves us not knowing whether an archive + // exists at all, which is the same state a timeout leaves us in; + // a failed download after a successful HEAD tells us an archive is + // there and unreadable. Only the second licenses the local + // fallback. Collapsing them (the `unwrap_or(false)` this replaced + // read a HEAD error as "no archive") skipped the refresh silently + // and then re-uploaded over a possibly-newer archive. + // + // `head_etag` rather than `exists`: the same request answers + // both questions, and the ETag it carries is the generation + // this write is based on. Carrying it to the release-side + // publish is what lets the store refuse a stale PUT, which + // is the only place that fence can hold: dropping an + // in-flight upload's future does not stop the request the + // server is already processing. + match tigris.head_etag(&owner_slug, repo_name).await { + Ok(Some(etag)) => { + debug!(repo = %repo_name, "write acquire: downloading latest from tigris"); + let fence = UploadPrecondition::IfMatch(etag); + match tigris.download(&owner_slug, repo_name, &local_path).await { + Ok(()) => Ok(fence), + Err(err) => Err(RefreshFailure::Download { err, fence }), + } + } + Ok(None) => Ok(UploadPrecondition::IfAbsent), + Err(e) => Err(RefreshFailure::Unknown(e)), + } + }, + ) + .await; + + match refreshed { + Some(Ok(fence)) => guard.publish_fence = fence, + Some(Err(RefreshFailure::Download { err, fence })) => { + // The archive is present but unreadable: a corrupt or partial + // upload, or a transient GET failure. We KNOW the fetch failed, + // so falling back to a valid local copy is sound and + // release(success) re-uploads a good archive. Only hard-fail + // when there is no local copy to fall back to. if local_path.exists() { - warn!(repo = %repo_name, err = %e, - "write acquire: tigris download failed — falling back to local copy"); + warn!(repo = %repo_name, err = %err, + "write acquire: tigris refresh failed — falling back to local copy"); + // Still fence on what the HEAD saw. The download failing + // says nothing about the generation stored, so publishing + // unconditionally here would reintroduce exactly the + // overwrite this carries the ETag to prevent. + guard.publish_fence = fence; } else { - return Err(e).context("downloading repo from tigris for write"); + // No local copy, so the write cannot proceed and the + // archive's readability is unknowable. Same epistemic + // class as the HEAD arm: a transient storage blip must be + // a retryable refusal, not a 500 that tells the client the + // failure is permanent. Wrap so the handler layer's + // `RepoUnavailable` downcast maps this to a retryable 503 + // with a fixed body; the detail (which repo, why) stays in + // this error chain for the log. + return Err(anyhow::Error::new(RepoUnavailable).context(format!( + "tigris download failed during acquire_write for {owner_slug}/{repo_name}: {err:#}" + ))); } } + Some(Err(RefreshFailure::Unknown(e))) => { + // The HEAD itself failed, so we do not know whether a newer + // archive exists. Refuse for the same reason the timeout arm + // below refuses: proceeding would write against a possibly-stale + // tree and then re-upload over another node's newer archive. A + // transient object-storage blip costs a retryable refusal here, + // which is the cheaper failure than silent overwrite. + warn!(repo = %repo_name, err = %e, + "write acquire: tigris HEAD failed — refusing the write rather than \ + guessing the archive is absent"); + return Err(anyhow::Error::new(RepoUnavailable).context(format!( + "tigris HEAD failed before a write for {owner_slug}/{repo_name}" + ))); + } + None => { + // TIMED OUT, which is NOT the same as failed, and must not reach + // the fallback above. Two reasons. We do not know whether we have + // the latest tree, so writing against the local copy and then + // re-uploading can silently overwrite another node's newer + // archive. Worse, the abandoned download's extraction runs in an + // uncancellable spawn_blocking that ends in remove_dir_all + + // rename over local_path, so proceeding would run git against a + // directory that a background task is about to delete. + // + // Refuse the acquire. Returning here drops the guard, whose Drop + // frees the lock and its pool slot. + // + // `error!`, not the sibling `warn!` above, and that is deliberate. + // The handler layer demotes every `RepoUnavailable` to warn because + // the common cause is an ordinary storage blip. A stall that ran out + // the whole bound is not that: it pinned a lock-pool slot for the + // full duration, and this raise-site `error!` is what keeps it + // paging. Do NOT "fix" it to match the arm above. + tracing::error!( + repo = %repo_name, + owner = %owner_slug, + bound_secs = self.lock_held_transfer_timeout.as_secs(), + "under-lock tigris refresh exceeded the transfer bound, refusing the write" + ); + return Err(anyhow::Error::new(RepoUnavailable).context(format!( + "tigris refresh exceeded the {}s under-lock bound for {owner_slug}/{repo_name}", + self.lock_held_transfer_timeout.as_secs() + ))); + } } } @@ -292,8 +588,25 @@ impl RepoStore { let repo_name = repo_name.to_string(); let path = local_path.clone(); tokio::spawn(async move { - if let Err(e) = tigris.upload(&owner_slug, &repo_name, &path).await { - warn!(repo = %repo_name, err = %e, "failed to upload new repo to tigris"); + // Create-only, and load-bearing: this uploads a freshly + // initialized EMPTY repo, so a user who pushes immediately + // after creating one would have their archive replaced by this + // background PUT if it were unconditional. A refusal means + // someone else already published this key and dropping our + // bytes is the correct outcome. + match tigris + .upload(&owner_slug, &repo_name, &path, UploadPrecondition::IfAbsent) + .await + { + Ok(()) => {} + // Distinct from the warn arm: the fence refusing is the + // design working, not a storage failure. + Err(UploadError::PreconditionLost { status }) => { + info!(repo = %repo_name, status, "dropped the empty-repo upload: another writer already published this repo"); + } + Err(e) => { + warn!(repo = %repo_name, err = %e, "failed to upload new repo to tigris"); + } } }); } @@ -303,19 +616,51 @@ impl RepoStore { /// Upload a repo to Tigris after a write operation (push, merge, fork, etc.). /// Call this after any operation that modifies the git repo on disk. - pub async fn release_after_write(&self, owner_did: &str, repo_name: &str) { + /// + /// Returns `Err(UploadError::PreconditionLost)` when the create-only upload was + /// refused because the key already exists. That is a DISTINCT outcome from a + /// plain upload failure: the sole caller (fork creation) uses it to refuse the + /// fork rather than create a DB record whose archive is shadowed by an orphan + /// other nodes would fetch. Plain upload failures are logged and return `Ok` + /// for the same reason the guard's release logs-and-succeeds: the local write + /// is done and the storage retry is the operator's. + pub async fn release_after_write( + &self, + owner_did: &str, + repo_name: &str, + ) -> Result<(), UploadError> { if let Some(ref tigris) = self.tigris { let (owner_slug, local_path) = match self.local_path(owner_did, repo_name) { Ok(p) => p, Err(e) => { warn!(repo = %repo_name, err = %e, "rejected unsafe path in release_after_write"); - return; + return Err(UploadError::Other(e)); } }; - if let Err(e) = tigris.upload(&owner_slug, repo_name, &local_path).await { - warn!(repo = %repo_name, err = %e, "failed to upload repo to tigris after write"); + // Create-only. The sole caller is fork creation, which rejects a + // name conflict in the database before it clones anything, so the + // key is expected absent here (and archive keys are never deleted: + // `delete` has no callers). A refusal therefore means someone else + // already published this key. + match tigris + .upload( + &owner_slug, + repo_name, + &local_path, + UploadPrecondition::IfAbsent, + ) + .await + { + Ok(()) => {} + // Propagated, not logged as success: an orphan archive under + // this key shadows the fork for every other node. + Err(e @ UploadError::PreconditionLost { .. }) => return Err(e), + Err(e) => { + warn!(repo = %repo_name, err = %e, "failed to upload repo to tigris after write"); + } } } + Ok(()) } /// Compute the local disk path and owner slug for a repo. @@ -544,6 +889,121 @@ fn validate_repo_name(repo_name: &str) -> Result<()> { Ok(()) } +/// Owns a lock-pool connection across an in-flight `pg_try_advisory_lock`. +/// +/// A cancelled `.await` does not cancel an already-sent SQL statement, so a +/// try-lock whose future is dropped still takes the lock server-side while the +/// caller abandons the result. Protection therefore has to exist *before* the +/// statement goes out, which is what this type is: its `Drop` closes any +/// connection still held, ending the session so Postgres frees the lock. +/// +/// `close_on_drop()` is a one-way setter, so the arming lives here in `Drop` +/// rather than being set up front and cleared on success; "disarming" is +/// `Option::take`, which is what `take_conn` does once an acquire is observed. +/// This is the only place that issues `pg_try_advisory_lock`. +struct LockProbe { + conn: Option>, + /// True only when we have POSITIVELY established that this session does not + /// hold the lock, i.e. `try_lock` came back `false`. + /// + /// The predicate has to be "we know nothing was acquired," not "we saw an + /// answer." A `true` answer means the lock IS held, so dropping without handing + /// the connection to a guard leaks it exactly as a cancellation would; an + /// earlier version of this flag meant "settled" and reopened that leak. Default + /// false so both the cancelled-mid-flight and lock-acquired cases close, and + /// only ordinary contention returns the connection. + lock_not_taken: bool, +} + +impl LockProbe { + fn new(conn: sqlx::pool::PoolConnection) -> Self { + Self { + conn: Some(conn), + lock_not_taken: false, + } + } + + /// Send the try-lock on the owned connection. + async fn try_lock(&mut self, key: i64) -> Result { + let conn = self + .conn + .as_mut() + .context("LockProbe::try_lock after the connection was taken")?; + // Cleared BEFORE the statement is sent, not after it answers. Once the + // statement is in flight this session may hold the lock, and an error or a + // cancellation gives us no way to find out, so the connection must not be + // returned to the pool on any path but a positive `false`. Assigning only on + // success would leave a previous `true`-derived value standing. + self.lock_not_taken = false; + let row: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") + .bind(key) + .fetch_one(&mut **conn) + .await + .context("trying advisory lock")?; + // Only a false answer licenses returning the connection: it means the + // statement completed and took nothing. A true answer means this session + // now holds the lock, so Drop must still close unless `take_conn` hands it + // to a guard. + self.lock_not_taken = !row.0; + Ok(row.0) + } + + /// Hand the lock-owning connection out, leaving `Drop` with nothing to close. + /// Only call this after `try_lock` returned true. + /// + /// Named `take_` rather than `into_` deliberately: clippy expects an `into_*` + /// method to consume `self`, which a type implementing `Drop` cannot do + /// without tripping E0509. + fn take_conn(&mut self) -> Option> { + self.conn.take() + } +} + +impl Drop for LockProbe { + fn drop(&mut self) { + let Some(mut conn) = self.conn.take() else { + // take_conn already handed the connection to the guard. + return; + }; + if self.lock_not_taken { + // The probe ran and reported that someone else holds the key, so nothing + // was acquired here. Return the connection to the pool: closing would + // make a 60-attempt spinner tear down 60 backends for ordinary + // contention. Dropping `conn` unarmed does exactly that. + return; + } + // Either the future was dropped before we saw an answer, or the answer was + // that we DID take the lock and nobody took the connection off us. Both mean + // a session may be holding the lock with no one to release it, so end the + // session — which is what makes Postgres free it. + warn!("advisory-lock probe dropped while its session may hold the lock — closing the session to free it"); + conn.close_on_drop(); + } +} + +/// Non-mutating snapshot of a repo's latest Tigris state. Owns the throwaway +/// temp dir it was unpacked into and removes it on drop; a snapshot that +/// borrowed the live local path owns nothing and drops as a no-op. +pub struct RepoSnapshot { + path: PathBuf, + owned: bool, +} + +impl RepoSnapshot { + /// Path to the snapshot's bare repo directory. + pub fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for RepoSnapshot { + fn drop(&mut self) { + if self.owned { + let _ = std::fs::remove_dir_all(&self.path); + } + } +} + /// Guard returned by `acquire_write()`. Holds the Postgres advisory lock and /// uploads to Tigris + releases the lock on `release()`. pub struct RepoWriteGuard { @@ -551,19 +1011,19 @@ pub struct RepoWriteGuard { repo_name: String, pub local_path: PathBuf, lock_key: i64, - /// The pooled connection that took the advisory lock. Session-level pg - /// advisory locks are connection-affine, so the guard pins that connection - /// for its whole lifetime and unlocks on it (in `release`, or the `Drop` - /// backstop). `None` only after the connection has been taken, either to run - /// the detached unlock in `Drop` or to be closed when `release`'s unlock - /// errored (#174 F3b). - conn: Option>, - /// Set once the advisory lock has actually been taken. A guard dropped - /// before the lock is held (or after `release`) performs no unlock. - locked: bool, - /// Set once `release` has run its unlock, making the `Drop` backstop inert. - released: bool, + /// The connection that TOOK the lock. Postgres advisory locks are + /// session-scoped, so only this session can release it; holding it here is + /// what makes `release` land on the right backend instead of an arbitrary + /// pooled one. + conn: Option>, tigris: Option, + /// Bound on the release-side upload, which runs with the lock still held. + lock_held_transfer_timeout: Duration, + /// The generation of the stored archive as observed by the HEAD inside + /// `acquire_write`, under the lock. `release` publishes fenced on it, so a + /// PUT abandoned by an earlier writer's timeout cannot land on top of a + /// successor's acknowledged archive. + publish_fence: UploadPrecondition, /// Test-only seam: when set, `release` parks on this gate at the exact point /// it is about to await `pg_advisory_unlock` (connection still owned, not yet /// released). Dropping the `release` future while it is parked reproduces a @@ -573,189 +1033,422 @@ pub struct RepoWriteGuard { test_pre_unlock_gate: Option>, } -/// Deadline for tearing down the connection that saw a failing `pg_advisory_unlock`. -/// Long enough that a healthy socket always finishes well inside it, short enough that -/// a blackholed one does not pin admission resources for a TCP timeout. -const UNLOCK_ERROR_CLOSE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); - -/// Await `close` under a deadline (#174 F3c). -/// -/// `release` awaits this INLINE while the global write permit, the per-source permit -/// and the write lease are all still held, and sqlx puts no deadline on `close()`: -/// it writes Terminate and then tears the socket down. The branch that reaches here is -/// by definition a connection whose last statement errored, and a blackholed TCP path -/// to Postgres (a cloud failover that drops packets without an RST) is a plausible -/// cause, so an unbounded await here parks every later push to the repo behind three -/// pinned admission resources until the steal bound. -/// -/// On elapsed the future is simply dropped, which drops the `PoolConnection` it owns. -/// Dropping it closes the socket, and closing the socket is what actually ends the -/// session and makes Postgres release the lock, so the deadline costs nothing the -/// graceful path was buying. -async fn close_conn_bounded( - repo_name: &str, - close: impl std::future::Future>, -) { - match tokio::time::timeout(UNLOCK_ERROR_CLOSE_TIMEOUT, close).await { - Ok(Ok(())) => {} - Ok(Err(e)) => { - warn!(repo = %repo_name, err = %e, - "closing the write-lock connection failed, the session teardown still frees the lock server-side"); - } - Err(_) => { - warn!(repo = %repo_name, timeout_secs = UNLOCK_ERROR_CLOSE_TIMEOUT.as_secs(), - "closing the write-lock connection timed out, dropping it instead; the socket goes down either way, which is what frees the lock server-side"); - } +impl RepoWriteGuard { + /// Backend pid of the session holding the lock. Test-only observable for the + /// must-not-over-close check: if `release` closed the session instead of + /// returning it, consecutive writes would report different pids. + #[cfg(test)] + async fn backend_pid_for_test(&mut self) -> i32 { + let conn = self + .conn + .as_mut() + .expect("guard still holds its connection"); + let pid: (i32,) = sqlx::query_as("SELECT pg_backend_pid()") + .fetch_one(&mut **conn) + .await + .expect("backend pid"); + pid.0 } -} -impl RepoWriteGuard { /// Path to the bare repo on local disk. pub fn path(&self) -> &Path { &self.local_path } + /// Publish the tree this guard wrote, fenced on the generation observed + /// under the lock, with at most ONE supersede-retry after a definite loss. + /// + /// Hard bound of two PUT attempts per release. No loop, no recursion: a + /// third attempt would have no more reason to terminate than the second. + async fn publish(&self, tigris: &TigrisClient) -> std::result::Result<(), PublishRefusal> { + match tigris + .upload( + &self.owner_slug, + &self.repo_name, + &self.local_path, + self.publish_fence.clone(), + ) + .await + { + Ok(()) => return Ok(()), + Err(UploadError::PreconditionLost { status }) => { + // EPISTEMIC ASYMMETRY, and it is why one retry is sound here + // while the timeout arm in `release` deliberately does nothing. + // A refused precondition is a DEFINITE outcome: the store told + // us the generation we observed under the lock is gone, and that + // our bytes did not land. A timeout tells us nothing at all. + // + // We also still hold the advisory lock, so no successor can have + // acquired and published. Whatever landed underneath was written + // WITHOUT the lock: init's create-only upload of a freshly + // created empty repo, or a PUT abandoned by an earlier writer + // whose own release timed out. This writer's tree is the + // authority over both, which is what makes exactly one + // supersede-retry correct rather than a race. + // + // Honest residual: when the thing underneath was a genuine + // orphan that landed AFTER this writer's refresh, the retry + // supersedes it with a tree that does not contain it. That is + // the same outcome today's unconditional publish produces. The + // fence protects an acknowledged successor from an orphan; it + // does not protect an unlocked orphan from the lock holder. + warn!( + repo = %self.repo_name, + status, + "publish fence lost: the stored archive changed under the lock, \ + republishing once on the current generation" + ); + } + Err(e) => return Err(PublishRefusal::Failed(e)), + } + + let fresh = match tigris.head_etag(&self.owner_slug, &self.repo_name).await { + Ok(Some(etag)) => UploadPrecondition::IfMatch(etag), + // Nothing is stored now, so create-only is the fence that matches + // what was just observed. + Ok(None) => UploadPrecondition::IfAbsent, + Err(e) => return Err(PublishRefusal::Failed(UploadError::Other(e))), + }; + match tigris + .upload(&self.owner_slug, &self.repo_name, &self.local_path, fresh) + .await + { + Ok(()) => Ok(()), + Err(UploadError::PreconditionLost { status }) => { + // Two definite losses in a row: something is publishing this key + // without the lock faster than we can fence on it. Refuse rather + // than escalate. The write is on local disk and in this node's + // tree, but it is NOT durable in object storage, so the caller + // must not report success. + warn!( + repo = %self.repo_name, + status, + "publish fence lost again on the refreshed generation, refusing the \ + write rather than attempting a third publish" + ); + Err(PublishRefusal::Fenced) + } + Err(e) => Err(PublishRefusal::Failed(e)), + } + } + /// Upload to Tigris (only when the write succeeded) and release the advisory /// lock. Pass `success = false` when the write operation failed — uploading a /// half-applied or otherwise inconsistent repo would propagate corruption to /// Tigris (and to every node that later downloads it). The lock is always /// released regardless, to avoid stale locks blocking future writes. - pub async fn release(mut self, success: bool) { + pub async fn release(mut self, success: bool) -> ReleaseOutcome { + let mut outcome = ReleaseOutcome::Released; // Upload to Tigris only on success. if success { - if let Some(ref tigris) = self.tigris { - if let Err(e) = tigris - .upload(&self.owner_slug, &self.repo_name, &self.local_path) - .await + if let Some(tigris) = self.tigris.clone() { + // ONE budget for the whole publish, covering both attempts and + // the HEAD between them, for the same reason the acquire-side + // refresh uses one for its HEAD and download together: this runs + // with the lock held and a lock-pool slot pinned, so bounding + // each attempt separately would double the worst-case occupancy. + match bounded_transfer( + "release-upload", + &self.repo_name, + self.lock_held_transfer_timeout, + self.publish(&tigris), + ) + .await { - warn!(repo = %self.repo_name, err = %e, "failed to upload repo to tigris after write"); + Some(Ok(())) => {} + // Both attempts were definitively refused. The raise site + // already logged which and why, so this only has to carry + // the refusal out to the caller. + Some(Err(PublishRefusal::Fenced)) => outcome = ReleaseOutcome::Fenced, + Some(Err(PublishRefusal::Failed(e))) => { + warn!(repo = %self.repo_name, err = %e, "failed to upload repo to tigris after write"); + } + None => { + // Timed out is UNKNOWABLE, not failed: the PUT may well + // still land after this returns, so there is deliberately + // no compensating action here. The lock is released + // normally regardless. Holding it would fence nothing, + // because `release` takes `mut self`: the guard drops the + // moment this function returns and `Drop` closes the + // session, so the lock would free within milliseconds + // either way. What actually protects a successor from a + // late publish is the conditional PUT on the upload, not + // the lifetime of this lock. + warn!( + repo = %self.repo_name, + "release upload exceeded its bound; the PUT may still land, so the \ + outcome is unknowable and the conditional upload is what keeps a \ + late publish from overwriting a successor's archive" + ); + } } } } else { warn!(repo = %self.repo_name, "write failed — skipping tigris upload to avoid propagating an inconsistent repo"); } - // Release the advisory lock on the SAME connection that took it (session - // advisory locks are connection-affine). Unlock through the connection - // while it is STILL owned by `self` — do not `take()` it first. If this - // future is cancelled during the unlock await, `self` is dropped with - // `conn == Some(..)` and `released == false`, so the `Drop` backstop still - // runs the detached unlock. `released` is set only AFTER the await - // resolves, so a cancellation cannot make the backstop inert (#174 F4). - if self.locked { - #[cfg(test)] - let pre_unlock_gate = self.test_pre_unlock_gate.clone(); - let unlock = if let Some(conn) = self.conn.as_deref_mut() { - // Test-only: park right before the unlock await so a test can drop - // this future mid-unlock (connection owned, not yet released). - #[cfg(test)] - if let Some(gate) = pre_unlock_gate { - gate.notified().await; - } - Some( - sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(self.lock_key) - .execute(&mut *conn) - .await, - ) - } else { - None - }; - // An unlock that ERRORS is a different failure from a cancellation: the - // await resolved, so `Drop` is about to be made inert by `released` - // below, but the session is still alive and still holds the lock - // (statement timeout, admin cancel, aborted transaction). Returning that - // `PoolConnection` to the pool would hand the next caller a connection - // holding a lock nobody tracks (#174 F3b). Connection disposal is the - // single mechanism here, and it is why we do not instead try to keep the - // `Drop` backstop armed: disposal needs `conn.take()`, and `Drop` - // early-returns on `conn == None`. Ending the session is what frees the - // lock, so `released = true` still holds. - if let Some(Err(e)) = unlock { - warn!(repo = %self.repo_name, err = %e, - "advisory unlock failed, closing the connection so the session ends and postgres drops the lock"); - if let Some(conn) = self.conn.take() { - // `close()` over `detach()`: both consume the `PoolConnection` by - // value in sqlx 0.8.6, but we are in an async fn, so `close()` - // sends Terminate and waits for the socket to go down before - // `release` returns. `detach()` would only end the session - // whenever the returned `PgConnection` is dropped and its - // background close completes. If this future is cancelled during - // `close()`, the connection is dropped mid-close, which still - // tears the session down. That last point is also why the await is - // safe to bound: see `close_conn_bounded`, which gives it the - // deadline sqlx does not. - close_conn_bounded(&self.repo_name, conn.close()).await; - } + // Release the advisory lock on the SAME session that took it. Unlocking + // through the pool would land on an arbitrary backend, where the call is a + // silent no-op. + // + // Read the boolean. `pg_advisory_unlock` reports "you did not hold this + // lock" as a false RETURN VALUE plus a server WARNING, never an error, so a + // discarded result cannot distinguish a real release from a no-op. A false + // here means this session's lock state is not what we believe it is, so the + // connection is left in `self.conn` for `Drop` to close rather than being + // handed back to the pool as clean. Only a confirmed unlock returns it. + let lock_key = self.lock_key; + // Test-only: park right before the unlock await so a test can drop this + // future mid-unlock (connection owned, not yet released). + #[cfg(test)] + if let Some(gate) = self.test_pre_unlock_gate.clone() { + gate.notified().await; + } + let unlock = match self.conn.as_mut() { + Some(conn) => Some( + sqlx::query_as::<_, (bool,)>("SELECT pg_advisory_unlock($1)") + .bind(lock_key) + .fetch_one(&mut **conn) + .await, + ), + None => None, + }; + match unlock { + Some(Ok((true,))) => { + // Confirmed released: safe to return to the pool. + self.conn.take(); + } + Some(Ok((false,))) => { + warn!( + repo = %self.repo_name, + lock_key, + "advisory unlock reported the session did not hold this lock — closing the session instead of pooling it" + ); + } + Some(Err(e)) => { + warn!( + repo = %self.repo_name, + lock_key, + err = %e, + "advisory unlock failed — closing the session so the lock cannot outlive it" + ); } + None => {} } - self.released = true; + + outcome } } impl Drop for RepoWriteGuard { - /// Cancellation-safe backstop: if the guard is dropped while still holding the - /// advisory lock (a `tokio::time::timeout` cancelled `acquire_write`, or a - /// handler future was dropped before `release`), unlock on the pinned - /// connection. This is NOT the backstop for an unlock that ran and returned an - /// error: that case is closed inside `release` by disposing of the connection, - /// because `Drop` early-returns on `conn == None` and the two mechanisms cannot - /// both apply (#174 F3b). `Drop` cannot await, so spawn a detached unlock — it runs on the - /// same session (connection-affine). An off-runtime drop has nothing to spawn onto, - /// so it disposes of the connection instead. On runtime - /// SHUTDOWN the spawned unlock task may be dropped before it polls, so the unlock - /// may not run — but shutdown tears down the pool, and closing the connection - /// releases the session-level advisory lock server-side, so this too is bounded. fn drop(&mut self) { - if self.released || !self.locked { - return; - } let Some(mut conn) = self.conn.take() else { + // release() already unlocked and handed the connection back. return; }; - let lock_key = self.lock_key; - let repo_name = self.repo_name.clone(); - match tokio::runtime::Handle::try_current() { - Ok(handle) => { - handle.spawn(async move { - let unlock = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(lock_key) - .execute(&mut *conn) - .await; - // Same failure as `release`'s (#174 F3b), one level down: the await - // RESOLVED with an error, so the session is alive and still holds - // the lock. Letting this async block end here would drop `conn` and - // RETURN it to the pool, handing the next caller a connection - // holding a lock nobody tracks. Close it instead, which both keeps - // it out of the pool and ends the session that holds the lock. - if let Err(e) = unlock { - warn!(repo = %repo_name, err = %e, "detached advisory-unlock on write-guard drop failed, closing the connection so the session ends and postgres drops the lock"); - close_conn_bounded(&repo_name, conn.close()).await; - } - }); - } - Err(_) => { - // No runtime to spawn the unlock onto, and the connection is already - // out of the guard, so there is no path that unlocks on this session. - // Returning it to the pool would hand the next caller a connection - // still holding the lock. `PoolConnection`'s own drop also spawns its - // return-to-pool task, which panics with no runtime. `detach` gives up - // the pool slot and yields a plain `PgConnection`; dropping that closes - // the socket, which ends the session and is what frees the lock - // server-side. `Drop` cannot await, so this is the whole disposal: - // `close_conn_bounded` is not available here. - drop(conn.detach()); - warn!( - repo = %repo_name, - "RepoWriteGuard dropped off a Tokio runtime; no detached unlock is \ - possible, so the pinned connection is disposed of instead: ending \ - the session is what releases the advisory lock" - ); - } + + // Reached on any exit that skipped release(): an early `?`, a panic, or an + // axum handler future cancelled when the client disconnected. The session + // still holds the advisory lock, so returning it to the pool would block + // every future write to this repo until sqlx recycles the connection. + // + // `PoolConnection::drop` spawns onto the runtime, both to close and to + // return, and panics outright when no runtime handle exists. A panic here + // would run inside a `Drop` and abort the process during unwind, so check + // for a runtime first. With none, the process is already going away: leak + // the handle deliberately rather than panic, and let socket teardown end + // the session, which is what frees the lock at exit anyway. + if tokio::runtime::Handle::try_current().is_ok() { + warn!( + repo = %self.repo_name, + "write guard dropped without release() — closing its session to free the advisory lock" + ); + conn.close_on_drop(); + } else { + warn!( + repo = %self.repo_name, + "write guard dropped with no runtime alive — detaching the connection so Drop cannot panic" + ); + // `PoolConnection::drop` spawns onto the runtime for BOTH closing and + // returning, and panics without a handle; a panic inside Drop aborts the + // process during unwind. `leak()` detaches the raw `PgConnection`, which + // has no Drop impl of its own, so dropping it closes the socket + // synchronously with no runtime involved. That frees the lock + // immediately rather than at process exit, and leaks no fd — strictly + // better than the mem::forget this replaced. + drop(conn.leak()); + } + } +} + +/// Default wall-clock cap on WAITING for the per-repo advisory lock. +/// +/// An attempt-count bound alone is not enough: 60 attempts each paying a pool +/// acquire plus a 1s sleep reach roughly 360s. +/// +/// This bounds the wait only, NOT the whole of `acquire_write`. The under-lock +/// refresh carries its own separate bound (`lock_held_transfer_timeout`, default +/// 300s), so the two compose rather than nest and a caller can legitimately spend +/// this deadline waiting and then that bound refreshing. Do not read 90s as a +/// promise that `acquire_write` returns inside the 120s proxy idle timeout in +/// `infra/fly/fly.toml`; it is not, and reconciling the two is tracked separately. +const LOCK_ACQUIRE_DEADLINE: Duration = Duration::from_secs(90); + +/// Why an under-lock refresh did not complete, split by what it leaves us knowing. +/// +/// `Unknown` (the existence check failed) and `Download` (the archive is there and +/// unreadable) must not share a branch: only the second establishes that the local +/// copy is a sound thing to fall back to and re-upload. +enum RefreshFailure { + Unknown(anyhow::Error), + /// Carries the fence the HEAD observed alongside the error, because the + /// fallback arm still publishes later and must be fenced on the generation + /// it saw. `Unknown` carries none: that arm refuses the write outright. + Download { + err: anyhow::Error, + fence: UploadPrecondition, + }, +} + +/// What `release` was able to do with the writer's tree. +/// +/// `#[must_use]` because dropping it is the whole defect this type exists to +/// prevent: a publish the store refused would otherwise return 201, fire +/// webhooks, and record a push that no successor can read. +#[derive(Debug)] +#[must_use = "a refused publish must reach the caller, or a write that never landed reports success"] +pub enum ReleaseOutcome { + /// The lock was released and nothing definitively refused the publish. + /// Also the answer when there was nothing to publish (a failed write, no + /// storage backend) and when the outcome is unknowable (the upload + /// exceeded its bound), which keeps those paths behaving as they do today. + Released, + /// The store refused the publish twice. The tree is on local disk but is + /// NOT in object storage, so the caller must not report success. + Fenced, +} + +impl ReleaseOutcome { + /// Fold into the `Result` a handler propagates with `?`. + /// + /// Call this at every publishing site IMMEDIATELY after `release`, before + /// any post-release effect. A refusal that short-circuits after the DB + /// write, the webhook, or the response body has already happened is not a + /// refusal at all. + pub fn into_result(self) -> anyhow::Result<()> { + match self { + ReleaseOutcome::Released => Ok(()), + // The raise site inside `publish` already logged the repo and the + // status, so this carries no detail: the handler layer turns it + // into a fixed 503 body. + ReleaseOutcome::Fenced => Err(anyhow::Error::new(RepoWriteFenced) + .context("release-side publish refused by the store on both attempts")), + } + } +} + +/// Why the release-side publish did not land, split by what it leaves the +/// caller able to claim. +/// +/// `Fenced` is a DEFINITE refusal by the store after both attempts, so the +/// write is not durable and the caller must not report success. `Failed` is +/// every other upload failure, which keeps today's behavior (log it, release +/// the lock, let the caller answer normally). +enum PublishRefusal { + Fenced, + Failed(UploadError), +} + +/// The per-repo advisory lock was not obtained within the acquire deadline. +/// +/// A distinct type rather than a bare `anyhow` string so the handler layer can map +/// it to a retryable 503 with a FIXED body. Contention is transient and ordinary, +/// and the internal message names the owner slug and repo, which must stay in the +/// log rather than reaching the client. +#[derive(Debug)] +pub struct RepoBusy; + +impl std::fmt::Display for RepoBusy { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("repository is busy") + } +} + +impl std::error::Error for RepoBusy {} + +/// The under-lock refresh could not establish what is in object storage, so the +/// write was refused rather than run against a possibly-stale tree. +/// +/// A distinct type rather than a bare `anyhow` string so the handler layer can map +/// it to a retryable 503 with a FIXED body. The internal message names the owner +/// slug and repo, which must stay in the log at the raise site rather than reaching +/// the client. +#[derive(Debug)] +pub struct RepoUnavailable; + +impl std::fmt::Display for RepoUnavailable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("repository is temporarily unavailable") + } +} + +impl std::error::Error for RepoUnavailable {} + +/// The release-side publish was refused by the store on both attempts, so the +/// write is not durable in object storage. +/// +/// A distinct type rather than a bare `anyhow` string, for the same reason as +/// its two siblings above: the handler layer maps it to a retryable 503 with a +/// FIXED body, and the detail (which repo, which status) stays in the log at +/// the raise site. Distinct FROM those siblings because the condition is +/// different: not contention and not an unreadable store, but another writer +/// holding the key. The client's retry re-runs the whole write against the tree +/// that actually won. +#[derive(Debug)] +pub struct RepoWriteFenced; + +impl std::fmt::Display for RepoWriteFenced { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("repository write was fenced by a concurrent publish") + } +} + +impl std::error::Error for RepoWriteFenced {} + +/// Run a future under a wall-clock bound, returning `None` if it did not finish. +/// +/// For the object-storage transfers that run while the per-repo advisory lock is +/// held. Those were free before the lock's connection was pinned to the guard; +/// now an unbounded transfer holds a lock-pool slot for as long as it stalls, and +/// enough of them deny every write on the node. +/// +/// A timed-out transfer is **unknowable**, not failed: it may well have landed. +/// Callers must not compensate as though it definitely failed. +async fn bounded_transfer(label: &str, repo: &str, limit: Duration, fut: F) -> Option +where + F: std::future::Future, +{ + match tokio::time::timeout(limit, fut).await { + Ok(v) => Some(v), + Err(_) => { + warn!( + repo = %repo, + transfer = label, + limit_secs = limit.as_secs(), + "object-storage transfer exceeded its under-lock bound — giving up so the advisory lock and its pool slot are not held longer" + ); + None } } } +/// Test-only re-export of the advisory-lock key derivation, so handler tests can +/// hold a repo's lock from an independent session. +#[cfg(test)] +pub fn advisory_lock_key_for_test(owner_slug: &str, repo_name: &str) -> i64 { + advisory_lock_key(owner_slug, repo_name) +} + /// Compute a stable i64 hash for a Postgres advisory lock key. /// /// Uses SHA-256 (not `DefaultHasher`) so the same `(owner_slug, repo_name)` @@ -1145,7 +1838,12 @@ mod tests { // the pool or the network. Fabricate a pool reference via PgPool::connect_lazy // so we don't need a live DB. let pool = sqlx::PgPool::connect_lazy("postgres://invalid").unwrap(); - RepoStore::new(PathBuf::from("/var/lib/gitlawb/repos"), None, pool) + RepoStore::new( + PathBuf::from("/var/lib/gitlawb/repos"), + None, + pool, + Duration::from_secs(300), + ) } #[tokio::test] @@ -1289,7 +1987,7 @@ mod tests { let mut checker = pool.acquire().await.expect("checker connection"); let guard = store.acquire_write(owner, name).await.expect("acquire"); - guard.release(false).await; + let _ = guard.release(false).await; let (free,): (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") .bind(key) @@ -1381,7 +2079,7 @@ mod tests { .acquire_write(owner, name) .await .expect("first acquire"); - guard.release(true).await; + let _ = guard.release(true).await; let again = tokio::time::timeout( std::time::Duration::from_secs(2), @@ -1390,7 +2088,7 @@ mod tests { .await .expect("second acquire_write must not hit the ~60s stale-lock retry loop") .expect("second acquire"); - again.release(true).await; + let _ = again.release(true).await; } // ── unlock error disposes the connection (#174 F3b, RED-before/GREEN-after) ─ @@ -1481,45 +2179,6 @@ mod tests { .expect("a second pool over the test database") } - /// F3c (P2): the connection teardown on the failing-unlock path must be BOUNDED. - /// `release` awaits it inline while the global write permit, the per-source permit - /// and the write lease are all still held, and sqlx's `close()` carries no deadline - /// of its own, so a blackholed socket would park every later push to that repo - /// behind three pinned admission resources. - /// - /// What this covers: the deadline itself. A close that never resolves still lets - /// `close_conn_bounded` return, which is the property `release` depends on. What it - /// does NOT cover, and is reasoned rather than run: that sqlx's own `close()` is - /// what stalls in production. Making a real `PgConnection::close` hang needs a - /// blackholed TCP path to Postgres, and the flip has to land after the unlock - /// statement round-trips but before the Terminate write, which is not a seam this - /// module exposes. A never-resolving future is the faithful stand-in for that - /// close, and the F3b tests above already cover that `release` really routes its - /// close through here. - /// - /// Time is paused, so nothing here depends on wall clock: the runtime auto-advances - /// to the next timer, and the assertion is on which timer fired, not on elapsed - /// time. The outer bound is what turns a removed deadline into a failure rather - /// than a hung suite. - /// - /// Load-bearing: drop the `tokio::time::timeout` in `close_conn_bounded` and the - /// inner future never resolves, so the outer bound fires and this fails. - #[tokio::test(start_paused = true)] - async fn unlock_error_connection_close_is_bounded() { - let hanging = std::future::pending::>(); - let outcome = tokio::time::timeout( - UNLOCK_ERROR_CLOSE_TIMEOUT * 4, - close_conn_bounded("boundedclosetest", hanging), - ) - .await; - assert!( - outcome.is_ok(), - "a connection close that never completes must not hold the write lease and \ - both admission permits open-endedly: close_conn_bounded must give up and \ - drop the connection" - ); - } - /// F3b (P1): when `pg_advisory_unlock` ERRORS while the session is still alive /// (statement timeout, admin cancel, aborted transaction), the lock must not /// survive `release`. The old code discarded the error with `let _ =` and set @@ -1562,7 +2221,7 @@ mod tests { "the poisoned session must still hold the lock before release" ); - guard.release(false).await; + let _ = guard.release(false).await; // Postgres drops the lock when the disposed session's backend exits, which is // asynchronous to our socket close: poll for it rather than sleeping a @@ -1600,7 +2259,7 @@ mod tests { let size_before = store_pool.size(); assert!(size_before > 0, "the pool owns the guard's connection"); - guard.release(false).await; + let _ = guard.release(false).await; // The pool's size drops when the closed connection's slot is given up, which // is not synchronous with `release` returning: poll rather than sleep. @@ -1630,7 +2289,7 @@ mod tests { let guard = store.acquire_write(owner, name).await.expect("acquire"); let size_before = pool.size(); - guard.release(false).await; + let _ = guard.release(false).await; tokio::time::sleep(std::time::Duration::from_millis(400)).await; assert_eq!( @@ -1708,16 +2367,33 @@ mod tests { .await; } - /// U8 regression guard on the success path: a detached unlock that SUCCEEDS must - /// still return the connection to the pool. Without this, "close the connection on - /// Drop" could be widened to "always close" and the test above would not notice. + /// U8, the off-runtime arm: with no Tokio runtime there is nothing to spawn the + /// unlock onto, and the connection has already been taken out of the guard, so the + /// old code dropped it with no unlock attempted at all, back to the pool, session + /// lock still held. Dropping a `PoolConnection` off a runtime is worse than that: + /// sqlx's return-to-pool path spawns, and its no-runtime fallback panics, so the + /// old arm also panicked in a destructor. + /// + /// Reached by dropping the guard on a plain `std::thread`, where + /// `Handle::try_current()` fails. + /// + /// Load-bearing: RED before the fix (the join sees the "requires a Tokio context" + /// panic from sqlx's return-to-pool spawn), GREEN after (`leak` gives up the + /// pool slot, so nothing is spawned and dropping the leaked connection closes + /// the socket, which ends the session and frees the lock). + /// + /// `leak`, not `detach`: the branch's off-runtime arm deliberately leaks the + /// slot (permanently checked out, `size()` unchanged) rather than detaching + /// (which lets the pool open a replacement), because at process-teardown time + /// there is no runtime to service the replacement's connect. The observable + /// is `num_idle()`: the leaked slot never returns to idle. #[sqlx::test] - async fn write_guard_drop_with_successful_unlock_keeps_the_connection(pool: sqlx::PgPool) { + async fn write_guard_dropped_off_runtime_disposes_the_connection(pool: sqlx::PgPool) { let dir = tempfile::TempDir::new().unwrap(); let store_pool = pool_without_idle_reaper(&pool).await; let store = RepoStore::for_testing(dir.path().to_path_buf(), store_pool.clone()); - let owner = "did:key:z6MkDropUnlockOkProofJJJJJJJJJJJJJJJJJJJJ"; - let name = "dropunlockoktest"; + let owner = "did:key:z6MkDropOffRuntimeProofKKKKKKKKKKKKKKKKKK"; + let name = "dropoffruntimetest"; let slug = owner.replace([':', '/'], "_"); let key = advisory_lock_key(&slug, name); @@ -1726,76 +2402,25 @@ mod tests { let size_before = store_pool.size(); assert!(size_before > 0, "the pool owns the guard's connection"); - drop(guard); + let dropped = std::thread::spawn(move || drop(guard)).join(); + assert!( + dropped.is_ok(), + "dropping a write guard off a Tokio runtime must not panic" + ); - // The connection goes back only once the detached unlock task has finished. + // The disposed connection must NOT come back to the pool as idle: that is + // the leak-vs-return distinction that made the old code return a session + // still holding the lock. `leak` keeps the slot permanently checked out, so + // `num_idle` cannot rise here. wait_until( - || store_pool.num_idle() > 0, - "the detached unlock to finish and hand the connection back", + || store_pool.num_idle() == 0, + "the leaked connection to never return to the pool's idle set", ) .await; - assert_eq!( - store_pool.size(), - size_before, - "a successful detached unlock must leave the connection in the pool" - ); wait_until_lock_free( &mut checker, key, - "the Drop backstop's successful unlock to free the lock", - ) - .await; - let _ = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(key) - .execute(&mut *checker) - .await; - } - - /// U8, the off-runtime arm: with no Tokio runtime there is nothing to spawn the - /// unlock onto, and the connection has already been taken out of the guard, so the - /// old code dropped it with no unlock attempted at all, back to the pool, session - /// lock still held. Dropping a `PoolConnection` off a runtime is worse than that: - /// sqlx's return-to-pool path spawns, and its no-runtime fallback panics, so the - /// old arm also panicked in a destructor. - /// - /// Reached by dropping the guard on a plain `std::thread`, where - /// `Handle::try_current()` fails. - /// - /// Load-bearing: RED before the fix (the join sees the "requires a Tokio context" - /// panic from sqlx's return-to-pool spawn), GREEN after (`detach` gives up the - /// pool slot, so nothing is spawned and dropping the detached connection closes - /// the socket, which ends the session and frees the lock). - #[sqlx::test] - async fn write_guard_dropped_off_runtime_disposes_the_connection(pool: sqlx::PgPool) { - let dir = tempfile::TempDir::new().unwrap(); - let store_pool = pool_without_idle_reaper(&pool).await; - let store = RepoStore::for_testing(dir.path().to_path_buf(), store_pool.clone()); - let owner = "did:key:z6MkDropOffRuntimeProofKKKKKKKKKKKKKKKKKK"; - let name = "dropoffruntimetest"; - let slug = owner.replace([':', '/'], "_"); - let key = advisory_lock_key(&slug, name); - - let mut checker = pool.acquire().await.expect("checker connection"); - let guard = store.acquire_write(owner, name).await.expect("acquire"); - let size_before = store_pool.size(); - assert!(size_before > 0, "the pool owns the guard's connection"); - - let dropped = std::thread::spawn(move || drop(guard)).join(); - assert!( - dropped.is_ok(), - "dropping a write guard off a Tokio runtime must not panic" - ); - - wait_until( - || store_pool.size() == size_before - 1, - "the connection of a guard dropped off a runtime to be disposed of rather \ - than returned to the pool with no unlock attempted", - ) - .await; - wait_until_lock_free( - &mut checker, - key, - "a guard dropped off a runtime to end its session so postgres drops the lock", + "a guard dropped off a runtime to end its session so postgres drops the lock", ) .await; let _ = sqlx::query("SELECT pg_advisory_unlock($1)") @@ -1820,13 +2445,14 @@ mod tests { local_path: dir.path().to_path_buf(), lock_key: key, conn: Some(pool.acquire().await.expect("conn")), - locked: false, - released: false, tigris: None, + lock_held_transfer_timeout: Duration::from_secs(300), + publish_fence: UploadPrecondition::Unconditional, + #[cfg(test)] test_pre_unlock_gate: None, }; // Must complete without panic and issue no unlock. - guard.release(false).await; + let _ = guard.release(false).await; let mut checker = pool.acquire().await.expect("checker"); let (free,): (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") @@ -1840,4 +2466,2802 @@ mod tests { .execute(&mut *checker) .await; } + + // ── U1: cancellation-safe lock probe ─────────────────────────────────── + + /// A pool with every reaping path disabled, so a leaked lock persists through + /// the observation window instead of being freed by ambient recycling. + async fn no_reap_pool(opts: &sqlx::postgres::PgConnectOptions, max: u32) -> PgPool { + sqlx::postgres::PgPoolOptions::new() + .max_connections(max) + .acquire_timeout(std::time::Duration::from_secs(5)) + .min_connections(0) + .idle_timeout(None) + .max_lifetime(None) + .test_before_acquire(false) + .connect_with(opts.clone()) + .await + .expect("no-reap pool") + } + + /// Poll a STANDALONE connection until the key is free, or the deadline passes. + /// + /// Standalone, never from the pool under test: pool reuse would hand the + /// observer the lock-holding session itself, where `pg_try_advisory_lock` + /// succeeds reentrantly and hides the very leak being measured. Polling rather + /// than asserting once because `PoolConnection::drop` spawns the close. + async fn poll_until_free( + opts: &sqlx::postgres::PgConnectOptions, + key: i64, + deadline: std::time::Duration, + ) -> bool { + use sqlx::Connection; + let start = std::time::Instant::now(); + let mut observer = sqlx::PgConnection::connect_with(opts) + .await + .expect("standalone observer connection"); + while start.elapsed() < deadline { + let got: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") + .bind(key) + .fetch_one(&mut observer) + .await + .expect("observer try-lock"); + if got.0 { + let _: (bool,) = sqlx::query_as("SELECT pg_advisory_unlock($1)") + .bind(key) + .fetch_one(&mut observer) + .await + .expect("observer unlock"); + return true; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + false + } + + /// THE COMMITTED GATE for the cancellation window (U1). + /// + /// Dropping the probe without taking its connection is exactly the state a + /// cancellation between the try-lock's send and the guard's construction + /// leaves behind. Deterministic on purpose: the timing sweep that first found + /// this window leaks about 1 in 600, which is not a signal a CI gate can rest + /// on. That sweep stays a local repro. + #[sqlx::test] + async fn lock_probe_dropped_without_taking_frees_the_lock(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 4).await; + let key: i64 = 990_001; + + { + let mut probe = LockProbe::new(lock_pool.acquire().await.unwrap()); + assert!( + probe.try_lock(key).await.unwrap(), + "probe should take a free key" + ); + // dropped here WITHOUT take_conn(): the cancellation shape + } + + assert!( + poll_until_free(&opts, key, std::time::Duration::from_secs(10)).await, + "lock must be freed after a probe is dropped without taking its connection" + ); + } + + /// Must-not: a successful acquire hands the connection out intact, so the + /// normal path does not pay a reconnect per write. + #[sqlx::test] + async fn lock_probe_take_conn_yields_a_usable_connection(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 4).await; + let key: i64 = 990_002; + + let mut probe = LockProbe::new(lock_pool.acquire().await.unwrap()); + assert!(probe.try_lock(key).await.unwrap()); + let mut conn = probe + .take_conn() + .expect("connection after a successful acquire"); + drop(probe); + + let one: (i32,) = sqlx::query_as("SELECT 1") + .fetch_one(&mut *conn) + .await + .expect("handed-out connection must still be usable"); + assert_eq!(one.0, 1); + + let released: (bool,) = sqlx::query_as("SELECT pg_advisory_unlock($1)") + .bind(key) + .fetch_one(&mut *conn) + .await + .unwrap(); + assert!(released.0, "the handed-out connection still owns the lock"); + } + + /// Must-not: a failed probe returns its connection without closing it. Nothing + /// was locked, so closing would be pure churn, and closing on every failed + /// probe would make a 60-attempt spinner tear down 60 backends. + #[sqlx::test] + async fn lock_probe_failed_acquire_does_not_hold_anything(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 4).await; + let key: i64 = 990_003; + + // a standalone holder takes the key first + use sqlx::Connection; + let mut holder = sqlx::PgConnection::connect_with(&opts).await.unwrap(); + let held: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") + .bind(key) + .fetch_one(&mut holder) + .await + .unwrap(); + assert!(held.0); + + { + let mut probe = LockProbe::new(lock_pool.acquire().await.unwrap()); + assert!( + !probe.try_lock(key).await.unwrap(), + "probe must observe false for a key held elsewhere" + ); + } + + // the holder still owns it: the failed probe neither took nor released it + let still: (i64,) = sqlx::query_as( + "SELECT count(*) FROM pg_locks WHERE locktype='advisory' \ + AND ((classid::bigint<<32)|objid::bigint) = $1", + ) + .bind(key) + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!(still.0, 1, "the original holder must still own the key"); + } + + // ── U3: the #279 acceptance tests ────────────────────────────────────── + + /// The store under test. Pre-U3 this ignores `opts` and shares the app pool, + /// which is exactly the broken shape; the wiring change swaps in a dedicated + /// no-reap lock pool without touching a single test body below. + async fn write_store(pool: &PgPool, opts: &sqlx::postgres::PgConnectOptions) -> RepoStore { + let _ = pool; + RepoStore::for_testing( + PathBuf::from("/tmp/gitlawb-u3"), + no_reap_pool(opts, 8).await, + ) + } + + fn advisory_locks_held(key: i64) -> String { + format!( + "SELECT count(*) FROM pg_locks WHERE locktype='advisory' \ + AND ((classid::bigint<<32)|objid::bigint) = {key}" + ) + } + + /// ACCEPTANCE 1 (#279): two writers on one node and the same repo must not + /// both hold the lock. On the pre-fix shape the second acquire succeeds + /// because the pool hands it the very session holding the lock, where + /// pg_try_advisory_lock is reentrant. + #[sqlx::test] + async fn two_writers_on_the_same_repo_are_not_both_admitted(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + 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 err = match store.acquire_write("did:key:z6MkU3Excl", "same-repo").await { + Err(e) => e, + Ok(second) => { + let _ = second.release(false).await; + panic!("a second writer must NOT be admitted while the first holds the guard"); + } + }; + assert!( + err.downcast_ref::().is_some(), + "the second writer must be shed as RepoBusy, got {err:#}" + ); + } + + /// ACCEPTANCE 2 (#279): a completed write leaves no advisory lock behind. + /// On the pre-fix shape the unlock runs on a different pooled session and + /// returns false, so the lock leaks on essentially every write. + #[sqlx::test] + async fn completed_write_releases_its_advisory_lock(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let store = write_store(&pool, &opts).await; + + let guard = store + .acquire_write("did:key:z6MkU3Rel", "leak-check") + .await + .expect("acquire"); + let _ = guard.release(true).await; + + let key = advisory_lock_key("did_key_z6MkU3Rel", "leak-check"); + let held: (i64,) = sqlx::query_as(&advisory_locks_held(key)) + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!( + held.0, 0, + "a completed write must leave zero advisory locks for its key" + ); + } + + // ── U4: a guard that dies without releasing must free the lock ────────── + + /// A guard dropped without `release()` (an early `?`, a panic, or a handler + /// future cancelled on client disconnect) must not return a lock-bearing + /// connection to the pool, where it would block every future write to that + /// repo until sqlx recycles the session. + #[sqlx::test] + async fn guard_dropped_without_release_frees_the_lock(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let store = write_store(&pool, &opts).await; + let key = advisory_lock_key("did_key_z6MkU4Drop", "dropped"); + + { + let _guard = store + .acquire_write("did:key:z6MkU4Drop", "dropped") + .await + .expect("acquire"); + // dropped here without release() + } + + assert!( + poll_until_free(&opts, key, std::time::Duration::from_secs(10)).await, + "lock must be freed when a guard is dropped without release()" + ); + } + + /// Must-not over-close: the normal path returns its connection to the pool, so + /// a healthy write does not pay a reconnect. Sized to one connection so the + /// backend pid is a direct observable: if `release` were closing the session, + /// each cycle would land on a fresh backend. + #[sqlx::test] + async fn normal_release_reuses_the_same_backend(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let store = RepoStore::for_testing( + PathBuf::from("/tmp/gitlawb-u4"), + no_reap_pool(&opts, 1).await, + ); + + let mut pids = Vec::new(); + for i in 0..4 { + let repo = format!("reuse-{i}"); + let mut guard = store + .acquire_write("did:key:z6MkU4Reuse", &repo) + .await + .expect("acquire"); + pids.push(guard.backend_pid_for_test().await); + let _ = guard.release(true).await; + } + assert!( + pids.windows(2).all(|w| w[0] == w[1]), + "a released guard must return its connection to the pool, so all four \ + writes share one backend; saw {pids:?}" + ); + } + + /// A guard abandoned while the runtime is tearing down must not panic. + /// `PoolConnection::drop` calls `crate::rt::spawn`, which panics without a + /// runtime handle, and a panic inside `Drop` during unwind aborts the process. + /// At real process exit the lock is freed by socket teardown, not by this Drop + /// body, so this asserts no-panic rather than lock release. + #[test] + fn guard_dropped_at_runtime_teardown_does_not_panic() { + // No silent skip: a test that returns green when its precondition is + // absent is worse than one that fails, because it reports coverage it does + // not have. CI provisions Postgres, so an absent DATABASE_URL is a broken + // environment rather than an expected one. + let url = std::env::var("DATABASE_URL") + .expect("DATABASE_URL must be set; this test cannot pass vacuously"); + let rt = tokio::runtime::Runtime::new().unwrap(); + let guard = rt.block_on(async { + let lock_pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(2) + .connect(&url) + .await + .expect("lock pool"); + let store = RepoStore::for_testing(PathBuf::from("/tmp/gitlawb-u4b"), lock_pool); + store + .acquire_write("did:key:z6MkU4Teardown", "teardown") + .await + .expect("acquire") + }); + // Shut the runtime down first, then drop the guard with no runtime alive. + drop(rt); + drop(guard); + } + + // ── U5: the unlock's boolean result must be observed ──────────────────── + + /// `pg_advisory_unlock` reports "you did not hold this lock" as a `false` + /// RETURN VALUE plus a server WARNING, never an error, so a discarded result + /// cannot tell a real release from a no-op. A session that did not hold the + /// key must not be returned to the pool as if it were clean. + /// + /// The observable is the backend pid: on a one-connection pool, a session that + /// was closed forces the next acquire onto a fresh backend, while one returned + /// normally is handed straight back. + #[sqlx::test] + async fn release_that_did_not_hold_the_lock_closes_the_session(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 1).await; + + let pid_before = { + 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 + }; + + // A guard whose key was never locked: release()'s unlock returns false. + let guard = RepoWriteGuard { + owner_slug: "did_key_z6MkU5".to_string(), + repo_name: "never-locked".to_string(), + local_path: PathBuf::from("/tmp/gitlawb-u5"), + lock_key: 995_001, + conn: Some(lock_pool.acquire().await.unwrap()), + tigris: None, + lock_held_transfer_timeout: Duration::from_secs(300), + // No backend, so nothing is ever published and the fence is unread. + publish_fence: UploadPrecondition::Unconditional, + #[cfg(test)] + test_pre_unlock_gate: None, + }; + let _ = guard.release(true).await; + + // Wait for the backend to actually go away rather than sleeping a fixed + // span, which is flaky on slow CI. The observer is a STANDALONE + // connection for the same reason `poll_until_free` uses one: taking it + // from the pool under test would hand us the very session being measured. + // Nothing but the close under test can retire that backend, because + // `no_reap_pool` disables idle timeout and max lifetime, so a zero count + // here is attributable to `release()` and to nothing else. + { + use sqlx::Connection; + let deadline = std::time::Duration::from_secs(5); + let start = std::time::Instant::now(); + let mut observer = sqlx::PgConnection::connect_with(&opts) + .await + .expect("standalone observer connection"); + while start.elapsed() < deadline { + let alive: (i64,) = + sqlx::query_as("SELECT count(*) FROM pg_stat_activity WHERE pid = $1") + .bind(pid_before) + .fetch_one(&mut observer) + .await + .expect("observer pg_stat_activity probe"); + if alive.0 == 0 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(10)).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 + }; + + assert_ne!( + pid_before, pid_after, + "an unlock that returned false means the session's lock state is not \ + what we think it is; that connection must be closed, not pooled" + ); + } + + // ── U6: under-lock transfers are bounded ──────────────────────────────── + + /// The bound itself. Driving a real stalled transfer through `acquire_write` + /// would need either the object-store abstraction (out of scope here) or a + /// process-global `AWS_ENDPOINT_URL_S3` mutation, which would make the suite + /// order-dependent under the concurrent test runner. So this covers the + /// mechanism deterministically and the wiring is verified by reading, which is + /// recorded as a coverage gap rather than papered over. + #[tokio::test] + async fn bounded_transfer_gives_up_past_the_limit() { + let slow = async { + tokio::time::sleep(std::time::Duration::from_secs(30)).await; + Ok::<(), anyhow::Error>(()) + }; + let out = + bounded_transfer("test", "repo", std::time::Duration::from_millis(50), slow).await; + assert!( + out.is_none(), + "a transfer past its limit must report None so the caller stops holding the lock" + ); + } + + /// Must-not: a transfer that finishes inside the limit is returned intact and + /// is not truncated by the bound. + #[tokio::test] + async fn bounded_transfer_passes_through_a_prompt_result() { + let quick = async { Ok::(7) }; + let out = bounded_transfer("test", "repo", std::time::Duration::from_secs(30), quick).await; + assert!( + matches!(out, Some(Ok(7))), + "a prompt transfer must pass through untouched" + ); + } + + /// F2 regression: an ordinary failed probe must RETURN its connection, not + /// close it. The old test asserted the holder's pg_locks count, which cannot + /// see what happened to the probe's own connection — so it passed while a + /// 60-attempt spinner tore down 60 backends. The observable that discriminates + /// is the backend pid on a one-connection pool. + #[sqlx::test] + async fn failed_probe_returns_its_connection_to_the_pool(pool: PgPool) { + use sqlx::Connection; + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 1).await; + let key: i64 = 991_100; + + // someone else holds the key, from an independent session + let mut holder = sqlx::PgConnection::connect_with(&opts).await.unwrap(); + let held: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") + .bind(key) + .fetch_one(&mut holder) + .await + .unwrap(); + assert!(held.0); + + let mut pids = Vec::new(); + for _ in 0..3 { + let mut probe = LockProbe::new(lock_pool.acquire().await.unwrap()); + let pid: (i32,) = sqlx::query_as("SELECT pg_backend_pid()") + .fetch_one(&mut **probe.conn.as_mut().unwrap()) + .await + .unwrap(); + pids.push(pid.0); + assert!(!probe.try_lock(key).await.unwrap(), "key is held elsewhere"); + drop(probe); + tokio::time::sleep(std::time::Duration::from_millis(150)).await; + } + assert!( + pids.windows(2).all(|w| w[0] == w[1]), + "a failed probe must return its connection so a spinner does not churn \ + backends; saw {pids:?}" + ); + } + + // ── F5: the tests the plan required and the first pass never wrote ──────── + + /// R4, the test the plan named as proving the pool split and the one that would + /// have caught PR #215's node-wide two-write ceiling. Holding N guards on + /// DISTINCT repos must pin N lock-pool connections while leaving the app pool + /// free to serve ordinary queries. + #[sqlx::test] + async fn lock_pool_exhaustion_does_not_starve_the_app_pool(pool: PgPool) { + const N: u32 = 3; + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, N).await; + let store = RepoStore::for_testing(PathBuf::from("/tmp/gitlawb-f5"), lock_pool.clone()); + + let mut guards = Vec::new(); + for i in 0..N { + guards.push( + store + .acquire_write(&format!("did:key:z6MkF5Iso{i}"), "iso") + .await + .expect("distinct repos each acquire"), + ); + } + + // Every slot is accounted for by a guard, so the pool really is exhausted + // rather than merely slow. Asserted directly, because the starvation check + // below cannot tell the two apart on its own. + assert_eq!( + lock_pool.size() as usize - lock_pool.num_idle(), + N as usize, + "all N slots must be checked out by the guards" + ); + + // An N+1th checkout must be refused BY THE POOL. The specific error matters: + // `Ok(Err(_)) | Err(_)` would also be satisfied by the outer tokio timeout + // firing for an unrelated reason, which would let this pass without the pool + // ever having refused anything. + let starved = + tokio::time::timeout(std::time::Duration::from_secs(8), lock_pool.acquire()).await; + match starved { + Ok(Err(sqlx::Error::PoolTimedOut)) => {} + Ok(Err(e)) => panic!("expected the pool's own timeout, got {e:?}"), + Ok(Ok(_)) => panic!("with N guards held, an N+1th lock-pool checkout must not succeed"), + Err(_) => panic!( + "the pool must refuse the checkout itself within its acquire_timeout; \ + the outer timeout firing means it never did" + ), + } + + // ...while the APP pool still serves queries. This is the whole point of + // the split: write pressure must not deny ordinary reads. Weak on its own (it + // is a different pool object, so it would serve regardless), so it is the + // exhaustion assertions above that carry the isolation claim; this only + // confirms the reads are actually reachable in that state. + let alive: (i32,) = sqlx::query_as("SELECT 1") + .fetch_one(&pool) + .await + .expect("app pool must remain usable while the lock pool is exhausted"); + assert_eq!(alive.0, 1); + + for g in guards.drain(..) { + let _ = g.release(true).await; + } + } + + /// A waiter spinning on a contended repo must hand its pool slot back for the + /// duration of each backoff, and must not block a write to an unrelated repo + /// (R5, both halves). + /// + /// The pool-counter sampling is the load-bearing half. A second `acquire_write` + /// succeeding proves only that two different lock keys do not collide, which is + /// true whether or not the spinner released anything: with the slot held through + /// the sleep, a pool of 3 still has room for it. So this samples what the + /// spinner actually occupies across more than two backoff cycles. Moving + /// `drop(probe)` after the backoff sleep turns it red. + #[sqlx::test] + async fn waiter_on_one_repo_does_not_block_another(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 3).await; + let store = std::sync::Arc::new(RepoStore::for_testing( + PathBuf::from("/tmp/gitlawb-f5b"), + lock_pool.clone(), + )); + + let held = store + .acquire_write("did:key:z6MkF5Cont", "contended") + .await + .unwrap(); + + let spinner = { + let s = store.clone(); + tokio::spawn(async move { s.acquire_write("did:key:z6MkF5Cont", "contended").await }) + }; + + // Sample across >2 backoff cycles. `held` accounts for exactly one + // checked-out connection throughout, so every sample above that is the + // spinner sitting on a slot it is not using. + let mut spinner_idle = 0; + let mut samples = 0; + for _ in 0..50 { + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + let checked_out = lock_pool.size() as usize - lock_pool.num_idle(); + if checked_out == 1 { + spinner_idle += 1; + } + samples += 1; + } + assert!( + spinner_idle * 10 >= samples * 7, + "a spinner must hold no lock-pool slot through its backoff: only {spinner_idle}/{samples} \ + samples showed just the held guard checked out" + ); + + let unrelated = tokio::time::timeout( + std::time::Duration::from_secs(8), + store.acquire_write("did:key:z6MkF5Other", "innocent"), + ) + .await + .expect("an unrelated repo must not wait on someone else's contention") + .expect("and must acquire"); + let _ = unrelated.release(true).await; + + spinner.abort(); + let _ = held.release(true).await; + } + + /// Lock contention that runs out the acquire deadline must surface as a + /// retryable 503 with a fixed body, not a 500 carrying the owner slug and repo + /// name. The deadline is a field so this does not wait out the 90s default. + #[sqlx::test] + async fn contended_acquire_sheds_as_repo_busy_not_internal_error(pool: PgPool) { + use axum::response::IntoResponse; + + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 4).await; + let store = RepoStore::for_testing(PathBuf::from("/tmp/gitlawb-busy"), lock_pool) + .with_lock_acquire_deadline(std::time::Duration::from_millis(300)); + + let held = store + .acquire_write("did:key:z6MkBusyOwner", "busyrepo") + .await + .expect("first writer acquires"); + + // Not `expect_err`: the guard is not Debug, and a guard obtained here must be + // released rather than dropped on a panic path. + let err = match store + .acquire_write("did:key:z6MkBusyOwner", "busyrepo") + .await + { + Err(e) => e, + Ok(second) => { + let _ = second.release(false).await; + panic!("a second writer must be shed once the deadline expires"); + } + }; + + // The internal chain keeps the operator detail... + let chain = format!("{err:#}"); + assert!( + chain.contains("busyrepo"), + "the log-side error must name the repo, got {chain}" + ); + + // ...and the client-visible mapping must carry neither it nor a 500. + let resp = crate::error::AppError::from(err).into_response(); + assert_eq!( + resp.status(), + axum::http::StatusCode::SERVICE_UNAVAILABLE, + "contention is transient and must be retryable" + ); + let body = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .expect("body"); + let body = String::from_utf8_lossy(&body); + assert!( + body.contains("repo_busy") && !body.contains("busyrepo"), + "the 503 body must be fixed and must not name the repo, got {body}" + ); + + let _ = held.release(true).await; + } + + /// An under-lock refresh refusal must surface as a retryable 503 with a fixed + /// body, not a 500 carrying the owner slug and repo name. Built directly from + /// the typed error so it needs no database; the `.context()` layer is kept + /// deliberately, because the real raise path wraps one and this proves anyhow + /// preserves downcastability through it. + #[tokio::test] + async fn repo_unavailable_maps_to_retryable_503_with_fixed_body() { + use axum::response::IntoResponse; + + let err = anyhow::Error::new(RepoUnavailable) + .context("tigris HEAD failed before a write for did_key_z6MkTest/secret-repo"); + + let resp = crate::error::AppError::from(err).into_response(); + assert_eq!( + resp.status(), + axum::http::StatusCode::SERVICE_UNAVAILABLE, + "a storage blip is transient and must be retryable, not a 500" + ); + let body = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .expect("body"); + let body = String::from_utf8_lossy(&body); + assert!( + body.contains("repo_unavailable"), + "the 503 must carry the repo_unavailable code, got {body}" + ); + assert!( + !body.contains("secret-repo"), + "the 503 body must be fixed and must not name the repo, got {body}" + ); + assert!( + !body.contains("did_key_z6MkTest"), + "the 503 body must be fixed and must not name the owner, got {body}" + ); + } + + /// The new downcast rung must be additive: an unrelated anyhow error still + /// falls through to the internal 500. + #[tokio::test] + async fn repo_unavailable_rung_does_not_swallow_unrelated_errors() { + use axum::response::IntoResponse; + + let resp = + crate::error::AppError::from(anyhow::anyhow!("some other failure")).into_response(); + assert_eq!( + resp.status(), + axum::http::StatusCode::INTERNAL_SERVER_ERROR, + "an unrelated failure must not be reclassified as retryable" + ); + let body = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .expect("body"); + let body = String::from_utf8_lossy(&body); + assert!( + body.contains("internal_error"), + "an unrelated failure must keep the internal_error code, got {body}" + ); + } + + /// A Tigris client aimed at a closed port, so every call fails at the + /// transport layer promptly and `exists()` returns `Err` rather than + /// `Ok(false)`. + #[cfg(test)] + fn unreachable_tigris() -> TigrisClient { + TigrisClient::for_testing_with_endpoint("test-bucket", "http://127.0.0.1:1") + } + + /// The under-lock sibling of the above. `acquire_write` already refuses on + /// this condition; this proves the `RefreshFailure::Unknown` arm end to end + /// against a real failing HEAD rather than by reading the code. + #[sqlx::test] + async fn acquire_write_refuses_when_the_head_check_fails(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 2).await; + let store = RepoStore::for_testing_with_tigris( + PathBuf::from("/tmp/gitlawb-headfail-write"), + lock_pool, + unreachable_tigris(), + ); + + // Not `expect_err`: the guard is not Debug, and a guard obtained here + // must be released rather than dropped on a panic path. + let err = match store + .acquire_write("did:key:z6MkHeadFail", "writerepo") + .await + { + Err(e) => e, + Ok(guard) => { + let _ = guard.release(false).await; + panic!("a failed HEAD must refuse the write rather than proceed on a stale tree"); + } + }; + assert!( + err.downcast_ref::().is_some(), + "the refusal must be typed so the handler layer maps it to a retryable 503, got {err:#}" + ); + } + + /// P2 (cold-cache): the under-lock refresh's HEAD succeeds but the GET fails + /// on a node with no local copy. The download arm must refuse as + /// `RepoUnavailable` (retryable 503), matching the HEAD arm and the + /// `acquire_fresh` sibling, not a bare anyhow error that the handler layer + /// maps to a permanent 500. + #[sqlx::test] + async fn acquire_write_refuses_when_the_download_fails_and_no_local_copy_exists(pool: PgPool) { + use axum::response::IntoResponse; + + let app = axum::Router::new().route( + "/{*key}", + axum::routing::any(|method: axum::http::Method| async move { + if method == axum::http::Method::HEAD { + // A real Tigris HEAD 200 carries the generation ETag. Without + // it `head_etag` errors and the test would exercise the HEAD + // arm, not the download arm this test exists for. + let mut resp = axum::http::StatusCode::OK.into_response(); + resp.headers_mut() + .insert("etag", axum::http::HeaderValue::from_static("\"gen-1\"")); + resp + } else { + axum::http::StatusCode::INTERNAL_SERVER_ERROR.into_response() + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let endpoint = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + let server = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 2).await; + let store = RepoStore::for_testing_with_tigris( + PathBuf::from("/tmp/gitlawb-getfail-write"), + lock_pool, + TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint), + ); + + let err = match store + .acquire_write("did:key:z6MkGetFailWrite", "writerepo") + .await + { + Err(e) => e, + Ok(guard) => { + let _ = guard.release(false).await; + panic!("a failed download with no local copy must refuse the write"); + } + }; + assert!( + err.downcast_ref::().is_some(), + "the refusal must be typed so the handler layer maps it to a retryable 503, got {err:#}" + ); + + server.abort(); + } + + /// The transfer bound is a knob, so it gets the same parse/default/reject-zero + /// coverage its sibling lock-pool-size knob has. + #[test] + fn lock_held_transfer_timeout_defaults_and_rejects_zero() { + use clap::Parser; + assert_eq!( + crate::config::Config::parse_from(["gitlawb-node"]).lock_held_transfer_timeout_secs, + 300 + ); + assert!(crate::config::Config::try_parse_from([ + "gitlawb-node", + "--lock-held-transfer-timeout-secs", + "0" + ]) + .is_err()); + } + + /// P1a: the non-owner pre-check must refresh from a NON-MUTATING snapshot. + /// A snapshot download must unpack into a throwaway temp dir and leave the + /// live repo path untouched, so an unlocked pre-check cannot delete or swap + /// the directory under a concurrent guarded write. + /// + /// Real S3 server (not a mock): upload an archive, then `read_snapshot` it, + /// and assert the snapshot path is a fresh temp dir distinct from the live + /// path, that the live path was never created, and that the snapshot reads + /// the same content. + #[sqlx::test] + async fn read_snapshot_is_non_mutating(pool: PgPool) { + use axum::response::IntoResponse; + + // A real in-process S3-compatible server via the SDK against an axum + // router is more plumbing than this test needs; instead, upload through + // the real Tigris client against an axum server that stores the object + // in memory, then snapshot through the same store. + // + // Simpler and equally load-bearing: build the archive bytes, serve them + // with a real HTTP server that answers HEAD 200 and GET with the bytes, + // then call read_snapshot and assert the live path is untouched and the + // snapshot content matches. + let mut archive_bytes = Vec::new(); + { + let dir = + std::env::temp_dir().join(format!("gitlawb-snap-src-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(dir.join("objects/info")).unwrap(); + std::fs::create_dir_all(dir.join("refs/heads")).unwrap(); + std::fs::write(dir.join("HEAD"), "ref: refs/heads/main\n").unwrap(); + std::fs::write(dir.join("objects/info/packs"), "").unwrap(); + let encoder = zstd::stream::Encoder::new(&mut archive_bytes, 3).unwrap(); + let mut tar = tar::Builder::new(encoder); + tar.append_dir_all(".", &dir).unwrap(); + tar.into_inner().unwrap().finish().unwrap(); + std::fs::remove_dir_all(&dir).unwrap(); + } + let archive = std::sync::Arc::new(archive_bytes); + + let app = axum::Router::new().route( + "/{*key}", + axum::routing::any(move |method: axum::http::Method| { + let archive = archive.clone(); + async move { + match method { + axum::http::Method::HEAD => axum::http::StatusCode::OK.into_response(), + axum::http::Method::GET => { + use axum::body::Body; + ( + [(axum::http::header::CONTENT_TYPE, "application/zstd")], + Body::from(archive.as_ref().clone()), + ) + .into_response() + } + _ => axum::http::StatusCode::METHOD_NOT_ALLOWED.into_response(), + } + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let endpoint = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + let server = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + + let opts = (*pool.connect_options()).clone(); + let lock_pool = no_reap_pool(&opts, 2).await; + let store = RepoStore::for_testing_with_tigris( + PathBuf::from("/tmp/gitlawb-snapshot-nonmut"), + lock_pool, + TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint), + ); + + let owner_did = "did:key:z6MkSnap"; + let (owner_slug, live_path) = store.local_path(owner_did, "snaprepo").unwrap(); + assert!( + !live_path.exists(), + "the live path must not exist before the snapshot" + ); + + let snap = store + .read_snapshot(owner_did, "snaprepo") + .await + .expect("snapshot reads the archive"); + let snap_path = snap.path().to_path_buf(); + assert_ne!( + snap_path, live_path, + "the snapshot must unpack into a temp dir, not the live path" + ); + assert!( + snap_path.starts_with(live_path.parent().unwrap()), + "the snapshot temp dir must live under the repo parent" + ); + assert!( + !live_path.exists(), + "the live path must remain untouched by a snapshot read" + ); + assert_eq!( + std::fs::read_to_string(snap_path.join("HEAD")).unwrap(), + "ref: refs/heads/main\n", + "the snapshot must contain the archive's content" + ); + drop(snap); + assert!( + !snap_path.exists(), + "dropping the snapshot must clean up its temp dir" + ); + let _ = owner_slug; + + server.abort(); + } + + /// Build a store whose release-side upload lands on `mock` and gives up + /// after 200ms, so a parked PUT reliably exceeds the bound. One lock-pool + /// connection on purpose: with a single slot the backend pid is a direct + /// observable for whether `release` pooled its session or closed it. + async fn timed_out_upload_store( + mock: &S3Mock, + opts: &sqlx::postgres::PgConnectOptions, + repos_dir: &str, + ) -> RepoStore { + RepoStore::new( + PathBuf::from(repos_dir), + Some(TigrisClient::for_testing_with_endpoint( + "test-bucket", + mock.endpoint(), + )), + no_reap_pool(opts, 1).await, + std::time::Duration::from_millis(200), + ) + } + + /// Seed the minimum bare-repo shape so the upload has something to archive. + fn seed_bare_repo(path: &Path) { + std::fs::create_dir_all(path.join("objects/info")).unwrap(); + std::fs::create_dir_all(path.join("refs/heads")).unwrap(); + std::fs::write(path.join("HEAD"), "ref: refs/heads/main\n").unwrap(); + } + + /// A timed-out release upload must still unlock on its OWN session and hand + /// that session back to the pool. The timeout says nothing about the lock: + /// holding it cannot fence a late PUT (`release` takes `mut self`, so the + /// guard drops and `Drop` frees the session the moment `release` returns), + /// and what actually protects a successor is the conditional PUT. + /// + /// Observable: the backend pid. On a one-connection pool a session that was + /// closed forces the next checkout onto a fresh backend, while a confirmed + /// unlock returns the same one. So an equal pid is the proof that the unlock + /// ran, returned true, and the connection was pooled rather than torn down. + #[sqlx::test] + async fn timed_out_release_upload_unlocks_on_its_own_session(pool: PgPool) { + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let store = timed_out_upload_store(&mock, &opts, "/tmp/gitlawb-u4-timeout-session").await; + + let mut guard = store + .acquire_write("did:key:z6MkU4TimeoutSess", "timedrepo") + .await + .expect("acquire"); + seed_bare_repo(&guard.local_path); + let pid_before = guard.backend_pid_for_test().await; + + // Park the upload so it is still in flight when the 200ms bound fires. + mock.park_next_put(); + let _ = guard.release(true).await; + assert_eq!( + mock.put_attempts().len(), + 1, + "the release upload must have reached the mock and parked, got {:?}", + mock.put_attempts() + ); + + let pid_after = { + let mut c = store.lock_pool.acquire().await.unwrap(); + let pid: (i32,) = sqlx::query_as("SELECT pg_backend_pid()") + .fetch_one(&mut *c) + .await + .unwrap(); + pid.0 + }; + assert_eq!( + pid_before, pid_after, + "a timed-out upload must not change the unlock decision: the guard must \ + unlock on its own session and return that connection to the pool, so the \ + next checkout lands on the same backend" + ); + + mock.open_gate(); + mock.shutdown(); + } + + /// Admission after the same timed-out upload: a successor must be let in + /// promptly. Useful as a property, but it is NOT what pins the removal of + /// the skip-unlock branch, because the lock frees within milliseconds under + /// either shape (the session closes as soon as `release` returns). + #[sqlx::test] + async fn successor_is_admitted_promptly_after_a_timed_out_release(pool: PgPool) { + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let store = timed_out_upload_store(&mock, &opts, "/tmp/gitlawb-u4-timeout-admit") + .await + .with_lock_acquire_deadline(std::time::Duration::from_secs(10)); + + let guard = store + .acquire_write("did:key:z6MkU4TimeoutAdmit", "timedrepo") + .await + .expect("acquire"); + seed_bare_repo(&guard.local_path); + + mock.park_next_put(); + let _ = guard.release(true).await; + + let started = std::time::Instant::now(); + let successor = store + .acquire_write("did:key:z6MkU4TimeoutAdmit", "timedrepo") + .await + .expect("a successor must be admitted after a timed-out release"); + assert!( + started.elapsed() < std::time::Duration::from_secs(5), + "the successor waited {}ms; a timed-out upload must not park the next writer", + started.elapsed().as_millis() + ); + let _ = successor.release(false).await; + + mock.open_gate(); + mock.shutdown(); + } + + /// P2: the lock-acquire deadline must bound EVERY await in the retry loop, + /// not just the sleep between attempts. A pool checkout that would exceed + /// the deadline must shed as `RepoBusy` rather than wait out the pool's own + /// acquire timeout past the promised wall-clock cap. + /// + /// Observable: hold every lock-pool slot from an independent store, then + /// acquire with a short deadline. The pool checkout will not complete within + /// the deadline, so `acquire_write` must refuse as `RepoBusy` once the + /// deadline fires — not hang for the pool's 5s acquire timeout. + #[sqlx::test] + async fn pool_checkout_past_the_deadline_sheds_as_repo_busy(pool: PgPool) { + let opts = (*pool.connect_options()).clone(); + + // Exhaust every slot of the lock pool. The checkouts must come from the + // pool the store will use, not from independent connections: a separate + // `PgConnection::connect_with` consumes no slot, so the store's checkout + // would succeed immediately and the deadline would never be reached. + const N: u32 = 2; + let lock_pool = no_reap_pool(&opts, N).await; + let mut holders = Vec::new(); + for _ in 0..N { + holders.push(lock_pool.acquire().await.expect("hold a lock-pool slot")); + } + + // The store shares that exhausted pool (`PgPool` is a handle to one + // inner pool, so the clone is the same set of slots). + let store = + RepoStore::for_testing(PathBuf::from("/tmp/gitlawb-deadline"), lock_pool.clone()) + .with_lock_acquire_deadline(std::time::Duration::from_millis(400)); + + // The pool is exhausted, so the checkout cannot complete within the + // deadline; the deadline must fire and shed as RepoBusy rather than let + // the pool's own 5s acquire timeout run. + let started = std::time::Instant::now(); + let err = match store + .acquire_write("did:key:z6MkDeadline", "deadline-repo") + .await + { + Err(e) => e, + Ok(guard) => { + let _ = guard.release(false).await; + panic!("with the pool exhausted, the deadline must shed, not succeed"); + } + }; + assert!( + err.downcast_ref::().is_some(), + "a checkout past the deadline must shed as RepoBusy, got {err:#}" + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(5), + "the refusal must come from the deadline, not the pool's own 5s acquire timeout" + ); + + // Release the holders so the test's pool can be torn down cleanly. + drop(holders); + } + + // ── conditional-semantics S3 mock (#279) ─────────────────────────────── + + /// A captured PUT: the body and the conditional headers as they arrived. + /// + /// Capture is deliberately separate from evaluation. When tokio drops an + /// SDK future the client can tear the TCP connection down and the server + /// side handler task is cancelled with it, so a parked handler that resumes + /// on its own is not something a test can depend on. Replaying what arrived + /// models the real S3 arm we care about (body fully transmitted, commit + /// decided later) with no timing in it. + #[derive(Clone, Debug)] + struct CapturedPut { + body: Vec, + if_match: Option, + if_none_match: Option, + } + + /// One PUT as the mock judged it, for tests that assert on attempt counts. + /// `status` is `None` while a PUT is parked: it arrived and was logged, but + /// no precondition has been evaluated for it yet. + #[derive(Clone, Debug, PartialEq)] + struct PutAttempt { + if_match: Option, + if_none_match: Option, + status: Option, + } + + #[derive(Default)] + struct MockState { + object: Option>, + etag: Option, + next_etag: u64, + puts: Vec, + /// Set by `park_next_put`, consumed by the next arriving PUT. + park_next_put: bool, + /// Set by `roll_generation_after_next_heads`, decremented per HEAD. + roll_after_heads: u32, + captured: Option, + } + + /// An in-process S3-compatible server with REAL conditional semantics. + /// + /// The fence tests downstream are only worth anything if a precondition can + /// actually fail here, so this helper carries its own semantics tests below. + struct S3Mock { + endpoint: String, + state: Arc>, + gate: Arc, + server: tokio::task::JoinHandle<()>, + } + + /// S3 quotes ETags. Compare unquoted so a value that round-tripped through + /// the SDK (which surfaces `e_tag()` with the quotes intact) matches what + /// the mock minted. + fn unquote_etag(raw: &str) -> &str { + raw.trim().trim_matches('"') + } + + /// The conditional evaluation, in one place so a live PUT and a replayed + /// one cannot drift apart. Returns the status, and on success the fresh + /// ETag. Preconditions are read against the state passed in, which is + /// always the state as of the CALL, never as of capture. + fn evaluate_put( + st: &mut MockState, + body: Vec, + if_match: Option<&str>, + if_none_match: Option<&str>, + ) -> (u16, Option) { + let refuse = |st: &mut MockState| { + st.puts.push(PutAttempt { + if_match: if_match.map(str::to_string), + if_none_match: if_none_match.map(str::to_string), + status: Some(412), + }); + (412u16, None) + }; + + if let Some(want) = if_match { + // An absent object matches nothing, so If-Match cannot pass. + match st.etag.as_deref() { + Some(have) if unquote_etag(have) == unquote_etag(want) => {} + _ => return refuse(st), + } + } + if if_none_match.map(str::trim) == Some("*") && st.object.is_some() { + return refuse(st); + } + + // A fresh ETag per successful PUT, from a counter rather than a content + // hash: two writers can publish byte-identical archives, and an ETag + // that repeated across them would let a fence pass on a generation it + // never observed. + st.next_etag += 1; + let etag = format!("\"mock-etag-{}\"", st.next_etag); + st.object = Some(body); + st.etag = Some(etag.clone()); + st.puts.push(PutAttempt { + if_match: if_match.map(str::to_string), + if_none_match: if_none_match.map(str::to_string), + status: Some(200), + }); + (200, Some(etag)) + } + + impl S3Mock { + async fn start() -> Self { + use axum::response::IntoResponse; + + let state = Arc::new(std::sync::Mutex::new(MockState::default())); + let gate = Arc::new(tokio::sync::Notify::new()); + + let app = axum::Router::new().route( + "/{*key}", + axum::routing::any({ + let state = state.clone(); + let gate = gate.clone(); + move |method: axum::http::Method, + headers: axum::http::HeaderMap, + body: axum::body::Bytes| { + let state = state.clone(); + let gate = gate.clone(); + async move { + let header = |name: &str| { + headers + .get(name) + .and_then(|v| v.to_str().ok()) + .map(str::to_string) + }; + match method { + axum::http::Method::PUT => { + let if_match = header("if-match"); + let if_none_match = header("if-none-match"); + + // A parked PUT records what arrived and then + // waits. The client will usually be gone by + // the time the gate opens, which is exactly + // why the deterministic arm is the replay. + let parked = { + let mut st = state.lock().unwrap(); + if st.park_next_put { + st.park_next_put = false; + st.puts.push(PutAttempt { + if_match: if_match.clone(), + if_none_match: if_none_match.clone(), + status: None, + }); + st.captured = Some(CapturedPut { + body: body.to_vec(), + if_match: if_match.clone(), + if_none_match: if_none_match.clone(), + }); + true + } else { + false + } + }; + if parked { + gate.notified().await; + return axum::http::StatusCode::OK.into_response(); + } + + let (status, etag) = { + let mut st = state.lock().unwrap(); + evaluate_put( + &mut st, + body.to_vec(), + if_match.as_deref(), + if_none_match.as_deref(), + ) + }; + match etag { + Some(etag) => ( + axum::http::StatusCode::OK, + [(axum::http::header::ETAG, etag)], + ) + .into_response(), + None => axum::http::StatusCode::from_u16(status) + .unwrap() + .into_response(), + } + } + axum::http::Method::HEAD | axum::http::Method::GET => { + let mut st = state.lock().unwrap(); + let answered = (st.object.clone(), st.etag.clone()); + // Fault injection for the two-consecutive- + // losses arm, and the only deterministic way + // to sit BETWEEN a caller's HEAD and the + // conditional PUT it derives from it. The + // gate cannot do this: a parked PUT is + // captured rather than evaluated and answers + // 200, so it can never produce a refusal. + // + // Only the generation moves, not the bytes, + // which is a real state a store reaches (two + // writers can publish byte-identical + // archives) and keeps the stored object a + // valid archive for whoever downloads next. + // `evaluate_put` is untouched, and this logs + // no PutAttempt, so attempt counts still + // count only the caller's own PUTs. + if method == axum::http::Method::HEAD + && st.roll_after_heads > 0 + && st.object.is_some() + { + st.roll_after_heads -= 1; + st.next_etag += 1; + st.etag = Some(format!("\"mock-etag-{}\"", st.next_etag)); + } + match answered { + (Some(bytes), Some(etag)) => ( + axum::http::StatusCode::OK, + [(axum::http::header::ETAG, etag)], + axum::body::Body::from(bytes), + ) + .into_response(), + _ => axum::http::StatusCode::NOT_FOUND.into_response(), + } + } + _ => axum::http::StatusCode::METHOD_NOT_ALLOWED.into_response(), + } + } + } + }), + ); + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let endpoint = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + let server = tokio::spawn(async move { + let _ = axum::serve(listener, app).await; + }); + + Self { + endpoint, + state, + gate, + server, + } + } + + fn endpoint(&self) -> &str { + &self.endpoint + } + + fn current_etag(&self) -> Option { + self.state.lock().unwrap().etag.clone() + } + + fn object(&self) -> Option> { + self.state.lock().unwrap().object.clone() + } + + fn put_attempts(&self) -> Vec { + self.state.lock().unwrap().puts.clone() + } + + /// Answer each of the next `n` HEADs from the current state, then + /// immediately move the object to a new generation. A caller that HEADs + /// to pick up a precondition and then PUTs on it is therefore fencing + /// on a generation that is already gone, which is the only way to drive + /// two consecutive lost preconditions deterministically. + fn roll_generation_after_next_heads(&self, n: u32) { + self.state.lock().unwrap().roll_after_heads = n; + } + + /// Park the next arriving PUT so the caller's transfer bound elapses + /// with the request in flight (the abandoned-writer arm). + fn park_next_put(&self) { + self.state.lock().unwrap().park_next_put = true; + } + + /// Let a parked handler go. Only the socket-level arm needs this; the + /// deterministic assertion is `replay_captured`. + fn open_gate(&self) { + self.gate.notify_waiters(); + } + + fn captured_put(&self) -> Option { + self.state.lock().unwrap().captured.clone() + } + + /// Re-run the captured PUT through the SAME evaluation the handler uses, + /// against the state as it is NOW. + fn replay_captured(&self) -> u16 { + let mut st = self.state.lock().unwrap(); + let captured = st.captured.clone().expect("a PUT was captured"); + evaluate_put( + &mut st, + captured.body, + captured.if_match.as_deref(), + captured.if_none_match.as_deref(), + ) + .0 + } + + fn shutdown(&self) { + self.server.abort(); + } + } + + /// An SDK client aimed at the mock. Built here rather than through + /// `TigrisClient` because these tests exercise raw conditional PUTs, which + /// the storage client does not expose. + fn mock_s3_client(endpoint: &str) -> aws_sdk_s3::Client { + use aws_sdk_s3::config::{retry::RetryConfig, Credentials, Region}; + + let config = aws_sdk_s3::config::Config::builder() + .endpoint_url(endpoint) + .credentials_provider(Credentials::new("test", "test", None, None, "test")) + .region(Region::new("auto")) + .retry_config(RetryConfig::disabled()) + .behavior_version_latest() + .build(); + aws_sdk_s3::Client::from_conf(config) + } + + /// PUT through the SDK, returning either the fresh ETag or the HTTP status + /// the mock refused with. + async fn mock_put( + client: &aws_sdk_s3::Client, + body: &[u8], + if_match: Option<&str>, + if_none_match: Option<&str>, + ) -> Result { + let mut req = client + .put_object() + .bucket("test-bucket") + .key("repos/v1/owner/repo.tar.zst") + .body(aws_sdk_s3::primitives::ByteStream::from(body.to_vec())); + if let Some(v) = if_match { + req = req.if_match(v); + } + if let Some(v) = if_none_match { + req = req.if_none_match(v); + } + match req.send().await { + Ok(out) => Ok(out + .e_tag() + .expect("a successful PUT returns an ETag") + .to_string()), + Err(e) => Err(e + .raw_response() + .map(|r| r.status().as_u16()) + .unwrap_or_else(|| panic!("expected an HTTP response from the mock, got {e:?}"))), + } + } + + /// HEAD through the SDK, reported the way `TigrisClient::exists` reports it: + /// `Ok(false)` for a not-found, `Ok(true)` for a hit whose ETag is present. + async fn mock_head(client: &aws_sdk_s3::Client) -> Result { + match client + .head_object() + .bucket("test-bucket") + .key("repos/v1/owner/repo.tar.zst") + .send() + .await + { + Ok(out) => { + out.e_tag().ok_or("a HEAD hit must carry an ETag")?; + Ok(true) + } + Err(e) if e.as_service_error().is_some_and(|e| e.is_not_found()) => Ok(false), + Err(e) => Err(format!("unexpected HEAD failure: {e}")), + } + } + + /// 1. A stale If-Match must be refused, and the refusal must not write. + #[tokio::test] + async fn mock_refuses_a_wrong_if_match_and_leaves_the_object_unchanged() { + let mock = S3Mock::start().await; + let client = mock_s3_client(mock.endpoint()); + + let etag = mock_put(&client, b"first", None, None) + .await + .expect("the seeding PUT succeeds"); + + let status = mock_put(&client, b"second", Some("\"not-the-current-etag\""), None) + .await + .expect_err("a stale If-Match must be refused"); + assert_eq!(status, 412, "a stale If-Match must answer 412"); + assert_eq!( + mock.object().as_deref(), + Some(b"first".as_slice()), + "a refused PUT must leave the stored object unchanged" + ); + assert_eq!( + mock.current_etag().as_deref().map(unquote_etag), + Some(unquote_etag(&etag)), + "a refused PUT must leave the ETag unchanged" + ); + + mock.shutdown(); + } + + /// 2. The matching If-Match is the write that must go through. + #[tokio::test] + async fn mock_accepts_a_matching_if_match_and_rotates_the_etag() { + let mock = S3Mock::start().await; + let client = mock_s3_client(mock.endpoint()); + + let first = mock_put(&client, b"first", None, None).await.expect("seed"); + let second = mock_put(&client, b"second", Some(&first), None) + .await + .expect("a matching If-Match must succeed"); + + assert_ne!( + unquote_etag(&first), + unquote_etag(&second), + "a successful conditional PUT must mint a fresh ETag" + ); + assert_eq!( + mock.object().as_deref(), + Some(b"second".as_slice()), + "the accepted body must be what is stored" + ); + assert_eq!( + mock.current_etag().as_deref().map(unquote_etag), + Some(unquote_etag(&second)), + "HEAD/GET must report the ETag the PUT returned" + ); + + mock.shutdown(); + } + + /// 3. If-None-Match `*` is the create-only fence, so an existing object + /// must refuse it. + #[tokio::test] + async fn mock_refuses_if_none_match_star_against_an_existing_object() { + let mock = S3Mock::start().await; + let client = mock_s3_client(mock.endpoint()); + + mock_put(&client, b"first", None, None).await.expect("seed"); + let status = mock_put(&client, b"second", None, Some("*")) + .await + .expect_err("create-only against an existing object must be refused"); + + assert_eq!(status, 412, "If-None-Match * on an existing object is 412"); + assert_eq!( + mock.object().as_deref(), + Some(b"first".as_slice()), + "the refused create-only PUT must not overwrite" + ); + + mock.shutdown(); + } + + /// 4. The same fence must ADMIT the first writer, or the fresh-repo path + /// could never publish. + #[tokio::test] + async fn mock_accepts_if_none_match_star_against_an_empty_store() { + let mock = S3Mock::start().await; + let client = mock_s3_client(mock.endpoint()); + + // HEAD both ways, because `exists()` reads a not-found as "fresh repo" + // and any other status as a hard refusal. A mock that answered 200 on + // an empty store would send every fresh-repo test down the wrong arm. + assert!( + !mock_head(&client).await.expect("HEAD on an empty store"), + "an absent object must HEAD 404" + ); + + let etag = mock_put(&client, b"first", None, Some("*")) + .await + .expect("create-only against an empty store must succeed"); + assert_eq!(mock.object().as_deref(), Some(b"first".as_slice())); + assert_eq!( + mock.current_etag().as_deref().map(unquote_etag), + Some(unquote_etag(&etag)) + ); + assert!( + mock_head(&client).await.expect("HEAD after the create"), + "a stored object must HEAD 200 with the ETag the PUT returned" + ); + + mock.shutdown(); + } + + /// 5. Identical bytes must still produce a new ETag. Without this, an + /// If-Match fence would pass on a generation it never observed. + #[tokio::test] + async fn mock_mints_a_distinct_etag_per_successful_put() { + let mock = S3Mock::start().await; + let client = mock_s3_client(mock.endpoint()); + + let first = mock_put(&client, b"same", None, None).await.expect("first"); + let second = mock_put(&client, b"same", Some(&first), None) + .await + .expect("second"); + + assert_ne!( + unquote_etag(&first), + unquote_etag(&second), + "successive successful PUTs of identical bytes must still differ in ETag" + ); + + mock.shutdown(); + } + + /// 6. The whole point of capture-and-replay: the commit is judged when it + /// is replayed, not when the bytes arrived. A capture that was valid on + /// arrival must lose to a write that landed in between. + #[tokio::test] + async fn mock_judges_a_replayed_put_against_the_state_at_replay_time() { + let mock = S3Mock::start().await; + let client = mock_s3_client(mock.endpoint()); + + let first = mock_put(&client, b"first", None, None).await.expect("seed"); + + // Park the abandoned writer's PUT. Its If-Match is valid at ARRIVAL. + mock.park_next_put(); + let parked = tokio::time::timeout( + std::time::Duration::from_millis(300), + mock_put(&client, b"abandoned", Some(&first), None), + ) + .await; + assert!( + parked.is_err(), + "the parked PUT must still be in flight when the caller's bound elapses" + ); + let captured = mock + .captured_put() + .expect("the parked PUT must be captured at arrival"); + assert_eq!(captured.body, b"abandoned".to_vec()); + assert_eq!( + captured.if_match.as_deref().map(unquote_etag), + Some(unquote_etag(&first)), + "the capture must record the conditional headers as they arrived" + ); + assert_eq!(captured.if_none_match, None); + + // A successor commits while the capture sits parked. + let second = mock_put(&client, b"successor", Some(&first), None) + .await + .expect("the successor's PUT is the one that lands"); + + // Replaying now must be judged against the successor's state. + assert_eq!( + mock.replay_captured(), + 412, + "a replayed PUT must be evaluated against the state at replay time, \ + not the state it was captured against" + ); + assert_eq!( + mock.object().as_deref(), + Some(b"successor".as_slice()), + "the refused replay must not clobber the successor's object" + ); + assert_eq!( + mock.current_etag().as_deref().map(unquote_etag), + Some(unquote_etag(&second)) + ); + // Seed, parked, successor, replay. The log is what later tests assert + // attempt counts against, so it is checked here rather than trusted. + let attempts = mock.put_attempts(); + assert_eq!( + attempts.len(), + 4, + "every PUT attempt must be logged, got {attempts:?}" + ); + assert_eq!( + attempts.iter().map(|a| a.status).collect::>(), + vec![Some(200), None, Some(200), Some(412)], + "the parked attempt is logged undecided; the replay is the 412" + ); + assert_eq!( + attempts[1].if_match.as_deref().map(unquote_etag), + Some(unquote_etag(&first)), + "the parked attempt must be logged with the headers it arrived with" + ); + + mock.open_gate(); + mock.shutdown(); + } + + // ── conditional upload through TigrisClient (#279) ───────────────────── + + /// A tiny directory for `upload` to compress. What is inside does not + /// matter to a precondition test, only that a PUT carrying a body happens. + fn payload_dir(marker: &str) -> TempDir { + let dir = TempDir::new().unwrap(); + std::fs::write(dir.path().join("HEAD"), marker.as_bytes()).unwrap(); + dir + } + + /// A router that answers every request with one fixed status. This is NOT a + /// second semantics mock: it exists only to pin how a status the real mock + /// never produces (409, 404, 500) is classified. + async fn start_fixed_status_stub(status: u16) -> (String, tokio::task::JoinHandle<()>) { + let app = axum::Router::new().route( + "/{*key}", + axum::routing::any( + move || async move { axum::http::StatusCode::from_u16(status).unwrap() }, + ), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let endpoint = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + let server = tokio::spawn(async move { + let _ = axum::serve(listener, app).await; + }); + (endpoint, server) + } + + /// The one place the header itself is asserted: a matching If-Match must + /// succeed AND must actually have travelled as an If-Match header. The + /// store-level tests deliberately assert behavior rather than headers, so + /// if this assertion is not here, nothing pins the wire format. + #[tokio::test] + async fn upload_if_match_with_the_current_etag_publishes_and_sends_the_header() { + let mock = S3Mock::start().await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()); + let seeded = mock_put(&mock_s3_client(mock.endpoint()), b"seed", None, None) + .await + .expect("seeding PUT"); + + let dir = payload_dir("winner"); + client + .upload( + "owner", + "repo", + dir.path(), + UploadPrecondition::IfMatch(seeded.clone()), + ) + .await + .expect("a matching If-Match must publish"); + + let last = mock + .put_attempts() + .last() + .cloned() + .expect("the upload must reach the mock"); + assert_eq!( + last.if_match.as_deref().map(unquote_etag), + Some(unquote_etag(&seeded)), + "the upload must carry the ETag it was fenced on as If-Match" + ); + assert_eq!(last.if_none_match, None); + assert_eq!(last.status, Some(200)); + + mock.shutdown(); + } + + #[tokio::test] + async fn upload_if_match_with_a_stale_etag_is_precondition_lost() { + let mock = S3Mock::start().await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()); + mock_put(&mock_s3_client(mock.endpoint()), b"seed", None, None) + .await + .expect("seeding PUT"); + + let dir = payload_dir("loser"); + let err = client + .upload( + "owner", + "repo", + dir.path(), + UploadPrecondition::IfMatch("\"stale\"".to_string()), + ) + .await + .expect_err("a stale If-Match must be refused"); + assert!( + matches!(err, UploadError::PreconditionLost { status: 412 }), + "a stale If-Match must classify as a lost precondition, got {err:?}" + ); + assert_eq!( + mock.object().as_deref(), + Some(b"seed".as_slice()), + "the refused upload must not have written" + ); + + mock.shutdown(); + } + + #[tokio::test] + async fn upload_if_absent_into_an_empty_store_publishes() { + let mock = S3Mock::start().await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()); + + let dir = payload_dir("first"); + client + .upload("owner", "repo", dir.path(), UploadPrecondition::IfAbsent) + .await + .expect("create-only into an empty store must publish"); + + let last = mock.put_attempts().last().cloned().expect("one attempt"); + assert_eq!(last.if_none_match.as_deref(), Some("*")); + assert_eq!(last.if_match, None); + assert!(mock.object().is_some(), "the create must have stored bytes"); + + mock.shutdown(); + } + + #[tokio::test] + async fn upload_if_absent_over_an_existing_object_is_precondition_lost() { + let mock = S3Mock::start().await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()); + mock_put(&mock_s3_client(mock.endpoint()), b"seed", None, None) + .await + .expect("seeding PUT"); + + let dir = payload_dir("late-backfill"); + let err = client + .upload("owner", "repo", dir.path(), UploadPrecondition::IfAbsent) + .await + .expect_err("create-only over an existing object must be refused"); + assert!( + matches!(err, UploadError::PreconditionLost { status: 412 }), + "got {err:?}" + ); + assert_eq!( + mock.object().as_deref(), + Some(b"seed".as_slice()), + "a refused backfill must not clobber what is already published" + ); + + mock.shutdown(); + } + + #[tokio::test] + async fn upload_unconditional_overwrites_regardless() { + let mock = S3Mock::start().await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()); + mock_put(&mock_s3_client(mock.endpoint()), b"seed", None, None) + .await + .expect("seeding PUT"); + + let dir = payload_dir("overwrite"); + client + .upload( + "owner", + "repo", + dir.path(), + UploadPrecondition::Unconditional, + ) + .await + .expect("an unconditional upload must succeed regardless of state"); + + let last = mock.put_attempts().last().cloned().expect("one attempt"); + assert_eq!(last.if_match, None, "no precondition may be sent"); + assert_eq!(last.if_none_match, None); + assert_ne!( + mock.object().as_deref(), + Some(b"seed".as_slice()), + "the unconditional upload must have replaced the seed" + ); + + mock.shutdown(); + } + + /// Tigris answers a create-only conflict with 409 rather than 412, so that + /// status has to classify as a lost precondition too, but ONLY when the + /// request was create-only. + #[tokio::test] + async fn upload_classifies_409_under_if_absent_as_precondition_lost() { + let (endpoint, server) = start_fixed_status_stub(409).await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + let dir = payload_dir("conflict"); + + let err = client + .upload("owner", "repo", dir.path(), UploadPrecondition::IfAbsent) + .await + .expect_err("409 must be an error"); + assert!( + matches!(err, UploadError::PreconditionLost { status: 409 }), + "409 under IfAbsent is a lost precondition, got {err:?}" + ); + + server.abort(); + } + + /// The P1 raw-response case: a conditional PUT refused with an UNPARSABLE + /// 409/412 body (malformed XML, premature close). The SDK cannot map that to + /// a modeled service error, so it surfaces as `SdkError::ResponseError`, and + /// the status has to be read off the raw response, not off a + /// `ServiceError`-only match. A lost precondition reported as `Other` here + /// would make `RepoWriteGuard::release` log-and-succeed instead of taking the + /// supersede retry, acknowledging a write that was definitively not + /// published. + #[tokio::test] + async fn upload_classifies_an_unparsable_409_as_precondition_lost() { + let app = axum::Router::new().route( + "/{*key}", + axum::routing::any(|| async { + ( + axum::http::StatusCode::CONFLICT, + "this is not xml, so the sdk cannot model an error from it", + ) + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let endpoint = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + let server = tokio::spawn(async move { + let _ = axum::serve(listener, app).await; + }); + let client = TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + let dir = payload_dir("unparsable-conflict"); + + let err = client + .upload("owner", "repo", dir.path(), UploadPrecondition::IfAbsent) + .await + .expect_err("409 must be an error"); + assert!( + matches!(err, UploadError::PreconditionLost { status: 409 }), + "an unparsable 409 under IfAbsent is still a lost precondition, got {err:?}" + ); + + server.abort(); + } + + #[tokio::test] + async fn upload_classifies_an_unparsable_412_as_precondition_lost() { + let app = axum::Router::new().route( + "/{*key}", + axum::routing::any(|| async { + (axum::http::StatusCode::PRECONDITION_FAILED, "also not xml") + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let endpoint = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + let server = tokio::spawn(async move { + let _ = axum::serve(listener, app).await; + }); + let client = TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + let dir = payload_dir("unparsable-stale"); + + let err = client + .upload( + "owner", + "repo", + dir.path(), + UploadPrecondition::IfMatch("\"stale\"".to_string()), + ) + .await + .expect_err("412 must be an error"); + assert!( + matches!(err, UploadError::PreconditionLost { status: 412 }), + "an unparsable 412 is a lost precondition, got {err:?}" + ); + + server.abort(); + } + + /// P2 (fork orphan): `release_after_write` must surface a refused create-only + /// upload as `PreconditionLost`, not swallow it as success. The fork handler + /// relies on that to refuse creating a DB record whose archive is shadowed by + /// an orphan (a failed `create_repo` left bytes under the key, or another + /// writer got there first). Without the propagation, the fork reports success + /// and every other node fetches the unrelated archive. + #[tokio::test] + async fn release_after_write_refuses_when_the_key_already_exists() { + let mock = S3Mock::start().await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()); + // Seed an orphan under the fork's would-be key. + mock_put(&mock_s3_client(mock.endpoint()), b"orphan", None, None) + .await + .expect("seeding the orphan archive"); + + let tmp = tempfile::TempDir::new().unwrap(); + let repos_dir = tmp.path().join("repos"); + // release_after_write("owner", "repo") uploads from local_path = + // /owner/repo.git, so the bare repo must exist there for the + // compress to have anything to read. + let local = repos_dir.join("owner").join("repo.git"); + std::fs::create_dir_all(local.parent().unwrap()).unwrap(); + store::init_bare(&local).expect("a bare repo to upload"); + + let store = RepoStore::new( + repos_dir, + Some(client), + sqlx::PgPool::connect_lazy(&std::env::var("DATABASE_URL").unwrap()).unwrap(), + Duration::from_secs(300), + ); + // The upload key is owner-slug/repo: mock_put seeded + // "repos/v1/owner/repo.tar.zst", and an owner_did of "owner" has no + // colons so its slug is exactly "owner" and the IfAbsent upload is + // refused against the seeded key. + let err = store + .release_after_write("owner", "repo") + .await + .expect_err("a create-only upload over an existing key must be refused"); + assert!( + matches!(err, UploadError::PreconditionLost { .. }), + "the fork upload must surface the lost precondition, got {err:?}" + ); + assert_eq!( + mock.object().as_deref(), + Some(b"orphan".as_slice()), + "the refused upload must not have replaced the orphan" + ); + + mock.shutdown(); + } + + /// MUST-NOT. A 404 is permanent (no such bucket, a misrouted endpoint), so + /// reporting it as a lost precondition would tell a client to retry + /// something that can never succeed. `delete` has no callers, so a racing + /// delete cannot produce this. + #[tokio::test] + async fn upload_classifies_404_as_other_under_either_precondition() { + let (endpoint, server) = start_fixed_status_stub(404).await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + + for precondition in [ + UploadPrecondition::IfAbsent, + UploadPrecondition::IfMatch("\"whatever\"".to_string()), + ] { + let dir = payload_dir("gone"); + let err = client + .upload("owner", "repo", dir.path(), precondition.clone()) + .await + .expect_err("404 must be an error"); + assert!( + matches!(err, UploadError::Other(_)), + "404 under {precondition:?} must NOT be a lost precondition, got {err:?}" + ); + } + + server.abort(); + } + + #[tokio::test] + async fn upload_classifies_500_as_other() { + let (endpoint, server) = start_fixed_status_stub(500).await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint); + + for precondition in [ + UploadPrecondition::IfAbsent, + UploadPrecondition::IfMatch("\"whatever\"".to_string()), + UploadPrecondition::Unconditional, + ] { + let dir = payload_dir("boom"); + let err = client + .upload("owner", "repo", dir.path(), precondition.clone()) + .await + .expect_err("500 must be an error"); + assert!( + matches!(err, UploadError::Other(_)), + "500 under {precondition:?} must be Other, got {err:?}" + ); + } + + server.abort(); + } + + #[tokio::test] + async fn head_etag_reports_the_current_etag_and_none_when_absent() { + let mock = S3Mock::start().await; + let client = TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()); + + assert_eq!( + client.head_etag("owner", "repo").await.expect("HEAD"), + None, + "an absent object must read as None, not an error" + ); + + let seeded = mock_put(&mock_s3_client(mock.endpoint()), b"seed", None, None) + .await + .expect("seeding PUT"); + let got = client + .head_etag("owner", "repo") + .await + .expect("HEAD") + .expect("a present object must report an ETag"); + assert_eq!( + unquote_etag(&got), + unquote_etag(&seeded), + "head_etag must report the ETag the last successful PUT minted" + ); + + mock.shutdown(); + } + + // ── the fenced release publish (#279) ────────────────────────────────── + + /// A store whose acquire-side refresh and release-side publish both land on + /// `mock`. The transfer bound is generous on purpose: these tests are about + /// the fence arms, and a short bound would let the timeout arm answer first. + async fn fenced_store( + mock: &S3Mock, + opts: &sqlx::postgres::PgConnectOptions, + repos_dir: &Path, + ) -> RepoStore { + RepoStore::new( + repos_dir.to_path_buf(), + Some(TigrisClient::for_testing_with_endpoint( + "test-bucket", + mock.endpoint(), + )), + no_reap_pool(opts, 2).await, + std::time::Duration::from_secs(30), + ) + } + + /// A client aimed at the same key the store under test publishes to, so a + /// test can seed the archive or land an interfering publish of its own. + fn mock_tigris(mock: &S3Mock) -> TigrisClient { + TigrisClient::for_testing_with_endpoint("test-bucket", mock.endpoint()) + } + + /// The slug `local_path` derives from a DID, needed because a test seeds + /// and reads the archive key directly. + fn owner_slug_of(owner_did: &str) -> String { + owner_did.replace([':', '/'], "_") + } + + /// A bare-repo-shaped directory carrying `marker`, so a test can tell whose + /// tree is stored without comparing compressed bytes. + fn marked_repo(path: &Path, marker: &str) { + seed_bare_repo(path); + std::fs::write(path.join("MARKER"), marker).unwrap(); + } + + /// The marker inside whatever archive is currently stored under the key. + async fn stored_marker(mock: &S3Mock, owner_slug: &str, repo_name: &str) -> String { + let out = TempDir::new().unwrap(); + let into = out.path().join("stored.git"); + mock_tigris(mock) + .download(owner_slug, repo_name, &into) + .await + .expect("the stored archive must be readable"); + std::fs::read_to_string(into.join("MARKER")).expect("the stored archive must be marked") + } + + /// A process-wide sink for warn-level tracing output, installed once. + /// + /// Global rather than per test on purpose. `tracing`'s scoped default is + /// thread-local, and these events fire inside futures the test runtime may + /// move between threads, so a scoped subscriber would drop them silently + /// and every log assertion would go vacuous. Tests instead give their repo + /// a unique name and read back only the lines carrying it. + fn log_sink() -> Arc>> { + static LOG_SINK: std::sync::OnceLock>>> = + std::sync::OnceLock::new(); + LOG_SINK + .get_or_init(|| { + let sink = Arc::new(std::sync::Mutex::new(Vec::new())); + let writer = sink.clone(); + // `try_init`, because another test may already have installed a + // subscriber; the assertions below fail loudly if nothing was + // captured, so a silent no-op here cannot pass for a green run. + let _ = tracing_subscriber::fmt() + .with_writer(move || SinkWriter(writer.clone())) + .with_ansi(false) + .with_max_level(tracing::Level::WARN) + .try_init(); + sink + }) + .clone() + } + + struct SinkWriter(Arc>>); + + impl std::io::Write for SinkWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + /// Captured warn lines naming `repo_name`, joined back into one string. + fn warn_lines_for(repo_name: &str) -> String { + let raw = log_sink().lock().unwrap().clone(); + String::from_utf8_lossy(&raw) + .lines() + .filter(|l| l.contains(repo_name)) + .collect::>() + .join("\n") + } + + /// An uncontended write publishes, and what lands is the writer's tree. + /// + /// This is the must-not-spuriously-fence negative, so it asserts ONLY the + /// outcome and the stored bytes, never which precondition header travelled. + /// Forcing the carried precondition back to `Unconditional` has to leave it + /// green, or it is a second copy of the fix rather than a guard against it; + /// the header itself is pinned by + /// `upload_if_match_with_the_current_etag_publishes_and_sends_the_header`. + #[sqlx::test] + async fn uncontended_write_publishes_the_writers_tree(pool: PgPool) { + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let store = fenced_store(&mock, &opts, repos.path()).await; + let owner = "did:key:z6MkFenceUncontended"; + let slug = owner_slug_of(owner); + + // Seed an archive so the acquire takes the download arm, which is the + // ordinary case: the repo already exists in object storage. + let seed = TempDir::new().unwrap(); + marked_repo(seed.path(), "seed"); + mock_tigris(&mock) + .upload( + &slug, + "repo", + seed.path(), + UploadPrecondition::Unconditional, + ) + .await + .expect("seeding the archive"); + + let guard = store.acquire_write(owner, "repo").await.expect("acquire"); + std::fs::write(guard.local_path.join("MARKER"), "writer").unwrap(); + guard + .release(true) + .await + .into_result() + .expect("an uncontended write must publish"); + + assert_eq!( + stored_marker(&mock, &slug, "repo").await, + "writer", + "an uncontended write must publish the writer's tree" + ); + + mock.shutdown(); + } + + /// The first write to an empty bucket, the absent-at-acquire path. Nothing + /// is stored when the lock is taken, so the publish is the one that creates + /// the key, and the writer's tree is what lands. + #[sqlx::test] + async fn first_write_to_an_empty_bucket_publishes_the_writers_tree(pool: PgPool) { + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let store = fenced_store(&mock, &opts, repos.path()).await; + let owner = "did:key:z6MkFenceFirstWrite"; + let slug = owner_slug_of(owner); + + let guard = store.acquire_write(owner, "repo").await.expect("acquire"); + marked_repo(&guard.local_path, "writer"); + guard + .release(true) + .await + .into_result() + .expect("the first write into an empty key must publish"); + + assert_eq!( + stored_marker(&mock, &slug, "repo").await, + "writer", + "the first write must publish the writer's tree into the empty key" + ); + + mock.shutdown(); + } + + /// THE INIT RACE, and the reason the fence needs a supersede-retry at all. + /// + /// `init` uploads a freshly created EMPTY repo create-only in the + /// background. A user who pushes immediately after creating a repo takes + /// the lock, sees nothing stored, and is fenced create-only too; the + /// background upload then wins the empty key and the push's publish loses. + /// A fence with no retry would turn every such push into a refusal. + /// + /// The retry is sound because the loss is DEFINITE and this writer still + /// holds the lock: what landed underneath was published without it, so this + /// tree supersedes it. + #[sqlx::test] + async fn a_lost_fence_republishes_once_and_the_writers_tree_wins(pool: PgPool) { + let _sink = log_sink(); + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let store = fenced_store(&mock, &opts, repos.path()).await; + let owner = "did:key:z6MkFenceInitRace"; + let repo = "fence-init-race-repo"; + let slug = owner_slug_of(owner); + + // Nothing is stored yet, so the acquire records the absent case. + let guard = store.acquire_write(owner, repo).await.expect("acquire"); + marked_repo(&guard.local_path, "writer"); + + // init's create-only background upload lands after that observation. + let empty = TempDir::new().unwrap(); + marked_repo(empty.path(), "empty-init"); + mock_tigris(&mock) + .upload(&slug, repo, empty.path(), UploadPrecondition::IfAbsent) + .await + .expect("the background init upload wins the empty key"); + let before = mock.put_attempts().len(); + assert_eq!(before, 1, "only the init upload has run so far"); + + guard + .release(true) + .await + .into_result() + .expect("the supersede-retry must leave the release reporting success"); + + let attempts = mock.put_attempts(); + assert_eq!( + attempts.len(), + before + 2, + "the release must attempt exactly twice, the fenced publish and one \ + supersede-retry, got {attempts:?}" + ); + assert_eq!( + attempts[before].status, + Some(412), + "the create-only publish must lose to what landed underneath, got {attempts:?}" + ); + assert_eq!( + attempts[before + 1].status, + Some(200), + "the supersede-retry must publish, got {attempts:?}" + ); + assert_eq!( + stored_marker(&mock, &slug, repo).await, + "writer", + "the lock holder's tree must be what is stored after the retry" + ); + assert!( + warn_lines_for(repo).contains("republishing"), + "the fired fence must be visible in the log, got {:?}", + warn_lines_for(repo) + ); + + mock.shutdown(); + } + + /// Two consecutive definite losses: the retry is bounded at ONE, so the + /// release refuses instead of escalating, and the refusal reaches the + /// caller rather than being logged and swallowed. + /// + /// The second loss is arranged by replacing the object right after the + /// re-HEAD answers, so the retry fences on a generation that is already + /// gone. That is fault injection at the only point where it can be + /// deterministic; the mock's PUT gate cannot do it, because a parked PUT is + /// captured rather than evaluated and answers 200. + #[sqlx::test] + async fn a_second_consecutive_loss_refuses_and_never_attempts_a_third(pool: PgPool) { + let _sink = log_sink(); + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let store = fenced_store(&mock, &opts, repos.path()).await; + let owner = "did:key:z6MkFenceDoubleLoss"; + let repo = "fence-double-loss-repo"; + let slug = owner_slug_of(owner); + + let seed = TempDir::new().unwrap(); + marked_repo(seed.path(), "seed"); + mock_tigris(&mock) + .upload(&slug, repo, seed.path(), UploadPrecondition::Unconditional) + .await + .expect("seeding the archive"); + + let guard = store.acquire_write(owner, repo).await.expect("acquire"); + std::fs::write(guard.local_path.join("MARKER"), "writer").unwrap(); + + // An unlocked publish lands after the acquire, so the carried fence is + // already stale before the release runs. + let orphan = TempDir::new().unwrap(); + marked_repo(orphan.path(), "orphan"); + mock_tigris(&mock) + .upload( + &slug, + repo, + orphan.path(), + UploadPrecondition::Unconditional, + ) + .await + .expect("an unconditional publish always lands"); + let before = mock.put_attempts().len(); + assert_eq!(before, 2, "the seed and the orphan have run so far"); + + // ... and the generation the retry HEADs for moves on before its PUT + // can use it, so the second attempt loses too. + mock.roll_generation_after_next_heads(1); + let outcome = guard.release(true).await; + + assert!( + matches!(outcome, ReleaseOutcome::Fenced), + "a publish refused twice must be reported to the caller, got {outcome:?}" + ); + let attempts = mock.put_attempts(); + assert_eq!( + attempts.len(), + before + 2, + "a release must attempt at most TWO publishes, never a third, got {attempts:?}" + ); + assert_eq!( + (attempts[before].status, attempts[before + 1].status), + (Some(412), Some(412)), + "both attempts must have been refused by the store, got {attempts:?}" + ); + let logged = warn_lines_for(repo); + assert!( + logged.contains("republishing"), + "the first loss must log the retry, got {logged:?}" + ); + assert!( + logged.contains("refusing the write"), + "the second loss must log its own distinct refusal, got {logged:?}" + ); + + mock.shutdown(); + } + + /// Driven from the client side at a publishing site: a refused publish must + /// render as the retryable 503 and never as a success body. + /// + /// `create_issue` bumps the author's trust score AFTER releasing the guard, + /// so an unchanged score is what proves the short-circuit actually precedes + /// the post-release effects rather than merely being written above them. A + /// `?` placed after the bump would leave the status assertion green and + /// this one red. + #[sqlx::test] + async fn a_fenced_publish_renders_as_503_and_skips_the_post_release_effects(pool: PgPool) { + use tower::ServiceExt; + + let _sink = log_sink(); + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let owner = "did:key:z6MkFenceHandlerAuthor"; + let repo = "fence-handler-repo"; + let slug = owner_slug_of(owner); + + let mut state = crate::test_support::test_state(pool.clone()).await; + state.repo_store = fenced_store(&mock, &opts, repos.path()).await; + let now = chrono::Utc::now(); + state + .db + .create_repo(&crate::db::RepoRecord { + id: uuid::Uuid::new_v4().to_string(), + name: repo.to_string(), + owner_did: owner.to_string(), + description: None, + is_public: true, + default_branch: "main".to_string(), + created_at: now, + updated_at: now, + disk_path: format!("/tmp/{repo}"), + forked_from: None, + machine_id: None, + }) + .await + .expect("seed repo"); + // The trust bump only moves a row that already exists, so the author has + // to be registered or the observable would be vacuously unchanged. + state + .db + .register_agent(owner, &[]) + .await + .expect("register the author"); + let score_before = state.db.get_trust_score(owner).await.expect("trust score"); + + // A real bare repo, on disk and published, so the acquire refresh has a + // valid archive to download and the handler's git work succeeds. + let local = repos.path().join(&slug).join(format!("{repo}.git")); + store::init_bare(&local).expect("init the bare repo"); + mock_tigris(&mock) + .upload(&slug, repo, &local, UploadPrecondition::Unconditional) + .await + .expect("publish the archive"); + + // Move the generation on after BOTH of the handler's HEADs: the one in + // `acquire_write`, so the fence it carries is stale by the time it + // publishes, and the one the supersede-retry does, so the retry loses + // too and the release refuses. + mock.roll_generation_after_next_heads(2); + + let router = axum::Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/issues", + axum::routing::post(crate::api::issues::create_issue), + ) + .with_state(state.clone()); + let resp = router + .oneshot(crate::test_support::signed_request_as( + owner, + axum::http::Method::POST, + &format!("/api/v1/repos/{owner}/{repo}/issues"), + axum::body::Body::from(r#"{"title":"t","body":"b"}"#), + )) + .await + .unwrap(); + + assert_eq!( + resp.status(), + axum::http::StatusCode::SERVICE_UNAVAILABLE, + "a publish the store refused must be a retryable 503, not a success" + ); + let body = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .expect("body"); + let body = String::from_utf8_lossy(&body); + assert!( + body.contains("repo_write_fenced"), + "the 503 must carry its own code so a client can tell it from contention, got {body}" + ); + assert!( + !body.contains(repo) && !body.contains(&slug), + "the body must be fixed and must not name the repo or owner, got {body}" + ); + assert_eq!( + state.db.get_trust_score(owner).await.expect("trust score"), + score_before, + "the post-release trust bump must not run when the publish was refused" + ); + + mock.shutdown(); + } + + // ── the abandoned writer's late PUT (#279) ───────────────────────────── + + /// A store whose under-lock transfer bound is short, so a parked PUT + /// actually runs the release past its budget instead of making the test sit + /// out `fenced_store`'s 30s. The acquire-side refresh shares the bound, + /// which is fine here: it moves a few KiB against an in-process mock. + async fn fenced_store_with_bound( + mock: &S3Mock, + opts: &sqlx::postgres::PgConnectOptions, + repos_dir: &Path, + bound: std::time::Duration, + ) -> RepoStore { + RepoStore::new( + repos_dir.to_path_buf(), + Some(TigrisClient::for_testing_with_endpoint( + "test-bucket", + mock.endpoint(), + )), + no_reap_pool(opts, 2).await, + bound, + ) + } + + /// The statuses of every logged PUT attempt, which is what the + /// abandoned-writer tests assert their attempt counts on. + fn attempt_statuses(mock: &S3Mock) -> Vec> { + mock.put_attempts().iter().map(|a| a.status).collect() + } + + /// THE HEADLINE ARM. An abandoned writer's PUT that lands after a successor + /// has published must be refused by the store, and the successor's archive + /// must survive it. + /// + /// This is the whole point of the change. Dropping the future of an + /// in-flight PUT does not cancel the request the server is already + /// processing, so the advisory lock cannot fence it: A's release returns, + /// the lock frees, B acquires and publishes, and A's bytes are still on + /// their way to a store that has already moved on. Only the conditional PUT + /// decides that race, and it decides it at COMMIT time, not at arrival. + /// + /// The mock models that by capturing A's PUT when it arrives and evaluating + /// it on replay, against the state as of the replay. That is deliberately + /// not a timing test: a parked handler whose client has gone away is + /// cancelled with the connection, so a test that waited for it to resume on + /// its own would be waiting on nothing. + #[sqlx::test] + async fn an_abandoned_writers_late_put_loses_to_the_successor(pool: PgPool) { + let _sink = log_sink(); + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let bound = std::time::Duration::from_millis(750); + let store = fenced_store_with_bound(&mock, &opts, repos.path(), bound).await; + let owner = "did:key:z6MkFenceLatePut"; + let repo = "fence-late-put-repo"; + let slug = owner_slug_of(owner); + + // Seed the key, so A's acquire observes a generation and carries + // If-Match on it. This is the ordinary case: the repo already exists. + let seed = TempDir::new().unwrap(); + marked_repo(seed.path(), "seed"); + mock_tigris(&mock) + .upload(&slug, repo, seed.path(), UploadPrecondition::Unconditional) + .await + .expect("seeding the archive"); + let seeded = mock.current_etag().expect("the seed minted an ETag"); + + // Writer A takes the lock, writes its tree, and has its publish parked + // past the transfer bound. + let guard_a = store.acquire_write(owner, repo).await.expect("A acquires"); + std::fs::write(guard_a.local_path.join("MARKER"), "writer-a").unwrap(); + mock.park_next_put(); + let started = std::time::Instant::now(); + let outcome_a = guard_a.release(true).await; + assert!( + started.elapsed() >= bound, + "A's release must have run out its transfer bound with the PUT in flight" + ); + // A timeout is UNKNOWABLE rather than failed, so the release reports an + // ordinary success and the lock frees. That is exactly why the fence has + // to live in the store: nothing here knows A's bytes are still coming. + assert!( + matches!(outcome_a, ReleaseOutcome::Released), + "an abandoned publish reports a plain release, got {outcome_a:?}" + ); + + assert!( + mock.captured_put().is_some(), + "A's PUT must have arrived and been captured before the bound elapsed" + ); + + // Writer B acquires the freed lock and publishes for real. + let b_started = std::time::Instant::now(); + let guard_b = store + .acquire_write(owner, repo) + .await + .expect("B must acquire once A's release frees the lock"); + assert!( + b_started.elapsed() < std::time::Duration::from_secs(5), + "B's acquire must be prompt, not blocked behind A's abandoned transfer" + ); + std::fs::write(guard_b.local_path.join("MARKER"), "writer-b").unwrap(); + guard_b + .release(true) + .await + .into_result() + .expect("B's publish is the one that must land"); + let after_b = mock.current_etag().expect("B's publish minted an ETag"); + assert_ne!( + unquote_etag(&after_b), + unquote_etag(&seeded), + "B's publish must have moved the generation on" + ); + + // NOW A's bytes reach the store's commit point. + assert_eq!( + mock.replay_captured(), + 412, + "A's late PUT must be refused: the generation it fenced on is gone" + ); + assert_eq!( + stored_marker(&mock, &slug, repo).await, + "writer-b", + "the successor's archive must survive the abandoned writer's late PUT" + ); + assert_eq!( + mock.current_etag().as_deref().map(unquote_etag), + Some(unquote_etag(&after_b)), + "a refused PUT must not rotate the generation either" + ); + + // Seed, A's parked PUT, B's publish, the deliberate replay. A never + // attempted a second PUT of its own: the timeout arm takes no + // compensating action precisely because the outcome is unknowable. + assert_eq!( + attempt_statuses(&mock), + vec![Some(200), None, Some(200), Some(412)], + "got {:?}", + mock.put_attempts() + ); + + // The header detail comes LAST on purpose. Asserting it up front would + // make a lost fence red here, on a wire-format check, rather than on the + // outcome above, and the outcome is what this test is for. + let captured = mock.captured_put().expect("A's PUT was captured"); + assert_eq!( + captured.if_match.as_deref().map(unquote_etag), + Some(unquote_etag(&seeded)), + "A's in-flight PUT must carry the generation it observed under the lock" + ); + assert_eq!(captured.if_none_match, None); + + mock.open_gate(); + mock.shutdown(); + } + + /// The create-only arm of the same race, and the one whose real-world + /// failure mode is SILENT: an ignored If-None-Match just returns 200, so a + /// publish that should have been fenced lands with no error anywhere. + #[sqlx::test] + async fn an_abandoned_writers_late_create_only_put_loses_to_the_successor(pool: PgPool) { + let _sink = log_sink(); + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let bound = std::time::Duration::from_millis(750); + let store = fenced_store_with_bound(&mock, &opts, repos.path(), bound).await; + let owner = "did:key:z6MkFenceLateCreate"; + let repo = "fence-late-create-repo"; + let slug = owner_slug_of(owner); + + // Nothing stored, so A's acquire records the absent case and is fenced + // create-only. + let guard_a = store.acquire_write(owner, repo).await.expect("A acquires"); + marked_repo(&guard_a.local_path, "writer-a"); + mock.park_next_put(); + let outcome_a = guard_a.release(true).await; + assert!( + matches!(outcome_a, ReleaseOutcome::Released), + "an abandoned publish reports a plain release, got {outcome_a:?}" + ); + + assert!( + mock.captured_put().is_some(), + "A's PUT must have arrived and been captured before the bound elapsed" + ); + + // B wins the empty key. + let guard_b = store + .acquire_write(owner, repo) + .await + .expect("B must acquire once A's release frees the lock"); + std::fs::write(guard_b.local_path.join("MARKER"), "writer-b").unwrap(); + guard_b + .release(true) + .await + .into_result() + .expect("B's create must land"); + + assert_eq!( + mock.replay_captured(), + 412, + "A's late create-only PUT must be refused now that the key exists" + ); + assert_eq!( + stored_marker(&mock, &slug, repo).await, + "writer-b", + "the successor's archive must survive the abandoned create-only PUT" + ); + assert_eq!( + attempt_statuses(&mock), + vec![None, Some(200), Some(412)], + "got {:?}", + mock.put_attempts() + ); + + // Last, for the same reason as the If-Match arm: the outcome is the + // claim, the header is the detail. + let captured = mock.captured_put().expect("A's PUT was captured"); + assert_eq!( + captured.if_none_match.as_deref(), + Some("*"), + "A's in-flight PUT must carry the create-only fence it observed" + ); + assert_eq!(captured.if_match, None); + + mock.open_gate(); + mock.shutdown(); + } + + /// THE CONTROL, and it is what makes the two arms above attributable. + /// + /// Same abandonment, same replay, but no successor publishes in between, so + /// the generation A fenced on is still current when its bytes commit and + /// the PUT must LAND. Without this, a green headline test would prove only + /// that replays are rejected, not that STALENESS is what rejects them. + /// + /// It must therefore stay green when the carried precondition is forced + /// back to `Unconditional`: it asserts an outcome the fence does not + /// change, which is the whole reason it can attribute the others' red. + #[sqlx::test] + async fn an_abandoned_put_still_on_the_current_generation_lands(pool: PgPool) { + let _sink = log_sink(); + let mock = S3Mock::start().await; + let opts = (*pool.connect_options()).clone(); + let repos = TempDir::new().unwrap(); + let bound = std::time::Duration::from_millis(750); + let store = fenced_store_with_bound(&mock, &opts, repos.path(), bound).await; + let owner = "did:key:z6MkFenceControl"; + let repo = "fence-control-repo"; + let slug = owner_slug_of(owner); + + let seed = TempDir::new().unwrap(); + marked_repo(seed.path(), "seed"); + mock_tigris(&mock) + .upload(&slug, repo, seed.path(), UploadPrecondition::Unconditional) + .await + .expect("seeding the archive"); + + let guard_a = store.acquire_write(owner, repo).await.expect("A acquires"); + std::fs::write(guard_a.local_path.join("MARKER"), "writer-a").unwrap(); + mock.park_next_put(); + let outcome_a = guard_a.release(true).await; + assert!( + matches!(outcome_a, ReleaseOutcome::Released), + "an abandoned publish reports a plain release, got {outcome_a:?}" + ); + + assert_eq!( + mock.replay_captured(), + 200, + "with nothing published in between, the abandoned PUT is still current \ + and must be accepted" + ); + assert_eq!( + stored_marker(&mock, &slug, repo).await, + "writer-a", + "the accepted late PUT must be what is stored" + ); + assert_eq!( + attempt_statuses(&mock), + vec![Some(200), None, Some(200)], + "got {:?}", + mock.put_attempts() + ); + + mock.open_gate(); + mock.shutdown(); + } } diff --git a/crates/gitlawb-node/src/git/tigris.rs b/crates/gitlawb-node/src/git/tigris.rs index cf7abfd5..00022e6b 100644 --- a/crates/gitlawb-node/src/git/tigris.rs +++ b/crates/gitlawb-node/src/git/tigris.rs @@ -11,6 +11,34 @@ use anyhow::{Context, Result}; use aws_sdk_s3::Client as S3Client; use tracing::{debug, info}; +/// The precondition an upload is fenced on. +/// +/// Object storage is the only place a fence can hold. Dropping the future of an +/// in-flight PUT does not cancel the request the server is already processing, +/// so no amount of local locking stops an abandoned writer's bytes from landing +/// after a successor has published. A conditional PUT the store itself refuses +/// is what actually stops it. +#[derive(Clone, Debug)] +pub enum UploadPrecondition { + /// Publish only if the stored object is still the generation we observed. + IfMatch(String), + /// Publish only if nothing is stored under the key yet. + IfAbsent, + /// No fence. Last writer wins. + Unconditional, +} + +/// Why an upload failed, split so a caller can tell "someone else already +/// published this key" (expected, and dropping our bytes is the correct +/// outcome) from a real storage failure. +#[derive(Debug, thiserror::Error)] +pub enum UploadError { + #[error("upload precondition lost (HTTP {status})")] + PreconditionLost { status: u16 }, + #[error(transparent)] + Other(#[from] anyhow::Error), +} + /// Wrapper around the S3 client with the configured bucket. #[derive(Clone)] pub struct TigrisClient { @@ -31,21 +59,29 @@ impl TigrisClient { }) } - /// Test-only constructor with an explicit S3 endpoint, region, and static - /// credentials — no env-var reads, so parallel tests cannot race each other's - /// `AWS_*` environment the way the env-based `new` would. Lets a test point - /// the client at a non-routable endpoint to exercise acquire-stall paths. + /// Build a client pointed at an arbitrary endpoint, for tests. + /// + /// The production constructor reads the endpoint and credentials from the + /// environment, which a test cannot steer without mutating process-global + /// state. This takes both explicitly so a test can aim the client at a + /// closed port and get a prompt transport error out of `exists()`. + /// + /// `RetryConfig::disabled()` is load-bearing, not tidiness: the SDK's default + /// policy retries a connection refusal with backoff, which turns each failing + /// call into seconds of waiting. #[cfg(test)] - pub(crate) async fn for_testing_with_endpoint(bucket: &str, endpoint_url: &str) -> Self { - let creds = aws_sdk_s3::config::Credentials::new("test", "test", None, None, "test"); - let config = aws_config::defaults(aws_config::BehaviorVersion::latest()) - .endpoint_url(endpoint_url) - .region(aws_config::Region::new("auto")) - .credentials_provider(creds) - .load() - .await; + pub fn for_testing_with_endpoint(bucket: &str, endpoint: &str) -> Self { + use aws_sdk_s3::config::{retry::RetryConfig, Credentials, Region}; + + let config = aws_sdk_s3::config::Config::builder() + .endpoint_url(endpoint) + .credentials_provider(Credentials::new("test", "test", None, None, "test")) + .region(Region::new("auto")) + .retry_config(RetryConfig::disabled()) + .behavior_version_latest() + .build(); Self { - s3: S3Client::new(&config), + s3: S3Client::from_conf(config), bucket: bucket.to_string(), } } @@ -77,8 +113,47 @@ impl TigrisClient { } } - /// Upload a local bare repo directory to Tigris as a tar.zst archive. - pub async fn upload(&self, owner_slug: &str, repo_name: &str, local_path: &Path) -> Result<()> { + /// Read the ETag of a repo archive, or `None` when nothing is stored under + /// the key. The ETag identifies the generation a later conditional upload + /// can fence itself on. + /// + /// Separate from `exists` rather than folded into it: `exists` has callers + /// that only want the boolean, and widening its return type would churn + /// every one of them for no benefit. + pub async fn head_etag(&self, owner_slug: &str, repo_name: &str) -> Result> { + let key = Self::repo_key(owner_slug, repo_name); + match self + .s3 + .head_object() + .bucket(&self.bucket) + .key(&key) + .send() + .await + { + Ok(out) => Ok(Some( + out.e_tag() + .context(format!("tigris HEAD {key}: hit carried no ETag"))? + .to_string(), + )), + Err(e) => { + if e.as_service_error().is_some_and(|e| e.is_not_found()) { + Ok(None) + } else { + Err(anyhow::anyhow!("tigris HEAD {key}: {e}")) + } + } + } + } + + /// Upload a local bare repo directory to Tigris as a tar.zst archive, + /// fenced by `precondition`. + pub async fn upload( + &self, + owner_slug: &str, + repo_name: &str, + local_path: &Path, + precondition: UploadPrecondition, + ) -> std::result::Result<(), UploadError> { let key = Self::repo_key(owner_slug, repo_name); debug!(key = %key, path = %local_path.display(), "uploading repo to tigris"); @@ -93,15 +168,57 @@ impl TigrisClient { let body = aws_sdk_s3::primitives::ByteStream::from(archive_bytes); - self.s3 + let mut req = self + .s3 .put_object() .bucket(&self.bucket) .key(&key) .body(body) - .content_type("application/zstd") - .send() - .await - .context(format!("tigris PUT {key}"))?; + .content_type("application/zstd"); + match &precondition { + UploadPrecondition::IfMatch(etag) => req = req.if_match(etag), + UploadPrecondition::IfAbsent => req = req.if_none_match("*"), + UploadPrecondition::Unconditional => {} + } + + if let Err(e) = req.send().await { + // `PutObjectError` models no PreconditionFailed variant (its arms are + // EncryptionTypeMismatch, InvalidRequest, InvalidWriteOffset, + // TooManyParts, Unhandled), so a refused precondition arrives as + // `Unhandled` and matching the enum would classify it as a generic + // failure. The raw HTTP status off the response is the only place the + // answer actually lives. + // + // Read it via `raw_response()`, not a `ServiceError`-only match: the + // SDK exposes the raw response for BOTH `ServiceError` and + // `ResponseError`, and a refused conditional PUT whose error body the + // SDK cannot parse (malformed XML, premature close) surfaces as + // `ResponseError`. Matching only `ServiceError` would classify that + // unparsable 409/412 as a generic failure, and `RepoWriteGuard::release` + // would log-and-succeed instead of taking the supersede retry, + // acknowledging a write that was definitively not published. + let status = e.raw_response().map(|raw| raw.status().as_u16()); + // 412 is always a lost precondition. 409 is one only when we asked + // for create-only, which is how S3-compatible stores report "the key + // already exists". Everything else, 404 included, is a real failure: + // archive keys are never deleted (`delete` has no callers), so a 404 + // here means something permanent like a missing bucket or a + // misrouted endpoint, and reporting that as a lost precondition + // would tell a caller to expect a successor that does not exist. + let lost = match status { + Some(412) => true, + Some(409) => matches!(precondition, UploadPrecondition::IfAbsent), + _ => false, + }; + if lost { + return Err(UploadError::PreconditionLost { + status: status.expect("a lost precondition came from a status"), + }); + } + return Err(UploadError::Other( + anyhow::Error::new(e).context(format!("tigris PUT {key}")), + )); + } info!(key = %key, "uploaded repo to tigris"); Ok(()) @@ -114,8 +231,28 @@ impl TigrisClient { repo_name: &str, local_path: &Path, ) -> Result<()> { + self.download_to(owner_slug, repo_name, local_path, true) + .await + .map(|_| ()) + } + + /// Download a repo archive from Tigris and extract it, returning the + /// directory that was populated. + /// + /// `publish` controls whether the extract is swapped into `target` in place + /// (the live-path mutation used by writes; returns `target`) or unpacked + /// into a fresh temp directory under `target`'s parent (a non-mutating + /// snapshot read; returns the temp dir, which the caller owns and cleans + /// up). The snapshot form never touches the live repo path. + pub async fn download_to( + &self, + owner_slug: &str, + repo_name: &str, + target: &Path, + publish: bool, + ) -> Result { let key = Self::repo_key(owner_slug, repo_name); - debug!(key = %key, path = %local_path.display(), "downloading repo from tigris"); + debug!(key = %key, path = %target.display(), "downloading repo from tigris"); let resp = self .s3 @@ -133,17 +270,69 @@ impl TigrisClient { .context("reading tigris response body")? .into_bytes(); - // Extract tar.zst to local path - tokio::task::spawn_blocking({ - let local_path = local_path.to_path_buf(); - move || decompress_repo(&data, &local_path) + // The snapshot temp dir is decided HERE, in the async layer, before the + // extraction runs. The extraction itself is uncancellable spawn_blocking, + // so the dir is created no matter what happens to this future; an + // async-layer guard that owns the path removes it when this future is + // dropped mid-await (client disconnect, a bounded-transfer timeout). + let snapshot_tmp = if publish { + None + } else { + let parent = target.parent().context("snapshot path has no parent")?; + std::fs::create_dir_all(parent).context("creating parent dir")?; + let file_name = target + .file_name() + .context("snapshot path has no file name")? + .to_string_lossy(); + Some(parent.join(format!( + ".{file_name}.tmp-snapshot.{}", + uuid::Uuid::new_v4() + ))) + }; + // Armed before the extraction await; disarmed on the success return via + // `mem::forget`, leaving the dir to the caller (RepoSnapshot::drop). On + // any other exit, including a cancelled future, the guard removes the + // dir. This is what closes the leak where a dropped read_snapshot future + // abandons a completed extraction. + let _cleanup = snapshot_tmp.as_ref().map(|p| SnapshotCleanup(p.clone())); + + // Extract tar.zst to a directory. + let extracted = tokio::task::spawn_blocking({ + let target = target.to_path_buf(); + let snapshot_tmp = snapshot_tmp.clone(); + move || -> Result { + if publish { + decompress_repo(&data, &target)?; + return Ok(target); + } + // Non-mutating snapshot: unpack into the temp dir decided above. + // The live repo path is never touched. + let tmp_dir = snapshot_tmp.expect("snapshot path was decided above"); + std::fs::create_dir_all(&tmp_dir).context("creating temp extract dir")?; + let unpack = (|| -> Result<()> { + let decoder = zstd::stream::Decoder::new(&data[..])?; + let mut archive = tar::Archive::new(decoder); + archive.unpack(&tmp_dir).context("unpacking tar.zst")?; + Ok(()) + })(); + if let Err(e) = unpack { + let _ = std::fs::remove_dir_all(&tmp_dir); + return Err(e); + } + Ok(tmp_dir) + } }) .await .context("extract task panicked")? .context("extracting repo")?; - info!(key = %key, path = %local_path.display(), "downloaded repo from tigris"); - Ok(()) + // The dir is now the caller's to own: drop the cleanup guard without + // removing anything. On a future drop before this point, `_cleanup` runs + // and removes the dir even though the extraction completed. + std::mem::forget(_cleanup); + + info!(key = %key, path = %target.display(), "downloaded repo from tigris"); + Ok(extracted) } /// Delete a repo archive from Tigris. @@ -193,6 +382,20 @@ fn publish_lock(local_path: &Path) -> Arc> { .clone() } +/// Async-layer cleanup for a snapshot temp dir that the extraction's +/// `spawn_blocking` created and that would otherwise outlive a cancelled +/// `download_to` future. Armed before the extraction await, disarmed (via +/// `mem::forget`) on success so `RepoSnapshot::drop` stays the single owner; on +/// any other exit the dir is removed even though the extraction ran to +/// completion. +struct SnapshotCleanup(PathBuf); + +impl Drop for SnapshotCleanup { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + /// Decompress a tar.zst byte vector into a local directory. /// /// Extraction is atomic with respect to `local_path`: the archive is unpacked @@ -243,3 +446,212 @@ fn decompress_repo(data: &[u8], local_path: &Path) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use aws_sdk_s3::primitives::ByteStream; + use futures::FutureExt; + + /// The envs the probe needs, all of them, or it does not run. + /// + /// `AWS_ENDPOINT_URL_S3` is included on purpose: without it the SDK resolves + /// to real AWS S3, and a probe that passed there would say nothing about + /// Tigris. + fn probe_env() -> Option { + if std::env::var("GITLAWB_TIGRIS_PROBE").ok().as_deref() != Some("1") { + return None; + } + for name in [ + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_ENDPOINT_URL_S3", + ] { + if std::env::var(name).is_err() { + eprintln!("tigris conditional-write probe: {name} is unset, skipping"); + return None; + } + } + match std::env::var("GITLAWB_TIGRIS_BUCKET") { + Ok(b) if !b.is_empty() => Some(b), + _ => { + eprintln!( + "tigris conditional-write probe: GITLAWB_TIGRIS_BUCKET is unset, skipping" + ); + None + } + } + } + + /// One conditional PUT, reported as the status that REFUSED it, or `None` + /// when the store accepted the write. + /// + /// Accepted is the interesting answer here, not an error: it means the + /// endpoint ignored the header we fenced on. + async fn conditional_put( + s3: &S3Client, + bucket: &str, + key: &str, + body: &'static [u8], + if_match: Option<&str>, + if_none_match: Option<&str>, + ) -> Result, String> { + let mut req = s3 + .put_object() + .bucket(bucket) + .key(key) + .body(ByteStream::from_static(body)); + if let Some(v) = if_match { + req = req.if_match(v); + } + if let Some(v) = if_none_match { + req = req.if_none_match(v); + } + match req.send().await { + Ok(_) => Ok(None), + Err(e) => match e.raw_response() { + // Same raw-response rule as `upload`: a refused conditional PUT + // whose body the SDK cannot parse surfaces as `ResponseError`, and + // the status has to come off the raw response for both variants. + // Without it, an unparsable 409/412 would report "no HTTP + // response" here and skip the supersede retry. + Some(raw) => Ok(Some(raw.status().as_u16())), + None => Err(format!("conditional PUT {key}: no HTTP response: {e}")), + }, + } + } + + /// The probe body, written to RETURN its failures rather than panic on + /// them, so the caller's cleanup is reached on every arm. + async fn conditional_write_probe(s3: &S3Client, bucket: &str, key: &str) -> Result<(), String> { + // 1. A plain PUT under a throwaway key. + let seeded = s3 + .put_object() + .bucket(bucket) + .key(key) + .body(ByteStream::from_static(b"probe-one")) + .send() + .await + .map_err(|e| format!("seeding PUT {key}: {e}"))?; + + // 2. Its ETag, which is the generation the next arm fences against. + let etag = seeded + .e_tag() + .ok_or_else(|| format!("seeding PUT {key} returned no ETag"))? + .to_string(); + + // 3. A deliberately wrong If-Match. A store honoring it answers 412. + let wrong = format!("\"{}\"", "0".repeat(32)); + if etag.trim_matches('"') == wrong.trim_matches('"') { + return Err(format!( + "the seeded ETag {etag} collides with the deliberately wrong one, \ + so this arm would prove nothing" + )); + } + match conditional_put(s3, bucket, key, b"probe-two", Some(&wrong), None).await? { + Some(412) => {} + Some(status) => { + return Err(format!( + "a stale If-Match must be refused with 412, the endpoint answered {status}" + )) + } + None => { + return Err( + "a stale If-Match was ACCEPTED: this endpoint does not honor If-Match, so \ + the release fence cannot hold here" + .to_string(), + ) + } + } + + // 4. If-None-Match `*` over the object that now exists. This arm matters + // MORE than the one above. An ignored If-Match eventually surfaces as + // odd behavior, because a stale writer overwrites and someone notices + // the lost tree. An ignored If-None-Match just returns 200, so a publish + // that should have been fenced lands with no error anywhere: the silent + // no-op the bucket-type caveat on this test describes. + match conditional_put(s3, bucket, key, b"probe-three", None, Some("*")).await? { + // Either status is a pass, and the asymmetry with the If-Match arm + // above mirrors `upload`'s classifier exactly: 412 is always a lost + // precondition, and 409 is one too when we asked for create-only. + // AWS documents 409 for a create-only conflict racing a delete, so a + // store answering it is enforcing the precondition and we already + // handle it. Pinning 412 alone here would fail the probe against a + // backend that is behaving correctly, which sends whoever runs it + // chasing a fault that is not there. + Some(412) | Some(409) => {} + Some(status) => { + return Err(format!( + "create-only over an existing object must be refused with 412 or 409, \ + the endpoint answered {status}" + )) + } + None => { + return Err( + "If-None-Match * was ACCEPTED over an existing object: this endpoint does \ + not honor create-only, so a fenced publish lands silently" + .to_string(), + ) + } + } + + Ok(()) + } + + /// Probe the REAL Tigris endpoint for the conditional-write semantics the + /// release fence depends on. + /// + /// UNTIL THIS IS RUN AGAINST REAL CREDENTIALS, the fence is verified against + /// vendor documentation and an in-process mock, not against the backend it + /// runs on. The mock implements the semantics we believe Tigris has; it + /// cannot tell us whether Tigris actually has them. + /// + /// The bucket matters, not just the endpoint. Tigris documents conditional + /// operations as supported on Single-region and Multi-region buckets only. + /// Global and Dual-region buckets are eventually consistent, and a + /// conditional PUT evaluated against a stale replica would make the fence a + /// silent no-op rather than an error. So point `GITLAWB_TIGRIS_BUCKET` at a + /// throwaway bucket of the SAME type production uses. + /// + /// Ignored by default and additionally gated on `GITLAWB_TIGRIS_PROBE=1`, + /// because it writes to a real bucket and costs real requests. Run with: + /// `GITLAWB_TIGRIS_PROBE=1 cargo test -p gitlawb-node --bin gitlawb-node + /// tigris_honors_conditional_writes -- --ignored --nocapture` + #[tokio::test] + #[ignore = "writes to a real Tigris bucket; needs GITLAWB_TIGRIS_PROBE=1 plus credentials"] + async fn tigris_honors_conditional_writes() { + let Some(bucket) = probe_env() else { + eprintln!( + "tigris conditional-write probe: skipped. Set GITLAWB_TIGRIS_PROBE=1, \ + GITLAWB_TIGRIS_BUCKET, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and \ + AWS_ENDPOINT_URL_S3 to run it." + ); + return; + }; + + let config = aws_config::load_defaults(aws_config::BehaviorVersion::latest()).await; + let s3 = S3Client::new(&config); + // A fresh key per run, so a probe that somehow orphaned an object on an + // earlier run cannot change what this one observes. + let key = format!("probe/conditional-write-{}.bin", uuid::Uuid::new_v4()); + + // CLEANUP MUST RUN ON EVERY ARM, and a failing assertion is precisely + // the case this probe exists to catch, so the delete cannot sit after + // the checks. The body returns its failures rather than panicking, and + // `catch_unwind` covers the panic an SDK call could still raise; either + // way the delete below is reached before the verdict is re-raised. + let outcome = std::panic::AssertUnwindSafe(conditional_write_probe(&s3, &bucket, &key)) + .catch_unwind() + .await; + + if let Err(e) = s3.delete_object().bucket(&bucket).key(&key).send().await { + eprintln!("tigris conditional-write probe: cleanup of {key} failed: {e}"); + } + + match outcome { + Ok(Ok(())) => {} + Ok(Err(msg)) => panic!("tigris conditional-write probe: {msg}"), + Err(payload) => std::panic::resume_unwind(payload), + } + } +} diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 10aaca5b..74ccfdc4 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -287,8 +287,20 @@ async fn main() -> Result<()> { None }; - let repo_store = - git::repo_store::RepoStore::new(config.repos_dir.clone(), tigris, db.pool().clone()); + // Advisory-lock connections come from their own pool: a write guard pins one + // for its whole lifetime, and sharing the application pool would let a push + // burst starve ordinary request handlers. + let lock_pool = db::Db::lock_pool( + &config.database_url, + config.db_lock_pool_max_connections, + std::time::Duration::from_secs(config.db_acquire_timeout_secs), + )?; + let repo_store = git::repo_store::RepoStore::new( + config.repos_dir.clone(), + tigris, + lock_pool, + std::time::Duration::from_secs(config.lock_held_transfer_timeout_secs), + ); // Per-DID limiter for the creation endpoints. Keyed on the authenticated // DID (attacker-varied), so bound its key set to cap memory.