rename() is not durable without an fsync of the containing directory

Open
  • Is something wrong?
#243 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
50/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
javascript, node.js
Domain
backend

Research direction

Start in the writeFile path where the temp file is fsynced and then renamed onto the target, and the code that reads the fsync option. Before adding anything, confirm with a maintainer whether the extra directory sync is wanted, since the issue says it doubles the sync cost. If it is, add a directory fsync after the rename, skipping EPERM and EISDIR, and a test that checks it runs when fsync is enabled.

Written by the indexing model from the issue text.

Description

writeFile fsyncs the temp file and renames it into place, but never syncs the directory the rename happens in:

open(tmp) → write → fsync(fd) → close → chmod → rename(tmp, target)

fsync(fd) makes the file's contents durable. rename() changes a directory entry, and that entry stays in the page cache until the filesystem commits it. A crash in that window leaves the target with its old contents and the temp file orphaned beside it — after writeFile has already returned success.

Measured, not inferred

dm-log-writes records every write at the block layer, the journal is replayed to each crash point onto a blank device, and mounting runs the filesystem's own recovery. Crash taken at the moment the call returned:

after recovery directory
current behaviour old contents target, target.tmp, lost+found
plus a directory fsync new contents target

The control is inside the same measurement: the temp file, which is fsynced, survives. Only the rename does not. Reproduces on ext4, xfs and btrfs.

The window is bounded by the filesystem's journal commit interval — roughly five seconds on ext4 at its default commit=5, roughly sixty at commit=60.

Possible fix

One more sync after the rename, gated on the existing fsync option, and tolerant of Windows where syncing a directory handle throws EPERM:

if (options.fsync !== false) {
  const dir = fs.openSync(path.dirname(truename), 'r')
  try {
    fs.fsyncSync(dir)
  } catch (err) {
    if (err.code !== 'EPERM' && err.code !== 'EISDIR') throw err
  } finally {
    fs.closeSync(dir)
  }
}

This costs a second sync per write, so it may well be a deliberate trade. If so, a line next to the fsync option would help callers who read "atomic" as "durable" — those are different guarantees, and only the first one is in the name. Happy to send a PR for either the fix or the note.

Method, full audit across twelve targets and three filesystems, and everything needed to reproduce: https://github.com/BOTIROFF-D/gray

Dominant language
JavaScript
Stars
256
Forks
51
No merged PRs in 30d

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from npm/write-file-atomic

All issues in npm/write-file-atomic

Similar issues

More JavaScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.