Improve handling of 413 HTTP responses

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

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<void> {
-  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;
 }

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<LoggerType, 'info'>;
   timeRemaining: number;
 }>): Promise<void> {
-  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;
+}

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<NormalMessageSendJobData
     } catch (err: unknown) {
       const formattedMessageSendErrors: Array<string> = [];
       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<NormalMessageSendJobData
         return;
       }
 
-      if (!isFinalAttempt) {
-        await sleepFor413RetryAfterTimeIfApplicable({
-          err: maybe413Error,
+      if (!isFinalAttempt && retryAfterError) {
+        await sleepFor413RetryAfterTime({
+          err: retryAfterError,
           log,
           timeRemaining,
         });

ts/test-node/jobs/helpers/getHttpErrorCode_test.ts

@@ -0,0 +1,44 @@
+// Copyright 2021 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import { assert } from 'chai';
+
+import { getHttpErrorCode } from '../../../jobs/helpers/getHttpErrorCode';
+
+describe('getHttpErrorCode', () => {
+  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
+    );
+  });
+});

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

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 {}