From c723b2c6bf85902177fea5a5bbd4575fb7b67f83 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 16 Jan 2026 11:59:25 -0500 Subject: [PATCH] Fix issue where backfilled dupes may get different mediaIds. --- .../securesms/database/AttachmentTable.kt | 29 +++++++++++---- ...eKeyForAttachmentsThatNeedArchiveUpload.kt | 35 +++++++++++++++++++ 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.kt index 4e0a33fb1d..2b766c7ccd 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentTable.kt @@ -735,6 +735,12 @@ class AttachmentTable( .readToList { AttachmentId(it.requireLong(ID)) } } + private data class AttachmentKeyDupeData( + val id: AttachmentId, + val quote: Boolean, + val sticker: Boolean + ) + /** * At archive creation time, we need to ensure that all relevant attachments have populated [REMOTE_KEY]s. * This does that. @@ -744,7 +750,7 @@ class AttachmentTable( var notQuoteOrStickerDupeNotFoundCount = 0 var notQuoteOrStickerDupeFoundCount = 0 - val missingKeys = readableDatabase + val missingKeys: Map> = readableDatabase .select(ID, DATA_FILE, QUOTE, STICKER_ID) .from(TABLE_NAME) .where( @@ -756,10 +762,16 @@ class AttachmentTable( """ ) .run() - .readToList { Triple(AttachmentId(it.requireLong(ID)), it.requireBoolean(QUOTE), it.requireInt(STICKER_ID) >= 0) to it.requireNonNullString(DATA_FILE) } + .readToList { + AttachmentKeyDupeData( + id = AttachmentId(it.requireLong(ID)), + quote = it.requireBoolean(QUOTE), + sticker = it.requireInt(STICKER_ID) >= 0 + ) to it.requireNonNullString(DATA_FILE) + } .groupBy({ (_, dataFile) -> dataFile }, { (record, _) -> record }) - missingKeys.forEach { dataFile, ids -> + missingKeys.forEach { (dataFile, dupeData) -> val duplicateAttachmentWithRemoteData = readableDatabase .select() .from(TABLE_NAME) @@ -777,7 +789,7 @@ class AttachmentTable( if (duplicateAttachmentWithRemoteData != null) { val (duplicateAttachment, duplicateAttachmentDataInfo) = duplicateAttachmentWithRemoteData - ids.forEach { (attachmentId, isQuote, isSticker) -> + dupeData.forEach { (attachmentId, isQuote, isSticker) -> Log.w(TAG, "[createRemoteKeyForAttachmentsThatNeedArchiveUpload][$attachmentId] Missing key but found same data file with remote data. Updating. isQuote:$isQuote isSticker:$isSticker") writableDatabase @@ -805,10 +817,13 @@ class AttachmentTable( totalCount++ } } else { - ids.forEach { (attachmentId, isQuote, isSticker) -> - Log.w(TAG, "[createRemoteKeyForAttachmentsThatNeedArchiveUpload][$attachmentId] Missing key. Generating. isQuote:$isQuote isSticker:$isSticker") + val key = Util.getSecretBytes(64) + if (dupeData.size > 1) { + Log.w(TAG, "[createRemoteKeyForAttachmentsThatNeedArchiveUpload] Generated new key for: ${dupeData.joinToString { it.id.toString() }}") + } - val key = Util.getSecretBytes(64) + dupeData.forEach { (attachmentId, isQuote, isSticker) -> + Log.w(TAG, "[createRemoteKeyForAttachmentsThatNeedArchiveUpload][$attachmentId] Missing key. Assigning new one. isQuote:$isQuote isSticker:$isSticker") writableDatabase.update(TABLE_NAME) .values(REMOTE_KEY to Base64.encodeWithPadding(key)) diff --git a/app/src/test/java/org/thoughtcrime/securesms/database/AttachmentTableTest_createRemoteKeyForAttachmentsThatNeedArchiveUpload.kt b/app/src/test/java/org/thoughtcrime/securesms/database/AttachmentTableTest_createRemoteKeyForAttachmentsThatNeedArchiveUpload.kt index 6e36704996..8f37bead48 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/database/AttachmentTableTest_createRemoteKeyForAttachmentsThatNeedArchiveUpload.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/database/AttachmentTableTest_createRemoteKeyForAttachmentsThatNeedArchiveUpload.kt @@ -273,6 +273,41 @@ class AttachmentTableTest_createRemoteKeyForAttachmentsThatNeedArchiveUpload { assertThat(SignalDatabase.attachments.getAttachment(targetAttachmentId)?.remoteKey).isNotNull() } + @Test + fun whenTwoAttachmentsShareDataFileAndBothNeedNewKey_bothGetSameKey() { + val sharedDataFile = "/shared/path/attachment.jpg" + + // Create two attachments sharing the same data file, neither with a remote key + val attachmentId1 = insertAttachmentDirectly( + dataFile = sharedDataFile, + transferState = AttachmentTable.TRANSFER_PROGRESS_DONE, + archiveTransferState = AttachmentTable.ArchiveTransferState.NONE.value, + remoteKey = null + ) + + val attachmentId2 = insertAttachmentDirectly( + dataFile = sharedDataFile, + transferState = AttachmentTable.TRANSFER_PROGRESS_DONE, + archiveTransferState = AttachmentTable.ArchiveTransferState.NONE.value, + remoteKey = null + ) + + // Verify neither has a remote key initially + assertThat(SignalDatabase.attachments.getAttachment(attachmentId1)?.remoteKey).isNull() + assertThat(SignalDatabase.attachments.getAttachment(attachmentId2)?.remoteKey).isNull() + + val result = SignalDatabase.attachments.createRemoteKeyForAttachmentsThatNeedArchiveUpload() + assertThat(result.totalCount).isEqualTo(2) + + // Verify both attachments got the SAME remote key + val attachment1 = SignalDatabase.attachments.getAttachment(attachmentId1)!! + val attachment2 = SignalDatabase.attachments.getAttachment(attachmentId2)!! + + assertThat(attachment1.remoteKey).isNotNull() + assertThat(attachment2.remoteKey).isNotNull() + assertThat(attachment1.remoteKey).isEqualTo(attachment2.remoteKey) + } + /** * Creates an attachment that meets all criteria for archive upload: * - ARCHIVE_TRANSFER_STATE = NONE (0)