Merge pull request #18237 from jjcoffee/fix-16637

Prevent duplicate requests on network failure
Expensify · May 11, 2023 · cd2025c · cd2025c
2 parents 4d23e82 + 22b971d
commit cd2025c
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 6 deletions.
diff --git a/src/CONST.js b/src/CONST.js
@@ -579,6 +579,7 @@ const CONST = {
     },
     JSON_CODE: {
         SUCCESS: 200,
+        BAD_REQUEST: 400,
         NOT_AUTHENTICATED: 407,
         EXP_ERROR: 666,
         MANY_WRITES_ERROR: 665,
@@ -612,12 +613,14 @@ const CONST = {
         SAFARI_CANNOT_PARSE_RESPONSE: 'cannot parse response',
         GATEWAY_TIMEOUT: 'Gateway Timeout',
         EXPENSIFY_SERVICE_INTERRUPTED: 'Expensify service interrupted',
+        DUPLICATE_RECORD: 'A record already exists with this ID',
     },
     ERROR_TYPE: {
         SOCKET: 'Expensify\\Auth\\Error\\Socket',
     },
     ERROR_TITLE: {
         SOCKET: 'Issue connecting to database',
+        DUPLICATE_RECORD: '400 Unique Constraints Violation',
     },
     NETWORK: {
         METHOD: {

diff --git a/src/libs/HttpUtils.js b/src/libs/HttpUtils.js
@@ -87,6 +87,15 @@ function processHTTPRequest(url, method = 'get', body = null, canCancel = true,
             return response.json();
         })
         .then((response) => {
+            // Some retried requests will result in a "Unique Constraints Violation" error from the server, which just means the record already exists
+            if (response.jsonCode === CONST.JSON_CODE.BAD_REQUEST && response.message === CONST.ERROR_TITLE.DUPLICATE_RECORD) {
+                throw new HttpsError({
+                    message: CONST.ERROR.DUPLICATE_RECORD,
+                    status: CONST.JSON_CODE.BAD_REQUEST,
+                    title: CONST.ERROR_TITLE.DUPLICATE_RECORD,
+                });
+            }
+
             // Auth is down or timed out while making a request
             if (response.jsonCode === CONST.JSON_CODE.EXP_ERROR && response.title === CONST.ERROR_TITLE.SOCKET && response.type === CONST.ERROR_TYPE.SOCKET) {
                 throw new HttpsError({

diff --git a/src/libs/Middleware/Logging.js b/src/libs/Middleware/Logging.js
@@ -112,6 +112,9 @@ function Logging(response, request) {
                 Log.hmmm('[Network] API request error: Expensify service interrupted or timed out', logParams);
             } else if (error.message === CONST.ERROR.THROTTLED) {
                 Log.hmmm('[Network] API request error: Expensify API throttled the request', logParams);
+            } else if (error.message === CONST.ERROR.DUPLICATE_RECORD) {
+                // Duplicate records can happen when a large upload is interrupted and we need to retry to see if the original request completed
+                Log.info('[Network] API request error: A record already exists with this ID', false, logParams);
             } else {
                 // If we get any error that is not known log an alert so we can learn more about it and document it here.
                 Log.alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown API request error caught while processing request`, logParams, false);

diff --git a/src/libs/Middleware/Reauthentication.js b/src/libs/Middleware/Reauthentication.js
@@ -49,12 +49,12 @@ function Reauthentication(response, request, isFromSequentialQueue) {
                 return;
             }
 
-            if (NetworkStore.isOffline()) {
-                // If we are offline and somehow handling this response we do not want to reauthenticate
-                throw new Error('Unable to reauthenticate because we are offline');
-            }
-
             if (data.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) {
+                if (NetworkStore.isOffline()) {
+                    // If we are offline and somehow handling this response we do not want to reauthenticate
+                    throw new Error('Unable to reauthenticate because we are offline');
+                }
+
                 // There are some API requests that should not be retried when there is an auth failure like
                 // creating and deleting logins. In those cases, they should handle the original response instead
                 // of the new response created by handleExpiredAuthToken.

diff --git a/src/libs/Network/SequentialQueue.js b/src/libs/Network/SequentialQueue.js
@@ -45,7 +45,8 @@ function process() {
         })
         .catch((error) => {
             // On sign out we cancel any in flight requests from the user. Since that user is no longer signed in their requests should not be retried.
-            if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
+            // Duplicate records don't need to be retried as they just mean the record already exists on the server
+            if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) {
                 PersistedRequests.remove(requestToProcess);
                 RequestThrottle.clear();
                 return process();