From 7198639db430ec56f392fbc5269a904ba5edb874 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Wed, 29 Apr 2026 19:51:49 +0100 Subject: [PATCH] loop --- CLAUDE.md | 119 +++++ .../deterministic-tests/src/test-registry.ts | 14 +- ...sable-sync-mid-create-no-duplicate.test.ts | 64 +++ ...ocal-rename-survives-remote-rename.test.ts | 80 ++++ ...collides-with-pending-local-create.test.ts | 72 +++ ...rename-chain-during-pending-create.test.ts | 54 +++ .../src/sync-operations/sync-event-queue.ts | 36 ++ .../sync-client/src/sync-operations/syncer.ts | 437 ++++++++---------- frontend/test-client/src/agent/mock-client.ts | 12 +- 9 files changed, 636 insertions(+), 252 deletions(-) create mode 100644 CLAUDE.md create mode 100644 frontend/deterministic-tests/src/tests/disable-sync-mid-create-no-duplicate.test.ts create mode 100644 frontend/deterministic-tests/src/tests/local-rename-survives-remote-rename.test.ts create mode 100644 frontend/deterministic-tests/src/tests/remote-rename-collides-with-pending-local-create.test.ts create mode 100644 frontend/deterministic-tests/src/tests/rename-chain-during-pending-create.test.ts diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..b74e52bc --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,119 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Project shape + +VaultLink is a self-hosted Obsidian file-sync system. Two halves of one repo: + +- `sync-server/` — Rust (axum + sqlx/SQLite). Source of truth for vault state, broadcasts changes via WebSocket. +- `frontend/` — npm workspaces. The sync engine (`sync-client`) is consumed by an Obsidian plugin, a standalone CLI, a fuzz E2E harness, a scripted determinism harness, and a history UI. + +The HTTP/WS API types are generated from Rust (`ts-rs`) and mirrored into the TS workspaces. **Never hand-edit files in `frontend/sync-client/src/services/types/` or `frontend/history-ui/src/lib/types/`** — run `scripts/update-api-types.sh` after changing anything Serde-derived in the server. + +### Frontend workspaces + +- `sync-client` — the sync engine; published to consumers via `dist/`. All other TS workspaces depend on it via `file:../sync-client`. +- `obsidian-plugin` — Obsidian plugin built from `sync-client`. +- `local-client-cli` — same engine wrapped as a standalone CLI. +- `history-ui` — vault-history web UI. +- `test-client` — fuzz E2E harness (random ops across N processes). +- `deterministic-tests` — scripted multi-client tests with an in-memory FS, run against a real server. + +## Common commands + +Pre-push hygiene (formats, lints, runs tests, requires clean git state): + +```sh +scripts/check.sh --fix +``` + +Run the fuzz E2E (N parallel processes): + +```sh +scripts/e2e.sh 12 +# Logs land in logs/log_.log. Clean with scripts/clean-up.sh +``` + +Run deterministic tests (require a release-built server in `sync-server/target/release/sync_server` — they spawn it themselves): + +```sh +cd sync-server && cargo build --release && cd .. +cd frontend +npm run build -w sync-client -w deterministic-tests +node deterministic-tests/dist/cli.js # all +node deterministic-tests/dist/cli.js --filter=rename # subset +node deterministic-tests/dist/cli.js --filter=… -j 4 # cap parallelism +``` + +Run a single sync-client unit test by file: + +```sh +cd frontend/sync-client && npx tsx --test 'src/**/sync-event-queue.test.ts' +``` + +Server: dev runs from `sync-server/` against `config-e2e.yml`: + +```sh +cd sync-server +cargo run config-e2e.yml # dev +cargo build --release # used by both e2e harnesses +cargo test # unit + ts-rs binding export tests +``` + +Frontend dev (sync-client + obsidian-plugin watch in parallel): + +```sh +cd frontend && npm install && npm run dev +``` + +Regenerate TS bindings from Rust types (touches `frontend/{sync-client,history-ui}/src/.../types/`): + +```sh +scripts/update-api-types.sh +``` + +## SQLite / sqlx + +The server uses `sqlx::query!` macros that need a prepared `.sqlx` cache to compile offline. Touching any SQL means regenerating it: + +```sh +cd sync-server +sqlx database create --database-url sqlite://db.sqlite3 +sqlx migrate run --source src/app_state/database/migrations --database-url sqlite://db.sqlite3 +cargo sqlx prepare --workspace +``` + +New migrations: `sqlx migrate add --source src/app_state/database/migrations `. + +## Sync engine architecture + +Read `frontend/sync-client/src/sync-operations/` to follow the sync engine; the rest of `sync-client` is plumbing (filesystem ops, persistence, services, telemetry). + +**`SyncEventQueue`** (`sync-event-queue.ts`) holds two things: + +- `documents: Map` — the local "settled" view of tracked docs. +- `events: SyncEvent[]` — pending operations (creates, updates, deletes, remote changes) in FIFO drain order. + +The map is keyed by `record.path`; the invariant `documents.get(record.path) === record` is maintained by every mutation point (constructor, `setDocument`, the rename branch in `enqueue`). `setDocument` mutates the same record object in place when relocating, so callers holding a reference to the record see path changes on the next read — this is load-bearing for `Syncer`'s drain handlers, which await across HTTP roundtrips and would otherwise see a captured-string-stale path. Always read `record.path` live; only snapshot it into a local for the explicit "did the path change during my await" comparison (`pathBeforeRoundtrip` in `handleMaybeMergingResponse` / `processRemoteUpdate`). + +**`Syncer`** (`syncer.ts`) drains events one at a time. Local creates/updates/deletes round-trip to the server over HTTP; remote changes arrive over the WebSocket and are enqueued as `RemoteChange` events that the same drain processes. `handleMaybeMergingResponse` is the shared response handler for create-and-update flows. + +**Conflict-uuid paths.** When a remote create or remote-rename can't claim its server-side path locally (the slot is occupied), the local file lands at `conflict--` and `record.intendedPath` records the path the server has it at. All server-bound requests honor `intendedPath`/`event.originalPath`, so the conflict-uuid path never leaks to the server. There is no automatic unwinding — convergence at conflict points is left to manual user resolution. + +**Watermark.** `lastSeenUpdateId` uses a `MinCovered` (a contiguous-prefix tracker over a stream of integers): we only advance the published min when the next consecutive id has been processed, so out-of-order RemoteChange ids don't fool the WebSocket handshake into requesting a too-recent catch-up. + +**Server catch-up.** The server's WS handshake replays events newer than the client's `last_seen_vault_update_id` from the `latest_document_versions` view (one row per doc, the latest). On those replayed rows `is_new_file` means *new to this client* (`creation_vault_update_id > last_seen_vault_update_id`), not "this row is the doc's first version" — necessary because the catch-up only carries the latest version; if a doc was created and updated past the watermark, the client never sees its create otherwise. + +## Two complementary E2E harnesses + +- **`test-client` (fuzz):** random ops across N parallel processes for many minutes. Used by `scripts/e2e.sh`. Catches bugs nobody thought to write a test for, but reproductions are noisy. +- **`deterministic-tests`:** scripted scenarios with an in-memory FS pinned to a real server. Used to *capture* a fuzz-discovered bug as a minimal repro before fixing it. See `frontend/deterministic-tests/README.md` for the step grammar (`pause-server`, `pause-websocket`, `barrier`, `assert-consistent`, etc.). + +When a fuzz failure surfaces, the workflow is: root-cause from logs → write a deterministic test that fails on the bug → fix → confirm both the deterministic test and `e2e.sh` pass. + +## Style + +- TS: 4-space indent, no tabs, LF, prettier (`trailingComma: "none"`). YAML/MD use 2-space indent. +- Rust: `rustfmt.toml` enforces 4-space spaces, LF. +- Lint: ESLint for TS, Clippy for Rust, `cargo machete` for unused deps. All wired into `scripts/check.sh`. diff --git a/frontend/deterministic-tests/src/test-registry.ts b/frontend/deterministic-tests/src/test-registry.ts index bab7d586..2ff4fd94 100644 --- a/frontend/deterministic-tests/src/test-registry.ts +++ b/frontend/deterministic-tests/src/test-registry.ts @@ -94,6 +94,10 @@ import { localUpdateSurvivesRemoteRenameTest } from "./tests/local-update-surviv import { mergingUpdateResponseSurvivesUserRenameTest } from "./tests/merging-update-response-survives-user-rename.test"; import { conflictUuidStashClearedAfterRenameDeconflictTest } from "./tests/conflict-uuid-stash-cleared-after-rename-deconflict.test"; import { catchupCreateAndUpdateNotSkippedTest } from "./tests/catchup-create-and-update-not-skipped.test"; +import { localRenameSurvivesRemoteRenameTest } from "./tests/local-rename-survives-remote-rename.test"; +import { renameChainDuringPendingCreateTest } from "./tests/rename-chain-during-pending-create.test"; +import { disableSyncMidCreateNoDuplicateTest } from "./tests/disable-sync-mid-create-no-duplicate.test"; +import { remoteRenameCollidesWithPendingLocalCreateTest } from "./tests/remote-rename-collides-with-pending-local-create.test"; export const TESTS: Partial> = { "rename-create-conflict": renameCreateConflictTest, @@ -212,5 +216,13 @@ export const TESTS: Partial> = { "conflict-uuid-stash-cleared-after-rename-deconflict": conflictUuidStashClearedAfterRenameDeconflictTest, "catchup-create-and-update-not-skipped": - catchupCreateAndUpdateNotSkippedTest + catchupCreateAndUpdateNotSkippedTest, + "local-rename-survives-remote-rename": + localRenameSurvivesRemoteRenameTest, + "rename-chain-during-pending-create": + renameChainDuringPendingCreateTest, + "disable-sync-mid-create-no-duplicate": + disableSyncMidCreateNoDuplicateTest, + "remote-rename-collides-with-pending-local-create": + remoteRenameCollidesWithPendingLocalCreateTest }; diff --git a/frontend/deterministic-tests/src/tests/disable-sync-mid-create-no-duplicate.test.ts b/frontend/deterministic-tests/src/tests/disable-sync-mid-create-no-duplicate.test.ts new file mode 100644 index 00000000..2d255eee --- /dev/null +++ b/frontend/deterministic-tests/src/tests/disable-sync-mid-create-no-duplicate.test.ts @@ -0,0 +1,64 @@ +import type { AssertableState } from "../utils/assertable-state"; +import type { TestDefinition } from "../test-definition"; + +export const disableSyncMidCreateNoDuplicateTest: TestDefinition = { + description: + "Client 0 creates `doc.md`. While the create's HTTP roundtrip is in " + + "flight (server paused), the user disables sync. Pre-fix: " + + "`SyncClient.pause()` calls `fetchController.startReset()`, which " + + "rejects the in-flight fetch with `SyncResetError`. The server " + + "still commits the document, but the client never learns the " + + "docId. The user then renames the file offline (`doc.md` -> " + + "`renamed.md`) and re-enables sync. The offline scan sees " + + "`renamed.md` as an untracked new file (the create's record was " + + "never settled) and creates a SECOND document on the server with " + + "the same content. WS catch-up then downloads the orphaned first " + + "doc back to `doc.md`, leaving the client with two files holding " + + "identical content. Post-fix: `pause({abortInFlight: false})` " + + "lets the create's HTTP land, the docId is captured into " + + "`queue.documents`, and the offline scan recognises `renamed.md` " + + "as a rename of an already-tracked doc — only one server doc " + + "exists.", + clients: 2, + steps: [ + { type: "enable-sync", client: 0 }, + { type: "enable-sync", client: 1 }, + + // Pause the server so the upcoming create's HTTP buffers at the + // OS level. The fetch is in flight from the client's perspective + // — exactly the state where pre-fix `startReset` would discard + // its eventual response. + { type: "pause-server" }, + + { type: "create", client: 0, path: "doc.md", content: "marker\n" }, + + // Disable sync. Pre-fix: aborts the in-flight create with + // SyncResetError; the server commits but the client forgets. + // Post-fix: blocks until the in-flight HTTP lands; queue + // records the doc. + { type: "resume-server" }, + { type: "disable-sync", client: 0 }, + + // User renames the file while offline. + { type: "rename", client: 0, oldPath: "doc.md", newPath: "renamed.md" }, + + // Re-enable sync. Post-fix: offline scan sees `renamed.md` as a + // rename of the tracked doc and PUTs a rename to the server. + // Pre-fix: scan sees `renamed.md` as new (queue.documents is + // empty for it) and CREATEs a second doc; WS catch-up later + // re-downloads the orphaned first doc to `doc.md`. + { type: "enable-sync", client: 0 }, + + { type: "barrier" }, + + { + type: "assert-consistent", + verify: (state: AssertableState): void => { + state.assertFileCount(1); + state.assertFileExists("renamed.md"); + state.assertFileNotExists("doc.md"); + state.assertContent("renamed.md", "marker\n"); + } + } + ] +}; diff --git a/frontend/deterministic-tests/src/tests/local-rename-survives-remote-rename.test.ts b/frontend/deterministic-tests/src/tests/local-rename-survives-remote-rename.test.ts new file mode 100644 index 00000000..c2b80af3 --- /dev/null +++ b/frontend/deterministic-tests/src/tests/local-rename-survives-remote-rename.test.ts @@ -0,0 +1,80 @@ +import type { AssertableState } from "../utils/assertable-state"; +import type { TestDefinition } from "../test-definition"; + +export const localRenameSurvivesRemoteRenameTest: TestDefinition = { + description: + "Drain processes a RemoteChange (remote rename for doc D) while a " + + "LocalUpdate (user rename of D) is also queued behind it. " + + "`processRemoteUpdate` moves the disk file and, because there is a " + + "pending LocalUpdate, takes the else branch — but its setDocument " + + "uses the stale `record.path` (= the user-rename target) instead of " + + "the actualPath the file just moved to. The queued LocalUpdate then " + + "reads from `record.path`, throws FileNotFoundError, and is " + + "silently dropped. Setup pins the queue order: a sentinel " + + "LocalUpdate keeps drain busy on a SIGSTOPped HTTP roundtrip while " + + "we resume client 0's WebSocket (enqueues RemoteChange) and then " + + "user-rename D (enqueues LocalUpdate after the RemoteChange). On " + + "server resume the drain pops the sentinel, then RemoteChange, then " + + "LocalUpdate — exactly the order that triggers the bug.", + clients: 2, + steps: [ + { type: "enable-sync", client: 0 }, + { type: "enable-sync", client: 1 }, + { type: "create", client: 0, path: "doc.md", content: "v1\n" }, + { type: "create", client: 0, path: "sentinel.md", content: "s\n" }, + { type: "barrier" }, + + // Pause client 0's WebSocket so the upcoming remote rename buffers. + { type: "pause-websocket", client: 0 }, + + // Server applies remote rename of doc.md -> remote.md. Broadcast + // is buffered on client 0's WebSocket. + { type: "rename", client: 1, oldPath: "doc.md", newPath: "remote.md" }, + { type: "sync", client: 1 }, + + // Pause the server BEFORE arming the sentinel, so the sentinel's + // HTTP request will buffer at the kernel and keep drain occupied. + { type: "pause-server" }, + + // Sentinel: a LocalUpdate on a *different* doc that drain pops + // first. Its HTTP roundtrip stalls on SIGSTOP, freezing drain + // until we resume the server. While drain is frozen we can grow + // the queue with additional events whose order we control. + { + type: "update", + client: 0, + path: "sentinel.md", + content: "s\nedit\n" + }, + + // Resume the WebSocket — buffered remote rename enqueues as a + // RemoteChange. Drain is still stuck on the sentinel HTTP. + { type: "resume-websocket", client: 0 }, + + // User renames doc.md -> local.md on client 0. queue.enqueue + // mutates the doc's record.path to "local.md" and pushes a + // LocalUpdate(rename) onto the tail of the queue. Queue is now + // [sentinel-update (in-flight), RemoteChange, LocalUpdate-rename]. + { type: "rename", client: 0, oldPath: "doc.md", newPath: "local.md" }, + + // Resume the server. Drain pops sentinel-update (succeeds), then + // RemoteChange. Pre-fix: processRemoteUpdate moves disk + // local.md -> remote.md, takes the else branch, and + // setDocument(record.path = "local.md", …) leaves record.path + // stale. Drain pops the LocalUpdate-rename and reads from the + // stale record.path, hits FileNotFoundError, silent skip. + // Post-fix: when a local event is pending, we re-queue the + // remote update without touching disk or record, so the local + // rename drains first and both ends converge. + { type: "resume-server" }, + + { type: "barrier" }, + + { + type: "assert-consistent", + verify: (state: AssertableState): void => { + state.assertFileCount(2); + } + } + ] +}; diff --git a/frontend/deterministic-tests/src/tests/remote-rename-collides-with-pending-local-create.test.ts b/frontend/deterministic-tests/src/tests/remote-rename-collides-with-pending-local-create.test.ts new file mode 100644 index 00000000..1e51bcab --- /dev/null +++ b/frontend/deterministic-tests/src/tests/remote-rename-collides-with-pending-local-create.test.ts @@ -0,0 +1,72 @@ +import type { AssertableState } from "../utils/assertable-state"; +import type { TestDefinition } from "../test-definition"; + +export const remoteRenameCollidesWithPendingLocalCreateTest: TestDefinition = { + description: + "Client 0 has doc D tracked at `original.md`. Client 1 owns doc E " + + "and renames it to `target.md` server-side. Before client 0's " + + "drain processes the WS broadcast for E, the user creates a new " + + "local file `target.md` (a different doc, untracked). When the " + + "buffered RemoteChange for E drains, `processRemoteUpdate` " + + "tries to move client 0's tracked file from its old slot onto " + + "`target.md`. Pre-fix: `MoveOnConflict.NEW` deflects the remote " + + "rename to a `conflict--target.md` stash on client 0, " + + "leaving a permanent local-only divergence (client 1 has no " + + "such stash). Post-fix: when the slot is held by a non-tracked " + + "file (typically the agent's own pending LocalCreate), " + + "`processRemoteUpdate` uses `MoveOnConflict.EXISTING` to " + + "displace it; `updatePendingCreatePath` retargets the displaced " + + "create's `event.path`, so its drain reads the file from the " + + "new location and the server's deconflict on its create lands " + + "the new doc at a clean path.", + clients: 2, + steps: [ + { type: "enable-sync", client: 0 }, + { type: "enable-sync", client: 1 }, + + { type: "create", client: 1, path: "original.md", content: "v1\n" }, + { type: "barrier" }, + + // Pause client 0's WS so the upcoming remote rename buffers and + // we can stage a colliding local create before the rename + // drains on client 0. + { type: "pause-websocket", client: 0 }, + + // Client 1 renames the doc. Server commits, broadcasts to + // client 0 (buffered). + { type: "rename", client: 1, oldPath: "original.md", newPath: "target.md" }, + { type: "sync", client: 1 }, + + // Client 0 still believes the doc is at `original.md`. The user + // creates a NEW file at `target.md` (an unrelated untracked + // doc). Disk on client 0 now has both `original.md` (the + // tracked doc) and `target.md` (the new untracked file). + { type: "create", client: 0, path: "target.md", content: "extra\n" }, + + // Resume client 0's WS. The buffered RemoteChange drains. + // Pre-fix: `MoveOnConflict.NEW` deflects the rename of the + // tracked doc into `conflict--target.md`, with + // `intendedPath=target.md`. + { type: "resume-websocket", client: 0 }, + + { type: "barrier" }, + + { + type: "assert-consistent", + verify: (state: AssertableState): void => { + state.assertFileCount(2); + for (const path of state.files.keys()) { + if (path.startsWith("conflict-")) { + throw new Error( + `Unexpected conflict-uuid stash on a converged client: ${path}` + ); + } + } + state.assertFileExists("target.md"); + state.assertContent("target.md", "v1\n"); + // The local create gets server-deconflicted to a + // sibling path (e.g. `target (1).md`). + } + } + ] +}; diff --git a/frontend/deterministic-tests/src/tests/rename-chain-during-pending-create.test.ts b/frontend/deterministic-tests/src/tests/rename-chain-during-pending-create.test.ts new file mode 100644 index 00000000..e908c163 --- /dev/null +++ b/frontend/deterministic-tests/src/tests/rename-chain-during-pending-create.test.ts @@ -0,0 +1,54 @@ +import type { AssertableState } from "../utils/assertable-state"; +import type { TestDefinition } from "../test-definition"; + +export const renameChainDuringPendingCreateTest: TestDefinition = { + description: + "User creates a doc, then renames it twice while the LocalCreate's " + + "HTTP roundtrip is still in flight (server paused). Each rename " + + "pushes a LocalUpdate whose `documentId` is the create's Promise " + + "(see `pendingDocumentId` in `SyncEventQueue.enqueue`). After the " + + "create resolves, the first rename drains successfully and " + + "`setDocument` walks `events[]` to retarget queued LocalUpdates' " + + "`event.path` to the new disk location — but the comparison " + + "`e.documentId === record.documentId` mismatches the still-Promise " + + "references, so the second rename's `event.path` stays at the " + + "vacated previous slot. On the next drain step `skipIfOversized`'s " + + "`getFileSize(event.path)` throws FileNotFoundError, which " + + "`processEvent` swallows as 'Skipping sync event ... because the " + + "file no longer exists' — losing the user's final rename. " + + "Post-fix: `resolveCreate` (and the displacement-merge branch in " + + "`processCreate`) swap the Promise references for the resolved id " + + "before `setDocument` runs, so retarget works.", + clients: 2, + steps: [ + { type: "enable-sync", client: 0 }, + { type: "enable-sync", client: 1 }, + { type: "barrier" }, + + // Pause the server so client 0's create stalls on the HTTP PUT + // while we queue rename events behind it. + { type: "pause-server" }, + + { type: "create", client: 0, path: "first.md", content: "v1\n" }, + { type: "rename", client: 0, oldPath: "first.md", newPath: "second.md" }, + { type: "rename", client: 0, oldPath: "second.md", newPath: "third.md" }, + + // Resume — drain pops LocalCreate (now resolves), then the two + // queued LocalUpdates. Pre-fix: only the first rename's + // file-system effect lands; the second is silently dropped. + // The server ends up with the doc at second.md, leaving + // client 0's local third.md untracked / out-of-sync. + { type: "resume-server" }, + + { type: "barrier" }, + + { + type: "assert-consistent", + verify: (state: AssertableState): void => { + state.assertFileCount(1); + state.assertFileExists("third.md"); + state.assertContent("third.md", "v1\n"); + } + } + ] +}; diff --git a/frontend/sync-client/src/sync-operations/sync-event-queue.ts b/frontend/sync-client/src/sync-operations/sync-event-queue.ts index e5983ef9..af2f4fed 100644 --- a/frontend/sync-client/src/sync-operations/sync-event-queue.ts +++ b/frontend/sync-client/src/sync-operations/sync-event-queue.ts @@ -311,6 +311,16 @@ export class SyncEventQueue { /** * Call once a create has been acknowledged by the server. + * + * Queued `LocalUpdate` / `LocalDelete` events that were pushed while + * this create was still in-flight carry the create's `resolvers.promise` + * as their `documentId` (see the `pendingDocumentId` branch of + * `enqueue`). We must rewrite those references to the resolved string + * id *before* calling `setDocument`, otherwise its event-rewrite loop + * (which compares `e.documentId === record.documentId`) would silently + * skip them — leaving their `event.path` pointing at the pre-rename + * slot and causing the next drain step's `getFileSize(event.path)` to + * throw `FileNotFoundError`, dropping the user's intent. */ public async resolveCreate( event: Extract, @@ -319,10 +329,36 @@ export class SyncEventQueue { if (removeFromArray(this.events, event)) { this.notifyPendingUpdateCountChanged(); } + this.replacePendingDocumentId( + event.resolvers.promise, + record.documentId + ); await this.setDocument(event.path, record); event.resolvers.resolve(record.documentId); } + /** + * Swap a pending create's `Promise` reference for the + * resolved string id across every queued `LocalUpdate` / `LocalDelete`. + * Call this whenever a create resolves (regular ack OR + * displacement-merge into an existing doc) — see `resolveCreate` for + * the failure mode if it's skipped. + */ + public replacePendingDocumentId( + promise: Promise, + documentId: DocumentId + ): void { + for (const e of this.events) { + if ( + (e.type === SyncEventType.LocalUpdate || + e.type === SyncEventType.LocalDelete) && + e.documentId === promise + ) { + e.documentId = documentId; + } + } + } + /** * Update the settled document map and persist the new document version. * diff --git a/frontend/sync-client/src/sync-operations/syncer.ts b/frontend/sync-client/src/sync-operations/syncer.ts index 31f04b55..de016c41 100644 --- a/frontend/sync-client/src/sync-operations/syncer.ts +++ b/frontend/sync-client/src/sync-operations/syncer.ts @@ -422,13 +422,132 @@ export class Syncer { contentBytes }); - await this.handleMaybeMergingResponse({ - response, - contentHash, - originalContentBytes: contentBytes, - createEvent: event + // `event.path` is mutated in place by `updatePendingCreatePath` + // when a user renames the pending create mid-roundtrip, so we + // read it live on every access — capturing it into a local + // would freeze it at function entry and write the merged bytes + // to the now-vacated path. + let remoteHash: string; + if (response.type === "MergingUpdate") { + const responseBytes = base64ToBytes(response.contentBase64); + await this.operations.write(event.path, contentBytes, responseBytes); + remoteHash = await hash(responseBytes); + await this.updateCache(response.vaultUpdateId, responseBytes, event.path); + } else { + remoteHash = contentHash; + await this.updateCache(response.vaultUpdateId, contentBytes, event.path); + } + + const newRecord = { + documentId: response.documentId, + parentVersionId: response.vaultUpdateId, + remoteRelativePath: response.relativePath + }; + + // Displacement-merge: while this LocalCreate sat in the queue, a + // RemoteCreate for `originalPath` was processed first, displaced + // our local file to a `conflict-…` path, and tracked the remote + // doc at `originalPath`. The server then de-duplicated our + // create into that already-tracked doc and returned its id. + // Slot the merged content into `response.relativePath` by + // deleting D's stale content there and renaming the conflict + // file in. Falling through to the regular `resolveCreate` path + // would call `setDocument(conflict-…, D)`, whose same-docId + // cleanup strips D's tracking from `originalPath` and orphans + // the file there. + const existing = this.queue.getDocumentByDocumentId(response.documentId); + if ( + existing !== undefined && + existing.path === response.relativePath && + existing.path !== event.path + ) { + // We can't `operations.write` the merged bytes onto the + // existing path: that runs a 3-way merge against the stale + // content as if it were a concurrent edit, which strips + // out the very content the server just merged. + await this.operations.delete(response.relativePath); + // We just deleted `response.relativePath`. With + // `MoveOnConflict.NEW` a stray racing occupant would route + // our file to a `conflict--` path; we'd then track + // the doc there with `intendedPath` set. + const moveResult = await this.operations.move( + event.path, + response.relativePath, + MoveOnConflict.NEW + ); + await this.queue.setDocument(moveResult.actualPath, { + ...newRecord, + path: moveResult.actualPath, + intendedPath: + moveResult.actualPath === response.relativePath + ? undefined + : response.relativePath, + remoteHash + }); + this.queue.consumeEvent(event); + event.resolvers.resolve(newRecord.documentId); + this.queue.lastSeenUpdateId = response.vaultUpdateId; + this.history.addHistoryEntry({ + status: SyncStatus.SUCCESS, + details: { type: SyncType.CREATE, relativePath: event.path }, + message: "Created file and merged with existing remote version", + author: response.userId, + timestamp: new Date(response.updatedDate) + }); + return; + } + + // Reconcile disk and tracking with the server-assigned path. + // Two cases produce a mismatch: + // 1. Server deconflicted (e.g. another client raced us): we + // know because `response.relativePath !== event.originalPath`. + // Move the local file to the server-assigned path, otherwise + // a later remote create at our original path would see a + // phantom local conflict and stash the new file under + // `conflict--`. + // 2. The create's local file was displaced to a `conflict-…` + // path while it sat in the queue, but the server still + // placed the doc at our original path (e.g. the existing + // doc that forced the displacement was meanwhile deleted, + // so the server-side merge / deconflict path didn't fire). + // Move the conflict file onto the original path so + // `resolveCreate` tracks the doc at the path the server + // returned, instead of the displaced conflict path which + // would orphan the file. + // + // We must NOT move when `event.path` differs from `originalPath` + // because of a *user rename* of the pending create (e.g. write + // A.md, rename to B.md): there the user's intent is the renamed + // path, the server places the doc at `originalPath`, and the + // queued `LocalUpdate` from the watcher will replay the rename + // to the server. + let resolvedPath = event.path; + let resolvedIntendedPath: RelativePath | undefined; + const needsMove = + response.relativePath !== event.originalPath || + (event.path !== response.relativePath && + CONFLICT_PATH_REGEX.test(event.path)); + if (needsMove) { + const moveResult = await this.operations.move( + event.path, + response.relativePath, + MoveOnConflict.NEW + ); + this.queue.updatePendingCreatePath(event.path, moveResult.actualPath); + resolvedPath = moveResult.actualPath; + resolvedIntendedPath = + moveResult.actualPath === response.relativePath + ? undefined + : response.relativePath; + } + await this.queue.resolveCreate(event, { + ...newRecord, + path: resolvedPath, + intendedPath: resolvedIntendedPath, + remoteHash }); + this.queue.lastSeenUpdateId = response.vaultUpdateId; this.history.addHistoryEntry({ status: SyncStatus.SUCCESS, details: { type: SyncType.CREATE, relativePath: event.path }, @@ -509,6 +628,13 @@ export class Syncer { return; } + // Snapshot of `record.path` before the HTTP roundtrip — used + // after the response to detect a user rename that ran while we + // were awaiting (`record.path !== pathBeforeRoundtrip`). All + // other reads go through `record.path` live so the merged + // bytes land at the user's new location, not the vacated one. + const pathBeforeRoundtrip = record.path; + const response = await this.sendUpdate({ record, relativePath: renameTarget, @@ -524,262 +650,75 @@ export class Syncer { return; } - await this.handleMaybeMergingResponse({ - record, - pathBeforeRoundtrip: record.path, - response, - contentHash, - originalContentBytes: contentBytes - }); + let remoteHash: string; + if (response.type === "MergingUpdate") { + const responseBytes = base64ToBytes(response.contentBase64); + await this.operations.write(record.path, contentBytes, responseBytes); + remoteHash = await hash(responseBytes); + await this.updateCache(response.vaultUpdateId, responseBytes, record.path); + } else { + remoteHash = contentHash; + await this.updateCache(response.vaultUpdateId, contentBytes, record.path); + } + + const newRecord = { + documentId: response.documentId, + parentVersionId: response.vaultUpdateId, + remoteRelativePath: response.relativePath + }; + + if (record.path === pathBeforeRoundtrip) { + // No user rename mid-flight. Move our local file onto the + // server-assigned path (which may differ from what we + // requested if the server deconflicted). `MoveOnConflict.NEW` + // routes ours to a `conflict--` path if the slot is + // locally taken; we record `intendedPath` so future + // server-bound requests use the path the server actually has + // it at. The other doc keeps its slot; local convergence is + // left to manual user resolution. + const moveResult = await this.operations.move( + record.path, + response.relativePath, + MoveOnConflict.NEW + ); + this.queue.updatePendingCreatePath(record.path, moveResult.actualPath); + await this.queue.setDocument(moveResult.actualPath, { + ...newRecord, + path: moveResult.actualPath, + intendedPath: + moveResult.actualPath === response.relativePath + ? undefined + : response.relativePath, + remoteHash + }); + } else { + // User renamed during the roundtrip. Leave the disk file at + // `record.path`; the queued rename's LocalUpdate will + // reconcile the server on its next drain. + await this.queue.setDocument(record.path, { + ...newRecord, + path: record.path, + remoteHash + }); + } + + this.queue.lastSeenUpdateId = response.vaultUpdateId; - const isMerge = "type" in response && response.type === "MergingUpdate"; this.history.addHistoryEntry({ status: SyncStatus.SUCCESS, details: { type: SyncType.UPDATE, relativePath: record.path }, - message: isMerge - ? "Updated file and merged with remote changes" - : "Successfully updated file on the server", + message: + response.type === "MergingUpdate" + ? "Updated file and merged with remote changes" + : "Successfully updated file on the server", author: response.userId, timestamp: new Date(response.updatedDate) }); } - private async handleMaybeMergingResponse({ - record, - pathBeforeRoundtrip, - response, - contentHash, - originalContentBytes, - createEvent - }: { - // Live record reference for a LocalUpdate flow. Path reads go - // through `record.path` so a user-rename mid-roundtrip is seen - // on every access. - record?: DocumentRecord; - // Snapshot of `record.path` captured before `sendUpdate` - // awaited. Compared against the live `record.path` after the - // roundtrip to decide whether a user rename happened in - // between. - pathBeforeRoundtrip?: RelativePath; - response: DocumentUpdateResponse; - contentHash: string; - originalContentBytes: Uint8Array; - // When processing a Create, pass the originating event so its - // `resolvers` promise can be fulfilled (or rejected, on a - // deleted response). The create flow reads the live disk path - // off `createEvent.path` (mutated by - // `updatePendingCreatePath` on a user rename). - createEvent?: Extract; - }): Promise { - const newRecord = { - documentId: response.documentId, - parentVersionId: response.vaultUpdateId, - remoteRelativePath: response.relativePath - }; - let remoteHash: string; - - // The two flows see rename retargeting through different live - // objects: - // - LocalUpdate: `record.path` is mutated in place by - // `queue.enqueue`'s rename branch and `setDocument`. - // - LocalCreate: the doc isn't tracked yet (no - // `resolveCreate` has run); the rename retargets - // `createEvent.path` via `updatePendingCreatePath`. - // In both cases reading the live property at write time keeps - // the merged bytes from being written to a vacated path. - const writePath = - createEvent !== undefined ? createEvent.path : record!.path; - - if ("type" in response && response.type === "MergingUpdate") { - const responseBytes = base64ToBytes(response.contentBase64); - await this.operations.write( - writePath, - originalContentBytes, - responseBytes - ); - - remoteHash = await hash(responseBytes); - - await this.updateCache(response.vaultUpdateId, responseBytes, writePath); - } else { - // Fast-forward update: no merge needed - remoteHash = contentHash; - - await this.updateCache( - response.vaultUpdateId, - originalContentBytes, - writePath - ); - } - - if (createEvent === undefined) { - if (record === undefined || pathBeforeRoundtrip === undefined) { - throw new Error( - "Unreachable: LocalUpdate flow must pass `record` and `pathBeforeRoundtrip`" - ); - } - // `record.path` is the *live* path. If a user rename ran - // during the roundtrip, `queue.enqueue` mutated it (and the - // queued LocalUpdate event's `path` field) to the user's - // new target; otherwise it still equals - // `pathBeforeRoundtrip`. - const currentPath = record.path; - if (currentPath === pathBeforeRoundtrip) { - // Move our local file onto the server-assigned path. - // `MoveOnConflict.NEW` means "if the target is taken - // locally by some other doc, route ours to a - // `conflict--` path instead of evicting them". - // We then record `intendedPath = response.relativePath` - // so future server-bound requests for this doc reference - // the path the server actually has it at, not the local - // conflict-uuid path. The other doc keeps its slot; - // local convergence is left to manual user resolution. - const moveResult = await this.operations.move( - currentPath, - response.relativePath, - MoveOnConflict.NEW - ); - this.queue.updatePendingCreatePath(currentPath, moveResult.actualPath); - await this.queue.setDocument(moveResult.actualPath, { - ...newRecord, - path: moveResult.actualPath, - intendedPath: - moveResult.actualPath === response.relativePath - ? undefined - : response.relativePath, - remoteHash - }); - } else { - // User renamed during the roundtrip. Leave the disk file - // at `currentPath`; the queued rename's LocalUpdate will - // reconcile the server on its next drain. - await this.queue.setDocument(currentPath, { - ...newRecord, - path: currentPath, - remoteHash - }); - } - } else { - // Displacement-merge: while this LocalCreate sat in the queue, a - // RemoteCreate for `originalPath` was processed first, displaced - // our local file to a `conflict-…` path, and tracked the remote - // doc at `originalPath`. The server then de-duplicated our - // create into that already-tracked doc and returned its id. - // Relocate the just-merged content from the conflict path to - // the existing tracked path (overwriting the older content the - // displacement wrote there) and drop the conflict file. - // - // Falling through to `resolveCreate(createEvent, ...)` would - // call `setDocument(conflict-…, D)`, whose same-docId cleanup - // strips D's tracking from `originalPath` and leaves the file - // there orphaned on disk. - const existing = this.queue.getDocumentByDocumentId( - response.documentId - ); - if ( - existing !== undefined && - existing.path === response.relativePath && - existing.path !== createEvent.path - ) { - // The merged content already lives at `createEvent.path` - // (the MergingUpdate branch above wrote it there). Slot - // it into `response.relativePath` by deleting D's stale - // content there and renaming the conflict file in. We - // can't `operations.write` the merged bytes onto the - // existing path: that runs a 3-way merge against the - // stale content as if it were a concurrent edit, which - // strips out the very content the server just merged. - await this.operations.delete(response.relativePath); - // We just deleted `response.relativePath`. With - // `MoveOnConflict.NEW` a stray racing occupant would - // route our file to a `conflict--` path; we'd - // then track the doc there with `intendedPath` set. - const moveResult = await this.operations.move( - createEvent.path, - response.relativePath, - MoveOnConflict.NEW - ); - await this.queue.setDocument(moveResult.actualPath, { - ...newRecord, - path: moveResult.actualPath, - intendedPath: - moveResult.actualPath === response.relativePath - ? undefined - : response.relativePath, - remoteHash - }); - this.queue.consumeEvent(createEvent); - createEvent.resolvers.resolve(newRecord.documentId); - this.queue.lastSeenUpdateId = response.vaultUpdateId; - return; - } - // Reconcile disk and tracking with the server-assigned path. - // Two cases produce a mismatch: - // 1. Server deconflicted (e.g. another client raced us): we - // know because `response.relativePath !== createEvent.originalPath`. - // Move the local file to the server-assigned path, otherwise - // a later remote create at our original path would see a - // phantom local conflict and stash the new file under - // `conflict--`. - // 2. The create's local file was displaced to a `conflict-…` - // path while it sat in the queue, but the server still - // placed the doc at our original path (e.g. the existing - // doc that forced the displacement was meanwhile deleted, - // so the server-side merge / deconflict path didn't - // fire). Move the conflict file onto the original path - // so `resolveCreate` tracks the doc at the path the - // server returned, instead of the displaced conflict - // path which would orphan the file. - // - // We must NOT move when `createEvent.path` differs from - // `originalPath` because of a *user rename* of the pending - // create (e.g. write A.md, rename to B.md): there the user's - // intent is the renamed path, the server places the doc at - // `originalPath`, and the queued `LocalUpdate` from the - // watcher will replay the rename to the server. - let resolvedPath = createEvent.path; - let resolvedIntendedPath: RelativePath | undefined; - if (response.relativePath !== createEvent.originalPath) { - const moveResult = await this.operations.move( - createEvent.path, - response.relativePath, - MoveOnConflict.NEW - ); - this.queue.updatePendingCreatePath(createEvent.path, moveResult.actualPath); - resolvedPath = moveResult.actualPath; - resolvedIntendedPath = - moveResult.actualPath === response.relativePath - ? undefined - : response.relativePath; - } else if ( - createEvent.path !== response.relativePath && - CONFLICT_PATH_REGEX.test(createEvent.path) - ) { - const moveResult = await this.operations.move( - createEvent.path, - response.relativePath, - MoveOnConflict.NEW - ); - this.queue.updatePendingCreatePath(createEvent.path, moveResult.actualPath); - resolvedPath = moveResult.actualPath; - resolvedIntendedPath = - moveResult.actualPath === response.relativePath - ? undefined - : response.relativePath; - } - await this.queue.resolveCreate(createEvent, { - ...newRecord, - path: resolvedPath, - intendedPath: resolvedIntendedPath, - remoteHash - }); - } - - this.queue.lastSeenUpdateId = response.vaultUpdateId; - } private async processRemoteChange( event: Extract diff --git a/frontend/test-client/src/agent/mock-client.ts b/frontend/test-client/src/agent/mock-client.ts index 84da4167..ffdca61b 100644 --- a/frontend/test-client/src/agent/mock-client.ts +++ b/frontend/test-client/src/agent/mock-client.ts @@ -106,10 +106,18 @@ export class MockClient extends debugging.InMemoryFileSystem { }); } + private slowEventChain: Promise = Promise.resolve(); + protected executeFileOperation(callback: () => unknown): void { if (this.useSlowFileEvents) { - // we aren't the best client and it takes some time to notice changes - setTimeout(callback, Math.random() * 100); + // we aren't the best client and it takes some time to notice + // changes, but they still arrive in the order they happened + this.slowEventChain = this.slowEventChain.then(async () => { + await new Promise((resolve) => + setTimeout(resolve, Math.random() * 100) + ); + await callback(); + }); } else { callback(); }