fix: retry transient attachment downloads

ts/components/conversation/Image.tsx

@@ -92,6 +92,7 @@ export const Image = (props: Props) => {
 
   const width = isNumber(_width) ? `${_width}px` : _width;
   const height = isNumber(_height) ? `${_height}px` : _height;
+  const attachmentFailed = Boolean((attachment as AttachmentTypeWithPath).error) || url === '';
 
   useEffect(() => {
     if (mounted && url === '') {
@@ -110,7 +111,7 @@ export const Image = (props: Props) => {
     }
   }, [imageBroken, loading, mounted, onErrorUrlFiltering, pending, url, urlToLoad]);
 
-  if (mounted && imageBroken) {
+  if (mounted && (imageBroken || attachmentFailed)) {
     return (
       <MessageGenericAttachment
         attachment={attachment as AttachmentTypeWithPath}

ts/models/message.ts

@@ -823,6 +823,7 @@ export class MessageModel extends Model<MessageAttributes> {
       thumbnail,
       fileName,
       caption,
+      error,
       isVoiceMessage: isVoiceMessageFromDb,
     } = attachment;
 
@@ -843,6 +844,7 @@ export class MessageModel extends Model<MessageAttributes> {
       fileSize: size ? filesize(size, { base: 10 }) : null,
       isVoiceMessage: isVoiceMessageBool,
       pending: Boolean(pending),
+      error,
       url: path ? getAbsoluteAttachmentPath(path) : '',
       screenshot: screenshot
         ? {

ts/session/utils/AttachmentDownloadRetry.ts

@@ -0,0 +1,111 @@
+import type { AttachmentDownloadMessageDetails } from '../../types/sqlSharedTypes';
+import { was404Error } from '../apis/snode_api/onions';
+import * as Constants from '../constants';
+
+type AttachmentDownloadJobType = AttachmentDownloadMessageDetails['type'];
+
+const RETRY_BACKOFF_BY_ATTEMPT = new Map<number, number>([
+  [1, Constants.DURATION.SECONDS * 30],
+  [2, Constants.DURATION.MINUTES * 30],
+  [3, Constants.DURATION.HOURS * 6],
+]);
+
+const MAX_RETRY_BACKOFF = Constants.DURATION.HOURS * 6;
+const MAX_RETRYABLE_ATTACHMENT_ATTEMPT = 10;
+const MAX_RETRYABLE_PREVIEW_ATTEMPT = 2;
+
+const TERMINAL_ERROR_MESSAGES = [
+  'DownloadFromFileServer: fileId is empty or not a file server url',
+  'Attachment is not raw but we do not have a key to decode it',
+  'Attachment expected size is 0',
+];
+
+export function getAttachmentDownloadRetryBackoff(attempt: number): number {
+  return RETRY_BACKOFF_BY_ATTEMPT.get(attempt) || MAX_RETRY_BACKOFF;
+}
+
+export function isAttachmentDownload404Error(error: unknown): boolean {
+  if ((error as any)?.code === 404) {
+    return true;
+  }
+
+  const message = (error as any)?.message;
+  if (typeof message !== 'string') {
+    return false;
+  }
+
+  return was404Error({ message } as Error);
+}
+
+export function isAttachmentDownloadTerminalError(error: unknown): boolean {
+  if (isAttachmentDownload404Error(error)) {
+    return true;
+  }
+
+  const message = (error as any)?.message;
+  if (typeof message !== 'string') {
+    return false;
+  }
+
+  return TERMINAL_ERROR_MESSAGES.some(terminalMessage => message.includes(terminalMessage));
+}
+
+export function hasAttachmentDownloadRetriesRemaining(
+  attempt: number,
+  type: AttachmentDownloadJobType = 'attachment'
+): boolean {
+  return (
+    attempt <=
+    (type === 'attachment' ? MAX_RETRYABLE_ATTACHMENT_ATTEMPT : MAX_RETRYABLE_PREVIEW_ATTEMPT)
+  );
+}
+
+export function shouldShowAttachmentDownloadRetryFailed(attempt: number): boolean {
+  return attempt >= 3;
+}
+
+export function getAttachmentDownloadRetryDecision({
+  error,
+  previousAttempts,
+  type,
+}: {
+  error: unknown;
+  previousAttempts: number;
+  type: AttachmentDownloadJobType;
+}) {
+  const attempt = (previousAttempts || 0) + 1;
+  const shouldRetry =
+    !isAttachmentDownloadTerminalError(error) &&
+    hasAttachmentDownloadRetriesRemaining(attempt, type);
+
+  return {
+    attempt,
+    retryBackoff: getAttachmentDownloadRetryBackoff(attempt),
+    shouldRetry,
+    shouldMarkAttachmentFailed:
+      shouldRetry && type === 'attachment' && shouldShowAttachmentDownloadRetryFailed(attempt),
+  };
+}
+
+export function buildAttachmentDownloadRetryJob(
+  job: any,
+  attempt: number,
+  retryBackoff: number,
+  now = Date.now()
+) {
+  return {
+    ...job,
+    pending: 0,
+    attempts: attempt,
+    timestamp: now + retryBackoff,
+  };
+}
+
+export function markAttachmentDownloadFailed(attachment: any) {
+  return {
+    ...attachment,
+    error: true,
+    pending: false,
+    downloadJobId: undefined,
+  };
+}

ts/session/utils/AttachmentsDownload.ts

@@ -8,20 +8,19 @@ import { downloadAttachmentFs, downloadAttachmentSogsV3 } from '../../receiver/a
 import { initializeAttachmentLogic, processNewAttachment } from '../../types/MessageAttachment';
 import { getAttachmentMetadata } from '../../types/message/initializeAttachmentMetadata';
 import { AttachmentDownloadMessageDetails } from '../../types/sqlSharedTypes';
-import { was404Error } from '../apis/snode_api/onions';
 import * as Constants from '../constants';
+import {
+  buildAttachmentDownloadRetryJob,
+  getAttachmentDownloadRetryDecision,
+  isAttachmentDownload404Error,
+  markAttachmentDownloadFailed,
+} from './AttachmentDownloadRetry';
 
 // 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;
 
 const TICK_INTERVAL = Constants.DURATION.MINUTES;
 
-const RETRY_BACKOFF = {
-  1: Constants.DURATION.SECONDS * 30,
-  2: Constants.DURATION.MINUTES * 30,
-  3: Constants.DURATION.HOURS * 6,
-};
-
 let enabled = false;
 let timeout: any;
 let logger: any;
@@ -180,7 +179,7 @@ async function _runJob(job: any) {
         : await downloadAttachmentFs(attachment);
     } catch (error) {
       // Attachments on the server expire after 60 days, then start returning 404
-      if (error && error.code === 404) {
+      if (isAttachmentDownload404Error(error)) {
         logger.warn(
           `_runJob: Got 404 from server, marking attachment from message ${found.idForLogging()} as permanent error`
         );
@@ -224,13 +223,17 @@ async function _runJob(job: any) {
 
     await _finishJob(found, id);
   } catch (error) {
-    const currentAttempt: 1 | 2 | 3 = (attempts || 0) + 1;
+    const retryDecision = getAttachmentDownloadRetryDecision({
+      error,
+      previousAttempts: attempts || 0,
+      type,
+    });
 
-    // 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)) {
+    // Stop immediately for terminal errors, and keep normal attachments retryable for longer than previews.
+    // 404s are terminal because attachments expire server-side after 60 days.
+    if (!retryDecision.shouldRetry) {
       logger.error(
-        `_runJob: ${currentAttempt} failed attempts, marking attachment ${id} from message ${found?.idForLogging()} as permanent error:`,
+        `_runJob: terminal failure, marking attachment ${id} from message ${found?.idForLogging()} as permanent error:`,
         error && error.message ? error.message : error
       );
 
@@ -249,48 +252,55 @@ async function _runJob(job: any) {
     }
 
     logger.error(
-      `_runJob: Failed to download attachment type ${type} for message ${found?.idForLogging()}, attempt ${currentAttempt}:`,
+      `_runJob: Failed to download attachment type ${type} for message ${found?.idForLogging()}, attempt ${retryDecision.attempt}:`,
       error && error.message ? error.message : error
     );
 
-    const failedJob = {
-      ...job,
-      pending: 0,
-      attempts: currentAttempt,
-      timestamp: Date.now() + RETRY_BACKOFF[currentAttempt],
-    };
+    if (retryDecision.shouldMarkAttachmentFailed) {
+      found = await Data.getMessageById(messageId);
+      try {
+        _addAttachmentToMessage(found, _markAttachmentAsError(attachment), { type, index });
+        await _commitMessageIfNeeded(found || null);
+      } catch (e) {
+        // just swallow this exception. We don't want to throw it from the catch block here as this will end up being a Uncaught global promise
+      }
+    }
 
-    await Data.saveAttachmentDownloadJob(failedJob);
+    await Data.saveAttachmentDownloadJob(
+      buildAttachmentDownloadRetryJob(job, retryDecision.attempt, retryDecision.retryBackoff)
+    );
 
     delete _activeAttachmentDownloadJobs[id];
     void _maybeStartJob();
   }
 }
 
 async function _finishJob(message: MessageModel | null, id: string) {
-  if (message) {
-    const conversation = message.getConversation();
-    if (conversation) {
-      await message.commit();
-    }
-  }
+  await _commitMessageIfNeeded(message);
 
   await Data.removeAttachmentDownloadJob(id);
 
   delete _activeAttachmentDownloadJobs[id];
   await _maybeStartJob();
 }
 
+async function _commitMessageIfNeeded(message: MessageModel | null) {
+  if (!message) {
+    return;
+  }
+
+  const conversation = message.getConversation();
+  if (conversation) {
+    await message.commit();
+  }
+}
+
 function getActiveJobCount() {
   return Object.keys(_activeAttachmentDownloadJobs).length;
 }
 
 function _markAttachmentAsError(attachment: any) {
-  return {
-    ...omit(attachment, ['key', 'digest', 'id']),
-    error: true,
-    pending: false,
-  };
+  return markAttachmentDownloadFailed(omit(attachment, ['toJSON']));
 }
 
 function _addAttachmentToMessage(

ts/test/session/unit/models/MessageModel_test.ts

@@ -0,0 +1,54 @@
+import { expect } from 'chai';
+import Sinon from 'sinon';
+
+import { MessageModel } from '../../../../models/message';
+import * as MessageAttachment from '../../../../types/MessageAttachment';
+
+describe('MessageModel', () => {
+  afterEach(() => {
+    Sinon.restore();
+  });
+
+  describe('getPropsForAttachment', () => {
+    it('propagates failed attachment state without exposing retry metadata', () => {
+      Sinon.stub(MessageAttachment, 'getAbsoluteAttachmentPath').returns('/attachment/path');
+
+      const message = new MessageModel({
+        conversationId: 'conversation-id',
+        source: 'source-id',
+        type: 'incoming',
+      });
+      const props = message.getPropsForAttachment({
+        contentType: 'image/png',
+        digest: 'digest',
+        error: true,
+        fileName: 'image.png',
+        key: 'key',
+        path: '',
+        pending: false,
+        screenshot: null,
+        thumbnail: null,
+        url: 'https://file.getsession.org/file/123',
+      } as any);
+
+      expect(props).to.deep.equal({
+        contentType: 'image/png',
+        caption: undefined,
+        size: 0,
+        width: 0,
+        height: 0,
+        path: '',
+        fileName: 'image.png',
+        fileSize: null,
+        isVoiceMessage: false,
+        pending: false,
+        error: true,
+        url: '',
+        screenshot: null,
+        thumbnail: null,
+      });
+      expect(props).to.not.have.property('key');
+      expect(props).to.not.have.property('digest');
+    });
+  });
+});