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 5 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
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.
…git test harness (#192) F1: a nonempty have-batch terminated by EOF means git aborted mid-negotiation, but the fetch loop folded RoundEnd::Eof into the Done arm, appended a synthetic 0009done, and POSTed it — so a cancelled private fetch still issued an authorized, signed upload-pack request and made the node generate and stream a full pack to a client that had gone away. Split the arm: only a real done pkt finalizes; a nonempty EOF terminates without a POST. The nonempty_eof_batch_posts_as_final_round test that locked in the wrong behavior is flipped to assert no POST. F2: fetch_with_helper piped stdout+stderr but only polled try_wait, so a fetch emitting more than an OS pipe buffer (~64 KiB) blocked git before it exited and the harness reported a false 30s deadlock. Drain both streams on reader threads while enforcing the deadline, so the deadline measures a real protocol hang only. F3: the integration repos were removed only on the success path (predictable pid/counter names), so a timeout/panic leaked real git repos and could poison a later run. Root them in a RAII tempfile::TempDir that drops on unwind. RED->GREEN: nonempty_eof_batch_aborts_without_posting (pre-fix POSTs a synthetic done → the aborted-negotiation POST fails against the single-response server; fixed makes one request, no POST). repos_are_cleaned_up_on_unwind (temp dir gone after a panic unwinds past the guard). Full git-remote-gitlawb suite 42 unit + 3 integration green, fmt + clippy clean.
|
All three addressed on Aborted EOF no longer POSTs (F1). The fetch loop folded Concurrent drain (F2). Factored the spawn/deadline logic into RAII cleanup (F3). The server and clone now live under a single Each fix is RED->GREEN with the guard reverted to confirm it is load-bearing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 272-325: Update run_bounded so a timeout terminates the spawned
git fetch process and all descendant remote-helper processes, using
process-group termination or an equivalent descendant-reaping mechanism before
joining out_reader and err_reader. Preserve the existing completed result and
output collection behavior while ensuring inherited pipe handles are closed and
reader threads cannot hang.
🪄 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: 58807c87-026a-40d6-9bcf-6e2e1dfc5edd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/git-remote-gitlawb/Cargo.tomlcrates/git-remote-gitlawb/src/main.rscrates/git-remote-gitlawb/tests/real_git_fetch.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/git-remote-gitlawb/src/main.rs
| /// Spawn `cmd`, draining stdout and stderr CONCURRENTLY on reader threads while | ||
| /// enforcing `timeout`. Returns `(completed_before_deadline, Output)` (#192, F2). | ||
| /// | ||
| /// The concurrent drain is the whole point: polling `try_wait` without reading lets | ||
| /// a child that writes more than an OS pipe buffer (~64 KiB) block on the full pipe | ||
| /// before it exits, so the deadline would trip on pipe backpressure and report a | ||
| /// false "deadlock signature" even while the child is making progress. Draining | ||
| /// continuously makes the deadline measure a real hang only. | ||
| fn run_bounded(mut cmd: Command, timeout: Duration) -> (bool, std::process::Output) { | ||
| let mut child = cmd | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| .expect("spawn 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 { | ||
| let _ = child.kill(); // closes the pipes so the readers finish | ||
| break false; // timed out: the real deadlock signature | ||
| } | ||
| std::thread::sleep(Duration::from_millis(50)); | ||
| }; | ||
|
|
||
| // The child has exited (or been killed), so the pipes are closed and the reader | ||
| // threads run to completion. Reap the child and collect the drained output. | ||
| let status = child.wait().expect("reap child"); | ||
| let stdout = out_reader.join().expect("stdout reader"); | ||
| let stderr = err_reader.join().expect("stderr reader"); | ||
| ( | ||
| completed, | ||
| std::process::Output { | ||
| status, | ||
| stdout, | ||
| stderr, | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant test file and nearby symbols.
ast-grep outline crates/git-remote-gitlawb/tests/real_git_fetch.rs --view expanded
# Show the exact section around run_bounded and the callers.
sed -n '240,360p' crates/git-remote-gitlawb/tests/real_git_fetch.rs
# Find any other timeout/kill/process-group handling in this test crate.
rg -n "process_group|kill\\(|try_wait|run_bounded|CommandExt|child\\.id\\(|SIGKILL|reqwest::blocking|timeout" crates/git-remote-gitlawb/tests crates/git-remote-gitlawb/srcRepository: Gitlawb/node
Length of output: 9463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the gitlawb remote helper source for how subprocesses are managed.
ast-grep outline crates/git-remote-gitlawb/src --view expanded || true
rg -n "std::process::Command|spawn\\(|stderr|stdout|process_group|CommandExt|reqwest::blocking|timeout|git fetch|remote helper" crates/git-remote-gitlawb/src crates/git-remote-gitlawb/testsRepository: Gitlawb/node
Length of output: 9454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact code paths that spawn the helper and the fetch subprocesses.
sed -n '1,260p' crates/git-remote-gitlawb/tests/real_git_fetch.rsRepository: Gitlawb/node
Length of output: 9734
Kill the whole process tree on timeout
child.kill() only stops the direct git fetch process. If the remote helper is still blocked in its HTTP timeout when the deadline fires, it can keep the inherited pipe handles open and leave the reader threads hanging forever. Kill the helper process group (or otherwise reap descendants) before joining.
🤖 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 272 - 325,
Update run_bounded so a timeout terminates the spawned git fetch process and all
descendant remote-helper processes, using process-group termination or an
equivalent descendant-reaping mechanism before joining out_reader and
err_reader. Preserve the existing completed result and output collection
behavior while ensuring inherited pipe handles are closed and reader threads
cannot hang.
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] Kill the entire fetch process tree before joining the drainers
crates/git-remote-gitlawb/tests/real_git_fetch.rs:305
On the timeout path this kills only the top-levelgit fetchprocess and then joins theread_to_endthreads. Itsgit-remote-gitlawbdescendant inherits those stdout/stderr pipe FDs, so a helper stuck in its HTTP request keeps the pipes open after Git dies; the joins then wait for that helper's 300-second request timeout instead of returning the promised 30-second deadlock result. Start the fetch in a process group and terminate/reap that group (or otherwise reap descendants) before joining the readers. This is also the unresolved current-head CodeRabbit request. -
[P2] Make the withheld-path integration test prove it reached the withheld response
crates/git-remote-gitlawb/tests/real_git_fetch.rs:518
The non-success branch accepts every promptgit fetchfailure and only prints the observed POST count. A regression that fails before the helper sends the first request to the shim—for example a broken advertisement, helper lookup, or connection—therefore passes as the expected rejected filtered pack, so this new test no longer verifies the path it claims to record. Require at least one POST (and preferably assert the expected protocol failure when a failure remains acceptable) before accepting the nonzero exit.
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
--stateless-rpcnegotiation, ensuring correct per-round request/response handling.git fetchagainst an in-process smart-HTTP shim.