From 10bde4bc3a86b5f9c64cf6d7b3e30f78b2400e6e Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sat, 29 Nov 2025 17:28:03 +0000 Subject: [PATCH] Fix race condition of client-side path deconflicting --- .../src/file-operations/file-operations.ts | 40 +++++++++++++++---- .../safe-filesystem-operations.ts | 39 ++++++++++++++---- .../sync-operations/unrestricted-syncer.ts | 18 ++++----- .../src/utils/data-structures/locks.ts | 8 ++-- 4 files changed, 77 insertions(+), 28 deletions(-) diff --git a/frontend/sync-client/src/file-operations/file-operations.ts b/frontend/sync-client/src/file-operations/file-operations.ts index 42409227..8f39ff69 100644 --- a/frontend/sync-client/src/file-operations/file-operations.ts +++ b/frontend/sync-client/src/file-operations/file-operations.ts @@ -61,12 +61,16 @@ export class FileOperations { public async ensureClearPath(path: RelativePath): Promise { 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}'` - ); + try { + this.logger.debug( + `Didn't expect ${path} to exist, deconflicting by moving it to '${deconflictedPath}'` + ); - this.database.move(path, deconflictedPath); - await this.fs.rename(path, deconflictedPath); + this.database.move(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 { // 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; + } + } + } } } diff --git a/frontend/sync-client/src/file-operations/safe-filesystem-operations.ts b/frontend/sync-client/src/file-operations/safe-filesystem-operations.ts index 72aa158d..add07b74 100644 --- a/frontend/sync-client/src/file-operations/safe-filesystem-operations.ts +++ b/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 { + public async exists( + path: RelativePath, + skipLock: boolean = false + ): Promise { 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 { @@ -92,19 +99,37 @@ export class SafeFileSystemOperations implements FileSystemOperations { public async rename( oldPath: RelativePath, - newPath: RelativePath + newPath: RelativePath, + skipLock: boolean = false ): Promise { this.logger.debug(`Renaming file '${oldPath}' to '${newPath}'`); return this.safeOperation( oldPath, - async () => - this.locks.withLock([oldPath, newPath], async () => - this.fs.rename(oldPath, newPath) - ), + async () => { + if (skipLock) { + 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(); } diff --git a/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts b/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts index cf94c48a..ebbb076f 100644 --- a/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts +++ b/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, diff --git a/frontend/sync-client/src/utils/data-structures/locks.ts b/frontend/sync-client/src/utils/data-structures/locks.ts index c2e7d73a..fccccf8c 100644 --- a/frontend/sync-client/src/utils/data-structures/locks.ts +++ b/frontend/sync-client/src/utils/data-structures/locks.ts @@ -78,7 +78,7 @@ export class Locks { * @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 { * @param key The key to wait for and lock * @returns Promise that resolves when lock is acquired */ - private async waitForLock(key: T): Promise { + public async waitForLock(key: T): Promise { if (this.tryLock(key)) { return Promise.resolve(); } @@ -121,9 +121,9 @@ export class Locks { * @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