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) {