From 358479f075a5d0099fa4fcc72945bfff8f26139b Mon Sep 17 00:00:00 2001 From: t Date: Sat, 11 Jul 2026 19:40:10 -0500 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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)" ); }