Don't trigger backup from reconciliation.

This commit is contained in:
Cody Henthorne
2026-08-17 14:36:14 -04:00
parent ec9e5e5dee
commit 8a4d7e19fa
7 changed files with 246 additions and 36 deletions
@@ -15,6 +15,7 @@ import assertk.assertions.isEqualTo
import assertk.assertions.isFalse
import assertk.assertions.isGreaterThan
import assertk.assertions.isNull
import assertk.assertions.isTrue
import io.mockk.Runs
import io.mockk.coEvery
import io.mockk.coVerify
@@ -45,6 +46,7 @@ import org.thoughtcrime.securesms.database.BackupMediaSnapshotTable.MediaEntry
import org.thoughtcrime.securesms.database.MessageType
import org.thoughtcrime.securesms.database.SignalDatabase
import org.thoughtcrime.securesms.dependencies.AppDependencies
import org.thoughtcrime.securesms.jobmanager.JobTracker
import org.thoughtcrime.securesms.keyvalue.SignalStore
import org.thoughtcrime.securesms.mms.IncomingMessage
import org.thoughtcrime.securesms.testing.SignalActivityRule
@@ -55,9 +57,11 @@ import java.io.ByteArrayInputStream
import java.io.IOException
import java.util.Optional
import java.util.UUID
import java.util.concurrent.CopyOnWriteArrayList
import kotlin.random.Random
import kotlin.time.Duration
import kotlin.time.Duration.Companion.days
import kotlin.time.Duration.Companion.seconds
@RunWith(AndroidJUnit4::class)
class ArchiveAttachmentReconciliationJobTest {
@@ -69,6 +73,10 @@ class ArchiveAttachmentReconciliationJobTest {
private val deletedFromCdn = slot<Set<ArchivedMediaObject>>()
private val watchedJobKeys = setOf(ArchiveAttachmentBackfillJob.KEY, ArchiveThumbnailBackfillJob.KEY, BackupMessagesJob.KEY)
private val enqueuedJobKeys: MutableList<String> = CopyOnWriteArrayList()
private val jobListener = JobTracker.JobListener { job, _ -> enqueuedJobKeys += job.factoryKey }
@Before
fun setUp() {
SignalStore.backup.backupTier = MessageBackupTier.PAID
@@ -77,12 +85,19 @@ class ArchiveAttachmentReconciliationJobTest {
SignalStore.backup.localRestoreReconcilePending = false
SignalStore.backup.lastUsedMessageCutoffTime = 0
AppDependencies.jobManager.addListener(JobTracker.JobFilter { it.factoryKey in watchedJobKeys }, jobListener)
mockkObject(BackupMessagesJob)
every { BackupMessagesJob.enqueue() } just Runs
mockkObject(ArchiveCommitAttachmentDeletesJob)
coEvery { ArchiveCommitAttachmentDeletesJob.deleteMediaObjectsFromCdn(any(), capture(deletedFromCdn), any(), any()) } returns null
}
@After
fun tearDown() {
AppDependencies.jobManager.removeListener(jobListener)
enqueuedJobKeys.clear()
unmockkAll()
}
@@ -137,10 +152,8 @@ class ArchiveAttachmentReconciliationJobTest {
* migration or the reconcile-first flow.
*/
@Test
fun givenFinishedMediaMissingFromCdn_whenAnOrdinaryPeriodicReconciliationRuns_thenItHealsToNoneAndReUploads() {
fun givenFinishedMediaMissingFromCdn_whenAnOrdinaryPeriodicReconciliationRuns_thenItHealsToNoneAndReUploadsWithoutANewBackup() {
SignalStore.backup.lastAttachmentReconciliationTime = System.currentTimeMillis() - 60.days.inWholeMilliseconds
mockkObject(BackupMessagesJob)
every { BackupMessagesJob.enqueue() } just Runs
val attachmentId = seedFinalizedAttachment("remote-key-periodic".toByteArray(), byteArrayOf(1, 2, 3, 4, 5))
SignalDatabase.attachments.setArchiveTransferState(attachmentId, AttachmentTable.ArchiveTransferState.FINISHED)
@@ -152,7 +165,10 @@ class ArchiveAttachmentReconciliationJobTest {
val healed = SignalDatabase.attachments.getAttachment(attachmentId)!!
assertThat(healed.archiveTransferState).isEqualTo(AttachmentTable.ArchiveTransferState.NONE)
assertThat(healed.archiveCdn).isNull()
verify(exactly = 1) { BackupMessagesJob.enqueue() }
assertThat(SignalDatabase.attachments.doAnyAttachmentsNeedArchiveUpload()).isTrue()
assertThat(awaitEnqueuedJob(ArchiveAttachmentBackfillJob.KEY)).isTrue()
assertThat(enqueuedJobKeys).doesNotContain(BackupMessagesJob.KEY)
verify(exactly = 0) { BackupMessagesJob.enqueue() }
}
/**
@@ -231,14 +247,19 @@ class ArchiveAttachmentReconciliationJobTest {
assertThat(SignalDatabase.attachments.getAttachment(attachmentId)!!.archiveTransferState).isEqualTo(AttachmentTable.ArchiveTransferState.NONE)
}
/**
* A local restore imports the CDN numbers its backup file claimed and optimistically marks them finished, so the export that ran ahead of this crawl published
* claims the crawl has since corrected. A full backup is what republishes the verified state, unlike the media-only repair the periodic path does.
*/
@Test
fun givenLocalRestoreReconcilePending_whenReconcileCompletes_thenIExpectFlagCleared() {
fun givenLocalRestoreReconcilePending_whenReconcileCompletes_thenIExpectFlagClearedAndABackup() {
SignalStore.backup.localRestoreReconcilePending = true
fakeCdnEmpty()
ArchiveAttachmentReconciliationJob(forced = true).run()
assertThat(SignalStore.backup.localRestoreReconcilePending).isFalse()
verify(exactly = 1) { BackupMessagesJob.enqueue() }
}
/**
@@ -486,6 +507,19 @@ class ArchiveAttachmentReconciliationJobTest {
return SignalDatabase.attachments.getAttachmentsForMessage(messageId).first().attachmentId
}
/**
* [JobTracker] dispatches to listeners on its own executor, so an enqueue that already happened may not have been reported yet.
*/
private fun awaitEnqueuedJob(factoryKey: String, timeout: Duration = 5.seconds): Boolean {
val deadline = System.currentTimeMillis() + timeout.inWholeMilliseconds
while (System.currentTimeMillis() < deadline && !enqueuedJobKeys.contains(factoryKey)) {
Thread.sleep(25)
}
return enqueuedJobKeys.contains(factoryKey)
}
private fun seedFinalizedAttachment(remoteKey: ByteArray, data: ByteArray, receivedAt: Duration = 0.days): AttachmentId {
val attachment = createAttachmentPointer(remoteKey, data.size)
val messageResult = SignalDatabase.messages.insertMessageInbox(createIncomingMessage(serverTime = receivedAt, attachment = attachment)).get()
@@ -60,6 +60,9 @@ object ArchiveUploadProgress {
private val attachmentProgress: MutableMap<AttachmentId, AttachmentProgressDetails> = ConcurrentHashMap()
/** Whether the media upload in flight began as part of a backup export. Not persisted, so a restart mid-upload defers CDN recording to the next backup. */
private var mediaUploadStartedByBackup: Boolean = false
private var debugAttachmentStartTime: Long = 0
private val debugTotalAttachments: AtomicInteger = AtomicInteger(0)
private val debugTotalBytes: AtomicLong = AtomicLong(0)
@@ -91,8 +94,10 @@ object ArchiveUploadProgress {
if (pendingMediaUploadBytes <= 0) {
Log.i(TAG, "No more pending bytes. Done!")
Log.d(TAG, "Upload finished! " + buildDebugStats(debugAttachmentStartTime, debugTotalAttachments.get(), debugTotalBytes.get()))
if (uploadProgress.mediaTotalBytes > 0) {
if (uploadProgress.mediaTotalBytes > 0 && mediaUploadStartedByBackup) {
Log.i(TAG, "We uploaded media as part of the backup. We should enqueue another backup now to ensure that CDN info is properly written.")
mediaUploadStartedByBackup = false
BackupMessagesJob.enqueue()
}
SignalStore.backup.finishedInitialBackup = true
@@ -201,6 +206,10 @@ object ArchiveUploadProgress {
fun onAttachmentSectionStarted(totalAttachmentBytes: Long) {
debugAttachmentStartTime = System.currentTimeMillis()
attachmentProgress.clear()
// Only a backup walks the export/upload states on its way here
mediaUploadStartedByBackup = uploadProgress.state == ArchiveUploadProgressState.State.Export || uploadProgress.state == ArchiveUploadProgressState.State.UploadBackupFile
updateState {
ArchiveUploadProgressState(
state = ArchiveUploadProgressState.State.UploadMedia,
@@ -99,9 +99,11 @@ fun DatabaseAttachment.hadIntegrityCheckPerformed(): Boolean {
/**
* Creates a [SignalServiceAttachmentPointer] for the archived attachment of the given [DatabaseAttachment].
*
* @param archiveCdnOverride The archive CDN to point at instead of the one we have stored, for retrying a download whose stored CDN looks wrong.
*/
@Throws(InvalidAttachmentException::class)
fun DatabaseAttachment.createArchiveAttachmentPointer(useArchiveCdn: Boolean): SignalServiceAttachmentPointer {
fun DatabaseAttachment.createArchiveAttachmentPointer(useArchiveCdn: Boolean, archiveCdnOverride: Int? = null): SignalServiceAttachmentPointer {
if (remoteKey.isNullOrBlank()) {
throw InvalidAttachmentException("empty encrypted key")
}
@@ -120,7 +122,7 @@ fun DatabaseAttachment.createArchiveAttachmentPointer(useArchiveCdn: Boolean): S
mediaId = this.requireMediaName().toMediaId(mediaRootBackupKey).encode()
)
id to (archiveCdn ?: RemoteConfig.backupFallbackArchiveCdn)
id to (archiveCdnOverride ?: archiveCdn ?: RemoteConfig.backupFallbackArchiveCdn)
} else {
if (remoteLocation.isNullOrEmpty()) {
throw InvalidAttachmentException("empty content id")
@@ -188,7 +188,7 @@ class ArchiveAttachmentReconciliationJob private constructor(
*/
private fun clearPendingLocalRestoreReconcile() {
if (SignalStore.backup.localRestoreReconcilePending) {
Log.i(TAG, "Local restore reconciliation complete. Clearing the pending flag and enqueueing a backup to upload any remaining media.", true)
Log.i(TAG, "Local restore reconciliation complete. Clearing the pending flag and enqueueing a backup to track and upload any remaining media.", true)
SignalStore.backup.localRestoreReconcilePending = false
BackupMessagesJob.enqueue()
}
@@ -261,12 +261,11 @@ class ArchiveAttachmentReconciliationJob private constructor(
if (mayNeedReUploadCount > 0) {
Log.w(TAG, "Found $mayNeedReUploadCount attachments that are present in the target snapshot, but could not be found on the CDN. This could be a bookkeeping error, or the upload may still be in progress. Checking.", true)
var newBackupJobRequired = false
var bookkeepingErrorCount = 0
var unrecoverableCount = 0
var fullSizeMismatchFound = false
var thumbnailMismatchFound = false
var fullSizeReUploadNeeded = false
var thumbnailReUploadNeeded = false
mediaObjectsThatMayNeedReUpload.forEach { mediaObjectCursor ->
val entry = BackupMediaSnapshotTable.MediaEntry.fromCursor(mediaObjectCursor)
@@ -287,8 +286,13 @@ class ArchiveAttachmentReconciliationJob private constructor(
when (resetResult) {
ArchiveTransferStateResetResult.RESET -> {
Log.w(TAG, "$logPrefix Reset transfer state by hash/key.", true)
newBackupJobRequired = true
bookkeepingErrorCount++
if (entry.isThumbnail) {
thumbnailReUploadNeeded = true
} else {
fullSizeReUploadNeeded = true
}
}
ArchiveTransferStateResetResult.SKIPPED_NO_LOCAL_DATA -> {
@@ -303,9 +307,9 @@ class ArchiveAttachmentReconciliationJob private constructor(
// Deliberately not set for SKIPPED_NO_LOCAL_DATA, since the precautionary backfills these drive could never upload media that has no local bytes.
if (entry.isThumbnail) {
thumbnailMismatchFound = true
thumbnailReUploadNeeded = true
} else {
fullSizeMismatchFound = true
fullSizeReUploadNeeded = true
}
}
}
@@ -314,6 +318,7 @@ class ArchiveAttachmentReconciliationJob private constructor(
if (bookkeepingErrorCount > 0) {
Log.w(TAG, "Found that $bookkeepingErrorCount/$mayNeedReUploadCount of the CDN mismatches were bookkeeping errors.", true)
maybePostReconciliationFailureNotification()
} else {
Log.i(TAG, "None of the $mayNeedReUploadCount CDN mismatches were bookkeeping errors.", true)
}
@@ -342,19 +347,15 @@ class ArchiveAttachmentReconciliationJob private constructor(
stopwatch.split("internal-lookup")
}
if (newBackupJobRequired) {
Log.w(TAG, "Some of the errors require re-uploading a new backup job to resolve.", true)
maybePostReconciliationFailureNotification()
BackupMessagesJob.enqueue()
} else {
if (fullSizeMismatchFound) {
Log.d(TAG, "Full size mismatch found. Enqueuing an attachment backfill job to be safe.", true)
AppDependencies.jobManager.add(ArchiveAttachmentBackfillJob())
}
if (thumbnailMismatchFound) {
Log.d(TAG, "Thumbnail mismatch found. Enqueuing a thumbnail backfill job to be safe.", true)
AppDependencies.jobManager.add(ArchiveThumbnailBackfillJob())
}
// No backup is started here on purpose. Re-uploading is the whole repair, and [ArchiveUploadProgress] is what decides whether the resulting CDN numbers
// warrant a fresh export once the backfill finishes uploading.
if (fullSizeReUploadNeeded) {
Log.d(TAG, "Full size mismatch found. Enqueuing an attachment backfill job.", true)
AppDependencies.jobManager.add(ArchiveAttachmentBackfillJob())
}
if (thumbnailReUploadNeeded) {
Log.d(TAG, "Thumbnail mismatch found. Enqueuing a thumbnail backfill job.", true)
AppDependencies.jobManager.add(ArchiveThumbnailBackfillJob())
}
} else {
Log.d(TAG, "No attachments need to be repaired.", true)
@@ -336,7 +336,8 @@ class RestoreAttachmentJob private constructor(
messageId: Long,
attachmentId: AttachmentId,
attachment: DatabaseAttachment,
forceTransitTier: Boolean = false
forceTransitTier: Boolean = false,
forceFallbackArchiveCdn: Boolean = false
) {
val maxReceiveSize: Long = RemoteConfig.maxAttachmentReceiveSizeBytes
val attachmentFile: File = ArchiveDatabaseExecutor.runBlocking {
@@ -366,7 +367,8 @@ class RestoreAttachmentJob private constructor(
}
val messageReceiver = AppDependencies.signalServiceMessageReceiver
val pointer = attachment.createArchiveAttachmentPointer(useArchiveCdn)
val archiveCdnOverride = RemoteConfig.backupFallbackArchiveCdn.takeIf { forceFallbackArchiveCdn }
val pointer = attachment.createArchiveAttachmentPointer(useArchiveCdn, archiveCdnOverride)
val progressListener = object : SignalServiceAttachment.ProgressListener {
override fun onAttachmentProgress(progress: AttachmentTransferProgress) {
@@ -380,7 +382,7 @@ class RestoreAttachmentJob private constructor(
ArchiveRestoreProgress.onDownloadStart(attachmentId)
val decryptingStream = if (useArchiveCdn) {
val cdnCredentials = runBlocking { AppDependencies.archiveService.getCdnReadCredentials(ArchiveService.CredentialType.MEDIA, attachment.archiveCdn ?: RemoteConfig.backupFallbackArchiveCdn) }.successOrThrow().headers
val cdnCredentials = runBlocking { AppDependencies.archiveService.getCdnReadCredentials(ArchiveService.CredentialType.MEDIA, pointer.cdnNumber) }.successOrThrow().headers
messageReceiver
.retrieveArchivedAttachment(
@@ -419,7 +421,8 @@ class RestoreAttachmentJob private constructor(
ArchiveDatabaseExecutor.throttledNotifyAttachmentAndChatListObservers()
}
if (useArchiveCdn && attachment.archiveCdn == null) {
if (useArchiveCdn && attachment.archiveCdn != pointer.cdnNumber) {
Log.i(TAG, "[$attachmentId] Recording the archive CDN we actually downloaded from. Was: ${attachment.archiveCdn}, now: ${pointer.cdnNumber}")
ArchiveDatabaseExecutor.runBlocking {
SignalDatabase.attachments.setArchiveCdn(attachmentId, pointer.cdnNumber)
}
@@ -447,6 +450,10 @@ class RestoreAttachmentJob private constructor(
}
markPermanentlyFailed(attachmentId)
return
} else if (useArchiveCdn && !forceFallbackArchiveCdn && attachment.archiveCdn != null && attachment.archiveCdn != RemoteConfig.backupFallbackArchiveCdn) {
// A stored CDN can be stale
Log.w(TAG, "[$attachmentId] Archive CDN ${attachment.archiveCdn} returned a 404. Retrying against the fallback CDN before falling back to transit.")
return retrieveAttachment(messageId, attachmentId, attachment, forceFallbackArchiveCdn = true)
} else if (SignalStore.backup.backsUpMedia && attachment.remoteLocation.isNotNullOrBlank()) {
Log.w(TAG, "[$attachmentId] Failed to download attachment from the archive CDN! Retrying download from transit CDN. hasPlaintextHash: ${attachment.dataHash != null}")
if (attachment.dataHash != null) {
@@ -365,18 +365,44 @@ class ArchiveUploadProgressTest {
assertThat(state.mediaUploadedBytes).isEqualTo(0L)
}
/**
* A backup exports before its media finishes uploading, so that file can carry empty CDNs. The follow-up export is what keeps it from staying that way, and
* reaching the media phase from [State.UploadBackupFile] is what identifies a backup as the cause.
*/
@Test
fun `progress flow - completes when no pending bytes remain`() {
setUploadProgress(ArchiveUploadProgressState(state = State.UploadMedia, mediaTotalBytes = 1000, mediaUploadedBytes = 0))
fun `progress flow - enqueues another backup when media upload followed a backup file upload`() {
setUploadProgress(ArchiveUploadProgressState(state = State.UploadBackupFile, backupFileTotalBytes = 500))
backsUpMedia = true
pendingArchiveUploadBytes = 0
pendingArchiveUploadBytes = 1000
ArchiveUploadProgress.onAttachmentSectionStarted(totalAttachmentBytes = 1000)
pendingArchiveUploadBytes = 0
ArchiveUploadProgress.triggerUpdate()
val state = awaitUploadProgress { it.state == State.None }
assertThat(state.mediaUploadedBytes).isEqualTo(1000L)
verify { backup.finishedInitialBackup = true }
verify { BackupMessagesJob.enqueue() }
verify(exactly = 1) { BackupMessagesJob.enqueue() }
}
/**
* A reconciliation repair enqueues the backfill directly, so the media phase begins from [State.None] with no backup in flight. There is no new backup file
* whose CDNs could be missing, so chaining an export would re-upload the whole database for nothing.
*/
@Test
fun `progress flow - completes without another backup when media upload began with no backup in flight`() {
setUploadProgress(ArchiveUploadProgressState(state = State.None))
backsUpMedia = true
pendingArchiveUploadBytes = 1000
ArchiveUploadProgress.onAttachmentSectionStarted(totalAttachmentBytes = 1000)
pendingArchiveUploadBytes = 0
ArchiveUploadProgress.triggerUpdate()
val state = awaitUploadProgress { it.state == State.None }
assertThat(state.mediaUploadedBytes).isEqualTo(1000L)
verify { backup.finishedInitialBackup = true }
verify(exactly = 0) { BackupMessagesJob.enqueue() }
}
@Test
@@ -0,0 +1,131 @@
/*
* Copyright 2026 Signal Messenger, LLC
* SPDX-License-Identifier: AGPL-3.0-only
*/
package org.thoughtcrime.securesms.backup.v2
import android.app.Application
import arrow.core.right
import assertk.assertThat
import assertk.assertions.isEqualTo
import io.mockk.coEvery
import io.mockk.every
import io.mockk.mockkObject
import io.mockk.unmockkObject
import org.junit.After
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import org.robolectric.annotation.Config
import org.signal.core.models.backup.MediaRootBackupKey
import org.signal.core.models.database.AttachmentId
import org.signal.core.util.Base64
import org.thoughtcrime.securesms.attachments.Cdn
import org.thoughtcrime.securesms.attachments.DatabaseAttachment
import org.thoughtcrime.securesms.database.AttachmentTable
import org.thoughtcrime.securesms.dependencies.AppDependencies
import org.thoughtcrime.securesms.keyvalue.SignalStore
import org.thoughtcrime.securesms.testutil.MockAppDependenciesRule
import org.thoughtcrime.securesms.testutil.MockSignalStoreRule
import org.thoughtcrime.securesms.util.RemoteConfig
/**
* Covers which archive CDN ends up in the pointer we download from. A restore that reaches for the wrong CDN gets a 404, and
* [org.thoughtcrime.securesms.jobs.RestoreAttachmentJob] treats a 404 as permanent, so the precedence here decides whether media survives a stale CDN number.
*/
@RunWith(RobolectricTestRunner::class)
@Config(manifest = Config.NONE, application = Application::class)
class DatabaseAttachmentArchiveUtilTest {
companion object {
private const val FALLBACK_CDN = 3
private const val STORED_CDN = 42
}
@get:Rule
val mockSignalStore = MockSignalStoreRule()
@get:Rule
val appDependencies = MockAppDependenciesRule()
@Before
fun setUp() {
every { SignalStore.backup.mediaRootBackupKey } returns MediaRootBackupKey(ByteArray(32))
coEvery { AppDependencies.archiveService.getArchivedMediaCdnPath() } returns "backups/media".right()
mockkObject(RemoteConfig)
every { RemoteConfig.backupFallbackArchiveCdn } returns FALLBACK_CDN
}
@After
fun tearDown() {
unmockkObject(RemoteConfig)
}
@Test
fun `uses the stored archive cdn when there is no override`() {
val pointer = archivedAttachment(archiveCdn = STORED_CDN).createArchiveAttachmentPointer(useArchiveCdn = true)
assertThat(pointer.cdnNumber).isEqualTo(STORED_CDN)
}
@Test
fun `falls back to the configured cdn when nothing is stored`() {
val pointer = archivedAttachment(archiveCdn = null).createArchiveAttachmentPointer(useArchiveCdn = true)
assertThat(pointer.cdnNumber).isEqualTo(FALLBACK_CDN)
}
/**
* The retry path: a stored CDN that 404s is worth re-attempting against the CDN a missing value would have resolved to, so the override has to win over it.
*/
@Test
fun `prefers the override over the stored archive cdn`() {
val pointer = archivedAttachment(archiveCdn = STORED_CDN).createArchiveAttachmentPointer(useArchiveCdn = true, archiveCdnOverride = FALLBACK_CDN)
assertThat(pointer.cdnNumber).isEqualTo(FALLBACK_CDN)
}
private fun archivedAttachment(archiveCdn: Int?): DatabaseAttachment {
return DatabaseAttachment(
attachmentId = AttachmentId(1L),
mmsId = 42L,
hasData = true,
hasThumbnail = false,
contentType = "image/jpeg",
transferProgress = AttachmentTable.TRANSFER_PROGRESS_DONE,
size = 1_000L,
fileName = "photo.jpg",
cdn = Cdn.CDN_3,
location = null,
key = Base64.encodeWithPadding(ByteArray(64) { 1 }),
digest = null,
incrementalDigest = null,
incrementalMacChunkSize = 0,
fastPreflightId = null,
voiceNote = false,
borderless = false,
videoGif = false,
width = 0,
height = 0,
quote = false,
caption = null,
stickerLocator = null,
blurHash = null,
audioHash = null,
transformProperties = null,
displayOrder = 0,
uploadTimestamp = 0,
dataHash = Base64.encodeWithPadding(ByteArray(32) { 2 }),
archiveCdn = archiveCdn,
thumbnailRestoreState = AttachmentTable.ThumbnailRestoreState.NONE,
archiveTransferState = AttachmentTable.ArchiveTransferState.FINISHED,
uuid = null,
quoteTargetContentType = null,
metadata = null
)
}
}