From 582f26824b9a1f8e842e3e246a956d7b85c68cac Mon Sep 17 00:00:00 2001 From: PathGao Date: Mon, 3 Aug 2026 17:34:22 +0800 Subject: [PATCH 1/2] fix(save): make atomic_write's temp name collision-proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The temp file was named from the target name, the pid and a nanosecond clock reading. macOS ticks coarser than a nanosecond, so two threads of one process writing the same target routinely derive the same name — and the collision took down both writers, not just the loser: the loser of `create_new` ran `fs::remove_file(&temp_path)` on a path it had never created, deleting the file the winner was about to rename, so the winner then failed with ENOENT. Uniqueness now comes from a process-wide atomic counter, which no two callers can be handed the same value from, rather than from a clock that cannot supply it. `create_new` still arbitrates across processes (a stale temp left by a dead process whose pid we inherited), so an AlreadyExists retries with a fresh name. The file handle is acquired before `temp_path` exists as a binding, so no cleanup can reach a file this call did not create. Reported in #424, which fixed the pinned-tags exposure by serialising that file's read-modify-write cycle and left the underlying weakness to `atomic_write`. Every other caller was still exposed: save_file, save_file_binary, save_theme, the VSIX theme install, save_window_state and the image-drop path. The new test runs 8 threads against one target and asserts every call returns Ok and the surviving file is one of the values written. Against the old naming it fails 20 times in 40 runs; against this, 0 in 40. Co-Authored-By: Claude Opus 5 --- src-tauri/src/lib.rs | 117 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 103 insertions(+), 14 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 53da47b..944b6a8 100644 --- a/src-tauri/src/lib.rs +++ b/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 = 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> = (0..WRITERS) + .map(|i| format!("{{\"writer\":{i}}}").into_bytes()) + .collect(); + + let failures: Vec = 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 = 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 From e87c373dc95b1861a17451fb65a77f879e584005 Mon Sep 17 00:00:00 2001 From: PathGao Date: Mon, 3 Aug 2026 17:34:34 +0800 Subject: [PATCH 2/2] fix(save): let saveContent disarm the debounce that races it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An explicit save and the 1.5s auto-save timer can be aimed at one tab. The timer 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 expiring while an explicit save is in flight starts a second write of the same file, and the two race to the rename. `atomic_write` was hardened separately so concurrent writers cannot corrupt the file or fail each other, but that is a safety property, not an ordering one. If the older snapshot lands last, the disk holds the earlier text while the tab records the newer one as saved: the buffer reads clean and stays a revision behind until the next keystroke. `cancelPendingAutoSave` already existed for exactly this, but was a call-site duty, and three of the six explicit-save entry points did not discharge it — Ctrl+S, the toolbar, and the preview task checkbox. It moves into `saveContent`, past the Save dialog so it never disarms a tab on a path the user can still cancel, and the four now-redundant call-site copies are removed. The discard branch of `canCloseTab` keeps its own, but as scope rather than as necessity: no `saveContent` is on that path to do it, and while the auto-save effect would drop the timer anyway once `isDirty` goes false three lines later, that route rests on effect flush ordering and on the effect running at all during teardown. A synchronous cancel rests on neither. Left alone because this change is about the save paths. Tests assert the ordering (cancel before write, since cancelling after would leave the timer free to fire during the very await it protects) and that no call site takes the duty back. Removing the cancel fails 3 of the 4; re-adding a call-site cancel fails the guard. Co-Authored-By: Claude Opus 5 --- scripts/explicitSaveCancelsAutoSave.test.ts | 186 ++++++++++++++++++++ src/lib/MarkdownViewer.svelte | 13 +- src/lib/sessions/documentSession.svelte.ts | 25 ++- 3 files changed, 218 insertions(+), 6 deletions(-) create mode 100644 scripts/explicitSaveCancelsAutoSave.test.ts diff --git a/scripts/explicitSaveCancelsAutoSave.test.ts b/scripts/explicitSaveCancelsAutoSave.test.ts new file mode 100644 index 0000000..f4881b1 --- /dev/null +++ b/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(); +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(); +/** + * 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'); +}); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 97b5865..9223ba7 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/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'); diff --git a/src/lib/sessions/documentSession.svelte.ts b/src/lib/sessions/documentSession.svelte.ts index 57f2983..53dfc8b 100644 --- a/src/lib/sessions/documentSession.svelte.ts +++ b/src/lib/sessions/documentSession.svelte.ts @@ -419,6 +419,21 @@ export function createDocumentSession(options: DocumentSessionOptions) { targetPath = selected; targetKey = await canonicalizePath(selected); } + // The pending debounce exists to write a tab that nobody is writing. + // This call is that writer, so the timer has nothing left to do — and + // leaving it armed is not merely redundant: it can fire *during* the + // await below and put a second write on the same file, racing this one + // to the rename. The loser publishes the older snapshot while the tab + // records the newer one as saved, so the buffer reads clean while the + // disk holds the earlier text. + // + // It belongs here rather than at the call sites: two of the five + // explicit saves remembered to cancel and three did not, and a sixth + // entry point would have had to remember too. Placed after the Save + // dialog above, so it never runs before a modal the user can still + // cancel — an untitled tab has no timer to cancel anyway, since the + // auto-save effect only arms tabs that already have a path. + options.cancelPendingAutoSave(tab.id); if (refuseIfLossilyDecoded(tab, targetPath, targetKey)) return false; const snapshot = tab.rawContent; markSelfWrite(targetPath); @@ -507,7 +522,6 @@ export function createDocumentSession(options: DocumentSessionOptions) { if (!tab || (!tab.isDirty && tab.path !== '')) return true; if (!tab.isDirty) return true; if (settings.autoSave && !settings.confirmBeforeSave && tab.path !== '') { - options.cancelPendingAutoSave(tabId); const success = await saveContent(tabId); if (success && !tab.isDirty) return true; if (success) options.onCloseSaveNewerEdits(); @@ -516,9 +530,16 @@ export function createDocumentSession(options: DocumentSessionOptions) { const response = await options.askClose(tab.title); if (response === 'cancel') return false; if (response === 'save') { - options.cancelPendingAutoSave(tabId); return saveContent(tabId); } + // Kept, but not because the timer would otherwise fire: the three lines + // below are synchronous, so nothing can run between them, and the + // auto-save effect drops the timer on its own once `isDirty` is false. + // That route rests on the effect flushing ahead of a pending macrotask + // and on the effect running at all — neither is obvious while a window + // is tearing down on the quit path. A synchronous cancel rests on + // neither, and unlike the branches above there is no `saveContent` here + // to do it on this path's behalf. options.cancelPendingAutoSave(tabId); tab.rawContent = tab.originalContent; tab.isDirty = false;