Fix document merging logic

frontend/sync-client/src/sync-operations/cursor-tracker.ts

@@ -10,6 +10,7 @@ import { hash } from "../utils/hash";
 import type { FileChangeNotifier } from "./file-change-notifier";
 import { Lock } from "../utils/data-structures/locks";
 import { EventListeners } from "../utils/data-structures/event-listeners";
+import { Logger } from "../tracing/logger";
 
 // Cursor positions are updated separately from documents. However, a given cursor position is only
 // valid within a certain version of the document it belongs to. This class tracks previous and the latest
@@ -22,7 +23,7 @@ export class CursorTracker {
         (cursors: MaybeOutdatedClientCursors[]) => unknown
     >();
 
-    private readonly updateLock = new Lock(CursorTracker.name);
+    private readonly updateLock: Lock;
 
     private knownRemoteCursors: (ClientCursors & {
         upToDateness: DocumentUpToDateness;
@@ -33,11 +34,14 @@ export class CursorTracker {
         [];
 
     public constructor(
+        private readonly logger: Logger,
         private readonly database: Database,
         private readonly webSocketManager: WebSocketManager,
         private readonly fileOperations: FileOperations,
         private readonly fileChangeNotifier: FileChangeNotifier
     ) {
+        this.updateLock = new Lock(CursorTracker.name, logger);
+
         this.webSocketManager.onRemoteCursorsUpdateReceived.add(
             async (clientCursors) => {
                 await this.updateLock.withLock(async () => {

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

@@ -48,6 +48,7 @@ export class Syncer {
         });
 
         this.updatedDocumentsByPathAndKeysLocks = new Locks<DocumentId>(
+            Syncer.name,
             this.logger
         );
 
@@ -88,6 +89,7 @@ export class Syncer {
     public async syncLocallyCreatedFile(
         relativePath: RelativePath
     ): Promise<void> {
+        // check whether someone else has already created the document in the database
         if (
             this.database.getLatestDocumentByRelativePath(relativePath)
                 ?.isDeleted === false
@@ -148,6 +150,24 @@ export class Syncer {
         oldPath?: RelativePath;
         relativePath: RelativePath;
     }): Promise<void> {
+        const document =
+            this.database.getLatestDocumentByRelativePath(oldPath ?? relativePath);
+
+        // must have been removed after a successful delete
+        if (document === undefined) {
+            this.logger.debug(
+                `Cannot find document ${relativePath} in the database, skipping`
+            );
+            return;
+        }
+
+        if (document.isDeleted) {
+            this.logger.debug(
+                `Document ${relativePath} has been deleted locally, skipping`
+            );
+            return;
+        }
+
         const documentAtNewPath =
             this.database.getLatestDocumentByRelativePath(relativePath);
 
@@ -168,8 +188,6 @@ export class Syncer {
             }
         }
 
-        const document =
-            this.database.getLatestDocumentByRelativePath(relativePath);
 
         if (
             oldPath !== undefined &&
@@ -181,21 +199,6 @@ export class Syncer {
             return;
         }
 
-        // must have been removed after a successful delete
-        if (document === undefined) {
-            this.logger.debug(
-                `Cannot find document ${relativePath} in the database, skipping`
-            );
-            return;
-        }
-
-        if (document.isDeleted) {
-            this.logger.debug(
-                `Document ${relativePath} has been deleted locally, skipping`
-            );
-            return;
-        }
-
         await this.enqueueSyncOperation(
             async () =>
                 this.unrestrictedSyncer.unrestrictedSyncLocallyCreatedOrUpdatedFile(
@@ -448,7 +451,7 @@ export class Syncer {
 
     private async enqueueSyncOperation<T>(
         operation: () => Promise<T>,
-        keys: (DocumentId | undefined | null)[]
+        keys: (string | undefined | null)[]
     ): Promise<T> {
         return this.updatedDocumentsByPathAndKeysLocks.withLock(
             keys.filter((k) => k !== undefined && k !== null),

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

@@ -473,7 +473,6 @@ export class UnrestrictedSyncer {
         }
 
         let actualPath = document.relativePath;
-        let mustCreate = false;
 
         if (isCreate) {
             // We have a file locally that got moved by another client to the same path as the one we're trying to create.
@@ -485,16 +484,16 @@ export class UnrestrictedSyncer {
             );
             if (existingDocument !== undefined) {
                 this.logger.info(
-                    `Merging document ${existingDocument.relativePath} into existing document ${document.relativePath
+                    `Merging existing document ${existingDocument.relativePath} into ${document.relativePath
                     } after concurrent move & creation`
                 );
-                this.database.removeDocument(document); // this was a (fake) pending document
                 if (!existingDocument.isDeleted) {
                     this.database.delete(existingDocument.relativePath); // make sure syncLocallyDeletedFile doesn't actually schedule deleting the new file
-                    await this.operations.delete(existingDocument.relativePath);
+                    this.database.removeDocument(existingDocument);
+                    await this.operations.move(existingDocument.relativePath, document.relativePath);
+                } else {
+                    this.database.removeDocument(existingDocument);
                 }
-                mustCreate = true;
-                document = existingDocument;
             }
         }
 
@@ -516,37 +515,21 @@ export class UnrestrictedSyncer {
             const responseBytes = base64ToBytes(response.contentBase64);
             contentHash = hash(responseBytes);
 
+            this.database.updateDocumentMetadata(
+                {
+                    documentId: response.documentId,
+                    parentVersionId: response.vaultUpdateId,
+                    hash: contentHash,
+                    remoteRelativePath: response.relativePath
+                },
+                document
+            );
 
-
-            if (mustCreate) {
-                this.database.createNewPendingDocument(actualPath);
-                this.database.updateDocumentMetadata(
-                    {
-                        documentId: response.documentId,
-                        parentVersionId: response.vaultUpdateId,
-                        hash: contentHash,
-                        remoteRelativePath: response.relativePath
-                    },
-                    document
-                );
-
-                await this.operations.create(actualPath, responseBytes);
-            } else {
-                this.database.updateDocumentMetadata(
-                    {
-                        documentId: response.documentId,
-                        parentVersionId: response.vaultUpdateId,
-                        hash: contentHash,
-                        remoteRelativePath: response.relativePath
-                    },
-                    document
-                );
-                await this.operations.write(
-                    actualPath,
-                    originalContentBytes,
-                    responseBytes
-                );
-            }
+            await this.operations.write(
+                actualPath,
+                originalContentBytes,
+                responseBytes
+            );
             await this.updateCache(
                 response.vaultUpdateId,
                 responseBytes,