From 57df8f4e277120f56d09cd1bcc7623cfedc9439a Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Mon, 9 Nov 2026 12:28:06 +0200 Subject: [PATCH 1/4] test(forge): cover cancelled transfers and SIGTERM during a clone (#316) --- crates/forge/tests/e2e_git.rs | 109 ++++++++++++++++++++++++++++++++++ crates/forge/tests/e2e_ops.rs | 87 +++++++++++++++++++++++++++ 2 files changed, 196 insertions(+) diff --git a/crates/forge/tests/e2e_git.rs b/crates/forge/tests/e2e_git.rs index b3135fb5..aea868cd 100644 --- a/crates/forge/tests/e2e_git.rs +++ b/crates/forge/tests/e2e_git.rs @@ -669,6 +669,115 @@ fn deleting_a_repository_ends_its_running_transports_first() { assert_eq!(r.status, 404); } +#[test] +fn a_cancelled_push_leaves_no_ref_lock_or_quarantine() { + let s = Srv::start(); + let c = Client::new("cancel-push"); + let url = repo(&s, "cancel-push"); + let auth = token(&s.admin); + let work = seeded(&c, &url, &auth); + std::fs::write(work.join("big.bin"), noise(20 << 20)).unwrap(); + c.ok(&work, None, &["add", "."]); + c.ok(&work, None, &["commit", "-q", "-m", "big"]); + let mut push = Command::new("git"); + push.env_clear() + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &c.dir) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_TERMINAL_PROMPT", "0") + .current_dir(&work) + .arg("-c") + .arg(format!("http.extraHeader=Authorization: {auth}")) + .args(["push", "-q", &url, "main"]); + let child = push.spawn().unwrap(); + let repo_dir = s.dir.join("repos").join("1.git"); + wait_for("push transport and quarantine", || { + s.app.life.registry.live() > 0 + && repo_dir + .join("objects") + .read_dir() + .ok() + .is_some_and(|mut e| { + e.any(|x| { + x.unwrap() + .file_name() + .to_string_lossy() + .starts_with("tmp_objdir-incoming-") + }) + }) + }); + let deleted = s.send( + "DELETE", + "/api/v1/repos/siteadmin/cancel-push", + Some(&auth), + None, + ); + assert_eq!(deleted.status, 204, "{}", deleted.msg()); + let out = child.wait_with_output().unwrap(); + assert!(!out.status.success(), "cancelled push succeeded"); + for dir in [ + repo_dir, + s.dir.join("trash/1.git"), + s.dir.join("orphans/1.git"), + ] { + if dir.exists() { + let mut pending = vec![dir.clone()]; + while let Some(path) = pending.pop() { + for entry in std::fs::read_dir(&path).unwrap() { + let entry = entry.unwrap(); + let name = entry.file_name().to_string_lossy().into_owned(); + assert!( + !name.ends_with(".lock") && !name.starts_with("tmp_objdir-incoming-"), + "leftover: {}", + entry.path().display() + ); + if entry.file_type().unwrap().is_dir() { + pending.push(entry.path()); + } + } + } + } + } +} + +#[test] +fn deleting_a_repository_during_a_clone_fails_the_clone() { + let s = Srv::start(); + let c = Client::new("delete-clone"); + let url = repo(&s, "delete-clone"); + let auth = token(&s.admin); + let work = seeded(&c, &url, &auth); + std::fs::write(work.join("big.bin"), noise(20 << 20)).unwrap(); + c.ok(&work, None, &["add", "."]); + c.ok(&work, None, &["commit", "-q", "-m", "big"]); + c.ok(&work, Some(&auth), &["push", "-q", &url, "main"]); + let mut clone = Command::new("git"); + clone + .env_clear() + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &c.dir) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_TERMINAL_PROMPT", "0") + .current_dir(&c.dir) + .arg("-c") + .arg(format!("http.extraHeader=Authorization: {auth}")) + .args(["clone", "-q", &url, c.dir.join("clone").to_str().unwrap()]); + let child = clone.spawn().unwrap(); + wait_for("upload-pack transport", || s.app.life.registry.live() > 0); + let deleted = s.send( + "DELETE", + "/api/v1/repos/siteadmin/delete-clone", + Some(&auth), + None, + ); + assert_eq!(deleted.status, 204, "{}", deleted.msg()); + let out = child.wait_with_output().unwrap(); + assert!( + !out.status.success(), + "clone succeeded after repository deletion" + ); +} + #[test] fn a_push_over_the_size_cap_fails_and_changes_nothing() { let s = Srv::start_transport(scl_forge::cgi::Cfg { diff --git a/crates/forge/tests/e2e_ops.rs b/crates/forge/tests/e2e_ops.rs index a90b5bb3..5ef0ae8a 100644 --- a/crates/forge/tests/e2e_ops.rs +++ b/crates/forge/tests/e2e_ops.rs @@ -240,6 +240,93 @@ fn a_stalled_request_does_not_hold_the_shutdown_past_its_deadline() { drop(s); } +#[test] +fn sigterm_during_a_clone_drains_or_cancels_cleanly() { + let root = tmp("clone-sigterm"); + let token = bootstrap(&root.0); + let mut p = Proc::start(&root.0, &["--shutdown-timeout", "2"]); + let created = p + .send( + "POST", + "/api/v1/user/repos", + Some(&token), + Some(&serde_json::json!({"name":"clone-shutdown","auto_init":true})), + ) + .unwrap(); + assert_eq!(created.0, 201, "{}", created.1); + let url = format!("http://127.0.0.1:{}/siteadmin/clone-shutdown.git", p.port); + // Add a large incompressible blob through a real Git client. + let work = root.0.join("work"); + let run = |args: &[&str], cwd: &Path| { + Command::new("git") + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &root.0) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_AUTHOR_NAME", "T") + .env("GIT_AUTHOR_EMAIL", "t@example.invalid") + .env("GIT_COMMITTER_NAME", "T") + .env("GIT_COMMITTER_EMAIL", "t@example.invalid") + .current_dir(cwd) + .args(args) + .output() + .unwrap() + }; + std::fs::create_dir(&work).unwrap(); + assert!(run(&["init", "-q", "-b", "main"], &work).status.success()); + let mut x: u64 = 0x2545_F491_4F6C_DD1D; + let bytes: Vec = (0..(20 << 20)) + .map(|_| { + x ^= x << 13; + x ^= x >> 7; + x ^= x << 17; + (x >> 24) as u8 + }) + .collect(); + std::fs::write(work.join("large.bin"), bytes).unwrap(); + for args in [&["add", "."][..], &["commit", "-q", "-m", "large"][..]] { + assert!(run(args, &work).status.success()); + } + let remote_auth = format!("-chttp.extraHeader=Authorization: token {token}"); + assert!(run(&[&remote_auth, "push", &url, "main"], &work) + .status + .success()); + let clone = root.0.join("clone"); + let client = Command::new("git") + .env_clear() + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &root.0) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_TERMINAL_PROMPT", "0") + .arg(&remote_auth) + .args(["clone", "-q", &url, clone.to_str().unwrap()]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .unwrap(); + // Give the server a bounded chance to accept the transfer before shutdown. + let connected = (0..100).any(|_| TcpStream::connect(("127.0.0.1", p.port)).is_ok()); + assert!( + connected, + "server stopped accepting before the clone started" + ); + p.sigterm(); + let status = p + .wait(Duration::from_secs(8)) + .expect("server exits by timeout plus margin"); + assert!(status.success(), "{status:?}"); + let mut client = client; + let end = Instant::now() + Duration::from_secs(8); + let status = loop { + if let Some(status) = client.try_wait().unwrap() { + break status; + } + assert!(Instant::now() < end, "clone hung after server shutdown"); + std::thread::sleep(Duration::from_millis(20)); + }; + let _ = status; +} + fn serve_with(tag: &str, listen: &str, extra: &[&str]) -> (Child, Tmp) { let root = tmp(tag); let child = Command::new(BIN) From 3f53c638a974965bffae269fb8700158892dd185 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Mon, 9 Nov 2026 12:49:21 +0200 Subject: [PATCH 2/4] test(forge): cover cancelled pushes, deletes during a clone and SIGTERM drains (#316) A push cancelled mid-pack moves no ref and leaves no ref lock; its quarantine can survive git's SIGTERM-then-SIGKILL cleanup, and maintenance reclaims it. A delete during a clone, or a SIGTERM drain deadline, cuts the response without its terminating chunk, and no git process group outlives the server; a clone that finishes within the drain completes. The client-disconnect path is bounded by a short idle timeout because the body read sometimes misses the EOF (recorded in the threat model). --- crates/forge/tests/e2e_git.rs | 215 ++++++++++++++++++++++------------ crates/forge/tests/e2e_ops.rs | 164 +++++++++++++++++--------- docs/THREAT-MODEL.md | 14 ++- 3 files changed, 259 insertions(+), 134 deletions(-) diff --git a/crates/forge/tests/e2e_git.rs b/crates/forge/tests/e2e_git.rs index aea868cd..44c576d0 100644 --- a/crates/forge/tests/e2e_git.rs +++ b/crates/forge/tests/e2e_git.rs @@ -669,75 +669,148 @@ fn deleting_a_repository_ends_its_running_transports_first() { assert_eq!(r.status, 404); } +/// Every entry under `dir` whose name a cancelled transport could leave behind. +fn leftovers(dir: &Path) -> Vec { + let mut found = Vec::new(); + let mut pending = vec![dir.to_path_buf()]; + while let Some(path) = pending.pop() { + for entry in std::fs::read_dir(&path).unwrap() { + let entry = entry.unwrap(); + let name = entry.file_name().to_string_lossy().into_owned(); + if name.ends_with(".lock") || name.starts_with("tmp_objdir-incoming-") { + found.push(entry.path()); + } + if entry.file_type().unwrap().is_dir() { + pending.push(entry.path()); + } + } + } + found +} + +fn quarantined(repo_dir: &Path) -> bool { + std::fs::read_dir(repo_dir.join("objects")) + .unwrap() + .any(|e| { + e.unwrap() + .file_name() + .to_string_lossy() + .starts_with("tmp_objdir-incoming-") + }) +} + #[test] fn a_cancelled_push_leaves_no_ref_lock_or_quarantine() { - let s = Srv::start(); + use std::io::Write; + // The server does not always see a client that leaves mid-body (the body read can miss the + // EOF, #316); the idle timeout then ends the transport. A short one bounds the test either way. + let s = Srv::start_transport(scl_forge::cgi::Cfg { + idle: std::time::Duration::from_secs(5), + ..Default::default() + }); let c = Client::new("cancel-push"); let url = repo(&s, "cancel-push"); let auth = token(&s.admin); let work = seeded(&c, &url, &auth); - std::fs::write(work.join("big.bin"), noise(20 << 20)).unwrap(); + let old = c.ok(&work, None, &["rev-parse", "HEAD"]).trim().to_string(); + std::fs::write(work.join("big.bin"), noise(8 << 20)).unwrap(); c.ok(&work, None, &["add", "."]); c.ok(&work, None, &["commit", "-q", "-m", "big"]); - let mut push = Command::new("git"); - push.env_clear() - .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") - .env("HOME", &c.dir) - .env("GIT_CONFIG_NOSYSTEM", "1") - .env("GIT_TERMINAL_PROMPT", "0") + let new = c.ok(&work, None, &["rev-parse", "HEAD"]).trim().to_string(); + // The real pack the client would send for `old..new`. + let mut pack_objects = Command::new("git") .current_dir(&work) - .arg("-c") - .arg(format!("http.extraHeader=Authorization: {auth}")) - .args(["push", "-q", &url, "main"]); - let child = push.spawn().unwrap(); + .args(["pack-objects", "--stdout", "--revs", "-q"]) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .spawn() + .unwrap(); + pack_objects + .stdin + .take() + .unwrap() + .write_all(format!("{new}\n^{old}\n").as_bytes()) + .unwrap(); + let pack = pack_objects.wait_with_output().unwrap().stdout; + assert!(pack.len() > 8 << 20); + let cmd = format!("{old} {new} refs/heads/main\0report-status\n"); + let mut body = format!("{:04x}{cmd}0000", cmd.len() + 4).into_bytes(); + body.extend_from_slice(&pack); + + let mut sock = std::net::TcpStream::connect(s.addr).unwrap(); + write!( + sock, + "POST /siteadmin/cancel-push.git/git-receive-pack HTTP/1.1\r\nHost: x\r\nAuthorization: {auth}\r\nContent-Type: application/x-git-receive-pack-request\r\nContent-Length: {}\r\n\r\n", + body.len() + ) + .unwrap(); + sock.write_all(&body[..body.len() / 2]).unwrap(); let repo_dir = s.dir.join("repos").join("1.git"); - wait_for("push transport and quarantine", || { - s.app.life.registry.live() > 0 - && repo_dir - .join("objects") - .read_dir() - .ok() - .is_some_and(|mut e| { - e.any(|x| { - x.unwrap() - .file_name() - .to_string_lossy() - .starts_with("tmp_objdir-incoming-") - }) - }) + // The receive-pack is running and writing the pushed objects into its quarantine. + wait_for("the push to be receiving into its quarantine", || { + s.app.life.registry.live() == 1 && quarantined(&repo_dir) }); - let deleted = s.send( - "DELETE", - "/api/v1/repos/siteadmin/cancel-push", - Some(&auth), - None, + // The client goes away mid-pack: the server cancels the transport (on the broken body, or + // at the idle timeout). + drop(sock); + wait_for("the transport to unregister", || { + s.app.life.registry.live() == 0 + }); + let tip = c.ok(&work, Some(&auth), &["ls-remote", &url, "refs/heads/main"]); + assert!( + tip.starts_with(&old), + "the cancelled push moved main: {tip}" ); - assert_eq!(deleted.status, 204, "{}", deleted.msg()); - let out = child.wait_with_output().unwrap(); - assert!(!out.status.success(), "cancelled push succeeded"); - for dir in [ - repo_dir, - s.dir.join("trash/1.git"), - s.dir.join("orphans/1.git"), - ] { - if dir.exists() { - let mut pending = vec![dir.clone()]; - while let Some(path) = pending.pop() { - for entry in std::fs::read_dir(&path).unwrap() { - let entry = entry.unwrap(); - let name = entry.file_name().to_string_lossy().into_owned(); - assert!( - !name.ends_with(".lock") && !name.starts_with("tmp_objdir-incoming-"), - "leftover: {}", - entry.path().display() - ); - if entry.file_type().unwrap().is_dir() { - pending.push(entry.path()); - } - } - } - } + let left = leftovers(&repo_dir); + assert!( + left.iter().all(|p| !p.to_string_lossy().ends_with(".lock")), + "no ref lock survives the cancel: {left:?}" + ); + // git does not always get to remove its quarantine before the group is killed (seen with + // Apple git 2.54); maintenance reclaims what is left once it is old enough to have no owner. + for p in &left { + std::fs::File::open(p) + .unwrap() + .set_modified(std::time::SystemTime::now() - std::time::Duration::from_secs(86_400)) + .unwrap(); } + let conn = scl_forge::db::connect(&s.app.data_dir).unwrap(); + let _ = scl_forge::maintenance::maintain_repo(&s.app, &conn, 1); + assert_eq!(leftovers(&repo_dir), Vec::::new()); +} + +/// Ask for `want`'s full pack over a raw socket and read only the response head, so the +/// upload-pack is mid-transfer (its output is blocked on this socket) until the rest is read. +fn slow_fetch( + addr: std::net::SocketAddr, + auth: &str, + repo: &str, + want: &str, +) -> std::net::TcpStream { + use std::io::{BufRead, Write}; + let body = format!("0032want {want}\n00000009done\n"); + let mut sock = std::net::TcpStream::connect(addr).unwrap(); + sock.set_read_timeout(Some(std::time::Duration::from_secs(30))) + .unwrap(); + write!( + sock, + "POST /siteadmin/{repo}.git/git-upload-pack HTTP/1.1\r\nHost: x\r\nConnection: close\r\nAuthorization: {auth}\r\nContent-Type: application/x-git-upload-pack-request\r\nContent-Length: {}\r\n\r\n{body}", + body.len() + ) + .unwrap(); + let mut r = std::io::BufReader::new(sock.try_clone().unwrap()); + let mut line = String::new(); + r.read_line(&mut line).unwrap(); + assert!(line.starts_with("HTTP/1.1 200"), "{line}"); + sock +} + +/// Read the rest of a chunked response; `true` when it ended with the terminating chunk. +fn ends_cleanly(mut sock: std::net::TcpStream) -> (bool, usize) { + use std::io::Read; + let mut buf = Vec::new(); + let _ = sock.read_to_end(&mut buf); + (buf.ends_with(b"\r\n0\r\n\r\n"), buf.len()) } #[test] @@ -751,19 +824,15 @@ fn deleting_a_repository_during_a_clone_fails_the_clone() { c.ok(&work, None, &["add", "."]); c.ok(&work, None, &["commit", "-q", "-m", "big"]); c.ok(&work, Some(&auth), &["push", "-q", &url, "main"]); - let mut clone = Command::new("git"); - clone - .env_clear() - .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") - .env("HOME", &c.dir) - .env("GIT_CONFIG_NOSYSTEM", "1") - .env("GIT_TERMINAL_PROMPT", "0") - .current_dir(&c.dir) - .arg("-c") - .arg(format!("http.extraHeader=Authorization: {auth}")) - .args(["clone", "-q", &url, c.dir.join("clone").to_str().unwrap()]); - let child = clone.spawn().unwrap(); - wait_for("upload-pack transport", || s.app.life.registry.live() > 0); + let want = c.ok(&work, None, &["rev-parse", "HEAD"]).trim().to_string(); + // Control: read at once, the same request ends with the terminating chunk. + let (clean, whole) = ends_cleanly(slow_fetch(s.addr, &auth, "delete-clone", &want)); + assert!(clean && whole > 20 << 20, "{whole} bytes"); + + let held = slow_fetch(s.addr, &auth, "delete-clone", &want); + wait_for("the upload-pack to be registered", || { + s.app.life.registry.live() == 1 + }); let deleted = s.send( "DELETE", "/api/v1/repos/siteadmin/delete-clone", @@ -771,10 +840,12 @@ fn deleting_a_repository_during_a_clone_fails_the_clone() { None, ); assert_eq!(deleted.status, 204, "{}", deleted.msg()); - let out = child.wait_with_output().unwrap(); + // The delete cancelled the transfer: the response stops without its terminating chunk, so + // the client sees an error, not a clean (but truncated) pack. + let (clean, got) = ends_cleanly(held); assert!( - !out.status.success(), - "clone succeeded after repository deletion" + !clean && got < whole, + "{got} of {whole} bytes, clean: {clean}" ); } diff --git a/crates/forge/tests/e2e_ops.rs b/crates/forge/tests/e2e_ops.rs index 5ef0ae8a..10399433 100644 --- a/crates/forge/tests/e2e_ops.rs +++ b/crates/forge/tests/e2e_ops.rs @@ -240,42 +240,41 @@ fn a_stalled_request_does_not_hold_the_shutdown_past_its_deadline() { drop(s); } -#[test] -fn sigterm_during_a_clone_drains_or_cancels_cleanly() { - let root = tmp("clone-sigterm"); - let token = bootstrap(&root.0); - let mut p = Proc::start(&root.0, &["--shutdown-timeout", "2"]); +/// A repository holding `len` bytes of incompressible content, pushed with a scrubbed git; returns +/// the tip commit. +fn big_repo(p: &Proc, root: &Path, token: &str, name: &str, len: usize) -> String { let created = p .send( "POST", "/api/v1/user/repos", - Some(&token), - Some(&serde_json::json!({"name":"clone-shutdown","auto_init":true})), + Some(token), + Some(&serde_json::json!({"name": name, "default_branch": "main"})), ) .unwrap(); assert_eq!(created.0, 201, "{}", created.1); - let url = format!("http://127.0.0.1:{}/siteadmin/clone-shutdown.git", p.port); - // Add a large incompressible blob through a real Git client. - let work = root.0.join("work"); - let run = |args: &[&str], cwd: &Path| { - Command::new("git") + let work = root.join("work"); + std::fs::create_dir_all(&work).unwrap(); + let git = |args: &[&str]| { + let out = Command::new("git") + .env_clear() .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") - .env("HOME", &root.0) + .env("HOME", root) .env("GIT_CONFIG_NOSYSTEM", "1") .env("GIT_TERMINAL_PROMPT", "0") .env("GIT_AUTHOR_NAME", "T") .env("GIT_AUTHOR_EMAIL", "t@example.invalid") .env("GIT_COMMITTER_NAME", "T") .env("GIT_COMMITTER_EMAIL", "t@example.invalid") - .current_dir(cwd) + .current_dir(&work) .args(args) .output() - .unwrap() + .unwrap(); + assert!(out.status.success(), "git {args:?}: {out:?}"); + String::from_utf8_lossy(&out.stdout).trim().to_string() }; - std::fs::create_dir(&work).unwrap(); - assert!(run(&["init", "-q", "-b", "main"], &work).status.success()); + git(&["init", "-q", "-b", "main"]); let mut x: u64 = 0x2545_F491_4F6C_DD1D; - let bytes: Vec = (0..(20 << 20)) + let bytes: Vec = (0..len) .map(|_| { x ^= x << 13; x ^= x >> 7; @@ -283,48 +282,97 @@ fn sigterm_during_a_clone_drains_or_cancels_cleanly() { (x >> 24) as u8 }) .collect(); - std::fs::write(work.join("large.bin"), bytes).unwrap(); - for args in [&["add", "."][..], &["commit", "-q", "-m", "large"][..]] { - assert!(run(args, &work).status.success()); - } - let remote_auth = format!("-chttp.extraHeader=Authorization: token {token}"); - assert!(run(&[&remote_auth, "push", &url, "main"], &work) - .status - .success()); - let clone = root.0.join("clone"); - let client = Command::new("git") - .env_clear() - .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") - .env("HOME", &root.0) - .env("GIT_CONFIG_NOSYSTEM", "1") - .env("GIT_TERMINAL_PROMPT", "0") - .arg(&remote_auth) - .args(["clone", "-q", &url, clone.to_str().unwrap()]) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .spawn() + std::fs::write(work.join("big.bin"), bytes).unwrap(); + git(&["add", "."]); + git(&["commit", "-q", "-m", "big"]); + let url = format!("http://127.0.0.1:{}/siteadmin/{name}.git", p.port); + let auth = format!("http.extraHeader=Authorization: token {token}"); + git(&["-c", &auth, "push", "-q", &url, "main"]); + git(&["rev-parse", "HEAD"]) +} + +/// Ask for `want`'s whole pack and read only the response head: the upload-pack is then +/// mid-transfer, blocked on this socket, until the rest is read. +fn held_fetch(p: &Proc, token: &str, name: &str, want: &str) -> TcpStream { + let body = format!("0032want {want}\n00000009done\n"); + let mut s = TcpStream::connect(("127.0.0.1", p.port)).unwrap(); + s.set_read_timeout(Some(Duration::from_secs(30))).unwrap(); + write!( + s, + "POST /siteadmin/{name}.git/git-upload-pack HTTP/1.1\r\nHost: x\r\nConnection: close\r\nAuthorization: token {token}\r\nContent-Type: application/x-git-upload-pack-request\r\nContent-Length: {}\r\n\r\n{body}", + body.len() + ) + .unwrap(); + let mut head = [0u8; 12]; + s.read_exact(&mut head).unwrap(); + assert_eq!(&head, b"HTTP/1.1 200", "the transfer started"); + s +} + +/// The process groups of the server's children (each git transport leads its own group). +fn child_groups(server: u32) -> Vec { + let out = Command::new("pgrep") + .args(["-P", &server.to_string()]) + .output() .unwrap(); - // Give the server a bounded chance to accept the transfer before shutdown. - let connected = (0..100).any(|_| TcpStream::connect(("127.0.0.1", p.port)).is_ok()); - assert!( - connected, - "server stopped accepting before the clone started" - ); + String::from_utf8_lossy(&out.stdout) + .split_whitespace() + .map(str::to_string) + .collect() +} + +fn group_alive(pgid: &str) -> bool { + Command::new("pgrep") + .args(["-g", pgid]) + .output() + .unwrap() + .status + .success() +} + +/// `(ended with the terminating chunk, bytes)` for the rest of a held response. +fn rest_of(mut s: TcpStream) -> (bool, usize) { + let mut buf = Vec::new(); + let _ = s.read_to_end(&mut buf); + (buf.ends_with(b"\r\n0\r\n\r\n"), buf.len()) +} + +#[test] +fn sigterm_lets_a_clone_in_flight_finish_within_the_drain() { + let root = tmp("clone-drain"); + let token = bootstrap(&root.0); + let mut p = Proc::start(&root.0, &["--shutdown-timeout", "30"]); + let want = big_repo(&p, &root.0, &token, "drained", 16 << 20); + let held = held_fetch(&p, &token, "drained", &want); p.sigterm(); - let status = p - .wait(Duration::from_secs(8)) - .expect("server exits by timeout plus margin"); - assert!(status.success(), "{status:?}"); - let mut client = client; - let end = Instant::now() + Duration::from_secs(8); - let status = loop { - if let Some(status) = client.try_wait().unwrap() { - break status; - } - assert!(Instant::now() < end, "clone hung after server shutdown"); - std::thread::sleep(Duration::from_millis(20)); - }; - let _ = status; + std::thread::sleep(Duration::from_millis(300)); // the drain has begun + let (clean, got) = rest_of(held); + assert!(clean && got > 16 << 20, "{got} bytes, clean: {clean}"); + let st = p.wait(Duration::from_secs(20)).expect("exits after it"); + assert!(st.success(), "{st:?}"); +} + +#[test] +fn sigterm_cancels_a_clone_still_running_at_the_drain_deadline() { + let root = tmp("clone-cancel"); + let token = bootstrap(&root.0); + let mut p = Proc::start(&root.0, &["--shutdown-timeout", "2"]); + let want = big_repo(&p, &root.0, &token, "cancelled", 16 << 20); + let held = held_fetch(&p, &token, "cancelled", &want); + let groups = child_groups(p.child.id()); + assert!(!groups.is_empty(), "the transfer's git is running"); + let t0 = Instant::now(); + p.sigterm(); + let st = p.wait(Duration::from_secs(15)).expect("exits"); + assert!(st.success(), "{st:?}"); + assert!(t0.elapsed() < Duration::from_secs(10), "{:?}", t0.elapsed()); + // Read only now: the transfer was cut, so the response has no terminating chunk (a client + // sees an error, not a clean but truncated pack), and no git outlives the server. + let (clean, got) = rest_of(held); + assert!(!clean, "{got} bytes ended cleanly"); + for g in &groups { + assert!(!group_alive(g), "git process group {g} survived the server"); + } } fn serve_with(tag: &str, listen: &str, extra: &[&str]) -> (Child, Tmp) { diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index a7bb0f24..31ba43f9 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -919,8 +919,11 @@ unless said otherwise). limit would also throttle Engine, whose clones come from one service account, so it needs a deliberate budget). A push's growth check credits its own written bytes back, but other pushes in flight are still counted twice (free space has dropped and they still hold the bytes), so it errs on the side of refusing. A killed push gets SIGTERM first (git removes its lock files and quarantine directory - from its signal handlers; we wait up to one second for the leader to exit and stderr to close, not tested) and then SIGKILL; a server killed outright can still leave a stale - `refs/heads/.lock` or `objects/incoming-*` behind (cleanup is F5). + from its signal handlers; we wait up to one second for the leader to exit and stderr to close) and then SIGKILL. A push + cancelled mid-pack leaves no ref lock and moves no ref, but its `objects/tmp_objdir-incoming-*` quarantine can survive + (seen with Apple git 2.54); maintenance removes it once it is old enough to have no owner, as it does for a server + killed outright (`a_cancelled_push_leaves_no_ref_lock_or_quarantine`). A client that leaves mid-body is sometimes + noticed only at `--git-idle-timeout` (the body read can miss the EOF, about 1 run in 8 locally, #316). - **Process control.** One supervisor thread owns each child and its process group and signals the group while the leader is still an unreaped zombie (so a reused group id cannot be hit); the registry holds a cancel flag, never a group id. A client that hangs up ends the child (`a_large_push_and_clone_stream_and_a_vanished_client_ends_the_child`); @@ -1158,8 +1161,11 @@ why each deletion is safe: line, so a supervisor that stops the service as soon as it is up still gets the drain. A stalled request does not hold the exit past the deadline plus up to ten seconds for the cancelled git processes to be reaped and the runtime's two-second teardown. Log writes cannot hold it either: they go through a bounded queue and a writer thread, and a stderr nobody reads costs dropped lines, - not a frozen server (a test fills a pipe with 6400 requests). Not covered by a test: a git transfer in flight across SIGTERM - (the order `drop(listener)`, stop signal, drain, `cancel_all` is source-reviewed). + not a frozen server (a test fills a pipe with 6400 requests). A git transfer in flight across SIGTERM finishes if it ends + within the drain (`sigterm_lets_a_clone_in_flight_finish_within_the_drain`); one still running at the deadline is cut + without its terminating chunk, so the client sees an error, and no git process group outlives the server + (`sigterm_cancels_a_clone_still_running_at_the_drain_deadline`). A repository delete cuts a clone the same way + (`deleting_a_repository_during_a_clone_fails_the_clone`). - **Bind gate.** Process tests pin it: `0.0.0.0`, `[::]`, a private address and a v4-mapped loopback are refused without `--behind-proxy` or `--allow-plaintext`; loopback needs no flag. Fail-closed on an address it cannot classify. - **Image.** Base images pinned by digest; trixie git (2.47) because the server needs 2.40 and bookworm has 2.39; runs as uid 10001; no From efe41f2d0586d0c691d7534737d90e2fa729b8ef Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Mon, 9 Nov 2026 12:53:43 +0200 Subject: [PATCH 3/4] test(forge): a hung response fails the transfer tests; the drain is shown to wait --- crates/forge/tests/e2e_git.rs | 12 +++++++++--- crates/forge/tests/e2e_ops.rs | 18 ++++++++++++++++-- docs/THREAT-MODEL.md | 7 ++++--- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/crates/forge/tests/e2e_git.rs b/crates/forge/tests/e2e_git.rs index 44c576d0..8831d699 100644 --- a/crates/forge/tests/e2e_git.rs +++ b/crates/forge/tests/e2e_git.rs @@ -700,7 +700,7 @@ fn quarantined(repo_dir: &Path) -> bool { } #[test] -fn a_cancelled_push_leaves_no_ref_lock_or_quarantine() { +fn a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed() { use std::io::Write; // The server does not always see a client that leaves mid-body (the body read can miss the // EOF, #316); the idle timeout then ends the transport. A short one bounds the test either way. @@ -761,10 +761,11 @@ fn a_cancelled_push_leaves_no_ref_lock_or_quarantine() { tip.starts_with(&old), "the cancelled push moved main: {tip}" ); + // git takes its ref locks only after the whole pack has arrived, so none is held here. let left = leftovers(&repo_dir); assert!( left.iter().all(|p| !p.to_string_lossy().ends_with(".lock")), - "no ref lock survives the cancel: {left:?}" + "no lock survives the cancel: {left:?}" ); // git does not always get to remove its quarantine before the group is killed (seen with // Apple git 2.54); maintenance reclaims what is left once it is old enough to have no owner. @@ -809,7 +810,12 @@ fn slow_fetch( fn ends_cleanly(mut sock: std::net::TcpStream) -> (bool, usize) { use std::io::Read; let mut buf = Vec::new(); - let _ = sock.read_to_end(&mut buf); + match sock.read_to_end(&mut buf) { + Ok(_) => {} + // A cut connection may end in a reset; a timeout means the server left it hanging. + Err(e) if e.kind() == std::io::ErrorKind::ConnectionReset => {} + Err(e) => panic!("the response neither ended nor broke: {e}"), + } (buf.ends_with(b"\r\n0\r\n\r\n"), buf.len()) } diff --git a/crates/forge/tests/e2e_ops.rs b/crates/forge/tests/e2e_ops.rs index 10399433..b1638671 100644 --- a/crates/forge/tests/e2e_ops.rs +++ b/crates/forge/tests/e2e_ops.rs @@ -333,7 +333,12 @@ fn group_alive(pgid: &str) -> bool { /// `(ended with the terminating chunk, bytes)` for the rest of a held response. fn rest_of(mut s: TcpStream) -> (bool, usize) { let mut buf = Vec::new(); - let _ = s.read_to_end(&mut buf); + match s.read_to_end(&mut buf) { + Ok(_) => {} + // A cut connection may end in a reset; a timeout means the server left it hanging. + Err(e) if e.kind() == std::io::ErrorKind::ConnectionReset => {} + Err(e) => panic!("the response neither ended nor broke: {e}"), + } (buf.ends_with(b"\r\n0\r\n\r\n"), buf.len()) } @@ -346,6 +351,10 @@ fn sigterm_lets_a_clone_in_flight_finish_within_the_drain() { let held = held_fetch(&p, &token, "drained", &want); p.sigterm(); std::thread::sleep(Duration::from_millis(300)); // the drain has begun + assert!( + p.child.try_wait().unwrap().is_none(), + "the server waits for the transfer" + ); let (clean, got) = rest_of(held); assert!(clean && got > 16 << 20, "{got} bytes, clean: {clean}"); let st = p.wait(Duration::from_secs(20)).expect("exits after it"); @@ -365,7 +374,12 @@ fn sigterm_cancels_a_clone_still_running_at_the_drain_deadline() { p.sigterm(); let st = p.wait(Duration::from_secs(15)).expect("exits"); assert!(st.success(), "{st:?}"); - assert!(t0.elapsed() < Duration::from_secs(10), "{:?}", t0.elapsed()); + // It waited out the two-second drain for the transfer, then cut it. + let took = t0.elapsed(); + assert!( + took >= Duration::from_millis(1900) && took < Duration::from_secs(10), + "{took:?}" + ); // Read only now: the transfer was cut, so the response has no terminating chunk (a client // sees an error, not a clean but truncated pack), and no git outlives the server. let (clean, got) = rest_of(held); diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index 31ba43f9..4d83f774 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -920,9 +920,10 @@ unless said otherwise). A push's growth check credits its own written bytes back, but other pushes in flight are still counted twice (free space has dropped and they still hold the bytes), so it errs on the side of refusing. A killed push gets SIGTERM first (git removes its lock files and quarantine directory from its signal handlers; we wait up to one second for the leader to exit and stderr to close) and then SIGKILL. A push - cancelled mid-pack leaves no ref lock and moves no ref, but its `objects/tmp_objdir-incoming-*` quarantine can survive - (seen with Apple git 2.54); maintenance removes it once it is old enough to have no owner, as it does for a server - killed outright (`a_cancelled_push_leaves_no_ref_lock_or_quarantine`). A client that leaves mid-body is sometimes + cancelled mid-pack moves no ref, but its `objects/tmp_objdir-incoming-*` quarantine can survive (seen with Apple git + 2.54); maintenance removes it once it is old enough to have no owner, as it does for a server killed outright + (`a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed`). git holds ref locks only inside its ref + transaction after the pack has arrived; a cancel inside that window is not tested (maintenance covers stale locks). A client that leaves mid-body is sometimes noticed only at `--git-idle-timeout` (the body read can miss the EOF, about 1 run in 8 locally, #316). - **Process control.** One supervisor thread owns each child and its process group and signals the group while the leader is still an unreaped zombie (so a reused group id cannot be hit); the registry holds a cancel flag, never a From 8a4b36218ee024caa9b1823ab3a50c42dec0c89c Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Mon, 9 Nov 2026 12:57:34 +0200 Subject: [PATCH 4/4] test(forge): a push cancelled while holding its ref lock releases it --- crates/forge/tests/e2e_git.rs | 60 ++++++++++++++++++++++++++++++++++- docs/THREAT-MODEL.md | 5 +-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/crates/forge/tests/e2e_git.rs b/crates/forge/tests/e2e_git.rs index 8831d699..85d18866 100644 --- a/crates/forge/tests/e2e_git.rs +++ b/crates/forge/tests/e2e_git.rs @@ -761,7 +761,7 @@ fn a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed() { tip.starts_with(&old), "the cancelled push moved main: {tip}" ); - // git takes its ref locks only after the whole pack has arrived, so none is held here. + // git takes its ref locks only after the whole pack has arrived (see the next test). let left = leftovers(&repo_dir); assert!( left.iter().all(|p| !p.to_string_lossy().ends_with(".lock")), @@ -780,6 +780,64 @@ fn a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed() { assert_eq!(leftovers(&repo_dir), Vec::::new()); } +#[test] +fn a_push_cancelled_while_holding_its_ref_lock_releases_it() { + use std::os::unix::fs::PermissionsExt; + let s = Srv::start(); + let c = Client::new("cancel-lock"); + let url = repo(&s, "cancel-lock"); + let auth = token(&s.admin); + let work = seeded(&c, &url, &auth); + let old = c.ok(&work, None, &["rev-parse", "HEAD"]).trim().to_string(); + put_commit(&c, &work, "b.txt", "two\n"); + // git runs `reference-transaction prepared` with the ref locks taken: hold the push there. + let marker = c.dir.join("locked"); + let hook = s.dir.join("hooks/reference-transaction"); + std::fs::write( + &hook, + format!( + "#!/bin/sh\n/bin/cat >/dev/null\n[ \"$1\" = prepared ] || exit 0\n: > '{m}'\nwhile [ -e '{m}' ]; do /bin/sleep 0.05; done\n", + m = marker.display() + ), + ) + .unwrap(); + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(0o755)).unwrap(); + let push = std::thread::spawn({ + let (dir, work, auth, url) = (c.dir.clone(), work.clone(), auth.clone(), url.clone()); + move || { + Command::new("git") + .env_clear() + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &dir) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_TERMINAL_PROMPT", "0") + .current_dir(&work) + .arg("-c") + .arg(format!("http.extraHeader=Authorization: {auth}")) + .args(["push", "-q", &url, "main"]) + .output() + .unwrap() + } + }); + let lock = s.dir.join("repos/1.git/refs/heads/main.lock"); + wait_for("the push to hold its ref lock", || { + marker.exists() && lock.exists() + }); + // Cancel the transport the way a delete or shutdown does, keeping the repository. + s.app.life.registry.close(1); + wait_for("the transport to end", || s.app.life.registry.live() == 0); + s.app.life.registry.reopen(1); + let _ = std::fs::remove_file(&marker); + std::fs::remove_file(&hook).unwrap(); + assert!(!push.join().unwrap().status.success(), "the push was cut"); + assert!(!lock.exists(), "git released the ref lock on SIGTERM"); + let tip = c.ok(&work, Some(&auth), &["ls-remote", &url, "refs/heads/main"]); + assert!( + tip.starts_with(&old), + "the cancelled push moved main: {tip}" + ); +} + /// Ask for `want`'s full pack over a raw socket and read only the response head, so the /// upload-pack is mid-transfer (its output is blocked on this socket) until the rest is read. fn slow_fetch( diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index 4d83f774..8d41595c 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -922,8 +922,9 @@ unless said otherwise). from its signal handlers; we wait up to one second for the leader to exit and stderr to close) and then SIGKILL. A push cancelled mid-pack moves no ref, but its `objects/tmp_objdir-incoming-*` quarantine can survive (seen with Apple git 2.54); maintenance removes it once it is old enough to have no owner, as it does for a server killed outright - (`a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed`). git holds ref locks only inside its ref - transaction after the pack has arrived; a cancel inside that window is not tested (maintenance covers stale locks). A client that leaves mid-body is sometimes + (`a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed`). A push cancelled while it holds its ref + lock (held in git's `reference-transaction prepared` hook) releases the lock and moves no ref + (`a_push_cancelled_while_holding_its_ref_lock_releases_it`). A client that leaves mid-body is sometimes noticed only at `--git-idle-timeout` (the body read can miss the EOF, about 1 run in 8 locally, #316). - **Process control.** One supervisor thread owns each child and its process group and signals the group while the leader is still an unreaped zombie (so a reused group id cannot be hit); the registry holds a cancel flag, never a