Fix race condition of client-side path deconflicting

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

@@ -61,12 +61,16 @@ export class FileOperations {
 	public async ensureClearPath(path: RelativePath): Promise<void> {
 		if (await this.fs.exists(path)) {
 			const deconflictedPath = await this.deconflictPath(path);
-			this.logger.debug(
+			try {
-				`Didn't expect ${path} to exist, deconflicting by moving it to '${deconflictedPath}'`
+				this.logger.debug(
-			);
+					`Didn't expect ${path} to exist, deconflicting by moving it to '${deconflictedPath}'`
+				);
 
-			this.database.move(path, deconflictedPath);
+				this.database.move(path, deconflictedPath);
-			await this.fs.rename(path, deconflictedPath);
+				await this.fs.rename(path, deconflictedPath, true);
+			} finally {
+				this.fs.unlock(deconflictedPath);
+			}
 		} else {
 			await this.createParentDirectories(path);
 		}
@@ -234,6 +238,13 @@ export class FileOperations {
 		}
 	}
 
+	/**
+	 * Deconflicts the given path by appending (1), (2), etc. before the file extension until a non-existent path is found.
+	 * The returned path has a lock acquired on it; it must be released by the caller when no longer needed.
+	 *
+	 * @param path The starting path to deconflict
+	 * @returns a non-existent path with a lock acquired on it
+	 */
 	private async deconflictPath(path: RelativePath): Promise<RelativePath> {
 		// eslint-disable-next-line prefer-const
 		let [directory, fileName] = FileOperations.getParentDirAndFile(path);
@@ -256,11 +267,24 @@ export class FileOperations {
 		stem = stem.replace(FileOperations.PARENTHESES_REGEX, "");
 
 		let newName = path;
-		do {
+
+		while (true) {
 			currentCount++;
 			newName = `${directory}${stem} (${currentCount})${extension}`;
-		} while (await this.fs.exists(newName));
 
-		return newName;
+			// Avoid multiple deconflictPath calls returning the same path
+			if (this.fs.tryLock(newName)) {
+				const newDocument =
+					this.database.getLatestDocumentByRelativePath(newName);
+				if (
+					newDocument?.isDeleted === false || // the document might have been confirmed by the server at a new path but haven't yet moved there locally
+					(await this.fs.exists(newName, true))
+				) {
+					this.fs.unlock(newName);
+				} else {
+					return newName;
+				}
+			}
+		}
 	}
 }

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

@@ -73,9 +73,16 @@ export class SafeFileSystemOperations implements FileSystemOperations {
 		);
 	}
 
-	public async exists(path: RelativePath): Promise<boolean> {
+	public async exists(
+		path: RelativePath,
+		skipLock: boolean = false
+	): Promise<boolean> {
 		this.logger.debug(`Checking if file '${path}' exists`);
-		return this.locks.withLock(path, async () => this.fs.exists(path));
+		if (skipLock) {
+			return this.fs.exists(path);
+		} else {
+			return this.locks.withLock(path, async () => this.fs.exists(path));
+		}
 	}
 
 	public async createDirectory(path: RelativePath): Promise<void> {
@@ -92,19 +99,37 @@ export class SafeFileSystemOperations implements FileSystemOperations {
 
 	public async rename(
 		oldPath: RelativePath,
-		newPath: RelativePath
+		newPath: RelativePath,
+		skipLock: boolean = false
 	): Promise<void> {
 		this.logger.debug(`Renaming file '${oldPath}' to '${newPath}'`);
 		return this.safeOperation(
 			oldPath,
-			async () =>
+			async () => {
-				this.locks.withLock([oldPath, newPath], async () =>
+				if (skipLock) {
-					this.fs.rename(oldPath, newPath)
+					return this.fs.rename(oldPath, newPath);
-				),
+				} else {
+					return this.locks.withLock([oldPath, newPath], async () =>
+						this.fs.rename(oldPath, newPath)
+					);
+				}
+			},
 			"rename"
 		);
 	}
 
+	public tryLock(path: RelativePath): boolean {
+		return this.locks.tryLock(path);
+	}
+
+	public waitForLock(path: RelativePath) {
+		return this.locks.waitForLock(path);
+	}
+
+	public unlock(path: RelativePath) {
+		this.locks.unlock(path);
+	}
+
 	public reset(): void {
 		this.locks.reset();
 	}

frontend/sync-client/src/sync-operations/unrestricted-syncer.ts

@@ -87,15 +87,6 @@ export class UnrestrictedSyncer {
 				contentBytes
 			});
 
-			this.database.updateDocumentMetadata(
-				{
-					parentVersionId: response.vaultUpdateId,
-					hash: contentHash,
-					remoteRelativePath: response.relativePath
-				},
-				document
-			);
-
 			// In case a document with the same name (but different ID) had existed remotely that we haven't known about
 			if (response.relativePath != originalRelativePath) {
 				this.logger.debug(
@@ -107,6 +98,15 @@ export class UnrestrictedSyncer {
 				); // this can throw FileNotFoundError
 			}
 
+			this.database.updateDocumentMetadata(
+				{
+					parentVersionId: response.vaultUpdateId,
+					hash: contentHash,
+					remoteRelativePath: response.relativePath
+				},
+				document
+			);
+
 			this.database.addSeenUpdateId(response.vaultUpdateId);
 			this.updateCache(
 				response.vaultUpdateId,

frontend/sync-client/src/utils/data-structures/locks.ts

@@ -78,7 +78,7 @@ export class Locks<T> {
 	 * @param key The key to lock
 	 * @returns `true` if lock acquired, `false` if already locked
 	 */
-	private tryLock(key: T): boolean {
+	public tryLock(key: T): boolean {
 		if (this.locked.has(key)) {
 			return false;
 		}
@@ -95,7 +95,7 @@ export class Locks<T> {
 	 * @param key The key to wait for and lock
 	 * @returns Promise that resolves when lock is acquired
 	 */
-	private async waitForLock(key: T): Promise<void> {
+	public async waitForLock(key: T): Promise<void> {
 		if (this.tryLock(key)) {
 			return Promise.resolve();
 		}
@@ -121,9 +121,9 @@ export class Locks<T> {
 	 * @param key The key to unlock
 	 * @throws {Error} If key is not currently locked
 	 */
-	private unlock(key: T): void {
+	public unlock(key: T): void {
 		if (!this.locked.has(key)) {
-			throw new Error(`Key '${key}' is not locked, cannot unlock`);
+			return;
 		}
 
 		// Remove first waiter to ensure FIFO order