From 2384a91afae1654515c263ffed3347e076845aaa Mon Sep 17 00:00:00 2001 From: PathGao Date: Mon, 3 Aug 2026 18:37:15 +0800 Subject: [PATCH] fix(save): let one write per tab be in flight at a time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #436 closed one direction of the save race: `saveContent` now disarms the auto-save debounce itself, so an *armed* timer can no longer fire during an explicit save. The other direction stayed open, and #436's own "Not covered" named it. The auto-save timer callback deletes itself from `autoSaveTimers` as its first statement and only then calls `saveContent`. Once the timer has fired there is nothing left for `cancelPendingAutoSave` to cancel, and `saveContent` had no in-flight guard — so a Cmd+S arriving while the auto-save write is still awaiting `invoke('save_file_content')` started a second concurrent write of the same file. `atomic_write` (also #436) makes that safe: neither writer can corrupt the file or fail the other. But safety is not ordering, and the older snapshot could still rename last. The severity is narrower than #436's, and that is worth saying plainly. Measured against the real `documentSession`, the losing continuation also runs last and sets `originalContent` to the older snapshot, so `isDirty` goes true: the buffer is 'B', the disk is 'A', and the dirty dot honestly reports the disagreement. The flag is accurate rather than spurious, and the auto-save effect re-arms off that same flag and rewrites 'B' about 1.5s later. No silent loss. With one exception, which is the reason to fix this at all. On the close path there is no tab left to carry the flag: with auto-save on and confirm off, `canCloseTab` saves, observes `isDirty` false, and closes; the older write then renames on top of it and nothing survives to say so. The user's last edits are gone and the close looked clean. `writeExclusively` chains a tab's writes so the second caller waits for the first and takes its snapshot on the far side of that wait. Waiting rather than returning the in-flight promise, because the second caller pressed Cmd+S *after* typing more: handing back the running write would report success for a file that does not contain those keystrokes. Keyed by tab rather than by path — the corrupted state is the tab's own, the two racers are by construction one tab's, and an untitled tab has no path to key on until its dialog closes. A chain rather than a bare `await inFlight`, because several callers awaiting one promise all wake together and then all write at once. `saveContentAs` shares the guard: its target is usually another file, but both continuations write the same tab's bookkeeping, and picking the tab's own file in the dialog is an allowed overwrite. Serialising also repairs `markSelfWrite`, whose failure mode under overlap was the more interesting consequence. Overlapping successes only push the suppression deadline further out, so the watcher stays correctly quiet. But `clearSelfWrite` in the catch deletes the entry unconditionally, so one write failing while the other succeeded erased the guard the successful write had just installed — measured: our own write then read as an external change and `resolveExternalChange` returned `reload`. Ordered, the failing write's catch can no longer reach a later write's guard. Five tests, all red without the guard and green with it, including the close-path case. Hoisting the snapshot back outside the wait fails only the test that pins snapshot placement. Co-Authored-By: Claude Opus 5 --- scripts/oneWritePerTabInFlight.test.ts | 272 +++++++++++++++++++++ src/lib/sessions/documentSession.svelte.ts | 129 +++++++--- 2 files changed, 368 insertions(+), 33 deletions(-) create mode 100644 scripts/oneWritePerTabInFlight.test.ts diff --git a/scripts/oneWritePerTabInFlight.test.ts b/scripts/oneWritePerTabInFlight.test.ts new file mode 100644 index 0000000..605c25c --- /dev/null +++ b/scripts/oneWritePerTabInFlight.test.ts @@ -0,0 +1,272 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +// #436 closed one direction of the save race: `saveContent` now disarms the +// auto-save debounce itself, so an *armed* timer can no longer fire during an +// explicit save. +// +// The other direction stays open. The timer callback deletes itself from +// `autoSaveTimers` as its FIRST statement and only then calls `saveContent`, so +// once the timer has fired there is nothing left for `cancelPendingAutoSave` to +// cancel — and `saveContent` has no in-flight guard. A Cmd+S landing while the +// auto-save write is still awaiting `invoke('save_file_content')` starts a +// second concurrent write: +// +// auto-save snapshot A ──┐ +// user types (buffer is B) ├── both in flight, both racing to the rename +// Cmd+S snapshot B ──┘ +// +// `atomic_write` (also #436) makes that safe — neither writer can corrupt the +// file or fail the other — but safety is not ordering. If A's rename lands +// last the disk holds A while the user's buffer holds B. +// +// These tests pin the ordering property `atomic_write` cannot supply: at most +// one write per tab is ever in flight. Not "one write" — the second caller +// still writes, because it is saving edits the first one never saw — but it +// writes *after*, from a snapshot taken *after*, so the last rename is the +// newest text. + +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(); +/** Writes that have started but not yet reached their rename. */ +let unsettledWrites = 0; +/** + * Set the moment a write starts while another is still unsettled. That is the + * whole defect: two renames aimed at one file with no ordering between them. + */ +let sawOverlap = false; +/** + * How long each successive write takes to reach its rename, in ms, in call + * order. Giving the FIRST write the longer delay is what makes the race + * observable rather than merely possible: unguarded, the second write renames + * first and the first write's older snapshot lands on top of it. + */ +let writeDurationsMs: number[] = []; +let writeCount = 0; +/** What the Save/Save As dialog returns next. `null` is the user cancelling. */ +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') { + const duration = writeDurationsMs[writeCount] ?? 0; + writeCount += 1; + if (unsettledWrites > 0) sawOverlap = true; + unsettledWrites += 1; + return new Promise((resolve) => { + setTimeout(() => { + // The rename. `invoke` resolves after it, which is why + // resolution order tracks rename order. + disk.set(args.path, args.content); + unsettledWrites -= 1; + resolve(null); + }, duration); + }); + } + 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 { settings } = await import('../src/lib/stores/settings.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: () => {}, + askClose: async () => 'discard' as const, + onCloseSaveNewerEdits: () => {}, + onCloseAutoSaveFailed: () => {}, + }); +} + +function reset() { + tabManager.closeAll(); + tabManager.recentlyClosed.length = 0; + localStore.clear(); + disk.clear(); + unsettledWrites = 0; + sawOverlap = false; + writeDurationsMs = []; + writeCount = 0; + nextSaveTarget = null; +} + +/** Open `path` in a tab and leave the buffer holding `edited`. */ +async function openDirtyTab(session: ReturnType, path: string, edited: string) { + disk.set(path, 'original'); + await session.loadMarkdown(path); + const tabId = tabManager.activeTabId!; + tabManager.updateTabRawContent(tabId, edited); + return tabId; +} + +test('a second save waits for the write already in flight on that tab', async () => { + reset(); + const session = makeSession(); + const tabId = await openDirtyTab(session, '/notes/a.md', 'A'); + + // The first write is the slow one. Unguarded, the second overtakes it and + // the first lands last, publishing 'A' over 'B'. + writeDurationsMs = [30, 5]; + + // The auto-save timer has already fired, so its `saveContent` is running + // and there is no timer left to cancel. `saveContent` reaches its `invoke` + // synchronously for a tab that already has a path, so this write is in + // flight by the time the call returns its promise. + const autoSave = session.saveContent(tabId); + // The user types on. + tabManager.updateTabRawContent(tabId, 'B'); + // The user presses Cmd+S. + const explicit = session.saveContent(tabId); + + assert.deepEqual(await Promise.all([autoSave, explicit]), [true, true]); + + assert.equal(sawOverlap, false, 'two writes to one file were in flight at once'); + assert.equal(disk.get('/notes/a.md'), 'B', 'the newest text must be what survives on disk'); + const tab = tabManager.tabs.find((item) => item.id === tabId)!; + assert.equal(tab.isDirty, false, 'the buffer matches the disk, so the tab must read clean'); +}); + +test('the waiting save writes the buffer as it is after the wait, not before', async () => { + // Waiting is only half of it. A guard that returned the in-flight promise + // instead would report success for a write that predates the keystrokes the + // user pressed Cmd+S to protect. The snapshot has to be taken on the far + // side of the wait. + reset(); + const session = makeSession(); + const tabId = await openDirtyTab(session, '/notes/b.md', 'A'); + writeDurationsMs = [30, 5]; + + const autoSave = session.saveContent(tabId); + tabManager.updateTabRawContent(tabId, 'B'); + const explicit = session.saveContent(tabId); + // Still typing while the first write drains — this is the text the second + // save must publish. + tabManager.updateTabRawContent(tabId, 'C'); + + await Promise.all([autoSave, explicit]); + + assert.equal(sawOverlap, false); + assert.equal(disk.get('/notes/b.md'), 'C'); + assert.equal(tabManager.tabs.find((item) => item.id === tabId)!.isDirty, false); +}); + +test('three saves stacked on one tab still produce one write at a time', async () => { + // The reason the guard is a chain and not a bare `await inFlight`: several + // callers awaiting the same promise all wake together and then all write + // concurrently, which is the bug again with extra steps. + reset(); + const session = makeSession(); + const tabId = await openDirtyTab(session, '/notes/c.md', 'A'); + writeDurationsMs = [30, 20, 10]; + + const first = session.saveContent(tabId); + tabManager.updateTabRawContent(tabId, 'B'); + const second = session.saveContent(tabId); + tabManager.updateTabRawContent(tabId, 'C'); + const third = session.saveContent(tabId); + + await Promise.all([first, second, third]); + + assert.equal(sawOverlap, false); + assert.equal(writeCount, 3, 'each caller still writes; they are ordered, not dropped'); + assert.equal(disk.get('/notes/c.md'), 'C'); +}); + +test('saveContentAs waits behind the same tab guard', async () => { + // `saveContentAs` writes too, and it mutates the same per-tab bookkeeping + // (`originalContent`, `isDirty`, and the tab's path). Its target is usually + // a different file, so the renames do not collide — but the two + // continuations still race for the tab's dirty flag, and picking the tab's + // own file in the dialog is an allowed overwrite that collides outright. + reset(); + const session = makeSession(); + const tabId = await openDirtyTab(session, '/notes/d.md', 'A'); + writeDurationsMs = [30, 5]; + + const autoSave = session.saveContent(tabId); + tabManager.updateTabRawContent(tabId, 'B'); + nextSaveTarget = '/notes/copy.md'; + const saveAs = session.saveContentAs(); + + assert.deepEqual(await Promise.all([autoSave, saveAs]), [true, true]); + + assert.equal(sawOverlap, false); + assert.equal(disk.get('/notes/copy.md'), 'B', 'the copy must hold the newest text'); + const tab = tabManager.tabs.find((item) => item.id === tabId)!; + assert.equal(tab.path, '/notes/copy.md'); + assert.equal(tab.isDirty, false, 'the older write must not leave the moved tab reading dirty'); +}); + +test('closing a tab mid-write does not leave the older snapshot on disk', async () => { + // The one path on which this race is SILENT, and the reason it is worth + // fixing at all. + // + // Everywhere else the tab survives to tell the user: the older write's + // continuation runs last and sets `originalContent` to the older snapshot, + // so `isDirty` goes true and the dirty dot honestly reports that the buffer + // and the disk disagree. Here the tab is gone before that can happen. + // `canCloseTab` saves, sees `isDirty` false, and closes; the older write + // then renames on top, and there is no longer a tab to raise a flag on. + reset(); + settings.autoSave = true; + settings.confirmBeforeSave = false; + const session = makeSession(); + const tabId = await openDirtyTab(session, '/notes/e.md', 'A'); + writeDurationsMs = [30, 5]; + + // The auto-save timer has fired; its write is in flight and past cancelling. + const autoSave = session.saveContent(tabId); + // The user types, then closes the tab. With auto-save on and no confirm + // prompt, `canCloseTab` saves silently and reports the tab closable. + tabManager.updateTabRawContent(tabId, 'B'); + assert.equal(await session.canCloseTab(tabId), true); + tabManager.closeTab(tabId); + + // The close already looked clean to the user. What matters is what the + // still-draining write does after the tab is gone. + await autoSave; + + assert.equal(sawOverlap, false); + assert.equal(disk.get('/notes/e.md'), 'B', 'the closed tab’s newest text must be what is left on disk'); +}); diff --git a/src/lib/sessions/documentSession.svelte.ts b/src/lib/sessions/documentSession.svelte.ts index 53dfc8b..0dfbe1b 100644 --- a/src/lib/sessions/documentSession.svelte.ts +++ b/src/lib/sessions/documentSession.svelte.ts @@ -82,6 +82,58 @@ export function createDocumentSession(options: DocumentSessionOptions) { selfWriteUntilByPath.delete(path); } + /** + * Per tab: a promise that settles once every save queued for it so far has + * finished. Absent means nothing is in flight. + */ + const writeChainByTab = new Map>(); + + /** + * Run `write` only once the tab's previous write has settled. + * + * Two saves can be aimed at one tab at the same instant. #436 stopped an + * *armed* auto-save debounce from firing during an explicit save, but the + * timer callback drops itself from the timer map as its first statement and + * only then calls `saveContent` — so a debounce that has ALREADY fired is + * past cancelling, and its write is free to race a Cmd+S that arrives while + * it awaits. `atomic_write` (also #436) makes two concurrent writers safe + * for the file, but that is a safety property, not an ordering one: the + * older snapshot can still rename last and publish itself over the newer. + * + * Waiting, rather than handing the in-flight promise back to the second + * caller — those are different answers. The second caller pressed Cmd+S + * *after* typing more, so returning the running write would report success + * for a file that does not contain those keystrokes. Waiting costs one disk + * write and ends with the text the user actually asked to save on disk. + * + * Keyed by tab rather than by path: the state these writes corrupt is the + * tab's own (`originalContent`, `isDirty`, its path), the two racers are by + * construction one tab's, and an untitled tab has no path to key on until + * its Save dialog closes. Two tabs pointing at one file still write + * concurrently — that is last-writer-wins between two documents, which this + * app permits by allowing the second tab at all, and is not this race. + * + * A chain and not a bare `await inFlight`, because several callers awaiting + * one promise all wake together and then all write at once — the race again. + */ + function writeExclusively(tabId: string, write: () => Promise): Promise { + const previous = writeChainByTab.get(tabId); + const result = previous ? previous.then(write) : write(); + // Outcome-free by design: a save that fails must not strand the queue + // behind it, and nothing here is the owner of that rejection. + const settled = result.then( + () => {}, + () => {}, + ); + writeChainByTab.set(tabId, settled); + void settled.then(() => { + // Only the tail clears the entry. An earlier link finishing must not + // drop a chain that later callers are still queued behind. + if (writeChainByTab.get(tabId) === settled) writeChainByTab.delete(tabId); + }); + return result; + } + function shouldReloadExternalChange(path: string) { const until = selfWriteUntilByPath.get(path); if (until === undefined) return true; @@ -435,23 +487,27 @@ export function createDocumentSession(options: DocumentSessionOptions) { // 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); - try { - await invoke('save_file_content', { path: targetPath, content: snapshot }); + return writeExclusively(tab.id, async () => { + // Snapshot taken on the far side of the wait. Anything typed while + // the previous write drained is part of what this save is for. + const snapshot = tab.rawContent; markSelfWrite(targetPath); - if (tab.path === '') { - tabManager.updateTabPath(tab.id, targetPath, targetKey); - options.saveRecentFile(targetPath); + try { + await invoke('save_file_content', { path: targetPath, content: snapshot }); + markSelfWrite(targetPath); + if (tab.path === '') { + tabManager.updateTabPath(tab.id, targetPath, targetKey); + options.saveRecentFile(targetPath); + } + tab.originalContent = snapshot; + tab.isDirty = tab.rawContent !== snapshot; + return true; + } catch (error) { + clearSelfWrite(targetPath); + options.onError('Failed to save file', error); + return false; } - tab.originalContent = snapshot; - tab.isDirty = tab.rawContent !== snapshot; - return true; - } catch (error) { - clearSelfWrite(targetPath); - options.onError('Failed to save file', error); - return false; - } + }); } async function saveContentAs(): Promise { @@ -476,25 +532,32 @@ export function createDocumentSession(options: DocumentSessionOptions) { // which land on the very same file. const selectedKey = await canonicalizePath(selected); if (refuseIfLossilyDecoded(tab, selected, selectedKey)) return false; - const snapshot = tab.rawContent; - markSelfWrite(selected); - try { - await invoke('save_file_content', { path: selected, content: snapshot }); + // Shares the tab's guard with `saveContent`. The target is usually a + // different file, so the renames need not collide — but both + // continuations write the same tab's `originalContent`, `isDirty` and + // path, and picking the tab's own file in the dialog is an allowed + // overwrite that collides outright. + return writeExclusively(tab.id, async () => { + const snapshot = tab.rawContent; markSelfWrite(selected); - tabManager.updateTabPath(tab.id, selected, selectedKey); - // The buffer now has a UTF-8 file of its own that it matches - // exactly, so it is safe to save from here on. - tabManager.setTabDecodedLossy(tab.id, false); - lossySaveWarnedTabs.delete(tab.id); - options.saveRecentFile(selected); - tab.originalContent = snapshot; - tab.isDirty = tab.rawContent !== snapshot; - return true; - } catch (error) { - clearSelfWrite(selected); - options.onError('Failed to save file as', error); - return false; - } + try { + await invoke('save_file_content', { path: selected, content: snapshot }); + markSelfWrite(selected); + tabManager.updateTabPath(tab.id, selected, selectedKey); + // The buffer now has a UTF-8 file of its own that it matches + // exactly, so it is safe to save from here on. + tabManager.setTabDecodedLossy(tab.id, false); + lossySaveWarnedTabs.delete(tab.id); + options.saveRecentFile(selected); + tab.originalContent = snapshot; + tab.isDirty = tab.rawContent !== snapshot; + return true; + } catch (error) { + clearSelfWrite(selected); + options.onError('Failed to save file as', error); + return false; + } + }); } async function toggleTaskCheckbox(sourceLine: number, nowChecked: boolean) {