diff --git a/frontend/sync-client/src/file-operations/file-operations.ts b/frontend/sync-client/src/file-operations/file-operations.ts index d93cedea..99863593 100644 --- a/frontend/sync-client/src/file-operations/file-operations.ts +++ b/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 { 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 { 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); } diff --git a/frontend/sync-client/src/persistence/database.ts b/frontend/sync-client/src/persistence/database.ts index 495c9ccd..1ac899e9 100644 --- a/frontend/sync-client/src/persistence/database.ts +++ b/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 { + this.documents.delete(relativePath); + await this.save(); + } + public async updatePath( oldRelativePath: RelativePath, newRelativePath: RelativePath diff --git a/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts b/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts index 2ddc29f3..86ed3089 100644 --- a/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts +++ b/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 diff --git a/frontend/test-client/src/agent/mock-agent.ts b/frontend/test-client/src/agent/mock-agent.ts index 3bc1da03..b7028593 100644 --- a/frontend/test-client/src/agent/mock-agent.ts +++ b/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 { - assert( - this.doDeletes, - `Agent ${this.name} tried to delete file ${path} while doDeletes is false` - ); - - await super.delete(path); - } - public async act(): Promise { const options: (() => Promise)[] = [ this.createFileAction.bind(this), @@ -97,8 +89,8 @@ export class MockAgent extends MockClient { } public async finish(): Promise { - 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 { 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; }