fix(node,git): bound a hung served git with a total-duration timeout (#62) (#165)

* feat(node,git): bound a hung served git with a total-duration timeout (#62)

Part 1 of #62. A served git upload-pack/receive-pack that neither finishes
nor disconnects parks forever, pinning a git PID (and, for receive-pack,
holding the write lock); on a public repo an anonymous caller can trigger
it. No tower TimeoutLayer exists and axum imposes no default.

Wrap the child interaction in tokio::time::timeout, bounded by a new
GITLAWB_GIT_SERVICE_TIMEOUT_SECS knob (default 600, must be positive).
Drain stdout/stderr concurrently with the stdin write so a large body
can't deadlock. On expiry run_git_service returns a typed GitServiceTimeout
and reaps the WHOLE process group before returning (SIGTERM so git clears
its .git/*.lock, wait for the leader and its grandchildren like index-pack
to exit, escalate to SIGKILL past a grace, hard-capped), so a caller
releasing the receive-pack write lock can't race a still-live git on the
same repo.

The handlers map GitServiceTimeout to a distinct 504 (not the generic 500
git error, and clear of the read-gate 404 / auth 401 the client keys
retries on) via a pure, unit-testable classifier rather than matching the
anyhow string. Surface git's own non-zero exit (its stderr) before a
stdin-write EPIPE so a malformed body is classified 400, not 500.

Out of scope, deferred in #62: the info/refs advertisement and the
withheld-blob path (spawn_blocking, which tokio timeout cannot cancel);
both remain unbounded (noted in the config knob's docs).

Tests: the timeout fires and tears the group down; it reaps the whole
group before returning (RED if the reap is leader-only); the wrap is
load-bearing (RED via an outer bound, not a hang); the error must be the
typed timeout; the 504 mapping and classifier are unit-tested; config
rejects 0. Full suite green.

* fix(review): harden #62 timeout tests and drop a dead reap flag

- test(node): cover the `protocol error` arm of git_service_app_error
  independently (the input has no "bad line length" substring, so it
  isolates the second classifier arm and goes RED if that arm is removed)
- test(node): make the reap-before-return test's grandchild IGNORE SIGTERM
  (`trap "" TERM`) and outlive the ~4s reap cap, so the SIGKILL escalation
  is load-bearing. Previously the grandchild self-exited under the cap, so
  neutering the SIGKILL still passed; the escalation was exercised but not
  required. Verified: RED with SIGKILL disabled, GREEN with it.
- test(node): add a receive-pack timeout test. Every prior timeout test used
  an upload-pack fake; this proves the push path (which also holds the repo
  write lock) is bounded too. RED-verified with the internal timeout removed.
- test(node): poll for the pidfile in the reap-before-return timeout test
  so a loaded runner can't false-panic before the fake git writes it
- remove the redundant `sigkilled` guard in reap_group_on_timeout; step
  == 200 fires exactly once in the 0..400 loop, so no re-entry guard is
  needed and the SIGKILL still fires exactly once

* fix(node): log malformed receive-pack as warn, not error (#165)

git_receive_pack's error classifier routed every non-Timeout error,
including client-caused BadRequest (400), through tracing::error!. Mirror
the git_upload_pack arm so a malformed push logs at warn level instead of
as a server error.

* test(node,git): make smart_http timeout tests pass under the parallel runner (#165)

Addresses jatmn's review of 41eca0b.

[P2] The #62 timeout tests failed under `cargo test --workspace`:
- run_git_service_tears_down_group_when_future_dropped re-polled a completed
  future ("async fn resumed after completion"): the advance-until-pids loop
  ignored the timeout's Ok(_) (future-finished) case and polled the resolved
  future again. It now breaks on early completion.
- Under fork-storm load a freshly-written fake `git` transiently fails to exec
  (ETXTBSY) or is timed out before recording its pids. Added a
  fake_git_run_with_pids retry helper (used by the four pid-reading tests) plus
  a matching inline retry on the drop test, with growing backoff for the
  correlated bursts and a 12-attempt cap that still fails loudly on a genuine
  never-spawns regression. Reverted two tests from a speculative 2000ms bound
  back to 300ms/1000ms now that the retry covers the timeout-vs-pidfile race.

[P3] Documented GITLAWB_GIT_SERVICE_TIMEOUT_SECS in .env.example and the README
config table (default 600; bounds upload-pack/receive-pack, not info/refs or the
withheld-blob path).

The tests stay load-bearing: breaking process_group(0), the post-reap disarm,
the timeout-arm reap, or the internal timeout each still turns the corresponding
test red (mutation-verified).

---------

Co-authored-by: t <t@t>

Author: beardthelion

.env.example

@@ -90,6 +90,12 @@ GITLAWB_PUBLIC_READ=true
 # Maximum git smart-HTTP pack request size, in bytes.
 GITLAWB_MAX_PACK_BYTES=2147483648
 
+# Max seconds a served git upload-pack / receive-pack (clone / push) may run
+# before it is aborted with a 504. Bounds a hung git that would otherwise pin a
+# worker and, on push, the repo write lock. Does NOT cover the info/refs
+# advertisement or the withheld-blob path, which remain unbounded. Default 600.
+GITLAWB_GIT_SERVICE_TIMEOUT_SECS=600
+
 # ── Push rate limiting (git-receive-pack flood brake) ─────────────────────
 # Max receive-pack requests (info/refs advertisement + push POST) per client
 # IP per hour. 0 disables. Default 600.

README.md

@@ -343,6 +343,7 @@ Important node settings:
 | `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require signed peer announce/sync writes. |
 | `GITLAWB_AUTO_SYNC` | Enable automatic sync from known peers. |
 | `GITLAWB_MAX_PACK_BYTES` | Max git pack body size for smart-HTTP routes. |
+| `GITLAWB_GIT_SERVICE_TIMEOUT_SECS` | Max seconds a served git upload-pack/receive-pack may run before it is aborted (504). Default 600. Does not bound `info/refs` or the withheld-blob path. |
 | `GITLAWB_TIGRIS_BUCKET` | Optional S3/Tigris shared repo storage bucket. |
 | `GITLAWB_PINATA_JWT` | Optional Pinata/IPFS warm-storage pinning. |
 | `GITLAWB_IRYS_URL` | Optional Irys/Arweave permanent anchoring. |

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

@@ -578,6 +578,26 @@ pub async fn git_info_refs(
         })
 }
 
+/// 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;
+/// callers add their own tracing.
+fn git_service_app_error(err: &anyhow::Error) -> AppError {
+    if err
+        .downcast_ref::<smart_http::GitServiceTimeout>()
+        .is_some()
+    {
+        AppError::Timeout("git service timed out".into())
+    } else {
+        let msg = err.to_string();
+        if msg.contains("bad line length") || msg.contains("protocol error") {
+            AppError::BadRequest(msg)
+        } else {
+            AppError::Git(msg)
+        }
+    }
+}
+
 /// POST /:owner/:repo.git/git-upload-pack
 pub async fn git_upload_pack(
     State(state): State<AppState>,
@@ -615,8 +635,9 @@ pub async fn git_upload_pack(
     // No path-scoped rule can withhold an individual blob, and the whole-repo
     // "/" gate above already enforced repo-level access. Skip the per-blob
     // withheld walk and serve the pack directly.
+    let git_timeout = std::time::Duration::from_secs(state.config.git_service_timeout_secs);
     let resp = if !visibility_pack::has_path_scoped_rule(&rules) {
-        smart_http::upload_pack(&disk_path, body).await
+        smart_http::upload_pack(&disk_path, body, git_timeout).await
     } else {
         // withheld_blob_oids walks every ref with blocking `git ls-tree`; keep
         // that off the async worker thread.
@@ -641,21 +662,22 @@ pub async fn git_upload_pack(
         };
 
         if withheld.is_empty() {
-            smart_http::upload_pack(&disk_path, body).await
+            smart_http::upload_pack(&disk_path, body, git_timeout).await
         } else {
             tracing::info!(repo = %name, caller = ?caller, withheld = withheld.len(), "serving filtered pack");
             smart_http::upload_pack_excluding(&disk_path, body, &withheld).await
         }
     }
     .map_err(|e| {
-        let msg = e.to_string();
-        if msg.contains("bad line length") || msg.contains("protocol error") {
-            tracing::warn!(repo = %name, err = %msg, "git-upload-pack: bad client request");
-            AppError::BadRequest(msg)
-        } else {
-            tracing::error!(repo = %name, err = %msg, "git-upload-pack failed");
-            AppError::Git(msg)
+        let app = git_service_app_error(&e);
+        match &app {
+            AppError::Timeout(_) => tracing::warn!(repo = %name, "git-upload-pack timed out"),
+            AppError::BadRequest(msg) => {
+                tracing::warn!(repo = %name, err = %msg, "git-upload-pack: bad client request")
+            }
+            _ => tracing::error!(repo = %name, err = %e, "git-upload-pack failed"),
         }
+        app
     })?;
     crate::metrics::record_fetch(&format!("{owner}/{name}"));
     crate::metrics::observe_pack_size(body_len as f64);
@@ -902,16 +924,24 @@ pub async fn git_receive_pack(
     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 receive_result = smart_http::receive_pack(&disk_path, body).await;
+    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;
 
     let result = receive_result.map_err(|e| {
-        tracing::error!(repo = %name, err = %e, "git receive-pack failed");
-        AppError::Git(e.to_string())
+        let app = git_service_app_error(&e);
+        match &app {
+            AppError::Timeout(_) => tracing::warn!(repo = %name, "git receive-pack timed out"),
+            AppError::BadRequest(msg) => {
+                tracing::warn!(repo = %name, err = %msg, "git receive-pack: bad client request")
+            }
+            _ => tracing::error!(repo = %name, err = %e, "git receive-pack failed"),
+        }
+        app
     })?;
 
     // Update the repo's updated_at timestamp after a successful push
@@ -1795,6 +1825,32 @@ mod tests {
     const OWNER_SHORT: &str = "z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH";
     const STRANGER_DID: &str = "did:key:z6Mkffonly5tranger0000000000000000000000000000000";
 
+    #[test]
+    fn git_service_app_error_classifies_timeout_bad_request_and_git() {
+        // GitServiceTimeout carried through anyhow -> 504 Timeout.
+        let timeout_err: anyhow::Error = smart_http::GitServiceTimeout.into();
+        assert!(matches!(
+            git_service_app_error(&timeout_err),
+            AppError::Timeout(_)
+        ));
+        // A malformed client request -> 400.
+        let bad = anyhow::anyhow!("fatal: bad line length character: 0000");
+        assert!(matches!(
+            git_service_app_error(&bad),
+            AppError::BadRequest(_)
+        ));
+        // The `protocol error` marker (with no "bad line length" substring) also
+        // -> 400, exercising the second arm of the classifier independently.
+        let proto = anyhow::anyhow!("fatal: protocol error: unexpected flush packet");
+        assert!(matches!(
+            git_service_app_error(&proto),
+            AppError::BadRequest(_)
+        ));
+        // Anything else -> 500 git error.
+        let other = anyhow::anyhow!("some other git failure");
+        assert!(matches!(git_service_app_error(&other), AppError::Git(_)));
+    }
+
     fn repo_owned_by(owner_did: &str) -> crate::db::RepoRecord {
         let now = chrono::Utc::now();
         crate::db::RepoRecord {

crates/gitlawb-node/src/config.rs

@@ -166,6 +166,22 @@ pub struct Config {
     /// in flight and exits. Default: 30s.
     #[arg(long, env = "GITLAWB_SHUTDOWN_GRACE_SECS", default_value_t = 30)]
     pub shutdown_grace_secs: u64,
+
+    /// Maximum wall-clock time a single served git operation (upload-pack /
+    /// receive-pack through `run_git_service`) may run before it is aborted and
+    /// its process group torn down, in seconds. Bounds a git that neither
+    /// finishes nor disconnects. Must be positive; set it very large to
+    /// effectively disable the bound. Default: 600s (10 min), generous for large
+    /// clones. Does not cover the ref advertisement (`info/refs`) or the
+    /// withheld-blob fetch path (`upload_pack_excluding`, a blocking
+    /// `spawn_blocking` a tokio timeout cannot cancel); both remain unbounded.
+    #[arg(
+        long,
+        env = "GITLAWB_GIT_SERVICE_TIMEOUT_SECS",
+        default_value_t = 600,
+        value_parser = clap::value_parser!(u64).range(1..)
+    )]
+    pub git_service_timeout_secs: u64,
 }
 
 impl Config {
@@ -183,3 +199,25 @@ impl Config {
         PathBuf::from(&self.key_path)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn git_service_timeout_defaults_to_600_and_rejects_zero() {
+        assert_eq!(
+            Config::parse_from(["gitlawb-node"]).git_service_timeout_secs,
+            600
+        );
+        assert_eq!(
+            Config::parse_from(["gitlawb-node", "--git-service-timeout-secs", "30"])
+                .git_service_timeout_secs,
+            30
+        );
+        // 0 is a footgun (immediate-504 on every request); clap must reject it.
+        assert!(
+            Config::try_parse_from(["gitlawb-node", "--git-service-timeout-secs", "0"]).is_err()
+        );
+    }
+}

crates/gitlawb-node/src/error.rs

@@ -44,6 +44,9 @@ pub enum AppError {
     #[error("git error: {0}")]
     Git(String),
 
+    #[error("git service timed out: {0}")]
+    Timeout(String),
+
     #[error("database error: {0}")]
     Db(#[from] sqlx::Error),
 
@@ -105,6 +108,9 @@ impl IntoResponse for AppError {
                 (StatusCode::UNPROCESSABLE_ENTITY, "incomplete", msg.clone())
             }
             AppError::Git(msg) => (StatusCode::INTERNAL_SERVER_ERROR, "git_error", msg.clone()),
+            // 504, distinct from the 500 git_error and from the read-gate's 404 /
+            // the auth 401, so the client can tell a deadline from a failure.
+            AppError::Timeout(msg) => (StatusCode::GATEWAY_TIMEOUT, "git_timeout", msg.clone()),
             AppError::Db(e) => (StatusCode::INTERNAL_SERVER_ERROR, "db_error", e.to_string()),
             AppError::Internal(e) => (
                 StatusCode::INTERNAL_SERVER_ERROR,
@@ -123,3 +129,21 @@ impl IntoResponse for AppError {
 }
 
 pub type Result<T> = std::result::Result<T, AppError>;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn timeout_maps_to_504_distinct_from_git_500() {
+        assert_eq!(
+            AppError::Timeout("x".into()).into_response().status(),
+            StatusCode::GATEWAY_TIMEOUT
+        );
+        // Guard against a swap with the generic git failure (500).
+        assert_eq!(
+            AppError::Git("x".into()).into_response().status(),
+            StatusCode::INTERNAL_SERVER_ERROR
+        );
+    }
+}