diff --git a/packages/super-sync-server/src/sync/sync.routes.ts b/packages/super-sync-server/src/sync/sync.routes.ts index a609e0859..58ec4678b 100644 --- a/packages/super-sync-server/src/sync/sync.routes.ts +++ b/packages/super-sync-server/src/sync/sync.routes.ts @@ -255,7 +255,7 @@ export const syncRoutes = async (fastify: FastifyInstance): Promise => { let newOps: import('./sync.types').ServerOperation[] | undefined; let latestSeq: number; let hasMorePiggyback = false; - const PIGGYBACK_LIMIT = 100; + const PIGGYBACK_LIMIT = 500; if (lastKnownServerSeq !== undefined) { const opsResult = await syncService.getOpsSinceWithSeq( @@ -380,7 +380,7 @@ export const syncRoutes = async (fastify: FastifyInstance): Promise => { let newOps: import('./sync.types').ServerOperation[] | undefined; let latestSeq: number; let hasMorePiggyback = false; - const PIGGYBACK_LIMIT = 100; + const PIGGYBACK_LIMIT = 500; if (lastKnownServerSeq !== undefined) { // Use atomic read to get ops and latestSeq together diff --git a/src/app/pfapi/api/sync/sync.service.spec.ts b/src/app/pfapi/api/sync/sync.service.spec.ts index b99f7845e..608ed2704 100644 --- a/src/app/pfapi/api/sync/sync.service.spec.ts +++ b/src/app/pfapi/api/sync/sync.service.spec.ts @@ -931,4 +931,103 @@ describe('SyncService', () => { expect(result.conflictData).toBeDefined(); }); }); + + describe('operation log sync', () => { + beforeEach(() => { + // Enable operation log sync for the mock provider + mockSyncProvider.supportsOperationSync = true; + }); + + it('should skip download when hasMorePiggyback is false (all ops fit in piggyback)', async () => { + // Configure upload to return no more piggyback ops + mockOperationLogSyncService.uploadPendingOps.and.returnValue( + Promise.resolve({ + uploadedCount: 5, + piggybackedOps: [], // Piggybacked ops processed internally + rejectedCount: 0, + rejectedOps: [], + hasMorePiggyback: false, // No more ops on server + }), + ); + + await service.sync(); + + // Upload should be called + expect(mockOperationLogSyncService.uploadPendingOps).toHaveBeenCalledWith( + mockSyncProvider, + ); + // Download should NOT be called because hasMorePiggyback is false + expect(mockOperationLogSyncService.downloadRemoteOps).not.toHaveBeenCalled(); + }); + + it('should call download when hasMorePiggyback is undefined (no upload happened)', async () => { + // When nothing was uploaded, hasMorePiggyback is undefined. + // We MUST download to check for remote ops. + mockOperationLogSyncService.uploadPendingOps.and.returnValue( + Promise.resolve({ + uploadedCount: 0, + piggybackedOps: [], + rejectedCount: 0, + rejectedOps: [], + // hasMorePiggyback is undefined - no API call was made + }), + ); + + await service.sync(); + + // Download SHOULD be called because hasMorePiggyback is undefined (not explicitly false) + expect(mockOperationLogSyncService.downloadRemoteOps).toHaveBeenCalledWith( + mockSyncProvider, + ); + }); + + it('should call download when hasMorePiggyback is true (more ops on server)', async () => { + // Configure upload to indicate more ops available + mockOperationLogSyncService.uploadPendingOps.and.returnValue( + Promise.resolve({ + uploadedCount: 5, + piggybackedOps: [], // Ops processed internally + rejectedCount: 0, + rejectedOps: [], + hasMorePiggyback: true, // More ops exist on server + }), + ); + + await service.sync(); + + // Upload should be called + expect(mockOperationLogSyncService.uploadPendingOps).toHaveBeenCalled(); + // Download SHOULD be called because hasMorePiggyback is true + expect(mockOperationLogSyncService.downloadRemoteOps).toHaveBeenCalledWith( + mockSyncProvider, + ); + }); + + it('should call download when upload returns null (fresh client)', async () => { + // Fresh clients return null from upload + mockOperationLogSyncService.uploadPendingOps.and.returnValue(Promise.resolve(null)); + + await service.sync(); + + // Download SHOULD be called for fresh clients + expect(mockOperationLogSyncService.downloadRemoteOps).toHaveBeenCalledWith( + mockSyncProvider, + ); + }); + + it('should return InSync status after operation log sync completes', async () => { + mockOperationLogSyncService.uploadPendingOps.and.returnValue( + Promise.resolve({ + uploadedCount: 0, + piggybackedOps: [], + rejectedCount: 0, + rejectedOps: [], + }), + ); + + const result = await service.sync(); + + expect(result.status).toBe(SyncStatus.InSync); + }); + }); }); diff --git a/src/app/pfapi/api/sync/sync.service.ts b/src/app/pfapi/api/sync/sync.service.ts index 9c61269ce..bb7f1b26d 100644 --- a/src/app/pfapi/api/sync/sync.service.ts +++ b/src/app/pfapi/api/sync/sync.service.ts @@ -110,8 +110,36 @@ export class SyncService { if (currentSyncProvider && this._supportsOpLogSync(currentSyncProvider)) { const uploadResult = await this._operationLogSyncService.uploadPendingOps(currentSyncProvider); - const downloadResult = - await this._operationLogSyncService.downloadRemoteOps(currentSyncProvider); + + // OPTIMIZATION: Skip download if all remote ops were already piggybacked during upload. + // This reduces sync to a single round-trip when remote changes are small (≤500 ops). + // When hasMorePiggyback=true, we still need to download remaining ops. + let downloadResult: { + serverMigrationHandled: boolean; + localWinOpsCreated: number; + newOpsCount: number; + }; + + // OPTIMIZATION: Skip download ONLY when server explicitly confirms no more ops. + // hasMorePiggyback === false means: we uploaded something AND server returned the flag. + // hasMorePiggyback === undefined means: no upload happened (nothing to upload) - MUST download. + // hasMorePiggyback === true means: server has more ops beyond the piggyback limit. + if (uploadResult && uploadResult.hasMorePiggyback === false) { + // Server confirmed all remote ops fit in piggyback - no download needed + const opCount = uploadResult.piggybackedOps.length; + PFLog.normal( + `${SyncService.L}.${this.sync.name}(): All ops piggybacked (${opCount}), skip download`, + ); + downloadResult = { + serverMigrationHandled: false, + localWinOpsCreated: 0, + newOpsCount: 0, + }; + } else { + // Need to download remaining ops + downloadResult = + await this._operationLogSyncService.downloadRemoteOps(currentSyncProvider); + } // Track if we need a re-upload: // 1. Server migration created a SYNC_IMPORT that needs uploading