diff --git a/ts/jobs/helpers/getHttpErrorCode.ts b/ts/jobs/helpers/getHttpErrorCode.ts new file mode 100644 index 000000000000..f69245560dfb --- /dev/null +++ b/ts/jobs/helpers/getHttpErrorCode.ts @@ -0,0 +1,27 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { isRecord } from '../../util/isRecord'; +import { parseIntWithFallback } from '../../util/parseIntWithFallback'; + +/** + * Looks for an HTTP code. First tries the top level error, then looks at its `httpError` + * property. + */ +export function getHttpErrorCode(maybeError: unknown): number { + if (!isRecord(maybeError)) { + return -1; + } + + const maybeTopLevelCode = parseIntWithFallback(maybeError.code, -1); + if (maybeTopLevelCode !== -1) { + return maybeTopLevelCode; + } + + const { httpError } = maybeError; + if (!isRecord(httpError)) { + return -1; + } + + return parseIntWithFallback(httpError.code, -1); +} diff --git a/ts/jobs/helpers/handleCommonJobRequestError.ts b/ts/jobs/helpers/handleCommonJobRequestError.ts index 7c11205709ec..70cac8a5208b 100644 --- a/ts/jobs/helpers/handleCommonJobRequestError.ts +++ b/ts/jobs/helpers/handleCommonJobRequestError.ts @@ -2,9 +2,8 @@ // SPDX-License-Identifier: AGPL-3.0-only import type { LoggerType } from '../../types/Logging'; -import { parseIntWithFallback } from '../../util/parseIntWithFallback'; -import { HTTPError } from '../../textsecure/Errors'; -import { sleepFor413RetryAfterTimeIfApplicable } from './sleepFor413RetryAfterTimeIfApplicable'; +import { sleepFor413RetryAfterTime } from './sleepFor413RetryAfterTime'; +import { getHttpErrorCode } from './getHttpErrorCode'; export async function handleCommonJobRequestError({ err, @@ -15,17 +14,14 @@ export async function handleCommonJobRequestError({ log: LoggerType; timeRemaining: number; }>): Promise { - if (!(err instanceof HTTPError)) { - throw err; + switch (getHttpErrorCode(err)) { + case 413: + await sleepFor413RetryAfterTime({ err, log, timeRemaining }); + return; + case 508: + log.info('server responded with 508. Giving up on this job'); + return; + default: + throw err; } - - const code = parseIntWithFallback(err.code, -1); - if (code === 508) { - log.info('server responded with 508. Giving up on this job'); - return; - } - - await sleepFor413RetryAfterTimeIfApplicable({ err, log, timeRemaining }); - - throw err; } diff --git a/ts/jobs/helpers/sleepFor413RetryAfterTimeIfApplicable.ts b/ts/jobs/helpers/sleepFor413RetryAfterTime.ts similarity index 60% rename from ts/jobs/helpers/sleepFor413RetryAfterTimeIfApplicable.ts rename to ts/jobs/helpers/sleepFor413RetryAfterTime.ts index 392c7c03bdbf..75da9608d389 100644 --- a/ts/jobs/helpers/sleepFor413RetryAfterTimeIfApplicable.ts +++ b/ts/jobs/helpers/sleepFor413RetryAfterTime.ts @@ -7,7 +7,7 @@ import { parseRetryAfter } from '../../util/parseRetryAfter'; import { isRecord } from '../../util/isRecord'; import { HTTPError } from '../../textsecure/Errors'; -export async function sleepFor413RetryAfterTimeIfApplicable({ +export async function sleepFor413RetryAfterTime({ err, log, timeRemaining, @@ -16,17 +16,12 @@ export async function sleepFor413RetryAfterTimeIfApplicable({ log: Pick; timeRemaining: number; }>): Promise { - if ( - timeRemaining <= 0 || - !(err instanceof HTTPError) || - err.code !== 413 || - !isRecord(err.responseHeaders) - ) { + if (timeRemaining <= 0) { return; } const retryAfter = Math.min( - parseRetryAfter(err.responseHeaders['retry-after']), + parseRetryAfter(findRetryAfterTime(err)), timeRemaining ); @@ -36,3 +31,19 @@ export async function sleepFor413RetryAfterTimeIfApplicable({ await sleep(retryAfter); } + +function findRetryAfterTime(err: unknown): unknown { + if (!isRecord(err)) { + return undefined; + } + + if (isRecord(err.responseHeaders)) { + return err.responseHeaders['retry-after']; + } + + if (err.httpError instanceof HTTPError) { + return err.httpError.responseHeaders?.['retry-after']; + } + + return undefined; +} diff --git a/ts/jobs/normalMessageSendJobQueue.ts b/ts/jobs/normalMessageSendJobQueue.ts index b73113078422..817633f86159 100644 --- a/ts/jobs/normalMessageSendJobQueue.ts +++ b/ts/jobs/normalMessageSendJobQueue.ts @@ -7,7 +7,7 @@ import PQueue from 'p-queue'; import type { LoggerType } from '../types/Logging'; import { exponentialBackoffMaxAttempts } from '../util/exponentialBackoff'; import { commonShouldJobContinue } from './helpers/commonShouldJobContinue'; -import { sleepFor413RetryAfterTimeIfApplicable } from './helpers/sleepFor413RetryAfterTimeIfApplicable'; +import { sleepFor413RetryAfterTime } from './helpers/sleepFor413RetryAfterTime'; import type { MessageModel } from '../models/messages'; import { getMessageById } from '../messages/getMessageById'; import type { ConversationModel } from '../models/conversations'; @@ -20,10 +20,8 @@ import { getSendOptions } from '../util/getSendOptions'; import { SignalService as Proto } from '../protobuf'; import { handleMessageSend } from '../util/handleMessageSend'; import type { CallbackResultType } from '../textsecure/Types.d'; -import { HTTPError } from '../textsecure/Errors'; import { isSent } from '../messages/MessageSendState'; import { getLastChallengeError, isOutgoing } from '../state/selectors/message'; -import { parseIntWithFallback } from '../util/parseIntWithFallback'; import * as Errors from '../types/errors'; import type { AttachmentType, @@ -38,6 +36,7 @@ import type { ParsedJob } from './types'; import { JobQueue } from './JobQueue'; import { jobQueueDatabaseStore } from './JobQueueDatabaseStore'; import { Job } from './Job'; +import { getHttpErrorCode } from './helpers/getHttpErrorCode'; const { loadAttachmentData, @@ -338,15 +337,12 @@ export class NormalMessageSendJobQueue extends JobQueue = []; let serverAskedUsToStop = false; - let maybe413Error: undefined | Error; + let retryAfterError: unknown; messageSendErrors.forEach((messageSendError: unknown) => { formattedMessageSendErrors.push(Errors.toLogFormat(messageSendError)); - if (!(messageSendError instanceof HTTPError)) { - return; - } - switch (parseIntWithFallback(messageSendError.code, -1)) { + switch (getHttpErrorCode(messageSendError)) { case 413: - maybe413Error ||= messageSendError; + retryAfterError ||= messageSendError; break; case 508: serverAskedUsToStop = true; @@ -370,9 +366,9 @@ export class NormalMessageSendJobQueue extends JobQueue { + it('returns -1 if not passed an object', () => { + assert.strictEqual(getHttpErrorCode(undefined), -1); + assert.strictEqual(getHttpErrorCode(null), -1); + assert.strictEqual(getHttpErrorCode(404), -1); + }); + + it('returns -1 if passed an object lacking a valid code', () => { + assert.strictEqual(getHttpErrorCode({}), -1); + assert.strictEqual(getHttpErrorCode({ code: 'garbage' }), -1); + assert.strictEqual( + getHttpErrorCode({ httpError: { code: 'garbage' } }), + -1 + ); + }); + + it('returns the top-level error code if it exists', () => { + assert.strictEqual(getHttpErrorCode({ code: 404 }), 404); + assert.strictEqual(getHttpErrorCode({ code: '404' }), 404); + }); + + it('returns a nested error code if available', () => { + assert.strictEqual(getHttpErrorCode({ httpError: { code: 404 } }), 404); + assert.strictEqual(getHttpErrorCode({ httpError: { code: '404' } }), 404); + }); + + it('"prefers" the first valid error code it finds if there is ambiguity', () => { + assert.strictEqual( + getHttpErrorCode({ code: '404', httpError: { code: 999 } }), + 404 + ); + assert.strictEqual( + getHttpErrorCode({ code: 'garbage', httpError: { code: 404 } }), + 404 + ); + }); +}); diff --git a/ts/test-node/jobs/helpers/sleepFor413RetryAfterTimeIfApplicable_test.ts b/ts/test-node/jobs/helpers/sleepFor413RetryAfterTime_test.ts similarity index 65% rename from ts/test-node/jobs/helpers/sleepFor413RetryAfterTimeIfApplicable_test.ts rename to ts/test-node/jobs/helpers/sleepFor413RetryAfterTime_test.ts index e012587ab152..cc03e86d59e4 100644 --- a/ts/test-node/jobs/helpers/sleepFor413RetryAfterTimeIfApplicable_test.ts +++ b/ts/test-node/jobs/helpers/sleepFor413RetryAfterTime_test.ts @@ -6,7 +6,7 @@ import * as sinon from 'sinon'; import { HTTPError } from '../../../textsecure/Errors'; import * as durations from '../../../util/durations'; -import { sleepFor413RetryAfterTimeIfApplicable } from '../../../jobs/helpers/sleepFor413RetryAfterTimeIfApplicable'; +import { sleepFor413RetryAfterTime } from '../../../jobs/helpers/sleepFor413RetryAfterTime'; describe('sleepFor413RetryAfterTimeIfApplicable', () => { const createLogger = () => ({ info: sinon.spy() }); @@ -23,39 +23,28 @@ describe('sleepFor413RetryAfterTimeIfApplicable', () => { sandbox.restore(); }); - it('does nothing if not passed a 413 HTTP error', async () => { + it('does nothing if no time remains', async () => { const log = createLogger(); - const errors = [ - undefined, - new Error('Normal error'), - new HTTPError('Uh oh', { code: 422, headers: {}, response: {} }), - ]; await Promise.all( - errors.map(async err => { - await sleepFor413RetryAfterTimeIfApplicable({ - err, + [-1, 0].map(timeRemaining => + sleepFor413RetryAfterTime({ + err: {}, log, - timeRemaining: 1234, - }); - }) + timeRemaining, + }) + ) ); sinon.assert.notCalled(log.info); }); - it('waits for 1 second if receiving a 413 HTTP error without a Retry-After header', async () => { - const err = new HTTPError('Slow down', { - code: 413, - headers: {}, - response: {}, - }); - + it('waits for 1 second if the error lacks Retry-After info', async () => { let done = false; (async () => { - await sleepFor413RetryAfterTimeIfApplicable({ - err, + await sleepFor413RetryAfterTime({ + err: {}, log: createLogger(), timeRemaining: 1234, }); @@ -69,7 +58,7 @@ describe('sleepFor413RetryAfterTimeIfApplicable', () => { assert.isTrue(done); }); - it('waits for Retry-After seconds if receiving a 413', async () => { + it('finds the Retry-After header on an HTTPError', async () => { const err = new HTTPError('Slow down', { code: 413, headers: { 'retry-after': '200' }, @@ -79,7 +68,7 @@ describe('sleepFor413RetryAfterTimeIfApplicable', () => { let done = false; (async () => { - await sleepFor413RetryAfterTimeIfApplicable({ + await sleepFor413RetryAfterTime({ err, log: createLogger(), timeRemaining: 123456789, @@ -94,6 +83,31 @@ describe('sleepFor413RetryAfterTimeIfApplicable', () => { assert.isTrue(done); }); + it('finds the Retry-After on an HTTPError nested under a wrapper error', async () => { + const httpError = new HTTPError('Slow down', { + code: 413, + headers: { 'retry-after': '200' }, + response: {}, + }); + + let done = false; + + (async () => { + await sleepFor413RetryAfterTime({ + err: { httpError }, + log: createLogger(), + timeRemaining: 123456789, + }); + done = true; + })(); + + await clock.tickAsync(199 * durations.SECOND); + assert.isFalse(done); + + await clock.tickAsync(2 * durations.SECOND); + assert.isTrue(done); + }); + it("won't wait longer than the remaining time", async () => { const err = new HTTPError('Slow down', { code: 413, @@ -104,7 +118,7 @@ describe('sleepFor413RetryAfterTimeIfApplicable', () => { let done = false; (async () => { - await sleepFor413RetryAfterTimeIfApplicable({ + await sleepFor413RetryAfterTime({ err, log: createLogger(), timeRemaining: 3 * durations.SECOND, @@ -124,7 +138,7 @@ describe('sleepFor413RetryAfterTimeIfApplicable', () => { response: {}, }); - sleepFor413RetryAfterTimeIfApplicable({ err, log, timeRemaining: 9999999 }); + sleepFor413RetryAfterTime({ err, log, timeRemaining: 9999999 }); await clock.nextAsync(); sinon.assert.calledOnce(log.info); diff --git a/ts/textsecure/Errors.ts b/ts/textsecure/Errors.ts index 1bff465055b2..059af813264b 100644 --- a/ts/textsecure/Errors.ts +++ b/ts/textsecure/Errors.ts @@ -99,9 +99,9 @@ export class OutgoingIdentityKeyError extends ReplayableError { } export class OutgoingMessageError extends ReplayableError { - identifier: string; + readonly identifier: string; - code?: number; + readonly httpError?: HTTPError; // Note: Data to resend message is no longer captured constructor( @@ -120,18 +120,20 @@ export class OutgoingMessageError extends ReplayableError { this.identifier = identifier; if (httpError) { - this.code = httpError.code; + this.httpError = httpError; appendStack(this, httpError); } } + + get code(): undefined | number { + return this.httpError?.code; + } } export class SendMessageNetworkError extends ReplayableError { - code: number; + readonly identifier: string; - identifier: string; - - responseHeaders?: HeaderListType | undefined; + readonly httpError: HTTPError; constructor(identifier: string, _m: unknown, httpError: HTTPError) { super({ @@ -140,11 +142,18 @@ export class SendMessageNetworkError extends ReplayableError { }); [this.identifier] = identifier.split('.'); - this.code = httpError.code; - this.responseHeaders = httpError.responseHeaders; + this.httpError = httpError; appendStack(this, httpError); } + + get code(): number { + return this.httpError.code; + } + + get responseHeaders(): undefined | HeaderListType { + return this.httpError.responseHeaders; + } } export type SendMessageChallengeData = { @@ -153,10 +162,10 @@ export type SendMessageChallengeData = { }; export class SendMessageChallengeError extends ReplayableError { - public code: number; - public identifier: string; + public readonly httpError: HTTPError; + public readonly data: SendMessageChallengeData | undefined; public readonly retryAfter: number; @@ -168,7 +177,8 @@ export class SendMessageChallengeError extends ReplayableError { }); [this.identifier] = identifier.split('.'); - this.code = httpError.code; + this.httpError = httpError; + this.data = httpError.response as SendMessageChallengeData; const headers = httpError.responseHeaders || {}; @@ -177,6 +187,10 @@ export class SendMessageChallengeError extends ReplayableError { appendStack(this, httpError); } + + get code(): number { + return this.httpError.code; + } } export class SendMessageProtoError extends Error implements CallbackResultType { @@ -244,7 +258,7 @@ export class SignedPreKeyRotationError extends ReplayableError { } export class MessageError extends ReplayableError { - code: number; + readonly httpError: HTTPError; constructor(_m: unknown, httpError: HTTPError) { super({ @@ -252,16 +266,20 @@ export class MessageError extends ReplayableError { message: httpError.message, }); - this.code = httpError.code; + this.httpError = httpError; appendStack(this, httpError); } + + get code(): number { + return this.httpError.code; + } } export class UnregisteredUserError extends Error { - identifier: string; + readonly identifier: string; - code: number; + readonly httpError: HTTPError; constructor(identifier: string, httpError: HTTPError) { const { message } = httpError; @@ -278,10 +296,14 @@ export class UnregisteredUserError extends Error { } this.identifier = identifier; - this.code = httpError.code; + this.httpError = httpError; appendStack(this, httpError); } + + get code(): number { + return this.httpError.code; + } } export class ConnectTimeoutError extends Error {}