diff --git a/CHANGELOG.md b/CHANGELOG.md index 7db28ae..3609367 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fail interrupted push responses instead of completing a success report when + Git emits truncated status data or exits unsuccessfully. + - Drain Git output while uploading push data, preventing pipe deadlocks when receive-pack produces progress or diagnostics before consuming the full pack. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index 65f747a..6b633ea 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -207,6 +207,8 @@ See [`src/git/handlers.rs:22-98`](src/git/handlers.rs:22-98) for the info-refs i Receive-pack feeds stdin while concurrently draining stdout and stderr, so progress or diagnostics cannot deadlock a large upload on full pipes. Its task reaps the child before releasing verification locks on cancellation or I/O error. +Merged push reports require complete native status framing and a successful Git +exit; transport failures cannot manufacture a terminal success report. The upload-pack streaming task retains a repository tracing span. Unexpected process failures include the exit status and stream outcome even when Git diff --git a/src/git/handlers.rs b/src/git/handlers.rs index 0ac7ac7..0d02c92 100644 --- a/src/git/handlers.rs +++ b/src/git/handlers.rs @@ -950,6 +950,22 @@ async fn stream_receive_pack_output( }; if !status.success() { + if push_plan.is_some() { + // The merged report is still buffered. A failed process must not + // complete the push response, even if some ref updates succeeded. + record_git_operation(&metrics, "push", "error"); + error!( + "Git receive-pack failed with {status}: {}", + String::from_utf8_lossy(&stderr_output) + ); + let _ = tx + .send(Err(io::Error::other(format!( + "git receive-pack failed with {status}" + )))) + .await; + return; + } + if let Some(flush) = terminal_flush { sent_stdout = true; if send_body_bytes(&tx, flush).await.is_err() { diff --git a/src/git/receive_pack_plan.rs b/src/git/receive_pack_plan.rs index 52bab63..a376f05 100644 --- a/src/git/receive_pack_plan.rs +++ b/src/git/receive_pack_plan.rs @@ -329,7 +329,10 @@ impl ReceivePackPlan { let mut header = [0; 4]; let first = stdout.read(&mut header[..1]).await?; if first == 0 { - break; + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "Git status ended before flush", + )); } stdout.read_exact(&mut header[1..]).await?; let len = std::str::from_utf8(&header) @@ -337,6 +340,9 @@ impl ReceivePackPlan { .and_then(|s| usize::from_str_radix(s, 16).ok()) .ok_or_else(|| io::Error::other("invalid Git status framing"))?; if len == 0 { + if !self.sideband { + report.extend(b"0000"); + } break; } if !(5..=65520).contains(&len) { @@ -371,6 +377,21 @@ impl ReceivePackPlan { return Err(io::Error::other("Git status report exceeds command budget")); } } + // Sideband can split the inner report at any byte. Validate its + // complete framing before manufacturing a merged terminal response. + let mut remaining = report.as_slice(); + loop { + let (packet, tail) = PktLine::parse(remaining).map_err(|_| { + io::Error::new(io::ErrorKind::UnexpectedEof, "incomplete Git status report") + })?; + remaining = tail; + if packet == PktLine::Flush { + if !remaining.is_empty() { + return Err(io::Error::other("trailing Git status data")); + } + break; + } + } Ok(()) }; let result: io::Result<()> = tokio::select! { @@ -659,6 +680,46 @@ mod tests { ); } + #[tokio::test] + async fn truncated_reports_are_transport_errors_not_synthetic_successes() { + for sideband in [false, true] { + for missing_inner_flush in [false, true] { + let (dir, oid) = fixture(); + let caps = if sideband { + "report-status side-band-64k" + } else { + "report-status" + }; + let mut plan = + ReceivePackPlan::prepare(&request(&oid, caps, false), &HashMap::new()).unwrap(); + plan.verify(dir.path()); + let mut report = native_report("ok refs/heads/feature\n"); + if missing_inner_flush || !sideband { + report.truncate(report.len() - 4); + } + let bytes = if sideband { + let mut payload = vec![1]; + payload.extend(report); + let mut bytes = PktLine::data(payload).encode(); + if missing_inner_flush { + bytes.extend(b"0000"); + } + bytes + } else { + report + }; + let (tx, mut rx) = mpsc::channel(4); + let (outcome, response) = + timeout(Duration::from_secs(5), plan.pump(bytes.as_slice(), &tx)) + .await + .expect("truncated report should terminate"); + assert_eq!(outcome, PumpResult::ReadError); + assert!(response.is_none()); + assert!(rx.try_recv().unwrap().is_err()); + } + } + } + #[tokio::test] async fn sideband_progress_streams_before_final_status_and_status_v2_survives() { let (dir, oid) = fixture(); diff --git a/tests/git_response_streaming.rs b/tests/git_response_streaming.rs index 6f961d7..99a9411 100644 --- a/tests/git_response_streaming.rs +++ b/tests/git_response_streaming.rs @@ -61,6 +61,15 @@ impl Drop for PathOverride { #[tokio::test] async fn receive_pack_response_streams_stdout_before_subprocess_exit() { + exercise_receive_pack_stream(false).await; +} + +#[tokio::test] +async fn receive_pack_failed_exit_does_not_emit_a_buffered_success_report() { + exercise_receive_pack_stream(true).await; +} + +async fn exercise_receive_pack_stream(failed_exit: bool) { let _env_lock = PATH_ENV_LOCK.lock().await; let fake_bin = tempfile::tempdir().expect("fake git bin tempdir"); @@ -69,6 +78,11 @@ async fn receive_pack_response_streams_stdout_before_subprocess_exit() { // Force output before consuming an upload larger than a pipe buffer. let script_path = fake_bin.path().join("git"); let script = std::fs::read_to_string(&script_path).unwrap(); + let script = if failed_exit { + script.replacen("exit 0", "exit 1", 1) + } else { + script + }; std::fs::write( &script_path, script @@ -110,6 +124,19 @@ async fn receive_pack_response_streams_stdout_before_subprocess_exit() { let mut streamed = read_first_progress(&mut body).await; // Git cannot exit until this test releases it, regardless of scheduling. git_gate.release().await; + if failed_exit { + timeout(Duration::from_secs(3), async { + while let Ok(frame) = body.frame().await.expect("transport error before EOF") { + streamed.extend_from_slice(&frame_data(frame)); + } + }) + .await + .expect("failed Git exit should fail the response"); + assert!(!streamed + .windows(b"unpack ok".len()) + .any(|part| part == b"unpack ok")); + return; + } finish_body(&mut body, &mut streamed).await; assert_eq!(streamed, expected_git_output()); }