From cec7bce4506accc5af072a995876346f6fbe53ea Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 29 Nov 2023 22:49:00 -0500 Subject: [PATCH] try copy instead --- .../filesys/src/rename_across_devices.rs | 4 ++ .../util/maybe_download_file_from_bucket.rs | 58 +++++++++++++++---- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/crates/lib/files/filesys/src/rename_across_devices.rs b/crates/lib/files/filesys/src/rename_across_devices.rs index 91a5ae9e67..60a4046976 100644 --- a/crates/lib/files/filesys/src/rename_across_devices.rs +++ b/crates/lib/files/filesys/src/rename_across_devices.rs @@ -24,6 +24,10 @@ pub fn rename_across_devices, Q: AsRef>(from: P, to: Q) -> } } + // FIXME(bt,2023-11-29): There appears to be a new bug where the std::fs::copy succeeds in + // copying zero bytes (perhaps the file was truncated to zero bytes before in std::fs::rename?), + // and we delete the source file (below). This only appears to happen in GCP currently, and this + // code has worked for months prior to this. let _num_bytes = std::fs::copy(&from, &to)?; std::fs::remove_file(&from)?; diff --git a/crates/service/job/inference_job/src/util/maybe_download_file_from_bucket.rs b/crates/service/job/inference_job/src/util/maybe_download_file_from_bucket.rs index 4640844b60..7c3285e70e 100644 --- a/crates/service/job/inference_job/src/util/maybe_download_file_from_bucket.rs +++ b/crates/service/job/inference_job/src/util/maybe_download_file_from_bucket.rs @@ -95,21 +95,40 @@ pub async fn maybe_download_file_from_bucket( info!("File size of {} temp download file {:?} is {original_size}", args.name_or_description_of_file, &temp_path); - info!("Renaming {} temp file from {:?} to {:?}!", + //info!("Renaming {} temp file from {:?} to {:?}!", + // args.name_or_description_of_file, &temp_path, &args.final_filesystem_file_path); + + //rename_across_devices(&temp_path, &args.final_filesystem_file_path) + // .map_err(|err| { + // error!("could not rename on disk: {:?}", err); + // safe_delete_temp_directory(&temp_dir); + // ProcessSingleJobError::from_io_error(err) + // })?; + + info!("Copying {} temp file from {:?} to {:?}!", args.name_or_description_of_file, &temp_path, &args.final_filesystem_file_path); - rename_across_devices(&temp_path, &args.final_filesystem_file_path) - .map_err(|err| { - error!("could not rename on disk: {:?}", err); - safe_delete_temp_directory(&temp_dir); - ProcessSingleJobError::from_io_error(err) - })?; + // NB: We're now seeing a bug where the resultant copied file is 0 bytes + match copy_with_logging(&temp_path, &args.final_filesystem_file_path) { + Err(err) => { + error!("Error Copying {} temp file from {:?} to {:?}! {err}", + args.name_or_description_of_file, &temp_path, &args.final_filesystem_file_path); + + return Err(ProcessSingleJobError::from_anyhow_error(err)); + } + Ok(false) => { + error!("Error copying {} temp file from {:?} to {:?}! File size in bytes did not match.", + args.name_or_description_of_file, &temp_path, &args.final_filesystem_file_path); + + reattempt_copy_if_failed(&args, &temp_dir, &temp_path, original_size)?; + } + Ok(true) => { + // Success case + } + } info!("Finished downloading {} file to {:?}", args.name_or_description_of_file, &args.final_filesystem_file_path); - // NB: We're now seeing a bug where the resultant copied file is 0 bytes - reattempt_copy_if_failed(&args, &temp_dir, &temp_path, original_size)?; - safe_delete_temp_directory(&temp_dir); Ok(()) @@ -161,3 +180,22 @@ fn reattempt_copy_if_failed(args: &MaybeDownloadArgs, temp_dir: &TempDir, temp_p Ok(()) } + + +pub fn copy_with_logging, Q: AsRef>(from: P, to: Q) -> AnyhowResult { + let original_size = file_size(&from)?; + + let num_bytes_copied = std::fs::copy(&from, &to)?; + + let destination_size = file_size(&to)?; + + info!(r#"Copied {:?} to {:?} + - Copied bytes: {num_bytes_copied} + - Orig. bytes: {original_size} + - Dest. bytes: {destination_size} + "#, from.as_ref(), to.as_ref()); + + let file_sizes_match = original_size == destination_size; + + Ok(file_sizes_match) +}