Skip to content

stop wallet scans when wallet disappears#699

Open
Sandipmandal25 wants to merge 2 commits intobitcoinppl:appmanager-walletmanager-lifecyclefrom
Sandipmandal25:fix/wallet-lifecycle-rust
Open

stop wallet scans when wallet disappears#699
Sandipmandal25 wants to merge 2 commits intobitcoinppl:appmanager-walletmanager-lifecyclefrom
Sandipmandal25:fix/wallet-lifecycle-rust

Conversation

@Sandipmandal25
Copy link
Copy Markdown
Collaborator

@Sandipmandal25 Sandipmandal25 commented Apr 22, 2026

Summary

stop_all_scans had a TODO for stopping wallet scans but was only clearing transaction watchers. Scan futures were being spawned without storing their handles, so they could not be cancelled.

This change stores an abort handle for each scan (initial, expanded, incremental) when it starts, and cancels them in stop_all_scans and Drop. This ensures scans stop promptly when the wallet disappears or the actor is dropped.

fixes part of #295

Checklist

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 088c2d7d-083a-4f2e-b7ad-3a5a8ad8486a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Sandipmandal25
Copy link
Copy Markdown
Collaborator Author

fixed the rust side stop_all_scans now cancels the active scan task. android side is next will do that in next pr.
cc @praveenperera

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 22, 2026

Greptile Summary

This PR stores a tokio::task::AbortHandle for each BDK wallet scan (initial, expanded, incremental) and calls .abort() in both stop_all_scans and the new Drop impl, closing the gap where scans continued running after the wallet actor was torn down. The implementation is correct for the normal sequential scan flow.

Confidence Score: 4/5

Safe to merge; all remaining findings are P2 hardening suggestions that do not affect correctness in the current sequential scan flow.

The core change achieves its stated goal correctly. Three P2 notes: (1) scan handles are overwritten without aborting the previous — harmless given sequential scan ordering but could silently leak a task if two scans overlap; (2) stop_all_scans does not reset self.state, leaving queued actor messages a small window to update a torn-down actor; (3) single-tx send_fut scans are deliberately untracked but deserve a comment.

rust/src/manager/wallet_manager/actor.rs — specifically the three scan-launch methods and stop_all_scans

Important Files Changed

Filename Overview
rust/src/manager/wallet_manager/actor.rs Adds a single scan_task: Option<AbortHandle> field to track the active BDK scan; stores the handle in all three scan launchers (initial, expanded, incremental) and cancels it in stop_all_scans and Drop. The sequential scan flow means the single-field approach is safe in practice, but the handle is overwritten without aborting the old one, and actor state is not reset in stop_all_scans.

Sequence Diagram

sequenceDiagram
    participant C as Caller
    participant A as WalletActor
    participant T1 as InitialScanTask
    participant T2 as ExpandedScanTask
    participant T3 as IncrementalScanTask

    C->>A: start_wallet_scan_in_task()
    alt No prior full scan
        A->>A: send!(perform_full_scan)
        A->>A: maybe_perform_initial_full_scan()
        A->>T1: tokio::spawn(initial scan)
        Note over A: scan_task = T1.abort_handle()
        T1-->>A: call!(handle_full_scan_complete)
        T1->>A: send!(maybe_perform_expanded_full_scan)
        A->>A: send!(perform_expanded_full_scan)
        A->>T2: tokio::spawn(expanded scan)
        Note over A: scan_task = T2.abort_handle() ⚠️ T1 handle dropped
        T2-->>A: send!(handle_full_scan_complete)
    else Prior full scan exists
        A->>A: send!(perform_incremental_scan)
        A->>T3: tokio::spawn(incremental scan)
        Note over A: scan_task = T3.abort_handle()
        T3-->>A: send!(handle_incremental_scan_complete)
    end

    C->>A: stop_all_scans()
    Note over A: scan_task.take().abort() ✅
    Note over A: transaction_watchers cleared ✅
    Note over A: self.state NOT reset ⚠️

    Note over A: Drop
    Note over A: scan_task.take().abort() ✅
Loading

Comments Outside Diff (1)

  1. rust/src/manager/wallet_manager/actor.rs, line 968-978 (link)

    P2 Single-tx scan future not tracked by scan_task

    perform_scan_for_single_tx_id spawns its network work via self.addr.send_fut, which is not stored in scan_task. When stop_all_scans or Drop fires, this in-flight sync continues unimpeded. For the lightweight single-transaction case this may be acceptable, but a comment explaining the intentional omission would help future contributors who might expect it to be cancelled alongside the main scans.

Reviews (1): Last reviewed commit: "stop wallet scans when wallet disappears" | Re-trigger Greptile

Comment on lines 1185 to 1200
@@ -1190,6 +1195,7 @@ impl WalletActor {
// update wallet state
send!(addr.handle_full_scan_complete(full_scan_result, FULL_SCAN_TYPE));
});
self.scan_task = Some(handle.abort_handle());

Produces::ok(())
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Silent overwrite of previous scan handle

self.scan_task is overwritten without first aborting the old handle. AbortHandle::drop does not abort the underlying tokio task; only calling .abort() does. In the normal sequential flow (initial scan fully completes before the expanded scan spawns) this is fine, but if two calls somehow overlap (e.g. two start_wallet_scan_in_task calls more than 15 seconds apart while a scan is still in progress), the earlier task becomes untrackable and stop_all_scans / Drop will only cancel the later one. The same pattern applies to perform_incremental_scan and perform_initial_full_scan.

Suggested change
if let Some(old) = self.scan_task.replace(handle.abort_handle()) {
old.abort();
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

scans run sequentially so the old handle is always for a finished task, .abort() on a finished task is a no op in tokio so this is harmless. done the change anyway as its defensive

Comment thread rust/src/manager/wallet_manager/actor.rs
@Sandipmandal25 Sandipmandal25 force-pushed the fix/wallet-lifecycle-rust branch from 25f70e6 to 64a9a36 Compare April 22, 2026 14:46
Comment thread rust/src/manager/wallet_manager/actor.rs Outdated
@Sandipmandal25 Sandipmandal25 force-pushed the fix/wallet-lifecycle-rust branch from 64a9a36 to 825cbd7 Compare April 23, 2026 08:07
Comment thread rust/src/manager/wallet_manager/actor.rs
Comment thread rust/src/manager/wallet_manager/actor.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants