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

scripts/explicitSaveCancelsAutoSave.test.ts

@@ -0,0 +1,186 @@
+import assert from 'node:assert/strict';
+import test from 'node:test';
+import { readFileSync } from 'node:fs';
+
+// An explicit save and the 1.5s auto-save debounce can both be aimed at one
+// tab. The debounce is armed on the last keystroke and disarmed by the
+// auto-save effect only once `isDirty` goes false — which happens *after* the
+// write resolves. So a timer that expires while an explicit save is still in
+// flight starts a SECOND write of the same file, and the two race to the
+// rename:
+//
+//   manual save   takes snapshot A ──┐
+//   user types (buffer becomes B)    ├── both in flight, order undefined
+//   timer fires   takes snapshot B ──┘
+//
+//   if A lands last: disk holds A, the tab recorded B as saved, isDirty is
+//   false. The buffer reads clean while the file is a revision behind, and it
+//   stays that way until the next keystroke.
+//
+// `atomic_write` was hardened separately so two concurrent writers cannot
+// corrupt the file or fail each other — but that is a safety property, not an
+// ordering one, and it cannot fix a stale winner. The ordering fix is simply
+// not to start the second write: whoever is about to save disarms the timer.
+//
+// The point of these tests is the *placement* of that duty. It used to sit at
+// the call sites, where three of six explicit-save entry points remembered it
+// and three did not; it now sits inside `saveContent`, so no entry point has
+// to know. These assert the duty is discharged there, and that no call site
+// has quietly taken it back.
+
+const g = globalThis as any;
+const runeEffect = (fn: () => void) => {
+	void fn;
+};
+runeEffect.root = (fn: () => unknown) => fn();
+g.$state = (value: unknown) => value;
+g.$state.raw = (value: unknown) => value;
+g.$state.snapshot = (value: unknown) => value;
+g.$derived = (value: unknown) => value;
+g.$derived.by = (fn: () => unknown) => fn();
+g.$effect = runeEffect;
+g.window = g.window ?? {};
+
+const localStore = new Map<string, string>();
+g.localStorage = {
+	getItem: (key: string) => (localStore.has(key) ? localStore.get(key)! : null),
+	setItem: (key: string, value: string) => void localStore.set(key, String(value)),
+	removeItem: (key: string) => void localStore.delete(key),
+	clear: () => localStore.clear(),
+};
+
+const disk = new Map<string, string>();
+/**
+ * Cancels and writes in the order they happened. One list on purpose: the
+ * question is not whether both occurred but which came first, and two counters
+ * cannot answer that.
+ */
+const events: string[] = [];
+/** What the Save As dialog returns next. `null` is the user pressing Cancel. */
+let nextSaveTarget: string | null = null;
+
+g.window.__TAURI_INTERNALS__ = {
+	metadata: { currentWindow: { label: 'main' }, currentWebview: { windowLabel: 'main', label: 'main' } },
+	invoke: (cmd: string, args: any) => {
+		if (cmd === 'canonicalize_path') return Promise.resolve(args.path);
+		if (cmd === 'read_file_content_checked') return Promise.resolve([disk.get(args.path) ?? '', false]);
+		if (cmd === 'open_markdown_preview') return Promise.resolve(['', disk.get(args.path) ?? '', true, false]);
+		if (cmd === 'save_file_content') {
+			events.push(`write:${args.path}`);
+			disk.set(args.path, args.content);
+			return Promise.resolve(null);
+		}
+		if (cmd === 'plugin:dialog|save') return Promise.resolve(nextSaveTarget);
+		if (cmd === 'get_os_type') return Promise.resolve('macos');
+		return Promise.resolve(null);
+	},
+};
+
+const { tabManager } = await import('../src/lib/stores/tabs.svelte.js');
+const { createDocumentSession } = await import('../src/lib/sessions/documentSession.svelte.js');
+
+function makeSession() {
+	return createDocumentSession({
+		setShowHome: () => {},
+		currentFile: () => tabManager.activeTab?.path ?? '',
+		resetScrollHistory: () => {},
+		renderMarkdown: async (raw: string) => raw,
+		afterLoad: async () => {},
+		saveRecentFile: () => {},
+		deleteRecentFile: () => {},
+		setLoadingTabs: () => {},
+		measureInitialViewport: () => {},
+		isScrolling: () => false,
+		renderRichContent: () => {},
+		onError: () => {},
+		selfWriteGraceMs: 400,
+		cancelPendingAutoSave: (tabId: string) => void events.push(`cancel:${tabId}`),
+		askClose: async () => 'discard' as const,
+		onCloseSaveNewerEdits: () => {},
+		onCloseAutoSaveFailed: () => {},
+	});
+}
+
+function reset() {
+	tabManager.closeAll();
+	tabManager.recentlyClosed.length = 0;
+	localStore.clear();
+	disk.clear();
+	events.length = 0;
+	nextSaveTarget = null;
+}
+
+test('an explicit save disarms the debounce before it issues the write', async () => {
+	// Order is the whole assertion. Cancelling *after* the write would leave
+	// the timer free to fire during the very await this is meant to protect —
+	// which is the race, not the fix.
+	reset();
+	const session = makeSession();
+	disk.set('/notes/a.md', 'original');
+
+	await session.loadMarkdown('/notes/a.md');
+	const tabId = tabManager.activeTabId!;
+	tabManager.updateTabRawContent(tabId, 'edited');
+
+	assert.equal(await session.saveContent(tabId), true);
+	assert.deepEqual(events, [`cancel:${tabId}`, 'write:/notes/a.md']);
+});
+
+test('a save reached without naming a tab disarms that tab too', async () => {
+	// Cmd+S and the toolbar button call `saveContent()` with no argument and
+	// let it find the active tab. That is one of the three entry points that
+	// never cancelled, so it is the one most worth pinning.
+	reset();
+	const session = makeSession();
+	disk.set('/notes/b.md', 'original');
+
+	await session.loadMarkdown('/notes/b.md');
+	const tabId = tabManager.activeTabId!;
+	tabManager.updateTabRawContent(tabId, 'edited');
+
+	assert.equal(await session.saveContent(), true);
+	assert.deepEqual(events, [`cancel:${tabId}`, 'write:/notes/b.md']);
+});
+
+test('a Save dialog the user cancels leaves the timer armed', async () => {
+	// The standing rule for `cancelPendingAutoSave`: never disarm on a path the
+	// user can still back out of. Cancelling the dialog is exactly that — the
+	// tab keeps its edits, so it must keep the writer that will flush them.
+	// (An untitled tab has no timer of its own, since the auto-save effect only
+	// arms tabs that already have a path; what this really pins is that the
+	// cancel sits after the dialog, not before it.)
+	reset();
+	const session = makeSession();
+	tabManager.addTab('');
+	const tabId = tabManager.activeTabId!;
+	tabManager.updateTabRawContent(tabId, 'unsaved draft');
+
+	nextSaveTarget = null;
+	assert.equal(await session.saveContent(tabId), false);
+	assert.deepEqual(events, [], 'no cancel, and nothing written');
+});
+
+test('no call site takes the cancel duty back from saveContent', async () => {
+	// The regression this guards is not a broken save — it is the duty
+	// drifting back out to the call sites, where the next entry point can
+	// forget it again. A cancel immediately before a `saveContent` is the
+	// shape that says someone stopped trusting the function to do it.
+	for (const path of ['src/lib/MarkdownViewer.svelte', 'src/lib/sessions/documentSession.svelte.ts']) {
+		const body = readFileSync(path, 'utf8');
+		assert.doesNotMatch(
+			body,
+			/cancelPendingAutoSave\([^)]*\);\s*(?:\/\/[^\n]*\n\s*)*(?:const \w+ = )?(?:await |return )*saveContent\(/,
+			`${path} pairs a manual cancel with a save; saveContent already does this`,
+		);
+	}
+
+	// And the duty is discharged before the write, not after — the source-level
+	// mirror of the ordering test above, so a refactor that reorders the two
+	// inside `saveContent` is caught even if the stub harness stops seeing it.
+	const session = readFileSync('src/lib/sessions/documentSession.svelte.ts', 'utf8');
+	const saveContentBody = session.slice(session.indexOf('async function saveContent('));
+	const cancel = saveContentBody.indexOf('options.cancelPendingAutoSave(');
+	const write = saveContentBody.indexOf("invoke('save_file_content'");
+	assert.ok(cancel !== -1, 'saveContent must cancel the pending auto-save');
+	assert.ok(cancel < write, 'the cancel must come before the write it protects');
+});

src-tauri/src/lib.rs

@@ -4,6 +4,7 @@ use std::borrow::Cow;
 use std::fs;
 use std::io::Write;
 use std::path::{Path, PathBuf};
+use std::sync::atomic::{AtomicU64, Ordering};
 use std::sync::LazyLock;
 use std::time::{SystemTime, UNIX_EPOCH};
 use tauri::{AppHandle, Emitter, Manager, State};
@@ -61,6 +62,11 @@ static TASK_SOURCE_RE: LazyLock<Regex> = LazyLock::new(|| {
     Regex::new(r"^\s*(?:>\s*)*(?:[-+*]|\d+[.)])\s+\[[ xX]\](?:\s|$)").unwrap()
 });
 
+/// Distinguishes the temp files of `atomic_write` calls that share a process.
+/// Never reset, and read with `fetch_add` so no two callers can be handed the
+/// same value — the property the wall clock could not supply.
+static TEMP_FILE_SEQ: AtomicU64 = AtomicU64::new(0);
+
 /// Write `bytes` to `target` durably and atomically: write to a sibling temp
 /// file, fsync it, then rename over the target. Atomic on both Unix and
 /// modern Windows — `std::fs::rename` calls `MoveFileExW` with
@@ -124,22 +130,49 @@ pub(crate) fn atomic_write(target: &Path, bytes: &[u8]) -> std::io::Result<()> {
         .file_name()
         .map(|n| n.to_string_lossy().into_owned())
         .unwrap_or_else(|| "markpad".to_string());
-    let nanos = SystemTime::now()
-        .duration_since(UNIX_EPOCH)
-        .map(|d| d.as_nanos())
-        .unwrap_or(0);
-    let pid = std::process::id();
-    let temp_name = format!(".{}.markpad-tmp-{}-{}", file_name, pid, nanos);
-    let mut temp_path = parent_path.clone();
-    temp_path.push(temp_name);
 
+    // Claim a temp file nobody else holds. The name must be unique or two
+    // concurrent writers of the same target collide, and a collision is not a
+    // harmless retry: the loser used to delete `temp_path` on its way out,
+    // which is the *winner's* file, and the winner then failed its rename with
+    // ENOENT. So uniqueness comes from a per-process counter rather than the
+    // clock — macOS timer granularity is coarser than a nanosecond, and two
+    // threads calling `SystemTime::now` back to back routinely read the same
+    // value. `create_new` still arbitrates across processes (a stale temp left
+    // by a dead process whose pid we inherited), hence the bounded retry.
+    let (mut file, temp_path) = {
+        let pid = std::process::id();
+        let mut attempts = 0;
+        loop {
+            let nanos = SystemTime::now()
+                .duration_since(UNIX_EPOCH)
+                .map(|d| d.as_nanos())
+                .unwrap_or(0);
+            let seq = TEMP_FILE_SEQ.fetch_add(1, Ordering::Relaxed);
+            let mut candidate = parent_path.clone();
+            candidate.push(format!(".{file_name}.markpad-tmp-{pid}-{nanos}-{seq}"));
+            match fs::OpenOptions::new()
+                .write(true)
+                .create_new(true)
+                .open(&candidate)
+            {
+                Ok(file) => break (file, candidate),
+                Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists && attempts < 16 => {
+                    attempts += 1;
+                }
+                // Anything else — no such directory, permission denied — will
+                // not improve on a retry, and neither will an `AlreadyExists`
+                // that survived 16 fresh names.
+                Err(e) => return Err(e),
+            }
+        }
+    };
+
+    // From here on `temp_path` is a file this call created, so cleaning it up
+    // can never touch another writer's temp file.
     let write_result = (|| -> std::io::Result<()> {
-        let mut f = fs::OpenOptions::new()
-            .write(true)
-            .create_new(true)
-            .open(&temp_path)?;
-        f.write_all(bytes)?;
-        f.sync_all()?;
+        file.write_all(bytes)?;
+        file.sync_all()?;
         Ok(())
     })();
 
@@ -3147,6 +3180,62 @@ mod tests {
         fs::remove_dir_all(dir).unwrap();
     }
 
+    #[test]
+    fn concurrent_atomic_writes_to_one_target_all_succeed() {
+        // Two threads used to derive the same temp name from the same clock
+        // reading (macOS ticks coarser than a nanosecond), and the collision
+        // took down *both* writers: the loser of `create_new` got EEXIST, and
+        // its cleanup deleted the winner's temp file, so the winner's rename
+        // failed with ENOENT. Concurrent writers are ordinary here — every
+        // `save_file`, theme write and window-state flush shares this path.
+        let dir = temp_path("atomic-concurrent");
+        fs::create_dir_all(&dir).unwrap();
+        let path = dir.join("contended.json");
+
+        const WRITERS: usize = 8;
+        let bodies: Vec<Vec<u8>> = (0..WRITERS)
+            .map(|i| format!("{{\"writer\":{i}}}").into_bytes())
+            .collect();
+
+        let failures: Vec<String> = std::thread::scope(|scope| {
+            let handles: Vec<_> = bodies
+                .iter()
+                .map(|body| {
+                    let path = path.as_path();
+                    scope.spawn(move || atomic_write(path, body).map_err(|e| e.to_string()))
+                })
+                .collect();
+            handles
+                .into_iter()
+                .filter_map(|h| h.join().unwrap().err())
+                .collect()
+        });
+        assert!(
+            failures.is_empty(),
+            "every concurrent write must succeed, got: {failures:?}",
+        );
+
+        // Last writer wins, and the winner is whole: a torn or empty file
+        // would mean the rename published a temp file some other thread had
+        // deleted or was still filling.
+        let final_bytes = fs::read(&path).unwrap();
+        assert!(
+            bodies.contains(&final_bytes),
+            "final contents are not one of the values written: {:?}",
+            String::from_utf8_lossy(&final_bytes),
+        );
+
+        let leftovers: Vec<String> = fs::read_dir(&dir)
+            .unwrap()
+            .flatten()
+            .map(|entry| entry.file_name().to_string_lossy().into_owned())
+            .filter(|name| name.contains("markpad-tmp"))
+            .collect();
+        assert!(leftovers.is_empty(), "temp files left behind: {leftovers:?}");
+
+        fs::remove_dir_all(dir).unwrap();
+    }
+
     #[test]
     fn every_read_path_decodes_legacy_encodings_leniently() {
         // "中文" in GBK. `read_to_string` rejects the whole document on the

src/lib/MarkdownViewer.svelte

@@ -190,6 +190,12 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu
 	// because if the user picks Cancel, the timer is gone forever and
 	// background auto-save is silently disabled for that tab until the
 	// next keystroke.
+	//
+	// The *save* half is no longer a call-site duty: `saveContent` cancels the
+	// tab's timer itself, past the point where it can still bail out. Only the
+	// discard path still calls this directly, because nothing saves on its
+	// behalf. Do not re-add a cancel before a `saveContent` — it is redundant,
+	// and reintroduces the question of whether every new entry point remembered.
 	function cancelPendingAutoSave(tabId: string) {
 		const t = autoSaveTimers.get(tabId);
 		if (t) {
@@ -1605,7 +1611,6 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu
 		// disables this flush too.
 		if (!settings.autoSave || settings.confirmBeforeSave) return;
 
-		cancelPendingAutoSave(tab.id);
 		const success = await saveContent(tab.id);
 		if (!success) {
 			// Reported, not obeyed. A file that cannot be written — read-only
@@ -2984,11 +2989,11 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu
 							// dirty tab that has a real path. Untitled tabs need a
 							// Save dialog, so the walk below handles them. A failed
 							// silent save is surfaced and its tab also goes to the
-							// walk. Timers are cancelled per tab right before its
-							// save to avoid duplicate writes.
+							// walk. `saveContent` cancels each tab's pending timer
+							// itself, so no writer here can be raced by its own
+							// debounce.
 							if (settings.autoSave && !settings.confirmBeforeSave) {
 								for (const tab of dirtyTabs.filter((t) => t.path !== '')) {
-									cancelPendingAutoSave(tab.id);
 									const ok = await saveContent(tab.id);
 									if (!ok) {
 										addToast(t('toast.autoSaveFailed', settings.language), 'error');