diff --git a/src/vault/fs.ts b/src/vault/fs.ts index 4b0a377..5988518 100644 --- a/src/vault/fs.ts +++ b/src/vault/fs.ts @@ -69,12 +69,13 @@ function abs(root: string, path: string): string { } // walk returns every file (not directory) under the root as vault relative, forward slash paths, -// excluding anything under .obsidian, mirroring what Obsidian's Vault.getFiles() leaves out. +// excluding dot prefixed entries (.obsidian, staged .geode-tmp writes), mirroring how Obsidian's +// Vault.getFiles() never indexes hidden files. function walk(root: string, dir = ""): string[] { const here = dir === "" ? root : abs(root, dir); const out: string[] = []; for (const entry of readdirSync(here, { withFileTypes: true })) { - if (entry.name === ".obsidian") { + if (entry.name.startsWith(".")) { continue; } const rel = dir === "" ? entry.name : `${dir}/${entry.name}`; diff --git a/src/vault/obsidian.test.ts b/src/vault/obsidian.test.ts index 6b64a02..b71a92c 100644 --- a/src/vault/obsidian.test.ts +++ b/src/vault/obsidian.test.ts @@ -1,7 +1,7 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import type { DataAdapter } from "obsidian"; -import { createObsidianStore } from "./obsidian.ts"; +import { createObsidianLocalWriter, createObsidianStore } from "./obsidian.ts"; import type { Snapshot } from "./vault.ts"; // fakeAdapter returns a DataAdapter whose exists/read/write operate over one in-memory file map, @@ -25,9 +25,162 @@ function fakeAdapter(seed: Record = {}): DataAdapter { return adapter as unknown as DataAdapter; } +// WriterBehavior configures the failure modes a fake writer adapter simulates: renameOverwrites +// false mimics a filesystem whose rename refuses to replace an existing destination (as mobile +// can), stagedRenameFails mimics a rename that fails whenever the staged temp file is the source +// (a lock on the staged bytes, a permissions error), and writeBinaryFails mimics an interrupted +// or failed write of the staged bytes. +type WriterBehavior = { + renameOverwrites: boolean; + stagedRenameFails: boolean; + writeBinaryFails: boolean; +}; + +// fakeWriterAdapter returns a DataAdapter whose binary file methods operate over one in-memory +// file map, enough to drive the local writer, plus an ordered log of every mutating call so tests +// can assert the destination is only ever renamed into, never written directly. +function fakeWriterAdapter( + behavior: WriterBehavior, + seed: Record = {}, +): { adapter: DataAdapter; files: Map; ops: string[] } { + const files = new Map(Object.entries(seed)); + const ops: string[] = []; + const adapter = { + exists: async (path: string) => files.has(path), + mkdir: async (path: string) => { + ops.push(`mkdir ${path}`); + }, + remove: async (path: string) => { + ops.push(`remove ${path}`); + if (!files.delete(path)) { + throw new Error(`no such file: ${path}`); + } + }, + rename: async (path: string, newPath: string) => { + ops.push(`rename ${path} -> ${newPath}`); + if (behavior.stagedRenameFails && path.endsWith(".geode-tmp")) { + throw new Error(`permission denied: ${path}`); + } + if (!behavior.renameOverwrites && files.has(newPath)) { + throw new Error(`destination exists: ${newPath}`); + } + const content = files.get(path); + if (content === undefined) { + throw new Error(`no such file: ${path}`); + } + files.delete(path); + files.set(newPath, content); + }, + writeBinary: async (path: string, data: ArrayBuffer) => { + ops.push(`writeBinary ${path}`); + if (behavior.writeBinaryFails) { + throw new Error(`disk full: ${path}`); + } + files.set(path, new TextDecoder().decode(data)); + }, + }; + return { adapter: adapter as unknown as DataAdapter, files, ops }; +} + +// bytes encodes body for a writeFile call. +function bytes(body: string): Uint8Array { + return new TextEncoder().encode(body); +} + const STATE_PATH = "state.json"; const empty: Snapshot = { files: [] }; +test("createObsidianLocalWriter: a pull is staged to a hidden temp file and renamed into place, never written directly to its destination", async () => { + const { adapter, files, ops } = fakeWriterAdapter({ + renameOverwrites: true, + stagedRenameFails: false, + writeBinaryFails: false, + }); + const writer = createObsidianLocalWriter(adapter); + + await writer.writeFile("notes/a.md", bytes("pulled content")); + + assert.equal(files.get("notes/a.md"), "pulled content"); + assert.equal(files.has("notes/.a.md.geode-tmp"), false); + assert.deepEqual(ops, [ + "mkdir notes", + "writeBinary notes/.a.md.geode-tmp", + "rename notes/.a.md.geode-tmp -> notes/a.md", + ]); +}); + +test("createObsidianLocalWriter: overwriting an existing file replaces it through the temp rename", async () => { + const { adapter, files, ops } = fakeWriterAdapter( + { renameOverwrites: true, stagedRenameFails: false, writeBinaryFails: false }, + { "a.md": "old content" }, + ); + const writer = createObsidianLocalWriter(adapter); + + await writer.writeFile("a.md", bytes("new content")); + + assert.equal(files.get("a.md"), "new content"); + assert.equal(files.has(".a.md.geode-tmp"), false); + assert.deepEqual(ops, ["writeBinary .a.md.geode-tmp", "rename .a.md.geode-tmp -> a.md"]); +}); + +test("createObsidianLocalWriter: an adapter whose rename refuses to overwrite replaces via the aside rename", async () => { + const { adapter, files, ops } = fakeWriterAdapter( + { renameOverwrites: false, stagedRenameFails: false, writeBinaryFails: false }, + { "a.md": "old content" }, + ); + const writer = createObsidianLocalWriter(adapter); + + await writer.writeFile("a.md", bytes("new content")); + + assert.equal(files.get("a.md"), "new content"); + assert.equal(files.has(".a.md.geode-tmp"), false); + assert.equal(files.has(".a.md.geode-old"), false); + assert.deepEqual(ops, [ + "writeBinary .a.md.geode-tmp", + "rename .a.md.geode-tmp -> a.md", + "rename a.md -> .a.md.geode-old", + "rename .a.md.geode-tmp -> a.md", + "remove .a.md.geode-old", + ]); +}); + +test("createObsidianLocalWriter: a rename that keeps failing for another reason restores the destination", async () => { + // The review edge case on #88: the first rename did not fail because the destination exists, + // it fails for a reason (a lock, permissions) the retry hits too. The destination must come + // through with its old content intact, never deleted on a wrong guess about why rename failed. + const { adapter, files, ops } = fakeWriterAdapter( + { renameOverwrites: false, stagedRenameFails: true, writeBinaryFails: false }, + { "a.md": "old content" }, + ); + const writer = createObsidianLocalWriter(adapter); + + await assert.rejects(writer.writeFile("a.md", bytes("new content"))); + + assert.equal(files.get("a.md"), "old content"); + assert.equal(files.has(".a.md.geode-old"), false); + assert.deepEqual(ops, [ + "writeBinary .a.md.geode-tmp", + "rename .a.md.geode-tmp -> a.md", + "rename a.md -> .a.md.geode-old", + "rename .a.md.geode-tmp -> a.md", + "rename .a.md.geode-old -> a.md", + ]); +}); + +test("createObsidianLocalWriter: a failed write of the staged bytes leaves the destination untouched", async () => { + // The scenario from #88: the write is interrupted partway. Staging means the destination still + // holds its previous content in full, so the next snapshot sees no phantom local edit to push. + const { adapter, files } = fakeWriterAdapter( + { renameOverwrites: true, stagedRenameFails: false, writeBinaryFails: true }, + { "a.md": "old content" }, + ); + const writer = createObsidianLocalWriter(adapter); + + await assert.rejects(writer.writeFile("a.md", bytes("new content"))); + + assert.equal(files.get("a.md"), "old content"); +}); + test("createObsidianStore: a missing state file reads back as empty", async () => { const store = createObsidianStore(fakeAdapter(), STATE_PATH); diff --git a/src/vault/obsidian.ts b/src/vault/obsidian.ts index 43df0be..569b61b 100644 --- a/src/vault/obsidian.ts +++ b/src/vault/obsidian.ts @@ -4,13 +4,16 @@ import { type FileInfo, isSnapshot, type Reader, type Snapshot, type Store } fro // createObsidianLocalWriter returns a LocalWriter that applies pulled remote changes straight // through the low level data adapter, rather than the Vault API, since a path pulled down for -// the first time has no TFile yet for Vault.modifyBinary/rename to operate on. +// the first time has no TFile yet for Vault.modifyBinary/rename to operate on. Pulled content is +// staged to a hidden temp file and renamed into place, never written directly to its destination, +// so an interrupted pull cannot leave torn bytes for the next snapshot to read as a local edit +// and push to the bucket (#88). export function createObsidianLocalWriter(adapter: DataAdapter): LocalWriter { return { writeFile: async (path, data) => { await ensureParentDir(adapter, path); const buffer = data.buffer.slice(data.byteOffset, data.byteOffset + data.byteLength); - await adapter.writeBinary(path, buffer as ArrayBuffer); + await writeThroughTemp(adapter, path, buffer as ArrayBuffer); }, deleteFile: async (path) => { const exists = await adapter.exists(path); @@ -94,3 +97,63 @@ async function ensureParentDir(adapter: DataAdapter, path: string): Promise