Fix syncing when network latency is present (#4)

* WIP

* Add debug

* Dedupe inserts

* Add deterministic ordering

* Fix whitespaces

* Update insta

* Add integration test script

* Rename

* Add test

* Working for non-deletes

* omg it mostly works for deletes

* Isdeleted fix

* remove created dates

* update api

* Take document id

* No max attempt

* works

* Use string uuids

* .

* working!!!! (hopefully)

* Improve bundling

* Add module

* lint

* .

* lint

* Fix CI

* use toolchain

* clean up

* Add useSlowFileEvents

* Delete fuzz

* Fix CI

* use docker

* fix script

* clean up

* Clean up

* change node version

* Build docker image on every commit

* fix ci

* 1 db per vault

* Add scritps folder

* Bump versions

* Lint

* .

* Fix tests for real

* Style

* .

* try

* Consistent ordering

* Fix tests

* hmm

* .

* Clean up diff

* Fixes

* .

* Fix version bump

* .

* .

* .

frontend/sync-client/src/file-operations/document-locks.ts

@@ -1,6 +1,9 @@
 import type { Logger } from "../tracing/logger";
 import type { RelativePath } from "../persistence/database";
 
+// Manages locks on documents to prevent concurrent modifications
+// allowing the client's FileOperations implementation to be simpler.
+// Locks are granted in a first-in-first-out order.
 export class DocumentLocks {
 	private readonly locked = new Set<RelativePath>();
 	private readonly waiters = new Map<RelativePath, (() => void)[]>();

frontend/sync-client/src/file-operations/file-operations.test.ts

@@ -1,16 +1,27 @@
-import type { FileSystemOperations } from "sync-client";
-import type { Database, RelativePath } from "../persistence/database";
+import type {
+	Database,
+	DocumentRecord,
+	RelativePath
+} from "../persistence/database";
 import { FileOperations } from "./file-operations";
 import { Logger } from "../tracing/logger";
 import { assertSetContainsExactly } from "../utils/assert-set-contains-exactly";
+import type { FileSystemOperations } from "./filesystem-operations";
 
 describe("File operations", () => {
-	class MockDatabase {
-		public async updatePath(
+	class MockDatabase implements Partial<Database> {
+		public getLatestDocumentByRelativePath(
+			_find: RelativePath
+		): DocumentRecord | undefined {
+			// no-op
+			return undefined;
+		}
+
+		public move(
 			_oldRelativePath: RelativePath,
 			_newRelativePath: RelativePath
-		): Promise<void> {
-			// this is called but irrelevant for this mock
+		): void {
+			// no-op
 		}
 	}
 

frontend/sync-client/src/file-operations/file-operations.ts

@@ -1,10 +1,6 @@
-import type { Logger } from "src/tracing/logger";
+import type { Logger } from "../tracing/logger";
 import type { FileSystemOperations } from "./filesystem-operations";
-import type {
-	Database,
-	DocumentId,
-	RelativePath
-} from "src/persistence/database";
+import type { Database, RelativePath } from "../persistence/database";
 import { isBinary, isFileTypeMergable, mergeText } from "sync_lib";
 import { SafeFileSystemOperations } from "./safe-filesystem-operations";
 
@@ -17,7 +13,7 @@ export class FileOperations {
 		private readonly database: Database,
 		fs: FileSystemOperations
 	) {
-		this.fs = new SafeFileSystemOperations(fs);
+		this.fs = new SafeFileSystemOperations(fs, logger);
 	}
 
 	public async listAllFiles(): Promise<RelativePath[]> {
@@ -35,7 +31,7 @@ export class FileOperations {
 
 		const decoder = new TextDecoder("utf-8");
 
-		// Normalize line endings to LF on Windows
+		// Normalize line-endings to LF on Windows
 		let text = decoder.decode(content);
 		text = text.replace(/\r\n/g, "\n");
 
@@ -46,10 +42,6 @@ export class FileOperations {
 		return this.fs.getFileSize(path);
 	}
 
-	public async getModificationTime(path: RelativePath): Promise<Date> {
-		return this.fs.getModificationTime(path);
-	}
-
 	public async exists(path: RelativePath): Promise<boolean> {
 		return this.fs.exists(path);
 	}
@@ -60,18 +52,23 @@ export class FileOperations {
 		path: RelativePath,
 		newContent: Uint8Array
 	): Promise<void> {
+		this.logger.debug(`Creating file: ${path}`);
+
+		await this.fs.write(path, newContent);
+	}
+
+	public async ensureClearPath(path: RelativePath): Promise<void> {
 		if (await this.fs.exists(path)) {
 			const deconflictedPath = await this.deconflictPath(path);
 			this.logger.debug(
 				`Didn't expect ${path} to exist, deconflicting by moving it to '${deconflictedPath}'`
 			);
-			await this.database.updatePath(path, deconflictedPath);
+
+			this.database.move(path, deconflictedPath);
 			await this.fs.rename(path, deconflictedPath);
 		} else {
 			await this.createParentDirectories(path);
 		}
-
-		await this.fs.write(path, newContent);
 	}
 
 	// Update the file at the given path.
@@ -126,40 +123,25 @@ export class FileOperations {
 		return new TextEncoder().encode(resultText);
 	}
 
-	public async remove(path: RelativePath): Promise<void> {
-		this.logger.debug(`Deleting file: ${path}`);
-		return this.fs.delete(path);
+	public async delete(path: RelativePath): Promise<void> {
+		if (await this.exists(path)) {
+			this.logger.debug(`Deleting file: ${path}`);
+			return this.fs.delete(path);
+		} else {
+			this.logger.debug(`No need to delete '${path}', it doesn't exist`);
+		}
 	}
 
 	public async move(
 		oldPath: RelativePath,
-		newPath: RelativePath,
-		documentId?: DocumentId
+		newPath: RelativePath
 	): Promise<void> {
 		if (oldPath === newPath) {
 			return;
 		}
+		await this.ensureClearPath(newPath);
 
-		if (await this.fs.exists(newPath)) {
-			const deconflictedPath = await this.deconflictPath(newPath);
-			this.logger.debug(
-				`Conflict when moving '${oldPath}' to '${newPath}', the latter already exists, deconflicting by moving it to '${deconflictedPath}'`
-			);
-
-			const existingMetadata = this.database.getDocument(newPath);
-			if (
-				existingMetadata === undefined ||
-				existingMetadata.documentId !== documentId
-			) {
-				await this.database.updatePath(newPath, deconflictedPath);
-				await this.fs.rename(newPath, deconflictedPath);
-			} else {
-				await this.database.deleteDocument(newPath);
-			}
-		} else {
-			await this.createParentDirectories(newPath);
-		}
-
+		this.database.move(oldPath, newPath);
 		await this.fs.rename(oldPath, newPath);
 	}
 
@@ -201,17 +183,12 @@ export class FileOperations {
 		);
 		stem = stem.replace(FileOperations.PARENTHESES_REGEX, "");
 
-		// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-		while (true) {
-			const newName =
-				currentCount === 0
-					? `${directory}${stem}${extension}`
-					: `${directory}${stem} (${currentCount})${extension}`;
-			if (await this.fs.exists(newName)) {
-				currentCount++;
-			} else {
-				return newName;
-			}
-		}
+		let newName = path;
+		do {
+			currentCount++;
+			newName = `${directory}${stem} (${currentCount})${extension}`;
+		} while (await this.fs.exists(newName));
+
+		return newName;
 	}
 }

frontend/sync-client/src/file-operations/filesystem-operations.ts

@@ -1,4 +1,4 @@
-import type { RelativePath } from "src/persistence/database";
+import type { RelativePath } from "../persistence/database";
 
 export interface FileSystemOperations {
 	listAllFiles: () => Promise<RelativePath[]>;
@@ -9,11 +9,8 @@ export interface FileSystemOperations {
 		updater: (currentContent: string) => string
 	) => Promise<string>;
 	getFileSize: (path: RelativePath) => Promise<number>;
-	getModificationTime: (path: RelativePath) => Promise<Date>;
 	exists: (path: RelativePath) => Promise<boolean>;
 	createDirectory: (path: RelativePath) => Promise<void>;
 	delete: (path: RelativePath) => Promise<void>;
-
-	// Must be able to handle renaming to a file that already exists
 	rename: (oldPath: RelativePath, newPath: RelativePath) => Promise<void>;
 }

frontend/sync-client/src/file-operations/safe-filesystem-operations.ts

@@ -1,5 +1,7 @@
-import type { RelativePath } from "src/persistence/database";
+import type { RelativePath } from "../persistence/database";
 import type { FileSystemOperations } from "./filesystem-operations";
+import type { Logger } from "../tracing/logger";
+import { DocumentLocks } from "./document-locks";
 
 export class FileNotFoundError extends Error {
 	public constructor(message: string) {
@@ -9,71 +11,134 @@ export class FileNotFoundError extends Error {
 }
 
 // Decorate FileSystemOperations replacing errors with FileNotFoundError
-// if the accessed file doesn't exist.
+// if the accessed file doesn't exist. It also ensures that there's only
+// ever a single request in-flight for any one file through the use of
+// DocumentLocks.
 export class SafeFileSystemOperations implements FileSystemOperations {
-	public constructor(private readonly fs: FileSystemOperations) {}
+	private readonly locks: DocumentLocks;
+
+	public constructor(
+		private readonly fs: FileSystemOperations,
+		private readonly logger: Logger
+	) {
+		this.locks = new DocumentLocks(logger);
+	}
 
 	public async listAllFiles(): Promise<RelativePath[]> {
 		return this.fs.listAllFiles();
 	}
 
 	public async read(path: RelativePath): Promise<Uint8Array> {
-		return this.safeOperation(path, async () => this.fs.read(path));
+		this.logger.debug(`Reading file: ${path}`);
+		return this.safeOperation(
+			path,
+			this.decorateToHoldLock(path, async () => this.fs.read(path)),
+			"read"
+		);
 	}
 
 	public async write(path: RelativePath, content: Uint8Array): Promise<void> {
-		return this.fs.write(path, content);
+		this.logger.debug(`Writing file: ${path}`);
+		return this.decorateToHoldLock(path, async () =>
+			this.fs.write(path, content)
+		)();
 	}
 
 	public async atomicUpdateText(
 		path: RelativePath,
 		updater: (currentContent: string) => string
 	): Promise<string> {
-		return this.safeOperation(path, async () =>
-			this.fs.atomicUpdateText(path, updater)
+		this.logger.debug(`Atomic update of file: ${path}`);
+		return this.safeOperation(
+			path,
+			this.decorateToHoldLock(path, async () =>
+				this.fs.atomicUpdateText(path, updater)
+			),
+			"atomicUpdateText"
 		);
 	}
 
 	public async getFileSize(path: RelativePath): Promise<number> {
-		return this.safeOperation(path, async () => this.fs.getFileSize(path));
-	}
-
-	public async getModificationTime(path: RelativePath): Promise<Date> {
-		return this.safeOperation(path, async () =>
-			this.fs.getModificationTime(path)
+		this.logger.debug(`Getting file size: ${path}`);
+		return this.safeOperation(
+			path,
+			this.decorateToHoldLock(path, async () =>
+				this.fs.getFileSize(path)
+			),
+			"getFileSize"
 		);
 	}
 
 	public async exists(path: RelativePath): Promise<boolean> {
-		return this.fs.exists(path);
+		this.logger.debug(`Checking if file exists: ${path}`);
+		return this.decorateToHoldLock(path, async () =>
+			this.fs.exists(path)
+		)();
 	}
 
 	public async createDirectory(path: RelativePath): Promise<void> {
-		return this.fs.createDirectory(path);
+		this.logger.debug(`Creating directory: ${path}`);
+		return this.decorateToHoldLock(path, async () =>
+			this.fs.createDirectory(path)
+		)();
 	}
 
 	public async delete(path: RelativePath): Promise<void> {
-		return this.fs.delete(path);
+		this.logger.debug(`Deleting file: ${path}`);
+		return this.decorateToHoldLock(path, async () =>
+			this.fs.delete(path)
+		)();
 	}
 
 	public async rename(
 		oldPath: RelativePath,
 		newPath: RelativePath
 	): Promise<void> {
-		return this.safeOperation(oldPath, async () =>
-			this.fs.rename(oldPath, newPath)
+		this.logger.debug(`Renaming file: ${oldPath} to ${newPath}`);
+		return this.safeOperation(
+			oldPath,
+			this.decorateToHoldLock([oldPath, newPath], async () =>
+				this.fs.rename(oldPath, newPath)
+			),
+			"rename"
 		);
 	}
 
+	private decorateToHoldLock<T>(
+		pathOrPaths: RelativePath | RelativePath[],
+		operation: () => Promise<T>
+	): () => Promise<T> {
+		return async () => {
+			const paths = Array.isArray(pathOrPaths)
+				? pathOrPaths
+				: [pathOrPaths];
+			await Promise.all(
+				paths.map(async (path) => this.locks.waitForDocumentLock(path))
+			);
+			try {
+				return await operation();
+			} finally {
+				await Promise.all(
+					paths.map((path) => {
+						this.locks.unlockDocument(path);
+					})
+				);
+			}
+		};
+	}
+
 	private async safeOperation<T>(
 		path: RelativePath,
-		operation: () => Promise<T>
+		operation: () => Promise<T>,
+		operationName: string
 	): Promise<T> {
 		// Without locking the file, this isn't atomic, however, it's good enough practicaly.
 		// This will only break if the file exists, gets deleted and then immediately
 		// recreated while `operation` is running.
 		if (!(await this.fs.exists(path))) {
-			throw new FileNotFoundError(path);
+			throw new FileNotFoundError(
+				`File not found: ${path} before trying to ${operationName}`
+			);
 		}
 		try {
 			return await operation();
@@ -81,7 +146,9 @@ export class SafeFileSystemOperations implements FileSystemOperations {
 			if (await this.fs.exists(path)) {
 				throw error;
 			} else {
-				throw new FileNotFoundError(path);
+				throw new FileNotFoundError(
+					`File not found: ${path} when trying to ${operationName}`
+				);
 			}
 		}
 	}