diff --git a/Cargo.lock b/Cargo.lock index d12daa43..ea28ff7f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3300,19 +3300,22 @@ dependencies = [ [[package]] name = "git-remote-gitlawb" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "gitlawb-core", + "libc", "mockito", "reqwest", + "tempfile", "tracing", "tracing-subscriber", + "windows-sys 0.61.2", ] [[package]] name = "gitlawb-attest" -version = "0.5.0" +version = "0.5.1" dependencies = [ "base64", "ed25519-dalek", @@ -3329,7 +3332,7 @@ dependencies = [ [[package]] name = "gitlawb-core" -version = "0.5.0" +version = "0.5.1" dependencies = [ "anyhow", "base64", @@ -3356,7 +3359,7 @@ dependencies = [ [[package]] name = "gitlawb-node" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", @@ -3412,7 +3415,7 @@ dependencies = [ [[package]] name = "gl" -version = "0.5.0" +version = "0.5.1" dependencies = [ "alloy", "anyhow", @@ -3824,6 +3827,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..022a7bed 100644 --- a/crates/git-remote-gitlawb/Cargo.toml +++ b/crates/git-remote-gitlawb/Cargo.toml @@ -19,3 +19,20 @@ tracing-subscriber = { workspace = true } [dev-dependencies] mockito = "1" +tempfile = "3" + +[target.'cfg(unix)'.dev-dependencies] +libc = "0.2" + +[target.'cfg(windows)'.dev-dependencies] +# Job Object bindings for the real_git_fetch harness: the Windows analog of the +# unix process-group teardown, so the whole `git fetch` tree can be killed +# together on both the timeout and clean-exit paths (INV-22). Win32_Security is +# required by CreateJobObjectW, and Win32_System_Threading by +# JOBOBJECT_EXTENDED_LIMIT_INFORMATION (it embeds IO_COUNTERS). +windows-sys = { version = "0.61", features = [ + "Win32_Foundation", + "Win32_Security", + "Win32_System_JobObjects", + "Win32_System_Threading", +] } diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index ca46c9ce..57cb3898 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,32 +310,14 @@ 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 — - // 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 ─────────────────────────────────────────── @@ -424,33 +411,55 @@ 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); + // A malformed 4-byte length header must NOT be silently collapsed to a + // 0000 flush (#192 F5): that masks corrupted/desynchronized protocol input + // and can emit accumulated have lines as a synthesized flush round. + let Ok(len_hex) = std::str::from_utf8(&len_bytes) else { + bail!("malformed pkt-line length prefix: non-UTF-8 bytes {len_bytes:02x?}"); + }; + let Ok(pkt_len) = usize::from_str_radix(len_hex, 16) else { + bail!("malformed pkt-line length prefix: non-hex {len_hex:?}"); + }; if pkt_len == 0 { - // Flush pkt "0000" — 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 +471,149 @@ 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)?; + + // 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); + + // 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 + } + // 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)?; + + 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,603 @@ 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" + ); + } + + /// 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" + ); + } + + // ── 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}" + ); + } + + /// #192 F5: a non-hex length header (e.g. "zzzz") is a malformed frame and must + /// be rejected, not silently collapsed to a 0000 flush. + #[test] + fn read_upload_pack_round_rejects_non_hex_length() { + let mut r = io::Cursor::new(b"zzzzdone\n".to_vec()); + let err = read_upload_pack_round(&mut r).unwrap_err(); + assert!( + err.to_string().to_lowercase().contains("pkt-line"), + "unexpected error: {err}" + ); + } + + /// #192 F5: a non-UTF-8 length header must be rejected, not treated as a flush. + #[test] + fn read_upload_pack_round_rejects_non_utf8_length() { + let mut r = io::Cursor::new(vec![0xffu8, 0xff, 0xff, 0xff]); + let err = read_upload_pack_round(&mut r).unwrap_err(); + assert!( + err.to_string().to_lowercase().contains("pkt-line"), + "unexpected error: {err}" + ); + } + + /// G2: an immediately-empty upload-pack request (EOF at the opening round, with + /// a 200 advertisement) skips the POST entirely. + #[test] + 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 (#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_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 — + // git aborted the negotiation. + let mut stdin = io::Cursor::new(stdin_bytes); + handle_connect(&repo_base, "git-upload-pack", None, &mut stdin) + .expect("an aborted (EOF) negotiation is a clean no-op, not an error"); + let requests = server.join().unwrap(); + assert_eq!( + requests.len(), + 1, + "an aborted EOF must NOT trigger an upload-pack POST (only the info/refs GET)" + ); + } + + /// 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" + ); + } } 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..26d88f27 --- /dev/null +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -0,0 +1,1009 @@ +//! #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 +} + +/// 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. +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 cmd = Command::new("git"); + cmd.args(["-c", "protocol.version=2"]) + .arg("-C") + .arg(clone) + .args(["fetch", "origin", "main"]) + .env("PATH", path_env) + // Pin the C locale so git's diagnostics (asserted on by the withheld-path + // test, e.g. `expected ACK/NAK`) are the untranslated English strings on a + // machine/CI with git-l10n installed and LANG set to a translated locale. + .env("LC_ALL", "C") + .env("GITLAWB_NODE", node_url) + .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch"); + run_bounded(cmd, Duration::from_secs(30)) +} + +/// 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 has no ECHILD analog: a Windows `wait` is handle-based, so even + // after the job terminate (or the `reap_tree` fallback) has taken down the + // leader on the timeout path, the later `wait` in `run_bounded` succeeds again + // against the still-open handle. This stays unreachable in practice; spawn a + // trivially-failing process to synthesize a nonzero status without an unstable + // constructor. + Command::new("cmd") + .args(["/C", "exit 1"]) + .status() + .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(); +} + +/// Best-effort tree teardown for the fallback non-unix, non-windows timeout path +/// (INV-22): the leader-only counterpart to the unix `reap_group`, for exotic targets +/// that are neither unix (which uses `process_group(0)` + `reap_group`) nor windows +/// (which uses the mandatory Job Object below). It attempts `taskkill /T /F` to walk a +/// parent-pid tree if that tool is present, then force-kills and reaps the leader so it +/// does not linger. With no group primitive on these targets it cannot guarantee a +/// descendant is reached; it is the honest best effort where neither real primitive +/// applies. Windows no longer routes here: the Job Object is mandatory there, so this +/// is not compiled on windows and cannot warn as dead code. +#[cfg(all(not(unix), not(windows)))] +fn reap_tree(child: &mut std::process::Child) { + let _ = Command::new("taskkill") + .args(["/T", "/F", "/PID", &child.id().to_string()]) + .output(); + let _ = child.kill(); + let _ = child.wait(); +} + +/// Windows analog of the unix process group: a Job Object that owns the whole +/// fetch tree so `git fetch` plus the `git-remote-gitlawb` helper it spawns can be +/// torn down together on BOTH the timeout and clean-exit paths (INV-22). The unix +/// path relies on `process_group(0)` + `reap_group`; Windows has no fork-style +/// group, so the tree is bounded by assigning the leader to a job and terminating +/// the job. Unlike `taskkill /T`, a job owns its members regardless of leader +/// liveness, which is what closes the clean-exit gap: once `git fetch` exits, the +/// parent-pid tree no longer reaches the still-blocked helper, but the job still +/// does. +/// +/// The handle is held in this RAII owner so `Drop` closes it. Configured with +/// `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, closing the last handle also kills any +/// member not already terminated, a safety net for a stray the explicit +/// `TerminateJobObject` did not cover. +#[cfg(windows)] +struct JobHandle(windows_sys::Win32::Foundation::HANDLE); + +#[cfg(windows)] +impl Drop for JobHandle { + fn drop(&mut self) { + // SAFETY: CloseHandle takes a single handle value and borrows no Rust + // memory. With KILL_ON_JOB_CLOSE, closing the last handle also kills any + // still-assigned member, the safety net for a member not explicitly torn + // down. Closing an already-closed-elsewhere job is not possible here since + // this owner holds the sole handle for its whole lifetime. + unsafe { + windows_sys::Win32::Foundation::CloseHandle(self.0); + } + } +} + +/// Create a Job Object, configure `KILL_ON_JOB_CLOSE`, and assign the freshly +/// spawned `child` to it, mirroring the unix `cmd.process_group(0)` at spawn. +/// +/// The job is MANDATORY: it is the only reliable Windows teardown for this harness, +/// so any failure to establish it PANICS rather than returning, which would leave +/// `run_bounded` with no way to bound its clean-exit path (taskkill `/T` cannot reach +/// a helper descendant once the `git fetch` leader has exited and no longer roots it +/// in the parent-PID tree). Nested unnamed jobs with `KILL_ON_JOB_CLOSE` are standard +/// on the shipped `x86_64-pc-windows-msvc` / CI runners, so failing loudly here beats +/// silently entering the unbounded ~300s-hang path (jatmn's #192 P2). +/// +/// Honest caveat: this assigns the child right after `spawn` rather than via +/// `CREATE_SUSPENDED` + resume (which std cannot do without exposing the main +/// thread handle), so there is a tiny window between the process starting and the +/// assignment landing. In this harness the child is `git`, which spawns the +/// `git-remote-gitlawb` helper only after it parses config and reads the +/// advertisement, so the assignment reliably lands before the helper exists and +/// the helper is created inside the job. `KILL_ON_JOB_CLOSE` covers any stray that +/// somehow raced ahead. +#[cfg(windows)] +fn assign_job(child: &std::process::Child) -> JobHandle { + use std::os::windows::io::AsRawHandle; + use windows_sys::Win32::Foundation::HANDLE; + use windows_sys::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JobObjectExtendedLimitInformation, + SetInformationJobObject, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }; + + // SAFETY: CreateJobObjectW(null, null) creates a new unnamed, unsecured job and + // returns its handle (or null on failure); the two null pointers are the + // documented "default attributes / no name" arguments and no Rust memory is + // borrowed. + let job = unsafe { CreateJobObjectW(std::ptr::null(), std::ptr::null()) }; + if job.is_null() { + panic!( + "real_git_fetch harness: CreateJobObjectW failed; the deadlock-bounding \ + harness requires a Windows Job Object to tear down the fetch tree" + ); + } + // Own the handle now so an early return below still closes it via Drop. + let owner = JobHandle(job); + + // SAFETY: `zeroed()` is a valid all-zero bit pattern for this plain-old-data + // struct (only integers and nested POD, no references or non-null invariants). + let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = unsafe { std::mem::zeroed() }; + info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + // SAFETY: passes a pointer to a fully-initialized, correctly-sized + // JOBOBJECT_EXTENDED_LIMIT_INFORMATION together with its byte length; the call + // copies out of the buffer and retains no reference to it. + let ok = unsafe { + SetInformationJobObject( + job, + JobObjectExtendedLimitInformation, + &info as *const _ as *const core::ffi::c_void, + std::mem::size_of::() as u32, + ) + }; + if ok == 0 { + // `owner` drops on unwind -> CloseHandle. + panic!( + "real_git_fetch harness: SetInformationJobObject(KILL_ON_JOB_CLOSE) failed; \ + the deadlock-bounding harness requires a Windows Job Object" + ); + } + + // SAFETY: assigns the child's OS process handle to the job. Both arguments are + // raw handle values and no Rust memory is borrowed; the `Child` owns the + // process handle and outlives this call, so `as_raw_handle` is valid here. + let ok = unsafe { AssignProcessToJobObject(job, child.as_raw_handle() as HANDLE) }; + if ok == 0 { + // `owner` drops on unwind -> CloseHandle. + panic!( + "real_git_fetch harness: AssignProcessToJobObject failed; the deadlock-bounding \ + harness requires the git fetch leader inside a Windows Job Object" + ); + } + owner +} + +/// Terminate every member of the job (INV-22): the Windows analog of `reap_group` +/// signalling `-pgid`, invoked on both the timeout and clean-exit teardown paths. +#[cfg(windows)] +fn terminate_job(job: &JobHandle) { + // SAFETY: TerminateJobObject takes the job handle and an integer exit code and + // borrows no Rust memory. Terminating an already-empty or already-terminated + // job is a harmless no-op, so this is safe to call on the clean-exit path where + // the descendant may already be gone. + unsafe { + windows_sys::Win32::System::JobObjects::TerminateJobObject(job.0, 1); + } +} + +/// Spawn `cmd`, draining stdout and stderr CONCURRENTLY on reader threads while +/// enforcing `timeout`. Returns `(completed_before_deadline, Output)` (#192, F2). +/// +/// 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) { + // 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"); + + // Windows analog of the unix `process_group(0)` above: Windows has no way to + // set a group before spawn, so assign the freshly spawned leader to a Job + // Object now. The job owns the whole fetch tree and is terminated on both + // teardown paths below (INV-22). It is mandatory: `assign_job` panics rather + // than returning if the job cannot be established, because without it the + // clean-exit path below cannot be bounded (taskkill `/T` cannot reach an + // orphaned helper once the leader has exited). + #[cfg(windows)] + let job = assign_job(&child); + + 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() + timeout; + let completed = loop { + if child.try_wait().unwrap().is_some() { + break true; + } + if Instant::now() >= deadline { + // 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. + // Taking down every member (the unix group signal, the windows job + // terminate) closes every write-end so the readers finish promptly + // (INV-22, mirrors gitlawb-node/src/git/smart_http.rs). + #[cfg(unix)] + reap_group(&mut child); + // Windows: terminate the whole job (every assigned member). The job is + // mandatory (`assign_job` panics otherwise), so there is no None fallback. + #[cfg(windows)] + { + terminate_job(&job); + let _ = child.wait(); + } + // Any other non-unix target keeps the leader-only tree walk. + #[cfg(all(not(unix), not(windows)))] + reap_tree(&mut child); + break false; // timed out: the real deadlock signature + } + std::thread::sleep(Duration::from_millis(50)); + }; + + // Collect the leader's status. On the timeout path `reap_group` already reaped + // it, so `wait` returns ECHILD; tolerate that and report the killed status the + // loop observed. On the clean path `try_wait` above already reaped it, so this + // returns the cached real status. + let status = child.wait().unwrap_or_else(|_| { + debug_assert!(!completed, "clean-exit path must still be reapable here"); + exited_status() + }); + + // A leader that exits on its own does NOT guarantee the pipes are closed: a + // descendant (the git-remote-gitlawb helper mid-HTTP request) can outlive the + // leader while still holding the inherited stdout/stderr write-ends, so the + // reader joins below would block on its ~300s HTTP timeout, past the deadline + // this function promises (#192 F2, INV-22). The timeout path already tore the + // group down; on the clean path, close it here too. A process-group ID stays + // reserved while any member is alive, so signalling the group is safe as long as + // `kill(-pgid, 0)` still reports a live member; once the group is empty the pipes + // are closed anyway and the leader's reaped pid may have been recycled, so skip. + #[cfg(unix)] + if completed { + let pgid = child.id() as i32; + // SAFETY: kill(-pgid, 0) only probes group liveness; it borrows no memory. + if unsafe { libc::kill(-pgid, 0) } == 0 { + reap_group(&mut child); + } + } + + // Windows clean-exit sibling of the unix reap above, and the core of this fix + // (jatmn's [P2]): a leader (`git fetch`) that exits cleanly does NOT let + // `taskkill /T` reach a still-blocked helper descendant, because the parent-pid + // tree no longer connects them once the leader is gone. So on the clean path the + // reader joins below would stall on the helper's ~300s HTTP timeout past the + // bound this function promises. Terminating the job kills every member + // regardless of leader liveness, so the joins return at the leader's exit. This + // runs whether or not the descendant is still alive; terminating an + // already-empty job is a harmless no-op (INV-22). + #[cfg(windows)] + if completed { + terminate_job(&job); + } + + let stdout = out_reader.join().expect("stdout reader"); + let stderr = err_reader.join().expect("stderr reader"); + ( + completed, + std::process::Output { + status, + stdout, + stderr, + }, + ) +} + +// --------------------------------------------------------------------------- +// 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). +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", "."]); + 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 = tmp.path().join("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", + ], + ); + Repos { + server, + clone, + _tmp: tmp, + } +} + +/// 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() { + // `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); + 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"]); + // No manual cleanup: `repos` drops here (or on unwind) and removes the tree. +} + +/// #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 ~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() { + // 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, + "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 +/// `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() { + // `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); + 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. + // + // 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}" + ); + // Bind to the SPECIFIC #191 break, not any nonzero exit. Real git rejects + // the NAK+pack mid-negotiation with `fatal: git fetch-pack: expected + // ACK/NAK, got '...'`. A truncated/corrupt response or a later HTTP failure + // after the first POST would also exit nonzero with posts>=1; asserting the + // signature keeps those from masquerading as the recorded rejection. + assert!( + stderr.contains("expected ACK/NAK"), + "nonzero exit with posts>=1 but not the recorded #191 rejection \ + (`expected ACK/NAK`): a different failure must not pass as the \ + withheld-path break. git stderr:\n{stderr}" + ); + eprintln!( + "WITHHELD-PATH NOTE (#117): real git did NOT accept a NAK+pack mid-negotiation \ + (observed {posts} POST(s)). Per the Withheld-Path Decision this routes to a node-side \ + follow-up or an accepted withheld=full-clone limitation, not helper code. git stderr:\n{stderr}" + ); + } + + // 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 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 job terminate are each covered on the +/// platform where they compile. Reverting either teardown to a leader-only +/// `child.kill()` turns this RED there (elapsed ~10s, the assert below fires). +#[test] +fn run_bounded_reaps_descendants_holding_the_pipe() { + // 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)); + 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 \ + grandchild fixture holding the pipes for its full lifetime)" + ); +} + +/// Fixture: spawn a detached grandchild (`fixture_sleep`) that inherits the stdio +/// pipes, then the LEADER returns immediately. This is the leader-exits-first +/// shape (#192 F2): the leader is gone almost at once, well before any deadline, +/// but the descendant keeps the caller's pipe write-ends open for its full +/// lifetime. Dropping the `Child` handle neither kills nor waits it, so the +/// grandchild stays alive holding the pipes. +// Leaving the grandchild unreaped is the point: the leader exits without waiting +// it, so a descendant outlives the leader still holding the inherited pipes. +#[allow(clippy::zombie_processes)] +#[test] +#[ignore = "self-exec fixture: only runs under GL_TEST_FIXTURE=exit_holder"] +fn fixture_exit_leaving_pipe_holder() { + if std::env::var("GL_TEST_FIXTURE").ok().as_deref() != Some("exit_holder") { + return; + } + // Stdio is inherited by default, so the grandchild holds the same pipe + // write-ends the caller handed this leader. Do not wait on it: the leader + // returns now, leaving the grandchild alive and holding the pipes. + let _detached = fixture_command("fixture_sleep", "sleep") + .spawn() + .expect("spawn grandchild fixture"); +} + +/// Leader-exits-first companion to `run_bounded_reaps_descendants_holding_the_pipe` +/// (#192 F2). The leader spawns a pipe-holding descendant and returns immediately, +/// so `try_wait` sees it gone and the loop breaks `completed` well before the +/// deadline. The descendant still holds the reader pipes, so without closing the +/// group (unix) / terminating the job (windows) on the clean-exit path the reader +/// joins block on the descendant's full lifetime (~10s here; the real helper's +/// ~300s HTTP timeout). Reverting the clean-path teardown in `run_bounded` turns +/// this RED (elapsed ~10s). +/// +/// Runs on both platforms so a future Windows CI lane exercises the Job Object +/// clean-exit teardown, which is exactly the case `taskkill /T` cannot cover: once +/// the leader exits, the parent-pid tree no longer reaches the descendant, but the +/// job still owns it. On this Linux box it runs the unix process-group path and +/// passes; the Windows teardown here runs once issue #228's Windows CI lane exists. +#[test] +fn run_bounded_bounds_join_when_leader_exits_leaving_a_pipe_holder() { + let cmd = fixture_command("fixture_exit_leaving_pipe_holder", "exit_holder"); + + let start = Instant::now(); + // A 30s deadline the leader never approaches: it exits almost immediately, so + // this exercises the clean-exit path, not the timeout path. + let (completed, _out) = run_bounded(cmd, Duration::from_secs(30)); + let elapsed = start.elapsed(); + + assert!( + completed, + "the leader exited well within the 30s deadline, so run_bounded must report \ + completion rather than a timeout" + ); + assert!( + elapsed < Duration::from_secs(5), + "reader joins blocked on a descendant that outlived the leader: \ + elapsed={elapsed:?} (expected <5s; the clean-exit path must close the \ + process group (unix) / terminate the job (windows) so the grandchild's \ + held pipes do not stall the joins)" + ); +} diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index b9cbc352..be9d308a 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -642,12 +642,20 @@ pub async fn git_upload_pack( .await .map_err(|e| AppError::Git(e.to_string()))?; let body_len = body.len(); + // Whether this POST finalized negotiation (carries `done`), computed before + // `body` is moved into upload_pack. Gates the completed-fetch metric below. + let finalizes_fetch = request_finalizes_fetch(&body); // No path-scoped rule can withhold an individual blob, and the whole-repo // "/" gate above already enforced repo-level access. Skip the per-blob // withheld walk and serve the pack directly. let git_timeout = std::time::Duration::from_secs(state.config.git_service_timeout_secs); - let resp = if !visibility_pack::has_path_scoped_rule(&rules) { + // The filtered serve path (upload_pack_excluding) replies NAK + a self-contained + // full pack regardless of negotiation, completing a fetch on the single POST that + // reaches it even when that POST carries no `done`. Track it so the completed- + // fetch metric counts that path too (#192 F1, filtered case). + let mut served_filtered_pack = false; + let (resp, served_pack) = if !visibility_pack::has_path_scoped_rule(&rules) { smart_http::upload_pack(&disk_path, body, git_timeout).await } else { // withheld_blob_oids walks every ref with blocking `git ls-tree`; keep @@ -675,6 +683,7 @@ pub async fn git_upload_pack( if withheld.is_empty() { smart_http::upload_pack(&disk_path, body, git_timeout).await } else { + served_filtered_pack = true; tracing::info!(repo = %name, caller = ?caller, withheld = withheld.len(), "serving filtered pack"); smart_http::upload_pack_excluding(&disk_path, body, &withheld).await } @@ -690,11 +699,101 @@ pub async fn git_upload_pack( } app })?; - crate::metrics::record_fetch(&format!("{owner}/{name}")); - crate::metrics::observe_pack_size(body_len as f64); + // Count a completed fetch (and observe the pack) only on the POST that actually + // completes one, keyed off the RESPONSE outcome rather than the request (#192 + // F1/F2). On the plain path that is the POST whose response delivered a pack: + // this catches a `no-done` completion (server replies `ACK ready` + pack) + // and skips a negotiation-only round (ACK/NAK, no pack), so an N-round + // stateless-RPC fetch counts exactly once. The filtered path always builds a + // self-contained pack and can't tell an accepted fresh clone from a rejected + // mid-negotiation response, so it is gated on the finalizing `done` round. + // NOTE: observe_pack_size still measures the request body, not the served pack; + // that mislabel predates this change and is tracked as a follow-up. + if should_count_fetch(finalizes_fetch, served_filtered_pack, served_pack) { + crate::metrics::record_fetch(&format!("{owner}/{name}")); + crate::metrics::observe_pack_size(body_len as f64); + } Ok(resp) } +/// Whether an upload-pack POST completed a fetch and should be counted once. +/// +/// Completion is signalled by the response outcome, split by serve path (#192 +/// F1/F2): +/// +/// - Plain path (`served_filtered_pack == false`): count exactly when the +/// response delivered a pack (`response_served_pack`). This counts a `no-done` +/// completion (server streams `ACK ready` + pack even though the request +/// carried no `done`) and does NOT count a negotiation-only round (ACK/NAK, no +/// pack), so a multi-round fetch is one completion, not N. +/// - Filtered path (`served_filtered_pack == true`): `upload_pack_excluding` +/// always builds a self-contained pack, so `response_served_pack` can't +/// distinguish an accepted fresh clone from a rejected mid-negotiation response. +/// Gate on the finalizing `done` round: a fresh filtered clone carries +/// `want`+`done`; the pre-#191 rejected two-POST scenario carries no `done`, so +/// it is not counted (and not double-counted). Interim until #191 makes filtered +/// negotiation valid. +/// +/// The rule is one isolated expression: the filtered branch is +/// `served_filtered_pack && finalizes_fetch` (reduces to `finalizes_fetch` here), +/// the plain branch is `response_served_pack`. +fn should_count_fetch( + finalizes_fetch: bool, + served_filtered_pack: bool, + response_served_pack: bool, +) -> bool { + if served_filtered_pack { + finalizes_fetch + } else { + response_served_pack + } +} + +/// True when an upload-pack request body carries a `done` pkt-line, i.e. the +/// client finished negotiation and is asking the server to stream the pack. +/// +/// The HTTP smart protocol runs upload-pack as stateless RPC: the client sends one +/// `git-upload-pack` POST per negotiation round, but only the finalizing round +/// sends `done`; the earlier flush-terminated rounds negotiate common history and +/// produce no pack. Counting a fetch only when this returns true keeps an N-round +/// incremental fetch from being recorded as N completed fetches (#192 F1). Parses +/// pkt-lines and fails closed (returns false) on a malformed body, so a garbled +/// request is never counted. +fn request_finalizes_fetch(body: &[u8]) -> bool { + let mut i = 0; + while i + 4 <= body.len() { + let Ok(hdr) = std::str::from_utf8(&body[i..i + 4]) else { + return false; + }; + let Ok(len) = usize::from_str_radix(hdr, 16) else { + return false; + }; + // 0000/0001/0002 are flush/delim/response-end markers: a 4-byte header + // with no payload. 0003 is not a valid pkt-line length (a pkt-line is + // either one of those markers or has length >= 4), so reject it as + // malformed framing rather than treating it as a marker. + if len < 4 { + if len == 3 { + return false; + } + i += 4; + continue; + } + if i + len > body.len() { + return false; // truncated/malformed: do not over-count + } + let payload = &body[i + 4..i + len]; + if payload.strip_suffix(b"\n").unwrap_or(payload) == b"done" { + // A `done` pkt-line finalizes the request; it must be the last thing + // in the body. Trailing bytes after it are malformed framing, so fail + // closed rather than count. + return i + len == body.len(); + } + i += len; + } + false +} + /// Decide whether the owner-push gate rejects a `git-receive-pack` request. /// /// Returns `Some(error)` when the push must be rejected, `None` when it may @@ -1836,6 +1935,151 @@ mod tests { const OWNER_SHORT: &str = "z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"; const STRANGER_DID: &str = "did:key:z6Mkffonly5tranger0000000000000000000000000000000"; + #[test] + fn upload_pack_request_finalizes_only_with_done_pktline() { + let want = "0032want 1111111111111111111111111111111111111111\n"; + let have = "0032have 2222222222222222222222222222222222222222\n"; + // Finalizing round: wants + flush + done. + let done_round = format!("{want}00000009done\n").into_bytes(); + assert!(request_finalizes_fetch(&done_round)); + // Negotiation-only round: wants + flush + haves + flush, no done. + let nego_round = format!("{want}0000{have}0000").into_bytes(); + assert!(!request_finalizes_fetch(&nego_round)); + // Degenerate: empty and a bare flush never finalize. + assert!(!request_finalizes_fetch(b"")); + assert!(!request_finalizes_fetch(b"0000")); + // `done` with no trailing newline (0008done) still finalizes. + assert!(request_finalizes_fetch(b"00000008done")); + // A payload that merely contains the substring "done" is not a done pkt + // (000c -> len 12 -> payload "wantdone"). + assert!(!request_finalizes_fetch(b"000cwantdone")); + // A malformed length prefix does not panic and does not count. + assert!(!request_finalizes_fetch(b"zzzzdone\n")); + // 0003 is a len<4 value that is NOT a valid marker (only 0000/0001/0002 + // are); treating it as a marker would skip 4 bytes and read the trailing + // `0009done\n` as a finalizer. Reject the malformed framing. + assert!(!request_finalizes_fetch(b"00030009done\n")); + // A trailing byte after the `done` pkt-line is malformed; fail closed. + assert!(!request_finalizes_fetch(b"0009done\nx")); + } + + #[test] + fn should_count_fetch_uses_response_outcome_split_by_path() { + // Plain path (served_filtered_pack = false): count == response_served_pack. + assert!(should_count_fetch(false, false, true)); // no-done, pack in response + assert!(should_count_fetch(true, false, true)); // done, pack in response + assert!(!should_count_fetch(false, false, false)); // negotiation-only, no pack + assert!(!should_count_fetch(true, false, false)); // no pack served -> not counted + + // Filtered path (served_filtered_pack = true): count == finalizes_fetch. + assert!(should_count_fetch(true, true, true)); // fresh clone: want+done + assert!(!should_count_fetch(false, true, true)); // rejected mid-negotiation: no done + assert!(!should_count_fetch(false, true, false)); + } + + // (a) #192 F1 — a `no-done` fetch: the client finishes a flush-terminated + // round and the server answers `ACK ready` + pack, with no `done` in the + // request. It must count exactly one completion. RED under the old + // `finalizes || served_filtered_pack` rule (both false across every round -> 0); + // GREEN once the plain path counts off the response pack. + #[test] + fn no_done_plain_fetch_counts_once() { + crate::metrics::init("0.0.0-test", "did:key:test"); + let (pack_bearing, negotiation_only) = smart_http::upload_pack_result_fixtures(); + + let want = "0032want 1111111111111111111111111111111111111111\n"; + let no_done = format!("{want}0000").into_bytes(); // no `done` pkt-line + + // Two rounds, neither request carrying `done`: a negotiation round (no pack + // in the response) then the completing round (pack in the response). + let rounds: [(&[u8], &[u8]); 2] = [ + (no_done.as_slice(), &negotiation_only), + (no_done.as_slice(), &pack_bearing), + ]; + let label = "fetchgate/no-done-plain"; + let before = crate::metrics::fetch_count_for_test(label); + for (req, output) in rounds { + let finalizes = request_finalizes_fetch(req); + let served_pack = smart_http::response_served_pack(output); + if should_count_fetch(finalizes, false, served_pack) { + crate::metrics::record_fetch(label); + } + } + assert_eq!( + crate::metrics::fetch_count_for_test(label) - before, + 1, + "a no-done fetch (server sends pack, request has no `done`) must count once" + ); + } + + // (b) A plain negotiation-only round (server replies ACK/NAK with no pack) + // must count zero, so the intermediate rounds of a multi-round fetch never + // over-count. + #[test] + fn plain_negotiation_only_round_counts_zero() { + crate::metrics::init("0.0.0-test", "did:key:test"); + let (_pack_bearing, negotiation_only) = smart_http::upload_pack_result_fixtures(); + + let want = "0032want 1111111111111111111111111111111111111111\n"; + let no_done = format!("{want}0000").into_bytes(); + let label = "fetchgate/plain-negotiation-only"; + let before = crate::metrics::fetch_count_for_test(label); + let served_pack = smart_http::response_served_pack(&negotiation_only); + if should_count_fetch(request_finalizes_fetch(&no_done), false, served_pack) { + crate::metrics::record_fetch(label); + } + assert_eq!( + crate::metrics::fetch_count_for_test(label) - before, + 0, + "a plain negotiation-only round (no pack in the response) must not count" + ); + } + + // (c) #192 F2 — the pre-#191 rejected filtered scenario: the node answers a + // mid-negotiation POST with NAK + a full pack, real git rejects it and (before + // #191) the exchange spans two filtered POSTs, neither carrying `done`. It must + // count zero, not two. RED under the old rule (each filtered POST set the flag + // -> counted twice); GREEN once the filtered path is gated on `done`. + #[test] + fn rejected_filtered_fetch_counts_zero_not_two() { + crate::metrics::init("0.0.0-test", "did:key:test"); + let want = "0032want 1111111111111111111111111111111111111111\n"; + let no_done = format!("{want}0000").into_bytes(); // no `done` + let label = "fetchgate/rejected-filtered"; + let before = crate::metrics::fetch_count_for_test(label); + // Two filtered POSTs (served_filtered_pack = true, each response carries a + // self-contained pack), neither carrying `done`. + for _ in 0..2 { + if should_count_fetch(request_finalizes_fetch(&no_done), true, true) { + crate::metrics::record_fetch(label); + } + } + assert_eq!( + crate::metrics::fetch_count_for_test(label) - before, + 0, + "a rejected filtered fetch (no `done`) must not count, and must not double-count" + ); + } + + // (d) A fresh filtered clone carries `want`+`done` and the node serves a full + // pack; it must count exactly one. + #[test] + fn fresh_filtered_clone_counts_once() { + crate::metrics::init("0.0.0-test", "did:key:test"); + let want = "0032want 1111111111111111111111111111111111111111\n"; + let done = format!("{want}00000009done\n").into_bytes(); // want + done + let label = "fetchgate/fresh-filtered-clone"; + let before = crate::metrics::fetch_count_for_test(label); + if should_count_fetch(request_finalizes_fetch(&done), true, true) { + crate::metrics::record_fetch(label); + } + assert_eq!( + crate::metrics::fetch_count_for_test(label) - before, + 1, + "a fresh filtered clone (want+done) must count exactly one" + ); + } + #[test] fn git_service_app_error_classifies_timeout_bad_request_and_git() { // GitServiceTimeout carried through anyhow -> 504 Timeout. diff --git a/crates/gitlawb-node/src/git/smart_http.rs b/crates/gitlawb-node/src/git/smart_http.rs index eeb35b99..ebface6c 100644 --- a/crates/gitlawb-node/src/git/smart_http.rs +++ b/crates/gitlawb-node/src/git/smart_http.rs @@ -52,19 +52,72 @@ pub async fn info_refs(repo_path: &Path, service: &str) -> Result { /// /// Serves pack data for a clone or fetch. This is stateless — the entire /// negotiation happens in a single request/response. +/// +/// Returns `(response, served_pack)` where `served_pack` is whether the +/// git-upload-pack-result actually delivered a packfile (vs a negotiation-only +/// ACK/NAK round). The handler counts a completed fetch from this outcome rather +/// than from the request's `done` flag (#192 F1). pub async fn upload_pack( repo_path: &Path, request_body: Bytes, timeout: Duration, -) -> Result { +) -> Result<(Response, bool)> { let output = run_git_service("git", "git-upload-pack", repo_path, request_body, timeout).await?; - Ok(Response::builder() + let served_pack = response_served_pack(&output); + let resp = Response::builder() .status(StatusCode::OK) .header("Content-Type", "application/x-git-upload-pack-result") .header("Cache-Control", "no-cache") - .body(Body::from(output))?) + .body(Body::from(output))?; + Ok((resp, served_pack)) +} + +/// True when a `git-upload-pack-result` stream actually delivered a packfile, +/// as opposed to a negotiation-only response (ACK/NAK with no pack). +/// +/// Lets the fetch-completion metric key off the response outcome instead of the +/// request's `done` flag (#192 F1): a `no-done` fetch that the server answers +/// with `ACK ready` + pack is a completion; a flush-terminated negotiation +/// round (the server replies only ACK/NAK, or nothing) is not. +/// +/// Handles both framings the serve paths emit: +/// - side-band-64k: the pack rides in band-1 (`0x01`) pkt-lines; the first such +/// chunk begins with the `PACK` magic (band-2 progress lines can precede it). +/// - non-sideband: the raw packfile follows the ACK/NAK pkt-lines and begins with +/// the `PACK` magic (which is not valid pkt-line hex, so parsing falls through +/// to the raw-stream check). +/// +/// Fails closed (returns false) on a truncated or malformed stream. +pub fn response_served_pack(output: &[u8]) -> bool { + let mut i = 0; + while i + 4 <= output.len() { + let Ok(hdr) = std::str::from_utf8(&output[i..i + 4]) else { + // Not a pkt-line header: the raw (non-sideband) pack stream has begun. + return output[i..].starts_with(b"PACK"); + }; + let Ok(len) = usize::from_str_radix(hdr, 16) else { + // `PACK` (and any other non-hex 4 bytes) lands here: the raw pack + // follows the NAK pkt-line in the non-sideband framing. + return output[i..].starts_with(b"PACK"); + }; + // 0000/0001/0002 are flush/delim/response-end markers, no payload. + if len < 4 { + i += 4; + continue; + } + if i + len > output.len() { + return false; // truncated + } + let payload = &output[i + 4..i + len]; + // side-band-64k: band 1 carries pack data; its first chunk is the PACK magic. + if payload.first() == Some(&0x01) && payload[1..].starts_with(b"PACK") { + return true; + } + i += len; + } + false } /// Handle `POST /:owner/:repo/git-receive-pack` @@ -409,7 +462,7 @@ pub async fn upload_pack_excluding( repo_path: &Path, request_body: Bytes, withheld: &HashSet, -) -> Result { +) -> Result<(Response, bool)> { // build_filtered_pack shells out to git (rev-list, pack-objects) with // blocking std::process I/O; run it off the async worker so a large repo's // pack build does not stall the tokio runtime. @@ -443,11 +496,19 @@ pub async fn upload_pack_excluding( body.extend_from_slice(&pack); } - Ok(Response::builder() + // The filtered path always builds and serves a self-contained pack, so this + // is always true; computing it (rather than hardcoding) keeps the signal + // honest if the framing ever changes. The handler does NOT count off this + // flag alone — a filtered response can't distinguish an accepted fresh clone + // from a rejected mid-negotiation one, so the count is gated on `done` too + // (#192 F2, see `should_count_fetch`). + let served_pack = response_served_pack(&body); + let resp = Response::builder() .status(StatusCode::OK) .header("Content-Type", "application/x-git-upload-pack-result") .header("Cache-Control", "no-cache") - .body(Body::from(body))?) + .body(Body::from(body))?; + Ok((resp, served_pack)) } /// True if `needle` occurs anywhere in `haystack`. Small substring scan used to @@ -461,6 +522,87 @@ fn memmem(haystack: &[u8], needle: &[u8]) -> bool { .any(|window| window == needle) } +/// Build real `git upload-pack --stateless-rpc` result fixtures for tests: +/// `(pack_bearing, negotiation_only)`. The first is a completing clone request +/// (`want` + flush + `done`) which git answers with `NAK` + a packfile; the +/// second is a negotiation round (`want` + flush + an unknown `have` + flush, no +/// `done`) which git answers with `NAK` and no pack. Used to exercise +/// [`response_served_pack`] and the completion-count rule against real protocol +/// output rather than a hand-rolled approximation. +#[cfg(test)] +pub(crate) fn upload_pack_result_fixtures() -> (Vec, Vec) { + use std::io::Write as _; + let dir = tempfile::TempDir::new().unwrap(); + let work = dir.path().join("work"); + let bare = dir.path().join("bare.git"); + std::fs::create_dir_all(work.join("sub")).unwrap(); + std::fs::write(work.join("a.txt"), b"hello\n").unwrap(); + std::fs::write(work.join("sub/b.txt"), b"world\n").unwrap(); + let g = |args: &[&str], d: &Path| { + assert!(std::process::Command::new("git") + .args(args) + .current_dir(d) + .status() + .unwrap() + .success()); + }; + g(&["init", "-q"], &work); + g(&["config", "user.email", "t@t"], &work); + g(&["config", "user.name", "t"], &work); + g(&["add", "."], &work); + g(&["commit", "-qm", "init"], &work); + g( + &[ + "clone", + "-q", + "--bare", + work.to_str().unwrap(), + bare.to_str().unwrap(), + ], + dir.path(), + ); + let sha = { + let o = std::process::Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&work) + .output() + .unwrap(); + String::from_utf8_lossy(&o.stdout).trim().to_string() + }; + let caps = "multi_ack_detailed side-band-64k thin-pack ofs-delta agent=git/test"; + let run = |req: &[u8]| -> Vec { + let mut child = std::process::Command::new("git") + .args(["upload-pack", "--stateless-rpc"]) + .arg(&bare) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + child.stdin.take().unwrap().write_all(req).unwrap(); + let out = child.wait_with_output().unwrap(); + assert!( + out.status.success(), + "upload-pack failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out.stdout + }; + // Completing: want + flush + done -> NAK + pack. + let mut completing = pkt_line(&format!("want {sha} {caps}\n")); + completing.extend_from_slice(b"0000"); + completing.extend_from_slice(&pkt_line("done\n")); + let pack_bearing = run(&completing); + // Negotiation-only: want + flush + an unknown have + flush, no done -> NAK, + // no pack. + let mut nego = pkt_line(&format!("want {sha} {caps}\n")); + nego.extend_from_slice(b"0000"); + nego.extend_from_slice(&pkt_line("have 0000000000000000000000000000000000000000\n")); + nego.extend_from_slice(b"0000"); + let negotiation_only = run(&nego); + (pack_bearing, negotiation_only) +} + #[cfg(test)] mod tests { use super::*; @@ -600,7 +742,11 @@ mod tests { b"0098want 0000000000000000000000000000000000000000 \ side-band-64k ofs-delta agent=git/2\n00000009done\n", ); - let resp = upload_pack_excluding(&bare, req, &withheld).await.unwrap(); + let (resp, served_pack) = upload_pack_excluding(&bare, req, &withheld).await.unwrap(); + assert!( + served_pack, + "the filtered serve path always delivers a pack" + ); let body = to_bytes(resp.into_body(), usize::MAX).await.unwrap(); let ids = pack_object_ids(&extract_pack(&body)); assert!( @@ -660,6 +806,7 @@ mod tests { upload_pack_excluding(&st.repo, body, &st.withheld) .await .unwrap() + .0 } /// Spawn the server for `bare`, withholding `withheld`. Returns the clone URL @@ -1513,4 +1660,58 @@ mod tests { stdin-write EPIPE (a generic 500); got: {msg}" ); } + + // ── #192 F1: response_served_pack detector (real git output) ──────────── + + // A completing request (want+flush+done) yields NAK + a real packfile; + // response_served_pack must detect the delivered pack. A negotiation-only + // round (want+flush+have+flush, no done) yields NAK with no pack and must + // detect false. Fixtures are captured from real `git upload-pack + // --stateless-rpc`, so the detector is validated against protocol output, not + // a hand-rolled approximation. + #[test] + fn response_served_pack_detects_real_pack_vs_negotiation_only() { + let (pack_bearing, negotiation_only) = upload_pack_result_fixtures(); + + // Sanity-check the fixtures are the shapes we think (a served pack carries + // the PACK magic; a NAK-only round does not). + assert!( + pack_bearing.windows(4).any(|w| w == b"PACK"), + "the completing fixture must carry a packfile" + ); + assert!( + !negotiation_only.windows(4).any(|w| w == b"PACK"), + "the negotiation-only fixture must carry no packfile" + ); + + assert!( + response_served_pack(&pack_bearing), + "a real NAK + pack response must be detected as a served pack" + ); + assert!( + !response_served_pack(&negotiation_only), + "a real ACK/NAK-only negotiation round must be detected as no pack" + ); + // An empty response (git produced nothing for a want+flush with no done) + // is a negotiation round, not a completion. + assert!(!response_served_pack(b"")); + } + + // The non-sideband framing (raw pack after the NAK pkt-line) must also detect + // true: the raw PACK magic is not valid pkt-line hex, so parsing falls through + // to the raw-stream check. Built by hand to pin that branch independent of the + // client's advertised capabilities. + #[test] + fn response_served_pack_detects_non_sideband_raw_pack() { + let mut raw = pkt_line("NAK\n"); + raw.extend_from_slice(b"PACK\x00\x00\x00\x02\x00\x00\x00\x00"); + assert!( + response_served_pack(&raw), + "a raw (non-sideband) pack after NAK must be detected" + ); + + // NAK with no following pack is a negotiation round. + let nak_only = pkt_line("NAK\n"); + assert!(!response_served_pack(&nak_only)); + } } diff --git a/crates/gitlawb-node/src/metrics.rs b/crates/gitlawb-node/src/metrics.rs index a21c6d8b..6888be81 100644 --- a/crates/gitlawb-node/src/metrics.rs +++ b/crates/gitlawb-node/src/metrics.rs @@ -57,10 +57,15 @@ static PEERS_CONNECTED: OnceLock = OnceLock::new(); /// more than once is a silent no-op. MUST be called from `main()` after /// the node DID is known. pub fn init(version: &str, node_did: &str) { - if REGISTRY.get().is_some() { - return; - } + // Guard with Once so two concurrent callers cannot both pass an unsynchronized + // `REGISTRY.get()` check and then race on `OnceLock::set(...).expect(...)`, + // panicking the loser (#192 F4). Once runs the body exactly once, so the + // `.expect("set X once")` calls below can never observe an already-set slot. + static INIT: std::sync::Once = std::sync::Once::new(); + INIT.call_once(|| init_inner(version, node_did)); +} +fn init_inner(version: &str, node_did: &str) { let registry = Registry::new(); let info = IntGaugeVec::new( @@ -218,6 +223,17 @@ pub fn record_fetch(repo: &str) { } } +/// Test-only: current `gitlawb_fetches_total` value for `repo` (0 if the registry +/// is not initialized). Lets api-layer tests assert the completed-fetch count with +/// a unique label instead of scraping the encoded text. +#[cfg(test)] +pub fn fetch_count_for_test(repo: &str) -> u64 { + FETCHES + .get() + .map(|c| c.with_label_values(&[repo]).get()) + .unwrap_or(0) +} + /// Record one HTTP signature check that passed. #[allow(dead_code)] // wired in a follow-up; helpers are part of the public metrics surface pub fn record_auth_success(route: &str) { @@ -282,11 +298,11 @@ pub fn encode() -> Result { mod tests { use super::*; - // Note: these tests are not run in parallel by default (cargo runs - // tests in a binary in parallel via threads but the OnceLock guard - // means only the first init() call succeeds; subsequent ones are - // no-ops). The encode test below is structured so it's safe to run - // alongside other tests in the same binary. + // Note: cargo runs tests in a binary in parallel via threads, and several + // tests here call init(). init() is guarded by std::sync::Once, so concurrent + // callers run the body exactly once and never race on the OnceLock setters + // (#192 F4). The encode test below is structured so it's safe to run alongside + // other tests in the same binary regardless of ordering. #[test] fn encode_after_init_returns_prometheus_text() { @@ -317,6 +333,24 @@ mod tests { ); } + /// #192 F4: `init` is idempotent and safe to call repeatedly. The panic that + /// hit concurrent callers (both passing the old unsynchronized `REGISTRY.get()` + /// check, then racing on `OnceLock::set(...).expect(...)`) is fixed by the + /// `Once` guard. That race is inherently non-deterministic to assert in the + /// shared-binary suite — a 32-thread barrier stress-test reproduces it only in + /// isolation (verified: RED before the `Once` fix, GREEN after). This guard + /// covers the deterministic half of the contract: a repeat call is a no-op, + /// never a panic, and the registry stays usable. + #[test] + fn init_is_idempotent_no_panic_on_repeat() { + init("0.0.0-a", "did:key:a"); + init("0.0.0-b", "did:key:b"); + assert!( + encode().is_ok(), + "registry must stay usable after a repeated init" + ); + } + #[test] fn record_helpers_are_noops_before_init() { // These don't panic. They also don't show up in encode() output