From 51ce405e352f875580b575ba2caf3bca2d61cb02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karol=20S=C3=B3jko?= Date: Thu, 1 Feb 2024 13:11:12 +0100 Subject: [PATCH] marking items as backed off and retrying with smaller subset --- .../Domain/Sync/SyncBackoffService.spec.ts | 84 ++++++++++++++++--- .../src/Domain/Sync/SyncBackoffService.ts | 24 +++++- .../Sync/SyncBackoffServiceInterface.ts | 5 +- .../services/src/Domain/Sync/SyncOpStatus.ts | 9 ++ .../snjs/lib/Services/Sync/SyncService.ts | 54 ++++++++++-- 5 files changed, 153 insertions(+), 23 deletions(-) diff --git a/packages/services/src/Domain/Sync/SyncBackoffService.spec.ts b/packages/services/src/Domain/Sync/SyncBackoffService.spec.ts index aea7bb3f44d..23ffe4a924e 100644 --- a/packages/services/src/Domain/Sync/SyncBackoffService.spec.ts +++ b/packages/services/src/Domain/Sync/SyncBackoffService.spec.ts @@ -1,21 +1,29 @@ import { AnyItemInterface } from '@standardnotes/models' import { SyncBackoffService } from './SyncBackoffService' +import { InternalEventBusInterface } from '../..' +import { Uuid } from '@standardnotes/domain-core' describe('SyncBackoffService', () => { - const createService = () => new SyncBackoffService() + let internalEventBus: InternalEventBusInterface + + const createService = () => new SyncBackoffService(internalEventBus) + + beforeEach(() => { + internalEventBus = {} as jest.Mocked + }) it('should not be in backoff if no backoff was set', () => { const service = createService() - expect(service.isItemInBackoff({ uuid: '123' } as jest.Mocked)).toBe(false) + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(false) }) it('should be in backoff if backoff was set', () => { const service = createService() - service.backoffItem({ uuid: '123' } as jest.Mocked) + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000000').getValue()) - expect(service.isItemInBackoff({ uuid: '123' } as jest.Mocked)).toBe(true) + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(true) }) it('should not be in backoff if backoff expired', () => { @@ -23,11 +31,11 @@ describe('SyncBackoffService', () => { jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) - service.backoffItem({ uuid: '123' } as jest.Mocked) + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000000').getValue()) jest.spyOn(Date, 'now').mockReturnValueOnce(2_000_000) - expect(service.isItemInBackoff({ uuid: '123' } as jest.Mocked)).toBe(false) + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(false) }) it('should double backoff penalty on each backoff', () => { @@ -35,21 +43,73 @@ describe('SyncBackoffService', () => { jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) - service.backoffItem({ uuid: '123' } as jest.Mocked) + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000000').getValue()) jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) - expect(service.isItemInBackoff({ uuid: '123' } as jest.Mocked)).toBe(true) + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(true) jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) - service.backoffItem({ uuid: '123' } as jest.Mocked) + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000000').getValue()) jest.spyOn(Date, 'now').mockReturnValueOnce(1_001_000) - expect(service.isItemInBackoff({ uuid: '123' } as jest.Mocked)).toBe(true) + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(true) jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) - service.backoffItem({ uuid: '123' } as jest.Mocked) + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000000').getValue()) jest.spyOn(Date, 'now').mockReturnValueOnce(1_003_000) - expect(service.isItemInBackoff({ uuid: '123' } as jest.Mocked)).toBe(true) + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(true) + }) + + it('should remove backoff penalty on successful sync', async () => { + const service = createService() + + jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) + + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000000').getValue()) + + jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(true) + + await service.handleEvent({ + type: 'CompletedIncrementalSync', + payload: { + uploadedPayloads: [ + { + uuid: '00000000-0000-0000-0000-000000000000', + }, + ], + }, + }) + + expect(service.isItemInBackoff({ uuid: '00000000-0000-0000-0000-000000000000' } as jest.Mocked)).toBe(false) + }) + + it('should get a smaller subset of item uuids in backoff that have lesser penalty', () => { + const service = createService() + + jest.spyOn(Date, 'now').mockReturnValueOnce(1_000_000) + + for (let i = 0; i < 5; i++) { + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000000').getValue()) + } + for (let i = 0; i < 4; i++) { + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000001').getValue()) + } + for (let i = 0; i < 3; i++) { + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000002').getValue()) + } + for (let i = 0; i < 2; i++) { + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000003').getValue()) + } + service.backoffItem(Uuid.create('00000000-0000-0000-0000-000000000004').getValue()) + + const subset = service.getSmallerSubsetOfItemUuidsInBackoff() + + expect(subset.length).toEqual(3) + + expect(subset[0].value).toBe('00000000-0000-0000-0000-000000000004') + expect(subset[1].value).toBe('00000000-0000-0000-0000-000000000003') + expect(subset[2].value).toBe('00000000-0000-0000-0000-000000000002') }) }) diff --git a/packages/services/src/Domain/Sync/SyncBackoffService.ts b/packages/services/src/Domain/Sync/SyncBackoffService.ts index 3d77921cd8c..72b848664c6 100644 --- a/packages/services/src/Domain/Sync/SyncBackoffService.ts +++ b/packages/services/src/Domain/Sync/SyncBackoffService.ts @@ -1,4 +1,5 @@ import { AnyItemInterface, ServerSyncPushContextualPayload } from '@standardnotes/models' +import { Uuid } from '@standardnotes/domain-core' import { SyncBackoffServiceInterface } from './SyncBackoffServiceInterface' import { AbstractService } from '../Service/AbstractService' @@ -21,6 +22,21 @@ export class SyncBackoffService this.backoffStartTimestamps = new Map() } + getSmallerSubsetOfItemUuidsInBackoff(): Uuid[] { + const uuids = Array.from(this.backoffPenalties.keys()) + + const uuidsSortedByPenaltyAscending = uuids.sort((a, b) => { + const penaltyA = this.backoffPenalties.get(a) || 0 + const penaltyB = this.backoffPenalties.get(b) || 0 + + return penaltyA - penaltyB + }) + + const halfLength = Math.ceil(uuidsSortedByPenaltyAscending.length / 2) + + return uuidsSortedByPenaltyAscending.slice(0, halfLength).map((uuid) => Uuid.create(uuid).getValue()) + } + async handleEvent(event: InternalEventInterface): Promise { if (event.type === ApplicationEvent.CompletedIncrementalSync) { for (const payload of (event.payload as Record) @@ -47,12 +63,12 @@ export class SyncBackoffService return backoffEndTimestamp > Date.now() } - backoffItem(item: AnyItemInterface): void { - const backoffPenalty = this.backoffPenalties.get(item.uuid) || 0 + backoffItem(itemUuid: Uuid): void { + const backoffPenalty = this.backoffPenalties.get(itemUuid.value) || 0 const newBackoffPenalty = backoffPenalty === 0 ? 1_000 : backoffPenalty * 2 - this.backoffPenalties.set(item.uuid, newBackoffPenalty) - this.backoffStartTimestamps.set(item.uuid, Date.now()) + this.backoffPenalties.set(itemUuid.value, newBackoffPenalty) + this.backoffStartTimestamps.set(itemUuid.value, Date.now()) } } diff --git a/packages/services/src/Domain/Sync/SyncBackoffServiceInterface.ts b/packages/services/src/Domain/Sync/SyncBackoffServiceInterface.ts index 2aab2295cf2..cc4a47c9c90 100644 --- a/packages/services/src/Domain/Sync/SyncBackoffServiceInterface.ts +++ b/packages/services/src/Domain/Sync/SyncBackoffServiceInterface.ts @@ -1,6 +1,9 @@ +import { Uuid } from '@standardnotes/domain-core' + import { AnyItemInterface } from '@standardnotes/models' export interface SyncBackoffServiceInterface { isItemInBackoff(item: AnyItemInterface): boolean - backoffItem(item: AnyItemInterface): void + backoffItem(itemUuid: Uuid): void + getSmallerSubsetOfItemUuidsInBackoff(): Uuid[] } diff --git a/packages/services/src/Domain/Sync/SyncOpStatus.ts b/packages/services/src/Domain/Sync/SyncOpStatus.ts index a6836d3f295..e390303a36a 100644 --- a/packages/services/src/Domain/Sync/SyncOpStatus.ts +++ b/packages/services/src/Domain/Sync/SyncOpStatus.ts @@ -17,6 +17,7 @@ export class SyncOpStatus { private databaseLoadTotal = 0 private databaseLoadDone = false private syncing = false + private isTooLarge = false private syncStart!: Date private timingMonitor?: any @@ -71,6 +72,14 @@ export class SyncOpStatus { this.syncing = false } + setIsTooLarge() { + this.isTooLarge = true + } + + get isTooLargeToSync() { + return this.isTooLarge + } + get syncInProgress() { return this.syncing === true } diff --git a/packages/snjs/lib/Services/Sync/SyncService.ts b/packages/snjs/lib/Services/Sync/SyncService.ts index 0609851e5c3..7afd22463a1 100644 --- a/packages/snjs/lib/Services/Sync/SyncService.ts +++ b/packages/snjs/lib/Services/Sync/SyncService.ts @@ -97,7 +97,7 @@ import { } from '@standardnotes/encryption' import { CreatePayloadFromRawServerItem } from './Account/Utilities' import { DecryptedServerConflictMap, TrustedServerConflictMap } from './Account/ServerConflictMap' -import { ContentType } from '@standardnotes/domain-core' +import { ContentType, Uuid } from '@standardnotes/domain-core' import { SyncFrequencyGuardInterface } from './SyncFrequencyGuardInterface' const DEFAULT_MAJOR_CHANGE_THRESHOLD = 15 @@ -459,7 +459,23 @@ export class SyncService const itemsWithoutBackoffPenalty = dirtyItems.filter((item) => !this.syncBackoffService.isItemInBackoff(item)) - return itemsWithoutBackoffPenalty + const itemsNeedingSync = itemsWithoutBackoffPenalty + + if (itemsWithoutBackoffPenalty.length === 0) { + const subsetOfBackoffUuids = this.syncBackoffService.getSmallerSubsetOfItemUuidsInBackoff() + if (subsetOfBackoffUuids.length > 0) { + this.logger.debug('All items are synced. Attempting to sync a subset of backoff items.') + + subsetOfBackoffUuids.forEach((uuid: Uuid) => { + const item = dirtyItems.find((item) => item.uuid === uuid.value) + if (item) { + itemsNeedingSync.push(item) + } + }) + } + } + + return itemsNeedingSync } public async markAllItemsAsNeedingSyncAndPersist(): Promise { @@ -941,8 +957,17 @@ export class SyncService releaseLock() - const { hasError } = await this.handleSyncOperationFinish(operation, options, neverSyncedDeleted, syncMode) + const { hasError, isPayloadTooLarge } = await this.handleSyncOperationFinish( + operation, + options, + neverSyncedDeleted, + syncMode, + ) if (hasError) { + if (isPayloadTooLarge && operation instanceof AccountSyncOperation) { + this.markTooLargePayloadsAsBackedOff(operation) + } + return } @@ -1023,6 +1048,7 @@ export class SyncService if (response.status === PAYLOAD_TOO_LARGE) { void this.notifyEvent(SyncEvent.PayloadTooLarge) + this.opStatus?.setIsTooLarge() } this.opStatus?.setError(response.error) @@ -1030,6 +1056,22 @@ export class SyncService void this.notifyEvent(SyncEvent.SyncError, response) } + private markTooLargePayloadsAsBackedOff(operation: AccountSyncOperation): void { + for (const payload of operation.payloads) { + this.logger.debug(`Marking item ${payload.uuid} as backed off due to request payload being too large`) + + const uuidOrError = Uuid.create(payload.uuid) + if (uuidOrError.isFailed()) { + this.logger.error('Failed to create uuid from payload', payload) + + continue + } + const uuid = uuidOrError.getValue() + + this.syncBackoffService.backoffItem(uuid) + } + } + private async handleSuccessServerResponse(operation: AccountSyncOperation, response: ServerSyncResponse) { if (this._simulate_latency) { await sleep(this._simulate_latency.latency) @@ -1305,11 +1347,11 @@ export class SyncService options: SyncOptions, neverSyncedDeleted: DeletedItemInterface[], syncMode: SyncMode, - ) { + ): Promise<{ hasError: boolean; isPayloadTooLarge: boolean }> { this.opStatus.setDidEnd() if (this.opStatus.hasError()) { - return { hasError: true } + return { hasError: true, isPayloadTooLarge: this.opStatus.isTooLargeToSync } } this.opStatus.reset() @@ -1332,7 +1374,7 @@ export class SyncService }) } - return { hasError: false } + return { hasError: false, isPayloadTooLarge: false } } private async handleDownloadFirstCompletionAndSyncAgain(online: boolean, options: SyncOptions) {