Isdeleted fix

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

@@ -8,7 +8,6 @@ export interface DocumentMetadata {
 	parentVersionId: VaultUpdateId;
 	documentId: DocumentId;
 	hash: string;
-	isDeleted: boolean;
 }
 
 export interface StoredDocumentMetadata {
@@ -16,7 +15,6 @@ export interface StoredDocumentMetadata {
 	parentVersionId: VaultUpdateId;
 	documentId: DocumentId;
 	hash: string;
-	isDeleted: boolean;
 }
 
 export interface StoredDatabase {
@@ -26,10 +24,11 @@ export interface StoredDatabase {
 
 export interface DocumentRecord {
 	identity: symbol;
-	parallelVersion: number;
 	relativePath: RelativePath;
 	metadata: DocumentMetadata | undefined;
+	isDeleted: boolean;
 	updates: Promise<void>[];
+	parallelVersion: number;
 }
 
 export class Database {
@@ -48,6 +47,7 @@ export class Database {
 				relativePath,
 				identity: Symbol(),
 				metadata,
+				isDeleted: false,
 				updates: [],
 				parallelVersion: 0
 			})) ?? [];
@@ -68,9 +68,7 @@ export class Database {
 	public get resolvedDocuments(): DocumentRecord[] {
 		const paths = new Map<string, DocumentRecord[]>();
 		this.documents
-			.filter(
+			.filter(({ metadata }) => metadata !== undefined)
-				({ metadata }) => metadata !== undefined && !metadata.isDeleted
-			)
 			.forEach((record) =>
 				paths.set(record.relativePath, [
 					record,
@@ -120,62 +118,70 @@ export class Database {
 			documentId,
 			relativePath,
 			parentVersionId,
-			hash,
+			hash
-			isDeleted
 		}: {
 			documentId: DocumentId;
 			relativePath: RelativePath;
 			parentVersionId: VaultUpdateId;
 			hash: string;
-			isDeleted: boolean;
 		},
 		identity?: symbol
 	): void {
 		let entry: DocumentRecord | undefined;
 		if (identity !== undefined) {
-			entry = this.getDocumentByIdentity(identity);
+			const entry = this.getDocumentByIdentity(identity);
 
-			if (entry !== undefined) {
+			this.documents = this.documents.filter(
-				this.documents = this.documents.filter(
+				({ identity }) => identity !== entry.identity
-					({ identity }) => identity !== entry!.identity
+			);
-				);
+
-			}
+			this.documents.push({
-		} else {
+				...entry,
-			entry = this.getLatestDocumentByRelativePath(relativePath);
+				relativePath,
-			if (
+				metadata: {
-				entry?.metadata?.documentId !== undefined &&
+					documentId,
-				entry.metadata.documentId !== documentId
+					parentVersionId,
-			) {
+					hash
-				this.documents.push({
+				}
-					// `entry` might be undefined if the document is new
+			});
-					identity: Symbol(),
+
-					relativePath,
+			this.save();
-					metadata: {
+			return;
-						documentId,
+		}
-						parentVersionId,
+
-						hash,
+		// We find a match based on relative path and we find one with a different document id
-						isDeleted
+		// meaning that two documents occupy the same path in terms of in-flight requests so we
-					},
+		// need to create a new parallel version.
-					updates: [],
+		entry = this.getLatestDocumentByRelativePath(relativePath);
-					parallelVersion: entry?.parallelVersion + 1
+		if (entry && entry.metadata?.documentId !== documentId) {
-				});
+			this.documents.push({
-			}
+				// `entry` might be undefined if the document is new
+				identity: Symbol(),
+				relativePath,
+				metadata: {
+					documentId,
+					parentVersionId,
+					hash
+				},
+				isDeleted: false,
+				updates: [],
+				parallelVersion: entry.parallelVersion + 1
+			});
 			this.save();
 			return;
 		}
 
 		this.documents.push({
-			// `entry` might be undefined if the document is new
+			identity: Symbol(),
-			identity: entry?.identity ?? Symbol(),
 			relativePath,
 			metadata: {
 				documentId,
 				parentVersionId,
-				hash,
+				hash
-				isDeleted
 			},
-			updates: entry?.updates ?? [],
+			isDeleted: false,
-			parallelVersion: entry?.parallelVersion ?? 0
+			updates: [],
+			parallelVersion: 0
 		});
 
 		this.save();
@@ -208,6 +214,7 @@ export class Database {
 				relativePath,
 				identity: Symbol(),
 				metadata: undefined,
+				isDeleted: false,
 				updates: [],
 				parallelVersion: 0
 			};
@@ -257,10 +264,9 @@ export class Database {
 		const oldDocument =
 			this.getLatestDocumentByRelativePath(oldRelativePath);
 		if (oldDocument === undefined) {
+			// We can try moving a non-existent document if it hasn't yet got created becasue it's
+			// the result of an offline event while this move happens online before.
 			return;
-			throw new Error(
-				`Document to be moved not found: ${oldRelativePath}`
-			);
 		}
 
 		this.documents = this.documents.filter(
@@ -274,6 +280,7 @@ export class Database {
 			identity: oldDocument.identity,
 			metadata: oldDocument.metadata,
 			relativePath: newRelativePath,
+			isDeleted: oldDocument.isDeleted,
 			updates: oldDocument.updates,
 			// We're in a strange state where the target of the move has just got deleted,
 			// however, its metadata might already have a bunch of updates queued up for
@@ -285,6 +292,15 @@ export class Database {
 		this.save();
 	}
 
+	public delete(relativePath: RelativePath): void {
+		const candidate = this.getLatestDocumentByRelativePath(relativePath);
+		if (candidate === undefined) {
+			// it's fine because the document to be deleted might not have been created yet
+			return;
+		}
+		candidate.isDeleted = true;
+	}
+
 	private save(): void {
 		this.logger.debug(JSON.stringify(this.documents, null, 2));
 

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

@@ -126,6 +126,8 @@ export class Syncer {
 			return;
 		}
 
+		this.database.delete(relativePath);
+
 		const [promise, resolve, reject] = createPromise();
 
 		await this.database.getResolvedDocumentByRelativePath(

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

@@ -34,52 +34,57 @@ export class UnrestrictedSyncer {
 		getLatestDocument: () => DocumentRecord,
 		updateTime?: Date
 	): Promise<void> {
-		const { relativePath, metadata } = getLatestDocument();
+		let latestDocument = getLatestDocument();
 
 		return this.executeSync(
-			[relativePath],
+			[latestDocument.relativePath],
 			SyncType.CREATE,
 			SyncSource.PUSH,
 			async () => {
-				if (metadata !== undefined && !metadata.isDeleted) {
+				if (
+					latestDocument.metadata !== undefined &&
+					!latestDocument.isDeleted
+				) {
 					this.logger.debug(
-						`Document ${relativePath} already exists in the database, no need to create it again`
+						`Document ${latestDocument.relativePath} already exists in the database, no need to create it again`
 					);
 					return;
 				}
 
-				const contentBytes = await this.operations.read(relativePath); // this can throw FileNotFoundError
+				const contentBytes = await this.operations.read(
+					latestDocument.relativePath
+				); // this can throw FileNotFoundError
 				const contentHash = hash(contentBytes);
 
-				updateTime ??=
+				updateTime ??= await this.operations.getModificationTime(
-					await this.operations.getModificationTime(relativePath); // this can throw FileNotFoundError
+					latestDocument.relativePath
+				); // this can throw FileNotFoundError
 
 				const response = await this.syncService.create({
-					relativePath,
+					relativePath: latestDocument.relativePath,
 					contentBytes,
 					createdDate: updateTime
 				});
 
-				const { relativePath: currentRelativePath, identity } =
+				latestDocument = getLatestDocument();
-					getLatestDocument();
 
 				this.history.addHistoryEntry({
 					status: SyncStatus.SUCCESS,
 					source: SyncSource.PUSH,
-					relativePath,
+					relativePath: latestDocument.relativePath,
 					message: `Successfully uploaded locally created file`,
 					type: SyncType.CREATE
 				});
 
 				const newMetadata = {
-					relativePath: currentRelativePath,
+					relativePath: latestDocument.relativePath,
 					documentId: response.documentId,
 					parentVersionId: response.vaultUpdateId,
 					hash: contentHash,
 					isDeleted: false
 				};
 
-				this.database.setDocument(newMetadata, identity);
+				this.database.setDocument(newMetadata, latestDocument.identity);
 
 				this.tryIncrementVaultUpdateId(response.vaultUpdateId);
 			}
@@ -95,12 +100,9 @@ export class UnrestrictedSyncer {
 			SyncType.DELETE,
 			SyncSource.PUSH,
 			async () => {
-				if (
+				if (document.metadata === undefined) {
-					document.metadata === undefined ||
-					document.metadata.isDeleted
-				) {
 					this.logger.debug(
-						`Document '${document.relativePath}' has been already deleted, no need to delete it again`
+						`Document '${document.relativePath}' has been created yet so deleting it remotely can be skipped`
 					);
 					return;
 				}
@@ -128,8 +130,7 @@ export class UnrestrictedSyncer {
 						relativePath: document.relativePath,
 						documentId: response.documentId,
 						parentVersionId: response.vaultUpdateId,
-						hash: EMPTY_HASH,
+						hash: EMPTY_HASH
-						isDeleted: true
 					},
 					document.identity
 				);
@@ -155,12 +156,9 @@ export class UnrestrictedSyncer {
 			SyncType.UPDATE,
 			SyncSource.PUSH,
 			async () => {
-				if (
+				if (document.metadata === undefined || document.isDeleted) {
-					document.metadata === undefined ||
-					document.metadata.isDeleted
-				) {
 					this.logger.debug(
-						`Document ${document.relativePath} has been already deleted, no need to update it, ${JSON.stringify(document)}, ${document.metadata?.isDeleted}`
+						`Document ${document.relativePath} has been already deleted, no need to update it`
 					);
 					return;
 				}
@@ -192,6 +190,22 @@ export class UnrestrictedSyncer {
 					createdDate: updateTime
 				});
 
+				// Update relativePath which is the only property that can change while this is running (due to a move)
+				document = getLatestDocument();
+
+				if (document.isDeleted) {
+					this.logger.info(
+						`Document ${document.relativePath} has been deleted before we could finish updating it`
+					);
+					return;
+				}
+
+				if (!document.metadata) {
+					throw new Error(
+						`Document ${document.relativePath} no longer has metadata after updating it`
+					);
+				}
+
 				if (
 					document.metadata.parentVersionId >= response.vaultUpdateId
 				) {
@@ -209,9 +223,6 @@ export class UnrestrictedSyncer {
 					type: SyncType.UPDATE
 				});
 
-				// Update relativePath which is the only property that can change while this is running (due to a move)
-				document = getLatestDocument();
-
 				if (response.isDeleted) {
 					await this.operations.delete(document.relativePath);
 
@@ -224,13 +235,13 @@ export class UnrestrictedSyncer {
 						type: SyncType.DELETE
 					});
 
+					this.database.delete(document.relativePath);
 					this.database.setDocument(
 						{
 							documentId: response.documentId,
 							relativePath: document.relativePath,
 							parentVersionId: response.vaultUpdateId,
-							hash: EMPTY_HASH,
+							hash: EMPTY_HASH
-							isDeleted: true
 						},
 						document.identity
 					);
@@ -267,6 +278,8 @@ export class UnrestrictedSyncer {
 					});
 				}
 
+				document = getLatestDocument();
+
 				this.database.setDocument(
 					{
 						documentId: response.documentId,
@@ -275,8 +288,7 @@ export class UnrestrictedSyncer {
 								? response.relativePath
 								: document.relativePath,
 						parentVersionId: response.vaultUpdateId,
-						hash: contentHash,
+						hash: contentHash
-						isDeleted: response.isDeleted
 					},
 					document.identity
 				);
@@ -321,6 +333,8 @@ export class UnrestrictedSyncer {
 							)
 					});
 				} else if (remoteVersion.isDeleted) {
+					// Either the doc hasn't made it to us before and therefore we don't need to delete it,
+					// or we already have it, in which case the preceeding if will deal with it
 					this.logger.debug(
 						`Document ${remoteVersion.relativePath} has been deleted remotely, no need to sync`
 					);
@@ -332,6 +346,20 @@ export class UnrestrictedSyncer {
 						documentId: remoteVersion.documentId
 					})
 				).contentBase64;
+
+				const latestDocument =
+					getLatestDocument?.() ??
+					this.database.getDocumentByDocumentId(
+						remoteVersion.documentId
+					);
+
+				if (latestDocument?.isDeleted) {
+					this.logger.info(
+						`Document ${remoteVersion.relativePath} has been deleted locally before we could finish updating it`
+					);
+					return;
+				}
+
 				const contentBytes = deserialize(content);
 
 				await this.operations.create(
@@ -345,16 +373,9 @@ export class UnrestrictedSyncer {
 						documentId: remoteVersion.documentId,
 						relativePath: remoteVersion.relativePath,
 						parentVersionId: remoteVersion.vaultUpdateId,
-						hash: hash(contentBytes),
+						hash: hash(contentBytes)
-						isDeleted: remoteVersion.isDeleted
 					},
-					getLatestDocument?.()?.identity ??
+					latestDocument?.identity
-						this.database.getDocumentByDocumentId(
-							remoteVersion.documentId
-						)?.identity ??
-						this.database.getLatestDocumentByRelativePath(
-							remoteVersion.relativePath
-						)?.identity
 				);
 
 				this.history.addHistoryEntry({

frontend/test-client/run.sh

@@ -41,6 +41,8 @@ print_failed_log() {
     return 1
 }
 
+echo "Monitoring $process_count processes"
+
 # Monitor processes
 while true; do
     if print_failed_log; then

frontend/test-client/src/cli.ts

@@ -96,19 +96,20 @@ async function runTests(): Promise<void> {
 	const iterations = [50, 200];
 	const doDeletes = [true, false];
 
-	for (const agentCount of agentCounts) {
+	for (let i = 0; i < 10; i++) {
-		for (const concurrency of concurrencies) {
+		for (const agentCount of agentCounts) {
-			for (const jitter of jitterScaleInSeconds) {
+			for (const concurrency of concurrencies) {
-				for (const iteration of iterations) {
+				for (const jitter of jitterScaleInSeconds) {
-					for (const deleteFiles of doDeletes) {
+					for (const iteration of iterations) {
-						await runTest({
+						for (const deleteFiles of doDeletes) {
-							agentCount,
+							await runTest({
-							concurrency,
+								agentCount,
-							iterations: iteration,
+								concurrency,
-							doDeletes: deleteFiles,
+								iterations: iteration,
-							jitterScaleInSeconds: jitter
+								doDeletes: deleteFiles,
-						});
+								jitterScaleInSeconds: jitter
-						return;
+							});
+						}
 					}
 				}
 			}