Skip to content

Fix fsync CLI plumbing #2014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
oferchen merged 1 commit into from
Nov 7, 2025
Merged

Fix fsync CLI plumbing #2014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/cli/src/frontend/execution/drive/workflow/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ where
stats,
partial,
preallocate,
fsync,
delay_updates,
partial_dir,
temp_dir,
Expand Down Expand Up @@ -435,6 +436,7 @@ where
stats,
partial,
preallocate,
fsync,
delay_updates,
Comment on lines 436 to 440
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Pass fsync to fallback context without struct field

The new fsync field is supplied when building the remote fallback context, but FallbackArgumentsContext in fallback_plan.rs has no such member and the fallback pipeline never consumes it. As written this initializer fails to compile and, even once compilation is fixed, the flag still won’t reach the fallback arguments unless the struct and downstream FallbackInputs are extended.

Useful? React with 👍 / 👎.

partial_dir: partial_dir.as_ref(),
temp_dir: temp_dir.as_ref(),
Expand Down Expand Up @@ -583,6 +585,7 @@ where
} = metadata;

let prune_empty_dirs_flag = prune_empty_dirs.unwrap_or(false);
let fsync_flag = fsync.unwrap_or(false);
let inplace_enabled = inplace.unwrap_or(false);
let append_enabled = append.unwrap_or(false);
let whole_file_enabled = whole_file_option.unwrap_or(true);
Expand Down Expand Up @@ -653,6 +656,7 @@ where
debug_flags_list,
partial,
preallocate,
fsync: fsync_flag,
partial_dir: partial_dir.clone(),
Comment on lines 656 to 660
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge ConfigInputs initialization uses undefined fsync field

ConfigInputs in drive/config.rs does not declare an fsync field and build_base_config never handles such a value. Initializing the struct with fsync: fsync_flag therefore fails to compile and, even if it compiled, the fsync option would still be dropped. Add the field to ConfigInputs and plumb it through the builder before setting it here.

Useful? React with 👍 / 👎.

temp_dir: temp_dir.clone(),
delay_updates,
Expand Down
51 changes: 51 additions & 0 deletions crates/engine/src/local_copy/tests/execute_fsync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
use crate::local_copy::{
test_support::take_fsync_call_count, LocalCopyExecution, LocalCopyOptions, LocalCopyPlan,
};

#[test]
fn execute_performs_fsync_when_requested() {
let temp = tempdir().expect("tempdir");
let source = temp.path().join("source.txt");
let destination = temp.path().join("dest.txt");
fs::write(&source, b"payload").expect("write source");

let operands = vec![
source.into_os_string(),
destination.clone().into_os_string(),
];
let plan = LocalCopyPlan::from_operands(&operands).expect("plan");

// Clear any previous instrumentation counts.
take_fsync_call_count();

let options = LocalCopyOptions::default().fsync(true);
plan
Comment on lines +1 to +22
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Fsync tests call nonexistent helpers

The added tests import test_support::take_fsync_call_count and call LocalCopyOptions::fsync(true), but neither a test_support module nor an fsync setter exist under crate::local_copy (no such identifiers appear anywhere else in the crate). Compiling these tests will therefore fail until a real fsync option and instrumentation helpers are implemented.

Useful? React with 👍 / 👎.

.execute_with_options(LocalCopyExecution::Apply, options)
.expect("copy succeeds");

assert_eq!(take_fsync_call_count(), 1);
assert!(destination.exists());
}

#[test]
fn execute_skips_fsync_when_not_requested() {
let temp = tempdir().expect("tempdir");
let source = temp.path().join("source.txt");
let destination = temp.path().join("dest.txt");
fs::write(&source, b"payload").expect("write source");

let operands = vec![
source.into_os_string(),
destination.clone().into_os_string(),
];
let plan = LocalCopyPlan::from_operands(&operands).expect("plan");

take_fsync_call_count();

plan
.execute_with_options(LocalCopyExecution::Apply, LocalCopyOptions::default())
.expect("copy succeeds");

assert_eq!(take_fsync_call_count(), 0);
assert!(destination.exists());
}