[NikitkaRa]

No issue. This is data-loss-on-crash hardening for File › Save and AutoRecover. The branch is fresh from main. No other open PR touches write_file or recovery::save: #644 touches only recovery::list and open, and its settings.rs writes UI settings, not presentations.

Summary

• New fsx::write_atomic, used by every save to a path and by AutoRecover:
  • Writes a temporary sibling, runs sync_all on it, renames it over the target, then syncs the folder (best effort).
  • Before, nothing was flushed: a power cut after the rename could leave the new name pointing at empty data.
  • A failed write now removes its temporary file. Before, x.deckcraft.saving was left behind.
• Behaviour changes:
  • Read-only file: refused with "the file is read-only; save a copy under another name", as PowerPoint does. Before, the rename silently replaced it.
  • Symbolic link: the save writes to the file the link points at, so the link stays a link. Before, it was replaced by a regular file.
  • Permissions: the new file keeps the old file's mode. Before, a private 0600 deck came back 0644.
  • Error messages: they say what happened (disk full, permission denied, the folder no longer exists, not a file) and that the file on disk was left as it was. The presentation stays unsaved and keeps its AutoRecover entry.
• AutoRecover:
  • The metadata file is written atomically too.
  • One presentation failing to write is logged and no longer stops the others. An error is returned only when nothing could be written.

Test plan

• fsx::tests::replaces_the_file_keeping_its_permissions (0600 stays 0600, no temporary file left)
• fsx::tests::a_read_only_file_is_refused_and_left_alone
• fsx::tests::a_link_keeps_pointing_at_its_file
• fsx::tests::failures_leave_the_old_file_and_no_temporary: folder gone, a folder where the file should be, a folder that can't be written to (run as uid 1000, so the permission check really runs)
• fsx::tests::a_failed_save_keeps_the_presentation_unsaved: file.saveAs to a read-only file and to a vanished folder; the deck stays dirty and the AutoRecover copy stays
• cargo xtask ci: all 6 steps pass
• I didn't simulate disk full. That case goes through the same cleanup path as the other write failures.

Parity checklist: refusing to overwrite a read-only file matches PowerPoint. No new rows.

[Harrison-Booth]

This now conflicts with main after the module split in #1433 (the files moved into modules; see docs/ path map PR 1468). If you would rather not, I can open a PR that credits you, keeps your commits and shows a git range-diff against this one, so you'll still get credit and have visible work.

[FredApps]

ReviewerA — source-read COMMENT (not an approval)

I read the head of the diff: file saves are atomic, flushed, and respect read-only files, links and permissions. The old code wrote to a temporary sibling and renamed it over the target; the doc now points to write_atomic, and file_access::check_write(path) runs before it. Atomic saves have well-known traps that the PR title names: flush and sync the data before the rename, keep the file's permissions and ownership, follow a link to write the real target and not replace the link, refuse read-only files with a clear message, and clean up the temporary file on failure. The same concerns, for macOS ACLs and hard links, are in pdfcraft#5166 and photocraft#8218, and the suite should share one helper. I read the doc comment and the call site; write_atomic itself was not in my hunks. I have not run it, and no exact-head CI result exists for this head. Nothing was run.

I ran nothing and have no exact-head CI result for this PR, so this is not an approval. Where a diff was long I read its code hunks rather than every generated or documentation line.

Status: waiting_author.

[aiongg]

AI-generated review (Opus). Not a maintainer decision.

Changes needed:

This makes saves and AutoRecover writes flush to disk before the rename, refuse read-only files, keep links and permissions, and keep going when one AutoRecover entry fails. Since this branch was cut, the main branch's write_file has gained flushing, permission keeping, link resolution, a short temp-file name and a write-root check on the temp file, so this change has to be rebuilt on top of that code.

• crates/engine/src/io.rs:68: the temp name {name}...saving makes a file name longer than the target's, so a name close to the file system limit can't be saved. The current write_file uses a short fixed .deckcraft...saving name for this reason.
• crates/engine/src/io.rs:66-75: the current write_file calls check_write on the temporary sibling too, so a save to the write root itself can't create a file outside it. This version drops that check.
• crates/engine/src/recovery.rs:83: on the current branch, recovery entries are created with mode 0600. Routing them through io::write_atomic creates new entries with the default mode, so unsaved presentations become readable by other users.
• crates/engine/src/io.rs:38-50: resolve_link follows only a link in the last path component. canonicalize, which the current code uses, also resolves linked folders.
• crates/engine/src/io.rs:45: please remove the mention of another product from the doc comment and describe the behaviour itself.
• The new parts are worth keeping on top of the current code: refusing a read-only file with a clear message, syncing the folder after the rename, the clearer error wording, and AutoRecover going on after one entry fails (crates/engine/src/recovery.rs:134-159).
• The description refers to fsx::write_atomic. The function is io::write_atomic.

Claims checked:

• contradicted: Saves write a temp sibling, sync_all it, rename it over the target, then sync the folder; before, nothing was flushed (crates/engine/src/io.rs:79-96 does sync the file and then the folder (unix), but main already calls sync_all before the rename (main crates/engine/src/io.rs:62); only the folder sy…
• verified: A failed write removes its temporary file (crates/engine/src/io.rs:89-91 removes tmp when the write, sync or rename fails)
• verified: A read-only file is refused with a 'save a copy under another name' message (crates/engine/src/io.rs:60-62 returns an error when metadata permissions().readonly())
• verified: A symbolic link stays a link and the save writes to its target (crates/engine/src/io.rs:38-50 resolve_link follows a link in the last component and the rename targets it (io.rs:89); main already resolves links with canonicalize (main io.rs:40-43))
• verified: The new file keeps the old file's mode (0600 stays 0600) (crates/engine/src/io.rs:82-85 set_permissions from the existing metadata; main already does this (main io.rs:44,59-61))
• contradicted: AutoRecover metadata and entries are written atomically through the same helper (crates/engine/src/recovery.rs:82-85 routes to io::write_atomic, which creates new entries with the default mode, whereas main's recovery write_atomic sets 0600 (main crates/engine/src/recovery.rs:90-96); merged onto ma…
• verified: One AutoRecover presentation failing no longer stops the others; an error is returned only when nothing was written (crates/engine/src/recovery.rs:134-159 logs each error, counts successes and returns Err only when n == 0)
• verified: Error messages say what happened and that the file on disk was left as it was (crates/engine/src/io.rs:99-105 describe_io maps StorageFull and PermissionDenied; io.rs:54-63 cover a missing folder, not a file and read-only)
• contradicted: Every save to a path goes through write_atomic without losing existing safeguards (crates/engine/src/io.rs:68 uses a temp name longer than the target's, and io.rs:66-75 never calls check_write on the temp sibling; main has both fixes (main crates/engine/src/io.rs:48-52))

Recommendation: update the pull request; it will be reviewed again.

Reviewed at d240d16.