feat(local): add fsync to LocalFileSystem for durability

[Barre]

• closes Reliable file writes for LocalFileSystem: explicit close error checking and opt-in sync #661

Call sync_all() on written files and fsync parent directories at all write-path boundaries (put, copy, rename, multipart complete) so that a successful return guarantees data is durable on disk, matching the implicit contract of cloud object stores.

Rationale for this change

When LocalFileSystem::put (or copy/rename/multipart complete) returns Ok, callers reasonably expect the data to be durable on disk as this is the implicit contract of every cloud object store like S3 or GCS.

However, LocalFileSystem never called fsync/sync_all, meaning the OS was free to keep the data in its page cache indefinitely. A crash or power loss after a successful put could result in data loss or zero-length files.

This change adds sync_all() calls on written files and fsync on parent directories at every write-path boundary (put_opts, copy_opts, rename_opts, multipart complete), ensuring that when an operation returns success, both the file contents and the directory entry pointing to them are durable on stable storage.

Are there any user-facing changes?

No.

[crepererum]

I think this is technically what we should do. I wonder however if we want to offer some kind of opt-out mechanism (I think in vein of "safety+sanity by default", it shouldn't be opt-in). WDYT @tustvold?

[tustvold]

I think calling fsync on the files makes a lot of sense, and ensures that we maintain atomicity (or at least try do our best to).

Calling fsync on directories also makes sense, but I think this PR doesn't go far enough, it needs to ensure that any recursively created directories are also fsynced...

I think it makes sense for this to be enabled by default (in a future breaking release), but it should be possible to turn this behaviour off, perhaps with a separate option that just fsync's files, ensuring atomicity.

There is also the question to me of what happens if a process is writing lots of files to the same directory, fsyncing the directory on every new file is rather wasteful, you really want some mechanism for the caller to fsync as part of some higher-level transaction. I don't know how we would expose this though...

Perhaps it doesn't matter - LocalFileSystem inherently trades performance for being able to interoperate with a filesystem, if someone wants the optimal disk performance they're probably better off using io_uring and something custom anyway.

[alamb]

I don't think we should automatically call fsync on filesystem writes. Users who want that behavior can already call fsync when the write returns. An option (defaulting to false) would make sense for convenience

There is also the question to me of what happens if a process is writing lots of files to the same directory, fsyncing the directory on every new file is rather wasteful, you really want some mechanism for the caller to fsync as part of some higher-level transaction.

I think it be best put in a layer higher up in the call stack (whatever is calling / orchestrating this higher level transaction using the object_store crate)

[Barre]

Defaulting to false seems a bit reckless to me, users are most certainly expecting the local "object store" to be durable, as are all other object store implementations in this crate (aside from memory...).

It was definitely a great surprise to me to learn that it was not.

[adamreeve]

Users who want that behavior can already call fsync when the write returns. An option (defaulting to false) would make sense for convenience

Is this true? You need a File to call fsync on, and you can't call fsync after the file is closed. The ObjectStore trait doesn't expose a way to get the file after a write as far as I can tell.

I think it be best put in a layer higher up in the call stack (whatever is calling / orchestrating this higher level transaction using the object_store crate)

For the same reason as above, I don't think this is really feasible.

[crepererum]

I don't think we should automatically call fsync on filesystem writes. Users who want that behavior can already call fsync when the write returns.

They can, but it's a major hassle to do. As written above, just calling fsync on the file isn't enough, so to implement this properly, you also have to walk the directories and figure out which ones you need to fsync (technically only the ones where new files or sub-directories have been created). That to me seems unpractical. Hence I think object-store should implement this, at least as an option.

To avoid confusion: calling fsync on a directory does NOT recursively fsync the whole subtree. It really just fsync "what's in this directory at this specific level" + "what's the state of the directory, e.g. permissions and attributes".

[alamb]

Defaulting to false seems a bit reckless to me, users are most certainly expecting the local "object store" to be durable, as are all other object store implementations in this crate (aside from memory...).

It was definitely a great surprise to me to learn that it was not.

I agree that if we were writing this library from scratch, having writes to files automatically call fsync would be a very reasonable behavior.

Likewise, I am not debating that adding some way to call fsync is valuable, I am simply voting against changing the default behavior to avoid causing hard to track down downstream implications (though it enough people vote the other way I will be willing to defer).

I recommend we do this in two PRs:

1. Add a new flag, default to false that adds fsync on write (there seems to be broad agreement that this is a useful feature)
2. Propose a separate PR to change the default of the new flag to true where we can debate the implications of that change.

Is this true? You need a File to call fsync on, and you can't call fsync after the file is closed. The ObjectStore trait doesn't expose a way to get the file after a write as far as I can tell.

This is a good point @adamreeve -- I stand corrected.

[Barre]

Latest commit should address your comments and concerns.

[crepererum]

While this gets the job done, I'm getting a bit uneasy about he number of IF-branches in the code and potential "we forgot another fsync here" issue. May I suggest that we pull out the file I/O ops into functions that are more re-usable, e.g.:

fn rename(file: ..., fsync: bool) -> Result<()> {
    if fsync {
        file.sync_all().map_err(|source| Error::UnableToSyncFile {
            source,
            path: src.clone(),
         })?;
     }

     std::fs::rename(&src, &s.dest)
         .map_err(|source| Error::UnableToRenameFile { source })?;

     if fsync {
         fsync_parent_dir(&s.dest)?;
     }

     Ok(())
}

fn hard_link(..., fsync: bool) -> Result<()> { ... }

[adamreeve]

Hi @Barre, are you still planning on following up on this and addressing the latest review comments? I'd also like to see this feature added and could help with this if you no longer have time.

[alamb]

Hi @Barre, are you still planning on following up on this and addressing the latest review comments? I'd also like to see this feature added and could help with this if you no longer have time.

Thanks -- I am also working on preparing the next release -- if you are interested in pulling this over the line @adamreeve I will put it on the list of

• Release object store 0.14.0 (breaking) - Target April 2026 #657

[Barre]

Thanks for the nudge 🙏🏻

I'll try to push something in few hours.

[alamb]

Thank you for bearing with us 🙏

[crepererum]

This critique still stands: #643 (review)

To keep this code maintainable, I think we need to encapsulate the std::fs operations with their respective fsync-calls into helper functions. Otherwise the risk of forgetting the fsync after the file system modification is just too high.

[Barre]

This critique still stands: #643 (review)

To keep this code maintainable, I think we need to encapsulate the std::fs operations with their respective fsync-calls into helper functions. Otherwise the risk of forgetting the fsync after the file system modification is just too high.

I hope this looks better to you now.

[alamb]

I reviewed this PR and I think it looks good now -- I verified test coverage via llvm-cov . Thank you so much @Barre and @crepererum

One thing I did notice is that basically all error paths are untested in this LocalFileSystem as they require IO errors to trigger. I think this is a gap that we should eventually fix, but as it was not made worse by this PR I don't think anything is required here.

cc @crepererum in case you want a chance to review again before we merge

Update I filed this ticket for error handling

• #753

[alamb]

I spent some time reviewing this and reviewing the calls to fsync -- to be clear the the File::fsync_all docs despite the name, only flushes the contents of this file descriptor to disk.

It also might be worthwhile pointing out that fsync_dir is actually doing three syscalls (open, fsync and then an implicit close). However, apparently this is the same pattern as sqlite uses

https://github.com/sqlite/sqlite/blob/9a5801af3a411c880f556233dda7a3097967f8af/src/os_unix.c#L3945-L3948

so looks good to me

[alamb]

I made a small fix in f9e526f for a rust docs CI failure here and then merged up from main to try and get a clean CI run

[alamb]

I'll plan to merge this tomorrow unless anyone else would like longer to review

FYI @jgiannuzzi and @adamreeve as I believe you expressed interest in this feature as well

[adamreeve]

This looks good to me, thanks for adding this @Barre!

[kevinjqliu]

LGTM

Codex flagged one potential issue: RenameTargetMode::Create copies with fsync but deletes the source via delete_location without fsyncing the source parent dir, so with_fsync(true) could make the destination durable while leaving the source deletion non-durable.
Heres the fix that it proposed f6e15ff

[alamb]

f6e15ff

Thanks @kevinjqliu -- for the case of moving things along, I will merge this PR. can you make the proposed fix a separate PR?

[kevinjqliu]

Create #758 as a follow up, thanks!