From 888e2031a702f3e12c61b666aa87fa8fc62d3cd4 Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Tue, 27 May 2025 08:24:53 -0500 Subject: [PATCH] [PM-21090] Vault - Repeated Syncs (#14740) * move `fullSync` contents to private methods in prep to storing the respective promise * store in-flight sync so multiple calls to the sync service are avoided * Revert "store in-flight sync so multiple calls to the sync service are avoided" This reverts commit 233c8e9d4b92d448f63e157b01c168d840c7e531. * Revert "move `fullSync` contents to private methods in prep to storing the respective promise" This reverts commit 3f686ac6a4b41e332180478f269d9fde85f562a1. * store inflight API calls for sync service - This avoids duplicate network requests in a relatively short amount of time but still allows consumers to call `fullSync` if needed * add debug log for duplicate sync --- .../sync/default-sync.service.spec.ts | 65 +++++++++++++++---- .../src/platform/sync/default-sync.service.ts | 32 ++++++++- 2 files changed, 82 insertions(+), 15 deletions(-) diff --git a/libs/common/src/platform/sync/default-sync.service.spec.ts b/libs/common/src/platform/sync/default-sync.service.spec.ts index 29543672dbe..fc6b9481bd5 100644 --- a/libs/common/src/platform/sync/default-sync.service.spec.ts +++ b/libs/common/src/platform/sync/default-sync.service.spec.ts @@ -130,23 +130,23 @@ describe("DefaultSyncService", () => { const user1 = "user1" as UserId; + const emptySyncResponse = new SyncResponse({ + profile: { + id: user1, + }, + folders: [], + collections: [], + ciphers: [], + sends: [], + domains: [], + policies: [], + }); + describe("fullSync", () => { beforeEach(() => { accountService.activeAccount$ = of({ id: user1 } as Account); Matrix.autoMockMethod(authService.authStatusFor$, () => of(AuthenticationStatus.Unlocked)); - apiService.getSync.mockResolvedValue( - new SyncResponse({ - profile: { - id: user1, - }, - folders: [], - collections: [], - ciphers: [], - sends: [], - domains: [], - policies: [], - }), - ); + apiService.getSync.mockResolvedValue(emptySyncResponse); Matrix.autoMockMethod(userDecryptionOptionsService.userDecryptionOptionsById$, () => of({ hasMasterPassword: true } satisfies UserDecryptionOptions), ); @@ -201,5 +201,44 @@ describe("DefaultSyncService", () => { expect(apiService.refreshIdentityToken).toHaveBeenCalledTimes(1); expect(apiService.getSync).toHaveBeenCalledTimes(1); }); + + describe("in-flight syncs", () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("does not call getSync when one is already in progress", async () => { + const fullSyncPromises = [sut.fullSync(true), sut.fullSync(false), sut.fullSync(false)]; + + jest.advanceTimersByTime(100); + + await Promise.all(fullSyncPromises); + + expect(apiService.getSync).toHaveBeenCalledTimes(1); + }); + + it("does not call refreshIdentityToken when one is already in progress", async () => { + const fullSyncPromises = [sut.fullSync(true), sut.fullSync(false), sut.fullSync(false)]; + + jest.advanceTimersByTime(100); + + await Promise.all(fullSyncPromises); + + expect(apiService.refreshIdentityToken).toHaveBeenCalledTimes(1); + }); + + it("resets the in-flight properties when the complete", async () => { + const fullSyncPromises = [sut.fullSync(true), sut.fullSync(true)]; + + await Promise.all(fullSyncPromises); + + expect(sut["inFlightApiCalls"].refreshToken).toBeNull(); + expect(sut["inFlightApiCalls"].sync).toBeNull(); + }); + }); }); }); diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index 6e1a8f28443..47ac3784c33 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -58,11 +58,21 @@ import { MessageSender } from "../messaging"; import { StateProvider } from "../state"; import { CoreSyncService } from "./core-sync.service"; +import { SyncResponse } from "./sync.response"; import { SyncOptions } from "./sync.service"; export class DefaultSyncService extends CoreSyncService { syncInProgress = false; + /** The promises associated with any in-flight api calls. */ + private inFlightApiCalls: { + refreshToken: Promise | null; + sync: Promise | null; + } = { + refreshToken: null, + sync: null, + }; + constructor( private masterPasswordService: InternalMasterPasswordServiceAbstraction, accountService: AccountService, @@ -141,9 +151,24 @@ export class DefaultSyncService extends CoreSyncService { try { if (!skipTokenRefresh) { - await this.apiService.refreshIdentityToken(); + // Store the promise so multiple calls to refresh the token are not made + if (this.inFlightApiCalls.refreshToken === null) { + this.inFlightApiCalls.refreshToken = this.apiService.refreshIdentityToken(); + } + + await this.inFlightApiCalls.refreshToken; } - const response = await this.apiService.getSync(); + + // Store the promise so multiple calls to sync are not made + if (this.inFlightApiCalls.sync === null) { + this.inFlightApiCalls.sync = this.apiService.getSync(); + } else { + this.logService.debug( + "Sync: Sync network call already in progress, returning existing promise", + ); + } + + const response = await this.inFlightApiCalls.sync; await this.syncProfile(response.profile); await this.syncFolders(response.folders, response.profile.id); @@ -162,6 +187,9 @@ export class DefaultSyncService extends CoreSyncService { } else { return this.syncCompleted(false, userId); } + } finally { + this.inFlightApiCalls.refreshToken = null; + this.inFlightApiCalls.sync = null; } }