From 427f91f9032f0dd0fbbbf838c00604349a190edc Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Wed, 26 Mar 2025 11:48:28 -0700 Subject: [PATCH] Allow backfill for more undownloadable attachments --- ts/jobs/AttachmentDownloadManager.ts | 17 ++++-- ts/jobs/helpers/attachmentBackfill.ts | 52 ++++++++++++++----- .../util/downloadAttachment_test.ts | 10 ++-- ts/textsecure/MessageReceiver.ts | 20 +++---- ts/textsecure/downloadAttachment.ts | 11 ++-- ts/textsecure/messageReceiverEvents.ts | 23 ++++---- ts/types/Attachment.ts | 9 ++++ ts/util/downloadAttachment.ts | 5 +- 8 files changed, 98 insertions(+), 49 deletions(-) diff --git a/ts/jobs/AttachmentDownloadManager.ts b/ts/jobs/AttachmentDownloadManager.ts index 4f35912456..89875aaeec 100644 --- a/ts/jobs/AttachmentDownloadManager.ts +++ b/ts/jobs/AttachmentDownloadManager.ts @@ -12,10 +12,7 @@ import { AttachmentDownloadUrgency, coreAttachmentDownloadJobSchema, } from '../types/AttachmentDownload'; -import { - AttachmentPermanentlyUndownloadableError, - downloadAttachment as downloadAttachmentUtil, -} from '../util/downloadAttachment'; +import { downloadAttachment as downloadAttachmentUtil } from '../util/downloadAttachment'; import { DataWriter } from '../sql/Client'; import { getValue } from '../RemoteConfig'; @@ -24,6 +21,7 @@ import { AttachmentSizeError, type AttachmentType, AttachmentVariant, + AttachmentPermanentlyUndownloadableError, mightBeOnBackupTier, } from '../types/Attachment'; import { type ReadonlyMessageAttributesType } from '../model-types.d'; @@ -61,6 +59,7 @@ import { markAttachmentAsPermanentlyErrored } from '../util/attachments/markAtta import { AttachmentBackfill, isPermanentlyUndownloadable, + isPermanentlyUndownloadableWithoutBackfill, } from './helpers/attachmentBackfill'; export { isPermanentlyUndownloadable }; @@ -365,6 +364,16 @@ async function runDownloadAttachmentJob({ try { log.info(`${logId}: Starting job`); + if ( + job.source !== AttachmentDownloadSource.BACKFILL && + isPermanentlyUndownloadableWithoutBackfill(job.attachment) + ) { + // We should only get to here only if + throw new AttachmentPermanentlyUndownloadableError( + 'Not downloadable without backfill' + ); + } + const result = await runDownloadAttachmentJobInner({ job, abortSignal: options.abortSignal, diff --git a/ts/jobs/helpers/attachmentBackfill.ts b/ts/jobs/helpers/attachmentBackfill.ts index e8ead6f5db..1ac5b11705 100644 --- a/ts/jobs/helpers/attachmentBackfill.ts +++ b/ts/jobs/helpers/attachmentBackfill.ts @@ -113,17 +113,6 @@ export class AttachmentBackfill { AttachmentData: { Status }, } = Proto.SyncMessage.AttachmentBackfillResponse; - if ('error' in response) { - if (response.error === ErrorEnum.MESSAGE_NOT_FOUND) { - window.reduxActions.globalModals.showBackfillFailureModal({ - kind: BackfillFailureKind.NotFound, - }); - } else { - throw missingCaseError(response.error); - } - return; - } - const { targetMessage, targetConversation } = response; const convo = getConversationFromTarget(targetConversation); @@ -141,13 +130,29 @@ export class AttachmentBackfill { const message = window.MessageCache.register(new MessageModel(attributes)); - // IMPORTANT: no awaits until we finish modifying attachments - const timer = this.#pendingRequests.get(message.id); if (timer != null) { clearTimeout(timer); } + if ('error' in response) { + // Don't show error if we didn't request the data or already timed out + if (timer == null) { + return; + } + + if (response.error === ErrorEnum.MESSAGE_NOT_FOUND) { + window.reduxActions.globalModals.showBackfillFailureModal({ + kind: BackfillFailureKind.NotFound, + }); + } else { + throw missingCaseError(response.error); + } + return; + } + + // IMPORTANT: no awaits until we finish modifying attachments + // Since we are matching remote attachments with local attachments we need // to make sure things are normalized before starting. message.set( @@ -463,3 +468,24 @@ export function isPermanentlyUndownloadable( // at this time. return !AttachmentBackfill.isEnabledForJob(disposition, message); } + +export function isPermanentlyUndownloadableWithoutBackfill( + attachment: AttachmentType +): boolean { + // Attachment is downloadable or user have not failed to download it yet + if (isDownloadable(attachment) || !attachment.error) { + return false; + } + + // Too big attachments cannot be retried anymore + if (attachment.wasTooBig) { + return true; + } + + // Previous backfill failed + if (attachment.backfillError) { + return true; + } + + return true; +} diff --git a/ts/test-electron/util/downloadAttachment_test.ts b/ts/test-electron/util/downloadAttachment_test.ts index 3cd89f1f44..0eca5e9379 100644 --- a/ts/test-electron/util/downloadAttachment_test.ts +++ b/ts/test-electron/util/downloadAttachment_test.ts @@ -7,16 +7,16 @@ import { noop } from 'lodash'; import { DataWriter } from '../../sql/Client'; import { IMAGE_PNG } from '../../types/MIME'; -import { - AttachmentPermanentlyUndownloadableError, - downloadAttachment, -} from '../../util/downloadAttachment'; +import { downloadAttachment } from '../../util/downloadAttachment'; import { MediaTier } from '../../types/AttachmentDownload'; import { HTTPError } from '../../textsecure/Errors'; import { getCdnNumberForBackupTier } from '../../textsecure/downloadAttachment'; import { MASTER_KEY, MEDIA_ROOT_KEY } from '../backup/helpers'; import { getMediaIdFromMediaName } from '../../services/backups/util/mediaId'; -import { AttachmentVariant } from '../../types/Attachment'; +import { + AttachmentVariant, + AttachmentPermanentlyUndownloadableError, +} from '../../types/Attachment'; describe('utils/downloadAttachment', () => { const baseAttachment = { diff --git a/ts/textsecure/MessageReceiver.ts b/ts/textsecure/MessageReceiver.ts index 512223672c..0eff06cdf8 100644 --- a/ts/textsecure/MessageReceiver.ts +++ b/ts/textsecure/MessageReceiver.ts @@ -3825,6 +3825,16 @@ export default class MessageReceiver logId ); + strictAssert( + targetMessage != null, + 'MessageReceiver.handleAttachmentBackfillResponse: invalid target message' + ); + strictAssert( + targetConversation != null, + 'MessageReceiver.handleAttachmentBackfillResponse: ' + + 'invalid target conversation' + ); + let eventData: AttachmentBackfillResponseSyncEventData; if (response.error != null) { eventData = { @@ -3833,16 +3843,6 @@ export default class MessageReceiver targetConversation, }; } else { - strictAssert( - targetMessage != null, - 'MessageReceiver.handleAttachmentBackfillResponse: invalid target message' - ); - strictAssert( - targetConversation != null, - 'MessageReceiver.handleAttachmentBackfillResponse: ' + - 'invalid target conversation' - ); - const { attachments } = response; strictAssert( attachments != null, diff --git a/ts/textsecure/downloadAttachment.ts b/ts/textsecure/downloadAttachment.ts index 7c9f1fc3a9..5a39fca99b 100644 --- a/ts/textsecure/downloadAttachment.ts +++ b/ts/textsecure/downloadAttachment.ts @@ -17,6 +17,7 @@ import { mightBeOnBackupTier, type AttachmentType, AttachmentVariant, + AttachmentPermanentlyUndownloadableError, } from '../types/Attachment'; import * as Bytes from '../Bytes'; import { @@ -115,9 +116,13 @@ export async function downloadAttachment( const { digest, incrementalMac, chunkSize, key, size } = attachment; - strictAssert(digest, `${logId}: missing digest`); - strictAssert(key, `${logId}: missing key`); - strictAssert(isNumber(size), `${logId}: missing size`); + try { + strictAssert(digest, `${logId}: missing digest`); + strictAssert(key, `${logId}: missing key`); + strictAssert(isNumber(size), `${logId}: missing size`); + } catch (error) { + throw new AttachmentPermanentlyUndownloadableError(error.message); + } const mediaTier = options?.mediaTier ?? diff --git a/ts/textsecure/messageReceiverEvents.ts b/ts/textsecure/messageReceiverEvents.ts index c2a603f185..b29103ec03 100644 --- a/ts/textsecure/messageReceiverEvents.ts +++ b/ts/textsecure/messageReceiverEvents.ts @@ -595,17 +595,18 @@ export type AttachmentBackfillAttachmentType = Readonly< >; export type AttachmentBackfillResponseSyncEventData = Readonly< - | { - error: Proto.SyncMessage.AttachmentBackfillResponse.Error; - targetMessage?: AddressableMessage; - targetConversation?: ConversationIdentifier; - } - | { - attachments: ReadonlyArray; - longText: AttachmentBackfillAttachmentType | undefined; - targetMessage: AddressableMessage; - targetConversation: ConversationIdentifier; - } + { + targetMessage: AddressableMessage; + targetConversation: ConversationIdentifier; + } & ( + | { + error: Proto.SyncMessage.AttachmentBackfillResponse.Error; + } + | { + attachments: ReadonlyArray; + longText: AttachmentBackfillAttachmentType | undefined; + } + ) >; export class AttachmentBackfillResponseSyncEvent extends ConfirmableEvent { diff --git a/ts/types/Attachment.ts b/ts/types/Attachment.ts index 31645d4035..e9efecb134 100644 --- a/ts/types/Attachment.ts +++ b/ts/types/Attachment.ts @@ -1,5 +1,6 @@ // Copyright 2018 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only +/* eslint-disable max-classes-per-file */ import moment from 'moment'; import { @@ -46,6 +47,14 @@ const MIN_HEIGHT = 50; export class AttachmentSizeError extends Error {} +// Used for downlaods + +export class AttachmentPermanentlyUndownloadableError extends Error { + constructor(message: string) { + super(`AttachmentPermanentlyUndownloadableError: ${message}`); + } +} + type ScreenshotType = Omit & { height: number; width: number; diff --git a/ts/util/downloadAttachment.ts b/ts/util/downloadAttachment.ts index 16e3a5d7fd..0cfe4dd0da 100644 --- a/ts/util/downloadAttachment.ts +++ b/ts/util/downloadAttachment.ts @@ -5,6 +5,7 @@ import { type AttachmentType, mightBeOnBackupTier, AttachmentVariant, + AttachmentPermanentlyUndownloadableError, getAttachmentIdForLogging, } from '../types/Attachment'; import { downloadAttachment as doDownloadAttachment } from '../textsecure/downloadAttachment'; @@ -14,8 +15,6 @@ import { HTTPError } from '../textsecure/Errors'; import { toLogFormat } from '../types/errors'; import type { ReencryptedAttachmentV2 } from '../AttachmentCrypto'; -export class AttachmentPermanentlyUndownloadableError extends Error {} - export async function downloadAttachment({ attachment, options: { variant = AttachmentVariant.Default, onSizeUpdate, abortSignal }, @@ -105,7 +104,7 @@ export async function downloadAttachment({ error instanceof HTTPError && (error.code === 404 || error.code === 403) ) { - throw new AttachmentPermanentlyUndownloadableError(); + throw new AttachmentPermanentlyUndownloadableError(`HTTP ${error.code}`); } else { throw error; }