Skip to content

fix(fs): tolerate FatFS VFS limits in atomic dir writes - #324

Open
Love4yzp wants to merge 1 commit into
pocket-stack:mainfrom
Love4yzp:fix/fs-fatfs-atomic-write
Open

fix(fs): tolerate FatFS VFS limits in atomic dir writes#324
Love4yzp wants to merge 1 commit into
pocket-stack:mainfrom
Love4yzp:fix/fs-fatfs-atomic-write

Conversation

@Love4yzp

Copy link
Copy Markdown

Problem

dir_write's atomic-overwrite path assumes two POSIX behaviors that ESP-IDF's FATFS VFS does not provide, so every atomic write through the fs module fails on ESP32 targets with a FatFS-backed root:

  • O_EXCL opens fail with ENOENT on the FATFS VFS, so the temp-file create_new call never succeeds and the write errors out before any payload lands.
  • FatFS f_rename refuses to overwrite an existing destination (FR_EXIST, surfaced as EEXIST) where POSIX rename replaces it, so rewriting an existing file fails even after the temp file landed and synced.

Fix

Both fallbacks arm only on the exact error kinds where the filesystem is known to diverge from POSIX:

  • Temp-file creation falls back to create+truncate on ENOENT. The tmp dir is module-owned and swept, and the monotonic counter keeps the name unique, so losing O_EXCL costs nothing.
  • The final rename falls back to remove+rename on EEXIST. The atomic-overwrite contract is not representable on FatFS, so the stale target is removed first.

Behavior on POSIX filesystems is unchanged — both fallback arms are unreachable there.

Verification

  • cargo test -p pocket-fs12/12 pass on the host.
  • Real hardware: Seeed reTerminal D1001 (ESP32-P4, ESP-IDF v6.1, FatFS on SD over the VFS) — data.fs writes and rewrites land and persist across reboots. Board-side receipts: pocketjs-d1001 verification doc.

Love4yzp added a commit to Love4yzp/pocketjs-d1001 that referenced this pull request Aug 22, 2026
@Love4yzp
Love4yzp marked this pull request as ready for review August 22, 2026 13:32
ESP-IDF's FATFS VFS diverges from POSIX in two places that break
dir_write's atomic-overwrite path:

- O_EXCL opens fail with ENOENT, so the temp-file create_new never
  succeeds and every atomic write errors out. Fall back to
  create+truncate: the tmp dir is module-owned and swept, and the
  monotonic counter keeps the name unique.
- FatFS f_rename refuses to overwrite an existing destination
  (surfaced as EEXIST) where POSIX rename replaces it. Fall back to
  remove+rename; the atomic-overwrite contract is not representable
  on FatFS.

Both fallback arms are unreachable on POSIX filesystems, so behavior
there is unchanged. Verified with cargo test -p pocket-fs (12/12) and
on an ESP32-P4 board (ESP-IDF v6.1, FatFS on SD over the VFS) where
data.fs writes and rewrites land and persist across reboots.
@Love4yzp
Love4yzp force-pushed the fix/fs-fatfs-atomic-write branch from 4536128 to 1e361c7 Compare August 22, 2026 15:21

@doodlewind doodlewind left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the hardware reproduction and the narrow error handling. I am requesting changes because the current fallback breaks the fs contract in two merge-blocking ways:

  1. After the first rename returns AlreadyExists, the code removes the old destination before attempting the second rename. If that rename fails or power is lost in between, the old file is gone; on the next FsModule construction the temp directory is swept, so the new file is lost too. FS_WRITE_TRUNCATE promises old content or new content after power loss, never a missing target. Please use a recoverable replace/journal sequence that preserves that invariant instead of silently downgrading the reference core.

  2. The public dir_rename path is unchanged, so FatFS still cannot replace an existing destination even though OP_RENAME requires atomic file replacement and is the documented whole-file update path for files larger than one IO chunk. Please centralize the filesystem-specific replace behavior so truncate writes and public rename cannot diverge.

The existing 12 host tests pass only on the POSIX path and do not execute either fallback. The hardware receipt proves normal write/rewrite persistence, but not interruption of the remove-then-rename window. Please add deterministic fault-injection coverage for the ENOENT and EEXIST fallbacks, second-rename failure, and startup recovery, and add pocket-fs to a PR CI path.

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.

2 participants