From f6a5b2f7d8fcdefa6077ece24c8eccd1740a0eeb Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 13:45:11 -0500 Subject: [PATCH 01/25] feat(node): add per-IP write-surface rate brake and apply to write_routes Introduces a dedicated write_rate_limiter bucket (GITLAWB_WRITE_RATE_LIMIT, default 600/hr, 0 disables), separate from the creation brake so a write flood cannot drain the creation budget (KTD-1). Attaches rate_limit_by_ip to write_routes, mirroring creation_routes. Per-DID deliberately not paired (a DID farm never trips it). Tests (route-level, execution-verified against Postgres): a write_routes sink (PUT /star) is 429'd before auth; an exhausted write bucket does not throttle repo creation (separate-bucket property). --- crates/gitlawb-node/src/api/repos.rs | 76 +++++++++++++++++++++++++ crates/gitlawb-node/src/auth/mod.rs | 1 + crates/gitlawb-node/src/main.rs | 22 +++++++ crates/gitlawb-node/src/server.rs | 16 +++++- crates/gitlawb-node/src/state.rs | 10 ++++ crates/gitlawb-node/src/test_support.rs | 1 + 6 files changed, 124 insertions(+), 2 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index b9cbc352..7bb3c000 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -2625,4 +2625,80 @@ mod tests { "repo creation must be IP-throttled before signature verification" ); } + + #[sqlx::test] + async fn write_route_is_rate_limited_by_ip(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + // Tiny write bucket, keyed on the socket peer (no trusted proxy). + state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + + let peer: SocketAddr = "203.0.113.88:7000".parse().unwrap(); + // Exhaust this peer's single-request write budget up front. + assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); + + let router = crate::server::build_router(state); + // A write_routes sink (star). The IP brake is outermost, so the 429 + // fires before auth/handler — the path only needs to match. + let mut req = Request::builder() + .method(Method::PUT) + .uri("/api/v1/repos/someowner/somerepo/star") + .body(Body::empty()) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + + let status = router.oneshot(req).await.unwrap().status(); + assert_eq!( + status, + StatusCode::TOO_MANY_REQUESTS, + "a write_routes sink must be IP-throttled before signature verification" + ); + } + + // KTD-1: the write bucket is separate from the creation bucket, so a write + // flood must not consume the creation budget (and vice versa). + #[sqlx::test] + async fn write_flood_does_not_drain_creation_budget(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + // Exhaust the write bucket for this peer; leave the creation bucket ample. + state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); + state.create_ip_rate_limiter = + crate::rate_limit::RateLimiter::new(1000, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + + let peer: SocketAddr = "203.0.113.99:7000".parse().unwrap(); + assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); + + let router = crate::server::build_router(state); + // Creation from the same peer must NOT be 429 — its bucket is untouched. + // (It fails later on missing signature; the point is it is not throttled.) + let mut req = Request::builder() + .method(Method::POST) + .uri("/api/v1/repos") + .header("content-type", "application/json") + .body(Body::from(r#"{"name":"legit","is_public":true}"#)) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + + let status = router.oneshot(req).await.unwrap().status(); + assert_ne!( + status, + StatusCode::TOO_MANY_REQUESTS, + "an exhausted write bucket must not throttle repo creation (separate buckets)" + ); + } } diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 720fb3ae..a9baa314 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -515,6 +515,7 @@ mod tests { repo_store: crate::git::repo_store::RepoStore::for_testing(PathBuf::from("/tmp"), pool), rate_limiter: RateLimiter::new(100, Duration::from_secs(60)), create_ip_rate_limiter: RateLimiter::new(1000, Duration::from_secs(3600)), + write_rate_limiter: RateLimiter::new(1000, Duration::from_secs(3600)), push_rate_limiter: RateLimiter::new(600, Duration::from_secs(3600)), push_limiter_trust: crate::rate_limit::TrustedProxy::None, sync_trigger_rate_limiter: RateLimiter::new(60, Duration::from_secs(3600)), diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index aa0483db..4fd6e922 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -307,6 +307,27 @@ async fn main() -> Result<()> { tracing::warn!("GITLAWB_CREATE_RATE_LIMIT=0 — per-IP creation rate limiting disabled"); } + // Per-IP brake for the authenticated non-creation write surface (issue/PR + // comments, labels, stars, merges, protect, replicas, visibility, tasks, + // bounties, profile, GraphQL mutations). Its own bucket, separate from the + // creation brake, so a write flood cannot drain the creation budget and vice + // versa (same rationale as the sync_trigger / peer_write split). Sized above + // any legitimate per-IP write rate — real agent automation makes many small + // writes per hour. GITLAWB_WRITE_RATE_LIMIT overrides; 0 disables. Bounded + // key set — the key is a client-influenced IP. + let write_limit = std::env::var("GITLAWB_WRITE_RATE_LIMIT") + .ok() + .and_then(|v| v.trim().parse::().ok()) + .unwrap_or(600); + let write_rate_limiter = rate_limit::RateLimiter::new_bounded( + write_limit, + std::time::Duration::from_secs(3600), + 200_000, + ); + if write_limit == 0 { + tracing::warn!("GITLAWB_WRITE_RATE_LIMIT=0 — per-IP write rate limiting disabled"); + } + // Push-path flood brake: max git-receive-pack requests per client IP per // hour (counts both the info/refs advertisement and the push POST). Sized // for heavy agent automation while still stopping flood traffic (the June @@ -373,6 +394,7 @@ async fn main() -> Result<()> { repo_store, rate_limiter, create_ip_rate_limiter, + write_rate_limiter, push_rate_limiter, push_limiter_trust, sync_trigger_rate_limiter, diff --git a/crates/gitlawb-node/src/server.rs b/crates/gitlawb-node/src/server.rs index f4c0d3e3..5df9ca0d 100644 --- a/crates/gitlawb-node/src/server.rs +++ b/crates/gitlawb-node/src/server.rs @@ -111,7 +111,17 @@ pub fn build_router(state: AppState) -> Router { .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) .layer(axum::Extension(create_ip_limiter)); - // ── Write routes — require HTTP Signature (no rate limit) ───────────── + // ── Write routes — require HTTP Signature + per-IP write brake ──────── + // Same per-IP flood defense as the creation routes, on its own `write` + // bucket (see `AppState::write_rate_limiter`). Per-DID is deliberately not + // paired (a DID farm never trips it; busy legitimate agents would false- + // positive). The bundled visibility GET is a read that also draws from the + // write bucket — harmless, since the bucket is sized far above any legit + // per-IP read rate. + let write_ip_limiter = rate_limit::IpRateLimiter { + limiter: state.write_rate_limiter.clone(), + trust: state.push_limiter_trust, + }; let write_routes = add_auth_layers( Router::new() .route( @@ -181,7 +191,9 @@ pub fn build_router(state: AppState) -> Router { axum::routing::delete(agents::deregister_agent), ), state.clone(), - ); + ) + .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(write_ip_limiter.clone())); // Body limit is raised to GITLAWB_MAX_PACK_BYTES (default 2 GB) for git // routes only — all other API routes keep axum's default 2 MB cap. diff --git a/crates/gitlawb-node/src/state.rs b/crates/gitlawb-node/src/state.rs index 6a84b3c2..d38541f9 100644 --- a/crates/gitlawb-node/src/state.rs +++ b/crates/gitlawb-node/src/state.rs @@ -61,6 +61,16 @@ pub struct AppState { /// resolved client IP is what actually stops a single-source flood (same /// rationale as `push_rate_limiter`). Keyed by `push_limiter_trust`. pub create_ip_rate_limiter: RateLimiter, + /// Per-client-IP rate limiter for the authenticated write surface that is + /// not repo/agent creation: issue/PR comments, labels, stars, merges, + /// protect/unprotect, replicas, visibility, tasks, bounties, profile, and + /// the GraphQL `MutationRoot`. Separate bucket from `create_ip_rate_limiter` + /// (its own budget, per the `sync_trigger`/`peer_write` precedent) so a + /// write flood cannot drain the creation budget and vice versa. Per-DID is + /// deliberately NOT paired here: a DID farm never trips it, and busy + /// legitimate agents would false-positive. `GITLAWB_WRITE_RATE_LIMIT` + /// overrides the default, 0 disables. Keyed by `push_limiter_trust`. + pub write_rate_limiter: RateLimiter, /// Per-client-IP rate limiter for git-receive-pack. Per-DID limits cannot /// brake a push flood from a DID farm (one throwaway DID per repo), so the /// push path throttles on the resolved client IP instead. diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 4fe52f64..687b5f90 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -77,6 +77,7 @@ fn build_state(db: Arc, pool: PgPool) -> AppState { repo_store: crate::git::repo_store::RepoStore::for_testing(PathBuf::from("/tmp"), pool), rate_limiter: RateLimiter::new(100, Duration::from_secs(60)), create_ip_rate_limiter: RateLimiter::new(1000, Duration::from_secs(3600)), + write_rate_limiter: RateLimiter::new(1000, Duration::from_secs(3600)), push_rate_limiter: RateLimiter::new(600, Duration::from_secs(3600)), push_limiter_trust: crate::rate_limit::TrustedProxy::None, sync_trigger_rate_limiter: RateLimiter::new(60, Duration::from_secs(3600)), From 7d680dc9c27fc10223da006e71a00fec12ecfcd1 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 14:11:38 -0500 Subject: [PATCH 02/25] feat(node): extend the per-IP write brake to the remaining write sinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attaches rate_limit_by_ip (write bucket) to task_write_routes, issue_write_routes, bounty_write_routes, profile_write_routes, and the /graphql POST/GET surface (KTD-5 — an HTTP-layer brake counts every /graphql request; /graphql/ws subscriptions stay unbraked). write_ip_limiter is now built once and shared. Every non-git, non-creation authenticated write group is braked. Tests (execution-verified against Postgres): /graphql POST is 429'd, and a representative REST write sink (issue comment) is 429'd, by the write brake. --- crates/gitlawb-node/src/api/repos.rs | 69 ++++++++++++++++++++++++++++ crates/gitlawb-node/src/server.rs | 41 ++++++++++++----- 2 files changed, 99 insertions(+), 11 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 7bb3c000..a9cfcc7e 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -2701,4 +2701,73 @@ mod tests { "an exhausted write bucket must not throttle repo creation (separate buckets)" ); } + + // KTD-5: /graphql POST (the MutationRoot surface) draws from the write bucket. + #[sqlx::test] + async fn graphql_post_is_rate_limited_by_ip(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + + let peer: SocketAddr = "203.0.113.111:7000".parse().unwrap(); + assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); + + let router = crate::server::build_router(state); + let mut req = Request::builder() + .method(Method::POST) + .uri("/graphql") + .header("content-type", "application/json") + .body(Body::from(r#"{"query":"{ __typename }"}"#)) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + + let status = router.oneshot(req).await.unwrap().status(); + assert_eq!( + status, + StatusCode::TOO_MANY_REQUESTS, + "/graphql must be IP-throttled by the write brake" + ); + } + + // Representative REST write group (issue comment) — same attachment as the + // task/bounty/profile groups. + #[sqlx::test] + async fn issue_comment_is_rate_limited_by_ip(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + + let peer: SocketAddr = "203.0.113.122:7000".parse().unwrap(); + assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); + + let router = crate::server::build_router(state); + let mut req = Request::builder() + .method(Method::POST) + .uri("/api/v1/repos/someowner/somerepo/issues/1/comments") + .header("content-type", "application/json") + .body(Body::from(r#"{"body":"flood"}"#)) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + + let status = router.oneshot(req).await.unwrap().status(); + assert_eq!( + status, + StatusCode::TOO_MANY_REQUESTS, + "issue-write routes must be IP-throttled by the write brake" + ); + } } diff --git a/crates/gitlawb-node/src/server.rs b/crates/gitlawb-node/src/server.rs index 5df9ca0d..0c24d953 100644 --- a/crates/gitlawb-node/src/server.rs +++ b/crates/gitlawb-node/src/server.rs @@ -58,15 +58,30 @@ fn add_auth_layers(router: Router, state: AppState) -> Router Router { // ── GraphQL routes ───────────────────────────────────────────────────── let schema = state.graphql_schema.as_ref().clone(); + + // Per-IP write brake, shared across every authenticated non-creation write + // group (see `AppState::write_rate_limiter`). Built once here so each group + // clones the same bucket; attached outermost on each group so a flood is + // rejected before auth runs. Separate bucket from the creation brake. + let write_ip_limiter = rate_limit::IpRateLimiter { + limiter: state.write_rate_limiter.clone(), + trust: state.push_limiter_trust, + }; + let graphql_routes = Router::new() .route("/graphql", get(graphql_playground).post(graphql_handler)) // Attach the verified DID to /graphql when a signature is present. The // layer covers only routes added before it, so /graphql/ws (added after, - // read-only subscriptions) stays open. + // read-only subscriptions) stays open. The per-IP write brake covers the + // same /graphql (KTD-5: an HTTP-layer brake cannot tell a query POST from + // a mutation POST, so it counts every /graphql request — acceptable, both + // cost work); /graphql/ws subscriptions stay unbraked. .layer(middleware::from_fn(auth::optional_signature)) + .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(write_ip_limiter.clone())) .route_service("/graphql/ws", GraphQLSubscription::new(schema)); - // ── Task routes (write — require HTTP Signature) ─────────────────────── + // ── Task routes (write — require HTTP Signature + per-IP write brake) ── let task_write_routes = add_auth_layers( Router::new() .route("/api/v1/tasks", post(tasks::create_task)) @@ -74,7 +89,9 @@ pub fn build_router(state: AppState) -> Router { .route("/api/v1/tasks/{id}/complete", post(tasks::complete_task)) .route("/api/v1/tasks/{id}/fail", post(tasks::fail_task)), state.clone(), - ); + ) + .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(write_ip_limiter.clone())); // ── Task routes (read — open) ────────────────────────────────────────── let task_read_routes = Router::new() @@ -118,10 +135,6 @@ pub fn build_router(state: AppState) -> Router { // positive). The bundled visibility GET is a read that also draws from the // write bucket — harmless, since the bucket is sized far above any legit // per-IP read rate. - let write_ip_limiter = rate_limit::IpRateLimiter { - limiter: state.write_rate_limiter.clone(), - trust: state.push_limiter_trust, - }; let write_routes = add_auth_layers( Router::new() .route( @@ -259,7 +272,9 @@ pub fn build_router(state: AppState) -> Router { post(bounties::dispute_bounty), ), state.clone(), - ); + ) + .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(write_ip_limiter.clone())); // ── Bounty routes (read — open) ────────────────────────────────────── let bounty_read_routes = Router::new() @@ -280,9 +295,11 @@ pub fn build_router(state: AppState) -> Router { let profile_write_routes = add_auth_layers( Router::new().route("/api/v1/profile", axum::routing::put(profiles::set_profile)), state.clone(), - ); + ) + .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(write_ip_limiter.clone())); - // ── Issue routes (write — require HTTP Signature, no rate limit) ───── + // ── Issue routes (write — require HTTP Signature + per-IP write brake) ─ let issue_write_routes = add_auth_layers( Router::new() .route( @@ -294,7 +311,9 @@ pub fn build_router(state: AppState) -> Router { post(issues::create_issue_comment), ), state.clone(), - ); + ) + .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(write_ip_limiter.clone())); // ── Peer discovery routes ───────────────────────────────────────────── // Peer writes accept signatures when present and can require them after a From 665b4ffcb58fe73a4e70ec02b59c77b0b2d9c3fd Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 14:14:15 -0500 Subject: [PATCH 03/25] test(node): adversarial TrustedProxy verification through the write brake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Middleware-level tests complementing the client_key unit coverage: a spoofer varying the client-controlled leftmost X-Forwarded-For hop cannot rotate its bucket key (keyed on the trusted rightmost hop); distinct trusted client hops get distinct buckets (no cross-starvation); and a request with no trusted header in a proxy mode falls back to the socket-peer key — the documented collapse, asserted by name so any change is deliberate. Operator-warning on missing-header deferred deliberately: a per-request warning is a log-noise risk and the security-load-bearing fallback (peer key, never skip) is what these tests lock down. --- crates/gitlawb-node/src/rate_limit.rs | 100 ++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/crates/gitlawb-node/src/rate_limit.rs b/crates/gitlawb-node/src/rate_limit.rs index d40e2691..4c41575c 100644 --- a/crates/gitlawb-node/src/rate_limit.rs +++ b/crates/gitlawb-node/src/rate_limit.rs @@ -605,4 +605,104 @@ mod tests { "an exhausted IP must be braked with 429 before auth runs, not leak to 401" ); } + + // ── Adversarial TrustedProxy verification through the middleware (U5) ── + // These complement the client_key unit tests above by driving hostile + // headers through rate_limit_by_ip on a real router: the property under test + // is that the *bucket key* cannot be rotated by a spoofer. + + async fn post_with( + router: &axum::Router, + peer: SocketAddr, + hdrs: &[(&str, &str)], + ) -> StatusCode { + use tower::ServiceExt; + let mut b = axum::http::Request::builder() + .method(axum::http::Method::POST) + .uri("/api/v1/repos"); + for (k, v) in hdrs { + b = b.header(*k, *v); + } + let mut req = b.body(axum::body::Body::empty()).unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + router.clone().oneshot(req).await.unwrap().status() + } + + #[tokio::test] + async fn xff_spoofed_leftmost_hop_cannot_rotate_bucket() { + // XForwardedFor mode, budget 1. A spoofer varies the client-controlled + // leftmost hop every request but the trusted proxy always appends the + // same rightmost hop. All requests must key to the rightmost hop, so the + // second is braked despite a fresh leftmost value. + let router = ip_limited_over_auth_router(IpRateLimiter { + limiter: RateLimiter::new(1, Duration::from_secs(60)), + trust: TrustedProxy::XForwardedFor, + }); + let p1: SocketAddr = "10.0.0.1:1".parse().unwrap(); + let p2: SocketAddr = "10.0.0.2:1".parse().unwrap(); + // First request passes the brake, reaches (failing) auth. + assert_eq!( + post_with(&router, p1, &[("x-forwarded-for", "9.9.9.1, 203.0.113.5")]).await, + StatusCode::UNAUTHORIZED + ); + // Different leftmost + different socket peer, SAME rightmost trusted hop: + // braked, because the key is the rightmost hop, not the spoofed value. + assert_eq!( + post_with(&router, p2, &[("x-forwarded-for", "9.9.9.2, 203.0.113.5")]).await, + StatusCode::TOO_MANY_REQUESTS, + "a spoofer varying the leftmost XFF hop must not rotate its bucket key" + ); + } + + #[tokio::test] + async fn xff_distinct_real_clients_get_distinct_buckets() { + // Distinct trusted (rightmost) hops are distinct clients: neither throttles + // the other, so a busy legitimate client cannot starve a different one. + let router = ip_limited_over_auth_router(IpRateLimiter { + limiter: RateLimiter::new(1, Duration::from_secs(60)), + trust: TrustedProxy::XForwardedFor, + }); + let peer: SocketAddr = "10.0.0.9:1".parse().unwrap(); + assert_eq!( + post_with( + &router, + peer, + &[("x-forwarded-for", "1.1.1.1, 203.0.113.5")] + ) + .await, + StatusCode::UNAUTHORIZED + ); + assert_eq!( + post_with( + &router, + peer, + &[("x-forwarded-for", "1.1.1.1, 203.0.113.6")] + ) + .await, + StatusCode::UNAUTHORIZED, + "a different trusted client hop must get its own bucket" + ); + } + + #[tokio::test] + async fn absent_trusted_header_collapses_to_socket_peer() { + // The documented fallback: in a trusted-proxy mode, a request with no + // forwarded header keys on the socket peer. Behind a real edge every + // request shares the proxy's socket, so they collapse onto one bucket — + // asserted here by name so any change to this behavior is deliberate. + let router = ip_limited_over_auth_router(IpRateLimiter { + limiter: RateLimiter::new(1, Duration::from_secs(60)), + trust: TrustedProxy::XForwardedFor, + }); + let proxy: SocketAddr = "172.16.0.1:1".parse().unwrap(); + assert_eq!( + post_with(&router, proxy, &[]).await, + StatusCode::UNAUTHORIZED + ); + assert_eq!( + post_with(&router, proxy, &[]).await, + StatusCode::TOO_MANY_REQUESTS, + "with no trusted header, requests fall back to the socket peer key (collapse)" + ); + } } From 69a98cf43186ae7813fe704bd47fa89b8dd4f621 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 14:28:45 -0500 Subject: [PATCH 04/25] feat(node): purge-spam admin subcommand for the empty-burst cleanup Adds a purge-spam subcommand (clap #[command(subcommand)] split; no-subcommand path still runs the node unchanged). Candidate selection is signature-based, not per-DID: a repo qualifies only if owned by the named burst DID AND verified empty (zero git refs) per repo. A hard exclusion gate runs BEFORE the empty check, so an empty repo owned by the content-bearing user or the mirror-bot DID is never a candidate. Dry-run is the default; deletion needs an explicit --execute flag and operates per-repo, never 'all repos of DID X'. Tests (RED->GREEN, exclusion gate confirmed load-bearing by targeting an excluded DID directly): empty target listed; target-with-refs spared; empty excluded/ intern repos spared (must-assert); dry-run deletes nothing; execute removes only the empty target on disk. --- crates/gitlawb-node/src/admin.rs | 469 ++++++++++++++++++++++++++++++ crates/gitlawb-node/src/config.rs | 21 +- crates/gitlawb-node/src/db/mod.rs | 29 ++ crates/gitlawb-node/src/main.rs | 25 ++ 4 files changed, 543 insertions(+), 1 deletion(-) create mode 100644 crates/gitlawb-node/src/admin.rs diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs new file mode 100644 index 00000000..3e6ba3cb --- /dev/null +++ b/crates/gitlawb-node/src/admin.rs @@ -0,0 +1,469 @@ +//! Admin subcommands invoked out-of-band, not part of the running node. +//! +//! `purge-spam` produces a reviewable dry-run list of empty spam-burst repos and, +//! only behind an explicit `--execute` flag, deletes them one at a time. The +//! selection logic is the load-bearing security part: a repo qualifies ONLY if it +//! is owned by the named burst DID AND is verified empty (zero git refs) PER REPO, +//! and a hard exclusion gate (evaluated BEFORE the empty check) keeps +//! content-bearing and intern/mirror-bot DIDs out no matter what. + +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use tracing::{info, warn}; + +use crate::db::{Db, RepoRecord}; +use crate::git::store; + +/// The did:key of the spam burst this tool targets. The purge is scoped to +/// exactly this owner; an empty repo owned by anyone else is never a candidate. +pub const SPAM_BURST_TARGET_DID: &str = "did:key:z6Mkopj6mhcMayipekXbTRFMZPM6Bsgy4FQZuN9fannXSLTC"; + +/// DIDs that must never be touched, even when they own an empty repo whose +/// signature otherwise matches the burst. The gate is evaluated BEFORE the empty +/// check so it wins unconditionally: +/// - `z6Mkk4L…` is a content-bearing live user. +/// - `z6MkqRz…` is the intern / mirror-bot DID. +pub const EXCLUDED_DIDS: &[&str] = &[ + "did:key:z6Mkk4LDvfA8VQmdehbJDvxp133sdtXUhR2UkUnMPguX7gnP", + "did:key:z6MkqRzACJ5iCDdkiymAPK3gq18z2iecZHeAuUyW6JnwRfoM", +]; + +/// A repo selected for purge, with the evidence that qualified it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Candidate { + pub id: String, + pub owner_did: String, + pub name: String, + /// Number of git refs found on disk. Always 0 for a selected candidate — kept + /// as explicit evidence in the dry-run output rather than an implicit "empty". + pub ref_count: usize, +} + +/// Whether a DID is on the hard exclusion list. +fn is_excluded(owner_did: &str) -> bool { + EXCLUDED_DIDS.contains(&owner_did) +} + +/// Pure candidate selector — the security core, isolated from disk and DB so the +/// exclusion + empty logic is directly testable. +/// +/// `repos` is the raw row set to consider (the caller supplies the target DID's +/// rows). `ref_count_of` returns the number of git refs for a given repo; the CLI +/// wires the real on-disk ref source, tests inject precomputed counts. +/// +/// A repo qualifies ONLY if, PER REPO: +/// 1. its owner is NOT on the exclusion list (gate evaluated FIRST), AND +/// 2. its owner is exactly the target burst DID, AND +/// 3. it is verified empty — `ref_count_of` returns 0. +/// +/// The exclusion gate is checked before the empty check so that an empty repo +/// owned by an excluded DID is dropped regardless of its ref signature. +pub fn select_spam_candidates( + repos: &[RepoRecord], + target_did: &str, + mut ref_count_of: F, +) -> Vec +where + F: FnMut(&RepoRecord) -> usize, +{ + let mut out = Vec::new(); + for repo in repos { + // Hard exclusion gate FIRST — wins over the empty signature. + if is_excluded(&repo.owner_did) { + continue; + } + // Scope to the named burst only. + if repo.owner_did != target_did { + continue; + } + // Per-repo empty check. + let ref_count = ref_count_of(repo); + if ref_count != 0 { + continue; + } + out.push(Candidate { + id: repo.id.clone(), + owner_did: repo.owner_did.clone(), + name: repo.name.clone(), + ref_count, + }); + } + out +} + +/// Count the git refs of a repo on disk. Zero refs means the repo is empty. +/// +/// Resolves the repo's bare path from `repos_dir` + owner/name (the same layout +/// `git::store::repo_disk_path` writes) and shells to `git for-each-ref` via +/// `store::list_refs`. A repo whose on-disk path is missing or unreadable is +/// treated as having an unknown, non-empty ref count so it is NOT selected — the +/// tool fails closed and never deletes on a read error. +fn on_disk_ref_count(repos_dir: &Path, repo: &RepoRecord) -> usize { + let path = store::repo_disk_path(repos_dir, &repo.owner_did, &repo.name); + match store::list_refs(&path) { + Ok(refs) => refs.len(), + Err(e) => { + // Fail closed: an unreadable repo is not provably empty, so keep it + // out of the candidate set (report it as one ref so it's excluded). + warn!(repo = %repo.id, path = %path.display(), err = %e, + "purge-spam: could not read refs — treating as non-empty (skipped)"); + 1 + } + } +} + +/// Run the `purge-spam` admin subcommand. +/// +/// Enumerates the target burst DID's repos, verifies each is empty on disk, +/// applies the exclusion gate, prints one dry-run row per candidate with owner + +/// ref-count evidence, and — only when `execute` is true — deletes the DB row of +/// each candidate one at a time. Dry-run (the default) deletes nothing. +pub async fn run_purge_spam(db: &Db, repos_dir: &Path, execute: bool) -> Result<()> { + let repos_dir: PathBuf = repos_dir.to_path_buf(); + let rows = db + .list_repos_by_owner_did(SPAM_BURST_TARGET_DID) + .await + .context("listing repos for the spam-burst target DID")?; + + let candidates = select_spam_candidates(&rows, SPAM_BURST_TARGET_DID, |repo| { + on_disk_ref_count(&repos_dir, repo) + }); + + info!( + target = SPAM_BURST_TARGET_DID, + scanned = rows.len(), + candidates = candidates.len(), + execute, + "purge-spam: candidate selection complete" + ); + + if candidates.is_empty() { + println!("purge-spam: no empty spam-burst repos found for {SPAM_BURST_TARGET_DID}"); + return Ok(()); + } + + println!( + "purge-spam: {} candidate(s) for {} ({} mode)", + candidates.len(), + SPAM_BURST_TARGET_DID, + if execute { "EXECUTE" } else { "dry-run" } + ); + for c in &candidates { + println!( + " {} owner={} name={} refs={}", + c.id, c.owner_did, c.name, c.ref_count + ); + } + + if !execute { + println!( + "purge-spam: dry-run — nothing deleted. Re-run with --execute to delete the {} candidate(s).", + candidates.len() + ); + return Ok(()); + } + + // Execute: delete per-repo, never a single blanket "delete all of owner X". + let mut deleted = 0u64; + for c in &candidates { + match db.delete_repo_by_id(&c.id).await { + Ok(n) => { + deleted += n; + info!(repo = %c.id, rows = n, "purge-spam: deleted repo row"); + } + Err(e) => { + warn!(repo = %c.id, err = %e, "purge-spam: failed to delete repo row"); + } + } + } + println!("purge-spam: deleted {deleted} repo row(s)."); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::Utc; + + const TARGET: &str = SPAM_BURST_TARGET_DID; + const EXCLUDED_CONTENT: &str = EXCLUDED_DIDS[0]; // z6Mkk4L… content-bearing live user + const EXCLUDED_INTERN: &str = EXCLUDED_DIDS[1]; // z6MkqRz… intern/mirror-bot + const UNRELATED: &str = "did:key:z6MkUnrelatedStrangerDidThatIsNotTheBurst"; + + fn repo(id: &str, owner: &str, name: &str) -> RepoRecord { + RepoRecord { + id: id.to_string(), + name: name.to_string(), + owner_did: owner.to_string(), + description: None, + is_public: true, + default_branch: "main".to_string(), + created_at: Utc::now(), + updated_at: Utc::now(), + disk_path: format!("/srv/{id}.git"), + forked_from: None, + machine_id: None, + } + } + + /// Ref counts keyed by repo id; anything absent defaults to 0 (empty). + fn refs_by_id<'a>(map: &'a [(&'a str, usize)]) -> impl Fn(&RepoRecord) -> usize + 'a { + move |r: &RepoRecord| { + map.iter() + .find(|(id, _)| *id == r.id) + .map(|(_, n)| *n) + .unwrap_or(0) + } + } + + // Test 1: an empty repo owned by the target DID is a candidate. + #[test] + fn empty_target_repo_is_a_candidate() { + let repos = vec![repo("t-empty", TARGET, "spam1")]; + let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + assert_eq!(got.len(), 1, "empty target repo must be selected"); + assert_eq!(got[0].id, "t-empty"); + assert_eq!(got[0].owner_did, TARGET); + assert_eq!( + got[0].ref_count, 0, + "candidate must carry ref-count evidence" + ); + } + + // Test 2: a target-owned repo WITH refs is absent (per-repo empty check, not + // per-DID). + #[test] + fn target_repo_with_refs_is_absent() { + let repos = vec![ + repo("t-empty", TARGET, "spam1"), + repo("t-nonempty", TARGET, "real"), + ]; + let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[("t-nonempty", 3)])); + let ids: Vec<&str> = got.iter().map(|c| c.id.as_str()).collect(); + assert!(ids.contains(&"t-empty"), "empty target repo still selected"); + assert!( + !ids.contains(&"t-nonempty"), + "a target repo WITH refs must NOT be selected (per-repo, not per-DID)" + ); + } + + // Test 3 (MUST ASSERT): an EMPTY repo owned by the excluded content DID is + // absent — the exclusion gate wins over the empty signature. + // + // Driven with the excluded DID passed AS the target so the exclusion gate is + // the ONLY barrier: with the gate removed this repo would match owner==target + // and be selected, so the test goes RED. This is what makes the gate + // load-bearing rather than shadowed by the target-scope check. + #[test] + fn empty_excluded_content_repo_is_absent() { + let repos = vec![repo("x-content", EXCLUDED_CONTENT, "anything")]; + let got = select_spam_candidates(&repos, EXCLUDED_CONTENT, refs_by_id(&[])); + assert!( + got.is_empty(), + "an empty repo owned by the excluded content DID must be excluded even \ + if that DID were the target, got {got:?}" + ); + // And of course it is also absent when the real burst DID is the target. + let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + assert!(got.is_empty()); + } + + // Test 4 (MUST ASSERT): an EMPTY repo owned by the intern DID is absent. + // Same construction as test 3: the intern DID is passed as the target so the + // exclusion gate is the sole reason it is dropped (RED without the gate). + #[test] + fn empty_intern_repo_is_absent() { + let repos = vec![repo("x-intern", EXCLUDED_INTERN, "mirror")]; + let got = select_spam_candidates(&repos, EXCLUDED_INTERN, refs_by_id(&[])); + assert!( + got.is_empty(), + "an empty repo owned by the intern/mirror-bot DID must be excluded even \ + if that DID were the target, got {got:?}" + ); + let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + assert!(got.is_empty()); + } + + // Test 5: an empty repo owned by an unrelated DID is absent — the tool targets + // the named burst only. + #[test] + fn empty_unrelated_repo_is_absent() { + let repos = vec![repo("u-empty", UNRELATED, "whatever")]; + let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + assert!( + got.is_empty(), + "an empty repo owned by a non-target DID must not be selected, got {got:?}" + ); + } + + // The full matrix in one selector pass: only the empty target repo survives. + #[test] + fn full_matrix_selects_only_empty_target() { + let repos = vec![ + repo("t-empty", TARGET, "spam1"), // selected + repo("t-nonempty", TARGET, "real"), // has refs → out + repo("x-content", EXCLUDED_CONTENT, "a"), // excluded, empty → out + repo("x-intern", EXCLUDED_INTERN, "b"), // excluded, empty → out + repo("u-empty", UNRELATED, "c"), // wrong owner → out + ]; + let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[("t-nonempty", 2)])); + assert_eq!( + got.iter().map(|c| c.id.as_str()).collect::>(), + vec!["t-empty"], + "only the empty target repo may survive the full matrix" + ); + } + + // An excluded DID that is empty is still out even when that excluded DID is + // itself the target — pins that the exclusion gate runs BEFORE the owner/empty + // checks and is the sole barrier here (RED without the gate). + #[test] + fn exclusion_gate_precedes_empty_check() { + let repos = vec![repo("collision", EXCLUDED_CONTENT, "spam1")]; + let got = select_spam_candidates(&repos, EXCLUDED_CONTENT, refs_by_id(&[])); + assert!(got.is_empty(), "exclusion must win even on an empty repo"); + } +} + +#[cfg(test)] +mod db_tests { + use super::*; + use crate::db::{Db, RepoRecord}; + use chrono::Utc; + use sqlx::PgPool; + + async fn db(pool: PgPool) -> Db { + let db = Db::for_testing(pool); + db.run_migrations().await.unwrap(); + db + } + + fn rec(id: &str, owner: &str, name: &str) -> RepoRecord { + RepoRecord { + id: id.to_string(), + name: name.to_string(), + owner_did: owner.to_string(), + description: None, + is_public: true, + default_branch: "main".to_string(), + created_at: Utc::now(), + updated_at: Utc::now(), + disk_path: format!("/srv/{id}.git"), + forked_from: None, + machine_id: None, + } + } + + async fn count_rows(db: &Db) -> i64 { + sqlx::query_scalar::<_, i64>("SELECT COUNT(*) FROM repos") + .fetch_one(db.pool()) + .await + .unwrap() + } + + // Test 6: a dry-run over the full matrix deletes nothing. The empty check is + // driven off a repos_dir with NO repos on disk, so every row reads as empty + // (list_refs on a missing repo returns Err → treated as non-empty and skipped), + // which is fine here: the assertion is that dry-run mutates no rows regardless. + #[sqlx::test] + async fn dry_run_deletes_nothing(pool: PgPool) { + let db = db(pool).await; + for r in [ + rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"), + rec("t-nonempty", SPAM_BURST_TARGET_DID, "real"), + rec("x-content", EXCLUDED_DIDS[0], "a"), + rec("x-intern", EXCLUDED_DIDS[1], "b"), + rec("u-empty", "did:key:z6MkUnrelated", "c"), + ] { + db.create_repo(&r).await.unwrap(); + } + let before = count_rows(&db).await; + assert_eq!(before, 5); + + // Empty repos dir: no repo exists on disk, so nothing is provably empty. + let tmp = tempfile::TempDir::new().unwrap(); + run_purge_spam(&db, tmp.path(), false).await.unwrap(); + + let after = count_rows(&db).await; + assert_eq!(after, before, "dry-run must not delete any repo rows"); + } + + // The DB accessor lists exactly the target DID's rows (exact owner match), and + // delete_repo_by_id removes exactly one row, so the execute path deletes per + // repo. This exercises the DB wiring end-to-end with a real empty repo on disk. + #[sqlx::test] + async fn execute_deletes_only_the_empty_target_repo_on_disk(pool: PgPool) { + let db = db(pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + + // One empty target repo (real bare repo, zero refs) and one target repo + // with a ref, plus an excluded-owner empty repo. + let empty = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + let nonempty = rec("t-refs", SPAM_BURST_TARGET_DID, "real"); + let excluded = rec("x-content", EXCLUDED_DIDS[0], "keep"); + for r in [&empty, &nonempty, &excluded] { + db.create_repo(r).await.unwrap(); + } + + // Materialize the two target repos on disk. + let empty_path = store::repo_disk_path(tmp.path(), &empty.owner_did, &empty.name); + store::init_bare(&empty_path).unwrap(); + let refs_path = store::repo_disk_path(tmp.path(), &nonempty.owner_did, &nonempty.name); + store::init_bare(&refs_path).unwrap(); + // Give the non-empty repo an actual ref. + seed_one_ref(&refs_path); + + // Sanity: our on-disk ref reader sees the expected counts. + assert_eq!(store::list_refs(&empty_path).unwrap().len(), 0); + assert!(!store::list_refs(&refs_path).unwrap().is_empty()); + + run_purge_spam(&db, tmp.path(), true).await.unwrap(); + + // Only the empty target repo row is gone. + assert!(db + .get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none()); + assert!(db + .get_repo(SPAM_BURST_TARGET_DID, "real") + .await + .unwrap() + .is_some()); + assert!(db + .get_repo(EXCLUDED_DIDS[0], "keep") + .await + .unwrap() + .is_some()); + } + + /// Create a single commit + ref in a bare repo via a throwaway worktree, so the + /// repo reads as non-empty (≥1 ref). + fn seed_one_ref(bare: &Path) { + use std::process::Command; + let wt = bare.join("_seed"); + let run = |args: &[&str], dir: &Path| { + let ok = Command::new("git") + .args(args) + .current_dir(dir) + .status() + .unwrap() + .success(); + assert!(ok, "git {args:?} failed"); + }; + run( + &["worktree", "add", "--orphan", "-b", "main", "_seed"], + bare, + ); + std::fs::write(wt.join("f.txt"), b"x").unwrap(); + run(&["config", "user.email", "t@t"], &wt); + run(&["config", "user.name", "t"], &wt); + run(&["add", "."], &wt); + run(&["commit", "-qm", "seed"], &wt); + let _ = Command::new("git") + .args(["worktree", "remove", "--force", "_seed"]) + .current_dir(bare) + .status(); + } +} diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index fc2247d9..38f31e6e 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -1,9 +1,28 @@ -use clap::Parser; +use clap::{Parser, Subcommand}; use std::path::PathBuf; +/// Optional admin subcommands. When none is given, the binary runs the node +/// daemon as before — the default (no-subcommand) startup path is unchanged. +#[derive(Subcommand, Debug, Clone)] +pub enum Command { + /// Dry-run (default) or, with --execute, delete empty spam-burst repos owned + /// by the known burst DID. Never touches the hard-excluded DIDs, and verifies + /// each repo is empty (zero git refs) per repo before selecting it. + PurgeSpam { + /// Actually delete the candidates. Omit for a dry-run that prints the + /// candidate list and deletes nothing. + #[arg(long, default_value_t = false)] + execute: bool, + }, +} + #[derive(Parser, Debug, Clone)] #[command(name = "gitlawb-node", about = "gitlawb node daemon", version)] pub struct Config { + /// Admin subcommand to run instead of the node daemon. Absent = run the node. + #[command(subcommand)] + pub command: Option, + /// Directory where bare git repositories are stored #[arg(long, env = "GITLAWB_REPOS_DIR", default_value = "./data/repos")] pub repos_dir: PathBuf, diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 5324a4b9..46b5be71 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -1259,6 +1259,35 @@ impl Db { .await?; Ok(()) } + + /// Every repo row owned by exactly `owner_did` (an exact `owner_did` match, no + /// did:key normalization and no dedup) so a caller enumerating one DID's repos + /// sees every physical row. Backs the `purge-spam` admin tool, whose selection + /// then applies its own per-repo empty check and exclusion gate. Ordered by + /// `id` for a stable candidate listing. + pub async fn list_repos_by_owner_did(&self, owner_did: &str) -> Result> { + let rows = sqlx::query( + "SELECT id, name, owner_did, description, is_public, default_branch, + created_at, updated_at, disk_path, forked_from, machine_id + FROM repos WHERE owner_did = $1 ORDER BY id", + ) + .bind(owner_did) + .fetch_all(&self.pool) + .await?; + Ok(rows.into_iter().map(row_to_repo).collect()) + } + + /// Delete a single repo row by its primary key `id`. Returns the number of + /// rows removed (0 if no such repo). Operates on one repo at a time by design: + /// the `purge-spam` tool deletes a vetted candidate list per-repo, never a + /// blanket "delete all repos of owner X". + pub async fn delete_repo_by_id(&self, id: &str) -> Result { + let result = sqlx::query("DELETE FROM repos WHERE id = $1") + .bind(id) + .execute(&self.pool) + .await?; + Ok(result.rows_affected()) + } } // ── Agents / Trust ──────────────────────────────────────────────────────────── diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 4fd6e922..88984469 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -1,3 +1,4 @@ +mod admin; mod api; mod arweave; mod auth; @@ -70,6 +71,12 @@ async fn main() -> Result<()> { let mut config = Config::parse(); + // Admin subcommands run out-of-band and exit, never starting the node. The + // no-subcommand path below is the unchanged daemon startup. + if let Some(command) = config.command.clone() { + return run_admin_command(command, &config).await; + } + // Merge the embedded seed list of public network nodes into the runtime // bootstrap peers. Operators can opt out via GITLAWB_BOOTSTRAP_DISABLE_SEEDS. bootstrap::merge_seeds(&mut config); @@ -586,6 +593,24 @@ async fn main() -> Result<()> { Ok(()) } +/// Dispatch an admin subcommand. Connects the database directly (no server, no +/// p2p, no metrics) and runs the requested tool to completion, then exits. +async fn run_admin_command(command: config::Command, config: &Config) -> Result<()> { + match command { + config::Command::PurgeSpam { execute } => { + let acquire_timeout = std::time::Duration::from_secs(config.db_acquire_timeout_secs); + let db = Db::connect( + &config.database_url, + config.db_max_connections, + acquire_timeout, + ) + .await + .context("connecting to database for purge-spam")?; + admin::run_purge_spam(&db, &config.repos_dir, execute).await + } + } +} + fn spawn_shutdown_signal(tx: watch::Sender) { tokio::spawn(async move { #[cfg(unix)] From 13e59b4e40d205142b3ec8d03f4dc2c8566f3381 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 14:47:07 -0500 Subject: [PATCH 05/25] fix(review): make purge-spam empty-check reject git-discovery reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 from code review: on_disk_ref_count shelled 'git for-each-ref' with only the repo path as cwd and no --git-dir, so a path that exists but is not a bare repo let git discover a parent .git (repos_dir may sit inside the operator's checkout) and read a DIFFERENT repo's refs — possibly 0, deleting a real repo. Now require the bare-repo markers (HEAD file + objects/ dir) before trusting any count; anything else fails closed (skipped). Tests: a non-git dir under a git ancestor (zero-ref) now fails closed — verified RED without the guard (read the ancestor's 0 refs); target-DID-never-excluded invariant pinned; write_flood test anchored to a genuinely-drained bucket so its assert_ne can't pass vacuously. --- crates/gitlawb-node/src/admin.rs | 60 ++++++++++++++++++++++++++++ crates/gitlawb-node/src/api/repos.rs | 16 ++++++++ 2 files changed, 76 insertions(+) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 3e6ba3cb..202b4a51 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -99,8 +99,24 @@ where /// `store::list_refs`. A repo whose on-disk path is missing or unreadable is /// treated as having an unknown, non-empty ref count so it is NOT selected — the /// tool fails closed and never deletes on a read error. +/// +/// Critically, a `0` count must come from THIS exact bare repo and never from +/// git's upward repository discovery. `git for-each-ref` runs with the repo path +/// as its cwd and no explicit `--git-dir`, so if the path exists but is not +/// itself a git dir, git walks parent directories for a `.git` — and `repos_dir` +/// may live inside the operator's own git checkout. That would read a DIFFERENT +/// repo's refs (possibly `0`) and delete a real repo. We defend by requiring the +/// bare-repo markers (`HEAD` file + `objects/` dir) before trusting any count; +/// anything else fails closed (treated non-empty, skipped). fn on_disk_ref_count(repos_dir: &Path, repo: &RepoRecord) -> usize { let path = store::repo_disk_path(repos_dir, &repo.owner_did, &repo.name); + if !path.join("HEAD").is_file() || !path.join("objects").is_dir() { + // Not a bare git repo at the exact expected path. Do NOT trust a ref + // count that git discovery could have read from a parent repository. + warn!(repo = %repo.id, path = %path.display(), + "purge-spam: path is not a bare git repo — treating as non-empty (skipped)"); + return 1; + } match store::list_refs(&path) { Ok(refs) => refs.len(), Err(e) => { @@ -466,4 +482,48 @@ mod db_tests { .current_dir(bare) .status(); } + + /// A `0` ref count must come only from a real bare repo at the exact path, + /// never from git discovery walking up to a parent `.git`. Load-bearing: the + /// tempdir root is itself a git repo (zero refs), so a naive `for-each-ref` + /// run from a child non-git dir would discover it and report 0 — the delete-a- + /// real-repo fail-open. The marker check must make that path fail closed. + #[test] + fn nongit_path_fails_closed_even_under_a_git_ancestor() { + let tmp = tempfile::TempDir::new().unwrap(); + // Make the tempdir root a git repo with zero refs (the discovery trap). + std::process::Command::new("git") + .args(["init", "-q"]) + .current_dir(tmp.path()) + .status() + .unwrap(); + + let repo = rec("t-x", SPAM_BURST_TARGET_DID, "spam1"); + let path = store::repo_disk_path(tmp.path(), &repo.owner_did, &repo.name); + + // (a) Path exists as a plain (non-git) directory under the git ancestor. + // Without the marker guard, git discovery reads the ancestor's 0 refs and + // this repo would be deleted. It must fail closed (>=1, skipped). + std::fs::create_dir_all(&path).unwrap(); + assert_eq!( + on_disk_ref_count(tmp.path(), &repo), + 1, + "a non-git dir under a git ancestor must fail closed, not read the ancestor's refs" + ); + + // (b) A real empty bare repo at the same path reads 0 — a genuine candidate. + std::fs::remove_dir_all(&path).unwrap(); + store::init_bare(&path).unwrap(); + assert_eq!(on_disk_ref_count(tmp.path(), &repo), 0); + } + + /// The exclusion gate and the target scope must never overlap: if the burst + /// target were ever set to an excluded DID, the gate would fail to protect it. + #[test] + fn target_did_is_never_excluded() { + assert!( + !EXCLUDED_DIDS.contains(&SPAM_BURST_TARGET_DID), + "the purge target must never be an excluded (protected) DID" + ); + } } diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index a9cfcc7e..6c1fd92e 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -2684,6 +2684,22 @@ mod tests { assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); let router = crate::server::build_router(state); + + // Anchor the test: prove the write bucket is genuinely drained at the + // router (a write sink from this peer 429s) so the creation assertion + // below cannot pass vacuously on some unrelated non-429 status. + let mut wreq = Request::builder() + .method(Method::PUT) + .uri("/api/v1/repos/someowner/somerepo/star") + .body(Body::empty()) + .unwrap(); + wreq.extensions_mut().insert(ConnectInfo(peer)); + assert_eq!( + router.clone().oneshot(wreq).await.unwrap().status(), + StatusCode::TOO_MANY_REQUESTS, + "write bucket must be drained for this peer (test precondition)" + ); + // Creation from the same peer must NOT be 429 — its bucket is untouched. // (It fails later on missing signature; the point is it is not throttled.) let mut req = Request::builder() From 458fe50312fbf373baed5d87b534b024c9086261 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 15:03:41 -0500 Subject: [PATCH 06/25] fix(review): normalization-consistent purge, brake helper, CLI + coverage Addresses the remaining code-review findings: - purge-spam DID matching now uses did:key normalization on both sides (list_repos_by_owner_did via OWNER_KEY_CASE_SQL, is_excluded and the target scope via normalize_owner_key), so short/full form is consistent and an excluded identity is protected in either form. Adds a short-form exclusion test. - Extract the per-IP write brake into a chainable WriteBraked helper so a future write group can't silently ship unbraked (kept separate from add_auth_layers by design; documented why). - Mark --repos-dir / --database-url global so admin subcommands accept them after the subcommand (was an 'unexpected argument' parse error); verified. - Document GITLAWB_WRITE_RATE_LIMIT in .env.example incl. the per-IP NAT-aggregate caveat so operators can size it. - Coverage: adoption-floor (under-limit write passes), write-limit=0 disables, task/bounty/profile route-level 429s, /graphql/ws stays unbraked. Full suite 927 pass / 0 fail; fmt + clippy -D warnings clean. --- .env.example | 11 +++ crates/gitlawb-node/src/admin.rs | 43 ++++++++- crates/gitlawb-node/src/api/repos.rs | 136 +++++++++++++++++++++++++++ crates/gitlawb-node/src/config.rs | 16 +++- crates/gitlawb-node/src/db/mod.rs | 26 ++--- crates/gitlawb-node/src/server.rs | 36 ++++--- 6 files changed, 237 insertions(+), 31 deletions(-) diff --git a/.env.example b/.env.example index bbd9a342..5987a527 100644 --- a/.env.example +++ b/.env.example @@ -127,6 +127,17 @@ GITLAWB_PUSH_RATE_LIMIT=600 # the client IP. 0 disables. Default 120. GITLAWB_CREATE_RATE_LIMIT=120 +# ── Write rate limiting (non-creation authenticated writes) ─────────────── +# Max non-creation write requests per client IP per hour: issue/PR comments, +# labels, stars, merges, protect/unprotect, replicas, visibility, tasks, +# bounties, profile, and GraphQL mutations. Its own bucket, separate from the +# creation and push brakes. Uses GITLAWB_TRUSTED_PROXY to resolve the client IP. +# NOTE: this is a per-IP aggregate across ALL those write actions, so behind a +# shared NAT/egress IP (or with GITLAWB_TRUSTED_PROXY unset) many users collapse +# onto one bucket — raise this for automation-heavy or multi-user single-IP +# deployments. 0 disables. Default 600. +GITLAWB_WRITE_RATE_LIMIT=600 + # ── Peer-sync rate limiting (per client IP, uses GITLAWB_TRUSTED_PROXY below) ─ # /api/v1/peers/announce and /api/v1/sync/notify accept unsigned requests from # known peers and run at higher frequency, so a generous bucket. Separate from diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 202b4a51..c428b765 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -40,9 +40,15 @@ pub struct Candidate { pub ref_count: usize, } -/// Whether a DID is on the hard exclusion list. +/// Whether a DID is on the hard exclusion list. Compared under did:key +/// normalization (the same convention the repos table and every ownership check +/// use), so an excluded identity stored in either `did:key:z6…` or bare `z6…` +/// form is protected regardless of the form the exclusion constant is written in. fn is_excluded(owner_did: &str) -> bool { - EXCLUDED_DIDS.contains(&owner_did) + let owner_key = crate::db::normalize_owner_key(owner_did); + EXCLUDED_DIDS + .iter() + .any(|d| crate::db::normalize_owner_key(d) == owner_key) } /// Pure candidate selector — the security core, isolated from disk and DB so the @@ -73,8 +79,11 @@ where if is_excluded(&repo.owner_did) { continue; } - // Scope to the named burst only. - if repo.owner_did != target_did { + // Scope to the named burst only, under did:key normalization so a burst + // row stored in either did:key or bare form is matched consistently. + if crate::db::normalize_owner_key(&repo.owner_did) + != crate::db::normalize_owner_key(target_did) + { continue; } // Per-repo empty check. @@ -526,4 +535,30 @@ mod db_tests { "the purge target must never be an excluded (protected) DID" ); } + + /// Exclusion is normalization-consistent: an excluded identity stored in the + /// bare short form (as mirror upserts write it) is still excluded, even though + /// the exclusion constants are full did:key form — and an empty repo it owns + /// is never selected. + #[test] + fn short_form_excluded_did_is_still_protected() { + let short = crate::db::normalize_owner_key(EXCLUDED_DIDS[1]); // bare z6MkqRz… + assert_ne!( + short, EXCLUDED_DIDS[1], + "fixture must actually be short form" + ); + assert!( + is_excluded(short), + "short-form of an excluded DID must be excluded" + ); + + // An empty repo owned by the short-form excluded DID is spared even though + // its ref signature (0) otherwise matches the burst. + let empty_excluded_short = rec("x-short", short, "spam"); + let cands = select_spam_candidates(&[empty_excluded_short], SPAM_BURST_TARGET_DID, |_| 0); + assert!( + cands.is_empty(), + "an empty repo owned by a short-form excluded DID must never be a candidate" + ); + } } diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 6c1fd92e..c48d2a09 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -2786,4 +2786,140 @@ mod tests { "issue-write routes must be IP-throttled by the write brake" ); } + + // Adoption floor: an under-limit write must NOT be throttled. Guards against + // an off-by-one that braked the first request (invisible to the 429 tests, + // which all pre-exhaust the bucket). + #[sqlx::test] + async fn under_limit_write_is_not_throttled(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + // Ample budget; bucket NOT exhausted. + state.write_rate_limiter = + crate::rate_limit::RateLimiter::new(100, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + let peer: SocketAddr = "203.0.113.150:7000".parse().unwrap(); + + let router = crate::server::build_router(state); + let mut req = Request::builder() + .method(Method::PUT) + .uri("/api/v1/repos/someowner/somerepo/star") + .body(Body::empty()) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + assert_ne!( + router.oneshot(req).await.unwrap().status(), + StatusCode::TOO_MANY_REQUESTS, + "an under-limit write must pass the brake, not be 429'd" + ); + } + + // GITLAWB_WRITE_RATE_LIMIT=0 disables the brake end-to-end: no write is 429'd + // however many arrive from one IP. + #[sqlx::test] + async fn write_rate_limit_zero_disables_the_brake(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + state.write_rate_limiter = crate::rate_limit::RateLimiter::new(0, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + let peer: SocketAddr = "203.0.113.151:7000".parse().unwrap(); + + let router = crate::server::build_router(state); + for _ in 0..5 { + let mut req = Request::builder() + .method(Method::PUT) + .uri("/api/v1/repos/someowner/somerepo/star") + .body(Body::empty()) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + assert_ne!( + router.clone().oneshot(req).await.unwrap().status(), + StatusCode::TOO_MANY_REQUESTS, + "a 0 write limit must disable the brake" + ); + } + } + + // The task/bounty/profile write groups share the write brake (same + // attachment as write_routes); prove each 429s at the route level. + #[sqlx::test] + async fn task_bounty_profile_writes_are_rate_limited(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + let peer: SocketAddr = "203.0.113.152:7000".parse().unwrap(); + assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); + + let router = crate::server::build_router(state); + for (method, uri) in [ + (Method::POST, "/api/v1/tasks"), + (Method::POST, "/api/v1/repos/o/r/bounties"), + (Method::PUT, "/api/v1/profile"), + ] { + let mut req = Request::builder() + .method(method) + .uri(uri) + .header("content-type", "application/json") + .body(Body::from("{}")) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + assert_eq!( + router.clone().oneshot(req).await.unwrap().status(), + StatusCode::TOO_MANY_REQUESTS, + "write group {uri} must be IP-throttled by the write brake" + ); + } + } + + // /graphql/ws (subscriptions) is deliberately mounted AFTER the write brake + // layer, so it must stay unbraked even when the write bucket is exhausted. + #[sqlx::test] + async fn graphql_ws_is_not_braked(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + let peer: SocketAddr = "203.0.113.153:7000".parse().unwrap(); + assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); + + let router = crate::server::build_router(state); + let mut req = Request::builder() + .method(Method::GET) + .uri("/graphql/ws") + .body(Body::empty()) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + // Not a real ws upgrade, so the subscription service rejects it with some + // non-429 status; the point is the write brake never sees it. + assert_ne!( + router.oneshot(req).await.unwrap().status(), + StatusCode::TOO_MANY_REQUESTS, + "/graphql/ws must not be behind the write brake" + ); + } } diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index 38f31e6e..06ff5081 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -23,15 +23,23 @@ pub struct Config { #[command(subcommand)] pub command: Option, - /// Directory where bare git repositories are stored - #[arg(long, env = "GITLAWB_REPOS_DIR", default_value = "./data/repos")] + /// Directory where bare git repositories are stored. `global` so it can + /// follow a subcommand (e.g. `gitlawb-node purge-spam --repos-dir …`). + #[arg( + long, + env = "GITLAWB_REPOS_DIR", + default_value = "./data/repos", + global = true + )] pub repos_dir: PathBuf, - /// PostgreSQL connection URL (Supabase or any Postgres instance) + /// PostgreSQL connection URL (Supabase or any Postgres instance). `global` so + /// admin subcommands accept it in either position. #[arg( long, env = "DATABASE_URL", - default_value = "postgresql://localhost/gitlawb" + default_value = "postgresql://localhost/gitlawb", + global = true )] pub database_url: String, diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 46b5be71..706d670b 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -1260,20 +1260,24 @@ impl Db { Ok(()) } - /// Every repo row owned by exactly `owner_did` (an exact `owner_did` match, no - /// did:key normalization and no dedup) so a caller enumerating one DID's repos - /// sees every physical row. Backs the `purge-spam` admin tool, whose selection - /// then applies its own per-repo empty check and exclusion gate. Ordered by - /// `id` for a stable candidate listing. + /// Every repo row whose owner resolves to `owner_did` under did:key + /// normalization, so `did:key:z6…` and bare `z6…` rows of the same identity + /// both match (mirroring how ownership is resolved everywhere else via + /// `OWNER_KEY_CASE_SQL` / the `idx_repos_owner_key_name` index). No dedup, so a + /// caller enumerating one DID's repos sees every physical row. Backs the + /// `purge-spam` admin tool, whose selection then applies its own per-repo empty + /// check and exclusion gate (both normalization-consistent). Ordered by `id`. pub async fn list_repos_by_owner_did(&self, owner_did: &str) -> Result> { - let rows = sqlx::query( + let sql = format!( "SELECT id, name, owner_did, description, is_public, default_branch, created_at, updated_at, disk_path, forked_from, machine_id - FROM repos WHERE owner_did = $1 ORDER BY id", - ) - .bind(owner_did) - .fetch_all(&self.pool) - .await?; + FROM repos WHERE {key} = $1 ORDER BY id", + key = OWNER_KEY_CASE_SQL + ); + let rows = sqlx::query(&sql) + .bind(normalize_owner_key(owner_did)) + .fetch_all(&self.pool) + .await?; Ok(rows.into_iter().map(row_to_repo).collect()) } diff --git a/crates/gitlawb-node/src/server.rs b/crates/gitlawb-node/src/server.rs index 0c24d953..1456b700 100644 --- a/crates/gitlawb-node/src/server.rs +++ b/crates/gitlawb-node/src/server.rs @@ -55,6 +55,24 @@ fn add_auth_layers(router: Router, state: AppState) -> Router Self; +} + +impl WriteBraked for Router { + fn write_braked(self, limiter: &rate_limit::IpRateLimiter) -> Self { + self.layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) + .layer(axum::Extension(limiter.clone())) + } +} + pub fn build_router(state: AppState) -> Router { // ── GraphQL routes ───────────────────────────────────────────────────── let schema = state.graphql_schema.as_ref().clone(); @@ -77,8 +95,7 @@ pub fn build_router(state: AppState) -> Router { // a mutation POST, so it counts every /graphql request — acceptable, both // cost work); /graphql/ws subscriptions stay unbraked. .layer(middleware::from_fn(auth::optional_signature)) - .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) - .layer(axum::Extension(write_ip_limiter.clone())) + .write_braked(&write_ip_limiter) .route_service("/graphql/ws", GraphQLSubscription::new(schema)); // ── Task routes (write — require HTTP Signature + per-IP write brake) ── @@ -90,8 +107,7 @@ pub fn build_router(state: AppState) -> Router { .route("/api/v1/tasks/{id}/fail", post(tasks::fail_task)), state.clone(), ) - .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) - .layer(axum::Extension(write_ip_limiter.clone())); + .write_braked(&write_ip_limiter); // ── Task routes (read — open) ────────────────────────────────────────── let task_read_routes = Router::new() @@ -205,8 +221,7 @@ pub fn build_router(state: AppState) -> Router { ), state.clone(), ) - .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) - .layer(axum::Extension(write_ip_limiter.clone())); + .write_braked(&write_ip_limiter); // Body limit is raised to GITLAWB_MAX_PACK_BYTES (default 2 GB) for git // routes only — all other API routes keep axum's default 2 MB cap. @@ -273,8 +288,7 @@ pub fn build_router(state: AppState) -> Router { ), state.clone(), ) - .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) - .layer(axum::Extension(write_ip_limiter.clone())); + .write_braked(&write_ip_limiter); // ── Bounty routes (read — open) ────────────────────────────────────── let bounty_read_routes = Router::new() @@ -296,8 +310,7 @@ pub fn build_router(state: AppState) -> Router { Router::new().route("/api/v1/profile", axum::routing::put(profiles::set_profile)), state.clone(), ) - .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) - .layer(axum::Extension(write_ip_limiter.clone())); + .write_braked(&write_ip_limiter); // ── Issue routes (write — require HTTP Signature + per-IP write brake) ─ let issue_write_routes = add_auth_layers( @@ -312,8 +325,7 @@ pub fn build_router(state: AppState) -> Router { ), state.clone(), ) - .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) - .layer(axum::Extension(write_ip_limiter.clone())); + .write_braked(&write_ip_limiter); // ── Peer discovery routes ───────────────────────────────────────────── // Peer writes accept signatures when present and can require them after a From 27b4a2a74778b4eee56b20762b42ba25a975691e Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 15:23:19 -0500 Subject: [PATCH 07/25] test(review): close the reasoned-not-run gaps by execution - purge-spam execute now re-verifies emptiness immediately before delete (TOCTOU: a push between selection and delete no longer risks deleting a now-non-empty repo) and removes the on-disk bare repo dir so DB and disk stay consistent; a per-repo delete failure warns and continues rather than aborting the batch. - Tests (RED-verified where they cover new behavior): partition_for_delete skips a repo no longer empty; execute removes the on-disk dir (RED without the removal); list_repos_by_owner_did finds a short-form burst row (RED without the SQL normalization); every braked write group passes under-limit (per-group grant path, not just the star representative). --- crates/gitlawb-node/src/admin.rs | 136 +++++++++++++++++++++++++-- crates/gitlawb-node/src/api/repos.rs | 42 +++++++++ 2 files changed, 171 insertions(+), 7 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index c428b765..01c03432 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -118,11 +118,17 @@ where /// bare-repo markers (`HEAD` file + `objects/` dir) before trusting any count; /// anything else fails closed (treated non-empty, skipped). fn on_disk_ref_count(repos_dir: &Path, repo: &RepoRecord) -> usize { - let path = store::repo_disk_path(repos_dir, &repo.owner_did, &repo.name); + ref_count_on_disk(repos_dir, &repo.owner_did, &repo.name) +} + +/// Core of [`on_disk_ref_count`], keyed on owner+name so the execute path can +/// re-verify emptiness right before deleting (using only a [`Candidate`]). +fn ref_count_on_disk(repos_dir: &Path, owner_did: &str, name: &str) -> usize { + let path = store::repo_disk_path(repos_dir, owner_did, name); if !path.join("HEAD").is_file() || !path.join("objects").is_dir() { // Not a bare git repo at the exact expected path. Do NOT trust a ref // count that git discovery could have read from a parent repository. - warn!(repo = %repo.id, path = %path.display(), + warn!(path = %path.display(), "purge-spam: path is not a bare git repo — treating as non-empty (skipped)"); return 1; } @@ -131,13 +137,36 @@ fn on_disk_ref_count(repos_dir: &Path, repo: &RepoRecord) -> usize { Err(e) => { // Fail closed: an unreadable repo is not provably empty, so keep it // out of the candidate set (report it as one ref so it's excluded). - warn!(repo = %repo.id, path = %path.display(), err = %e, + warn!(path = %path.display(), err = %e, "purge-spam: could not read refs — treating as non-empty (skipped)"); 1 } } } +/// Split selected candidates into (delete, skip) by a fresh emptiness re-check, +/// so a repo that gained a ref between selection and deletion (a TOCTOU push) +/// is never deleted. Pure over the `recheck` closure so the skip branch is +/// directly testable; the CLI wires the real on-disk re-check. +fn partition_for_delete( + candidates: &[Candidate], + mut recheck: F, +) -> (Vec<&Candidate>, Vec<&Candidate>) +where + F: FnMut(&Candidate) -> usize, +{ + let mut to_delete = Vec::new(); + let mut to_skip = Vec::new(); + for c in candidates { + if recheck(c) == 0 { + to_delete.push(c); + } else { + to_skip.push(c); + } + } + (to_delete, to_skip) +} + /// Run the `purge-spam` admin subcommand. /// /// Enumerates the target burst DID's repos, verifies each is empty on disk, @@ -189,20 +218,44 @@ pub async fn run_purge_spam(db: &Db, repos_dir: &Path, execute: bool) -> Result< return Ok(()); } + // Re-verify emptiness immediately before deleting: a push may have landed + // between selection and now (TOCTOU). Anything no longer empty is skipped, + // never deleted. + let (to_delete, to_skip) = partition_for_delete(&candidates, |c| { + ref_count_on_disk(&repos_dir, &c.owner_did, &c.name) + }); + for c in &to_skip { + warn!(repo = %c.id, "purge-spam: repo no longer empty at delete time — skipped (TOCTOU)"); + } + // Execute: delete per-repo, never a single blanket "delete all of owner X". + // A per-repo failure warns and continues rather than aborting the batch. let mut deleted = 0u64; - for c in &candidates { + for c in &to_delete { match db.delete_repo_by_id(&c.id).await { + Ok(0) => { + warn!(repo = %c.id, "purge-spam: repo row already gone — nothing to delete"); + } Ok(n) => { deleted += n; - info!(repo = %c.id, rows = n, "purge-spam: deleted repo row"); + // Remove the now-orphaned on-disk bare repo (empty, so cheap) so + // the DB row and disk stay consistent. + let path = store::repo_disk_path(&repos_dir, &c.owner_did, &c.name); + if let Err(e) = std::fs::remove_dir_all(&path) { + warn!(repo = %c.id, path = %path.display(), err = %e, + "purge-spam: deleted DB row but could not remove on-disk repo dir"); + } + info!(repo = %c.id, rows = n, "purge-spam: deleted repo row + on-disk dir"); } Err(e) => { - warn!(repo = %c.id, err = %e, "purge-spam: failed to delete repo row"); + warn!(repo = %c.id, err = %e, "purge-spam: failed to delete repo row — continuing"); } } } - println!("purge-spam: deleted {deleted} repo row(s)."); + println!( + "purge-spam: deleted {deleted} repo row(s), skipped {} (no longer empty).", + to_skip.len() + ); Ok(()) } @@ -349,6 +402,30 @@ mod tests { let got = select_spam_candidates(&repos, EXCLUDED_CONTENT, refs_by_id(&[])); assert!(got.is_empty(), "exclusion must win even on an empty repo"); } + + // TOCTOU: a candidate that gained a ref between selection and the pre-delete + // re-check must be skipped, not deleted; the rest of the batch still deletes. + #[test] + fn partition_for_delete_skips_repos_no_longer_empty() { + let cand = |id: &str| Candidate { + id: id.into(), + owner_did: "o".into(), + name: id.into(), + ref_count: 0, + }; + let cands = vec![cand("still-empty"), cand("now-nonempty")]; + // Re-check reports the second repo as no longer empty. + let (to_delete, to_skip) = + partition_for_delete(&cands, |c| usize::from(c.id == "now-nonempty")); + assert_eq!( + to_delete.iter().map(|c| c.id.as_str()).collect::>(), + ["still-empty"] + ); + assert_eq!( + to_skip.iter().map(|c| c.id.as_str()).collect::>(), + ["now-nonempty"] + ); + } } #[cfg(test)] @@ -463,6 +540,51 @@ mod db_tests { .is_some()); } + // Execute removes the on-disk bare repo dir too, so the DB row and disk stay + // consistent (no orphaned empty git dir left behind). + #[sqlx::test] + async fn execute_removes_the_on_disk_repo_dir(pool: PgPool) { + let db = db(pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let empty = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&empty).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &empty.owner_did, &empty.name); + store::init_bare(&path).unwrap(); + assert!(path.exists(), "precondition: on-disk repo exists"); + + run_purge_spam(&db, tmp.path(), true).await.unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none(), + "DB row deleted" + ); + assert!(!path.exists(), "on-disk bare repo dir must be removed too"); + } + + // The DB query normalizes did:key form (OWNER_KEY_CASE_SQL), so a burst repo + // stored in SHORT (bare) form is still found when querying by the full-form + // target DID — the SQL side of the normalization fix. + #[sqlx::test] + async fn list_repos_by_owner_did_matches_short_form(pool: PgPool) { + let db = db(pool).await; + let short = crate::db::normalize_owner_key(SPAM_BURST_TARGET_DID); + assert_ne!(short, SPAM_BURST_TARGET_DID, "fixture must be short form"); + let repo = rec("short-owned", short, "spam"); + db.create_repo(&repo).await.unwrap(); + + let rows = db + .list_repos_by_owner_did(SPAM_BURST_TARGET_DID) + .await + .unwrap(); + assert!( + rows.iter().any(|r| r.id == "short-owned"), + "a short-form burst row must be found when querying by the full-form target" + ); + } + /// Create a single commit + ref in a bare repo via a throwaway worktree, so the /// repo reads as non-empty (≥1 ref). fn seed_one_ref(bare: &Path) { diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index c48d2a09..3b9d888e 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -2922,4 +2922,46 @@ mod tests { "/graphql/ws must not be behind the write brake" ); } + + // Adoption floor, per group: with an un-exhausted bucket, a write to EVERY + // braked group passes the brake (reaches auth/handler), not 429. Guards each + // group's grant path, not just the star representative. + #[sqlx::test] + async fn every_write_group_passes_under_limit(pool: sqlx::PgPool) { + use axum::body::Body; + use axum::extract::ConnectInfo; + use axum::http::{Method, Request, StatusCode}; + use std::net::SocketAddr; + use std::time::Duration; + use tower::ServiceExt; + + let mut state = crate::test_support::test_state(pool).await; + state.write_rate_limiter = + crate::rate_limit::RateLimiter::new(100, Duration::from_secs(60)); + state.push_limiter_trust = crate::rate_limit::TrustedProxy::None; + let peer: SocketAddr = "203.0.113.160:7000".parse().unwrap(); + + let router = crate::server::build_router(state); + for (method, uri) in [ + (Method::PUT, "/api/v1/repos/o/r/star"), + (Method::POST, "/graphql"), + (Method::POST, "/api/v1/repos/o/r/issues/1/comments"), + (Method::POST, "/api/v1/tasks"), + (Method::POST, "/api/v1/repos/o/r/bounties"), + (Method::PUT, "/api/v1/profile"), + ] { + let mut req = Request::builder() + .method(method) + .uri(uri) + .header("content-type", "application/json") + .body(Body::from(r#"{"query":"{ __typename }"}"#)) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(peer)); + assert_ne!( + router.clone().oneshot(req).await.unwrap().status(), + StatusCode::TOO_MANY_REQUESTS, + "under-limit write to {uri} must pass the brake, not 429" + ); + } + } } From 98150db98cd9a27af5fd24a96af8a176f7b9bcab Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 21:53:23 -0500 Subject: [PATCH 08/25] fix(node): reject path-traversal repo names in purge-spam before remove_dir_all A peer-mirror row skips API name validation, so a burst-owned repo can carry a '../' name; repo_disk_path joined it verbatim and the emptiness marker check passed on the traversed target, so purge-spam selected and remove_dir_all'd a repo OUTSIDE repos_dir. Validate the name via repo_store::validate_repo_name at selection (fail closed -> non-candidate) and assert canonical containment inside repos_dir immediately before the destructive remove. Adversarial RED->GREEN: a victim bare repo reachable via '../../victim' is deleted before, survives after; symlink-escape covered by path_within. --- crates/gitlawb-node/src/admin.rs | 100 +++++++++++++++++++++- crates/gitlawb-node/src/git/repo_store.rs | 2 +- 2 files changed, 99 insertions(+), 3 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 01c03432..90e3162a 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -124,6 +124,16 @@ fn on_disk_ref_count(repos_dir: &Path, repo: &RepoRecord) -> usize { /// Core of [`on_disk_ref_count`], keyed on owner+name so the execute path can /// re-verify emptiness right before deleting (using only a [`Candidate`]). fn ref_count_on_disk(repos_dir: &Path, owner_did: &str, name: &str) -> usize { + // Fail closed on an unsafe repo name BEFORE building any on-disk path. A + // peer-mirror row (which skips API name validation) can carry a `../` name; + // `repo_disk_path` would join it verbatim and resolve OUTSIDE `repos_dir`, + // pointing this "empty" check — and later the delete — at an unrelated repo. + // Reject it here so such a row is never a candidate (treated non-empty). + if let Err(e) = crate::git::repo_store::validate_repo_name(name) { + warn!(name = %name, err = %e, + "purge-spam: unsafe repo name — treating as non-empty (skipped)"); + return 1; + } let path = store::repo_disk_path(repos_dir, owner_did, name); if !path.join("HEAD").is_file() || !path.join("objects").is_dir() { // Not a bare git repo at the exact expected path. Do NOT trust a ref @@ -144,6 +154,17 @@ fn ref_count_on_disk(repos_dir: &Path, owner_did: &str, name: &str) -> usize { } } +/// Whether `path` resolves canonically inside `root`. Both are canonicalized so +/// symlinks and `..` segments are fully resolved before the containment test; a +/// path that does not exist (or a root that cannot be canonicalized) fails closed +/// to `false`. Used as the last gate before a destructive `remove_dir_all`. +fn path_within(path: &Path, root: &Path) -> bool { + match (std::fs::canonicalize(path), std::fs::canonicalize(root)) { + (Ok(p), Ok(r)) => p.starts_with(&r), + _ => false, + } +} + /// Split selected candidates into (delete, skip) by a fresh emptiness re-check, /// so a repo that gained a ref between selection and deletion (a TOCTOU push) /// is never deleted. Pure over the `recheck` closure so the skip branch is @@ -239,9 +260,16 @@ pub async fn run_purge_spam(db: &Db, repos_dir: &Path, execute: bool) -> Result< Ok(n) => { deleted += n; // Remove the now-orphaned on-disk bare repo (empty, so cheap) so - // the DB row and disk stay consistent. + // the DB row and disk stay consistent. Belt-and-suspenders: assert + // the resolved path is canonically INSIDE repos_dir before any + // remove_dir_all, so a symlinked slug dir or any residual traversal + // can never delete outside the repo root (the name is already + // validated at selection; this guards the destructive op itself). let path = store::repo_disk_path(&repos_dir, &c.owner_did, &c.name); - if let Err(e) = std::fs::remove_dir_all(&path) { + if !path_within(&path, &repos_dir) { + warn!(repo = %c.id, path = %path.display(), + "purge-spam: on-disk path escapes repos_dir — refusing to remove"); + } else if let Err(e) = std::fs::remove_dir_all(&path) { warn!(repo = %c.id, path = %path.display(), err = %e, "purge-spam: deleted DB row but could not remove on-disk repo dir"); } @@ -564,6 +592,47 @@ mod db_tests { assert!(!path.exists(), "on-disk bare repo dir must be removed too"); } + // U1 (M3): a burst-owned row whose NAME traverses out of repos_dir must never + // cause a delete OUTSIDE repos_dir. Adversarial must-not: a real empty bare repo + // planted as a "victim" beside repos_dir is reachable from repos_dir// via + // a `../../victim` name; because the traversed path IS a real bare repo, the + // marker check passes and the candidate is selected — then remove_dir_all would + // delete the victim. The name validator must reject it so the victim survives. + #[sqlx::test] + async fn traversal_name_cannot_delete_a_repo_outside_repos_dir(pool: PgPool) { + let db = db(pool).await; + let root = tempfile::TempDir::new().unwrap(); + let repos_dir = root.path().join("repos"); + std::fs::create_dir_all(&repos_dir).unwrap(); + + // The burst DID's own slug dir must exist for the OS to resolve the `..` + // segments (a burst that owns any normal repo already has this dir). + let slug = SPAM_BURST_TARGET_DID.replace([':', '/'], "_"); + std::fs::create_dir_all(repos_dir.join(&slug)).unwrap(); + + // Victim: a real empty bare repo OUTSIDE repos_dir (sibling under root). + let victim = root.path().join("victim.git"); + store::init_bare(&victim).unwrap(); + assert!(victim.join("HEAD").is_file(), "victim precondition"); + + // Sanity: the evil name resolves from repos_dir// onto the victim. + let evil = rec("evil", SPAM_BURST_TARGET_DID, "../../victim"); + let traversed = store::repo_disk_path(&repos_dir, &evil.owner_did, &evil.name); + assert_eq!( + std::fs::canonicalize(&traversed).unwrap(), + std::fs::canonicalize(&victim).unwrap(), + "test setup: the evil name must resolve onto the victim" + ); + + db.create_repo(&evil).await.unwrap(); + run_purge_spam(&db, &repos_dir, true).await.unwrap(); + + assert!( + victim.join("HEAD").is_file(), + "a repo OUTSIDE repos_dir must never be deleted via a traversal name" + ); + } + // The DB query normalizes did:key form (OWNER_KEY_CASE_SQL), so a burst repo // stored in SHORT (bare) form is still found when querying by the full-form // target DID — the SQL side of the normalization fix. @@ -648,6 +717,33 @@ mod db_tests { assert_eq!(on_disk_ref_count(tmp.path(), &repo), 0); } + /// The belt-and-suspenders containment gate (`path_within`) must reject a path + /// that resolves outside repos_dir even when the *name* itself is innocuous — + /// e.g. the owner slug dir is a symlink pointing elsewhere. Layer 1 (name + /// validation) can't see this; only the canonical-containment check catches it. + #[test] + fn path_within_rejects_symlink_escape() { + let root = tempfile::TempDir::new().unwrap(); + let repos_dir = root.path().join("repos"); + std::fs::create_dir_all(&repos_dir).unwrap(); + + // A real dir outside repos_dir, and a symlink INTO repos_dir that targets it. + let outside = root.path().join("outside.git"); + std::fs::create_dir_all(&outside).unwrap(); + let link = repos_dir.join("evil.git"); + std::os::unix::fs::symlink(&outside, &link).unwrap(); + + // The name is innocuous, but the path resolves outside repos_dir. + assert!( + !path_within(&link, &repos_dir), + "a symlink escaping repos_dir must fail the containment gate" + ); + // A genuine path inside repos_dir passes. + let inside = repos_dir.join("real.git"); + std::fs::create_dir_all(&inside).unwrap(); + assert!(path_within(&inside, &repos_dir), "an in-root path must pass"); + } + /// The exclusion gate and the target scope must never overlap: if the burst /// target were ever set to an excluded DID, the gate would fail to protect it. #[test] diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index a5c367e9..c7aa954a 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -324,7 +324,7 @@ fn validate_owner_did(owner_did: &str) -> Result<()> { Ok(()) } -fn validate_repo_name(repo_name: &str) -> Result<()> { +pub(crate) fn validate_repo_name(repo_name: &str) -> Result<()> { if repo_name.is_empty() { anyhow::bail!("repo_name is empty"); } From 1688bdc05d54e638f8290331008acec3e20ebc26 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 21:58:31 -0500 Subject: [PATCH 09/25] fix(node): delete child rows transactionally in delete_repo_by_id delete_repo_by_id was a bare DELETE FROM repos with no cascade and the schema has no FKs, so every child row (ref_certificates, pull_requests + pr_reviews/ pr_comments, webhooks, push_events, protected_branches, repo_stars, repo_replicas, repo_labels, visibility_rules, encrypted_blobs, repo_icaptcha_proofs, agent_tasks, and the slug-keyed branch_cids/sync_queue/received_ref_updates/arweave_anchors) was orphaned and could re-join a later repo reusing the id. Wrap parent+child deletes in one transaction, resolving the owner_short/name slug for the repo-TEXT children. bounties (financial) and issue_comments (unmappable) are left intact by design. RED->GREEN test seeds a repo_id child, a slug child, and a PR grandchild. --- crates/gitlawb-node/src/db/mod.rs | 138 +++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 706d670b..5b2fb79e 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -1286,10 +1286,77 @@ impl Db { /// the `purge-spam` tool deletes a vetted candidate list per-repo, never a /// blanket "delete all repos of owner X". pub async fn delete_repo_by_id(&self, id: &str) -> Result { + let mut tx = self.pool.begin().await?; + + // Resolve the row's identity so the `repo`-slug-keyed children (keyed on + // `normalize_owner_key(owner)/name`, not `repos.id`) can be matched. Absent + // row → nothing to delete; commit the empty tx and report 0. + let row: Option<(String, String)> = + sqlx::query_as("SELECT owner_did, name FROM repos WHERE id = $1") + .bind(id) + .fetch_optional(&mut *tx) + .await?; + let Some((owner_did, name)) = row else { + tx.commit().await?; + return Ok(0); + }; + let slug = format!("{}/{}", normalize_owner_key(&owner_did), name); + + // Grandchildren first: PR reviews/comments key on pr_id, so delete them + // before the parent PRs vanish. + for table in ["pr_reviews", "pr_comments"] { + sqlx::query(&format!( + "DELETE FROM {table} WHERE pr_id IN (SELECT id FROM pull_requests WHERE repo_id = $1)" + )) + .bind(id) + .execute(&mut *tx) + .await?; + } + + // Direct children keyed on repos.id. + for table in [ + "push_events", + "ref_certificates", + "pull_requests", + "webhooks", + "agent_tasks", + "protected_branches", + "repo_stars", + "repo_replicas", + "repo_labels", + "visibility_rules", + "encrypted_blobs", + "repo_icaptcha_proofs", + ] { + sqlx::query(&format!("DELETE FROM {table} WHERE repo_id = $1")) + .bind(id) + .execute(&mut *tx) + .await?; + } + + // Children keyed on the derived `owner_short/name` slug rather than the id. + for table in [ + "branch_cids", + "sync_queue", + "received_ref_updates", + "arweave_anchors", + ] { + sqlx::query(&format!("DELETE FROM {table} WHERE repo = $1")) + .bind(&slug) + .execute(&mut *tx) + .await?; + } + // NOTE: `bounties` (financial: amount/wallet/tx_hash) and `issue_comments` + // (no issues table to map issue_id → repo) are deliberately NOT cascaded + // here — dropping money records or unmappable rows on a repo delete would + // be wrong. They are left intact by design. + let result = sqlx::query("DELETE FROM repos WHERE id = $1") .bind(id) - .execute(&self.pool) + .execute(&mut *tx) .await?; + + tx.commit().await?; Ok(result.rows_affected()) } } @@ -3570,6 +3637,75 @@ mod dedup_db_tests { } } + /// U2 (M7): deleting a repo must not orphan its child rows. Seeds one child in + /// a `repo_id`-keyed table (ref_certificates), one in a `repo`-slug-keyed table + /// (branch_cids), and a PR with a grandchild review (`pr_id`-keyed). Before the + /// transactional cascade these all survive `delete_repo_by_id` (RED); after, the + /// row and every child are gone (GREEN). + #[sqlx::test] + async fn delete_repo_by_id_removes_child_rows(pool: PgPool) { + let db = db(pool).await; + let owner = "did:key:z6MkChildOwnerFixtureForCascadeDelete"; + let repo = rec( + "rid-cascade", + owner, + "victim", + "d", + "2026-01-01T00:00:00Z", + "2026-01-01T00:00:00Z", + ); + db.create_repo(&repo).await.unwrap(); + let slug = format!("{}/victim", crate::db::normalize_owner_key(owner)); + + sqlx::query( + "INSERT INTO ref_certificates (id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at) + VALUES ('rc1', 'rid-cascade', 'refs/heads/main', '0', '1', 'p', 'n', 'sig', '2026-01-01T00:00:00Z')", + ).execute(db.pool()).await.unwrap(); + sqlx::query( + "INSERT INTO branch_cids (repo, ref_name, sha, cid, node_did, updated_at) + VALUES ($1, 'refs/heads/main', '1', 'cid', 'n', '2026-01-01T00:00:00Z')", + ).bind(&slug).execute(db.pool()).await.unwrap(); + sqlx::query( + "INSERT INTO pull_requests (id, repo_id, number, title, author_did, source_branch, target_branch, created_at, updated_at) + VALUES ('pr1', 'rid-cascade', 1, 't', 'a', 'b', 'main', '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')", + ).execute(db.pool()).await.unwrap(); + sqlx::query( + "INSERT INTO pr_reviews (id, pr_id, reviewer_did, status, created_at) + VALUES ('rev1', 'pr1', 'r', 'approved', '2026-01-01T00:00:00Z')", + ).execute(db.pool()).await.unwrap(); + + let removed = db.delete_repo_by_id("rid-cascade").await.unwrap(); + assert_eq!(removed, 1, "parent repo row deleted"); + + async fn count(db: &Db, sql: &str, arg: &str) -> i64 { + sqlx::query_scalar::<_, i64>(sql) + .bind(arg) + .fetch_one(db.pool()) + .await + .unwrap() + } + assert_eq!( + count(&db, "SELECT COUNT(*) FROM ref_certificates WHERE repo_id=$1", "rid-cascade").await, + 0, + "repo_id-keyed child (ref_certificates) must be deleted" + ); + assert_eq!( + count(&db, "SELECT COUNT(*) FROM branch_cids WHERE repo=$1", &slug).await, + 0, + "slug-keyed child (branch_cids) must be deleted" + ); + assert_eq!( + count(&db, "SELECT COUNT(*) FROM pull_requests WHERE repo_id=$1", "rid-cascade").await, + 0, + "pull_requests must be deleted" + ); + assert_eq!( + count(&db, "SELECT COUNT(*) FROM pr_reviews WHERE pr_id=$1", "pr1").await, + 0, + "PR grandchild (pr_reviews) must be deleted with its parent PR" + ); + } + /// The canonical `did:key:` row and the short-owner mirror row of one logical /// repo collapse to a single deduped entry: the canonical row wins and inherits /// the group's most recent `updated_at`. From 4943430f58bd7494df7ef02ee62c534ead7d1a89 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:03:49 -0500 Subject: [PATCH 10/25] fix(node): reap write_rate_limiter in the periodic cleanup loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR's new write_rate_limiter was the one limiter omitted from main()'s hand-maintained cleanup list, so its key map never shed expired entries until the 200k cap. Move the sweep into AppState::cleanup_rate_limiters (co-located with the limiter fields, single source of truth) and call it from the loop, so a newly-added limiter is reaped rather than silently dropped. Guard test seeds a reclaimable entry into write_rate_limiter and asserts the sweep reaps it — RED if write_rate_limiter is dropped from cleanup_rate_limiters (verified). --- crates/gitlawb-node/src/main.rs | 15 +++------ crates/gitlawb-node/src/rate_limit.rs | 18 +++++++++++ crates/gitlawb-node/src/state.rs | 44 +++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 88984469..4d492db9 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -437,22 +437,17 @@ async fn main() -> Result<()> { // Periodic cleanup of expired rate limit entries + consumed-proof ledger { - let rl = state.rate_limiter.clone(); - let create_ip_rl = state.create_ip_rate_limiter.clone(); - let push_rl = state.push_rate_limiter.clone(); - let sync_trigger_rl = state.sync_trigger_rate_limiter.clone(); - let peer_write_rl = state.peer_write_rate_limiter.clone(); + // Clone the whole state so the sweep uses AppState::cleanup_rate_limiters + // (the single source of truth that reaps EVERY limiter, including + // write_rate_limiter) rather than a hand-maintained list that can omit one. + let state = state.clone(); let db = state.db.clone(); let mut shutdown_rx = state.subscribe_shutdown(); tokio::spawn(async move { loop { tokio::select! { _ = tokio::time::sleep(std::time::Duration::from_secs(300)) => { - rl.cleanup().await; - create_ip_rl.cleanup().await; - push_rl.cleanup().await; - sync_trigger_rl.cleanup().await; - peer_write_rl.cleanup().await; + state.cleanup_rate_limiters().await; let now = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_secs() as i64) diff --git a/crates/gitlawb-node/src/rate_limit.rs b/crates/gitlawb-node/src/rate_limit.rs index 4c41575c..1d38a0ae 100644 --- a/crates/gitlawb-node/src/rate_limit.rs +++ b/crates/gitlawb-node/src/rate_limit.rs @@ -130,6 +130,24 @@ impl RateLimiter { !w.timestamps.is_empty() }); } + + /// Test-only: insert a fully-expired entry (no live timestamps) that the next + /// `cleanup()` must reclaim. Models a key whose window has aged out, without + /// depending on wall-clock sleeps or a short window. Used cross-module by the + /// AppState cleanup guard test (H2). + #[cfg(test)] + pub(crate) async fn insert_reclaimable_for_test(&self, key: &str) { + self.state + .lock() + .await + .insert(key.to_string(), Window { timestamps: vec![] }); + } + + /// Test-only: number of distinct keys currently tracked. + #[cfg(test)] + pub(crate) async fn tracked_keys(&self) -> usize { + self.state.lock().await.len() + } } pub async fn rate_limit_by_did(request: Request, next: Next) -> Response { diff --git a/crates/gitlawb-node/src/state.rs b/crates/gitlawb-node/src/state.rs index d38541f9..6c033392 100644 --- a/crates/gitlawb-node/src/state.rs +++ b/crates/gitlawb-node/src/state.rs @@ -127,4 +127,48 @@ impl AppState { pub fn is_shutting_down(&self) -> bool { *self.shutdown_tx.borrow() } + + /// Reclaim expired entries from EVERY rate limiter. Single source of truth for + /// the periodic cleanup loop (main.rs), co-located with the limiter fields so a + /// newly-added limiter is cleaned here rather than silently omitted from a + /// hand-maintained list in main() (H2: `write_rate_limiter` was the omitted + /// one). Every limiter field above MUST be swept here; the guard test + /// `cleanup_rate_limiters_reaps_the_write_limiter` fails if it is not. + pub(crate) async fn cleanup_rate_limiters(&self) { + self.rate_limiter.cleanup().await; + self.create_ip_rate_limiter.cleanup().await; + self.write_rate_limiter.cleanup().await; + self.push_rate_limiter.cleanup().await; + self.sync_trigger_rate_limiter.cleanup().await; + self.peer_write_rate_limiter.cleanup().await; + } +} + +#[cfg(test)] +mod tests { + /// H2 guard: the write-surface limiter must be reaped by the shared cleanup + /// routine. Seeds a reclaimable entry into `write_rate_limiter` and asserts + /// `cleanup_rate_limiters` removes it. Goes RED if `write_rate_limiter` is + /// dropped from `cleanup_rate_limiters` — the exact H2 omission, now guarded. + #[tokio::test] + async fn cleanup_rate_limiters_reaps_the_write_limiter() { + let state = crate::test_support::test_state_lazy(); + state + .write_rate_limiter + .insert_reclaimable_for_test("some-ip") + .await; + assert_eq!( + state.write_rate_limiter.tracked_keys().await, + 1, + "precondition: the reclaimable entry is tracked" + ); + + state.cleanup_rate_limiters().await; + + assert_eq!( + state.write_rate_limiter.tracked_keys().await, + 0, + "write_rate_limiter must be reaped by cleanup_rate_limiters (H2)" + ); + } } From 130516743ae5880bdf929324fdb8d9bed63c1065 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:14:15 -0500 Subject: [PATCH 11/25] fix(node): hold the per-repo advisory lock across purge recheck+delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit purge-spam took no write lock, so a concurrent receive-pack could land a ref between the emptiness recheck and the delete, silently losing a live push. Add RepoStore::try_lock_repo — a lock-only, non-blocking counterpart to acquire_write (no Tigris I/O) whose guard holds a dedicated pooled connection so the advisory lock and its unlock run on the same session. purge now locks each candidate, rechecks emptiness UNDER the lock, deletes, and releases; a repo held by a live writer is skipped, not force-deleted. RED->GREEN: a locked empty repo survives purge, then deletes once the writer releases (neuter-check confirms the lock is load-bearing). --- crates/gitlawb-node/src/admin.rs | 113 +++++++++++++++++++--- crates/gitlawb-node/src/git/repo_store.rs | 48 +++++++++ crates/gitlawb-node/src/main.rs | 9 +- 3 files changed, 156 insertions(+), 14 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 90e3162a..2d3911c1 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -194,7 +194,12 @@ where /// applies the exclusion gate, prints one dry-run row per candidate with owner + /// ref-count evidence, and — only when `execute` is true — deletes the DB row of /// each candidate one at a time. Dry-run (the default) deletes nothing. -pub async fn run_purge_spam(db: &Db, repos_dir: &Path, execute: bool) -> Result<()> { +pub async fn run_purge_spam( + db: &Db, + repo_store: &crate::git::repo_store::RepoStore, + repos_dir: &Path, + execute: bool, +) -> Result<()> { let repos_dir: PathBuf = repos_dir.to_path_buf(); let rows = db .list_repos_by_owner_did(SPAM_BURST_TARGET_DID) @@ -252,7 +257,32 @@ pub async fn run_purge_spam(db: &Db, repos_dir: &Path, execute: bool) -> Result< // Execute: delete per-repo, never a single blanket "delete all of owner X". // A per-repo failure warns and continues rather than aborting the batch. let mut deleted = 0u64; + let mut skipped_locked = 0usize; for c in &to_delete { + // Hold the per-repo advisory lock across the FINAL emptiness recheck and + // the delete, so a concurrent receive-pack cannot land a ref in the window + // between recheck and delete (M4). A repo currently locked by a live writer + // is skipped, never force-deleted out from under the push. + let guard = match repo_store.try_lock_repo(&c.owner_did, &c.name).await { + Ok(Some(g)) => g, + Ok(None) => { + warn!(repo = %c.id, "purge-spam: repo is locked by a live writer — skipped"); + skipped_locked += 1; + continue; + } + Err(e) => { + warn!(repo = %c.id, err = %e, "purge-spam: could not acquire repo lock — skipped"); + skipped_locked += 1; + continue; + } + }; + // Authoritative recheck UNDER the lock: a ref that landed before we locked + // makes the repo non-empty, so it must not be deleted. + if ref_count_on_disk(&repos_dir, &c.owner_did, &c.name) != 0 { + warn!(repo = %c.id, "purge-spam: repo no longer empty under lock — skipped (TOCTOU)"); + guard.release().await; + continue; + } match db.delete_repo_by_id(&c.id).await { Ok(0) => { warn!(repo = %c.id, "purge-spam: repo row already gone — nothing to delete"); @@ -279,9 +309,10 @@ pub async fn run_purge_spam(db: &Db, repos_dir: &Path, execute: bool) -> Result< warn!(repo = %c.id, err = %e, "purge-spam: failed to delete repo row — continuing"); } } + guard.release().await; } println!( - "purge-spam: deleted {deleted} repo row(s), skipped {} (no longer empty).", + "purge-spam: deleted {deleted} repo row(s), skipped {} (no longer empty), {skipped_locked} (locked by a live writer).", to_skip.len() ); Ok(()) @@ -463,12 +494,18 @@ mod db_tests { use chrono::Utc; use sqlx::PgPool; - async fn db(pool: PgPool) -> Db { - let db = Db::for_testing(pool); + async fn db(pool: &PgPool) -> Db { + let db = Db::for_testing(pool.clone()); db.run_migrations().await.unwrap(); db } + /// Lock-only RepoStore over a test pool + repos_dir (no Tigris), for the purge + /// callers that now take the advisory lock (M4). + fn test_store(repos_dir: &Path, pool: &PgPool) -> crate::git::repo_store::RepoStore { + crate::git::repo_store::RepoStore::for_testing(repos_dir.to_path_buf(), pool.clone()) + } + fn rec(id: &str, owner: &str, name: &str) -> RepoRecord { RepoRecord { id: id.to_string(), @@ -498,7 +535,7 @@ mod db_tests { // which is fine here: the assertion is that dry-run mutates no rows regardless. #[sqlx::test] async fn dry_run_deletes_nothing(pool: PgPool) { - let db = db(pool).await; + let db = db(&pool).await; for r in [ rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"), rec("t-nonempty", SPAM_BURST_TARGET_DID, "real"), @@ -513,7 +550,8 @@ mod db_tests { // Empty repos dir: no repo exists on disk, so nothing is provably empty. let tmp = tempfile::TempDir::new().unwrap(); - run_purge_spam(&db, tmp.path(), false).await.unwrap(); + let store = test_store(tmp.path(), &pool); + run_purge_spam(&db, &store, tmp.path(), false).await.unwrap(); let after = count_rows(&db).await; assert_eq!(after, before, "dry-run must not delete any repo rows"); @@ -524,7 +562,7 @@ mod db_tests { // repo. This exercises the DB wiring end-to-end with a real empty repo on disk. #[sqlx::test] async fn execute_deletes_only_the_empty_target_repo_on_disk(pool: PgPool) { - let db = db(pool).await; + let db = db(&pool).await; let tmp = tempfile::TempDir::new().unwrap(); // One empty target repo (real bare repo, zero refs) and one target repo @@ -548,7 +586,8 @@ mod db_tests { assert_eq!(store::list_refs(&empty_path).unwrap().len(), 0); assert!(!store::list_refs(&refs_path).unwrap().is_empty()); - run_purge_spam(&db, tmp.path(), true).await.unwrap(); + let repo_store = test_store(tmp.path(), &pool); + run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); // Only the empty target repo row is gone. assert!(db @@ -572,7 +611,7 @@ mod db_tests { // consistent (no orphaned empty git dir left behind). #[sqlx::test] async fn execute_removes_the_on_disk_repo_dir(pool: PgPool) { - let db = db(pool).await; + let db = db(&pool).await; let tmp = tempfile::TempDir::new().unwrap(); let empty = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); db.create_repo(&empty).await.unwrap(); @@ -580,7 +619,8 @@ mod db_tests { store::init_bare(&path).unwrap(); assert!(path.exists(), "precondition: on-disk repo exists"); - run_purge_spam(&db, tmp.path(), true).await.unwrap(); + let repo_store = test_store(tmp.path(), &pool); + run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); assert!( db.get_repo(SPAM_BURST_TARGET_DID, "spam1") @@ -592,6 +632,52 @@ mod db_tests { assert!(!path.exists(), "on-disk bare repo dir must be removed too"); } + // U4 (M4): a repo whose per-repo advisory lock is held by a live writer must be + // SKIPPED by purge, not deleted out from under the push. Holds the lock via the + // same RepoStore (a separate pooled connection), runs execute, and asserts the + // row + on-disk dir survive; once the writer releases, purge deletes it. + #[sqlx::test] + async fn locked_repo_is_skipped_not_deleted(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let empty = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&empty).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &empty.owner_did, &empty.name); + store::init_bare(&path).unwrap(); + + let repo_store = test_store(tmp.path(), &pool); + // A live writer holds the per-repo advisory lock. + let held = repo_store + .try_lock_repo(&empty.owner_did, &empty.name) + .await + .unwrap() + .expect("lock should be free initially"); + + run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + + // Locked → skipped: both the row and the on-disk dir survive. + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_some(), + "a repo locked by a live writer must NOT be deleted" + ); + assert!(path.exists(), "on-disk dir must survive while locked"); + + // Once the writer releases, the empty repo is deleted (the lock was the + // only thing protecting it — baseline both ways). + held.release().await; + run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none(), + "once unlocked, the empty repo is deleted" + ); + } + // U1 (M3): a burst-owned row whose NAME traverses out of repos_dir must never // cause a delete OUTSIDE repos_dir. Adversarial must-not: a real empty bare repo // planted as a "victim" beside repos_dir is reachable from repos_dir// via @@ -600,7 +686,7 @@ mod db_tests { // delete the victim. The name validator must reject it so the victim survives. #[sqlx::test] async fn traversal_name_cannot_delete_a_repo_outside_repos_dir(pool: PgPool) { - let db = db(pool).await; + let db = db(&pool).await; let root = tempfile::TempDir::new().unwrap(); let repos_dir = root.path().join("repos"); std::fs::create_dir_all(&repos_dir).unwrap(); @@ -625,7 +711,8 @@ mod db_tests { ); db.create_repo(&evil).await.unwrap(); - run_purge_spam(&db, &repos_dir, true).await.unwrap(); + let repo_store = test_store(&repos_dir, &pool); + run_purge_spam(&db, &repo_store, &repos_dir, true).await.unwrap(); assert!( victim.join("HEAD").is_file(), @@ -638,7 +725,7 @@ mod db_tests { // target DID — the SQL side of the normalization fix. #[sqlx::test] async fn list_repos_by_owner_did_matches_short_form(pool: PgPool) { - let db = db(pool).await; + let db = db(&pool).await; let short = crate::db::normalize_owner_key(SPAM_BURST_TARGET_DID); assert_ne!(short, SPAM_BURST_TARGET_DID, "fixture must be short form"); let repo = rec("short-owned", short, "spam"); diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index c7aa954a..669d8d51 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -207,6 +207,35 @@ impl RepoStore { }) } + /// Try to acquire ONLY the per-repo advisory lock, non-blocking, with no + /// Tigris I/O — the lightweight counterpart to [`acquire_write`](Self::acquire_write) + /// for out-of-band admin ops (purge-spam) that must mutually exclude a live + /// push during a destructive delete but never download/re-upload the repo. + /// + /// Returns `Some(guard)` if the lock was free, `None` if another writer holds + /// it (so the caller can skip rather than block). The guard holds a dedicated + /// pooled connection for its whole lifetime, so the lock lives on that one + /// connection and `release()` unlocks it on the SAME connection — a plain + /// pool query could unlock on a different connection and silently fail. + pub async fn try_lock_repo( + &self, + owner_did: &str, + repo_name: &str, + ) -> Result> { + let (owner_slug, _local) = self.local_path(owner_did, repo_name)?; + let lock_key = advisory_lock_key(&owner_slug, repo_name); + let mut conn = self.pool.acquire().await.context("acquiring lock connection")?; + let row: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") + .bind(lock_key) + .fetch_one(&mut *conn) + .await + .context("try advisory lock")?; + if !row.0 { + return Ok(None); + } + Ok(Some(RepoLockGuard { conn, lock_key })) + } + /// Initialize a new bare repo on local disk and upload to Tigris. pub async fn init(&self, owner_did: &str, repo_name: &str) -> Result { let (owner_slug, local_path) = self.local_path(owner_did, repo_name)?; @@ -392,6 +421,25 @@ impl RepoWriteGuard { } } +/// Lock-only guard from [`RepoStore::try_lock_repo`]. Holds the Postgres advisory +/// lock (and the dedicated connection it lives on) until `release()`. No Tigris +/// I/O — unlike [`RepoWriteGuard`], releasing does NOT upload anything. +pub struct RepoLockGuard { + conn: sqlx::pool::PoolConnection, + lock_key: i64, +} + +impl RepoLockGuard { + /// Release the advisory lock on the same connection that took it, then return + /// the connection to the pool. + pub async fn release(mut self) { + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(self.lock_key) + .execute(&mut *self.conn) + .await; + } +} + /// Compute a stable i64 hash for a Postgres advisory lock key. fn advisory_lock_key(owner_slug: &str, repo_name: &str) -> i64 { use std::hash::{Hash, Hasher}; diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 4d492db9..ff53a927 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -601,7 +601,14 @@ async fn run_admin_command(command: config::Command, config: &Config) -> Result< ) .await .context("connecting to database for purge-spam")?; - admin::run_purge_spam(&db, &config.repos_dir, execute).await + // Lock-only RepoStore (no Tigris): purge takes the per-repo advisory + // lock to exclude a live push, but never downloads/uploads archives. + let repo_store = git::repo_store::RepoStore::new( + config.repos_dir.clone(), + None, + db.pool().clone(), + ); + admin::run_purge_spam(&db, &repo_store, &config.repos_dir, execute).await } } } From aab70ec25d542e050271b238229badd5b7fb1b29 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:20:09 -0500 Subject: [PATCH 12/25] fix(node): count purge-spam disk-removal failures separately from deletes The Ok(n) arm incremented the deleted total before remove_dir_all, whose failure only warn!'d, so the summary reported N deleted even when the on-disk dir survived (DB/disk drift shown as clean success). run_purge_spam now returns a PurgeSummary and counts a failed (or containment-refused) on-disk removal in disk_failed, never folding it into deleted; the summary line surfaces 'M on-disk removal(s) failed'. RED->GREEN: a read-only parent dir forces the removal to fail; deleted=1 AND disk_failed=1 with the dir surviving (neuter-check confirms the counter is load-bearing). --- crates/gitlawb-node/src/admin.rs | 92 +++++++++++++++++++++++++++----- crates/gitlawb-node/src/main.rs | 4 +- 2 files changed, 83 insertions(+), 13 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 2d3911c1..2ab52529 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -29,6 +29,25 @@ pub const EXCLUDED_DIDS: &[&str] = &[ "did:key:z6MkqRzACJ5iCDdkiymAPK3gq18z2iecZHeAuUyW6JnwRfoM", ]; +/// Outcome tally of a `run_purge_spam` execute pass. Returned so the DB-delete +/// count is never conflated with full success: `disk_failed` records repos whose +/// row was deleted but whose on-disk dir could not be removed (or escaped the +/// repos_dir containment check), so an operator sees the DB/disk drift instead of +/// a clean "N deleted" summary. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct PurgeSummary { + /// Repo rows actually deleted from the DB. + pub deleted: u64, + /// Candidates skipped because they were no longer empty (pre-filter or the + /// authoritative recheck under the lock). + pub skipped_not_empty: usize, + /// Candidates skipped because a live writer held the per-repo lock. + pub skipped_locked: usize, + /// Rows deleted whose on-disk dir removal FAILED (or was refused by the + /// containment guard) — DB/disk drift the operator must reconcile. + pub disk_failed: usize, +} + /// A repo selected for purge, with the evidence that qualified it. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Candidate { @@ -199,7 +218,7 @@ pub async fn run_purge_spam( repo_store: &crate::git::repo_store::RepoStore, repos_dir: &Path, execute: bool, -) -> Result<()> { +) -> Result { let repos_dir: PathBuf = repos_dir.to_path_buf(); let rows = db .list_repos_by_owner_did(SPAM_BURST_TARGET_DID) @@ -220,7 +239,7 @@ pub async fn run_purge_spam( if candidates.is_empty() { println!("purge-spam: no empty spam-burst repos found for {SPAM_BURST_TARGET_DID}"); - return Ok(()); + return Ok(PurgeSummary::default()); } println!( @@ -241,7 +260,7 @@ pub async fn run_purge_spam( "purge-spam: dry-run — nothing deleted. Re-run with --execute to delete the {} candidate(s).", candidates.len() ); - return Ok(()); + return Ok(PurgeSummary::default()); } // Re-verify emptiness immediately before deleting: a push may have landed @@ -256,8 +275,10 @@ pub async fn run_purge_spam( // Execute: delete per-repo, never a single blanket "delete all of owner X". // A per-repo failure warns and continues rather than aborting the batch. - let mut deleted = 0u64; - let mut skipped_locked = 0usize; + let mut summary = PurgeSummary { + skipped_not_empty: to_skip.len(), + ..PurgeSummary::default() + }; for c in &to_delete { // Hold the per-repo advisory lock across the FINAL emptiness recheck and // the delete, so a concurrent receive-pack cannot land a ref in the window @@ -267,12 +288,12 @@ pub async fn run_purge_spam( Ok(Some(g)) => g, Ok(None) => { warn!(repo = %c.id, "purge-spam: repo is locked by a live writer — skipped"); - skipped_locked += 1; + summary.skipped_locked += 1; continue; } Err(e) => { warn!(repo = %c.id, err = %e, "purge-spam: could not acquire repo lock — skipped"); - skipped_locked += 1; + summary.skipped_locked += 1; continue; } }; @@ -280,6 +301,7 @@ pub async fn run_purge_spam( // makes the repo non-empty, so it must not be deleted. if ref_count_on_disk(&repos_dir, &c.owner_did, &c.name) != 0 { warn!(repo = %c.id, "purge-spam: repo no longer empty under lock — skipped (TOCTOU)"); + summary.skipped_not_empty += 1; guard.release().await; continue; } @@ -288,22 +310,28 @@ pub async fn run_purge_spam( warn!(repo = %c.id, "purge-spam: repo row already gone — nothing to delete"); } Ok(n) => { - deleted += n; + summary.deleted += n; // Remove the now-orphaned on-disk bare repo (empty, so cheap) so // the DB row and disk stay consistent. Belt-and-suspenders: assert // the resolved path is canonically INSIDE repos_dir before any // remove_dir_all, so a symlinked slug dir or any residual traversal // can never delete outside the repo root (the name is already // validated at selection; this guards the destructive op itself). + // A disk-removal failure is counted separately, NOT folded into the + // deleted total, so the summary never reports a clean success while + // an on-disk dir survives (DB/disk drift the operator must fix). let path = store::repo_disk_path(&repos_dir, &c.owner_did, &c.name); if !path_within(&path, &repos_dir) { warn!(repo = %c.id, path = %path.display(), "purge-spam: on-disk path escapes repos_dir — refusing to remove"); + summary.disk_failed += 1; } else if let Err(e) = std::fs::remove_dir_all(&path) { warn!(repo = %c.id, path = %path.display(), err = %e, "purge-spam: deleted DB row but could not remove on-disk repo dir"); + summary.disk_failed += 1; + } else { + info!(repo = %c.id, rows = n, "purge-spam: deleted repo row + on-disk dir"); } - info!(repo = %c.id, rows = n, "purge-spam: deleted repo row + on-disk dir"); } Err(e) => { warn!(repo = %c.id, err = %e, "purge-spam: failed to delete repo row — continuing"); @@ -312,10 +340,10 @@ pub async fn run_purge_spam( guard.release().await; } println!( - "purge-spam: deleted {deleted} repo row(s), skipped {} (no longer empty), {skipped_locked} (locked by a live writer).", - to_skip.len() + "purge-spam: deleted {} repo row(s); skipped {} (no longer empty), {} (locked by a live writer); {} on-disk removal(s) failed.", + summary.deleted, summary.skipped_not_empty, summary.skipped_locked, summary.disk_failed ); - Ok(()) + Ok(summary) } #[cfg(test)] @@ -678,6 +706,46 @@ mod db_tests { ); } + // U5 (M6): a repo whose DB row is deleted but whose on-disk removal FAILS must + // be counted in `disk_failed`, never folded into a clean "deleted" success — + // else the summary reports success while the on-disk dir survives (DB/disk + // drift). Forces the failure by making the parent (slug) dir read-only. + #[sqlx::test] + async fn disk_removal_failure_is_counted_not_reported_as_success(pool: PgPool) { + use std::os::unix::fs::PermissionsExt; + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let empty = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&empty).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &empty.owner_did, &empty.name); + store::init_bare(&path).unwrap(); + + // Read-only parent (slug) dir: remove_dir_all cannot unlink the repo dir. + let slug_dir = path.parent().unwrap().to_path_buf(); + let orig = std::fs::metadata(&slug_dir).unwrap().permissions(); + std::fs::set_permissions(&slug_dir, std::fs::Permissions::from_mode(0o555)).unwrap(); + + let repo_store = test_store(tmp.path(), &pool); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + + // Restore perms so TempDir cleanup works regardless of assertion outcome. + std::fs::set_permissions(&slug_dir, orig).unwrap(); + + assert_eq!(summary.deleted, 1, "the DB row was deleted"); + assert_eq!( + summary.disk_failed, 1, + "a failed on-disk removal must be counted, not reported as clean success" + ); + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none(), + "DB row is gone (delete succeeded)" + ); + assert!(path.exists(), "on-disk dir survived the failed removal (drift)"); + } + // U1 (M3): a burst-owned row whose NAME traverses out of repos_dir must never // cause a delete OUTSIDE repos_dir. Adversarial must-not: a real empty bare repo // planted as a "victim" beside repos_dir is reachable from repos_dir// via diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index ff53a927..90281611 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -608,7 +608,9 @@ async fn run_admin_command(command: config::Command, config: &Config) -> Result< None, db.pool().clone(), ); - admin::run_purge_spam(&db, &repo_store, &config.repos_dir, execute).await + admin::run_purge_spam(&db, &repo_store, &config.repos_dir, execute) + .await + .map(|_| ()) } } } From cecca67f20c89b71f458cde602b7157fac0b6aa0 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:22:32 -0500 Subject: [PATCH 13/25] fix(node): warn on unparseable rate-limit env vars instead of silently defaulting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GITLAWB_CREATE/WRITE/PUSH_RATE_LIMIT parsed with .ok().and_then(parse).unwrap_or, so a typo (e.g. 5O with a letter O) silently reverted to the default and left the operator believing a stricter cap was in force. Route all three through rate_limit_from_env, which distinguishes absent/empty (default, silent) from present-but-unparseable (default, WARN). Pure resolve_rate_limit is unit-tested across absent/empty/valid/zero/unparseable — 0 stays valid (its own ==0 warn handles the disable case). --- crates/gitlawb-node/src/main.rs | 80 ++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 12 deletions(-) diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 90281611..e8824ca2 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -301,10 +301,7 @@ async fn main() -> Result<()> { // capped regardless of how many identities it mints. Sized well above any // legitimate per-IP creation rate; GITLAWB_CREATE_RATE_LIMIT overrides, 0 // disables. Bounded key set — the key is a client-influenced IP. - let create_limit = std::env::var("GITLAWB_CREATE_RATE_LIMIT") - .ok() - .and_then(|v| v.trim().parse::().ok()) - .unwrap_or(120); + let create_limit = rate_limit_from_env("GITLAWB_CREATE_RATE_LIMIT", 120); let create_ip_rate_limiter = rate_limit::RateLimiter::new_bounded( create_limit, std::time::Duration::from_secs(3600), @@ -322,10 +319,7 @@ async fn main() -> Result<()> { // any legitimate per-IP write rate — real agent automation makes many small // writes per hour. GITLAWB_WRITE_RATE_LIMIT overrides; 0 disables. Bounded // key set — the key is a client-influenced IP. - let write_limit = std::env::var("GITLAWB_WRITE_RATE_LIMIT") - .ok() - .and_then(|v| v.trim().parse::().ok()) - .unwrap_or(600); + let write_limit = rate_limit_from_env("GITLAWB_WRITE_RATE_LIMIT", 600); let write_rate_limiter = rate_limit::RateLimiter::new_bounded( write_limit, std::time::Duration::from_secs(3600), @@ -340,10 +334,7 @@ async fn main() -> Result<()> { // for heavy agent automation while still stopping flood traffic (the June // 2026 attack pushed several times per second per IP). GITLAWB_PUSH_RATE_LIMIT // overrides; 0 disables. Bounded key set — the key is a client-influenced IP. - let push_limit = std::env::var("GITLAWB_PUSH_RATE_LIMIT") - .ok() - .and_then(|v| v.trim().parse::().ok()) - .unwrap_or(600); + let push_limit = rate_limit_from_env("GITLAWB_PUSH_RATE_LIMIT", 600); let push_rate_limiter = rate_limit::RateLimiter::new_bounded( push_limit, std::time::Duration::from_secs(3600), @@ -588,6 +579,37 @@ async fn main() -> Result<()> { Ok(()) } +/// Pure resolver for a usize rate-limit env value. Returns `(resolved, invalid)`: +/// an absent or empty value resolves to `default` and is NOT invalid; a non-empty +/// value that fails to parse resolves to `default` and IS invalid (so the caller +/// warns rather than silently disabling the intended, usually stricter, brake). +fn resolve_rate_limit(raw: Option<&str>, default: usize) -> (usize, bool) { + match raw.map(str::trim) { + None | Some("") => (default, false), + Some(t) => match t.parse::() { + Ok(n) => (n, false), + Err(_) => (default, true), + }, + } +} + +/// Read a rate-limit env var, warning on a present-but-unparseable value. A typo +/// like `GITLAWB_WRITE_RATE_LIMIT=5O` must not silently revert to the default and +/// leave the operator believing a stricter cap is in force. +fn rate_limit_from_env(var: &str, default: usize) -> usize { + let raw = std::env::var(var).ok(); + let (value, invalid) = resolve_rate_limit(raw.as_deref(), default); + if invalid { + tracing::warn!( + var, + value = raw.as_deref().unwrap_or(""), + default, + "invalid rate-limit value — using default; the intended brake is NOT applied" + ); + } + value +} + /// Dispatch an admin subcommand. Connects the database directly (no server, no /// p2p, no metrics) and runs the requested tool to completion, then exits. async fn run_admin_command(command: config::Command, config: &Config) -> Result<()> { @@ -1075,6 +1097,40 @@ fn load_or_create_keypair(config: &Config) -> Result { } } +#[cfg(test)] +mod rate_limit_env_tests { + use super::resolve_rate_limit; + + #[test] + fn absent_uses_default_without_warning() { + assert_eq!(resolve_rate_limit(None, 600), (600, false)); + } + + #[test] + fn empty_or_whitespace_uses_default_without_warning() { + assert_eq!(resolve_rate_limit(Some(""), 600), (600, false)); + assert_eq!(resolve_rate_limit(Some(" "), 600), (600, false)); + } + + #[test] + fn valid_value_parses() { + assert_eq!(resolve_rate_limit(Some("50"), 600), (50, false)); + assert_eq!(resolve_rate_limit(Some(" 50 "), 600), (50, false)); + // 0 is a VALID value (disables the brake); the ==0 warning is handled + // separately at the call site. It must NOT be flagged invalid here. + assert_eq!(resolve_rate_limit(Some("0"), 600), (0, false)); + } + + #[test] + fn present_but_unparseable_defaults_and_flags_invalid() { + // The L8 case: a fat-fingered stricter cap ("5O", letter O) must default + // AND be flagged so the caller warns, not silently disable the brake. + assert_eq!(resolve_rate_limit(Some("5O"), 600), (600, true)); + assert_eq!(resolve_rate_limit(Some("abc"), 600), (600, true)); + assert_eq!(resolve_rate_limit(Some("-1"), 600), (600, true)); + } +} + #[cfg(test)] mod gossip_ssrf_tests { use super::ping_peer_health; From f4d5bec549262ba89f662cb5329ab4bd6d7ed03e Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:23:31 -0500 Subject: [PATCH 14/25] docs(node): document GITLAWB_IPFS_API/TIGRIS_BUCKET/METRICS_ADDR/SHUTDOWN_GRACE_SECS in .env.example Four config knobs read in config.rs were absent from .env.example, so operators had no template for them. Add each in its matching section with the config default (INV-24: operator docs match the config the code reads, in the same PR that touches the file). --- .env.example | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.env.example b/.env.example index 5987a527..e4593025 100644 --- a/.env.example +++ b/.env.example @@ -13,9 +13,19 @@ GITLAWB_PUBLIC_URL=https://your-node.example.com # ── Server ──────────────────────────────────────────────────────────────── GITLAWB_HOST=0.0.0.0 GITLAWB_PORT=7545 +# Optional address to bind a Prometheus /metrics exposition endpoint on (e.g. +# 127.0.0.1:9091). Leave empty (default) to disable. Bind to localhost or a +# private interface — the metrics endpoint is unauthenticated. +GITLAWB_METRICS_ADDR= +# Max seconds to wait for in-flight requests to drain on shutdown before the +# server returns 503 to anything still in flight and exits. Default: 30. +GITLAWB_SHUTDOWN_GRACE_SECS=30 # ── Storage ─────────────────────────────────────────────────────────────── GITLAWB_REPOS_DIR=/data/repos +# Tigris (S3-compatible) bucket for repo storage. Leave empty (default) to +# disable Tigris and use local-only storage. +GITLAWB_TIGRIS_BUCKET= # PostgreSQL connection URL. Required. # When using the bundled docker-compose, this is wired automatically. @@ -39,6 +49,9 @@ GITLAWB_DB_RETRY_INITIAL_SECS=5 GITLAWB_DB_RETRY_MAX_SECS=60 # ── IPFS pinning (Pinata) ───────────────────────────────────────────────── +# URL of a local IPFS/Kubo node HTTP API (e.g. http://127.0.0.1:5001). Leave +# empty (default) to disable local IPFS. +GITLAWB_IPFS_API= # Get a JWT at https://app.pinata.cloud/developers/api-keys GITLAWB_PINATA_JWT= GITLAWB_PINATA_UPLOAD_URL=https://uploads.pinata.cloud/v3/files From 6da0cf5dcfcde4eb3a06e77bd37f312721f4f289 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:26:29 -0500 Subject: [PATCH 15/25] test(node): exercise the dry-run guard with a real on-disk candidate present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dry_run_deletes_nothing ran against an empty repos_dir, so select_spam_candidates found nothing and run_purge_spam hit the no-candidate early return — the if !execute { return } guard was never exercised with candidates present (a vacuous guard, INV-21). Materialize a real empty bare repo so a genuine candidate exists, then assert dry-run leaves both the row and the on-disk dir. Verified load-bearing: removing the !execute guard turns the test RED (dry-run deletes). --- crates/gitlawb-node/src/admin.rs | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 2ab52529..005645ca 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -576,13 +576,39 @@ mod db_tests { let before = count_rows(&db).await; assert_eq!(before, 5); - // Empty repos dir: no repo exists on disk, so nothing is provably empty. + // Materialize a REAL empty bare repo for the empty target so it is a genuine + // purge candidate. Without an on-disk candidate, run_purge_spam hits the + // no-candidate early return and the `if !execute { return }` guard is never + // exercised with candidates present — the L10 gap this test now closes. let tmp = tempfile::TempDir::new().unwrap(); + let target_dir = + store::repo_disk_path(tmp.path(), SPAM_BURST_TARGET_DID, "spam1"); + store::init_bare(&target_dir).unwrap(); + assert_eq!( + store::list_refs(&target_dir).unwrap().len(), + 0, + "precondition: the candidate is a real empty bare repo" + ); + let store = test_store(tmp.path(), &pool); - run_purge_spam(&db, &store, tmp.path(), false).await.unwrap(); + let summary = run_purge_spam(&db, &store, tmp.path(), false).await.unwrap(); + // A candidate existed, but dry-run deletes nothing — DB row and on-disk dir + // both survive. RED if the `if !execute` guard is removed. + assert_eq!(summary.deleted, 0, "dry-run must delete no rows"); let after = count_rows(&db).await; assert_eq!(after, before, "dry-run must not delete any repo rows"); + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_some(), + "the candidate row must survive a dry-run" + ); + assert!( + target_dir.exists(), + "dry-run must not remove the on-disk repo dir" + ); } // The DB accessor lists exactly the target DID's rows (exact owner match), and From ffdc1672b592f7c7892794175e646c4700474bd5 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:27:00 -0500 Subject: [PATCH 16/25] style(node): cargo fmt for the #196 fix set --- crates/gitlawb-node/src/admin.rs | 41 +++++++++++++++++------ crates/gitlawb-node/src/db/mod.rs | 25 +++++++++++--- crates/gitlawb-node/src/git/repo_store.rs | 6 +++- crates/gitlawb-node/src/main.rs | 7 ++-- 4 files changed, 58 insertions(+), 21 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 005645ca..a03e93ed 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -581,8 +581,7 @@ mod db_tests { // no-candidate early return and the `if !execute { return }` guard is never // exercised with candidates present — the L10 gap this test now closes. let tmp = tempfile::TempDir::new().unwrap(); - let target_dir = - store::repo_disk_path(tmp.path(), SPAM_BURST_TARGET_DID, "spam1"); + let target_dir = store::repo_disk_path(tmp.path(), SPAM_BURST_TARGET_DID, "spam1"); store::init_bare(&target_dir).unwrap(); assert_eq!( store::list_refs(&target_dir).unwrap().len(), @@ -591,7 +590,9 @@ mod db_tests { ); let store = test_store(tmp.path(), &pool); - let summary = run_purge_spam(&db, &store, tmp.path(), false).await.unwrap(); + let summary = run_purge_spam(&db, &store, tmp.path(), false) + .await + .unwrap(); // A candidate existed, but dry-run deletes nothing — DB row and on-disk dir // both survive. RED if the `if !execute` guard is removed. @@ -641,7 +642,9 @@ mod db_tests { assert!(!store::list_refs(&refs_path).unwrap().is_empty()); let repo_store = test_store(tmp.path(), &pool); - run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); // Only the empty target repo row is gone. assert!(db @@ -674,7 +677,9 @@ mod db_tests { assert!(path.exists(), "precondition: on-disk repo exists"); let repo_store = test_store(tmp.path(), &pool); - run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); assert!( db.get_repo(SPAM_BURST_TARGET_DID, "spam1") @@ -707,7 +712,9 @@ mod db_tests { .unwrap() .expect("lock should be free initially"); - run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); // Locked → skipped: both the row and the on-disk dir survive. assert!( @@ -722,7 +729,9 @@ mod db_tests { // Once the writer releases, the empty repo is deleted (the lock was the // only thing protecting it — baseline both ways). held.release().await; - run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); assert!( db.get_repo(SPAM_BURST_TARGET_DID, "spam1") .await @@ -752,7 +761,9 @@ mod db_tests { std::fs::set_permissions(&slug_dir, std::fs::Permissions::from_mode(0o555)).unwrap(); let repo_store = test_store(tmp.path(), &pool); - let summary = run_purge_spam(&db, &repo_store, tmp.path(), true).await.unwrap(); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); // Restore perms so TempDir cleanup works regardless of assertion outcome. std::fs::set_permissions(&slug_dir, orig).unwrap(); @@ -769,7 +780,10 @@ mod db_tests { .is_none(), "DB row is gone (delete succeeded)" ); - assert!(path.exists(), "on-disk dir survived the failed removal (drift)"); + assert!( + path.exists(), + "on-disk dir survived the failed removal (drift)" + ); } // U1 (M3): a burst-owned row whose NAME traverses out of repos_dir must never @@ -806,7 +820,9 @@ mod db_tests { db.create_repo(&evil).await.unwrap(); let repo_store = test_store(&repos_dir, &pool); - run_purge_spam(&db, &repo_store, &repos_dir, true).await.unwrap(); + run_purge_spam(&db, &repo_store, &repos_dir, true) + .await + .unwrap(); assert!( victim.join("HEAD").is_file(), @@ -922,7 +938,10 @@ mod db_tests { // A genuine path inside repos_dir passes. let inside = repos_dir.join("real.git"); std::fs::create_dir_all(&inside).unwrap(); - assert!(path_within(&inside, &repos_dir), "an in-root path must pass"); + assert!( + path_within(&inside, &repos_dir), + "an in-root path must pass" + ); } /// The exclusion gate and the target scope must never overlap: if the burst diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 5b2fb79e..a963f544 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -3664,7 +3664,11 @@ mod dedup_db_tests { sqlx::query( "INSERT INTO branch_cids (repo, ref_name, sha, cid, node_did, updated_at) VALUES ($1, 'refs/heads/main', '1', 'cid', 'n', '2026-01-01T00:00:00Z')", - ).bind(&slug).execute(db.pool()).await.unwrap(); + ) + .bind(&slug) + .execute(db.pool()) + .await + .unwrap(); sqlx::query( "INSERT INTO pull_requests (id, repo_id, number, title, author_did, source_branch, target_branch, created_at, updated_at) VALUES ('pr1', 'rid-cascade', 1, 't', 'a', 'b', 'main', '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')", @@ -3672,7 +3676,10 @@ mod dedup_db_tests { sqlx::query( "INSERT INTO pr_reviews (id, pr_id, reviewer_did, status, created_at) VALUES ('rev1', 'pr1', 'r', 'approved', '2026-01-01T00:00:00Z')", - ).execute(db.pool()).await.unwrap(); + ) + .execute(db.pool()) + .await + .unwrap(); let removed = db.delete_repo_by_id("rid-cascade").await.unwrap(); assert_eq!(removed, 1, "parent repo row deleted"); @@ -3685,7 +3692,12 @@ mod dedup_db_tests { .unwrap() } assert_eq!( - count(&db, "SELECT COUNT(*) FROM ref_certificates WHERE repo_id=$1", "rid-cascade").await, + count( + &db, + "SELECT COUNT(*) FROM ref_certificates WHERE repo_id=$1", + "rid-cascade" + ) + .await, 0, "repo_id-keyed child (ref_certificates) must be deleted" ); @@ -3695,7 +3707,12 @@ mod dedup_db_tests { "slug-keyed child (branch_cids) must be deleted" ); assert_eq!( - count(&db, "SELECT COUNT(*) FROM pull_requests WHERE repo_id=$1", "rid-cascade").await, + count( + &db, + "SELECT COUNT(*) FROM pull_requests WHERE repo_id=$1", + "rid-cascade" + ) + .await, 0, "pull_requests must be deleted" ); diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 669d8d51..5ab8f270 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -224,7 +224,11 @@ impl RepoStore { ) -> Result> { let (owner_slug, _local) = self.local_path(owner_did, repo_name)?; let lock_key = advisory_lock_key(&owner_slug, repo_name); - let mut conn = self.pool.acquire().await.context("acquiring lock connection")?; + let mut conn = self + .pool + .acquire() + .await + .context("acquiring lock connection")?; let row: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") .bind(lock_key) .fetch_one(&mut *conn) diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index e8824ca2..2fe8164f 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -625,11 +625,8 @@ async fn run_admin_command(command: config::Command, config: &Config) -> Result< .context("connecting to database for purge-spam")?; // Lock-only RepoStore (no Tigris): purge takes the per-repo advisory // lock to exclude a live push, but never downloads/uploads archives. - let repo_store = git::repo_store::RepoStore::new( - config.repos_dir.clone(), - None, - db.pool().clone(), - ); + let repo_store = + git::repo_store::RepoStore::new(config.repos_dir.clone(), None, db.pool().clone()); admin::run_purge_spam(&db, &repo_store, &config.repos_dir, execute) .await .map(|_| ()) From 010c625f936d25dfd0d5ef0b74cdbb1482dbba09 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 15 Jul 2026 16:43:59 -0500 Subject: [PATCH 17/25] fix(node): pin the writer's advisory-lock connection in RepoWriteGuard acquire_write took the session-scoped advisory lock through the shared pool and returned the lock-owning connection to the pool, so release unlocked on a different connection (silent no-op) and a concurrent try_lock_repo could be handed the idle lock-owning connection and reentrantly re-grab the lock, defeating the purge tool's mutual exclusion. Pin a dedicated PoolConnection for the guard's lifetime and unlock on it, mirroring RepoLockGuard. Load-bearing contention test (single-connection pool to force the reentrant grab deterministic): RED (Ok(Some)) before, GREEN after. --- crates/gitlawb-node/src/git/repo_store.rs | 89 +++++++++++++++++++++-- 1 file changed, 83 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 5ab8f270..ed691960 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -157,13 +157,26 @@ 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 connection for the guard's whole lifetime so the + // session-scoped advisory lock lives on ONE connection and `release` + // unlocks it on that SAME connection. A plain `&self.pool` query returns + // its connection to the pool between calls, which (a) leaves the lock on + // an idle connection `release`'s unlock never reaches, and (b) lets + // `try_lock_repo`'s `pool.acquire()` be handed that idle connection and + // reentrantly re-grab the lock. Mirrors `RepoLockGuard`. + let mut conn = self + .pool + .acquire() + .await + .context("acquiring write-lock connection")?; + // Acquire Postgres advisory lock with retry using pg_try_advisory_lock // to avoid blocking indefinitely on stale locks from crashed connections. let mut acquired = false; for attempt in 0..60 { let row: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") .bind(lock_key) - .fetch_one(&self.pool) + .fetch_one(&mut *conn) .await .context("trying advisory lock")?; if row.0 { @@ -202,7 +215,7 @@ impl RepoStore { repo_name: repo_name.to_string(), local_path, lock_key, - pool: self.pool.clone(), + conn, tigris: self.tigris.clone(), }) } @@ -387,7 +400,11 @@ pub struct RepoWriteGuard { repo_name: String, pub local_path: PathBuf, lock_key: i64, - pool: PgPool, + /// Dedicated pooled connection the advisory lock lives on. Held for the + /// guard's lifetime so `release` unlocks on the SAME session that locked, + /// and so a concurrent `try_lock_repo` cannot be handed this connection and + /// reentrantly re-grab the lock. + conn: sqlx::pool::PoolConnection, tigris: Option, } @@ -402,7 +419,7 @@ impl RepoWriteGuard { /// 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(self, success: bool) { + pub async fn release(mut self, success: bool) { // Upload to Tigris only on success. if success { if let Some(ref tigris) = self.tigris { @@ -417,10 +434,11 @@ impl RepoWriteGuard { warn!(repo = %self.repo_name, "write failed — skipping tigris upload to avoid propagating an inconsistent repo"); } - // Release advisory lock + // Release the advisory lock on the SAME connection that took it, then + // the connection returns to the pool on drop. let _ = sqlx::query("SELECT pg_advisory_unlock($1)") .bind(self.lock_key) - .execute(&self.pool) + .execute(&mut *self.conn) .await; } } @@ -614,4 +632,63 @@ mod tests { ); } } + + // ── advisory-lock mutual exclusion (P1a) ──────────────────────────────── + + // A live `acquire_write` guard must exclude a concurrent `try_lock_repo` + // (the purge path) on the same repo. This is load-bearing for the purge + // tool's M4 mutual exclusion. + // + // The pool MUST be single-connection. `try_lock_repo`'s own return is the + // only observable that exposes the bug (it acquires through the pool), and + // it is only deterministic when the writer's connection is the ONLY one, so + // `try_lock_repo`'s `pool.acquire()` is forced onto it: + // * Pre-fix, `acquire_write` locks via `.fetch_one(&self.pool)` and returns + // the lock-owning connection to the pool. `try_lock_repo` re-acquires + // that same connection and `pg_try_advisory_lock` re-locks it + // reentrantly -> Ok(Some) (RED). (On a multi-connection pool the reentrant + // grab is non-deterministic: `acquire` may hand back a different idle + // connection, so the bug hides — verified by execution.) + // * Post-fix, `acquire_write` pins the connection, so `try_lock_repo`'s + // `pool.acquire()` cannot get one and times out -> Err. Either way the + // fix guarantees try_lock does NOT reentrantly grab the held lock. + #[sqlx::test] + async fn acquire_write_guard_blocks_try_lock( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: sqlx::postgres::PgConnectOptions, + ) { + let pool = pool_opts + .max_connections(1) + .acquire_timeout(std::time::Duration::from_secs(3)) + .connect_with(connect_opts) + .await + .unwrap(); + let tmp = tempfile::TempDir::new().unwrap(); + let store = RepoStore::for_testing(tmp.path().to_path_buf(), pool); + let owner = "did:key:z6MkWriterLockOwnerAAAAAAAAAAAAAAAAAAAAAAAA"; + let name = "locktest"; + + // A live writer holds the per-repo advisory lock. + let guard = store.acquire_write(owner, name).await.unwrap(); + + // The purge path must NOT reentrantly acquire the same lock while held. + // Pre-fix: Ok(Some) (reentrant grab). Post-fix: Err (connection pinned). + // The invariant is "not Ok(Some)". + let contended = store.try_lock_repo(owner, name).await; + assert!( + !matches!(contended, Ok(Some(_))), + "try_lock_repo must not reentrantly acquire a lock a live acquire_write \ + guard holds (got Ok(Some) — the reentrant-grab bug)" + ); + + // After the writer releases (on its pinned connection), the lock is free + // and the single connection is back in the pool. + guard.release(true).await; + let after = store.try_lock_repo(owner, name).await.unwrap(); + assert!( + after.is_some(), + "lock must be free once the writer releases it" + ); + after.unwrap().release().await; + } } From c192c1f18d4186024ddd136c146f33b11f43acf2 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 15 Jul 2026 16:54:48 -0500 Subject: [PATCH 18/25] refactor(node): introduce ObjectStore trait seam over TigrisClient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract an async ObjectStore trait (exists/upload/download/delete) that TigrisClient implements, and hold Option> in RepoStore instead of Option. A dyn async trait needs async-trait (added as a direct dep; already transitively present), which E0038 would otherwise reject. Pure refactor — no behavior change; full suite stays green. Enables an in-test fake for the purge Tigris work (U3). Cargo.lock also picks up the pre-existing 0.5.0->0.5.1 version sync (#185: the committed lock was stale vs the manifests) that any cargo run regenerates. --- Cargo.lock | 11 ++++--- crates/gitlawb-node/Cargo.toml | 1 + crates/gitlawb-node/src/git/repo_store.rs | 38 +++++++++++++---------- crates/gitlawb-node/src/git/tigris.rs | 33 ++++++++++++++------ crates/gitlawb-node/src/main.rs | 4 ++- 5 files changed, 54 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d12daa43..47f2824f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3300,7 +3300,7 @@ dependencies = [ [[package]] name = "git-remote-gitlawb" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "gitlawb-core", @@ -3312,7 +3312,7 @@ dependencies = [ [[package]] name = "gitlawb-attest" -version = "0.5.0" +version = "0.5.1" dependencies = [ "base64", "ed25519-dalek", @@ -3329,7 +3329,7 @@ dependencies = [ [[package]] name = "gitlawb-core" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "base64", @@ -3356,13 +3356,14 @@ dependencies = [ [[package]] name = "gitlawb-node" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", "async-compression", "async-graphql", "async-graphql-axum", + "async-trait", "aws-config", "aws-sdk-s3", "axum", @@ -3412,7 +3413,7 @@ dependencies = [ [[package]] name = "gl" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", diff --git a/crates/gitlawb-node/Cargo.toml b/crates/gitlawb-node/Cargo.toml index 9e42c084..3e721181 100644 --- a/crates/gitlawb-node/Cargo.toml +++ b/crates/gitlawb-node/Cargo.toml @@ -12,6 +12,7 @@ path = "src/main.rs" [dependencies] gitlawb-core = { path = "../gitlawb-core" } +async-trait = "0.1" ed25519-dalek = { workspace = true } base64 = { workspace = true } tokio = { workspace = true } diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index ed691960..66ef055a 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -18,17 +18,17 @@ use tokio::sync::Mutex; use tracing::{debug, info, warn}; use super::store; -use super::tigris::TigrisClient; +use super::tigris::ObjectStore; -/// Centralized repo storage: local disk cache + optional Tigris backend. +/// Centralized repo storage: local disk cache + optional object-store backend. #[derive(Clone)] pub struct RepoStore { repos_dir: PathBuf, - tigris: Option, + object_store: Option>, /// Shared Postgres pool for advisory locks. pool: PgPool, - /// Tracks repos already confirmed to exist in Tigris — avoids redundant - /// HEAD checks and background uploads for repos we've already migrated. + /// Tracks repos already confirmed to exist in the object store — avoids + /// redundant HEAD checks and background uploads for repos we've migrated. migrated: Arc>>, } @@ -37,16 +37,20 @@ impl RepoStore { pub fn for_testing(repos_dir: PathBuf, pool: PgPool) -> Self { Self { repos_dir, - tigris: None, + object_store: None, pool, migrated: Arc::new(tokio::sync::Mutex::new(std::collections::HashSet::new())), } } - pub fn new(repos_dir: PathBuf, tigris: Option, pool: PgPool) -> Self { + pub fn new( + repos_dir: PathBuf, + object_store: Option>, + pool: PgPool, + ) -> Self { Self { repos_dir, - tigris, + object_store, pool, migrated: Arc::new(Mutex::new(HashSet::new())), } @@ -63,7 +67,7 @@ impl RepoStore { if local_path.exists() { // Lazy migration: if Tigris is enabled and we haven't confirmed this // repo is in Tigris yet, check and upload in the background. - if let Some(ref tigris) = self.tigris { + if let Some(ref tigris) = self.object_store { let key = format!("{owner_slug}/{repo_name}"); let already_migrated = self.migrated.lock().await.contains(&key); if !already_migrated { @@ -99,7 +103,7 @@ impl RepoStore { } // Try downloading from Tigris - if let Some(ref tigris) = self.tigris { + if let Some(ref tigris) = self.object_store { if tigris.exists(&owner_slug, repo_name).await.unwrap_or(false) { debug!(repo = %repo_name, "cache miss — downloading from tigris"); tigris @@ -127,7 +131,7 @@ impl RepoStore { pub async fn acquire_fresh(&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 let Some(ref tigris) = self.object_store { 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 { @@ -193,7 +197,7 @@ impl RepoStore { // Always download the latest from Tigris before writing. // Local disk may be stale if another machine pushed since our last access. - if let Some(ref tigris) = self.tigris { + if let Some(ref tigris) = self.object_store { 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 { @@ -216,7 +220,7 @@ impl RepoStore { local_path, lock_key, conn, - tigris: self.tigris.clone(), + object_store: self.object_store.clone(), }) } @@ -260,7 +264,7 @@ impl RepoStore { store::init_bare(&local_path).context("initializing bare repo")?; // Upload to Tigris in background - if let Some(ref tigris) = self.tigris { + if let Some(ref tigris) = self.object_store { let tigris = tigris.clone(); let owner_slug = owner_slug.clone(); let repo_name = repo_name.to_string(); @@ -278,7 +282,7 @@ 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) { - if let Some(ref tigris) = self.tigris { + if let Some(ref tigris) = self.object_store { let (owner_slug, local_path) = match self.local_path(owner_did, repo_name) { Ok(p) => p, Err(e) => { @@ -405,7 +409,7 @@ pub struct RepoWriteGuard { /// and so a concurrent `try_lock_repo` cannot be handed this connection and /// reentrantly re-grab the lock. conn: sqlx::pool::PoolConnection, - tigris: Option, + object_store: Option>, } impl RepoWriteGuard { @@ -422,7 +426,7 @@ impl RepoWriteGuard { pub async fn release(mut self, success: bool) { // Upload to Tigris only on success. if success { - if let Some(ref tigris) = self.tigris { + if let Some(ref tigris) = self.object_store { if let Err(e) = tigris .upload(&self.owner_slug, &self.repo_name, &self.local_path) .await diff --git a/crates/gitlawb-node/src/git/tigris.rs b/crates/gitlawb-node/src/git/tigris.rs index ad26ddc5..c4f54fb0 100644 --- a/crates/gitlawb-node/src/git/tigris.rs +++ b/crates/gitlawb-node/src/git/tigris.rs @@ -11,6 +11,22 @@ use anyhow::{Context, Result}; use aws_sdk_s3::Client as S3Client; use tracing::{debug, info}; +/// Object storage for git bare-repo archives. The seam that lets `RepoStore` +/// (and tests) depend on storage behavior without a live bucket. `TigrisClient` +/// is the production implementation. +#[async_trait::async_trait] +pub trait ObjectStore: Send + Sync { + /// Whether an archive exists for this repo. + async fn exists(&self, owner_slug: &str, repo_name: &str) -> Result; + /// Upload a local bare repo directory as an archive. + async fn upload(&self, owner_slug: &str, repo_name: &str, local_path: &Path) -> Result<()>; + /// Download and extract a repo archive to local disk. + async fn download(&self, owner_slug: &str, repo_name: &str, local_path: &Path) -> Result<()>; + /// Delete a repo archive. (Wired into purge-spam in U3.) + #[allow(dead_code)] + async fn delete(&self, owner_slug: &str, repo_name: &str) -> Result<()>; +} + /// Wrapper around the S3 client with the configured bucket. #[derive(Clone)] pub struct TigrisClient { @@ -35,9 +51,12 @@ impl TigrisClient { fn repo_key(owner_slug: &str, repo_name: &str) -> String { format!("repos/v1/{owner_slug}/{repo_name}.tar.zst") } +} +#[async_trait::async_trait] +impl ObjectStore for TigrisClient { /// Check if a repo archive exists in Tigris. - pub async fn exists(&self, owner_slug: &str, repo_name: &str) -> Result { + async fn exists(&self, owner_slug: &str, repo_name: &str) -> Result { let key = Self::repo_key(owner_slug, repo_name); match self .s3 @@ -59,7 +78,7 @@ 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<()> { + async fn upload(&self, owner_slug: &str, repo_name: &str, local_path: &Path) -> Result<()> { let key = Self::repo_key(owner_slug, repo_name); debug!(key = %key, path = %local_path.display(), "uploading repo to tigris"); @@ -89,12 +108,7 @@ impl TigrisClient { } /// Download a repo archive from Tigris and extract to local disk. - pub async fn download( - &self, - owner_slug: &str, - repo_name: &str, - local_path: &Path, - ) -> Result<()> { + async fn download(&self, owner_slug: &str, repo_name: &str, local_path: &Path) -> Result<()> { let key = Self::repo_key(owner_slug, repo_name); debug!(key = %key, path = %local_path.display(), "downloading repo from tigris"); @@ -128,8 +142,7 @@ impl TigrisClient { } /// Delete a repo archive from Tigris. - #[allow(dead_code)] - pub async fn delete(&self, owner_slug: &str, repo_name: &str) -> Result<()> { + async fn delete(&self, owner_slug: &str, repo_name: &str) -> Result<()> { let key = Self::repo_key(owner_slug, repo_name); self.s3 .delete_object() diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 2fe8164f..32063b9a 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -286,8 +286,10 @@ async fn main() -> Result<()> { None }; + let object_store = + tigris.map(|t| std::sync::Arc::new(t) as std::sync::Arc); let repo_store = - git::repo_store::RepoStore::new(config.repos_dir.clone(), tigris, db.pool().clone()); + git::repo_store::RepoStore::new(config.repos_dir.clone(), object_store, db.pool().clone()); // Per-DID limiter for the creation endpoints. Keyed on the authenticated // DID (attacker-varied), so bound its key set to cap memory. From 2155c958ac29edaa6d92f3dd0526a851aafa27cb Mon Sep 17 00:00:00 2001 From: t Date: Wed, 15 Jul 2026 17:07:47 -0500 Subject: [PATCH 19/25] fix(node): make purge-spam Tigris-authoritative and archive-deleting purge-spam built its RepoStore with no object store, so on a Tigris deployment it rechecked emptiness against possibly-stale local disk (deleting a repo with live remote refs) and left the archive behind (to be re-downloaded into a later same-owner/name repo). Give the admin command the configured object store, refresh the local copy from the authoritative archive under the per-repo lock before the recheck (fail-closed on a store error via a new skipped_store_error), and delete the archive on a successful purge (failures counted in a new archive_failed, never folded into success). When no bucket is configured the None path is byte-for-byte unchanged. Three load-bearing guards verified RED->GREEN on the Tigris-blind purge: remote-refs-not-deleted, archive-deleted, fail-closed-on-unreachable. --- crates/gitlawb-node/src/admin.rs | 277 +++++++++++++++++++++- crates/gitlawb-node/src/git/repo_store.rs | 28 +++ crates/gitlawb-node/src/git/tigris.rs | 3 +- crates/gitlawb-node/src/main.rs | 33 ++- 4 files changed, 332 insertions(+), 9 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index a03e93ed..35016424 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -43,9 +43,17 @@ pub struct PurgeSummary { pub skipped_not_empty: usize, /// Candidates skipped because a live writer held the per-repo lock. pub skipped_locked: usize, + /// Candidates skipped because the object store could not be consulted for the + /// authoritative emptiness recheck (fail-closed: never delete on a store + /// error rather than risk deleting a repo with live remote refs). + pub skipped_store_error: usize, /// Rows deleted whose on-disk dir removal FAILED (or was refused by the /// containment guard) — DB/disk drift the operator must reconcile. pub disk_failed: usize, + /// Rows+dirs deleted whose object-store archive removal FAILED — the archive + /// survives and could be re-downloaded into a later same-owner/name repo, so + /// this is tracked separately and never folded into a clean success. + pub archive_failed: usize, } /// A repo selected for purge, with the evidence that qualified it. @@ -297,8 +305,21 @@ pub async fn run_purge_spam( continue; } }; + // Refresh the local copy from the authoritative object store (if any) + // before the recheck: on a Tigris deployment the admin node's local disk + // can be stale, so an emptiness check against local alone could delete a + // repo that has live remote refs. Fail closed on any store error — never + // delete on an unverified view. + if let Err(e) = repo_store.refresh_from_archive(&c.owner_did, &c.name).await { + warn!(repo = %c.id, err = %e, + "purge-spam: could not consult the object store — skipped (fail-closed)"); + summary.skipped_store_error += 1; + guard.release().await; + continue; + } // Authoritative recheck UNDER the lock: a ref that landed before we locked - // makes the repo non-empty, so it must not be deleted. + // (or that the object-store refresh surfaced) makes the repo non-empty, so + // it must not be deleted. if ref_count_on_disk(&repos_dir, &c.owner_did, &c.name) != 0 { warn!(repo = %c.id, "purge-spam: repo no longer empty under lock — skipped (TOCTOU)"); summary.skipped_not_empty += 1; @@ -332,6 +353,15 @@ pub async fn run_purge_spam( } else { info!(repo = %c.id, rows = n, "purge-spam: deleted repo row + on-disk dir"); } + // Delete the object-store archive too (a no-op when no store is + // configured), else it survives and can be downloaded into a + // later repo created with the same owner/name. Counted separately + // so a surviving archive never reads as a clean success. + if let Err(e) = repo_store.delete_archive(&c.owner_did, &c.name).await { + warn!(repo = %c.id, err = %e, + "purge-spam: deleted row + dir but could not delete the object-store archive"); + summary.archive_failed += 1; + } } Err(e) => { warn!(repo = %c.id, err = %e, "purge-spam: failed to delete repo row — continuing"); @@ -340,8 +370,13 @@ pub async fn run_purge_spam( guard.release().await; } println!( - "purge-spam: deleted {} repo row(s); skipped {} (no longer empty), {} (locked by a live writer); {} on-disk removal(s) failed.", - summary.deleted, summary.skipped_not_empty, summary.skipped_locked, summary.disk_failed + "purge-spam: deleted {} repo row(s); skipped {} (no longer empty), {} (locked by a live writer), {} (object store unreachable); {} on-disk removal(s) failed, {} archive removal(s) failed.", + summary.deleted, + summary.skipped_not_empty, + summary.skipped_locked, + summary.skipped_store_error, + summary.disk_failed, + summary.archive_failed ); Ok(summary) } @@ -979,4 +1014,240 @@ mod db_tests { "an empty repo owned by a short-form excluded DID must never be a candidate" ); } + + // ── Tigris-authoritative purge (P1b) ─────────────────────────────────── + + /// In-test object store. `download` materializes a bare repo with (or + /// without) a ref so the purge tool's authoritative recheck can be driven + /// without a live bucket; error flags exercise the fail-closed and + /// archive-delete-failure paths. + struct FakeStore { + has_archive: bool, + archive_has_refs: bool, + fail_recheck: bool, + fail_delete: bool, + deleted: std::sync::Arc, + } + + #[async_trait::async_trait] + impl crate::git::tigris::ObjectStore for FakeStore { + async fn exists(&self, _owner: &str, _repo: &str) -> Result { + if self.fail_recheck { + anyhow::bail!("fake object store unreachable"); + } + Ok(self.has_archive) + } + async fn upload(&self, _owner: &str, _repo: &str, _path: &Path) -> Result<()> { + Ok(()) + } + async fn download(&self, _owner: &str, _repo: &str, local_path: &Path) -> Result<()> { + if self.fail_recheck { + anyhow::bail!("fake object store unreachable"); + } + // Materialize the authoritative archive on local disk so + // ref_count_on_disk reflects remote state. + let _ = std::fs::remove_dir_all(local_path); + store::init_bare(local_path).unwrap(); + if self.archive_has_refs { + seed_one_ref(local_path); + } + Ok(()) + } + async fn delete(&self, _owner: &str, _repo: &str) -> Result<()> { + self.deleted + .store(true, std::sync::atomic::Ordering::SeqCst); + if self.fail_delete { + anyhow::bail!("fake object store delete failed"); + } + Ok(()) + } + } + + fn store_backed( + repos_dir: &Path, + pool: &PgPool, + fake: FakeStore, + ) -> crate::git::repo_store::RepoStore { + crate::git::repo_store::RepoStore::new( + repos_dir.to_path_buf(), + Some(std::sync::Arc::new(fake)), + pool.clone(), + ) + } + + fn fake( + has_archive: bool, + archive_has_refs: bool, + fail_recheck: bool, + fail_delete: bool, + ) -> (FakeStore, std::sync::Arc) { + let deleted = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + ( + FakeStore { + has_archive, + archive_has_refs, + fail_recheck, + fail_delete, + deleted: deleted.clone(), + }, + deleted, + ) + } + + // A locally-empty repo whose authoritative archive HAS refs (pushed via + // another machine) must NOT be purged on the stale-local view. Load-bearing: + // RED on the Tigris-blind purge (deletes it), GREEN once the recheck + // consults the object store. + #[sqlx::test] + async fn purge_skips_repo_with_remote_refs_when_local_empty(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-remote", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &target.owner_did, &target.name); + store::init_bare(&path).unwrap(); + assert_eq!( + store::list_refs(&path).unwrap().len(), + 0, + "local starts empty" + ); + + let (f, _deleted) = fake(true, true, false, false); + let repo_store = store_backed(tmp.path(), &pool, f); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_some(), + "a repo with live remote refs must not be purged on a stale-local view" + ); + assert_eq!(summary.deleted, 0); + assert_eq!(summary.skipped_not_empty, 1); + } + + // A genuinely-empty repo (empty archive) is deleted AND its archive removed, + // else the archive can be re-downloaded into a later same-name repo. + // Load-bearing: RED on the Tigris-blind purge (archive survives), GREEN once + // the delete wires the archive removal. + #[sqlx::test] + async fn purge_deletes_archive_on_successful_delete(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &target.owner_did, &target.name); + store::init_bare(&path).unwrap(); + + let (f, deleted) = fake(true, false, false, false); + let repo_store = store_backed(tmp.path(), &pool, f); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none(), + "a genuinely-empty repo is deleted" + ); + assert!( + deleted.load(std::sync::atomic::Ordering::SeqCst), + "the object-store archive must be deleted on a successful purge" + ); + assert_eq!(summary.deleted, 1); + assert_eq!(summary.archive_failed, 0); + } + + // An unreachable object store during the recheck must fail closed (skip), + // never delete on an unverified view. Load-bearing: RED on the Tigris-blind + // purge (deletes), GREEN once the recheck consults (and fails closed on) the + // store. + #[sqlx::test] + async fn purge_fails_closed_when_store_unreachable(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &target.owner_did, &target.name); + store::init_bare(&path).unwrap(); + + let (f, _deleted) = fake(true, false, true, false); + let repo_store = store_backed(tmp.path(), &pool, f); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_some(), + "must not delete when the object store is unreachable (fail-closed)" + ); + assert_eq!(summary.deleted, 0); + assert_eq!(summary.skipped_store_error, 1); + } + + // An archive-delete failure after the row+dir are removed is counted + // separately, never folded into a clean success. + #[sqlx::test] + async fn purge_archive_delete_failure_counted_separately(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &target.owner_did, &target.name); + store::init_bare(&path).unwrap(); + + let (f, deleted) = fake(true, false, false, true); + let repo_store = store_backed(tmp.path(), &pool, f); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none(), + "the row+dir are still deleted" + ); + assert!(deleted.load(std::sync::atomic::Ordering::SeqCst)); + assert_eq!(summary.deleted, 1); + assert_eq!( + summary.archive_failed, 1, + "a surviving archive must be counted, not reported as clean success" + ); + } + + // R7: with no object store configured (single-machine), an empty repo is + // deleted exactly as before and nothing touches an archive. + #[sqlx::test] + async fn purge_tigris_disabled_deletes_empty_unchanged(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &target.owner_did, &target.name); + store::init_bare(&path).unwrap(); + + let repo_store = test_store(tmp.path(), &pool); // Tigris = None + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!(db + .get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none()); + assert_eq!(summary.deleted, 1); + assert_eq!(summary.skipped_store_error, 0); + assert_eq!(summary.archive_failed, 0); + } } diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 66ef055a..9a3a8814 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -341,6 +341,34 @@ impl RepoStore { Ok((owner_slug, local_path)) } + + /// Refresh the local copy from the authoritative object-store archive so an + /// emptiness recheck reflects remote state (used by purge-spam on a Tigris + /// deployment, where the admin node's local disk can be stale). Downloads + /// the archive over the local path when it exists; a no-op when no object + /// store is configured (single-machine). Errors propagate so the caller can + /// fail closed rather than delete on a stale-local view. + pub async fn refresh_from_archive(&self, owner_did: &str, repo_name: &str) -> Result<()> { + let Some(ref store) = self.object_store else { + return Ok(()); + }; + let (owner_slug, local_path) = self.local_path(owner_did, repo_name)?; + if store.exists(&owner_slug, repo_name).await? { + store.download(&owner_slug, repo_name, &local_path).await?; + } + Ok(()) + } + + /// Delete the object-store archive for a repo. A no-op when no object store + /// is configured. Used by purge-spam so a deleted repo's archive cannot be + /// downloaded into a later repo created with the same owner/name. + pub async fn delete_archive(&self, owner_did: &str, repo_name: &str) -> Result<()> { + let Some(ref store) = self.object_store else { + return Ok(()); + }; + let (owner_slug, _local_path) = self.local_path(owner_did, repo_name)?; + store.delete(&owner_slug, repo_name).await + } } /// Strict allowlist validator for `owner_did` and `repo_name`. diff --git a/crates/gitlawb-node/src/git/tigris.rs b/crates/gitlawb-node/src/git/tigris.rs index c4f54fb0..9b447696 100644 --- a/crates/gitlawb-node/src/git/tigris.rs +++ b/crates/gitlawb-node/src/git/tigris.rs @@ -22,8 +22,7 @@ pub trait ObjectStore: Send + Sync { async fn upload(&self, owner_slug: &str, repo_name: &str, local_path: &Path) -> Result<()>; /// Download and extract a repo archive to local disk. async fn download(&self, owner_slug: &str, repo_name: &str, local_path: &Path) -> Result<()>; - /// Delete a repo archive. (Wired into purge-spam in U3.) - #[allow(dead_code)] + /// Delete a repo archive. async fn delete(&self, owner_slug: &str, repo_name: &str) -> Result<()>; } diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 32063b9a..f85eaded 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -625,10 +625,35 @@ async fn run_admin_command(command: config::Command, config: &Config) -> Result< ) .await .context("connecting to database for purge-spam")?; - // Lock-only RepoStore (no Tigris): purge takes the per-repo advisory - // lock to exclude a live push, but never downloads/uploads archives. - let repo_store = - git::repo_store::RepoStore::new(config.repos_dir.clone(), None, db.pool().clone()); + // Build the object store so purge is authoritative against remote + // state: the emptiness recheck consults the archive (not just + // possibly-stale local disk) and a successful purge deletes the + // archive too. When no bucket is configured this stays None and purge + // is local-only, exactly as before. + let object_store: Option> = if config + .tigris_bucket + .is_empty() + { + None + } else { + match git::tigris::TigrisClient::new(&config.tigris_bucket).await { + Ok(client) => Some(std::sync::Arc::new(client)), + Err(e) => { + // Fail closed: without the configured store we cannot + // do the authoritative recheck, and a local-only purge + // on a Tigris deployment is the stale-view delete this + // guards against. Refuse to run rather than fall back. + anyhow::bail!( + "purge-spam: GITLAWB_TIGRIS_BUCKET is set but the object-store client failed to initialize ({e}); refusing to run a local-only purge on a Tigris deployment" + ); + } + } + }; + let repo_store = git::repo_store::RepoStore::new( + config.repos_dir.clone(), + object_store, + db.pool().clone(), + ); admin::run_purge_spam(&db, &repo_store, &config.repos_dir, execute) .await .map(|_| ()) From 8842272560d36291a274592ced9c0260e56387bd Mon Sep 17 00:00:00 2001 From: t Date: Wed, 15 Jul 2026 17:37:43 -0500 Subject: [PATCH 20/25] fix(review): don't hold/leak the pool connection in acquire_write Two regressions the connection-pinning fix (010c625) introduced, caught in pre-push review: - Pool pressure: acquire_write held its pinned connection through the whole 60s lock-retry backoff; pre-change the retry probed via the pool and returned the connection between attempts. Under a same-repo write burst every spinner now held a pool connection through its sleeps and could starve the shared pool. Restructure the loop to return the connection between FAILED attempts and keep it only on the winning one. - Lock leak: on the object-store download-error early-return the pinned connection dropped back to the pool with the advisory lock still held (no guard Drop runs pre-construction, and sqlx does not release session locks on connection return), blocking every future write to that repo until the pool reaped the connection. Unlock on the pinned connection before bailing. Add acquire_write_guard_excludes_try_lock_across_connections: a multi-connection test proving the exclusion holds across sessions (the production topology), which also catches a pin-but-don't-lock regression the single-connection test cannot. --- crates/gitlawb-node/src/git/repo_store.rs | 128 +++++++++++++++------- 1 file changed, 89 insertions(+), 39 deletions(-) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 9a3a8814..b1f64b50 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -161,54 +161,73 @@ 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 connection for the guard's whole lifetime so the - // session-scoped advisory lock lives on ONE connection and `release` - // unlocks it on that SAME connection. A plain `&self.pool` query returns - // its connection to the pool between calls, which (a) leaves the lock on - // an idle connection `release`'s unlock never reaches, and (b) lets - // `try_lock_repo`'s `pool.acquire()` be handed that idle connection and - // reentrantly re-grab the lock. Mirrors `RepoLockGuard`. - let mut conn = self - .pool - .acquire() - .await - .context("acquiring write-lock connection")?; - - // Acquire Postgres advisory lock with retry using pg_try_advisory_lock - // to avoid blocking indefinitely on stale locks from crashed connections. - let mut acquired = false; - for attempt in 0..60 { - let row: (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") - .bind(lock_key) - .fetch_one(&mut *conn) - .await - .context("trying advisory lock")?; - if row.0 { - acquired = true; - break; + // Acquire the advisory lock on a DEDICATED connection, held for the + // guard's whole lifetime so the session-scoped lock lives on ONE + // connection and `release` unlocks it on that SAME connection, and so a + // concurrent `try_lock_repo`'s `pool.acquire()` cannot be handed the + // lock-owning connection and reentrantly re-grab it. Mirrors + // `RepoLockGuard`. Use pg_try_advisory_lock with retry to avoid blocking + // indefinitely on a stale lock from a crashed connection. + // + // Between FAILED attempts the connection is returned to the pool (the + // `drop` below) so a writer spinning on a contended repo does NOT hold a + // pool connection through the retry backoff — holding one per spinner + // would starve the shared pool under a same-repo write burst. Only the + // WINNING attempt keeps its connection (the lock lives on it). + let mut conn = { + let mut acquired = None; + for attempt in 0..60 { + let mut c = self + .pool + .acquire() + .await + .context("acquiring write-lock connection")?; + 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 = Some(c); + break; + } + drop(c); + if attempt < 59 { + tokio::time::sleep(std::time::Duration::from_secs(1)).await; + } } - if attempt < 59 { - tokio::time::sleep(std::time::Duration::from_secs(1)).await; + match acquired { + Some(c) => c, + None => anyhow::bail!( + "could not acquire advisory lock after 60s — possible stale lock for {owner_slug}/{repo_name}" + ), } - } - if !acquired { - anyhow::bail!("could not acquire advisory lock after 60s — possible stale lock for {owner_slug}/{repo_name}"); - } + }; - // Always download the latest from Tigris before writing. + // Always download the latest from the object store before writing. // Local disk may be stale if another machine pushed since our last access. - if let Some(ref tigris) = self.object_store { - 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 { + if let Some(ref store) = self.object_store { + if store.exists(&owner_slug, repo_name).await.unwrap_or(false) { + debug!(repo = %repo_name, "write acquire: downloading latest from object store"); + if let Err(e) = store.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 + // archive must not block a write when a valid local copy // exists — release(success) will re-upload a good archive. if local_path.exists() { warn!(repo = %repo_name, err = %e, - "write acquire: tigris download failed — falling back to local copy"); + "write acquire: object-store download failed — falling back to local copy"); } else { - return Err(e).context("downloading repo from tigris for write"); + // Unlock on the pinned connection before bailing, else the + // advisory lock orphans on it: the guard is not constructed + // yet (so its release never runs), and sqlx does not release + // session locks when a connection returns to the pool — the + // lock would block every future write to this repo until the + // pool reaps the connection. + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(lock_key) + .execute(&mut *conn) + .await; + return Err(e).context("downloading repo from object store for write"); } } } @@ -723,4 +742,35 @@ mod tests { ); after.unwrap().release().await; } + + // The exclusion is real ACROSS sessions, not just the reentrant + // same-connection case: on a multi-connection pool the writer pins its + // connection, so `try_lock_repo`'s `pool.acquire()` gets a DIFFERENT + // connection whose `pg_try_advisory_lock` correctly observes the lock held + // -> Ok(None). This is the actual production topology (writer and purge on + // different sessions), and it also catches a pin-but-don't-actually-lock + // regression that the single-connection test (which passes via a + // pool-exhaustion Err) cannot distinguish from a real held lock. + #[sqlx::test] + async fn acquire_write_guard_excludes_try_lock_across_connections(pool: PgPool) { + let tmp = tempfile::TempDir::new().unwrap(); + let store = RepoStore::for_testing(tmp.path().to_path_buf(), pool); + let owner = "did:key:z6MkCrossConnExclusionAAAAAAAAAAAAAAAAAAAAA"; + let name = "xconn"; + + let guard = store.acquire_write(owner, name).await.unwrap(); + + // A second, distinct pooled connection must see the lock held. + let contended = store.try_lock_repo(owner, name).await.unwrap(); + assert!( + contended.is_none(), + "a live acquire_write guard must exclude try_lock_repo on a different \ + connection (Ok(None)); Ok(Some) would mean the lock is not held" + ); + + guard.release(true).await; + let after = store.try_lock_repo(owner, name).await.unwrap(); + assert!(after.is_some(), "lock is free after release"); + after.unwrap().release().await; + } } From 1d011beeeb9f51c0a9665da7a3a2bc7fd3d4d73d Mon Sep 17 00:00:00 2001 From: t Date: Thu, 16 Jul 2026 09:38:54 -0500 Subject: [PATCH 21/25] fix(node): make advisory-lock guards drop-safe and reorder acquire_write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RepoWriteGuard and RepoLockGuard now hold their pooled connection as an Option: release() takes it (returns it to the pool) while a Drop impl closes the connection when release() was never reached — an early ?, a panic, or the handler future cancelled on client disconnect. Closing the connection ends the Postgres session so the server frees the session-scoped advisory lock, instead of returning a lock-holding connection to the pool where it would block every later write to that repo until the connection is reaped (up to ~30min at the pool's default max_lifetime). acquire_write now wraps the winning connection in the guard BEFORE the freshness download, so a cancellation during that await is covered by the guard's Drop rather than orphaning the lock on a bare connection. Tests observe the leak by disabling the pool's ambient reaping (which otherwise masks it ~1.8s after drop), so the lock stays held until the fix's own unlock. RED before / GREEN after for both guards and the acquire-side cancel; each protection line proven load-bearing by revert (INV-21). --- crates/gitlawb-node/src/git/repo_store.rs | 347 +++++++++++++++++++--- 1 file changed, 309 insertions(+), 38 deletions(-) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index b1f64b50..04e5dfc3 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -174,7 +174,7 @@ impl RepoStore { // pool connection through the retry backoff — holding one per spinner // would starve the shared pool under a same-repo write burst. Only the // WINNING attempt keeps its connection (the lock lives on it). - let mut conn = { + let conn = { let mut acquired = None; for attempt in 0..60 { let mut c = self @@ -204,43 +204,51 @@ impl RepoStore { } }; + // Wrap the winning connection in the guard IMMEDIATELY, before any + // further await. The download below can be cancelled (the caller's + // future dropped mid-op) or fail; once the connection lives inside the + // guard, its `Drop` frees the advisory lock on every such exit. Before + // this ordering the lock orphaned on a bare connection returned to the + // pool holding it. (KTD-2.) + let guard = RepoWriteGuard { + owner_slug, + repo_name: repo_name.to_string(), + local_path, + lock_key, + conn: Some(conn), + object_store: self.object_store.clone(), + }; + // Always download the latest from the object store before writing. // Local disk may be stale if another machine pushed since our last access. if let Some(ref store) = self.object_store { - if store.exists(&owner_slug, repo_name).await.unwrap_or(false) { - debug!(repo = %repo_name, "write acquire: downloading latest from object store"); - if let Err(e) = store.download(&owner_slug, repo_name, &local_path).await { + if store + .exists(&guard.owner_slug, &guard.repo_name) + .await + .unwrap_or(false) + { + debug!(repo = %guard.repo_name, "write acquire: downloading latest from object store"); + if let Err(e) = store + .download(&guard.owner_slug, &guard.repo_name, &guard.local_path) + .await + { // Same self-healing fallback as acquire_fresh: a corrupt/unreadable // archive must not block a write when a valid local copy // exists — release(success) will re-upload a good archive. - if local_path.exists() { - warn!(repo = %repo_name, err = %e, + if guard.local_path.exists() { + warn!(repo = %guard.repo_name, err = %e, "write acquire: object-store download failed — falling back to local copy"); } else { - // Unlock on the pinned connection before bailing, else the - // advisory lock orphans on it: the guard is not constructed - // yet (so its release never runs), and sqlx does not release - // session locks when a connection returns to the pool — the - // lock would block every future write to this repo until the - // pool reaps the connection. - let _ = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(lock_key) - .execute(&mut *conn) - .await; + // Dropping the guard frees the advisory lock (its `Drop` + // closes the connection). No Tigris upload on a bail, + // unlike release(true). return Err(e).context("downloading repo from object store for write"); } } } } - Ok(RepoWriteGuard { - owner_slug, - repo_name: repo_name.to_string(), - local_path, - lock_key, - conn, - object_store: self.object_store.clone(), - }) + Ok(guard) } /// Try to acquire ONLY the per-repo advisory lock, non-blocking, with no @@ -273,7 +281,11 @@ impl RepoStore { if !row.0 { return Ok(None); } - Ok(Some(RepoLockGuard { conn, lock_key })) + Ok(Some(RepoLockGuard { + conn: Some(conn), + lock_key, + repo_name: repo_name.to_string(), + })) } /// Initialize a new bare repo on local disk and upload to Tigris. @@ -454,8 +466,9 @@ pub struct RepoWriteGuard { /// Dedicated pooled connection the advisory lock lives on. Held for the /// guard's lifetime so `release` unlocks on the SAME session that locked, /// and so a concurrent `try_lock_repo` cannot be handed this connection and - /// reentrantly re-grab the lock. - conn: sqlx::pool::PoolConnection, + /// reentrantly re-grab the lock. `Option` so `release` can take it (return + /// it to the pool) while `Drop` closes it when release was never reached. + conn: Option>, object_store: Option>, } @@ -485,12 +498,32 @@ impl RepoWriteGuard { 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, then - // the connection returns to the pool on drop. - let _ = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(self.lock_key) - .execute(&mut *self.conn) - .await; + // Unlock on the SAME connection that took the lock, then TAKE the + // connection so it returns to the pool on drop and the guard's `Drop` + // sees `None` (no close-on-drop). If this future is cancelled before the + // take completes, the connection stays in `self.conn` and `Drop` frees + // the lock by closing it instead. + if let Some(mut conn) = self.conn.take() { + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(self.lock_key) + .execute(&mut *conn) + .await; + } + } +} + +impl Drop for RepoWriteGuard { + fn drop(&mut self) { + // If `release()` was never reached (an early `?`, a panic, or the + // caller's future cancelled on client disconnect), the connection still + // holds the session advisory lock. Close it so Postgres frees the lock + // at session end, rather than returning a lock-holding connection to the + // pool where it would block every future write to this repo. (R1.) + if let Some(mut conn) = self.conn.take() { + warn!(repo = %self.repo_name, + "RepoWriteGuard dropped without release() — closing its connection to free the advisory lock"); + conn.close_on_drop(); + } } } @@ -498,18 +531,34 @@ impl RepoWriteGuard { /// lock (and the dedicated connection it lives on) until `release()`. No Tigris /// I/O — unlike [`RepoWriteGuard`], releasing does NOT upload anything. pub struct RepoLockGuard { - conn: sqlx::pool::PoolConnection, + conn: Option>, lock_key: i64, + repo_name: String, } impl RepoLockGuard { /// Release the advisory lock on the same connection that took it, then return /// the connection to the pool. pub async fn release(mut self) { - let _ = sqlx::query("SELECT pg_advisory_unlock($1)") - .bind(self.lock_key) - .execute(&mut *self.conn) - .await; + if let Some(mut conn) = self.conn.take() { + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(self.lock_key) + .execute(&mut *conn) + .await; + } + } +} + +impl Drop for RepoLockGuard { + fn drop(&mut self) { + // Same guarantee as RepoWriteGuard: a lock guard dropped without + // release() closes its connection so Postgres frees the advisory lock, + // rather than returning a lock-holding connection to the pool. (R1.) + if let Some(mut conn) = self.conn.take() { + warn!(repo = %self.repo_name, + "RepoLockGuard dropped without release() — closing its connection to free the advisory lock"); + conn.close_on_drop(); + } } } @@ -773,4 +822,226 @@ mod tests { assert!(after.is_some(), "lock is free after release"); after.unwrap().release().await; } + + // ── Drop-safety of the advisory-lock guards (U1: R1, R2; AE2) ─────────── + + // A minimal ObjectStore double: `exists()` is configurable, `download()` can + // park on a gate until notified (to hold `acquire_write` inside its + // post-lock await for the cancellation test), and `upload()` records its + // calls. Serves U1's reorder test and U3's serialization tests. + struct GatedStore { + exists: bool, + download_gate: Option>, + uploads: std::sync::Arc>>, + } + + impl GatedStore { + fn new(exists: bool) -> Self { + Self { + exists, + download_gate: None, + uploads: std::sync::Arc::new(std::sync::Mutex::new(Vec::new())), + } + } + } + + #[async_trait::async_trait] + impl crate::git::tigris::ObjectStore for GatedStore { + async fn exists(&self, _o: &str, _r: &str) -> anyhow::Result { + Ok(self.exists) + } + async fn upload(&self, o: &str, r: &str, _p: &std::path::Path) -> anyhow::Result<()> { + self.uploads + .lock() + .unwrap() + .push((o.to_string(), r.to_string())); + Ok(()) + } + async fn download(&self, _o: &str, _r: &str, _p: &std::path::Path) -> anyhow::Result<()> { + if let Some(gate) = &self.download_gate { + gate.notified().await; + } + Ok(()) + } + async fn delete(&self, _o: &str, _r: &str) -> anyhow::Result<()> { + Ok(()) + } + } + + // Non-perturbing probe: true iff advisory lock `key` is free (grabs it and + // immediately releases it on the observer's own separate session). + async fn advisory_lock_is_free(conn: &mut sqlx::PgConnection, key: i64) -> bool { + let (got,): (bool,) = sqlx::query_as("SELECT pg_try_advisory_lock($1)") + .bind(key) + .fetch_one(&mut *conn) + .await + .unwrap(); + if got { + let _ = sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(key) + .execute(&mut *conn) + .await; + } + got + } + + async fn poll_until_free(conn: &mut sqlx::PgConnection, key: i64) -> bool { + for _ in 0..50 { + if advisory_lock_is_free(conn, key).await { + return true; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + } + false + } + + fn lock_key_for(owner: &str, name: &str) -> i64 { + advisory_lock_key(&owner.replace([':', '/'], "_"), name) + } + + // Build a pool whose idle/lifetime reaping is disabled, so a leaked + // (dropped-without-release) connection is NOT reclaimed by ambient sqlx + // maintenance during the poll window. Without this the default sqlx-test + // pool reaps the connection ~1.8s after drop, freeing the lock on its own + // and masking the leak — the test must observe ONLY the fix's own unlock. + async fn no_reap_pool( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: &sqlx::postgres::PgConnectOptions, + ) -> PgPool { + pool_opts + .max_connections(5) + .min_connections(0) + .idle_timeout(None) + .max_lifetime(None) + .test_before_acquire(false) + .connect_with(connect_opts.clone()) + .await + .unwrap() + } + + // R1/AE2: a RepoWriteGuard dropped WITHOUT release() must free its advisory + // lock — its connection is closed rather than returned to the pool still + // holding the session lock. Pre-fix (no Drop impl) the lock leaks -> RED. + #[sqlx::test] + async fn write_guard_dropped_without_release_frees_lock( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: sqlx::postgres::PgConnectOptions, + ) { + use sqlx::ConnectOptions; + let pool = no_reap_pool(pool_opts, &connect_opts).await; + let tmp = tempfile::TempDir::new().unwrap(); + let store = RepoStore::new(tmp.path().to_path_buf(), None, pool); + let owner = "did:key:z6MkDropFreesLockAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let name = "droptest"; + let key = lock_key_for(owner, name); + let mut observer = connect_opts.connect().await.unwrap(); + + let guard = store.acquire_write(owner, name).await.unwrap(); + assert!( + !advisory_lock_is_free(&mut observer, key).await, + "lock must be held while the guard is alive" + ); + + drop(guard); // NO release() + + assert!( + poll_until_free(&mut observer, key).await, + "advisory lock must be freed after a RepoWriteGuard is dropped without release()" + ); + } + + // R1: same guarantee for the purge lock-only guard. + #[sqlx::test] + async fn repo_lock_guard_dropped_without_release_frees_lock( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: sqlx::postgres::PgConnectOptions, + ) { + use sqlx::ConnectOptions; + let pool = no_reap_pool(pool_opts, &connect_opts).await; + let tmp = tempfile::TempDir::new().unwrap(); + let store = RepoStore::new(tmp.path().to_path_buf(), None, pool); + let owner = "did:key:z6MkLockGuardDropFreesAAAAAAAAAAAAAAAAAAAAAA"; + let name = "lockdrop"; + let key = lock_key_for(owner, name); + let mut observer = connect_opts.connect().await.unwrap(); + + let guard = store.try_lock_repo(owner, name).await.unwrap().unwrap(); + assert!( + !advisory_lock_is_free(&mut observer, key).await, + "lock must be held while the lock guard is alive" + ); + + drop(guard); // NO release() + + assert!( + poll_until_free(&mut observer, key).await, + "advisory lock must be freed after a RepoLockGuard is dropped without release()" + ); + } + + // R2/KTD-2: cancelling acquire_write DURING its post-lock freshness download + // must free the lock. Pre-reorder the lock is won onto a bare connection + // before the guard exists, so cancellation leaks it -> RED; post-reorder the + // guard wraps the connection first and its Drop frees the lock. + #[sqlx::test] + async fn acquire_write_cancelled_during_download_frees_lock( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: sqlx::postgres::PgConnectOptions, + ) { + use sqlx::ConnectOptions; + let pool = no_reap_pool(pool_opts, &connect_opts).await; + let tmp = tempfile::TempDir::new().unwrap(); + let mut ts = GatedStore::new(true); + ts.download_gate = Some(std::sync::Arc::new(tokio::sync::Notify::new())); + let store = RepoStore::new( + tmp.path().to_path_buf(), + Some(std::sync::Arc::new(ts)), + pool, + ); + let owner = "did:key:z6MkCancelDownloadFreesAAAAAAAAAAAAAAAAAAAA"; + let name = "canceldl"; + let key = lock_key_for(owner, name); + let mut observer = connect_opts.connect().await.unwrap(); + + { + let fut = store.acquire_write(owner, name); + tokio::pin!(fut); + tokio::select! { + _ = &mut fut => panic!("acquire_write should be parked in download, not complete"), + _ = tokio::time::sleep(std::time::Duration::from_millis(400)) => {} + } + // `fut` is dropped here — cancels acquire_write mid-download. + } + + assert!( + poll_until_free(&mut observer, key).await, + "advisory lock must be freed when acquire_write is cancelled mid-download" + ); + } + + // R2 negative: the normal release() path must NOT close the connection — it + // returns to the pool, so writes don't churn the pool. On a single-connection + // pool a second acquire_write can only succeed if the first returned its + // connection (unlocked, not closed). + #[sqlx::test] + async fn release_returns_connection_to_pool( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: sqlx::postgres::PgConnectOptions, + ) { + let pool = pool_opts + .max_connections(1) + .acquire_timeout(std::time::Duration::from_secs(3)) + .connect_with(connect_opts) + .await + .unwrap(); + let tmp = tempfile::TempDir::new().unwrap(); + let store = RepoStore::new(tmp.path().to_path_buf(), None, pool); + let owner = "did:key:z6MkReleasePoolsConnAAAAAAAAAAAAAAAAAAAAAAAA"; + let name = "releasepool"; + + for _ in 0..3 { + let guard = store.acquire_write(owner, name).await.unwrap(); + guard.release(true).await; + } + } } From 2f679d07a4f75cd7d352814e65b8a7fc7370ffde Mon Sep 17 00:00:00 2001 From: t Date: Thu, 16 Jul 2026 09:53:43 -0500 Subject: [PATCH 22/25] fix(node): complete a fully-received push after client disconnect git_receive_pack now runs the acquire -> receive-pack -> release core in a spawned task the handler awaits but whose cancellation it does not propagate. The pack body is fully buffered before the handler runs, so the task is self-contained: a client that disconnects mid-apply drops the handler future without cancelling the task, and the push still applies, uploads, and releases its lock in order. Drop-safety (the guard's close-on-drop) remains the backstop; this closes the dominant window where a disconnect both bricked the lock and abandoned a received push. The post-push bookkeeping tail stays in the cancellable handler (a disconnect still forgoes it, as today). Settled OQ1 by execution: a guard abandoned inside a detached task when the tokio runtime tears down does not panic in Drop (5/5 runs), so detached-task shutdown tracking stays deferred. Kept the check as a regression guard. Disconnect regression drives a real push with a sleeping pre-receive hook and drops the handler mid-hook: RED on the inline handler (the git group is killed, ref absent), GREEN once detached. --- crates/gitlawb-node/src/api/repos.rs | 211 ++++++++++++++++++++-- crates/gitlawb-node/src/git/repo_store.rs | 46 +++++ 2 files changed, 242 insertions(+), 15 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 3b9d888e..3e4b3256 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -924,24 +924,46 @@ pub async fn git_receive_pack( } tracing::debug!(repo = %name, "acquiring write lock"); - let guard = state - .repo_store - .acquire_write(&record.owner_did, &record.name) - .await - .map_err(|e| { - tracing::error!(repo = %name, err = %e, "acquire_write failed"); - AppError::Git(e.to_string()) - })?; - let disk_path = guard.path().to_path_buf(); - tracing::debug!(repo = %name, path = %disk_path.display(), "running git receive-pack"); let body_len = body.len(); let git_timeout = std::time::Duration::from_secs(state.config.git_service_timeout_secs); - let receive_result = smart_http::receive_pack(&disk_path, body, git_timeout).await; - // 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(receive_result.is_ok()).await; + // Detach the acquire → receive-pack → release core from THIS handler future. + // The pack `body` is already fully buffered, so the spawned task is + // self-contained: a client disconnect drops the handler future but does NOT + // cancel the task, so a fully-received push still completes server-side — + // applies the pack, uploads, and releases the lock in order. What a + // disconnect forgoes is the cancellable post-push bookkeeping below, not the + // push itself. (KTD-3, R3.) + let repo_store = state.repo_store.clone(); + let owner_did = record.owner_did.clone(); + let repo_name = record.name.clone(); + let receive_result = tokio::spawn(async move { + let guard = repo_store + .acquire_write(&owner_did, &repo_name) + .await + .map_err(|e| { + tracing::error!(repo = %repo_name, err = %e, "acquire_write failed"); + AppError::Git(e.to_string()) + })?; + let disk_path = guard.path().to_path_buf(); + tracing::debug!(repo = %repo_name, path = %disk_path.display(), "running git receive-pack"); + let result = smart_http::receive_pack(&disk_path, body, git_timeout).await; + // 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(result.is_ok()).await; + Ok::<_, AppError>(result) + }) + .await + .map_err(|e| { + tracing::error!(repo = %name, err = %e, "receive-pack task panicked"); + AppError::Internal(anyhow::anyhow!("receive-pack task failed: {e}")) + })??; + + // Recompute the on-disk path for the post-push tail below — the guard that + // exposed it now lives inside the detached task. Identical to the path + // acquire_write resolved (repos_dir/owner_slug/name.git). + let disk_path = store::repo_disk_path(&state.config.repos_dir, &record.owner_did, &record.name); let result = receive_result.map_err(|e| { let app = git_service_app_error(&e); @@ -2579,6 +2601,165 @@ mod tests { ); } + // U2/R3/AE1: a fully-received push must complete server-side even when the + // client disconnects during the apply. The pack is buffered before the + // handler runs, so the acquire→receive→release core is detached from the + // handler future; dropping that future (the disconnect) must NOT cancel the + // push. A sleeping pre-receive hook creates a deterministic mid-apply window; + // dropping the handler during it kills the git group on the inline (pre-fix) + // code but not on the detached code, so the hook completes and the ref lands + // only after the fix. RED pre-fix (marker + ref absent), GREEN after. + #[sqlx::test] + async fn fully_received_push_completes_after_client_disconnect(pool: sqlx::PgPool) { + use axum::extract::{Path as AxPath, State}; + use axum::Extension; + use std::os::unix::fs::PermissionsExt; + use std::process::{Command, Stdio}; + use std::time::Duration; + + let mut state = crate::test_support::test_state(pool.clone()).await; + let repos_dir = tempfile::TempDir::new().unwrap(); + state.repo_store = crate::git::repo_store::RepoStore::for_testing( + repos_dir.path().to_path_buf(), + pool.clone(), + ); + + let owner = "z6u2pushowner"; + let name = "u2push"; + let owner_slug = owner.replace([':', '/'], "_"); + let bare = repos_dir + .path() + .join(&owner_slug) + .join(format!("{name}.git")); + + state + .db + .upsert_mirror_repo(owner, name, &bare.to_string_lossy(), None, false) + .await + .unwrap(); + + // Helper: run git, asserting success. + fn git(args: &[&str], dir: &std::path::Path) -> String { + let out = Command::new("git") + .args(args) + .current_dir(dir) + .output() + .unwrap(); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).trim().to_string() + } + + // Build a commit in a scratch working repo. + let work = tempfile::TempDir::new().unwrap(); + git(&["init", "-q", "-b", "main", "."], work.path()); + git(&["config", "user.email", "t@t"], work.path()); + git(&["config", "user.name", "t"], work.path()); + std::fs::write(work.path().join("f.txt"), "hi").unwrap(); + git(&["add", "f.txt"], work.path()); + git(&["commit", "-q", "-m", "c"], work.path()); + let oid = git(&["rev-parse", "HEAD"], work.path()); + + // Server bare repo: has the object (from the clone) but no refs/heads/main, + // so the push is a create satisfiable with an empty pack. + std::fs::create_dir_all(bare.parent().unwrap()).unwrap(); + let out = Command::new("git") + .args([ + "clone", + "--bare", + "-q", + &work.path().to_string_lossy(), + &bare.to_string_lossy(), + ]) + .output() + .unwrap(); + assert!( + out.status.success(), + "clone --bare failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + git( + &[ + "-C", + &bare.to_string_lossy(), + "update-ref", + "-d", + "refs/heads/main", + ], + work.path(), + ); + + // Sleeping pre-receive hook — the mid-apply window; writes a marker at the + // end so we can see the git child ran to completion. + let marker = repos_dir.path().join("hook_ran.marker"); + let hook = bare.join("hooks").join("pre-receive"); + std::fs::create_dir_all(hook.parent().unwrap()).unwrap(); + std::fs::write( + &hook, + format!( + "#!/bin/sh\ncat >/dev/null\nsleep 2\necho done > '{}'\nexit 0\n", + marker.display() + ), + ) + .unwrap(); + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(0o755)).unwrap(); + + // Receive-pack POST body: create refs/heads/main -> oid, plus an empty pack. + let zero = "0".repeat(40); + let cmd = format!("{zero} {oid} refs/heads/main\0report-status\n"); + let mut body = Vec::new(); + let len = cmd.len() + 4; + body.extend_from_slice(format!("{len:04x}").as_bytes()); + body.extend_from_slice(cmd.as_bytes()); + body.extend_from_slice(b"0000"); + let pack = Command::new("git") + .args([ + "-C", + &bare.to_string_lossy(), + "pack-objects", + "--stdout", + "-q", + ]) + .stdin(Stdio::null()) + .output() + .unwrap(); + assert!( + pack.status.success(), + "pack-objects failed: {}", + String::from_utf8_lossy(&pack.stderr) + ); + body.extend_from_slice(&pack.stdout); + let body = Bytes::from(body); + + // Drive the handler, then DROP it mid-hook (the "client disconnect"). + let handler = super::git_receive_pack( + State(state.clone()), + AxPath((owner.to_string(), format!("{name}.git"))), + Extension(crate::auth::AuthenticatedDid(owner.to_string())), + body, + ); + let _ = tokio::time::timeout(Duration::from_millis(800), handler).await; + + // Wait past the hook's sleep so a surviving (detached) push can finish. + tokio::time::sleep(Duration::from_millis(3000)).await; + + let ref_present = bare.join("refs/heads/main").exists() + || std::fs::read_to_string(bare.join("packed-refs")) + .unwrap_or_default() + .contains("refs/heads/main"); + assert!( + marker.exists(), + "pre-receive hook must run to completion despite the client disconnect (detached push)" + ); + assert!( + ref_present, + "refs/heads/main must be created after a fully-received push despite client disconnect" + ); + } + /// Repo creation must be throttled by the per-IP creation limiter BEFORE /// signature verification — otherwise a DID farm (one throwaway did:key per /// repo, each carrying a valid but machine-solved iCaptcha proof) walks past diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 04e5dfc3..ad9e7bf8 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -1044,4 +1044,50 @@ mod tests { guard.release(true).await; } } + + // OQ1: settle whether a guard abandoned inside a detached task when the + // tokio runtime tears down panics in Drop. `PoolConnection::drop` spawns a + // task (both the close_on_drop path and the normal return path), and + // `rt::spawn` panics via `missing_rt` if no runtime handle is current. U2 + // makes receive-pack a detached task that graceful shutdown does not drain, + // so at process exit such a task can be dropped mid-flight holding a guard. + // This drops a real runtime with a parked guard-holding task and asserts the + // process survives (a panic-in-Drop during unwind would abort the binary). + #[test] + fn guard_drop_during_runtime_shutdown_does_not_panic() { + let url = match std::env::var("DATABASE_URL") { + Ok(u) => u, + Err(_) => return, // no DB configured — skip (mirrors sqlx::test gating) + }; + let rt = tokio::runtime::Builder::new_multi_thread() + .worker_threads(2) + .enable_all() + .build() + .unwrap(); + let ready = std::sync::Arc::new(tokio::sync::Notify::new()); + let ready2 = ready.clone(); + rt.block_on(async move { + let pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(5) + .connect(&url) + .await + .unwrap(); + let tmp = tempfile::TempDir::new().unwrap(); + let store = RepoStore::new(tmp.path().to_path_buf(), None, pool); + tokio::spawn(async move { + let _guard = store + .acquire_write("did:key:z6MkOQ1ShutdownAAAAAAAAAAAAAAAAAAAAAAAAA", "oq1") + .await + .unwrap(); + ready2.notify_one(); + // Park forever holding the guard. + std::future::pending::<()>().await; + }); + ready.notified().await; + }); + // Drop the runtime while the detached task is parked holding the guard: + // the task is cancelled, dropping the guard during runtime teardown. Its + // Drop must not panic. Reaching the end of the test proves it didn't. + drop(rt); + } } From c3fedadb145d2fdca2721b9429eb8ce7c021c9cc Mon Sep 17 00:00:00 2001 From: t Date: Thu, 16 Jul 2026 10:06:06 -0500 Subject: [PATCH 23/25] fix(node): serialize archive uploads under the per-repo advisory lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lazy-migration upload in acquire(), init()'s background upload, and release_after_write() (fork's upload) previously PUT to the object store with no lock, so an in-flight upload could resurrect an archive purge-spam had just deleted under the same lock, and init's background upload could clobber a freshly-pushed archive. All three now go through upload_locked(): take the per-repo lock, re-check the local dir exists under it (a purge removes the dir under its lock, so a post-purge uploader finds it gone and skips), upload, then release. The background migration/init uploads skip on contention and self-heal; fork's foreground upload waits (bounded) instead, since a skipped fork upload has no later retry. Tests (recording object-store double): init upload skips under a held lock, fork upload waits then PUTs once after release, a late upload after a purge-style delete does not resurrect the archive, and an uncontended upload PUTs once. Load-bearing per INV-21 — neutralizing the lock+dir-check turns the three contention/resurrect tests RED. --- crates/gitlawb-node/src/git/repo_store.rs | 306 ++++++++++++++++++++-- 1 file changed, 279 insertions(+), 27 deletions(-) diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index ad9e7bf8..211830e0 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -71,10 +71,11 @@ impl RepoStore { let key = format!("{owner_slug}/{repo_name}"); let already_migrated = self.migrated.lock().await.contains(&key); if !already_migrated { + let store = self.clone(); let tigris = tigris.clone(); + let did = owner_did.to_string(); let slug = owner_slug.clone(); let name = repo_name.to_string(); - let path = local_path.clone(); let migrated = Arc::clone(&self.migrated); tokio::spawn(async move { // Check if already in Tigris before uploading @@ -84,8 +85,10 @@ 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"); + // Upload under the per-repo lock so it can't race + // a purge. If it skipped (lock contended or dir + // gone), do NOT mark migrated — retry next acquire. + if !store.upload_locked(&did, &name, false).await { return; } info!(repo = %name, "lazy migration to tigris complete"); @@ -290,41 +293,121 @@ impl RepoStore { /// Initialize a new bare repo on local disk and upload to Tigris. pub async fn init(&self, owner_did: &str, repo_name: &str) -> Result { - let (owner_slug, local_path) = self.local_path(owner_did, repo_name)?; + let (_owner_slug, local_path) = self.local_path(owner_did, repo_name)?; store::init_bare(&local_path).context("initializing bare repo")?; - // Upload to Tigris in background - if let Some(ref tigris) = self.object_store { - let tigris = tigris.clone(); - let owner_slug = owner_slug.clone(); - let repo_name = repo_name.to_string(); - let path = local_path.clone(); + // Upload to the object store in the background, UNDER the per-repo lock + // so the PUT can't race a purge. Skips on contention; a skipped init + // upload self-heals — the first write's release re-uploads the state. + if self.object_store.is_some() { + let store = self.clone(); + let did = owner_did.to_string(); + let name = repo_name.to_string(); 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"); - } + store.upload_locked(&did, &name, false).await; }); } Ok(local_path) } - /// 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. + /// Upload a repo to Tigris after a write operation (fork, etc.). Call this + /// after any operation that modifies the git repo on disk. Uploads UNDER the + /// per-repo advisory lock so the PUT cannot race a concurrent purge (which + /// deletes the repo under the same lock) and resurrect a deleted archive. + /// Waits (bounded) for the lock — a skipped fork upload would have no later + /// retry; in practice the fork target's lock is uncontended (its DB row is + /// created after this upload). pub async fn release_after_write(&self, owner_did: &str, repo_name: &str) { - if let Some(ref tigris) = self.object_store { - let (owner_slug, local_path) = match self.local_path(owner_did, repo_name) { - Ok(p) => p, + self.upload_locked(owner_did, repo_name, true).await; + } + + /// Upload the local repo to the object store while holding the per-repo + /// advisory lock, then release. The lock serializes the PUT against a + /// concurrent `purge-spam` that deletes the repo (row + dir + archive) under + /// the same lock, so an in-flight upload cannot resurrect a just-deleted + /// archive. Re-checks the local dir exists UNDER the lock — a purge removes + /// the dir under its lock, so a post-purge uploader finds it gone and skips. + /// Returns true iff a PUT was performed. A no-op when no store is configured. + /// + /// `wait`: fork's foreground upload waits (bounded) for the lock; the + /// background migration/init uploads pass `false` and skip on contention — + /// they self-heal (lazy migration retries on the next `acquire`, init's + /// state is re-uploaded by the first write's release). + async fn upload_locked(&self, owner_did: &str, repo_name: &str, wait: bool) -> bool { + let Some(ref store) = self.object_store else { + return false; + }; + 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 before object-store upload"); + return false; + } + }; + let guard = if wait { + match self.lock_repo_blocking(owner_did, repo_name).await { + Ok(Some(g)) => g, + Ok(None) => { + warn!(repo = %repo_name, "object-store upload skipped — repo lock still held after retry"); + return false; + } + Err(e) => { + warn!(repo = %repo_name, err = %e, "object-store upload skipped — could not acquire repo lock"); + return false; + } + } + } else { + match self.try_lock_repo(owner_did, repo_name).await { + Ok(Some(g)) => g, + Ok(None) => { + debug!(repo = %repo_name, "object-store upload skipped — repo locked by a live writer"); + return false; + } Err(e) => { - warn!(repo = %repo_name, err = %e, "rejected unsafe path in release_after_write"); - return; + warn!(repo = %repo_name, err = %e, "object-store upload skipped — could not acquire repo lock"); + return false; } - }; - 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"); } + }; + // Re-check under the lock: a purge removes the on-disk dir under its lock, + // so a post-purge uploader must find it gone and NOT recreate the archive. + if !local_path.exists() { + warn!(repo = %repo_name, "object-store upload skipped — local repo dir gone under lock (purged?)"); + guard.release().await; + return false; } + let uploaded = match store.upload(&owner_slug, repo_name, &local_path).await { + Ok(()) => true, + Err(e) => { + warn!(repo = %repo_name, err = %e, "failed to upload repo to object store"); + false + } + }; + guard.release().await; + uploaded + } + + /// Bounded-wait lock-only acquire — the fork foreground upload's counterpart + /// to `acquire_write`'s spin, without any object-store I/O. Retries + /// `try_lock_repo` with short backoff. In practice the fork target's lock is + /// uncontended (its DB row is created after the upload), so this returns on + /// the first attempt. + async fn lock_repo_blocking( + &self, + owner_did: &str, + repo_name: &str, + ) -> Result> { + for attempt in 0..30 { + if let Some(g) = self.try_lock_repo(owner_did, repo_name).await? { + return Ok(Some(g)); + } + if attempt < 29 { + tokio::time::sleep(std::time::Duration::from_millis(200)).await; + } + } + Ok(None) } /// Compute the local disk path and owner slug for a repo. @@ -830,17 +913,21 @@ mod tests { // post-lock await for the cancellation test), and `upload()` records its // calls. Serves U1's reorder test and U3's serialization tests. struct GatedStore { - exists: bool, + // `exists` is dynamic so a delete() flips it false and an upload() flips + // it true — lets U3's resurrect test assert a deleted archive stays gone. + exists: std::sync::Arc, download_gate: Option>, uploads: std::sync::Arc>>, + deletes: std::sync::Arc>>, } impl GatedStore { fn new(exists: bool) -> Self { Self { - exists, + exists: std::sync::Arc::new(std::sync::atomic::AtomicBool::new(exists)), download_gate: None, uploads: std::sync::Arc::new(std::sync::Mutex::new(Vec::new())), + deletes: std::sync::Arc::new(std::sync::Mutex::new(Vec::new())), } } } @@ -848,13 +935,14 @@ mod tests { #[async_trait::async_trait] impl crate::git::tigris::ObjectStore for GatedStore { async fn exists(&self, _o: &str, _r: &str) -> anyhow::Result { - Ok(self.exists) + Ok(self.exists.load(std::sync::atomic::Ordering::SeqCst)) } async fn upload(&self, o: &str, r: &str, _p: &std::path::Path) -> anyhow::Result<()> { self.uploads .lock() .unwrap() .push((o.to_string(), r.to_string())); + self.exists.store(true, std::sync::atomic::Ordering::SeqCst); Ok(()) } async fn download(&self, _o: &str, _r: &str, _p: &std::path::Path) -> anyhow::Result<()> { @@ -863,7 +951,13 @@ mod tests { } Ok(()) } - async fn delete(&self, _o: &str, _r: &str) -> anyhow::Result<()> { + async fn delete(&self, o: &str, r: &str) -> anyhow::Result<()> { + self.deletes + .lock() + .unwrap() + .push((o.to_string(), r.to_string())); + self.exists + .store(false, std::sync::atomic::Ordering::SeqCst); Ok(()) } } @@ -1090,4 +1184,162 @@ mod tests { // Drop must not panic. Reaching the end of the test proves it didn't. drop(rt); } + + // ── U3: archive uploads serialize under the per-repo lock (R7, R8, R9) ─── + + fn repo_dir_of(repos_dir: &std::path::Path, owner: &str, name: &str) -> std::path::PathBuf { + repos_dir + .join(owner.replace([':', '/'], "_")) + .join(format!("{name}.git")) + } + + // R7/R8/AE6: init's background upload must SKIP while a live writer holds the + // repo lock. Pre-fix (unlocked upload) it PUTs regardless -> RED. + #[sqlx::test] + async fn init_upload_skips_while_repo_locked( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: sqlx::postgres::PgConnectOptions, + ) { + let pool = pool_opts + .max_connections(5) + .connect_with(connect_opts) + .await + .unwrap(); + let tmp = tempfile::TempDir::new().unwrap(); + let ts = GatedStore::new(false); + let uploads = ts.uploads.clone(); + let store = RepoStore::new( + tmp.path().to_path_buf(), + Some(std::sync::Arc::new(ts)), + pool, + ); + let owner = "did:key:z6MkInitSkipAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let name = "initskip"; + + let held = store.try_lock_repo(owner, name).await.unwrap().unwrap(); + store.init(owner, name).await.unwrap(); + // Let the spawned upload attempt-and-skip. + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + assert!( + uploads.lock().unwrap().is_empty(), + "init's background upload must skip while the repo is locked by a live writer" + ); + held.release().await; + } + + // R8/AE6: fork's foreground upload (release_after_write) WAITS for the lock + // rather than skipping, then PUTs once after it frees. Pre-fix it PUTs + // immediately while the lock is held -> RED on the "still empty" assert. + #[sqlx::test] + async fn release_after_write_waits_then_uploads_after_lock_frees( + pool_opts: sqlx::postgres::PgPoolOptions, + connect_opts: sqlx::postgres::PgConnectOptions, + ) { + let pool = pool_opts + .max_connections(5) + .connect_with(connect_opts) + .await + .unwrap(); + let tmp = tempfile::TempDir::new().unwrap(); + let ts = GatedStore::new(false); + let uploads = ts.uploads.clone(); + let store = RepoStore::new( + tmp.path().to_path_buf(), + Some(std::sync::Arc::new(ts)), + pool, + ); + let owner = "did:key:z6MkForkWaitAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let name = "forkwait"; + let slug = owner.replace([':', '/'], "_"); + std::fs::create_dir_all(repo_dir_of(tmp.path(), owner, name)).unwrap(); + + let held = store.try_lock_repo(owner, name).await.unwrap().unwrap(); + let store2 = store.clone(); + let owner2 = owner.to_string(); + let name2 = name.to_string(); + let h = tokio::spawn(async move { store2.release_after_write(&owner2, &name2).await }); + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + assert!( + uploads.lock().unwrap().is_empty(), + "fork upload must wait (not skip) while the repo lock is held" + ); + + held.release().await; + h.await.unwrap(); + let ups = uploads.lock().unwrap(); + assert_eq!( + ups.len(), + 1, + "fork upload must PUT exactly once after the lock frees" + ); + assert_eq!(ups[0], (slug, name.to_string())); + } + + // R9/AE6: after a purge deletes the archive AND removes the on-disk dir under + // the lock, a late upload (that lost the race) must NOT resurrect the archive + // — it finds the dir gone under the lock and skips. Pre-fix (no dir recheck) + // it re-PUTs and revives the archive -> RED. + #[sqlx::test] + async fn upload_does_not_resurrect_a_purged_archive(pool: sqlx::PgPool) { + use std::sync::atomic::Ordering::SeqCst; + let tmp = tempfile::TempDir::new().unwrap(); + let ts = GatedStore::new(false); + let uploads = ts.uploads.clone(); + let deletes = ts.deletes.clone(); + let exists = ts.exists.clone(); + let store = RepoStore::new( + tmp.path().to_path_buf(), + Some(std::sync::Arc::new(ts)), + pool, + ); + let owner = "did:key:z6MkResurrectAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let name = "resurrect"; + let dir = repo_dir_of(tmp.path(), owner, name); + std::fs::create_dir_all(&dir).unwrap(); + + store.release_after_write(owner, name).await; + assert!(exists.load(SeqCst), "archive exists after the first upload"); + assert_eq!(uploads.lock().unwrap().len(), 1); + + // Simulate a purge: delete the archive and remove the on-disk dir. + store.delete_archive(owner, name).await.unwrap(); + std::fs::remove_dir_all(&dir).unwrap(); + assert_eq!(deletes.lock().unwrap().len(), 1, "archive delete recorded"); + assert!(!exists.load(SeqCst), "archive is deleted by the purge"); + + // A late upload attempt must find the dir gone and skip, not resurrect. + store.release_after_write(owner, name).await; + assert!( + !exists.load(SeqCst), + "a purged archive must stay deleted — the upload found no dir and skipped" + ); + assert_eq!( + uploads.lock().unwrap().len(), + 1, + "no second PUT after the repo dir was purged" + ); + } + + // Negative: an uncontended fork upload PUTs exactly once (the lock is free). + #[sqlx::test] + async fn uncontended_release_after_write_uploads_once(pool: sqlx::PgPool) { + let tmp = tempfile::TempDir::new().unwrap(); + let ts = GatedStore::new(false); + let uploads = ts.uploads.clone(); + let store = RepoStore::new( + tmp.path().to_path_buf(), + Some(std::sync::Arc::new(ts)), + pool, + ); + let owner = "did:key:z6MkUncontendedAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let name = "uncontended"; + std::fs::create_dir_all(repo_dir_of(tmp.path(), owner, name)).unwrap(); + + store.release_after_write(owner, name).await; + assert_eq!( + uploads.lock().unwrap().len(), + 1, + "an uncontended fork upload must PUT exactly once" + ); + } } From f3cf6fc08794c90b4df2ceeed393e2ecc070c9e1 Mon Sep 17 00:00:00 2001 From: t Date: Thu, 16 Jul 2026 10:18:15 -0500 Subject: [PATCH 24/25] fix(node): let purge-spam reach repos that exist only as archives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a multi-machine (object-store) deployment a spam repo can exist only as a Tigris archive with no local copy. Selection filtered on a ref count that returns non-empty for a missing local path, so such a repo was never a candidate and the tool it was hardened for could not reach it. Selection now distinguishes missing-local (Option::None) from a one-ref repo and admits a missing-local row as a *remote-unverified* candidate only when an object store is configured (via RepoStore::has_object_store); partition_for_delete passes those through rather than re-dropping them on the local ref count. The execute loop is unchanged — it already refreshes from the archive and rechecks emptiness under the per-repo lock — so a remote-only empty archive is deleted, one with refs is refreshed and skipped, and missing-both fails closed (refresh no-ops, the local recheck reports non-empty). Storeless deployments keep the old fail-closed behavior. Dry-run marks remote-only candidates distinctly and counts them in a new PurgeSummary field. Tests (recording object-store double): remote-only empty archive deleted, remote-only archive-with-refs skipped, missing-both fail-closed, storeless missing-local not a candidate. Load-bearing per INV-21 — reverting the store-configured admission turns the three remote-reach tests RED. --- crates/gitlawb-node/src/admin.rs | 315 ++++++++++++++++++---- crates/gitlawb-node/src/git/repo_store.rs | 7 + 2 files changed, 273 insertions(+), 49 deletions(-) diff --git a/crates/gitlawb-node/src/admin.rs b/crates/gitlawb-node/src/admin.rs index 35016424..a5c2edf5 100644 --- a/crates/gitlawb-node/src/admin.rs +++ b/crates/gitlawb-node/src/admin.rs @@ -54,6 +54,10 @@ pub struct PurgeSummary { /// survives and could be re-downloaded into a later same-owner/name repo, so /// this is tracked separately and never folded into a clean success. pub archive_failed: usize, + /// Candidates with no local copy, admitted only because an object store is + /// configured (emptiness decided under the lock at execute time). Reported so + /// the dry-run can surface them distinctly from locally-verified candidates. + pub remote_unverified: usize, } /// A repo selected for purge, with the evidence that qualified it. @@ -62,9 +66,14 @@ pub struct Candidate { pub id: String, pub owner_did: String, pub name: String, - /// Number of git refs found on disk. Always 0 for a selected candidate — kept - /// as explicit evidence in the dry-run output rather than an implicit "empty". + /// Number of git refs found on disk. 0 for a locally-verified empty candidate; + /// also 0 for a remote-unverified one whose emptiness is decided under the lock. pub ref_count: usize, + /// True when the repo has no local copy and was admitted only because an object + /// store is configured. Its emptiness has NOT been verified — the execute path + /// must refresh from the archive and recheck UNDER the per-repo lock before any + /// delete; the dry-run lists it distinctly and never touches it. + pub remote_unverified: bool, } /// Whether a DID is on the hard exclusion list. Compared under did:key @@ -82,23 +91,29 @@ fn is_excluded(owner_did: &str) -> bool { /// exclusion + empty logic is directly testable. /// /// `repos` is the raw row set to consider (the caller supplies the target DID's -/// rows). `ref_count_of` returns the number of git refs for a given repo; the CLI -/// wires the real on-disk ref source, tests inject precomputed counts. +/// rows). `local_refs_of` returns `Some(n)` when a local bare repo exists (n +/// refs) and `None` when there is no local copy; the CLI wires the real on-disk +/// source, tests inject precomputed states. `store_configured` gates whether a +/// missing-local repo may be admitted. /// /// A repo qualifies ONLY if, PER REPO: /// 1. its owner is NOT on the exclusion list (gate evaluated FIRST), AND /// 2. its owner is exactly the target burst DID, AND -/// 3. it is verified empty — `ref_count_of` returns 0. +/// 3. EITHER it is locally verified empty (`Some(0)`), OR it has no local copy +/// (`None`) AND an object store is configured — in which case it is admitted +/// as remote-unverified and its emptiness is decided under the lock later. /// -/// The exclusion gate is checked before the empty check so that an empty repo -/// owned by an excluded DID is dropped regardless of its ref signature. +/// The exclusion gate is checked before everything so an empty repo owned by an +/// excluded DID is dropped regardless of its ref signature. A missing-local repo +/// with no object store fails closed (skipped). pub fn select_spam_candidates( repos: &[RepoRecord], target_did: &str, - mut ref_count_of: F, + store_configured: bool, + mut local_refs_of: F, ) -> Vec where - F: FnMut(&RepoRecord) -> usize, + F: FnMut(&RepoRecord) -> Option, { let mut out = Vec::new(); for repo in repos { @@ -113,16 +128,28 @@ where { continue; } - // Per-repo empty check. - let ref_count = ref_count_of(repo); - if ref_count != 0 { - continue; - } + // `local_refs_of` is `Some(n)` when a local bare repo exists and `None` + // when it does not. A local empty repo (Some(0)) is a verified candidate; + // a local non-empty repo is skipped; a missing local copy is a candidate + // ONLY when an object store is configured (its emptiness is then decided + // authoritatively under the lock after refresh), else it fails closed. + let remote_unverified = match local_refs_of(repo) { + Some(0) => false, + Some(_) => continue, + None => { + if store_configured { + true + } else { + continue; + } + } + }; out.push(Candidate { id: repo.id.clone(), owner_did: repo.owner_did.clone(), name: repo.name.clone(), - ref_count, + ref_count: 0, + remote_unverified, }); } out @@ -144,12 +171,40 @@ where /// repo's refs (possibly `0`) and delete a real repo. We defend by requiring the /// bare-repo markers (`HEAD` file + `objects/` dir) before trusting any count; /// anything else fails closed (treated non-empty, skipped). -fn on_disk_ref_count(repos_dir: &Path, repo: &RepoRecord) -> usize { - ref_count_on_disk(repos_dir, &repo.owner_did, &repo.name) +/// Local ref state for selection: `Some(n)` when a local bare repo exists (n +/// refs), `None` when there is no local copy. `None` is what lets selection +/// distinguish a missing-local repo (a remote-unverified candidate when a store +/// is configured) from a one-ref repo — both of which `ref_count_on_disk` +/// collapses to a non-zero count. An unsafe name or an unreadable repo fails +/// closed to `Some(1)` so it is skipped, never admitted as remote-unverified. +fn local_refs_on_disk(repos_dir: &Path, owner_did: &str, name: &str) -> Option { + // Fail closed on an unsafe repo name BEFORE building any on-disk path (a + // peer-mirror row can carry a `../` name). Report it as non-empty so it is + // never a candidate — never as missing (which could admit it remote-unverified). + if let Err(e) = crate::git::repo_store::validate_repo_name(name) { + warn!(name = %name, err = %e, + "purge-spam: unsafe repo name — treating as non-empty (skipped)"); + return Some(1); + } + let path = store::repo_disk_path(repos_dir, owner_did, name); + if !path.join("HEAD").is_file() || !path.join("objects").is_dir() { + // No local bare repo at the expected path. + return None; + } + match store::list_refs(&path) { + Ok(refs) => Some(refs.len()), + Err(e) => { + warn!(path = %path.display(), err = %e, + "purge-spam: could not read refs — treating as non-empty (skipped)"); + Some(1) + } + } } -/// Core of [`on_disk_ref_count`], keyed on owner+name so the execute path can -/// re-verify emptiness right before deleting (using only a [`Candidate`]). +/// Ref count keyed on owner+name (returns 1 for a missing/unsafe/unreadable +/// repo — fail closed), used by the execute path to re-verify emptiness right +/// before deleting (using only a [`Candidate`]) and, after `refresh_from_archive` +/// downloads a remote-unverified candidate, to decide its emptiness under the lock. fn ref_count_on_disk(repos_dir: &Path, owner_did: &str, name: &str) -> usize { // Fail closed on an unsafe repo name BEFORE building any on-disk path. A // peer-mirror row (which skips API name validation) can carry a `../` name; @@ -233,9 +288,16 @@ pub async fn run_purge_spam( .await .context("listing repos for the spam-burst target DID")?; - let candidates = select_spam_candidates(&rows, SPAM_BURST_TARGET_DID, |repo| { - on_disk_ref_count(&repos_dir, repo) - }); + // A repo with no local copy is admitted as a remote-unverified candidate + // only when an object store is configured — its emptiness is then decided + // under the lock after refresh_from_archive. Without a store, missing-local + // fails closed (skipped), preserving the wrong-machine safety rule. + let store_configured = repo_store.has_object_store(); + let candidates = + select_spam_candidates(&rows, SPAM_BURST_TARGET_DID, store_configured, |repo| { + local_refs_on_disk(&repos_dir, &repo.owner_did, &repo.name) + }); + let remote_unverified_count = candidates.iter().filter(|c| c.remote_unverified).count(); info!( target = SPAM_BURST_TARGET_DID, @@ -257,25 +319,39 @@ pub async fn run_purge_spam( if execute { "EXECUTE" } else { "dry-run" } ); for c in &candidates { + let marker = if c.remote_unverified { + " [remote-only, emptiness verified under lock at execute]" + } else { + "" + }; println!( - " {} owner={} name={} refs={}", + " {} owner={} name={} refs={}{marker}", c.id, c.owner_did, c.name, c.ref_count ); } if !execute { println!( - "purge-spam: dry-run — nothing deleted. Re-run with --execute to delete the {} candidate(s).", + "purge-spam: dry-run — nothing deleted ({remote_unverified_count} remote-only, verified under lock only on --execute). Re-run with --execute to delete the {} candidate(s).", candidates.len() ); - return Ok(PurgeSummary::default()); + return Ok(PurgeSummary { + remote_unverified: remote_unverified_count, + ..PurgeSummary::default() + }); } // Re-verify emptiness immediately before deleting: a push may have landed - // between selection and now (TOCTOU). Anything no longer empty is skipped, - // never deleted. + // between selection and now (TOCTOU). A remote-unverified candidate has no + // local copy yet, so `ref_count_on_disk` would report it non-empty and drop + // it here — pass it straight through instead; the authoritative emptiness + // check for it happens under the lock in the execute loop after refresh. let (to_delete, to_skip) = partition_for_delete(&candidates, |c| { - ref_count_on_disk(&repos_dir, &c.owner_did, &c.name) + if c.remote_unverified { + 0 + } else { + ref_count_on_disk(&repos_dir, &c.owner_did, &c.name) + } }); for c in &to_skip { warn!(repo = %c.id, "purge-spam: repo no longer empty at delete time — skipped (TOCTOU)"); @@ -284,6 +360,7 @@ pub async fn run_purge_spam( // Execute: delete per-repo, never a single blanket "delete all of owner X". // A per-repo failure warns and continues rather than aborting the batch. let mut summary = PurgeSummary { + remote_unverified: remote_unverified_count, skipped_not_empty: to_skip.len(), ..PurgeSummary::default() }; @@ -408,12 +485,15 @@ mod tests { } /// Ref counts keyed by repo id; anything absent defaults to 0 (empty). - fn refs_by_id<'a>(map: &'a [(&'a str, usize)]) -> impl Fn(&RepoRecord) -> usize + 'a { + fn refs_by_id<'a>(map: &'a [(&'a str, usize)]) -> impl Fn(&RepoRecord) -> Option + 'a { + // A local bare repo exists for every test row (Some), with `n` refs. move |r: &RepoRecord| { - map.iter() - .find(|(id, _)| *id == r.id) - .map(|(_, n)| *n) - .unwrap_or(0) + Some( + map.iter() + .find(|(id, _)| *id == r.id) + .map(|(_, n)| *n) + .unwrap_or(0), + ) } } @@ -421,7 +501,7 @@ mod tests { #[test] fn empty_target_repo_is_a_candidate() { let repos = vec![repo("t-empty", TARGET, "spam1")]; - let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + let got = select_spam_candidates(&repos, TARGET, false, refs_by_id(&[])); assert_eq!(got.len(), 1, "empty target repo must be selected"); assert_eq!(got[0].id, "t-empty"); assert_eq!(got[0].owner_did, TARGET); @@ -439,7 +519,7 @@ mod tests { repo("t-empty", TARGET, "spam1"), repo("t-nonempty", TARGET, "real"), ]; - let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[("t-nonempty", 3)])); + let got = select_spam_candidates(&repos, TARGET, false, refs_by_id(&[("t-nonempty", 3)])); let ids: Vec<&str> = got.iter().map(|c| c.id.as_str()).collect(); assert!(ids.contains(&"t-empty"), "empty target repo still selected"); assert!( @@ -458,14 +538,14 @@ mod tests { #[test] fn empty_excluded_content_repo_is_absent() { let repos = vec![repo("x-content", EXCLUDED_CONTENT, "anything")]; - let got = select_spam_candidates(&repos, EXCLUDED_CONTENT, refs_by_id(&[])); + let got = select_spam_candidates(&repos, EXCLUDED_CONTENT, false, refs_by_id(&[])); assert!( got.is_empty(), "an empty repo owned by the excluded content DID must be excluded even \ if that DID were the target, got {got:?}" ); // And of course it is also absent when the real burst DID is the target. - let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + let got = select_spam_candidates(&repos, TARGET, false, refs_by_id(&[])); assert!(got.is_empty()); } @@ -475,13 +555,13 @@ mod tests { #[test] fn empty_intern_repo_is_absent() { let repos = vec![repo("x-intern", EXCLUDED_INTERN, "mirror")]; - let got = select_spam_candidates(&repos, EXCLUDED_INTERN, refs_by_id(&[])); + let got = select_spam_candidates(&repos, EXCLUDED_INTERN, false, refs_by_id(&[])); assert!( got.is_empty(), "an empty repo owned by the intern/mirror-bot DID must be excluded even \ if that DID were the target, got {got:?}" ); - let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + let got = select_spam_candidates(&repos, TARGET, false, refs_by_id(&[])); assert!(got.is_empty()); } @@ -490,7 +570,7 @@ mod tests { #[test] fn empty_unrelated_repo_is_absent() { let repos = vec![repo("u-empty", UNRELATED, "whatever")]; - let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[])); + let got = select_spam_candidates(&repos, TARGET, false, refs_by_id(&[])); assert!( got.is_empty(), "an empty repo owned by a non-target DID must not be selected, got {got:?}" @@ -507,7 +587,7 @@ mod tests { repo("x-intern", EXCLUDED_INTERN, "b"), // excluded, empty → out repo("u-empty", UNRELATED, "c"), // wrong owner → out ]; - let got = select_spam_candidates(&repos, TARGET, refs_by_id(&[("t-nonempty", 2)])); + let got = select_spam_candidates(&repos, TARGET, false, refs_by_id(&[("t-nonempty", 2)])); assert_eq!( got.iter().map(|c| c.id.as_str()).collect::>(), vec!["t-empty"], @@ -521,7 +601,7 @@ mod tests { #[test] fn exclusion_gate_precedes_empty_check() { let repos = vec![repo("collision", EXCLUDED_CONTENT, "spam1")]; - let got = select_spam_candidates(&repos, EXCLUDED_CONTENT, refs_by_id(&[])); + let got = select_spam_candidates(&repos, EXCLUDED_CONTENT, false, refs_by_id(&[])); assert!(got.is_empty(), "exclusion must win even on an empty repo"); } @@ -534,6 +614,7 @@ mod tests { owner_did: "o".into(), name: id.into(), ref_count: 0, + remote_unverified: false, }; let cands = vec![cand("still-empty"), cand("now-nonempty")]; // Re-check reports the second repo as no longer empty. @@ -934,19 +1015,30 @@ mod db_tests { let path = store::repo_disk_path(tmp.path(), &repo.owner_did, &repo.name); // (a) Path exists as a plain (non-git) directory under the git ancestor. - // Without the marker guard, git discovery reads the ancestor's 0 refs and - // this repo would be deleted. It must fail closed (>=1, skipped). + // Without the marker guard, git discovery would read the ancestor's 0 + // refs and this repo would be deleted. local_refs_on_disk must report no + // local repo (None) WITHOUT running list_refs — never the ancestor's 0. + // None is skipped unless a store is configured, and the under-lock + // recheck (ref_count_on_disk) fails closed on the same markers regardless. std::fs::create_dir_all(&path).unwrap(); assert_eq!( - on_disk_ref_count(tmp.path(), &repo), + local_refs_on_disk(tmp.path(), &repo.owner_did, &repo.name), + None, + "a non-git dir under a git ancestor must report no local repo, not read the ancestor's refs" + ); + assert_eq!( + ref_count_on_disk(tmp.path(), &repo.owner_did, &repo.name), 1, - "a non-git dir under a git ancestor must fail closed, not read the ancestor's refs" + "the under-lock recheck must also fail closed on a non-git dir" ); - // (b) A real empty bare repo at the same path reads 0 — a genuine candidate. + // (b) A real empty bare repo at the same path reads Some(0) — a candidate. std::fs::remove_dir_all(&path).unwrap(); store::init_bare(&path).unwrap(); - assert_eq!(on_disk_ref_count(tmp.path(), &repo), 0); + assert_eq!( + local_refs_on_disk(tmp.path(), &repo.owner_did, &repo.name), + Some(0) + ); } /// The belt-and-suspenders containment gate (`path_within`) must reject a path @@ -1008,7 +1100,12 @@ mod db_tests { // An empty repo owned by the short-form excluded DID is spared even though // its ref signature (0) otherwise matches the burst. let empty_excluded_short = rec("x-short", short, "spam"); - let cands = select_spam_candidates(&[empty_excluded_short], SPAM_BURST_TARGET_DID, |_| 0); + let cands = select_spam_candidates( + &[empty_excluded_short], + SPAM_BURST_TARGET_DID, + false, + |_| Some(0), + ); assert!( cands.is_empty(), "an empty repo owned by a short-form excluded DID must never be a candidate" @@ -1225,6 +1322,126 @@ mod db_tests { ); } + // AE3/R4: a repo that exists ONLY as an object-store archive (no local copy) + // with an EMPTY archive is reached, refreshed under the lock, and deleted + // (row + dir + archive). Pre-U4 a missing-local row was never a candidate + // (treated non-empty, skipped) -> RED; admitting it remote-unverified -> GREEN. + #[sqlx::test] + async fn purge_deletes_remote_only_empty_archive(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-remote-empty", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + // NO local repo — it exists only as an archive. + let path = store::repo_disk_path(tmp.path(), &target.owner_did, &target.name); + assert!(!path.exists(), "no local copy — remote-only"); + + let (f, deleted) = fake(true, false, false, false); // archive exists, empty + let repo_store = store_backed(tmp.path(), &pool, f); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_none(), + "a remote-only empty archive must be reached and deleted" + ); + assert!( + deleted.load(std::sync::atomic::Ordering::SeqCst), + "the archive must be deleted too" + ); + assert_eq!(summary.deleted, 1); + assert_eq!( + summary.remote_unverified, 1, + "the candidate was admitted as remote-unverified" + ); + } + + // AE4/R4: a remote-only archive that turns out to HAVE refs is refreshed under + // the lock and then skipped — never deleted on the missing-local view. + #[sqlx::test] + async fn purge_skips_remote_only_archive_with_refs(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-remote-refs", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + let path = store::repo_disk_path(tmp.path(), &target.owner_did, &target.name); + assert!(!path.exists()); + + let (f, _deleted) = fake(true, true, false, false); // archive exists, HAS refs + let repo_store = store_backed(tmp.path(), &pool, f); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_some(), + "a remote-only archive with refs must be refreshed and skipped, not deleted" + ); + assert_eq!(summary.deleted, 0); + assert_eq!(summary.skipped_not_empty, 1); + } + + // AE5/R5: no local copy AND no archive — the candidate is admitted (a store is + // configured) but the under-lock recheck finds nothing and fails closed. + #[sqlx::test] + async fn purge_skips_remote_unverified_with_no_archive(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-missing-both", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + + let (f, _deleted) = fake(false, false, false, false); // NO archive + let repo_store = store_backed(tmp.path(), &pool, f); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_some(), + "missing local AND no archive must fail closed (not deleted)" + ); + assert_eq!(summary.deleted, 0); + assert_eq!( + summary.skipped_not_empty, 1, + "skipped by the authoritative under-lock recheck" + ); + } + + // R5: with NO object store, a repo with no local copy is not even a candidate + // (the missing-local admission is gated on a configured store). + #[sqlx::test] + async fn purge_storeless_missing_local_is_not_a_candidate(pool: PgPool) { + let db = db(&pool).await; + let tmp = tempfile::TempDir::new().unwrap(); + let target = rec("t-nolocal", SPAM_BURST_TARGET_DID, "spam1"); + db.create_repo(&target).await.unwrap(); + // Storeless RepoStore, no local repo on disk. + let repo_store = test_store(tmp.path(), &pool); + let summary = run_purge_spam(&db, &repo_store, tmp.path(), true) + .await + .unwrap(); + + assert!( + db.get_repo(SPAM_BURST_TARGET_DID, "spam1") + .await + .unwrap() + .is_some(), + "storeless + missing-local must fail closed — never a candidate" + ); + assert_eq!(summary.deleted, 0); + assert_eq!(summary.remote_unverified, 0); + } + // R7: with no object store configured (single-machine), an empty repo is // deleted exactly as before and nothing touches an archive. #[sqlx::test] diff --git a/crates/gitlawb-node/src/git/repo_store.rs b/crates/gitlawb-node/src/git/repo_store.rs index 211830e0..f9a57c3a 100644 --- a/crates/gitlawb-node/src/git/repo_store.rs +++ b/crates/gitlawb-node/src/git/repo_store.rs @@ -323,6 +323,13 @@ impl RepoStore { self.upload_locked(owner_did, repo_name, true).await; } + /// Whether an object store is configured. Used by purge-spam to decide + /// whether a repo with no local copy can be a remote-unverified candidate + /// (its emptiness verified under the lock after a refresh) versus fail-closed. + pub fn has_object_store(&self) -> bool { + self.object_store.is_some() + } + /// Upload the local repo to the object store while holding the per-repo /// advisory lock, then release. The lock serializes the PUT against a /// concurrent `purge-spam` that deletes the repo (row + dir + archive) under From 2b10fa0ac7cb5f976596cc7b51c04b3409f58aa6 Mon Sep 17 00:00:00 2001 From: t Date: Thu, 16 Jul 2026 10:26:42 -0500 Subject: [PATCH 25/25] refactor(node): dedup limiter setup + write-brake test driver; fix .env wording - .env.example: the write brake wraps the whole /graphql route, so its docs now say it covers all GraphQL HTTP requests (queries and the playground GET), not only mutations, with WebSocket subscriptions called out as excluded. - main.rs: collapse the identical create/write/push per-IP limiter setup into build_ip_limiter (resolve-env, build bounded, warn-on-zero), preserving the exact limits/defaults and the present-but-unparseable warning in rate_limit_from_env; each limiter now also info-logs its configured value. - api/repos.rs: the nine per-IP write-brake tests share a send_from helper mirroring rate_limit.rs's post_from/post_with, replacing the repeated request-build + ConnectInfo + oneshot + status boilerplate. Behavior identical. --- .env.example | 7 +- crates/gitlawb-node/src/api/repos.rs | 228 ++++++++++++++------------- crates/gitlawb-node/src/main.rs | 48 +++--- 3 files changed, 147 insertions(+), 136 deletions(-) diff --git a/.env.example b/.env.example index e4593025..c0820201 100644 --- a/.env.example +++ b/.env.example @@ -143,8 +143,11 @@ GITLAWB_CREATE_RATE_LIMIT=120 # ── Write rate limiting (non-creation authenticated writes) ─────────────── # Max non-creation write requests per client IP per hour: issue/PR comments, # labels, stars, merges, protect/unprotect, replicas, visibility, tasks, -# bounties, profile, and GraphQL mutations. Its own bucket, separate from the -# creation and push brakes. Uses GITLAWB_TRUSTED_PROXY to resolve the client IP. +# bounties, profile, and all GraphQL HTTP requests. The brake wraps the whole +# /graphql route, so queries and the playground GET consume this bucket too, not +# only mutations (GraphQL WebSocket subscriptions are excluded). Its own bucket, +# separate from the creation and push brakes. Uses GITLAWB_TRUSTED_PROXY to +# resolve the client IP. # NOTE: this is a per-IP aggregate across ALL those write actions, so behind a # shared NAT/egress IP (or with GITLAWB_TRUSTED_PROXY unset) many users collapse # onto one bucket — raise this for automation-heavy or multi-user single-IP diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 3e4b3256..1656a83b 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -2807,14 +2807,35 @@ mod tests { ); } + // Shared request driver for the per-IP write-brake tests below: build a + // request with the given method/uri/headers/body, attach the socket peer as + // ConnectInfo (what the IP limiter keys on), send it through the router, and + // return the status. Mirrors post_from/post_with in rate_limit.rs. + async fn send_from( + router: &axum::Router, + method: axum::http::Method, + uri: &str, + headers: &[(&str, &str)], + body: axum::body::Body, + peer: std::net::SocketAddr, + ) -> axum::http::StatusCode { + use tower::ServiceExt; + let mut b = axum::http::Request::builder().method(method).uri(uri); + for (k, v) in headers { + b = b.header(*k, *v); + } + let mut req = b.body(body).unwrap(); + req.extensions_mut() + .insert(axum::extract::ConnectInfo(peer)); + router.clone().oneshot(req).await.unwrap().status() + } + #[sqlx::test] async fn write_route_is_rate_limited_by_ip(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; // Tiny write bucket, keyed on the socket peer (no trusted proxy). @@ -2828,14 +2849,15 @@ mod tests { let router = crate::server::build_router(state); // A write_routes sink (star). The IP brake is outermost, so the 429 // fires before auth/handler — the path only needs to match. - let mut req = Request::builder() - .method(Method::PUT) - .uri("/api/v1/repos/someowner/somerepo/star") - .body(Body::empty()) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); - - let status = router.oneshot(req).await.unwrap().status(); + let status = send_from( + &router, + Method::PUT, + "/api/v1/repos/someowner/somerepo/star", + &[], + Body::empty(), + peer, + ) + .await; assert_eq!( status, StatusCode::TOO_MANY_REQUESTS, @@ -2848,11 +2870,9 @@ mod tests { #[sqlx::test] async fn write_flood_does_not_drain_creation_budget(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; // Exhaust the write bucket for this peer; leave the creation bucket ample. @@ -2869,29 +2889,31 @@ mod tests { // Anchor the test: prove the write bucket is genuinely drained at the // router (a write sink from this peer 429s) so the creation assertion // below cannot pass vacuously on some unrelated non-429 status. - let mut wreq = Request::builder() - .method(Method::PUT) - .uri("/api/v1/repos/someowner/somerepo/star") - .body(Body::empty()) - .unwrap(); - wreq.extensions_mut().insert(ConnectInfo(peer)); assert_eq!( - router.clone().oneshot(wreq).await.unwrap().status(), + send_from( + &router, + Method::PUT, + "/api/v1/repos/someowner/somerepo/star", + &[], + Body::empty(), + peer, + ) + .await, StatusCode::TOO_MANY_REQUESTS, "write bucket must be drained for this peer (test precondition)" ); // Creation from the same peer must NOT be 429 — its bucket is untouched. // (It fails later on missing signature; the point is it is not throttled.) - let mut req = Request::builder() - .method(Method::POST) - .uri("/api/v1/repos") - .header("content-type", "application/json") - .body(Body::from(r#"{"name":"legit","is_public":true}"#)) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); - - let status = router.oneshot(req).await.unwrap().status(); + let status = send_from( + &router, + Method::POST, + "/api/v1/repos", + &[("content-type", "application/json")], + Body::from(r#"{"name":"legit","is_public":true}"#), + peer, + ) + .await; assert_ne!( status, StatusCode::TOO_MANY_REQUESTS, @@ -2903,11 +2925,9 @@ mod tests { #[sqlx::test] async fn graphql_post_is_rate_limited_by_ip(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); @@ -2917,15 +2937,15 @@ mod tests { assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); let router = crate::server::build_router(state); - let mut req = Request::builder() - .method(Method::POST) - .uri("/graphql") - .header("content-type", "application/json") - .body(Body::from(r#"{"query":"{ __typename }"}"#)) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); - - let status = router.oneshot(req).await.unwrap().status(); + let status = send_from( + &router, + Method::POST, + "/graphql", + &[("content-type", "application/json")], + Body::from(r#"{"query":"{ __typename }"}"#), + peer, + ) + .await; assert_eq!( status, StatusCode::TOO_MANY_REQUESTS, @@ -2938,11 +2958,9 @@ mod tests { #[sqlx::test] async fn issue_comment_is_rate_limited_by_ip(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); @@ -2952,15 +2970,15 @@ mod tests { assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); let router = crate::server::build_router(state); - let mut req = Request::builder() - .method(Method::POST) - .uri("/api/v1/repos/someowner/somerepo/issues/1/comments") - .header("content-type", "application/json") - .body(Body::from(r#"{"body":"flood"}"#)) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); - - let status = router.oneshot(req).await.unwrap().status(); + let status = send_from( + &router, + Method::POST, + "/api/v1/repos/someowner/somerepo/issues/1/comments", + &[("content-type", "application/json")], + Body::from(r#"{"body":"flood"}"#), + peer, + ) + .await; assert_eq!( status, StatusCode::TOO_MANY_REQUESTS, @@ -2974,11 +2992,9 @@ mod tests { #[sqlx::test] async fn under_limit_write_is_not_throttled(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; // Ample budget; bucket NOT exhausted. @@ -2988,14 +3004,16 @@ mod tests { let peer: SocketAddr = "203.0.113.150:7000".parse().unwrap(); let router = crate::server::build_router(state); - let mut req = Request::builder() - .method(Method::PUT) - .uri("/api/v1/repos/someowner/somerepo/star") - .body(Body::empty()) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); assert_ne!( - router.oneshot(req).await.unwrap().status(), + send_from( + &router, + Method::PUT, + "/api/v1/repos/someowner/somerepo/star", + &[], + Body::empty(), + peer, + ) + .await, StatusCode::TOO_MANY_REQUESTS, "an under-limit write must pass the brake, not be 429'd" ); @@ -3006,11 +3024,9 @@ mod tests { #[sqlx::test] async fn write_rate_limit_zero_disables_the_brake(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; state.write_rate_limiter = crate::rate_limit::RateLimiter::new(0, Duration::from_secs(60)); @@ -3019,14 +3035,16 @@ mod tests { let router = crate::server::build_router(state); for _ in 0..5 { - let mut req = Request::builder() - .method(Method::PUT) - .uri("/api/v1/repos/someowner/somerepo/star") - .body(Body::empty()) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); assert_ne!( - router.clone().oneshot(req).await.unwrap().status(), + send_from( + &router, + Method::PUT, + "/api/v1/repos/someowner/somerepo/star", + &[], + Body::empty(), + peer, + ) + .await, StatusCode::TOO_MANY_REQUESTS, "a 0 write limit must disable the brake" ); @@ -3038,11 +3056,9 @@ mod tests { #[sqlx::test] async fn task_bounty_profile_writes_are_rate_limited(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); @@ -3056,15 +3072,16 @@ mod tests { (Method::POST, "/api/v1/repos/o/r/bounties"), (Method::PUT, "/api/v1/profile"), ] { - let mut req = Request::builder() - .method(method) - .uri(uri) - .header("content-type", "application/json") - .body(Body::from("{}")) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); assert_eq!( - router.clone().oneshot(req).await.unwrap().status(), + send_from( + &router, + method, + uri, + &[("content-type", "application/json")], + Body::from("{}"), + peer, + ) + .await, StatusCode::TOO_MANY_REQUESTS, "write group {uri} must be IP-throttled by the write brake" ); @@ -3076,11 +3093,9 @@ mod tests { #[sqlx::test] async fn graphql_ws_is_not_braked(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; state.write_rate_limiter = crate::rate_limit::RateLimiter::new(1, Duration::from_secs(60)); @@ -3089,16 +3104,18 @@ mod tests { assert!(state.write_rate_limiter.check(&peer.ip().to_string()).await); let router = crate::server::build_router(state); - let mut req = Request::builder() - .method(Method::GET) - .uri("/graphql/ws") - .body(Body::empty()) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); // Not a real ws upgrade, so the subscription service rejects it with some // non-429 status; the point is the write brake never sees it. assert_ne!( - router.oneshot(req).await.unwrap().status(), + send_from( + &router, + Method::GET, + "/graphql/ws", + &[], + Body::empty(), + peer + ) + .await, StatusCode::TOO_MANY_REQUESTS, "/graphql/ws must not be behind the write brake" ); @@ -3110,11 +3127,9 @@ mod tests { #[sqlx::test] async fn every_write_group_passes_under_limit(pool: sqlx::PgPool) { use axum::body::Body; - use axum::extract::ConnectInfo; - use axum::http::{Method, Request, StatusCode}; + use axum::http::{Method, StatusCode}; use std::net::SocketAddr; use std::time::Duration; - use tower::ServiceExt; let mut state = crate::test_support::test_state(pool).await; state.write_rate_limiter = @@ -3131,15 +3146,16 @@ mod tests { (Method::POST, "/api/v1/repos/o/r/bounties"), (Method::PUT, "/api/v1/profile"), ] { - let mut req = Request::builder() - .method(method) - .uri(uri) - .header("content-type", "application/json") - .body(Body::from(r#"{"query":"{ __typename }"}"#)) - .unwrap(); - req.extensions_mut().insert(ConnectInfo(peer)); assert_ne!( - router.clone().oneshot(req).await.unwrap().status(), + send_from( + &router, + method, + uri, + &[("content-type", "application/json")], + Body::from(r#"{"query":"{ __typename }"}"#), + peer, + ) + .await, StatusCode::TOO_MANY_REQUESTS, "under-limit write to {uri} must pass the brake, not 429" ); diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index f85eaded..83656be1 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -303,15 +303,7 @@ async fn main() -> Result<()> { // capped regardless of how many identities it mints. Sized well above any // legitimate per-IP creation rate; GITLAWB_CREATE_RATE_LIMIT overrides, 0 // disables. Bounded key set — the key is a client-influenced IP. - let create_limit = rate_limit_from_env("GITLAWB_CREATE_RATE_LIMIT", 120); - let create_ip_rate_limiter = rate_limit::RateLimiter::new_bounded( - create_limit, - std::time::Duration::from_secs(3600), - 200_000, - ); - if create_limit == 0 { - tracing::warn!("GITLAWB_CREATE_RATE_LIMIT=0 — per-IP creation rate limiting disabled"); - } + let create_ip_rate_limiter = build_ip_limiter("GITLAWB_CREATE_RATE_LIMIT", 120, "creation"); // Per-IP brake for the authenticated non-creation write surface (issue/PR // comments, labels, stars, merges, protect, replicas, visibility, tasks, @@ -321,30 +313,14 @@ async fn main() -> Result<()> { // any legitimate per-IP write rate — real agent automation makes many small // writes per hour. GITLAWB_WRITE_RATE_LIMIT overrides; 0 disables. Bounded // key set — the key is a client-influenced IP. - let write_limit = rate_limit_from_env("GITLAWB_WRITE_RATE_LIMIT", 600); - let write_rate_limiter = rate_limit::RateLimiter::new_bounded( - write_limit, - std::time::Duration::from_secs(3600), - 200_000, - ); - if write_limit == 0 { - tracing::warn!("GITLAWB_WRITE_RATE_LIMIT=0 — per-IP write rate limiting disabled"); - } + let write_rate_limiter = build_ip_limiter("GITLAWB_WRITE_RATE_LIMIT", 600, "write"); // Push-path flood brake: max git-receive-pack requests per client IP per // hour (counts both the info/refs advertisement and the push POST). Sized // for heavy agent automation while still stopping flood traffic (the June // 2026 attack pushed several times per second per IP). GITLAWB_PUSH_RATE_LIMIT // overrides; 0 disables. Bounded key set — the key is a client-influenced IP. - let push_limit = rate_limit_from_env("GITLAWB_PUSH_RATE_LIMIT", 600); - let push_rate_limiter = rate_limit::RateLimiter::new_bounded( - push_limit, - std::time::Duration::from_secs(3600), - 200_000, - ); - if push_limit == 0 { - tracing::warn!("GITLAWB_PUSH_RATE_LIMIT=0 — per-IP push rate limiting disabled"); - } + let push_rate_limiter = build_ip_limiter("GITLAWB_PUSH_RATE_LIMIT", 600, "push"); // Which forwarded header the edge is trusted to set. Default None (trust // nothing, key on the socket peer). Fly nodes set GITLAWB_TRUSTED_PROXY=fly; @@ -352,7 +328,7 @@ async fn main() -> Result<()> { let push_limiter_trust = rate_limit::TrustedProxy::from_env_value( &std::env::var("GITLAWB_TRUSTED_PROXY").unwrap_or_default(), ); - tracing::info!(trust = ?push_limiter_trust, push_limit, "push rate limiter configured"); + tracing::info!(trust = ?push_limiter_trust, "push rate limiter trust configured"); // Peer-sync flood brakes, keyed on the resolved client IP (per-DID is useless // here — a did:key farm self-registers). Two buckets so an unsigned notify @@ -612,6 +588,22 @@ fn rate_limit_from_env(var: &str, default: usize) -> usize { value } +/// Build a bounded per-client-IP rate limiter from an env var: resolve the limit +/// (honoring the present-but-unparseable warning in `rate_limit_from_env`), build +/// the limiter with the shared 1-hour window and 200k-key cap, and warn when the +/// limit is 0 (disabled). Collapses the identical create/write/push setup. +fn build_ip_limiter(var: &str, default: usize, label: &str) -> rate_limit::RateLimiter { + let limit = rate_limit_from_env(var, default); + let limiter = + rate_limit::RateLimiter::new_bounded(limit, std::time::Duration::from_secs(3600), 200_000); + if limit == 0 { + tracing::warn!("{var}=0 — per-IP {label} rate limiting disabled"); + } else { + tracing::info!(%var, limit, "per-IP {label} rate limiter configured"); + } + limiter +} + /// Dispatch an admin subcommand. Connects the database directly (no server, no /// p2p, no metrics) and runs the requested tool to completion, then exits. async fn run_admin_command(command: config::Command, config: &Config) -> Result<()> {