Skip to content

feat(local): reliable file writes for LocalFileSystem#662

Closed
jgiannuzzi wants to merge 1 commit intoapache:mainfrom
jgiannuzzi:sync-on-close
Closed

feat(local): reliable file writes for LocalFileSystem#662
jgiannuzzi wants to merge 1 commit intoapache:mainfrom
jgiannuzzi:sync-on-close

Conversation

@jgiannuzzi
Copy link

@jgiannuzzi jgiannuzzi commented Mar 16, 2026

Which issue does this PR close?

Closes #661.

Rationale for this change

See issue.

What changes are included in this PR?

  • Add close_file() helper that calls libc::close() (Unix) / CloseHandle (Windows) directly and propagates errors, instead of relying on File::drop(). Note that this uses unsafe, like in the platforms' respective drop() implementations.
  • Add with_sync_on_close(bool) builder method on LocalFileSystem that calls File::sync_all() before closing, providing stronger durability guarantees for network filesystems.
  • Apply both to the put and put_multipart (via LocalUpload) code paths.
  • Add libc (Unix) and windows-sys (Windows) as optional dependencies behind the fs feature for their respective platforms.
  • Duplicate the existing integration test to also run with sync_on_close(true). Note that this doesn't "do" anything and is just there to validate that the additional behaviour does not create issues.
  • Add unit test verifying close_file detects EBADF when the file descriptor is closed behind Rust's back. This is just to validate that close_file properly returns the OS error.

Are there any user-facing changes?

New public API: LocalFileSystem::with_sync_on_close(bool) -> Self. Default is false (no behaviour change for existing users).

Close errors are now detected and propagated as Error::UnableToCopyDataToFile. Previously these were silently ignored. This is a correctness improvement but could surface errors that were previously hidden.

Add explicit close with error checking on Unix (libc::close) and Windows (CloseHandle), replacing silent drop.
Add opt-in sync_on_close for stronger durability guarantees on network filesystems (e.g. NFS).
@jgiannuzzi
Copy link
Author

Hi @crepererum, could you please take a look at this change? Thanks!

@crepererum
Copy link
Contributor

As far as I can see, this has the same issue as #643: syncing the file itself is NOT enough for durability. You have to sync the directories as well.

@jgiannuzzi jgiannuzzi changed the title feat: reliable file writes for LocalFileSystem feat: report close errors for LocalFileSystem Mar 23, 2026
@jgiannuzzi jgiannuzzi closed this Mar 23, 2026
@jgiannuzzi jgiannuzzi deleted the sync-on-close branch March 23, 2026 13:05
@jgiannuzzi jgiannuzzi changed the title feat: report close errors for LocalFileSystem feat: reliable file writes for LocalFileSystem Mar 23, 2026
@jgiannuzzi
Copy link
Author

Superseded by #676 which implements only the explicit close with error reporting.

@jgiannuzzi
Copy link
Author

Thanks @crepererum, I had only looked at issues and not pull requests and thus hadn't seen #643. I have extracted the close bits of this PR into #676. Could you please take a look at it?

@jgiannuzzi jgiannuzzi changed the title feat: reliable file writes for LocalFileSystem feat(local): reliable file writes for LocalFileSystem Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reliable file writes for LocalFileSystem: explicit close error checking and opt-in sync

2 participants