From 5c2439a2a89d5274b13b5909a18e57117b6fe565 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Thu, 15 Oct 2026 07:04:05 +0200 Subject: [PATCH 1/2] test(sync): reproduce a replacement during save readback --- src/main.persistence.test.ts | 28 ++++++++++ tests/e2e/sync-save-readback.test.ts | 83 ++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 tests/e2e/sync-save-readback.test.ts diff --git a/src/main.persistence.test.ts b/src/main.persistence.test.ts index f8ad1489..2ab0bd09 100644 --- a/src/main.persistence.test.ts +++ b/src/main.persistence.test.ts @@ -69,6 +69,34 @@ function makePlugin( const settled = () => new Promise((resolve) => setTimeout(resolve, 0)); +it("merges a valid readback replacement without a failure Notice and writes once more", async () => { + const notice = vi.spyOn(obsidian, "Notice"); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const plugin = makePlugin(); + const clock = manualClock(); + attachDataFileWriter(plugin, clock); + const external = encodePodNotesData(structuredClone(DEFAULT_SETTINGS)); + external.savedFeeds = { + Synced: { title: "Synced", url: "https://example.com/synced.xml", artworkUrl: "" }, + }; + const write = plugin.saveData.bind(plugin); + const save = vi.fn(write).mockImplementationOnce(async () => write(external)); + Object.assign(plugin, { saveData: save }); + plugin.settings.skipForwardLength = 25; + + await saveInBackground(plugin); + await plugin.flushSaves(); + + expect(notice).not.toHaveBeenCalled(); + expect(await plugin.loadData()).toMatchObject({ + skipForwardLength: 25, + savedFeeds: external.savedFeeds, + }); + expect(plugin.settings.savedFeeds).toEqual(external.savedFeeds); + expect(save).toHaveBeenCalledTimes(2); + expect(clock.pending()).toBe(0); +}); + function savedByThisVersion(released: Record): Record { const data: Record = { ...structuredClone(released), diff --git a/tests/e2e/sync-save-readback.test.ts b/tests/e2e/sync-save-readback.test.ts new file mode 100644 index 00000000..e2e40034 --- /dev/null +++ b/tests/e2e/sync-save-readback.test.ts @@ -0,0 +1,83 @@ +import { mkdir } from "node:fs/promises"; +import { resolve } from "node:path"; +import { expect, onTestFinished, test } from "vitest"; +import { createPodNotesE2EHarness, type PodNotesData } from "./harness"; +import { closeSettings, openPodNotesSettings } from "./journey/actions"; +import { typeInto } from "./journey/input"; +import { ui } from "./journey/targets"; +import { jsLiteral, RELOAD_OPTIONS, WAIT_OPTS } from "./runtime"; + +const getContext = createPodNotesE2EHarness("sync-save-readback"); + +test("merges a sync replacement during a settings save without a failure Notice", async () => { + const { obsidian, plugin } = getContext(); + await plugin.updateDataAndReload((data) => { + data.skipForwardLength = 15; + data.savedFeeds = {}; + }, RELOAD_OPTIONS); + await openPodNotesSettings(obsidian); + const external = await plugin.data().read(); + external.savedFeeds = { + Synced: { title: "Synced", url: "https://example.com/synced.xml", artworkUrl: "" }, + }; + const dataPath = await plugin.dataPath(); + await obsidian.dev.evalJson(`(() => { + const adapter = app.vault.adapter; + const original = adapter.applyWriteOptions; + const probe = { writes: 0, injected: false, notices: [] }; + const seen = new WeakSet(document.querySelectorAll(".notice")); + const observer = new MutationObserver(() => { + for (const el of document.querySelectorAll(".notice")) { + if (!seen.has(el)) { seen.add(el); probe.notices.push(el.textContent); } + } + }); + observer.observe(document.body, { childList: true, subtree: true }); + adapter.applyWriteOptions = async function(file, options) { + if (file === ${jsLiteral(dataPath)}) { + probe.writes++; + if (!probe.injected) { + require("fs").writeFileSync(file, ${jsLiteral(JSON.stringify(external))}); + probe.injected = true; + } + } + return original.call(this, file, options); + }; + probe.restore = () => { adapter.applyWriteOptions = original; observer.disconnect(); }; + window.__syncSaveReadback = probe; + return true; + })()`); + onTestFinished(async () => { + await obsidian.dev.evalJson( + "window.__syncSaveReadback?.restore(), delete window.__syncSaveReadback, true", + ); + }); + await typeInto(obsidian, ui.settingControl("Skip forward (s)", "input"), "25", { + replace: true, + }); + const disk = await plugin.waitForData( + (data) => data.skipForwardLength === 25 && Boolean(data.savedFeeds?.Synced), + WAIT_OPTS, + ); + await closeSettings(obsidian); + await obsidian.dev.evalJsonAsync( + "(async () => { await app.plugins.plugins.podnotes.flushSaves(); return true; })()", + ); + const result = await obsidian.dev.evalJson<{ + injected: boolean; + writes: number; + notices: string[]; + }>( + "({ injected: window.__syncSaveReadback.injected, writes: window.__syncSaveReadback.writes, notices: window.__syncSaveReadback.notices })", + ); + console.info(JSON.stringify({ ...result, disk, version: await obsidian.exec("version") })); + const directory = resolve(".obsidian-e2e-artifacts/sync-save-readback"); + await mkdir(directory, { recursive: true }); + await obsidian.dev.screenshot(resolve(directory, "after-save.png")); + expect(result.injected).toBe(true); + expect(disk.savedFeeds?.Synced).toEqual(external.savedFeeds.Synced); + expect( + await obsidian.dev.evalJson("app.plugins.plugins.podnotes.settings.skipForwardLength"), + ).toBe(25); + expect(result.writes).toBe(2); + expect(result.notices).not.toContain("PodNotes could not save your changes."); +}); From 3acdd4f744b44729e37495c1c6b01face617d437 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Thu, 15 Oct 2026 09:32:01 +0200 Subject: [PATCH 2/2] fix(sync): adopt valid replacements during save readback --- src/main.persistence.test.ts | 214 +++++++++++++++++++++++++++ src/main.ts | 193 +++++++++++++----------- src/persistence/writer.test.ts | 121 ++++++++++++++- src/persistence/writer.ts | 103 ++++++++----- tests/e2e/settings-tab.test.ts | 3 + tests/e2e/sync-save-readback.test.ts | 92 +++++++++--- 6 files changed, 585 insertions(+), 141 deletions(-) diff --git a/src/main.persistence.test.ts b/src/main.persistence.test.ts index 2ab0bd09..849ff18c 100644 --- a/src/main.persistence.test.ts +++ b/src/main.persistence.test.ts @@ -97,6 +97,166 @@ it("merges a valid readback replacement without a failure Notice and writes once expect(clock.pending()).toBe(0); }); +it("writes nothing more when the file that replaced a save holds it as written", async () => { + const plugin = makePlugin(); + const write = plugin.saveData.bind(plugin); + const save = vi.fn(write).mockImplementationOnce(async (data) => { + const synced = JSON.parse(JSON.stringify(data)); + synced.savedFeeds = { + Synced: { title: "Synced", url: "https://example.com/synced.xml", artworkUrl: "" }, + }; + await write(synced); + }); + Object.assign(plugin, { saveData: save }); + plugin.settings.skipForwardLength = 25; + + await plugin.saveSettingsStrict(); + + expect(save).toHaveBeenCalledTimes(1); + expect(Object.keys(plugin.settings.savedFeeds)).toEqual(["Synced"]); +}); + +describe("when a synced private feed address is in data.json during a save", () => { + const rawUrl = "https://private.example/feed.xml?auth=SYNCED_SENTINEL"; + + async function loaded() { + const plugin = makePlugin(); + await plugin.loadSettings(); + const clock = manualClock(); + attachDataFileWriter(plugin, clock); + return { plugin, clock, write: plugin.saveData.bind(plugin) }; + } + + function withPrivateFeed(file: unknown) { + const synced = JSON.parse(JSON.stringify(file)); + synced.savedFeeds = { Private: { title: "Private", url: rawUrl, artworkUrl: "" } }; + return synced; + } + + it("fails a write that left the file as it was once, and retries it later", async () => { + const notice = vi.spyOn(obsidian, "Notice"); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const { plugin, clock, write } = await loaded(); + await write(withPrivateFeed(await plugin.loadData())); + let failing = true; + const save = vi.fn(async (data: unknown) => { + if (!failing) return write(data); + // Obsidian's saveData resolves when its write fails. A save that keeps + // writing stops here. + if (save.mock.calls.length > 3) throw new Error("PodNotes kept writing"); + }); + Object.assign(plugin, { saveData: save }); + plugin.settings.defaultVolume = 0.4; + + await saveInBackground(plugin); + expect(notice.mock.calls).toEqual([["PodNotes could not save your changes."]]); + expect(save).toHaveBeenCalledTimes(1); + expect(clock.delays).toEqual([10_000]); + + failing = false; + clock.advance(10_000); + clock.runAll(); + await settled(); + + const disk = await plugin.loadData(); + expect(disk).toMatchObject({ + defaultVolume: 0.4, + savedFeeds: { Private: { url: "podnotes-private-feed:Private" } }, + }); + expect(resolveFeedUrl(disk.savedFeeds.Private, plugin.feedUrls)).toBe(rawUrl); + expect(notice).toHaveBeenCalledTimes(1); + expect(save).toHaveBeenCalledTimes(2); + }); + + it("writes the file that replaced a save protected, even when it holds every edit", async () => { + const notice = vi.spyOn(obsidian, "Notice"); + const { plugin, write } = await loaded(); + const save = vi + .fn(write) + .mockImplementationOnce(async (data) => write(withPrivateFeed(data))); + Object.assign(plugin, { saveData: save }); + plugin.settings.defaultVolume = 0.4; + + await plugin.saveSettingsStrict(); + + const disk = await plugin.loadData(); + expect(JSON.stringify(disk)).not.toContain("SYNCED_SENTINEL"); + expect(disk).toMatchObject({ + defaultVolume: 0.4, + savedFeeds: { Private: { url: "podnotes-private-feed:Private" } }, + }); + expect(resolveFeedUrl(disk.savedFeeds.Private, plugin.feedUrls)).toBe(rawUrl); + expect(resolveFeedUrl(plugin.settings.savedFeeds.Private, plugin.feedUrls)).toBe(rawUrl); + expect(save).toHaveBeenCalledTimes(2); + expect(notice).not.toHaveBeenCalled(); + }); + + it("writes the earlier file protected again when only an address copied into it is raw", async () => { + const plugin = makePlugin(); + const write = plugin.saveData.bind(plugin); + const copied = (address: string) => `copied from ${address} on another device`; + await write({ ...withPrivateFeed(await plugin.loadData()), copiedNote: copied(rawUrl) }); + await plugin.loadSettings(); + const clock = manualClock(); + attachDataFileWriter(plugin, clock); + const earlier = await plugin.loadData(); + expect(earlier.copiedNote).toBe(copied("podnotes-private-feed:Private")); + const save = vi + .fn(write) + .mockImplementationOnce(async () => write({ ...earlier, copiedNote: copied(rawUrl) })); + Object.assign(plugin, { saveData: save }); + const notice = vi.spyOn(obsidian, "Notice"); + plugin.settings.defaultVolume = 0.4; + + await saveInBackground(plugin); + await plugin.flushSaves(); + + expect(notice).not.toHaveBeenCalled(); + const disk = await plugin.loadData(); + expect(JSON.stringify(disk)).not.toContain("SYNCED_SENTINEL"); + expect(disk).toMatchObject({ + defaultVolume: 0.4, + copiedNote: copied("podnotes-private-feed:Private"), + savedFeeds: { Private: { url: "podnotes-private-feed:Private" } }, + }); + expect(resolveFeedUrl(disk.savedFeeds.Private, plugin.feedUrls)).toBe(rawUrl); + expect(save).toHaveBeenCalledTimes(2); + expect(clock.pending()).toBe(0); + }); +}); + +it("takes in the file a save read back before one that lands after it", async () => { + const notice = vi.spyOn(obsidian, "Notice"); + const plugin = makePlugin(); + const write = plugin.saveData.bind(plugin); + const library = (libraryId: string, title: string) => { + const file = encodePodNotesData(structuredClone(DEFAULT_SETTINGS), { libraryId }); + file.savedFeeds = { + [title]: { title, url: `https://example.com/${title}.xml`, artworkUrl: "" }, + }; + return file; + }; + const save = vi.fn(write).mockImplementationOnce(async () => { + await write(library("", "A")); + // Another device's library lands after the save read the file back. + vi.mocked(plugin.loadData).mockImplementationOnce(async () => { + await write(library("2026-10-15T00:00:00.000Z other", "B")); + return library("2026-10-15T00:00:00.000Z other", "B"); + }); + }); + Object.assign(plugin, { saveData: save }); + plugin.settings.defaultVolume = 0.4; + + await plugin.saveSettingsStrict(); + + expect(notice).not.toHaveBeenCalled(); + expect(Object.keys(plugin.settings.savedFeeds)).toEqual(["A", "B"]); + expect(await plugin.loadData()).toMatchObject({ + defaultVolume: 0.4, + savedFeeds: { A: { title: "A" }, B: { title: "B" } }, + }); +}); + function savedByThisVersion(released: Record): Record { const data: Record = { ...structuredClone(released), @@ -1558,6 +1718,25 @@ describe("PodNotes persistence integration", () => { expect(plugin.settings).toEqual(DEFAULT_SETTINGS); }); + it("fails closed when another writer replaces a migration's write", async () => { + const plugin = makePlugin(); + const write = plugin.saveData.bind(plugin); + const external = encodePodNotesData(structuredClone(DEFAULT_SETTINGS)); + external.savedFeeds = { + Synced: { title: "Synced", url: "https://example.com/synced.xml", artworkUrl: "" }, + }; + Object.assign(plugin, { + loadData: vi + .fn() + .mockResolvedValue({ schemaVersion: 2, openAIApiKey: "must-stay-unread" }), + saveData: () => write(external), + }); + vi.spyOn(console, "error").mockImplementation(() => undefined); + + await expect(plugin.loadSettings()).rejects.toThrow(/could not write/); + expect(plugin.settings).toEqual(DEFAULT_SETTINGS); + }); + it("shows one Notice per failed-write streak and recovers silently", async () => { const notice = vi.spyOn(obsidian, "Notice"); vi.spyOn(console, "error").mockImplementation(() => undefined); @@ -1718,6 +1897,41 @@ describe("PodNotes persistence integration", () => { }); }); + it.each([ + ["could not be read", () => Promise.reject(new Error("EBUSY: resource busy"))], + ["is not JSON", async () => '{"schemaVersion": 2, "savedFeeds": {"Other device'], + [ + "had to be repaired", + async () => + dataFileText({ + ...encodePodNotesData(structuredClone(DEFAULT_SETTINGS)), + savedFeeds: { + Synced: { + title: "Synced", + url: "https://example.com/s.xml", + artworkUrl: "", + }, + }, + defaultVolume: "loud", + }), + ], + ["is from a newer PodNotes", async () => dataFileText({ schemaVersion: 3 })], + ])("fails a save whose file %s right after the write", async (_, readBack) => { + const notice = vi.spyOn(obsidian, "Notice"); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const plugin = makePlugin(); + vi.mocked(plugin.app.vault.adapter.read) + .mockImplementationOnce(readBack) + .mockImplementationOnce(readBack); + plugin.settings.skipForwardLength = 25; + + await saveInBackground(plugin); + await expect(plugin.saveSettingsStrict()).rejects.toThrow(/could not write/); + + expect(notice.mock.calls).toEqual([["PodNotes could not save your changes."]]); + expect(plugin.settings.savedFeeds).toEqual({}); + }); + describe("when a write is cut short, as on a full disk", () => { function cutShortOnce(keptBytes = 100) { const plugin = makePlugin(); diff --git a/src/main.ts b/src/main.ts index 3340433e..167dcf54 100644 --- a/src/main.ts +++ b/src/main.ts @@ -49,6 +49,7 @@ import { DataFileWriter, type ReadOutcome, type Refusal, + type WriteOutcome, WriteRefusedError, } from "./persistence/writer"; import { CredentialRepository } from "./services/CredentialRepository"; @@ -73,15 +74,6 @@ const MAIN_WINDOW_CLOCK: Clock = { now: () => window.performance.now(), }; -/** Whether data.json's text holds this data. */ -function holdsData(text: string, data: unknown): boolean { - try { - return jsonValuesEqual(JSON.parse(text), JSON.parse(JSON.stringify(data))); - } catch { - return false; - } -} - class PodNotesSchemaMigrationError extends Error { constructor( public readonly originalCause: unknown, @@ -420,10 +412,11 @@ export default class PodNotes extends Plugin implements IPodNotes { : {}; settings = { ...settings, ...references }; const addresses = privateFeedAddresses(settings.savedFeeds, this.feedUrls); - await this.writeProtectedDataFile( + const written = await this.writeProtectedDataFile( encodePodNotesData(settings, decoded.unknownFields, addresses), addresses, ); + if (written.kind !== "verified") throw this.writeFailure(); } catch (error) { throw new PodNotesSchemaMigrationError( error, @@ -528,90 +521,113 @@ export default class PodNotes extends Plugin implements IPodNotes { } private dataFileAccess(): DataFileAccess { + // As found: reading a private feed URL again stores it under a new reference, so + // the same file never reads as the same document twice. + let lastRead: unknown; return { read: async (base): Promise> => { const data: unknown = await this.loadData(); + lastRead = data; if (base && jsonValuesEqual(data, base)) return { kind: "file", doc: structuredClone(base) }; - const path = this.dataFilePath(); - const unreadable: ReadOutcome = { - kind: "refused", - refusal: { - recovery: "retry", - message: `PodNotes could not read ${path}, so it leaves the file unchanged. It tries again automatically and saves your changes once the file can be read.`, - }, - }; - if (data == null) { - return (await this.dataFileHoldsNothing()) ? { kind: "missing" } : unreadable; - } - let decoded: DecodedPodNotesData; - try { - decoded = decodePodNotesData(data); - } catch (error) { - if (!(error instanceof PodNotesDataError)) throw error; - if (error.code === "invalid-data") return unreadable; - return { - kind: "refused", - refusal: { - recovery: "stop", - message: `${path} was written by a newer version of PodNotes, likely synced from another device. PodNotes saves nothing until you update PodNotes.`, - }, - }; - } - // Only startup moves an older file's API keys into SecretStorage; a save now - // would drop them. - if (decoded.sourceVersion !== PODNOTES_DATA_SCHEMA_VERSION) { - return { - kind: "refused", - refusal: { - recovery: "stop", - message: `${path} was replaced by a copy from an older version of PodNotes, likely synced from another device. PodNotes saves nothing until you restart Obsidian, which updates the file and keeps its API keys.`, - }, - }; - } - // With nothing loaded, an entry the codec drops has nothing to be kept from. - if (!base && decoded.changed) return unreadable; - // A private feed synced here raw is protected before it can be merged and written. - const protection = protectPrivateFeeds(decoded.settings, this.feedUrls); - const addresses = new Map([ - ...privateFeedAddresses(protection.settings.savedFeeds, this.feedUrls), - ...privateFeedAddresses(this.settings.savedFeeds, this.feedUrls), - ]); - const doc: Record = toDocument( - protection.settings, - decoded.unknownFields, - addresses, + return this.decodeDataFile(data, base); + }, + write: async (doc) => { + const written = await this.writeProtectedDataFile( + doc, + privateFeedAddresses(this.settings.savedFeeds, this.feedUrls), ); - // An entry the codec dropped while repairing a value (an invalid episode, a - // collection reset) is not a deletion on the other device. Keep it. - const raw = data as Record; - let restored = false; - for (const [key, value] of Object.entries(base ?? {})) { - if (jsonValuesEqual(raw[key], doc[key])) continue; - const kept = restoreDropped(value, doc[key], raw[key], [key]); - if (jsonValuesEqual(kept, doc[key])) continue; - doc[key] = kept; - restored = true; - } - // toDocument redacted the rest. A restored entry can quote a private address, - // or name a private feed whose address other fields quote. + if (written.kind === "verified") return written; + if (jsonValuesEqual(written.doc, lastRead)) throw this.writeFailure(); + // Without a base, a file that needs a repair is unreadable, so a damaged file + // never passes for one another writer saved. + const read = await this.decodeDataFile(written.doc, null); + if (read.kind !== "file") throw this.writeFailure(); return { - kind: "file", - doc: (restored - ? redactPrivateFeedAddresses(doc, addresses) - : doc) as PersistedPodNotesDataV2, + kind: "different", + doc: read.doc, + rewrite: !jsonValuesEqual(read.doc, written.doc), }; }, - write: (doc) => - this.writeProtectedDataFile( - doc, - privateFeedAddresses(this.settings.savedFeeds, this.feedUrls), - ), local: () => this.localDocument(), publish: (doc) => this.publishDocument(doc), }; } + private async decodeDataFile( + data: unknown, + base: PersistedPodNotesDataV2 | null, + ): Promise> { + const path = this.dataFilePath(); + const unreadable: ReadOutcome = { + kind: "refused", + refusal: { + recovery: "retry", + message: `PodNotes could not read ${path}, so it leaves the file unchanged. It tries again automatically and saves your changes once the file can be read.`, + }, + }; + if (data == null) { + return (await this.dataFileHoldsNothing()) ? { kind: "missing" } : unreadable; + } + let decoded: DecodedPodNotesData; + try { + decoded = decodePodNotesData(data); + } catch (error) { + if (!(error instanceof PodNotesDataError)) throw error; + if (error.code === "invalid-data") return unreadable; + return { + kind: "refused", + refusal: { + recovery: "stop", + message: `${path} was written by a newer version of PodNotes, likely synced from another device. PodNotes saves nothing until you update PodNotes.`, + }, + }; + } + // Only startup moves an older file's API keys into SecretStorage; a save now + // would drop them. + if (decoded.sourceVersion !== PODNOTES_DATA_SCHEMA_VERSION) { + return { + kind: "refused", + refusal: { + recovery: "stop", + message: `${path} was replaced by a copy from an older version of PodNotes, likely synced from another device. PodNotes saves nothing until you restart Obsidian, which updates the file and keeps its API keys.`, + }, + }; + } + // With nothing loaded, an entry the codec drops has nothing to be kept from. + if (!base && decoded.changed) return unreadable; + // A private feed synced here raw is protected before it can be merged and written. + const protection = protectPrivateFeeds(decoded.settings, this.feedUrls); + const addresses = new Map([ + ...privateFeedAddresses(protection.settings.savedFeeds, this.feedUrls), + ...privateFeedAddresses(this.settings.savedFeeds, this.feedUrls), + ]); + const doc: Record = toDocument( + protection.settings, + decoded.unknownFields, + addresses, + ); + // An entry the codec dropped while repairing a value (an invalid episode, a + // collection reset) is not a deletion on the other device. Keep it. + const raw = data as Record; + let restored = false; + for (const [key, value] of Object.entries(base ?? {})) { + if (jsonValuesEqual(raw[key], doc[key])) continue; + const kept = restoreDropped(value, doc[key], raw[key], [key]); + if (jsonValuesEqual(kept, doc[key])) continue; + doc[key] = kept; + restored = true; + } + // toDocument redacted the rest. A restored entry can quote a private address, + // or name a private feed whose address other fields quote. + return { + kind: "file", + doc: (restored + ? redactPrivateFeedAddresses(doc, addresses) + : doc) as PersistedPodNotesDataV2, + }; + } + /** The stores of the persisted collections, from settings. The player's state is not one. */ private setCollectionStores(): void { // Before downloads: their mirror rebuilds Local Files from this store's playlist. @@ -655,7 +671,7 @@ export default class PodNotes extends Plugin implements IPodNotes { private async writeProtectedDataFile( data: unknown, privateFeedAddresses: ReadonlyMap, - ): Promise { + ): Promise> { if (this.closing) throw new WriteRefusedError(CLOSING_REFUSALS[this.closing]); if (redactPrivateFeedAddresses(data, privateFeedAddresses) !== data) { throw new Error("PodNotes does not write a private feed URL to data.json."); @@ -669,10 +685,19 @@ export default class PodNotes extends Plugin implements IPodNotes { } // saveData resolves even when the write failed (Obsidian's writeJson swallows the // error), so read the file back. - const text = await this.app.vault.adapter.read(this.dataFilePath()).catch(() => null); - if (text === null || !holdsData(text, data)) { - throw new Error(`PodNotes could not write ${this.dataFilePath()}.`); + let found: unknown; + try { + found = JSON.parse(await this.app.vault.adapter.read(this.dataFilePath())); + } catch { + throw this.writeFailure(); } + return jsonValuesEqual(found, JSON.parse(JSON.stringify(data))) + ? { kind: "verified" } + : { kind: "different", doc: found }; + } + + private writeFailure(): Error { + return new Error(`PodNotes could not write ${this.dataFilePath()}.`); } private dataFilePath(): string { diff --git a/src/persistence/writer.test.ts b/src/persistence/writer.test.ts index 16aa5b7a..88bf15c3 100644 --- a/src/persistence/writer.test.ts +++ b/src/persistence/writer.test.ts @@ -5,6 +5,7 @@ import { DataFileWriter, type ReadOutcome, type Refusal, + type WriteOutcome, type WriterStart, } from "./writer"; @@ -34,6 +35,8 @@ function refused(refusal: Refusal): ReadOutcome { return { kind: "refused", refusal }; } +const VERIFIED: WriteOutcome = { kind: "verified" }; + const settled = () => new Promise((resolve) => setTimeout(resolve, 0)); /** data.json and the in-memory state as two plain documents. */ @@ -47,9 +50,10 @@ function memoryFile(initial: Doc = doc()) { }; const access = { read: vi.fn(async (): Promise> => found(state.disk)), - write: vi.fn(async (next: Doc) => { + write: vi.fn(async (next: Doc): Promise> => { state.writes.push(structuredClone(next)); state.disk = structuredClone(next); + return VERIFIED; }), local: () => structuredClone(state.memory), publish: (next: Doc) => { @@ -69,6 +73,22 @@ function setup(initial: Doc = doc(), start: WriterStart = { base: structure return { ...file, clock, events, writer }; } +function replacedWith( + state: ReturnType["state"], + replacement: (before: Doc, written: Doc) => Doc, +) { + return async (next: Doc): Promise> => { + state.writes.push(structuredClone(next)); + state.disk = replacement(structuredClone(state.disk!), structuredClone(next)); + return { kind: "different", doc: structuredClone(state.disk) }; + }; +} + +function withFeed(file: Doc, title: string): Doc { + file.savedFeeds[title] = { title }; + return file; +} + afterEach(() => { vi.restoreAllMocks(); }); @@ -128,6 +148,7 @@ describe("DataFileWriter saves", () => { await new Promise((resolve) => (finishWrite = resolve)); state.writes.push(structuredClone(next)); state.disk = structuredClone(next); + return VERIFIED; }); state.memory.defaultVolume = 0.5; writer.request(); @@ -183,6 +204,7 @@ describe("DataFileWriter saves", () => { access.write.mockImplementationOnce(async (next: Doc) => { await new Promise((resolve) => (finishWrite = resolve)); state.disk = structuredClone(next); + return VERIFIED; }); state.memory.defaultVolume = 0.5; const saving = writer.save(); @@ -258,6 +280,7 @@ describe("DataFileWriter saves", () => { state.memory.playedEpisodes["Pod::Ep"] = { time: 42 }; state.writes.push(structuredClone(next)); state.disk = structuredClone(next); + return VERIFIED; }); await writer.save(); @@ -439,6 +462,102 @@ describe("DataFileWriter saves", () => { }); }); +describe("DataFileWriter saves another writer replaced", () => { + it("takes in the file a sync client put in place of a save and writes the merge once", async () => { + const { access, clock, events, state, writer } = setup(); + access.write.mockImplementationOnce( + replacedWith(state, (before) => withFeed(before, "Synced")), + ); + state.memory.defaultVolume = 0.5; + + writer.request(); + await writer.flush(); + + const merged = withFeed({ ...doc(), defaultVolume: 0.5 }, "Synced"); + expect(state.writes).toEqual([{ ...doc(), defaultVolume: 0.5 }, merged]); + expect(state.disk).toEqual(merged); + expect(state.memory).toEqual(merged); + expect(events.onWriteFailed).not.toHaveBeenCalled(); + expect(clock.pending()).toBe(0); + }); + + it("fails a save that left the file as it was, even when that differs from what it loaded", async () => { + const { access, clock, events, state, writer } = setup(); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const before = withFeed(doc(), "Synced"); + state.disk = structuredClone(before); + // Obsidian's saveData resolves even when its write fails. + access.write.mockImplementationOnce(async () => ({ + kind: "different", + doc: structuredClone(before), + })); + state.memory.defaultVolume = 0.5; + + writer.request(); + await writer.flush(); + expect(events.onWriteFailed).toHaveBeenCalledTimes(1); + expect(access.write).toHaveBeenCalledTimes(1); + expect(state.published).toHaveLength(0); + expect(clock.delays).toEqual([10_000]); + + clock.advance(10_000); + clock.runAll(); + await writer.flush(); + expect(state.disk).toEqual({ ...before, defaultVolume: 0.5 }); + expect(state.memory).toEqual(state.disk); + }); + + it("keeps edits made during a replaced save, even one undoing the edit it wrote", async () => { + const { access, state, writer } = setup(); + access.write.mockImplementationOnce( + replacedWith(state, (_, written) => { + state.memory.defaultVolume = 1; + state.memory.playedEpisodes["Pod::Ep"] = { time: 42 }; + return withFeed(written, "Synced"); + }), + ); + state.memory.defaultVolume = 0.5; + + await writer.save(); + + const expected = withFeed(doc(), "Synced"); + expected.playedEpisodes["Pod::Ep"] = { time: 42 }; + expect(state.memory).toEqual(expected); + expect(state.disk).toEqual(expected); + }); + + it("finishes a strict save through every replacement in a row", async () => { + const { access, state, writer } = setup(); + for (const title of ["A", "B", "C"]) { + access.write.mockImplementationOnce( + replacedWith(state, (before) => withFeed(before, title)), + ); + } + state.memory.defaultVolume = 0.5; + + await writer.save(); + + const expected = ["A", "B", "C"].reduce(withFeed, { ...doc(), defaultVolume: 0.5 }); + expect(state.writes).toHaveLength(4); + expect(state.disk).toEqual(expected); + expect(state.memory).toEqual(expected); + }); + + it("writes nothing more when the file that replaced a save already holds its edit", async () => { + const { access, state, writer } = setup(); + access.write.mockImplementationOnce( + replacedWith(state, (_, written) => withFeed(written, "Synced")), + ); + state.memory.defaultVolume = 0.5; + + await writer.save(); + await writer.save(); + + expect(state.writes).toHaveLength(1); + expect(state.memory).toEqual(withFeed({ ...doc(), defaultVolume: 0.5 }, "Synced")); + }); +}); + describe("DataFileWriter changes made elsewhere", () => { it("applies a change made elsewhere to memory and writes nothing back", async () => { const { clock, state, writer } = setup(); diff --git a/src/persistence/writer.ts b/src/persistence/writer.ts index c87a75b2..68e7993a 100644 --- a/src/persistence/writer.ts +++ b/src/persistence/writer.ts @@ -29,6 +29,14 @@ export type ReadOutcome = | { readonly kind: "missing" } | { readonly kind: "refused"; readonly refusal: Refusal }; +/** + * What the file held right after a write. `rewrite`: reading the file changed it, as + * by protecting a private feed URL, so `doc` still has to be written. + */ +export type WriteOutcome = + | { readonly kind: "verified" } + | { readonly kind: "different"; readonly doc: Doc; readonly rewrite?: boolean }; + /** How the writer reaches data.json and the in-memory state, as plain JSON documents. */ export interface DataFileAccess { /** @@ -36,7 +44,11 @@ export interface DataFileAccess { * has them. Rejects only on a defect, which is not retried. */ read(base: Doc | null): Promise>; - write(doc: Doc): Promise; + /** + * Rejects when the write fails, leaves the file as the last read found it, or leaves + * a file that does not read as a document without repairs. + */ + write(doc: Doc): Promise>; /** The in-memory state as the document it would write. */ local(): Doc; /** Replace the in-memory state with a document that came from disk. */ @@ -269,45 +281,64 @@ export class DataFileWriter { private async persist(warnOnFailure: boolean): Promise { const blocked = this.blocked(); if (blocked) throw new WriteRefusedError(blocked.message); - if (!this.pending()) return; + if (this.pending()) await this.writeMerged(await this.readForWrite(), warnOnFailure); + } + private async readForWrite(): Promise { this.lastSaveAt = this.clock.now(); const read = await this.file.read(this.base); if (read.kind === "refused") { this.refuse(read.refusal); throw new WriteRefusedError(read.refusal.message); } - await this.writeMerged(read.kind === "file" ? read.doc : null, warnOnFailure); + return read.kind === "file" ? read.doc : null; } private async writeMerged(disk: Doc | null, warnOnFailure: boolean): Promise { this.lift(); // Before the read settles a failed write the file holds, which does not end its streak. const reportFailure = warnOnFailure && !this.failedWrite; - this.settleFailedWrite(disk); - const base = this.base; - // Captured after the read, so edits made while it was in flight are written too. - const local = this.local(); - let toWrite = local; - if (disk && base) { - toWrite = mergeDataFile(base, local, disk); - } else if (!disk && this.failedWrite) { - // A failed write that left nothing on disk held the changes made elsewhere: - // write them again, with the edits made here since. - const { local: before, written } = this.failedWrite; - toWrite = threeWayMergeSettings(before, local, written); - } - try { - await this.file.write(toWrite); - } catch (error) { - if (error instanceof WriteRefusedError) throw error; - if (reportFailure) this.events.onWriteFailed(); - this.failedWrite = { local, written: toWrite }; - this.scheduleCheckpoint(); - throw error; + for (;;) { + this.settleFailedWrite(disk); + const base = this.base; + // Captured after the read, so edits made while it was in flight are written too. + const local = this.local(); + let toWrite = local; + if (disk && base) { + toWrite = mergeDataFile(base, local, disk); + } else if (!disk && this.failedWrite) { + // A failed write that left nothing on disk held the changes made elsewhere: + // write them again, with the edits made here since. + const { local: before, written } = this.failedWrite; + toWrite = threeWayMergeSettings(before, local, written); + } + let outcome: WriteOutcome; + try { + outcome = await this.file.write(toWrite); + // A file that needs a rewrite can read as `disk` only once reading changed it. + if ( + outcome.kind === "different" && + !outcome.rewrite && + jsonValuesEqual(outcome.doc, disk) + ) { + throw new Error("PodNotes could not write data.json."); + } + } catch (error) { + if (error instanceof WriteRefusedError) throw error; + if (reportFailure) this.events.onWriteFailed(); + this.failedWrite = { local, written: toWrite }; + this.scheduleCheckpoint(); + throw error; + } + if (outcome.kind === "verified") { + this.failedWrite = null; + this.settle(local, toWrite); + return; + } + this.adopt(outcome.doc, local); + if (!outcome.rewrite && !this.pending()) return; + disk = await this.readForWrite(); } - this.failedWrite = null; - this.settle(local, toWrite); } /** @@ -348,16 +379,22 @@ export class DataFileWriter { } if (!disk) return; this.settleFailedWrite(disk); + if (this.base && jsonValuesEqual(disk, this.base)) return; - const base = this.base; - if (base && jsonValuesEqual(disk, base)) return; + const merged = this.adopt(disk, this.local()); + if (jsonValuesEqual(merged, disk)) this.cancelTimer(); + else this.request(); + } - const local = this.local(); - const merged = base ? mergeDataFile(base, local, disk) : structuredClone(disk); + private adopt(disk: Doc, captured: Doc): Doc { + const current = this.local(); + const external = this.base + ? mergeDataFile(this.base, captured, disk) + : structuredClone(disk); + const merged = threeWayMergeSettings(captured, current, external); this.base = disk; this.lift(); - if (!jsonValuesEqual(merged, local)) this.publish(merged); - if (jsonValuesEqual(merged, disk)) this.cancelTimer(); - else this.request(); + if (!jsonValuesEqual(merged, current)) this.publish(merged); + return merged; } } diff --git a/tests/e2e/settings-tab.test.ts b/tests/e2e/settings-tab.test.ts index 99f3797d..ace80ed6 100644 --- a/tests/e2e/settings-tab.test.ts +++ b/tests/e2e/settings-tab.test.ts @@ -245,6 +245,9 @@ describe("PodNotes settings tab", () => { timeoutMs: 5_000, intervalMs: 100, }); + await obsidian.dev.evalJsonAsync( + "app.plugins.plugins.podnotes.flushSaves().then(() => true)", + ); const synced = await plugin.data().read(); if (!synced.note) throw new Error("The saved episode note settings are missing"); synced.note.path = remote; diff --git a/tests/e2e/sync-save-readback.test.ts b/tests/e2e/sync-save-readback.test.ts index e2e40034..177e3eb4 100644 --- a/tests/e2e/sync-save-readback.test.ts +++ b/tests/e2e/sync-save-readback.test.ts @@ -1,5 +1,6 @@ import { mkdir } from "node:fs/promises"; import { resolve } from "node:path"; +import type { ObsidianClient } from "obsidian-e2e"; import { expect, onTestFinished, test } from "vitest"; import { createPodNotesE2EHarness, type PodNotesData } from "./harness"; import { closeSettings, openPodNotesSettings } from "./journey/actions"; @@ -9,18 +10,11 @@ import { jsLiteral, RELOAD_OPTIONS, WAIT_OPTS } from "./runtime"; const getContext = createPodNotesE2EHarness("sync-save-readback"); -test("merges a sync replacement during a settings save without a failure Notice", async () => { - const { obsidian, plugin } = getContext(); - await plugin.updateDataAndReload((data) => { - data.skipForwardLength = 15; - data.savedFeeds = {}; - }, RELOAD_OPTIONS); - await openPodNotesSettings(obsidian); - const external = await plugin.data().read(); - external.savedFeeds = { - Synced: { title: "Synced", url: "https://example.com/synced.xml", artworkUrl: "" }, - }; - const dataPath = await plugin.dataPath(); +const FAILURE = "PodNotes could not save your changes."; + +/** Replaces data.json with `external` between the next save's write and its readback. */ +async function replaceNextWrite(obsidian: ObsidianClient, external: unknown): Promise { + const { plugin } = getContext(); await obsidian.dev.evalJson(`(() => { const adapter = app.vault.adapter; const original = adapter.applyWriteOptions; @@ -33,7 +27,7 @@ test("merges a sync replacement during a settings save without a failure Notice" }); observer.observe(document.body, { childList: true, subtree: true }); adapter.applyWriteOptions = async function(file, options) { - if (file === ${jsLiteral(dataPath)}) { + if (file === ${jsLiteral(await plugin.dataPath())}) { probe.writes++; if (!probe.injected) { require("fs").writeFileSync(file, ${jsLiteral(JSON.stringify(external))}); @@ -51,24 +45,42 @@ test("merges a sync replacement during a settings save without a failure Notice" "window.__syncSaveReadback?.restore(), delete window.__syncSaveReadback, true", ); }); +} + +async function saveSkipForward(obsidian: ObsidianClient) { await typeInto(obsidian, ui.settingControl("Skip forward (s)", "input"), "25", { replace: true, }); - const disk = await plugin.waitForData( - (data) => data.skipForwardLength === 25 && Boolean(data.savedFeeds?.Synced), - WAIT_OPTS, - ); +} + +async function settledProbe(obsidian: ObsidianClient) { await closeSettings(obsidian); await obsidian.dev.evalJsonAsync( "(async () => { await app.plugins.plugins.podnotes.flushSaves(); return true; })()", ); - const result = await obsidian.dev.evalJson<{ - injected: boolean; - writes: number; - notices: string[]; - }>( + return obsidian.dev.evalJson<{ injected: boolean; writes: number; notices: string[] }>( "({ injected: window.__syncSaveReadback.injected, writes: window.__syncSaveReadback.writes, notices: window.__syncSaveReadback.notices })", ); +} + +test("merges a sync replacement during a settings save without a failure Notice", async () => { + const { obsidian, plugin } = getContext(); + await plugin.updateDataAndReload((data) => { + data.skipForwardLength = 15; + data.savedFeeds = {}; + }, RELOAD_OPTIONS); + await openPodNotesSettings(obsidian); + const external = await plugin.data().read(); + external.savedFeeds = { + Synced: { title: "Synced", url: "https://example.com/synced.xml", artworkUrl: "" }, + }; + await replaceNextWrite(obsidian, external); + await saveSkipForward(obsidian); + const disk = await plugin.waitForData( + (data) => data.skipForwardLength === 25 && Boolean(data.savedFeeds?.Synced), + WAIT_OPTS, + ); + const result = await settledProbe(obsidian); console.info(JSON.stringify({ ...result, disk, version: await obsidian.exec("version") })); const directory = resolve(".obsidian-e2e-artifacts/sync-save-readback"); await mkdir(directory, { recursive: true }); @@ -79,5 +91,39 @@ test("merges a sync replacement during a settings save without a failure Notice" await obsidian.dev.evalJson("app.plugins.plugins.podnotes.settings.skipForwardLength"), ).toBe(25); expect(result.writes).toBe(2); - expect(result.notices).not.toContain("PodNotes could not save your changes."); + expect(result.notices).not.toContain(FAILURE); +}); + +test("protects a private feed address that a sync copies back during a settings save", async () => { + const { obsidian, plugin } = getContext(); + const secret = "SYNC_SAVE_READBACK_SENTINEL"; + const address = `https://private.example/feed.xml?auth=${secret}`; + const copied = (url: string) => `copied from ${url} on another device`; + await plugin.updateDataAndReload>((data) => { + data.skipForwardLength = 15; + data.savedFeeds = { Private: { title: "Private", url: address, artworkUrl: "" } }; + data.copiedNote = copied(address); + }, RELOAD_OPTIONS); + const earlier = await plugin.waitForData>( + (data) => !JSON.stringify(data).includes(secret), + WAIT_OPTS, + ); + expect(earlier.copiedNote).toBe(copied("podnotes-private-feed:Private")); + await openPodNotesSettings(obsidian); + await replaceNextWrite(obsidian, { ...earlier, copiedNote: copied(address) }); + await saveSkipForward(obsidian); + const disk = await plugin.waitForData>( + (data) => data.skipForwardLength === 25, + WAIT_OPTS, + ); + const result = await settledProbe(obsidian); + console.info(JSON.stringify({ ...result, disk, version: await obsidian.exec("version") })); + expect(result.injected).toBe(true); + expect(result.notices).not.toContain(FAILURE); + expect(JSON.stringify(disk)).not.toContain(secret); + expect(disk).toMatchObject({ + copiedNote: copied("podnotes-private-feed:Private"), + savedFeeds: { Private: { url: "podnotes-private-feed:Private" } }, + }); + expect(result.writes).toBe(2); });