diff --git a/src/main.ts b/src/main.ts index 83b1d41..2511029 100644 --- a/src/main.ts +++ b/src/main.ts @@ -208,6 +208,11 @@ export default class GeodePlugin extends Plugin { const previous = await stateStore.read(); const outcome = await syncOnce(previous, reader, localWriter, storage, Date.now()); if (!outcome.ok) { + // A failed pass can still have made progress worth keeping (#87): the snapshot records what + // completed so it is never re-planned, while each failed file stays pending for next pass. + if (outcome.snapshot !== null) { + await stateStore.write(outcome.snapshot); + } for (const failure of outcome.failures) { this.logger.error(`sync: ${failure.path}: ${failure.message}`); } diff --git a/src/sync/execute.test.ts b/src/sync/execute.test.ts index 13e28a7..4075490 100644 --- a/src/sync/execute.test.ts +++ b/src/sync/execute.test.ts @@ -16,7 +16,7 @@ test("executeSyncPlan: push reads the local file and puts it remotely", async () const { writer, files } = fakeLocalWriter(); const { storage, objects } = fakeStorage(); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "push", path: "a.md" }], empty, reader, @@ -45,7 +45,7 @@ test("executeSyncPlan: pull fetches the remote object and writes it locally", as const { writer, files } = fakeLocalWriter(); const { storage } = fakeStorage({ "a.md": "hello" }); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "pull", path: "a.md" }], empty, reader, @@ -65,7 +65,7 @@ test("executeSyncPlan: pull overwrites a local file that still matches the snaps const local = snapshot(file("a.md", await hashOf("unchanged"))); const { storage } = fakeStorage({ "a.md": "remote edit" }); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "pull", path: "a.md" }], local, reader, @@ -93,7 +93,7 @@ test("executeSyncPlan: pull onto a file edited after the snapshot is refused and { kind: "pull", path: "a.md" }, { kind: "pull", path: "b.md" }, ]; - const failures = await executeSyncPlan(actions, local, reader, writer, storage, 1); + const { failures } = await executeSyncPlan(actions, local, reader, writer, storage, 1); assert.deepEqual(failures, [ { path: "a.md", message: "changed locally mid sync; sync again to reconcile" }, @@ -112,7 +112,7 @@ test("executeSyncPlan: pull onto a file created after the snapshot is refused", files.set("a.md", "created after snapshot"); const { storage } = fakeStorage({ "a.md": "remote edit" }); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "pull", path: "a.md" }], empty, reader, @@ -149,7 +149,7 @@ test("executeSyncPlan: pullDelete of a file edited after the snapshot is refused const local = snapshot(file("a.md", await hashOf("as snapshotted"))); const { storage } = fakeStorage(); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "pullDelete", path: "a.md" }], local, reader, @@ -177,7 +177,7 @@ test("executeSyncPlan: pullDelete of a file that exists but cannot be read is re const local = snapshot(file("a.md", await hashOf("hello"))); const { storage } = fakeStorage(); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "pullDelete", path: "a.md" }], local, reader, @@ -197,7 +197,7 @@ test("executeSyncPlan: a conflict renames the local copy, pushes it to storage, const { storage, objects } = fakeStorage({ "a.md": "remote edit" }); const now = Date.parse("2026-07-14T10:00:00.000Z"); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "conflict", path: "a.md", deletedSide: "none" }], empty, reader, @@ -221,7 +221,7 @@ test("executeSyncPlan: a conflict with nothing local to preserve just pulls the const { storage } = fakeStorage({ "a.md": "remote edit" }); const now = Date.parse("2026-07-14T10:00:00.000Z"); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "conflict", path: "a.md", deletedSide: "local" }], empty, reader, @@ -245,7 +245,7 @@ test("executeSyncPlan: a conflict restore onto a path recreated after the snapsh const { storage } = fakeStorage({ "a.md": "remote edit" }); const now = Date.parse("2026-07-14T10:00:00.000Z"); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "conflict", path: "a.md", deletedSide: "local" }], empty, reader, @@ -267,7 +267,7 @@ test("executeSyncPlan: a conflict with nothing remote to pull preserves the loca const { storage, objects } = fakeStorage(); const now = Date.parse("2026-07-14T10:00:00.000Z"); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "conflict", path: "a.md", deletedSide: "remote" }], empty, reader, @@ -294,7 +294,7 @@ test("executeSyncPlan: a push whose local file vanished is reported and doesn't { kind: "push", path: "a.md" }, { kind: "push", path: "b.md" }, ]; - const failures = await executeSyncPlan(actions, empty, reader, writer, storage, 1); + const { failures } = await executeSyncPlan(actions, empty, reader, writer, storage, 1); assert.deepEqual(failures, [{ path: "a.md", message: "no such file: a.md" }]); assert.equal(objects.get("b.md"), "world"); @@ -314,7 +314,7 @@ test("executeSyncPlan: a conflict whose local file vanished is reported, nothing { kind: "conflict", path: "a.md", deletedSide: "none" }, { kind: "push", path: "b.md" }, ]; - const failures = await executeSyncPlan(actions, empty, reader, writer, storage, now); + const { failures } = await executeSyncPlan(actions, empty, reader, writer, storage, now); assert.deepEqual(failures, [{ path: "a.md", message: "no such file: a.md" }]); // No conflict copy was created locally or remotely from a file that wasn't there to preserve. @@ -340,11 +340,61 @@ test("executeSyncPlan: a failed push is reported and doesn't stop the rest of th { kind: "push", path: "a.md" }, { kind: "push", path: "b.md" }, ]; - const failures = await executeSyncPlan(actions, empty, reader, writer, storage, 1); + const { completed, failed, failures } = await executeSyncPlan( + actions, + empty, + reader, + writer, + storage, + 1, + ); assert.deepEqual(failures, [{ path: "a.md", message: "Storage rejected the write (500)" }]); assert.equal(objects.get("b.md"), "world"); assert.equal(files.size, 0); + // The pass reports exactly which actions completed and which didn't, so syncOnce can record + // b.md's progress in the manifest while leaving a.md pending for the next pass (#87). + assert.deepEqual(completed, [{ kind: "push", path: "b.md" }]); + assert.deepEqual(failed, [{ kind: "push", path: "a.md" }]); +}); + +test("executeSyncPlan: a conflict whose copy push fails is a failed action, even though the failure names the copy path", async () => { + // The copy push failure is recorded against copyPath, not the action's own path; failed must + // still carry the action itself, so syncOnce reverts the right path and the manifest never + // claims a copy the bucket refused. + const reader = fakeReader({ "a.md": "local edit" }); + const { writer, files } = fakeLocalWriter(); + files.set("a.md", "local edit"); + const { storage, objects } = fakeStorage({ "a.md": "remote edit" }); + const inner = storage.putObject; + storage.putObject = async (key, body, condition) => { + if (key !== "a.md") { + return { ok: false, status: "server", message: "Storage rejected the write (500)" }; + } + return inner(key, body, condition); + }; + const now = Date.parse("2026-07-14T10:00:00.000Z"); + + const action: SyncAction = { kind: "conflict", path: "a.md", deletedSide: "none" }; + const { completed, failed, failures } = await executeSyncPlan( + [action], + empty, + reader, + writer, + storage, + now, + ); + + assert.deepEqual(failures, [ + { path: conflictCopyPath("a.md", now), message: "Storage rejected the write (500)" }, + ]); + assert.deepEqual(completed, []); + assert.deepEqual(failed, [action]); + // The remote version still landed locally, and the local edit survived under its copy name, + // ready to push next pass. + assert.equal(files.get("a.md"), "remote edit"); + assert.equal(files.get(conflictCopyPath("a.md", now)), "local edit"); + assert.equal(objects.has(conflictCopyPath("a.md", now)), false); }); test("executeSyncPlan: a pull whose local write throws is reported and doesn't stop the rest of the plan", async () => { @@ -364,7 +414,7 @@ test("executeSyncPlan: a pull whose local write throws is reported and doesn't s { kind: "pull", path: "a.md" }, { kind: "pull", path: "b.md" }, ]; - const failures = await executeSyncPlan(actions, empty, reader, writer, storage, 1); + const { failures } = await executeSyncPlan(actions, empty, reader, writer, storage, 1); assert.deepEqual(failures, [{ path: "a.md", message: "EACCES: permission denied" }]); assert.equal(files.get("b.md"), "pulled"); @@ -383,7 +433,7 @@ test("executeSyncPlan: a conflict whose rename throws is reported and the local const { storage, objects } = fakeStorage({ "a.md": "remote edit" }); const now = Date.parse("2026-07-14T10:00:00.000Z"); - const failures = await executeSyncPlan( + const { failures } = await executeSyncPlan( [{ kind: "conflict", path: "a.md", deletedSide: "none" }], empty, reader, diff --git a/src/sync/execute.ts b/src/sync/execute.ts index d9385ba..2826397 100644 --- a/src/sync/execute.ts +++ b/src/sync/execute.ts @@ -6,6 +6,16 @@ import { conflictCopyPath, type SyncAction } from "./plan.ts"; // was planned from; the next sync re-snapshots and replans the path as a conflict. const DRIFT_MESSAGE = "changed locally mid sync; sync again to reconcile"; +// ExecuteResult reports what executeSyncPlan carried out: completed holds every action fully +// applied, failed the actions that weren't, and failures the per file detail of why. failed is +// carried separately from failures because a conflict's failure can name its copy path rather +// than the action's own path. +export type ExecuteResult = { + completed: SyncAction[]; + failed: SyncAction[]; + failures: SyncFailure[]; +}; + // LocalWriter applies changes decided by a sync to the local vault. The real implementation // writes through the vault adapter (see vault/obsidian.ts); tests use an in-memory fake. export type LocalWriter = { @@ -21,10 +31,11 @@ export type SyncFailure = { }; // executeSyncPlan carries out every action against reader/localWriter (the local vault) and -// storage (the remote bucket), and reports whatever couldn't be completed. local is the snapshot -// the plan was made from, so each destructive local write can first check the file hasn't changed -// since (#86). now is passed in rather than read internally so a conflict's copy name is -// deterministic under test. +// storage (the remote bucket), and reports what completed and what couldn't be, so one failed +// file never discards the progress of the rest of the pass (#87). local is the snapshot the plan +// was made from, so each destructive local write can first check the file hasn't changed since +// (#86). now is passed in rather than read internally so a conflict's copy name is deterministic +// under test. export async function executeSyncPlan( actions: SyncAction[], local: Snapshot, @@ -32,140 +43,32 @@ export async function executeSyncPlan( localWriter: LocalWriter, storage: StorageClient, now: number, -): Promise { +): Promise { + const completed: SyncAction[] = []; + const failed: SyncAction[] = []; const failures: SyncFailure[] = []; const localByPath = byPath(local.files); for (const action of actions) { - if (action.kind === "push") { - let bytes: Uint8Array; - try { - bytes = await reader.readFile(action.path); - } catch (err) { - failures.push({ path: action.path, message: localFailureMessage(err) }); - continue; - } - const result = await storage.putObject(action.path, bytes); - if (!result.ok) { - failures.push({ path: action.path, message: result.message }); - } - continue; - } - - if (action.kind === "pushDelete") { - const result = await storage.deleteObject(action.path); - if (!result.ok) { - failures.push({ path: action.path, message: result.message }); - } - continue; - } - - if (action.kind === "pull") { - const drift = await checkLocalDrift(reader, action.path, localByPath.get(action.path)); - if (drift !== null) { - failures.push(drift); - continue; - } - const result = await storage.getObject(action.path); - if (!result.ok || result.body === null) { - failures.push({ path: action.path, message: result.message }); - continue; - } - const body = result.body; - const failure = await applyLocalWrite(action.path, () => - localWriter.writeFile(action.path, body), - ); - if (failure !== null) { - failures.push(failure); - } - continue; - } - - if (action.kind === "pullDelete") { - const drift = await checkLocalDrift(reader, action.path, localByPath.get(action.path)); - if (drift !== null) { - failures.push(drift); - continue; - } - const failure = await applyLocalWrite(action.path, () => localWriter.deleteFile(action.path)); - if (failure !== null) { - failures.push(failure); - } - continue; - } - - // conflict, deletedSide "local": the user deleted their copy, so there is no local edit to - // preserve; the remote edit simply wins and is restored onto the local path. The snapshot has - // no entry here, so any file found now was recreated after it and must not be overwritten. - if (action.deletedSide === "local") { - const drift = await checkLocalDrift(reader, action.path, localByPath.get(action.path)); - if (drift !== null) { - failures.push(drift); - continue; - } - const result = await storage.getObject(action.path); - if (!result.ok || result.body === null) { - failures.push({ path: action.path, message: result.message }); - continue; - } - const body = result.body; - const failure = await applyLocalWrite(action.path, () => - localWriter.writeFile(action.path, body), - ); - if (failure !== null) { - failures.push(failure); - } - continue; - } - - // conflict, deletedSide "remote" or "none": preserve the local edit under a new name and - // push that copy to storage too, so the diverged edit lands on every device and the manifest - // we later upload isn't claiming a remote object that doesn't exist. Neither side's edit is - // ever silently discarded. - const copyPath = conflictCopyPath(action.path, now); - let localBytes: Uint8Array; - try { - localBytes = await reader.readFile(action.path); - } catch (err) { - failures.push({ path: action.path, message: localFailureMessage(err) }); - continue; - } - // A failed rename means the local edit is still sitting at action.path untouched. Bail before - // the pull below would overwrite it, so a diverged edit is never silently discarded by an I/O - // error the way it would be if we pushed on to restore the remote version. - const renameFailure = await applyLocalWrite(action.path, () => - localWriter.renameFile(action.path, copyPath), + const actionFailures = await executeAction( + action, + localByPath, + reader, + localWriter, + storage, + now, ); - if (renameFailure !== null) { - failures.push(renameFailure); + if (actionFailures.length === 0) { + completed.push(action); continue; } - const pushed = await storage.putObject(copyPath, localBytes); - if (!pushed.ok) { - failures.push({ path: copyPath, message: pushed.message }); - } - - // deletedSide "remote": there is nothing at this path remotely to pull, the rename above - // already vacated it locally, and that is the correct final state, not a failure to report. - if (action.deletedSide === "remote") { - continue; - } - - const result = await storage.getObject(action.path); - if (!result.ok || result.body === null) { - failures.push({ path: action.path, message: result.message }); - continue; - } - const body = result.body; - const writeFailure = await applyLocalWrite(action.path, () => - localWriter.writeFile(action.path, body), - ); - if (writeFailure !== null) { - failures.push(writeFailure); + failed.push(action); + for (const failure of actionFailures) { + failures.push(failure); } } - return failures; + return { completed, failed, failures }; } // applyLocalWrite runs one localWriter mutation, converting a thrown I/O error into a SyncFailure @@ -216,6 +119,146 @@ async function checkLocalDrift( return null; } +// executeAction carries out a single action and returns every failure it hit, an empty array +// meaning the action fully completed. An action can report more than one failure: a conflict +// whose copy push fails still pulls the remote version, so the diverged local edit lands on disk +// even when the bucket refuses the copy. +async function executeAction( + action: SyncAction, + localByPath: Map, + reader: Reader, + localWriter: LocalWriter, + storage: StorageClient, + now: number, +): Promise { + if (action.kind === "push") { + let bytes: Uint8Array; + try { + bytes = await reader.readFile(action.path); + } catch (err) { + return [{ path: action.path, message: localFailureMessage(err) }]; + } + const result = await storage.putObject(action.path, bytes); + if (!result.ok) { + return [{ path: action.path, message: result.message }]; + } + + return []; + } + + if (action.kind === "pushDelete") { + const result = await storage.deleteObject(action.path); + if (!result.ok) { + return [{ path: action.path, message: result.message }]; + } + + return []; + } + + if (action.kind === "pull") { + const drift = await checkLocalDrift(reader, action.path, localByPath.get(action.path)); + if (drift !== null) { + return [drift]; + } + const result = await storage.getObject(action.path); + if (!result.ok || result.body === null) { + return [{ path: action.path, message: result.message }]; + } + const body = result.body; + const failure = await applyLocalWrite(action.path, () => + localWriter.writeFile(action.path, body), + ); + if (failure !== null) { + return [failure]; + } + + return []; + } + + if (action.kind === "pullDelete") { + const drift = await checkLocalDrift(reader, action.path, localByPath.get(action.path)); + if (drift !== null) { + return [drift]; + } + const failure = await applyLocalWrite(action.path, () => localWriter.deleteFile(action.path)); + if (failure !== null) { + return [failure]; + } + + return []; + } + + // conflict, deletedSide "local": the user deleted their copy, so there is no local edit to + // preserve; the remote edit simply wins and is restored onto the local path. The snapshot has + // no entry here, so any file found now was recreated after it and must not be overwritten. + if (action.deletedSide === "local") { + const drift = await checkLocalDrift(reader, action.path, localByPath.get(action.path)); + if (drift !== null) { + return [drift]; + } + const result = await storage.getObject(action.path); + if (!result.ok || result.body === null) { + return [{ path: action.path, message: result.message }]; + } + const body = result.body; + const failure = await applyLocalWrite(action.path, () => + localWriter.writeFile(action.path, body), + ); + if (failure !== null) { + return [failure]; + } + + return []; + } + + // conflict, deletedSide "remote" or "none": preserve the local edit under a new name and + // push that copy to storage too, so the diverged edit lands on every device and the manifest + // we later upload isn't claiming a remote object that doesn't exist. Neither side's edit is + // ever silently discarded. + const copyPath = conflictCopyPath(action.path, now); + let localBytes: Uint8Array; + try { + localBytes = await reader.readFile(action.path); + } catch (err) { + return [{ path: action.path, message: localFailureMessage(err) }]; + } + // A failed rename means the local edit is still sitting at action.path untouched. Bail before + // the pull below would overwrite it, so a diverged edit is never silently discarded by an I/O + // error the way it would be if we pushed on to restore the remote version. + const renameFailure = await applyLocalWrite(action.path, () => + localWriter.renameFile(action.path, copyPath), + ); + if (renameFailure !== null) { + return [renameFailure]; + } + const failures: SyncFailure[] = []; + const pushed = await storage.putObject(copyPath, localBytes); + if (!pushed.ok) { + failures.push({ path: copyPath, message: pushed.message }); + } + + // deletedSide "remote": there is nothing at this path remotely to pull, the rename above + // already vacated it locally, and that is the correct final state, not a failure to report. + if (action.deletedSide === "remote") { + return failures; + } + + const result = await storage.getObject(action.path); + if (!result.ok || result.body === null) { + failures.push({ path: action.path, message: result.message }); + return failures; + } + const body = result.body; + const writeFailure = await applyLocalWrite(action.path, () => + localWriter.writeFile(action.path, body), + ); + if (writeFailure !== null) { + failures.push(writeFailure); + } + + return failures; +} + // localFailureMessage turns whatever a local vault operation threw into a SyncFailure message. // readFile throws when a file vanishes between the snapshot and now (a user deleting it mid sync), // and writeFile/deleteFile/renameFile can throw on a disk full or permission error; routing all of diff --git a/src/sync/sync.itest.ts b/src/sync/sync.itest.ts index 1736d37..f66c25b 100644 --- a/src/sync/sync.itest.ts +++ b/src/sync/sync.itest.ts @@ -81,13 +81,19 @@ async function deleteLocal(d: Device, path: string): Promise { } // sync runs one pass for a device, mirroring the plugin's runSync spine: read previous state, run -// syncOnce, persist the new snapshot on success. +// syncOnce, persist the new snapshot whenever one comes back (a full success, or a failed pass +// that still made progress). async function sync(d: Device, now = Date.now()): Promise { const previous = await d.stateStore.read(); const outcome = await syncOnce(previous, d.reader, d.writer, storage, now); if (outcome.ok) { await d.stateStore.write(outcome.snapshot); + return outcome; } + if (outcome.snapshot !== null) { + await d.stateStore.write(outcome.snapshot); + } + return outcome; } diff --git a/src/sync/sync.test.ts b/src/sync/sync.test.ts index c7be488..db31f8f 100644 --- a/src/sync/sync.test.ts +++ b/src/sync/sync.test.ts @@ -1,9 +1,10 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import { hashBytes, type Snapshot } from "../vault/vault.ts"; +import type { LocalWriter } from "./execute.ts"; import { empty, fakeLocalWriter, fakeReader, fakeStorage, file, snapshot } from "./fake.ts"; import { conflictCopyPath, MANIFEST_KEY } from "./plan.ts"; -import { adoptLiveStats, readRemoteManifest, syncOnce } from "./sync.ts"; +import { adoptLiveStats, readRemoteManifest, revertFailedPaths, syncOnce } from "./sync.ts"; // hashOf returns the real content hash of text, for snapshots whose entries executeSyncPlan's // drift check will verify against live bytes. @@ -154,6 +155,7 @@ test("syncOnce: a manifest overwritten by another device mid sync fails the pass ok: false, message: "another device synced at the same time; sync again", failures: [], + snapshot: null, }); // B's manifest survived; A's never landed. assert.equal(objects.get(MANIFEST_KEY), bManifest); @@ -243,6 +245,13 @@ test("syncOnce: a file edited mid sync is never overwritten by a pull, and the r const { writer, files } = fakeLocalWriter(); files.set("a.md", "a v1"); files.set("b.md", "b v1"); + // Mirror pulls into the reader, as writing to a real vault would: the retry below re-snapshots + // through the reader and must see what the first pass's completed pull actually left on disk. + const innerWrite = writer.writeFile; + writer.writeFile = async (path, data) => { + await innerWrite(path, data); + readerFiles[path] = new TextDecoder().decode(data); + }; const inner = storage.getObject; let edited = false; storage.getObject = async (key) => { @@ -257,15 +266,25 @@ test("syncOnce: a file edited mid sync is never overwritten by a pull, and the r const outcome = await syncOnce(ancestor, reader, writer, storage, now); - assert.equal(outcome.ok, false); - // The edit survived, a.md's pull still landed, and no manifest was uploaded. + assert.ok(!outcome.ok); + // The edit survived and a.md's pull still landed. assert.equal(files.get("b.md"), "edited mid sync"); assert.equal(files.get("a.md"), "a v2"); - assert.equal(objects.get(MANIFEST_KEY), remoteManifest); + // A manifest was still uploaded (#87), and it records exactly what the bucket really holds: + // both remote versions, untouched by this pass's pulls. + const manifestBody = objects.get(MANIFEST_KEY); + assert.ok(manifestBody !== undefined); + const manifest = JSON.parse(manifestBody) as Snapshot; + const hashes = new Map(manifest.files.map((f) => [f.path, f.hash])); + assert.equal(hashes.get("a.md"), await hashOf("a v2")); + assert.equal(hashes.get("b.md"), await hashOf("b v2")); - // The retry diffs against the same ancestor: b.md changed locally and remotely, a genuine - // conflict, so the edit is renamed to a conflict copy, pushed, and the remote version pulled. - const retry = await syncOnce(ancestor, reader, writer, storage, now); + // The pass still returned a snapshot recording its progress, with b.md held at the ancestor's + // view. The retry diffs b.md against that same ancestor: changed locally and remotely, a + // genuine conflict, so the edit is renamed to a conflict copy, pushed, and the remote version + // pulled. + assert.ok(outcome.snapshot !== null); + const retry = await syncOnce(outcome.snapshot, reader, writer, storage, now); assert.equal(retry.ok, true); const copyPath = conflictCopyPath("b.md", now); @@ -274,6 +293,161 @@ test("syncOnce: a file edited mid sync is never overwritten by a pull, and the r assert.equal(files.get("b.md"), "b v2"); }); +test("syncOnce: a failed push doesn't discard the progress of the rest of the pass", async () => { + // Reproduces #87. Two new local files; a.md's push is rejected by the provider, b.md's lands. + // Before the fix the pass bailed without uploading a manifest or returning a snapshot: b.md sat + // in the bucket invisible to every other device, all completed work was re-planned from scratch + // next time, and a file that fails permanently wedged sync forever. The pass must instead + // record b.md's progress in both the manifest and the returned snapshot, leaving only a.md + // pending. + const reader = fakeReader({ "a.md": "alpha", "b.md": "world" }); + const { writer } = fakeLocalWriter(); + const { storage, objects } = fakeStorage(); + const inner = storage.putObject; + let rejectA = true; + let bPushes = 0; + storage.putObject = async (key, body, condition) => { + if (key === "a.md" && rejectA) { + return { ok: false, status: "server", message: "Storage rejected the write (500)" }; + } + if (key === "b.md") { + bPushes++; + } + return inner(key, body, condition); + }; + + const outcome = await syncOnce(empty, reader, writer, storage, 1); + + assert.ok(!outcome.ok); + assert.deepEqual(outcome.failures, [ + { path: "a.md", message: "Storage rejected the write (500)" }, + ]); + // b.md's push landed and the manifest records it, so other devices can already see it; a.md + // never reached the bucket and the manifest doesn't claim it. + assert.equal(objects.get("b.md"), "world"); + const manifestBody = objects.get(MANIFEST_KEY); + assert.ok(manifestBody !== undefined); + const manifest = JSON.parse(manifestBody) as Snapshot; + assert.deepEqual( + manifest.files.map((f) => f.path), + ["b.md"], + ); + // The snapshot records the same progress: b.md done, a.md still absent so it re-plans as a + // push. + assert.ok(outcome.snapshot !== null); + assert.deepEqual( + outcome.snapshot.files.map((f) => f.path), + ["b.md"], + ); + + // Once the provider accepts a.md, the retry pushes only it; b.md's completed work is never + // re-done. + rejectA = false; + const retry = await syncOnce(outcome.snapshot, reader, writer, storage, 1); + + assert.equal(retry.ok, true); + assert.equal(objects.get("a.md"), "alpha"); + assert.equal(bPushes, 1); +}); + +test("syncOnce: the failure message counts files, not operation failures", async () => { + // A conflict whose copy push and pull both fail reports two operation failures for one vault + // path. The user facing message must count the one file, not the two operations. + const ancestor = snapshot({ path: "a.md", size: 4, mtime: 1, hash: await hashOf("a v1") }); + const remoteManifest = JSON.stringify( + snapshot({ path: "a.md", size: 4, mtime: 1, hash: await hashOf("a v2") }), + ); + // The manifest claims a.md but the object is absent, so the conflict's pull 404s; the copy push + // is rejected by the override below. Both sides changed relative to the ancestor, so the plan is + // a single conflict (deletedSide "none") for a.md. + const { storage } = fakeStorage({ [MANIFEST_KEY]: remoteManifest }); + const copyPath = conflictCopyPath("a.md", 1); + const inner = storage.putObject; + storage.putObject = async (key, body, condition) => { + if (key === copyPath) { + return { ok: false, status: "server", message: "Storage rejected the write (500)" }; + } + return inner(key, body, condition); + }; + const reader = fakeReader({ "a.md": "a local" }); + const { writer } = fakeLocalWriter(); + + const outcome = await syncOnce(ancestor, reader, writer, storage, 1); + + assert.ok(!outcome.ok); + assert.deepEqual(outcome.failures, [ + { path: copyPath, message: "Storage rejected the write (500)" }, + { path: "a.md", message: "Storage rejected the read (404)" }, + ]); + assert.equal(outcome.message, "1 file(s) failed to sync"); +}); + +test("syncOnce: a failed pull records progress without the ancestor ever advancing past it", async () => { + // The other half of #87, and the reason a failed action's path must revert to the ancestor's + // view: a.md was edited remotely and its pull fails (a locked file, say), while b.md is new + // remotely and pulls fine. The pass must record b.md's progress, but if a.md's entry advanced + // to the manifest's view the unchanged local copy would read as a fresh local edit on the next + // pass and be pushed over the newer remote version, quietly undoing the remote edit. + const ancestor = snapshot({ path: "a.md", size: 4, mtime: 1, hash: await hashOf("a v1") }); + const remoteManifest = JSON.stringify( + snapshot( + { path: "a.md", size: 4, mtime: 1, hash: await hashOf("a v2") }, + { path: "b.md", size: 3, mtime: 1, hash: await hashOf("bee") }, + ), + ); + const { storage, objects } = fakeStorage({ + [MANIFEST_KEY]: remoteManifest, + "a.md": "a v2", + "b.md": "bee", + }); + const inner = storage.putObject; + let aPushes = 0; + storage.putObject = async (key, body, condition) => { + if (key === "a.md") { + aPushes++; + } + return inner(key, body, condition); + }; + const readerFiles: Record = { "a.md": "a v1" }; + const reader = fakeReader(readerFiles); + let lockA = true; + const writer: LocalWriter = { + writeFile: async (path, data) => { + if (path === "a.md" && lockA) { + throw new Error("EBUSY: resource busy or locked"); + } + readerFiles[path] = new TextDecoder().decode(data); + }, + deleteFile: async (path) => { + delete readerFiles[path]; + }, + renameFile: async () => { + throw new Error("unexpected rename"); + }, + }; + + const outcome = await syncOnce(ancestor, reader, writer, storage, 1); + + assert.ok(!outcome.ok); + assert.deepEqual(outcome.failures, [{ path: "a.md", message: "EBUSY: resource busy or locked" }]); + // b.md's pull landed and is recorded; a.md stays at the ancestor's view, not the manifest's. + assert.equal(readerFiles["b.md"], "bee"); + assert.ok(outcome.snapshot !== null); + const entries = new Map(outcome.snapshot.files.map((f) => [f.path, f.hash])); + assert.equal(entries.get("a.md"), await hashOf("a v1")); + assert.equal(entries.get("b.md"), await hashOf("bee")); + + // Once the file unlocks, the retry pulls the newer remote version; the stale local copy is + // never pushed over it. + lockA = false; + const retry = await syncOnce(outcome.snapshot, reader, writer, storage, 1); + + assert.equal(retry.ok, true); + assert.equal(readerFiles["a.md"], "a v2"); + assert.equal(objects.get("a.md"), "a v2"); + assert.equal(aPushes, 0); +}); + test("syncOnce: two first syncs racing for an empty bucket, the loser fails instead of clobbering", async () => { // Both devices see no manifest and plan a first sync. The other device's manifest lands while // this one is mid pass; the "ifAbsent" conditional upload must lose rather than overwrite it. @@ -324,3 +498,20 @@ test("adoptLiveStats: a mid sync creation is never added to the manifest", () => assert.deepEqual(adoptLiveStats(empty, live), empty); }); + +test("revertFailedPaths: a failed action's path is restored to the ancestor's entry", () => { + const manifest = snapshot(file("a.md", "h2"), file("b.md", "h3")); + const ancestor = snapshot(file("a.md", "h1")); + + const result = revertFailedPaths(manifest, ancestor, [{ kind: "pull", path: "a.md" }]); + + assert.deepEqual(result, snapshot(file("a.md", "h1"), file("b.md", "h3"))); +}); + +test("revertFailedPaths: a path the ancestor never knew is dropped, so it re-plans from scratch", () => { + const manifest = snapshot(file("a.md", "h2")); + + const result = revertFailedPaths(manifest, empty, [{ kind: "pull", path: "a.md" }]); + + assert.deepEqual(result, empty); +}); diff --git a/src/sync/sync.ts b/src/sync/sync.ts index 4139ee8..8bd84ea 100644 --- a/src/sync/sync.ts +++ b/src/sync/sync.ts @@ -8,14 +8,18 @@ import { takeSnapshot, } from "../vault/vault.ts"; import { executeSyncPlan, type LocalWriter, type SyncFailure } from "./execute.ts"; -import { MANIFEST_KEY, manifestAfterSync, planSync } from "./plan.ts"; +import { MANIFEST_KEY, manifestAfterSync, planSync, type SyncAction } from "./plan.ts"; // SyncOutcome is the result of a single sync pass. On success it carries the new snapshot to // persist as the next sync's starting point and how many actions were applied; on failure it -// carries a short user facing message and any per file failures for logging. +// carries a short user facing message and any per file failures for logging. A failure outcome +// still carries a snapshot when the pass made progress worth persisting (#87): completed actions +// are recorded so they are never re-planned, while each failed action's path stays at the +// ancestor's view and is re-planned next pass. snapshot is null when nothing advanced (the +// manifest never uploaded, or never got that far). export type SyncOutcome = | { ok: true; snapshot: Snapshot; changeCount: number } - | { ok: false; message: string; failures: SyncFailure[] }; + | { ok: false; message: string; failures: SyncFailure[]; snapshot: Snapshot | null }; // adoptLiveStats returns manifest with each entry swapped for the live vault's entry at the same // path wherever the content hashes match, so state.json carries local size and mtime and the next @@ -89,6 +93,32 @@ export async function readRemoteManifest( return { ok: false, message: fetched.message }; } +// revertFailedPaths returns snapshot with every failed action's path restored to the ancestor's +// view of it, so state.json never advances past an action that didn't complete: those paths diff +// against the same ancestor next pass and are re-planned, while every completed path keeps its +// new entry. Reverting is what makes recording progress around a failed pull safe — advancing +// that path to the manifest's entry would make the unchanged local content read as a fresh local +// edit, and the next pass would push it over the newer remote version. Exported for its tests; +// syncOnce is the only production caller. +export function revertFailedPaths( + snapshot: Snapshot, + ancestor: Snapshot, + failed: SyncAction[], +): Snapshot { + const files = byPath(snapshot.files); + const ancestorByPath = byPath(ancestor.files); + for (const action of failed) { + const entry = ancestorByPath.get(action.path); + if (entry === undefined) { + files.delete(action.path); + continue; + } + files.set(action.path, entry); + } + + return { files: [...files.values()] }; +} + // syncOnce runs one full sync pass over the injected local vault (reader/localWriter) and remote // bucket (storage): it snapshots the local vault against previous (the last synced snapshot), // reads the remote manifest, plans and executes the reconciliation, then uploads a manifest @@ -105,7 +135,7 @@ export async function syncOnce( ): Promise { const remote = await readRemoteManifest(storage); if (!remote.ok) { - return { ok: false, message: remote.message, failures: [] }; + return { ok: false, message: remote.message, failures: [], snapshot: null }; } // No remote manifest means no prior sync ever completed against this bucket, so previous (the @@ -121,18 +151,18 @@ export async function syncOnce( const local = await takeSnapshot(reader, ancestor); const actions = planSync(ancestor, local, remote.snapshot); - const failures = await executeSyncPlan(actions, local, reader, localWriter, storage, now); - if (failures.length > 0) { - return { ok: false, message: `${failures.length} file(s) failed to sync`, failures }; - } + const executed = await executeSyncPlan(actions, local, reader, localWriter, storage, now); // The manifest is derived from what the plan just did to the bucket, never from a fresh disk // snapshot: a file edited while the plan ran would land in a re-snapshot claiming content the // bucket never received, the edit would then never upload (state.json already agrees with the // manifest), and another device could later push the stale bucket copy back over it (#84). The // re-snapshot here only refreshes stats, so a mid sync edit keeps its bucket entry and reads as - // a local change on the next pass. - const manifest = manifestAfterSync(local, remote.snapshot, actions, now); + // a local change on the next pass. Only completed actions feed in, so a failed action's path + // keeps the entry the bucket really holds; the manifest is uploaded even when some actions + // failed, so one bad file never leaves the rest of the pass's pushes invisible to every other + // device (#87). + const manifest = manifestAfterSync(local, remote.snapshot, executed.completed, now); const final = adoptLiveStats(manifest, await takeSnapshot(reader, local)); const manifestBody = new TextEncoder().encode(JSON.stringify(final)); @@ -152,10 +182,22 @@ export async function syncOnce( return { ok: false, message: "another device synced at the same time; sync again", - failures: [], + failures: executed.failures, + snapshot: null, }; } - return { ok: false, message: uploaded.message, failures: [] }; + return { ok: false, message: uploaded.message, failures: executed.failures, snapshot: null }; + } + + // The count comes from failed (one entry per planned path), not failures: a conflict can report + // two operation failures (copy push and pull) for the same file, and the message counts files. + if (executed.failed.length > 0) { + return { + ok: false, + message: `${executed.failed.length} file(s) failed to sync`, + failures: executed.failures, + snapshot: revertFailedPaths(final, ancestor, executed.failed), + }; } return { ok: true, snapshot: final, changeCount: actions.length };