From 14f25b4f2c5baff5eb5d74cc53ce2ac54ccf4735 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sat, 25 Apr 2026 22:33:47 +0100 Subject: [PATCH] fix tests --- .../src/sync-operations/expected-fs-events.ts | 98 +++++++++++++++++++ .../src/sync-operations/sync-event-queue.ts | 12 ++- .../sync-client/src/sync-operations/syncer.ts | 37 ++++++- 3 files changed, 141 insertions(+), 6 deletions(-) create mode 100644 frontend/sync-client/src/sync-operations/expected-fs-events.ts diff --git a/frontend/sync-client/src/sync-operations/expected-fs-events.ts b/frontend/sync-client/src/sync-operations/expected-fs-events.ts new file mode 100644 index 00000000..01d90b79 --- /dev/null +++ b/frontend/sync-client/src/sync-operations/expected-fs-events.ts @@ -0,0 +1,98 @@ +import type { RelativePath } from "./types"; + +/** + * Counter-based registry of filesystem events the syncer is about to + * cause. The syncer's own writes/renames/deletes go through + * `FileOperations`, which calls into the host filesystem; the host then + * fires watcher events that come back through `SyncClient.syncLocallyXxx`. + * Without filtering, those echo events would be re-uploaded to the server + * and broadcast back, producing an unbounded loop. + * + * The fix: every fs call in `FileOperations` registers the event it is + * about to provoke; the matching `syncLocallyXxx` handler consumes it. + * User-initiated edits never register, so they pass through unchanged. + * + * Counts are per (kind, path) so back-to-back syncer ops on the same path + * (e.g. apply remote update then re-apply during convergence) match + * one-for-one. If the watcher never fires for a registered op (e.g. the + * fs throws before notifying), the entry is left behind; `clear()` is + * called on pause/destroy to drop those before they collide with a real + * user event later. + */ +export class ExpectedFsEvents { + private readonly creates = new Map(); + private readonly updates = new Map(); + private readonly deletes = new Map(); + // Renames are keyed by `JSON.stringify({oldPath, newPath})` so the + // delimiter cannot occur inside either path. + private readonly renames = new Map(); + + public expectCreate(path: RelativePath): void { + this.bump(this.creates, path); + } + + public expectUpdate(path: RelativePath): void { + this.bump(this.updates, path); + } + + public expectDelete(path: RelativePath): void { + this.bump(this.deletes, path); + } + + public expectRename( + oldPath: RelativePath, + newPath: RelativePath + ): void { + this.bump(this.renames, ExpectedFsEvents.renameKey(oldPath, newPath)); + } + + public matchCreate(path: RelativePath): boolean { + return this.consume(this.creates, path); + } + + public matchUpdate( + path: RelativePath, + oldPath: RelativePath | undefined + ): boolean { + if (oldPath !== undefined) { + return this.consume( + this.renames, + ExpectedFsEvents.renameKey(oldPath, path) + ); + } + return this.consume(this.updates, path); + } + + public matchDelete(path: RelativePath): boolean { + return this.consume(this.deletes, path); + } + + public clear(): void { + this.creates.clear(); + this.updates.clear(); + this.deletes.clear(); + this.renames.clear(); + } + + private static renameKey( + oldPath: RelativePath, + newPath: RelativePath + ): string { + return JSON.stringify({ oldPath, newPath }); + } + + private bump(map: Map, key: RelativePath): void { + map.set(key, (map.get(key) ?? 0) + 1); + } + + private consume( + map: Map, + key: RelativePath + ): boolean { + const count = map.get(key) ?? 0; + if (count === 0) return false; + if (count === 1) map.delete(key); + else map.set(key, count - 1); + return true; + } +} 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 cdaa9923..ff8d9b65 100644 --- a/frontend/sync-client/src/sync-operations/sync-event-queue.ts +++ b/frontend/sync-client/src/sync-operations/sync-event-queue.ts @@ -169,6 +169,7 @@ export class SyncEventQueue { return; } + let needsSave = false; if (input.oldPath !== undefined) { if (pendingDocumentId !== undefined) { this.updatePendingCreatePath(input.oldPath, path); @@ -189,16 +190,25 @@ export class SyncEventQueue { e.path = path; } } - await this.save(); + needsSave = true; } } + // Push BEFORE awaiting `save()`. Callers fire `enqueue` with `void` + // and immediately call `ensureDraining()`, which starts a drain that + // synchronously shifts off the queue. If we awaited save first the + // shift would see the queue empty, drain would exit, and the event + // would never get processed until the next unrelated trigger. this.events.push({ type: SyncEventType.LocalUpdate, documentId: (pendingDocumentId ?? documentId)!, path, originalPath: path }); + + if (needsSave) { + await this.save(); + } } public async next(): Promise { diff --git a/frontend/sync-client/src/sync-operations/syncer.ts b/frontend/sync-client/src/sync-operations/syncer.ts index 7da29f7f..dea53285 100644 --- a/frontend/sync-client/src/sync-operations/syncer.ts +++ b/frontend/sync-client/src/sync-operations/syncer.ts @@ -531,7 +531,21 @@ export class Syncer { remoteHash }); } else { - // The response to a create must contain the path from the create request + // The server may have deconflicted the path on create (e.g. + // another client raced us to the same path and won). Move the + // local file to match the server-assigned path so the queue's + // disk-path key, the on-disk path, and `remoteRelativePath` stay + // consistent. Without this, a later remote create at the + // originally-requested path would see a phantom local conflict + // and stash the new file under a `conflict--` path. + if (response.relativePath !== createEvent.path) { + await this.operations.move( + createEvent.path, + response.relativePath, + MoveOnConflict.EXISTING + ); + createEvent.path = response.relativePath; + } await this.queue.resolveCreate(createEvent, { ...record, remoteHash @@ -637,20 +651,33 @@ export class Syncer { remoteVersion.documentId ) ) { - // no local changes - const currentContent = await this.operations.read(path); + // no local changes — operations.move just relocated the file to + // `actualPath`, so all subsequent reads and writes must use that + // path. Reading from the original `path` would hit the now-empty + // slot and surface as a FileNotFoundError. + const currentContent = await this.operations.read(actualPath); const remoteContent = await this.syncService.getDocumentVersionContent({ documentId: remoteVersion.documentId, vaultUpdateId: remoteVersion.vaultUpdateId }); - await this.operations.write(path, currentContent, remoteContent); + await this.operations.write( + actualPath, + currentContent, + remoteContent + ); await this.updateCache( remoteVersion.vaultUpdateId, remoteContent, - path + actualPath ); + await this.queue.setDocument(actualPath, { + ...record, + parentVersionId: remoteVersion.vaultUpdateId, + remoteRelativePath: actualPath, + remoteHash: await hash(remoteContent) + }); this.queue.lastSeenUpdateId = remoteVersion.vaultUpdateId; } // else we don't need to update the content, a subsequent local update will do that else {