From 26290ffd8bacb873351bcc40c2773269c7c53c83 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 17 Oct 2022 16:53:54 +1100 Subject: [PATCH] fix: mark attachment as failure if we get a 404 --- .../apis/file_server_api/FileServerApi.ts | 3 ++ .../open_group_api/sogsv3/sogsV3FetchFile.ts | 8 ++- ts/session/apis/snode_api/onions.ts | 8 +++ ts/session/onions/onionSend.ts | 22 ++++++-- ts/session/utils/AttachmentsDownload.ts | 51 ++++++++----------- 5 files changed, 55 insertions(+), 37 deletions(-) diff --git a/ts/session/apis/file_server_api/FileServerApi.ts b/ts/session/apis/file_server_api/FileServerApi.ts index 5d8b0cefa..6481c085b 100644 --- a/ts/session/apis/file_server_api/FileServerApi.ts +++ b/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)); diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3FetchFile.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3FetchFile.ts index 8b6febaa1..3d0703fb8 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3FetchFile.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3FetchFile.ts @@ -20,6 +20,7 @@ export async function fetchBinaryFromSogsWithOnionV4(sendOptions: { headers: Record | null; roomId: string; fileId: string; + throwError: boolean; }): Promise { 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; }; diff --git a/ts/session/apis/snode_api/onions.ts b/ts/session/apis/snode_api/onions.ts index a1672b480..f7a471923 100644 --- a/ts/session/apis/snode_api/onions.ts +++ b/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. diff --git a/ts/session/onions/onionSend.ts b/ts/session/onions/onionSend.ts index 097305f21..f5fe4759c 100644 --- a/ts/session/onions/onionSend.ts +++ b/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 - throw new pRetry.AbortError( + window.log.warn( `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 { - 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 ); diff --git a/ts/session/utils/AttachmentsDownload.ts b/ts/session/utils/AttachmentsDownload.ts index eea2a697b..acdec08ea 100644 --- a/ts/session/utils/AttachmentsDownload.ts +++ b/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()}` );