fix(node,concurrency): resolve #174 code-review findings

Review of the served-git concurrency cap found no P0/P1; these are the
verified P2 follow-ups.

config: the max_concurrent_git_ops doc overclaimed that "every capped path
is duration-bounded." The rev-list object enumeration in the withheld-blob
path still runs in an uncancellable spawn_blocking, so a stuck rev-list can
hold its slot until git exits. Scope the guarantee to the streaming stages
and name the residual. Also tighten the fairness claim: the receive-pack
advertisement shares the read pool (a shed advertisement is a cheap retryable
GET); only the push POST is on the isolated write pool.

api/repos: add info_refs_per_caller_cap_keys_on_did_not_ip, the missing
handler proof that a signed caller is keyed by its DID, not its source IP.
Filling the DID slot sheds a request from a free IP; collapsing read_caller_key
to its IP arm turns the assertion green-not-503 (mutation-verified RED).

api/repos: extract acquire_read_caller_permit so both read handlers share one
shed path instead of a duplicated match block.

rate_limit: recover from a poisoned PerCallerConcurrency mutex instead of
panicking. The critical section is pure counter arithmetic and cannot poison
the lock, but a panic there would brick the limiter for every caller.

505 tests pass; clippy -D warnings and fmt clean.

Part of #174.

Author: t

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

@@ -575,18 +575,12 @@ pub async fn git_info_refs(
         peer,
         state.push_limiter_trust,
     );
-    let _caller_permit = match &caller_key {
-        Some(k) => match state.git_read_per_caller.try_acquire(k) {
-            Some(p) => Some(p),
-            None => {
-                tracing::warn!(repo = %name, caller = %k, "per-caller read cap reached; shedding info/refs with 503");
-                return Err(AppError::Overloaded(
-                    "git read service at capacity for this caller, retry shortly".into(),
-                ));
-            }
-        },
-        None => None,
-    };
+    let _caller_permit = acquire_read_caller_permit(
+        &state.git_read_per_caller,
+        caller_key.as_deref(),
+        name,
+        "info/refs",
+    )?;
 
     // For receive-pack (push), download the latest from Tigris so the client
     // sees the same refs that acquire_write() will operate on.
@@ -656,6 +650,30 @@ fn read_caller_key(
     }
 }
 
+/// Acquire the per-caller read sub-cap permit (#174), or shed with a 503. `key` is
+/// `None` when no caller key resolves — that request is bounded by the global read
+/// pool only and is never shed here (returns `Ok(None)`). `handler` labels the shed
+/// log line. Shared by both read handlers so the two acquire sites cannot drift.
+fn acquire_read_caller_permit(
+    limiter: &crate::rate_limit::PerCallerConcurrency,
+    key: Option<&str>,
+    repo: &str,
+    handler: &str,
+) -> Result<Option<crate::rate_limit::PerCallerPermit>> {
+    match key {
+        Some(k) => match limiter.try_acquire(k) {
+            Some(p) => Ok(Some(p)),
+            None => {
+                tracing::warn!(repo = %repo, caller = %k, handler, "per-caller read cap reached; shedding with 503");
+                Err(AppError::Overloaded(
+                    "git read service at capacity for this caller, retry shortly".into(),
+                ))
+            }
+        },
+        None => Ok(None),
+    }
+}
+
 /// Map an error from a `smart_http` git service call to the right `AppError`:
 /// [`smart_http::GitServiceTimeout`] to 504, a malformed client request to 400,
 /// anything else to a 500 git error. Pure (no logging) so it is unit-testable;
@@ -712,18 +730,12 @@ pub async fn git_upload_pack(
     // visibility-denied caller never consumes a scarce read slot. Keyed per-DID
     // when signed, else per-source-IP; no resolvable key -> global read pool only.
     let caller_key = read_caller_key(caller, &headers, peer, state.push_limiter_trust);
-    let _caller_permit = match &caller_key {
-        Some(k) => match state.git_read_per_caller.try_acquire(k) {
-            Some(p) => Some(p),
-            None => {
-                tracing::warn!(repo = %name, caller = %k, "per-caller read cap reached; shedding upload-pack with 503");
-                return Err(AppError::Overloaded(
-                    "git read service at capacity for this caller, retry shortly".into(),
-                ));
-            }
-        },
-        None => None,
-    };
+    let _caller_permit = acquire_read_caller_permit(
+        &state.git_read_per_caller,
+        caller_key.as_deref(),
+        name,
+        "upload-pack",
+    )?;
 
     let disk_path = state
         .repo_store
@@ -2798,6 +2810,73 @@ mod tests {
         );
     }
 
+    /// #174 SC2 (per-DID key): a SIGNED caller is keyed by its DID, not its source
+    /// IP. Fill the DID's single read slot, then drive a request carrying that
+    /// `AuthenticatedDid` from an IP whose own slot is free — it must shed 503,
+    /// proving the key is the DID. A second request from the SAME IP but a DIFFERENT
+    /// DID is not shed. Collapse `read_caller_key` to its IP arm and the first
+    /// assertion goes green-not-503 (the IP slot is free) — this is the per-DID half
+    /// of the keying proof that the per-IP probes above do not cover.
+    #[sqlx::test]
+    async fn info_refs_per_caller_cap_keys_on_did_not_ip(pool: sqlx::PgPool) {
+        use axum::body::Body;
+        use axum::extract::ConnectInfo;
+        use axum::http::{Method, Request, StatusCode};
+        use std::net::SocketAddr;
+        use tower::ServiceExt;
+
+        let mut state = crate::test_support::test_state(pool).await;
+        state.git_read_per_caller = crate::rate_limit::PerCallerConcurrency::new(1, 100);
+        state.push_limiter_trust = crate::rate_limit::TrustedProxy::None;
+        state
+            .db
+            .upsert_mirror_repo("z6pcdid", "pc", "/tmp/pc-nonexistent", None, false)
+            .await
+            .unwrap();
+
+        let did_a = "did:key:z6MkPerCallerKeyingProofDidAAAAAAAAAAAAAAAA";
+        let did_b = "did:key:z6MkPerCallerKeyingProofDidBBBBBBBBBBBBBBBB";
+        let peer: SocketAddr = "203.0.113.51:5000".parse().unwrap();
+
+        // Fill DID_A's single read slot; the IP's own slot is left untouched.
+        let _slot = state
+            .git_read_per_caller
+            .try_acquire(did_a)
+            .expect("first slot for this DID");
+
+        // Signed as DID_A, from `peer` (whose IP slot is free): keyed by DID -> shed.
+        let router = crate::server::build_router(state.clone());
+        let mut req = Request::builder()
+            .method(Method::GET)
+            .uri("/z6pcdid/pc/info/refs?service=git-upload-pack")
+            .body(Body::empty())
+            .unwrap();
+        req.extensions_mut().insert(ConnectInfo(peer));
+        req.extensions_mut()
+            .insert(crate::auth::AuthenticatedDid(did_a.to_string()));
+        assert_eq!(
+            router.oneshot(req).await.unwrap().status(),
+            StatusCode::SERVICE_UNAVAILABLE,
+            "a signed caller at its per-DID read cap must shed with 503 (keyed by DID, not the free IP slot)"
+        );
+
+        // Same IP, DIFFERENT DID -> its own budget -> not shed by DID_A's saturation.
+        let router2 = crate::server::build_router(state.clone());
+        let mut req2 = Request::builder()
+            .method(Method::GET)
+            .uri("/z6pcdid/pc/info/refs?service=git-upload-pack")
+            .body(Body::empty())
+            .unwrap();
+        req2.extensions_mut().insert(ConnectInfo(peer));
+        req2.extensions_mut()
+            .insert(crate::auth::AuthenticatedDid(did_b.to_string()));
+        assert_ne!(
+            router2.oneshot(req2).await.unwrap().status(),
+            StatusCode::SERVICE_UNAVAILABLE,
+            "a different DID from the same IP must not be shed by another DID's saturated budget"
+        );
+    }
+
     /// #174 SC2 (None-key): a request with no resolvable caller key (no ConnectInfo,
     /// no trusted header) must NOT be shed by the per-caller cap even when another
     /// caller's budget is full — it is bounded by the global read pool only. A None

crates/gitlawb-node/src/config.rs

@@ -244,19 +244,23 @@ pub struct Config {
     /// pack-objects + threads). Size below the process budget with headroom.
     ///
     /// This is the READ pool (`git_read_semaphore`): upload-pack and both info/refs
-    /// advertisements. Authenticated pushes draw from a separate write pool
-    /// (`max_concurrent_git_pushes`) and each caller is additionally bounded by
-    /// `max_concurrent_reads_per_caller`, so an anonymous flood can neither shed a
-    /// push nor monopolize reads (#174).
+    /// advertisements. The authenticated push POST draws from a separate write pool
+    /// (`max_concurrent_git_pushes`) that anonymous reads can never reach, and each
+    /// caller is additionally bounded by `max_concurrent_reads_per_caller`, so an
+    /// anonymous flood cannot shed the actual push nor monopolize reads (#174). (The
+    /// receive-pack advertisement itself shares the read pool; a shed advertisement
+    /// is a cheap retryable GET, and the write POST it precedes always has capacity.)
     ///
-    /// A permit is held for the whole op, and every capped path is now
+    /// A permit is held for the whole op. Every git subprocess that STREAMS is
     /// duration-bounded and reaps its process group on disconnect: upload-pack,
     /// receive-pack, and both info/refs advertisements run under
     /// `git_service_timeout_secs` with `process_group(0)` teardown, and the
     /// withheld-blob (`upload_pack_excluding`) pack-objects stage runs on the async
-    /// side under the same teardown. So a hung git can no longer pin a slot and a
-    /// client disconnect no longer orphans its git child (#174 closed the two
-    /// duration/cancellation gaps this comment previously tracked as follow-up).
+    /// side under the same teardown (#174 closed the two duration/cancellation gaps
+    /// this comment previously tracked). The one remaining blocking stage is the
+    /// `rev-list` object enumeration in the withheld-blob path — a bounded walk that
+    /// terminates, run off the async runtime; it is not process-group-reaped on
+    /// disconnect, so a stuck rev-list can still hold its slot until git exits.
     ///
     /// Default: 128. Must be between 1 and 1_048_576; the ceiling keeps the value
     /// well under tokio's `Semaphore` permit limit so an oversized value is a

crates/gitlawb-node/src/rate_limit.rs

@@ -178,7 +178,12 @@ impl PerCallerConcurrency {
     /// `None` (shed) otherwise. Reject-before-insert: a new key at `max_keys` is
     /// rejected without allocating.
     pub fn try_acquire(&self, key: &str) -> Option<PerCallerPermit> {
-        let mut map = self.state.lock().expect("per-caller mutex poisoned");
+        // Recover from a poisoned lock rather than panicking: the critical section
+        // is pure counter arithmetic and cannot itself panic, so a poisoned mutex
+        // would only ever come from an unrelated abort, and a slightly-off count
+        // self-heals as permits drop. A panic here would instead brick the limiter
+        // for every caller (each subsequent lock re-panics).
+        let mut map = self.state.lock().unwrap_or_else(|e| e.into_inner());
         match map.get_mut(key) {
             Some(count) => {
                 if *count >= self.per_caller {
@@ -207,7 +212,7 @@ impl PerCallerConcurrency {
 
 impl Drop for PerCallerPermit {
     fn drop(&mut self) {
-        let mut map = self.state.lock().expect("per-caller mutex poisoned");
+        let mut map = self.state.lock().unwrap_or_else(|e| e.into_inner());
         if let Some(count) = map.get_mut(&self.key) {
             *count -= 1;
             if *count == 0 {