From ae90a74cef706c10fcc91ca3855629850b4e72b2 Mon Sep 17 00:00:00 2001 From: trevor-signal <131492920+trevor-signal@users.noreply.github.com> Date: Thu, 12 Feb 2026 12:36:53 -0500 Subject: [PATCH] Deduplicate incoming stickers from installed sticker packs --- app/attachments.node.ts | 3 +- ts/jobs/AttachmentDownloadManager.preload.ts | 123 ++---------------- .../AttachmentDownloadManager_test.preload.ts | 63 ++------- ts/test-mock/storage/sticker_test.node.ts | 58 ++++++++- ts/types/Stickers.preload.ts | 54 +++++--- .../deduplicateAttachment.preload.ts | 108 +++++++++++++++ ts/util/migrations.preload.ts | 6 - ts/util/queueAttachmentDownloads.preload.ts | 14 +- 8 files changed, 235 insertions(+), 194 deletions(-) create mode 100644 ts/util/attachments/deduplicateAttachment.preload.ts diff --git a/app/attachments.node.ts b/app/attachments.node.ts index 0eef95273d..04c76b5306 100644 --- a/app/attachments.node.ts +++ b/app/attachments.node.ts @@ -316,6 +316,7 @@ export const readAndDecryptDataFromDisk = async ({ return Buffer.concat(chunks); }; +export const CURRENT_ATTACHMENT_VERSION = 2; export const writeNewAttachmentData = async ({ data, getAbsoluteAttachmentPath, @@ -333,7 +334,7 @@ export const writeNewAttachmentData = async ({ }); return { - version: 2, + version: CURRENT_ATTACHMENT_VERSION, plaintextHash, size: data.byteLength, path, diff --git a/ts/jobs/AttachmentDownloadManager.preload.ts b/ts/jobs/AttachmentDownloadManager.preload.ts index c66e55d028..61d6c50338 100644 --- a/ts/jobs/AttachmentDownloadManager.preload.ts +++ b/ts/jobs/AttachmentDownloadManager.preload.ts @@ -21,7 +21,6 @@ import { import { maybeDeleteAttachmentFile, deleteDownloadFile, - doesAttachmentExist, processNewAttachment, } from '../util/migrations.preload.js'; import { DataReader, DataWriter } from '../sql/Client.preload.js'; @@ -62,11 +61,7 @@ import { type JobManagerJobResultType, type JobManagerJobType, } from './JobManager.std.js'; -import { - IMAGE_WEBP, - type MIMEType, - stringToMIMEType, -} from '../types/MIME.std.js'; +import { IMAGE_WEBP } from '../types/MIME.std.js'; import { AttachmentDownloadSource } from '../sql/Interface.std.js'; import { drop } from '../util/drop.std.js'; import { type ReencryptedAttachmentV2 } from '../AttachmentCrypto.node.js'; @@ -92,7 +87,7 @@ import { isAbortError } from '../util/isAbortError.std.js'; import { itemStorage } from '../textsecure/Storage.preload.js'; import { calculateExpirationTimestamp } from '../util/expirationTimer.std.js'; import { cleanupAttachmentFiles } from '../types/Message2.preload.js'; -import type { WithRequiredProperties } from '../types/Util.std.js'; +import { getExistingAttachmentDataForReuse } from '../util/attachments/deduplicateAttachment.preload.js'; const { noop, omit, throttle } = lodash; @@ -862,14 +857,19 @@ export async function runDownloadAttachmentJobInner({ }, }); + const existingAttachmentData = await getExistingAttachmentDataForReuse({ + plaintextHash: downloadedAttachment.plaintextHash, + contentType: attachment.contentType, + logId, + }); + + if (existingAttachmentData) { + await dependencies.maybeDeleteAttachmentFile(downloadedAttachment.path); + } + const attachmentDataToUse: Partial = { ...downloadedAttachment, - ...(await getExistingAttachmentDataForReuse({ - downloadedAttachment, - contentType: attachment.contentType, - logId, - dependencies, - })), + ...existingAttachmentData, }; const upgradedAttachment = await dependencies.processNewAttachment( @@ -1055,100 +1055,3 @@ function _markAttachmentAsTransientlyErrored( ): AttachmentType { return { ...attachment, pending: false, error: true }; } - -type AttachmentDataToBeReused = WithRequiredProperties< - Pick< - AttachmentType, - | 'path' - | 'localKey' - | 'version' - | 'thumbnail' - | 'screenshot' - | 'width' - | 'height' - >, - 'path' | 'localKey' | 'version' ->; -async function getExistingAttachmentDataForReuse({ - downloadedAttachment, - contentType, - logId, - dependencies, -}: { - downloadedAttachment: ReencryptedAttachmentV2; - contentType: MIMEType; - logId: string; - dependencies: Pick; -}): Promise { - const existingAttachmentData = - await DataWriter.getAndProtectExistingAttachmentPath({ - plaintextHash: downloadedAttachment.plaintextHash, - version: downloadedAttachment.version, - contentType, - }); - - if (!existingAttachmentData) { - return null; - } - - strictAssert(existingAttachmentData.path, 'path must exist for reuse'); - strictAssert( - existingAttachmentData.version === downloadedAttachment.version, - 'version mismatch' - ); - strictAssert(existingAttachmentData.localKey, 'localKey must exist'); - - if (!(await doesAttachmentExist(existingAttachmentData.path))) { - log.warn( - `${logId}: Existing attachment no longer exists, using newly downloaded one` - ); - return null; - } - - log.info(`${logId}: Reusing existing attachment`); - - await dependencies.maybeDeleteAttachmentFile(downloadedAttachment.path); - - const dataToReuse: AttachmentDataToBeReused = { - path: existingAttachmentData.path, - localKey: existingAttachmentData.localKey, - version: existingAttachmentData.version, - width: existingAttachmentData.width ?? undefined, - height: existingAttachmentData.height ?? undefined, - }; - const { thumbnailPath, thumbnailSize, thumbnailContentType } = - existingAttachmentData; - - if ( - thumbnailPath && - thumbnailSize && - thumbnailContentType && - (await doesAttachmentExist(thumbnailPath)) - ) { - dataToReuse.thumbnail = { - path: thumbnailPath, - localKey: existingAttachmentData.thumbnailLocalKey ?? undefined, - version: existingAttachmentData.thumbnailVersion ?? undefined, - size: thumbnailSize, - contentType: stringToMIMEType(thumbnailContentType), - }; - } - - const { screenshotPath, screenshotSize, screenshotContentType } = - existingAttachmentData; - if ( - screenshotPath && - screenshotSize && - screenshotContentType && - (await doesAttachmentExist(screenshotPath)) - ) { - dataToReuse.screenshot = { - path: screenshotPath, - localKey: existingAttachmentData.screenshotLocalKey ?? undefined, - version: existingAttachmentData.screenshotVersion ?? undefined, - size: screenshotSize, - contentType: stringToMIMEType(screenshotContentType), - }; - } - return dataToReuse; -} diff --git a/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts b/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts index 5c5fca2313..f1f5b569ec 100644 --- a/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts +++ b/ts/test-electron/services/AttachmentDownloadManager_test.preload.ts @@ -1432,12 +1432,24 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { await writeAttachmentFile('existingThumbnailPath'); await writeAttachmentFile('existingScreenshotPath'); - // @ts-expect-error new attachment version + await DataWriter.saveMessages( + [ + { + ...existingMessageWithDownloadedAttachment, + attachments: [{ ...existingAttachment, version: 1 }], + }, + ], + { + ourAci: generateAci(), + postSaveUpdates: () => Promise.resolve(), + } + ); + downloadAttachment.callsFake(async ({ attachment }) => { return { path: 'newlyDownloadedPath', plaintextHash: existingAttachment.plaintextHash, - version: existingAttachment.version + 1, + version: 2, localKey: testAttachmentLocalKey(), size: existingAttachment.size, digest: attachment.digest, @@ -1562,52 +1574,5 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => { assert.strictEqual(attachment?.path, 'existingPath'); assert.strictEqual(attachment?.thumbnail?.path, 'newThumbnailPath'); }); - - it('does not reuse attachment if version is not the same as requested version', async () => { - await writeAttachmentFile('existingPath'); - await writeAttachmentFile('existingThumbnailPath'); - await writeAttachmentFile('existingScreenshotPath'); - - // @ts-expect-error version is wrong - downloadAttachment.callsFake(async ({ attachment }) => { - return { - path: 'newlyDownloadedPath', - plaintextHash: existingAttachment.plaintextHash, - version: existingAttachment.version + 1, // Newer version! - localKey: testAttachmentLocalKey(), - size: existingAttachment.size, - digest: attachment.digest, - }; - }); - const job = composeJob({ - messageId: newMessage.id, - receivedAt: newMessage.received_at, - attachmentOverrides: undownloadedAttachment, - }); - - await runDownloadAttachmentJobInner({ - job, - isForCurrentlyVisibleMessage: false, - hasMediaBackups: false, - abortSignal: abortController.signal, - maxAttachmentSizeInKib: 100 * MEBIBYTE, - maxTextAttachmentSizeInKib: 2 * MEBIBYTE, - messageExpiresAt: null, - dependencies: { - cleanupAttachmentFiles, - maybeDeleteAttachmentFile, - deleteDownloadFile, - downloadAttachment, - processNewAttachment, - }, - }); - - // Cleans up newly downloaded path - assert.equal(maybeDeleteAttachmentFile.callCount, 0); - - const updatedMessage = window.MessageCache.getById(newMessage.id); - const attachment = updatedMessage?.attributes.attachments?.[0]; - assert.strictEqual(attachment?.path, 'newlyDownloadedPath'); - }); }); }); diff --git a/ts/test-mock/storage/sticker_test.node.ts b/ts/test-mock/storage/sticker_test.node.ts index e301eef8f9..210736a0a1 100644 --- a/ts/test-mock/storage/sticker_test.node.ts +++ b/ts/test-mock/storage/sticker_test.node.ts @@ -14,10 +14,15 @@ import { getStickerPackRecordPredicate, getStickerPackLink, } from './fixtures.node.js'; +import { + getMessageInTimelineByTimestamp, + sendTextMessage, +} from '../helpers.node.js'; +import { strictAssert } from '../../util/assert.std.js'; const { StickerPackOperation } = Proto.SyncMessage; -describe('storage service', function (this: Mocha.Suite) { +describe('stickers', function (this: Mocha.Suite) { this.timeout(durations.MINUTE); let bootstrap: Bootstrap; @@ -251,5 +256,56 @@ describe('storage service', function (this: Mocha.Suite) { const finalState = await phone.expectStorageState('consistency check'); assert.strictEqual(finalState.version, 5); + + debug( + 'verifying that stickers from packs can be received and paths are deduplicated' + ); + const firstTimestamp = bootstrap.getTimestamp(); + const secondTimestamp = bootstrap.getTimestamp(); + await sendTextMessage({ + from: firstContact, + to: desktop, + desktop, + text: undefined, + sticker: { + packId: STICKER_PACKS[0].id, + packKey: STICKER_PACKS[0].key, + stickerId: 0, + }, + timestamp: firstTimestamp, + }); + + await sendTextMessage({ + from: firstContact, + to: desktop, + desktop, + text: undefined, + sticker: { + packId: STICKER_PACKS[0].id, + packKey: STICKER_PACKS[0].key, + stickerId: 0, + }, + timestamp: secondTimestamp, + }); + + await window.getByRole('button', { name: 'Go back' }).click(); + await getMessageInTimelineByTimestamp(window, firstTimestamp) + .locator('.module-image--loaded') + .waitFor(); + + await getMessageInTimelineByTimestamp(window, secondTimestamp) + .locator('.module-image--loaded') + .waitFor(); + + const firstStickerData = (await app.getMessagesBySentAt(firstTimestamp))[0] + .sticker?.data; + const secondStickerData = ( + await app.getMessagesBySentAt(secondTimestamp) + )[0].sticker?.data; + + strictAssert(firstStickerData?.path, 'path exists'); + strictAssert(firstStickerData?.localKey, 'localKey exists'); + assert.strictEqual(firstStickerData.path, secondStickerData?.path); + assert.strictEqual(firstStickerData.localKey, secondStickerData?.localKey); }); }); diff --git a/ts/types/Stickers.preload.ts b/ts/types/Stickers.preload.ts index 98f8afcadd..1aab5724d7 100644 --- a/ts/types/Stickers.preload.ts +++ b/ts/types/Stickers.preload.ts @@ -13,7 +13,7 @@ import { getMessagesById } from '../messages/getMessagesById.preload.js'; import * as Bytes from '../Bytes.std.js'; import * as Errors from './errors.std.js'; import { deriveStickerPackKey, decryptAttachmentV1 } from '../Crypto.node.js'; -import { IMAGE_WEBP } from './MIME.std.js'; +import { IMAGE_WEBP, type MIMEType } from './MIME.std.js'; import { sniffImageMimeType } from '../util/sniffImageMimeType.std.js'; import type { AttachmentType, @@ -34,12 +34,10 @@ import { processNewEphemeralSticker, processNewSticker, deleteTempFile, - getAbsoluteStickerPath, - copyStickerIntoAttachmentsDirectory, - readAttachmentData, deleteSticker, readStickerData, writeNewStickerData, + writeNewAttachmentData, } from '../util/migrations.preload.js'; import { drop } from '../util/drop.std.js'; import { isNotNil } from '../util/isNotNil.std.js'; @@ -52,6 +50,7 @@ import { getSticker as doGetSticker, getStickerPackManifest, } from '../textsecure/WebAPI.preload.js'; +import { getExistingAttachmentDataForReuse } from '../util/attachments/deduplicateAttachment.preload.js'; const { isNumber, reject, groupBy, values, chunk } = lodash; @@ -1106,33 +1105,46 @@ export async function copyStickerToAttachments( ); } - const { path: stickerPath } = sticker; - const absolutePath = getAbsoluteStickerPath(stickerPath); - const { path, size } = - await copyStickerIntoAttachmentsDirectory(absolutePath); - - const newSticker: AttachmentType = { - ...sticker, - path, - size, - - // Fall-back - contentType: IMAGE_WEBP, - }; - const data = await readAttachmentData(newSticker); + const data = await readStickerData(sticker); + const size = data.byteLength; + const plaintextHash = getPlaintextHashForInMemoryAttachment(data); + let contentType: MIMEType; const sniffedMimeType = sniffImageMimeType(data); if (sniffedMimeType) { - newSticker.contentType = sniffedMimeType; + contentType = sniffedMimeType; } else { log.warn( 'copyStickerToAttachments: Unable to sniff sticker MIME type; falling back to WebP' ); + contentType = IMAGE_WEBP; } - newSticker.plaintextHash = getPlaintextHashForInMemoryAttachment(data); + const newSticker: AttachmentType = { + width: sticker.width, + height: sticker.height, + version: sticker.version, + size, + contentType, + plaintextHash, + }; - return newSticker; + const existingAttachmentData = await getExistingAttachmentDataForReuse({ + plaintextHash, + contentType, + logId: 'copyStickerToAttachments', + }); + + if (existingAttachmentData) { + return { + ...newSticker, + ...existingAttachmentData, + }; + } + + const newAttachmentData = await writeNewAttachmentData(data); + + return { ...newSticker, ...newAttachmentData }; } // In the case where a sticker pack is uninstalled, we want to delete it if there are no diff --git a/ts/util/attachments/deduplicateAttachment.preload.ts b/ts/util/attachments/deduplicateAttachment.preload.ts new file mode 100644 index 0000000000..e4759f392f --- /dev/null +++ b/ts/util/attachments/deduplicateAttachment.preload.ts @@ -0,0 +1,108 @@ +// Copyright 2026 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { createLogger } from '../../logging/log.std.js'; +import { DataWriter } from '../../sql/Client.preload.js'; +import { doesAttachmentExist } from '../migrations.preload.js'; + +import { type MIMEType, stringToMIMEType } from '../../types/MIME.std.js'; +import { strictAssert } from '../assert.std.js'; +import { type WithRequiredProperties } from '../../types/Util.std.js'; +import { type AttachmentType } from '../../types/Attachment.std.js'; +import { CURRENT_ATTACHMENT_VERSION } from '../../../app/attachments.node.js'; + +const log = createLogger('deduplicateAttachment'); + +type AttachmentDataToBeReused = WithRequiredProperties< + Pick< + AttachmentType, + | 'path' + | 'localKey' + | 'version' + | 'thumbnail' + | 'screenshot' + | 'width' + | 'height' + >, + 'path' | 'localKey' | 'version' +>; + +export async function getExistingAttachmentDataForReuse({ + plaintextHash, + contentType, + logId, +}: { + plaintextHash: string; + contentType: MIMEType; + logId?: string; +}): Promise { + const existingAttachmentData = + await DataWriter.getAndProtectExistingAttachmentPath({ + plaintextHash, + version: CURRENT_ATTACHMENT_VERSION, + contentType, + }); + + if (!existingAttachmentData) { + return null; + } + + strictAssert(existingAttachmentData.path, 'path must exist for reuse'); + strictAssert( + existingAttachmentData.version === CURRENT_ATTACHMENT_VERSION, + 'version mismatch' + ); + strictAssert(existingAttachmentData.localKey, 'localKey must exist'); + + if (!(await doesAttachmentExist(existingAttachmentData.path))) { + log.warn( + `${logId}: Existing attachment no longer exists, using newly downloaded one` + ); + return null; + } + + log.info(`${logId}: Reusing existing attachment`); + + const dataToReuse: AttachmentDataToBeReused = { + path: existingAttachmentData.path, + localKey: existingAttachmentData.localKey, + version: existingAttachmentData.version, + width: existingAttachmentData.width ?? undefined, + height: existingAttachmentData.height ?? undefined, + }; + const { thumbnailPath, thumbnailSize, thumbnailContentType } = + existingAttachmentData; + + if ( + thumbnailPath && + thumbnailSize && + thumbnailContentType && + (await doesAttachmentExist(thumbnailPath)) + ) { + dataToReuse.thumbnail = { + path: thumbnailPath, + localKey: existingAttachmentData.thumbnailLocalKey ?? undefined, + version: existingAttachmentData.thumbnailVersion ?? undefined, + size: thumbnailSize, + contentType: stringToMIMEType(thumbnailContentType), + }; + } + + const { screenshotPath, screenshotSize, screenshotContentType } = + existingAttachmentData; + if ( + screenshotPath && + screenshotSize && + screenshotContentType && + (await doesAttachmentExist(screenshotPath)) + ) { + dataToReuse.screenshot = { + path: screenshotPath, + localKey: existingAttachmentData.screenshotLocalKey ?? undefined, + version: existingAttachmentData.screenshotVersion ?? undefined, + size: screenshotSize, + contentType: stringToMIMEType(screenshotContentType), + }; + } + return dataToReuse; +} diff --git a/ts/util/migrations.preload.ts b/ts/util/migrations.preload.ts index dd28e6479a..c7fbef84bd 100644 --- a/ts/util/migrations.preload.ts +++ b/ts/util/migrations.preload.ts @@ -137,12 +137,6 @@ export const getAbsoluteStickerPath = createAbsolutePathGetter(STICKERS_PATH); export const writeNewStickerData = createEncryptedWriterForNew(STICKERS_PATH); export const deleteSticker = createDeleter(STICKERS_PATH); export const readStickerData = createEncryptedReader(STICKERS_PATH); -export const copyStickerIntoAttachmentsDirectory = copyIntoAttachmentsDirectory( - { - sourceDir: STICKERS_PATH, - targetDir: ATTACHMENTS_PATH, - } -); export const getAbsoluteBadgeImageFilePath = createAbsolutePathGetter(BADGES_PATH); diff --git a/ts/util/queueAttachmentDownloads.preload.ts b/ts/util/queueAttachmentDownloads.preload.ts index 3c8e52e334..af0f9f7266 100644 --- a/ts/util/queueAttachmentDownloads.preload.ts +++ b/ts/util/queueAttachmentDownloads.preload.ts @@ -342,9 +342,8 @@ export async function queueAttachmentDownloads( try { log.info(`${logId}: Copying sticker from installed pack`); copiedSticker = true; - count += 1; - const data = await copyStickerToAttachments(packId, stickerId); + const data = await copyStickerToAttachments(packId, stickerId); // Refresh sticker attachment since we had to await above const freshSticker = message.get('sticker'); strictAssert(freshSticker != null, 'Sticker is gone while copying'); @@ -460,13 +459,16 @@ export async function queueAttachmentDownloads( message.set({ editHistory }); } - if (count <= 0) { - return false; + if (count > 0) { + log.info(`${logId}: Queued ${count} total attachment downloads`); + return true; } - log.info(`${logId}: Queued ${count} total attachment downloads`); + if (copiedSticker) { + return true; + } - return true; + return false; } export async function queueNormalAttachments({