From 2855cbf93747c543aa16f05d9030b35eddbe0a46 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 3 Jul 2026 17:53:01 -0500 Subject: [PATCH 1/4] 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. --- crates/gitlawb-node/src/api/repos.rs | 70 ++++- crates/gitlawb-node/src/config.rs | 38 +++ crates/gitlawb-node/src/error.rs | 24 ++ crates/gitlawb-node/src/git/smart_http.rs | 334 ++++++++++++++++++++-- 4 files changed, 433 insertions(+), 33 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index afbd37f3..cbab2541 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/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::() + .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, @@ -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,7 +924,8 @@ 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 @@ -910,8 +933,12 @@ pub async fn git_receive_pack( 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"), + _ => tracing::error!(repo = %name, err = %e, "git receive-pack failed"), + } + app })?; // Update the repo's updated_at timestamp after a successful push @@ -1795,6 +1822,25 @@ 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(_) + )); + // 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 { diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index 1f0a482e..4ccae835 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/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() + ); + } +} diff --git a/crates/gitlawb-node/src/error.rs b/crates/gitlawb-node/src/error.rs index aed680af..7a232a76 100644 --- a/crates/gitlawb-node/src/error.rs +++ b/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 = std::result::Result; + +#[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 + ); + } +} diff --git a/crates/gitlawb-node/src/git/smart_http.rs b/crates/gitlawb-node/src/git/smart_http.rs index 212f0e03..a6e84d60 100644 --- a/crates/gitlawb-node/src/git/smart_http.rs +++ b/crates/gitlawb-node/src/git/smart_http.rs @@ -6,7 +6,8 @@ use bytes::Bytes; use std::collections::HashSet; use std::path::Path; use std::process::Stdio; -use tokio::io::AsyncWriteExt; +use std::time::Duration; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::process::Command; /// Handle `GET /:owner/:repo/info/refs?service=git-upload-pack` @@ -51,8 +52,13 @@ pub async fn info_refs(repo_path: &Path, service: &str) -> Result { /// /// Serves pack data for a clone or fetch. This is stateless — the entire /// negotiation happens in a single request/response. -pub async fn upload_pack(repo_path: &Path, request_body: Bytes) -> Result { - let output = run_git_service("git", "git-upload-pack", repo_path, request_body).await?; +pub async fn upload_pack( + repo_path: &Path, + request_body: Bytes, + timeout: Duration, +) -> Result { + let output = + run_git_service("git", "git-upload-pack", repo_path, request_body, timeout).await?; Ok(Response::builder() .status(StatusCode::OK) @@ -65,8 +71,13 @@ pub async fn upload_pack(repo_path: &Path, request_body: Bytes) -> Result Result { - let output = run_git_service("git", "git-receive-pack", repo_path, request_body).await?; +pub async fn receive_pack( + repo_path: &Path, + request_body: Bytes, + timeout: Duration, +) -> Result { + let output = + run_git_service("git", "git-receive-pack", repo_path, request_body, timeout).await?; Ok(Response::builder() .status(StatusCode::OK) @@ -112,17 +123,86 @@ impl Drop for KillGroupOnDrop { } } +/// Error returned when [`run_git_service`] aborts a git op on its timeout. +/// +/// Carried through the `anyhow` chain so the HTTP handler can `downcast_ref` it +/// and map to 504 Gateway Timeout, distinct from the generic 500 git error. +#[derive(Debug, thiserror::Error)] +#[error("git service timed out")] +pub struct GitServiceTimeout; + +/// On a served-git timeout, tear the process group down AND reap the leader +/// before returning, so a caller that releases a write lock (receive-pack) can't +/// race a still-live git touching the same repo. SIGTERM first (lets git remove +/// its `.git/*.lock` files), escalate to SIGKILL if the leader lingers past a +/// grace, then reap. Bounded, so a git that ignores SIGTERM can't block the +/// response unboundedly. +#[cfg(unix)] +async fn reap_group_on_timeout(child: &mut tokio::process::Child) { + let Some(pid) = child.id() else { + // Never got a pid; nothing to signal, best-effort reap. + let _ = child.wait().await; + return; + }; + let pgid = pid as i32; + // SAFETY: kill(2) takes only integers and borrows no Rust memory; ESRCH on an + // already-gone group is ignored. + unsafe { + libc::kill(-pgid, libc::SIGTERM); + } + // Wait for the WHOLE group to exit before returning — not just the leader but + // grandchildren (index-pack / pack-objects) that reparent to init and can + // still be touching the repo, so a caller releasing a write lock can't race + // them. Reap our direct child along the way (`try_wait`, else it lingers as a + // zombie) and poll the group's liveness with `kill(-pgid, 0)` (ESRCH once + // every member is gone). SIGTERM grace, then SIGKILL, then a hard cap so a + // stuck process can never block the response unboundedly. + let mut sigkilled = false; + for step in 0..400u32 { + let _ = child.try_wait(); + if unsafe { libc::kill(-pgid, 0) } != 0 { + return; // ESRCH: every group member has exited + } + if step == 200 && !sigkilled { + // ~2s SIGTERM grace elapsed; force the group down. + unsafe { + libc::kill(-pgid, libc::SIGKILL); + } + sigkilled = true; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + // ~4s hard cap. Only reached if a group member survives SIGKILL, which means + // it is wedged in uninterruptible (D-state) I/O — a fsync on stuck storage, a + // hung mount — that no signal interrupts until the kernel returns. Nothing in + // userspace can kill it; blocking here (or holding the write lock) would just + // re-create the "hung git pins the repo" problem this bounds. Reap best-effort + // (tokio's orphan reaper collects it once the syscall unblocks) and return; + // git-level quarantine/ref-locking still guards the brief window before the + // D-state process finishes. + tracing::warn!( + pid, + "served git survived SIGKILL past the teardown cap (uninterruptible I/O?); \ + releasing without a confirmed group reap" + ); + let _ = child.try_wait(); +} + /// Run a stateless-rpc git service and return its stdout. /// /// `git_bin` is the git executable to spawn; production callers pass `"git"` /// (resolved via `PATH`). It is injectable purely so the process-group teardown /// wiring (`process_group(0)` + [`KillGroupOnDrop`]) can be driven end-to-end by /// a fake `git` in tests without mutating the process-global `PATH`. +/// +/// `timeout` bounds the whole child interaction; on expiry the op is aborted with +/// [`GitServiceTimeout`] and its process group is torn down. async fn run_git_service( git_bin: &str, service: &str, repo_path: &Path, input: Bytes, + timeout: Duration, ) -> Result> { let mut command = Command::new(git_bin); command @@ -142,33 +222,85 @@ async fn run_git_service( // Arm the group-kill guard for the lifetime of the request. With // process_group(0) the child is its own group leader, so pgid == its pid. + // This fires on a client disconnect (the whole future is dropped mid-request). #[cfg(unix)] let mut group_guard = KillGroupOnDrop { pgid: child.id().map(|id| id as i32), }; - // Write the request body to git's stdin, but don't early-return on a write - // error: always reap the child first (below), so the guard only ever fires on - // an actual future-drop (client disconnect), never on a pid we just reaped. - let write_result: std::io::Result<()> = match child.stdin.take() { - Some(mut stdin) => stdin.write_all(&input).await, - None => Ok(()), + // Own the pipes so `child` stays reap-able after a timeout: wait_with_output + // would consume it, but on timeout we must actively reap the group before + // returning (see reap_group_on_timeout). + let mut stdin = child.stdin.take(); + let mut stdout = child.stdout.take().context("git stdout was not piped")?; + let mut stderr = child.stderr.take().context("git stderr was not piped")?; + let mut out = Vec::new(); + let mut err = Vec::new(); + + // Bound the whole child interaction: a git that neither finishes nor + // disconnects would otherwise pin the pid forever. Write stdin, drain stdout + // and stderr, and wait for the child all concurrently (draining while writing + // avoids a pipe deadlock on a large body), under one deadline. + let interact = async { + // Don't early-return on a write error: the join still waits on the child, + // so an error is surfaced only after the child has been waited on. + let write = async { + match stdin.take() { + Some(mut s) => s.write_all(&input).await, + None => Ok(()), + } + }; + let (write_result, r_out, r_err, status) = tokio::join!( + write, + stdout.read_to_end(&mut out), + stderr.read_to_end(&mut err), + child.wait(), + ); + r_out?; + r_err?; + Ok::<_, anyhow::Error>((write_result, status?)) }; - let output = child.wait_with_output().await?; - - // Child reaped, so its group is gone: disarm before surfacing any error. - #[cfg(unix)] - group_guard.disarm(); - - write_result.context("failed to write to git stdin")?; + let timed = tokio::time::timeout(timeout, interact).await; + let (write_result, status) = match timed { + Ok(result) => { + // The join runs all arms to completion, so the child is reaped: disarm + // before surfacing any interaction error (a read/wait error), else the + // guard's drop would fire SIGTERM on the reaped, possibly-reused pgid. + #[cfg(unix)] + group_guard.disarm(); + result? + } + Err(_elapsed) => { + // Timeout: tear the whole group down and reap it before returning so a + // caller releasing a write lock can't race a still-live git. Then disarm + // the (now redundant) guard so its drop can't hit a reused pgid. + #[cfg(unix)] + { + reap_group_on_timeout(&mut child).await; + group_guard.disarm(); + } + #[cfg(not(unix))] + { + let _ = child.start_kill(); + let _ = tokio::time::timeout(Duration::from_secs(2), child.wait()).await; + } + return Err(GitServiceTimeout.into()); + } + }; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); + // Surface git's own failure (its stderr, which the handler may classify as a + // 400) before any stdin-write error: when git rejects a malformed body it + // exits non-zero and closes stdin, so the write's EPIPE would otherwise mask + // the real cause. + if !status.success() { + let stderr = String::from_utf8_lossy(&err); bail!("{service} failed: {stderr}"); } - Ok(output.stdout) + write_result.context("failed to write to git stdin")?; + + Ok(out) } fn service_to_command(service: &str) -> &str { @@ -957,6 +1089,7 @@ mod tests { "git-upload-pack", tmp.path(), Bytes::new(), + Duration::from_secs(60), )); // Advance the future until the fake has spawned its grandchild. @@ -1032,6 +1165,7 @@ mod tests { "git-upload-pack", tmp.path(), Bytes::new(), + Duration::from_secs(60), ) .await; @@ -1077,6 +1211,7 @@ mod tests { "git-upload-pack", tmp.path(), Bytes::new(), + Duration::from_secs(60), ) .await; @@ -1097,4 +1232,161 @@ mod tests { tokio::time::sleep(std::time::Duration::from_millis(10)).await; } } + + // A git that hangs (never finishes, never disconnects) must be bounded: it + // returns GitServiceTimeout (the handler maps that to 504) and its process + // group is torn down. Goes RED if the timeout wrap is removed — the OUTER + // bound below then fires instead of run_git_service's own, so the test fails + // loudly rather than hanging CI. + #[cfg(unix)] + #[tokio::test] + async fn run_git_service_times_out_and_tears_down_a_hung_git() { + let tmp = tempfile::TempDir::new().unwrap(); + let pidfile = tmp.path().join("pids"); + // Fork a grandchild, then hang forever. + let body = format!( + "#!/bin/sh\nsleep 300 &\nprintf '%s\\n%s\\n' \"$$\" \"$!\" > \"{}\"\nwait\n", + pidfile.display() + ); + let git_bin = write_fake_git(tmp.path(), &body); + + // Generous vs the fake's microsecond pidfile write, so a loaded runner + // can't fire the timeout before the fake records its pids; still far under + // the 5s outer bound. + let git_timeout = Duration::from_millis(1000); + // Outer bound well above git_timeout: if run_git_service's own timeout is + // broken it hangs, and this fires instead so we assert-fail, not hang. + let outcome = tokio::time::timeout( + Duration::from_secs(5), + run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + Bytes::new(), + git_timeout, + ), + ) + .await; + + assert!( + outcome.is_ok(), + "run_git_service must return via its own timeout, not hang" + ); + let err = outcome + .unwrap() + .expect_err("a hung git must surface as Err"); + assert!( + err.downcast_ref::().is_some(), + "the error must be GitServiceTimeout (maps to 504), got: {err:#}" + ); + + // The hung git's group must be torn down (the armed guard fires on the + // timeout return), not leaked. Poll the pidfile for fs-visibility rather + // than a single read that could race a loaded runner. + let mut pids = None; + for _ in 0..200 { + if let Some(p) = read_two_pids(&pidfile) { + pids = Some(p); + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + let (_leader, grandchild) = pids.expect("fake git should have written its pids"); + let _cleanup = ReapOnPanic(vec![grandchild]); + let mut gone = false; + for _ in 0..500 { + if !alive(grandchild) { + gone = true; + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert!( + gone, + "a timed-out git's process group must be torn down, not leaked" + ); + } + + // On timeout, run_git_service must REAP the group before returning, so a + // caller releasing a write lock (receive-pack) can't race a still-live git. + // The fake traps SIGTERM and lingers ~1s (git-cleanup stand-in): with the + // reap the call waits for it to exit, so the leader is dead by the time we + // return; without it, the call returns while the fake is still mid-trap. + // Goes RED if the timeout arm's reap is removed. + #[cfg(unix)] + #[tokio::test] + async fn run_git_service_reaps_the_group_before_returning_on_timeout() { + let tmp = tempfile::TempDir::new().unwrap(); + let pidfile = tmp.path().join("pids"); + // Leader traps SIGTERM and lingers ~1s; the grandchild (a sub-shell) traps + // and lingers ~3s, past the 2s SIGKILL grace, so "wait for the whole group" + // (leader AND grandchildren) is observable and the SIGKILL escalation is + // exercised. + let body = format!( + "#!/bin/sh\ntrap 'sleep 1; exit 0' TERM\nsh -c 'trap \"sleep 3; exit 0\" TERM; sleep 300' >/dev/null 2>&1 &\nprintf '%s\\n%s\\n' \"$$\" \"$!\" > \"{}\"\nwait\n", + pidfile.display() + ); + let git_bin = write_fake_git(tmp.path(), &body); + + let outcome = tokio::time::timeout( + Duration::from_secs(10), + run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + Bytes::new(), + Duration::from_millis(300), + ), + ) + .await; + assert!(outcome.is_ok(), "must return via its own timeout, not hang"); + assert!(outcome.unwrap().is_err(), "a hung git must surface as Err"); + + // The WHOLE group (leader AND the lingering grandchild) must be gone by the + // time we get here. + let (leader, grandchild) = + read_two_pids(&pidfile).expect("fake git should have written its pids"); + let _cleanup = ReapOnPanic(vec![grandchild]); + assert!( + !alive(leader) && !alive(grandchild), + "run_git_service must reap the whole git process group (leader AND \ + grandchildren) before returning on a timeout, so a write-lock release \ + can't race a still-live git" + ); + } + + // A malformed request: git exits non-zero with a recognizable pkt-line error + // on stderr AND does not read stdin, so a body larger than the pipe buffer + // makes the write EPIPE. run_git_service must surface git's stderr (which the + // handler classifies as a 400), not the EPIPE write error (a generic 500). + // Goes RED if the stdin-write check runs before the exit-status check. + #[cfg(unix)] + #[tokio::test] + async fn run_git_service_surfaces_git_stderr_over_a_stdin_epipe() { + let tmp = tempfile::TempDir::new().unwrap(); + let git_bin = write_fake_git( + tmp.path(), + "#!/bin/sh\necho 'fatal: bad line length character: 0000' >&2\nexit 128\n", + ); + + // Larger than a pipe buffer (~64 KiB) so the write blocks then EPIPEs when + // the fake exits without draining stdin, exercising the ordering. + let big = Bytes::from(vec![0u8; 256 * 1024]); + let result = run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + big, + Duration::from_secs(60), + ) + .await; + + let err = result.expect_err("non-zero git must surface as Err"); + let msg = format!("{err:#}"); + assert!( + msg.contains("bad line length"), + "run_git_service must surface git's stderr (a classifiable 400), not the \ + stdin-write EPIPE (a generic 500); got: {msg}" + ); + } } From e72f4609a4e1dd2fa0d0c70183f09d2759093385 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 7 Jul 2026 16:14:53 -0500 Subject: [PATCH 2/4] 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 --- crates/gitlawb-node/src/api/repos.rs | 7 +++ crates/gitlawb-node/src/git/smart_http.rs | 70 +++++++++++++++++++---- 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index cbab2541..8101aa2f 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -1836,6 +1836,13 @@ mod tests { 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(_))); diff --git a/crates/gitlawb-node/src/git/smart_http.rs b/crates/gitlawb-node/src/git/smart_http.rs index a6e84d60..89686838 100644 --- a/crates/gitlawb-node/src/git/smart_http.rs +++ b/crates/gitlawb-node/src/git/smart_http.rs @@ -157,18 +157,17 @@ async fn reap_group_on_timeout(child: &mut tokio::process::Child) { // zombie) and poll the group's liveness with `kill(-pgid, 0)` (ESRCH once // every member is gone). SIGTERM grace, then SIGKILL, then a hard cap so a // stuck process can never block the response unboundedly. - let mut sigkilled = false; for step in 0..400u32 { let _ = child.try_wait(); if unsafe { libc::kill(-pgid, 0) } != 0 { return; // ESRCH: every group member has exited } - if step == 200 && !sigkilled { - // ~2s SIGTERM grace elapsed; force the group down. + if step == 200 { + // ~2s SIGTERM grace elapsed; force the group down. `step == 200` fires + // exactly once in the 0..400 loop, so no re-entry guard is needed. unsafe { libc::kill(-pgid, libc::SIGKILL); } - sigkilled = true; } tokio::time::sleep(Duration::from_millis(10)).await; } @@ -1318,12 +1317,16 @@ mod tests { async fn run_git_service_reaps_the_group_before_returning_on_timeout() { let tmp = tempfile::TempDir::new().unwrap(); let pidfile = tmp.path().join("pids"); - // Leader traps SIGTERM and lingers ~1s; the grandchild (a sub-shell) traps - // and lingers ~3s, past the 2s SIGKILL grace, so "wait for the whole group" - // (leader AND grandchildren) is observable and the SIGKILL escalation is - // exercised. + // Leader traps SIGTERM and exits after ~1s. The grandchild (a sub-shell) + // IGNORES SIGTERM entirely (`trap "" TERM`) and sleeps far past the ~4s reap + // cap, so nothing but the untrappable SIGKILL escalation can kill it within + // the cap. That makes the escalation load-bearing: neuter the SIGKILL and + // this test goes RED. (A grandchild that merely trapped-and-slept UNDER the + // cap would self-exit and hide a broken escalation.) "Wait for the whole + // group" (leader AND grandchild) is observable because both must be dead + // before run_git_service returns. let body = format!( - "#!/bin/sh\ntrap 'sleep 1; exit 0' TERM\nsh -c 'trap \"sleep 3; exit 0\" TERM; sleep 300' >/dev/null 2>&1 &\nprintf '%s\\n%s\\n' \"$$\" \"$!\" > \"{}\"\nwait\n", + "#!/bin/sh\ntrap 'sleep 1; exit 0' TERM\nsh -c 'trap \"\" TERM; sleep 300' >/dev/null 2>&1 &\nprintf '%s\\n%s\\n' \"$$\" \"$!\" > \"{}\"\nwait\n", pidfile.display() ); let git_bin = write_fake_git(tmp.path(), &body); @@ -1344,8 +1347,17 @@ mod tests { // The WHOLE group (leader AND the lingering grandchild) must be gone by the // time we get here. - let (leader, grandchild) = - read_two_pids(&pidfile).expect("fake git should have written its pids"); + // Poll for fs-visibility of the pidfile rather than a single read that + // could race a loaded runner where the fake hasn't written it yet. + let mut pids = None; + for _ in 0..200 { + if let Some(p) = read_two_pids(&pidfile) { + pids = Some(p); + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + let (leader, grandchild) = pids.expect("fake git should have written its pids"); let _cleanup = ReapOnPanic(vec![grandchild]); assert!( !alive(leader) && !alive(grandchild), @@ -1355,6 +1367,42 @@ mod tests { ); } + // The timeout bounds git-receive-pack too, not just upload-pack: on the push + // path a hung git also holds the repo's write lock, so an unbounded receive-pack + // pins the repo until the process dies. run_git_service must return + // Err(GitServiceTimeout) for the receive-pack service exactly as it does for + // upload-pack. Goes RED if the internal timeout is removed (the fake never + // exits, so the outer bound trips and the outcome is not Ok). + #[cfg(unix)] + #[tokio::test] + async fn run_git_service_times_out_a_hung_receive_pack() { + let tmp = tempfile::TempDir::new().unwrap(); + let git_bin = write_fake_git(tmp.path(), "#!/bin/sh\nsleep 300\n"); + let outcome = tokio::time::timeout( + Duration::from_secs(5), + run_git_service( + git_bin.to_str().unwrap(), + "git-receive-pack", + tmp.path(), + Bytes::new(), + Duration::from_millis(200), + ), + ) + .await; + assert!( + outcome.is_ok(), + "receive-pack must return via its own timeout, not hang" + ); + let err = outcome + .unwrap() + .expect_err("a hung receive-pack must surface as Err"); + assert!( + err.downcast_ref::().is_some(), + "a hung receive-pack must time out as GitServiceTimeout (maps to 504); \ + got: {err:#}" + ); + } + // A malformed request: git exits non-zero with a recognizable pkt-line error // on stderr AND does not read stdin, so a body larger than the pipe buffer // makes the write EPIPE. run_git_service must surface git's stderr (which the From 41eca0bbedd325c0ff32534b555a6ea7f22942e3 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 7 Jul 2026 19:57:26 -0500 Subject: [PATCH 3/4] 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. --- crates/gitlawb-node/src/api/repos.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 8101aa2f..bf0fe59f 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -936,6 +936,9 @@ pub async fn git_receive_pack( 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 From 957116c7985588364c84f6d9faa09e85f04fac70 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 8 Jul 2026 00:06:47 -0500 Subject: [PATCH 4/4] 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). --- .env.example | 6 + README.md | 1 + crates/gitlawb-node/src/git/smart_http.rs | 268 ++++++++++++++-------- 3 files changed, 179 insertions(+), 96 deletions(-) diff --git a/.env.example b/.env.example index b9124d98..1d39c126 100644 --- a/.env.example +++ b/.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. diff --git a/README.md b/README.md index 0be37d20..57ce2885 100644 --- a/README.md +++ b/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. | diff --git a/crates/gitlawb-node/src/git/smart_http.rs b/crates/gitlawb-node/src/git/smart_http.rs index 89686838..eeb35b99 100644 --- a/crates/gitlawb-node/src/git/smart_http.rs +++ b/crates/gitlawb-node/src/git/smart_http.rs @@ -1066,6 +1066,60 @@ mod tests { Some((leader, grandchild)) } + /// Retry policy for the fake-git spawn race, shared by `fake_git_run_with_pids` + /// and the drop test's inline loop (which can't use the helper — it must keep + /// the winning future pending). `nth` retry waits `n * STEP_MS` ms. + #[cfg(unix)] + const FAKE_GIT_RETRY_ATTEMPTS: u64 = 12; + #[cfg(unix)] + const FAKE_GIT_BACKOFF_STEP_MS: u64 = 100; + + /// Run a fake-git-based `run` under the parallel test runner, retrying if the + /// fake transiently fails to record its pids. Under `cargo test --workspace` + /// fork-storm load a freshly-written fake `git` can fail to exec (ETXTBSY: a + /// concurrent worker forked while its write fd was still open) or be killed by + /// the service timeout before it is scheduled to write its pidfile; both leave + /// no pids. These misses are *correlated* (bursty) under fork pressure, so each + /// retry backs off (growing) to let a spike subside rather than burning every + /// attempt inside one. `pidfile` is removed before each attempt. Returns the + /// successful attempt's outcome together with the recorded pids; a genuine + /// never-spawns bug still fails loudly after the cap. Retried attempts don't + /// leak in practice: a miss almost always means the fake never exec'd (nothing + /// to clean up), and a spawn that got far enough is reaped by the timeout/drop + /// path before this returns. + #[cfg(unix)] + async fn fake_git_run_with_pids( + pidfile: &std::path::Path, + mut run: F, + ) -> (R, (i32, i32)) + where + F: FnMut() -> Fut, + Fut: std::future::Future, + { + for i in 0..FAKE_GIT_RETRY_ATTEMPTS { + let _ = std::fs::remove_file(pidfile); + let outcome = run().await; + // A successful run writes its pids before it returns, so a short poll + // catches them (allowing only for fs-visibility lag). + for _ in 0..50 { + if let Some(p) = read_two_pids(pidfile) { + return (outcome, p); + } + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + // Miss: back off before retrying so a bursty fork-pressure spike + // subsides (see the doc note on correlated ETXTBSY/EAGAIN failures). + tokio::time::sleep(std::time::Duration::from_millis( + FAKE_GIT_BACKOFF_STEP_MS * (i + 1), + )) + .await; + } + panic!( + "fake git failed to spawn and record its pids after {FAKE_GIT_RETRY_ATTEMPTS} \ + attempts (persistent failure, not a transient parallel-runner miss)" + ); + } + // Dropping the request future mid-flight (client disconnect) must SIGTERM the // whole group so git AND its pack-objects grandchild die together. Goes RED // if `process_group(0)` or the guard-arming is removed: without its own @@ -1083,34 +1137,67 @@ mod tests { ); let git_bin = write_fake_git(tmp.path(), &body); - let mut fut = Box::pin(run_git_service( - git_bin.to_str().unwrap(), - "git-upload-pack", - tmp.path(), - Bytes::new(), - Duration::from_secs(60), - )); - - // Advance the future until the fake has spawned its grandchild. - let mut pids = None; - for _ in 0..500 { - let _ = tokio::time::timeout(std::time::Duration::from_millis(10), &mut fut).await; - if let Some(p) = read_two_pids(&pidfile) { - pids = Some(p); - break; - } - } - // Dropping the future (below or on the None arm) makes tokio reap the - // fake leader itself, so only the grandchild needs panic-cleanup — - // carrying the already-reaped leader pid risks SIGKILLing a recycled pid - // under parallel test load. - let (_leader, grandchild) = match pids { - Some(p) => p, - // Drop the still-armed future first so its guard reaps the fake - // group, then fail — otherwise a spawn hiccup orphans the processes. - None => { - drop(fut); - panic!("fake git should spawn a grandchild and write its pids"); + // Retry under fork-storm load: a freshly-written fake `git` can transiently + // fail to exec (ETXTBSY) or exit before recording its pids; both leave no + // pids and are independent per attempt (see fake_git_run_with_pids). The + // winning attempt's future is kept PENDING so the drop below exercises the + // client-disconnect teardown. Dropping a losing attempt's future makes its + // guard reap anything that spawned, so retries don't leak. + let (fut, grandchild) = { + let mut attempt = 0u64; + loop { + attempt += 1; + let _ = std::fs::remove_file(&pidfile); + let mut fut = Box::pin(run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + Bytes::new(), + Duration::from_secs(60), + )); + + // Advance the future a slice at a time until the fake records its + // pids. `Ok(_)` means run_git_service returned before the pidfile + // appeared (spawn error / early exit); stop polling then, since + // re-polling a completed future panics with `async fn resumed after + // completion`. Read the pidfile first so a fake that wrote its pids + // and then exited is still captured. + let mut pids = None; + for _ in 0..500 { + let finished = + tokio::time::timeout(std::time::Duration::from_millis(10), &mut fut) + .await + .is_ok(); + if let Some(p) = read_two_pids(&pidfile) { + pids = Some(p); + break; + } + if finished { + break; + } + } + // Only the grandchild needs panic-cleanup: dropping the future makes + // tokio reap the fake leader, and carrying the reaped leader pid + // risks SIGKILLing a recycled pid under parallel test load. + match pids { + Some((_leader, g)) => break (fut, g), + None => { + // Transient spawn miss: drop the still-armed future so its + // guard reaps anything that spawned, then back off (growing) + // so a bursty fork-pressure spike subsides before retrying. + drop(fut); + assert!( + attempt < FAKE_GIT_RETRY_ATTEMPTS, + "fake git failed to spawn and record its pids after \ + {FAKE_GIT_RETRY_ATTEMPTS} attempts (persistent failure, \ + not a transient parallel-runner miss)" + ); + tokio::time::sleep(std::time::Duration::from_millis( + FAKE_GIT_BACKOFF_STEP_MS * attempt, + )) + .await; + } + } } }; let _cleanup = ReapOnPanic(vec![grandchild]); @@ -1159,20 +1246,20 @@ mod tests { ); let git_bin = write_fake_git(tmp.path(), &body); - let result = run_git_service( - git_bin.to_str().unwrap(), - "git-upload-pack", - tmp.path(), - Bytes::new(), - Duration::from_secs(60), - ) - .await; - // wait_with_output already reaped the fake leader, so only the grandchild // needs panic-cleanup — SIGKILLing the reaped leader pid could hit a - // recycled pid under parallel test load. - let (_leader, grandchild) = - read_two_pids(&pidfile).expect("fake git should have written its pids"); + // recycled pid under parallel test load. Retry the run so a transient spawn + // miss under fork-storm load doesn't fail the test (see fake_git_run_with_pids). + let (result, (_leader, grandchild)) = fake_git_run_with_pids(&pidfile, || { + run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + Bytes::new(), + Duration::from_secs(60), + ) + }) + .await; let _cleanup = ReapOnPanic(vec![grandchild]); assert!(result.is_err(), "non-zero git exit must surface as Err"); @@ -1205,17 +1292,18 @@ mod tests { ); let git_bin = write_fake_git(tmp.path(), &body); - let result = run_git_service( - git_bin.to_str().unwrap(), - "git-upload-pack", - tmp.path(), - Bytes::new(), - Duration::from_secs(60), - ) + // Retry the run so a transient spawn miss under fork-storm load doesn't + // fail the test (see fake_git_run_with_pids). + let (result, (_leader, grandchild)) = fake_git_run_with_pids(&pidfile, || { + run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + Bytes::new(), + Duration::from_secs(60), + ) + }) .await; - - let (_leader, grandchild) = - read_two_pids(&pidfile).expect("fake git should have written its pids"); let _cleanup = ReapOnPanic(vec![grandchild]); assert!(result.is_ok(), "zero-exit git must surface as Ok"); @@ -1249,22 +1337,23 @@ mod tests { ); let git_bin = write_fake_git(tmp.path(), &body); - // Generous vs the fake's microsecond pidfile write, so a loaded runner - // can't fire the timeout before the fake records its pids; still far under - // the 5s outer bound. - let git_timeout = Duration::from_millis(1000); // Outer bound well above git_timeout: if run_git_service's own timeout is // broken it hangs, and this fires instead so we assert-fail, not hang. - let outcome = tokio::time::timeout( - Duration::from_secs(5), - run_git_service( - git_bin.to_str().unwrap(), - "git-upload-pack", - tmp.path(), - Bytes::new(), - git_timeout, - ), - ) + // fake_git_run_with_pids retries a transient spawn/schedule miss under + // fork-storm load and returns the pidfile the fake recorded. + let git_timeout = Duration::from_millis(1000); + let (outcome, (_leader, grandchild)) = fake_git_run_with_pids(&pidfile, || { + tokio::time::timeout( + Duration::from_secs(5), + run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + Bytes::new(), + git_timeout, + ), + ) + }) .await; assert!( @@ -1280,17 +1369,7 @@ mod tests { ); // The hung git's group must be torn down (the armed guard fires on the - // timeout return), not leaked. Poll the pidfile for fs-visibility rather - // than a single read that could race a loaded runner. - let mut pids = None; - for _ in 0..200 { - if let Some(p) = read_two_pids(&pidfile) { - pids = Some(p); - break; - } - tokio::time::sleep(Duration::from_millis(10)).await; - } - let (_leader, grandchild) = pids.expect("fake git should have written its pids"); + // timeout return), not leaked. let _cleanup = ReapOnPanic(vec![grandchild]); let mut gone = false; for _ in 0..500 { @@ -1331,33 +1410,30 @@ mod tests { ); let git_bin = write_fake_git(tmp.path(), &body); - let outcome = tokio::time::timeout( - Duration::from_secs(10), - run_git_service( - git_bin.to_str().unwrap(), - "git-upload-pack", - tmp.path(), - Bytes::new(), - Duration::from_millis(300), - ), - ) + // fake_git_run_with_pids retries a transient spawn/schedule miss under + // fork-storm load and returns the pids the fake recorded; the outer 10s + // bound fires only if run_git_service's own timeout is broken (so we + // assert-fail rather than hang), well above the git_timeout plus the + // SIGKILL-escalation reap of the SIGTERM-ignoring grandchild. + let git_timeout = Duration::from_millis(300); + let (outcome, (leader, grandchild)) = fake_git_run_with_pids(&pidfile, || { + tokio::time::timeout( + Duration::from_secs(10), + run_git_service( + git_bin.to_str().unwrap(), + "git-upload-pack", + tmp.path(), + Bytes::new(), + git_timeout, + ), + ) + }) .await; assert!(outcome.is_ok(), "must return via its own timeout, not hang"); assert!(outcome.unwrap().is_err(), "a hung git must surface as Err"); // The WHOLE group (leader AND the lingering grandchild) must be gone by the // time we get here. - // Poll for fs-visibility of the pidfile rather than a single read that - // could race a loaded runner where the fake hasn't written it yet. - let mut pids = None; - for _ in 0..200 { - if let Some(p) = read_two_pids(&pidfile) { - pids = Some(p); - break; - } - tokio::time::sleep(Duration::from_millis(10)).await; - } - let (leader, grandchild) = pids.expect("fake git should have written its pids"); let _cleanup = ReapOnPanic(vec![grandchild]); assert!( !alive(leader) && !alive(grandchild),