Treat 413 and 429 as rate limits everywhere

This commit is contained in:
Jamie Kyle 2023-01-05 14:29:02 -08:00 committed by GitHub
parent 6dd32456c6
commit 465b4cb0fb
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 116 additions and 102 deletions

View file

@ -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);
}

View file

@ -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);

View file

@ -101,12 +101,13 @@ describe('util/profiles', () => {
assert.strictEqual(runCount, 0);
});
it('clears all outstanding jobs if we get a 413, then pauses', async () => {
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 413', {
code: 413,
const error = new HTTPError(`fake ${code}`, {
code,
headers: {
'retry-after': '1',
},
@ -125,7 +126,7 @@ describe('util/profiles', () => {
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);
@ -137,5 +138,6 @@ describe('util/profiles', () => {
assert.strictEqual(runCount, 3, 'after await');
});
}
});
});

View file

@ -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,8 +101,10 @@ describe('handleMultipleSendErrors', () => {
);
});
describe('413 handling', () => {
it('sleeps for the longest 413 Retry-After time', async () => {
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 () => {
@ -111,9 +113,9 @@ describe('handleMultipleSendErrors', () => {
...defaultOptions,
errors: [
new Error('Other'),
make413(10),
make413(999),
make413(20),
makeSlowDown(code, 10),
makeSlowDown(code, 999),
makeSlowDown(code, 20),
],
timeRemaining: 99999999,
toThrow: new Error('to throw'),
@ -138,7 +140,7 @@ describe('handleMultipleSendErrors', () => {
try {
await handleMultipleSendErrors({
...defaultOptions,
errors: [make413(9999)],
errors: [makeSlowDown(code, 9999)],
timeRemaining: 99,
toThrow: new Error('to throw'),
});
@ -164,13 +166,19 @@ describe('handleMultipleSendErrors', () => {
);
});
});
}
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'),
})