Sync → Interrupted pulls no longer leave torn files

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}`;

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<string, string> = {}): 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<string, string> = {},
+): { adapter: DataAdapter; files: Map<string, string>; ops: string[] } {
+  const files = new Map<string, string>(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);
 

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<void
     await adapter.mkdir(dir);
   }
 }
+
+// hiddenSiblingPath returns a dot prefixed sibling of path carrying suffix, the naming scheme for
+// geode's staging files: hidden so Obsidian never indexes them and they can never appear in a
+// snapshot, deterministic so a leftover from an interrupted write is reclaimed by the next write
+// to the same path rather than accumulating.
+function hiddenSiblingPath(path: string, suffix: string): string {
+  const lastSlash = path.lastIndexOf("/");
+
+  return `${path.slice(0, lastSlash + 1)}.${path.slice(lastSlash + 1)}${suffix}`;
+}
+
+// replaceViaAside installs the staged file over an existing destination for an adapter whose
+// rename refuses to overwrite: the current content is renamed aside, the staged file claims the
+// path, and only then is the aside copy removed. The destination's bytes are never deleted while
+// a restore is still possible, so if the rename actually failed for some other reason
+// (permissions, a transient I/O error) and the retry fails the same way, the aside copy is
+// renamed straight back and the file survives untouched.
+async function replaceViaAside(
+  adapter: DataAdapter,
+  tempPath: string,
+  path: string,
+): Promise<void> {
+  const asidePath = hiddenSiblingPath(path, ".geode-old");
+  const leftover = await adapter.exists(asidePath);
+  if (leftover) {
+    await adapter.remove(asidePath);
+  }
+  await adapter.rename(path, asidePath);
+  try {
+    await adapter.rename(tempPath, path);
+  } catch (err) {
+    await adapter.rename(asidePath, path);
+    throw err;
+  }
+  await adapter.remove(asidePath);
+}
+
+// writeThroughTemp stages data at a hidden temp path beside its destination, then renames it into
+// place, so a crash mid write leaves the destination either untouched or fully written, never
+// holding torn bytes (#88). Desktop's adapter rename replaces an existing destination atomically;
+// a rename that fails while the destination exists is retried through replaceViaAside, shrinking
+// the exposure from the whole download and write to the instant between the two renames, where a
+// crash leaves the path absent and the next sync replans the pull instead of pushing corruption.
+async function writeThroughTemp(
+  adapter: DataAdapter,
+  path: string,
+  data: ArrayBuffer,
+): Promise<void> {
+  const tempPath = hiddenSiblingPath(path, ".geode-tmp");
+  await adapter.writeBinary(tempPath, data);
+  try {
+    await adapter.rename(tempPath, path);
+  } catch (err) {
+    const exists = await adapter.exists(path);
+    if (!exists) {
+      throw err;
+    }
+    await replaceViaAside(adapter, tempPath, path);
+  }
+}