Merge pull request #2559 from Bilb/fix-attachments-failure-mark-as-error

fix: mark attachment as failure if we get a 404

ts/session/apis/file_server_api/FileServerApi.ts

@@ -77,10 +77,13 @@ export const downloadFileFromFileServer = async (
   if (window.sessionFeatureFlags?.debug.debugFileServerRequests) {
     window.log.info(`about to try to download fsv2: "${urlToGet}"`);
   }
+
+  // this throws if we get a 404 from the file server
   const result = await OnionSending.getBinaryViaOnionV4FromFileServer({
     abortSignal: new AbortController().signal,
     endpoint: urlToGet,
     method: 'GET',
+    throwError: true,
   });
   if (window.sessionFeatureFlags?.debug.debugFileServerRequests) {
     window.log.info(`download fsv2: "${urlToGet} got result:`, JSON.stringify(result));

ts/session/apis/open_group_api/sogsv3/sogsV3FetchFile.ts

@@ -20,6 +20,7 @@ export async function fetchBinaryFromSogsWithOnionV4(sendOptions: {
   headers: Record<string, any> | null;
   roomId: string;
   fileId: string;
+  throwError: boolean;
 }): Promise<Uint8Array | null> {
   const {
     serverUrl,
@@ -30,6 +31,7 @@ export async function fetchBinaryFromSogsWithOnionV4(sendOptions: {
     doNotIncludeOurSogsHeaders,
     roomId,
     fileId,
+    throwError,
   } = sendOptions;
 
   const stringifiedBody = null;
@@ -62,12 +64,12 @@ export async function fetchBinaryFromSogsWithOnionV4(sendOptions: {
       body: stringifiedBody,
       useV4: true,
     },
-    false,
+    throwError,
     abortSignal
   );
 
   if (!res?.bodyBinary) {
-    window.log.info('fetchBinaryFromSogsWithOnionV4 no binary content');
+    window.log.info('fetchBinaryFromSogsWithOnionV4 no binary content with code', res?.status_code);
     return null;
   }
   return res.bodyBinary;
@@ -169,6 +171,7 @@ const sogsV3FetchPreview = async (
     doNotIncludeOurSogsHeaders: true,
     roomId: roomInfos.roomId,
     fileId: roomInfos.imageID,
+    throwError: false,
   });
   if (fetched && fetched.byteLength) {
     return fetched;
@@ -198,6 +201,7 @@ export const sogsV3FetchFileByFileID = async (
     doNotIncludeOurSogsHeaders: true,
     roomId: roomInfos.roomId,
     fileId,
+    throwError: true,
   });
   return fetched && fetched.byteLength ? fetched : null;
 };

ts/session/apis/snode_api/onions.ts

@@ -27,6 +27,14 @@ export const resetSnodeFailureCount = () => {
 const snodeFailureThreshold = 3;
 
 export const OXEN_SERVER_ERROR = 'Oxen Server error';
+
+// Not ideal, but a pRetry.AbortError only lets us customize the message, and not the code
+const errorContent404 = ': 404 ';
+export const was404Error = (error: Error) => error.message.includes(errorContent404);
+
+export const buildErrorMessageWithFailedCode = (prefix: string, code: number, suffix: string) =>
+  `${prefix}: ${code} ${suffix}`;
+
 /**
  * When sending a request over onion, we might get two status.
  * The first one, on the request itself, the other one in the json returned.

ts/session/onions/onionSend.ts

@@ -2,6 +2,7 @@
 
 import { OnionPaths } from '.';
 import {
+  buildErrorMessageWithFailedCode,
   FinalDestNonSnodeOptions,
   FinalRelayOptions,
   Onions,
@@ -212,10 +213,17 @@ const sendViaOnionV4ToNonSnodeWithRetries = async (
             };
           }
           if (foundStatusCode === 404) {
-            // this is most likely that a 404 won't fix itself. So just stop right here retries by throwing a non retryable error
+            window.log.warn(
-            throw new pRetry.AbortError(
               `Got 404 while sendViaOnionV4ToNonSnodeWithRetries with url:${url}. Stopping retries`
             );
+            // most likely, a 404 won't fix itself. So just stop right here retries by throwing a non retryable error
+            throw new pRetry.AbortError(
+              buildErrorMessageWithFailedCode(
+                'sendViaOnionV4ToNonSnodeWithRetries',
+                404,
+                `with url:${url}. Stopping retries`
+              )
+            );
           }
           // we consider those cases as an error, and trigger a retry (if possible), by throwing a non-abortable error
           throw new Error(
@@ -239,7 +247,7 @@ const sendViaOnionV4ToNonSnodeWithRetries = async (
       }
     );
   } catch (e) {
-    window?.log?.warn('sendViaOnionV4ToNonSnodeRetryable failed ', e.message);
+    window?.log?.warn('sendViaOnionV4ToNonSnodeRetryable failed ', e.message, throwErrors);
     if (throwErrors) {
       throw e;
     }
@@ -455,8 +463,9 @@ async function getBinaryViaOnionV4FromFileServer(sendOptions: {
   endpoint: string;
   method: string;
   abortSignal: AbortSignal;
+  throwError: boolean;
 }): Promise<OnionV4BinarySnodeResponse | null> {
-  const { endpoint, method, abortSignal } = sendOptions;
+  const { endpoint, method, abortSignal, throwError } = sendOptions;
   if (!endpoint.startsWith('/')) {
     throw new Error('endpoint needs a leading /');
   }
@@ -465,6 +474,9 @@ async function getBinaryViaOnionV4FromFileServer(sendOptions: {
   if (window.sessionFeatureFlags?.debug.debugFileServerRequests) {
     window.log.info(`getBinaryViaOnionV4FromFileServer fsv2: "${builtUrl} `);
   }
+
+  // this throws for a bunch of reasons.
+  // One of them, is if we get a 404 (i.e. the file server was reached but reported no such attachments exists)
   const res = await OnionSending.sendViaOnionV4ToNonSnodeWithRetries(
     fileServerPubKey,
     builtUrl,
@@ -474,7 +486,7 @@ async function getBinaryViaOnionV4FromFileServer(sendOptions: {
       body: null,
       useV4: true,
     },
-    false,
+    throwError,
     abortSignal
   );
 

ts/session/utils/AttachmentsDownload.ts

@@ -8,6 +8,7 @@ import { MessageModel } from '../../models/message';
 import { downloadAttachment, downloadAttachmentSogsV3 } from '../../receiver/attachments';
 import { initializeAttachmentLogic, processNewAttachment } from '../../types/MessageAttachment';
 import { getAttachmentMetadata } from '../../types/message/initializeAttachmentMetadata';
+import { was404Error } from '../apis/snode_api/onions';
 
 // this may cause issues if we increment that value to > 1, but only having one job will block the whole queue while one attachment is downloading
 const MAX_ATTACHMENT_JOB_PARALLELISM = 3;
@@ -175,6 +176,7 @@ async function _runJob(job: any) {
     let downloaded;
 
     try {
+      // those two functions throw if they get a 404
       if (isOpenGroupV2) {
         downloaded = await downloadAttachmentSogsV3(attachment, openGroupV2Details);
       } else {
@@ -189,14 +191,12 @@ async function _runJob(job: any) {
           } from message ${found.idForLogging()} as permanent error`
         );
 
-        await _finishJob(found, id);
-
         // Make sure to fetch the message from DB here right before writing it.
         // This is to avoid race condition where multiple attachments in a single message get downloaded at the same time,
         // and tries to update the same message.
         found = await Data.getMessageById(messageId);
-
         _addAttachmentToMessage(found, _markAttachmentAsError(attachment), { type, index });
+        await _finishJob(found, id);
 
         return;
       }
@@ -232,7 +232,9 @@ async function _runJob(job: any) {
     // tslint:disable: restrict-plus-operands
     const currentAttempt: 1 | 2 | 3 = (attempts || 0) + 1;
 
-    if (currentAttempt >= 3) {
+    // if we get a 404 error for attachment downloaded, we can safely assume that the attachment expired server-side.
+    // so there is no need to continue trying to download it.
+    if (currentAttempt >= 3 || was404Error(error)) {
       logger.error(
         `_runJob: ${currentAttempt} failed attempts, marking attachment ${id} from message ${found?.idForLogging()} as permament error:`,
         error && error.stack ? error.stack : error
@@ -321,40 +323,29 @@ function _addAttachmentToMessage(
     return;
   }
 
-  if (type === 'preview') {
+  // for quote and previews, if the attachment cannot be downloaded we just erase it from the message itself, so just the title or body is rendered
+  if (type === 'preview' || type === 'quote') {
+    if (type === 'quote') {
+      const quote = message.get('quote');
+      if (!quote) {
+        throw new Error("_addAttachmentToMessage: quote didn't exist");
+      }
+
+      delete message.attributes.quote.attachments;
+
+      return;
+    }
     const preview = message.get('preview');
     if (!preview || preview.length <= index) {
       throw new Error(`_addAttachmentToMessage: preview didn't exist or ${index} was too large`);
     }
-    const item = preview[index];
-    if (!item) {
-      throw new Error(`_addAttachmentToMessage: preview ${index} was falsey`);
-    }
-    _replaceAttachment(item, 'image', attachment, logPrefix);
-    return;
-  }
-
-  if (type === 'quote') {
-    const quote = message.get('quote');
-    if (!quote) {
-      throw new Error("_addAttachmentToMessage: quote didn't exist");
-    }
-    const { attachments } = quote;
-    if (!attachments || attachments.length <= index) {
-      throw new Error(
-        `_addAttachmentToMessage: quote attachments didn't exist or ${index} was too large`
-      );
-    }
-
-    const item = attachments[index];
-    if (!item) {
-      throw new Error(`_addAttachmentToMessage: attachment ${index} was falsey`);
-    }
-    _replaceAttachment(item, 'thumbnail', attachment, logPrefix);
 
+    delete message.attributes.preview[0].image;
     return;
   }
 
+  // for quote and previews, if the attachment cannot be downloaded we just erase it from the message itself, so just the title or body is rendered
+
   throw new Error(
     `_addAttachmentToMessage: Unknown job type ${type} for message ${message.idForLogging()}`
   );