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-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 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;