mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-18 00:46:45 +00:00
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.
This commit is contained in:
parent
f3585b5dc1
commit
f1396ec253
4 changed files with 207 additions and 25 deletions
|
|
@ -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[] = [];
|
||||
|
||||
|
|
|
|||
|
|
@ -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<SyncProviderId> & OperationSyncCapable;
|
||||
payload: unknown;
|
||||
clientId: string;
|
||||
vectorClock: VectorClock;
|
||||
isPayloadEncrypted: boolean;
|
||||
logPrefix: string;
|
||||
}): Promise<SnapshotUploadResult> {
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue