From a60b02ad847872f8d3ff1f1623f129b5fa4f65ac Mon Sep 17 00:00:00 2001 From: Nathan Disidore Date: Fri, 5 Jun 2026 19:04:10 -0500 Subject: [PATCH 1/3] dofs: Track provider renames in sync Move provider renames through a filesystem primitive so local rename behavior and sync output share one implementation. A rename stamps the moved inode subtree with one revision and records tombstones for the old paths, letting the existing change stream represent moves without a new wire opcode. Directory renames are O(subtree) in database writes and wire entries. That cost is explicit in the protocol docs. The apply path also resolves type conflicts by replacing the local node tree with the upstream entry, and rm now unlinks final symlinks without following them to their targets. rename and rm resolve symlinked parents to a real path before they mutate, so the read-only mount guard is re-checked against that resolved source and destination. The earlier guard only saw the unresolved request, which let a symlink into a read-only mount carry a delete or a move past it. Structural replacement and removal now unlink one dirent at a time and reap the inode only once its last link is gone, so a sibling hardlink survives a type change at another name. That refcount-gated unlink lives in one helper shared by rm, rename, and the apply path. The directory self-move guard is inode-based: it tests the resolved destination parent against the source subtree, so a destination that traverses a symlink out of the source is allowed while one that lands back inside is rejected. A textual prefix test on the unresolved destination could do neither and is gone. --- docs/02_sync_protocol.md | 22 ++ docs/03_filesystem_schema.md | 6 +- packages/dofs/src/fs/mount-guard.test.ts | 75 ++++ packages/dofs/src/fs/rename.ts | 211 +++++++++++ packages/dofs/src/fs/rm.test.ts | 60 +++ packages/dofs/src/fs/rm.ts | 43 ++- packages/dofs/src/fs/unlink.ts | 34 ++ packages/dofs/src/provider.test.ts | 347 ++++++++++++++++- packages/dofs/src/provider.ts | 113 +----- packages/dofs/src/sync/apply.test.ts | 455 ++++++++++++++++++++++- packages/dofs/src/sync/apply.ts | 159 +++++++- packages/dofs/src/sync/coalesce.test.ts | 23 ++ packages/dofs/src/sync/coalesce.ts | 28 +- packages/dofs/src/sync/paths.ts | 26 ++ 14 files changed, 1445 insertions(+), 157 deletions(-) create mode 100644 packages/dofs/src/fs/rename.ts create mode 100644 packages/dofs/src/fs/unlink.ts create mode 100644 packages/dofs/src/sync/paths.ts diff --git a/docs/02_sync_protocol.md b/docs/02_sync_protocol.md index 1dd6d99a..1bee5fb9 100644 --- a/docs/02_sync_protocol.md +++ b/docs/02_sync_protocol.md @@ -83,6 +83,28 @@ A typical `exec()` round-trip: step 1 is "this single change", steps 3–6 are skipped. `workspace.push()` runs step 1 on demand; `workspace.pull()` runs steps 4–6. +Renames are local inode moves, but the sync wire has no rename opcode. +The wire stays final-state based — live entries plus tombstones — so +apply remains idempotent and does not need operation-order replay. The +cost is that a directory rename stamps every moved inode with one new +revision and records tombstones for the old paths in one synchronous +transaction. Large directory renames are therefore O(subtree) in local +writes and wire entries, with no separate cap beyond the caller's own +workload. Parent directory mtimes are not changed by rename, which +differs from POSIX `rename(2)` but keeps parent directory metadata out +of content sync. + +Directory entries carry mode and mtime. New directories are created +with the incoming mtime, but idempotence for an existing directory is +mode-only. That keeps mtime drift on matching directories from +becoming sync traffic. + +When an upstream file, directory, or symlink lands where the receiver +has a different node type, the receiver removes the local node tree and +applies the upstream entry. This is last-writer-wins conflict handling: +it converges the tree, but local-only children under the conflicting +path are discarded without separate tombstones. + ### Chunking Files are split at a fixed `CHUNK_SIZE` (512 KiB). Chunk boundaries are diff --git a/docs/03_filesystem_schema.md b/docs/03_filesystem_schema.md index f61a458d..87c36363 100644 --- a/docs/03_filesystem_schema.md +++ b/docs/03_filesystem_schema.md @@ -14,8 +14,10 @@ ultimately hits one of these tables. All tables are prefixed with collide with application-owned tables in the same DO storage. Paths are resolved through an inode-style indirection (`vfs_dirents` -→ `vfs_nodes`), so renames are O(1) regardless of subtree size and -hardlinks fall out for free. +→ `vfs_nodes`), so the local namespace move in a rename is O(1) and +hardlinks fall out for free. Directory rename sync still walks the +moved subtree because the wire represents the move as live entries at +the new paths plus tombstones at the old paths. ## Tables diff --git a/packages/dofs/src/fs/mount-guard.test.ts b/packages/dofs/src/fs/mount-guard.test.ts index 03bfa575..cc729a48 100644 --- a/packages/dofs/src/fs/mount-guard.test.ts +++ b/packages/dofs/src/fs/mount-guard.test.ts @@ -7,7 +7,10 @@ import { getReadOnlyMountRoots, invalidateReadOnlyMountCache, } from "./mount-guard.js"; +import { rename } from "./rename.js"; +import { resolveInode } from "./resolve.js"; import { rm } from "./rm.js"; +import { symlink } from "./symlink.js"; import { withDB } from "./with-db.js"; import { writeFile, writeFileSync } from "./writeFile.js"; @@ -240,4 +243,76 @@ describe("rm under a read-only mount", () => { expect(() => rm(db, "/workspace/rw/hi.txt", {})).not.toThrow(); }); }); + + it("rejects rm through a symlinked parent that resolves into a read-only mount", async () => { + await withDB((db) => { + mkdir(db, "/mnt", { recursive: true }, () => 0); + writeFileSync(db, "/mnt/file.txt", new Uint8Array([1]), {}, () => 0); + symlink(db, "/mnt", "/link", () => 0); + stageMount(db, "/mnt", "read-only"); + const before = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + expect(() => rm(db, "/link/file.txt", {})).toThrowError( + expect.objectContaining({ code: "EROFS" }), + ); + + expect(resolveInode(db, "/mnt/file.txt")).not.toBeNull(); + expect(db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'")).toBe(before); + }); + }); + + it("rejects recursive rm of a directory inside a read-only mount via a symlinked parent", async () => { + await withDB((db) => { + mkdir(db, "/mnt/dir", { recursive: true }, () => 0); + writeFileSync(db, "/mnt/dir/file.txt", new Uint8Array([1]), {}, () => 0); + symlink(db, "/mnt", "/link", () => 0); + stageMount(db, "/mnt", "read-only"); + const before = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + expect(() => rm(db, "/link/dir", { recursive: true })).toThrowError( + expect.objectContaining({ code: "EROFS" }), + ); + + expect(resolveInode(db, "/mnt/dir/file.txt")).not.toBeNull(); + expect(db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'")).toBe(before); + }); + }); +}); + +describe("rename under a read-only mount", () => { + it("rejects rename from a symlinked parent that resolves into a read-only mount", async () => { + await withDB((db) => { + mkdir(db, "/mnt", { recursive: true }, () => 0); + writeFileSync(db, "/mnt/file.txt", new Uint8Array([1]), {}, () => 0); + symlink(db, "/mnt", "/link", () => 0); + stageMount(db, "/mnt", "read-only"); + const before = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + expect(() => rename(db, "/link/file.txt", "/moved.txt")).toThrowError( + expect.objectContaining({ code: "EROFS" }), + ); + + expect(resolveInode(db, "/mnt/file.txt")).not.toBeNull(); + expect(resolveInode(db, "/moved.txt")).toBeNull(); + expect(db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'")).toBe(before); + }); + }); + + it("rejects rename to a symlinked parent that resolves into a read-only mount", async () => { + await withDB((db) => { + mkdir(db, "/mnt", { recursive: true }, () => 0); + writeFileSync(db, "/src.txt", new Uint8Array([1]), {}, () => 0); + symlink(db, "/mnt", "/link", () => 0); + stageMount(db, "/mnt", "read-only"); + const before = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + expect(() => rename(db, "/src.txt", "/link/file.txt")).toThrowError( + expect.objectContaining({ code: "EROFS" }), + ); + + expect(resolveInode(db, "/src.txt")).not.toBeNull(); + expect(resolveInode(db, "/mnt/file.txt")).toBeNull(); + expect(db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'")).toBe(before); + }); + }); }); diff --git a/packages/dofs/src/fs/rename.ts b/packages/dofs/src/fs/rename.ts new file mode 100644 index 00000000..d9398a01 --- /dev/null +++ b/packages/dofs/src/fs/rename.ts @@ -0,0 +1,211 @@ +import { createWorkspaceError } from "../errors.js"; +import { canonicalizePath } from "../path.js"; +import { incrementRev } from "../rev.js"; +import type { Database } from "../storage.js"; +import { recordDelete } from "../sync/changes.js"; +import { pathOf } from "../sync/paths.js"; +import { assertNotReadOnly } from "./mount-guard.js"; +import { resolveInode } from "./resolve.js"; +import { unlinkDirent } from "./unlink.js"; + +interface DirChild { + name: string; + child_inode: number; + type: NodeType; +} + +interface SubtreeEntry { + path: string; + inode: number; + type: NodeType; +} + +type NodeType = "file" | "dir" | "symlink"; + +export function rename(db: Database, oldPath: string, newPath: string): void { + const { path: oldCanonical } = canonicalizePath(oldPath); + const { parts: newParts, path: newCanonical } = canonicalizePath(newPath); + + if (oldCanonical === "/") { + throw createWorkspaceError("EINVAL", "cannot rename root", oldCanonical); + } + if (newParts.length === 0) { + throw createWorkspaceError("EINVAL", "cannot rename onto root", newCanonical); + } + + assertNotReadOnly(db, oldCanonical); + assertNotReadOnly(db, newCanonical); + + db.transactionSync(() => { + const source = resolveInode(db, oldCanonical, { followSymlinks: false }); + if (source === null) { + throw createWorkspaceError("ENOENT", `no such path: ${oldCanonical}`, oldCanonical); + } + + // Resolve the source's real parent dirent. The parent path is + // resolved with symlinks followed so a request through a symlinked + // directory lands on the real container; the inode is then + // identified by (parent_inode, name) rather than by child_inode so + // a hardlinked source touches only the requested name. + const { parts: oldParts } = canonicalizePath(oldCanonical); + const oldName = oldParts[oldParts.length - 1]; + const oldParentPath = oldParts.length === 1 ? "/" : `/${oldParts.slice(0, -1).join("/")}`; + const oldParent = resolveInode(db, oldParentPath); + if (oldParent === null || oldParent.type !== "dir") { + throw createWorkspaceError("ENOENT", `no such path: ${oldCanonical}`, oldCanonical); + } + const oldParentReal = pathOf(db, oldParent.inode); + if (oldParentReal === null) { + throw createWorkspaceError("ENOENT", `no such path: ${oldCanonical}`, oldCanonical); + } + const oldRealPath = oldParentReal === "/" ? `/${oldName}` : `${oldParentReal}/${oldName}`; + assertNotReadOnly(db, oldRealPath); + + if (oldCanonical === newCanonical) return; + + const newName = newParts[newParts.length - 1]; + const newParentPath = newParts.length === 1 ? "/" : `/${newParts.slice(0, -1).join("/")}`; + const newParent = resolveInode(db, newParentPath); + if (newParent === null || newParent.type !== "dir") { + throw createWorkspaceError( + "ENOENT", + `parent directory missing: ${newCanonical}`, + newCanonical, + ); + } + const newParentReal = pathOf(db, newParent.inode); + if (newParentReal === null) { + throw createWorkspaceError( + "ENOENT", + `parent directory missing: ${newCanonical}`, + newCanonical, + ); + } + const newRealPath = newParentReal === "/" ? `/${newName}` : `${newParentReal}/${newName}`; + assertNotReadOnly(db, newRealPath); + + // A rename whose source and destination resolve to the very same + // dirent (same real parent and name, e.g. through a symlinked path) + // is a true no-op: leave the tree and the change stream untouched. + // This is distinct from renaming one hardlink onto another, where + // the names differ and the source link must still be removed. + if (oldParent.inode === newParent.inode && oldName === newName) return; + + const existing = db.one<{ child_inode: number; type: "file" | "dir" | "symlink" }>( + `SELECT d.child_inode AS child_inode, n.type AS type + FROM vfs_dirents d + JOIN vfs_nodes n ON n.inode = d.child_inode + WHERE d.parent_inode = ? AND d.name = ?`, + newParent.inode, + newName, + ); + + const oldEntries = + source.type === "dir" + ? collectSubtree(db, source.inode, oldRealPath) + : [{ path: oldRealPath, inode: source.inode, type: source.type }]; + if (source.type === "dir") { + // Authoritative directory self-move guard. It tests the *resolved* + // destination parent inode against the source subtree, so it + // catches a symlinked destination that lands inside the source and + // allows one that resolves outside it. A textual prefix test on the + // unresolved path cannot do either and is intentionally absent. + assertDestinationParentOutsideSource(oldEntries, newParent.inode, oldRealPath, newCanonical); + } + + if (existing !== undefined) { + assertCompatibleOverwrite(source.type, existing.type, newCanonical); + if (existing.type === "dir") { + const childCount = db.scalar( + "SELECT COUNT(*) FROM vfs_dirents WHERE parent_inode = ?", + existing.child_inode, + ); + if ((childCount ?? 0) > 0) { + throw createWorkspaceError("ENOTEMPTY", `not empty: ${newCanonical}`, newCanonical); + } + } + // Displace only the destination name. The displaced inode may + // carry other hardlinks (or be the source inode itself), so reap + // its chunks and node row only once the final link disappears. + // Order matters: displace before unlinking the source so a + // hardlink-onto-hardlink rename never momentarily drops to zero + // links and reaps the inode it is about to re-point. + unlinkDirent(db, newParent.inode, newName, existing.child_inode, existing.type); + } + + // Unlink only the source name; a hardlinked source keeps its other + // names alive. + db.run("DELETE FROM vfs_dirents WHERE parent_inode = ? AND name = ?", oldParent.inode, oldName); + db.run( + "INSERT INTO vfs_dirents (parent_inode, name, child_inode) VALUES (?, ?, ?)", + newParent.inode, + newName, + source.inode, + ); + + const rev = incrementRev(db); + // Rename is represented on the wire as old-path tombstones plus + // live entries for the moved inode subtree, so stamp only that + // subtree with the shared rev. Parent directory mtimes are left + // unchanged on purpose; this diverges from POSIX rename(2), but + // avoids treating the old and new parents as content changes. + for (const entry of oldEntries) { + db.run("UPDATE vfs_nodes SET rev = ? WHERE inode = ?", rev, entry.inode); + recordDelete(db, rev, entry.path); + } + }); +} + +function assertCompatibleOverwrite( + sourceType: NodeType, + existingType: NodeType, + path: string, +): void { + if (sourceType === "dir" && existingType === "dir") return; + if (existingType === "dir") { + throw createWorkspaceError("EISDIR", `cannot overwrite directory: ${path}`, path); + } + if (sourceType === "dir") { + throw createWorkspaceError("ENOTDIR", `cannot overwrite non-directory: ${path}`, path); + } +} + +function assertDestinationParentOutsideSource( + oldEntries: SubtreeEntry[], + newParentInode: number, + oldCanonical: string, + newCanonical: string, +): void { + if (!oldEntries.some((entry) => entry.inode === newParentInode)) return; + + throw createWorkspaceError( + "EINVAL", + `cannot rename a directory into itself: ${oldCanonical}`, + newCanonical, + ); +} + +function collectSubtree(db: Database, rootInode: number, rootPath: string): SubtreeEntry[] { + const entries: SubtreeEntry[] = [{ path: rootPath, inode: rootInode, type: "dir" }]; + for (let idx = 0; idx < entries.length; idx++) { + const entry = entries[idx]; + if (entry.type !== "dir") continue; + const children = db.all( + `SELECT d.name AS name, d.child_inode AS child_inode, n.type AS type + FROM vfs_dirents d + JOIN vfs_nodes n ON n.inode = d.child_inode + WHERE d.parent_inode = ? + ORDER BY d.name`, + entry.inode, + ); + for (const child of children) { + const childPath = entry.path === "/" ? `/${child.name}` : `${entry.path}/${child.name}`; + entries.push({ + path: childPath, + inode: child.child_inode, + type: child.type, + }); + } + } + return entries; +} diff --git a/packages/dofs/src/fs/rm.test.ts b/packages/dofs/src/fs/rm.test.ts index 5ba45528..5acd002d 100644 --- a/packages/dofs/src/fs/rm.test.ts +++ b/packages/dofs/src/fs/rm.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import type { Database } from "../storage.js"; import { mkdir } from "./mkdir.js"; import { readdir } from "./readdir.js"; +import { readFile } from "./readFile.js"; import { resolveInode } from "./resolve.js"; import { rm } from "./rm.js"; import { symlink } from "./symlink.js"; @@ -41,6 +42,23 @@ describe("rm", () => { }); }); + it("records tombstones at the resolved path through intermediate symlinks", async () => { + await withDB(async (db) => { + mkdir(db, "/real", {}, () => 0); + await writeFile(db, "/real/file.txt", "content", {}, () => 0); + symlink(db, "/real", "/link", () => 0); + + rm(db, "/link/file.txt", {}); + + expect(listChanges(db)).toContainEqual( + expect.objectContaining({ path: "/real/file.txt", op: "delete" }), + ); + expect(listChanges(db)).not.toContainEqual( + expect.objectContaining({ path: "/link/file.txt", op: "delete" }), + ); + }); + }); + it("bumps rev once per call", async () => { await withDB(async (db) => { await writeFile(db, "/a.txt", "hi", {}, () => 0); @@ -86,6 +104,16 @@ describe("rm", () => { }); }); + it("removes a dangling symlink", async () => { + await withDB((db) => { + symlink(db, "/missing", "/dangling", () => 0); + + rm(db, "/dangling", {}); + + expect(resolveInode(db, "/dangling", { followSymlinks: false })).toBeNull(); + }); + }); + it("rejects ENOENT for a missing path", async () => { await withDB((db) => { expect(() => rm(db, "/missing", {})).toThrowError( @@ -143,6 +171,19 @@ describe("rm", () => { }); }); + it("recursive removes a symlink to a directory without deleting its target", async () => { + await withDB(async (db) => { + mkdir(db, "/target/sub", { recursive: true }, () => 0); + await writeFile(db, "/target/sub/file.txt", "content", {}, () => 0); + symlink(db, "/target", "/link", () => 0); + + rm(db, "/link", { recursive: true }); + + expect(resolveInode(db, "/link", { followSymlinks: false })).toBeNull(); + expect(await readFile(db, "/target/sub/file.txt", "utf8")).toBe("content"); + }); + }); + it("recursive records one tombstone per removed path", async () => { await withDB(async (db) => { mkdir(db, "/d", {}, () => 0); @@ -156,6 +197,25 @@ describe("rm", () => { }); }); + it("recursive records resolved subtree tombstones through intermediate symlinks", async () => { + await withDB(async (db) => { + mkdir(db, "/real/dir", { recursive: true }, () => 0); + await writeFile(db, "/real/dir/a", "x", {}, () => 0); + await writeFile(db, "/real/dir/b", "y", {}, () => 0); + symlink(db, "/real", "/link", () => 0); + + rm(db, "/link/dir", { recursive: true }); + + const paths = listChanges(db) + .map((r) => r.path) + .sort(); + expect(paths).toEqual(expect.arrayContaining(["/real/dir", "/real/dir/a", "/real/dir/b"])); + expect(paths).not.toEqual( + expect.arrayContaining(["/link/dir", "/link/dir/a", "/link/dir/b"]), + ); + }); + }); + it("recursive still bumps rev only once for the whole tree", async () => { await withDB(async (db) => { mkdir(db, "/d", {}, () => 0); diff --git a/packages/dofs/src/fs/rm.ts b/packages/dofs/src/fs/rm.ts index 1dc2bcdb..9ce423fa 100644 --- a/packages/dofs/src/fs/rm.ts +++ b/packages/dofs/src/fs/rm.ts @@ -3,8 +3,10 @@ import { canonicalizePath } from "../path.js"; import { incrementRev } from "../rev.js"; import type { Database } from "../storage.js"; import { recordDelete } from "../sync/changes.js"; +import { pathOf } from "../sync/paths.js"; import { assertNotReadOnly } from "./mount-guard.js"; import { resolveInode } from "./resolve.js"; +import { unlinkDirent } from "./unlink.js"; export interface RmOptions { recursive?: boolean; @@ -92,15 +94,34 @@ export function rm(db: Database, path: string, options: RmOptions): void { } } + // Resolve the entry's real path from its parent rather than from + // the inode: a hardlinked file has several names, and pathOf would + // pick an arbitrary one. Following symlinks on the parent lets a + // request through a symlinked directory land on the real container + // while still removing exactly the requested name. + const name = parts[parts.length - 1]; + const parentPath = parts.length === 1 ? "/" : `/${parts.slice(0, -1).join("/")}`; + const parent = resolveInode(db, parentPath); + if (parent === null || parent.type !== "dir") { + throw createWorkspaceError("ENOENT", `no such path: ${canonical}`, canonical); + } + const parentReal = pathOf(db, parent.inode); + if (parentReal === null) { + throw createWorkspaceError("ENOENT", `no such path: ${canonical}`, canonical); + } + const realPath = parentReal === "/" ? `/${name}` : `${parentReal}/${name}`; + assertNotReadOnly(db, realPath); + const rev = incrementRev(db); - if (node.type === "file" || !recursive) { + if (node.type !== "dir" || !recursive) { // Single entry removal — file, symlink, or empty directory. A // file inode may have multiple dirents (hardlinks), so remove // only the requested name and reap chunks/node after the final - // link disappears. - removeEntry(db, canonical, node.inode, node.type); - recordDelete(db, rev, canonical); + // link disappears. The tombstone is recorded at the resolved + // real path so sync sees the move-aware location. + removeEntry(db, realPath, node.inode, node.type); + recordDelete(db, rev, realPath); return; } @@ -108,7 +129,7 @@ export function rm(db: Database, path: string, options: RmOptions): void { // sees an empty parent by the time we get to it. File entries may // be hardlinked outside this subtree, so delete by path rather // than by child inode. - for (const entry of walkPostOrder(db, node.inode, canonical)) { + for (const entry of walkPostOrder(db, node.inode, realPath)) { removeEntry(db, entry.path, entry.inode, entry.type); recordDelete(db, rev, entry.path); } @@ -129,15 +150,5 @@ function removeEntry( throw createWorkspaceError("ENOENT", `parent directory missing: ${canonical}`, canonical); } - db.run("DELETE FROM vfs_dirents WHERE parent_inode = ? AND name = ?", parent.inode, name); - const remaining = db.scalar( - "SELECT COUNT(*) FROM vfs_dirents WHERE child_inode = ?", - inode, - ); - if ((remaining ?? 0) > 0) return; - - if (type === "file") { - db.run("DELETE FROM vfs_chunks WHERE inode = ?", inode); - } - db.run("DELETE FROM vfs_nodes WHERE inode = ?", inode); + unlinkDirent(db, parent.inode, name, inode, type); } diff --git a/packages/dofs/src/fs/unlink.ts b/packages/dofs/src/fs/unlink.ts new file mode 100644 index 00000000..7a7d33c2 --- /dev/null +++ b/packages/dofs/src/fs/unlink.ts @@ -0,0 +1,34 @@ +import type { Database } from "../storage.js"; + +type NodeType = "file" | "dir" | "symlink"; + +// Remove a single (parent, name) dirent and reap the child inode's +// node and chunk rows only once its last link disappears. A file inode +// can carry several hardlink names, so the node and its chunks survive +// until the final dirent is gone. Returns true when the inode was +// reaped, false when other links keep it alive. +// +// Callers own rev bumps and tombstones; this helper touches only +// vfs_dirents, vfs_chunks, and vfs_nodes. It is the single place the +// refcount-gated reap is implemented — rm, rename, and the sync apply +// path all funnel through here so the invariant lives once. +export function unlinkDirent( + db: Database, + parentInode: number, + name: string, + childInode: number, + type: NodeType, +): boolean { + db.run("DELETE FROM vfs_dirents WHERE parent_inode = ? AND name = ?", parentInode, name); + const remaining = db.scalar( + "SELECT COUNT(*) FROM vfs_dirents WHERE child_inode = ?", + childInode, + ); + if ((remaining ?? 0) > 0) return false; + + if (type === "file") { + db.run("DELETE FROM vfs_chunks WHERE inode = ?", childInode); + } + db.run("DELETE FROM vfs_nodes WHERE inode = ?", childInode); + return true; +} diff --git a/packages/dofs/src/provider.test.ts b/packages/dofs/src/provider.test.ts index 299f777e..18a4d5ab 100644 --- a/packages/dofs/src/provider.test.ts +++ b/packages/dofs/src/provider.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it } from "vitest"; import { withDB } from "./fs/with-db.js"; import { SQLiteWorkspaceProvider } from "./provider.js"; +import type { Database } from "./storage.js"; +import type { ChangeEntry } from "./sync/changes.js"; +import { coalesceChanges } from "./sync/coalesce.js"; // Each provider test gets a fresh DB via withDB, which the workers // runner aliases to a DO-backed implementation. The provider holds @@ -10,6 +13,22 @@ async function withProvider(fn: (p: SQLiteWorkspaceProvider) => T | Promise fn(new SQLiteWorkspaceProvider(db, { now: () => 1000 }))); } +async function withProviderAndDB( + fn: (p: SQLiteWorkspaceProvider, db: Database) => T | Promise, +): Promise { + return withDB((db) => fn(new SQLiteWorkspaceProvider(db, { now: () => 1000 }), db)); +} + +async function drainChanges(db: Database, sinceRev: number): Promise { + const out: ChangeEntry[] = []; + for await (const entry of coalesceChanges(db, sinceRev)) out.push(entry); + return out; +} + +function kindPath(entries: ChangeEntry[]): Array<[ChangeEntry["kind"], string]> { + return entries.map((entry) => [entry.kind, entry.path]).sort((a, b) => a[1].localeCompare(b[1])); +} + describe("SQLiteWorkspaceProvider — capability flags", () => { it("reports the supported feature set", async () => { await withProvider((p) => { @@ -142,6 +161,18 @@ describe("SQLiteWorkspaceProvider — implemented methods", () => { }); }); + it("unlinkSync removes a symlink without deleting its target", async () => { + await withProvider((p) => { + p.writeFileSync("/target", "content"); + p.symlinkSync("/target", "/link"); + + p.unlinkSync("/link"); + + expect(p.existsSync("/link")).toBe(false); + expect(p.readFileSync("/target", "utf8")).toBe("content"); + }); + }); + it("rmdirSync removes an empty directory", async () => { await withProvider((p) => { p.mkdirSync("/a", {}); @@ -202,6 +233,119 @@ describe("SQLiteWorkspaceProvider — implemented methods", () => { }); describe("SQLiteWorkspaceProvider — renameSync overwrite matrix", () => { + it("records rename as an old-path delete and new-path live entry", async () => { + await withProviderAndDB(async (p, db) => { + p.writeFileSync("/src", "new"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/src", "/dst"); + + const entries = await drainChanges(db, cursor); + expect(kindPath(entries)).toEqual([ + ["file", "/dst"], + ["delete", "/src"], + ]); + }); + }); + + it("records directory rename for the whole moved subtree", async () => { + await withProviderAndDB(async (p, db) => { + p.mkdirSync("/src", {}); + p.writeFileSync("/src/a.txt", "a"); + p.mkdirSync("/src/sub", {}); + p.writeFileSync("/src/sub/b.txt", "b"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/src", "/dst"); + + const entries = await drainChanges(db, cursor); + expect(kindPath(entries)).toEqual([ + ["dir", "/dst"], + ["file", "/dst/a.txt"], + ["dir", "/dst/sub"], + ["file", "/dst/sub/b.txt"], + ["delete", "/src"], + ["delete", "/src/a.txt"], + ["delete", "/src/sub"], + ["delete", "/src/sub/b.txt"], + ]); + }); + }); + + it("records rename tombstones at the resolved old file path", async () => { + await withProviderAndDB(async (p, db) => { + p.mkdirSync("/real", {}); + p.writeFileSync("/real/file.txt", "x"); + p.symlinkSync("/real", "/link"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/link/file.txt", "/dst.txt"); + + const entries = await drainChanges(db, cursor); + expect(kindPath(entries)).toEqual([ + ["file", "/dst.txt"], + ["delete", "/real/file.txt"], + ]); + }); + }); + + it("records directory rename tombstones at the resolved old subtree paths", async () => { + await withProviderAndDB(async (p, db) => { + p.mkdirSync("/real", {}); + p.mkdirSync("/real/dir", {}); + p.writeFileSync("/real/dir/a.txt", "a"); + p.mkdirSync("/real/dir/sub", {}); + p.writeFileSync("/real/dir/sub/b.txt", "b"); + p.symlinkSync("/real", "/link"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/link/dir", "/dst"); + + const entries = await drainChanges(db, cursor); + expect(kindPath(entries)).toEqual([ + ["dir", "/dst"], + ["file", "/dst/a.txt"], + ["dir", "/dst/sub"], + ["file", "/dst/sub/b.txt"], + ["delete", "/real/dir"], + ["delete", "/real/dir/a.txt"], + ["delete", "/real/dir/sub"], + ["delete", "/real/dir/sub/b.txt"], + ]); + }); + }); + + it("same-inode rename through a symlinked file path is a no-op", async () => { + await withProviderAndDB(async (p, db) => { + p.mkdirSync("/real", {}); + p.writeFileSync("/real/file.txt", "x"); + p.symlinkSync("/real", "/link"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/real/file.txt", "/link/file.txt"); + + expect(p.readFileSync("/real/file.txt", "utf8")).toBe("x"); + expect(p.readFileSync("/link/file.txt", "utf8")).toBe("x"); + expect(await drainChanges(db, cursor)).toEqual([]); + }); + }); + + it("same-inode rename through a symlinked directory path is a no-op", async () => { + await withProviderAndDB(async (p, db) => { + p.mkdirSync("/real", {}); + p.mkdirSync("/real/dir", {}); + p.writeFileSync("/real/dir/a.txt", "a"); + p.symlinkSync("/real", "/link"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/real/dir", "/link/dir"); + + expect(p.readFileSync("/real/dir/a.txt", "utf8")).toBe("a"); + expect(p.readFileSync("/link/dir/a.txt", "utf8")).toBe("a"); + expect(await drainChanges(db, cursor)).toEqual([]); + }); + }); + it("file → existing file overwrites atomically", async () => { await withProvider((p) => { p.writeFileSync("/src", "new"); @@ -237,6 +381,119 @@ describe("SQLiteWorkspaceProvider — renameSync overwrite matrix", () => { }); }); + it.each([ + { + name: "file → existing symlink", + setup(p: SQLiteWorkspaceProvider) { + p.writeFileSync("/target", "x"); + p.writeFileSync("/src", "new"); + p.symlinkSync("/target", "/dst"); + }, + assertRenamed(p: SQLiteWorkspaceProvider) { + expect(p.existsSync("/src")).toBe(false); + expect(p.lstatSync("/dst").isFile()).toBe(true); + expect(p.readFileSync("/dst", "utf8")).toBe("new"); + expect(p.readFileSync("/target", "utf8")).toBe("x"); + }, + }, + { + name: "symlink → existing file", + setup(p: SQLiteWorkspaceProvider) { + p.writeFileSync("/target", "x"); + p.symlinkSync("/target", "/src"); + p.writeFileSync("/dst", "old"); + }, + assertRenamed(p: SQLiteWorkspaceProvider) { + expect(p.existsSync("/src")).toBe(false); + expect(p.lstatSync("/dst").isSymbolicLink()).toBe(true); + expect(p.readlinkSync("/dst")).toBe("/target"); + }, + }, + { + name: "symlink → existing symlink", + setup(p: SQLiteWorkspaceProvider) { + p.writeFileSync("/target", "x"); + p.writeFileSync("/other", "y"); + p.symlinkSync("/target", "/src"); + p.symlinkSync("/other", "/dst"); + }, + assertRenamed(p: SQLiteWorkspaceProvider) { + expect(p.existsSync("/src")).toBe(false); + expect(p.lstatSync("/dst").isSymbolicLink()).toBe(true); + expect(p.readlinkSync("/dst")).toBe("/target"); + }, + }, + ])("$name overwrites atomically", async ({ setup, assertRenamed }) => { + await withProvider((p) => { + setup(p); + p.renameSync("/src", "/dst"); + assertRenamed(p); + }); + }); + + it.each([ + { + name: "file → existing empty dir", + code: "EISDIR", + setup(p: SQLiteWorkspaceProvider) { + p.writeFileSync("/src", "new"); + p.mkdirSync("/dst", {}); + }, + assertUnchanged(p: SQLiteWorkspaceProvider) { + expect(p.readFileSync("/src", "utf8")).toBe("new"); + expect(p.statSync("/dst").isDirectory()).toBe(true); + }, + }, + { + name: "symlink → existing empty dir", + code: "EISDIR", + setup(p: SQLiteWorkspaceProvider) { + p.writeFileSync("/target", "x"); + p.symlinkSync("/target", "/src"); + p.mkdirSync("/dst", {}); + }, + assertUnchanged(p: SQLiteWorkspaceProvider) { + expect(p.readlinkSync("/src")).toBe("/target"); + expect(p.statSync("/dst").isDirectory()).toBe(true); + }, + }, + { + name: "dir → existing file", + code: "ENOTDIR", + setup(p: SQLiteWorkspaceProvider) { + p.mkdirSync("/src", {}); + p.writeFileSync("/dst", "old"); + }, + assertUnchanged(p: SQLiteWorkspaceProvider) { + expect(p.statSync("/src").isDirectory()).toBe(true); + expect(p.readFileSync("/dst", "utf8")).toBe("old"); + }, + }, + { + name: "dir → existing symlink", + code: "ENOTDIR", + setup(p: SQLiteWorkspaceProvider) { + p.writeFileSync("/target", "x"); + p.mkdirSync("/src", {}); + p.symlinkSync("/target", "/dst"); + }, + assertUnchanged(p: SQLiteWorkspaceProvider) { + expect(p.statSync("/src").isDirectory()).toBe(true); + expect(p.readlinkSync("/dst")).toBe("/target"); + }, + }, + ])("$name rejects without recording sync changes", async ({ code, setup, assertUnchanged }) => { + await withProviderAndDB(async (p, db) => { + setup(p); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + expect(() => p.renameSync("/src", "/dst")).toThrowError(expect.objectContaining({ code })); + + assertUnchanged(p); + expect(await drainChanges(db, cursor)).toEqual([]); + }); + }); + it("source missing throws ENOENT", async () => { await withProvider((p) => { expect(() => p.renameSync("/missing", "/dst")).toThrowError( @@ -245,6 +502,35 @@ describe("SQLiteWorkspaceProvider — renameSync overwrite matrix", () => { }); }); + it("same-path rename validates the source before no-op", async () => { + await withProvider((p) => { + expect(() => p.renameSync("/missing", "/missing")).toThrowError( + expect.objectContaining({ code: "ENOENT" }), + ); + }); + }); + + it("same-path rename of an existing file is a no-op", async () => { + await withProviderAndDB(async (p, db) => { + p.writeFileSync("/src", "x"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/src", "/src"); + + expect(p.readFileSync("/src", "utf8")).toBe("x"); + expect(await drainChanges(db, cursor)).toEqual([]); + }); + }); + + it("rename of a file into itself as a parent does not report directory self-move", async () => { + await withProvider((p) => { + p.writeFileSync("/file", "x"); + expect(() => p.renameSync("/file", "/file/child")).toThrowError( + expect.objectContaining({ code: "ENOENT" }), + ); + }); + }); + it("rename onto root throws EINVAL", async () => { await withProvider((p) => { p.writeFileSync("/src", "x"); @@ -263,11 +549,62 @@ describe("SQLiteWorkspaceProvider — renameSync overwrite matrix", () => { }); }); - // TODO: renaming a symlink should move the link itself, not the - // target. Today renameSync uses resolveInode() with the default - // followSymlinks: true, so a rename on a symlink actually moves the - // pointed-at file. Pin this once renameSync grows an - // lresolveInode-style call (see provider.ts:248). + it("rename of a symlink moves the link itself", async () => { + await withProvider((p) => { + p.writeFileSync("/target", "x"); + p.symlinkSync("/target", "/link"); + p.renameSync("/link", "/moved"); + expect(p.existsSync("/target")).toBe(true); + expect(p.readlinkSync("/moved")).toBe("/target"); + expect(p.existsSync("/link")).toBe(false); + }); + }); + + it("rename of a directory into its own subtree throws EINVAL", async () => { + await withProvider((p) => { + p.mkdirSync("/src", {}); + p.mkdirSync("/src/sub", {}); + expect(() => p.renameSync("/src", "/src/sub/dst")).toThrowError( + expect.objectContaining({ code: "EINVAL" }), + ); + }); + }); + + it("rename of a directory through a symlink into its own subtree throws EINVAL", async () => { + await withProviderAndDB(async (p, db) => { + p.mkdirSync("/src", {}); + p.mkdirSync("/src/sub", {}); + p.symlinkSync("/src/sub", "/link"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + expect(() => p.renameSync("/src", "/link/dst")).toThrowError( + expect.objectContaining({ code: "EINVAL" }), + ); + + expect(p.statSync("/src/sub").isDirectory()).toBe(true); + expect(p.readlinkSync("/link")).toBe("/src/sub"); + expect(await drainChanges(db, cursor)).toEqual([]); + }); + }); + + it("rename of a directory through a symlink out of its subtree succeeds", async () => { + await withProviderAndDB(async (p, db) => { + p.mkdirSync("/src", {}); + p.mkdirSync("/other", {}); + // /src/link points outside the source subtree, so the destination + // /src/link/dst resolves to /other/dst even though the literal path + // is lexically under /src. The self-move guard is inode-based and + // must allow this move rather than reject it on a textual prefix. + p.symlinkSync("/other", "/src/link"); + const cursor = db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; + + p.renameSync("/src", "/src/link/dst"); + + expect(p.statSync("/other/dst").isDirectory()).toBe(true); + expect(p.existsSync("/src")).toBe(false); + expect(await drainChanges(db, cursor)).not.toEqual([]); + }); + }); }); describe("SQLiteWorkspaceProvider — unimplemented surface (stubs)", () => { diff --git a/packages/dofs/src/provider.ts b/packages/dofs/src/provider.ts index 19354c15..0746828f 100644 --- a/packages/dofs/src/provider.ts +++ b/packages/dofs/src/provider.ts @@ -15,6 +15,7 @@ import { mkdir as mkdirImpl } from "./fs/mkdir.js"; import { readdir as readdirImpl } from "./fs/readdir.js"; import { readRangeSync as readRangeSyncImpl } from "./fs/readFile.js"; import { readlink as readlinkImpl } from "./fs/readlink.js"; +import { rename as renameImpl } from "./fs/rename.js"; import { resolveInode } from "./fs/resolve.js"; import { rm as rmImpl } from "./fs/rm.js"; import { stat as statImpl } from "./fs/stat.js"; @@ -317,105 +318,27 @@ export class SQLiteWorkspaceProvider { } renameSync(oldPath: string, newPath: string): void { - // The FS module doesn't expose rename as a standalone operation - // yet; we lean on the existing schema-level pieces here. When - // rename grows up (cross-directory, overwriting an existing file, - // ...) it should move into fs/rename.ts with its own tests. + // Commit any still-pending creates at either end before the rename + // touches dirents: the source needs a real inode to move, and a + // pending buffer at the destination would otherwise slip past + // rename's dirent-based existence check and lose bytes on release. flushPendingByPath(this.db, oldPath, this.now); flushPendingByPath(this.db, newPath, this.now); - const node = resolveInode(this.db, oldPath); - if (node === null) { - throw createWorkspaceError("ENOENT", `no such path: ${oldPath}`, oldPath); - } - const { parts: oldParts, path: oldCanonical } = canonicalizePath(oldPath); - const oldName = oldParts[oldParts.length - 1]; - const oldParentPath = oldParts.length === 1 ? "/" : `/${oldParts.slice(0, -1).join("/")}`; - const oldParent = resolveInode(this.db, oldParentPath, { followSymlinks: false }); - if (oldParent === null || oldParent.type !== "dir") { - throw createWorkspaceError( - "ENOENT", - `parent directory missing: ${oldCanonical}`, - oldCanonical, - ); - } - const { parts, path: newCanonical } = canonicalizePath(newPath); - if (oldCanonical === newCanonical) return; - if (parts.length === 0) { - throw createWorkspaceError("EINVAL", "cannot rename onto root", newCanonical); - } - const newName = parts[parts.length - 1]; - const newParentPath = parts.length === 1 ? "/" : `/${parts.slice(0, -1).join("/")}`; - const newParent = resolveInode(this.db, newParentPath); - if (newParent === null || newParent.type !== "dir") { - throw createWorkspaceError( - "ENOENT", - `parent directory missing: ${newCanonical}`, - newCanonical, - ); - } - this.db.transactionSync(() => { - // If the destination already exists, displace it before linking - // the source dirent. POSIX rename(2) is atomic and overwrites a - // regular file or empty directory at the target. We follow the - // same semantics: an existing file at newPath is unlinked, and - // its inode (and chunks / blobs) are reaped via the existing - // gc() safety window. - const existing = this.db.one<{ child_inode: number; type: string }>( - `SELECT d.child_inode AS child_inode, n.type AS type - FROM vfs_dirents d JOIN vfs_nodes n ON n.inode = d.child_inode - WHERE d.parent_inode = ? AND d.name = ?`, - newParent.inode, - newName, - ); - const destinationAlreadyNamesSource = existing?.child_inode === node.inode; - if (existing !== undefined && !destinationAlreadyNamesSource) { - // Refuse to overwrite a non-empty directory or replace a - // directory with a file (Linux rename semantics). - if (existing.type === "dir") { - const childCount = this.db.scalar( - "SELECT COUNT(*) FROM vfs_dirents WHERE parent_inode = ?", - existing.child_inode, - ); - if ((childCount ?? 0) > 0) { - throw createWorkspaceError("ENOTEMPTY", `not empty: ${newCanonical}`, newCanonical); - } - } - // Unlink only the displaced destination name. If other - // hardlinks still reference the displaced file inode, keep its - // chunks and node alive. - this.db.run( - "DELETE FROM vfs_dirents WHERE parent_inode = ? AND name = ?", - newParent.inode, - newName, - ); - const remaining = this.db.scalar( - "SELECT COUNT(*) FROM vfs_dirents WHERE child_inode = ?", - existing.child_inode, - ); - if ((remaining ?? 0) === 0) { - this.db.run("DELETE FROM vfs_chunks WHERE inode = ?", existing.child_inode); - this.db.run("DELETE FROM vfs_nodes WHERE inode = ?", existing.child_inode); - // The displaced inode is gone from SQL. Any open write - // buffer keyed by it would otherwise hold bytes pointed at - // a dead inode; release would then commit chunks against a - // missing row (0-row UPDATE, silent data loss). - deleteWriteBuffer(this.db, existing.child_inode); - } - } - this.db.run( - "DELETE FROM vfs_dirents WHERE parent_inode = ? AND name = ?", - oldParent.inode, - oldName, + // Capture the destination inode before the rename so we can evict + // its write-buffer cache entry if the rename displaced and reaped + // it. Without this, a release on an open destination would commit + // chunks against a dead inode (0-row UPDATE, silent data loss). + const displaced = resolveInode(this.db, newPath, { followSymlinks: false }); + renameImpl(this.db, oldPath, newPath); + if (displaced !== null) { + const stillAlive = this.db.scalar( + "SELECT inode FROM vfs_nodes WHERE inode = ?", + displaced.inode, ); - if (!destinationAlreadyNamesSource) { - this.db.run( - "INSERT INTO vfs_dirents (parent_inode, name, child_inode) VALUES (?, ?, ?)", - newParent.inode, - newName, - node.inode, - ); + if (stillAlive === undefined) { + deleteWriteBuffer(this.db, displaced.inode); } - }); + } } // -- Default implementations --------------------------------------- diff --git a/packages/dofs/src/sync/apply.test.ts b/packages/dofs/src/sync/apply.test.ts index b326e0be..fa20ef5e 100644 --- a/packages/dofs/src/sync/apply.test.ts +++ b/packages/dofs/src/sync/apply.test.ts @@ -1,16 +1,21 @@ import { describe, expect, it } from "vitest"; +import { link } from "../fs/link.js"; import { mkdir } from "../fs/mkdir.js"; import { invalidateReadOnlyMountCache } from "../fs/mount-guard.js"; import { readFile } from "../fs/readFile.js"; +import { readlink } from "../fs/readlink.js"; +import { rename } from "../fs/rename.js"; import { resolveInode } from "../fs/resolve.js"; +import { rm } from "../fs/rm.js"; +import { symlink } from "../fs/symlink.js"; import { withDB, withTwoDBs } from "../fs/with-db.js"; -import { writeFile } from "../fs/writeFile.js"; +import { writeFile, writeFileSync } from "../fs/writeFile.js"; import { applyChanges, applyChangesSync } from "./apply.js"; import type { ChangeEntry } from "./changes.js"; import { coalesceChanges } from "./coalesce.js"; import { fetchObjects } from "./fetch.js"; -import { writeWatermark } from "./watermarks.js"; +import { currentRev, writeWatermark } from "./watermarks.js"; async function drain(it: AsyncIterable): Promise { const out: T[] = []; @@ -24,6 +29,10 @@ function hex(bytes: Uint8Array): string { return s; } +function deepChildPath(depth: number): string { + return `/dst/${Array.from({ length: depth }, () => "d").join("/")}/old.txt`; +} + async function collectObjects( db: import("../storage.js").Database, entries: ChangeEntry[], @@ -113,6 +122,448 @@ describe("applyChanges", () => { ); }); + it("applies a file rename over an existing file", async () => { + await withTwoDBs( + async (a) => { + await writeFile(a, "/src", "new", {}, () => 1); + await writeFile(a, "/dst", "old", {}, () => 2); + const cursor = currentRev(a); + + rename(a, "/src", "/dst"); + + const entries = await drain(coalesceChanges(a, cursor)); + return { entries, objects: await collectObjects(a, entries) }; + }, + async (b, { entries, objects }) => { + await writeFile(b, "/src", "new", {}, () => 1); + await writeFile(b, "/dst", "old", {}, () => 2); + + await applyChanges(b, entries, objects); + + expect(resolveInode(b, "/src")).toBeNull(); + expect(await readFile(b, "/dst", "utf8")).toBe("new"); + }, + ); + }); + + it("applies a directory rename over an existing empty directory", async () => { + await withTwoDBs( + async (a) => { + mkdir(a, "/src", { mode: 0o700, recursive: true }, () => 1); + await writeFile(a, "/src/inside", "x", {}, () => 2); + mkdir(a, "/dst", { mode: 0o755 }, () => 3); + const cursor = currentRev(a); + + rename(a, "/src", "/dst"); + + const entries = await drain(coalesceChanges(a, cursor)); + return { entries, objects: await collectObjects(a, entries) }; + }, + async (b, { entries, objects }) => { + mkdir(b, "/src", { mode: 0o700, recursive: true }, () => 1); + await writeFile(b, "/src/inside", "x", {}, () => 2); + mkdir(b, "/dst", { mode: 0o755 }, () => 3); + + await applyChanges(b, entries, objects); + + expect(resolveInode(b, "/src")).toBeNull(); + expect(resolveInode(b, "/dst", { followSymlinks: false })).toMatchObject({ + type: "dir", + mode: 0o700, + mtime: 1, + }); + expect(await readFile(b, "/dst/inside", "utf8")).toBe("x"); + }, + ); + }); + + it("applyChangesSync updates existing directory metadata", async () => { + await withDB((db) => { + mkdir(db, "/dst", { mode: 0o755 }, () => 3); + + applyChangesSync( + db, + [{ kind: "dir", rev: 99, path: "/dst", mode: 0o700, mtime: 1 }], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })).toMatchObject({ + type: "dir", + mode: 0o700, + mtime: 1, + }); + }); + }); + + it("does not update existing directory mtime for upstream entries with matching mode", async () => { + await withDB(async (db) => { + mkdir(db, "/dst", { mode: 0o700 }, () => 3); + + await applyChanges( + db, + [{ kind: "dir", rev: 99, path: "/dst", mode: 0o700, mtime: 1 }], + new Map(), + { source: "upstream" }, + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })).toMatchObject({ + type: "dir", + mode: 0o700, + mtime: 3, + }); + }); + }); + + it("applies a file rename over an existing symlink", async () => { + await withTwoDBs( + async (a) => { + await writeFile(a, "/target", "target", {}, () => 1); + await writeFile(a, "/src", "new", {}, () => 2); + symlink(a, "/target", "/dst", () => 3); + const cursor = currentRev(a); + + rename(a, "/src", "/dst"); + + const entries = await drain(coalesceChanges(a, cursor)); + return { entries, objects: await collectObjects(a, entries) }; + }, + async (b, { entries, objects }) => { + await writeFile(b, "/target", "target", {}, () => 1); + await writeFile(b, "/src", "new", {}, () => 2); + symlink(b, "/target", "/dst", () => 3); + + await applyChanges(b, entries, objects); + + expect(resolveInode(b, "/src")).toBeNull(); + expect(resolveInode(b, "/dst", { followSymlinks: false })?.type).toBe("file"); + expect(await readFile(b, "/dst", "utf8")).toBe("new"); + expect(await readFile(b, "/target", "utf8")).toBe("target"); + }, + ); + }); + + it("applies a symlink rename over an existing file", async () => { + await withTwoDBs( + async (a) => { + await writeFile(a, "/target", "target", {}, () => 1); + symlink(a, "/target", "/src", () => 2); + await writeFile(a, "/dst", "old", {}, () => 3); + const cursor = currentRev(a); + + rename(a, "/src", "/dst"); + + const entries = await drain(coalesceChanges(a, cursor)); + return { entries, objects: await collectObjects(a, entries) }; + }, + async (b, { entries, objects }) => { + await writeFile(b, "/target", "target", {}, () => 1); + symlink(b, "/target", "/src", () => 2); + await writeFile(b, "/dst", "old", {}, () => 3); + + await applyChanges(b, entries, objects); + + expect(resolveInode(b, "/src", { followSymlinks: false })).toBeNull(); + expect(resolveInode(b, "/dst", { followSymlinks: false })?.type).toBe("symlink"); + expect(readlink(b, "/dst")).toBe("/target"); + expect(await readFile(b, "/target", "utf8")).toBe("target"); + }, + ); + }); + + it("applies a symlink rename over an existing symlink", async () => { + await withTwoDBs( + async (a) => { + await writeFile(a, "/target", "target", {}, () => 1); + await writeFile(a, "/other", "other", {}, () => 2); + symlink(a, "/target", "/src", () => 3); + symlink(a, "/other", "/dst", () => 4); + const cursor = currentRev(a); + + rename(a, "/src", "/dst"); + + const entries = await drain(coalesceChanges(a, cursor)); + return { entries, objects: await collectObjects(a, entries) }; + }, + async (b, { entries, objects }) => { + await writeFile(b, "/target", "target", {}, () => 1); + await writeFile(b, "/other", "other", {}, () => 2); + symlink(b, "/target", "/src", () => 3); + symlink(b, "/other", "/dst", () => 4); + + await applyChanges(b, entries, objects); + + expect(resolveInode(b, "/src", { followSymlinks: false })).toBeNull(); + expect(resolveInode(b, "/dst", { followSymlinks: false })?.type).toBe("symlink"); + expect(readlink(b, "/dst")).toBe("/target"); + expect(await readFile(b, "/target", "utf8")).toBe("target"); + expect(await readFile(b, "/other", "utf8")).toBe("other"); + }, + ); + }); + + it("applies a directory rename containing a symlink without deleting its target", async () => { + await withTwoDBs( + async (a) => { + await writeFile(a, "/target", "target", {}, () => 1); + mkdir(a, "/src", {}, () => 2); + symlink(a, "/target", "/src/link", () => 3); + const cursor = currentRev(a); + + rename(a, "/src", "/dst"); + + const entries = await drain(coalesceChanges(a, cursor)); + return { entries, objects: await collectObjects(a, entries) }; + }, + async (b, { entries, objects }) => { + await writeFile(b, "/target", "target", {}, () => 1); + mkdir(b, "/src", {}, () => 2); + symlink(b, "/target", "/src/link", () => 3); + + await applyChanges(b, entries, objects); + + expect(resolveInode(b, "/src/link", { followSymlinks: false })).toBeNull(); + expect(resolveInode(b, "/dst/link", { followSymlinks: false })?.type).toBe("symlink"); + expect(readlink(b, "/dst/link")).toBe("/target"); + expect(await readFile(b, "/target", "utf8")).toBe("target"); + }, + ); + }); + + it("applies a file over an existing directory subtree", async () => { + await withDB(async (db) => { + mkdir(db, "/dst/sub", { recursive: true }, () => 1); + await writeFile(db, "/dst/sub/old.txt", "old", {}, () => 2); + + await applyChanges( + db, + [ + { + kind: "file", + rev: 99, + path: "/dst", + mode: 0o644, + mtime: 3, + size: 0, + chunks: [], + }, + ], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })?.type).toBe("file"); + expect(resolveInode(db, "/dst/sub/old.txt")).toBeNull(); + }); + }); + + it("applies a file over a deeply nested existing directory subtree", async () => { + await withDB(async (db) => { + const oldPath = deepChildPath(12_000); + mkdir(db, oldPath.slice(0, -"/old.txt".length), { recursive: true }, () => 1); + await writeFile(db, oldPath, "old", {}, () => 2); + + await applyChanges( + db, + [ + { + kind: "file", + rev: 99, + path: "/dst", + mode: 0o644, + mtime: 3, + size: 0, + chunks: [], + }, + ], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })?.type).toBe("file"); + expect(resolveInode(db, oldPath, { followSymlinks: false })).toBeNull(); + }); + }); + + it("applies a symlink over an existing directory subtree", async () => { + await withDB((db) => { + mkdir(db, "/dst/sub", { recursive: true }, () => 1); + writeFileSync(db, "/dst/sub/old.txt", new Uint8Array(), {}, () => 2); + + applyChangesSync( + db, + [ + { + kind: "symlink", + rev: 99, + path: "/dst", + mode: 0o777, + mtime: 3, + target: "/target", + }, + ], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })?.type).toBe("symlink"); + expect(readlink(db, "/dst")).toBe("/target"); + expect(resolveInode(db, "/dst/sub/old.txt")).toBeNull(); + }); + }); + + it("applies a symlink over one hardlink without removing sibling hardlinks", async () => { + await withDB(async (db) => { + await writeFile(db, "/dst", "shared", {}, () => 1); + link(db, "/dst", "/other"); + + await applyChanges( + db, + [ + { + kind: "symlink", + rev: 99, + path: "/dst", + mode: 0o777, + mtime: 2, + target: "/target", + }, + ], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })?.type).toBe("symlink"); + expect(readlink(db, "/dst")).toBe("/target"); + expect(await readFile(db, "/other", "utf8")).toBe("shared"); + }); + }); + + it("applies a directory over one hardlink without removing sibling hardlinks", async () => { + await withDB(async (db) => { + await writeFile(db, "/dst", "shared", {}, () => 1); + link(db, "/dst", "/other"); + + await applyChanges( + db, + [{ kind: "dir", rev: 99, path: "/dst", mode: 0o755, mtime: 2 }], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })?.type).toBe("dir"); + expect(await readFile(db, "/other", "utf8")).toBe("shared"); + }); + }); + + it("applies a file over a directory whose subtree hardlinks a file outside it", async () => { + await withDB(async (db) => { + await mkdir(db, "/dir", { mode: 0o755 }, () => 1); + await writeFile(db, "/dir/inner", "shared", {}, () => 1); + // /outside is a second name for /dir/inner; replacing /dir must + // walk the subtree by name and keep /outside alive. + link(db, "/dir/inner", "/outside"); + + await applyChanges( + db, + [ + { + kind: "file", + rev: 99, + path: "/dir", + mode: 0o644, + mtime: 2, + size: 0, + chunks: [], + }, + ], + new Map(), + ); + + expect(resolveInode(db, "/dir", { followSymlinks: false })?.type).toBe("file"); + expect(resolveInode(db, "/dir/inner", { followSymlinks: false })).toBeNull(); + expect(await readFile(db, "/outside", "utf8")).toBe("shared"); + }); + }); + + it("applyChangesSync applies a symlink over a deeply nested existing directory subtree", async () => { + await withDB((db) => { + const oldPath = deepChildPath(12_000); + mkdir(db, oldPath.slice(0, -"/old.txt".length), { recursive: true }, () => 1); + writeFileSync(db, oldPath, new Uint8Array(), {}, () => 2); + + applyChangesSync( + db, + [ + { + kind: "symlink", + rev: 99, + path: "/dst", + mode: 0o777, + mtime: 3, + target: "/target", + }, + ], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })?.type).toBe("symlink"); + expect(readlink(db, "/dst")).toBe("/target"); + expect(resolveInode(db, oldPath, { followSymlinks: false })).toBeNull(); + }); + }); + + it("applies a directory over an existing file", async () => { + await withDB(async (db) => { + await writeFile(db, "/dst", "old", {}, () => 1); + + await applyChanges( + db, + [{ kind: "dir", rev: 99, path: "/dst", mode: 0o755, mtime: 2 }], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })).toMatchObject({ + type: "dir", + mode: 0o755, + mtime: 2, + }); + }); + }); + + it("applies a directory over an existing symlink", async () => { + await withDB((db) => { + symlink(db, "/target", "/dst", () => 1); + + applyChangesSync( + db, + [{ kind: "dir", rev: 99, path: "/dst", mode: 0o755, mtime: 2 }], + new Map(), + ); + + expect(resolveInode(db, "/dst", { followSymlinks: false })).toMatchObject({ + type: "dir", + mode: 0o755, + mtime: 2, + }); + }); + }); + + it("applies a coalesced file-to-directory replacement", async () => { + await withTwoDBs( + async (a) => { + await writeFile(a, "/dst", "old", {}, () => 1); + const cursor = currentRev(a); + rm(a, "/dst", { force: true }); + mkdir(a, "/dst", {}, () => 2); + const entries = await drain(coalesceChanges(a, cursor)); + expect(entries).toEqual([expect.objectContaining({ kind: "dir", path: "/dst" })]); + return { entries, objects: await collectObjects(a, entries) }; + }, + async (b, { entries, objects }) => { + await writeFile(b, "/dst", "old", {}, () => 1); + + await applyChanges(b, entries, objects); + + expect(resolveInode(b, "/dst", { followSymlinks: false })?.type).toBe("dir"); + }, + ); + }); + it("handles delete entries", async () => { await withDB(async (db) => { await writeFile(db, "/gone.txt", "bye", {}, () => 1); diff --git a/packages/dofs/src/sync/apply.ts b/packages/dofs/src/sync/apply.ts index 3b02b202..c984f8ec 100644 --- a/packages/dofs/src/sync/apply.ts +++ b/packages/dofs/src/sync/apply.ts @@ -3,7 +3,10 @@ import { readOnlyRootFor } from "../fs/mount-guard.js"; import { resolveInode } from "../fs/resolve.js"; import { rm } from "../fs/rm.js"; import { symlink } from "../fs/symlink.js"; +import { unlinkDirent } from "../fs/unlink.js"; import { writeFile, writeFileSync } from "../fs/writeFile.js"; +import { canonicalizePath } from "../path.js"; +import { incrementRev } from "../rev.js"; import type { Database } from "../storage.js"; import type { ChangeEntry } from "./changes.js"; import { computeManifestHash } from "./manifests.js"; @@ -74,12 +77,136 @@ export interface ApplyOptions { const DEFAULT_MAX_BYTES = 64 * 1024 * 1024; const DEFAULT_MAX_PATHS = 1024; +type NodeType = "file" | "dir" | "symlink"; + function hex(bytes: Uint8Array): string { let s = ""; for (let i = 0; i < bytes.byteLength; i++) s += bytes[i].toString(16).padStart(2, "0"); return s; } +function removeReplaceableFinalEntry( + db: Database, + path: string, + incomingKind: "file" | "symlink", +): void { + const existing = resolveInode(db, path, { followSymlinks: false }); + if (existing === null) return; + if (incomingKind === "file" && existing.type === "file") return; + + removeInodeTreeAtPath(db, path, existing.inode, existing.type); +} + +// Structural conflict cleanup for upstream applies. This removes +// the local shape without recording tombstones because the incoming +// entry is the authoritative state for this path. +function removeInodeTreeAtPath(db: Database, path: string, inode: number, type: NodeType): void { + const root = direntForPath(db, path, inode); + const stack: Array<{ + path: string; + parentInode: number; + name: string; + inode: number; + type: NodeType; + expanded: boolean; + }> = [ + { + path, + parentInode: root.parentInode, + name: root.name, + inode, + type, + expanded: false, + }, + ]; + + while (stack.length > 0) { + const current = stack.pop(); + if (current === undefined) break; + + if (current.type === "dir" && !current.expanded) { + const children = db.all<{ name: string; child_inode: number; type: NodeType }>( + `SELECT d.name AS name, d.child_inode AS child_inode, n.type AS type + FROM vfs_dirents d + JOIN vfs_nodes n ON n.inode = d.child_inode + WHERE d.parent_inode = ?`, + current.inode, + ); + stack.push({ ...current, expanded: true }); + for (let i = children.length - 1; i >= 0; i--) { + const child = children[i]; + const childPath = current.path === "/" ? `/${child.name}` : `${current.path}/${child.name}`; + stack.push({ + path: childPath, + parentInode: current.inode, + name: child.name, + inode: child.child_inode, + type: child.type, + expanded: false, + }); + } + continue; + } + + // Unlink this one name and reap the inode only when its last link + // disappears, so a sibling hardlink (inside or outside the subtree) + // keeps the file alive. (parent, name) is unique, so this removes + // exactly the dirent the walk is visiting. + unlinkDirent(db, current.parentInode, current.name, current.inode, current.type); + } +} + +function direntForPath( + db: Database, + path: string, + inode: number, +): { parentInode: number; name: string } { + const { parts, path: canonical } = canonicalizePath(path); + if (parts.length === 0) { + throw new Error(`applyChanges: cannot structurally replace root ${canonical}`); + } + const name = parts[parts.length - 1]; + const parentPath = parts.length === 1 ? "/" : `/${parts.slice(0, -1).join("/")}`; + const parent = resolveInode(db, parentPath, { followSymlinks: false }); + if (parent === null || parent.type !== "dir") { + throw new Error(`applyChanges: parent missing for structural replacement ${canonical}`); + } + const child = db.scalar( + "SELECT child_inode FROM vfs_dirents WHERE parent_inode = ? AND name = ?", + parent.inode, + name, + ); + if (child !== inode) { + throw new Error(`applyChanges: dirent mismatch for structural replacement ${canonical}`); + } + return { parentInode: parent.inode, name }; +} + +function applyDirectoryEntry(db: Database, entry: Extract): void { + const mode = entry.mode & 0o7777; + const existing = resolveInode(db, entry.path, { followSymlinks: false }); + if (existing === null) { + mkdir(db, entry.path, { mode, recursive: true }, () => entry.mtime); + return; + } + if (existing.type !== "dir") { + removeInodeTreeAtPath(db, entry.path, existing.inode, existing.type); + mkdir(db, entry.path, { mode, recursive: true }, () => entry.mtime); + return; + } + + db.transactionSync(() => { + const rev = incrementRev(db); + db.run( + "UPDATE vfs_nodes SET mode = ?, mtime = ?, rev = ? WHERE inode = ?", + mode, + entry.mtime, + rev, + existing.inode, + ); + }); +} + // Drive a ChangeEntry stream against `db`, batching writes so peak // memory stays bounded and a crash mid-apply leaves the DB in a // consistent state. Each batch runs inside a single transactionSync @@ -150,13 +277,14 @@ export async function applyChanges( continue; } if (entry.kind === "dir") { - mkdir(db, entry.path, { mode: entry.mode, recursive: true }, () => entry.mtime); + applyDirectoryEntry(db, entry); applied++; pathsInBatch++; if (pathsInBatch >= maxPaths) flush(); continue; } if (entry.kind === "symlink") { + removeReplaceableFinalEntry(db, entry.path, "symlink"); symlink(db, entry.target, entry.path, () => entry.mtime); applied++; pathsInBatch++; @@ -190,6 +318,7 @@ export async function applyChanges( buf.set(p, off); off += p.byteLength; } + removeReplaceableFinalEntry(db, entry.path, "file"); await writeFile(db, entry.path, buf, { mode: entry.mode }, () => entry.mtime); applied++; bytesInBatch += total; @@ -226,7 +355,12 @@ export async function applyChanges( // In the unsafe case we leave pushRev alone. The next pushOnce // drains both the unpushed locals and the apply's own bumps; // the receiver's alreadyApplied() check suppresses the latter. - // One redundant round-trip per apply, bounded. + // Normal loopback suppression still catches the common case by + // advancing pushRev after the upstream apply. When pending local + // writes make that unsafe, the apply's rev bumps may be sent back + // once, but the origin sees matching live state in alreadyApplied() + // and drops them without another bump. That makes directory-entry + // re-application a bounded echo rather than an unbounded ping-pong. if (options.source === "upstream") { const revAfter = currentRev(db); const existing = readWatermark(db, "pushRev", options.backend); @@ -291,13 +425,14 @@ export function applyChangesSync( continue; } if (entry.kind === "dir") { - mkdir(db, entry.path, { mode: entry.mode, recursive: true }, () => entry.mtime); + applyDirectoryEntry(db, entry); applied++; pathsInBatch++; if (pathsInBatch >= maxPaths) flush(); continue; } if (entry.kind === "symlink") { + removeReplaceableFinalEntry(db, entry.path, "symlink"); symlink(db, entry.target, entry.path, () => entry.mtime); applied++; pathsInBatch++; @@ -328,6 +463,7 @@ export function applyChangesSync( buf.set(p, off); off += p.byteLength; } + removeReplaceableFinalEntry(db, entry.path, "file"); writeFileSync(db, entry.path, buf, { mode: entry.mode }, () => entry.mtime); applied++; bytesInBatch += total; @@ -354,12 +490,8 @@ export function applyChangesSync( } // Compare an entry against the local node graph. Returns true when -// the entry would be a no-op apply: the manifest hash (files), -// mode + symlink target (symlinks), or mode (dirs) already matches. -// We deliberately skip mtime comparison — mtime is metadata -// the source decides on, and re-applying it would still bump the -// local rev counter for nothing. Receivers see eventual mtime -// drift between peers; the wire stays quiet. +// the entry would be a no-op apply: the manifest hash (files), mode +// (dirs), or mode + symlink target (symlinks) already matches. function alreadyApplied(db: Database, entry: Exclude): boolean { const live = resolveInode(db, entry.path, { followSymlinks: false }); if (live === null) return false; @@ -375,7 +507,7 @@ function alreadyApplied(db: Database, entry: Exclude, +): boolean { + return (live.mode & 0o7777) === (entry.mode & 0o7777); +} + function uint8Equal(a: Uint8Array, b: Uint8Array): boolean { if (a.byteLength !== b.byteLength) return false; for (let i = 0; i < a.byteLength; i++) { diff --git a/packages/dofs/src/sync/coalesce.test.ts b/packages/dofs/src/sync/coalesce.test.ts index 38429820..c3a4b229 100644 --- a/packages/dofs/src/sync/coalesce.test.ts +++ b/packages/dofs/src/sync/coalesce.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { mkdir } from "../fs/mkdir.js"; import { rm } from "../fs/rm.js"; +import { symlink } from "../fs/symlink.js"; import { withDB } from "../fs/with-db.js"; import { writeFile } from "../fs/writeFile.js"; import { coalesceChanges } from "./coalesce.js"; @@ -57,6 +58,28 @@ describe("coalesceChanges", () => { }); }); + it("emits resolved delete paths for removes through intermediate symlinks", async () => { + await withDB(async (db) => { + mkdir(db, "/real", {}, () => 1); + await writeFile(db, "/real/file.txt", "x", {}, () => 2); + symlink(db, "/real", "/link", () => 3); + + rm(db, "/link/file.txt", {}); + + const entries = await drain(coalesceChanges(db, 0)); + expect(entries).toContainEqual({ + kind: "delete", + rev: expect.any(Number), + path: "/real/file.txt", + }); + expect(entries).not.toContainEqual({ + kind: "delete", + rev: expect.any(Number), + path: "/link/file.txt", + }); + }); + }); + it("delete-then-recreate yields a single live entry, not a delete", async () => { await withDB(async (db) => { await writeFile(db, "/x.txt", "first", {}, () => 1); diff --git a/packages/dofs/src/sync/coalesce.ts b/packages/dofs/src/sync/coalesce.ts index 6dabb005..acd9d725 100644 --- a/packages/dofs/src/sync/coalesce.ts +++ b/packages/dofs/src/sync/coalesce.ts @@ -1,33 +1,7 @@ -import { ROOT_INODE } from "../schema/index.js"; import type { Database } from "../storage.js"; import { type ChangeEntry, materialiseChange } from "./changes.js"; import { isIgnored } from "./ignore.js"; - -// Walk vfs_dirents from `inode` up to ROOT_INODE, gathering the path -// segments along the way. Returns null when the inode is unreachable -// (orphan after a partially-applied rm; should not happen inside a -// healthy DB but the caller treats null as "skip this entry"). -function pathOf(db: Database, inode: number): string | null { - if (inode === ROOT_INODE) return "/"; - const segments: string[] = []; - let current = inode; - // Bound the walk: a million levels deep is well past any real FS; - // anything beyond that is corruption and should not loop forever. - for (let i = 0; i < 1_000_000; i++) { - const row = db.one<{ parent_inode: number; name: string }>( - "SELECT parent_inode, name FROM vfs_dirents WHERE child_inode = ?", - current, - ); - if (row === undefined) return null; - segments.push(row.name); - if (row.parent_inode === ROOT_INODE) { - segments.reverse(); - return `/${segments.join("/")}`; - } - current = row.parent_inode; - } - return null; -} +import { pathOf } from "./paths.js"; // Yield one ChangeEntry per path touched since `sinceRev`. Per-path // coalescing: five rewrites of the same path between watermarks diff --git a/packages/dofs/src/sync/paths.ts b/packages/dofs/src/sync/paths.ts new file mode 100644 index 00000000..c49722b7 --- /dev/null +++ b/packages/dofs/src/sync/paths.ts @@ -0,0 +1,26 @@ +import { ROOT_INODE } from "../schema/index.js"; +import type { Database } from "../storage.js"; + +// Walk vfs_dirents from `inode` up to ROOT_INODE, gathering the path +// segments along the way. Returns null when the inode is unreachable. +export function pathOf(db: Database, inode: number): string | null { + if (inode === ROOT_INODE) return "/"; + const segments: string[] = []; + let current = inode; + // Bound the walk: a million levels deep is well past any real FS; + // anything beyond that is corruption and should not loop forever. + for (let i = 0; i < 1_000_000; i++) { + const row = db.one<{ parent_inode: number; name: string }>( + "SELECT parent_inode, name FROM vfs_dirents WHERE child_inode = ?", + current, + ); + if (row === undefined) return null; + segments.push(row.name); + if (row.parent_inode === ROOT_INODE) { + segments.reverse(); + return `/${segments.join("/")}`; + } + current = row.parent_inode; + } + return null; +} From ceb4fe1a4667e6a8a50e44d2632c648d2001d061 Mon Sep 17 00:00:00 2001 From: Nathan Disidore Date: Sat, 6 Jun 2026 15:12:23 -0500 Subject: [PATCH 2/3] rpc,dofs: Track fetch cursors by rev and path Large directory renames can produce thousands of entries at one revision. A scalar fetch watermark can only resume at rev boundaries, so a crash in the middle of one of those streams forces the next pull to replay the whole rev. Store fetch progress as a rev/path cursor and checkpoint committed batches inside a rev. fetchChanges now advertises a current cursor and streams only entries at or before that cursor, which keeps retry behavior deterministic while materialized entries read current data. This changes the RPC fetch shape from scalar revs to cursors. The durable object and wsd are deployed as a matched pair, so the protocol is updated in lockstep rather than negotiated across mixed versions. coalesceChanges emits one entry per name of a touched inode rather than the single name pathOf returns, so a hardlinked file reaches the wire under every name and a rename of such a file no longer drops its new path. pullOnce owns the fetchChanges result envelope in a try/finally, disposing it on the cross-side invariant trip and on an apply error as well as on the clean drain, so a failing pull no longer leaks the stream stub for the life of the session. A cursor is a resume point, not a point-in-time snapshot handle. coalesceChanges materializes each path's current state and the store keeps no history, so a path that races past the advertised cursor is deferred to a later pull rather than frozen at the cursor's rev. The docs say so explicitly: a path=null cursor means every change committed through that rev has been offered, and convergence holds because the cursor never advances past the rev that would redeliver a deferred path. --- docs/02_sync_protocol.md | 138 +++++-- docs/03_filesystem_schema.md | 24 +- docs/06_mount_interface.md | 2 +- docs/08_capnweb_interface.md | 70 ++-- docs/11_lifecycle.md | 31 +- packages/dofs/README.md | 2 +- packages/dofs/src/index.ts | 13 +- packages/dofs/src/provider.test.ts | 4 +- packages/dofs/src/schema/index.ts | 5 + packages/dofs/src/schema/sync.ts | 11 + packages/dofs/src/sync/apply.test.ts | 29 +- packages/dofs/src/sync/apply.ts | 31 +- packages/dofs/src/sync/coalesce.test.ts | 189 +++++++++- packages/dofs/src/sync/coalesce.ts | 93 ++++- packages/dofs/src/sync/fetch.test.ts | 11 + packages/dofs/src/sync/fetch.ts | 5 +- packages/dofs/src/sync/invariant.test.ts | 30 +- packages/dofs/src/sync/invariant.ts | 25 +- packages/dofs/src/sync/paths.ts | 20 + packages/dofs/src/sync/watermarks.test.ts | 90 ++++- packages/dofs/src/sync/watermarks.ts | 80 +++- packages/rpc/src/interface.ts | 45 ++- packages/rpc/src/server.ts | 42 ++- packages/rpc/src/sync-driver.test.ts | 347 +++++++++++++++++- packages/rpc/src/sync-driver.ts | 293 +++++++-------- packages/rpc/tests/wire.test.ts | 61 ++- packages/workspace/src/mounts/index.test.ts | 8 +- .../workspace/src/observe-integration.test.ts | 8 +- packages/workspace/src/shell.test.ts | 2 +- packages/workspace/src/stub.test.ts | 8 +- packages/workspace/src/workspace.test.ts | 14 +- packages/workspace/tests/stub-soak-worker.ts | 8 +- packages/wsd/src/cli/wsd.test.ts | 5 +- packages/wsd/src/fuse/vfs.test.ts | 6 +- 34 files changed, 1316 insertions(+), 434 deletions(-) diff --git a/docs/02_sync_protocol.md b/docs/02_sync_protocol.md index 1bee5fb9..4cc1fe19 100644 --- a/docs/02_sync_protocol.md +++ b/docs/02_sync_protocol.md @@ -38,7 +38,12 @@ A typical `exec()` round-trip: 1. **Push.** The DO streams every `ChangeEntry` with a higher revision than the container has seen, **coalesced to one entry per path** (the latest state wins — five rewrites of the same - path between execs cost one entry on the wire, not five). Bytes + path between execs cost one entry on the wire, not five). A + hardlinked inode carries several names: `coalesceChanges` emits + one entry **per name**, so every path materialises on the + receiver. Hardlink identity is not preserved across the wire — + each name becomes an independent file with the same content, not + a shared inode. Bytes are not inline; entries carry chunk hashes only. The **sender** (the DO) calls `remote.hasObjects(...)` on the referenced hashes and follows up with `remote.pushObjects(missing)` for the subset @@ -54,10 +59,10 @@ A typical `exec()` round-trip: [06. Mount Interface](./06_mount_interface.md). 3. **Exec.** The command runs. FUSE writes are captured by the in-container VFS as they happen, each stamped with a fresh revision. -4. **Fetch.** The DO calls `fetchChanges({ sinceRev: fetchRev })`. The - container streams `ChangeEntry` records — one per touched path, - per-file entries carrying `chunks: (hash, size)[]`. No bytes - inline. +4. **Fetch.** The DO calls `fetchChanges({ after: fetchCursor })`. + The container streams `ChangeEntry` records after that `(rev, path)` + cursor — one per touched path, per-file entries carrying + `chunks: (hash, size)[]`. No bytes inline. 5. **Diff.** The DO reads up to `PULL_BATCH_SIZE` (256) entries from the stream, unions the chunk hashes referenced by that batch, probes its own `vfs_blobs` for which it already has, and calls @@ -69,13 +74,13 @@ A typical `exec()` round-trip: `transactionSync` inside `writeFile`/`mkdir`/`rm`/`symlink` is the real durability boundary. The driver then loops back to step 5 for the next batch. - `fetchRev` is advanced **per committed batch** to the max `rev` - any entry in that batch carried. `coalesceChanges` emits entries - in ascending rev order so this checkpoint is safe — everything - below `batchMaxRev` has been applied. A crash mid-pull resumes - from the last per-batch advance, so re-fetched work is bounded - by `PULL_BATCH_SIZE` (256) entries, not the whole stream. The - receiver's `alreadyApplied` check inside `applyChanges` still + The fetch cursor is advanced **per committed batch** to the last + streamed entry's `(rev, path)`. `coalesceChanges` emits entries in + ascending `rev`, then ascending `path`, so this checkpoint is safe + even when one rev contains more than one batch. A crash mid-pull + resumes from the last per-batch advance, so re-fetched work is + bounded by `PULL_BATCH_SIZE` (256) entries, not the whole stream. + The receiver's `alreadyApplied` check inside `applyChanges` still drops already-applied entries on the floor so re-apply is idempotent and cheap. @@ -105,6 +110,70 @@ applies the upstream entry. This is last-writer-wins conflict handling: it converges the tree, but local-only children under the conflicting path are discarded without separate tombstones. +## Alternatives considered + +Representing a rename as a full-subtree restamp produces one wire entry +per subtree item at a single revision. Two cheaper encodings were +considered and rejected. + +### A rename opcode + +A dedicated `rename` entry carrying `{ fromPath, toPath, inode }` would +collapse a directory move to one wire row. It was rejected because it is +an operation, while the rest of the protocol is state-based: +`materialiseChange` resolves each entry to the path's current state at +fetch time, the receiver reconciles against its own live state, and +`alreadyApplied` makes re-apply idempotent without ordered replay. + +An opcode breaks that model in three ways. It is relative to the +receiver's prior state: `relink(from -> to)` is meaningless to a peer +that never held `from`, so a cold-start peer pulling from rev 0 has +nothing to relink. The pull path is deliberately receiver-history +agnostic: the producer answers "changes after cursor X" by +materialising current state and knows nothing about what a given +receiver has seen, so it cannot decide when an opcode is safe to emit. +And making the opcode idempotent against final state requires +re-deriving the same state reconciliation the opcode was meant to avoid, +while still not solving cold start. The per-subtree cost is the price of +keeping one state-based representation that bootstraps, converges, and +replays under a single rule. + +### Chunking a rename across revisions + +A scalar fetch watermark can only resume at revision boundaries, so a +large rename at one rev forces a crash to replay the whole rev. One way +to bound that without a path cursor is to split a single rename across +many revisions, so a scalar watermark resumes at a chunk boundary. + +This was rejected because it weakens an invariant the protocol relies +on: `rev` is bumped atomically once per mutation (see +[03. Filesystem Schema](./03_filesystem_schema.md)), so every `rev` +value names one committed point in the mutation log. Chunking would +mint intermediate revisions that never committed as a whole, leaving +most `rev` values describing tree states that never existed. + +The `(rev, path)` fetch cursor avoids that. `path` is an orthogonal +second coordinate that records resume progress within a rev. A rename +still stamps exactly one revision across its subtree, while a crash can +resume mid-rev. Resumability is bought without minting phantom +revisions. + +**What a cursor guarantees.** A `(rev, path)` cursor is a *resume +point*, not a snapshot handle. `{rev, path: null}` means "every change +committed at or before `rev` has been offered to the receiver"; a +non-null `path` means "offered up to `path` within `rev`." It is +deliberately not a point-in-time snapshot read: `coalesceChanges` +materialises each path's *current* state at stream time, because the +store keeps no content history. A path that is rewritten or deleted +again after a snapshot opens has its live rev pushed past the +advertised `currentCursor`, so that entry is dropped from the current +stream and redelivered under a later cursor. The receiver's tree at +cursor `{5, null}` therefore need not byte-match the rev-5 snapshot for +a path that raced ahead — but convergence holds, because the rev that +caused the drop is greater than `currentCursor.rev`, so the next pull +re-scans and delivers the path's then-current state. The cursor never +advances past the rev that would redeliver an omitted path. + ### Chunking Files are split at a fixed `CHUNK_SIZE` (512 KiB). Chunk boundaries are @@ -127,10 +196,10 @@ one name. | Watermark | Owner | Meaning | | --- | --- | --- | | `pushRev` | DO | Last DO-side `rev` successfully pushed to the container. | -| `fetchRev` | DO | Last container-side `rev` the DO has fetched. | +| `fetchCursor` | DO | Last container-side cursor the DO has fetched; `path = null` means every change committed at or before that rev has been offered (a resume point, not a point-in-time snapshot — see above). | | `currentRev` | DO | Latest `rev` stamped on a DO-side mutation. | | `currentRev` | Container | Latest `rev` stamped on a container-side mutation. | -| `appliedPushRev` | Container | Largest DO `rev` the container has fully applied. Echoed on every **push** response. | +| `appliedPushCursor` | Container | DO-side cursor the container has applied. Echoed on every **push** and **fetchChanges** response. | The DO watermarks live in the `_vfs_watermark` table so they survive DO restarts. The container's watermarks live in the same `Database` @@ -144,15 +213,14 @@ fresh receiver). ### Cross-side invariant After every successful `push` **and** every `fetchChanges`, the -response carries the receiver's current `appliedPushRev` (the -largest `senderRev` it has fully applied). The DO asserts -`appliedPushRev >= pushRev` before continuing. The two sides never -share a single clock, but echoing the largest applied rev makes the -"receiver is caught up with our pushes" invariant inspectable on -the wire instead of load-bearing in-process state. A regression in -the post-apply `pushRev` advancement path (see step 1 above) trips -the assertion on the next push or pull rather than corrupting data -silently. +response carries the receiver's current `appliedPushCursor`. The DO +asserts that cursor covers its local `{ rev: pushRev, path: null }` +before continuing. The two sides never share a single clock, but +echoing the applied cursor makes the "receiver is caught up with our +pushes" invariant inspectable on the wire instead of load-bearing +in-process state. A regression in the post-apply cursor advancement +path trips the assertion on the next push or pull rather than +corrupting data silently. ## Wire shape @@ -161,10 +229,14 @@ records, both probe with `hasObjects`, both transfer bytes by hash. Naming follows git's vocabulary — the DO *pushes* entries and objects to the container, and *fetches* entries and objects back. +The DO and `wsd` are deployed as a matched pair. The protocol has no +version negotiation, so changes to request or response shapes are hard +wire breaks and require lockstep rollout. + | RPC | Direction | Returns | Notes | | --- | --- | --- | --- | -| `push({ senderRev, changes })` | DO → container | `{ rev, appliedPushRev }` | Streams a coalesced batch of `ChangeEntry` via the `changes` `ReadableStream`. The sender then calls `hasObjects` on the referenced hashes and follows up with `pushObjects` for the missing subset. See the `senderRev` branches below. | -| `fetchChanges({ sinceRev?, ignore? })` | container → DO | `Promise<{ currentRev, appliedPushRev, stream: ReadableStream }>` | Streams one entry per touched path. For files, `chunks: (hash, size)[]` (no bytes inline); for dirs, metadata; for deletes, a tombstone. `currentRev` is the receiver's rev at stream open; the puller advances `fetchRev` no further than this. `appliedPushRev` carries the cross-side invariant check on the pull path. | +| `push({ senderRev, changes })` | DO → container | `{ rev, appliedPushCursor }` | Streams a coalesced batch of `ChangeEntry` via the `changes` `ReadableStream`. The sender then calls `hasObjects` on the referenced hashes and follows up with `pushObjects` for the missing subset. See the `senderRev` branches below. | +| `fetchChanges({ after?, ignore? })` | container → DO | `Promise<{ currentCursor, appliedPushCursor, stream: ReadableStream }>` | Streams one entry per touched path after `after`, ordered by `rev` then `path`. For files, `chunks: (hash, size)[]` (no bytes inline); for dirs, metadata; for deletes, a tombstone. `currentCursor` is `{ rev: currentRev, path: null }` at stream open; the puller writes it after a clean drain. `appliedPushCursor` carries the cross-side invariant check on the pull path. | | `hasObjects(hashes[])` | sender probes receiver | `Uint8Array[]` | Returns the subset of the input the receiver already holds. The git `have` line, batched. | | `fetchObjects(hashes[])` | container → DO | `ReadableStream<{ hash, bytes }>` | Streams chunk bytes by hash. The git `want`/pack response on the fetch path. | | `pushObjects(objects)` | DO → container | `void` | Streams chunk bytes by hash. The push-direction mirror of `fetchObjects`. | @@ -177,7 +249,8 @@ load-test rationale): - **`senderRev > 0` — sync peer.** A DO calling its container counterpart (or vice versa). The receiver applies the batch as `upstream`, - advances its own `fetchRev` to `senderRev`, and on the *sender's* + advances its own fetch cursor to `{ rev: senderRev, path: null }`, + and on the *sender's* side `pushRev` is advanced past the rev just shipped (gated on no interleaved local writes — see step 1 above). - **`senderRev === 0` — external writer / fresh receiver.** Used by @@ -197,7 +270,7 @@ edited file) shows up exactly once on the wire. See - **Container restart mid-exec.** The DO's connection detects the closed WebSocket and self-destructs. The next call transparently rebuilds against the still-running `wsd` (or restarts it if needed). - `pushRev` and `fetchRev` mean the catch-up is incremental, modulo + `pushRev` and the fetch cursor mean the catch-up is incremental, modulo whatever the container's deployment chose for its DB lifetime. - **Container crash mid-apply.** `push` is atomic from the DO's perspective on the receiver: the server wraps the whole batch in a @@ -209,11 +282,12 @@ edited file) shows up exactly once on the wire. See applied so far; the receiver never sees a partial push. The pull path keeps the per-mutation model because the streaming batches can't hold a synchronous transaction across network I/O. -- **DO restart mid-pull.** `fetchRev` advances per committed batch - to the max `rev` the batch carried, so a restart mid-pull resumes - from the last per-batch checkpoint. Wasted work is bounded by - `PULL_BATCH_SIZE` entries (256), not the whole stream. End state is - correct either way — apply is idempotent. +- **DO restart mid-pull.** The fetch cursor advances per committed + batch to the last entry's `(rev, path)`, so a restart mid-pull + resumes from the last per-batch checkpoint, including within a + single large rev. Wasted work is bounded by `PULL_BATCH_SIZE` + entries (256), not the whole stream. End state is correct either + way — apply is idempotent. - **DO restart.** Watermarks are persisted, so the new DO instance picks up where the old one left off. The container keeps `wsd` alive across the gap. diff --git a/docs/03_filesystem_schema.md b/docs/03_filesystem_schema.md index 87c36363..9df4dfaa 100644 --- a/docs/03_filesystem_schema.md +++ b/docs/03_filesystem_schema.md @@ -70,10 +70,9 @@ positional read primitive can read it directly instead of running `SUM(size) FROM vfs_chunks` on every call. Every write path stamps it alongside `mode`/`mtime`/`rev`. -The `vfs_nodes_by_rev` index supports `coalesceChanges`'s -`WHERE rev > sinceRev` scan over live inodes, which the sync protocol -calls once per pull to enumerate everything modified since the last -fetch watermark. +The `vfs_nodes_by_rev` index supports `coalesceChanges`'s cursor scan +over live inodes, which the sync protocol calls once per pull to +enumerate everything modified after the last fetch cursor. There is no `ignored` column: ignored paths are entirely invisible to the DO-side filesystem API (see @@ -218,6 +217,23 @@ Stores `pushRev` and `fetchRev` (see [02. Sync Protocol](./02_sync_protocol.md#watermarks)). Survives DO restarts so reconnects resume cleanly. +### `_vfs_fetch_cursor` — fetch path tie-breaker + +```sql +CREATE TABLE _vfs_fetch_cursor ( + k TEXT PRIMARY KEY CHECK(k = 'fetch'), + path TEXT +); +``` + +Stores the path component for the fetch cursor. The numeric rev remains +in `_vfs_watermark.fetchRev`; `path = NULL` means every change committed +at or before that rev has been offered to the receiver, and a non-null +`path` resumes within `fetchRev`. The cursor is a resume point, not a +point-in-time snapshot: a path rewritten after a fetch opens is deferred +to a later cursor rather than frozen at `fetchRev`. See +[02. Sync Protocol](./02_sync_protocol.md) for the full contract. + ### `_vfs_mounts` — mount index state ```sql diff --git a/docs/06_mount_interface.md b/docs/06_mount_interface.md index 3f361276..3c911fb9 100644 --- a/docs/06_mount_interface.md +++ b/docs/06_mount_interface.md @@ -284,7 +284,7 @@ This is deliberate, not an oversight: - R2 and GitHub have no monotonic rev clock. Treating them as peers would force per-tick polling and a diff against a remembered snapshot. -- The protocol's invariants — `appliedPushRev`, watermark +- The protocol's invariants — `appliedPushCursor`, watermark reconciliation, tombstones — assume one peer. They don't generalize to N peers without a real CRDT / LWW story. - The "container always wins" conflict policy is a deliberate diff --git a/docs/08_capnweb_interface.md b/docs/08_capnweb_interface.md index 00771a79..b28aa5e8 100644 --- a/docs/08_capnweb_interface.md +++ b/docs/08_capnweb_interface.md @@ -58,34 +58,42 @@ interface SyncRPC { // not inline: the DO sends ChangeEntry records with chunk hashes, // the receiver calls back via hasObjects / pushObjects for the // missing subset. Returns the receiver's new rev plus the - // appliedPushRev it stamped for this batch. + // appliedPushCursor it stamped for this batch. push(input: { senderRev: number; changes: ReadableStream; - }): Promise<{ rev: number; appliedPushRev: number }>; + }): Promise<{ + rev: number; + appliedPushCursor: { rev: number; path: string | null }; + }>; - // Container ← DO. Stream every ChangeEntry with rev > sinceRev, - // alongside the receiver's currentRev (cursor the puller advances - // fetchRev to) and appliedPushRev (cross-side invariant check on - // the pull path, mirroring the push response). Per-file entries - // carry (hash, size) chunk lists; no bytes inline. + // Container ← DO. Stream every ChangeEntry after `after`, ordered + // by rev then path. The cursor is a resume point, not a snapshot + // handle: path=null means every change committed at or before that + // rev has been offered; a cursor with path set resumes after that + // path inside the same rev. A path rewritten after the stream opens + // is deferred to a later cursor rather than frozen at this rev (see + // docs/02). `currentCursor` is the receiver's currentRev at stream + // open with path=null, and `appliedPushCursor` is the receiver's + // cursor for sender changes it has applied. Per-file entries carry + // (hash, size) chunk lists; no bytes inline. fetchChanges(input: { - sinceRev?: number; - ignore?: string[]; + after?: { rev: number; path: string | null }; + ignore?: string[]; }): Promise<{ - currentRev: number; - appliedPushRev: number; + currentCursor: { rev: number; path: string | null }; + appliedPushCursor: { rev: number; path: string | null }; stream: ReadableStream; }>; // Diagnostic surface for soak tests, dashboards, and the agent // when it wants to wait for the wire to drain. pushRev / - // fetchRev only move when the receiver is acting as a sync - // peer; otherwise they sit at 0. + // fetchCursor only move when the receiver is acting as a sync + // peer; otherwise they sit at 0 / { rev: 0, path: null }. watermarks(): Promise<{ currentRev: number; pushRev: number; - fetchRev: number; + fetchCursor: { rev: number; path: string | null }; }>; // Materialise a single path as a ChangeEntry without driving @@ -116,26 +124,32 @@ interface SyncRPC { } ``` +The durable object and `wsd` are deployed as a matched pair. This +interface has no version negotiation, so request and response shape +changes are hard wire breaks and require lockstep rollout. + `ChangeEntry` is defined in `packages/dofs/src/sync/changes.ts`. Schema column references match [03. Filesystem Schema](./03_filesystem_schema.md). #### Rev-0 baseline (no separate snapshot) There is no dedicated `snapshot()` RPC. A fresh DO with no watermark -calls `fetchChanges({ sinceRev: 0 })`, which streams every live entry -plus any tombstones the receiver has retained. Treating the baseline as -a degenerate fetch keeps the wire shape minimal: the same pull path -covers both cold-start replication and incremental catch-up. +calls `fetchChanges({ after: { rev: 0, path: null } })`, which streams +every live entry plus any tombstones the receiver has retained. +Treating the baseline as a degenerate fetch keeps the wire shape +minimal: the same pull path covers both cold-start replication and +incremental catch-up. #### Push semantics: peer vs external `push` distinguishes two callers via `senderRev`: - **`senderRev > 0` — sync peer.** The sender is replicating its own - log forward. The receiver advances `fetchRev` to `senderRev` once - the batch settles, echoes it back as `appliedPushRev`, and uses - that value to silence the loopback (the next `fetchChanges` from - the peer won't replay these entries back at it). + log forward. The receiver advances its fetch cursor to + `{ rev: senderRev, path: null }` once the batch settles, echoes it + back as `appliedPushCursor`, and uses that value to silence the + loopback (the next `fetchChanges` from the peer won't replay these + entries back at it). - **`senderRev === 0` — external orchestrator.** The sender doesn't have a rev space of its own (an agent, a CI script, a one-shot writer). The receiver applies the batch as ordinary local writes, @@ -199,19 +213,19 @@ ends: streaming `ChangeEntry` records, the container calls `hasObjects` on the chunk hashes referenced, the DO follows up with `pushObjects` (itself a stream) for the missing subset, the container applies the - batch and returns `{ rev, appliedPushRev }`. -- **Fetch (container → DO).** The DO calls `fetchChanges({ sinceRev })`, - which returns `{ currentRev, appliedPushRev, stream }` in one round- - trip. `currentRev` is the target watermark; `appliedPushRev` + batch and returns `{ rev, appliedPushCursor }`. +- **Fetch (container → DO).** The DO calls `fetchChanges({ after })`, + which returns `{ currentCursor, appliedPushCursor, stream }` in one round- + trip. `currentCursor` is the target cursor; `appliedPushCursor` carries the cross-side invariant (the puller asserts it covers - the local `pushRev` before draining). The DO then streams + `{ rev: pushRev, path: null }` before draining). The DO then streams `ChangeEntry` records, accumulates chunk hashes, calls `hasObjects` on itself (cheap, local) to find what it already has, then calls `fetchObjects` for the rest. | Aspect | Value | | --- | --- | -| Round-trips per fetch | 1 streaming `fetchChanges` (carries `currentRev` + `appliedPushRev` + entry stream) + 1 `hasObjects` per batch + 1 streaming `fetchObjects` per batch (only if any hashes are missing) | +| Round-trips per fetch | 1 streaming `fetchChanges` (carries `currentCursor` + `appliedPushCursor` + entry stream) + 1 `hasObjects` per batch + 1 streaming `fetchObjects` per batch (only if any hashes are missing) | | Round-trips per push | 1 streaming `push` (carries `senderRev`) + 1 `hasObjects` (server-driven) + 1 streaming `pushObjects` (only if any hashes are missing) | | Bytes inline in `ChangeEntry` | None — entries carry chunk hashes only | | Object transfer shape | `ReadableStream<{ hash, bytes }>` in both directions | diff --git a/docs/11_lifecycle.md b/docs/11_lifecycle.md index ecb990cf..d8781b48 100644 --- a/docs/11_lifecycle.md +++ b/docs/11_lifecycle.md @@ -44,7 +44,7 @@ The 1:1 mapping is load-bearing for several reasons: - The container's WebSocket peer is unambiguous — there is at most one capnweb session per DO at any time. -- The DO's persisted watermarks (`pushRev`, `fetchRev`) speak to a +- The DO's persisted watermarks (`pushRev`, fetch cursor) speak to a single counterparty. - Hibernation enablement (see below) becomes tractable because the DO doesn't have to multiplex multiple WS peers. @@ -79,9 +79,9 @@ an incarnation boundary. What survives is: - The SQLite store backing `Workspace.fs` — every committed `writeFile`, `mkdir`, `rm`, `symlink` is durable. -- The `_vfs_watermark` row, which holds `pushRev` (last DO-side rev - successfully pushed to the container) and `fetchRev` (last - container-side rev the DO has fetched). These are written via the +- The sync watermark rows, which hold `pushRev` (last DO-side rev + successfully pushed to the container) and the fetch cursor (last + container-side cursor the DO has fetched). These are written via the same SQLite transaction as the data they describe, so they cannot drift out of sync with the store. @@ -186,14 +186,15 @@ Because the rev counters drive every operation, a torn RPC is safe to retry against a fresh session. Specifically: - **`pushOnce`.** `pushRev` is written only after - `assertAppliedPushRev` succeeds. A torn push leaves `pushRev` at + `assertAppliedPushCursor` succeeds. A torn push leaves `pushRev` at the previous value; the next push replays the same batch. `applyChanges` on the receiver is idempotent. -- **`pullOnce`.** `fetchRev` advances per committed batch to the max - `rev` the batch carried. A torn pull leaves `fetchRev` at the last - per-batch checkpoint; the next pull re-fetches only the entries - past that point. `applyChanges`'s `alreadyApplied` check drops any - duplicates the resume happens to overlap with. +- **`pullOnce`.** The fetch cursor advances per committed batch to the + last streamed entry's `(rev, path)`. A torn pull leaves the cursor at + the last per-batch checkpoint; the next pull re-fetches only entries + past that point, including within the same rev. `applyChanges`'s + `alreadyApplied` check drops any duplicates the resume happens to + overlap with. - **`exec.events`.** Each event carries a monotonic `seq` per exec id. The client reattaches via `getExec({ id, after: seq })`. @@ -334,11 +335,11 @@ Two things have to change for capnweb + hibernation to work: wake as a fresh session. The peer must retry any in-flight RPC. This is the same semantics as a transport reset, which the protocol already handles via the rev cursors. - - **Sync streams: nothing to store.** `pushRev` and `fetchRev` - are already written to `_vfs_watermark` inside the same SQLite - transaction as the data they describe. On wake, the next - `pushOnce` / `pullOnce` reads them from durable storage and - resumes. No attachment write is required. + - **Sync streams: nothing to store.** `pushRev` and the durable + fetch cursor are already written to SQLite alongside the data + they describe. On wake, the next `pushOnce` / `pullOnce` reads + them from durable storage and resumes. No attachment write is + required. - **Exec streams: store `{ [id]: seq }` per in-flight exec.** The `WorkspaceShell` driver inside the DO is the only place that knows where the consumer got to in the event stream. diff --git a/packages/dofs/README.md b/packages/dofs/README.md index c9fb8470..737c9e0b 100644 --- a/packages/dofs/README.md +++ b/packages/dofs/README.md @@ -17,7 +17,7 @@ This package exposes a JavaScript module, not a CLI. It bundles three layers tha - A `Database` wrapper around Durable Object SQL storage plus `initializeSchema` for the `vfs_*` tables. - Filesystem primitives under `src/fs/*` (`mkdir`, `writeFile`, `readFile`, `rm`, `readdir`, `stat`, `lstat`, `chmod`, `find`, `ls`, `grep`, `symlink`, `readlink`, `gc`, `watch`) operating on a `Database`. - `SQLiteWorkspaceProvider`, a `@platformatic/vfs` adapter that composes those primitives into a node-shaped filesystem (fd table, positional `readSync`/`writeSync`, `watchSync`, symlinks). This is what `wsd` mounts via FUSE. -- Sync protocol building blocks operating on the same `Database`: `applyChanges`, `stageBlob`, `materialiseChange`, `coalesceChanges`, `fetchChanges`, `fetchObjects`, `hasObjects`, `pushObjects`, `buildManifest`, `currentRev`, `readWatermark`/`writeWatermark`, `assertAppliedPushRev`, and `DEFAULT_IGNORE`/`isIgnored`. The wire wiring lives in `@cloudflare/workspace-rpc`. +- Sync protocol building blocks operating on the same `Database`: `applyChanges`, `stageBlob`, `materialiseChange`, `coalesceChanges`, `fetchChanges`, `fetchObjects`, `hasObjects`, `pushObjects`, `buildManifest`, `currentRev`, `compareChangeCursors`, `readWatermark`/`writeWatermark`, `assertAppliedPushCursor`, and `DEFAULT_IGNORE`/`isIgnored`. The wire wiring lives in `@cloudflare/workspace-rpc`. Minimal DO-side usage — initialize the schema; the `Database` becomes the handle every other helper takes: diff --git a/packages/dofs/src/index.ts b/packages/dofs/src/index.ts index abca31e7..6277a640 100644 --- a/packages/dofs/src/index.ts +++ b/packages/dofs/src/index.ts @@ -43,12 +43,19 @@ export type { CoalesceOptions } from "./sync/coalesce.js"; export { coalesceChanges } from "./sync/coalesce.js"; export { fetchChanges, fetchObjects, hasObjects } from "./sync/fetch.js"; export { DEFAULT_IGNORE, isIgnored } from "./sync/ignore.js"; -export { assertAppliedPushRev } from "./sync/invariant.js"; +export { assertAppliedPushCursor } from "./sync/invariant.js"; export type { ManifestChunk } from "./sync/manifests.js"; export { buildManifest, MANIFEST_VERSION } from "./sync/manifests.js"; export { pushObjects } from "./sync/push.js"; -export type { WatermarkKey } from "./sync/watermarks.js"; -export { currentRev, readWatermark, writeWatermark } from "./sync/watermarks.js"; +export type { ChangeCursor, WatermarkKey } from "./sync/watermarks.js"; +export { + compareChangeCursors, + currentRev, + readFetchCursor, + readWatermark, + writeFetchCursor, + writeWatermark, +} from "./sync/watermarks.js"; export type { ExecutedStatement } from "./testing-recording.js"; // RecordingStorage is workerd-safe (pure JS). SQLiteTestStorage // wraps node:sqlite and must be imported from diff --git a/packages/dofs/src/provider.test.ts b/packages/dofs/src/provider.test.ts index 18a4d5ab..bb25cae9 100644 --- a/packages/dofs/src/provider.test.ts +++ b/packages/dofs/src/provider.test.ts @@ -19,9 +19,9 @@ async function withProviderAndDB( return withDB((db) => fn(new SQLiteWorkspaceProvider(db, { now: () => 1000 }), db)); } -async function drainChanges(db: Database, sinceRev: number): Promise { +async function drainChanges(db: Database, afterRev: number): Promise { const out: ChangeEntry[] = []; - for await (const entry of coalesceChanges(db, sinceRev)) out.push(entry); + for await (const entry of coalesceChanges(db, afterRev)) out.push(entry); return out; } diff --git a/packages/dofs/src/schema/index.ts b/packages/dofs/src/schema/index.ts index 3e479940..e0147e96 100644 --- a/packages/dofs/src/schema/index.ts +++ b/packages/dofs/src/schema/index.ts @@ -64,6 +64,11 @@ export function initializeSchema(db: Database, now: () => number): void { "fetchRev", 0, ); + db.run( + "INSERT OR IGNORE INTO _vfs_fetch_cursor (k, backend, path) VALUES (?, 'default', ?)", + "fetch", + null, + ); db.run( `INSERT OR IGNORE INTO vfs_nodes diff --git a/packages/dofs/src/schema/sync.ts b/packages/dofs/src/schema/sync.ts index 3680b455..fdbac101 100644 --- a/packages/dofs/src/schema/sync.ts +++ b/packages/dofs/src/schema/sync.ts @@ -34,6 +34,17 @@ export const SYNC_STATEMENTS = [ v INTEGER NOT NULL, PRIMARY KEY (k, backend) )`, + // The fetch cursor's same-rev `path` component, keyed by + // (k, backend) so each backend resumes a partially-drained rev + // independently. The rev component lives in _vfs_watermark under + // 'fetchRev'; this table only holds the in-rev path. `backend` + // mirrors _vfs_watermark and defaults to 'default'. + `CREATE TABLE IF NOT EXISTS _vfs_fetch_cursor ( + k TEXT NOT NULL CHECK(k = 'fetch'), + backend TEXT NOT NULL DEFAULT 'default', + path TEXT, + PRIMARY KEY (k, backend) + )`, // The `mode` column was added at schema v2; `schema/migrations.ts` // owns the ALTER for existing databases. Keep the CHECK // constraint here aligned with the migration's CHECK so fresh diff --git a/packages/dofs/src/sync/apply.test.ts b/packages/dofs/src/sync/apply.test.ts index fa20ef5e..314e2a9e 100644 --- a/packages/dofs/src/sync/apply.test.ts +++ b/packages/dofs/src/sync/apply.test.ts @@ -15,7 +15,7 @@ import { applyChanges, applyChangesSync } from "./apply.js"; import type { ChangeEntry } from "./changes.js"; import { coalesceChanges } from "./coalesce.js"; import { fetchObjects } from "./fetch.js"; -import { currentRev, writeWatermark } from "./watermarks.js"; +import { currentRev } from "./watermarks.js"; async function drain(it: AsyncIterable): Promise { const out: T[] = []; @@ -73,33 +73,6 @@ describe("applyChanges", () => { ); }); - it("advances fetchRev to the largest applied rev", async () => { - await withTwoDBs( - async (a) => { - await writeFile(a, "/x.txt", "x", {}, () => 1); - const entries = await drain(coalesceChanges(a, 0)); - return { entries, objects: await collectObjects(a, entries) }; - }, - async (b, { entries, objects }) => { - await applyChanges(b, entries, objects, { advanceFetchRev: 5 }); - const got = await import("./watermarks.js").then((m) => m.readWatermark(b, "fetchRev")); - expect(got).toBe(5); - }, - ); - }); - - it("does not regress fetchRev on partial replay", async () => { - await withDB(async (db) => { - // Pretend a previous apply pass advanced fetchRev to 10. - writeWatermark(db, "fetchRev", 10); - await applyChanges(db, [], new Map(), { advanceFetchRev: 3 }); - const got = await import("./watermarks.js").then((m) => m.readWatermark(db, "fetchRev")); - // The helper takes the max of current and requested, never - // moves backwards. - expect(got).toBe(10); - }); - }); - it("commits in batches capped by byte budget", async () => { // Force many small files; with a tiny byte budget the apply // path should still converge, just across more batches. We diff --git a/packages/dofs/src/sync/apply.ts b/packages/dofs/src/sync/apply.ts index c984f8ec..62a6362f 100644 --- a/packages/dofs/src/sync/apply.ts +++ b/packages/dofs/src/sync/apply.ts @@ -50,11 +50,6 @@ export interface ApplyOptions { maxBytesPerBatch?: number; // Soft cap on entries per batch. Default 1024 paths. maxPathsPerBatch?: number; - // After the stream drains, advance fetchRev to this value if it's - // higher than the current persisted value. Callers pass the - // sender's currentRev so the next pull resumes from the right - // cursor. Never regresses the watermark. - advanceFetchRev?: number; // Where the entries came from. 'local' (default) treats the apply // path like any other mutation: writeFile/mkdir/etc bump vfs_meta.rev // and the push loop later ships those new revs upstream. 'upstream' @@ -326,16 +321,6 @@ export async function applyChanges( if (bytesInBatch >= maxBytes || pathsInBatch >= maxPaths) flush(); } - // Advance fetchRev only after the stream drains so a crash - // mid-apply leaves the watermark behind and the next pull - // re-fetches anything not yet committed. - if (options.advanceFetchRev !== undefined) { - const current = readWatermark(db, "fetchRev", options.backend); - if (options.advanceFetchRev > current) { - writeWatermark(db, "fetchRev", options.advanceFetchRev, options.backend); - } - } - // Loopback suppression: when this apply pass reflects entries // from upstream, the writeFile/mkdir/symlink/rm calls inside // bumped vfs_meta.rev. Without this advance, the next push tick @@ -471,13 +456,6 @@ export function applyChangesSync( if (bytesInBatch >= maxBytes || pathsInBatch >= maxPaths) flush(); } - if (options.advanceFetchRev !== undefined) { - const current = readWatermark(db, "fetchRev", options.backend); - if (options.advanceFetchRev > current) { - writeWatermark(db, "fetchRev", options.advanceFetchRev, options.backend); - } - } - if (options.source === "upstream") { const revAfter = currentRev(db); const existing = readWatermark(db, "pushRev", options.backend); @@ -507,7 +485,7 @@ function alreadyApplied(db: Database, entry: Exclude, -): boolean { - return (live.mode & 0o7777) === (entry.mode & 0o7777); -} - function uint8Equal(a: Uint8Array, b: Uint8Array): boolean { if (a.byteLength !== b.byteLength) return false; for (let i = 0; i < a.byteLength; i++) { diff --git a/packages/dofs/src/sync/coalesce.test.ts b/packages/dofs/src/sync/coalesce.test.ts index c3a4b229..3bbc9749 100644 --- a/packages/dofs/src/sync/coalesce.test.ts +++ b/packages/dofs/src/sync/coalesce.test.ts @@ -1,11 +1,14 @@ import { describe, expect, it } from "vitest"; +import { link } from "../fs/link.js"; import { mkdir } from "../fs/mkdir.js"; +import { rename } from "../fs/rename.js"; import { rm } from "../fs/rm.js"; import { symlink } from "../fs/symlink.js"; import { withDB } from "../fs/with-db.js"; -import { writeFile } from "../fs/writeFile.js"; +import { writeFile, writeFileSync } from "../fs/writeFile.js"; import { coalesceChanges } from "./coalesce.js"; +import { currentRev } from "./watermarks.js"; // Drain an async iterable into an array. Tests stay synchronous-looking // while the production code can stream. @@ -23,7 +26,7 @@ describe("coalesceChanges", () => { }); }); - it("yields one entry per touched path since sinceRev", async () => { + it("yields one entry per touched path after the cursor", async () => { await withDB(async (db) => { mkdir(db, "/d", { mode: 0o755 }, () => 1); await writeFile(db, "/d/a.txt", "alpha", {}, () => 2); @@ -92,7 +95,7 @@ describe("coalesceChanges", () => { }); }); - it("sinceRev filters out changes the receiver has already seen", async () => { + it("cursor rev filters out changes the receiver has already seen", async () => { await withDB(async (db) => { await writeFile(db, "/old.txt", "old", {}, () => 1); // Read current rev counter to use as the cursor. @@ -125,6 +128,123 @@ describe("coalesceChanges", () => { } }); }); + + it("resumes after a path within the same rev", async () => { + await withDB(async (db) => { + mkdir(db, "/src", {}, () => 1); + await writeFile(db, "/src/a.txt", "a", {}, () => 2); + await writeFile(db, "/src/b.txt", "b", {}, () => 3); + const beforeRename = currentRev(db); + + rename(db, "/src", "/dst"); + const renameRev = currentRev(db); + expect(renameRev).toBeGreaterThan(beforeRename); + + const allSameRev = await drain(coalesceChanges(db, { rev: beforeRename, path: null })); + const paths = allSameRev.map((entry) => entry.path); + expect(paths).toEqual([...paths].sort()); + expect(new Set(allSameRev.map((entry) => entry.rev))).toEqual(new Set([renameRev])); + + const resumed = await drain(coalesceChanges(db, { rev: renameRev, path: "/dst/a.txt" })); + expect(resumed.map((entry) => entry.path)).toEqual( + paths.filter((path) => path > "/dst/a.txt"), + ); + }); + }); + + it("skips a whole rev when the cursor path is null", async () => { + await withDB(async (db) => { + mkdir(db, "/src", {}, () => 1); + await writeFile(db, "/src/a.txt", "a", {}, () => 2); + const beforeRename = currentRev(db); + + rename(db, "/src", "/dst"); + const renameRev = currentRev(db); + expect(await drain(coalesceChanges(db, { rev: beforeRename, path: null }))).not.toEqual([]); + expect(await drain(coalesceChanges(db, { rev: renameRev, path: null }))).toEqual([]); + }); + }); + + it("orders entries deterministically by rev then path", async () => { + await withDB(async (db) => { + mkdir(db, "/src", {}, () => 1); + await writeFile(db, "/src/b.txt", "b", {}, () => 2); + await writeFile(db, "/src/a.txt", "a", {}, () => 3); + rename(db, "/src", "/dst"); + + const entries = await drain(coalesceChanges(db, { rev: 0, path: null })); + const pairs = entries.map((entry) => [entry.rev, entry.path] as const); + expect(pairs).toEqual( + [...pairs].sort((a, b) => { + if (a[0] !== b[0]) return a[0] - b[0]; + return a[1].localeCompare(b[1]); + }), + ); + }); + }); + + it("excludes entries newer than the through rev", async () => { + await withDB(async (db) => { + await writeFile(db, "/included.txt", "included", {}, () => 1); + const throughRev = currentRev(db); + await writeFile(db, "/excluded.txt", "excluded", {}, () => 2); + + const entries = await drain( + coalesceChanges(db, { rev: 0, path: null }, { through: { rev: throughRev, path: null } }), + ); + + expect(entries.map((entry) => entry.path)).toContain("/included.txt"); + expect(entries.map((entry) => entry.path)).not.toContain("/excluded.txt"); + }); + }); + + it("excludes same-rev entries after the through path", async () => { + await withDB(async (db) => { + mkdir(db, "/src", {}, () => 1); + await writeFile(db, "/src/a.txt", "a", {}, () => 2); + await writeFile(db, "/src/b.txt", "b", {}, () => 3); + const beforeRename = currentRev(db); + + rename(db, "/src", "/dst"); + const renameRev = currentRev(db); + + const entries = await drain( + coalesceChanges( + db, + { rev: beforeRename, path: null }, + { + through: { rev: renameRev, path: "/dst/a.txt" }, + }, + ), + ); + + expect(entries.map((entry) => entry.path)).toEqual(["/dst", "/dst/a.txt"]); + }); + }); + + it("skips an entry that materializes beyond the through cursor", async () => { + await withDB(async (db) => { + await writeFile(db, "/file.txt", "first", {}, () => 1); + const throughRev = currentRev(db); + + const originalOne = db.one.bind(db); + let rewrote = false; + db.one = ((query: string, ...bindings: unknown[]) => { + if (!rewrote && query === "SELECT rev FROM vfs_nodes WHERE inode = ?") { + rewrote = true; + writeFileSync(db, "/file.txt", new TextEncoder().encode("second"), {}, () => 2); + } + return originalOne(query, ...bindings); + }) as typeof db.one; + + const entries = await drain( + coalesceChanges(db, { rev: 0, path: null }, { through: { rev: throughRev, path: null } }), + ); + + expect(rewrote).toBe(true); + expect(entries).toEqual([]); + }); + }); }); describe("coalesceChanges (ignore)", () => { @@ -156,4 +276,67 @@ describe("coalesceChanges (ignore)", () => { expect(entries.some((e) => e.path === "/node_modules/x.js")).toBe(true); }); }); + + it("emits an entry for every hardlink name of a touched inode", async () => { + await withDB(async (db) => { + await writeFile(db, "/a", "shared", {}, () => 1); + link(db, "/a", "/b"); + + const entries = await drain(coalesceChanges(db, 0)); + const files = entries.filter((e) => e.kind === "file").map((e) => e.path); + // Both names share one inode; pathOf would pick only one of them. + // The wire has to carry both so the receiver materialises each. + expect(files).toContain("/a"); + expect(files).toContain("/b"); + }); + }); + + it("defers a path whose live state raced past the snapshot, then redelivers it", async () => { + // Pins the documented cursor contract (docs/02_sync_protocol.md): + // `through` is a resume bound, not a point-in-time snapshot. A path + // deleted at the snapshot rev but recreated above it is dropped from + // the bounded scan, because materialiseChange reads the live state + // and inCursorWindow filters it out. The omission is not loss — the + // recreate's rev is above the snapshot, so the next scan redelivers + // the path. Convergence holds without the store keeping history. + await withDB(async (db) => { + await writeFile(db, "/x", "v1", {}, () => 1); + rm(db, "/x", {}); + const snapshot = currentRev(db); + await writeFile(db, "/x", "v2", {}, () => 2); + + // Bounded by the snapshot: the rev-`snapshot` delete is a + // candidate, but the live entry now sits above the window, so /x + // is omitted from this snapshot rather than frozen at the delete. + const bounded = await drain( + coalesceChanges(db, 0, { through: { rev: snapshot, path: null } }), + ); + expect(bounded.some((e) => e.path === "/x")).toBe(false); + + // The next scan resumes after the snapshot rev and redelivers /x + // at its current state. The rev that caused the drop is above the + // snapshot, so a later window always covers it. + const next = await drain(coalesceChanges(db, snapshot)); + expect(next.find((e) => e.path === "/x")).toMatchObject({ kind: "file", path: "/x" }); + }); + }); + + it("emits the new name when a hardlinked file is renamed", async () => { + await withDB(async (db) => { + await writeFile(db, "/a", "shared", {}, () => 1); + link(db, "/a", "/b"); + // /a and /b now share an inode. Renaming /a to /c leaves the + // inode named /b and /c; pathOf might resolve it to /b and never + // emit /c, dropping the renamed name on the wire. + const baseline = currentRev(db); + rename(db, "/a", "/c"); + + const entries = await drain(coalesceChanges(db, baseline)); + const live = entries.filter((e) => e.kind !== "delete").map((e) => e.path); + const deletes = entries.filter((e) => e.kind === "delete").map((e) => e.path); + expect(live).toContain("/c"); + expect(live).toContain("/b"); + expect(deletes).toContain("/a"); + }); + }); }); diff --git a/packages/dofs/src/sync/coalesce.ts b/packages/dofs/src/sync/coalesce.ts index acd9d725..bfd4b304 100644 --- a/packages/dofs/src/sync/coalesce.ts +++ b/packages/dofs/src/sync/coalesce.ts @@ -1,9 +1,10 @@ import type { Database } from "../storage.js"; import { type ChangeEntry, materialiseChange } from "./changes.js"; import { isIgnored } from "./ignore.js"; -import { pathOf } from "./paths.js"; +import { pathsOf } from "./paths.js"; +import { type ChangeCursor, compareChangeCursors } from "./watermarks.js"; -// Yield one ChangeEntry per path touched since `sinceRev`. Per-path +// Yield one ChangeEntry per path touched after `after`. Per-path // coalescing: five rewrites of the same path between watermarks // produce one entry (the latest state wins). Tombstoned paths get a // delete entry unless they have been recreated, in which case the @@ -21,14 +22,24 @@ export interface CoalesceOptions { // Path-segment patterns to drop before yielding. The wire never // carries entries under an ignored segment. ignore?: string[]; + // Internal snapshot bound. When set, entries are limited to the + // cursor window `after < entry <= through`. The bound is applied to + // each path's *live* rev at materialise time, not to a frozen + // snapshot: a path whose state moves past `through` after the scan + // (a concurrent rewrite or delete) is dropped here and redelivered + // under a later cursor, since the cursor never advances past the rev + // that caused the drop. See the redelivery note on the yield loop. + through?: ChangeCursor; } export async function* coalesceChanges( db: Database, - sinceRev: number, + after: ChangeCursor | number, options: CoalesceOptions = {}, ): AsyncIterable { const ignore = options.ignore ?? []; + const cursor = typeof after === "number" ? { rev: after, path: null } : after; + const through = options.through; // Build the per-path candidate set in two passes, keeping the // highest rev seen for each path. A live mutation that landed @@ -40,28 +51,48 @@ export async function* coalesceChanges( // Live mutations: every mkdir / writeFile / symlink bumps // vfs_nodes.rev. The by_rev index makes this a range scan. - const touched = db.all<{ inode: number; rev: number }>( - "SELECT inode, rev FROM vfs_nodes WHERE rev > ? ORDER BY rev", - sinceRev, - ); + const lowerRev = cursor.path === null ? cursor.rev : cursor.rev - 1; + const touched = + through === undefined + ? db.all<{ inode: number; rev: number }>( + "SELECT inode, rev FROM vfs_nodes WHERE rev > ? ORDER BY rev", + lowerRev, + ) + : db.all<{ inode: number; rev: number }>( + "SELECT inode, rev FROM vfs_nodes WHERE rev > ? AND rev <= ? ORDER BY rev", + lowerRev, + through.rev, + ); for (const { inode, rev } of touched) { - const path = pathOf(db, inode); - if (path === null) continue; - if (isIgnored(path, ignore)) continue; - const prior = candidates.get(path); - if (prior === undefined || rev > prior.rev) { - candidates.set(path, { path, rev }); + // One inode can carry several hardlink names; every name has to + // become a candidate so the wire materialises each, not just the + // arbitrary one pathOf would return. + for (const path of pathsOf(db, inode)) { + if (!inCursorWindow({ rev, path }, cursor, through)) continue; + if (isIgnored(path, ignore)) continue; + const prior = candidates.get(path); + if (prior === undefined || rev > prior.rev) { + candidates.set(path, { path, rev }); + } } } // Tombstones: each rm appends a row to vfs_changes with the // post-bump rev. The highest rev per path wins (a path can be // deleted-recreated-deleted; we want the last rm's rev). - const tombs = db.all<{ path: string; rev: number }>( - "SELECT path, MAX(rev) AS rev FROM vfs_changes WHERE rev > ? AND op = 'delete' GROUP BY path", - sinceRev, - ); + const tombs = + through === undefined + ? db.all<{ path: string; rev: number }>( + "SELECT path, MAX(rev) AS rev FROM vfs_changes WHERE rev > ? AND op = 'delete' GROUP BY path", + lowerRev, + ) + : db.all<{ path: string; rev: number }>( + "SELECT path, MAX(rev) AS rev FROM vfs_changes WHERE rev > ? AND rev <= ? AND op = 'delete' GROUP BY path", + lowerRev, + through.rev, + ); for (const { path, rev } of tombs) { + if (!inCursorWindow({ rev, path }, cursor, through)) continue; if (isIgnored(path, ignore)) continue; const prior = candidates.get(path); if (prior === undefined || rev > prior.rev) { @@ -77,8 +108,34 @@ export async function* coalesceChanges( return a.path < b.path ? -1 : a.path > b.path ? 1 : 0; }); + // materialiseChange reads each path's *current* state, not the state + // it held at the candidate's rev — the VFS keeps no content history. + // If a path was rewritten or deleted again after the scan, its live + // rev can now sit above `through`; inCursorWindow drops it. The + // dropped change is not lost: the rev that pushed it past `through` + // is, by definition, greater than `through.rev` (the cursor the + // puller persists), so the next pull's `after < entry` scan finds the + // path again and delivers its then-current state. The consequence is + // that a `{rev, null}` cursor means "every change committed at or + // before rev has been *offered*"; it does not guarantee the receiver + // tree byte-matches the rev snapshot for a path that raced ahead. + // Convergence is preserved because the cursor never advances past the + // racing rev. See docs/02_sync_protocol.md. for (const { path } of ordered) { const entry = materialiseChange(db, path); - if (entry !== null) yield entry; + if (entry !== null && inCursorWindow(entry, cursor, through)) { + yield entry; + } } } + +function inCursorWindow( + entry: ChangeCursor & { path: string }, + after: ChangeCursor, + through?: ChangeCursor, +): boolean { + return ( + compareChangeCursors(entry, after) > 0 && + (through === undefined || compareChangeCursors(entry, through) <= 0) + ); +} diff --git a/packages/dofs/src/sync/fetch.test.ts b/packages/dofs/src/sync/fetch.test.ts index 9ef3eaec..8d36c08d 100644 --- a/packages/dofs/src/sync/fetch.test.ts +++ b/packages/dofs/src/sync/fetch.test.ts @@ -38,6 +38,17 @@ describe("fetch wire", () => { }); }); + it("fetchChanges resumes from a rev/path cursor", async () => { + await withDB(async (db) => { + await writeFile(db, "/a.txt", "alpha", {}, () => 1); + await writeFile(db, "/b.txt", "beta", {}, () => 2); + const entries = await drain(fetchChanges(db, { rev: 0, path: null })); + const first = entries[0]; + const resumed = await drain(fetchChanges(db, { rev: first.rev, path: first.path })); + expect(resumed).toEqual(entries.slice(1)); + }); + }); + it("fetchObjects yields each hash exactly once", async () => { await withDB(async (db) => { await writeFile(db, "/a.txt", "shared", {}, () => 1); diff --git a/packages/dofs/src/sync/fetch.ts b/packages/dofs/src/sync/fetch.ts index f722a06d..9d4e3c07 100644 --- a/packages/dofs/src/sync/fetch.ts +++ b/packages/dofs/src/sync/fetch.ts @@ -2,6 +2,7 @@ import type { Database } from "../storage.js"; import type { ChangeEntry } from "./changes.js"; import { coalesceChanges } from "./coalesce.js"; import { pushObjects } from "./push.js"; +import type { ChangeCursor } from "./watermarks.js"; // The fetch wire is the mirror of the push wire: same SQL, // opposite direction. The DO calls fetchChanges / fetchObjects on @@ -10,10 +11,10 @@ import { pushObjects } from "./push.js"; export function fetchChanges( db: Database, - sinceRev: number, + after: ChangeCursor | number, options: { ignore?: string[] } = {}, ): AsyncIterable { - return coalesceChanges(db, sinceRev, options); + return coalesceChanges(db, after, options); } export function fetchObjects( diff --git a/packages/dofs/src/sync/invariant.test.ts b/packages/dofs/src/sync/invariant.test.ts index 65a80bf4..7e40d9f3 100644 --- a/packages/dofs/src/sync/invariant.test.ts +++ b/packages/dofs/src/sync/invariant.test.ts @@ -1,18 +1,32 @@ import { describe, expect, it } from "vitest"; -import { assertAppliedPushRev } from "./invariant.js"; +import { assertAppliedPushCursor } from "./invariant.js"; -describe("assertAppliedPushRev", () => { - it("passes when applied >= pushed", () => { - expect(() => assertAppliedPushRev(10, 10)).not.toThrow(); - expect(() => assertAppliedPushRev(11, 10)).not.toThrow(); +describe("assertAppliedPushCursor", () => { + it("passes when applied covers pushed", () => { + expect(() => + assertAppliedPushCursor({ rev: 10, path: null }, { rev: 10, path: null }), + ).not.toThrow(); + expect(() => + assertAppliedPushCursor({ rev: 11, path: "/partial.txt" }, { rev: 10, path: null }), + ).not.toThrow(); }); it("passes at zero", () => { - expect(() => assertAppliedPushRev(0, 0)).not.toThrow(); + expect(() => + assertAppliedPushCursor({ rev: 0, path: null }, { rev: 0, path: null }), + ).not.toThrow(); }); - it("throws when the container is behind the DO's push watermark", () => { - expect(() => assertAppliedPushRev(5, 10)).toThrowError(/appliedPushRev.*5.*pushRev.*10/i); + it("throws when the receiver only partially applied the pushed rev", () => { + expect(() => + assertAppliedPushCursor({ rev: 10, path: "/partial.txt" }, { rev: 10, path: null }), + ).toThrowError(/appliedPushCursor.*pushCursor/i); + }); + + it("throws when the receiver is behind the sender's push cursor", () => { + expect(() => + assertAppliedPushCursor({ rev: 5, path: null }, { rev: 10, path: null }), + ).toThrowError(/appliedPushCursor.*pushCursor/i); }); }); diff --git a/packages/dofs/src/sync/invariant.ts b/packages/dofs/src/sync/invariant.ts index dfdf04f6..036fd0e7 100644 --- a/packages/dofs/src/sync/invariant.ts +++ b/packages/dofs/src/sync/invariant.ts @@ -1,22 +1,29 @@ +import { type ChangeCursor, compareChangeCursors } from "./watermarks.js"; + // Cross-side invariant: every fetchChanges and push response carries -// the container's current appliedPushRev. The DO asserts -// appliedPushRev >= pushRev on every response. +// the receiver's current applied push cursor. The sender asserts that +// cursor covers its local push cursor on every response. // // The two sides never share a single clock, but echoing the largest -// applied DO rev makes the "container is caught up with the DO's +// applied sender cursor makes the "receiver is caught up with our // pushes" invariant inspectable on the wire instead of load-bearing -// in-process state. A regression in the suppress-dirty-tracking -// apply path trips the assertion immediately rather than corrupting -// data silently. +// in-process state. A regression in the suppress-dirty-tracking apply +// path trips the assertion immediately rather than corrupting data +// silently. // // Throwing an Error is the right escalation: a violation means the // protocol is broken; the connection should tear down and rebuild // rather than soldiering on with stale state. -export function assertAppliedPushRev(appliedPushRev: number, pushRev: number): void { - if (appliedPushRev < pushRev) { +export function assertAppliedPushCursor( + appliedPushCursor: ChangeCursor, + pushCursor: ChangeCursor, +): void { + if (compareChangeCursors(appliedPushCursor, pushCursor) < 0) { throw new Error( - `cross-side invariant violated: appliedPushRev (${appliedPushRev}) < pushRev (${pushRev})`, + `cross-side invariant violated: appliedPushCursor (${JSON.stringify( + appliedPushCursor, + )}) < pushCursor (${JSON.stringify(pushCursor)})`, ); } } diff --git a/packages/dofs/src/sync/paths.ts b/packages/dofs/src/sync/paths.ts index c49722b7..d68a1e14 100644 --- a/packages/dofs/src/sync/paths.ts +++ b/packages/dofs/src/sync/paths.ts @@ -24,3 +24,23 @@ export function pathOf(db: Database, inode: number): string | null { } return null; } + +// Every path that currently names `inode`. A file may carry several +// hardlink names; pathOf collapses them to one arbitrary name, which +// is wrong for the change stream — every name has to reach the wire so +// the receiver materialises each. Directories cannot be hardlinked, so +// each parent walk is unambiguous. +export function pathsOf(db: Database, inode: number): string[] { + if (inode === ROOT_INODE) return ["/"]; + const dirents = db.all<{ parent_inode: number; name: string }>( + "SELECT parent_inode, name FROM vfs_dirents WHERE child_inode = ?", + inode, + ); + const paths: string[] = []; + for (const { parent_inode, name } of dirents) { + const parent = pathOf(db, parent_inode); + if (parent === null) continue; + paths.push(parent === "/" ? `/${name}` : `${parent}/${name}`); + } + return paths; +} diff --git a/packages/dofs/src/sync/watermarks.test.ts b/packages/dofs/src/sync/watermarks.test.ts index e6c380c2..2cb14636 100644 --- a/packages/dofs/src/sync/watermarks.test.ts +++ b/packages/dofs/src/sync/watermarks.test.ts @@ -2,7 +2,14 @@ import { describe, expect, it } from "vitest"; import { withDB } from "../fs/with-db.js"; import { writeFile } from "../fs/writeFile.js"; -import { currentRev, readWatermark, writeWatermark } from "./watermarks.js"; +import { + compareChangeCursors, + currentRev, + readFetchCursor, + readWatermark, + writeFetchCursor, + writeWatermark, +} from "./watermarks.js"; describe("watermarks", () => { it("readWatermark returns 0 for a fresh DB", async () => { @@ -22,6 +29,87 @@ describe("watermarks", () => { }); }); + it("persists the fetch cursor rev and path separately", async () => { + await withDB(async (db) => { + expect(readFetchCursor(db)).toEqual({ rev: 0, path: null }); + writeFetchCursor(db, { rev: 12, path: "/dir/file.txt" }); + expect(readWatermark(db, "fetchRev")).toBe(12); + expect(readFetchCursor(db)).toEqual({ rev: 12, path: "/dir/file.txt" }); + writeFetchCursor(db, { rev: 13, path: null }); + expect(readWatermark(db, "fetchRev")).toBe(13); + expect(readFetchCursor(db)).toEqual({ rev: 13, path: null }); + }); + }); + + it("clears a stale fetch cursor path when writing fetchRev directly", async () => { + await withDB(async (db) => { + writeFetchCursor(db, { rev: 12, path: "/z.txt" }); + + writeWatermark(db, "fetchRev", 13); + + expect(readFetchCursor(db)).toEqual({ rev: 13, path: null }); + }); + }); + + it("does not persist an intermediate full-rev cursor when a partial cursor write fails", async () => { + await withDB(async (db) => { + writeFetchCursor(db, { rev: 12, path: null }); + + const originalRun = db.run.bind(db); + db.run = ((query: string, ...bindings: unknown[]) => { + if (query.includes("_vfs_fetch_cursor")) { + throw new Error("forced cursor path failure"); + } + return originalRun(query, ...bindings); + }) as typeof db.run; + + expect(() => writeFetchCursor(db, { rev: 13, path: "/partial.txt" })).toThrow( + "forced cursor path failure", + ); + expect(readFetchCursor(db)).toEqual({ rev: 12, path: null }); + }); + }); + + it("does not persist an intermediate cursor when a direct fetchRev write fails", async () => { + await withDB(async (db) => { + writeFetchCursor(db, { rev: 12, path: "/old.txt" }); + + const originalRun = db.run.bind(db); + db.run = ((query: string, ...bindings: unknown[]) => { + if (query.includes("_vfs_fetch_cursor")) { + throw new Error("forced cursor path clear failure"); + } + return originalRun(query, ...bindings); + }) as typeof db.run; + + expect(() => writeWatermark(db, "fetchRev", 13)).toThrow("forced cursor path clear failure"); + expect(readFetchCursor(db)).toEqual({ rev: 12, path: "/old.txt" }); + }); + }); + + it("returns a fresh start cursor for a zero fetchRev", async () => { + await withDB(async (db) => { + const cursor = readFetchCursor(db); + cursor.rev = 99; + cursor.path = "/mutated.txt"; + + expect(readFetchCursor(db)).toEqual({ rev: 0, path: null }); + }); + }); + + it("orders partial cursors before full same-rev cursors", () => { + expect(compareChangeCursors({ rev: 0, path: null }, { rev: 0, path: null })).toBe(0); + expect(compareChangeCursors({ rev: 12, path: null }, { rev: 12, path: "/partial.txt" })).toBe( + 1, + ); + expect(compareChangeCursors({ rev: 12, path: "/partial.txt" }, { rev: 12, path: null })).toBe( + -1, + ); + expect(compareChangeCursors({ rev: 13, path: "/partial.txt" }, { rev: 12, path: null })).toBe( + 1, + ); + }); + it("watermarks advance monotonically (the caller enforces this)", async () => { await withDB(async (db) => { writeWatermark(db, "pushRev", 5); diff --git a/packages/dofs/src/sync/watermarks.ts b/packages/dofs/src/sync/watermarks.ts index 96858775..e93f44b6 100644 --- a/packages/dofs/src/sync/watermarks.ts +++ b/packages/dofs/src/sync/watermarks.ts @@ -1,8 +1,8 @@ import type { Database } from "../storage.js"; -// Watermarks owned by the DO. Keyed by (k, backend) so a single -// workspace can host more than one backend and each keeps its own -// sync cursors. The container's appliedPushRev lives in-memory on +// Watermarks owned by the local database. Keyed by (k, backend) so a +// single workspace can host more than one backend and each keeps its +// own sync cursors. The container's appliedPushRev lives in-memory on // the container side; we don't store it here. // // pushRev — last DO-side rev successfully pushed to the backend. @@ -22,6 +22,13 @@ export type WatermarkKey = "pushRev" | "fetchRev"; export const DEFAULT_BACKEND_ID = "default"; +// Cursor into the remote change stream. `path: null` means `rev` +// is fully drained and the next fetch resumes strictly after that +// rev. A string path means resume inside the same rev after that +// path. The empty string is a real path value, not a sentinel, and +// must not be used to mean "start of rev". +export type ChangeCursor = { rev: number; path: string | null }; + export function readWatermark( db: Database, key: WatermarkKey, @@ -32,7 +39,7 @@ export function readWatermark( ); } -export function writeWatermark( +function writeWatermarkValue( db: Database, key: WatermarkKey, value: number, @@ -47,10 +54,71 @@ export function writeWatermark( ); } +function writeFetchCursorPath( + db: Database, + path: string | null, + backend: string = DEFAULT_BACKEND_ID, +): void { + db.run( + "INSERT INTO _vfs_fetch_cursor (k, backend, path) VALUES (?, ?, ?) " + + "ON CONFLICT(k, backend) DO UPDATE SET path = excluded.path", + "fetch", + backend, + path, + ); +} + +export function writeWatermark( + db: Database, + key: WatermarkKey, + value: number, + backend: string = DEFAULT_BACKEND_ID, +): void { + if (key !== "fetchRev") { + writeWatermarkValue(db, key, value, backend); + return; + } + + db.transactionSync(() => { + writeWatermarkValue(db, key, value, backend); + writeFetchCursorPath(db, null, backend); + }); +} + +export function readFetchCursor(db: Database, backend: string = DEFAULT_BACKEND_ID): ChangeCursor { + const rev = readWatermark(db, "fetchRev", backend); + if (rev === 0) return { rev: 0, path: null }; + const path = db.scalar( + "SELECT path FROM _vfs_fetch_cursor WHERE k = ? AND backend = ?", + "fetch", + backend, + ); + return { rev, path: path ?? null }; +} + +export function writeFetchCursor( + db: Database, + cursor: ChangeCursor, + backend: string = DEFAULT_BACKEND_ID, +): void { + db.transactionSync(() => { + writeWatermarkValue(db, "fetchRev", cursor.rev, backend); + writeFetchCursorPath(db, cursor.path, backend); + }); +} + +export function compareChangeCursors(a: ChangeCursor, b: ChangeCursor): number { + if (a.rev !== b.rev) return a.rev - b.rev; + if (a.path === b.path) return 0; + if (a.path === null) return 1; + if (b.path === null) return -1; + return a.path < b.path ? -1 : 1; +} + // The latest rev stamped on any DO-side mutation. coalesceChanges // reads this implicitly via vfs_nodes.rev; the sync layer exposes it -// to callers that want to record "what cursor should I pass back as -// sinceRev next time". +// to callers that want to record the rev component of their next +// cursor. export function currentRev(db: Database): number { return db.scalar("SELECT v FROM vfs_meta WHERE k = 'rev'") ?? 0; } diff --git a/packages/rpc/src/interface.ts b/packages/rpc/src/interface.ts index 9d9b4dee..a618b509 100644 --- a/packages/rpc/src/interface.ts +++ b/packages/rpc/src/interface.ts @@ -17,39 +17,37 @@ // WorkspaceRPC, so the wire stub exposes one stable surface while // the two halves stay internally separable. -import type { ChangeEntry } from "@cloudflare/dofs"; +import type { ChangeCursor, ChangeEntry } from "@cloudflare/dofs"; export interface SyncRPC { // DO → container. Stream a coalesced batch of changes. Bytes are // not inline: the DO sends ChangeEntry records with chunk hashes, // the container calls back via hasObjects / asks for the missing // bytes through pushObjects. `senderRev` is the sender's - // currentRev at the moment it captured the batch — the - // receiver advances its fetchRev to this value after the apply - // settles, and echoes it back as `appliedPushRev` so the sender - // can assert applied ≥ pushed on every response. + // currentRev at the moment it captured the batch. The receiver + // advances its fetch cursor to this completed rev after the apply + // settles, and echoes that cursor back as `appliedPushCursor` so + // the sender can assert applied covers pushed on every response. push(input: { senderRev: number; changes: ReadableStream }): Promise<{ rev: number; - appliedPushRev: number; + appliedPushCursor: ChangeCursor; }>; - // Container ← DO. Stream every ChangeEntry with rev > sinceRev, - // alongside two scalars used to drive the pull: + // Container ← DO. Stream every ChangeEntry after the supplied cursor, + // alongside cursors used to drive the pull: // - // currentRev — the receiver's currentRev captured at the - // start of the stream. The puller advances - // fetchRev no further than this on any pull - // so the watermark stays consistent with what - // the stream actually carried. - // appliedPushRev — the largest senderRev the receiver has - // fully applied. The puller asserts - // appliedPushRev >= local.pushRev before - // draining, mirroring the same check on push. + // currentCursor — the receiver's currentRev captured at the + // start of the stream, with path=null to mark + // the whole rev complete after a clean drain. + // appliedPushCursor — the receiver's cursor for sender changes + // it has applied. The puller asserts this + // covers local pushRev before draining, + // mirroring the same check on push. // // Per-file entries carry (hash, size) chunk lists; no bytes inline. - fetchChanges(input: { sinceRev?: number; ignore?: string[] }): Promise<{ - currentRev: number; - appliedPushRev: number; + fetchChanges(input: { after?: ChangeCursor; ignore?: string[] }): Promise<{ + currentCursor: ChangeCursor; + appliedPushCursor: ChangeCursor; stream: ReadableStream; }>; @@ -60,11 +58,12 @@ export interface SyncRPC { // // currentRev — latest rev stamped on any local mutation. // pushRev — highest rev already shipped to the upstream. - // fetchRev — highest upstream rev applied locally. + // fetchCursor — upstream fetch cursor, including same-rev path + // progress. // - // pushRev / fetchRev only move when the receiver is acting as + // pushRev / fetchCursor only move when the receiver is acting as // a sync peer. Otherwise they sit at 0. - watermarks(): Promise<{ currentRev: number; pushRev: number; fetchRev: number }>; + watermarks(): Promise<{ currentRev: number; pushRev: number; fetchCursor: ChangeCursor }>; // Materialise the receiver's view of a single path as a // ChangeEntry. Returns null when the path doesn't exist and diff --git a/packages/rpc/src/server.ts b/packages/rpc/src/server.ts index d7f12ac8..9deaa222 100644 --- a/packages/rpc/src/server.ts +++ b/packages/rpc/src/server.ts @@ -6,16 +6,20 @@ import { applyChangesSync, + type ChangeCursor, type ChangeEntry, coalesceChanges, + compareChangeCursors, currentRev, type Database, DEFAULT_IGNORE, fetchObjects, hasObjects, materialiseChange, + readFetchCursor, readWatermark, stageBlob, + writeFetchCursor, } from "@cloudflare/dofs"; import { newWebSocketRpcSession, nodeHttpBatchRpcResponse, RpcTarget } from "capnweb"; @@ -93,7 +97,7 @@ class SyncRPCServer extends RpcTarget implements SyncRPC { async push(input: { senderRev: number; changes: ReadableStream; - }): Promise<{ rev: number; appliedPushRev: number }> { + }): Promise<{ rev: number; appliedPushCursor: ChangeCursor }> { const entries: ChangeEntry[] = []; const reader = input.changes.getReader(); try { @@ -106,7 +110,7 @@ class SyncRPCServer extends RpcTarget implements SyncRPC { reader.releaseLock(); } // senderRev > 0 — the caller is a sync peer with its - // own rev space; advance fetchRev to that point and let + // own rev space; advance the fetch cursor to that point and let // loopback suppression silence the outbound push so we // don't ping-pong the same entries back. // @@ -125,8 +129,13 @@ class SyncRPCServer extends RpcTarget implements SyncRPC { this.db.transactionSync(() => { applyChangesSync(this.db, entries, new Map(), { source: isPeer ? "upstream" : "local", - ...(isPeer ? { advanceFetchRev: input.senderRev } : {}), }); + if (isPeer) { + const nextCursor = { rev: input.senderRev, path: null }; + if (compareChangeCursors(nextCursor, readFetchCursor(this.db)) > 0) { + writeFetchCursor(this.db, nextCursor); + } + } }); if (this.options.afterApply !== undefined && entries.length > 0) { try { @@ -140,13 +149,13 @@ class SyncRPCServer extends RpcTarget implements SyncRPC { } return { rev: currentRev(this.db), - appliedPushRev: input.senderRev, + appliedPushCursor: { rev: input.senderRev, path: null }, }; } - async fetchChanges(input: { sinceRev?: number; ignore?: string[] }): Promise<{ - currentRev: number; - appliedPushRev: number; + async fetchChanges(input: { after?: ChangeCursor; ignore?: string[] }): Promise<{ + currentCursor: ChangeCursor; + appliedPushCursor: ChangeCursor; stream: ReadableStream; }> { if (this.options.beforeFetch !== undefined) { @@ -159,16 +168,17 @@ class SyncRPCServer extends RpcTarget implements SyncRPC { console.warn("[SyncRPCServer] beforeFetch hook failed:", err); } } - const sinceRev = input.sinceRev ?? 0; + const after = input.after ?? { rev: 0, path: null }; const ignore = input.ignore ?? (this.options.ignore.length > 0 ? this.options.ignore : DEFAULT_IGNORE); - // appliedPushRev == fetchRev on the receiver: every senderRev > 0 - // push advances fetchRev to senderRev on apply, so fetchRev is - // the largest senderRev the receiver has fully applied. + const snapshotRev = currentRev(this.db); + const currentCursor = { rev: snapshotRev, path: null }; return { - currentRev: currentRev(this.db), - appliedPushRev: readWatermark(this.db, "fetchRev"), - stream: iterableToReadableStream(coalesceChanges(this.db, sinceRev, { ignore })), + currentCursor, + appliedPushCursor: readFetchCursor(this.db), + stream: iterableToReadableStream( + coalesceChanges(this.db, after, { ignore, through: currentCursor }), + ), }; } @@ -176,11 +186,11 @@ class SyncRPCServer extends RpcTarget implements SyncRPC { return materialiseChange(this.db, path); } - async watermarks(): Promise<{ currentRev: number; pushRev: number; fetchRev: number }> { + async watermarks(): Promise<{ currentRev: number; pushRev: number; fetchCursor: ChangeCursor }> { return { currentRev: currentRev(this.db), pushRev: readWatermark(this.db, "pushRev"), - fetchRev: readWatermark(this.db, "fetchRev"), + fetchCursor: readFetchCursor(this.db), }; } diff --git a/packages/rpc/src/sync-driver.test.ts b/packages/rpc/src/sync-driver.test.ts index 472bc55f..a59bbb68 100644 --- a/packages/rpc/src/sync-driver.test.ts +++ b/packages/rpc/src/sync-driver.test.ts @@ -3,9 +3,11 @@ import { currentRev, Database, initializeSchema, + readFetchCursor, readWatermark, SQLiteWorkspaceProvider, stageBlob, + writeFetchCursor, writeWatermark, } from "@cloudflare/dofs"; import { SQLiteTestStorage } from "@cloudflare/dofs/testing"; @@ -33,6 +35,55 @@ function fileEntries(db: Database): string[] { .map((r) => r.name); } +async function drainStream(stream: ReadableStream): Promise { + const out: T[] = []; + const reader = stream.getReader(); + try { + while (true) { + const { value, done } = await reader.read(); + if (done) break; + out.push(value); + } + } finally { + reader.releaseLock(); + } + return out; +} + +// Wrap a SyncRPC so the fetchChanges result carries a tracked +// [Symbol.dispose]. pullOnce owns that envelope and must dispose it on +// every exit, including the throwing paths. Options inject the two +// failure modes: a lying appliedPushCursor (trips the cross-side +// invariant before the stream is read) and an empty hasObjects (forces +// applyChanges to throw on a missing object mid-stream). +function trackFetchDisposal( + rpc: SyncRPC, + opts: { lowerCursor?: boolean; failHasObjects?: boolean } = {}, +): { rpc: SyncRPC; disposeCount: () => number } { + let disposeCount = 0; + const wrapped = new Proxy(rpc as object, { + get(target, prop, receiver) { + if (prop === "fetchChanges") { + return async (...args: Parameters) => { + const real = await Reflect.get(target, prop, receiver).call(target, ...args); + return { + ...real, + ...(opts.lowerCursor ? { appliedPushCursor: { rev: 0, path: null } } : {}), + [Symbol.dispose]() { + disposeCount += 1; + }, + }; + }; + } + if (prop === "hasObjects" && opts.failHasObjects) { + return async () => []; + } + return Reflect.get(target, prop, receiver); + }, + }) as SyncRPC; + return { rpc: wrapped, disposeCount: () => disposeCount }; +} + describe("sync driver — pullOnce", () => { it("pulls a single entry from upstream", async () => { const a = makePeer(); @@ -335,6 +386,130 @@ describe("SyncRPC server — beforeFetch hook", () => { }); }); +describe("SyncRPC server — fetchChanges snapshots", () => { + it("bounds the returned stream to the advertised currentCursor", async () => { + const upstream = makePeer(); + try { + const provider = new SQLiteWorkspaceProvider(upstream.db, { now: () => 1 }); + provider.writeFileSync("/before.txt", "before"); + + const originalAll = upstream.db.all.bind(upstream.db); + let rewrote = false; + upstream.db.all = ((query: string, ...bindings: unknown[]) => { + if (!rewrote && query.startsWith("SELECT inode, rev FROM vfs_nodes WHERE rev > ?")) { + rewrote = true; + provider.writeFileSync("/after.txt", "after"); + } + return originalAll(query, ...bindings); + }) as typeof upstream.db.all; + + const result = await upstream.rpc.fetchChanges({ after: { rev: 0, path: null } }); + const advertisedCursor = result.currentCursor; + + const entries = await drainStream(result.stream); + expect(rewrote).toBe(true); + expect(result.currentCursor).toEqual(advertisedCursor); + expect(entries.map((entry) => entry.path)).toContain("/before.txt"); + expect(entries.map((entry) => entry.path)).not.toContain("/after.txt"); + expect(advertisedCursor.rev).toBeLessThan(currentRev(upstream.db)); + } finally { + upstream.close(); + } + }); + + it("pullOnce persists only the advertised snapshot cursor", async () => { + const upstream = makePeer(); + const downstream = makePeer(); + try { + const providerA = new SQLiteWorkspaceProvider(upstream.db, { now: () => 1 }); + providerA.writeFileSync("/before.txt", "before"); + + let advertisedCursor: { rev: number; path: string | null } | undefined; + const wrapped = new Proxy(upstream.rpc as object, { + get(target, prop, receiver) { + if (prop === "fetchChanges") { + return async (...args: Parameters) => { + const result = await Reflect.get(target, prop, receiver).call(target, ...args); + advertisedCursor = result.currentCursor; + providerA.writeFileSync("/after.txt", "after"); + return result; + }; + } + return Reflect.get(target, prop, receiver); + }, + }) as typeof upstream.rpc; + + await pullOnce(downstream.db, wrapped); + + expect(advertisedCursor).toBeDefined(); + expect(fileEntries(downstream.db)).toContain("before.txt"); + expect(fileEntries(downstream.db)).not.toContain("after.txt"); + expect(readFetchCursor(downstream.db)).toEqual(advertisedCursor); + expect(readFetchCursor(downstream.db).rev).toBeLessThan(currentRev(upstream.db)); + } finally { + upstream.close(); + downstream.close(); + } + }); +}); + +describe("sync driver — pullOnce envelope disposal", () => { + it("disposes the fetchChanges envelope when the cross-side invariant trips", async () => { + const remote = makePeer(); + try { + const local = new Database(new SQLiteTestStorage()); + initializeSchema(local, () => 1000); + // Local claims to have pushed rev 42; the wrapper echoes back a + // rev-0 cursor, so assertAppliedPushCursor throws before the + // stream is ever read. + writeWatermark(local, "pushRev", 42); + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/seed.txt", "x"); + + const tracked = trackFetchDisposal(remote.rpc, { lowerCursor: true }); + await expect(pullOnce(local, tracked.rpc)).rejects.toThrow(/cross-side invariant violated/i); + expect(tracked.disposeCount()).toBe(1); + } finally { + remote.close(); + } + }); + + it("disposes the fetchChanges envelope when applyChanges throws mid-stream", async () => { + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/seed.txt", "needs-bytes"); + + // hasObjects lies (returns nothing), so pullOnce stages no blob + // bytes and applyChanges throws on the missing object. + const tracked = trackFetchDisposal(remote.rpc, { failHasObjects: true }); + await expect(pullOnce(local.db, tracked.rpc)).rejects.toThrow(/missing object/i); + expect(tracked.disposeCount()).toBe(1); + } finally { + remote.close(); + local.close(); + } + }); + + it("disposes the fetchChanges envelope on the happy path", async () => { + const remote = makePeer(); + const local = makePeer(); + try { + const providerR = new SQLiteWorkspaceProvider(remote.db, { now: () => 1 }); + providerR.writeFileSync("/seed.txt", "x"); + + const tracked = trackFetchDisposal(remote.rpc); + await pullOnce(local.db, tracked.rpc); + expect(tracked.disposeCount()).toBe(1); + expect(fileEntries(local.db)).toContain("seed.txt"); + } finally { + remote.close(); + local.close(); + } + }); +}); + describe("sync driver — bidirectional convergence", () => { it("two peers writing in parallel converge after a few ticks", async () => { const a = makePeer(); @@ -406,21 +581,21 @@ describe("sync driver — bidirectional convergence", () => { }); describe("sync driver — cross-side invariant", () => { - it("pushOnce throws when the remote echoes back a lower appliedPushRev", async () => { + it("pushOnce throws when the remote echoes back a lower appliedPushCursor", async () => { const a = makePeer(); const b = makePeer(); try { const providerA = new SQLiteWorkspaceProvider(a.db, { now: () => 1 }); providerA.writeFileSync("/x.txt", "x"); - // Wrap B's rpc to lie about appliedPushRev. Simulates a + // Wrap B's rpc to lie about appliedPushCursor. Simulates a // regression in the suppress-dirty-tracking apply path. const lyingRpc = new Proxy(b.rpc as object, { get(target, prop, receiver) { if (prop === "push") { return async (input: { senderRev: number; changes: ReadableStream }) => { const real = await Reflect.get(target, prop, receiver).call(target, input); - return { ...real, appliedPushRev: 0 }; + return { ...real, appliedPushCursor: { rev: 0, path: null } }; }; } return Reflect.get(target, prop, receiver); @@ -434,10 +609,10 @@ describe("sync driver — cross-side invariant", () => { } }); - it("pullOnce throws when fetchChanges echoes back a lower appliedPushRev", async () => { + it("pullOnce throws when fetchChanges echoes back a lower appliedPushCursor", async () => { // Symmetric to the push case. fetchChanges returns the remote's - // appliedPushRev alongside the entry stream; the DO asserts - // appliedPushRev >= pushRev before draining, so a regression in + // appliedPushCursor alongside the entry stream; the DO asserts + // appliedPushCursor covers pushRev before draining, so a regression in // the remote's apply path that loses applied state trips the // invariant on the next pull instead of corrupting fetchRev. const remote = makePeer(); @@ -455,9 +630,9 @@ describe("sync driver — cross-side invariant", () => { const lyingRpc = new Proxy(remote.rpc as object, { get(target, prop, receiver) { if (prop === "fetchChanges") { - return (input: { sinceRev?: number; ignore?: string[] }) => { + return (input: Parameters[0]) => { const real = Reflect.get(target, prop, receiver).call(target, input); - return { ...real, appliedPushRev: 0 }; + return { ...real, appliedPushCursor: { rev: 0, path: null } }; }; } return Reflect.get(target, prop, receiver); @@ -469,6 +644,50 @@ describe("sync driver — cross-side invariant", () => { remote.close(); } }); + + it("fetchChanges reports applied push progress as a cursor", async () => { + const remote = makePeer(); + try { + writeFetchCursor(remote.db, { rev: 42, path: "/partial.txt" }); + + const result = await remote.rpc.fetchChanges({ after: { rev: 0, path: null } }); + + expect(result.appliedPushCursor).toEqual({ rev: 42, path: "/partial.txt" }); + await result.stream.cancel(); + } finally { + remote.close(); + } + }); + + it("pullOnce rejects when the remote only partially applied local pushRev", async () => { + const remote = makePeer(); + try { + const local = new Database(new SQLiteTestStorage()); + initializeSchema(local, () => 1000); + writeWatermark(local, "pushRev", 42); + writeFetchCursor(remote.db, { rev: 42, path: "/partial.txt" }); + + await expect(pullOnce(local, remote.rpc)).rejects.toThrow(/cross-side invariant violated/i); + } finally { + remote.close(); + } + }); + + it("pullOnce accepts a partial remote cursor after local pushRev", async () => { + const remote = makePeer(); + try { + const local = new Database(new SQLiteTestStorage()); + initializeSchema(local, () => 1000); + writeWatermark(local, "pushRev", 42); + writeFetchCursor(remote.db, { rev: 43, path: "/partial.txt" }); + + const result = await pullOnce(local, remote.rpc); + + expect(result).toEqual({ applied: 0, skipped: [] }); + } finally { + remote.close(); + } + }); }); describe("sync driver — streaming pullOnce", () => { @@ -562,6 +781,114 @@ describe("sync driver — streaming pullOnce", () => { b.close(); } }); + + it("does not let an older overlapping pull move the fetch cursor backward", async () => { + const upstream = makePeer(); + const downstream = makePeer(); + try { + const providerA = new SQLiteWorkspaceProvider(upstream.db, { now: () => 1 }); + const providerB = new SQLiteWorkspaceProvider(downstream.db, { now: () => 1 }); + for (let i = 0; i < 300; i++) { + providerA.writeFileSync(`/f${i.toString().padStart(3, "0")}.txt`, `old ${i}`); + } + + let releaseOldHasObjects: (() => void) | undefined; + let oldHasObjectsEntered: (() => void) | undefined; + const oldHasObjectsStarted = new Promise((resolve) => { + oldHasObjectsEntered = resolve; + }); + const oldHasObjectsGate = new Promise((resolve) => { + releaseOldHasObjects = resolve; + }); + const sampledCursorBeforeSecondOldBatch: Array<{ rev: number; path: string | null }> = []; + let oldHasObjectsCalls = 0; + const olderRpc = new Proxy(upstream.rpc as object, { + get(target, prop, receiver) { + if (prop === "hasObjects") { + return async (hashes: Uint8Array[]) => { + oldHasObjectsCalls++; + if (oldHasObjectsCalls === 1) { + oldHasObjectsEntered?.(); + await oldHasObjectsGate; + } else { + sampledCursorBeforeSecondOldBatch.push(readFetchCursor(downstream.db)); + } + return Reflect.get(target, prop, receiver).call(target, hashes); + }; + } + return Reflect.get(target, prop, receiver); + }, + }) as typeof upstream.rpc; + + const olderPull = pullOnce(downstream.db, olderRpc); + await oldHasObjectsStarted; + + providerA.writeFileSync("/newer.txt", "newer"); + const newestCursor = { rev: currentRev(upstream.db), path: null }; + const newerPull = await pullOnce(downstream.db, upstream.rpc); + expect(newerPull.applied).toBe(301); + expect(readFetchCursor(downstream.db)).toEqual(newestCursor); + + releaseOldHasObjects?.(); + const olderPullResult = await olderPull; + + expect(olderPullResult.applied).toBe(0); + expect(providerB.readFileSync("/newer.txt", "utf8")).toBe("newer"); + expect(sampledCursorBeforeSecondOldBatch).toEqual([newestCursor]); + expect(readFetchCursor(downstream.db)).toEqual(newestCursor); + } finally { + upstream.close(); + downstream.close(); + } + }); + + it("resumes inside one large same-rev rename after a failed batch", async () => { + const a = makePeer(); + const b = makePeer(); + try { + const providerA = new SQLiteWorkspaceProvider(a.db, { now: () => 1 }); + const providerB = new SQLiteWorkspaceProvider(b.db, { now: () => 1 }); + providerA.mkdirSync("/src", {}); + for (let i = 0; i < 300; i++) { + providerA.writeFileSync(`/src/f${i.toString().padStart(3, "0")}.txt`, `content ${i}`); + } + + await pullOnce(b.db, a.rpc); + expect(providerB.readFileSync("/src/f299.txt", "utf8")).toBe("content 299"); + + providerA.renameSync("/src", "/dst"); + const renameRev = currentRev(a.db); + let hasObjectsCalls = 0; + const flaky = new Proxy(a.rpc as object, { + get(target, prop, receiver) { + if (prop === "hasObjects") { + return async (hashes: Uint8Array[]) => { + hasObjectsCalls++; + if (hasObjectsCalls === 2) { + throw new Error("injected pull failure after first batch"); + } + return Reflect.get(target, prop, receiver).call(target, hashes); + }; + } + return Reflect.get(target, prop, receiver); + }, + }) as typeof a.rpc; + + await expect(pullOnce(b.db, flaky)).rejects.toThrow(/injected pull failure/); + expect(readFetchCursor(b.db)).toMatchObject({ rev: renameRev }); + expect(readFetchCursor(b.db).path).not.toBeNull(); + + const retried = await pullOnce(b.db, a.rpc); + expect(retried.applied).toBeGreaterThan(0); + expect(providerB.existsSync("/src")).toBe(false); + expect(providerB.readFileSync("/dst/f000.txt", "utf8")).toBe("content 0"); + expect(providerB.readFileSync("/dst/f299.txt", "utf8")).toBe("content 299"); + expect(readFetchCursor(b.db)).toEqual({ rev: renameRev, path: null }); + } finally { + a.close(); + b.close(); + } + }); }); describe("sync driver — push atomicity", () => { @@ -626,7 +953,7 @@ describe("sync driver — reconcileWatermarks", () => { // container lifetimes; the container's watermarks are // process-lifetime in today's wsd. After a container restart with // no new DO-side writes, pushOnce's localRev <= sincePush - // early-return means the assertAppliedPushRev check never runs and + // early-return means the assertAppliedPushCursor check never runs and // the container's empty FUSE mount is invisible to the DO. The // reconcile catches the mismatch by comparing the local cursors // against the remote's watermarks(), resetting fetchRev to 0 when @@ -654,7 +981,7 @@ describe("sync driver — reconcileWatermarks", () => { const remote = makePeer(); try { // Local pushRev = 17, but the remote is fresh: its pushRev, - // which doubles as appliedPushRev on the wire, is 0. + // which is echoed as appliedPushCursor on the wire, is 0/null. const local = new Database(new SQLiteTestStorage()); initializeSchema(local, () => 1000); writeWatermark(local, "fetchRev", 0); diff --git a/packages/rpc/src/sync-driver.ts b/packages/rpc/src/sync-driver.ts index 355d5f29..148c0d66 100644 --- a/packages/rpc/src/sync-driver.ts +++ b/packages/rpc/src/sync-driver.ts @@ -12,14 +12,18 @@ import { type ApplyResult, applyChanges, - assertAppliedPushRev, + assertAppliedPushCursor, + type ChangeCursor, type ChangeEntry, coalesceChanges, + compareChangeCursors, currentRev, type Database, + readFetchCursor, readWatermark, type SkippedEntry, stageBlob, + writeFetchCursor, writeWatermark, } from "@cloudflare/dofs"; @@ -31,38 +35,6 @@ function hex(bytes: Uint8Array): string { return s; } -// Pipe `stream` through an identity TransformStream that fires `onDone` -// exactly once when the stream finishes — clean end, cancel, or error. -// Used to release a capnweb result envelope as soon as the stream it -// carried is drained, without having to keep the envelope reference -// alive across the consume site. `onDone` errors are swallowed: the -// stream's content is the load-bearing thing, not the cleanup. -function disposeOnDone(stream: ReadableStream, onDone: () => void): ReadableStream { - let fired = false; - const fire = () => { - if (fired) return; - fired = true; - try { - onDone(); - } catch { - // ignore — disposer errors are not actionable here - } - }; - return stream.pipeThrough( - new TransformStream({ - transform(chunk, controller) { - controller.enqueue(chunk); - }, - flush() { - fire(); - }, - cancel() { - fire(); - }, - }), - ); -} - // Best-effort dispose of a capnweb result envelope. Real envelopes // expose [Symbol.dispose]; the test fakes return plain objects, so // the symbol may be absent. @@ -78,7 +50,7 @@ function maybeDispose(value: unknown): void { const PULL_BATCH_SIZE = 256; // Pull every entry the remote has produced since the last successful -// pull, apply locally, advance fetchRev. Returns an `ApplyResult` +// pull, apply locally, advance the fetch cursor. Returns an `ApplyResult` // folded across every batch so callers see both the applied count // (decide whether to tick again) and any entries skipped because // they targeted a read-only mount root (surface to the user). @@ -88,140 +60,159 @@ const PULL_BATCH_SIZE = 256; // dedup work without per-chunk round-trips. // // The entry stream is drained in batches of PULL_BATCH_SIZE so peak -// memory stays bounded on a large tree. fetchRev still advances -// once at the end of the whole stream — per-batch advance would -// require the wire to carry a rev cursor per entry. Crash safety is -// idempotent re-apply: the receiver's alreadyApplied() check inside -// applyChanges drops a re-fetched batch on the floor. +// memory stays bounded on a large tree. The durable fetch cursor +// advances after each committed batch to the last streamed entry's +// (rev, path), so a retry can resume inside a single large rev. The +// cursor is read and written per backend so concurrent backends keep +// independent resume points. export async function pullOnce( db: Database, remote: SyncRPC, backend?: string, ): Promise { - const sinceRev = readWatermark(db, "fetchRev", backend); + const after = readFetchCursor(db, backend); const localPushRev = readWatermark(db, "pushRev", backend); - // fetchChanges hands back the remote's currentRev (cursor we - // advance fetchRev to), its appliedPushRev (cross-side invariant - // check on the pull path), and the entry stream itself. One - // round-trip instead of the previous currentRev() + fetchChanges() - // pair. + // fetchChanges hands back the remote's currentCursor (cursor we + // advance to after a clean drain), its appliedPushCursor (cross-side + // invariant check on the pull path), and the entry stream itself. + // One round-trip instead of the previous currentRev() + + // fetchChanges() pair. // The fetchChanges return is a capnweb result envelope wrapping a // stream stub; without explicit disposal it sits in the exports // table until the session ends. We can't bind it to `using` // because the stream inside outlives this scope (we hand it off // to the reader loop below), so wrap the stream in a transform // that disposes the envelope when the stream finishes draining. - const fetchResult = await remote.fetchChanges({ sinceRev }); - const { currentRev: remoteRev, appliedPushRev } = fetchResult; - // Run the cross-side invariant check before touching the stream. - // Symmetric to the push response check: the remote must have - // applied at least everything we claimed to push. A drop here - // means apply lost state on the receiver; tear down and rebuild - // rather than corrupt watermarks. - assertAppliedPushRev(appliedPushRev, localPushRev); - const stream = disposeOnDone(fetchResult.stream, () => maybeDispose(fetchResult)); - if (remoteRev <= sinceRev) { - // Drain the (empty) stream so the remote's iterator is - // released; cancel is the right surface for that. - await stream.cancel().catch(() => {}); - return { applied: 0, skipped: [] }; - } - - const reader = stream.getReader(); - let totalApplied = 0; - const totalSkipped: SkippedEntry[] = []; - let streamDone = false; + // pullOnce owns the fetchChanges result envelope: it wraps a stream + // stub that sits in the exports table until disposed. The stream is + // fully consumed within this call (drained, cancelled, or abandoned + // on a throw), so a try/finally disposing the envelope covers every + // exit — the clean drain, the early-complete return, the cross-side + // invariant trip, and any throw inside the batch loop. Disposing the + // envelope tears down the contained stream stub, releasing the + // remote iterator. + const fetchResult = await remote.fetchChanges({ after }); try { - while (!streamDone) { - // Read up to PULL_BATCH_SIZE entries before processing the batch. - const batch: ChangeEntry[] = []; - const wantedHashes: Uint8Array[] = []; - const seenHash = new Set(); - let batchMaxRev = 0; - while (batch.length < PULL_BATCH_SIZE) { - const { value, done } = await reader.read(); - if (done) { - streamDone = true; - break; - } - batch.push(value); - if (value.rev > batchMaxRev) batchMaxRev = value.rev; - if (value.kind === "file") { - for (const c of value.chunks) { - const k = hex(c.hash); - if (!seenHash.has(k)) { - seenHash.add(k); - wantedHashes.push(c.hash); + const { currentCursor, appliedPushCursor } = fetchResult; + // Cross-side invariant, symmetric to the push response check: the + // remote must have applied at least everything we claimed to push. + // A gap means apply lost state on the receiver; tear down and + // rebuild rather than corrupt watermarks. Runs inside the try so a + // trip still disposes the envelope. + assertAppliedPushCursor(appliedPushCursor, { rev: localPushRev, path: null }); + if (cursorComplete(after, currentCursor)) { + return { applied: 0, skipped: [] }; + } + + const reader = fetchResult.stream.getReader(); + let totalApplied = 0; + const totalSkipped: SkippedEntry[] = []; + let streamDone = false; + try { + while (!streamDone) { + // Read up to PULL_BATCH_SIZE entries before processing the batch. + const batch: ChangeEntry[] = []; + const wantedHashes: Uint8Array[] = []; + const seenHash = new Set(); + while (batch.length < PULL_BATCH_SIZE) { + const { value, done } = await reader.read(); + if (done) { + streamDone = true; + break; + } + batch.push(value); + if (value.kind === "file") { + for (const c of value.chunks) { + const k = hex(c.hash); + if (!seenHash.has(k)) { + seenHash.add(k); + wantedHashes.push(c.hash); + } } } } - } - if (batch.length === 0) break; + if (batch.length === 0) break; - // Probe + fetch missing chunk bytes for just this batch. Bytes - // the receiver already holds (or the remote doesn't have) are - // skipped, so the per-batch network cost is bounded. - if (wantedHashes.length > 0) { - const haveSubset = await remote.hasObjects(wantedHashes); - const remoteHasLocally = new Set(); - for (const h of haveSubset) remoteHasLocally.add(hex(h)); - const missing = wantedHashes.filter((h) => { - const k = hex(h); - if (!remoteHasLocally.has(k)) return false; - const row = db.one<{ hash: Uint8Array }>("SELECT hash FROM vfs_blobs WHERE hash = ?", h); - return row === undefined; - }); - if (missing.length > 0) { - // Bare ReadableStream return — no envelope to dispose, - // capnweb releases the stream stub when the stream itself - // closes. The reader-loop below drains to completion. - const bytesStream = await remote.fetchObjects(missing); - const bytesReader = bytesStream.getReader(); - try { - while (true) { - const { value, done } = await bytesReader.read(); - if (done) break; - stageBlob(db, value.hash, value.bytes, Date.now()); + // Probe + fetch missing chunk bytes for just this batch. Bytes + // the receiver already holds (or the remote doesn't have) are + // skipped, so the per-batch network cost is bounded. + if (wantedHashes.length > 0) { + const haveSubset = await remote.hasObjects(wantedHashes); + const remoteHasLocally = new Set(); + for (const h of haveSubset) remoteHasLocally.add(hex(h)); + const missing = wantedHashes.filter((h) => { + const k = hex(h); + if (!remoteHasLocally.has(k)) return false; + const row = db.one<{ hash: Uint8Array }>( + "SELECT hash FROM vfs_blobs WHERE hash = ?", + h, + ); + return row === undefined; + }); + if (missing.length > 0) { + // Bare ReadableStream return — no envelope to dispose, + // capnweb releases the stream stub when the stream itself + // closes. The reader-loop below drains to completion. + const bytesStream = await remote.fetchObjects(missing); + const bytesReader = bytesStream.getReader(); + try { + while (true) { + const { value, done } = await bytesReader.read(); + if (done) break; + stageBlob(db, value.hash, value.bytes, Date.now()); + } + } finally { + bytesReader.releaseLock(); } - } finally { - bytesReader.releaseLock(); } } - } - // Advance fetchRev per committed batch. Because coalesceChanges - // emits in ascending rev order, every entry already applied - // (this batch + all previous ones) has rev <= batchMaxRev, so - // it's safe to checkpoint here. A crash after this commit - // means the next pull resumes from batchMaxRev and re-fetches - // only entries from later batches — bounded by PULL_BATCH_SIZE - // instead of the whole stream. - const batchResult = await applyChanges(db, batch, new Map(), { - source: "upstream", - advanceFetchRev: batchMaxRev, - backend, - }); - totalApplied += batchResult.applied; - if (batchResult.skipped.length > 0) { - for (const s of batchResult.skipped) totalSkipped.push(s); + const batchResult = await applyChanges(db, batch, new Map(), { + source: "upstream", + backend, + }); + const last = batch[batch.length - 1]; + // Cursor advancement intentionally happens after applyChanges() + // and is not atomic with it. A crash between apply and this + // checkpoint re-fetches the batch; upstream apply is idempotent + // because alreadyApplied() drops entries whose live state + // already matches. + writeFetchCursorIfAhead(db, { rev: last.rev, path: last.path }, backend); + totalApplied += batchResult.applied; + if (batchResult.skipped.length > 0) { + for (const s of batchResult.skipped) totalSkipped.push(s); + } } + } finally { + reader.releaseLock(); } + + // Mark the receiver's captured current rev as fully drained. This + // preserves the ignored/no-op window behavior: if the stream had no + // entries because every path was filtered out, the next pull still + // starts after that rev. + // fetchChanges() is snapshot-bounded by currentCursor, so this + // final drain marker cannot skip beyond any per-batch cursor written + // above. + writeFetchCursorIfAhead(db, currentCursor, backend); + return { applied: totalApplied, skipped: totalSkipped }; } finally { - reader.releaseLock(); + maybeDispose(fetchResult); } +} + +function cursorComplete(after: ChangeCursor, current: ChangeCursor): boolean { + if (current.rev < after.rev) return true; + return current.rev === after.rev && after.path === null; +} - // Nudge fetchRev to the remote's currentRev captured at the start. - // The per-batch advances above bring fetchRev up to the max rev - // any entry carried, but if the rev window contained entries that - // were all filtered out (e.g. ignored paths), the stream is empty - // and the per-batch path never fires. Advancing to remoteRev here - // is still safe because we captured it before draining the stream - // and never regress. - const current = readWatermark(db, "fetchRev", backend); - if (remoteRev > current) { - writeWatermark(db, "fetchRev", remoteRev, backend); +function writeFetchCursorIfAhead(db: Database, cursor: ChangeCursor, backend?: string): void { + // Overlapping pulls can complete out of order, so checkpoint writes + // compare against the latest persisted cursor instead of the value + // observed when this pull started. + if (compareChangeCursors(cursor, readFetchCursor(db, backend)) > 0) { + writeFetchCursor(db, cursor, backend); } - return { applied: totalApplied, skipped: totalSkipped }; } // Push every entry the local store has produced since the last @@ -235,7 +226,7 @@ export async function pushOnce(db: Database, remote: SyncRPC, backend?: string): const entries: ChangeEntry[] = []; const wantedHashes: Uint8Array[] = []; const seenHash = new Set(); - for await (const e of coalesceChanges(db, sincePush)) { + for await (const e of coalesceChanges(db, { rev: sincePush, path: null })) { entries.push(e); if (e.kind === "file") { for (const c of e.chunks) { @@ -289,11 +280,11 @@ export async function pushOnce(db: Database, remote: SyncRPC, backend?: string): }); const response = await remote.push({ senderRev: localRev, changes: entryStream }); - // Cross-side invariant: the receiver must echo back at least - // the rev we just claimed to push. A drift means the apply - // path lost data, or a stale receiver is serving an old - // snapshot. Tear down loudly rather than corrupt watermarks. - assertAppliedPushRev(response.appliedPushRev, localRev); + // Cross-side invariant: the receiver must echo back a cursor that + // covers the rev we just claimed to push. A drift means the apply + // path lost data, or a stale receiver is serving an old snapshot. + // Tear down loudly rather than corrupt watermarks. + assertAppliedPushCursor(response.appliedPushCursor, { rev: localRev, path: null }); // Local pushRev advances to the rev we observed at the start of // this round. Anything written after that gets caught next tick. @@ -337,17 +328,17 @@ export async function reconcileWatermarks( backend?: string, ): Promise<{ fetchRevReset: boolean; pushRevReset: boolean }> { const remoteWatermarks = await remote.watermarks(); - const localFetchRev = readWatermark(db, "fetchRev", backend); + const localFetchCursor = readFetchCursor(db, backend); const localPushRev = readWatermark(db, "pushRev", backend); let fetchRevReset = false; let pushRevReset = false; - // If the remote's currentRev is below our fetchRev, the remote's + // If the remote's currentRev is below our fetch cursor rev, the remote's // log is shorter than we remember — it lost state since we last // pulled. Re-baseline from 0. - if (remoteWatermarks.currentRev < localFetchRev) { - writeWatermark(db, "fetchRev", 0, backend); + if (remoteWatermarks.currentRev < localFetchCursor.rev) { + writeFetchCursor(db, { rev: 0, path: null }, backend); fetchRevReset = true; } diff --git a/packages/rpc/tests/wire.test.ts b/packages/rpc/tests/wire.test.ts index 6db78517..72f7919c 100644 --- a/packages/rpc/tests/wire.test.ts +++ b/packages/rpc/tests/wire.test.ts @@ -2,13 +2,16 @@ import { createServer, type Server } from "node:http"; import type { AddressInfo } from "node:net"; import { + assertAppliedPushCursor, type ChangeEntry, coalesceChanges, Database, fetchObjects, initializeSchema, ROOT_INODE, + readFetchCursor, SQLiteWorkspaceProvider, + writeFetchCursor, } from "@cloudflare/dofs"; import { SQLiteTestStorage } from "@cloudflare/dofs/testing"; import { afterEach, describe, expect, it } from "vitest"; @@ -64,7 +67,7 @@ describe("SyncRPC over a real WebSocket", () => { const client = createSyncClient({ url: harness.url }); try { - const { stream } = await client.fetchChanges({ sinceRev: 0, ignore: [] }); + const { stream } = await client.fetchChanges({ after: { rev: 0, path: null }, ignore: [] }); const entries: ChangeEntry[] = []; const reader = stream.getReader(); try { @@ -157,7 +160,7 @@ describe("SyncRPC push convergence", () => { const entries: ChangeEntry[] = []; const hashes: Uint8Array[] = []; const seenHash = new Set(); - for await (const e of coalesceChanges(senderDb, 0)) { + for await (const e of coalesceChanges(senderDb, { rev: 0, path: null })) { entries.push(e); if (e.kind === "file") { for (const c of e.chunks) { @@ -361,8 +364,6 @@ describe("WireError propagation", () => { }); }); -import { assertAppliedPushRev } from "@cloudflare/dofs"; - describe("cross-side invariant", () => { let harness: Harness | undefined; afterEach(async () => { @@ -370,7 +371,7 @@ describe("cross-side invariant", () => { harness = undefined; }); - it("push returns a {rev, appliedPushRev} that satisfies appliedPushRev >= 0", async () => { + it("push returns an appliedPushCursor that covers senderRev", async () => { harness = await startHarness(); const client = createSyncClient({ url: harness.url }); try { @@ -380,12 +381,13 @@ describe("cross-side invariant", () => { }, }); const result = await client.push({ senderRev: 0, changes: empty }); - expect(result.appliedPushRev).toBeGreaterThanOrEqual(0); - // The DO would pass result.appliedPushRev as `applied` - // and its own pushRev counter as `pushed`. With the - // container reporting >= 0 and the DO holding 0 at this - // point, the invariant holds. - expect(() => assertAppliedPushRev(result.appliedPushRev, 0)).not.toThrow(); + expect(result.appliedPushCursor).toEqual({ rev: 0, path: null }); + // The durable object would pass result.appliedPushCursor as + // `applied` and its own push cursor as `pushed`. With both at + // zero, the invariant holds. + expect(() => + assertAppliedPushCursor(result.appliedPushCursor, { rev: 0, path: null }), + ).not.toThrow(); } finally { await client.close(); } @@ -496,12 +498,12 @@ describe("push semantics — external vs sync peer", () => { // outbound sync loop (a wsd with UPSTREAM_URL set) would // see the new entry on the next tick. expect(readWatermark(harness.db, "pushRev")).toBe(0); - // fetchRev was NOT advanced either — the sender has + // The fetch cursor was NOT advanced either — the sender has // no rev space. expect(readWatermark(harness.db, "fetchRev")).toBe(0); // The entry is in the coalesce stream. const drained: { path: string }[] = []; - for await (const e of coalesceChanges(harness.db, 0)) drained.push(e); + for await (const e of coalesceChanges(harness.db, { rev: 0, path: null })) drained.push(e); expect(drained.some((e) => e.path === "/external.txt")).toBe(true); } finally { await client.close(); @@ -551,8 +553,37 @@ describe("push semantics — external vs sync peer", () => { // entries back to the peer. const cur = currentRev(harness.db); expect(readWatermark(harness.db, "pushRev")).toBe(cur); - // fetchRev was advanced to senderRev. - expect(readWatermark(harness.db, "fetchRev")).toBe(42); + // The fetch cursor was advanced to senderRev. + expect(readFetchCursor(harness.db)).toEqual({ rev: 42, path: null }); + } finally { + await client.close(); + } + }); + + it("push with senderRev>0 advances the fetch cursor monotonically", async () => { + harness = await startHarness(); + const client = createSyncClient({ url: harness.url }); + try { + writeFetchCursor(harness.db, { rev: 10, path: "/partial.txt" }); + await client.push({ + senderRev: 10, + changes: new ReadableStream({ + start(c) { + c.close(); + }, + }), + }); + expect(readFetchCursor(harness.db)).toEqual({ rev: 10, path: null }); + + await client.push({ + senderRev: 3, + changes: new ReadableStream({ + start(c) { + c.close(); + }, + }), + }); + expect(readFetchCursor(harness.db)).toEqual({ rev: 10, path: null }); } finally { await client.close(); } diff --git a/packages/workspace/src/mounts/index.test.ts b/packages/workspace/src/mounts/index.test.ts index 738b255b..2a637930 100644 --- a/packages/workspace/src/mounts/index.test.ts +++ b/packages/workspace/src/mounts/index.test.ts @@ -485,12 +485,12 @@ describe("mount indexer", () => { } finally { reader.releaseLock(); } - return { rev: 0, appliedPushRev: input.senderRev }; + return { rev: 0, appliedPushCursor: { rev: input.senderRev, path: null } }; }, async fetchChanges() { return { - currentRev: 0, - appliedPushRev: 0, + currentCursor: { rev: 0, path: null }, + appliedPushCursor: { rev: 0, path: null }, stream: new ReadableStream({ start(c) { c.close(); @@ -512,7 +512,7 @@ describe("mount indexer", () => { }); }, async watermarks() { - return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + return { currentRev: 0, pushRev: 0, fetchCursor: { rev: 0, path: null } }; }, async pushObjects(objects) { const reader = objects.getReader(); diff --git a/packages/workspace/src/observe-integration.test.ts b/packages/workspace/src/observe-integration.test.ts index ebc6b0ed..62295c38 100644 --- a/packages/workspace/src/observe-integration.test.ts +++ b/packages/workspace/src/observe-integration.test.ts @@ -33,12 +33,12 @@ function fakeSync(): SyncRPC { } finally { reader.releaseLock(); } - return { rev: 0, appliedPushRev: input.senderRev }; + return { rev: 0, appliedPushCursor: { rev: input.senderRev, path: null } }; }, async fetchChanges() { return { - currentRev: 0, - appliedPushRev: 0, + currentCursor: { rev: 0, path: null }, + appliedPushCursor: { rev: 0, path: null }, stream: new ReadableStream({ start(c) { c.close(); @@ -60,7 +60,7 @@ function fakeSync(): SyncRPC { }); }, async watermarks() { - return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + return { currentRev: 0, pushRev: 0, fetchCursor: { rev: 0, path: null } }; }, async pushObjects(objects) { const reader = objects.getReader(); diff --git a/packages/workspace/src/shell.test.ts b/packages/workspace/src/shell.test.ts index 5b6d522d..96a9de88 100644 --- a/packages/workspace/src/shell.test.ts +++ b/packages/workspace/src/shell.test.ts @@ -113,7 +113,7 @@ function fakeRpc(options: FakeRpcOptions = {}): FakeRpc { return null; }, async watermarks() { - return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + return { currentRev: 0, pushRev: 0, fetchCursor: { rev: 0, path: null } }; }, async hasObjects() { return []; diff --git a/packages/workspace/src/stub.test.ts b/packages/workspace/src/stub.test.ts index 423182f7..09272690 100644 --- a/packages/workspace/src/stub.test.ts +++ b/packages/workspace/src/stub.test.ts @@ -44,12 +44,12 @@ function composite( function fakeSync(): import("@cloudflare/workspace-rpc").SyncRPC { return { async push() { - return { rev: 0, appliedPushRev: 0 }; + return { rev: 0, appliedPushCursor: { rev: 0, path: null } }; }, async fetchChanges() { return { - currentRev: 0, - appliedPushRev: 0, + currentCursor: { rev: 0, path: null }, + appliedPushCursor: { rev: 0, path: null }, stream: new ReadableStream({ start(c) { c.close(); @@ -61,7 +61,7 @@ function fakeSync(): import("@cloudflare/workspace-rpc").SyncRPC { return null; }, async watermarks() { - return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + return { currentRev: 0, pushRev: 0, fetchCursor: { rev: 0, path: null } }; }, async hasObjects() { return []; diff --git a/packages/workspace/src/workspace.test.ts b/packages/workspace/src/workspace.test.ts index 0e849088..98f03886 100644 --- a/packages/workspace/src/workspace.test.ts +++ b/packages/workspace/src/workspace.test.ts @@ -58,12 +58,12 @@ function fakeRpc(): import("@cloudflare/workspace-rpc").SyncRPC { } finally { reader.releaseLock(); } - return { rev: 0, appliedPushRev: input.senderRev }; + return { rev: 0, appliedPushCursor: { rev: input.senderRev, path: null } }; }, async fetchChanges() { return { - currentRev: 0, - appliedPushRev: 0, + currentCursor: { rev: 0, path: null }, + appliedPushCursor: { rev: 0, path: null }, stream: new ReadableStream({ start(c) { c.close(); @@ -99,7 +99,7 @@ function fakeRpc(): import("@cloudflare/workspace-rpc").SyncRPC { }); }, async watermarks() { - return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + return { currentRev: 0, pushRev: 0, fetchCursor: { rev: 0, path: null } }; }, async pushObjects(objects) { const reader = objects.getReader(); @@ -512,7 +512,7 @@ describe("Workspace backend selection", () => { ...fakeRpc(), async watermarks() { watermarksCalls++; - return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + return { currentRev: 0, pushRev: 0, fetchCursor: { rev: 0, path: null } }; }, }; const storage = makeStorage(); @@ -690,7 +690,7 @@ describe("Workspace mutation serialization", () => { reader.releaseLock(); } inFlight.push--; - return { rev: 0, appliedPushRev: input.senderRev }; + return { rev: 0, appliedPushCursor: { rev: input.senderRev, path: null } }; }, }; @@ -733,7 +733,7 @@ describe("Workspace mutation serialization", () => { } finally { reader.releaseLock(); } - return { rev: 0, appliedPushRev: input.senderRev }; + return { rev: 0, appliedPushCursor: { rev: input.senderRev, path: null } }; }, }; const ws = new Workspace({ storage: makeStorage(), backends: [makeBackend("fake", rpc)] }); diff --git a/packages/workspace/tests/stub-soak-worker.ts b/packages/workspace/tests/stub-soak-worker.ts index f3a43cc5..8c7f7a87 100644 --- a/packages/workspace/tests/stub-soak-worker.ts +++ b/packages/workspace/tests/stub-soak-worker.ts @@ -38,12 +38,12 @@ export interface Env { function fakeBackend(): WorkspaceBackend { const sync: SyncRPC = { async push() { - return { rev: 0, appliedPushRev: 0 }; + return { rev: 0, appliedPushCursor: { rev: 0, path: null } }; }, async fetchChanges() { return { - currentRev: 0, - appliedPushRev: 0, + currentCursor: { rev: 0, path: null }, + appliedPushCursor: { rev: 0, path: null }, stream: new ReadableStream({ start(c) { c.close(); @@ -55,7 +55,7 @@ function fakeBackend(): WorkspaceBackend { return null; }, async watermarks() { - return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + return { currentRev: 0, pushRev: 0, fetchCursor: { rev: 0, path: null } }; }, async hasObjects() { return []; diff --git a/packages/wsd/src/cli/wsd.test.ts b/packages/wsd/src/cli/wsd.test.ts index 64933b6d..9cf132b5 100644 --- a/packages/wsd/src/cli/wsd.test.ts +++ b/packages/wsd/src/cli/wsd.test.ts @@ -116,7 +116,10 @@ test("/ws serves a capnweb WorkspaceRPC session", async (_ctx) => { // hasObjects against a fresh DB returns the empty subset. expect(await client.sync.hasObjects([])).toEqual([]); // fetchChanges streams zero entries against a fresh DB. - const { stream } = await client.sync.fetchChanges({ sinceRev: 0, ignore: [] }); + const { stream } = await client.sync.fetchChanges({ + after: { rev: 0, path: null }, + ignore: [], + }); const reader = stream.getReader(); const entries = []; while (true) { diff --git a/packages/wsd/src/fuse/vfs.test.ts b/packages/wsd/src/fuse/vfs.test.ts index 7fd35061..c6111f96 100644 --- a/packages/wsd/src/fuse/vfs.test.ts +++ b/packages/wsd/src/fuse/vfs.test.ts @@ -30,8 +30,8 @@ test("createNodeVirtualFileSystem pulls initial state from an upstream SyncRPC", async fetchChanges() { fetchChangesCalls++; return { - currentRev: 1, - appliedPushRev: 0, + currentCursor: { rev: 1, path: null }, + appliedPushCursor: { rev: 0, path: null }, stream: new ReadableStream({ start(c) { c.enqueue({ @@ -62,7 +62,7 @@ test("createNodeVirtualFileSystem pulls initial state from an upstream SyncRPC", }); }, async push() { - return { rev: 0, appliedPushRev: 0 }; + return { rev: 0, appliedPushCursor: { rev: 0, path: null } }; }, async pushObjects() {}, }; From dea9c3cb22b02cb19f9bc9b50631a84685818242 Mon Sep 17 00:00:00 2001 From: Nathan Disidore Date: Sun, 7 Jun 2026 12:15:24 -0500 Subject: [PATCH 3/3] dofs: Make fetch progress cursor-only Fetch progress is now a rev/path cursor, but the exported watermark helper still accepted scalar fetchRev writes. That left two public write paths for one logical cursor. Restrict the scalar watermark API to pushRev, move fetch progress callers to readFetchCursor and writeFetchCursor, and normalize equal-rev partial cursors when a peer push proves the full rev was applied. --- docs/11_lifecycle.md | 10 +++--- packages/dofs/src/sync/watermarks.test.ts | 38 ++------------------- packages/dofs/src/sync/watermarks.ts | 38 ++++++++++++--------- packages/rpc/src/sync-driver.bench.ts | 7 ++-- packages/rpc/src/sync-driver.test.ts | 41 +++++++++++++++++++---- packages/rpc/tests/wire.test.ts | 6 ++-- packages/workspace/src/workspace.test.ts | 9 +++-- 7 files changed, 78 insertions(+), 71 deletions(-) diff --git a/docs/11_lifecycle.md b/docs/11_lifecycle.md index d8781b48..55e0157c 100644 --- a/docs/11_lifecycle.md +++ b/docs/11_lifecycle.md @@ -335,10 +335,12 @@ Two things have to change for capnweb + hibernation to work: wake as a fresh session. The peer must retry any in-flight RPC. This is the same semantics as a transport reset, which the protocol already handles via the rev cursors. - - **Sync streams: nothing to store.** `pushRev` and the durable - fetch cursor are already written to SQLite alongside the data - they describe. On wake, the next `pushOnce` / `pullOnce` reads - them from durable storage and resumes. No attachment write is + - **Sync streams: nothing to store.** `pushRev` is written to + SQLite with the pushed data it describes. The durable fetch + cursor is written after each committed pull batch, not in the + same transaction as the data apply. On wake, the next `pushOnce` + / `pullOnce` reads the durable counters and resumes; any overlap + is dropped by the idempotent apply path. No attachment write is required. - **Exec streams: store `{ [id]: seq }` per in-flight exec.** The `WorkspaceShell` driver inside the DO is the only place diff --git a/packages/dofs/src/sync/watermarks.test.ts b/packages/dofs/src/sync/watermarks.test.ts index 2cb14636..7b9213b0 100644 --- a/packages/dofs/src/sync/watermarks.test.ts +++ b/packages/dofs/src/sync/watermarks.test.ts @@ -15,7 +15,6 @@ describe("watermarks", () => { it("readWatermark returns 0 for a fresh DB", async () => { await withDB(async (db) => { expect(readWatermark(db, "pushRev")).toBe(0); - expect(readWatermark(db, "fetchRev")).toBe(0); }); }); @@ -23,9 +22,6 @@ describe("watermarks", () => { await withDB(async (db) => { writeWatermark(db, "pushRev", 42); expect(readWatermark(db, "pushRev")).toBe(42); - expect(readWatermark(db, "fetchRev")).toBe(0); - writeWatermark(db, "fetchRev", 7); - expect(readWatermark(db, "fetchRev")).toBe(7); }); }); @@ -33,20 +29,8 @@ describe("watermarks", () => { await withDB(async (db) => { expect(readFetchCursor(db)).toEqual({ rev: 0, path: null }); writeFetchCursor(db, { rev: 12, path: "/dir/file.txt" }); - expect(readWatermark(db, "fetchRev")).toBe(12); expect(readFetchCursor(db)).toEqual({ rev: 12, path: "/dir/file.txt" }); writeFetchCursor(db, { rev: 13, path: null }); - expect(readWatermark(db, "fetchRev")).toBe(13); - expect(readFetchCursor(db)).toEqual({ rev: 13, path: null }); - }); - }); - - it("clears a stale fetch cursor path when writing fetchRev directly", async () => { - await withDB(async (db) => { - writeFetchCursor(db, { rev: 12, path: "/z.txt" }); - - writeWatermark(db, "fetchRev", 13); - expect(readFetchCursor(db)).toEqual({ rev: 13, path: null }); }); }); @@ -70,23 +54,6 @@ describe("watermarks", () => { }); }); - it("does not persist an intermediate cursor when a direct fetchRev write fails", async () => { - await withDB(async (db) => { - writeFetchCursor(db, { rev: 12, path: "/old.txt" }); - - const originalRun = db.run.bind(db); - db.run = ((query: string, ...bindings: unknown[]) => { - if (query.includes("_vfs_fetch_cursor")) { - throw new Error("forced cursor path clear failure"); - } - return originalRun(query, ...bindings); - }) as typeof db.run; - - expect(() => writeWatermark(db, "fetchRev", 13)).toThrow("forced cursor path clear failure"); - expect(readFetchCursor(db)).toEqual({ rev: 12, path: "/old.txt" }); - }); - }); - it("returns a fresh start cursor for a zero fetchRev", async () => { await withDB(async (db) => { const cursor = readFetchCursor(db); @@ -136,9 +103,8 @@ describe("watermarks", () => { }); it("rejects unknown watermark keys at the type level via the helper signature", () => { - // Compile-time only: writeWatermark only accepts the union - // "pushRev" | "fetchRev". This test is a placeholder that - // documents the contract; the type system catches misuse. + // Compile-time only: writeWatermark only accepts "pushRev". + // Fetch progress must go through readFetchCursor/writeFetchCursor. expect(true).toBe(true); }); diff --git a/packages/dofs/src/sync/watermarks.ts b/packages/dofs/src/sync/watermarks.ts index e93f44b6..084c3c68 100644 --- a/packages/dofs/src/sync/watermarks.ts +++ b/packages/dofs/src/sync/watermarks.ts @@ -5,20 +5,24 @@ import type { Database } from "../storage.js"; // own sync cursors. The container's appliedPushRev lives in-memory on // the container side; we don't store it here. // -// pushRev — last DO-side rev successfully pushed to the backend. -// fetchRev — last backend-side rev the DO has fetched and applied. +// pushRev — last DO-side rev successfully pushed to the backend. // -// initializeSchema() seeds both at 0 in _vfs_watermark for the +// initializeSchema() seeds pushRev at 0 in _vfs_watermark for the // default backend. The schema table is the durability surface; // readers and writers always go through this module so the SQL // stays in one place. // // `backend` defaults to DEFAULT_BACKEND_ID so older callers that // only ran one backend (or ran the package against a schema before -// per-backend keying landed) keep working unchanged. The v2 → v3 +// per-backend keying landed) keep working unchanged. The v3 → v4 // schema migration backfills the column on existing rows with the // same default. -export type WatermarkKey = "pushRev" | "fetchRev"; +// +// Fetch progress is a `{ rev, path }` cursor. Its rev component is +// still stored in _vfs_watermark for schema compatibility, but callers +// must use readFetchCursor() / writeFetchCursor() so rev and path stay +// consistent. +export type WatermarkKey = "pushRev"; export const DEFAULT_BACKEND_ID = "default"; @@ -39,9 +43,19 @@ export function readWatermark( ); } +function readFetchRev(db: Database, backend: string = DEFAULT_BACKEND_ID): number { + return ( + db.scalar( + "SELECT v FROM _vfs_watermark WHERE k = ? AND backend = ?", + "fetchRev", + backend, + ) ?? 0 + ); +} + function writeWatermarkValue( db: Database, - key: WatermarkKey, + key: WatermarkKey | "fetchRev", value: number, backend: string = DEFAULT_BACKEND_ID, ): void { @@ -74,19 +88,11 @@ export function writeWatermark( value: number, backend: string = DEFAULT_BACKEND_ID, ): void { - if (key !== "fetchRev") { - writeWatermarkValue(db, key, value, backend); - return; - } - - db.transactionSync(() => { - writeWatermarkValue(db, key, value, backend); - writeFetchCursorPath(db, null, backend); - }); + writeWatermarkValue(db, key, value, backend); } export function readFetchCursor(db: Database, backend: string = DEFAULT_BACKEND_ID): ChangeCursor { - const rev = readWatermark(db, "fetchRev", backend); + const rev = readFetchRev(db, backend); if (rev === 0) return { rev: 0, path: null }; const path = db.scalar( "SELECT path FROM _vfs_fetch_cursor WHERE k = ? AND backend = ?", diff --git a/packages/rpc/src/sync-driver.bench.ts b/packages/rpc/src/sync-driver.bench.ts index 00f9ea7e..f44bd205 100644 --- a/packages/rpc/src/sync-driver.bench.ts +++ b/packages/rpc/src/sync-driver.bench.ts @@ -16,6 +16,7 @@ import { currentRev, Database, initializeSchema, + readFetchCursor, readWatermark, SQLiteWorkspaceProvider, } from "@cloudflare/dofs"; @@ -84,7 +85,7 @@ describe("sync driver — push throughput", () => { for (let i = 0; i < bytes.byteLength; i += 4096) { bytes[i] = (i * 31) & 0xff; } - provider.writeFileSync("/big.bin", bytes); + provider.writeFileSync("/big.bin", Buffer.from(bytes)); await pushOnce(a.db, b.rpc); } finally { a.close(); @@ -105,7 +106,7 @@ describe("sync driver — push throughput", () => { for (let i = 0; i < bytes.byteLength; i += 4096) { bytes[i] = (i * 31) & 0xff; } - provider.writeFileSync("/big.bin", bytes); + provider.writeFileSync("/big.bin", Buffer.from(bytes)); await pushOnce(a.db, b.rpc); } finally { a.close(); @@ -199,7 +200,7 @@ describe("sync driver — bidirectional convergence", () => { // time. Reading watermarks here adds noise we can // tolerate vs. running a no-op closure for the // baseline. Sanity assert outside the iteration body: - if (readWatermark(b.db, "fetchRev") <= 0) throw new Error("pull didn't advance"); + if (readFetchCursor(b.db).rev <= 0) throw new Error("pull didn't advance"); if (currentRev(b.db) <= 0) throw new Error("apply didn't bump rev"); } finally { a.close(); diff --git a/packages/rpc/src/sync-driver.test.ts b/packages/rpc/src/sync-driver.test.ts index a59bbb68..8b8e8e29 100644 --- a/packages/rpc/src/sync-driver.test.ts +++ b/packages/rpc/src/sync-driver.test.ts @@ -756,7 +756,7 @@ describe("sync driver — streaming pullOnce", () => { get(target, prop, receiver) { if (prop === "hasObjects") { return async (hashes: Uint8Array[]) => { - sampledRevs.push(readWatermark(b.db, "fetchRev")); + sampledRevs.push(readFetchCursor(b.db).rev); return Reflect.get(target, prop, receiver).call(target, hashes); }; } @@ -775,7 +775,7 @@ describe("sync driver — streaming pullOnce", () => { expect(sampledRevs[i]).toBeGreaterThan(sampledRevs[i - 1]); } // End state still matches the remote. - expect(readWatermark(b.db, "fetchRev")).toBe(remoteFinalRev); + expect(readFetchCursor(b.db).rev).toBe(remoteFinalRev); } finally { a.close(); b.close(); @@ -889,6 +889,33 @@ describe("sync driver — streaming pullOnce", () => { b.close(); } }); + + it("lets a peer push supersede a partial pull cursor", async () => { + const a = makePeer(); + const b = makePeer(); + try { + const providerA = new SQLiteWorkspaceProvider(a.db, { now: () => 1 }); + const providerB = new SQLiteWorkspaceProvider(b.db, { now: () => 1 }); + providerA.writeFileSync("/before-stale-path.txt", "pushed"); + const pushedRev = currentRev(a.db); + + writeFetchCursor(b.db, { rev: pushedRev, path: "/zzzz" }); + + expect(await pushOnce(a.db, b.rpc)).toBeGreaterThan(0); + expect(readFetchCursor(b.db)).toEqual({ rev: pushedRev, path: null }); + expect(providerB.readFileSync("/before-stale-path.txt", "utf8")).toBe("pushed"); + + providerA.writeFileSync("/after-push.txt", "pulled later"); + const pulled = await pullOnce(b.db, a.rpc); + + expect(pulled.applied).toBe(1); + expect(providerB.readFileSync("/after-push.txt", "utf8")).toBe("pulled later"); + expect(readFetchCursor(b.db)).toEqual({ rev: currentRev(a.db), path: null }); + } finally { + a.close(); + b.close(); + } + }); }); describe("sync driver — push atomicity", () => { @@ -966,11 +993,11 @@ describe("sync driver — reconcileWatermarks", () => { // (currentRev = 1 from initializeSchema seeding the root). const local = new Database(new SQLiteTestStorage()); initializeSchema(local, () => 1000); - writeWatermark(local, "fetchRev", 42); + writeFetchCursor(local, { rev: 42, path: null }); writeWatermark(local, "pushRev", 0); await reconcileWatermarks(local, remote.rpc); - expect(readWatermark(local, "fetchRev")).toBe(0); + expect(readFetchCursor(local)).toEqual({ rev: 0, path: null }); expect(readWatermark(local, "pushRev")).toBe(0); } finally { remote.close(); @@ -984,7 +1011,7 @@ describe("sync driver — reconcileWatermarks", () => { // which is echoed as appliedPushCursor on the wire, is 0/null. const local = new Database(new SQLiteTestStorage()); initializeSchema(local, () => 1000); - writeWatermark(local, "fetchRev", 0); + writeFetchCursor(local, { rev: 0, path: null }); writeWatermark(local, "pushRev", 17); await reconcileWatermarks(local, remote.rpc); @@ -1004,11 +1031,11 @@ describe("sync driver — reconcileWatermarks", () => { const local = new Database(new SQLiteTestStorage()); initializeSchema(local, () => 1000); const remoteCurrent = currentRev(remote.db); - writeWatermark(local, "fetchRev", remoteCurrent); + writeFetchCursor(local, { rev: remoteCurrent, path: null }); writeWatermark(local, "pushRev", 0); await reconcileWatermarks(local, remote.rpc); - expect(readWatermark(local, "fetchRev")).toBe(remoteCurrent); + expect(readFetchCursor(local)).toEqual({ rev: remoteCurrent, path: null }); expect(readWatermark(local, "pushRev")).toBe(0); } finally { remote.close(); diff --git a/packages/rpc/tests/wire.test.ts b/packages/rpc/tests/wire.test.ts index 72f7919c..0ebd5837 100644 --- a/packages/rpc/tests/wire.test.ts +++ b/packages/rpc/tests/wire.test.ts @@ -500,7 +500,7 @@ describe("push semantics — external vs sync peer", () => { expect(readWatermark(harness.db, "pushRev")).toBe(0); // The fetch cursor was NOT advanced either — the sender has // no rev space. - expect(readWatermark(harness.db, "fetchRev")).toBe(0); + expect(readFetchCursor(harness.db)).toEqual({ rev: 0, path: null }); // The entry is in the coalesce stream. const drained: { path: string }[] = []; for await (const e of coalesceChanges(harness.db, { rev: 0, path: null })) drained.push(e); @@ -512,7 +512,9 @@ describe("push semantics — external vs sync peer", () => { it("push with senderRev>0 (sync peer) advances pushRev to silence loopback", async () => { harness = await startHarness(); - const { currentRev, readWatermark, writeWatermark } = await import("@cloudflare/dofs"); + const { currentRev, readFetchCursor, readWatermark, writeWatermark } = await import( + "@cloudflare/dofs" + ); const client = createSyncClient({ url: harness.url }); try { // Seed pushRev at the current point so the F1 guard diff --git a/packages/workspace/src/workspace.test.ts b/packages/workspace/src/workspace.test.ts index 98f03886..e21dc7dd 100644 --- a/packages/workspace/src/workspace.test.ts +++ b/packages/workspace/src/workspace.test.ts @@ -517,14 +517,17 @@ describe("Workspace backend selection", () => { }; const storage = makeStorage(); const ws = new Workspace({ storage, backends: [makeBackend("only", sync)] }); - const { writeWatermark, readWatermark } = await import("@cloudflare/dofs"); + // Pre-seed local watermarks for the "only" backend. + const { readFetchCursor, readWatermark, writeFetchCursor, writeWatermark } = await import( + "@cloudflare/dofs" + ); writeWatermark(ws.db, "pushRev", 17, "only"); - writeWatermark(ws.db, "fetchRev", 42, "only"); + writeFetchCursor(ws.db, { rev: 42, path: null }, "only"); // ready() alone no longer dials; ready(id) forces the connect. await ws.ready("only"); expect(watermarksCalls).toBe(1); expect(readWatermark(ws.db, "pushRev", "only")).toBe(0); - expect(readWatermark(ws.db, "fetchRev", "only")).toBe(0); + expect(readFetchCursor(ws.db, "only")).toEqual({ rev: 0, path: null }); }); it("skips push/pull when the backend declares sync: 'none'", async () => {