diff --git a/ts/components/conversation/Image.tsx b/ts/components/conversation/Image.tsx index 37f865b8f1..2d4389d346 100644 --- a/ts/components/conversation/Image.tsx +++ b/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 ( { thumbnail, fileName, caption, + error, isVoiceMessage: isVoiceMessageFromDb, } = attachment; @@ -843,6 +844,7 @@ export class MessageModel extends Model { fileSize: size ? filesize(size, { base: 10 }) : null, isVoiceMessage: isVoiceMessageBool, pending: Boolean(pending), + error, url: path ? getAbsoluteAttachmentPath(path) : '', screenshot: screenshot ? { diff --git a/ts/session/utils/AttachmentDownloadRetry.ts b/ts/session/utils/AttachmentDownloadRetry.ts new file mode 100644 index 0000000000..748ba89d9b --- /dev/null +++ b/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([ + [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, + }; +} diff --git a/ts/session/utils/AttachmentsDownload.ts b/ts/session/utils/AttachmentsDownload.ts index 7b4f6f457e..c8698e6937 100644 --- a/ts/session/utils/AttachmentsDownload.ts +++ b/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,18 +252,23 @@ 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(); @@ -268,12 +276,7 @@ async function _runJob(job: any) { } 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); @@ -281,16 +284,23 @@ async function _finishJob(message: MessageModel | null, id: string) { 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( diff --git a/ts/test/session/unit/models/MessageModel_test.ts b/ts/test/session/unit/models/MessageModel_test.ts new file mode 100644 index 0000000000..d533996bc0 --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/ts/test/session/unit/utils/AttachmentDownloadRetry_test.ts b/ts/test/session/unit/utils/AttachmentDownloadRetry_test.ts new file mode 100644 index 0000000000..b963afe25a --- /dev/null +++ b/ts/test/session/unit/utils/AttachmentDownloadRetry_test.ts @@ -0,0 +1,235 @@ +import { expect } from 'chai'; + +import { + buildAttachmentDownloadRetryJob, + getAttachmentDownloadRetryBackoff, + getAttachmentDownloadRetryDecision, + hasAttachmentDownloadRetriesRemaining, + isAttachmentDownloadTerminalError, + isAttachmentDownload404Error, + markAttachmentDownloadFailed, + shouldShowAttachmentDownloadRetryFailed, +} from '../../../../session/utils/AttachmentDownloadRetry'; +import * as Constants from '../../../../session/constants'; + +describe('AttachmentDownloadRetry', () => { + describe('getAttachmentDownloadRetryBackoff', () => { + it('uses the configured delayed retry schedule', () => { + expect(getAttachmentDownloadRetryBackoff(1)).to.equal(Constants.DURATION.SECONDS * 30); + expect(getAttachmentDownloadRetryBackoff(2)).to.equal(Constants.DURATION.MINUTES * 30); + expect(getAttachmentDownloadRetryBackoff(3)).to.equal(Constants.DURATION.HOURS * 6); + }); + + it('clamps later retries to the maximum backoff', () => { + expect(getAttachmentDownloadRetryBackoff(4)).to.equal(Constants.DURATION.HOURS * 6); + expect(getAttachmentDownloadRetryBackoff(20)).to.equal(Constants.DURATION.HOURS * 6); + }); + }); + + describe('isAttachmentDownload404Error', () => { + it('detects 404 errors by code', () => { + expect(isAttachmentDownload404Error({ code: 404 })).to.equal(true); + }); + + it('detects 404 errors by onion error message', () => { + expect(isAttachmentDownload404Error(new Error('download failed: 404 missing'))).to.equal( + true + ); + }); + + it('does not classify transient errors as 404 errors', () => { + expect(isAttachmentDownload404Error(new Error('network timeout'))).to.equal(false); + }); + }); + + describe('isAttachmentDownloadTerminalError', () => { + it('keeps transient failures retryable', () => { + expect(isAttachmentDownloadTerminalError(new Error('network timeout'))).to.equal(false); + }); + + it('stops retrying 404 failures immediately', () => { + expect(isAttachmentDownloadTerminalError(new Error('download failed: 404 missing'))).to.equal( + true + ); + }); + + it('stops retrying malformed attachment pointers', () => { + expect( + isAttachmentDownloadTerminalError( + new Error('DownloadFromFileServer: fileId is empty or not a file server url') + ) + ).to.equal(true); + }); + }); + + describe('shouldShowAttachmentDownloadRetryFailed', () => { + it('keeps the existing pending state for early transient failures', () => { + expect(shouldShowAttachmentDownloadRetryFailed(1)).to.equal(false); + expect(shouldShowAttachmentDownloadRetryFailed(2)).to.equal(false); + }); + + it('allows later transient failures to show as failed while the job keeps retrying', () => { + expect(shouldShowAttachmentDownloadRetryFailed(3)).to.equal(true); + expect(shouldShowAttachmentDownloadRetryFailed(4)).to.equal(true); + }); + }); + + describe('getAttachmentDownloadRetryDecision', () => { + it('keeps transient attachment failures retryable and visible after the display threshold', () => { + expect( + getAttachmentDownloadRetryDecision({ + error: new Error('network timeout'), + previousAttempts: 2, + type: 'attachment', + }) + ).to.deep.equal({ + attempt: 3, + retryBackoff: Constants.DURATION.HOURS * 6, + shouldRetry: true, + shouldMarkAttachmentFailed: true, + }); + }); + + it('keeps early transient preview failures retryable without marking the message attachment failed', () => { + expect( + getAttachmentDownloadRetryDecision({ + error: new Error('network timeout'), + previousAttempts: 1, + type: 'preview', + }) + ).to.deep.equal({ + attempt: 2, + retryBackoff: Constants.DURATION.MINUTES * 30, + shouldRetry: true, + shouldMarkAttachmentFailed: false, + }); + }); + + it('keeps transient preview failures terminal at the old retry boundary', () => { + expect( + getAttachmentDownloadRetryDecision({ + error: new Error('network timeout'), + previousAttempts: 2, + type: 'preview', + }) + ).to.deep.equal({ + attempt: 3, + retryBackoff: Constants.DURATION.HOURS * 6, + shouldRetry: false, + shouldMarkAttachmentFailed: false, + }); + }); + + it('does not retry terminal server failures', () => { + expect( + getAttachmentDownloadRetryDecision({ + error: { code: 404 }, + previousAttempts: 0, + type: 'attachment', + }) + ).to.deep.equal({ + attempt: 1, + retryBackoff: Constants.DURATION.SECONDS * 30, + shouldRetry: false, + shouldMarkAttachmentFailed: false, + }); + }); + + it('stops retrying transient failures after the capped retry window', () => { + expect( + getAttachmentDownloadRetryDecision({ + error: new Error('network timeout'), + previousAttempts: 10, + type: 'attachment', + }) + ).to.deep.equal({ + attempt: 11, + retryBackoff: Constants.DURATION.HOURS * 6, + shouldRetry: false, + shouldMarkAttachmentFailed: false, + }); + }); + }); + + describe('hasAttachmentDownloadRetriesRemaining', () => { + it('keeps transient failures retryable through the capped retry window', () => { + expect(hasAttachmentDownloadRetriesRemaining(1)).to.equal(true); + expect(hasAttachmentDownloadRetriesRemaining(10)).to.equal(true); + }); + + it('stops retrying after the capped retry window', () => { + expect(hasAttachmentDownloadRetriesRemaining(11)).to.equal(false); + }); + + it('keeps preview retries at the original shorter retry window', () => { + expect(hasAttachmentDownloadRetriesRemaining(2, 'preview')).to.equal(true); + expect(hasAttachmentDownloadRetriesRemaining(3, 'preview')).to.equal(false); + }); + }); + + describe('buildAttachmentDownloadRetryJob', () => { + it('clears pending state and schedules the next retry without dropping attachment metadata', () => { + expect( + buildAttachmentDownloadRetryJob( + { + id: 'job-id', + messageId: 'message-id', + type: 'attachment', + index: 0, + pending: 1, + attempts: 2, + timestamp: 100, + attachment: { + url: 'https://file.getsession.org/file/123', + key: 'key', + digest: 'digest', + }, + }, + 3, + Constants.DURATION.HOURS * 6, + 1000 + ) + ).to.deep.equal({ + id: 'job-id', + messageId: 'message-id', + type: 'attachment', + index: 0, + pending: 0, + attempts: 3, + timestamp: 1000 + Constants.DURATION.HOURS * 6, + attachment: { + url: 'https://file.getsession.org/file/123', + key: 'key', + digest: 'digest', + }, + }); + }); + }); + + describe('markAttachmentDownloadFailed', () => { + it('preserves retry metadata while clearing pending runtime state', () => { + const failed = markAttachmentDownloadFailed({ + url: 'https://file.getsession.org/file/123', + key: 'key', + digest: 'digest', + id: 'attachment-id', + size: 123, + contentType: 'image/png', + pending: true, + downloadJobId: 'job-id', + }); + + expect(failed).to.deep.equal({ + url: 'https://file.getsession.org/file/123', + key: 'key', + digest: 'digest', + id: 'attachment-id', + size: 123, + contentType: 'image/png', + pending: false, + downloadJobId: undefined, + error: true, + }); + }); + }); +}); diff --git a/ts/test/session/unit/utils/AttachmentsDownload_test.ts b/ts/test/session/unit/utils/AttachmentsDownload_test.ts new file mode 100644 index 0000000000..095a8db1ce --- /dev/null +++ b/ts/test/session/unit/utils/AttachmentsDownload_test.ts @@ -0,0 +1,97 @@ +import { expect } from 'chai'; +import Sinon from 'sinon'; + +import { Data } from '../../../../data/data'; +import { MessageModel } from '../../../../models/message'; +import * as ReceiverAttachments from '../../../../receiver/attachments'; +import * as Constants from '../../../../session/constants'; +import * as AttachmentsDownload from '../../../../session/utils/AttachmentsDownload'; +import { waitUntil } from '../../../../session/utils/Promise'; +import { TestUtils } from '../../../test-utils'; + +describe('AttachmentsDownload', () => { + afterEach(() => { + AttachmentsDownload.stop(); + Sinon.restore(); + }); + + describe('start', () => { + it('marks later transient attachment failures as visible failed while keeping the job queued', async () => { + TestUtils.stubWindowLog(); + + const now = 1000; + const attachment = { + url: 'https://file.getsession.org/file/123', + key: 'key', + digest: 'digest', + id: 'attachment-id', + size: 123, + contentType: 'image/png', + pending: true, + downloadJobId: 'job-id', + }; + const job = { + id: 'job-id', + messageId: 'message-id', + type: 'attachment', + index: 0, + isOpenGroupV2: false, + openGroupV2Details: undefined, + pending: 0, + attempts: 2, + timestamp: 0, + attachment, + }; + const message = new MessageModel({ + id: 'message-id', + conversationId: 'conversation-id', + source: 'source-id', + type: 'incoming', + attachments: [attachment], + }); + const logger = { + debug: Sinon.stub(), + error: Sinon.stub(), + info: Sinon.stub(), + warn: Sinon.stub(), + }; + + Sinon.stub(Date, 'now').returns(now); + Sinon.stub(message, 'isTrustedForAttachmentDownload').returns(true); + Sinon.stub(message, 'getConversation').returns({ + idForLogging: () => 'conversation-id', + } as any); + const commitStub = Sinon.stub(message, 'commit').resolves('message-id'); + Sinon.stub(ReceiverAttachments, 'downloadAttachmentFs').rejects(new Error('network timeout')); + Sinon.stub(Data, 'resetAttachmentDownloadPending').resolves(); + Sinon.stub(Data, 'getMessageById').resolves(message); + const setPendingStub = Sinon.stub(Data, 'setAttachmentDownloadJobPending').resolves(); + const saveJobStub = Sinon.stub(Data, 'saveAttachmentDownloadJob').resolves(); + const removeJobStub = Sinon.stub(Data, 'removeAttachmentDownloadJob').resolves(); + const getNextJobsStub = Sinon.stub(Data, 'getNextAttachmentDownloadJobs'); + getNextJobsStub.onFirstCall().resolves([job]); + getNextJobsStub.resolves([]); + + await AttachmentsDownload.start({ logger }); + await waitUntil(() => saveJobStub.calledOnce, 1000); + + expect(setPendingStub.calledOnceWithExactly('job-id', true)).to.equal(true); + expect(removeJobStub.called).to.equal(false); + expect(commitStub.calledOnce).to.equal(true); + expect(saveJobStub.firstCall.args[0]).to.deep.equal({ + ...job, + pending: 0, + attempts: 3, + timestamp: now + Constants.DURATION.HOURS * 6, + }); + expect(message.get('attachments')).to.deep.equal([ + { + ...attachment, + error: true, + pending: false, + downloadJobId: undefined, + }, + ]); + }); + }); +});