From 465b4cb0fbd81405e17826b8c06257ea162cc3bd Mon Sep 17 00:00:00 2001 From: Jamie Kyle <113370520+jamiebuilds-signal@users.noreply.github.com> Date: Thu, 5 Jan 2023 14:29:02 -0800 Subject: [PATCH] Treat 413 and 429 as rate limits everywhere --- ts/services/profiles.ts | 18 ++- ts/services/username.ts | 8 +- ts/test-electron/services/profiles_test.ts | 60 ++++---- .../helpers/handleMultipleSendErrors_test.ts | 132 ++++++++++-------- 4 files changed, 116 insertions(+), 102 deletions(-) diff --git a/ts/services/profiles.ts b/ts/services/profiles.ts index 4217feeae5a..1cf5adf5894 100644 --- a/ts/services/profiles.ts +++ b/ts/services/profiles.ts @@ -42,13 +42,13 @@ type JobType = { }; // Goals for this service: -// 1. Ensure that when we get a 413 from the server, we stop firing off profile +// 1. Ensure that when we get a 413/429 from the server, we stop firing off profile // fetches for a while. // 2. Ensure that all existing profile fetches don't hang in this case; to solve this we -// cancel all outstanding requests when we hit a 413, and throw instead of queueing -// something new if we're waiting due to a retry-after. Note: It's no worse than what -// we were doing before, failing all requests and pushing the retry-after time out -// further. +// cancel all outstanding requests when we hit a 413/429, and throw instead of +// queueing something new if we're waiting due to a retry-after. Note: It's no worse +// than what we were doing before, failing all requests and pushing the retry-after +// time out further. // 3. Require no changes to callers. // Potential future goals for this problem area: @@ -121,8 +121,12 @@ export class ProfileService { return; } - if (isRecord(error) && 'code' in error && error.code === 413) { - this.clearAll('got 413 from server'); + if ( + isRecord(error) && + 'code' in error && + (error.code === 413 || error.code === 429) + ) { + this.clearAll(`got ${error.code} from server`); const time = findRetryAfterTimeFromError(error); void this.pause(time); } diff --git a/ts/services/username.ts b/ts/services/username.ts index 9f6512c00bf..f9dbc1da7e8 100644 --- a/ts/services/username.ts +++ b/ts/services/username.ts @@ -78,9 +78,9 @@ export async function reserveUsername( if (error.code === 409) { return { ok: false, error: ReserveUsernameError.Conflict }; } - if (error.code === 413) { + if (error.code === 413 || error.code === 429) { const time = findRetryAfterTimeFromError(error); - log.warn(`reserveUsername: got 413, waiting ${time}ms`); + log.warn(`reserveUsername: got ${error.code}, waiting ${time}ms`); await sleep(time, abortSignal); return reserveUsername(options); @@ -139,9 +139,9 @@ export async function confirmUsername( await updateUsernameAndSyncProfile(username); } catch (error) { if (error instanceof HTTPError) { - if (error.code === 413) { + if (error.code === 413 || error.code === 429) { const time = findRetryAfterTimeFromError(error); - log.warn(`confirmUsername: got 413, waiting ${time}ms`); + log.warn(`confirmUsername: got ${error.code}, waiting ${time}ms`); await sleep(time, abortSignal); return confirmUsername(reservation, abortSignal); diff --git a/ts/test-electron/services/profiles_test.ts b/ts/test-electron/services/profiles_test.ts index 9a4fd432325..de74db1a89a 100644 --- a/ts/test-electron/services/profiles_test.ts +++ b/ts/test-electron/services/profiles_test.ts @@ -101,41 +101,43 @@ describe('util/profiles', () => { assert.strictEqual(runCount, 0); }); - it('clears all outstanding jobs if we get a 413, then pauses', async () => { - let runCount = 0; - const getProfileWhichThrows = async () => { - runCount += 1; - const error = new HTTPError('fake 413', { - code: 413, - headers: { - 'retry-after': '1', - }, - }); - throw error; - }; - const service = new ProfileService(getProfileWhichThrows); + for (const code of [413, 429] as const) { + it(`clears all outstanding jobs if we get a ${code}, then pauses`, async () => { + let runCount = 0; + const getProfileWhichThrows = async () => { + runCount += 1; + const error = new HTTPError(`fake ${code}`, { + code, + headers: { + 'retry-after': '1', + }, + }); + throw error; + }; + const service = new ProfileService(getProfileWhichThrows); - // Queued and immediately started due to concurrency = 3 - const promise1 = service.get(UUID_1); - const promise2 = service.get(UUID_2); - const promise3 = service.get(UUID_3); + // Queued and immediately started due to concurrency = 3 + const promise1 = service.get(UUID_1); + const promise2 = service.get(UUID_2); + const promise3 = service.get(UUID_3); - // Never started, but queued - const promise4 = service.get(UUID_4); + // Never started, but queued + const promise4 = service.get(UUID_4); - assert.strictEqual(runCount, 3, 'before await'); + assert.strictEqual(runCount, 3, 'before await'); - await assert.isRejected(promise1, 'fake 413'); + await assert.isRejected(promise1, `fake ${code}`); - // Never queued - const promise5 = service.get(UUID_5); + // Never queued + const promise5 = service.get(UUID_5); - await assert.isRejected(promise2, 'job cancelled'); - await assert.isRejected(promise3, 'job cancelled'); - await assert.isRejected(promise4, 'job cancelled'); - await assert.isRejected(promise5, 'paused queue'); + await assert.isRejected(promise2, 'job cancelled'); + await assert.isRejected(promise3, 'job cancelled'); + await assert.isRejected(promise4, 'job cancelled'); + await assert.isRejected(promise5, 'paused queue'); - assert.strictEqual(runCount, 3, 'after await'); - }); + assert.strictEqual(runCount, 3, 'after await'); + }); + } }); }); diff --git a/ts/test-node/jobs/helpers/handleMultipleSendErrors_test.ts b/ts/test-node/jobs/helpers/handleMultipleSendErrors_test.ts index 2f0626f323b..0f78cedfe68 100644 --- a/ts/test-node/jobs/helpers/handleMultipleSendErrors_test.ts +++ b/ts/test-node/jobs/helpers/handleMultipleSendErrors_test.ts @@ -34,9 +34,9 @@ describe('maybeExpandErrors', () => { }); describe('handleMultipleSendErrors', () => { - const make413 = (retryAfter: number): HTTPError => + const makeSlowDown = (code: 413 | 429, retryAfter: number): HTTPError => new HTTPError('Slow down', { - code: 413, + code, headers: { 'retry-after': retryAfter.toString() }, response: {}, }); @@ -101,76 +101,84 @@ describe('handleMultipleSendErrors', () => { ); }); - describe('413 handling', () => { - it('sleeps for the longest 413 Retry-After time', async () => { - let done = false; + for (const code of [413, 429] as const) { + // eslint-disable-next-line no-loop-func + describe(`${code} handling`, () => { + it(`sleeps for the longest ${code} Retry-After time`, async () => { + let done = false; - void (async () => { - try { - await handleMultipleSendErrors({ + void (async () => { + try { + await handleMultipleSendErrors({ + ...defaultOptions, + errors: [ + new Error('Other'), + makeSlowDown(code, 10), + makeSlowDown(code, 999), + makeSlowDown(code, 20), + ], + timeRemaining: 99999999, + toThrow: new Error('to throw'), + }); + } catch (err) { + // No-op + } finally { + done = true; + } + })(); + + await clock.tickAsync(900 * SECOND); + assert.isFalse(done, "Didn't sleep for long enough"); + await clock.tickAsync(100 * SECOND); + assert.isTrue(done, 'Slept for too long'); + }); + + it("doesn't sleep longer than the remaining time", async () => { + let done = false; + + void (async () => { + try { + await handleMultipleSendErrors({ + ...defaultOptions, + errors: [makeSlowDown(code, 9999)], + timeRemaining: 99, + toThrow: new Error('to throw'), + }); + } catch (err) { + // No-op + } finally { + done = true; + } + })(); + + await clock.tickAsync(100); + assert.isTrue(done); + }); + + it("doesn't sleep if it's the final attempt", async () => { + await assert.isRejected( + handleMultipleSendErrors({ ...defaultOptions, - errors: [ - new Error('Other'), - make413(10), - make413(999), - make413(20), - ], - timeRemaining: 99999999, + errors: [new Error('uh oh')], + isFinalAttempt: true, toThrow: new Error('to throw'), - }); - } catch (err) { - // No-op - } finally { - done = true; - } - })(); - - await clock.tickAsync(900 * SECOND); - assert.isFalse(done, "Didn't sleep for long enough"); - await clock.tickAsync(100 * SECOND); - assert.isTrue(done, 'Slept for too long'); + }) + ); + }); }); - - it("doesn't sleep longer than the remaining time", async () => { - let done = false; - - void (async () => { - try { - await handleMultipleSendErrors({ - ...defaultOptions, - errors: [make413(9999)], - timeRemaining: 99, - toThrow: new Error('to throw'), - }); - } catch (err) { - // No-op - } finally { - done = true; - } - })(); - - await clock.tickAsync(100); - assert.isTrue(done); - }); - - it("doesn't sleep if it's the final attempt", async () => { - await assert.isRejected( - handleMultipleSendErrors({ - ...defaultOptions, - errors: [new Error('uh oh')], - isFinalAttempt: true, - toThrow: new Error('to throw'), - }) - ); - }); - }); + } describe('508 handling', () => { it('resolves with no error if any 508 is received', async () => { await assert.isFulfilled( handleMultipleSendErrors({ ...defaultOptions, - errors: [new Error('uh oh'), { code: 508 }, make413(99999)], + errors: [ + new Error('uh oh'), + { code: 508 }, + makeSlowDown(413, 99999), + makeSlowDown(429, 99999), + ], markFailed: noop, toThrow: new Error('to throw'), })