From 39c5591d369aab19a43f54abde60e0cdff76d10c Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 3 May 2026 09:35:56 +0100 Subject: [PATCH] more tests --- .../src/deterministic-agent.ts | 9 ++ .../src/managed-websocket.ts | 11 +++ .../src/test-definition.ts | 4 +- .../deterministic-tests/src/test-registry.ts | 11 ++- .../deterministic-tests/src/test-runner.ts | 8 ++ ...rphan-stash-on-create-dedupe-merge.test.ts | 97 +++++++++++++++++++ .../orphan-stash-on-create-merge.test.ts | 59 +++++++++++ ...remote-update-survives-user-rename.test.ts | 84 ++++++++++++++++ 8 files changed, 281 insertions(+), 2 deletions(-) create mode 100644 frontend/deterministic-tests/src/tests/orphan-stash-on-create-dedupe-merge.test.ts create mode 100644 frontend/deterministic-tests/src/tests/orphan-stash-on-create-merge.test.ts create mode 100644 frontend/deterministic-tests/src/tests/remote-update-survives-user-rename.test.ts diff --git a/frontend/deterministic-tests/src/deterministic-agent.ts b/frontend/deterministic-tests/src/deterministic-agent.ts index da1435b0..a18c3652 100644 --- a/frontend/deterministic-tests/src/deterministic-agent.ts +++ b/frontend/deterministic-tests/src/deterministic-agent.ts @@ -115,6 +115,15 @@ export class DeterministicAgent extends debugging.InMemoryFileSystem { this.log("Sync complete"); } + public async reset(): Promise { + this.log("Resetting client (clears tracked state, keeps disk files)"); + await this.drainPendingSyncOperations(); + await this.client.reset(); + if (this.isSyncEnabled) { + await this.waitForWebSocket(); + } + } + public async disableSync(): Promise { this.log("Disabling sync"); // Drain pending enqueued operations before disabling so the SyncClient diff --git a/frontend/deterministic-tests/src/managed-websocket.ts b/frontend/deterministic-tests/src/managed-websocket.ts index d8801d1b..421561fd 100644 --- a/frontend/deterministic-tests/src/managed-websocket.ts +++ b/frontend/deterministic-tests/src/managed-websocket.ts @@ -177,10 +177,19 @@ class ManagedWebSocket implements WebSocket { */ export class ManagedWebSocketFactory { private readonly instances: ManagedWebSocket[] = []; + // Sticky pause state: applied to current instances on `pause()` AND + // to any new instance created later (e.g. WS reconnect after a + // `disable-sync` / `reset` cycle). Without this, a test pausing the + // WS before the agent reconnects would silently see the new socket + // start un-paused and miss the messages it meant to buffer. + private currentlyPaused = false; public get constructorFn(): typeof globalThis.WebSocket { const trackInstance = (instance: ManagedWebSocket): void => { this.instances.push(instance); + if (this.currentlyPaused) { + instance.pause(); + } }; class TrackedManagedWebSocket extends ManagedWebSocket { public constructor( @@ -195,12 +204,14 @@ export class ManagedWebSocketFactory { } public pause(): void { + this.currentlyPaused = true; for (const ws of this.instances) { ws.pause(); } } public resume(): void { + this.currentlyPaused = false; for (const ws of this.instances) { ws.resume(); } diff --git a/frontend/deterministic-tests/src/test-definition.ts b/frontend/deterministic-tests/src/test-definition.ts index 826c6014..81cdfd12 100644 --- a/frontend/deterministic-tests/src/test-definition.ts +++ b/frontend/deterministic-tests/src/test-definition.ts @@ -18,7 +18,9 @@ export type TestStep = | { type: "barrier" } | { type: "assert-consistent"; verify?: (state: AssertableState) => void } | { type: "pause-websocket"; client: number } - | { type: "resume-websocket"; client: number }; + | { type: "resume-websocket"; client: number } + | { type: "sleep"; ms: number } + | { type: "reset"; client: number }; export interface TestDefinition { description?: string; diff --git a/frontend/deterministic-tests/src/test-registry.ts b/frontend/deterministic-tests/src/test-registry.ts index eeb4d0d2..b67762ad 100644 --- a/frontend/deterministic-tests/src/test-registry.ts +++ b/frontend/deterministic-tests/src/test-registry.ts @@ -97,6 +97,9 @@ import { catchupCreateAndUpdateNotSkippedTest } from "./tests/catchup-create-and import { localRenameSurvivesRemoteRenameTest } from "./tests/local-rename-survives-remote-rename.test"; import { renameChainDuringPendingCreateTest } from "./tests/rename-chain-during-pending-create.test"; import { remoteRenameCollidesWithPendingLocalCreateTest } from "./tests/remote-rename-collides-with-pending-local-create.test"; +import { remoteUpdateSurvivesUserRenameTest } from "./tests/remote-update-survives-user-rename.test"; +import { orphanStashOnCreateMergeTest } from "./tests/orphan-stash-on-create-merge.test"; +import { orphanStashOnCreateDedupeMergeTest } from "./tests/orphan-stash-on-create-dedupe-merge.test"; export const TESTS: Partial> = { "rename-create-conflict": renameCreateConflictTest, @@ -221,5 +224,11 @@ export const TESTS: Partial> = { "rename-chain-during-pending-create": renameChainDuringPendingCreateTest, "remote-rename-collides-with-pending-local-create": - remoteRenameCollidesWithPendingLocalCreateTest + remoteRenameCollidesWithPendingLocalCreateTest, + "remote-update-survives-user-rename": + remoteUpdateSurvivesUserRenameTest, + "orphan-stash-on-create-merge": + orphanStashOnCreateMergeTest, + "orphan-stash-on-create-dedupe-merge": + orphanStashOnCreateDedupeMergeTest }; diff --git a/frontend/deterministic-tests/src/test-runner.ts b/frontend/deterministic-tests/src/test-runner.ts index 6e83883e..1285b41e 100644 --- a/frontend/deterministic-tests/src/test-runner.ts +++ b/frontend/deterministic-tests/src/test-runner.ts @@ -198,6 +198,14 @@ export class TestRunner { this.getAgent(step.client).resumeWebSocket(); break; + case "sleep": + await sleep(step.ms); + break; + + case "reset": + await this.getAgent(step.client).reset(); + break; + default: { const unknownStep = step as { type: string }; throw new Error(`Unknown step type: ${unknownStep.type}`); diff --git a/frontend/deterministic-tests/src/tests/orphan-stash-on-create-dedupe-merge.test.ts b/frontend/deterministic-tests/src/tests/orphan-stash-on-create-dedupe-merge.test.ts new file mode 100644 index 00000000..be139445 --- /dev/null +++ b/frontend/deterministic-tests/src/tests/orphan-stash-on-create-dedupe-merge.test.ts @@ -0,0 +1,97 @@ +import type { AssertableState } from "../utils/assertable-state"; +import type { TestDefinition } from "../test-definition"; + +export const orphanStashOnCreateDedupeMergeTest: TestDefinition = { + description: + "When the server's create endpoint dedupe-merges a client's local " + + "create into an existing fresh remote doc that the client has " + + "already tracked at a `conflict--` stash (because the " + + "remote create's broadcast displaced its content there), " + + "`processCreate`'s response handler relocates the doc's record " + + "onto the canonical path via `setDocument` but the stash file on " + + "disk is left behind — outliving its tracking record and " + + "diverging from every other client. Reproducing the merge half " + + "of the dedupe is delicate: the server's merge gate requires the " + + "POST's `last_seen_vault_update_id` to be *strictly less than* " + + "the existing doc's `creation_vault_update_id`. A normal sync " + + "advances the watermark contiguously, so on the canonical " + + "create-vs-create race the watermark would already include the " + + "remote doc's create when the local POST ships, the merge gate " + + "fails, and the server deconflicts to `(1)`. This test pokes a " + + "permanent gap into the watermark via the catch-up replay's " + + "latest-only semantics: a tempdoc created and deleted while " + + "Client 0 is offline lives in catch-up only as the delete event, " + + "which the client processes (advancing the watermark to the " + + "delete) without ever filling the create's update id — so the " + + "watermark's contiguous-prefix min stays below the next doc's " + + "creation. With that gap in place, Client 0's post-displacement " + + "POST satisfies the server's merge gate, the server returns the " + + "existing docId, and `processCreate` walks straight into the " + + "orphan-stash bug. Pre-fix: `Files from agent-0 missing in " + + "agent-1` (the `conflict--` stash). Post-fix: cleaned up.", + clients: 2, + steps: [ + { type: "enable-sync", client: 0 }, + { type: "enable-sync", client: 1 }, + { type: "barrier" }, + + // Client 0 goes offline. Its watermark is saved at v=0. + { type: "disable-sync", client: 0 }, + + // Tempdoc that lives only as a delete in Client 0's catch-up + // (the create at v=1 is collapsed away by latest-only replay). + // The processed delete advances the watermark to v=2 but leaves + // v=1 unfilled, parking the contiguous-prefix min at 0. + { type: "create", client: 1, path: "tempdoc.md", content: "x\n" }, + { type: "sync", client: 1 }, + { type: "delete", client: 1, path: "tempdoc.md" }, + { type: "sync", client: 1 }, + + // The doc whose dedupe-merge we want to trigger — fresh + // (creation == latest), mergeable text. Its creation v=3 is + // strictly greater than Client 0's stuck min of 0, so the + // server's merge gate will fire. + { type: "create", client: 1, path: "file.md", content: "from-1\n" }, + { type: "sync", client: 1 }, + + // Re-arm the WS pause for the new socket Client 0 is about to + // create on enable-sync, so the catch-up broadcast is buffered + // until we explicitly release it. Without sticky pause across + // factory `constructorFn` calls this would silently miss the + // catch-up. + { type: "pause-websocket", client: 0 }, + { type: "enable-sync", client: 0 }, + + // Server pause arrives before the buffered catch-up is released + // so the resume below parks Client 0's drain on the GET for + // file.md's content (the only fetching event in the catch-up; + // the tempdoc delete needs no fetch and runs through quickly, + // leaving the watermark gap intact). + { type: "pause-server" }, + { type: "resume-websocket", client: 0 }, + + // Yield so the drain has time to traverse the WS handler → + // listener → enqueue → drain → processRemoteCreateForNewDocument + // → fetch hops before the local create runs. + { type: "sleep", ms: 100 }, + + // Client 0 creates file.md locally while the GET is parked. The + // file occupies the canonical slot, so when the GET returns the + // remote create displaces D's bytes to `conflict--file.md` + // and tracks D there with `intendedPath=file.md`. The + // LocalCreate enqueues behind the in-flight RemoteChange. + { type: "create", client: 0, path: "file.md", content: "from-0\n" }, + + { type: "resume-server" }, + + { type: "barrier" }, + + { + type: "assert-consistent", + verify: (s: AssertableState): void => { + s.assertFileCount(1); + s.assertFileExists("file.md"); + } + } + ] +}; diff --git a/frontend/deterministic-tests/src/tests/orphan-stash-on-create-merge.test.ts b/frontend/deterministic-tests/src/tests/orphan-stash-on-create-merge.test.ts new file mode 100644 index 00000000..ed2b4efa --- /dev/null +++ b/frontend/deterministic-tests/src/tests/orphan-stash-on-create-merge.test.ts @@ -0,0 +1,59 @@ +import type { AssertableState } from "../utils/assertable-state"; +import type { TestDefinition } from "../test-definition"; + +export const orphanStashOnCreateMergeTest: TestDefinition = { + description: + "Client 1 creates file.md (server doc D). Client 0's WebSocket is " + + "paused, so the broadcast is buffered. The server is paused, then " + + "the WebSocket released — Client 0 enters " + + "`processRemoteCreateForNewDocument` and parks on the GET for D's " + + "content. While parked, Client 0 creates file.md locally. The GET " + + "returns and the remote create displaces to " + + "`conflict--file.md` (slot occupied), tracking D there with " + + "`intendedPath=file.md`. Client 0's LocalCreate POST then drains " + + "and the server deconflicts (because Client 0's lastSeenVaultUpdateId " + + "now equals D's creation, so the merge condition fails) — creating " + + "a sibling doc D' at `file (1).md`. The convergence path then " + + "needs `unwindReadyStashes` to slide D off the conflict-uuid stash " + + "back to file.md once Client 0's local file moves to file (1).md, " + + "leaving both clients with [file.md, file (1).md]. Documents the " + + "displacement-then-deconflict-then-unwind path the fix to " + + "`processCreate`'s same-docId orphan cleanup must not regress.", + clients: 2, + steps: [ + { type: "enable-sync", client: 0 }, + { type: "enable-sync", client: 1 }, + { type: "barrier" }, + + { type: "pause-websocket", client: 0 }, + + { type: "create", client: 1, path: "file.md", content: "from-1\n" }, + { type: "sync", client: 1 }, + + { type: "pause-server" }, + + { type: "resume-websocket", client: 0 }, + + // Yield long enough for the drain to traverse all the microtask + // hops between the WS handler and the GET, so the request is + // queued at the (paused) server before the local create runs. + { type: "sleep", ms: 50 }, + + { type: "create", client: 0, path: "file.md", content: "from-0\n" }, + + { type: "resume-server" }, + + { type: "barrier" }, + + { + type: "assert-consistent", + verify: (s: AssertableState): void => { + s.assertFileCount(2); + s.assertFileExists("file.md"); + s.assertFileExists("file (1).md"); + s.assertAnyFileContains("from-0"); + s.assertAnyFileContains("from-1"); + } + } + ] +}; diff --git a/frontend/deterministic-tests/src/tests/remote-update-survives-user-rename.test.ts b/frontend/deterministic-tests/src/tests/remote-update-survives-user-rename.test.ts new file mode 100644 index 00000000..b78ad143 --- /dev/null +++ b/frontend/deterministic-tests/src/tests/remote-update-survives-user-rename.test.ts @@ -0,0 +1,84 @@ +import type { AssertableState } from "../utils/assertable-state"; +import type { TestDefinition } from "../test-definition"; + +export const remoteUpdateSurvivesUserRenameTest: TestDefinition = { + description: + "Client 0 updates a tracked doc; while Client 1 is processing the " + + "broadcast and parked on the GET for the new version's content, the " + + "user renames the doc on Client 1. Pre-fix: `processRemoteUpdate` " + + "captures `actualPath` before the await and, after the GET returns, " + + "calls `write(actualPath, …)` (no-op — file was renamed away), " + + "`updateCache(actualPath, …)`, and `setDocument(actualPath, …)`. " + + "`setDocument` mutates the same record in place so its `path` is " + + "yanked from the user's renamed slot back to the pre-rename path, " + + "wiping the rename out of the queue's documents map. The queued " + + "`LocalUpdate` then reads from the now-stale `record.path`, hits " + + "`FileNotFoundError`, and is silently dropped — the user's rename " + + "never reaches the server. Post-fix: the handler defers when a " + + "local event landed mid-await, so the rename drains first and " + + "the deferred remote update is folded into the broadcast that " + + "follows the rename round-trip.", + clients: 2, + steps: [ + { type: "create", client: 0, path: "doc.md", content: "v1\n" }, + { type: "enable-sync", client: 0 }, + { type: "enable-sync", client: 1 }, + { type: "barrier" }, + + // Buffer Client 1's incoming broadcasts so it doesn't see + // Client 0's update until we've paused the server. + { type: "pause-websocket", client: 1 }, + + // Server now holds v=2 of doc.md. + { type: "update", client: 0, path: "doc.md", content: "v2\n" }, + { type: "sync", client: 0 }, + + // Pause the server. Client 1's upcoming GET for the new version + // content blocks at the OS layer until resume. + { type: "pause-server" }, + + // Release the buffered broadcast. Client 1's drain enters + // `processRemoteUpdate`, captures `actualPath`, fires the GET, + // and parks awaiting the response. + { type: "resume-websocket", client: 1 }, + + // Yield long enough for the drain to traverse all microtask + // hops between the WS handler and the GET, so the HTTP request + // is queued at the (paused) server before the rename runs. + // Without this yield the rename would be enqueued before + // `processRemoteUpdate`'s entry-time `hasPendingLocalEvents` + // check and the early-defer branch would mask the bug. + { type: "sleep", ms: 50 }, + + // While the GET is in flight the user renames the doc. The queue + // mutates `record.path` to "renamed.md" in place and pushes a + // LocalUpdate carrying the rename target. + { + type: "rename", + client: 1, + oldPath: "doc.md", + newPath: "renamed.md" + }, + + // Resume the server. The GET response unblocks + // `processRemoteUpdate`. With the fix in place it sees the + // queued LocalUpdate and defers; without the fix it walks past + // the rename and clobbers the documents map, dropping the + // pending LocalUpdate's read on the way back through. + { type: "resume-server" }, + + { type: "barrier" }, + + { + type: "assert-consistent", + verify: (s: AssertableState): void => { + s.assertFileCount(1); + s.assertFileExists("renamed.md"); + s.assertFileNotExists("doc.md"); + // Both edits survive: the user's rename and Client 0's + // content update at v=2. + s.assertContent("renamed.md", "v2\n"); + } + } + ] +};