From 358479f075a5d0099fa4fcc72945bfff8f26139b Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 19:40:10 -0500 Subject: [PATCH 01/15] fix(git-remote): drive multi-round fetch as a v0 stateless-RPC client loop (#117) git-remote-gitlawb advertised `connect`, so git spoke the stateful native protocol, but handle_connect collapsed the exchange into one GET plus one POST. A multi-round fetch (more than ~32 overlapping commits) deadlocked: git sent a flush-terminated have-batch and blocked for an ACK/NAK the helper never sent, while the helper blocked reading for a `done` git never sent. Turn Phase 2 into a per-round stateless-RPC client loop. read_upload_pack_round returns one round at a time (at a flush, done, or EOF) instead of buffering past flushes waiting for `done`; negotiate_upload_pack captures the wants once and POSTs a self-contained request per flush-terminated batch: wants, one flush, every have accumulated so far, and exactly one terminator. Each ACK/NAK is streamed back to git so it advances, until the done round returns the pack. The node's `git upload-pack --stateless-rpc` keeps no state between POSTs, so wants and all prior haves are re-sent every round and no intermediate flush survives into a body (which would truncate the negotiation server-side). Signing is preserved per round: every POST carries the Phase-1 decision (signed after the 404 escalation for a private repo, anonymous for a public one), and a mid-negotiation denial surfaces through the sanitized error path rather than reading as an empty or successful fetch. The receive-pack path is unchanged. Covers the deadlock repro, multi-round accumulation on the wire, per-round signing for private and public fetches, mid-negotiation denial surfacing, the withheld-shaped forwarding path, and single-round non-regression. --- crates/git-remote-gitlawb/src/main.rs | 674 ++++++++++++++++++++++++-- 1 file changed, 638 insertions(+), 36 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index ca46c9ce..49b3f9fe 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -276,27 +276,32 @@ fn handle_connect( // ── Phase 2: pack exchange (POST /) ────────────────────────────── // - // The two services behave differently with their write pipe: + // The two services frame Phase 2 differently: // // git-upload-pack (clone/fetch): - // Client sends pkt-line want/have negotiation ending with "done\n", - // but does NOT close its write pipe — it waits for the pack response. - // We must detect the terminal "done\n" pkt-line to know when to POST. + // Git speaks the stateful native protocol over `connect`: it sends its + // wants, a flush, then have-batches each terminated by a flush, and blocks + // for the server's ACK/NAK after every flush before it sends more haves or + // the terminal "done\n". The node serves `git upload-pack --stateless-rpc`, + // which keeps no state between POSTs, so we run a per-round loop: POST a + // self-contained request once per flush-terminated batch and stream each + // response back to git, until the "done" round returns the pack. Collapsing + // this into one POST deadlocks a multi-round fetch (#117). // // git-receive-pack (push): - // Client sends ref-update commands + complete PACK blob, then closes - // its write pipe. read_to_end is safe and correct here. + // Git sends ref-update commands + the complete PACK blob, then closes its + // write pipe. read_to_end is safe and correct here, a single POST. - let request_body = if service == "git-upload-pack" { - read_upload_pack_request(stdin).context("reading upload-pack request")? - } else { - let mut buf = Vec::new(); - stdin - .read_to_end(&mut buf) - .context("reading receive-pack request")?; - buf - }; + let post_url = format!("{}/{}", repo_base, service); + if service == "git-upload-pack" { + return negotiate_upload_pack(&client, &post_url, service, signing_key, stdin, &mut stdout); + } + + let mut request_body = Vec::new(); + stdin + .read_to_end(&mut request_body) + .context("reading receive-pack request")?; tracing::debug!("pack request: {} bytes from git", request_body.len()); if request_body.is_empty() { @@ -305,9 +310,7 @@ fn handle_connect( return Ok(()); } - let post_url = format!("{}/{}", repo_base, service); tracing::debug!("POST {post_url} ({} bytes)", request_body.len()); - let req = build_pack_post_request(&client, &post_url, service, &request_body, signing_key); // Attach the body after signing so the pack bytes are moved, not cloned — @@ -424,33 +427,48 @@ fn parse_gitlawb_url(url: &str) -> Result<(String, String, String)> { Ok((did_string.to_string(), short_owner, repo_name)) } -/// Read a complete git-upload-pack request from the pkt-line stream. -/// -/// For upload-pack, git sends its want/have negotiation ending with the pkt-line -/// `"done\n"` but does NOT close its write pipe afterwards — it waits for the -/// server's pack response. We detect the terminal "done\n" and stop reading. +/// How a single upload-pack negotiation round ended. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +enum RoundEnd { + /// A flush pkt (`0000`) ended the round; more negotiation may follow. + Flush, + /// The terminal `done\n` pkt ended the round; the pack follows. + Done, + /// The stream ended (git closed its write pipe). + Eof, +} + +/// Read ONE round of git's upload-pack request from the pkt-line stream. /// -/// We also handle the flush-only case (`"0000"`) that git sends when it already -/// has everything it needs (up-to-date clone). -fn read_upload_pack_request(stdin: &mut R) -> Result> { - let mut buf = Vec::new(); +/// Git speaks the stateful native protocol over the `connect` capability: it +/// sends its `want` lines, a flush, then `have` batches each terminated by a +/// flush, and blocks for the server's ACK/NAK after every flush before sending +/// more haves or the terminal `done\n`. Each call returns one such round: the +/// pkt-lines up to (but not including) the terminating flush or `done`, plus +/// which terminator ended it. `negotiate_upload_pack` reassembles these into +/// self-contained stateless-RPC POST bodies. Returning at the flush, instead of +/// buffering past it waiting for `done`, is what lets a multi-round fetch make +/// progress rather than deadlock (#117). +fn read_upload_pack_round(stdin: &mut R) -> Result<(Vec, RoundEnd)> { + let mut content = Vec::new(); loop { - // Read the 4-byte hex pkt-line length prefix + // Read the 4-byte hex pkt-line length prefix. let mut len_bytes = [0u8; 4]; match stdin.read_exact(&mut len_bytes) { Ok(_) => {} - Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => break, + Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => { + return Ok((content, RoundEnd::Eof)); + } Err(e) => return Err(e.into()), } - buf.extend_from_slice(&len_bytes); let len_hex = std::str::from_utf8(&len_bytes).unwrap_or("0000"); let pkt_len = usize::from_str_radix(len_hex, 16).unwrap_or(0); if pkt_len == 0 { - // Flush pkt "0000" — keep buffering (more pkt-lines may follow) - continue; + // Flush pkt "0000": this round is complete. + return Ok((content, RoundEnd::Flush)); } if pkt_len < 4 { @@ -462,16 +480,140 @@ fn read_upload_pack_request(stdin: &mut R) -> Result> { stdin .read_exact(&mut data) .context("reading pkt-line data")?; - buf.extend_from_slice(&data); - // "done\n" signals the end of the want/have negotiation + // "done\n" ends the negotiation; the pack follows. The terminator is + // reported out-of-band and re-emitted by the caller, so it is not + // appended to `content`. if data == b"done\n" { - tracing::debug!("upload-pack: got 'done', request complete"); - break; + tracing::debug!("upload-pack: got 'done', round complete"); + return Ok((content, RoundEnd::Done)); + } + + content.extend_from_slice(&len_bytes); + content.extend_from_slice(&data); + } +} + +/// POST one self-contained stateless-RPC request body and stream the response to +/// `stdout`. Signs the request when `signing_key` is present, so every round of a +/// private-repo fetch carries the caller's signature (the node visibility-gates +/// each POST at "/"). A non-2xx status surfaces through the sanitized error path +/// rather than being rendered as an empty or successful fetch (INV-6/INV-8). +fn post_pack_round( + client: &reqwest::blocking::Client, + post_url: &str, + service: &str, + signing_key: Option<&Keypair>, + body: Vec, + stdout: &mut impl Write, +) -> Result<()> { + tracing::debug!("POST {post_url} ({} bytes)", body.len()); + let req = build_pack_post_request(client, post_url, service, &body, signing_key); + // Attach the body after signing so the bytes are moved, not cloned. + let resp = req + .body(body) + .send() + .with_context(|| format!("POST {post_url}"))?; + + if !resp.status().is_success() { + let status = resp.status(); + let err_body = read_error_body(resp); + let path = format!("/{service}"); + bail!( + "{}", + http_error_message("POST", &path, status, &err_body, None) + ); + } + + let bytes = resp.bytes().context("reading pack response")?; + tracing::debug!("pack response: {} bytes from node", bytes.len()); + stdout.write_all(&bytes)?; + stdout.flush()?; + Ok(()) +} + +/// Drive a git-upload-pack fetch as a v0 smart-HTTP stateless-RPC client. +/// +/// Git (over `connect`) speaks the stateful native protocol; the node serves +/// `git upload-pack --stateless-rpc`, which keeps no state between POSTs. Bridge +/// the two: capture the want block once, then for each flush-terminated have +/// batch POST a self-contained request (the wants plus every have accumulated so +/// far, then exactly one terminator) and stream the ACK/NAK back to git so it can +/// continue. The `done` round returns the pack. Every POST re-sends the wants and +/// all prior haves because the server is stateless; leaving an intermediate flush +/// in the body would truncate the negotiation, so each body carries exactly one +/// terminator. +fn negotiate_upload_pack( + client: &reqwest::blocking::Client, + post_url: &str, + service: &str, + signing_key: Option<&Keypair>, + stdin: &mut R, + stdout: &mut impl Write, +) -> Result<()> { + // Opening round: the want block, a bare `done`, or an empty/flush-only + // request from an up-to-date or aborted fetch. + let (wants, wend) = read_upload_pack_round(stdin)?; + match wend { + // Up-to-date with an empty request, truncated, or aborted before a + // terminator: nothing safe to POST. + RoundEnd::Eof => return Ok(()), + // A `done` with no preceding want-section flush: the bare-`done` request + // the single-round tests feed. Forward it verbatim as one POST. + RoundEnd::Done => { + let mut body = wants; + body.extend_from_slice(b"0009done\n"); + return post_pack_round(client, post_url, service, signing_key, body, stdout); } + // Normal: the want section ended with its flush; negotiate the haves. + // (An empty want block here is a flush-only opener and falls through to + // the loop, which POSTs nothing and returns at EOF.) + RoundEnd::Flush => {} } - Ok(buf) + // The node is stateless between POSTs, so re-send the wants and every have so + // far in each round. + let mut acc_haves: Vec = Vec::new(); + loop { + let (batch, bend) = read_upload_pack_round(stdin)?; + + if batch.is_empty() && bend == RoundEnd::Eof { + // git closed with no new haves (clone with wants+flush+done is handled + // by the Done arm below; this is an up-to-date flush-only opener or a + // clean abort): nothing more to POST. + return Ok(()); + } + + acc_haves.extend_from_slice(&batch); + + // Compose a self-contained stateless-RPC request: wants + one flush + all + // accumulated haves + exactly one terminator. No intermediate flush + // survives into the body, or the stateless server treats the first flush + // after the haves as end-of-request and truncates the negotiation. + let mut body = wants.clone(); + body.extend_from_slice(b"0000"); + body.extend_from_slice(&acc_haves); + + let final_round = match bend { + RoundEnd::Flush => { + body.extend_from_slice(b"0000"); + false + } + // A `done` round is final. A nonempty EOF-terminated batch is treated + // as final too (a test-shim convenience; a real pipe ends a fetch with + // `done`, and a genuine mid-negotiation EOF means git aborted). + RoundEnd::Done | RoundEnd::Eof => { + body.extend_from_slice(b"0009done\n"); + true + } + }; + + post_pack_round(client, post_url, service, signing_key, body, stdout)?; + + if final_round { + return Ok(()); + } + } } /// Strip the HTTP smart-protocol service announcement from a GET /info/refs response. @@ -1093,6 +1235,35 @@ mod tests { "a tampered @path must fail verification" ); } + + // #117: a multi-round POST body (wants + flush + accumulated haves + done) + // signs and verifies over its EXACT bytes. Each per-round POST a private + // fetch emits is a different accumulated body, and build_pack_post_request + // signs the same bytes it sends, so every round is independently accepted + // by the node verifier, not just the single-round `done` body. + let mut acc = Vec::new(); + acc.extend_from_slice(&pkt(&format!( + "want {} multi_ack_detailed side-band-64k\n", + "a".repeat(40) + ))); + acc.extend_from_slice(b"0000"); + acc.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + acc.extend_from_slice(&pkt(&format!("have {}\n", "2".repeat(40)))); + acc.extend_from_slice(&pkt("done\n")); + let post_url = "http://node.example/zOwner/myrepo/git-upload-pack"; + let req = build_pack_post_request(&client, post_url, "git-upload-pack", &acc, Some(&kp)) + .body(acc.clone()) + .build() + .unwrap(); + node_verifies( + "POST", + &path_and_query(&req), + &acc, + &header(&req, "signature-input"), + &header(&req, "signature"), + &header(&req, "content-digest"), + ) + .expect("a multi-round accumulated POST body must verify under the node's verifier"); } #[test] @@ -1316,4 +1487,435 @@ mod tests { assert!(help.contains("GITLAWB_NODE")); assert!(help.ends_with('\n')); } + + // ── #117 multi-round fetch negotiation ─────────────────────────────────── + + /// Encode a git pkt-line: 4-byte hex length (incl. the 4 bytes) + data. + fn pkt(data: &str) -> Vec { + format!("{:04x}{}", data.len() + 4, data).into_bytes() + } + + /// A realistic capability-bearing opening want line (git puts its capability + /// list on the first want), so the multi-round tests exercise the want shape + /// real git actually sends, not a bare `want `. + fn want_line(sha: &str) -> Vec { + pkt(&format!( + "want {sha} multi_ack_detailed side-band-64k ofs-delta agent=git/2.43\n" + )) + } + + /// A reader that yields `seed`, then BLOCKS until `gate` fires. Models git + /// holding the upload-pack pipe open after a have-batch flush, waiting for the + /// server's ACK/NAK before it sends more. A real pipe blocks here; an in-memory + /// Cursor would EOF and hide the bug, which is why the pre-#117 tests missed it. + struct BlockAfterSeed { + seed: io::Cursor>, + gate: std::sync::mpsc::Receiver<()>, + } + impl Read for BlockAfterSeed { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + let n = self.seed.read(buf)?; + if n > 0 { + return Ok(n); + } + let _ = self.gate.recv(); // block like a live pipe; unblock -> EOF + Ok(0) + } + } + + /// #117 primitive contract (matrix item 1): `read_upload_pack_round` returns at + /// a flush-terminated batch instead of buffering past it waiting for `done`. + /// The pre-#117 `read_upload_pack_request` blocked here (the deadlock). A + /// blocking reader proves the return under live-pipe semantics: if the reader + /// buffered past the flush it would block on the gate and the test would time out. + #[test] + fn read_upload_pack_round_returns_on_flush_without_done() { + let mut seed = Vec::new(); + seed.extend_from_slice(&want_line(&"a".repeat(40))); + seed.extend_from_slice(b"0000"); // end of wants + for i in 0..32u32 { + seed.extend_from_slice(&pkt(&format!("have {:040x}\n", i))); + } + seed.extend_from_slice(b"0000"); // have-batch flush; git awaits ACK, sends NO done + + let (tx, rx) = std::sync::mpsc::channel::<()>(); + let mut reader = BlockAfterSeed { + seed: io::Cursor::new(seed), + gate: rx, + }; + + let (done_tx, done_rx) = std::sync::mpsc::channel::<(Vec, RoundEnd)>(); + let handle = std::thread::spawn(move || { + // Opening round (wants) returns at the first flush; the have batch + // returns at the second flush. Neither may block for a `done`. + let _wants = read_upload_pack_round(&mut reader).unwrap(); + let haves = read_upload_pack_round(&mut reader).unwrap(); + let _ = done_tx.send(haves); + }); + + let got = done_rx.recv_timeout(std::time::Duration::from_secs(2)); + let _ = tx.send(()); // unblock the reader so the thread can exit + let _ = handle.join(); + + let (batch, end) = got.expect( + "read_upload_pack_round blocked past a flush-terminated have-batch \ + waiting for `done` (multi-round fetch deadlock #117)", + ); + assert_eq!(end, RoundEnd::Flush, "a flush terminates the round"); + assert!( + batch.windows(4).any(|w| w == b"have"), + "the have batch content is returned to the caller" + ); + } + + /// #117 core (matrix item 2): a multi-round negotiation issues MORE THAN ONE + /// POST, and each POST body is a self-contained stateless-RPC request: wants + + /// one flush + every have accumulated so far + exactly one terminator. The + /// round-2 body is asserted byte-for-byte, which pins both accumulation (round + /// 1's have is present) and the one-terminator invariant (no intermediate flush + /// survives). Pre-#117 this was a single POST. + #[test] + fn multi_round_fetch_posts_each_round_with_accumulated_wants_and_haves() { + let want_sha = "a".repeat(40); + let have1 = "1".repeat(40); + let have2 = "2".repeat(40); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + // Round 1 (flush-terminated): an ACK/NAK continuation, no pack yet. + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + // Round 2 (done): final response plus the (fake) pack. + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + + let wants = want_line(&want_sha); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&wants); + stdin_bytes.extend_from_slice(b"0000"); // end of wants + stdin_bytes.extend_from_slice(&pkt(&format!("have {have1}\n"))); + stdin_bytes.extend_from_slice(b"0000"); // round-1 flush: git awaits ACK + stdin_bytes.extend_from_slice(&pkt(&format!("have {have2}\n"))); + stdin_bytes.extend_from_slice(&pkt("done\n")); // round-2 terminator + + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("multi-round fetch should complete"); + + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 3, + "GET + two POSTs, one per negotiation round" + ); + assert!( + request_body(&requests[1]) + .windows(want_sha.len()) + .any(|w| w == want_sha.as_bytes()), + "round-1 POST must carry the wants" + ); + + // Round-2 body: wants + one flush + BOTH haves + done, with no flush + // between the haves (the accumulation + one-terminator invariant). + let mut expected = Vec::new(); + expected.extend_from_slice(&wants); + expected.extend_from_slice(b"0000"); + expected.extend_from_slice(&pkt(&format!("have {have1}\n"))); + expected.extend_from_slice(&pkt(&format!("have {have2}\n"))); + expected.extend_from_slice(&pkt("done\n")); + assert_eq!( + request_body(&requests[2]), + expected, + "round-2 POST body must be wants + flush + all accumulated haves + one done terminator" + ); + } + + /// Extract the HTTP request body (bytes after the header/body separator) from a + /// captured request string. pkt-lines are ASCII, so the lossy round-trip is exact. + fn request_body(request: &str) -> Vec { + match request.split_once("\r\n\r\n") { + Some((_, body)) => body.as_bytes().to_vec(), + None => Vec::new(), + } + } + + /// U4 / INV-8 / INV-12: a private-repo multi-round fetch escalates on the 404 + /// exactly once, then EVERY per-round POST carries the signature. Must-not: no + /// round goes out unsigned (an unsigned round 2 would 404 the owner's own fetch). + #[test] + fn private_multi_round_signs_every_post() { + let kp = Keypair::generate(); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "404 Not Found", + body: r#"{"message":"not found"}"#, + }, + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(two_round_stdin()); + handle_connect(&repo_base, "git-upload-pack", Some(&kp), &mut stdin) + .expect("private multi-round fetch should complete"); + + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 4, + "anon GET, signed GET retry, then two signed POSTs" + ); + assert!( + !requests[0].to_lowercase().contains("signature-input"), + "first GET is anonymous" + ); + assert!( + requests[1].to_lowercase().contains("signature-input"), + "GET retry is signed" + ); + assert!( + requests[2].to_lowercase().contains("signature-input"), + "round-1 POST is signed" + ); + assert!( + requests[3].to_lowercase().contains("signature-input"), + "round-2 POST is signed: no per-round POST may go out unsigned for a private repo" + ); + } + + /// U4: a public multi-round fetch stays anonymous on EVERY round even with a + /// keypair present. Must-not: no round signs (no DID disclosure on a public fetch). + #[test] + fn public_multi_round_stays_anonymous_every_post() { + let kp = Keypair::generate(); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(two_round_stdin()); + handle_connect(&repo_base, "git-upload-pack", Some(&kp), &mut stdin) + .expect("public multi-round fetch should complete"); + + let requests = server.join().unwrap(); + assert_eq!(requests.len(), 3, "GET + two POSTs, no retry"); + for (i, r) in requests.iter().enumerate() { + assert!( + !r.to_lowercase().contains("signature-input"), + "request {i} must stay anonymous on a public fetch" + ); + } + } + + /// U4 / INV-8 / INV-12: a denial mid-negotiation (round-2 POST 404) surfaces as + /// an error, not a silent empty/successful fetch, and does NOT re-trigger the + /// Phase-1 signed-retry escalation (that is a GET-only, Phase-1-only behavior). + #[test] + fn mid_negotiation_post_denial_surfaces_without_reescalation() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\n", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "404 Not Found", + body: r#"{"message":"repository is no longer readable"}"#, + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(two_round_stdin()); + let err = handle_connect(&repo_base, "git-upload-pack", None, &mut stdin).unwrap_err(); + let message = err.to_string(); + assert!( + message.contains("POST /git-upload-pack returned 404 Not Found"), + "the mid-negotiation denial must surface, not read as an empty/successful fetch" + ); + assert!(message.contains("repository is no longer readable")); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 3, + "GET + two POSTs, then bail: a POST denial must not re-run the Phase-1 escalation" + ); + } + + /// Two-round upload-pack request: wants, flush, have1, flush (round 1), have2, + /// done (round 2). Shared by the U4 signing/denial tests. + fn two_round_stdin() -> Vec { + let mut v = Vec::new(); + v.extend_from_slice(&want_line(&"a".repeat(40))); + v.extend_from_slice(b"0000"); + v.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + v.extend_from_slice(b"0000"); + v.extend_from_slice(&pkt(&format!("have {}\n", "2".repeat(40)))); + v.extend_from_slice(&pkt("done\n")); + v + } + + /// U5 (matrix item 5, plumbing only): the node's withheld-blob path + /// (`upload_pack_excluding`) ignores negotiation and answers the first POST + /// with NAK plus a full self-contained pack. The helper must forward that + /// response and terminate without hanging. Whether REAL git accepts a pack + /// where it expected an ACK continuation is U7's real-git scenario, not this + /// mock (a Cursor never reacts to the response). + #[test] + fn withheld_shaped_response_is_forwarded_without_hanging() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + // Withheld shape: NAK + full pack on the FIRST POST, negotiation ignored. + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfullselfcontainedpack", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + // A single flush-terminated round then EOF: the helper POSTs once, forwards + // the NAK+pack, and terminates at EOF rather than looping forever. + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + stdin_bytes.extend_from_slice(b"0000"); + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("withheld-shaped fetch should forward the pack and terminate"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "GET + one POST; the forwarded pack does not hang the helper" + ); + } + + /// U6 (matrix item 6): a fresh clone (wants, flush, done) issues exactly ONE POST. + #[test] + fn fresh_clone_issues_single_post() { + let want_sha = "a".repeat(40); + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&want_sha)); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt("done\n")); + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("clone should complete"); + let requests = server.join().unwrap(); + assert_eq!(requests.len(), 2, "fresh clone is exactly one POST"); + assert!(request_body(&requests[1]) + .windows(want_sha.len()) + .any(|w| w == want_sha.as_bytes())); + } + + /// U6 (matrix item 6): an up-to-date / flush-only opener issues ZERO POSTs and + /// completes without entering an ACK wait. + #[test] + fn flush_only_opener_skips_post() { + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(b"0000".to_vec()); // flush only, nothing to fetch + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("up-to-date fetch should complete"); + let requests = server.join().unwrap(); + assert_eq!(requests.len(), 1, "flush-only opener issues no POST"); + } + + /// U6 (matrix item 6): a single-shot fetch git resolved in one round (wants, + /// flush, haves, done, no intermediate flush) issues exactly ONE POST. Must-not: + /// the fix must not split a single-shot negotiation into multiple POSTs. + #[test] + fn single_shot_fetch_is_one_post() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "2".repeat(40)))); + stdin_bytes.extend_from_slice(&pkt("done\n")); // done with no intermediate flush + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("fetch should complete"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "a single-shot negotiation must be exactly one POST, not split" + ); + } } From e02ea757a7753ca6fe694fd5db876065caba2271 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 19:46:39 -0500 Subject: [PATCH 02/15] test(git-remote): real-git integration proof of the multi-round fetch loop (#117) A committed integration test drives a real `git fetch` through the built helper against a real `git upload-pack --stateless-rpc`, so the stateful-to-stateless ACK bridging is proven by execution rather than reasoned. The main.rs mock tests cannot falsify it: a pre-scripted Cursor never reacts to the server's ACKs. An in-test shim replicates the node's v0 serving; the fetch is forced to at least two negotiation rounds (a fixture that resolved in one round fails the test) and the resulting object graph is verified. The second scenario drives the withheld-blob shape (a full pack on the first POST, as upload_pack_excluding does) through real git and records the outcome: real git rejects a pack where it expected an ACK continuation ("expected ACK/NAK, got ..."), so a multi-round fetch of a withheld repo does not complete. The helper forwards and terminates cleanly rather than hanging, so this is a node-side concern, not a helper defect, left as a follow-up per the plan's Withheld-Path Decision. The test guards the helper's non-hang behavior and surfaces the break if the assumption ever changes. --- .../tests/real_git_fetch.rs | 436 ++++++++++++++++++ 1 file changed, 436 insertions(+) create mode 100644 crates/git-remote-gitlawb/tests/real_git_fetch.rs diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs new file mode 100644 index 00000000..094762b0 --- /dev/null +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -0,0 +1,436 @@ +//! #117 end-to-end: drive a REAL `git fetch` through the built helper against a +//! REAL `git upload-pack --stateless-rpc`, so the stateful-to-stateless bridging +//! is proven by execution, not reasoned. The unit tests in `main.rs` use a +//! pre-scripted Cursor and a canned HTTP mock, which cannot tell whether real git +//! (a stateful client over `connect`) actually parses the stateless-RPC server's +//! ACK responses and converges. That is the one load-bearing bet in the fix, and +//! only this test can falsify it. +//! +//! The in-test shim replicates the node's v0 smart-HTTP serving +//! (`gitlawb-node/src/git/smart_http.rs`): the info/refs advertisement wrapped in +//! the `# service=` pkt-line + flush, and each POST piped to +//! `git upload-pack --stateless-rpc`. The node crate's own tests already require +//! git, so committing this always-on is consistent with the suite. + +use std::io::{BufRead, BufReader, Read, Write}; +use std::net::{TcpListener, TcpStream}; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +/// git pkt-line: 4-byte hex length (incl. the 4 bytes) + data. +fn pkt(data: &[u8]) -> Vec { + let mut out = format!("{:04x}", data.len() + 4).into_bytes(); + out.extend_from_slice(data); + out +} + +fn unique_dir(tag: &str) -> PathBuf { + static COUNTER: AtomicUsize = AtomicUsize::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("gitlawb-u7-{}-{}-{}", tag, std::process::id(), n)); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +/// Run git in `dir` with deterministic identity/config, asserting success. +fn git(dir: &Path, args: &[&str]) -> Vec { + let out = Command::new("git") + .args([ + "-c", + "user.name=t", + "-c", + "user.email=t@example.invalid", + "-c", + "commit.gpgsign=false", + "-c", + "init.defaultBranch=main", + "-c", + "protocol.version=2", + ]) + .arg("-C") + .arg(dir) + .args(args) + .output() + .unwrap_or_else(|e| panic!("failed to spawn git {args:?}: {e}")); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out.stdout +} + +/// Run `git upload-pack --stateless-rpc [--advertise-refs] `, optionally +/// feeding `input` on stdin. Returns raw stdout (the wire bytes). +fn upload_pack(repo: &Path, advertise: bool, input: &[u8]) -> Vec { + let mut cmd = Command::new("git"); + cmd.arg("upload-pack").arg("--stateless-rpc"); + if advertise { + cmd.arg("--advertise-refs"); + } + cmd.arg(repo) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + let mut child = cmd.spawn().expect("spawn git upload-pack"); + let mut stdin = child.stdin.take().unwrap(); + let input = input.to_vec(); + let writer = std::thread::spawn(move || { + let _ = stdin.write_all(&input); + // drop closes stdin + }); + let out = child.wait_with_output().expect("wait upload-pack"); + writer.join().ok(); + assert!( + out.status.success(), + "git upload-pack failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out.stdout +} + +#[derive(Clone, Copy)] +enum ShimMode { + /// Faithful v0 serving: pipe each POST to `git upload-pack --stateless-rpc`. + Normal, + /// The withheld-blob shape: ignore negotiation and answer the FIRST POST with + /// a full self-contained pack (as `upload_pack_excluding` does on the node). + WithheldFirstPost, +} + +struct Shim { + base_url: String, + posts: Arc, + stop: Arc, + handle: Option>, +} + +impl Drop for Shim { + fn drop(&mut self) { + self.stop.store(true, Ordering::SeqCst); + // Nudge the accept loop with a throwaway connection so it observes `stop`. + if let Ok(addr) = self + .base_url + .trim_start_matches("http://") + .parse::() + { + let _ = TcpStream::connect_timeout(&addr, Duration::from_millis(200)); + } + if let Some(h) = self.handle.take() { + let _ = h.join(); + } + } +} + +/// Start a minimal smart-HTTP shim serving `repo` on 127.0.0.1. Handles the +/// upload-pack advertisement (GET) and pack negotiation (POST), counting POSTs. +fn start_shim(repo: PathBuf, mode: ShimMode) -> Shim { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let addr = listener.local_addr().unwrap(); + let base_url = format!("http://{addr}"); + let posts = Arc::new(AtomicUsize::new(0)); + let stop = Arc::new(AtomicBool::new(false)); + + let posts_t = posts.clone(); + let stop_t = stop.clone(); + let handle = std::thread::spawn(move || { + while !stop_t.load(Ordering::SeqCst) { + match listener.accept() { + Ok((stream, _)) => { + handle_conn(stream, &repo, mode, &posts_t); + } + Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => { + std::thread::sleep(Duration::from_millis(5)); + } + Err(_) => break, + } + } + }); + + Shim { + base_url, + posts, + stop, + handle: Some(handle), + } +} + +fn handle_conn(stream: TcpStream, repo: &Path, mode: ShimMode, posts: &AtomicUsize) { + stream.set_read_timeout(Some(Duration::from_secs(30))).ok(); + let mut reader = BufReader::new(stream); + + // Request line. + let mut request_line = String::new(); + if reader.read_line(&mut request_line).unwrap_or(0) == 0 { + return; + } + let mut parts = request_line.split_whitespace(); + let method = parts.next().unwrap_or("").to_string(); + let target = parts.next().unwrap_or("").to_string(); + + // Headers. + let mut content_length = 0usize; + loop { + let mut line = String::new(); + if reader.read_line(&mut line).unwrap_or(0) == 0 { + break; + } + if line == "\r\n" || line == "\n" { + break; + } + if let Some((name, value)) = line.split_once(':') { + if name.eq_ignore_ascii_case("content-length") { + content_length = value.trim().parse().unwrap_or(0); + } + } + } + + // Body. + let mut body = vec![0u8; content_length]; + if content_length > 0 { + reader.read_exact(&mut body).ok(); + } + + let (content_type, payload) = if method == "GET" && target.contains("/info/refs") { + // v0 advertisement, wrapped exactly as the node's info_refs does. + let adv = upload_pack(repo, true, b""); + let mut wrapped = pkt(b"# service=git-upload-pack\n"); + wrapped.extend_from_slice(b"0000"); + wrapped.extend_from_slice(&adv); + ("application/x-git-upload-pack-advertisement", wrapped) + } else if method == "POST" && target.ends_with("/git-upload-pack") { + let n = posts.fetch_add(1, Ordering::SeqCst); + let out = match mode { + ShimMode::Normal => upload_pack(repo, false, &body), + ShimMode::WithheldFirstPost if n == 0 => full_pack_response(repo), + ShimMode::WithheldFirstPost => upload_pack(repo, false, &body), + }; + ("application/x-git-upload-pack-result", out) + } else { + write_response(reader.into_inner(), "404 Not Found", "text/plain", b"no"); + return; + }; + + write_response(reader.into_inner(), "200 OK", content_type, &payload); +} + +/// The `upload_pack_excluding` shape: NAK plus a full self-contained pack, +/// negotiation ignored. Built by asking a real upload-pack for the tip with no +/// haves (want + done), which yields exactly `NAK` + the full pack. +fn full_pack_response(repo: &Path) -> Vec { + let head = String::from_utf8(git(repo, &["rev-parse", "HEAD"])).unwrap(); + let head = head.trim(); + let mut req = + pkt(format!("want {head} multi_ack_detailed side-band-64k ofs-delta\n").as_bytes()); + req.extend_from_slice(b"0000"); + req.extend_from_slice(&pkt(b"done\n")); + upload_pack(repo, false, &req) +} + +fn write_response(mut stream: TcpStream, status: &str, content_type: &str, body: &[u8]) { + let header = format!( + "HTTP/1.1 {status}\r\nContent-Type: {content_type}\r\nContent-Length: {}\r\nCache-Control: no-cache\r\nConnection: close\r\n\r\n", + body.len() + ); + let _ = stream.write_all(header.as_bytes()); + let _ = stream.write_all(body); + let _ = stream.flush(); +} + +/// Run `git fetch` in `clone` through the helper, with a hard timeout so a +/// regression to the deadlock fails fast instead of hanging the suite. +fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Output) { + let helper_bin = PathBuf::from(env!("CARGO_BIN_EXE_git-remote-gitlawb")); + let helper_dir = helper_bin.parent().unwrap().to_path_buf(); + let path_env = match std::env::var_os("PATH") { + Some(p) => { + let mut dirs = vec![helper_dir.clone()]; + dirs.extend(std::env::split_paths(&p)); + std::env::join_paths(dirs).unwrap() + } + None => helper_dir.clone().into_os_string(), + }; + + let mut child = Command::new("git") + .args(["-c", "protocol.version=2"]) + .arg("-C") + .arg(clone) + .args(["fetch", "origin", "main"]) + .env("PATH", path_env) + .env("GITLAWB_NODE", node_url) + .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn git fetch"); + + let deadline = Instant::now() + Duration::from_secs(30); + loop { + if let Some(_status) = child.try_wait().unwrap() { + let out = child.wait_with_output().unwrap(); + return (true, out); + } + if Instant::now() >= deadline { + let _ = child.kill(); + let out = child.wait_with_output().unwrap(); + return (false, out); // timed out: the deadlock signature + } + std::thread::sleep(Duration::from_millis(50)); + } +} + +/// Build a server repo with a shared history deep enough to force multi-round +/// negotiation (>~32 haves), plus a clone of it, then advance the server so the +/// fetch has something to negotiate. Returns (server, clone). +fn build_divergent_repos(shared_commits: usize) -> (PathBuf, PathBuf) { + let server = unique_dir("server"); + git(&server, &["init", "-q"]); + std::fs::write(server.join("base.txt"), b"base").unwrap(); + git(&server, &["add", "."]); + git(&server, &["commit", "-q", "-m", "base"]); + // A deep shared history the clone will offer as haves. + for i in 0..shared_commits { + git( + &server, + &[ + "commit", + "-q", + "--allow-empty", + "-m", + &format!("shared-{i}"), + ], + ); + } + + let clone = unique_dir("clone"); + // Clone over file:// so the clone shares the full history. + let status = Command::new("git") + .args(["clone", "-q"]) + .arg(&server) + .arg(&clone) + .output() + .unwrap(); + assert!( + status.status.success(), + "clone failed: {}", + String::from_utf8_lossy(&status.stderr) + ); + + // Advance the server so the fetch must transfer new objects. + for i in 0..3 { + git( + &server, + &[ + "commit", + "-q", + "--allow-empty", + "-m", + &format!("server-{i}"), + ], + ); + } + + // Point the clone's origin at the gitlawb:// scheme so git invokes the helper. + git( + &clone, + &[ + "remote", + "set-url", + "origin", + "gitlawb://did:key:zTESTOWNER/myrepo", + ], + ); + (server, clone) +} + +/// Matrix item 7, bridging half: a real multi-round `git fetch` through the +/// helper against a real `git upload-pack --stateless-rpc` completes, is observed +/// at >=2 POSTs (the executable form of the trigger; a fixture that resolved in +/// one round would fail this), and produces a correct object graph. +#[test] +fn real_git_multi_round_fetch_completes() { + let (server, clone) = build_divergent_repos(50); + let shim = start_shim(server.clone(), ShimMode::Normal); + + let (completed, out) = fetch_with_helper(&clone, &shim.base_url); + let posts = shim.posts.load(Ordering::SeqCst); + + assert!( + completed, + "git fetch did not complete within the timeout (deadlock signature). stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + out.status.success(), + "git fetch failed. stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + posts >= 2, + "fixture did not force multi-round negotiation (observed {posts} POST(s)); the bridging path was not exercised" + ); + + // The fetched tip is present and the clone's object graph is intact. + let server_head = String::from_utf8(git(&server, &["rev-parse", "HEAD"])).unwrap(); + let fetched = String::from_utf8(git(&clone, &["rev-parse", "FETCH_HEAD"])).unwrap(); + assert_eq!( + server_head.trim(), + fetched.trim(), + "FETCH_HEAD must match the server tip" + ); + git(&clone, &["fsck", "--full"]); + + cleanup(&[server, clone]); +} + +/// Matrix item 7, withheld half (the Withheld-Path Decision gate): the node's +/// `upload_pack_excluding` answers the FIRST POST with NAK plus a full pack, +/// mid-negotiation. Drive that shape through REAL git and record whether it +/// accepts a pack where it expected an ACK continuation. If real git rejects it, +/// this test captures the break mode that the decision's remedy addresses. +#[test] +fn real_git_withheld_shaped_first_post() { + let (server, clone) = build_divergent_repos(50); + let shim = start_shim(server.clone(), ShimMode::WithheldFirstPost); + + let (completed, out) = fetch_with_helper(&clone, &shim.base_url); + let posts = shim.posts.load(Ordering::SeqCst); + let stderr = String::from_utf8_lossy(&out.stderr).to_string(); + + // The helper itself must not hang or panic regardless of git's verdict. + assert!( + completed, + "helper hung on a withheld-shaped response (should forward-and-terminate, not deadlock). stderr:\n{stderr}" + ); + + if out.status.success() { + // Real git accepted the mid-negotiation pack: the withheld multi-round + // path works end to end with no extra handling. + let server_head = String::from_utf8(git(&server, &["rev-parse", "HEAD"])).unwrap(); + let fetched = String::from_utf8(git(&clone, &["rev-parse", "FETCH_HEAD"])).unwrap(); + assert_eq!(server_head.trim(), fetched.trim()); + git(&clone, &["fsck", "--full"]); + } else { + // Real git rejected the mid-negotiation pack. This is a genuine outcome, + // not a helper defect (the helper forwarded and terminated cleanly). The + // Withheld-Path Decision's remedy owns the fix; this branch documents the + // observed break so a regression in the assumption is visible in CI. + eprintln!( + "WITHHELD-PATH NOTE (#117): real git did NOT accept a NAK+pack mid-negotiation \ + (observed {posts} POST(s)). Per the Withheld-Path Decision this routes to a node-side \ + follow-up or an accepted withheld=full-clone limitation, not helper code. git stderr:\n{stderr}" + ); + } + + cleanup(&[server, clone]); +} + +fn cleanup(dirs: &[PathBuf]) { + for d in dirs { + let _ = std::fs::remove_dir_all(d); + } +} From bd05407880a71f182a853b85b15e93fa235e1f5f Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 19:53:24 -0500 Subject: [PATCH 03/15] fix(git-remote): skip content-free flush rounds in the fetch loop (#117) A code-review pass found the negotiation loop would POST once per bare `0000` flush that carried no haves, so a malformed `wants + N*0000 + EOF` stream amplified into N signed POSTs. Real git never sends a content-free mid-negotiation flush and the peer is upstream of the local git process, so this was not reachable in practice, but it left the loop unbounded per input. Skip a round that carries no new haves (an empty flush) and only POST rounds with have content or a terminator, which also makes the loop provably bounded by git's finite have set. The fresh-clone done-with-no-haves round still POSTs. --- crates/git-remote-gitlawb/src/main.rs | 52 ++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 49b3f9fe..9d8a7934 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -577,11 +577,16 @@ fn negotiate_upload_pack( loop { let (batch, bend) = read_upload_pack_round(stdin)?; - if batch.is_empty() && bend == RoundEnd::Eof { - // git closed with no new haves (clone with wants+flush+done is handled - // by the Done arm below; this is an up-to-date flush-only opener or a - // clean abort): nothing more to POST. - return Ok(()); + // Only POST when a round carries new haves or a terminator. A round with no + // new haves needs no request: an empty EOF ends the fetch, and a bare flush + // (git never sends a content-free flush mid-negotiation, but a malformed + // stream could) is skipped so a run of empty flushes cannot amplify into one + // signed POST each. A `done` round with no haves still POSTs (the fresh-clone + // case), handled by the terminator match below. + match (batch.is_empty(), bend) { + (true, RoundEnd::Eof) => return Ok(()), + (true, RoundEnd::Flush) => continue, + _ => {} } acc_haves.extend_from_slice(&batch); @@ -1918,4 +1923,41 @@ mod tests { "a single-shot negotiation must be exactly one POST, not split" ); } + + /// Hardening: content-free flush pkts between the wants and the first haves do + /// not each fire a POST. Real git never sends a bare mid-negotiation flush, but + /// a malformed stream must not amplify into one signed POST per empty flush; the + /// loop skips them and POSTs only the round that carries haves + done. + #[test] + fn repeated_bare_flushes_do_not_amplify_posts() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(b"0000"); // bare flush, no haves + stdin_bytes.extend_from_slice(b"0000"); // bare flush, no haves + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + stdin_bytes.extend_from_slice(&pkt("done\n")); + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("fetch with stray flushes should complete"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "content-free flushes must not each produce a POST" + ); + } } From 00e3f834021eb80cab775ae67618f2e5e60c5ac4 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 20:25:26 -0500 Subject: [PATCH 04/15] test(git-remote): close branch-coverage gaps in the fetch loop (#117) Adds executed coverage for the negotiation branches the first pass left unrun: an invalid pkt-line length (rejected, not underflowed), a non-EOF read error (propagated, not swallowed as end-of-stream), an immediately empty upload-pack request (skips the POST), a nonempty have-batch terminated by EOF (POSTed as a final done round), and an empty receive-pack body (skips the POST). Each was mutation-checked to fail without its guard, so the coverage is load-bearing rather than vacuous. --- crates/git-remote-gitlawb/src/main.rs | 108 ++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 9d8a7934..e2593179 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -1960,4 +1960,112 @@ mod tests { "content-free flushes must not each produce a POST" ); } + + // ── Branch-coverage closure: exercise the remaining changed arms ───────── + + /// A reader that fails with a non-EOF io error, to exercise the error + /// propagation arm of `read_upload_pack_round` (distinct from a clean EOF). + struct FailingReader; + impl Read for FailingReader { + fn read(&mut self, _buf: &mut [u8]) -> io::Result { + Err(io::Error::other("simulated read failure")) + } + } + + /// G4: a non-EOF read error propagates; it is not swallowed as end-of-stream. + #[test] + fn read_upload_pack_round_propagates_read_error() { + let mut r = FailingReader; + assert!(read_upload_pack_round(&mut r).is_err()); + } + + /// G1: an invalid pkt-line length (1..=3) is rejected rather than underflowing + /// `pkt_len - 4`. + #[test] + fn read_upload_pack_round_rejects_invalid_pkt_length() { + let mut r = io::Cursor::new(b"0001".to_vec()); // pkt_len 1, < 4 + let err = read_upload_pack_round(&mut r).unwrap_err(); + assert!( + err.to_string().contains("invalid pkt-line length"), + "unexpected error: {err}" + ); + } + + /// G2: an immediately-empty upload-pack request (EOF at the opening round, with + /// a 200 advertisement) skips the POST entirely. + #[test] + fn upload_pack_empty_request_skips_post() { + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(Vec::::new()); // immediate EOF + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("empty upload-pack request should skip the POST"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 1, + "empty request must issue the GET only, no POST" + ); + } + + /// G3: a nonempty have-batch terminated by EOF (no flush, no done) is POSTed as + /// the final round, with a `done` terminator appended. + #[test] + fn nonempty_eof_batch_posts_as_final_round() { + let (base_url, server) = serve_http(vec![ + TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }, + TestResponse { + request_line: "POST /zOwner/myrepo/git-upload-pack", + status: "200 OK", + body: "0008NAK\nPACKfake", + }, + ]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin_bytes = Vec::new(); + stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); + stdin_bytes.extend_from_slice(b"0000"); + stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); + // No trailing flush or done: the stream just ends (EOF) after the have. + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("nonempty EOF batch should POST as a final round"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 2, + "one POST for the EOF-terminated final round" + ); + assert!( + request_body(&requests[1]).ends_with(b"0009done\n"), + "an EOF-terminated final round is sent with a done terminator" + ); + } + + /// G5: an empty receive-pack (push) body skips the POST, unchanged from before. + #[test] + fn receive_pack_empty_body_skips_post() { + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-receive-pack", + status: "200 OK", + body: "0000", + }]); + let repo_base = format!("{base_url}/zOwner/myrepo"); + let mut stdin = io::Cursor::new(Vec::::new()); // empty push + handle_connect(&repo_base, "git-receive-pack", None, &mut stdin) + .expect("empty receive-pack should skip the POST"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 1, + "empty push must issue the GET only, no POST" + ); + } } From e3c322cdf15441931d944e58478e0a80b622a30f Mon Sep 17 00:00:00 2001 From: t Date: Mon, 13 Jul 2026 19:13:04 -0500 Subject: [PATCH 05/15] fix(git-remote): abort fetch on EOF without POSTing; harden the real-git test harness (#192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: a nonempty have-batch terminated by EOF means git aborted mid-negotiation, but the fetch loop folded RoundEnd::Eof into the Done arm, appended a synthetic 0009done, and POSTed it — so a cancelled private fetch still issued an authorized, signed upload-pack request and made the node generate and stream a full pack to a client that had gone away. Split the arm: only a real done pkt finalizes; a nonempty EOF terminates without a POST. The nonempty_eof_batch_posts_as_final_round test that locked in the wrong behavior is flipped to assert no POST. F2: fetch_with_helper piped stdout+stderr but only polled try_wait, so a fetch emitting more than an OS pipe buffer (~64 KiB) blocked git before it exited and the harness reported a false 30s deadlock. Drain both streams on reader threads while enforcing the deadline, so the deadline measures a real protocol hang only. F3: the integration repos were removed only on the success path (predictable pid/counter names), so a timeout/panic leaked real git repos and could poison a later run. Root them in a RAII tempfile::TempDir that drops on unwind. RED->GREEN: nonempty_eof_batch_aborts_without_posting (pre-fix POSTs a synthetic done → the aborted-negotiation POST fails against the single-response server; fixed makes one request, no POST). repos_are_cleaned_up_on_unwind (temp dir gone after a panic unwinds past the guard). Full git-remote-gitlawb suite 42 unit + 3 integration green, fmt + clippy clean. --- Cargo.lock | 12 +- crates/git-remote-gitlawb/Cargo.toml | 1 + crates/git-remote-gitlawb/src/main.rs | 58 +++--- .../tests/real_git_fetch.rs | 168 ++++++++++++++---- 4 files changed, 174 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d12daa43..83d0f8ab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3300,19 +3300,20 @@ dependencies = [ [[package]] name = "git-remote-gitlawb" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "gitlawb-core", "mockito", "reqwest", + "tempfile", "tracing", "tracing-subscriber", ] [[package]] name = "gitlawb-attest" -version = "0.5.0" +version = "0.5.1" dependencies = [ "base64", "ed25519-dalek", @@ -3329,7 +3330,7 @@ dependencies = [ [[package]] name = "gitlawb-core" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "base64", @@ -3356,7 +3357,7 @@ dependencies = [ [[package]] name = "gitlawb-node" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", @@ -3412,7 +3413,7 @@ dependencies = [ [[package]] name = "gl" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", @@ -3824,6 +3825,7 @@ dependencies = [ "reqwest", "serde", "serde_json", + "sha2", "thiserror 2.0.18", "tracing", ] diff --git a/crates/git-remote-gitlawb/Cargo.toml b/crates/git-remote-gitlawb/Cargo.toml index 24380c62..1e0d16ac 100644 --- a/crates/git-remote-gitlawb/Cargo.toml +++ b/crates/git-remote-gitlawb/Cargo.toml @@ -19,3 +19,4 @@ tracing-subscriber = { workspace = true } [dev-dependencies] mockito = "1" +tempfile = "3" diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index e2593179..7140e1fc 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -604,13 +604,17 @@ fn negotiate_upload_pack( body.extend_from_slice(b"0000"); false } - // A `done` round is final. A nonempty EOF-terminated batch is treated - // as final too (a test-shim convenience; a real pipe ends a fetch with - // `done`, and a genuine mid-negotiation EOF means git aborted). - RoundEnd::Done | RoundEnd::Eof => { + // Only a real `done` pkt finalizes the stateless request. + RoundEnd::Done => { body.extend_from_slice(b"0009done\n"); true } + // A nonempty batch terminated by EOF (no flush, no done) means git + // ABORTED mid-negotiation (#192, F1). Terminate WITHOUT POSTing: + // synthesizing a `done` here would make the node generate and stream a + // full pack — signed, for a private fetch — to a client that has gone + // away. The accumulated haves are discarded; there is no one to serve. + RoundEnd::Eof => return Ok(()), }; post_pack_round(client, post_url, service, signing_key, body, stdout)?; @@ -2012,40 +2016,40 @@ mod tests { ); } - /// G3: a nonempty have-batch terminated by EOF (no flush, no done) is POSTed as - /// the final round, with a `done` terminator appended. + /// G3 (#192, F1): a nonempty have-batch terminated by EOF (no flush, no done) + /// means git ABORTED mid-negotiation. The helper must terminate WITHOUT POSTing + /// — synthesizing a `done` and sending it would make the node generate and + /// stream a full pack (signed, for a private fetch) to a client that has gone + /// away. Only a real `done` pkt finalizes the stateless request. RED before the + /// fix (the folded `Done | Eof` arm POSTs a synthetic done → 2 requests), GREEN + /// after (only the info/refs GET, no upload-pack POST). #[test] - fn nonempty_eof_batch_posts_as_final_round() { - let (base_url, server) = serve_http(vec![ - TestResponse { - request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", - status: "200 OK", - body: "0000", - }, - TestResponse { - request_line: "POST /zOwner/myrepo/git-upload-pack", - status: "200 OK", - body: "0008NAK\nPACKfake", - }, - ]); + fn nonempty_eof_batch_aborts_without_posting() { + // Only the info/refs GET is served (serve_http accepts exactly N conns). The + // fixed helper makes exactly that one request and stops. The pre-fix code + // additionally POSTs the synthetic done; that POST hits the now-closed + // listener and errors, so handle_connect returns Err and the `.expect` below + // fires — the RED. The fixed code returns Ok with one recorded request. + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /zOwner/myrepo/info/refs?service=git-upload-pack", + status: "200 OK", + body: "0000", + }]); let repo_base = format!("{base_url}/zOwner/myrepo"); let mut stdin_bytes = Vec::new(); stdin_bytes.extend_from_slice(&want_line(&"a".repeat(40))); stdin_bytes.extend_from_slice(b"0000"); stdin_bytes.extend_from_slice(&pkt(&format!("have {}\n", "1".repeat(40)))); - // No trailing flush or done: the stream just ends (EOF) after the have. + // No trailing flush or done: the stream just ends (EOF) after the have — + // git aborted the negotiation. let mut stdin = io::Cursor::new(stdin_bytes); handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) - .expect("nonempty EOF batch should POST as a final round"); + .expect("an aborted (EOF) negotiation is a clean no-op, not an error"); let requests = server.join().unwrap(); assert_eq!( requests.len(), - 2, - "one POST for the EOF-terminated final round" - ); - assert!( - request_body(&requests[1]).ends_with(b"0009done\n"), - "an EOF-terminated final round is sent with a done terminator" + 1, + "an aborted EOF must NOT trigger an upload-pack POST (only the info/refs GET)" ); } diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index 094762b0..5a4d07a4 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -27,12 +27,15 @@ fn pkt(data: &[u8]) -> Vec { out } -fn unique_dir(tag: &str) -> PathBuf { - static COUNTER: AtomicUsize = AtomicUsize::new(0); - let n = COUNTER.fetch_add(1, Ordering::Relaxed); - let dir = std::env::temp_dir().join(format!("gitlawb-u7-{}-{}-{}", tag, std::process::id(), n)); - std::fs::create_dir_all(&dir).unwrap(); - dir +/// A server+clone fixture rooted in a single RAII temp dir (#192, F3). The +/// `TempDir` removes the whole tree on drop — including while unwinding from a +/// failed assertion, timeout, or panic — so a failing test never leaves real git +/// repos behind, and the randomized root name cannot collide with or poison a +/// later run the way the old pid/counter names could. +struct Repos { + server: PathBuf, + clone: PathBuf, + _tmp: tempfile::TempDir, } /// Run git in `dir` with deterministic identity/config, asserting success. @@ -255,39 +258,79 @@ fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Outpu None => helper_dir.clone().into_os_string(), }; - let mut child = Command::new("git") - .args(["-c", "protocol.version=2"]) + let mut cmd = Command::new("git"); + cmd.args(["-c", "protocol.version=2"]) .arg("-C") .arg(clone) .args(["fetch", "origin", "main"]) .env("PATH", path_env) .env("GITLAWB_NODE", node_url) - .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch") + .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch"); + run_bounded(cmd, Duration::from_secs(30)) +} + +/// Spawn `cmd`, draining stdout and stderr CONCURRENTLY on reader threads while +/// enforcing `timeout`. Returns `(completed_before_deadline, Output)` (#192, F2). +/// +/// The concurrent drain is the whole point: polling `try_wait` without reading lets +/// a child that writes more than an OS pipe buffer (~64 KiB) block on the full pipe +/// before it exits, so the deadline would trip on pipe backpressure and report a +/// false "deadlock signature" even while the child is making progress. Draining +/// continuously makes the deadline measure a real hang only. +fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Output) { + let mut child = cmd .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() - .expect("spawn git fetch"); + .expect("spawn child"); + + let mut out_pipe = child.stdout.take().expect("stdout piped"); + let mut err_pipe = child.stderr.take().expect("stderr piped"); + let out_reader = std::thread::spawn(move || { + let mut b = Vec::new(); + let _ = out_pipe.read_to_end(&mut b); + b + }); + let err_reader = std::thread::spawn(move || { + let mut b = Vec::new(); + let _ = err_pipe.read_to_end(&mut b); + b + }); - let deadline = Instant::now() + Duration::from_secs(30); - loop { - if let Some(_status) = child.try_wait().unwrap() { - let out = child.wait_with_output().unwrap(); - return (true, out); + let deadline = Instant::now() + timeout; + let completed = loop { + if child.try_wait().unwrap().is_some() { + break true; } if Instant::now() >= deadline { - let _ = child.kill(); - let out = child.wait_with_output().unwrap(); - return (false, out); // timed out: the deadlock signature + let _ = child.kill(); // closes the pipes so the readers finish + break false; // timed out: the real deadlock signature } std::thread::sleep(Duration::from_millis(50)); - } + }; + + // The child has exited (or been killed), so the pipes are closed and the reader + // threads run to completion. Reap the child and collect the drained output. + let status = child.wait().expect("reap child"); + let stdout = out_reader.join().expect("stdout reader"); + let stderr = err_reader.join().expect("stderr reader"); + ( + completed, + std::process::Output { + status, + stdout, + stderr, + }, + ) } /// Build a server repo with a shared history deep enough to force multi-round /// negotiation (>~32 haves), plus a clone of it, then advance the server so the /// fetch has something to negotiate. Returns (server, clone). -fn build_divergent_repos(shared_commits: usize) -> (PathBuf, PathBuf) { - let server = unique_dir("server"); +fn build_divergent_repos(shared_commits: usize) -> Repos { + let tmp = tempfile::TempDir::new().unwrap(); + let server = tmp.path().join("server"); + std::fs::create_dir_all(&server).unwrap(); git(&server, &["init", "-q"]); std::fs::write(server.join("base.txt"), b"base").unwrap(); git(&server, &["add", "."]); @@ -306,7 +349,7 @@ fn build_divergent_repos(shared_commits: usize) -> (PathBuf, PathBuf) { ); } - let clone = unique_dir("clone"); + let clone = tmp.path().join("clone"); // Clone over file:// so the clone shares the full history. let status = Command::new("git") .args(["clone", "-q"]) @@ -344,7 +387,11 @@ fn build_divergent_repos(shared_commits: usize) -> (PathBuf, PathBuf) { "gitlawb://did:key:zTESTOWNER/myrepo", ], ); - (server, clone) + Repos { + server, + clone, + _tmp: tmp, + } } /// Matrix item 7, bridging half: a real multi-round `git fetch` through the @@ -353,7 +400,10 @@ fn build_divergent_repos(shared_commits: usize) -> (PathBuf, PathBuf) { /// one round would fail this), and produces a correct object graph. #[test] fn real_git_multi_round_fetch_completes() { - let (server, clone) = build_divergent_repos(50); + // `repos` owns the RAII temp dir; keep it in scope so cleanup runs on drop, + // including while unwinding from any assertion below (#192, F3). + let repos = build_divergent_repos(50); + let (server, clone) = (repos.server.clone(), repos.clone.clone()); let shim = start_shim(server.clone(), ShimMode::Normal); let (completed, out) = fetch_with_helper(&clone, &shim.base_url); @@ -383,8 +433,63 @@ fn real_git_multi_round_fetch_completes() { "FETCH_HEAD must match the server tip" ); git(&clone, &["fsck", "--full"]); + // No manual cleanup: `repos` drops here (or on unwind) and removes the tree. +} - cleanup(&[server, clone]); +/// #192 (F3): a panic (any unwind) mid-test still removes the repos. The old +/// success-only `cleanup()` ran after the assertions, so a failing test leaked real +/// git repos; the RAII `TempDir` drops during unwind instead. Load-bearing: the +/// path exists inside the closure and must be gone once the panic has unwound past +/// the guard — a non-RAII cleanup would leave it behind. +#[test] +fn repos_are_cleaned_up_on_unwind() { + let captured = Arc::new(std::sync::Mutex::new(None::)); + let c = captured.clone(); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let repos = build_divergent_repos(1); + assert!( + repos._tmp.path().exists(), + "temp dir exists during the test" + ); + *c.lock().unwrap() = Some(repos._tmp.path().to_path_buf()); + panic!("simulated mid-test failure"); + })); + assert!(result.is_err(), "the closure panicked as designed"); + let path = captured + .lock() + .unwrap() + .take() + .expect("captured the temp path"); + assert!( + !path.exists(), + "the RAII temp dir must be removed on unwind, not leaked: {}", + path.display() + ); +} + +/// #192 (F2): `run_bounded` must DRAIN a child that emits more than an OS pipe +/// buffer, not deadlock. A child writing ~140 KiB to BOTH stdout and stderr and +/// then exiting must complete within the deadline. Under a `try_wait`-only harness +/// (no concurrent drain) the child blocks on the full pipe, the deadline trips, and +/// `completed` is false — the exact false "deadlock signature" F2 fixes. Reverting +/// the concurrent drain to poll-then-read turns this test RED (completed=false). +#[test] +fn run_bounded_drains_large_output_without_deadlock() { + // seq 1..=25000 is ~140 KiB on each stream — well past a ~64 KiB pipe buffer. + let mut cmd = Command::new("sh"); + cmd.arg("-c").arg("seq 1 25000; seq 1 25000 1>&2"); + let (completed, out) = run_bounded(cmd, Duration::from_secs(10)); + assert!( + completed, + "a child emitting >64 KiB must be drained and complete, not deadlock (F2)" + ); + assert!(out.status.success(), "the child exited cleanly"); + assert!( + out.stdout.len() > 64 * 1024 && out.stderr.len() > 64 * 1024, + "both streams exceeded a pipe buffer (stdout={}, stderr={})", + out.stdout.len(), + out.stderr.len() + ); } /// Matrix item 7, withheld half (the Withheld-Path Decision gate): the node's @@ -394,7 +499,10 @@ fn real_git_multi_round_fetch_completes() { /// this test captures the break mode that the decision's remedy addresses. #[test] fn real_git_withheld_shaped_first_post() { - let (server, clone) = build_divergent_repos(50); + // `repos` owns the RAII temp dir; keep it in scope so cleanup runs on drop, + // including while unwinding from any assertion below (#192, F3). + let repos = build_divergent_repos(50); + let (server, clone) = (repos.server.clone(), repos.clone.clone()); let shim = start_shim(server.clone(), ShimMode::WithheldFirstPost); let (completed, out) = fetch_with_helper(&clone, &shim.base_url); @@ -426,11 +534,5 @@ fn real_git_withheld_shaped_first_post() { ); } - cleanup(&[server, clone]); -} - -fn cleanup(dirs: &[PathBuf]) { - for d in dirs { - let _ = std::fs::remove_dir_all(d); - } + // No manual cleanup: `repos` drops here (or on unwind) and removes the tree. } From 4a3f59c06a8f360a3b3f4fc3c7e6a267a4ec565d Mon Sep 17 00:00:00 2001 From: t Date: Wed, 15 Jul 2026 08:28:25 -0500 Subject: [PATCH 06/15] test(git-remote): reap the whole fetch process group and make the withheld guard load-bearing (#192) Address jatmn's two P2 review findings on real_git_fetch.rs. F1 (INV-22): run_bounded killed only the top-level `git fetch` on the timeout path, so the git-remote-gitlawb helper descendant kept the inherited stdout/stderr pipe write-ends open and the reader joins blocked on the helper's ~300s HTTP timeout instead of the 30s deadline. Spawn the child with process_group(0) and, on timeout, tear the whole group down (SIGTERM, ESRCH poll, SIGKILL escalation, bounded cap, then reap) before joining, mirroring gitlawb-node/src/git/smart_http.rs. libc is added as a cfg(unix) dev-dep; non-unix keeps the child.kill() fallback so the always-on test still compiles. New regression run_bounded_reaps_descendants_holding_the_pipe: a backgrounded sleep inherits the pipe and outlives a 1s-deadline leader; reverting to a leader-only kill turns it RED (elapsed ~10s). F2 (INV-21): the non-success branch of real_git_withheld_shaped_first_post accepted any `git fetch` failure and only printed the POST count, so a failure before the first POST (broken advertisement, helper lookup, connection) passed as the expected withheld rejection. Assert posts >= 1 before accepting the nonzero exit. Forcing a pre-POST failure (0 POSTs) turns the assertion RED. --- Cargo.lock | 1 + crates/git-remote-gitlawb/Cargo.toml | 3 + .../tests/real_git_fetch.rs | 138 +++++++++++++++++- 3 files changed, 135 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 83d0f8ab..cfdda903 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3304,6 +3304,7 @@ version = "0.5.1" dependencies = [ "anyhow", "gitlawb-core", + "libc", "mockito", "reqwest", "tempfile", diff --git a/crates/git-remote-gitlawb/Cargo.toml b/crates/git-remote-gitlawb/Cargo.toml index 1e0d16ac..566ac6c4 100644 --- a/crates/git-remote-gitlawb/Cargo.toml +++ b/crates/git-remote-gitlawb/Cargo.toml @@ -20,3 +20,6 @@ tracing-subscriber = { workspace = true } [dev-dependencies] mockito = "1" tempfile = "3" + +[target.'cfg(unix)'.dev-dependencies] +libc = "0.2" diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index 5a4d07a4..990c3ec9 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -269,6 +269,63 @@ fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Outpu run_bounded(cmd, Duration::from_secs(30)) } +/// A synthetic non-success `ExitStatus` used only on the unix timeout path, where +/// `reap_group` has already consumed the child and the later `wait` returns ECHILD. +/// The timeout branch already reports `completed == false`, so the exact status is +/// not load-bearing; a killed/nonzero placeholder preserves the `Output` contract. +#[cfg(unix)] +fn exited_status() -> std::process::ExitStatus { + use std::os::unix::process::ExitStatusExt; + // Encode "terminated by SIGKILL" (signal 9), matching the group teardown. + std::process::ExitStatus::from_raw(libc::SIGKILL) +} + +#[cfg(not(unix))] +fn exited_status() -> std::process::ExitStatus { + // Non-unix never takes the reap-then-ECHILD path; a real `wait` always succeeds + // there, so this is unreachable in practice. Spawn a trivially-failing process + // to synthesize a nonzero status without an unstable constructor. + Command::new("cmd") + .args(["/C", "exit 1"]) + .status() + .expect("synthesize exit status") +} + +/// Tear down the child's whole process group on the timeout path (INV-22). +/// +/// With `process_group(0)` the child is its own group leader, so pgid == child pid. +/// SIGTERM the group, poll for ESRCH (every member gone) with a SIGKILL escalation +/// after a short grace, then a hard cap so a wedged process can never block the +/// caller unboundedly. Finally reap the leader so it does not linger as a zombie. +/// This closes ALL inherited pipe write-ends (leader AND the git-remote-gitlawb +/// descendant) so the reader joins in `run_bounded` return promptly. +#[cfg(unix)] +fn reap_group(child: &mut std::process::Child) { + let pgid = child.id() as i32; + // SAFETY: kill(2) takes only integers and borrows no Rust memory; ESRCH on an + // already-gone group is ignored below via the kill(-pgid, 0) probe. + unsafe { + libc::kill(-pgid, libc::SIGTERM); + } + for step in 0..100u32 { + // SAFETY: kill(-pgid, 0) only probes group liveness; no memory is touched. + // A nonzero return means ESRCH — every member of the group has exited. + if unsafe { libc::kill(-pgid, 0) } != 0 { + break; + } + if step == 20 { + // ~200ms SIGTERM grace elapsed; force the group down. Fires exactly once. + // SAFETY: same as above — integer-only kill, no borrowed memory. + unsafe { + libc::kill(-pgid, libc::SIGKILL); + } + } + std::thread::sleep(Duration::from_millis(10)); + } + // ~1s hard cap total; reap the leader (best-effort) so it is not left a zombie. + let _ = child.wait(); +} + /// Spawn `cmd`, draining stdout and stderr CONCURRENTLY on reader threads while /// enforcing `timeout`. Returns `(completed_before_deadline, Output)` (#192, F2). /// @@ -278,11 +335,19 @@ fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Outpu /// false "deadlock signature" even while the child is making progress. Draining /// continuously makes the deadline measure a real hang only. fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Output) { - let mut child = cmd - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .expect("spawn child"); + // Run the child in its own process group so the whole fetch tree (git plus the + // git-remote-gitlawb helper it spawns) can be torn down together on the timeout + // path. Without this, killing only the leader leaves the helper alive, holding + // the stderr pipe write-end until its own ~300s HTTP timeout, so the reader + // joins below block far past the deadline instead of returning promptly (INV-22). + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + cmd.process_group(0); + } + + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + let mut child = cmd.spawn().expect("spawn child"); let mut out_pipe = child.stdout.take().expect("stdout piped"); let mut err_pipe = child.stderr.take().expect("stderr piped"); @@ -303,6 +368,15 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp break true; } if Instant::now() >= deadline { + // Tear down the WHOLE group, not just the leader: the git-remote-gitlawb + // helper is a descendant that inherited the stdout/stderr pipe write-ends, + // so killing only `child` leaves those ends open and the reader joins wait + // on the helper's ~300s HTTP timeout instead of returning at the deadline. + // Signalling the group closes every write-end so the readers finish + // promptly (INV-22, mirrors gitlawb-node/src/git/smart_http.rs). + #[cfg(unix)] + reap_group(&mut child); + #[cfg(not(unix))] let _ = child.kill(); // closes the pipes so the readers finish break false; // timed out: the real deadlock signature } @@ -310,8 +384,13 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp }; // The child has exited (or been killed), so the pipes are closed and the reader - // threads run to completion. Reap the child and collect the drained output. - let status = child.wait().expect("reap child"); + // threads run to completion. Reap the child and collect the drained output. On + // the unix timeout path `reap_group` already reaped the leader, so `wait` here + // returns ECHILD; tolerate that and report the killed status the loop observed. + let status = child.wait().unwrap_or_else(|_| { + debug_assert!(!completed, "clean-exit path must still be reapable here"); + exited_status() + }); let stdout = out_reader.join().expect("stdout reader"); let stderr = err_reader.join().expect("stderr reader"); ( @@ -527,6 +606,18 @@ fn real_git_withheld_shaped_first_post() { // not a helper defect (the helper forwarded and terminated cleanly). The // Withheld-Path Decision's remedy owns the fix; this branch documents the // observed break so a regression in the assumption is visible in CI. + // + // Guard the path this test claims to record (INV-21): the nonzero exit is + // only the withheld rejection if the helper actually reached the shim and + // sent the withheld-shaped POST. A failure BEFORE the first POST — a broken + // advertisement, helper lookup, or connection — must not masquerade as the + // expected rejection, or a regression that never hits the shim would pass. + assert!( + posts >= 1, + "fetch failed with 0 POSTs to the shim: a pre-POST failure (broken \ + advertisement / helper lookup / connection) is NOT the withheld \ + rejection this test records. git stderr:\n{stderr}" + ); eprintln!( "WITHHELD-PATH NOTE (#117): real git did NOT accept a NAK+pack mid-negotiation \ (observed {posts} POST(s)). Per the Withheld-Path Decision this routes to a node-side \ @@ -536,3 +627,36 @@ fn real_git_withheld_shaped_first_post() { // No manual cleanup: `repos` drops here (or on unwind) and removes the tree. } + +/// #192 (F1, INV-22): on the timeout path `run_bounded` must tear down the WHOLE +/// process group, not just the leader. The leader here backgrounds a descendant +/// (`sleep 10 &`) that inherits the stderr pipe write-end and then `exec`s another +/// `sleep 10`, so the leader itself is long-lived too. With a 1s deadline the +/// timeout fires; if only the leader were killed, the surviving background +/// descendant would keep the pipe open and the reader joins would block for the +/// descendant's full ~10s lifetime. Signalling the group closes every write-end, +/// so `run_bounded` returns promptly. Reverting the fix to a leader-only +/// `child.kill()` turns this RED (elapsed ~10s, the assert below fires). +#[cfg(unix)] +#[test] +fn run_bounded_reaps_descendants_holding_the_pipe() { + let mut cmd = Command::new("sh"); + // Background a descendant that inherits the pipe FDs and outlives the leader, + // then exec another long sleep so the leader is not the only thing to reap. + cmd.arg("-c").arg("sleep 10 & exec sleep 10"); + + let start = Instant::now(); + let (completed, _out) = run_bounded(cmd, Duration::from_secs(1)); + let elapsed = start.elapsed(); + + assert!( + !completed, + "the leader outlived the 1s deadline, so run_bounded must report a timeout" + ); + assert!( + elapsed < Duration::from_secs(4), + "reader joins blocked on a surviving descendant instead of the deadline: \ + elapsed={elapsed:?} (expected <4s; a leader-only kill leaves the \ + backgrounded `sleep 10` holding the stderr pipe for its full lifetime)" + ); +} From 34040be6d9af9284f70b3c85878f28dfac62a4e4 Mon Sep 17 00:00:00 2001 From: t Date: Thu, 16 Jul 2026 00:26:39 -0500 Subject: [PATCH 07/15] test(git-remote): make the run_bounded harness portable to the shipped windows target (#192) The large-output and descendants tests re-invoke the test binary as their fixtures instead of spawning sh/seq, so they run on x86_64-pc-windows-msvc; the non-unix timeout path tears down the whole fetch tree with taskkill /T before joining the drainers instead of killing only the leader. Also folds the receive-pack POST block into post_pack_round (review nitpick), removing the duplicated status-check/forward logic. --- crates/git-remote-gitlawb/src/main.rs | 32 +--- .../tests/real_git_fetch.rs | 151 +++++++++++++++--- 2 files changed, 134 insertions(+), 49 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 7140e1fc..1901e758 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -310,30 +310,14 @@ fn handle_connect( return Ok(()); } - tracing::debug!("POST {post_url} ({} bytes)", request_body.len()); - let req = build_pack_post_request(&client, &post_url, service, &request_body, signing_key); - - // Attach the body after signing so the pack bytes are moved, not cloned — - // packs can be large and the clone doubled peak memory on push. - let pack_resp = req - .body(request_body) - .send() - .with_context(|| format!("POST {post_url}"))?; - - if !pack_resp.status().is_success() { - let status = pack_resp.status(); - let body = read_error_body(pack_resp); - let path = format!("/{service}"); - bail!("{}", http_error_message("POST", &path, status, &body, None)); - } - - let pack_bytes = pack_resp.bytes().context("reading pack response")?; - tracing::debug!("pack response: {} bytes from node", pack_bytes.len()); - - stdout.write_all(&pack_bytes)?; - stdout.flush()?; - - Ok(()) + post_pack_round( + &client, + &post_url, + service, + signing_key, + request_body, + &mut stdout, + ) } // ── Smart-protocol request builders ─────────────────────────────────────────── diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index 990c3ec9..c531ce47 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -282,9 +282,11 @@ fn exited_status() -> std::process::ExitStatus { #[cfg(not(unix))] fn exited_status() -> std::process::ExitStatus { - // Non-unix never takes the reap-then-ECHILD path; a real `wait` always succeeds - // there, so this is unreachable in practice. Spawn a trivially-failing process - // to synthesize a nonzero status without an unstable constructor. + // Non-unix has no ECHILD analog: a Windows `wait` is handle-based, so even + // after `reap_tree` has already reaped the leader on the timeout path, the + // later `wait` in `run_bounded` succeeds again against the still-open handle. + // This stays unreachable in practice; spawn a trivially-failing process to + // synthesize a nonzero status without an unstable constructor. Command::new("cmd") .args(["/C", "exit 1"]) .status() @@ -326,6 +328,30 @@ fn reap_group(child: &mut std::process::Child) { let _ = child.wait(); } +/// Tear down the child's whole process tree on the timeout path (INV-22): the +/// non-unix counterpart of `reap_group`. +/// +/// `taskkill /T /F` walks the parent-pid tree from the leader and force-kills +/// every member, so ALL inherited pipe write-ends (leader AND the +/// git-remote-gitlawb descendant) are closed and the reader joins in +/// `run_bounded` return promptly instead of waiting out the helper's ~300s HTTP +/// timeout. taskkill ships in System32 on every supported Windows, which avoids +/// both a windows-only dev-dependency (a Job Object binding) and `unsafe` in +/// test code. Known caveat: /T resolves the parent-pid tree at invocation time, +/// which is sufficient for this harness because git and the helper are both +/// still alive (blocked, not exiting) at the deadline. The `child.kill()` below +/// is a best-effort leader-only fallback for the taskkill-unavailable case; +/// finally reap the leader so it does not linger, mirroring `reap_group`'s +/// contract. +#[cfg(not(unix))] +fn reap_tree(child: &mut std::process::Child) { + let _ = Command::new("taskkill") + .args(["/T", "/F", "/PID", &child.id().to_string()]) + .output(); + let _ = child.kill(); + let _ = child.wait(); +} + /// Spawn `cmd`, draining stdout and stderr CONCURRENTLY on reader threads while /// enforcing `timeout`. Returns `(completed_before_deadline, Output)` (#192, F2). /// @@ -368,16 +394,17 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp break true; } if Instant::now() >= deadline { - // Tear down the WHOLE group, not just the leader: the git-remote-gitlawb + // Tear down the WHOLE tree, not just the leader: the git-remote-gitlawb // helper is a descendant that inherited the stdout/stderr pipe write-ends, // so killing only `child` leaves those ends open and the reader joins wait // on the helper's ~300s HTTP timeout instead of returning at the deadline. - // Signalling the group closes every write-end so the readers finish - // promptly (INV-22, mirrors gitlawb-node/src/git/smart_http.rs). + // Taking down every member (the unix group signal, the windows taskkill + // tree kill) closes every write-end so the readers finish promptly + // (INV-22, mirrors gitlawb-node/src/git/smart_http.rs). #[cfg(unix)] reap_group(&mut child); #[cfg(not(unix))] - let _ = child.kill(); // closes the pipes so the readers finish + reap_tree(&mut child); break false; // timed out: the real deadlock signature } std::thread::sleep(Duration::from_millis(50)); @@ -403,6 +430,81 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp ) } +// --------------------------------------------------------------------------- +// Self-exec fixtures (#192 review, portability of the run_bounded tests). +// +// The harness tests below need child processes with controlled behavior: a bulk +// writer and a pipe-holding descendant tree. Spawning `sh` for these breaks the +// shipped x86_64-pc-windows-msvc target, where neither `sh` nor `seq` is a +// guaranteed dependency; the one executable guaranteed present on every target +// is this test binary itself. Each fixture is an `#[ignore]`d test that no-ops +// unless its GL_TEST_FIXTURE mode is set, so normal `cargo test` runs skip them +// and even an explicit `--ignored` run without the env var does nothing. +// `fixture_command` builds the re-invocation: the positional libtest filter +// plus `--exact` selects exactly one test, `--ignored` opts into it, and +// `--nocapture` keeps libtest from interposing on the streams. +// --------------------------------------------------------------------------- + +/// Build a Command that re-invokes this test binary to run one fixture test. +fn fixture_command(fixture_test: &str, mode: &str) -> Command { + let mut cmd = Command::new(std::env::current_exe().expect("current_exe")); + cmd.args([fixture_test, "--exact", "--ignored", "--nocapture"]) + .env("GL_TEST_FIXTURE", mode); + cmd +} + +/// Fixture: write ~136 KiB of numbered lines to BOTH stdout and stderr, then +/// exit 0. Direct handle writes (not println!) so libtest output capture cannot +/// swallow the bytes. The harness noise libtest prints ("running 1 test", the +/// result line) also lands on stdout; the caller's >64 KiB assertions are +/// insensitive to it. +#[test] +#[ignore = "self-exec fixture: only runs under GL_TEST_FIXTURE=emit"] +fn fixture_emit_large_output() { + if std::env::var("GL_TEST_FIXTURE").ok().as_deref() != Some("emit") { + return; + } + let mut payload = Vec::with_capacity(160 * 1024); + for i in 1..=25_000u32 { + writeln!(payload, "{i}").expect("write to Vec"); + } + std::io::stdout().write_all(&payload).expect("write stdout"); + std::io::stderr().write_all(&payload).expect("write stderr"); +} + +/// Fixture: sleep 10s while holding whatever stdio handles were inherited. +#[test] +#[ignore = "self-exec fixture: only runs under GL_TEST_FIXTURE=sleep"] +fn fixture_sleep() { + if std::env::var("GL_TEST_FIXTURE").ok().as_deref() != Some("sleep") { + return; + } + std::thread::sleep(Duration::from_secs(10)); +} + +/// Fixture: spawn a grandchild (`fixture_sleep`) whose stdio is inherited, so a +/// DESCENDANT holds the caller's pipe write-ends, then stay alive 10s so the +/// leader is long-lived too. This mirrors the retired +/// `sh -c "sleep 10 & exec sleep 10"` shape: one leader plus one descendant, +/// both holding the pipes well past any deadline the caller sets. +#[test] +#[ignore = "self-exec fixture: only runs under GL_TEST_FIXTURE=hold"] +fn fixture_hold_pipe_with_descendant() { + if std::env::var("GL_TEST_FIXTURE").ok().as_deref() != Some("hold") { + return; + } + // Stdio is inherited by default, so the grandchild holds the same pipe + // write-ends the caller handed this leader. + let mut grandchild = fixture_command("fixture_sleep", "sleep") + .spawn() + .expect("spawn grandchild fixture"); + std::thread::sleep(Duration::from_secs(10)); + // Reap on the natural-exit path (both sleeps are 10s, so this returns almost + // immediately). Under the harness the whole tree is killed at the deadline, + // long before this line runs. + let _ = grandchild.wait(); +} + /// Build a server repo with a shared history deep enough to force multi-round /// negotiation (>~32 haves), plus a clone of it, then advance the server so the /// fetch has something to negotiate. Returns (server, clone). @@ -547,16 +649,16 @@ fn repos_are_cleaned_up_on_unwind() { } /// #192 (F2): `run_bounded` must DRAIN a child that emits more than an OS pipe -/// buffer, not deadlock. A child writing ~140 KiB to BOTH stdout and stderr and +/// buffer, not deadlock. A child writing ~136 KiB to BOTH stdout and stderr and /// then exiting must complete within the deadline. Under a `try_wait`-only harness /// (no concurrent drain) the child blocks on the full pipe, the deadline trips, and /// `completed` is false — the exact false "deadlock signature" F2 fixes. Reverting /// the concurrent drain to poll-then-read turns this test RED (completed=false). #[test] fn run_bounded_drains_large_output_without_deadlock() { - // seq 1..=25000 is ~140 KiB on each stream — well past a ~64 KiB pipe buffer. - let mut cmd = Command::new("sh"); - cmd.arg("-c").arg("seq 1 25000; seq 1 25000 1>&2"); + // The emit fixture writes ~136 KiB per stream, well past a ~64 KiB pipe + // buffer (self-exec, not `sh -c seq`: portable to the shipped windows target). + let cmd = fixture_command("fixture_emit_large_output", "emit"); let (completed, out) = run_bounded(cmd, Duration::from_secs(10)); assert!( completed, @@ -629,21 +731,20 @@ fn real_git_withheld_shaped_first_post() { } /// #192 (F1, INV-22): on the timeout path `run_bounded` must tear down the WHOLE -/// process group, not just the leader. The leader here backgrounds a descendant -/// (`sleep 10 &`) that inherits the stderr pipe write-end and then `exec`s another -/// `sleep 10`, so the leader itself is long-lived too. With a 1s deadline the -/// timeout fires; if only the leader were killed, the surviving background -/// descendant would keep the pipe open and the reader joins would block for the -/// descendant's full ~10s lifetime. Signalling the group closes every write-end, -/// so `run_bounded` returns promptly. Reverting the fix to a leader-only -/// `child.kill()` turns this RED (elapsed ~10s, the assert below fires). -#[cfg(unix)] +/// process tree, not just the leader. The hold fixture spawns a grandchild that +/// inherits the pipe write-ends and sleeps 10s, and the leader stays alive 10s +/// too. With a 1s deadline the timeout fires; if only the leader were killed, +/// the surviving grandchild would keep the pipes open and the reader joins would +/// block for its full ~10s lifetime. Tearing down every member closes every +/// write-end, so `run_bounded` returns promptly. This runs on every target, so +/// the unix group signal and the windows taskkill tree kill are each covered on +/// the platform where they compile. Reverting either teardown to a leader-only +/// `child.kill()` turns this RED there (elapsed ~10s, the assert below fires). #[test] fn run_bounded_reaps_descendants_holding_the_pipe() { - let mut cmd = Command::new("sh"); - // Background a descendant that inherits the pipe FDs and outlives the leader, - // then exec another long sleep so the leader is not the only thing to reap. - cmd.arg("-c").arg("sleep 10 & exec sleep 10"); + // A long-lived leader plus a pipe-holding descendant (the self-exec + // replacement for `sh -c "sleep 10 & exec sleep 10"`). + let cmd = fixture_command("fixture_hold_pipe_with_descendant", "hold"); let start = Instant::now(); let (completed, _out) = run_bounded(cmd, Duration::from_secs(1)); @@ -657,6 +758,6 @@ fn run_bounded_reaps_descendants_holding_the_pipe() { elapsed < Duration::from_secs(4), "reader joins blocked on a surviving descendant instead of the deadline: \ elapsed={elapsed:?} (expected <4s; a leader-only kill leaves the \ - backgrounded `sleep 10` holding the stderr pipe for its full lifetime)" + grandchild fixture holding the pipes for its full lifetime)" ); } From e5baece67e7e8e4b8486af68cde1ef9aeca9192b Mon Sep 17 00:00:00 2001 From: t Date: Fri, 17 Jul 2026 23:13:46 -0500 Subject: [PATCH 08/15] fix(node): count a completed fetch once per fetch, not per stateless-RPC POST git_upload_pack recorded a fetch and observed a pack size on every successful POST. In HTTP stateless-RPC the plain serve path streams a pack only on the finalizing `done` round, so an N-round incremental fetch was reported as N completed fetches. The filtered path (upload_pack_excluding) instead serves a self-contained full pack on the single POST that reaches it, with no `done`, so gating purely on `done` would drop those private (path-scoped) fetches to zero. Count once on the POST that actually completes a fetch: the `done` round on the plain path (request_finalizes_fetch parses pkt-lines and fails closed on a malformed body), or the one filtered-serve POST (served_filtered_pack). should_count_fetch captures the decision; a completed fetch is exactly one such POST, so a multi-round negotiation is never double-counted. The observe_pack_size call still measures the request body, not the served pack; that mislabel predates this change and is left as a follow-up. Verified by execution: a three-round plain fetch and a no-`done` filtered serve each record exactly one completion; reverting the gate to count per POST makes the plain fetch record 3, and breaking the decision to AND drops both to 0. --- crates/gitlawb-node/src/api/repos.rs | 137 ++++++++++++++++++++++++++- crates/gitlawb-node/src/metrics.rs | 11 +++ 2 files changed, 146 insertions(+), 2 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index b9cbc352..8f52ba63 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -642,11 +642,19 @@ pub async fn git_upload_pack( .await .map_err(|e| AppError::Git(e.to_string()))?; let body_len = body.len(); + // Whether this POST finalized negotiation (carries `done`), computed before + // `body` is moved into upload_pack. Gates the completed-fetch metric below. + let finalizes_fetch = request_finalizes_fetch(&body); // 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); + // The filtered serve path (upload_pack_excluding) replies NAK + a self-contained + // full pack regardless of negotiation, completing a fetch on the single POST that + // reaches it even when that POST carries no `done`. Track it so the completed- + // fetch metric counts that path too (#192 F1, filtered case). + let mut served_filtered_pack = false; let resp = if !visibility_pack::has_path_scoped_rule(&rules) { smart_http::upload_pack(&disk_path, body, git_timeout).await } else { @@ -675,6 +683,7 @@ pub async fn git_upload_pack( if withheld.is_empty() { smart_http::upload_pack(&disk_path, body, git_timeout).await } else { + served_filtered_pack = true; tracing::info!(repo = %name, caller = ?caller, withheld = withheld.len(), "serving filtered pack"); smart_http::upload_pack_excluding(&disk_path, body, &withheld).await } @@ -690,11 +699,68 @@ pub async fn git_upload_pack( } app })?; - crate::metrics::record_fetch(&format!("{owner}/{name}")); - crate::metrics::observe_pack_size(body_len as f64); + // Count a completed fetch (and observe the pack) only on the POST that actually + // completes one. On the plain path that is the finalizing `done` round; counting + // per POST would record an N-round stateless-RPC fetch as N completions (#192 + // F1). The filtered path serves its whole pack on one no-`done` POST, counted via + // served_filtered_pack. Either way a completed fetch is exactly one such POST. + // NOTE: observe_pack_size still measures the request body, not the served pack; + // that mislabel predates this change and is tracked as a follow-up. + if should_count_fetch(finalizes_fetch, served_filtered_pack) { + crate::metrics::record_fetch(&format!("{owner}/{name}")); + crate::metrics::observe_pack_size(body_len as f64); + } Ok(resp) } +/// Whether an upload-pack POST completed a fetch and should be counted once. +/// +/// The plain serve path streams a pack only on the finalizing `done` round +/// (`finalizes_fetch`); the filtered path (`upload_pack_excluding`) serves a +/// self-contained pack on the one POST that reaches it regardless of negotiation +/// (`served_filtered_pack`). A completed fetch is exactly one such POST, so this +/// never double-counts a multi-round negotiation. +fn should_count_fetch(finalizes_fetch: bool, served_filtered_pack: bool) -> bool { + finalizes_fetch || served_filtered_pack +} + +/// True when an upload-pack request body carries a `done` pkt-line, i.e. the +/// client finished negotiation and is asking the server to stream the pack. +/// +/// The HTTP smart protocol runs upload-pack as stateless RPC: the client sends one +/// `git-upload-pack` POST per negotiation round, but only the finalizing round +/// sends `done`; the earlier flush-terminated rounds negotiate common history and +/// produce no pack. Counting a fetch only when this returns true keeps an N-round +/// incremental fetch from being recorded as N completed fetches (#192 F1). Parses +/// pkt-lines and fails closed (returns false) on a malformed body, so a garbled +/// request is never counted. +fn request_finalizes_fetch(body: &[u8]) -> bool { + let mut i = 0; + while i + 4 <= body.len() { + let Ok(hdr) = std::str::from_utf8(&body[i..i + 4]) else { + return false; + }; + let Ok(len) = usize::from_str_radix(hdr, 16) else { + return false; + }; + // 0000/0001/0002 are flush/delim/response-end markers: a 4-byte header + // with no payload. Advance past the header and keep scanning. + if len < 4 { + i += 4; + continue; + } + if i + len > body.len() { + return false; // truncated/malformed: do not over-count + } + let payload = &body[i + 4..i + len]; + if payload.strip_suffix(b"\n").unwrap_or(payload) == b"done" { + return true; + } + i += len; + } + false +} + /// Decide whether the owner-push gate rejects a `git-receive-pack` request. /// /// Returns `Some(error)` when the push must be rejected, `None` when it may @@ -1836,6 +1902,73 @@ mod tests { const OWNER_SHORT: &str = "z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"; const STRANGER_DID: &str = "did:key:z6Mkffonly5tranger0000000000000000000000000000000"; + #[test] + fn upload_pack_request_finalizes_only_with_done_pktline() { + let want = "0032want 1111111111111111111111111111111111111111\n"; + let have = "0032have 2222222222222222222222222222222222222222\n"; + // Finalizing round: wants + flush + done. + let done_round = format!("{want}00000009done\n").into_bytes(); + assert!(request_finalizes_fetch(&done_round)); + // Negotiation-only round: wants + flush + haves + flush, no done. + let nego_round = format!("{want}0000{have}0000").into_bytes(); + assert!(!request_finalizes_fetch(&nego_round)); + // Degenerate: empty and a bare flush never finalize. + assert!(!request_finalizes_fetch(b"")); + assert!(!request_finalizes_fetch(b"0000")); + // `done` with no trailing newline (0008done) still finalizes. + assert!(request_finalizes_fetch(b"00000008done")); + // A payload that merely contains the substring "done" is not a done pkt + // (000c -> len 12 -> payload "wantdone"). + assert!(!request_finalizes_fetch(b"000cwantdone")); + // A malformed length prefix does not panic and does not count. + assert!(!request_finalizes_fetch(b"zzzzdone\n")); + } + + #[test] + fn should_count_fetch_counts_done_or_filtered_but_not_bare_negotiation() { + assert!(should_count_fetch(true, false)); // plain: finalizing `done` round + assert!(should_count_fetch(false, true)); // filtered: full pack, no `done` + assert!(should_count_fetch(true, true)); // filtered fresh clone (want+done) + assert!(!should_count_fetch(false, false)); // plain negotiation-only round + } + + #[test] + fn fetch_completion_counts_once_per_fetch_plain_and_filtered() { + crate::metrics::init("0.0.0-test", "did:key:test"); + + let want = "0032want 1111111111111111111111111111111111111111\n"; + let nego = format!("{want}0000").into_bytes(); // negotiation-only, no done + let done = format!("{want}00000009done\n").into_bytes(); // finalizing + + // Plain path: one POST per round, only the finalizing `done` round streams a + // pack. Drive the same decision the handler uses; an N-round fetch counts once. + let plain = "fetchgate/plain-counts-once"; + let before = crate::metrics::fetch_count_for_test(plain); + for body in [nego.as_slice(), nego.as_slice(), done.as_slice()] { + if should_count_fetch(request_finalizes_fetch(body), false) { + crate::metrics::record_fetch(plain); + } + } + assert_eq!( + crate::metrics::fetch_count_for_test(plain) - before, + 1, + "a plain multi-round fetch must record exactly one completed fetch" + ); + + // Filtered path: upload_pack_excluding serves a self-contained pack on the one + // POST that reaches it, which carries no `done`. It must still count once. + let filtered = "fetchgate/filtered-counts-once"; + let before = crate::metrics::fetch_count_for_test(filtered); + if should_count_fetch(request_finalizes_fetch(&nego), true) { + crate::metrics::record_fetch(filtered); + } + assert_eq!( + crate::metrics::fetch_count_for_test(filtered) - before, + 1, + "a filtered fetch served without `done` must still record one completed fetch" + ); + } + #[test] fn git_service_app_error_classifies_timeout_bad_request_and_git() { // GitServiceTimeout carried through anyhow -> 504 Timeout. diff --git a/crates/gitlawb-node/src/metrics.rs b/crates/gitlawb-node/src/metrics.rs index a21c6d8b..ebe0ab1d 100644 --- a/crates/gitlawb-node/src/metrics.rs +++ b/crates/gitlawb-node/src/metrics.rs @@ -218,6 +218,17 @@ pub fn record_fetch(repo: &str) { } } +/// Test-only: current `gitlawb_fetches_total` value for `repo` (0 if the registry +/// is not initialized). Lets api-layer tests assert the completed-fetch count with +/// a unique label instead of scraping the encoded text. +#[cfg(test)] +pub fn fetch_count_for_test(repo: &str) -> u64 { + FETCHES + .get() + .map(|c| c.with_label_values(&[repo]).get()) + .unwrap_or(0) +} + /// Record one HTTP signature check that passed. #[allow(dead_code)] // wired in a follow-up; helpers are part of the public metrics surface pub fn record_auth_success(route: &str) { From 6e78eb2eb301248889cda15cac4cf97c3f3315c5 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 17 Jul 2026 23:13:46 -0500 Subject: [PATCH 09/15] test(git-remote): bound the fetch reader joins when the leader exits before a pipe-holder run_bounded only tore down the process group on the deadline path. When the git leader exits on its own but a descendant (the git-remote-gitlawb helper mid-HTTP request) outlives it holding the inherited stdout/stderr write-ends, the reader joins blocked on the helper's ~300s timeout, past the deadline run_bounded promises. Close the group on the clean-exit path too, guarded by a kill(-pgid, 0) liveness probe so a recycled pid is never signalled (the process-group ID stays reserved while any member is alive). Adds a leader-exits-first fixture and a regression test. Verified by execution: with the clean-path reap removed the new test blocks ~10s on the descendant (RED); with the reap it returns promptly. --- .../tests/real_git_fetch.rs | 84 ++++++++++++++++++- 1 file changed, 80 insertions(+), 4 deletions(-) diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index c531ce47..56e8edae 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -410,14 +410,33 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp std::thread::sleep(Duration::from_millis(50)); }; - // The child has exited (or been killed), so the pipes are closed and the reader - // threads run to completion. Reap the child and collect the drained output. On - // the unix timeout path `reap_group` already reaped the leader, so `wait` here - // returns ECHILD; tolerate that and report the killed status the loop observed. + // Collect the leader's status. On the timeout path `reap_group` already reaped + // it, so `wait` returns ECHILD; tolerate that and report the killed status the + // loop observed. On the clean path `try_wait` above already reaped it, so this + // returns the cached real status. let status = child.wait().unwrap_or_else(|_| { debug_assert!(!completed, "clean-exit path must still be reapable here"); exited_status() }); + + // A leader that exits on its own does NOT guarantee the pipes are closed: a + // descendant (the git-remote-gitlawb helper mid-HTTP request) can outlive the + // leader while still holding the inherited stdout/stderr write-ends, so the + // reader joins below would block on its ~300s HTTP timeout, past the deadline + // this function promises (#192 F2, INV-22). The timeout path already tore the + // group down; on the clean path, close it here too. A process-group ID stays + // reserved while any member is alive, so signalling the group is safe as long as + // `kill(-pgid, 0)` still reports a live member; once the group is empty the pipes + // are closed anyway and the leader's reaped pid may have been recycled, so skip. + #[cfg(unix)] + if completed { + let pgid = child.id() as i32; + // SAFETY: kill(-pgid, 0) only probes group liveness; it borrows no memory. + if unsafe { libc::kill(-pgid, 0) } == 0 { + reap_group(&mut child); + } + } + let stdout = out_reader.join().expect("stdout reader"); let stderr = err_reader.join().expect("stderr reader"); ( @@ -761,3 +780,60 @@ fn run_bounded_reaps_descendants_holding_the_pipe() { grandchild fixture holding the pipes for its full lifetime)" ); } + +/// Fixture: spawn a detached grandchild (`fixture_sleep`) that inherits the stdio +/// pipes, then the LEADER returns immediately. This is the leader-exits-first +/// shape (#192 F2): the leader is gone almost at once, well before any deadline, +/// but the descendant keeps the caller's pipe write-ends open for its full +/// lifetime. Dropping the `Child` handle neither kills nor waits it, so the +/// grandchild stays alive holding the pipes. +// Leaving the grandchild unreaped is the point: the leader exits without waiting +// it, so a descendant outlives the leader still holding the inherited pipes. +#[allow(clippy::zombie_processes)] +#[test] +#[ignore = "self-exec fixture: only runs under GL_TEST_FIXTURE=exit_holder"] +fn fixture_exit_leaving_pipe_holder() { + if std::env::var("GL_TEST_FIXTURE").ok().as_deref() != Some("exit_holder") { + return; + } + // Stdio is inherited by default, so the grandchild holds the same pipe + // write-ends the caller handed this leader. Do not wait on it: the leader + // returns now, leaving the grandchild alive and holding the pipes. + let _detached = fixture_command("fixture_sleep", "sleep") + .spawn() + .expect("spawn grandchild fixture"); +} + +/// Leader-exits-first companion to `run_bounded_reaps_descendants_holding_the_pipe` +/// (#192 F2). The leader spawns a pipe-holding descendant and returns immediately, +/// so `try_wait` sees it gone and the loop breaks `completed` well before the +/// deadline. The descendant still holds the reader pipes, so without closing the +/// group on the clean-exit path the reader joins block on the descendant's full +/// lifetime (~10s here; the real helper's ~300s HTTP timeout). Reverting the +/// clean-path group reap in `run_bounded` turns this RED (elapsed ~10s). Unix-only: +/// the fix relies on the process group staying reserved while a member is alive, +/// and the Windows `taskkill /T` tree walk cannot reach a descendant once its +/// leader has exited. +#[cfg(unix)] +#[test] +fn run_bounded_bounds_join_when_leader_exits_leaving_a_pipe_holder() { + let cmd = fixture_command("fixture_exit_leaving_pipe_holder", "exit_holder"); + + let start = Instant::now(); + // A 30s deadline the leader never approaches: it exits almost immediately, so + // this exercises the clean-exit path, not the timeout path. + let (completed, _out) = run_bounded(cmd, Duration::from_secs(30)); + let elapsed = start.elapsed(); + + assert!( + completed, + "the leader exited well within the 30s deadline, so run_bounded must report \ + completion rather than a timeout" + ); + assert!( + elapsed < Duration::from_secs(5), + "reader joins blocked on a descendant that outlived the leader: \ + elapsed={elapsed:?} (expected <5s; the clean-exit path must close the \ + process group so the grandchild's held pipes do not stall the joins)" + ); +} From 3e33a87f5c53863e407ed4ada9b3a3b27a13aaaa Mon Sep 17 00:00:00 2001 From: t Date: Sun, 19 Jul 2026 12:19:00 -0500 Subject: [PATCH 10/15] fix(git-remote): reject malformed pkt-line length headers instead of flushing A non-UTF-8 or non-hex 4-byte length prefix was collapsed to 0000 via unwrap_or, so a garbled header was handled exactly like a flush -- masking corrupted/desynchronized protocol input and letting accumulated have lines go out as a synthesized flush round. Propagate the parse error instead; a real 0000 still parses as a valid flush. (#192) --- crates/git-remote-gitlawb/src/main.rs | 34 +++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 1901e758..57cb3898 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -447,8 +447,15 @@ fn read_upload_pack_round(stdin: &mut R) -> Result<(Vec, RoundEnd)> Err(e) => return Err(e.into()), } - let len_hex = std::str::from_utf8(&len_bytes).unwrap_or("0000"); - let pkt_len = usize::from_str_radix(len_hex, 16).unwrap_or(0); + // A malformed 4-byte length header must NOT be silently collapsed to a + // 0000 flush (#192 F5): that masks corrupted/desynchronized protocol input + // and can emit accumulated have lines as a synthesized flush round. + let Ok(len_hex) = std::str::from_utf8(&len_bytes) else { + bail!("malformed pkt-line length prefix: non-UTF-8 bytes {len_bytes:02x?}"); + }; + let Ok(pkt_len) = usize::from_str_radix(len_hex, 16) else { + bail!("malformed pkt-line length prefix: non-hex {len_hex:?}"); + }; if pkt_len == 0 { // Flush pkt "0000": this round is complete. @@ -1979,6 +1986,29 @@ mod tests { ); } + /// #192 F5: a non-hex length header (e.g. "zzzz") is a malformed frame and must + /// be rejected, not silently collapsed to a 0000 flush. + #[test] + fn read_upload_pack_round_rejects_non_hex_length() { + let mut r = io::Cursor::new(b"zzzzdone\n".to_vec()); + let err = read_upload_pack_round(&mut r).unwrap_err(); + assert!( + err.to_string().to_lowercase().contains("pkt-line"), + "unexpected error: {err}" + ); + } + + /// #192 F5: a non-UTF-8 length header must be rejected, not treated as a flush. + #[test] + fn read_upload_pack_round_rejects_non_utf8_length() { + let mut r = io::Cursor::new(vec![0xffu8, 0xff, 0xff, 0xff]); + let err = read_upload_pack_round(&mut r).unwrap_err(); + assert!( + err.to_string().to_lowercase().contains("pkt-line"), + "unexpected error: {err}" + ); + } + /// G2: an immediately-empty upload-pack request (EOF at the opening round, with /// a 200 advertisement) skips the POST entirely. #[test] From 5f4e31fe48c5b2e72b1cb28d78f717177c0df9e0 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 19 Jul 2026 12:27:17 -0500 Subject: [PATCH 11/15] fix(node): make metrics::init race-safe with Once init() did an unsynchronized REGISTRY.get() check then OnceLock::set(...). expect(...) for each metric, so two concurrent callers both passed the check and the loser panicked at 'set INFO once'. Two tests call init(), so the parallel test runner could hit this. Guard the body with std::sync::Once so it runs exactly once. Verified: a 32-thread barrier stress-test panics before the fix and passes after (run in isolation; not committed since the process-global registry makes the race non-deterministic in the shared-binary suite). (#192) --- crates/gitlawb-node/src/metrics.rs | 39 ++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/gitlawb-node/src/metrics.rs b/crates/gitlawb-node/src/metrics.rs index ebe0ab1d..6888be81 100644 --- a/crates/gitlawb-node/src/metrics.rs +++ b/crates/gitlawb-node/src/metrics.rs @@ -57,10 +57,15 @@ static PEERS_CONNECTED: OnceLock = OnceLock::new(); /// more than once is a silent no-op. MUST be called from `main()` after /// the node DID is known. pub fn init(version: &str, node_did: &str) { - if REGISTRY.get().is_some() { - return; - } + // Guard with Once so two concurrent callers cannot both pass an unsynchronized + // `REGISTRY.get()` check and then race on `OnceLock::set(...).expect(...)`, + // panicking the loser (#192 F4). Once runs the body exactly once, so the + // `.expect("set X once")` calls below can never observe an already-set slot. + static INIT: std::sync::Once = std::sync::Once::new(); + INIT.call_once(|| init_inner(version, node_did)); +} +fn init_inner(version: &str, node_did: &str) { let registry = Registry::new(); let info = IntGaugeVec::new( @@ -293,11 +298,11 @@ pub fn encode() -> Result { mod tests { use super::*; - // Note: these tests are not run in parallel by default (cargo runs - // tests in a binary in parallel via threads but the OnceLock guard - // means only the first init() call succeeds; subsequent ones are - // no-ops). The encode test below is structured so it's safe to run - // alongside other tests in the same binary. + // Note: cargo runs tests in a binary in parallel via threads, and several + // tests here call init(). init() is guarded by std::sync::Once, so concurrent + // callers run the body exactly once and never race on the OnceLock setters + // (#192 F4). The encode test below is structured so it's safe to run alongside + // other tests in the same binary regardless of ordering. #[test] fn encode_after_init_returns_prometheus_text() { @@ -328,6 +333,24 @@ mod tests { ); } + /// #192 F4: `init` is idempotent and safe to call repeatedly. The panic that + /// hit concurrent callers (both passing the old unsynchronized `REGISTRY.get()` + /// check, then racing on `OnceLock::set(...).expect(...)`) is fixed by the + /// `Once` guard. That race is inherently non-deterministic to assert in the + /// shared-binary suite — a 32-thread barrier stress-test reproduces it only in + /// isolation (verified: RED before the `Once` fix, GREEN after). This guard + /// covers the deterministic half of the contract: a repeat call is a no-op, + /// never a panic, and the registry stays usable. + #[test] + fn init_is_idempotent_no_panic_on_repeat() { + init("0.0.0-a", "did:key:a"); + init("0.0.0-b", "did:key:b"); + assert!( + encode().is_ok(), + "registry must stay usable after a repeated init" + ); + } + #[test] fn record_helpers_are_noops_before_init() { // These don't panic. They also don't show up in encode() output From 620526c1caf96a6e1c2a869f10805de5349c70e2 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 19 Jul 2026 13:14:25 -0500 Subject: [PATCH 12/15] fix(node): count fetch completion from the response outcome, not the request The gitlawb_fetches_total gate keyed on whether the request carried `done`, which both undercounts a no-done completion (a client can finish a flush- terminated have round with `ACK ready` + pack and no `done` -> counted 0) and, on the filtered path, counts a rejected response per POST (the pre-#191 two-POST negotiation the client refuses was counted twice). Derive completion from the response instead. upload_pack / upload_pack_excluding now return whether they served a pack; response_served_pack detects the PACK magic in both side-band-64k and raw framing. The plain path counts when the response delivered a pack (catches no-done, skips negotiation-only rounds so an N-round fetch counts once); the filtered path stays gated on the finalizing `done` round until #191 makes that negotiation valid. Also tighten request_finalizes_fetch to reject 0003 framing and trailing bytes after the `done` pkt-line. (#192 F1/F2 + CodeRabbit repos.rs:761) --- crates/gitlawb-node/src/api/repos.rs | 195 +++++++++++++++----- crates/gitlawb-node/src/git/smart_http.rs | 215 +++++++++++++++++++++- 2 files changed, 361 insertions(+), 49 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 8f52ba63..be9d308a 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -655,7 +655,7 @@ pub async fn git_upload_pack( // reaches it even when that POST carries no `done`. Track it so the completed- // fetch metric counts that path too (#192 F1, filtered case). let mut served_filtered_pack = false; - let resp = if !visibility_pack::has_path_scoped_rule(&rules) { + let (resp, served_pack) = if !visibility_pack::has_path_scoped_rule(&rules) { smart_http::upload_pack(&disk_path, body, git_timeout).await } else { // withheld_blob_oids walks every ref with blocking `git ls-tree`; keep @@ -700,13 +700,16 @@ pub async fn git_upload_pack( app })?; // Count a completed fetch (and observe the pack) only on the POST that actually - // completes one. On the plain path that is the finalizing `done` round; counting - // per POST would record an N-round stateless-RPC fetch as N completions (#192 - // F1). The filtered path serves its whole pack on one no-`done` POST, counted via - // served_filtered_pack. Either way a completed fetch is exactly one such POST. + // completes one, keyed off the RESPONSE outcome rather than the request (#192 + // F1/F2). On the plain path that is the POST whose response delivered a pack: + // this catches a `no-done` completion (server replies `ACK ready` + pack) + // and skips a negotiation-only round (ACK/NAK, no pack), so an N-round + // stateless-RPC fetch counts exactly once. The filtered path always builds a + // self-contained pack and can't tell an accepted fresh clone from a rejected + // mid-negotiation response, so it is gated on the finalizing `done` round. // NOTE: observe_pack_size still measures the request body, not the served pack; // that mislabel predates this change and is tracked as a follow-up. - if should_count_fetch(finalizes_fetch, served_filtered_pack) { + if should_count_fetch(finalizes_fetch, served_filtered_pack, served_pack) { crate::metrics::record_fetch(&format!("{owner}/{name}")); crate::metrics::observe_pack_size(body_len as f64); } @@ -715,13 +718,35 @@ pub async fn git_upload_pack( /// Whether an upload-pack POST completed a fetch and should be counted once. /// -/// The plain serve path streams a pack only on the finalizing `done` round -/// (`finalizes_fetch`); the filtered path (`upload_pack_excluding`) serves a -/// self-contained pack on the one POST that reaches it regardless of negotiation -/// (`served_filtered_pack`). A completed fetch is exactly one such POST, so this -/// never double-counts a multi-round negotiation. -fn should_count_fetch(finalizes_fetch: bool, served_filtered_pack: bool) -> bool { - finalizes_fetch || served_filtered_pack +/// Completion is signalled by the response outcome, split by serve path (#192 +/// F1/F2): +/// +/// - Plain path (`served_filtered_pack == false`): count exactly when the +/// response delivered a pack (`response_served_pack`). This counts a `no-done` +/// completion (server streams `ACK ready` + pack even though the request +/// carried no `done`) and does NOT count a negotiation-only round (ACK/NAK, no +/// pack), so a multi-round fetch is one completion, not N. +/// - Filtered path (`served_filtered_pack == true`): `upload_pack_excluding` +/// always builds a self-contained pack, so `response_served_pack` can't +/// distinguish an accepted fresh clone from a rejected mid-negotiation response. +/// Gate on the finalizing `done` round: a fresh filtered clone carries +/// `want`+`done`; the pre-#191 rejected two-POST scenario carries no `done`, so +/// it is not counted (and not double-counted). Interim until #191 makes filtered +/// negotiation valid. +/// +/// The rule is one isolated expression: the filtered branch is +/// `served_filtered_pack && finalizes_fetch` (reduces to `finalizes_fetch` here), +/// the plain branch is `response_served_pack`. +fn should_count_fetch( + finalizes_fetch: bool, + served_filtered_pack: bool, + response_served_pack: bool, +) -> bool { + if served_filtered_pack { + finalizes_fetch + } else { + response_served_pack + } } /// True when an upload-pack request body carries a `done` pkt-line, i.e. the @@ -744,8 +769,13 @@ fn request_finalizes_fetch(body: &[u8]) -> bool { return false; }; // 0000/0001/0002 are flush/delim/response-end markers: a 4-byte header - // with no payload. Advance past the header and keep scanning. + // with no payload. 0003 is not a valid pkt-line length (a pkt-line is + // either one of those markers or has length >= 4), so reject it as + // malformed framing rather than treating it as a marker. if len < 4 { + if len == 3 { + return false; + } i += 4; continue; } @@ -754,7 +784,10 @@ fn request_finalizes_fetch(body: &[u8]) -> bool { } let payload = &body[i + 4..i + len]; if payload.strip_suffix(b"\n").unwrap_or(payload) == b"done" { - return true; + // A `done` pkt-line finalizes the request; it must be the last thing + // in the body. Trailing bytes after it are malformed framing, so fail + // closed rather than count. + return i + len == body.len(); } i += len; } @@ -1922,50 +1955,128 @@ mod tests { assert!(!request_finalizes_fetch(b"000cwantdone")); // A malformed length prefix does not panic and does not count. assert!(!request_finalizes_fetch(b"zzzzdone\n")); + // 0003 is a len<4 value that is NOT a valid marker (only 0000/0001/0002 + // are); treating it as a marker would skip 4 bytes and read the trailing + // `0009done\n` as a finalizer. Reject the malformed framing. + assert!(!request_finalizes_fetch(b"00030009done\n")); + // A trailing byte after the `done` pkt-line is malformed; fail closed. + assert!(!request_finalizes_fetch(b"0009done\nx")); } #[test] - fn should_count_fetch_counts_done_or_filtered_but_not_bare_negotiation() { - assert!(should_count_fetch(true, false)); // plain: finalizing `done` round - assert!(should_count_fetch(false, true)); // filtered: full pack, no `done` - assert!(should_count_fetch(true, true)); // filtered fresh clone (want+done) - assert!(!should_count_fetch(false, false)); // plain negotiation-only round + fn should_count_fetch_uses_response_outcome_split_by_path() { + // Plain path (served_filtered_pack = false): count == response_served_pack. + assert!(should_count_fetch(false, false, true)); // no-done, pack in response + assert!(should_count_fetch(true, false, true)); // done, pack in response + assert!(!should_count_fetch(false, false, false)); // negotiation-only, no pack + assert!(!should_count_fetch(true, false, false)); // no pack served -> not counted + + // Filtered path (served_filtered_pack = true): count == finalizes_fetch. + assert!(should_count_fetch(true, true, true)); // fresh clone: want+done + assert!(!should_count_fetch(false, true, true)); // rejected mid-negotiation: no done + assert!(!should_count_fetch(false, true, false)); + } + + // (a) #192 F1 — a `no-done` fetch: the client finishes a flush-terminated + // round and the server answers `ACK ready` + pack, with no `done` in the + // request. It must count exactly one completion. RED under the old + // `finalizes || served_filtered_pack` rule (both false across every round -> 0); + // GREEN once the plain path counts off the response pack. + #[test] + fn no_done_plain_fetch_counts_once() { + crate::metrics::init("0.0.0-test", "did:key:test"); + let (pack_bearing, negotiation_only) = smart_http::upload_pack_result_fixtures(); + + let want = "0032want 1111111111111111111111111111111111111111\n"; + let no_done = format!("{want}0000").into_bytes(); // no `done` pkt-line + + // Two rounds, neither request carrying `done`: a negotiation round (no pack + // in the response) then the completing round (pack in the response). + let rounds: [(&[u8], &[u8]); 2] = [ + (no_done.as_slice(), &negotiation_only), + (no_done.as_slice(), &pack_bearing), + ]; + let label = "fetchgate/no-done-plain"; + let before = crate::metrics::fetch_count_for_test(label); + for (req, output) in rounds { + let finalizes = request_finalizes_fetch(req); + let served_pack = smart_http::response_served_pack(output); + if should_count_fetch(finalizes, false, served_pack) { + crate::metrics::record_fetch(label); + } + } + assert_eq!( + crate::metrics::fetch_count_for_test(label) - before, + 1, + "a no-done fetch (server sends pack, request has no `done`) must count once" + ); } + // (b) A plain negotiation-only round (server replies ACK/NAK with no pack) + // must count zero, so the intermediate rounds of a multi-round fetch never + // over-count. #[test] - fn fetch_completion_counts_once_per_fetch_plain_and_filtered() { + fn plain_negotiation_only_round_counts_zero() { crate::metrics::init("0.0.0-test", "did:key:test"); + let (_pack_bearing, negotiation_only) = smart_http::upload_pack_result_fixtures(); let want = "0032want 1111111111111111111111111111111111111111\n"; - let nego = format!("{want}0000").into_bytes(); // negotiation-only, no done - let done = format!("{want}00000009done\n").into_bytes(); // finalizing - - // Plain path: one POST per round, only the finalizing `done` round streams a - // pack. Drive the same decision the handler uses; an N-round fetch counts once. - let plain = "fetchgate/plain-counts-once"; - let before = crate::metrics::fetch_count_for_test(plain); - for body in [nego.as_slice(), nego.as_slice(), done.as_slice()] { - if should_count_fetch(request_finalizes_fetch(body), false) { - crate::metrics::record_fetch(plain); + let no_done = format!("{want}0000").into_bytes(); + let label = "fetchgate/plain-negotiation-only"; + let before = crate::metrics::fetch_count_for_test(label); + let served_pack = smart_http::response_served_pack(&negotiation_only); + if should_count_fetch(request_finalizes_fetch(&no_done), false, served_pack) { + crate::metrics::record_fetch(label); + } + assert_eq!( + crate::metrics::fetch_count_for_test(label) - before, + 0, + "a plain negotiation-only round (no pack in the response) must not count" + ); + } + + // (c) #192 F2 — the pre-#191 rejected filtered scenario: the node answers a + // mid-negotiation POST with NAK + a full pack, real git rejects it and (before + // #191) the exchange spans two filtered POSTs, neither carrying `done`. It must + // count zero, not two. RED under the old rule (each filtered POST set the flag + // -> counted twice); GREEN once the filtered path is gated on `done`. + #[test] + fn rejected_filtered_fetch_counts_zero_not_two() { + crate::metrics::init("0.0.0-test", "did:key:test"); + let want = "0032want 1111111111111111111111111111111111111111\n"; + let no_done = format!("{want}0000").into_bytes(); // no `done` + let label = "fetchgate/rejected-filtered"; + let before = crate::metrics::fetch_count_for_test(label); + // Two filtered POSTs (served_filtered_pack = true, each response carries a + // self-contained pack), neither carrying `done`. + for _ in 0..2 { + if should_count_fetch(request_finalizes_fetch(&no_done), true, true) { + crate::metrics::record_fetch(label); } } assert_eq!( - crate::metrics::fetch_count_for_test(plain) - before, - 1, - "a plain multi-round fetch must record exactly one completed fetch" + crate::metrics::fetch_count_for_test(label) - before, + 0, + "a rejected filtered fetch (no `done`) must not count, and must not double-count" ); + } - // Filtered path: upload_pack_excluding serves a self-contained pack on the one - // POST that reaches it, which carries no `done`. It must still count once. - let filtered = "fetchgate/filtered-counts-once"; - let before = crate::metrics::fetch_count_for_test(filtered); - if should_count_fetch(request_finalizes_fetch(&nego), true) { - crate::metrics::record_fetch(filtered); + // (d) A fresh filtered clone carries `want`+`done` and the node serves a full + // pack; it must count exactly one. + #[test] + fn fresh_filtered_clone_counts_once() { + crate::metrics::init("0.0.0-test", "did:key:test"); + let want = "0032want 1111111111111111111111111111111111111111\n"; + let done = format!("{want}00000009done\n").into_bytes(); // want + done + let label = "fetchgate/fresh-filtered-clone"; + let before = crate::metrics::fetch_count_for_test(label); + if should_count_fetch(request_finalizes_fetch(&done), true, true) { + crate::metrics::record_fetch(label); } assert_eq!( - crate::metrics::fetch_count_for_test(filtered) - before, + crate::metrics::fetch_count_for_test(label) - before, 1, - "a filtered fetch served without `done` must still record one completed fetch" + "a fresh filtered clone (want+done) must count exactly one" ); } diff --git a/crates/gitlawb-node/src/git/smart_http.rs b/crates/gitlawb-node/src/git/smart_http.rs index eeb35b99..ebface6c 100644 --- a/crates/gitlawb-node/src/git/smart_http.rs +++ b/crates/gitlawb-node/src/git/smart_http.rs @@ -52,19 +52,72 @@ 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. +/// +/// Returns `(response, served_pack)` where `served_pack` is whether the +/// git-upload-pack-result actually delivered a packfile (vs a negotiation-only +/// ACK/NAK round). The handler counts a completed fetch from this outcome rather +/// than from the request's `done` flag (#192 F1). pub async fn upload_pack( repo_path: &Path, request_body: Bytes, timeout: Duration, -) -> Result { +) -> Result<(Response, bool)> { let output = run_git_service("git", "git-upload-pack", repo_path, request_body, timeout).await?; - Ok(Response::builder() + let served_pack = response_served_pack(&output); + let resp = Response::builder() .status(StatusCode::OK) .header("Content-Type", "application/x-git-upload-pack-result") .header("Cache-Control", "no-cache") - .body(Body::from(output))?) + .body(Body::from(output))?; + Ok((resp, served_pack)) +} + +/// True when a `git-upload-pack-result` stream actually delivered a packfile, +/// as opposed to a negotiation-only response (ACK/NAK with no pack). +/// +/// Lets the fetch-completion metric key off the response outcome instead of the +/// request's `done` flag (#192 F1): a `no-done` fetch that the server answers +/// with `ACK ready` + pack is a completion; a flush-terminated negotiation +/// round (the server replies only ACK/NAK, or nothing) is not. +/// +/// Handles both framings the serve paths emit: +/// - side-band-64k: the pack rides in band-1 (`0x01`) pkt-lines; the first such +/// chunk begins with the `PACK` magic (band-2 progress lines can precede it). +/// - non-sideband: the raw packfile follows the ACK/NAK pkt-lines and begins with +/// the `PACK` magic (which is not valid pkt-line hex, so parsing falls through +/// to the raw-stream check). +/// +/// Fails closed (returns false) on a truncated or malformed stream. +pub fn response_served_pack(output: &[u8]) -> bool { + let mut i = 0; + while i + 4 <= output.len() { + let Ok(hdr) = std::str::from_utf8(&output[i..i + 4]) else { + // Not a pkt-line header: the raw (non-sideband) pack stream has begun. + return output[i..].starts_with(b"PACK"); + }; + let Ok(len) = usize::from_str_radix(hdr, 16) else { + // `PACK` (and any other non-hex 4 bytes) lands here: the raw pack + // follows the NAK pkt-line in the non-sideband framing. + return output[i..].starts_with(b"PACK"); + }; + // 0000/0001/0002 are flush/delim/response-end markers, no payload. + if len < 4 { + i += 4; + continue; + } + if i + len > output.len() { + return false; // truncated + } + let payload = &output[i + 4..i + len]; + // side-band-64k: band 1 carries pack data; its first chunk is the PACK magic. + if payload.first() == Some(&0x01) && payload[1..].starts_with(b"PACK") { + return true; + } + i += len; + } + false } /// Handle `POST /:owner/:repo/git-receive-pack` @@ -409,7 +462,7 @@ pub async fn upload_pack_excluding( repo_path: &Path, request_body: Bytes, withheld: &HashSet, -) -> Result { +) -> Result<(Response, bool)> { // build_filtered_pack shells out to git (rev-list, pack-objects) with // blocking std::process I/O; run it off the async worker so a large repo's // pack build does not stall the tokio runtime. @@ -443,11 +496,19 @@ pub async fn upload_pack_excluding( body.extend_from_slice(&pack); } - Ok(Response::builder() + // The filtered path always builds and serves a self-contained pack, so this + // is always true; computing it (rather than hardcoding) keeps the signal + // honest if the framing ever changes. The handler does NOT count off this + // flag alone — a filtered response can't distinguish an accepted fresh clone + // from a rejected mid-negotiation one, so the count is gated on `done` too + // (#192 F2, see `should_count_fetch`). + let served_pack = response_served_pack(&body); + let resp = Response::builder() .status(StatusCode::OK) .header("Content-Type", "application/x-git-upload-pack-result") .header("Cache-Control", "no-cache") - .body(Body::from(body))?) + .body(Body::from(body))?; + Ok((resp, served_pack)) } /// True if `needle` occurs anywhere in `haystack`. Small substring scan used to @@ -461,6 +522,87 @@ fn memmem(haystack: &[u8], needle: &[u8]) -> bool { .any(|window| window == needle) } +/// Build real `git upload-pack --stateless-rpc` result fixtures for tests: +/// `(pack_bearing, negotiation_only)`. The first is a completing clone request +/// (`want` + flush + `done`) which git answers with `NAK` + a packfile; the +/// second is a negotiation round (`want` + flush + an unknown `have` + flush, no +/// `done`) which git answers with `NAK` and no pack. Used to exercise +/// [`response_served_pack`] and the completion-count rule against real protocol +/// output rather than a hand-rolled approximation. +#[cfg(test)] +pub(crate) fn upload_pack_result_fixtures() -> (Vec, Vec) { + use std::io::Write as _; + let dir = tempfile::TempDir::new().unwrap(); + let work = dir.path().join("work"); + let bare = dir.path().join("bare.git"); + std::fs::create_dir_all(work.join("sub")).unwrap(); + std::fs::write(work.join("a.txt"), b"hello\n").unwrap(); + std::fs::write(work.join("sub/b.txt"), b"world\n").unwrap(); + let g = |args: &[&str], d: &Path| { + assert!(std::process::Command::new("git") + .args(args) + .current_dir(d) + .status() + .unwrap() + .success()); + }; + g(&["init", "-q"], &work); + g(&["config", "user.email", "t@t"], &work); + g(&["config", "user.name", "t"], &work); + g(&["add", "."], &work); + g(&["commit", "-qm", "init"], &work); + g( + &[ + "clone", + "-q", + "--bare", + work.to_str().unwrap(), + bare.to_str().unwrap(), + ], + dir.path(), + ); + let sha = { + let o = std::process::Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&work) + .output() + .unwrap(); + String::from_utf8_lossy(&o.stdout).trim().to_string() + }; + let caps = "multi_ack_detailed side-band-64k thin-pack ofs-delta agent=git/test"; + let run = |req: &[u8]| -> Vec { + let mut child = std::process::Command::new("git") + .args(["upload-pack", "--stateless-rpc"]) + .arg(&bare) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + child.stdin.take().unwrap().write_all(req).unwrap(); + let out = child.wait_with_output().unwrap(); + assert!( + out.status.success(), + "upload-pack failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out.stdout + }; + // Completing: want + flush + done -> NAK + pack. + let mut completing = pkt_line(&format!("want {sha} {caps}\n")); + completing.extend_from_slice(b"0000"); + completing.extend_from_slice(&pkt_line("done\n")); + let pack_bearing = run(&completing); + // Negotiation-only: want + flush + an unknown have + flush, no done -> NAK, + // no pack. + let mut nego = pkt_line(&format!("want {sha} {caps}\n")); + nego.extend_from_slice(b"0000"); + nego.extend_from_slice(&pkt_line("have 0000000000000000000000000000000000000000\n")); + nego.extend_from_slice(b"0000"); + let negotiation_only = run(&nego); + (pack_bearing, negotiation_only) +} + #[cfg(test)] mod tests { use super::*; @@ -600,7 +742,11 @@ mod tests { b"0098want 0000000000000000000000000000000000000000 \ side-band-64k ofs-delta agent=git/2\n00000009done\n", ); - let resp = upload_pack_excluding(&bare, req, &withheld).await.unwrap(); + let (resp, served_pack) = upload_pack_excluding(&bare, req, &withheld).await.unwrap(); + assert!( + served_pack, + "the filtered serve path always delivers a pack" + ); let body = to_bytes(resp.into_body(), usize::MAX).await.unwrap(); let ids = pack_object_ids(&extract_pack(&body)); assert!( @@ -660,6 +806,7 @@ mod tests { upload_pack_excluding(&st.repo, body, &st.withheld) .await .unwrap() + .0 } /// Spawn the server for `bare`, withholding `withheld`. Returns the clone URL @@ -1513,4 +1660,58 @@ mod tests { stdin-write EPIPE (a generic 500); got: {msg}" ); } + + // ── #192 F1: response_served_pack detector (real git output) ──────────── + + // A completing request (want+flush+done) yields NAK + a real packfile; + // response_served_pack must detect the delivered pack. A negotiation-only + // round (want+flush+have+flush, no done) yields NAK with no pack and must + // detect false. Fixtures are captured from real `git upload-pack + // --stateless-rpc`, so the detector is validated against protocol output, not + // a hand-rolled approximation. + #[test] + fn response_served_pack_detects_real_pack_vs_negotiation_only() { + let (pack_bearing, negotiation_only) = upload_pack_result_fixtures(); + + // Sanity-check the fixtures are the shapes we think (a served pack carries + // the PACK magic; a NAK-only round does not). + assert!( + pack_bearing.windows(4).any(|w| w == b"PACK"), + "the completing fixture must carry a packfile" + ); + assert!( + !negotiation_only.windows(4).any(|w| w == b"PACK"), + "the negotiation-only fixture must carry no packfile" + ); + + assert!( + response_served_pack(&pack_bearing), + "a real NAK + pack response must be detected as a served pack" + ); + assert!( + !response_served_pack(&negotiation_only), + "a real ACK/NAK-only negotiation round must be detected as no pack" + ); + // An empty response (git produced nothing for a want+flush with no done) + // is a negotiation round, not a completion. + assert!(!response_served_pack(b"")); + } + + // The non-sideband framing (raw pack after the NAK pkt-line) must also detect + // true: the raw PACK magic is not valid pkt-line hex, so parsing falls through + // to the raw-stream check. Built by hand to pin that branch independent of the + // client's advertised capabilities. + #[test] + fn response_served_pack_detects_non_sideband_raw_pack() { + let mut raw = pkt_line("NAK\n"); + raw.extend_from_slice(b"PACK\x00\x00\x00\x02\x00\x00\x00\x00"); + assert!( + response_served_pack(&raw), + "a raw (non-sideband) pack after NAK must be detected" + ); + + // NAK with no following pack is a negotiation round. + let nak_only = pkt_line("NAK\n"); + assert!(!response_served_pack(&nak_only)); + } } From 3ceba33b4c17f62fb116c93789812f6e6a0b15d5 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 19 Jul 2026 13:14:25 -0500 Subject: [PATCH 13/15] test(git-remote): bind the withheld-path test to the expected ACK/NAK rejection The test accepted any nonzero git exit (with posts>=1) as the recorded #191 withheld rejection, so a truncated response or a later HTTP failure after the first POST could pass green. Assert the specific `expected ACK/NAK` signature so only that break passes, and pin LC_ALL=C so the diagnostic stays the untranslated English string on a git build with l10n installed. (#192 F6) --- crates/git-remote-gitlawb/tests/real_git_fetch.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index 56e8edae..235c7f63 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -264,6 +264,10 @@ fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Outpu .arg(clone) .args(["fetch", "origin", "main"]) .env("PATH", path_env) + // Pin the C locale so git's diagnostics (asserted on by the withheld-path + // test, e.g. `expected ACK/NAK`) are the untranslated English strings on a + // machine/CI with git-l10n installed and LANG set to a translated locale. + .env("LC_ALL", "C") .env("GITLAWB_NODE", node_url) .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch"); run_bounded(cmd, Duration::from_secs(30)) @@ -739,6 +743,17 @@ fn real_git_withheld_shaped_first_post() { advertisement / helper lookup / connection) is NOT the withheld \ rejection this test records. git stderr:\n{stderr}" ); + // Bind to the SPECIFIC #191 break, not any nonzero exit. Real git rejects + // the NAK+pack mid-negotiation with `fatal: git fetch-pack: expected + // ACK/NAK, got '...'`. A truncated/corrupt response or a later HTTP failure + // after the first POST would also exit nonzero with posts>=1; asserting the + // signature keeps those from masquerading as the recorded rejection. + assert!( + stderr.contains("expected ACK/NAK"), + "nonzero exit with posts>=1 but not the recorded #191 rejection \ + (`expected ACK/NAK`): a different failure must not pass as the \ + withheld-path break. git stderr:\n{stderr}" + ); eprintln!( "WITHHELD-PATH NOTE (#117): real git did NOT accept a NAK+pack mid-negotiation \ (observed {posts} POST(s)). Per the Withheld-Path Decision this routes to a node-side \ From 443908d87f933401bcce64267339e811bb6df2e1 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 21 Jul 2026 10:36:27 -0500 Subject: [PATCH 14/15] test(git-remote): tear down the fetch tree with a Job Object on Windows (#192) run_bounded's clean-exit descendant reap was Unix-only: on Windows a git fetch leader that exits cleanly leaves the git-remote-gitlawb helper blocked in its ~300s HTTP request holding the inherited pipes, and taskkill /T can't reach a descendant once the leader is gone, so the reader joins blew past the promised bound. The child is now assigned to a Job Object at spawn (the analog of the Unix process_group(0)), and the job is terminated on both the timeout and clean-exit paths, which kills the whole tree regardless of leader liveness. KILL_ON_JOB_CLOSE is the safety net. The clean-exit regression is now cross-platform so it exercises this on a future Windows CI lane (#228). Unix tests unchanged and green; the Windows path is compile-verified for x86_64-pc-windows-msvc (runtime coverage lands with #228's CI lane). --- Cargo.lock | 1 + crates/git-remote-gitlawb/Cargo.toml | 13 ++ .../tests/real_git_fetch.rs | 208 +++++++++++++++--- 3 files changed, 191 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cfdda903..ea28ff7f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3310,6 +3310,7 @@ dependencies = [ "tempfile", "tracing", "tracing-subscriber", + "windows-sys 0.61.2", ] [[package]] diff --git a/crates/git-remote-gitlawb/Cargo.toml b/crates/git-remote-gitlawb/Cargo.toml index 566ac6c4..022a7bed 100644 --- a/crates/git-remote-gitlawb/Cargo.toml +++ b/crates/git-remote-gitlawb/Cargo.toml @@ -23,3 +23,16 @@ tempfile = "3" [target.'cfg(unix)'.dev-dependencies] libc = "0.2" + +[target.'cfg(windows)'.dev-dependencies] +# Job Object bindings for the real_git_fetch harness: the Windows analog of the +# unix process-group teardown, so the whole `git fetch` tree can be killed +# together on both the timeout and clean-exit paths (INV-22). Win32_Security is +# required by CreateJobObjectW, and Win32_System_Threading by +# JOBOBJECT_EXTENDED_LIMIT_INFORMATION (it embeds IO_COUNTERS). +windows-sys = { version = "0.61", features = [ + "Win32_Foundation", + "Win32_Security", + "Win32_System_JobObjects", + "Win32_System_Threading", +] } diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index 235c7f63..b11f0a12 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -287,10 +287,11 @@ fn exited_status() -> std::process::ExitStatus { #[cfg(not(unix))] fn exited_status() -> std::process::ExitStatus { // Non-unix has no ECHILD analog: a Windows `wait` is handle-based, so even - // after `reap_tree` has already reaped the leader on the timeout path, the - // later `wait` in `run_bounded` succeeds again against the still-open handle. - // This stays unreachable in practice; spawn a trivially-failing process to - // synthesize a nonzero status without an unstable constructor. + // after the job terminate (or the `reap_tree` fallback) has taken down the + // leader on the timeout path, the later `wait` in `run_bounded` succeeds again + // against the still-open handle. This stays unreachable in practice; spawn a + // trivially-failing process to synthesize a nonzero status without an unstable + // constructor. Command::new("cmd") .args(["/C", "exit 1"]) .status() @@ -332,21 +333,20 @@ fn reap_group(child: &mut std::process::Child) { let _ = child.wait(); } -/// Tear down the child's whole process tree on the timeout path (INV-22): the -/// non-unix counterpart of `reap_group`. +/// Fallback tree teardown for the non-unix timeout path (INV-22): the leader-only +/// counterpart used only when the Job Object could not be established at spawn. /// -/// `taskkill /T /F` walks the parent-pid tree from the leader and force-kills -/// every member, so ALL inherited pipe write-ends (leader AND the -/// git-remote-gitlawb descendant) are closed and the reader joins in -/// `run_bounded` return promptly instead of waiting out the helper's ~300s HTTP -/// timeout. taskkill ships in System32 on every supported Windows, which avoids -/// both a windows-only dev-dependency (a Job Object binding) and `unsafe` in -/// test code. Known caveat: /T resolves the parent-pid tree at invocation time, -/// which is sufficient for this harness because git and the helper are both -/// still alive (blocked, not exiting) at the deadline. The `child.kill()` below -/// is a best-effort leader-only fallback for the taskkill-unavailable case; -/// finally reap the leader so it does not linger, mirroring `reap_group`'s -/// contract. +/// The Job Object below is the correct primitive on Windows (it owns the tree +/// regardless of leader liveness), so this is now a best-effort fallback rather +/// than the main path. `taskkill /T /F` walks the parent-pid tree from the leader +/// and force-kills every member reachable through it, closing the inherited pipe +/// write-ends so the reader joins in `run_bounded` return instead of waiting out +/// the helper's ~300s HTTP timeout. taskkill ships in System32 on every supported +/// Windows and needs no `unsafe`. Its known limit is exactly why the job exists: +/// /T resolves the parent-pid tree at invocation time and cannot reach a +/// descendant once its leader has already exited. The `child.kill()` below is a +/// further leader-only fallback for the taskkill-unavailable case; finally reap +/// the leader so it does not linger, mirroring `reap_group`'s contract. #[cfg(not(unix))] fn reap_tree(child: &mut std::process::Child) { let _ = Command::new("taskkill") @@ -356,6 +356,113 @@ fn reap_tree(child: &mut std::process::Child) { let _ = child.wait(); } +/// Windows analog of the unix process group: a Job Object that owns the whole +/// fetch tree so `git fetch` plus the `git-remote-gitlawb` helper it spawns can be +/// torn down together on BOTH the timeout and clean-exit paths (INV-22). The unix +/// path relies on `process_group(0)` + `reap_group`; Windows has no fork-style +/// group, so the tree is bounded by assigning the leader to a job and terminating +/// the job. Unlike `taskkill /T`, a job owns its members regardless of leader +/// liveness, which is what closes the clean-exit gap: once `git fetch` exits, the +/// parent-pid tree no longer reaches the still-blocked helper, but the job still +/// does. +/// +/// The handle is held in this RAII owner so `Drop` closes it. Configured with +/// `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, closing the last handle also kills any +/// member not already terminated, a safety net for a stray the explicit +/// `TerminateJobObject` did not cover. +#[cfg(windows)] +struct JobHandle(windows_sys::Win32::Foundation::HANDLE); + +#[cfg(windows)] +impl Drop for JobHandle { + fn drop(&mut self) { + // SAFETY: CloseHandle takes a single handle value and borrows no Rust + // memory. With KILL_ON_JOB_CLOSE, closing the last handle also kills any + // still-assigned member, the safety net for a member not explicitly torn + // down. Closing an already-closed-elsewhere job is not possible here since + // this owner holds the sole handle for its whole lifetime. + unsafe { + windows_sys::Win32::Foundation::CloseHandle(self.0); + } + } +} + +/// Create a Job Object, configure `KILL_ON_JOB_CLOSE`, and assign the freshly +/// spawned `child` to it, mirroring the unix `cmd.process_group(0)` at spawn. +/// Returns `None` if any step fails, in which case the caller falls back to the +/// leader-only `reap_tree` tree walk. +/// +/// Honest caveat: this assigns the child right after `spawn` rather than via +/// `CREATE_SUSPENDED` + resume (which std cannot do without exposing the main +/// thread handle), so there is a tiny window between the process starting and the +/// assignment landing. In this harness the child is `git`, which spawns the +/// `git-remote-gitlawb` helper only after it parses config and reads the +/// advertisement, so the assignment reliably lands before the helper exists and +/// the helper is created inside the job. `KILL_ON_JOB_CLOSE` covers any stray that +/// somehow raced ahead. +#[cfg(windows)] +fn assign_job(child: &std::process::Child) -> Option { + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::Foundation::HANDLE; + use windows_sys::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JobObjectExtendedLimitInformation, + SetInformationJobObject, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }; + + // SAFETY: CreateJobObjectW(null, null) creates a new unnamed, unsecured job and + // returns its handle (or null on failure); the two null pointers are the + // documented "default attributes / no name" arguments and no Rust memory is + // borrowed. + let job = unsafe { CreateJobObjectW(std::ptr::null(), std::ptr::null()) }; + if job.is_null() { + return None; + } + // Own the handle now so an early return below still closes it via Drop. + let owner = JobHandle(job); + + // SAFETY: `zeroed()` is a valid all-zero bit pattern for this plain-old-data + // struct (only integers and nested POD, no references or non-null invariants). + let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = unsafe { std::mem::zeroed() }; + info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + // SAFETY: passes a pointer to a fully-initialized, correctly-sized + // JOBOBJECT_EXTENDED_LIMIT_INFORMATION together with its byte length; the call + // copies out of the buffer and retains no reference to it. + let ok = unsafe { + SetInformationJobObject( + job, + JobObjectExtendedLimitInformation, + &info as *const _ as *const core::ffi::c_void, + std::mem::size_of::() as u32, + ) + }; + if ok == 0 { + return None; // `owner` drops here -> CloseHandle. + } + + // SAFETY: assigns the child's OS process handle to the job. Both arguments are + // raw handle values and no Rust memory is borrowed; the `Child` owns the + // process handle and outlives this call, so `as_raw_handle` is valid here. + let ok = unsafe { AssignProcessToJobObject(job, child.as_raw_handle() as HANDLE) }; + if ok == 0 { + return None; + } + Some(owner) +} + +/// Terminate every member of the job (INV-22): the Windows analog of `reap_group` +/// signalling `-pgid`, invoked on both the timeout and clean-exit teardown paths. +#[cfg(windows)] +fn terminate_job(job: &JobHandle) { + // SAFETY: TerminateJobObject takes the job handle and an integer exit code and + // borrows no Rust memory. Terminating an already-empty or already-terminated + // job is a harmless no-op, so this is safe to call on the clean-exit path where + // the descendant may already be gone. + unsafe { + windows_sys::Win32::System::JobObjects::TerminateJobObject(job.0, 1); + } +} + /// Spawn `cmd`, draining stdout and stderr CONCURRENTLY on reader threads while /// enforcing `timeout`. Returns `(completed_before_deadline, Output)` (#192, F2). /// @@ -379,6 +486,14 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); let mut child = cmd.spawn().expect("spawn child"); + // Windows analog of the unix `process_group(0)` above: Windows has no way to + // set a group before spawn, so assign the freshly spawned leader to a Job + // Object now. The job owns the whole fetch tree and is terminated on both + // teardown paths below (INV-22). `None` if the job could not be established, in + // which case teardown falls back to the taskkill tree walk. + #[cfg(windows)] + let job = assign_job(&child); + let mut out_pipe = child.stdout.take().expect("stdout piped"); let mut err_pipe = child.stderr.take().expect("stderr piped"); let out_reader = std::thread::spawn(move || { @@ -402,12 +517,23 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp // helper is a descendant that inherited the stdout/stderr pipe write-ends, // so killing only `child` leaves those ends open and the reader joins wait // on the helper's ~300s HTTP timeout instead of returning at the deadline. - // Taking down every member (the unix group signal, the windows taskkill - // tree kill) closes every write-end so the readers finish promptly + // Taking down every member (the unix group signal, the windows job + // terminate) closes every write-end so the readers finish promptly // (INV-22, mirrors gitlawb-node/src/git/smart_http.rs). #[cfg(unix)] reap_group(&mut child); - #[cfg(not(unix))] + // Windows: terminate the whole job (every assigned member); the taskkill + // tree walk stays only as the fallback when the job was not established. + #[cfg(windows)] + match &job { + Some(job) => { + terminate_job(job); + let _ = child.wait(); + } + None => reap_tree(&mut child), + } + // Any other non-unix target keeps the leader-only tree walk. + #[cfg(all(not(unix), not(windows)))] reap_tree(&mut child); break false; // timed out: the real deadlock signature } @@ -441,6 +567,22 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp } } + // Windows clean-exit sibling of the unix reap above, and the core of this fix + // (jatmn's [P2]): a leader (`git fetch`) that exits cleanly does NOT let + // `taskkill /T` reach a still-blocked helper descendant, because the parent-pid + // tree no longer connects them once the leader is gone. So on the clean path the + // reader joins below would stall on the helper's ~300s HTTP timeout past the + // bound this function promises. Terminating the job kills every member + // regardless of leader liveness, so the joins return at the leader's exit. This + // runs whether or not the descendant is still alive; terminating an + // already-empty job is a harmless no-op (INV-22). + #[cfg(windows)] + if completed { + if let Some(job) = &job { + terminate_job(job); + } + } + let stdout = out_reader.join().expect("stdout reader"); let stderr = err_reader.join().expect("stderr reader"); ( @@ -771,8 +913,8 @@ fn real_git_withheld_shaped_first_post() { /// the surviving grandchild would keep the pipes open and the reader joins would /// block for its full ~10s lifetime. Tearing down every member closes every /// write-end, so `run_bounded` returns promptly. This runs on every target, so -/// the unix group signal and the windows taskkill tree kill are each covered on -/// the platform where they compile. Reverting either teardown to a leader-only +/// the unix group signal and the windows job terminate are each covered on the +/// platform where they compile. Reverting either teardown to a leader-only /// `child.kill()` turns this RED there (elapsed ~10s, the assert below fires). #[test] fn run_bounded_reaps_descendants_holding_the_pipe() { @@ -823,13 +965,16 @@ fn fixture_exit_leaving_pipe_holder() { /// (#192 F2). The leader spawns a pipe-holding descendant and returns immediately, /// so `try_wait` sees it gone and the loop breaks `completed` well before the /// deadline. The descendant still holds the reader pipes, so without closing the -/// group on the clean-exit path the reader joins block on the descendant's full -/// lifetime (~10s here; the real helper's ~300s HTTP timeout). Reverting the -/// clean-path group reap in `run_bounded` turns this RED (elapsed ~10s). Unix-only: -/// the fix relies on the process group staying reserved while a member is alive, -/// and the Windows `taskkill /T` tree walk cannot reach a descendant once its -/// leader has exited. -#[cfg(unix)] +/// group (unix) / terminating the job (windows) on the clean-exit path the reader +/// joins block on the descendant's full lifetime (~10s here; the real helper's +/// ~300s HTTP timeout). Reverting the clean-path teardown in `run_bounded` turns +/// this RED (elapsed ~10s). +/// +/// Runs on both platforms so a future Windows CI lane exercises the Job Object +/// clean-exit teardown, which is exactly the case `taskkill /T` cannot cover: once +/// the leader exits, the parent-pid tree no longer reaches the descendant, but the +/// job still owns it. On this Linux box it runs the unix process-group path and +/// passes; the Windows teardown here runs once issue #228's Windows CI lane exists. #[test] fn run_bounded_bounds_join_when_leader_exits_leaving_a_pipe_holder() { let cmd = fixture_command("fixture_exit_leaving_pipe_holder", "exit_holder"); @@ -849,6 +994,7 @@ fn run_bounded_bounds_join_when_leader_exits_leaving_a_pipe_holder() { elapsed < Duration::from_secs(5), "reader joins blocked on a descendant that outlived the leader: \ elapsed={elapsed:?} (expected <5s; the clean-exit path must close the \ - process group so the grandchild's held pipes do not stall the joins)" + process group (unix) / terminate the job (windows) so the grandchild's \ + held pipes do not stall the joins)" ); } From 050536a317a1147eb53ee39ba50ccc19602bb318 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 22 Jul 2026 06:14:36 -0500 Subject: [PATCH 15/15] test(git-remote): make the Windows Job Object mandatory in run_bounded Resolves jatmn's #192 P2. run_bounded's clean-exit teardown only terminated the job when one had been established; when assign_job returned None it did nothing, so a git-remote-gitlawb helper descendant that outlived the `git fetch` leader while holding the inherited stdout/stderr pipe write-ends stalled the reader joins on the helper's ~300s HTTP timeout, past the bound run_bounded promises. The timeout path's reap_tree (taskkill /T) fallback cannot rescue the clean path: once the leader has cleanly exited it no longer roots the descendant in the parent-PID tree, so taskkill /T cannot reach the orphan. The Job Object is the only reliable Windows teardown, so make it mandatory: assign_job now returns JobHandle and panics per-step if the job cannot be established, and both teardown paths unconditionally terminate it. Nested unnamed jobs with KILL_ON_JOB_CLOSE are standard on the shipped x86_64-pc-windows-msvc / CI runners, so this costs no coverage; it converts a silent ~300s hang into a loud, actionable failure and strengthens the bound (KILL_ON_JOB_CLOSE kills the descendant at latest when JobHandle drops). reap_tree, whose only Windows use was the removed None arm, is narrowed to cfg(all(not(unix), not(windows))). The change is entirely cfg(windows). Verified by cross-compiling the tests for x86_64-pc-windows-gnu (compiles clean, clippy -D warnings clean, no dead_code from the reap_tree narrowing) and by the unchanged Linux git-remote-gitlawb suite (green). The Windows runtime teardown is reasoned-not-run here; CI's Windows job is the runtime gate. --- .../tests/real_git_fetch.rs | 79 +++++++++++-------- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index b11f0a12..26d88f27 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -333,21 +333,16 @@ fn reap_group(child: &mut std::process::Child) { let _ = child.wait(); } -/// Fallback tree teardown for the non-unix timeout path (INV-22): the leader-only -/// counterpart used only when the Job Object could not be established at spawn. -/// -/// The Job Object below is the correct primitive on Windows (it owns the tree -/// regardless of leader liveness), so this is now a best-effort fallback rather -/// than the main path. `taskkill /T /F` walks the parent-pid tree from the leader -/// and force-kills every member reachable through it, closing the inherited pipe -/// write-ends so the reader joins in `run_bounded` return instead of waiting out -/// the helper's ~300s HTTP timeout. taskkill ships in System32 on every supported -/// Windows and needs no `unsafe`. Its known limit is exactly why the job exists: -/// /T resolves the parent-pid tree at invocation time and cannot reach a -/// descendant once its leader has already exited. The `child.kill()` below is a -/// further leader-only fallback for the taskkill-unavailable case; finally reap -/// the leader so it does not linger, mirroring `reap_group`'s contract. -#[cfg(not(unix))] +/// Best-effort tree teardown for the fallback non-unix, non-windows timeout path +/// (INV-22): the leader-only counterpart to the unix `reap_group`, for exotic targets +/// that are neither unix (which uses `process_group(0)` + `reap_group`) nor windows +/// (which uses the mandatory Job Object below). It attempts `taskkill /T /F` to walk a +/// parent-pid tree if that tool is present, then force-kills and reaps the leader so it +/// does not linger. With no group primitive on these targets it cannot guarantee a +/// descendant is reached; it is the honest best effort where neither real primitive +/// applies. Windows no longer routes here: the Job Object is mandatory there, so this +/// is not compiled on windows and cannot warn as dead code. +#[cfg(all(not(unix), not(windows)))] fn reap_tree(child: &mut std::process::Child) { let _ = Command::new("taskkill") .args(["/T", "/F", "/PID", &child.id().to_string()]) @@ -389,8 +384,14 @@ impl Drop for JobHandle { /// Create a Job Object, configure `KILL_ON_JOB_CLOSE`, and assign the freshly /// spawned `child` to it, mirroring the unix `cmd.process_group(0)` at spawn. -/// Returns `None` if any step fails, in which case the caller falls back to the -/// leader-only `reap_tree` tree walk. +/// +/// The job is MANDATORY: it is the only reliable Windows teardown for this harness, +/// so any failure to establish it PANICS rather than returning, which would leave +/// `run_bounded` with no way to bound its clean-exit path (taskkill `/T` cannot reach +/// a helper descendant once the `git fetch` leader has exited and no longer roots it +/// in the parent-PID tree). Nested unnamed jobs with `KILL_ON_JOB_CLOSE` are standard +/// on the shipped `x86_64-pc-windows-msvc` / CI runners, so failing loudly here beats +/// silently entering the unbounded ~300s-hang path (jatmn's #192 P2). /// /// Honest caveat: this assigns the child right after `spawn` rather than via /// `CREATE_SUSPENDED` + resume (which std cannot do without exposing the main @@ -401,7 +402,7 @@ impl Drop for JobHandle { /// the helper is created inside the job. `KILL_ON_JOB_CLOSE` covers any stray that /// somehow raced ahead. #[cfg(windows)] -fn assign_job(child: &std::process::Child) -> Option { +fn assign_job(child: &std::process::Child) -> JobHandle { use std::os::windows::io::AsRawHandle; use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::System::JobObjects::{ @@ -416,7 +417,10 @@ fn assign_job(child: &std::process::Child) -> Option { // borrowed. let job = unsafe { CreateJobObjectW(std::ptr::null(), std::ptr::null()) }; if job.is_null() { - return None; + panic!( + "real_git_fetch harness: CreateJobObjectW failed; the deadlock-bounding \ + harness requires a Windows Job Object to tear down the fetch tree" + ); } // Own the handle now so an early return below still closes it via Drop. let owner = JobHandle(job); @@ -437,7 +441,11 @@ fn assign_job(child: &std::process::Child) -> Option { ) }; if ok == 0 { - return None; // `owner` drops here -> CloseHandle. + // `owner` drops on unwind -> CloseHandle. + panic!( + "real_git_fetch harness: SetInformationJobObject(KILL_ON_JOB_CLOSE) failed; \ + the deadlock-bounding harness requires a Windows Job Object" + ); } // SAFETY: assigns the child's OS process handle to the job. Both arguments are @@ -445,9 +453,13 @@ fn assign_job(child: &std::process::Child) -> Option { // process handle and outlives this call, so `as_raw_handle` is valid here. let ok = unsafe { AssignProcessToJobObject(job, child.as_raw_handle() as HANDLE) }; if ok == 0 { - return None; + // `owner` drops on unwind -> CloseHandle. + panic!( + "real_git_fetch harness: AssignProcessToJobObject failed; the deadlock-bounding \ + harness requires the git fetch leader inside a Windows Job Object" + ); } - Some(owner) + owner } /// Terminate every member of the job (INV-22): the Windows analog of `reap_group` @@ -489,8 +501,10 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp // Windows analog of the unix `process_group(0)` above: Windows has no way to // set a group before spawn, so assign the freshly spawned leader to a Job // Object now. The job owns the whole fetch tree and is terminated on both - // teardown paths below (INV-22). `None` if the job could not be established, in - // which case teardown falls back to the taskkill tree walk. + // teardown paths below (INV-22). It is mandatory: `assign_job` panics rather + // than returning if the job cannot be established, because without it the + // clean-exit path below cannot be bounded (taskkill `/T` cannot reach an + // orphaned helper once the leader has exited). #[cfg(windows)] let job = assign_job(&child); @@ -522,15 +536,12 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp // (INV-22, mirrors gitlawb-node/src/git/smart_http.rs). #[cfg(unix)] reap_group(&mut child); - // Windows: terminate the whole job (every assigned member); the taskkill - // tree walk stays only as the fallback when the job was not established. + // Windows: terminate the whole job (every assigned member). The job is + // mandatory (`assign_job` panics otherwise), so there is no None fallback. #[cfg(windows)] - match &job { - Some(job) => { - terminate_job(job); - let _ = child.wait(); - } - None => reap_tree(&mut child), + { + terminate_job(&job); + let _ = child.wait(); } // Any other non-unix target keeps the leader-only tree walk. #[cfg(all(not(unix), not(windows)))] @@ -578,9 +589,7 @@ fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Outp // already-empty job is a harmless no-op (INV-22). #[cfg(windows)] if completed { - if let Some(job) = &job { - terminate_job(job); - } + terminate_job(&job); } let stdout = out_reader.join().expect("stdout reader");