fix: Treat 413 HTTP status as recoverable for events. (#348)

Author: kinyoklion

packages/shared/common/__tests__/internal/events/EventProcessor.test.ts

@@ -658,7 +658,9 @@ describe('given an event processor', () => {
     await expect(eventProcessor.flush()).rejects.toThrow('some error');
 
     eventProcessor.sendEvent(new InputIdentifyEvent(Context.fromLDContext(user)));
-    await expect(eventProcessor.flush()).rejects.toThrow(/SDK key is invalid/);
+    await expect(eventProcessor.flush()).rejects.toThrow(
+      'Events cannot be posted because a permanent error has been encountered.',
+    );
   });
 
   it('swallows errors from failed background flush', async () => {

packages/shared/common/src/errors.ts

@@ -43,9 +43,29 @@ export class LDClientError extends Error {
   }
 }
 
+/**
+ * Check if the HTTP error is recoverable. This will return false if a request
+ * made with any payload could not recover. If the reason for the failure
+ * is payload specific, for instance a payload that is too large, then
+ * it could recover with a different payload.
+ */
 export function isHttpRecoverable(status: number) {
   if (status >= 400 && status < 500) {
     return status === 400 || status === 408 || status === 429;
   }
   return true;
 }
+
+/**
+ * Returns true if the status could recover for a different payload.
+ *
+ * When used with event processing this indicates that we should discard
+ * the payload, but that a subsequent payload may succeed. Therefore we should
+ * not stop event processing.
+ */
+export function isHttpLocallyRecoverable(status: number) {
+  if (status === 413) {
+    return true;
+  }
+  return isHttpRecoverable(status);
+}

packages/shared/common/src/internal/events/EventProcessor.ts

@@ -168,7 +168,11 @@ export default class EventProcessor implements LDEventProcessor {
 
   async flush(): Promise<void> {
     if (this.shutdown) {
-      throw new LDInvalidSDKKeyError('Events cannot be posted because SDK key is invalid');
+      throw new LDInvalidSDKKeyError(
+        'Events cannot be posted because a permanent error has been encountered. ' +
+          'This is most likely an invalid SDK key. The specific error information ' +
+          'is logged independently.',
+      );
     }
 
     const eventsToFlush = this.queue;

packages/shared/common/src/internal/events/EventSender.test.ts

@@ -196,6 +196,23 @@ describe('given an event sender', () => {
     });
   });
 
+  it('given a result for too large of a payload', async () => {
+    setupMockFetch(413);
+    eventSenderResult = await eventSender.sendEventData(
+      LDEventType.AnalyticsEvents,
+      testEventData1,
+    );
+
+    const errorMessage = `Received error 413 for event posting - giving up permanently`;
+
+    const { status, error } = eventSenderResult;
+
+    expect(mockFetch).toHaveBeenCalledTimes(1);
+    expect(status).toEqual(LDDeliveryStatus.Failed);
+    expect(error.name).toEqual('LaunchDarklyUnexpectedResponseError');
+    expect(error.message).toEqual(errorMessage);
+  });
+
   describe.each([401, 403])('given unrecoverable errors', (responseStatusCode) => {
     beforeEach(async () => {
       setupMockFetch(responseStatusCode);

packages/shared/common/src/internal/events/EventSender.ts

@@ -5,7 +5,11 @@ import {
   LDEventSenderResult,
   LDEventType,
 } from '../../api/subsystem';
-import { isHttpRecoverable, LDUnexpectedResponseError } from '../../errors';
+import {
+  isHttpLocallyRecoverable,
+  isHttpRecoverable,
+  LDUnexpectedResponseError,
+} from '../../errors';
 import { ClientContext } from '../../options';
 import { defaultHeaders, httpErrorMessage, sleep } from '../../utils';
 
@@ -80,7 +84,15 @@ export default class EventSender implements LDEventSender {
       );
 
       if (!isHttpRecoverable(status)) {
-        tryRes.status = LDDeliveryStatus.FailedAndMustShutDown;
+        // If the HTTP request isn't recoverable. Meaning if we made the same request it
+        // would not recover, then we check if a different request could recover.
+        // If a different request could not recover, then we shutdown. If a different request could
+        // recover, then we just don't retry this specific request.
+        if (!isHttpLocallyRecoverable(status)) {
+          tryRes.status = LDDeliveryStatus.FailedAndMustShutDown;
+        } else {
+          tryRes.status = LDDeliveryStatus.Failed;
+        }
         tryRes.error = error;
         return tryRes;
       }