Skip to content

fix(services/fs): make writes crash-safe on supported platforms - #8315

Open
Huliiiiii wants to merge 1 commit into
apache:mainfrom
Huliiiiii:crash-safe-unix
Open

Huliiiiii wants to merge 1 commit into
apache:mainfrom
Huliiiiii:crash-safe-unix

Conversation

@Huliiiiii

Copy link
Copy Markdown

Which issue does this PR close?

Closes #.

Rationale for this change

Write is not crash-safe on unix.

What changes are included in this PR?

  • Write metas to the tempfile before renaming it, rather than writing it after fsync.
  • Fsync parent directories after renaming the file.

Are there any user-facing changes?

N/A

Breaking changes

N/A

AI Usage Statement

GPT: Investigation, research, and exploration
Guided code implementation.

@Huliiiiii
Huliiiiii requested a review from Xuanwo as a code owner September 16, 2026 06:17
@github-actions github-actions Bot added core releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 16, 2026
@Huliiiiii

Huliiiiii commented Sep 16, 2026 •

Copy link
Copy Markdown
Author

On Apple's platforms, std file sync will use full fsync instead of regular fsync, with higher overheads.

Matching the behavior on other platforms would require adding a dependency such as rustix.

Some Unix platforms don't support fsync on directories, but they are uncommon, so I think it's fine to leave them unsupported for now.

I also ran into another issue. fsync errors should generally not be retried, but new_std_io_error marks other io errors as temporary. I have to mark them as permanent everywhere.

@Xuanwo

Xuanwo commented Sep 16, 2026

Copy link
Copy Markdown
Member

Could you run a benchmark to measure the differences?

@Huliiiiii

Copy link
Copy Markdown
Author

Performance impact

Size Prev Current Difference
4 KiB 1.614 ms 4.393 ms +2.779 ms (+172.2%)
64 KiB 1.742 ms 4.492 ms +2.750 ms (+157.9%)
1 MiB 2.329 ms 5.134 ms +2.805 ms (+120.4%)
16 MiB 12.585 ms 15.540 ms +2.955 ms (+23.5%)

@erickguan erickguan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would majority of use cases have temp_path = None?

What if two paths are in two different file systems?

@Huliiiiii

Copy link
Copy Markdown
Author

Would majority of use cases have temp_path = None?

The temp_path is Some only when atomic_write_dir is enabled.

What if two paths are in two different file systems?

rename will throw an EXDEV error.
https://man7.org/linux/man-pages/man2/rename.2.html

@Xuanwo Xuanwo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's postpone this change until we figure out how to avoid surprising our users.

@erickguan

erickguan commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

The current fs behavior looks quite stable. If we document atomic_write_dir, this feature is good enough.

If you want to go further, uutils looks quite promising. But uutils wants to implement cp. uutils might not optimize performance, neither has proper crates to reuse the code. I would suggest caution because uttils work requires lots of efforts.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants