diff --git a/src/sync/plan.test.ts b/src/sync/plan.test.ts index ddc9f37..ba698ac 100644 --- a/src/sync/plan.test.ts +++ b/src/sync/plan.test.ts @@ -1,7 +1,7 @@ import assert from "node:assert/strict"; import { test } from "node:test"; import { empty, file, snapshot } from "./fake.ts"; -import { conflictCopyPath, MANIFEST_KEY, planSync } from "./plan.ts"; +import { conflictCopyPath, MANIFEST_KEY, manifestAfterSync, planSync } from "./plan.ts"; test("planSync: a path only changed locally is pushed", () => { const previous = empty; @@ -89,6 +89,87 @@ test("planSync: the manifest's own path is never turned into an action", () => { assert.deepEqual(planSync(previous, local, remote), []); }); +test("manifestAfterSync: a push records the local snapshot's entry", () => { + const local = snapshot(file("a.md", "h2")); + const remote = snapshot(file("a.md", "h1"), file("b.md", "h3")); + + const result = manifestAfterSync(local, remote, [{ kind: "push", path: "a.md" }], 1); + + assert.deepEqual(result, snapshot(file("a.md", "h2"), file("b.md", "h3"))); +}); + +test("manifestAfterSync: a pushDelete removes the entry", () => { + const local = empty; + const remote = snapshot(file("a.md", "h1"), file("b.md", "h3")); + + const result = manifestAfterSync(local, remote, [{ kind: "pushDelete", path: "a.md" }], 1); + + assert.deepEqual(result, snapshot(file("b.md", "h3"))); +}); + +test("manifestAfterSync: pull and pullDelete leave the bucket, and so the manifest, untouched", () => { + const local = empty; + const remote = snapshot(file("a.md", "h1")); + + const result = manifestAfterSync( + local, + remote, + [ + { kind: "pull", path: "a.md" }, + { kind: "pullDelete", path: "b.md" }, + ], + 1, + ); + + assert.deepEqual(result, remote); +}); + +test("manifestAfterSync: a content conflict keeps the remote entry and adds the pushed copy", () => { + const now = Date.parse("2026-07-14T10:00:00.000Z"); + const local = snapshot(file("a.md", "h2")); + const remote = snapshot(file("a.md", "h3")); + + const result = manifestAfterSync( + local, + remote, + [{ kind: "conflict", path: "a.md", deletedSide: "none" }], + now, + ); + + const copyPath = "a (conflicted copy 2026-07-14T10-00-00-000Z).md"; + assert.deepEqual(result, snapshot(file("a.md", "h3"), { ...file("a.md", "h2"), path: copyPath })); +}); + +test("manifestAfterSync: a remote deletion conflict records only the pushed copy", () => { + const now = Date.parse("2026-07-14T10:00:00.000Z"); + const local = snapshot(file("a.md", "h2")); + const remote = empty; + + const result = manifestAfterSync( + local, + remote, + [{ kind: "conflict", path: "a.md", deletedSide: "remote" }], + now, + ); + + const copyPath = "a (conflicted copy 2026-07-14T10-00-00-000Z).md"; + assert.deepEqual(result, snapshot({ ...file("a.md", "h2"), path: copyPath })); +}); + +test("manifestAfterSync: a local deletion conflict pushes nothing, the remote entry stands", () => { + const local = empty; + const remote = snapshot(file("a.md", "h2")); + + const result = manifestAfterSync( + local, + remote, + [{ kind: "conflict", path: "a.md", deletedSide: "local" }], + 1, + ); + + assert.deepEqual(result, remote); +}); + test("conflictCopyPath: keeps the extension", () => { assert.equal( conflictCopyPath("notes/todo.md", Date.parse("2026-07-14T10:00:00.000Z")), diff --git a/src/sync/plan.ts b/src/sync/plan.ts index a6da2c5..7a6b6e6 100644 --- a/src/sync/plan.ts +++ b/src/sync/plan.ts @@ -29,6 +29,57 @@ export function conflictCopyPath(path: string, now: number): string { return `${path.slice(0, lastDot)} (conflicted copy ${stamp})${path.slice(lastDot)}`; } +// manifestAfterSync returns the snapshot of what the bucket holds once every action in the plan +// has succeeded: remote as it was read, minus pushed deletions, plus pushed files and conflict +// copies recorded at the local snapshot's entry. It is computed from the plan rather than +// re-snapshotted from disk so the manifest can never record content the bucket does not have +// (#84): a file that changed while the plan ran keeps its bucket entry, and the next sync sees +// the drift as a local change and pushes it. The one race left, a push whose bytes drifted past +// the local snapshot before they were read, only ever understates the bucket, and the next pass +// simply pushes again. +export function manifestAfterSync( + local: Snapshot, + remote: Snapshot, + actions: SyncAction[], + now: number, +): Snapshot { + const files = byPath(remote.files); + const localByPath = byPath(local.files); + + for (const action of actions) { + // pull and pullDelete only change the local vault; the bucket is untouched. + if (action.kind === "pull" || action.kind === "pullDelete") { + continue; + } + if (action.kind === "pushDelete") { + files.delete(action.path); + continue; + } + if (action.kind === "push") { + // A push is only ever planned for a file present in the local snapshot, so the guard is + // narrowing, not a real branch; a miss would mean planSync broke that invariant. + const pushed = localByPath.get(action.path); + if (pushed !== undefined) { + files.set(action.path, pushed); + } + continue; + } + // conflict: a local deletion pushes nothing, the remote entry stands as is. The other two + // sides push the local edit under its conflict copy name; the original path is already + // correct in remote (present for deletedSide "none", absent for "remote"). + if (action.deletedSide === "local") { + continue; + } + const copied = localByPath.get(action.path); + if (copied !== undefined) { + const copyPath = conflictCopyPath(action.path, now); + files.set(copyPath, { ...copied, path: copyPath }); + } + } + + return { files: [...files.values()] }; +} + // planSync compares what changed locally since the last successful sync against what changed // remotely since that same sync, and decides what to push, what to pull, and what's a genuine // conflict: a path that changed on both sides to different content. previous is the snapshot diff --git a/src/sync/sync.test.ts b/src/sync/sync.test.ts index 81ea8b8..164e50f 100644 --- a/src/sync/sync.test.ts +++ b/src/sync/sync.test.ts @@ -3,7 +3,7 @@ import { test } from "node:test"; import type { Snapshot } from "../vault/vault.ts"; import { empty, fakeLocalWriter, fakeReader, fakeStorage, file, snapshot } from "./fake.ts"; import { MANIFEST_KEY } from "./plan.ts"; -import { readRemoteManifest, syncOnce } from "./sync.ts"; +import { adoptLiveStats, readRemoteManifest, syncOnce } from "./sync.ts"; test("readRemoteManifest: a 404 is treated as an empty snapshot", async () => { const { storage } = fakeStorage(); @@ -164,6 +164,55 @@ test("syncOnce: a manifest overwritten by another device mid sync fails the pass assert.equal(files.get("a.md"), "xy"); }); +test("syncOnce: a file changed mid sync is not recorded in the manifest and is pushed next pass", async () => { + // Reproduces #84. The vault is in sync (a.md, unchanged), and b.md is new locally, so the pass + // pushes b.md. While that push is in flight the user edits a.md and creates c.md. Before the + // fix the manifest was a re-snapshot of the disk taken after the plan ran, so it recorded both + // with content the bucket never received; neither then ever uploaded (state.json already agreed + // with the manifest), and another device could push the stale bucket copy of a.md back over the + // edit. The manifest must instead keep claiming only what the bucket holds, leaving both files + // as local changes for the next pass to push. + const ancestor = snapshot(file("a.md", "h1")); + const { storage, objects } = fakeStorage({ [MANIFEST_KEY]: JSON.stringify(ancestor) }); + // a.md matches the ancestor's size and mtime so takeSnapshot reuses its hash and sees no local + // change there; b.md is the new local file whose push is the mid sync moment to interleave on. + const readerFiles: Record = { "a.md": "xy", "b.md": "beta" }; + const reader = fakeReader(readerFiles); + const { writer } = fakeLocalWriter(); + const inner = storage.putObject; + let edited = false; + storage.putObject = async (key, body, condition) => { + if (key === "b.md" && !edited) { + edited = true; + readerFiles["a.md"] = "edited mid sync"; + readerFiles["c.md"] = "created mid sync"; + } + return inner(key, body, condition); + }; + + const outcome = await syncOnce(ancestor, reader, writer, storage, 1); + + assert.ok(outcome.ok); + // The manifest still records a.md as the bucket knows it, and doesn't know c.md at all: neither + // file's new content ever reached the bucket. + const manifestBody = objects.get(MANIFEST_KEY); + assert.ok(manifestBody !== undefined); + const manifest = JSON.parse(manifestBody) as Snapshot; + const paths = manifest.files.map((f) => f.path); + assert.deepEqual(paths.sort(), ["a.md", "b.md"]); + assert.deepEqual( + manifest.files.filter((f) => f.path === "a.md"), + [file("a.md", "h1")], + ); + assert.equal(objects.has("c.md"), false); + + // The next pass sees both as plain local changes and pushes them. + const retry = await syncOnce(outcome.snapshot, reader, writer, storage, 1); + assert.equal(retry.ok, true); + assert.equal(objects.get("a.md"), "edited mid sync"); + assert.equal(objects.get("c.md"), "created mid sync"); +}); + 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. @@ -188,3 +237,29 @@ test("syncOnce: two first syncs racing for an empty bucket, the loser fails inst assert.equal(objects.get(MANIFEST_KEY), otherManifest); assert.equal(files.get("a.md"), "alpha"); }); + +test("adoptLiveStats: an entry whose content matches the live vault adopts the live stats", () => { + const manifest = snapshot({ path: "a.md", size: 2, mtime: 5, hash: "h1" }); + const live = snapshot({ path: "a.md", size: 2, mtime: 9, hash: "h1" }); + + assert.deepEqual(adoptLiveStats(manifest, live), live); +}); + +test("adoptLiveStats: a mid sync edit keeps the manifest's entry, so the next diff sees it", () => { + const manifest = snapshot(file("a.md", "h1")); + const live = snapshot({ path: "a.md", size: 7, mtime: 9, hash: "h2" }); + + assert.deepEqual(adoptLiveStats(manifest, live), manifest); +}); + +test("adoptLiveStats: a mid sync deletion keeps the manifest's entry, so the next diff sees it", () => { + const manifest = snapshot(file("a.md", "h1")); + + assert.deepEqual(adoptLiveStats(manifest, empty), manifest); +}); + +test("adoptLiveStats: a mid sync creation is never added to the manifest", () => { + const live = snapshot(file("c.md", "h9")); + + assert.deepEqual(adoptLiveStats(empty, live), empty); +}); diff --git a/src/sync/sync.ts b/src/sync/sync.ts index 92db9e5..155c515 100644 --- a/src/sync/sync.ts +++ b/src/sync/sync.ts @@ -1,7 +1,14 @@ import type { PutCondition, StorageClient } from "../storage/storage.ts"; -import { isSnapshot, type Reader, type Snapshot, takeSnapshot } from "../vault/vault.ts"; +import { + byPath, + type FileState, + isSnapshot, + type Reader, + type Snapshot, + takeSnapshot, +} from "../vault/vault.ts"; import { executeSyncPlan, type LocalWriter, type SyncFailure } from "./execute.ts"; -import { MANIFEST_KEY, planSync } from "./plan.ts"; +import { MANIFEST_KEY, manifestAfterSync, planSync } 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 @@ -10,6 +17,27 @@ export type SyncOutcome = | { ok: true; snapshot: Snapshot; changeCount: number } | { ok: false; message: string; failures: SyncFailure[] }; +// 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 +// snapshot can stat-skip the rehash. An entry whose live content differs (a mid sync edit) and a +// live file the manifest doesn't know (a mid sync creation) both keep the manifest's view, so the +// next sync's diff picks them up as local changes. Exported for its tests; syncOnce is the only +// production caller. +export function adoptLiveStats(manifest: Snapshot, live: Snapshot): Snapshot { + const liveByPath = byPath(live.files); + const files: FileState[] = []; + for (const entry of manifest.files) { + const liveEntry = liveByPath.get(entry.path); + if (liveEntry !== undefined && liveEntry.hash === entry.hash) { + files.push(liveEntry); + continue; + } + files.push(entry); + } + + return { files }; +} + // readRemoteManifest fetches and parses the remote manifest. A confirmed 404 means no manifest // has ever been written, the safe assumption for a first sync against an empty bucket, so that's // treated as an empty snapshot flagged firstSync. Any other failure (network, auth, a real 5xx) @@ -63,8 +91,8 @@ export async function readRemoteManifest( // 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 re-snapshots and uploads -// the manifest so it always matches what is really on disk. previous is passed in and the new +// reads the remote manifest, plans and executes the reconciliation, then uploads a manifest +// reflecting what the bucket now actually holds. previous is passed in and the new // snapshot returned rather than read or written internally, so the caller owns persistence (the // plugin through state.json, tests through their own store) and this stays pure over its inputs. // now is injected so a conflict copy's name is deterministic under test. @@ -98,10 +126,14 @@ export async function syncOnce( return { ok: false, message: `${failures.length} file(s) failed to sync`, failures }; } - // Re-snapshot rather than hand merging local with the plan's outcome: this is the only way to - // be sure the manifest we upload matches what's really on disk after every pull, delete, and - // conflict rename just applied. - const final = await takeSnapshot(reader, local); + // 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); + const final = adoptLiveStats(manifest, await takeSnapshot(reader, local)); const manifestBody = new TextEncoder().encode(JSON.stringify(final)); // The upload is conditional on the remote manifest still being exactly what this pass read at