Skip to content

fix: prevent crash during wallet backup #1092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jvsena42 wants to merge 3 commits into
base: release-2.4.0
Choose a base branch
from
+50 −12
Draft

fix: prevent crash during wallet backup #1092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed99c6d
test: regression test for simulate crash on paykit failure
jvsena42 Jul 17, 2026
41139bd
fix: handle paykit snapshot failure in wallet backup
jvsena42 Jul 17, 2026
2ba4d65
chore: add changelog fragment
jvsena42 Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions app/src/main/java/to/bitkit/repositories/BackupRepo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import to.bitkit.di.IoDispatcher
import to.bitkit.di.json
import to.bitkit.ext.formatPlural
import to.bitkit.ext.nowMillis
import to.bitkit.ext.runSuspendCatching
import to.bitkit.models.ActivityBackupV1
import to.bitkit.models.BackupCategory
import to.bitkit.models.BackupItemStatus
Expand Down Expand Up @@ -435,7 +436,11 @@ class BackupRepo @Inject constructor(
it.copy(running = true, required = backupRequired)
}

vssBackupClient.putObject(key = category.name, data = getBackupDataBytes(category))
val data = runSuspendCatching { getBackupDataBytes(category) }
.onFailure { markBackupFailed(category, backupRequired, it) }
.getOrNull() ?: return@withContext

@ben-kaufman ben-kaufman Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

triggerBackup now infers Unit because this path returns without a value, but it returned Result<VssItem> before this change. That makes a snapshot failure look like successful completion to callers and changes the method signature. Could we return the snapshot failure as a Result and declare the return type explicitly?


vssBackupClient.putObject(key = category.name, data = data)
.onSuccess {
runningBackups -= category
failedBackupRequired -= category
Expand All @@ -447,18 +452,20 @@ class BackupRepo @Inject constructor(
}
Logger.info("Backup succeeded for: '$category'", context = TAG)
}
.onFailure { e ->
runningBackups -= category
cacheStore.updateBackupStatus(category) {
if (it.required == backupRequired) {
failedBackupRequired[category] = backupRequired
} else {
failedBackupRequired -= category
}
it.copy(running = false)
}
Logger.error("Backup failed for: '$category'", e, context = TAG)
.onFailure { markBackupFailed(category, backupRequired, it) }
}

private suspend fun markBackupFailed(category: BackupCategory, backupRequired: Long, e: Throwable) {
runningBackups -= category
cacheStore.updateBackupStatus(category) {
if (it.required == backupRequired) {
failedBackupRequired[category] = backupRequired
} else {
failedBackupRequired -= category
}
it.copy(running = false)
}
Logger.error("Backup failed for: '$category'", e, context = TAG)
}

private suspend fun getBackupDataBytes(category: BackupCategory): ByteArray = when (category) {
Expand Down
30 changes: 30 additions & 0 deletions app/src/test/java/to/bitkit/repositories/BackupRepoTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import to.bitkit.services.PaykitSdkService
import to.bitkit.test.BaseUnitTest
import to.bitkit.utils.AppError
import javax.inject.Provider
import kotlin.test.assertFalse
import kotlin.test.assertTrue
import kotlin.time.Clock
import kotlin.time.ExperimentalTime
Expand Down Expand Up @@ -111,6 +112,35 @@ class BackupRepoTest : BaseUnitTest() {
verify(settingsStore, never()).update(any())
}

@Test
fun `automatic wallet backup marks failure when Paykit snapshot fails`() = test {
whenever { privatePaykitRepo.backupSnapshot() }
.thenReturn(Result.failure(BackupRepoTestError("paykit session missing capabilities")))
val backupStatuses = MutableStateFlow(
mapOf(
BackupCategory.WALLET to BackupItemStatus(
synced = 1_000,
required = 2_000,
),
)
)
val allowWalletClear = CompletableDeferred<Unit>().apply { complete(Unit) }
stubBackupStatuses(backupStatuses, allowWalletClear) {}
stubBackupObservers()

try {
sut.startObservingBackups()

@ben-kaufman ben-kaufman Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test still passes if startObservingBackups() is removed, so it can pass without calling backupSnapshot(). The test clock is also 1_000, which makes triggerBackup() set required equal to synced instead of leaving a failed backup pending. Could we set the clock after synced, verify backupSnapshot() is called once, assert the status is still required with running = false, and advance another debounce to confirm it is not retried?

runCurrent()
advanceTimeBy(5_000)
runCurrent()

verify(vssBackupClient, never()).putObject(eq(BackupCategory.WALLET.name), any())
assertFalse(backupStatuses.value.getValue(BackupCategory.WALLET).running)
} finally {
sut.stopObservingBackups()
}
}

@Test
fun `start observing backs up stale required status after clearing running flag`() = test {
val backupStatuses = MutableStateFlow(
Expand Down
1 change: 1 addition & 0 deletions changelog.d/next/1092.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed a crash during automatic wallet backup when Paykit state could not be read.
Loading