From 67532f5d0c08ce6c01c26dba77f1377cd7c2ea03 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Wed, 12 Mar 2025 21:16:40 +0000 Subject: [PATCH] Isdeleted fix --- .../sync-client/src/persistence/database.ts | 104 ++++++++++-------- .../sync-client/src/sync-operations/syncer.ts | 2 + .../sync-operations/unrestricted-syncer.ts | 103 ++++++++++------- frontend/test-client/run.sh | 2 + frontend/test-client/src/cli.ts | 27 ++--- 5 files changed, 140 insertions(+), 98 deletions(-) diff --git a/frontend/sync-client/src/persistence/database.ts b/frontend/sync-client/src/persistence/database.ts index 0bbbd5b1..4418f55c 100644 --- a/frontend/sync-client/src/persistence/database.ts +++ b/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[]; + 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(); this.documents - .filter( - ({ metadata }) => metadata !== undefined && !metadata.isDeleted - ) + .filter(({ metadata }) => metadata !== undefined) .forEach((record) => paths.set(record.relativePath, [ record, @@ -120,62 +118,70 @@ export class Database { documentId, relativePath, parentVersionId, - hash, - isDeleted + hash }: { 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( - ({ identity }) => identity !== entry!.identity - ); - } - } else { - entry = this.getLatestDocumentByRelativePath(relativePath); - if ( - entry?.metadata?.documentId !== undefined && - entry.metadata.documentId !== documentId - ) { - this.documents.push({ - // `entry` might be undefined if the document is new - identity: Symbol(), - relativePath, - metadata: { - documentId, - parentVersionId, - hash, - isDeleted - }, - updates: [], - parallelVersion: entry?.parallelVersion + 1 - }); - } + this.documents = this.documents.filter( + ({ identity }) => identity !== entry.identity + ); + + this.documents.push({ + ...entry, + relativePath, + metadata: { + documentId, + parentVersionId, + hash + } + }); + + this.save(); + return; + } + + // We find a match based on relative path and we find one with a different document id + // meaning that two documents occupy the same path in terms of in-flight requests so we + // need to create a new parallel version. + entry = this.getLatestDocumentByRelativePath(relativePath); + 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: entry?.identity ?? Symbol(), + identity: Symbol(), relativePath, metadata: { documentId, parentVersionId, - hash, - isDeleted + hash }, - updates: entry?.updates ?? [], - parallelVersion: entry?.parallelVersion ?? 0 + isDeleted: false, + 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)); diff --git a/frontend/sync-client/src/sync-operations/syncer.ts b/frontend/sync-client/src/sync-operations/syncer.ts index e61e7c45..531e735a 100644 --- a/frontend/sync-client/src/sync-operations/syncer.ts +++ b/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( diff --git a/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts b/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts index 109cc673..5e4cf754 100644 --- a/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts +++ b/frontend/sync-client/src/sync-operations/unrestricted-syncer.ts @@ -34,52 +34,57 @@ export class UnrestrictedSyncer { getLatestDocument: () => DocumentRecord, updateTime?: Date ): Promise { - 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 ??= - await this.operations.getModificationTime(relativePath); // this can throw FileNotFoundError + updateTime ??= await this.operations.getModificationTime( + latestDocument.relativePath + ); // this can throw FileNotFoundError const response = await this.syncService.create({ - relativePath, + relativePath: latestDocument.relativePath, contentBytes, createdDate: updateTime }); - const { relativePath: currentRelativePath, identity } = - getLatestDocument(); + latestDocument = 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 ( - document.metadata === undefined || - document.metadata.isDeleted - ) { + if (document.metadata === undefined) { 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, - isDeleted: true + hash: EMPTY_HASH }, document.identity ); @@ -155,12 +156,9 @@ export class UnrestrictedSyncer { SyncType.UPDATE, SyncSource.PUSH, async () => { - if ( - document.metadata === undefined || - document.metadata.isDeleted - ) { + if (document.metadata === undefined || document.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, - isDeleted: true + hash: EMPTY_HASH }, 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, - isDeleted: response.isDeleted + hash: contentHash }, 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), - isDeleted: remoteVersion.isDeleted + hash: hash(contentBytes) }, - getLatestDocument?.()?.identity ?? - this.database.getDocumentByDocumentId( - remoteVersion.documentId - )?.identity ?? - this.database.getLatestDocumentByRelativePath( - remoteVersion.relativePath - )?.identity + latestDocument?.identity ); this.history.addHistoryEntry({ diff --git a/frontend/test-client/run.sh b/frontend/test-client/run.sh index 5bf19a63..5bb39e94 100755 --- a/frontend/test-client/run.sh +++ b/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 diff --git a/frontend/test-client/src/cli.ts b/frontend/test-client/src/cli.ts index e87666f3..27c59f75 100644 --- a/frontend/test-client/src/cli.ts +++ b/frontend/test-client/src/cli.ts @@ -96,19 +96,20 @@ async function runTests(): Promise { const iterations = [50, 200]; const doDeletes = [true, false]; - for (const agentCount of agentCounts) { - for (const concurrency of concurrencies) { - for (const jitter of jitterScaleInSeconds) { - for (const iteration of iterations) { - for (const deleteFiles of doDeletes) { - await runTest({ - agentCount, - concurrency, - iterations: iteration, - doDeletes: deleteFiles, - jitterScaleInSeconds: jitter - }); - return; + for (let i = 0; i < 10; i++) { + for (const agentCount of agentCounts) { + for (const concurrency of concurrencies) { + for (const jitter of jitterScaleInSeconds) { + for (const iteration of iterations) { + for (const deleteFiles of doDeletes) { + await runTest({ + agentCount, + concurrency, + iterations: iteration, + doDeletes: deleteFiles, + jitterScaleInSeconds: jitter + }); + } } } }