From 1b426385539992ce58762c80d543d06b00fa6f4d Mon Sep 17 00:00:00 2001 From: revett <2796074+revett@users.noreply.github.com> Date: Tue, 21 Jul 2026 22:08:58 +0100 Subject: [PATCH 1/2] Fix issue --- src/vault/fs.ts | 5 +- src/vault/obsidian.test.ts | 123 ++++++++++++++++++++++++++++++++++++- src/vault/obsidian.ts | 43 ++++++++++++- 3 files changed, 166 insertions(+), 5 deletions(-) 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..4006bca 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,130 @@ 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), writeBinaryFails mimics an interrupted or failed write of the staged bytes. +type WriterBehavior = { + renameOverwrites: 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.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, + 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, 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 falls back to remove then rename", async () => { + const { adapter, files, ops } = fakeWriterAdapter( + { renameOverwrites: 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", + "remove a.md", + "rename .a.md.geode-tmp -> 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, 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..990043d 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,39 @@ async function ensureParentDir(adapter: DataAdapter, path: string): Promise