From f1396ec253f1abdd2f7f278efc2d8d77a38e3240 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Tue, 17 Mar 2026 19:27:44 +0100 Subject: [PATCH] fix(sync): add 429 retry for encryption toggle upload and improve error messages Add automatic retry with backoff for 429 rate limit errors during the critical upload-after-delete step in encryption toggle. This prevents the user from seeing a scary error when the server rate-limits during initial setup. Also improve user-facing error messages by stripping raw JSON payloads while preserving the original error via Error cause chain for debugging. --- .../imex/sync/snapshot-upload.service.spec.ts | 77 ++++++++++++++++ src/app/imex/sync/snapshot-upload.service.ts | 87 +++++++++++++++++-- ...upersync-encryption-toggle.service.spec.ts | 46 ++++++---- .../supersync-encryption-toggle.service.ts | 22 ++++- 4 files changed, 207 insertions(+), 25 deletions(-) diff --git a/src/app/imex/sync/snapshot-upload.service.spec.ts b/src/app/imex/sync/snapshot-upload.service.spec.ts index c66f83c4de..bf5c0e64de 100644 --- a/src/app/imex/sync/snapshot-upload.service.spec.ts +++ b/src/app/imex/sync/snapshot-upload.service.spec.ts @@ -304,6 +304,83 @@ describe('SnapshotUploadService', () => { ).toBeRejectedWithError(/Snapshot upload failed/); }); + it('should retry upload on 429 rate limit error', async () => { + let callCount = 0; + mockSyncProvider.uploadSnapshot.and.callFake(async () => { + callCount++; + if (callCount === 1) { + throw new Error( + 'SuperSync API error: 429 Too Many Requests - {"statusCode":429,"message":"Rate limit exceeded, retry in 0 seconds"}', + ); + } + return { accepted: true, serverSeq: 42 }; + }); + + const result = await service.deleteAndReuploadWithNewEncryption({ + encryptKey: undefined, + isEncryptionEnabled: false, + logPrefix: 'TestPrefix', + }); + + expect(result.accepted).toBeTrue(); + expect(callCount).toBe(2); + }); + + it('should not retry on non-429 errors', async () => { + mockSyncProvider.uploadSnapshot.and.rejectWith(new Error('Network timeout')); + + await expectAsync( + service.deleteAndReuploadWithNewEncryption({ + encryptKey: undefined, + isEncryptionEnabled: false, + logPrefix: 'TestPrefix', + }), + ).toBeRejectedWithError(/Network timeout/); + + expect(mockSyncProvider.uploadSnapshot).toHaveBeenCalledTimes(1); + }); + + it('should throw after exhausting retries on repeated 429', async () => { + mockSyncProvider.uploadSnapshot.and.callFake(async () => { + throw new Error( + 'SuperSync API error: 429 Too Many Requests - {"statusCode":429,"message":"Rate limit exceeded, retry in 0 seconds"}', + ); + }); + + await expectAsync( + service.deleteAndReuploadWithNewEncryption({ + encryptKey: undefined, + isEncryptionEnabled: false, + logPrefix: 'TestPrefix', + }), + ).toBeRejectedWithError(/429/); + + // 1 initial + 2 retries = 3 total attempts + expect(mockSyncProvider.uploadSnapshot).toHaveBeenCalledTimes(3); + }); + + it('should parse retry delay from minutes in error message', async () => { + let callCount = 0; + mockSyncProvider.uploadSnapshot.and.callFake(async () => { + callCount++; + if (callCount === 1) { + throw new Error( + 'SuperSync API error: 429 Too Many Requests - {"statusCode":429,"message":"Rate limit exceeded, retry in 0 minutes"}', + ); + } + return { accepted: true, serverSeq: 42 }; + }); + + const result = await service.deleteAndReuploadWithNewEncryption({ + encryptKey: undefined, + isEncryptionEnabled: false, + logPrefix: 'TestPrefix', + }); + + expect(result.accepted).toBeTrue(); + expect(callCount).toBe(2); + }); + it('should execute steps in correct order', async () => { const callOrder: string[] = []; diff --git a/src/app/imex/sync/snapshot-upload.service.ts b/src/app/imex/sync/snapshot-upload.service.ts index e3654ccf3b..ac83f3731d 100644 --- a/src/app/imex/sync/snapshot-upload.service.ts +++ b/src/app/imex/sync/snapshot-upload.service.ts @@ -244,15 +244,16 @@ export class SnapshotUploadService { isEncryptionEnabled, } as SuperSyncPrivateCfg); - // Upload snapshot - SyncLog.normal(`${logPrefix}: Uploading snapshot...`); - const result = await this.uploadSnapshot( + // Upload snapshot with retry for rate limiting + // Critical: server data is already deleted, so we must try hard to get the upload through + const result = await this._uploadWithRateLimitRetry({ syncProvider, payload, clientId, vectorClock, - isEncryptionEnabled && !!encryptKey, - ); + isPayloadEncrypted: isEncryptionEnabled && !!encryptKey, + logPrefix, + }); if (!result.accepted) { throw new Error(`Snapshot upload failed: ${result.error}`); @@ -262,4 +263,80 @@ export class SnapshotUploadService { return { ...result, existingCfg }; } + + /** + * Uploads a snapshot with automatic retry on 429 rate limit errors. + * Used after destructive delete where upload failure leaves server in bad state. + */ + private async _uploadWithRateLimitRetry(options: { + syncProvider: SyncProviderBase & OperationSyncCapable; + payload: unknown; + clientId: string; + vectorClock: VectorClock; + isPayloadEncrypted: boolean; + logPrefix: string; + }): Promise { + const { + syncProvider, + payload, + clientId, + vectorClock, + isPayloadEncrypted, + logPrefix, + } = options; + const MAX_RETRIES = 2; + // Server rate limits are typically 5-10 minutes; honor the full delay since + // the dialog shows a spinner and server data is already deleted + const MAX_DELAY_MS = 10 * 60_000; + + for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { + try { + SyncLog.normal(`${logPrefix}: Uploading snapshot (attempt ${attempt + 1})...`); + return await this.uploadSnapshot( + syncProvider, + payload, + clientId, + vectorClock, + isPayloadEncrypted, + ); + } catch (error) { + const delayMs = this._parseRateLimitDelayMs(error); + if (delayMs !== null && attempt < MAX_RETRIES) { + const cappedDelay = Math.min(delayMs, MAX_DELAY_MS); + SyncLog.warn( + `${logPrefix}: Rate limited (429) on attempt ${attempt + 1}, ` + + `retrying in ${(cappedDelay / 1000).toFixed(0)}s...`, + ); + await new Promise((resolve) => setTimeout(resolve, cappedDelay)); + continue; + } + throw error; + } + } + throw new Error('Snapshot upload failed after retries'); + } + + /** + * Parses a rate limit (429) error and returns the suggested retry delay in ms. + * Returns null if the error is not a rate limit error. + */ + private _parseRateLimitDelayMs(error: unknown): number | null { + const message = error instanceof Error ? error.message : String(error); + if (!/\b429\b/.test(message)) { + return null; + } + + const minutesMatch = message.match(/retry in (\d+)\s*minute/i); + if (minutesMatch) { + return parseInt(minutesMatch[1], 10) * 60_000; + } + + const secondsMatch = message.match(/retry in (\d+)\s*second/i); + if (secondsMatch) { + return parseInt(secondsMatch[1], 10) * 1000; + } + + // Default retry delay for unspecified 429 + return 60_000; + } } diff --git a/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts b/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts index ea35290bc8..4b7f78934f 100644 --- a/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts +++ b/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts @@ -103,7 +103,7 @@ describe('SuperSyncEncryptionToggleService', () => { ); await expectAsync(service.enableEncryption('my-secret-key')).toBeRejectedWithError( - /CRITICAL/, + /Failed to upload encrypted snapshot/, ); // Should revert using the config captured BEFORE the destructive call @@ -118,24 +118,30 @@ describe('SuperSyncEncryptionToggleService', () => { ); }); - it('should throw with CRITICAL message on failure', async () => { + it('should throw with descriptive message including reason on failure', async () => { mockSnapshotUploadService.deleteAndReuploadWithNewEncryption.and.rejectWith( new Error('Server rejected'), ); await expectAsync(service.enableEncryption('my-secret-key')).toBeRejectedWithError( - /CRITICAL: Failed to upload encrypted snapshot after deleting server data/, + /Failed to upload encrypted snapshot.*Reason: Server rejected/, ); }); - it('should include original error message in CRITICAL error', async () => { + it('should preserve original error as cause', async () => { + const originalError = new Error('Network error'); mockSnapshotUploadService.deleteAndReuploadWithNewEncryption.and.rejectWith( - new Error('Network error'), + originalError, ); - await expectAsync(service.enableEncryption('my-secret-key')).toBeRejectedWithError( - /CRITICAL.*Network error/, - ); + try { + await service.enableEncryption('my-secret-key'); + fail('Expected error to be thrown'); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch(/Reason: Network error/); + expect((error as Error).cause).toBe(originalError); + } }); }); @@ -165,7 +171,9 @@ describe('SuperSyncEncryptionToggleService', () => { new Error('Upload failed'), ); - await expectAsync(service.disableEncryption()).toBeRejectedWithError(/CRITICAL/); + await expectAsync(service.disableEncryption()).toBeRejectedWithError( + /Failed to upload unencrypted snapshot/, + ); // Should restore the original config including the encryption key expect(mockProviderManager.setProviderConfig).toHaveBeenCalledWith( @@ -174,24 +182,30 @@ describe('SuperSyncEncryptionToggleService', () => { ); }); - it('should throw with CRITICAL message on failure', async () => { + it('should throw with descriptive message including reason on failure', async () => { mockSnapshotUploadService.deleteAndReuploadWithNewEncryption.and.rejectWith( new Error('Server rejected'), ); await expectAsync(service.disableEncryption()).toBeRejectedWithError( - /CRITICAL: Failed to upload unencrypted snapshot after deleting server data/, + /Failed to upload unencrypted snapshot.*Reason: Server rejected/, ); }); - it('should include original error message in CRITICAL error', async () => { + it('should preserve original error as cause', async () => { + const originalError = new Error('Network error'); mockSnapshotUploadService.deleteAndReuploadWithNewEncryption.and.rejectWith( - new Error('Network error'), + originalError, ); - await expectAsync(service.disableEncryption()).toBeRejectedWithError( - /CRITICAL.*Network error/, - ); + try { + await service.disableEncryption(); + fail('Expected error to be thrown'); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch(/Reason: Network error/); + expect((error as Error).cause).toBe(originalError); + } }); }); }); diff --git a/src/app/imex/sync/supersync-encryption-toggle.service.ts b/src/app/imex/sync/supersync-encryption-toggle.service.ts index dc5cfa4b13..6ac413e2fe 100644 --- a/src/app/imex/sync/supersync-encryption-toggle.service.ts +++ b/src/app/imex/sync/supersync-encryption-toggle.service.ts @@ -7,6 +7,18 @@ import { SyncProviderId } from '../../op-log/sync-providers/provider.const'; const LOG_PREFIX = 'SuperSyncEncryptionToggleService'; +/** + * Extracts a user-friendly message from an error, stripping raw JSON payloads. + * e.g. "SuperSync API error: 429 Too Many Requests - {"statusCode":429,...}" + * -> "SuperSync API error: 429 Too Many Requests" + */ +const _friendlyErrorMessage = (error: unknown): string => { + const raw = error instanceof Error ? error.message : String(error); + // Strip JSON body appended after " - {" by the SuperSync provider + const jsonStart = raw.indexOf(' - {'); + return jsonStart > 0 ? raw.substring(0, jsonStart) : raw; +}; + /** * Service for enabling/disabling encryption for SuperSync. * @@ -70,9 +82,10 @@ export class SuperSyncEncryptionToggleService { } as SuperSyncPrivateCfg); throw new Error( - 'CRITICAL: Failed to upload encrypted snapshot after deleting server data. ' + + 'Failed to upload encrypted snapshot after deleting server data. ' + 'Your local data is safe. Encryption has been reverted. Please use "Sync Now" to re-upload your data. ' + - `Original error: ${error instanceof Error ? error.message : error}`, + `Reason: ${_friendlyErrorMessage(error)}`, + { cause: error }, ); } } @@ -112,9 +125,10 @@ export class SuperSyncEncryptionToggleService { } throw new Error( - 'CRITICAL: Failed to upload unencrypted snapshot after deleting server data. ' + + 'Failed to upload unencrypted snapshot after deleting server data. ' + 'Your local data is safe. Encryption is still enabled. Please try disabling encryption again. ' + - `Original error: ${uploadError instanceof Error ? uploadError.message : uploadError}`, + `Reason: ${_friendlyErrorMessage(uploadError)}`, + { cause: uploadError }, ); } }