fix(save): stop two writers on one file from breaking each other

[PathGao]

Follow-up to #424, which reported this and deliberately did not fix it:

The temp-name collision above is a property of atomic_write, not of this file. It needs two threads writing the same target at the same nanosecond […] Reported here rather than fixed; a fix belongs with atomic_write.

Two commits, one focused problem: two writers on one file used to break each other. The first fixes the primitive so concurrent writers are safe; the second removes the in-app source of the concurrency. They touch disjoint files and are independently reviewable — the split is by layer, and either can be dropped without the other failing to compile or test.

---

1. atomic_write's temp name was a collision waiting on the clock

The temp file was named from the target name, the pid, and a nanosecond clock reading:

let nanos = SystemTime::now().duration_since(UNIX_EPOCH)...as_nanos();
let temp_name = format!(".{}.markpad-tmp-{}-{}", file_name, pid, nanos);

macOS's timer granularity is coarser than a nanosecond, so two threads calling SystemTime::now() back to back read the same value and derive the same name.

The collision took down both writers, not just the loser

This is the part that makes it a defect rather than a retry. create_new picking one winner is correct and intended. The damage is in the loser's cleanup:

  thread A                              thread B
 ①  now() → 1754_331_000                now() → 1754_331_000    ← same reading
 ②  .a.md.markpad-tmp-9182-1754_331_000 (byte-identical name)
 ③  create_new ✓ wins                   create_new ✗ EEXIST(17)
 ④  write_all + fsync                   remove_file(temp_path)
                                             ↑ deletes A's file
                                        return Err(17)          ← B fails
 ⑤  rename(temp → a.md)
    ✗ ENOENT(2)                                                 ← A fails too

The loser ran fs::remove_file(&temp_path) on a path it had computed but never successfully created. At that moment the path names the winner's file. So one collision produced the File exists (os error 17) / No such file or directory (os error 2) pair reported in #424.

The window is small, which does not make it rare

The overlap needed is not "two writes overlapping" — it is "two now() calls in one clock tick":

  ├─ now() ─ format ─ create_new ─┤─ write_all ─ fsync ─ rename ─ fsync(dir) ─┤
  └──── the vulnerable window ────┘└──── irrelevant to whether names collide ──┘

The predicate's width is the clock tick, not the write duration. Eight threads starting together land in one or two ticks.

The fix

Uniqueness comes from a process-wide AtomicU64 instead of a clock that cannot supply it — fetch_add is an atomic read-modify-write, so no two callers can be handed the same value. pid and nanos stay in the name so it is still unique across processes and readable in a directory listing.

create_new still arbitrates across processes — a stale temp left by a dead process whose pid we inherited — so AlreadyExists retries with a fresh name, bounded at 16. Any other error returns immediately; retrying will not help.

The cleanup path is fixed independently of the naming: the file handle is acquired before temp_path exists as a binding, so every subsequent remove_file provably refers to a file this call created. Even if the naming were broken again, a collision would degrade to an ordinary error affecting only its own caller.

The fsync / rename / permission-restore / symlink semantics in the doc comment are untouched.

Who was exposed

#424 fixed pinned-tags.json by serialising that file's read-modify-write cycle, which incidentally removed its exposure. The weakness itself was untouched, so every other caller still had it: save_file, save_file_binary, save_theme, the VSIX theme install, save_window_state, and the image-drop path.

Test

concurrent_atomic_writes_to_one_target_all_succeed runs 8 threads against one target and asserts every call returns Ok, that the surviving file is byte-identical to one of the values written (a torn file would mean a rename published someone else's half-filled temp), and that no temp files remain.

Counter-proof, old naming and cleanup restored, nothing else changed (macOS/APFS, 40 runs each):

	failures
clock-derived name + unconditional cleanup	20 / 40
this branch	0 / 40

---

2. An explicit save and its own debounce could both be in flight

atomic_write now makes concurrent writers safe. It does not make them ordered, and it cannot — atomicity is per-write. So the second half is about not starting the second write.

The auto-save timer is armed on the last keystroke. It is disarmed by the auto-save effect only once isDirty goes false, and that happens after await invoke('save_file_content', …) resolves. A timer expiring inside that window issues a second write of the same file:

  last keystroke
  ├────────────────── 1500ms debounce ──────────────────┤
  │                                                     ▼ timer fires
  │                        ┌──── Ctrl+S ────┐
  │                        │ invoke in flight│
  │                        │ isDirty still true → timer still armed
  │                        └────────────────┘
  │                                     └── overlap = one invoke's duration

If the user typed during the flight, the two writes carry different snapshots and race to the rename. If the older one lands last, the disk holds the earlier text while the tab has recorded the newer one as saved — isDirty is false, the buffer reads clean, and it stays a revision behind until the next keystroke.

The window is milliseconds on a local SSD. On a network or removable volume it is seconds — save_file_content's own doc comment is why (atomic_write fsyncs twice, which is what made the command async in the first place). The defect scales with the storage the user can least afford to lose work on.

The duty was at the call sites, and half of them missed it

cancelPendingAutoSave already existed for exactly this, with a documented contract: never disarm on a path the user can still cancel. But calling it was each caller's job, and of the six explicit-save entry points, three did not:

entry point	cancelled before?
Ctrl+S	✗
toolbar / menu Save	✗
preview task checkbox (toggleTaskCheckbox)	✗
leaving edit mode / closing split (flushBeforeLeavingEditableMode)	✓
closing a dirty tab (canCloseTab)	✓
quit-time bulk save	✓

The fix

The cancel moves into saveContent, placed after the Save dialog so it still cannot disarm a tab on a path the user can back out of, and before the write it protects. The four now-redundant call-site copies are removed.

One call site keeps its own: the discard branch of canCloseTab. This is kept as scope, not as a proven necessity, and it is the one thing worth a reviewer's eye. No saveContent is on that path to do it — but the three statements after it are synchronous, so no timer can fire between them, and the auto-save effect drops the timer on its own once isDirty goes false. Removing the call would therefore probably still be correct; correctness would just rest on the effect flushing ahead of a pending macrotask, and on the effect running at all while a window tears down on the quit path. A synchronous cancel rests on neither, and rewriting it is a separate judgement from the one this change makes about the save paths. Happy to drop it if you would rather it went.

Moving it also fixes the class rather than the three instances — a seventh entry point cannot reintroduce this by forgetting.

Tests

scripts/explicitSaveCancelsAutoSave.test.ts, four cases against the real TabManager and the real documentSession:

• the cancel happens before the write — cancelling after would leave the timer free to fire during the very await this protects, so ordering is the assertion, not presence;
• a save reached without naming a tab (saveContent(), which is what Ctrl+S and the toolbar do) disarms the active tab too;
• a Save dialog the user cancels leaves the timer alone — the standing contract, pinned so the placement cannot drift back before the modal;
• no call site takes the duty back, which is the actual regression being guarded.

Counter-proofs:

change	result
remove the cancel from saveContent	3 of 4 fail
re-add a call-site cancel before a saveContent	the call-site guard fails

The dialog-cancel case is an invariant rather than a regression test for this change, and correctly stays green under both.

---

Verification

Run on the rebased branch (current upstream/master, fab137c):

npm run check   0 errors, 0 warnings   (645 files — #432's scripts/ checking covers the new test)
npm test        566 / 566
cargo test      150 / 150
cargo clippy    3 warnings — the pre-existing baseline, none from this change

Not covered

• Cross-window writers to one file. Tab de-duplication (fix(tabs): ask the filesystem whether two paths name the same file #416) is per-window: two windows can each hold the same document, with independent timers and independent isDirty. No frontend coordination reaches across them — this is precisely why part 1 belongs in atomic_write and part 2 cannot replace it. Both writes now succeed, the later one wins, and the other window's file watcher raises the existing external-change bar. That is the pre-existing product semantic and is unchanged here.
• A write slower than the 1.5s debounce. The timer can still be re-armed by typing and fire during a very long explicit save. Part 1 makes that safe; the residual is the transient stale-on-disk described above, which heals on the next keystroke. Serialising writes per path would close it, at the cost of a promise-chain and snapshot relocation — not taken, as the remaining exposure is narrow and no longer unsafe.
• No per-path lock in atomic_write. It would not have fixed fix(tags): stop two windows from erasing each other's pinned tags #424: a read-modify-write cycle needs the lock above the read, which is why fix(tags): stop two windows from erasing each other's pinned tags #424's mutex sits in window_runtime.rs. For whole-value writers, part 1 already guarantees safety and wholeness, and last-write-wins is the semantic the callers hand down.
• No live multi-window or slow-volume run. Part 1 is verified by threads against the real function; part 2 by the real documentSession against a stubbed backend. Neither was reproduced by hand in the running app.

🤖 Generated with Claude Code