fix: isolate activity backup restore slices

[jvsena42]

Fixes #1167
Related to #1275

This PR applies the three Core-owned slices of an activity backup independently on restore, so one rejected record can no longer discard the others, and holds ordinary backup uploads for the whole restore so they cannot overwrite the envelope before it is read.

Follow-up to #1163, and the app half of synonymdev/bitkit-core#134.

Description

The activity restore upserted activities, tags and closed channels inside a single runCatching. Core fails a bulk write as a whole, so the first slice to throw aborted the calls after it. A single tag whose parent activity was missing therefore cost the activities and the closed channels too. This was reproduced on regtest while building #1163, where activity_tags went from one row to zero and closed channels were skipped entirely:

Failed to restore activity backup [errorDetails=Failed to insert tag: FOREIGN KEY constraint failed]

#1163 removed the hardware wallet trigger by carrying those tags as pre-activity metadata, so this is no longer easy to hit, but it can still happen. The backup envelope is assembled from three separate Core reads rather than one snapshot, so a tag written against a brand-new activity between the first and the third read lands in the envelope without its parent, in the default wallet scope, with no hardware wallet involved.

• Runs each restore slice in its own runSuspendCatching so a failing slice no longer skips the slices after it.
• Logs every slice failure once, with the slice name, in the activity repository.
• Keeps returning a failure when any slice failed, so the migration rewrite stays gated on a full restore and a partial restore never rewrites the backup on the spot.
• Notifies activity and tag observers when at least one slice was applied, so restored activities reach the UI without waiting for the next sync, and skips the notification when every slice failed.
• Raises the tag signal only when the tags slice itself was applied, so a rejected tags slice no longer marks the metadata backup as changed.
• Replaces the duplicate error log in the backup repository with a warning that the activity backup rewrite was skipped.
• Propagates cancellation of a restore instead of treating it as a failed slice.

Holding ordinary uploads for the whole restore

Isolating the slices is not enough on its own, as the device run on this PR showed: a second restore of the same wallet came back without the tag and without the closed channel, because three ACTIVITY uploads landed before the restore had read the backup.

_isRestoring is only raised inside performFullRestoreFromLatestBackup, which the restore flow reaches after getLatestBackupTime() has probed VSS (bounded by VSS_TIMESTAMP_TIMEOUT, 60s). The node start only waits TIMEOUT_RESTORE_WAIT (30s) for the restore, so the node starts, syncs, and its activity traffic bumps activitiesChanged while every ordinary upload path is still open — the data listeners, the status observers and the post-debounce re-check in scheduleBackup all consult shouldSkipBackup(), which was still false.

• Adds BackupRepo.setRestorePending() and folds it into shouldSkipBackup(), so the whole restore flow is covered rather than just the part after the envelope is fetched.
• WalletViewModel.restoreWallet raises the gate as the restore starts, and it is cleared when the restore returns, and when the wallet was never created so nothing reaches the restore at all.
• The gate cannot suppress uploads indefinitely: it expires after 10 minutes, it is held in memory only so a process death clears it, it is cleared on every terminal path of the restore flow, and it never blocks an explicit triggerBackup — the restore's own migration rewrite and the Data Backups retry button keep working.

bitkit-ios already gates the same way (AppScene.restoreFromMostRecentBackup sets BackupService.setRestoring(true) before the timestamp probe, with the same "triggerBackup deliberately not gated" note), so this brings Android in line, with an expiry iOS does not have.

Out of Scope

• bitkit-core: keeping the valid records within a single bulk write is tracked in fix: keep valid tags when a bulk upsert hits a bad record bitkit-core#134. The two fixes are complementary and independent: Core limits the loss to the bad record, this limits it to the affected slice.
• ActivityRepo.restoreFromBackup: returns the first slice failure rather than an aggregate error; the caller only needs success or failure.
• BackupRepo: the new "skipped rewrite" warning is logged on any restore failure, including when no migration made a rewrite due.
• Backup envelope creation: reading the three slices as one consistent snapshot, which would remove the orphaned-tag window at its source.
• A restore interrupted by a process death: the gate is in memory, so the next launch uploads whatever is local. Persisting it would risk suppressing uploads for good, and this is unchanged from master.
• iOS parity: bitkit-ios already carries the upload gate, but still applies the three activity slices in one block, so the slice isolation half should be ported in a separate PR.

Design

N/A — no UI changes.

Preview

N/A — no user-visible UI change; the fix only affects how restored data is written, so there is nothing new to screenshot.

QA Notes

Reaching the failing-slice path by hand needs a tag whose parent activity is missing from the envelope, which #1163 made hard to produce, so the isolation contract is covered by unit tests and manual QA is a restore regression pass.

Manual Tests

• 1a. regression: Wallet with tagged activity and a closed channel → wipe wallet → restore from seed → Home: activities and their tags are present.
  • 1b. regression: Settings → Lightning Connections → Closed: the closed channel is present.
• 2. regression: Restore the same wallet a second time → Settings → Data Backups: Transaction Log is not re-uploaded.

Journey

Test 1 is now a committed journey, as suggested in review. It walks the whole route — record the phrase, receive 500 000 regtest sats, tag the activity restoretag, buy a Blocktank channel, close it, wait for the Transaction Log, reset, restore — and ends by repeating the reset and restore once more, since the second restore is the one the ungated upload used to lose.

journeys/backup-restore/restore-keeps-tags-and-closed-channels.xml

<journey name="restore keeps activity tags and closed channels">
  <description>
    A wallet restored from its recovery phrase must come back with its activity tags and its closed
    Lightning connections, not only its activities. The backup envelope carries three Core-owned
    slices — activities, activity tags and closed channels — and the restore applies each of them
    independently, so one rejected record can no longer discard the rest.

    It also covers the upload gate the restore depends on: from the moment the restore flow starts,
    ordinary activity uploads are held, so the fresh wallet's own state cannot replace the stored
    envelope before the restore reads it. Running this twice on the same wallet is the sharper check,
    because the second restore is where the ungated upload used to win the race.

    Precondition: a dev (regtest) build, onboarded with a NEW wallet, and the staging LSP reachable
    through the `lsp` helper at the repo root for funding and mining (see ../README.md). The journey
    wipes the wallet half way through, so do not run it against a wallet whose funds matter.
  </description>
  <actions>
    <action>Launch the Bitkit app and go to the wallet home screen</action>
    <action>Open the menu, navigate to Settings, then the "Security" tab (testTag "Tab-security"), then tap "Back Up Your Money" (testTag "BackupWallet")</action>
    <action>Continue past the intro (testTag "BackupIntroViewContinue"), tap to reveal the phrase (testTag "TapToReveal") and record every word of the seed container (testTag "SeedContainer") — the rest of the journey depends on them</action>
    <action>Leave the backup flow and return to the wallet home screen</action>
    <action>Tap the Receive button (testTag "Receive"), tap "Show Details" (testTag "ShowDetails") and copy the on-chain savings address (testTag "ReceiveCopyQR")</action>
    <action>Run `./lsp POST /regtest/chain/deposit '{"address":"&lt;savings address&gt;","amountSat":500000}'` then `./lsp POST /regtest/chain/mine '{"count":3}'`, and wait for the 500 000 sat receive to appear on Home</action>
    <action>Tap the received activity row (testTag "ActivityShort-0") and verify its detail screen opens</action>
    <action>Tap "Tag" (testTag "ActivityTag"), enter the tag restoretag in the tag input (testTag "AddTagInput") and save it (testTag "AddTagSave")</action>
    <action>Verify the tag restoretag is listed on the activity detail screen (testTag "ActivityTags"), then go back to Home</action>
    <action>Tap the SAVINGS balance tile (testTag "ActivitySavings"), then "Transfer To Spending" (testTag "TransferToSpending")</action>
    <action>Pass any first-run intros on the way: transfer intro (testTag "TransferIntro-button"), the transfer funding option (testTag "FundTransfer") and the spending intro (testTag "SpendingIntro-button")</action>
    <action>On the spending amount screen (testTag "SpendingAmount") enter 100 000 on the number pad ("N1" then "N0" five times) and tap Continue (testTag "SpendingAmountContinue")</action>
    <action>Wait for the Blocktank order to be quoted, then confirm the transfer by swiping the confirm handle (testTag "GRAB")</action>
    <action>Wait for the channel to open and leave the flow through its final button (testTag "TransferSuccess-button")</action>
    <action>Open Settings, then the "Advanced" tab (testTag "Tab-advanced"), then "Lightning Connections" (testTag "Channels")</action>
    <action>Tap the open connection (testTag "Channel"), tap "Close Connection" (testTag "CloseConnection") and confirm (testTag "CloseConnectionButton")</action>
    <action>Verify the connection has left the open list, and that it is listed under "Show Closed &amp; Failed" (testTag "ChannelsClosed")</action>
    <action>Open Settings, then the "Security" tab, then "Data Backups" (testTag "BackupSettings"), and wait on the list (testTag "BackupScrollView") until the "Transaction Log" row reports a recent backup — that row carries no testTag, so read its text</action>
    <action>Open Settings, then the "Security" tab, then "Reset And Restore" (testTag "ResetAndRestore"), tap "Reset Wallet" (testTag "restore_reset_button") and confirm in the dialog (testTag "restore_reset_dialog")</action>
    <action>On the onboarding screen tap "Restore" (testTag "RestoreWallet"), enter the recorded words into the word inputs (testTag "Word-0" onwards) and tap Restore (testTag "RestoreButton")</action>
    <action>Wait for the restore to finish and tap "Get Started" (testTag "GetStartedButton")</action>
    <action>Verify Home lists the restored activities, including the 500 000 sat receive</action>
    <action>Tap the received activity row (testTag "ActivityShort-0") and verify it still shows the tag restoretag (testTag "ActivityTags")</action>
    <action>Open Settings, then the "Advanced" tab, then "Lightning Connections", tap "Show Closed &amp; Failed" (testTag "ChannelsClosed") and verify the closed connection is listed</action>
    <action>Read the app log and verify the restore applied all three slices, as `Restored 3 activities, 1 activity tags, 1 closed channels`, and that no `Backup succeeded for: 'ACTIVITY'` line sits between the reset and `Full restore starting`</action>
    <action>Repeat the reset and the restore once more from the same recovery phrase, and verify the tag and the closed connection are still there — the second restore is the one the ungated upload used to lose</action>
  </actions>
</journey>

Every identifier it names was checked against app/src/main on this branch. The suite has its own journeys/backup-restore/README.md, and journeys/README.md carries its suites row and a cross-platform row.

Automated Checks

• Unit tests added in ActivityRepoTest.kt: all slices succeed; tags, activities and closed channels slices each fail while the other two are still applied and the failure is returned; a failing tags slice raises the activity signal but not the tag signal; several slices fail and the first failure is returned; every slice fails and observers are not notified; cancellation is rethrown.
• Unit tests added in BackupRepoTest.kt for the upload gate: an ordinary activity upload is skipped while a restore is pending; it resumes once the restore is no longer pending; the gate stops holding uploads once it expires; the migration rewrite still uploads while a restore is pending.
• Unit tests added in WalletViewModelTest.kt: the restore holds ordinary backups from its start until it completes, and releases them when the wallet is never created.
• Negative control: dropping || isRestorePending() from shouldSkipBackup() fails the two gate tests and leaves the expiry and rewrite tests green; dropping the setRestorePending(true) call fails the view model ordering test. Both were restored and re-run green.
• Existing coverage in BackupRepoTest.kt stays green, including activity backup is not rewritten when core restore fails, which pins the VSS rewrite gate this PR relies on.
• Local just compile, just test, just test file BackupRepoTest, just test file WalletViewModelTest, just test file ActivityRepoTest and just lint pass.

[greptile-apps]

Confidence Score: 4/5

This PR is not safe to merge until partial restore notifications cannot trigger a backup upload.

Findings

1.  Partial restore can overwrite backup ▶

Summary

Activity backups now restore activities, tags, and closed channels independently, so one rejected slice no longer blocks the others. Partial restores still fail the overall restore and skip rewriting the existing backup.

• Each restore slice runs separately and logs its own failure.
• Observers update when at least one slice succeeds.
• Cancellation is rethrown instead of treated as a slice failure.

Diagram

sequenceDiagram
    actor Caller
    participant BackupRepo
    participant ActivityRepo
    participant Core
    participant Observer as Backup observers
    participant VSS

    Caller->>BackupRepo: performFullRestoreFromLatestBackup()
    BackupRepo->>BackupRepo: "_isRestoring = true"
    BackupRepo->>ActivityRepo: restoreFromBackup(payload)
    loop activities, tags, closed channels
        ActivityRepo->>Core: upsert slice
        alt slice succeeds
            Core-->>ActivityRepo: Unit
        else slice throws
            Core--xActivityRepo: error
            ActivityRepo->>ActivityRepo: log error and continue
        end
    end
    opt at least one slice succeeds
        ActivityRepo-->>Observer: activitiesChanged and activityTagsChanged
    end
    alt any slice failed
        ActivityRepo-->>BackupRepo: Result.failure(first error)
        BackupRepo->>BackupRepo: skip migration rewrite
    else all slices succeeded
        ActivityRepo-->>BackupRepo: Result.success(Unit)
        opt migration changed
            BackupRepo->>VSS: rewrite migrated backup
        end
    end
    BackupRepo->>BackupRepo: "_isRestoring = false"
    alt observer runs before flag clears
        Observer->>BackupRepo: change signal
        BackupRepo-->>Observer: skip while restoring
    else observer runs after flag clears
        Observer->>BackupRepo: change signal
        BackupRepo->>BackupRepo: mark backup required
        BackupRepo->>VSS: upload current data
    end

Loading

_{Reviews (1) · Last reviewed commit: "fix: skip restore notify when every acti..."}

[github-actions]

Regtest APK

Built from b6c81c8 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

[jvsena42]

Approved — no HIGH/MEDIUM findings at head 1bb6ee877. (Posted as a comment because GitHub does not allow approving your own PR.)

Checked: Traced ActivityRepo.restoreFromBackup (app/src/main/java/to/bitkit/repositories/ActivityRepo.kt:993-1020) at the PR head: each slice runs in its own runSuspendCatching, so a Core rejection of tags no longer skips closed channels; CancellationException is rethrown by runSuspendCatching (app/src/main/java/to/bitkit/ext/Coroutines.kt) and no slice result is fabricated for it. The overall Result is still a failure on any slice error, so BackupRepo.restoreActivityBackup (BackupRepo.kt:710-714) computes needsRewrite=false and rewriteMigratedBackups (BackupRepo.kt:659-661, 823-828) never re-uploads ACTIVITY from a partial local set; BackupRepoTest 'activity backup is not rewritten when core restore fails' pins that. Checked what is marked restored after a partial failure: performRestore (BackupRepo.kt:849-851) writes synced=required=createdAt for ACTIVITY and the full restore sets backupVerified regardless of the activity outcome, but that is identical on master (only WALLET is fatal) and is not changed by this PR. The tag signal now fires only when the tags slice was applied (ActivityRepo.kt:1010), so a rejected tags slice cannot dirty METADATA; the remaining path where the next ordinary activity change uploads a partial local ACTIVITY set after _isRestoring is cleared (BackupRepo.kt:657, 337) was raised by greptile, accepted by the author as a pre-existing BackupRepo limitation and recorded in Out of Scope, so it is not re-raised. The restore is only invoked from WalletViewModel.collectStates inside viewModelScope, so cancellation is not user-reachable mid-restore. iOS BackupService.swift:232-234 still restores the three slices in one block; no iOS twin PR exists, which the PR body already lists as out of scope.

[jvsena42]

Thanks — the gate is in, in aa06584.

What I verified first

Your reading of the trace holds, and the window is wider than the 30s wait alone:

• restoreWallet sets RestoreState.InProgress.Wallet (WalletViewModel.kt:563), and the node start waits on that state for at most TIMEOUT_RESTORE_WAIT = 30s (WalletViewModel.kt:74, :311-315).
• _isRestoring is only raised inside performFullRestoreFromLatestBackup (BackupRepo.kt:635), which the flow reaches after getLatestBackupTime() (WalletViewModel.kt:228), itself bounded by VSS_TIMESTAMP_TIMEOUT = 60s (BackupRepo.kt:886) and six VSS reads. So the gap between the wait expiring and _isRestoring going true can be a minute or more, which matches your 14:40:59 → 14:41:20 log.
• Every ordinary upload path is open during that gap, and all of them funnel through shouldSkipBackup(): the data-store listeners (BackupRepo.kt:265, :276, :288, :299, :312, :323, :337, :371, :385), the status observers via BackupItemStatus.shouldBackup (:156-160, :247) and the post-debounce re-check in scheduleBackup (:423). The ACTIVITY one is observeBackupChanges(activityRepo.activitiesChanged, ACTIVITY) (:357) — the node's first sync bumps it, and that is the three Backup succeeded for: 'ACTIVITY' lines.

The gate

BackupRepo.setRestorePending() (BackupRepo.kt:148), folded into shouldSkipBackup() (:155). WalletViewModel.restoreWallet raises it as the restore flow starts (:566) and it is cleared when the restore returns (:222) and when the wallet was never created, so nothing reaches the restore at all (:573).

It is the same reach _isRestoring already had — ordinary, observer-driven uploads only — just starting earlier. Four reasons it cannot deadlock into permanent suppression:

1. It expires. setRestorePending(true) stores a deadline, not a flag: RESTORE_PENDING_TIMEOUT_MS = 10 min (BackupRepo.kt:892), comfortably longer than any real restore and short enough that a restore that never returns cannot hold the session.
2. It is in memory only. An app killed mid-restore comes back with the gate open; nothing is persisted that could strand it.
3. It is cleared on every terminal path of the restore flow, including the failure paths, not only on success.
4. It never blocks an explicit triggerBackup (BackupRepo.kt:484), which does not consult shouldSkipBackup(). The restore's own migration rewrite and the Data Backups retry button keep working while the gate is up — pinned by a test.

Cross-checked against bitkit-ios while writing it: AppScene.restoreFromMostRecentBackup already does exactly this, setting BackupService.setRestoring(true) before the timestamp probe with a defer to clear it, and carries the same "triggerBackup is deliberately the ungated entry point" note. So this brings Android in line, plus the expiry iOS does not have.

Tests and negative control

Four in BackupRepoTest.kt, two in WalletViewModelTest.kt:

• an ordinary activity upload is skipped while a restore is pending — the ordering you observed, node sync bumping activitiesChanged before the restore reads the envelope;
• it resumes once the restore is no longer pending;
• the gate stops holding uploads once it expires;
• the migration rewrite still uploads while a restore is pending;
• the restore holds ordinary backups from its start until it completes (inOrder on set/clear);
• the restore releases them when the wallet is never created.

Negative control, run through the same task: dropping || isRestorePending() from shouldSkipBackup() fails exactly the two gate tests and leaves the expiry and rewrite tests green — they assert uploads do happen, so they are the guard against over-gating. Dropping the setRestorePending(true) call fails the view model ordering test. Both mutations reverted and re-run green.

Journey

Your Test 1 is committed as journeys/backup-restore/restore-keeps-tags-and-closed-channels.xml, with a suite README and its suites + cross-platform rows in journeys/README.md. Every identifier it names was checked against app/src/main on this branch, and the Data Backups "Transaction Log" row is called out as text-only since it carries no test tag. It ends by repeating the reset and restore once more, since the second restore is where the race was lost. The cross-platform row records that iOS already has the gate but still applies the three slices in one block, so the journey travels with the iOS slice fix.

Manual test 2 stays unticked — I have no device here, so that one is still yours to confirm.

just compile, just test, just lint pass locally.

[jvsena42]

Approved — no blocking HIGH/MEDIUM findings at head ba3359f47. (Posted as a comment because GitHub does not allow approving your own PR.)

Checked: Follow-up on aa06584 (backup gate while a restore is pending, plus journey): Traced the new gate at head ba3359f (aa06584). Permanent suppression: restorePendingUntil is an in-memory deadline (BackupRepo.kt:128,149), expiry is 10 minutes (BackupRepo.kt:892), and every terminal path of the restore clears it: restoreFromBackup wraps restoreFromMostRecentBackup in runCatching so success, failure and even a CancellationException from a cleared viewModelScope still reach setRestorePending(false) (WalletViewModel.kt:216-222); a failed walletRepo.restoreWallet clears it (WalletViewModel.kt:571-573) and that repo uses runCatching so a cancelled DataStore write also returns failure (WalletRepo.kt:459-473); process death drops it; the only path that leaves it up is the top-level rememberCoroutineScope (MainActivity.kt:122) being cancelled by Activity destruction inside the few milliseconds of keychain/settings writes, which also strands _restoreState on master and is bounded by the expiry. Wipe cannot run mid-restore (onboarding only) and reset() leaving the deadline in place is harmless since createWallet after a stuck restore still triggers restoreFromBackup, which clears it. Nothing else reads restorePendingUntil, and triggerBackup (retry button, rewriteMigratedBackups) stays ungated. Partial-restore overwrite: the gate now covers getLatestBackupTime, performFullRestoreFromLatestBackup, pubkyRepo.initialize and the RN fallback, so the three pre-restore 'Backup succeeded for: ACTIVITY' uploads QA observed are dropped at observeBackupChanges (BackupRepo.kt:385) and at the post-debounce recheck (BackupRepo.kt:423); the post-restore ordinary upload of a partial local set after the gate clears is the pre-existing limitation already accepted in the greptile thread and is not re-raised. Persistence/upgrade: not persisted, initial value 0L, so a released build upgrading has the gate open and no migration is needed. RN path: signals raised by applyRNRemote* while the gate is up are dropped instead of deferred, so VSS seeding of RN-migrated widgets waits for the next change in that category; on master that upload only happened when the node beat the 30s wait, the RN remote backup still holds the data, and every other category re-marks on its own within minutes, so no user-visible outcome. Tests: the two BackupRepoTest gate tests drive the real markBackupRequired -> status observer -> scheduleBackup -> putObject chain through stubBackupStatuses (BackupRepoTest.kt:1006-1023) and assert never() on putObject, so they fail without the shouldSkipBackup change; the expiry and rewrite tests assert the upload does happen (mocked clock at 700_000 > 1_000 + 600_000), and the WalletViewModelTest inOrder test fails without the setRestorePending(true) call. Journey: every testTag named resolves at head (Tab-${name.lowercase()} in CustomTabRowWithSpacing.kt:62, N$text in NumberPad.kt:240, ActivityShort-$index in ActivityListSimple.kt:52, Word-$index in RestoreWalletScreen.kt:416, restore_reset_button/dialog in ResetAndRestoreScreen.kt:125,139, the rest by literal), the log lines match BackupRepo.kt:510,633 and ActivityRepo.kt:1018-1020, the flow order matches the code, the suite is listed in the suites table and the cross-platform table records it as not ported with the reason; no bitkit-ios twin PR exists.

addressed - reaudit confirmed

[ovi-reviewer]

Verdict: ✅ Approve

---

Reaudit: diff 1 file.
Retest suggested: items 1, 2 (Restore upload gate awaits queued first- and second-restore device regression pass).

Findings:
N/A

Audit:
Already done in comment.

Coverage:
QA: journeys and manual tests await green CI checks

---

Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)

[ovitrif]

utACk