diff --git a/crates/engine/src/local_copy/context.rs b/crates/engine/src/local_copy/context.rs index b08f63ca6..b60742300 100644 --- a/crates/engine/src/local_copy/context.rs +++ b/crates/engine/src/local_copy/context.rs @@ -401,14 +401,27 @@ impl<'a> FinalizeMetadataParams<'a> { #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] pub(crate) struct FileCopyOutcome { literal_bytes: u64, + matched_bytes: u64, compressed_bytes: Option, } +/// Matched-byte count for copy paths that never match a basis block. +/// +/// A whole-file copy, a sparse whole-file copy, and an append all reach the +/// destination without consulting a signature, so nothing is "matched" even +/// though most of the resulting file may not be literal either. upstream: +/// match_sums() only reaches `matched()` (match.c:121) via `hash_search()`, +/// which is skipped when `s->count == 0` - the whole-file case - and append +/// mode zeroes `s->count` outright (match.c:389-390). +pub(crate) const MATCHED_NONE: u64 = 0; + impl FileCopyOutcome { - /// Creates a new outcome with the given literal and optional compressed byte counts. - const fn new(literal_bytes: u64, compressed_bytes: Option) -> Self { + /// Creates a new outcome with the given literal, matched, and optional + /// compressed byte counts. + const fn new(literal_bytes: u64, matched_bytes: u64, compressed_bytes: Option) -> Self { Self { literal_bytes, + matched_bytes, compressed_bytes, } } @@ -418,6 +431,16 @@ impl FileCopyOutcome { self.literal_bytes } + /// Returns the number of bytes supplied by block matches against the basis. + /// + /// Reported by the copy paths that actually emit matches, never inferred + /// from the file size. upstream: match.c:121 - `stats.matched_data` grows + /// only inside `matched()`, so a whole-file copy and an append both leave + /// it at zero even though most of their bytes were not literal. + pub(crate) const fn matched_bytes(self) -> u64 { + self.matched_bytes + } + /// Returns the compressed byte count, if compression was used. pub(crate) const fn compressed_bytes(self) -> Option { self.compressed_bytes @@ -997,21 +1020,21 @@ mod tests { #[test] fn file_copy_outcome_new_stores_values() { - let outcome = FileCopyOutcome::new(1000, Some(500)); + let outcome = FileCopyOutcome::new(1000, 0, Some(500)); assert_eq!(outcome.literal_bytes(), 1000); assert_eq!(outcome.compressed_bytes(), Some(500)); } #[test] fn file_copy_outcome_new_without_compression() { - let outcome = FileCopyOutcome::new(2000, None); + let outcome = FileCopyOutcome::new(2000, 0, None); assert_eq!(outcome.literal_bytes(), 2000); assert!(outcome.compressed_bytes().is_none()); } #[test] fn file_copy_outcome_zero_bytes() { - let outcome = FileCopyOutcome::new(0, Some(0)); + let outcome = FileCopyOutcome::new(0, 0, Some(0)); assert_eq!(outcome.literal_bytes(), 0); assert_eq!(outcome.compressed_bytes(), Some(0)); } @@ -1025,7 +1048,7 @@ mod tests { #[test] fn file_copy_outcome_clone() { - let outcome = FileCopyOutcome::new(100, Some(50)); + let outcome = FileCopyOutcome::new(100, 0, Some(50)); let cloned = outcome; assert_eq!(cloned.literal_bytes(), 100); assert_eq!(cloned.compressed_bytes(), Some(50)); @@ -1033,7 +1056,7 @@ mod tests { #[test] fn file_copy_outcome_debug_format() { - let outcome = FileCopyOutcome::new(100, None); + let outcome = FileCopyOutcome::new(100, 0, None); let debug = format!("{outcome:?}"); assert!(debug.contains("FileCopyOutcome")); assert!(debug.contains("100")); @@ -1041,9 +1064,9 @@ mod tests { #[test] fn file_copy_outcome_eq() { - let a = FileCopyOutcome::new(100, Some(50)); - let b = FileCopyOutcome::new(100, Some(50)); - let c = FileCopyOutcome::new(100, Some(60)); + let a = FileCopyOutcome::new(100, 0, Some(50)); + let b = FileCopyOutcome::new(100, 0, Some(50)); + let c = FileCopyOutcome::new(100, 0, Some(60)); assert_eq!(a, b); assert_ne!(a, c); } diff --git a/crates/engine/src/local_copy/context_impl/delta_transfer.rs b/crates/engine/src/local_copy/context_impl/delta_transfer.rs index 35d30eef7..f37ab4a91 100644 --- a/crates/engine/src/local_copy/context_impl/delta_transfer.rs +++ b/crates/engine/src/local_copy/context_impl/delta_transfer.rs @@ -119,6 +119,14 @@ impl<'a> CopyContext<'a> { // match_report()). let mut probe = ProbeCounters::default(); let mut file_matches = 0u64; + // Accumulated where matches are produced, never derived from + // `total_size - literal_bytes`. upstream: match.c:121 `matched()` is the + // only place `stats.matched_data` grows, so a byte counts as matched + // exactly when a block match emitted it - bytes that are neither + // literal nor matched (the prefix an append skips over) count as + // neither. upstream: match.c:389-390 sets `last_match = s->flength; + // s->count = 0;` so append mode never reaches `matched()` at all. + let mut matched_bytes = 0u64; let mut sparse_state = SparseWriteState::default(); sparse_state.set_preallocated_len(preallocated_len); let mut window: VecDeque = VecDeque::with_capacity(index.block_length()); @@ -281,6 +289,7 @@ impl<'a> CopyContext<'a> { } total_bytes = total_bytes.saturating_add(block_len as u64); + matched_bytes = matched_bytes.saturating_add(block_len as u64); let progressed = initial_bytes.saturating_add(total_bytes); self.notify_progress(relative, Some(total_size), progressed, start.elapsed()); window.clear(); @@ -395,6 +404,7 @@ impl<'a> CopyContext<'a> { } total_bytes = total_bytes.saturating_add(block_len as u64); + matched_bytes = matched_bytes.saturating_add(block_len as u64); let progressed = initial_bytes.saturating_add(total_bytes); self.notify_progress(relative, Some(total_size), progressed, start.elapsed()); window.clear(); @@ -477,9 +487,9 @@ impl<'a> CopyContext<'a> { let delta = compressed_total.saturating_sub(compressed_progress); self.register_limiter_bytes(delta); self.record_adaptive_compression(literal_bytes, compressed_total); - FileCopyOutcome::new(literal_bytes, Some(compressed_total)) + FileCopyOutcome::new(literal_bytes, matched_bytes, Some(compressed_total)) } else { - FileCopyOutcome::new(literal_bytes, None) + FileCopyOutcome::new(literal_bytes, matched_bytes, None) }; Ok(outcome) diff --git a/crates/engine/src/local_copy/context_impl/transfer.rs b/crates/engine/src/local_copy/context_impl/transfer.rs index 2e49a6234..d1985cbf8 100644 --- a/crates/engine/src/local_copy/context_impl/transfer.rs +++ b/crates/engine/src/local_copy/context_impl/transfer.rs @@ -634,7 +634,7 @@ impl<'a> CopyContext<'a> { let progressed = initial_bytes.saturating_add(copied); self.notify_progress(relative, Some(total_size), progressed, start.elapsed()); } - return Ok(FileCopyOutcome::new(copied, None)); + return Ok(FileCopyOutcome::new(copied, MATCHED_NONE, None)); } if sparse { @@ -715,9 +715,9 @@ impl<'a> CopyContext<'a> { let delta = compressed_total.saturating_sub(compressed_progress); self.register_limiter_bytes(delta); self.record_adaptive_compression(literal_bytes, compressed_total); - FileCopyOutcome::new(literal_bytes, Some(compressed_total)) + FileCopyOutcome::new(literal_bytes, MATCHED_NONE, Some(compressed_total)) } else { - FileCopyOutcome::new(literal_bytes, None) + FileCopyOutcome::new(literal_bytes, MATCHED_NONE, None) }; Ok(outcome) @@ -830,9 +830,9 @@ impl<'a> CopyContext<'a> { let delta = compressed_total.saturating_sub(compressed_progress); self.register_limiter_bytes(delta); self.record_adaptive_compression(literal_bytes, compressed_total); - FileCopyOutcome::new(literal_bytes, Some(compressed_total)) + FileCopyOutcome::new(literal_bytes, MATCHED_NONE, Some(compressed_total)) } else { - FileCopyOutcome::new(literal_bytes, None) + FileCopyOutcome::new(literal_bytes, MATCHED_NONE, None) }; Ok(outcome) diff --git a/crates/engine/src/local_copy/executor/file/append.rs b/crates/engine/src/local_copy/executor/file/append.rs index 2183046bf..da12f7305 100644 --- a/crates/engine/src/local_copy/executor/file/append.rs +++ b/crates/engine/src/local_copy/executor/file/append.rs @@ -14,15 +14,32 @@ pub(crate) enum AppendMode { Disabled, /// Destination is already at least as large as the source; skip the file. Skip, - /// Destination is shorter; append starting from the given offset. - Append(u64), + /// Destination is shorter; append starting from `offset`. + Append { + /// Byte offset the appended tail starts at - the destination's length. + offset: u64, + /// Whether `--append-verify`'s whole-file re-checksum will fail, so the + /// caller must retain the appended result and redo the file in phase 2. + verify_failed: bool, + }, } /// Decides the append strategy for a file based on existing destination size. /// /// Returns `Disabled` when append is off, `Skip` when the destination is -/// already at least as large, or `Append(offset)` when the destination is -/// shorter and the transfer should resume from that offset. +/// already at least as large, or `Append` when the destination is shorter and +/// the transfer should resume from that offset. +/// +/// Under `--append-verify` a mismatching prefix does **not** cancel the append. +/// Upstream appends first and only then compares whole-file checksums: the +/// sender sums the source's first `flength` bytes and the receiver sums the +/// destination's, both followed by the identical appended tail +/// (match.c:372-391, receiver.c:352-379), so the comparison reduces exactly to +/// "do the two prefixes agree". A disagreement is reported through +/// `verify_failed` rather than acted on here, because upstream keeps the +/// appended bytes on disk (receiver.c:1029, reached because `--append` implies +/// `--inplace` - options.c:2400-2411) and redoes the file in phase 2 against +/// that retained partial. // upstream: receiver.c:recv_files() - append mode size comparison pub(crate) fn determine_append_mode( append_allowed: bool, @@ -60,21 +77,23 @@ pub(crate) fn determine_append_mode( return Ok(AppendMode::Skip); } - if append_verify { - let matches = verify_append_prefix(reader, source, destination, existing_len)?; - reader - .seek(SeekFrom::Start(0)) - .map_err(|error| LocalCopyError::io("copy file", source, error))?; - if !matches { - return Ok(AppendMode::Disabled); - } + // Plain `--append` (append_mode == 1) never re-checksums: upstream skips the + // prefix in `sum_update` on both sides (match.c:373-391 runs the CHUNK_SIZE + // loop only when `append_mode == 2`), so the two whole-file sums cover just + // the appended tail and always agree. + let verify_failed = if append_verify { + !verify_append_prefix(reader, source, destination, existing_len)? } else { - reader - .seek(SeekFrom::Start(0)) - .map_err(|error| LocalCopyError::io("copy file", source, error))?; - } + false + }; + reader + .seek(SeekFrom::Start(0)) + .map_err(|error| LocalCopyError::io("copy file", source, error))?; - Ok(AppendMode::Append(existing_len)) + Ok(AppendMode::Append { + offset: existing_len, + verify_failed, + }) } /// Verifies that the existing destination prefix matches the source. @@ -272,7 +291,15 @@ mod tests { .expect("determine"); match result { - AppendMode::Append(offset) => assert_eq!(offset, 6), // "source" is 6 bytes + AppendMode::Append { + offset, + verify_failed, + } => { + assert_eq!(offset, 6); // "source" is 6 bytes + // Without --append-verify upstream never re-checksums the + // prefix, so no redo can be requested. + assert!(!verify_failed); + } AppendMode::Disabled | AppendMode::Skip => panic!("expected Append mode"), } } @@ -299,13 +326,19 @@ mod tests { .expect("determine"); match result { - AppendMode::Append(offset) => assert_eq!(offset, 15), + AppendMode::Append { + offset, + verify_failed, + } => { + assert_eq!(offset, 15); + assert!(!verify_failed); + } AppendMode::Disabled | AppendMode::Skip => panic!("expected Append mode"), } } #[test] - fn determine_append_mode_with_verify_disabled_when_prefix_mismatch() { + fn determine_append_mode_still_appends_when_verify_will_fail() { let temp = tempdir().expect("tempdir"); let source_path = temp.path().join("source.txt"); let dest_path = temp.path().join("dest.txt"); @@ -325,7 +358,25 @@ mod tests { ) .expect("determine"); - assert!(matches!(result, AppendMode::Disabled)); + // Degrading to a plain whole-file copy here is exactly the bug this + // encodes against: upstream appends the tail regardless, keeps the + // result (--append implies --inplace, options.c:2400-2411), and only + // then reports the failed whole-file re-checksum so the generator can + // redo the file against the retained partial (generator.c:2175-2217). + // Cancelling the append would leave nothing to re-delta and would make + // the transfer look like a clean single-pass copy. + match result { + AppendMode::Append { + offset, + verify_failed, + } => { + assert_eq!(offset, 16); + assert!(verify_failed); + } + AppendMode::Disabled | AppendMode::Skip => { + panic!("a failed verification must still append, then redo") + } + } } #[test] diff --git a/crates/engine/src/local_copy/executor/file/copy/dry_run.rs b/crates/engine/src/local_copy/executor/file/copy/dry_run.rs index 590ebc506..0c05523b2 100644 --- a/crates/engine/src/local_copy/executor/file/copy/dry_run.rs +++ b/crates/engine/src/local_copy/executor/file/copy/dry_run.rs @@ -160,8 +160,12 @@ pub(super) fn handle_dry_run( )); return Ok(()); } + // A dry run never writes, so the phase-2 redo a failed `--append-verify` + // would trigger has nothing to report: upstream's `if (!do_xfers)` leg + // (receiver.c:805-810) logs the item and `continue`s, never reaching the + // `recv_ok` switch that queues the redo. let append_offset = match append_mode { - AppendMode::Append(offset) => offset, + AppendMode::Append { offset, .. } => offset, AppendMode::Disabled | AppendMode::Skip => 0, }; let bytes_transferred = file_size.saturating_sub(append_offset); diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/clonefile.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/clonefile.rs index 5f4824eb2..657080f83 100644 --- a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/clonefile.rs +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/clonefile.rs @@ -156,7 +156,9 @@ pub(super) fn try_clone( context.record_hard_link(metadata, destination); context .summary_mut() - .record_file(file_size, file_size, None); + // A whole-file clone/copy is all literal: no signature was + // consulted, so nothing was matched. + .record_file(file_size, file_size, crate::local_copy::MATCHED_NONE, None); context .summary_mut() .record_copy_method(CopyMethodKind::from_platform(clone_method)); diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/ficlone.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/ficlone.rs index 071edb091..d32db315a 100644 --- a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/ficlone.rs +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/ficlone.rs @@ -143,7 +143,9 @@ pub(super) fn try_clone( context.record_hard_link(metadata, destination); context .summary_mut() - .record_file(file_size, file_size, None); + // A whole-file clone/copy is all literal: no signature was + // consulted, so nothing was matched. + .record_file(file_size, file_size, crate::local_copy::MATCHED_NONE, None); context .summary_mut() .record_copy_method(CopyMethodKind::Ficlone); diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/iouring.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/iouring.rs index 4265a6cb2..2ab2cec60 100644 --- a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/iouring.rs +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/iouring.rs @@ -118,7 +118,9 @@ pub(super) fn try_dispatch( context.record_hard_link(metadata, destination); context .summary_mut() - .record_file(file_size, file_size, None); + // A whole-file clone/copy is all literal: no signature was + // consulted, so nothing was matched. + .record_file(file_size, file_size, crate::local_copy::MATCHED_NONE, None); context .summary_mut() .record_copy_method(CopyMethodKind::IoUring); diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/mod.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/mod.rs index 1e5696fab..d5d699d9e 100644 --- a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/mod.rs +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/mod.rs @@ -1,9 +1,10 @@ //! Main file transfer orchestration. //! -//! Hosts [`execute_transfer`], the central function that drives a single file -//! copy: skip detection, backup, append-mode resume, delta signature, +//! Hosts [`execute_transfer_once`], the central function that drives a single +//! file copy pass: skip detection, backup, append-mode resume, delta signature, //! writer-strategy selection (direct, inplace, temp-file), buffer allocation, -//! data copy, and post-transfer bookkeeping. +//! data copy, and post-transfer bookkeeping. The `verify_redo` module wraps it +//! to supply the second pass an `--append-verify` failure requires. //! //! # Submodules //! @@ -43,10 +44,10 @@ use super::super::super::comparison::build_delta_signature; use super::super::super::compute_backup_path; use super::super::super::guard::remove_incomplete_destination; use super::super::super::preallocate::maybe_preallocate_destination; -use super::TransferFlags; use super::finalize::finalize_guard_and_metadata; use super::open::open_source_file; use super::write_strategy::{open_destination_writer, select_write_strategy}; +use super::{TransferFlags, TransferOutcome}; use skip::try_skip_up_to_date; @@ -57,7 +58,7 @@ use skip::try_skip_up_to_date; /// when a usable basis file exists at the destination. The caller is /// responsible for pre-checks (dry-run, size filters, link processing). #[allow(clippy::too_many_arguments)] -pub(in crate::local_copy) fn execute_transfer( +pub(in crate::local_copy) fn execute_transfer_once( context: &mut CopyContext, source: &Path, destination: &Path, @@ -76,7 +77,7 @@ pub(in crate::local_copy) fn execute_transfer( // rather than a network transfer (`>`). // upstream: generator.c:1039 - itemize(..., ITEM_LOCAL_CHANGE, ...). reference_basis: Option, -) -> Result<(), LocalCopyError> { +) -> Result { #[cfg(not(all(unix, any(feature = "xattr", feature = "acl"))))] let _ = mode; @@ -122,7 +123,7 @@ pub(in crate::local_copy) fn execute_transfer( &flags, mode, )? { - return Ok(()); + return Ok(TransferOutcome::Complete); } } @@ -223,6 +224,8 @@ pub(in crate::local_copy) fn execute_transfer( // instead falls through to the generic read/write loop below, which streams // its `file_size` bytes just like a regular file (upstream sender.c:410-418). if !file_type.is_file() && device_as_file_size.is_none() { + // A placeholder write carries no appended tail and no whole-file + // checksum, so it can never fail verification. return super::special::copy_special_as_regular_file( context, source, @@ -236,7 +239,8 @@ pub(in crate::local_copy) fn execute_transfer( relative, mode, flags, - ); + ) + .map(|()| TransferOutcome::Complete); } // Fast path: macOS clonefile for new whole-file copies. Skipped for @@ -265,7 +269,7 @@ pub(in crate::local_copy) fn execute_transfer( flags, )? { - return Ok(()); + return Ok(TransferOutcome::Complete); } // Fast path: Windows CopyFileExW / ReFS reflink for new whole-file copies. @@ -296,7 +300,7 @@ pub(in crate::local_copy) fn execute_transfer( flags, )? { - return Ok(()); + return Ok(TransferOutcome::Complete); } // Fast path: Linux FICLONE reflink for new whole-file copies on Btrfs, @@ -325,7 +329,7 @@ pub(in crate::local_copy) fn execute_transfer( flags, )? { - return Ok(()); + return Ok(TransferOutcome::Complete); } let mut reader = open_source_file(source, context.open_noatime_enabled()) @@ -355,11 +359,19 @@ pub(in crate::local_copy) fn execute_transfer( Duration::default(), Some(metadata_snapshot), )); - return Ok(()); + return Ok(TransferOutcome::Complete); } + // `verify_failed` is upstream's `recv_ok == 0` for this file, decided before + // the append rather than after it because the local executor can compare the + // two prefixes directly. The append still runs: upstream retains the result + // and re-deltas it in phase 2 (receiver.c:1029, generator.c:2175-2217). + let mut verify_failed = false; let append_offset = match append_mode { - AppendMode::Append(offset) => { + AppendMode::Append { + offset, + verify_failed: failed, + } => { debug_log!( Send, 2, @@ -367,6 +379,7 @@ pub(in crate::local_copy) fn execute_transfer( record_path.display(), offset ); + verify_failed = failed; offset } AppendMode::Disabled | AppendMode::Skip => 0, @@ -467,7 +480,7 @@ pub(in crate::local_copy) fn execute_transfer( mode, flags, )? { - return Ok(()); + return Ok(TransferOutcome::Complete); } let mut writer = open_destination_writer( @@ -662,9 +675,12 @@ pub(in crate::local_copy) fn execute_transfer( } let compressed_bytes = outcome.compressed_bytes(); - context - .summary_mut() - .record_file(file_size, outcome.literal_bytes(), compressed_bytes); + context.summary_mut().record_file( + file_size, + outcome.literal_bytes(), + outcome.matched_bytes(), + compressed_bytes, + ); context .summary_mut() .record_copy_method(CopyMethodKind::Standard); @@ -778,7 +794,16 @@ pub(in crate::local_copy) fn execute_transfer( preserve_acls, )?; - Ok(()) + // upstream: receiver.c:1015 - `recv_ok = receive_data(...)` compares the + // sender's whole-file checksum against the receiver's. The local executor + // reduces that comparison to the pre-append prefix comparison recorded in + // `verify_failed` (see `determine_append_mode`), because both sides sum the + // same appended tail. + Ok(if verify_failed { + TransferOutcome::VerificationFailed + } else { + TransferOutcome::Complete + }) } /// Finds a fuzzy delta basis for `destination` when the exact destination is diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/wincopy.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/wincopy.rs index d21e2ca6f..2be2bf1f0 100644 --- a/crates/engine/src/local_copy/executor/file/copy/transfer/execute/wincopy.rs +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/execute/wincopy.rs @@ -128,7 +128,9 @@ pub(super) fn try_copy( context.record_hard_link(metadata, destination); context .summary_mut() - .record_file(file_size, file_size, None); + // A whole-file clone/copy is all literal: no signature was + // consulted, so nothing was matched. + .record_file(file_size, file_size, crate::local_copy::MATCHED_NONE, None); context .summary_mut() .record_copy_method(CopyMethodKind::from_platform(dispatched_method)); diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/mod.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/mod.rs index 489500795..6bc37c078 100644 --- a/crates/engine/src/local_copy/executor/file/copy/transfer/mod.rs +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/mod.rs @@ -17,11 +17,29 @@ mod execute; mod finalize; mod open; mod special; +mod verify_redo; mod write_strategy; -pub(super) use execute::execute_transfer; #[cfg(test)] pub(crate) use open::take_fsync_call_count; +pub(super) use verify_redo::execute_transfer; + +/// Whether a single transfer pass completed or must be redone. +/// +/// Mirrors upstream's `recv_ok` for the two outcomes the local executor can +/// produce: `recv_ok == 1` (committed) and `recv_ok == 0` (the whole-file +/// re-checksum failed, so the update is kept and the file is queued for the +/// phase-2 redo). upstream: receiver.c:1061-1101. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(in crate::local_copy) enum TransferOutcome { + /// The pass committed; nothing further is owed for this file. + Complete, + /// An `--append-verify` pass appended its tail but the whole-file + /// re-checksum disagreed. The appended bytes stay on disk and the caller + /// must rerun the file as an ordinary delta transfer. + /// upstream: receiver.c:1096 `send_msg_int(MSG_REDO, ndx)`. + VerificationFailed, +} /// Boolean flags controlling file transfer behavior. /// diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/special.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/special.rs index ba9d544a8..7956fcb7f 100644 --- a/crates/engine/src/local_copy/executor/file/copy/transfer/special.rs +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/special.rs @@ -83,7 +83,11 @@ pub(in crate::local_copy) fn copy_special_as_regular_file( context.record_hard_link(metadata, hard_link_path); let elapsed = start.elapsed(); - context.summary_mut().record_file(metadata.len(), 0, None); + // A placeholder is written from nothing: no literal bytes off the wire + // and no basis block matched. + context + .summary_mut() + .record_file(metadata.len(), 0, crate::local_copy::MATCHED_NONE, None); context.summary_mut().record_elapsed(elapsed); let metadata_snapshot = LocalCopyMetadata::from_metadata(metadata, None); let total_bytes = Some(metadata_snapshot.len()); diff --git a/crates/engine/src/local_copy/executor/file/copy/transfer/verify_redo.rs b/crates/engine/src/local_copy/executor/file/copy/transfer/verify_redo.rs new file mode 100644 index 000000000..23066f24f --- /dev/null +++ b/crates/engine/src/local_copy/executor/file/copy/transfer/verify_redo.rs @@ -0,0 +1,233 @@ +//! The `--append-verify` append -> verify -> retain -> redo cycle. +//! +//! Upstream gets this for free on a local transfer because "local" is just a +//! client and a server exchanging the ordinary protocol over a socketpair +//! (main.c:1468 sets `local_server`, main.c:649-655 forks `local_child`, and +//! main.c:1050-1132 forks again so `recv_files()` and `generate_files()` run +//! concurrently). The local executor is a separate implementation of that +//! transfer, so it has to reproduce the semantics explicitly: +//! +//! 1. **Append first.** The sender jumps `last_match` to the destination's +//! length and zeroes the block count (match.c:372-391), and the generator +//! writes a sum header with no block sums (generator.c:787), so pass one is +//! always a pure append. +//! 2. **Verify the whole file.** `receive_data()` compares the sender's +//! whole-file checksum with the receiver's (receiver.c:518-519). With +//! `--append-verify` both sides fold the pre-existing prefix into that sum +//! (match.c:373-386, receiver.c:357-371) and then the identical appended +//! tail, so the comparison is exactly "do the two prefixes agree". +//! 3. **Retain the result.** `--append` implies `--inplace` +//! (options.c:2400-2411), so `finish_transfer()` runs even for `recv_ok == 0` +//! (receiver.c:1029) and the appended bytes stay on disk. +//! 4. **Warn and request the redo** (receiver.c:1063-1097). +//! 5. **Redo as an ordinary delta.** The generator re-enters `recv_generator()` +//! with `append_mode` negated and `ignore_times` bumped +//! (generator.c:2186-2200), and `whole_file` was already forced to 0 for the +//! whole session because append mode is active (generator.c:2288-2289), so +//! the retained partial is described as the delta basis +//! (generator.c:1967 -> generate_and_send_sums). + +use std::fs; +use std::path::{Path, PathBuf}; + +use ::metadata::MetadataOptions; + +use crate::local_copy::{CopyContext, LocalCopyError, LocalCopyExecution}; + +use super::execute::execute_transfer_once; +use super::{TransferFlags, TransferOutcome}; + +/// Executes the data transfer for a single regular file, including the +/// `--append-verify` phase-2 redo when the first pass fails verification. +/// +/// The common case is a single call to [`execute_transfer_once`]. Only an +/// `--append-verify` pass whose whole-file re-checksum disagreed runs a second +/// pass, and that pass is an ordinary delta transfer against the partial the +/// first pass left on disk. +#[allow(clippy::too_many_arguments)] +pub(in crate::local_copy) fn execute_transfer( + context: &mut CopyContext, + source: &Path, + destination: &Path, + metadata: &fs::Metadata, + metadata_options: MetadataOptions, + record_path: &Path, + existing_metadata: Option<&fs::Metadata>, + destination_previously_existed: bool, + file_type: fs::FileType, + relative: Option<&Path>, + flags: TransferFlags, + mode: LocalCopyExecution, + copy_source_override: Option, + reference_basis: Option, +) -> Result<(), LocalCopyError> { + let outcome = execute_transfer_once( + context, + source, + destination, + metadata, + metadata_options.clone(), + record_path, + existing_metadata, + destination_previously_existed, + file_type, + relative, + flags, + mode, + copy_source_override.clone(), + reference_basis.clone(), + )?; + + if outcome == TransferOutcome::Complete { + return Ok(()); + } + + warn_verification_failed(context, record_path); + + // The first pass appended in place, so the destination now holds the full + // source length with a wrong prefix. Re-stat it: that partial is the delta + // basis for the redo, exactly as upstream's `generate_and_send_sums()` reads + // the retained file. Losing it between the passes means there is nothing to + // re-delta, so a stat failure is a hard error rather than a fall-back to a + // whole-file copy. + let retained = fs::symlink_metadata(destination).map_err(|error| { + LocalCopyError::io("inspect retained partial", destination.to_path_buf(), error) + })?; + + execute_transfer_once( + context, + source, + destination, + metadata, + metadata_options, + record_path, + Some(&retained), + // The destination existed before this pass: upstream counts + // `stats.created_files++` only on the non-redo leg (receiver.c:778). + true, + file_type, + relative, + redo_flags(flags, context.sparse_enabled()), + mode, + copy_source_override, + reference_basis, + )?; + + Ok(()) +} + +/// The flag set upstream's generator installs around the phase-2 redo. +/// +/// upstream: generator.c:2186-2188 negates `append_mode` and bumps +/// `ignore_times`; generator.c:2288-2289 already forced `whole_file = 0` for the +/// session because append mode is active, which is what overrides the +/// `whole_file = 1` default a local transfer would otherwise carry +/// (main.c:652-653); receiver.c:761,771 negates `sparse_files` alongside +/// `append_mode`, restoring whatever `--sparse` asked for. +fn redo_flags(flags: TransferFlags, sparse_enabled: bool) -> TransferFlags { + TransferFlags { + append_allowed: false, + append_verify: false, + whole_file_enabled: false, + // Without this the quick-check would skip the redo outright: the + // retained partial now has the source's size, and pass one already + // stamped it with the source's mtime. + ignore_times_enabled: true, + use_sparse_writes: sparse_enabled, + ..flags + } +} + +/// Emits upstream's retained-update warning for a failed verification. +/// +/// upstream: receiver.c:1071-1094. The line is gated behind +/// `INFO_GTE(NAME, 1) || stdout_format_has_i` (receiver.c:1072) and carries the +/// transfer-relative name, never an absolute path. +/// +/// `keptstr` is unconditionally `"retained"`. Upstream reaches that string +/// because `--append` implies `--inplace` (options.c:2400-2411), which takes +/// receiver.c:1073-1078 past both the `"discarded"` and the +/// `"put into partial-dir"` leg. The local executor arrives at the same place by +/// a different route: this warning is reachable only when `append_offset > 0`, +/// which pins `select_write_strategy` to `WriteStrategy::Append`, and that +/// strategy writes straight into the destination - so the update is retained +/// there no matter what `--partial-dir` says. +/// +/// `redostr` is `" (will try again)"` because the local executor never replays a +/// batch, and the code is `FWARNING` rather than `FERROR_XFER` because the redo +/// this warning announces has not run yet (upstream's `redoing` is still 0). +/// +/// The line goes to stderr because upstream emits it as `FWARNING` +/// (rsync.h:278, routed by log.c:314-316), matching the other stderr notices +/// this executor writes. +fn warn_verification_failed(context: &CopyContext, record_path: &Path) { + if !logging::info_gte(logging::InfoFlag::Name, 1) && !context.options().is_itemize_active() { + return; + } + eprintln!( + "WARNING: {} failed verification -- update retained (will try again).", + record_path.display() + ); +} + +#[cfg(test)] +mod tests { + use super::*; + + fn flags() -> TransferFlags { + TransferFlags { + append_allowed: true, + append_verify: true, + whole_file_enabled: true, + inplace_enabled: false, + partial_enabled: false, + use_sparse_writes: false, + compress_enabled: false, + size_only_enabled: false, + ignore_times_enabled: false, + checksum_enabled: false, + #[cfg(all(any(unix, windows), feature = "xattr"))] + preserve_xattrs: false, + xattrs_changed: false, + #[cfg(all(any(unix, windows), feature = "acl"))] + preserve_acls: false, + } + } + + #[test] + fn redo_negates_append_and_forces_a_delta_pass() { + // Each of these is load-bearing for the redo to reproduce upstream's + // second pass: still appending would re-append nothing, whole-file + // would re-send the file as pure literal instead of re-deltaing the + // retained partial, and honouring times would skip the file entirely + // because pass one already gave it the source's size and mtime. + let redo = redo_flags(flags(), false); + assert!(!redo.append_allowed); + assert!(!redo.append_verify); + assert!(!redo.whole_file_enabled); + assert!(redo.ignore_times_enabled); + } + + #[test] + fn redo_restores_sparse_writes_from_the_session_setting() { + // upstream: receiver.c:761,771 negates sparse_files alongside + // append_mode, so --sparse suppressed during the append comes back for + // the redo. Without --sparse it must stay off. + assert!(redo_flags(flags(), true).use_sparse_writes); + assert!(!redo_flags(flags(), false).use_sparse_writes); + } + + #[test] + fn redo_preserves_unrelated_flags() { + // The redo negates only what upstream negates; everything else is the + // session's setting and must survive. + let mut base = flags(); + base.compress_enabled = true; + base.checksum_enabled = true; + base.inplace_enabled = true; + let redo = redo_flags(base, false); + assert!(redo.compress_enabled); + assert!(redo.checksum_enabled); + assert!(redo.inplace_enabled); + } +} diff --git a/crates/engine/src/local_copy/mod.rs b/crates/engine/src/local_copy/mod.rs index c239128d5..ace98cceb 100644 --- a/crates/engine/src/local_copy/mod.rs +++ b/crates/engine/src/local_copy/mod.rs @@ -109,7 +109,7 @@ pub use skip_compress::{SkipCompressList, SkipCompressParseError}; pub(crate) use compressor::ActiveCompressor; pub(crate) use context::{ CopyContext, CopyOutcome, CreatedEntryKind, DeferredUpdate, FinalizeMetadataParams, - MetadataPathContext, OwnedPathContext, + MATCHED_NONE, MetadataPathContext, OwnedPathContext, }; #[allow(unused_imports)] // REASON: convenience re-export; not all items used in every module diff --git a/crates/engine/src/local_copy/plan/summary.rs b/crates/engine/src/local_copy/plan/summary.rs index ba50a83e5..3af9f44df 100644 --- a/crates/engine/src/local_copy/plan/summary.rs +++ b/crates/engine/src/local_copy/plan/summary.rs @@ -783,17 +783,28 @@ impl LocalCopySummary { } } + /// Records one transferred file's contribution to the summary. + /// + /// `matched_bytes` is supplied by the copy path that produced the matches, + /// never inferred here. It used to be derived as `file_size - + /// literal_bytes`, which silently assumed every non-literal byte came from + /// a block match. Append mode falsifies that: the pre-existing prefix is + /// neither literal nor matched, so the derivation counted the whole prefix + /// as matched while upstream reports zero. upstream: match.c:121 grows + /// `stats.matched_data` only inside `matched()`, and append mode never + /// reaches it (match.c:389-390 sets `last_match = s->flength; s->count = + /// 0;` and the hash loop is skipped). pub(in crate::local_copy) fn record_file( &mut self, file_size: u64, literal_bytes: u64, + matched_bytes: u64, compressed: Option, ) { self.files_copied = self.files_copied.saturating_add(1); self.transferred_file_size = self.transferred_file_size.saturating_add(file_size); self.bytes_copied = self.bytes_copied.saturating_add(literal_bytes); - let matched = file_size.saturating_sub(literal_bytes); - self.matched_bytes = self.matched_bytes.saturating_add(matched); + self.matched_bytes = self.matched_bytes.saturating_add(matched_bytes); let transmitted = compressed.unwrap_or(literal_bytes); // A local copy emulates the protocol sender: it writes the file data // (counted as sent) but receives no data payload back. Counting the data @@ -1348,7 +1359,7 @@ mod tests { #[test] fn record_file_increments_counters() { let mut summary = LocalCopySummary::default(); - summary.record_file(1000, 800, None); + summary.record_file(1000, 800, 200, None); assert_eq!(summary.files_copied(), 1); assert_eq!(summary.transferred_file_size(), 1000); @@ -1361,7 +1372,7 @@ mod tests { #[test] fn record_file_with_compression() { let mut summary = LocalCopySummary::default(); - summary.record_file(1000, 800, Some(400)); + summary.record_file(1000, 800, 200, Some(400)); assert_eq!(summary.bytes_copied(), 800); assert_eq!(summary.compressed_bytes(), 400); @@ -1392,8 +1403,8 @@ mod tests { #[test] fn record_multiple_files_accumulates() { let mut summary = LocalCopySummary::default(); - summary.record_file(100, 80, None); - summary.record_file(200, 150, None); + summary.record_file(100, 80, 20, None); + summary.record_file(200, 150, 50, None); assert_eq!(summary.files_copied(), 2); assert_eq!(summary.transferred_file_size(), 300); @@ -1600,7 +1611,7 @@ mod tests { // `bytes_sent` untouched - that data-only figure is what upstream's // `Total bytes sent` is built on for a local copy. let mut summary = LocalCopySummary::default(); - summary.record_file(1_000, 1_000, None); + summary.record_file(1_000, 1_000, 0, None); summary.record_file_list_entry(40); assert_eq!(summary.bytes_sent(), 1_000); diff --git a/crates/engine/src/local_copy/tests/bandwidth.rs b/crates/engine/src/local_copy/tests/bandwidth.rs index ccaa881af..542313f97 100644 --- a/crates/engine/src/local_copy/tests/bandwidth.rs +++ b/crates/engine/src/local_copy/tests/bandwidth.rs @@ -71,11 +71,21 @@ fn execute_with_append_appends_missing_bytes() { assert_eq!(fs::read(&destination).expect("read dest"), b"abcdef"); assert_eq!(summary.bytes_copied(), 3); - assert_eq!(summary.matched_bytes(), 3); + + // Zero, not three. MEASURED against rsync 3.4.4 on this exact fixture + // (`-a --append --ignore-times --stats`, source "abcdef", dest "abc"): + // `Literal data: 3 bytes`, `Matched data: 0 bytes`. + // + // The 3-byte prefix the append skipped is neither literal nor matched. + // Asserting 3 here encoded the old derivation `file_size - literal_bytes`, + // which treats every non-literal byte as matched; upstream only ever + // increments `stats.matched_data` inside `matched()` (match.c:121), which + // append mode never reaches (match.c:389-390). + assert_eq!(summary.matched_bytes(), 0); } #[test] -fn execute_with_append_verify_rewrites_on_mismatch() { +fn execute_with_append_verify_appends_then_redoes_on_mismatch() { let temp = tempdir().expect("tempdir"); let source = temp.path().join("source.txt"); let destination = temp.path().join("dest.txt"); @@ -96,7 +106,24 @@ fn execute_with_append_verify_rewrites_on_mismatch() { .expect("append verify succeeds"); assert_eq!(fs::read(&destination).expect("read dest"), b"abcdef"); - assert_eq!(summary.bytes_copied(), 6); + + // MEASURED against rsync 3.4.4 on this exact fixture + // (`-a --append-verify --ignore-times --stats`, source "abcdef", seed + // "abx"): 2 transfers, `Literal data: 9 bytes`, `Matched data: 0 bytes`. + // + // Nine literal bytes, not six: pass one appends the 3-byte tail, then the + // failed whole-file re-checksum retains it and the redo re-sends all 6 + // bytes as literal, because the 6-byte basis is one short block that does + // not match. Asserting 6 would be asserting that the append never happened. + assert_eq!(summary.bytes_copied(), 9); + + // Zero matched bytes, matching upstream. The 3-byte prefix pass one + // appended past is neither literal nor matched: upstream never calls + // `matched()` in append mode (match.c:389-390 sets + // `last_match = s->flength; s->count = 0;` so the hash loop is skipped), + // and `stats.matched_data` grows nowhere else (match.c:121). Pass two is an + // ordinary delta whose single 6-byte basis block does not match, so it + // contributes nothing either. assert_eq!(summary.matched_bytes(), 0); } diff --git a/crates/engine/src/local_copy/tests/execute_append.rs b/crates/engine/src/local_copy/tests/execute_append.rs index cc6db1f22..3179ebbc0 100644 --- a/crates/engine/src/local_copy/tests/execute_append.rs +++ b/crates/engine/src/local_copy/tests/execute_append.rs @@ -323,7 +323,7 @@ fn append_verify_succeeds_when_prefix_matches() { } #[test] -fn append_verify_retransfers_when_prefix_mismatch() { +fn append_verify_appends_then_redoes_when_prefix_mismatch() { let temp = tempdir().expect("tempdir"); let source = temp.path().join("source.txt"); let destination = temp.path().join("dest.txt"); @@ -345,8 +345,15 @@ fn append_verify_retransfers_when_prefix_mismatch() { ) .expect("copy succeeds"); - // File should be re-transferred completely due to mismatch - assert_eq!(summary.files_copied(), 1); + // Two passes, not one. A failed --append-verify is not a licence to fall + // back to a single whole-file copy: upstream appends the tail, keeps it + // (--append implies --inplace, options.c:2400-2411, so receiver.c:1029 + // commits even for recv_ok == 0), warns, and redoes the file in phase 2 + // against that retained partial (generator.c:2175-2217). Each pass counts + // as a transfer, which is how the redo is observable at all - collapsing to + // one means the retain-and-redo model is missing. + // MEASURED against rsync 3.4.4: `Number of regular files transferred: 2`. + assert_eq!(summary.files_copied(), 2); assert_eq!( fs::read(&destination).expect("read dest"), b"correct source content plus more" @@ -412,14 +419,154 @@ fn append_verify_detects_corruption_in_middle() { ) .expect("copy succeeds"); - // File should be re-transferred due to corruption detection - assert_eq!(summary.files_copied(), 1); + // Corruption inside the existing prefix takes the same append -> verify -> + // retain -> redo route as any other mismatch, so it is two passes. + // MEASURED against rsync 3.4.4 on this exact fixture: 2 transfers, + // `Literal data: 30 bytes`, and the retained-update WARNING on stderr. + assert_eq!(summary.files_copied(), 2); assert_eq!( fs::read(&destination).expect("read dest"), b"0123456789ABCDEFGHIJ" ); } +#[test] +fn append_verify_failure_retains_the_appended_partial_for_the_redo() { + // The load-bearing property of the whole cycle: after the verification + // fails, the bytes the append wrote must still be on disk, because they are + // the delta basis the second pass re-deltas against. This is what upstream + // buys with `--append` implying `--inplace` (options.c:2400-2411), which + // sends receiver.c:1029 down the `|| inplace` leg for recv_ok == 0. + // + // The check is indirect but exact: the source's second half is byte-equal to + // the destination's second half after pass one, so if the partial were + // discarded the redo would have nothing to match and the final file could + // only be reconstructed as pure literal. Asserting the two-pass count plus + // byte-correctness pins both halves of that. + let temp = tempdir().expect("tempdir"); + let source = temp.path().join("source.bin"); + let destination = temp.path().join("dest.bin"); + + // A prefix long enough to span several delta blocks, so the redo has real + // matchable content in the tail the append contributed. + let source_bytes: Vec = (0..40_000u32).map(|i| (i % 251) as u8).collect(); + let seed = vec![0u8; 20_000]; + fs::write(&source, &source_bytes).expect("write source"); + fs::write(&destination, &seed).expect("write mismatching partial"); + + let operands = vec![ + source.into_os_string(), + destination.clone().into_os_string(), + ]; + let plan = LocalCopyPlan::from_operands(&operands).expect("plan"); + + let summary = plan + .execute_with_options( + LocalCopyExecution::Apply, + LocalCopyOptions::default().append_verify(true), + ) + .expect("copy succeeds"); + + assert_eq!( + summary.files_copied(), + 2, + "one append pass plus one redo pass" + ); + assert_eq!( + fs::read(&destination).expect("read dest"), + source_bytes, + "the redo must reconstruct the file exactly" + ); +} + +#[test] +fn append_verify_success_stays_a_single_pass() { + // The redo must be reachable only through a real verification failure. A + // matching prefix means the whole-file sums agree (receiver.c:518-519), so + // upstream never sends MSG_REDO and the file is transferred once. Without + // this guard a redo that fired unconditionally would still produce correct + // bytes and go unnoticed. + let temp = tempdir().expect("tempdir"); + let source = temp.path().join("source.bin"); + let destination = temp.path().join("dest.bin"); + + let source_bytes: Vec = (0..40_000u32).map(|i| (i % 251) as u8).collect(); + fs::write(&source, &source_bytes).expect("write source"); + fs::write(&destination, &source_bytes[..20_000]).expect("write matching partial"); + + let operands = vec![ + source.into_os_string(), + destination.clone().into_os_string(), + ]; + let plan = LocalCopyPlan::from_operands(&operands).expect("plan"); + + let summary = plan + .execute_with_options( + LocalCopyExecution::Apply, + LocalCopyOptions::default().append_verify(true), + ) + .expect("copy succeeds"); + + assert_eq!(summary.files_copied(), 1, "a clean append must not redo"); + assert_eq!(fs::read(&destination).expect("read dest"), source_bytes); +} + +#[test] +fn plain_append_never_redoes_even_with_a_mismatching_prefix() { + // upstream: match.c:373-386 folds the pre-existing prefix into the + // whole-file sum only when `append_mode == 2` (--append-verify). Plain + // --append leaves both sides summing just the appended tail, so the sums + // always agree, no redo is ever requested, and the destination is left + // knowingly wrong. Pinning this keeps the redo from leaking onto --append. + let temp = tempdir().expect("tempdir"); + let source = temp.path().join("source.txt"); + let destination = temp.path().join("dest.txt"); + + fs::write(&source, b"0123456789ABCDEFGHIJ").expect("write source"); + fs::write(&destination, b"01234XXX89").expect("write mismatching partial"); + + let operands = vec![ + source.into_os_string(), + destination.clone().into_os_string(), + ]; + let plan = LocalCopyPlan::from_operands(&operands).expect("plan"); + + let summary = plan + .execute_with_options( + LocalCopyExecution::Apply, + LocalCopyOptions::default().append(true), + ) + .expect("copy succeeds"); + + assert_eq!(summary.files_copied(), 1, "--append must stay single-pass"); + assert_eq!( + fs::read(&destination).expect("read dest"), + b"01234XXX89ABCDEFGHIJ", + "--append keeps the wrong prefix - that is the documented trade-off" + ); + + // The prefix an append skips is neither literal nor matched. Upstream + // accumulates `stats.matched_data` only in `matched()` (match.c:121), and + // append mode never reaches it, so upstream reports 0 - MEASURED on this + // shape: `oc -a --append --ignore-times --stats` over a 100 KiB prefix + // gives upstream `Literal 102,400 / Matched 0`. + // + // This is the assertion that catches deriving the figure as + // `file_size - literal_bytes`: that derivation assumes every non-literal + // byte was matched, which is exactly what append falsifies, and it would + // report the whole 10-byte prefix here. + assert_eq!( + summary.bytes_copied(), + 10, + "only the appended tail is literal" + ); + assert_eq!( + summary.matched_bytes(), + 0, + "an appended-over prefix is not matched data" + ); +} + #[test] fn append_without_verify_blindly_appends() { let temp = tempdir().expect("tempdir");