diff --git a/crates/cli/src/commands/cp.rs b/crates/cli/src/commands/cp.rs index acbbd6b..7b681cc 100644 --- a/crates/cli/src/commands/cp.rs +++ b/crates/cli/src/commands/cp.rs @@ -25,6 +25,7 @@ use crate::exit_code::ExitCode; use crate::output::{Formatter, OutputConfig, ProgressBar, V3SuccessEnvelope}; use crate::secret_input::{SecretLocator, resolve_secret_locator}; +use super::object_identity::set_source_identity; use super::transfer_fidelity::{MetadataDirectiveArg, TaggingDirectiveArg, TransferFidelityArgs}; const CP_AFTER_HELP: &str = "\ @@ -689,7 +690,10 @@ fn validate_fidelity_directions( && sources.iter().any(|source| { matches!( source, - ParsedPath::Remote(source) if target.as_remote().is_some_and(|target| source.alias == target.alias) + ParsedPath::Remote(source) + if target + .as_remote() + .is_some_and(|target| source.alias == target.alias) ) }); if any_remote && target_remote { @@ -1245,6 +1249,7 @@ async fn perform_planned_download( struct PlannedRemoteCopyResult { bytes_copied: u64, source_version_id: Option, + source_etag: Option, destination_version_id: Option, upload_id: Option, object: ObjectInfo, @@ -1312,6 +1317,7 @@ async fn perform_planned_remote_copy( return Ok(PlannedRemoteCopyResult { bytes_copied: copied.bytes_copied, source_version_id: options.source_version_id, + source_etag: current.etag.clone(), destination_version_id: copied.object.version_id.clone(), upload_id: Some(copied.upload_id), object: copied.object, @@ -1332,6 +1338,7 @@ async fn perform_planned_remote_copy( .source_version_id .clone() .or_else(|| source_info.version_id.clone()), + source_etag: source_info.etag.clone(), destination_version_id: copied.version_id.clone(), upload_id: None, object: copied, @@ -1427,6 +1434,7 @@ async fn perform_cross_alias_remote_copy( Ok(PlannedRemoteCopyResult { bytes_copied, source_version_id: current.version_id.clone(), + source_etag: current.etag.clone(), destination_version_id: object.version_id.clone(), upload_id: None, object, @@ -1451,6 +1459,46 @@ fn source_identity_matches(planned: &ObjectInfo, current: &ObjectInfo) -> bool { } } +/// Copy one object between two aliases by streaming through the client. +/// +/// `rc mv` shares this path so a cross-alias move behaves exactly like a +/// cross-alias copy followed by a source delete, rather than reimplementing the +/// download/upload streaming and its source-change checks. +#[derive(Debug, Clone)] +pub(super) struct CrossAliasCopyResult { + pub(super) object: ObjectInfo, + pub(super) source_version_id: Option, + pub(super) source_etag: Option, +} + +pub(super) async fn copy_object_across_aliases( + source_client: &S3Client, + target_client: &S3Client, + source: &RemotePath, + target: &RemotePath, + encryption: Option<&ObjectEncryptionRequest>, +) -> rc_core::Result { + let source_info = source_client.head_object(source).await?; + let args = CpArgs::single(source.to_string(), target.to_string()); + let ignore_progress = |_: u64| {}; + let result = perform_cross_alias_remote_copy( + source_client, + target_client, + source, + target, + &source_info, + encryption, + &ignore_progress, + &args, + ) + .await?; + Ok(CrossAliasCopyResult { + object: result.object, + source_version_id: result.source_version_id, + source_etag: result.source_etag, + }) +} + fn piped_copy_write_options( args: &CpArgs, source: &ObjectInfo, @@ -1463,20 +1511,27 @@ fn piped_copy_write_options( args.destination_customer_key.as_ref(), args.storage_class.clone(), )?; - if matches!( + let replace_metadata = matches!( requested_metadata_directive(args), Some(MetadataDirective::Replace) - ) { - return Ok(options); - } + ); let mut attributes = options.attributes.take().unwrap_or_default(); - if attributes.content_type.is_none() { - attributes.content_type = source.content_type.clone(); + if !replace_metadata { + if attributes.content_type.is_none() { + attributes.content_type = source.content_type.clone(); + } + if attributes.user_metadata.is_empty() + && let Some(metadata) = &source.metadata + { + attributes.user_metadata.clone_from(metadata); + } } - if attributes.user_metadata.is_empty() - && let Some(metadata) = &source.metadata - { - attributes.user_metadata.clone_from(metadata); + // The destination computes its own ETag, so record the source ETag the same + // way `rc mirror` does. Without this a later `mirror --compare auto` cannot + // tell a faithful cross-alias copy from a changed object and recopies it. + // This is `rc` bookkeeping rather than user data, so it survives --metadata-directive replace. + if let Some(source_etag) = source.etag.as_deref() { + set_source_identity(&mut attributes, source_etag); } if attributes != ObjectAttributes::default() { options.attributes = Some(attributes); @@ -1502,7 +1557,14 @@ fn multipart_options_from_source(source: &ObjectInfo) -> rc_core::Result, modified: Option, etag: Option, + /// Source ETag recorded by a previous `rc mirror` or cross-alias `rc cp`. + /// ListObjects never returns user metadata, so this is filled by HeadObject. + identity_etag: Option, + /// The object changed between LIST and the identity HEAD request. + snapshot_conflict: bool, } /// Execute the diff command @@ -153,17 +183,31 @@ pub async fn execute(args: DiffArgs, output_config: OutputConfig) -> ExitCode { } }; - let second_objects = match list_objects_map(&second_client, &second_path, args.recursive).await - { - Ok(o) => o, - Err(e) => { - formatter.error(&format!("Failed to list second path: {e}")); - return ExitCode::NetworkError; - } - }; + let mut second_objects = + match list_objects_map(&second_client, &second_path, args.recursive).await { + Ok(o) => o, + Err(e) => { + formatter.error(&format!("Failed to list second path: {e}")); + return ExitCode::NetworkError; + } + }; + + enrich_second_identity( + &second_client, + &second_path, + &first_objects, + &mut second_objects, + args.compare, + ) + .await; // Compare objects - let entries = compare_objects(&first_objects, &second_objects, args.diff_only); + let entries = compare_objects( + &first_objects, + &second_objects, + args.diff_only, + args.compare, + ); // Calculate summary let mut summary = DiffSummary { @@ -266,30 +310,26 @@ async fn list_objects_map( let relative_key = item.key.strip_prefix(base_prefix).unwrap_or(&item.key); let relative_key = relative_key.trim_start_matches('/').to_string(); - if relative_key.is_empty() { + let map_key = if relative_key.is_empty() { // Single object case - let filename = Path::new(&item.key) + Path::new(&item.key) .file_name() .map(|s| s.to_string_lossy().to_string()) - .unwrap_or(item.key.clone()); - objects.insert( - filename, - FileInfo { - size: item.size_bytes, - modified: item.last_modified.map(|t| t.to_string()), - etag: item.etag, - }, - ); + .unwrap_or_else(|| item.key.clone()) } else { - objects.insert( - relative_key, - FileInfo { - size: item.size_bytes, - modified: item.last_modified.map(|t| t.to_string()), - etag: item.etag, - }, - ); - } + relative_key + }; + objects.insert( + map_key, + FileInfo { + key: item.key, + size: item.size_bytes, + modified: item.last_modified.map(|t| t.to_string()), + etag: item.etag, + identity_etag: None, + snapshot_conflict: false, + }, + ); } if result.truncated { @@ -302,10 +342,112 @@ async fn list_objects_map( Ok(objects) } +/// Decide whether the second object already holds the first object's data. +/// +/// A client-streamed copy cannot preserve the source ETag, so `auto` also +/// accepts a recorded source identity. This is the same rule `rc mirror` uses to +/// skip a copy, which keeps `diff` from reporting a difference for a pair that +/// `mirror` considers synchronized. +fn objects_match(first: &FileInfo, second: &FileInfo, compare: CompareMode) -> bool { + if first.snapshot_conflict || second.snapshot_conflict { + return false; + } + let (Some(first_size), Some(second_size)) = (first.size, second.size) else { + return false; + }; + if first_size != second_size { + return false; + } + match compare { + CompareMode::Size => true, + CompareMode::Etag => first.etag.is_some() && first.etag == second.etag, + CompareMode::Auto => { + if first.etag.is_some() && first.etag == second.etag { + return true; + } + first + .etag + .as_ref() + .zip(second.identity_etag.as_ref()) + .is_some_and(|(first_etag, identity_etag)| first_etag == identity_etag) + } + } +} + +/// Whether HeadObject on the second entry could still prove the pair identical. +/// +/// Restricted to same-size pairs whose listed ETags differ, so an unchanged tree +/// costs no extra requests. +fn second_needs_identity_lookup(first: &FileInfo, second: &FileInfo, compare: CompareMode) -> bool { + if !matches!(compare, CompareMode::Auto) { + return false; + } + if first.size.is_none() || first.size != second.size { + return false; + } + let Some(first_etag) = first.etag.as_ref() else { + return false; + }; + if second.etag.as_ref() == Some(first_etag) { + return false; + } + second.identity_etag.is_none() +} + +/// Fill recorded source identities for entries that could still match. +/// +/// ListObjects omits user metadata, so the identity has to come from HeadObject. +/// A failed lookup leaves the entry unenriched and it is reported as different. +async fn enrich_second_identity( + client: &S3Client, + path: &RemotePath, + first: &HashMap, + second: &mut HashMap, + compare: CompareMode, +) { + let pending: Vec = second + .iter() + .filter(|(key, second_info)| { + first.get(*key).is_some_and(|first_info| { + second_needs_identity_lookup(first_info, second_info, compare) + }) + }) + .map(|(key, _)| key.clone()) + .collect(); + + for map_key in pending { + let Some(second_info) = second.get(&map_key) else { + continue; + }; + let object_path = RemotePath::new(&path.alias, &path.bucket, &second_info.key); + match client.head_object(&object_path).await { + Ok(info) => { + let listed_matches = second_snapshot_matches_head(second_info, &info); + if let Some(entry) = second.get_mut(&map_key) { + entry.snapshot_conflict = !listed_matches; + if listed_matches { + entry.identity_etag = identity_etag_from_metadata(info.metadata.as_ref()); + } + } + } + Err(_) => { + if let Some(entry) = second.get_mut(&map_key) { + entry.snapshot_conflict = true; + } + } + } + } +} + +fn second_snapshot_matches_head(listed: &FileInfo, head: &ObjectInfo) -> bool { + listed.size == head.size_bytes && listed.etag == head.etag +} + fn compare_objects( first: &HashMap, second: &HashMap, diff_only: bool, + compare: CompareMode, ) -> Vec { let mut entries = Vec::new(); @@ -313,13 +455,7 @@ fn compare_objects( for (key, first_info) in first { if let Some(second_info) = second.get(key) { // Object exists in both - let is_same = first_info.size == second_info.size - && matches!( - (&first_info.etag, &second_info.etag), - (Some(first_etag), Some(second_etag)) if first_etag == second_etag - ); - - let status = if is_same { + let status = if objects_match(first_info, second_info, compare) { DiffStatus::Same } else { DiffStatus::Different @@ -375,99 +511,86 @@ fn format_size(size: i64) -> String { mod tests { use super::*; + fn entry(size: i64, etag: Option<&str>) -> FileInfo { + FileInfo { + key: "prefix/file.txt".to_string(), + size: Some(size), + modified: None, + etag: etag.map(ToOwned::to_owned), + identity_etag: None, + snapshot_conflict: false, + } + } + + fn entry_with_identity(size: i64, etag: &str, identity_etag: &str) -> FileInfo { + FileInfo { + identity_etag: Some(identity_etag.to_string()), + ..entry(size, Some(etag)) + } + } + + fn one(key: &str, info: FileInfo) -> HashMap { + HashMap::from([(key.to_string(), info)]) + } + #[test] fn test_compare_objects_same() { - let mut first = HashMap::new(); - first.insert( - "file.txt".to_string(), - FileInfo { - size: Some(100), - modified: None, - etag: Some("abc123".to_string()), - }, - ); - - let mut second = HashMap::new(); - second.insert( - "file.txt".to_string(), - FileInfo { - size: Some(100), - modified: None, - etag: Some("abc123".to_string()), - }, - ); + let first = one("file.txt", entry(100, Some("abc123"))); + let second = one("file.txt", entry(100, Some("abc123"))); - let entries = compare_objects(&first, &second, false); + let entries = compare_objects(&first, &second, false, CompareMode::Auto); assert_eq!(entries.len(), 1); assert_eq!(entries[0].status, DiffStatus::Same); } #[test] fn test_compare_objects_different() { - let mut first = HashMap::new(); - first.insert( - "file.txt".to_string(), - FileInfo { - size: Some(100), - modified: None, - etag: Some("abc123".to_string()), - }, - ); - - let mut second = HashMap::new(); - second.insert( - "file.txt".to_string(), - FileInfo { - size: Some(200), - modified: None, - etag: Some("def456".to_string()), - }, - ); + let first = one("file.txt", entry(100, Some("abc123"))); + let second = one("file.txt", entry(200, Some("def456"))); - let entries = compare_objects(&first, &second, false); + let entries = compare_objects(&first, &second, false, CompareMode::Auto); assert_eq!(entries.len(), 1); assert_eq!(entries[0].status, DiffStatus::Different); } + #[test] + fn identity_head_must_match_the_listed_size_and_etag() { + let listed = entry(100, Some("listed-etag")); + let mut head = ObjectInfo::file("prefix/file.txt", 100); + head.etag = Some("listed-etag".to_string()); + assert!(second_snapshot_matches_head(&listed, &head)); + + head.size_bytes = Some(101); + assert!(!second_snapshot_matches_head(&listed, &head)); + head.size_bytes = Some(100); + head.etag = Some("changed-etag".to_string()); + assert!(!second_snapshot_matches_head(&listed, &head)); + + let mut conflicted = listed.clone(); + conflicted.snapshot_conflict = true; + assert!(!objects_match( + &entry(100, Some("listed-etag")), + &conflicted, + CompareMode::Auto + )); + } + #[test] fn test_compare_objects_missing_etag_is_different() { - let first = HashMap::from([( - "file.txt".to_string(), - FileInfo { - size: Some(100), - modified: None, - etag: None, - }, - )]); - let second = HashMap::from([( - "file.txt".to_string(), - FileInfo { - size: Some(100), - modified: None, - etag: Some("second-etag".to_string()), - }, - )]); - - let entries = compare_objects(&first, &second, false); + let first = one("file.txt", entry(100, None)); + let second = one("file.txt", entry(100, Some("second-etag"))); + + let entries = compare_objects(&first, &second, false, CompareMode::Auto); assert_eq!(entries[0].status, DiffStatus::Different); } #[test] fn test_compare_objects_only_first() { - let mut first = HashMap::new(); - first.insert( - "file.txt".to_string(), - FileInfo { - size: Some(100), - modified: None, - etag: None, - }, - ); - + let first = one("file.txt", entry(100, None)); let second = HashMap::new(); - let entries = compare_objects(&first, &second, false); + let entries = compare_objects(&first, &second, false, CompareMode::Auto); assert_eq!(entries.len(), 1); assert_eq!(entries[0].status, DiffStatus::OnlyFirst); } @@ -475,19 +598,180 @@ mod tests { #[test] fn test_compare_objects_only_second() { let first = HashMap::new(); + let second = one("file.txt", entry(100, None)); + + let entries = compare_objects(&first, &second, false, CompareMode::Auto); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].status, DiffStatus::OnlySecond); + } + + #[test] + fn auto_compare_treats_a_recorded_source_identity_as_same() { + let first = one("file.txt", entry(100, Some("source-etag"))); + let second = one( + "file.txt", + entry_with_identity(100, "multipart-etag-1", "source-etag"), + ); + + let entries = compare_objects(&first, &second, false, CompareMode::Auto); + + assert_eq!( + entries[0].status, + DiffStatus::Same, + "auto must agree with mirror --compare auto" + ); + } + + #[test] + fn etag_compare_ignores_a_recorded_source_identity() { + let first = one("file.txt", entry(100, Some("source-etag"))); + let second = one( + "file.txt", + entry_with_identity(100, "multipart-etag-1", "source-etag"), + ); + + let entries = compare_objects(&first, &second, false, CompareMode::Etag); + + assert_eq!(entries[0].status, DiffStatus::Different); + } + + #[test] + fn size_compare_ignores_etag_differences() { + let first = one("file.txt", entry(100, Some("source-etag"))); + let second = one("file.txt", entry(100, Some("other-etag"))); + + let entries = compare_objects(&first, &second, false, CompareMode::Size); + + assert_eq!(entries[0].status, DiffStatus::Same); + } + + #[test] + fn auto_compare_reports_a_mismatched_identity_as_different() { + let first = one("file.txt", entry(100, Some("source-etag"))); + let second = one( + "file.txt", + entry_with_identity(100, "multipart-etag-1", "other-etag"), + ); - let mut second = HashMap::new(); - second.insert( - "file.txt".to_string(), - FileInfo { - size: Some(100), - modified: None, - etag: None, - }, + let entries = compare_objects(&first, &second, false, CompareMode::Auto); + + assert_eq!(entries[0].status, DiffStatus::Different); + } + + #[test] + fn size_mismatch_is_different_in_every_compare_mode() { + let first = one("file.txt", entry(100, Some("source-etag"))); + let second = one( + "file.txt", + entry_with_identity(200, "source-etag", "source-etag"), + ); + + for compare in [CompareMode::Auto, CompareMode::Etag, CompareMode::Size] { + let entries = compare_objects(&first, &second, false, compare); + assert_eq!( + entries[0].status, + DiffStatus::Different, + "{compare:?} must not call different sizes the same" + ); + } + } + + #[test] + fn unknown_sizes_are_never_assumed_equal() { + let mut missing = entry(100, Some("source-etag")); + missing.size = None; + let first = one("file.txt", missing.clone()); + let second = one("file.txt", missing); + + for compare in [CompareMode::Auto, CompareMode::Etag, CompareMode::Size] { + let entries = compare_objects(&first, &second, false, compare); + assert_eq!( + entries[0].status, + DiffStatus::Different, + "{compare:?} must not assume equality without sizes" + ); + } + } + + #[test] + fn identity_lookup_is_limited_to_auto_same_size_etag_mismatches() { + let source = entry(100, Some("source-etag")); + let mismatched = entry(100, Some("other-etag")); + + assert!(second_needs_identity_lookup( + &source, + &mismatched, + CompareMode::Auto + )); + + assert!( + !second_needs_identity_lookup( + &source, + &entry(100, Some("source-etag")), + CompareMode::Auto + ), + "matching ETags already prove equality" + ); + assert!( + !second_needs_identity_lookup( + &source, + &entry_with_identity(100, "other-etag", "source-etag"), + CompareMode::Auto + ), + "an entry that already has an identity needs no lookup" + ); + assert!( + !second_needs_identity_lookup( + &source, + &entry(200, Some("other-etag")), + CompareMode::Auto + ), + "different sizes can never match" + ); + assert!(!second_needs_identity_lookup( + &source, + &mismatched, + CompareMode::Etag + )); + assert!(!second_needs_identity_lookup( + &source, + &mismatched, + CompareMode::Size + )); + + let mut unknown_size = source.clone(); + unknown_size.size = None; + assert!( + !second_needs_identity_lookup(&unknown_size, &mismatched, CompareMode::Auto), + "an unknown source size cannot be reconciled by metadata" ); - let entries = compare_objects(&first, &second, false); + let mut no_etag = source.clone(); + no_etag.etag = None; + assert!( + !second_needs_identity_lookup(&no_etag, &mismatched, CompareMode::Auto), + "without a source ETag there is nothing to match an identity against" + ); + } + + #[test] + fn diff_only_hides_matching_entries_in_auto_mode() { + let first = HashMap::from([ + ("same.txt".to_string(), entry(100, Some("source-etag"))), + ("changed.txt".to_string(), entry(100, Some("source-etag"))), + ]); + let second = HashMap::from([ + ( + "same.txt".to_string(), + entry_with_identity(100, "multipart-etag-1", "source-etag"), + ), + ("changed.txt".to_string(), entry(100, Some("other-etag"))), + ]); + + let entries = compare_objects(&first, &second, true, CompareMode::Auto); + assert_eq!(entries.len(), 1); - assert_eq!(entries[0].status, DiffStatus::OnlySecond); + assert_eq!(entries[0].key, "changed.txt"); + assert_eq!(entries[0].status, DiffStatus::Different); } } diff --git a/crates/cli/src/commands/mirror.rs b/crates/cli/src/commands/mirror.rs index bea53ba..88fb778 100644 --- a/crates/cli/src/commands/mirror.rs +++ b/crates/cli/src/commands/mirror.rs @@ -1,6 +1,6 @@ //! mirror command - Synchronize trees between local filesystems and S3-compatible storage. -use std::collections::{BTreeMap, HashMap}; +use std::collections::BTreeMap; use std::future::Future; use std::path::{Component, Path, PathBuf}; use std::sync::Arc; @@ -19,11 +19,10 @@ use serde::Serialize; use tokio::io::AsyncWriteExt as _; use super::cp::{parse_age_cutoff, parse_byte_rate}; +use super::object_identity::{identity_etag_from_metadata, set_source_identity}; use crate::exit_code::ExitCode; use crate::output::{Formatter, OutputConfig}; -const SOURCE_IDENTITY_METADATA_KEY: &str = "rc-source-etag"; - /// How `rc mirror` decides that a destination object already matches the source. #[derive(Copy, Clone, Debug, Default, Eq, PartialEq, ValueEnum)] pub enum CompareMode { @@ -282,19 +281,15 @@ impl MirrorRemoteTransfer for S3Client { condition: RemoteWriteCondition, identity_etag: Option<&str>, ) -> rc_core::Result { - let mut user_metadata = HashMap::new(); + let mut attributes = ObjectAttributes { + content_type: content_type.map(ToString::to_string), + ..ObjectAttributes::default() + }; if let Some(identity_etag) = identity_etag { - user_metadata.insert( - SOURCE_IDENTITY_METADATA_KEY.to_string(), - identity_etag.to_string(), - ); + set_source_identity(&mut attributes, identity_etag); } let options = ObjectWriteOptions { - attributes: Some(ObjectAttributes { - content_type: content_type.map(ToString::to_string), - user_metadata, - ..ObjectAttributes::default() - }), + attributes: Some(attributes), ..ObjectWriteOptions::default() }; match condition { @@ -1559,18 +1554,6 @@ fn snapshot_from_object(object: &ObjectInfo) -> rc_core::Result }) } -fn identity_etag_from_metadata(metadata: Option<&HashMap>) -> Option { - metadata.and_then(|metadata| { - metadata.iter().find_map(|(key, value)| { - let normalized = key.to_ascii_lowercase(); - let key = normalized - .strip_prefix("x-amz-meta-") - .unwrap_or(normalized.as_str()); - (key == SOURCE_IDENTITY_METADATA_KEY && !value.is_empty()).then(|| value.clone()) - }) - }) -} - fn destination_needs_identity_lookup( source: &MirrorEntry, target: &MirrorEntry, diff --git a/crates/cli/src/commands/mirror/roadmap_tests.rs b/crates/cli/src/commands/mirror/roadmap_tests.rs index 4f85e43..b90e782 100644 --- a/crates/cli/src/commands/mirror/roadmap_tests.rs +++ b/crates/cli/src/commands/mirror/roadmap_tests.rs @@ -847,6 +847,26 @@ async fn remote_source_change_after_download_blocks_upload_and_cleans_staging() assert!(!downloads[0].exists()); } +#[tokio::test] +async fn remote_source_preflight_runs_before_target_identity_check() { + let (mut source, operation) = remote_transfer_fixture(4, None); + source.info.etag = Some("replacement-etag".to_string()); + let MirrorLocation::Remote(source_path) = &operation.source.location else { + panic!("expected remote source") + }; + let target_checked = Arc::new(Mutex::new(false)); + let target_checked_for_check = Arc::clone(&target_checked); + + let result = source_preflight_then_target(&source, source_path, &operation, || async move { + *target_checked_for_check.lock().expect("target check lock") = true; + Ok::<(), Error>(()) + }) + .await; + + assert!(matches!(result, Err(Error::Conflict(_)))); + assert!(!*target_checked.lock().expect("target check lock")); +} + #[tokio::test] async fn truncated_remote_download_is_rejected_and_cleaned_before_upload() { let (mut source, operation) = remote_transfer_fixture(4, None); @@ -876,26 +896,6 @@ async fn truncated_remote_download_is_rejected_and_cleaned_before_upload() { assert!(!downloads[0].exists()); } -#[tokio::test] -async fn remote_source_preflight_runs_before_target_identity_check() { - let (mut source, operation) = remote_transfer_fixture(4, None); - source.info.etag = Some("replacement-etag".to_string()); - let MirrorLocation::Remote(source_path) = &operation.source.location else { - panic!("expected remote source") - }; - let target_checked = Arc::new(Mutex::new(false)); - let target_checked_for_check = Arc::clone(&target_checked); - - let result = source_preflight_then_target(&source, source_path, &operation, || async move { - *target_checked_for_check.lock().expect("target check lock") = true; - Ok::<(), Error>(()) - }) - .await; - - assert!(matches!(result, Err(Error::Conflict(_)))); - assert!(!*target_checked.lock().expect("target check lock")); -} - #[tokio::test] async fn cancelling_a_remote_transfer_drops_and_removes_its_staging_file() { let (mut source, operation) = remote_transfer_fixture(4, None); diff --git a/crates/cli/src/commands/mod.rs b/crates/cli/src/commands/mod.rs index e408f4c..23340db 100644 --- a/crates/cli/src/commands/mod.rs +++ b/crates/cli/src/commands/mod.rs @@ -45,6 +45,7 @@ mod mirror; mod multipart; mod mv; mod object; +mod object_identity; mod ops_output; mod ping; mod pipe; diff --git a/crates/cli/src/commands/mv.rs b/crates/cli/src/commands/mv.rs index 81100a0..24cccb5 100644 --- a/crates/cli/src/commands/mv.rs +++ b/crates/cli/src/commands/mv.rs @@ -3,11 +3,15 @@ //! Moves objects between locations (copy + delete). use clap::Args; -use rc_core::{AliasManager, ListOptions, ObjectStore as _, ParsedPath, RemotePath, parse_path}; +use rc_core::{ + AliasManager, CopyObjectOptions, DeleteRequestOptions, ListOptions, ObjectEncryptionRequest, + ObjectInfo, ObjectStore as _, ParsedPath, RemotePath, parse_path, +}; use rc_s3::S3Client; use serde::Serialize; use std::path::{Path, PathBuf}; +use super::cp; use crate::exit_code::ExitCode; use crate::output::{Formatter, OutputConfig}; @@ -366,6 +370,98 @@ async fn move_s3_prefix_to_local( } } +/// Copy one object for a move, picking the transfer that the alias pair allows. +/// +/// `target_client` is `Some` only for a cross-alias move, where server-side +/// CopyObject is not available and the object must stream through the client. +#[derive(Debug)] +struct MoveCopyResult { + object: ObjectInfo, + source_version_id: Option, + source_etag: Option, +} + +async fn copy_for_move( + source_client: &S3Client, + target_client: Option<&S3Client>, + source: &RemotePath, + target: &RemotePath, + encryption: Option<&ObjectEncryptionRequest>, +) -> rc_core::Result { + match target_client { + Some(target_client) => { + let result = cp::copy_object_across_aliases( + source_client, + target_client, + source, + target, + encryption, + ) + .await?; + Ok(MoveCopyResult { + object: result.object, + source_version_id: result.source_version_id, + source_etag: result.source_etag, + }) + } + None => { + let source_info = source_client.head_object(source).await?; + let copy_options = + CopyObjectOptions::for_source_version(source_info.version_id.clone())?; + let object = source_client + .copy_object_with_options(source, target, ©_options, encryption) + .await?; + Ok(MoveCopyResult { + object, + source_version_id: source_info.version_id, + source_etag: source_info.etag, + }) + } + } +} + +async fn delete_moved_source( + client: &S3Client, + source: &RemotePath, + copied: &MoveCopyResult, +) -> rc_core::Result<()> { + match move_delete_condition(copied)? { + MoveDeleteCondition::Version(version_id) => { + rc_core::ObjectStore::delete_object_with_options( + client, + source, + DeleteRequestOptions { + version_id: Some(version_id), + ..DeleteRequestOptions::default() + }, + ) + .await?; + Ok(()) + } + MoveDeleteCondition::Etag(etag) => client.delete_object_if_match(source, &etag).await, + } +} + +enum MoveDeleteCondition { + Version(String), + Etag(String), +} + +fn move_delete_condition(copied: &MoveCopyResult) -> rc_core::Result { + if let Some(version_id) = copied.source_version_id.clone() { + return Ok(MoveDeleteCondition::Version(version_id)); + } + copied + .source_etag + .clone() + .map(MoveDeleteCondition::Etag) + .ok_or_else(|| { + rc_core::Error::Conflict( + "Refusing to delete moved source because its ETag is unavailable".to_string(), + ) + }) +} + async fn move_s3_to_s3( src: &RemotePath, dst: &RemotePath, @@ -382,12 +478,6 @@ async fn move_s3_to_s3( Err(error) => return formatter.fail(ExitCode::UsageError, &error), }; - // For S3-to-S3, we need same alias for server-side copy - if src.alias != dst.alias { - formatter.error("Cross-alias S3-to-S3 move not yet supported."); - return ExitCode::UnsupportedFeature; - } - if args.recursive && remote_prefixes_overlap(src, dst) { formatter.error("Recursive move source and destination prefixes must not overlap."); return ExitCode::UsageError; @@ -417,6 +507,27 @@ async fn move_s3_to_s3( } }; + // A different alias means a different endpoint or credentials, so server-side + // CopyObject cannot be used. Build a destination client and stream through it. + let target_client = if src.alias == dst.alias { + None + } else { + let target_alias = match alias_manager.get(&dst.alias) { + Ok(a) => a, + Err(_) => { + formatter.error(&format!("Alias '{}' not found", dst.alias)); + return ExitCode::NotFound; + } + }; + match S3Client::new(target_alias).await { + Ok(c) => Some(c), + Err(e) => { + formatter.error(&format!("Failed to create destination S3 client: {e}")); + return ExitCode::NetworkError; + } + } + }; + let src_display = format!("{}/{}/{}", src.alias, src.bucket, src.key); let dst_display = format!("{}/{}/{}", dst.alias, dst.bucket, dst.key); @@ -506,11 +617,16 @@ async fn move_s3_to_s3( let src_obj_display = src_obj.to_string(); let dst_obj_display = dst_obj.to_string(); - match client - .copy_object(&src_obj, &dst_obj, encryption.as_ref()) - .await + match copy_for_move( + &client, + target_client.as_ref(), + &src_obj, + &dst_obj, + encryption.as_ref(), + ) + .await { - Ok(_) => match client.delete_object(&src_obj).await { + Ok(copied) => match delete_moved_source(&client, &src_obj, &copied).await { Ok(()) => { moved_count += 1; if !formatter.is_json() { @@ -575,10 +691,18 @@ async fn move_s3_to_s3( } } else { // Copy - match client.copy_object(src, dst, encryption.as_ref()).await { - Ok(info) => { + match copy_for_move( + &client, + target_client.as_ref(), + src, + dst, + encryption.as_ref(), + ) + .await + { + Ok(copied) => { // Delete source - if let Err(e) = client.delete_object(src).await { + if let Err(e) = delete_moved_source(&client, src, &copied).await { formatter.error(&format!("Copied but failed to delete source: {e}")); return ExitCode::GeneralError; } @@ -588,13 +712,13 @@ async fn move_s3_to_s3( status: "success", source: src_display, target: dst_display, - size_bytes: info.size_bytes, + size_bytes: copied.object.size_bytes, }; formatter.json(&output); } else { formatter.println(&format!( "{src_display} -> {dst_display} ({})", - info.size_human.unwrap_or_default() + copied.object.size_human.unwrap_or_default() )); } ExitCode::Success @@ -763,6 +887,39 @@ mod tests { assert!(!remote_prefixes_overlap(&source, &separate_target)); } + #[test] + fn moved_source_deletion_prefers_exact_version_then_etag_condition() { + let versioned = MoveCopyResult { + object: ObjectInfo::file("target", 1), + source_version_id: Some("source-v1".to_string()), + source_etag: Some("source-etag".to_string()), + }; + assert!(matches!( + move_delete_condition(&versioned), + Ok(MoveDeleteCondition::Version(version)) if version == "source-v1" + )); + + let unversioned = MoveCopyResult { + object: ObjectInfo::file("target", 1), + source_version_id: None, + source_etag: Some("source-etag".to_string()), + }; + assert!(matches!( + move_delete_condition(&unversioned), + Ok(MoveDeleteCondition::Etag(etag)) if etag == "source-etag" + )); + + let without_identity = MoveCopyResult { + object: ObjectInfo::file("target", 1), + source_version_id: None, + source_etag: None, + }; + assert!(matches!( + move_delete_condition(&without_identity), + Err(rc_core::Error::Conflict(_)) + )); + } + #[test] fn test_mv_output_serialization() { let output = MvOutput { diff --git a/crates/cli/src/commands/object_identity.rs b/crates/cli/src/commands/object_identity.rs new file mode 100644 index 0000000..27368c9 --- /dev/null +++ b/crates/cli/src/commands/object_identity.rs @@ -0,0 +1,140 @@ +//! Shared source-identity metadata for copy-style commands. +//! +//! A remote-to-remote transfer that streams through the client cannot preserve +//! the source ETag: the destination computes its own, and multipart completion +//! makes it differ even when the bytes are identical. Commands therefore record +//! the source ETag in user metadata so a later run can recognize an unchanged +//! object. `mirror`, `cp`, and `diff` must agree on this key and on how it is +//! read back, otherwise one command re-copies what another already migrated. + +use std::collections::HashMap; + +use rc_core::ObjectAttributes; + +/// User-metadata key holding the source ETag of a client-streamed copy. +/// +/// Stored without the `x-amz-meta-` prefix; the S3 layer adds it on the wire. +pub(super) const SOURCE_IDENTITY_METADATA_KEY: &str = "rc-source-etag"; + +/// Read the recorded source ETag from object user metadata. +/// +/// Backends differ on whether they echo the `x-amz-meta-` prefix and on header +/// case, so both are normalized. An empty value is treated as absent because it +/// cannot identify a source object. +pub(super) fn identity_etag_from_metadata( + metadata: Option<&HashMap>, +) -> Option { + metadata.and_then(|metadata| { + metadata.iter().find_map(|(key, value)| { + let normalized = key.to_ascii_lowercase(); + let key = normalized + .strip_prefix("x-amz-meta-") + .unwrap_or(normalized.as_str()); + (key == SOURCE_IDENTITY_METADATA_KEY && !value.is_empty()).then(|| value.clone()) + }) + }) +} + +/// Record `identity_etag` on a destination write. +/// +/// This is bookkeeping owned by `rc` rather than user data, so it is applied +/// even when the caller replaces user metadata. Without it, a later incremental +/// run could not tell a faithful copy from a changed object. +pub(super) fn set_source_identity(attributes: &mut ObjectAttributes, identity_etag: &str) { + if identity_etag.is_empty() { + return; + } + attributes.user_metadata.insert( + SOURCE_IDENTITY_METADATA_KEY.to_string(), + identity_etag.to_string(), + ); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn identity_etag_is_read_through_prefix_and_case_variants() { + for key in [ + "rc-source-etag", + "Rc-Source-Etag", + "x-amz-meta-rc-source-etag", + "X-Amz-Meta-Rc-Source-Etag", + ] { + let metadata = HashMap::from([(key.to_string(), "source-etag".to_string())]); + assert_eq!( + identity_etag_from_metadata(Some(&metadata)).as_deref(), + Some("source-etag"), + "{key} should resolve the identity ETag" + ); + } + } + + #[test] + fn absent_empty_and_unrelated_metadata_have_no_identity() { + assert_eq!(identity_etag_from_metadata(None), None); + assert_eq!( + identity_etag_from_metadata(Some(&HashMap::new())), + None, + "empty metadata has no identity" + ); + assert_eq!( + identity_etag_from_metadata(Some(&HashMap::from([( + "rc-source-etag".to_string(), + String::new(), + )]))), + None, + "an empty value cannot identify a source object" + ); + assert_eq!( + identity_etag_from_metadata(Some(&HashMap::from([( + "owner".to_string(), + "storage".to_string(), + )]))), + None + ); + } + + #[test] + fn set_source_identity_records_the_key_without_the_wire_prefix() { + let mut attributes = ObjectAttributes::default(); + set_source_identity(&mut attributes, "source-etag"); + + assert_eq!( + attributes.user_metadata.get(SOURCE_IDENTITY_METADATA_KEY), + Some(&"source-etag".to_string()) + ); + assert_eq!( + identity_etag_from_metadata(Some(&attributes.user_metadata)).as_deref(), + Some("source-etag"), + "what one command writes another must be able to read" + ); + } + + #[test] + fn set_source_identity_preserves_unrelated_user_metadata() { + let mut attributes = ObjectAttributes { + user_metadata: HashMap::from([("owner".to_string(), "storage".to_string())]), + ..ObjectAttributes::default() + }; + set_source_identity(&mut attributes, "source-etag"); + + assert_eq!( + attributes.user_metadata.get("owner"), + Some(&"storage".to_string()) + ); + assert_eq!(attributes.user_metadata.len(), 2); + } + + #[test] + fn set_source_identity_ignores_an_empty_etag() { + let mut attributes = ObjectAttributes::default(); + set_source_identity(&mut attributes, ""); + + assert!( + attributes.user_metadata.is_empty(), + "an empty ETag must not create an unreadable identity entry" + ); + } +} diff --git a/crates/cli/tests/recursive_remote_copy.rs b/crates/cli/tests/recursive_remote_copy.rs index 5957a2f..96a25d8 100644 --- a/crates/cli/tests/recursive_remote_copy.rs +++ b/crates/cli/tests/recursive_remote_copy.rs @@ -390,6 +390,494 @@ fn missing_object() -> Response { } } +/// A ListObjects page for an arbitrary bucket, prefix, and ETag. +/// +/// `list_result` hardcodes the source bucket, which cannot describe the +/// destination side of a `diff`. +fn bucket_list_result(bucket: &str, prefix: &str, key: &str, size: u64, etag: &str) -> Response { + Response::xml(format!( + "\ + \ + {bucket}{prefix}\ + {key}{size}"{etag}"\ + false" + )) +} + +fn is_bucket_list_request(request: &Request, bucket: &str) -> bool { + request.method == "GET" + && (request.target.starts_with(&format!("/{bucket}?")) + || request.target.starts_with(&format!("/{bucket}/?"))) + && request.target.contains("list-type=2") +} + +/// HeadObject reply carrying the source-identity metadata a client copy records. +fn head_with_identity(size: usize, etag: &str, identity_etag: &str) -> Response { + Response { + status: "200 OK", + headers: vec![ + ("content-length", size.to_string()), + ("etag", format!("\"{etag}\"")), + ("x-amz-meta-rc-source-etag", identity_etag.to_string()), + ], + body: String::new(), + } +} + +#[test] +fn cross_alias_copy_records_the_source_etag_for_later_incremental_runs() { + let mock = S3Mock::start(|request| { + if request.method == "HEAD" && request.target.starts_with("/source/a.txt") { + return Response::head_with_etag(5, "source-etag"); + } + if request.method == "GET" && request.target.starts_with("/source/a.txt") { + return object_get_result("hello"); + } + if is_upload_request(request) && request.target.starts_with("/destination/b.txt") { + return Response { + status: "200 OK", + headers: vec![("etag", "\"multipart-etag-1\"".to_string())], + body: String::new(), + }; + } + if request.method == "HEAD" && request.target.starts_with("/destination/") { + return missing_object(); + } + Response { + status: "500 Internal Server Error", + headers: Vec::new(), + body: "UnexpectedRequest".to_string(), + } + }); + + let output = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &[ + "cp", + "--overwrite=true", + "alpha/source/a.txt", + "beta/destination/b.txt", + ], + ); + + assert!( + output.status.success(), + "stdout: {}\nstderr: {}\nrequests: {:#?}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + mock.requests() + ); + let requests = mock.requests(); + let upload = requests + .iter() + .find(|request| is_upload_request(request)) + .expect("cross-alias copy should upload"); + assert_eq!( + upload + .headers + .get("x-amz-meta-rc-source-etag") + .map(String::as_str), + Some("source-etag"), + "cp must record the source ETag so mirror --compare auto can skip it: {requests:#?}" + ); +} + +#[test] +fn cross_alias_move_uploads_then_deletes_the_source() { + let mock = S3Mock::start(|request| { + if request.method == "HEAD" && request.target.starts_with("/source/a.txt") { + return Response::head_with_etag(5, "source-etag"); + } + if request.method == "GET" && request.target.starts_with("/source/a.txt") { + return object_get_result("hello"); + } + if is_upload_request(request) && request.target.starts_with("/destination/b.txt") { + return Response { + status: "200 OK", + headers: vec![("etag", "\"multipart-etag-1\"".to_string())], + body: String::new(), + }; + } + if request.method == "DELETE" && request.target.starts_with("/source/a.txt") { + return Response { + status: "204 No Content", + headers: vec![("content-length", "0".to_string())], + body: String::new(), + }; + } + if request.method == "HEAD" && request.target.starts_with("/destination/") { + return missing_object(); + } + Response { + status: "500 Internal Server Error", + headers: Vec::new(), + body: "UnexpectedRequest".to_string(), + } + }); + + let output = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &["mv", "alpha/source/a.txt", "beta/destination/b.txt"], + ); + + assert!( + output.status.success(), + "stdout: {}\nstderr: {}\nrequests: {:#?}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + mock.requests() + ); + let requests = mock.requests(); + assert!( + requests.iter().any(|request| { + request.method == "GET" && request.target.starts_with("/source/a.txt") + }), + "cross-alias move should download the source: {requests:#?}" + ); + assert!( + requests.iter().any(is_upload_request), + "cross-alias move should upload to the destination: {requests:#?}" + ); + assert!( + requests.iter().all(|request| !is_copy_request(request)), + "cross-alias move must not use CopyObject: {requests:#?}" + ); + assert!( + requests.iter().any(|request| { + request.method == "DELETE" && request.target.starts_with("/source/a.txt") + }), + "a move must delete the source after a successful copy: {requests:#?}" + ); + let delete = requests + .iter() + .find(|request| request.method == "DELETE" && request.target.starts_with("/source/a.txt")) + .expect("cross-alias move should issue one conditional source delete"); + assert_eq!( + delete.headers.get("if-match").map(String::as_str), + Some("source-etag"), + "move must delete only the source version that was copied: {requests:#?}" + ); +} + +#[test] +fn cross_alias_move_keeps_the_source_when_conditional_delete_fails() { + let mock = S3Mock::start(|request| { + if request.method == "HEAD" && request.target.starts_with("/source/a.txt") { + return Response::head_with_etag(5, "source-etag"); + } + if request.method == "GET" && request.target.starts_with("/source/a.txt") { + return object_get_result("hello"); + } + if is_upload_request(request) && request.target.starts_with("/destination/b.txt") { + return Response { + status: "200 OK", + headers: vec![("etag", "\"multipart-etag-1\"".to_string())], + body: String::new(), + }; + } + if request.method == "DELETE" && request.target.starts_with("/source/a.txt") { + return Response { + status: "412 Precondition Failed", + headers: Vec::new(), + body: "PreconditionFailed".to_string(), + }; + } + if request.method == "HEAD" && request.target.starts_with("/destination/") { + return missing_object(); + } + Response { + status: "500 Internal Server Error", + headers: Vec::new(), + body: "UnexpectedRequest".to_string(), + } + }); + + let output = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &["mv", "alpha/source/a.txt", "beta/destination/b.txt"], + ); + + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("Copied but failed to delete source"), + "stderr: {stderr}\nrequests: {:?}", + mock.requests() + ); + let delete = mock + .requests() + .into_iter() + .find(|request| request.method == "DELETE") + .expect("move should attempt the conditional delete"); + assert_eq!( + delete.headers.get("if-match").map(String::as_str), + Some("source-etag") + ); +} + +#[test] +fn cross_alias_move_keeps_the_source_when_the_upload_fails() { + let mock = S3Mock::start(|request| { + if request.method == "HEAD" && request.target.starts_with("/source/a.txt") { + return Response::head_with_etag(5, "source-etag"); + } + if request.method == "GET" && request.target.starts_with("/source/a.txt") { + return object_get_result("hello"); + } + if is_upload_request(request) { + return Response::access_denied(); + } + if request.method == "HEAD" && request.target.starts_with("/destination/") { + return missing_object(); + } + Response { + status: "500 Internal Server Error", + headers: Vec::new(), + body: "UnexpectedRequest".to_string(), + } + }); + + let output = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &["mv", "alpha/source/a.txt", "beta/destination/b.txt"], + ); + + assert!( + !output.status.success(), + "a failed upload must not report success: {:#?}", + mock.requests() + ); + let requests = mock.requests(); + assert!( + requests.iter().any(|request| { + request.method == "GET" && request.target.starts_with("/source/a.txt") + }), + "the move should have taken the cross-alias download path: {requests:#?}" + ); + assert!( + requests.iter().all(|request| request.method != "DELETE"), + "a failed cross-alias move must never delete the source: {requests:#?}" + ); +} + +/// The migration path this change exists for: a first pass with `cp --recursive` +/// across aliases, then incremental `mirror --compare auto`. The second command +/// must recognize what the first wrote, or the whole tree is copied twice. +#[test] +fn mirror_auto_compare_skips_objects_a_cross_alias_copy_already_migrated() { + let recorded_identity: Arc>> = Arc::new(Mutex::new(None)); + let mock = { + let recorded_identity = Arc::clone(&recorded_identity); + S3Mock::start(move |request| { + if is_bucket_list_request(request, "source") { + return bucket_list_result("source", "src/", "src/a.txt", 5, "source-etag"); + } + if is_bucket_list_request(request, "destination") { + // The destination computed its own ETag during the upload. + return bucket_list_result( + "destination", + "dst/", + "dst/a.txt", + 5, + "multipart-etag-1", + ); + } + if request.method == "HEAD" && request.target.starts_with("/source/src/a.txt") { + return Response::head_with_etag(5, "source-etag"); + } + if request.method == "GET" && request.target.starts_with("/source/src/a.txt") { + return object_get_result("hello"); + } + if is_upload_request(request) && request.target.starts_with("/destination/dst/a.txt") { + *recorded_identity.lock().expect("record identity") = + request.headers.get("x-amz-meta-rc-source-etag").cloned(); + return Response { + status: "200 OK", + headers: vec![("etag", "\"multipart-etag-1\"".to_string())], + body: String::new(), + }; + } + if request.method == "HEAD" && request.target.starts_with("/destination/dst/a.txt") { + // Serve back whatever the upload persisted, as a real backend would. + return match recorded_identity.lock().expect("read identity").as_deref() { + Some(identity) => head_with_identity(5, "multipart-etag-1", identity), + None => Response::head_with_etag(5, "multipart-etag-1"), + }; + } + if request.method == "HEAD" && request.target.starts_with("/destination/") { + return missing_object(); + } + Response { + status: "500 Internal Server Error", + headers: Vec::new(), + body: "UnexpectedRequest".to_string(), + } + }) + }; + + let copy = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &[ + "cp", + "--recursive", + "--overwrite=true", + "--concurrency", + "1", + "alpha/source/src/", + "beta/destination/dst/", + ], + ); + assert!( + copy.status.success(), + "cross-alias copy failed\nstdout: {}\nstderr: {}\nrequests: {:#?}", + String::from_utf8_lossy(©.stdout), + String::from_utf8_lossy(©.stderr), + mock.requests() + ); + assert_eq!( + recorded_identity + .lock() + .expect("identity recorded") + .as_deref(), + Some("source-etag"), + "the copy must persist the source ETag: {:#?}", + mock.requests() + ); + + let mirror = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &[ + "--json", + "mirror", + "alpha/source/src/", + "beta/destination/dst/", + "--overwrite", + "--dry-run", + "--compare", + "auto", + ], + ); + assert!( + mirror.status.success(), + "mirror failed\nstdout: {}\nstderr: {}\nrequests: {:#?}", + String::from_utf8_lossy(&mirror.stdout), + String::from_utf8_lossy(&mirror.stderr), + mock.requests() + ); + let payload: serde_json::Value = + serde_json::from_slice(&mirror.stdout).expect("mirror JSON output"); + assert_eq!( + payload["copied"], 0, + "mirror must not recopy what cp already migrated: {payload}" + ); + assert_eq!(payload["skipped"], 1, "{payload}"); +} + +#[test] +fn diff_auto_compare_treats_a_recorded_source_identity_as_same() { + let mock = S3Mock::start(|request| { + if is_bucket_list_request(request, "source") { + return bucket_list_result("source", "src/", "src/a.txt", 1, "source-etag"); + } + if is_bucket_list_request(request, "destination") { + return bucket_list_result("destination", "dst/", "dst/a.txt", 1, "multipart-etag-1"); + } + if request.method == "HEAD" && request.target.starts_with("/destination/dst/a.txt") { + return head_with_identity(1, "multipart-etag-1", "source-etag"); + } + Response { + status: "500 Internal Server Error", + headers: Vec::new(), + body: "UnexpectedRequest".to_string(), + } + }); + + let output = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &[ + "diff", + "--recursive", + "alpha/source/src/", + "beta/destination/dst/", + ], + ); + + assert!( + output.status.success(), + "auto compare should report no differences\nstdout: {}\nstderr: {}\nrequests: {:#?}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + mock.requests() + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("1 same, 0 different"), + "expected one matching object: {stdout}" + ); + assert!( + mock.requests() + .iter() + .any(|request| request.method == "HEAD"), + "auto compare must HeadObject to read the recorded identity: {:#?}", + mock.requests() + ); +} + +#[test] +fn diff_etag_compare_ignores_the_recorded_identity_without_head() { + let mock = S3Mock::start(|request| { + if is_bucket_list_request(request, "source") { + return bucket_list_result("source", "src/", "src/a.txt", 1, "source-etag"); + } + if is_bucket_list_request(request, "destination") { + return bucket_list_result("destination", "dst/", "dst/a.txt", 1, "multipart-etag-1"); + } + Response { + status: "500 Internal Server Error", + headers: Vec::new(), + body: "UnexpectedRequest".to_string(), + } + }); + + let output = run_rc_with_hosts( + &mock, + &[("alpha", ""), ("beta", "")], + &[ + "diff", + "--recursive", + "--compare", + "etag", + "alpha/source/src/", + "beta/destination/dst/", + ], + ); + + assert!( + !output.status.success(), + "etag compare should still report a difference: {:#?}", + mock.requests() + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("0 same, 1 different"), + "expected one differing object: {stdout}" + ); + assert!( + mock.requests() + .iter() + .all(|request| request.method != "HEAD"), + "etag compare must not spend HeadObject requests: {:#?}", + mock.requests() + ); +} + #[test] fn single_copy_and_pipe_send_supported_storage_classes() { let mock = S3Mock::start(|request| { @@ -1679,7 +2167,7 @@ fn cross_alias_copy_downloads_then_uploads_without_copy_source() { } #[test] -fn cross_alias_copy_allows_metadata_replace() { +fn cross_alias_copy_allows_metadata_replace_and_records_source_identity() { let mock = S3Mock::start(|request| { if request.method == "HEAD" && request.target == "/source/a.txt" { return Response::head_with_etag(5, "source-etag"); @@ -1737,6 +2225,10 @@ fn cross_alias_copy_allows_metadata_replace() { upload.headers.get("x-amz-meta-owner"), Some(&"analytics".to_string()) ); + assert_eq!( + upload.headers.get("x-amz-meta-rc-source-etag"), + Some(&"source-etag".to_string()) + ); assert!( requests.iter().all(|request| !is_copy_request(request)), "cross-alias replace must not use CopyObject: {requests:#?}" diff --git a/crates/s3/src/client.rs b/crates/s3/src/client.rs index d5dd1ee..8d692ff 100644 --- a/crates/s3/src/client.rs +++ b/crates/s3/src/client.rs @@ -5677,7 +5677,8 @@ impl ObjectStore for S3Client { let plan = multipart .plan() .map_err(|error| self.redact_sensitive_error(error))?; - let attributes = if matches!(transfer.metadata_directive, Some(MetadataDirective::Copy)) { + let mut attributes = if matches!(transfer.metadata_directive, Some(MetadataDirective::Copy)) + { let mut source_options = transfer.source.clone(); if source_options.version_id.is_none() { source_options.version_id = multipart.source_version_id.clone(); @@ -5692,6 +5693,15 @@ impl ObjectStore for S3Client { ..ObjectAttributes::default() } }; + // Multipart callers may carry the rc source identity, which is + // bookkeeping rather than source metadata and is not exposed by HEAD. + // Preserve that identity even when the user requested metadata COPY, + // without overriding user metadata returned by the source. + if let Some(identity) = multipart.metadata.get("rc-source-etag") { + attributes + .user_metadata + .insert("rc-source-etag".to_string(), identity.clone()); + } if cancellation.is_cancelled() { return Err(self.redact_sensitive_error(Error::Interrupted( "Multipart copy cancelled after metadata preflight".to_string(), @@ -11894,7 +11904,10 @@ mod tests { ]); let src = RemotePath::new("test", "source-bucket", "src.bin"); let dst = RemotePath::new("test", "destination-bucket", "dst.bin"); - let multipart = multipart_copy_options(S3_MULTIPART_COPY_MIN_PART_SIZE); + let mut multipart = multipart_copy_options(S3_MULTIPART_COPY_MIN_PART_SIZE); + multipart + .metadata + .insert("rc-source-etag".to_string(), "source-etag".to_string()); let transfer = TransferCopyOptions { metadata_directive: Some(MetadataDirective::Copy), ..TransferCopyOptions::default() @@ -11931,6 +11944,10 @@ mod tests { assert_eq!(create.headers().get("content-language"), Some("en")); assert!(create.headers().get("expires").is_some()); assert_eq!(create.headers().get("x-amz-meta-owner"), Some("source")); + assert_eq!( + create.headers().get("x-amz-meta-rc-source-etag"), + Some("source-etag") + ); } #[tokio::test] diff --git a/docs/reference/rc/cp.md b/docs/reference/rc/cp.md index da2b07a..c511c10 100644 --- a/docs/reference/rc/cp.md +++ b/docs/reference/rc/cp.md @@ -113,9 +113,11 @@ Recursive downloads map object keys onto the local filesystem using `/` separato `--portable-names` is additive. Unix destinations no longer apply Windows filename rules by default, so keys containing `:` are accepted. Remote-to-remote copies keep the original key. This PR must be marked `BREAKING` because `docs/reference/rc/cp.md` is a protected CLI behavior contract. No JSON schema or config `schema_version` bump applies. +Cross-alias copies record the source ETag in the destination user metadata `x-amz-meta-rc-source-etag`, the same key `rc mirror` writes. The destination computes its own ETag during the upload, so without this record a later `rc mirror --compare auto` or `rc diff --compare auto` could not tell a faithful copy from a changed object and would copy the tree a second time. This entry is `rc` bookkeeping rather than user data, so it is written even with `--metadata-directive replace`. Same-alias copies use server-side CopyObject, which preserves the ETag, and do not need it. + ### BREAKING cross-alias copy contract migration -Cross-alias `rc cp` is additive and does not change same-alias CopyObject behavior. Destinations on a different alias are copied by download then upload instead of failing as `unsupported_feature`. Source content type and user metadata are preserved unless `--metadata-directive replace` is set. This PR must be marked `BREAKING` because `docs/reference/rc/cp.md` is a protected CLI behavior contract. No JSON schema or config `schema_version` bump applies. +Cross-alias `rc cp` is additive and does not change same-alias CopyObject behavior. Destinations on a different alias are copied by download then upload instead of failing as `unsupported_feature`. Source content type and user metadata are preserved unless `--metadata-directive replace` is set. Cross-alias uploads additionally record `x-amz-meta-rc-source-etag` so incremental `rc mirror` and `rc diff` runs recognize the copy. This PR must be marked `BREAKING` because `docs/reference/rc/cp.md` is a protected CLI behavior contract. No JSON schema or config `schema_version` bump applies. Global options shown in command syntax use the same meaning everywhere: diff --git a/docs/reference/rc/diff.md b/docs/reference/rc/diff.md index 7c8b6bd..869f315 100644 --- a/docs/reference/rc/diff.md +++ b/docs/reference/rc/diff.md @@ -18,18 +18,38 @@ rc [GLOBAL OPTIONS] diff [OPTIONS] | `TARGET` | Local or remote target path. | | `-r, --recursive` | Compare recursively. | | `--diff-only` | Show only differences instead of all compared entries. | +| `--compare ` | Choose how two objects are judged identical. Defaults to `auto`. | ## Examples ```bash rc diff local/reports backup/reports --recursive rc diff ./reports local/reports --recursive --json +rc diff stage/data/ prod/data/ --recursive --compare etag ``` ## Behavior Diff is read-only. Use it before copy, mirror, or remove workflows to inspect drift between two locations. +### Comparison rules + +`--compare` selects when two objects are reported as identical. Sizes must always match; an entry whose size is unknown on either side is always reported as different. + +| Mode | Same when | +| --- | --- | +| `auto` (default) | ETags match, or sizes match and the target records the source ETag in `x-amz-meta-rc-source-etag` from a previous `rc mirror` or cross-alias `rc cp`. | +| `etag` | Both ETags are present and identical. | +| `size` | Object sizes match, ignoring ETag differences. | + +These are the same rules as `rc mirror --compare`, so the two commands cannot disagree about whether a pair of objects is already synchronized. A remote-to-remote copy that streams through the client cannot preserve the source ETag, so comparing listed ETags alone would report a difference for data that is byte-identical. + +ListObjects does not return user metadata, so `auto` issues HeadObject only for same-size targets whose listed ETags differ. An unchanged tree costs no extra requests, and `etag` and `size` never issue HeadObject. + +### BREAKING comparison contract migration + +`rc diff` previously required both ETags to be present and identical, which is the current `--compare etag` behavior. The default is now `auto`, so a target that records the source ETag is reported as `Same` instead of `Different`, and the command exits `0` where it previously exited `1`. Sizes that are unknown on either side are now always reported as different rather than being treated as equal. Pass `--compare etag` to keep the previous comparison. This PR must be marked `BREAKING` because `docs/reference/rc/diff.md` is a protected CLI behavior contract. No JSON schema or config `schema_version` bump applies. + Global options shown in command syntax use the same meaning everywhere: | Option | Description | diff --git a/docs/reference/rc/mirror.md b/docs/reference/rc/mirror.md index c225b0f..74c42bf 100644 --- a/docs/reference/rc/mirror.md +++ b/docs/reference/rc/mirror.md @@ -64,6 +64,8 @@ Entries are compared by size and the strongest stable metadata available. `--com Remote-to-remote copies download through a temporary file and upload to the destination. Multipart completion often stores a different ETag than the source, so a second `auto` run would recopy every object if it compared ListObjects ETags alone. `rc mirror` therefore writes `x-amz-meta-rc-source-etag` on remote uploads that have a source ETag. ListObjects does not return user metadata, so `auto` issues HeadObject for same-size destinations whose listed ETags differ. +Cross-alias `rc cp` writes the same metadata key, and `rc diff --compare auto` reads it. A tree migrated with `rc cp --recursive` across aliases is therefore recognized by a later incremental `rc mirror` run instead of being copied again. + Downloads preserve the source modification time, and local-to-remote restart checks accept a same-size destination written no earlier than the source. A completed entry is skipped on a restarted command. `--overwrite` authorizes replacing a changed destination; it does not disable concurrency checks. Mirror revalidates sources and compares local destination metadata again before persistence, so changes observed by those checks fail with the conflict exit code. New remote objects use `If-None-Match: *`, while existing remote objects and remote removals use the planned ETag as a condition; these service-side conditions also reject remote races after the final client-side check. Local replacement is atomic but is not a filesystem compare-and-swap, so a local writer racing after the final metadata check may be replaced. diff --git a/docs/reference/rc/mv.md b/docs/reference/rc/mv.md index 476a162..cb85b0d 100644 --- a/docs/reference/rc/mv.md +++ b/docs/reference/rc/mv.md @@ -15,7 +15,7 @@ rc [GLOBAL OPTIONS] mv [OPTIONS] | Parameter | Description | | --- | --- | | `SOURCE` | Local path or remote object/prefix path to move. | -| `TARGET` | Local or remote destination path. | +| `TARGET` | Local or remote destination path. Remote destinations may use a different alias than the source. | | `-r, --recursive` | Move a directory or prefix recursively. | | `--dry-run` | Show planned changes without moving data. | | `--continue-on-error` | Continue recursive moves after per-object failures. | @@ -31,9 +31,22 @@ rc mv local/inbox/a.txt local/archive/a.txt --enc-s3 local/archive/a.txt rc mv local/inbox/ local/archive/ --recursive --enc-kms local/archive/=alias/archive-key ``` +Move between aliases: + +```bash +rc mv stage/data/report.json prod/archive/report.json +rc mv stage/data/ prod/archive/ --recursive +``` + ## Behavior -Move operations copy data to the target and remove the source after a successful copy. Review recursive moves with `--dry-run` before running destructive operations. +Move operations copy data to the target and remove the source after a successful copy. The source is removed only after its own copy succeeds, so a failed or partial move never destroys data that did not arrive. Review recursive moves with `--dry-run` before running destructive operations. + +Same-alias moves between remote paths use server-side CopyObject. Moves between different aliases cannot use CopyObject, so they stream through the client exactly like a cross-alias `rc cp`, then delete the source. Cross-alias moves therefore inherit the copy path's constraints: `SSE-C` is rejected, and `--storage-class` is rejected for objects large enough to require a multipart upload. + +### BREAKING cross-alias move contract migration + +Remote-to-remote moves between different aliases previously failed with the unsupported-feature exit code. They now succeed by copying through the client and deleting the source. Automation that relied on the old rejection to detect an unsupported operation must be updated. Same-alias moves are unchanged. This PR must be marked `BREAKING` because `docs/reference/rc/mv.md` is a protected CLI behavior contract. No JSON schema or config `schema_version` bump applies. Destination encryption flags apply only to remote writes. On `rc mv`, the selector in `--enc-s3` or `--enc-kms` must match the command destination exactly: diff --git a/scripts/regression/cross-alias-identity.sh b/scripts/regression/cross-alias-identity.sh new file mode 100755 index 0000000..82d9b00 --- /dev/null +++ b/scripts/regression/cross-alias-identity.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# +# Regression tests for shared source-identity semantics across commands. +# +# Usage: +# ./scripts/regression/cross-alias-identity.sh +# +# These tests do not require a running S3 backend. They cover: +# - The shared x-amz-meta-rc-source-etag helper read/write contract +# - Cross-alias cp recording the source ETag (including --metadata-directive replace) +# - Cross-alias mv streaming through the client and deleting the source only on success +# - rc diff --compare auto|etag|size agreeing with rc mirror --compare +# - The migration path: cp --recursive across aliases, then mirror --compare auto skips +# + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" +cd "$PROJECT_ROOT" + +RED='\033[0;31m' +GREEN='\033[0;32m' +BLUE='\033[0;34m' +NC='\033[0m' + +log_info() { echo -e "${BLUE}[INFO]${NC} $*"; } +log_success() { echo -e "${GREEN}[PASS]${NC} $*"; } +log_error() { echo -e "${RED}[FAIL]${NC} $*"; } + +run_tests() { + local description="$1" + shift + log_info "$description" + if "$@"; then + log_success "$description" + else + log_error "$description" + return 1 + fi +} + +log_info "Cross-alias identity regression suite" + +run_tests "shared source-identity helper unit tests" \ + cargo test -p rustfs-cli --lib -- commands::object_identity:: + +run_tests "cp identity metadata unit tests" \ + cargo test -p rustfs-cli --lib -- \ + piped_copy_records_the_source_etag_as_identity_metadata \ + piped_copy_records_identity_even_when_metadata_is_replaced \ + piped_copy_omits_identity_when_the_source_has_no_etag \ + piped_copy_preserves_source_content_type_unless_replaced \ + piped_copy_preserves_source_user_metadata_unless_replaced + +run_tests "diff compare-mode unit tests" \ + cargo test -p rustfs-cli --lib -- commands::diff:: + +run_tests "mirror identity unit tests still hold" \ + cargo test -p rustfs-cli --lib -- \ + auto_compare_skips_when_source_etag_is_preserved_in_destination_metadata \ + auto_compare_copies_when_identity_metadata_is_missing \ + auto_compare_copies_when_identity_metadata_does_not_match \ + size_mismatch_never_skips_regardless_of_compare_mode \ + destination_race_check_ignores_identity_metadata + +run_tests "cross-alias cp, mv, and diff integration tests" \ + cargo test -p rustfs-cli --test recursive_remote_copy -- \ + cross_alias_copy_records_the_source_etag_for_later_incremental_runs \ + cross_alias_move_uploads_then_deletes_the_source \ + cross_alias_move_keeps_the_source_when_the_upload_fails \ + diff_auto_compare_treats_a_recorded_source_identity_as_same \ + diff_etag_compare_ignores_the_recorded_identity_without_head \ + mirror_auto_compare_skips_objects_a_cross_alias_copy_already_migrated + +run_tests "CLI help contract includes diff --compare" \ + cargo test -p rustfs-cli --test help_contract -- top_level_command_help_contract + +log_success "Cross-alias identity regression suite passed"