fix(git-remote): drive multi-round fetch as a v0 stateless-RPC client loop (#117)#192
fix(git-remote): drive multi-round fetch as a v0 stateless-RPC client loop (#117)#192beardthelion wants to merge 4 commits into
Conversation
… loop (#117) git-remote-gitlawb advertised `connect`, so git spoke the stateful native protocol, but handle_connect collapsed the exchange into one GET plus one POST. A multi-round fetch (more than ~32 overlapping commits) deadlocked: git sent a flush-terminated have-batch and blocked for an ACK/NAK the helper never sent, while the helper blocked reading for a `done` git never sent. Turn Phase 2 into a per-round stateless-RPC client loop. read_upload_pack_round returns one round at a time (at a flush, done, or EOF) instead of buffering past flushes waiting for `done`; negotiate_upload_pack captures the wants once and POSTs a self-contained request per flush-terminated batch: wants, one flush, every have accumulated so far, and exactly one terminator. Each ACK/NAK is streamed back to git so it advances, until the done round returns the pack. The node's `git upload-pack --stateless-rpc` keeps no state between POSTs, so wants and all prior haves are re-sent every round and no intermediate flush survives into a body (which would truncate the negotiation server-side). Signing is preserved per round: every POST carries the Phase-1 decision (signed after the 404 escalation for a private repo, anonymous for a public one), and a mid-negotiation denial surfaces through the sanitized error path rather than reading as an empty or successful fetch. The receive-pack path is unchanged. Covers the deadlock repro, multi-round accumulation on the wire, per-round signing for private and public fetches, mid-negotiation denial surfacing, the withheld-shaped forwarding path, and single-round non-regression.
… loop (#117) A committed integration test drives a real `git fetch` through the built helper against a real `git upload-pack --stateless-rpc`, so the stateful-to-stateless ACK bridging is proven by execution rather than reasoned. The main.rs mock tests cannot falsify it: a pre-scripted Cursor never reacts to the server's ACKs. An in-test shim replicates the node's v0 serving; the fetch is forced to at least two negotiation rounds (a fixture that resolved in one round fails the test) and the resulting object graph is verified. The second scenario drives the withheld-blob shape (a full pack on the first POST, as upload_pack_excluding does) through real git and records the outcome: real git rejects a pack where it expected an ACK continuation ("expected ACK/NAK, got ..."), so a multi-round fetch of a withheld repo does not complete. The helper forwards and terminates cleanly rather than hanging, so this is a node-side concern, not a helper defect, left as a follow-up per the plan's Withheld-Path Decision. The test guards the helper's non-hang behavior and surfaces the break if the assumption ever changes.
A code-review pass found the negotiation loop would POST once per bare `0000` flush that carried no haves, so a malformed `wants + N*0000 + EOF` stream amplified into N signed POSTs. Real git never sends a content-free mid-negotiation flush and the peer is upstream of the local git process, so this was not reachable in practice, but it left the loop unbounded per input. Skip a round that carries no new haves (an empty flush) and only POST rounds with have content or a terminator, which also makes the loop provably bounded by git's finite have set. The fresh-clone done-with-no-haves round still POSTs.
Adds executed coverage for the negotiation branches the first pass left unrun: an invalid pkt-line length (rejected, not underflowed), a non-EOF read error (propagated, not swallowed as end-of-stream), an immediately empty upload-pack request (skips the POST), a nonempty have-batch terminated by EOF (POSTed as a final done round), and an empty receive-pack body (skips the POST). Each was mutation-checked to fail without its guard, so the coverage is load-bearing rather than vacuous.
📝 WalkthroughWalkthroughThe helper now supports multi-round ChangesUpload-pack negotiation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:git-remote
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/git-remote-gitlawb/src/main.rs (1)
301-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates
post_pack_round's POST/status-check/forward logic.Lines 313-334 re-implement almost exactly what
post_pack_round(lines 502-533) does: build the signed POST, send, check status viahttp_error_message, read the response body, and forward it to stdout. Sincepost_pack_roundalready takes(client, post_url, service, signing_key, body, stdout)matching this call site's variables, the receive-pack path can just call it directly instead of duplicating the block.♻️ Suggested refactor
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() { // e.g., already up-to-date — nothing to send tracing::debug!("empty request body — skipping POST"); return Ok(()); } - tracing::debug!("POST {post_url} ({} bytes)", request_body.len()); - let req = build_pack_post_request(&client, &post_url, service, &request_body, signing_key); - - // Attach the body after signing so the pack bytes are moved, not cloned — - // packs can be large and the clone doubled peak memory on push. - let pack_resp = req - .body(request_body) - .send() - .with_context(|| format!("POST {post_url}"))?; - - if !pack_resp.status().is_success() { - let status = pack_resp.status(); - let body = read_error_body(pack_resp); - let path = format!("/{service}"); - bail!("{}", http_error_message("POST", &path, status, &body, None)); - } - - let pack_bytes = pack_resp.bytes().context("reading pack response")?; - tracing::debug!("pack response: {} bytes from node", pack_bytes.len()); - - stdout.write_all(&pack_bytes)?; - stdout.flush()?; - - Ok(()) + post_pack_round(&client, &post_url, service, signing_key, request_body, &mut stdout) }This keeps behavior identical (same log lines, same error path) while removing a divergence risk between the two copies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-remote-gitlawb/src/main.rs` around lines 301 - 336, Replace the duplicated POST, status-check, response-reading, and stdout-forwarding block after the empty-body guard with a direct call to post_pack_round, passing client, post_url, service, signing_key, request_body, and stdout. Preserve the existing request-body logging and empty-body early return while reusing post_pack_round for the shared behavior.crates/git-remote-gitlawb/tests/real_git_fetch.rs (1)
354-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp repos leak on assertion failure.
cleanup(&[server, clone])(lines 387, 429) only runs if every priorassert!/assert_eq!in the test succeeds. A failed assertion (e.g.completed,fsck, or theFETCH_HEADequality checks) panics before cleanup, leaking theunique_dir()-created server/clone repos (each with a real object DB) under the OS temp dir.Shimalready avoids this class of leak for its own thread/listener viaDrop(lines 111-126); the repo directories don't get the same RAII treatment.♻️ Wrap temp dirs in an RAII guard so they're removed even on panic
struct TempDir(PathBuf); impl std::ops::Deref for TempDir { type Target = Path; fn deref(&self) -> &Path { &self.0 } } impl Drop for TempDir { fn drop(&mut self) { let _ = std::fs::remove_dir_all(&self.0); } }Then have
unique_dir/build_divergent_reposreturnTempDirand drop the explicitcleanup(...)calls at the end of each test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-remote-gitlawb/tests/real_git_fetch.rs` around lines 354 - 436, Introduce an RAII temporary-directory guard for paths created by unique_dir and returned through build_divergent_repos, implementing path access and Drop to remove each directory. Update both real_git_multi_round_fetch_completes and real_git_withheld_shaped_first_post to use the guarded values and remove the explicit cleanup calls, ensuring directories are deleted during assertion panics as well as normal completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/git-remote-gitlawb/tests/real_git_fetch.rs`:
- Around line 244-284: Update fetch_with_helper so the child’s stdout and stderr
are drained concurrently while the parent polls for completion, preventing
pipe-buffer backpressure from blocking git fetch. Preserve the existing
30-second timeout, kill-and-collect behavior, and boolean result semantics.
---
Nitpick comments:
In `@crates/git-remote-gitlawb/src/main.rs`:
- Around line 301-336: Replace the duplicated POST, status-check,
response-reading, and stdout-forwarding block after the empty-body guard with a
direct call to post_pack_round, passing client, post_url, service, signing_key,
request_body, and stdout. Preserve the existing request-body logging and
empty-body early return while reusing post_pack_round for the shared behavior.
In `@crates/git-remote-gitlawb/tests/real_git_fetch.rs`:
- Around line 354-436: Introduce an RAII temporary-directory guard for paths
created by unique_dir and returned through build_divergent_repos, implementing
path access and Drop to remove each directory. Update both
real_git_multi_round_fetch_completes and real_git_withheld_shaped_first_post to
use the guarded values and remove the explicit cleanup calls, ensuring
directories are deleted during assertion panics as well as normal completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38d3b6d3-d10b-460c-b569-9017a993168a
📒 Files selected for processing (2)
crates/git-remote-gitlawb/src/main.rscrates/git-remote-gitlawb/tests/real_git_fetch.rs
| /// Run `git fetch` in `clone` through the helper, with a hard timeout so a | ||
| /// regression to the deadlock fails fast instead of hanging the suite. | ||
| fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Output) { | ||
| let helper_bin = PathBuf::from(env!("CARGO_BIN_EXE_git-remote-gitlawb")); | ||
| let helper_dir = helper_bin.parent().unwrap().to_path_buf(); | ||
| let path_env = match std::env::var_os("PATH") { | ||
| Some(p) => { | ||
| let mut dirs = vec![helper_dir.clone()]; | ||
| dirs.extend(std::env::split_paths(&p)); | ||
| std::env::join_paths(dirs).unwrap() | ||
| } | ||
| None => helper_dir.clone().into_os_string(), | ||
| }; | ||
|
|
||
| let mut child = Command::new("git") | ||
| .args(["-c", "protocol.version=2"]) | ||
| .arg("-C") | ||
| .arg(clone) | ||
| .args(["fetch", "origin", "main"]) | ||
| .env("PATH", path_env) | ||
| .env("GITLAWB_NODE", node_url) | ||
| .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch") | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| .expect("spawn git fetch"); | ||
|
|
||
| let deadline = Instant::now() + Duration::from_secs(30); | ||
| loop { | ||
| if let Some(_status) = child.try_wait().unwrap() { | ||
| let out = child.wait_with_output().unwrap(); | ||
| return (true, out); | ||
| } | ||
| if Instant::now() >= deadline { | ||
| let _ = child.kill(); | ||
| let out = child.wait_with_output().unwrap(); | ||
| return (false, out); // timed out: the deadlock signature | ||
| } | ||
| std::thread::sleep(Duration::from_millis(50)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Drain git fetch output while polling.
fetch_with_helper only polls try_wait() and never reads stdout/stderr until the child exits. If the helper writes enough to fill the pipe buffers, the child can block on I/O and the test will only hit the timeout path, which looks like the deadlock this test is meant to catch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/git-remote-gitlawb/tests/real_git_fetch.rs` around lines 244 - 284,
Update fetch_with_helper so the child’s stdout and stderr are drained
concurrently while the parent polls for completion, preventing pipe-buffer
backpressure from blocking git fetch. Preserve the existing 30-second timeout,
kill-and-collect behavior, and boolean result semantics.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Do not convert an aborted negotiation into a
donerequest
crates/git-remote-gitlawb/src/main.rs:610
A nonempty EOF is explicitly documented here as the case where Git aborted mid-negotiation, but it is combined withRoundEnd::Done, receives a synthetic0009done\n, and is sent to the node. Cancelling a fetch after writing a partial have batch therefore still issues an authorized (and, for a private fetch, signed) upload-pack request, making the node generate and stream a pack to a client that has gone away. EOF should terminate the helper without a POST; only the actualdonepkt should finalize the stateless request. The newnonempty_eof_batch_posts_as_final_roundtest currently locks in the incorrect behavior. -
[P2] Drain the real Git child output while waiting for it
crates/git-remote-gitlawb/tests/real_git_fetch.rs:266
fetch_with_helperpipes both stdout and stderr, then only pollstry_waituntil the process exits. A fetch that emits more progress or error output than an OS pipe can hold blocks in Git before it exits, so this harness reaches its 30-second timeout and reports the protocol deadlock even when the helper is making progress. Drain both streams concurrently while enforcing the deadline (or do not pipe them) so the new load-bearing integration test can distinguish a real negotiation hang from pipe backpressure. -
[P3] Make the integration-test repositories cleanup-safe
crates/git-remote-gitlawb/tests/real_git_fetch.rs:356
The server and clone directories are deleted only after every assertion succeeds. Any timeout, failed fetch,fsckfailure, or panic exits beforecleanup, leaving real Git repositories in the shared temporary directory; the predictable pid/counter names can also poison a later run if a pid is reused. Return RAII-owned temporary directories (or usetempfile) so cleanup runs while unwinding as well.
2 participants
Fixes #117.
git-remote-gitlawbadvertised theconnectcapability, so git spoke the stateful native protocol, buthandle_connectcollapsed the exchange into a single GET + single POST. A multi-round fetch (more than ~32 commits of overlapping history, the common "pull into an existing clone") deadlocked: git sent a flush-terminatedhavebatch and blocked for an ACK/NAK the helper never sent, while the helper blocked reading for adonegit never sent.Phase 2 for
git-upload-packis now a per-round v0 smart-HTTP stateless-RPC client loop, the role git's ownremote-curlplays.read_upload_pack_roundreturns one negotiation round at a time (at a flush,done, or EOF) instead of buffering past flushes;negotiate_upload_packcaptures the wants once and POSTs a self-contained request per flush-terminated batch (wants + one flush + every have accumulated so far + exactly one terminator), streams each ACK/NAK back to git, and stops on thedoneround. The node'sgit upload-pack --stateless-rpckeeps no state between POSTs, so wants and all prior haves are re-sent every round and no intermediate flush survives into a body.Signing is preserved per round: every POST carries the Phase-1 decision (signed after the 404 escalation for a private repo, anonymous for a public one), and a mid-negotiation denial surfaces through the sanitized error path rather than reading as an empty or successful fetch. The
git-receive-pack(push) path is unchanged, and no node code is touched.Verification
tests/real_git_fetch.rs) drives a realgit fetch(forced to >=2 rounds) through the built helper against a realgit upload-pack --stateless-rpc, so the stateful-to-stateless bridging is executed, not reasoned.Follow-up
The one case this does not make work is an incremental fetch of a repo with a path-scoped withheld subtree: the node's
upload_pack_excludingsends its pack on the first POST, which real git rejects mid-negotiation. The helper forwards and terminates cleanly, so it is a node-side issue, filed as #191. A committed test here (real_git_withheld_shaped_first_post) records that rejection so a node-side fix can verify against it.STRATEGY track: stabilize.
Summary by CodeRabbit
Bug Fixes
Tests