Fix correctness issues

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

@@ -1,12 +1,16 @@
 import type { Logger } from "src/tracing/logger";
 import type { FileSystemOperations } from "./filesystem-operations";
-import type { Database, RelativePath } from "src/persistence/database";
+import type {
+	Database,
+	DocumentId,
+	RelativePath
+} from "src/persistence/database";
 import { isBinary, isFileTypeMergable, mergeText } from "sync_lib";
 import { SafeFileSystemOperations } from "./safe-filesystem-operations";
 
 export class FileOperations {
-	private readonly fs: SafeFileSystemOperations;
 	private static readonly PARENTHESES_REGEX = / \((\d+)\)$/;
+	private readonly fs: SafeFileSystemOperations;
 
 	public constructor(
 		private readonly logger: Logger,
@@ -57,14 +61,16 @@ export class FileOperations {
 		newContent: Uint8Array
 	): Promise<void> {
 		if (await this.fs.exists(path)) {
+			const deconflictedPath = await this.deconflictPath(path);
 			this.logger.debug(
-				`Didn't expect ${path} to exist, when trying to create it, merging instead`
+				`Didn't expect ${path} to exist, deconflicting by moving it to '${deconflictedPath}'`
 			);
-			await this.write(path, new Uint8Array(0), newContent);
-			return;
+			await this.database.updatePath(path, deconflictedPath);
+			await this.fs.rename(path, deconflictedPath);
+		} else {
+			await this.createParentDirectories(path);
 		}
 
-		await this.createParentDirectories(path);
 		await this.fs.write(path, newContent);
 	}
 
@@ -127,7 +133,8 @@ export class FileOperations {
 
 	public async move(
 		oldPath: RelativePath,
-		newPath: RelativePath
+		newPath: RelativePath,
+		documentId?: DocumentId
 	): Promise<void> {
 		if (oldPath === newPath) {
 			return;
@@ -136,10 +143,20 @@ export class FileOperations {
 		if (await this.fs.exists(newPath)) {
 			const deconflictedPath = await this.deconflictPath(newPath);
 			this.logger.debug(
-				`Conflict when moving '${oldPath}' to '${newPath}', latter already exists, deconflicting by moving it to '${deconflictedPath}'`
+				`Conflict when moving '${oldPath}' to '${newPath}', the latter already exists, deconflicting by moving it to '${deconflictedPath}'`
 			);
-			await this.database.updatePath(newPath, deconflictedPath);
-			await this.fs.rename(newPath, 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);
+				await this.fs.delete(newPath);
+			}
 		} else {
 			await this.createParentDirectories(newPath);
 		}

frontend/sync-client/src/persistence/database.ts

@@ -99,6 +99,11 @@ export class Database {
 		return this.documents.get(relativePath);
 	}
 
+	public async deleteDocument(relativePath: RelativePath): Promise<void> {
+		this.documents.delete(relativePath);
+		await this.save();
+	}
+
 	public async updatePath(
 		oldRelativePath: RelativePath,
 		newRelativePath: RelativePath

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

@@ -144,17 +144,17 @@ export class UnrestrictedSyncer {
 			SyncType.UPDATE,
 			SyncSource.PUSH,
 			async () => {
-				const localMetadata = this.database.getDocument(
-					oldPath ?? relativePath
-				);
+				// Check the new path first in case the metadata has been already moved
+				let localMetadata = this.database.getDocument(relativePath);
+				let metadataPath = relativePath;
+
+				if (localMetadata === undefined && oldPath !== undefined) {
+					localMetadata = this.database.getDocument(oldPath);
+					metadataPath = oldPath;
+				}
 
 				if (!localMetadata) {
-					this.history.addHistoryEntry({
-						status: SyncStatus.NO_OP,
-						relativePath,
-						message: `Document metadata doesn't exist for ${oldPath ?? relativePath}, it must have been already deleted`,
-						type: SyncType.UPDATE
-					});
+					// It's fine, a subsequent sync operation must have dealt with this
 					return;
 				}
 
@@ -243,7 +243,8 @@ export class UnrestrictedSyncer {
 						await this.operations.move(
 							// this can throw FileNotFoundError
 							relativePath,
-							response.relativePath
+							response.relativePath,
+							response.documentId
 						);
 					}
 
@@ -268,9 +269,14 @@ export class UnrestrictedSyncer {
 						});
 					}
 
-					await this.database.moveDocument({
+					if (metadataPath !== response.relativePath) {
+						await this.database.updatePath(
+							metadataPath,
+							response.relativePath
+						);
+					}
+					await this.database.setDocument({
 						documentId: localMetadata.documentId,
-						oldRelativePath: oldPath ?? relativePath,
 						relativePath: response.relativePath,
 						parentVersionId: response.vaultUpdateId,
 						hash: contentHash
@@ -394,7 +400,7 @@ export class UnrestrictedSyncer {
 
 				const [relativePath, metadata] = localMetadata;
 
-				if (metadata.parentVersionId === remoteVersion.vaultUpdateId) {
+				if (remoteVersion.vaultUpdateId <= metadata.parentVersionId) {
 					this.logger.debug(
 						`Document ${relativePath} is already up to date`
 					);
@@ -438,6 +444,12 @@ export class UnrestrictedSyncer {
 							// TODO: this can fail, that's bad
 							await this.operations.move(
 								// this can throw FileNotFoundError
+								relativePath,
+								remoteVersion.relativePath,
+								remoteVersion.documentId
+							);
+
+							await this.database.updatePath(
 								relativePath,
 								remoteVersion.relativePath
 							);
@@ -448,9 +460,8 @@ export class UnrestrictedSyncer {
 							currentContent,
 							contentBytes
 						);
-						await this.database.moveDocument({
+						await this.database.setDocument({
 							documentId: remoteVersion.documentId,
-							oldRelativePath: relativePath,
 							relativePath: remoteVersion.relativePath,
 							parentVersionId: remoteVersion.vaultUpdateId,
 							hash: contentHash

frontend/test-client/src/agent/mock-agent.ts

@@ -33,6 +33,7 @@ export class MockAgent extends MockClient {
 					console.error(formatted);
 					// Let's not ignore errors
 					process.exit(1);
+					break;
 				case LogLevel.WARNING:
 					console.warn(formatted);
 					break;
@@ -48,15 +49,6 @@ export class MockAgent extends MockClient {
 		this.client.logger.info("Agent initialized");
 	}
 
-	public async delete(path: RelativePath): Promise<void> {
-		assert(
-			this.doDeletes,
-			`Agent ${this.name} tried to delete file ${path} while doDeletes is false`
-		);
-
-		await super.delete(path);
-	}
-
 	public async act(): Promise<void> {
 		const options: (() => Promise<unknown>)[] = [
 			this.createFileAction.bind(this),
@@ -97,8 +89,8 @@ export class MockAgent extends MockClient {
 	}
 
 	public async finish(): Promise<void> {
-		await Promise.all(this.pendingActions);
 		await this.client.settings.setSetting("isSyncEnabled", true);
+		await Promise.all(this.pendingActions);
 		this.client.stop();
 		await this.client.syncer.waitForSyncQueue();
 		await this.client.syncer.applyRemoteChangesLocally();
@@ -196,7 +188,7 @@ export class MockAgent extends MockClient {
 	private async createFileAction(): Promise<void> {
 		const file = this.getFileName();
 
-		if (await this.exists(file)) {
+		if (this.doNotTouch.includes(file) || (await this.exists(file))) {
 			return;
 		}
 
@@ -246,7 +238,7 @@ export class MockAgent extends MockClient {
 
 		const newName = this.getFileName();
 
-		if (await this.exists(newName)) {
+		if (this.doNotTouch.includes(newName) || (await this.exists(newName))) {
 			return;
 		}