GH-598: Receivables scheduling hot fix #750
Conversation
| impl RescheduleScanOnErrorResolverReal { | ||
| fn resolve_new_payables( | ||
| err: &StartScanError, | ||
| is_externally_triggered: bool, |
There was a problem hiding this comment.
I have an idea!
How about StartScanError are of following types:
enum StartScanError {
Automatic(AutomaticError),
Manual(ManualError),
Test
}
enum AutomaticError {
ScanAlreadyRunning {
cross_scan_cause_opt: Option<ScanType>,
started_at: SystemTime,
},
Common(CommonError)
}
enum ManualError {
AutomaticScanConflict,
UnnecessaryRequest { hint_opt: Option<String> },
Common(CommonError)
}
enum CommonError {
NothingToProcess,
NoConsumingWalletFound
}This way, we can get rid of the flag is_externally_triggered and hopefully unreachable!() too if we refactor it further.
There was a problem hiding this comment.
This costs me a lot of pain for little in return 🤕
I gave up, this idea was a no-pass-through.
node/src/test_utils/recorder.rs
Outdated
| pub fn build_and_provide_addresses(self) -> (PeerActors, PeerActorAddrs) { | ||
| // | ||
| // The addresses may be helpful for setting up the Counter Messages. | ||
| pub fn build_providing_addresses(self) -> (PeerActors, PeerActorAddrs) { |
There was a problem hiding this comment.
build_and_provide_addresses was a better name.
… perspective of ScanAlreadyRunning error
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Receivables can stall if completion message missing
Handler<ScanForReceivables> no longer schedules the next receivables scan; scheduling now happens only when ReceivedPayments arrives. If the receivable scan starts but the expected completion message never reaches Accountant (lost, actor stopped, bridge misbehavior without ScanError), periodic receivables scanning can stop indefinitely.
Additional Locations (1)
| Err(StartScanError::no_consuming_wallet_found( | ||
| response_skeleton_opt, | ||
| )) | ||
| } |
There was a problem hiding this comment.
Bug: NullScanner error semantics changed unexpectedly
NullScanner::start_scan now returns StartScanError::no_consuming_wallet_found(...) instead of a dedicated “null scanner invoked” error. If NullScanner ever leaks into non-test paths (miswiring, partial replacement), it can be misdiagnosed as a missing wallet and feed into rescheduling/error-handling decisions incorrectly.
…itectonal attempt and replacing it with a much simpler one
| let scheduling_hint = self.handle_request_of_scan_for_receivable(msg.response_skeleton_opt); | ||
|
|
||
| match scheduling_hint { | ||
| ScanReschedulingAfterEarlyStop::Schedule(other_scan_type) => unreachable!( |
There was a problem hiding this comment.
Does this mean that we are never scheduling another scan? (it's confusing why is it like that)
utkarshg6
left a comment
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/mod.rs:415
| expected = "internal error: entered unreachable code: Early stopped receivable scan \ | ||
| was suggested to be followed up by the scan for PendingPayables, which is not supported though" | ||
| )] | ||
| fn start_scan_error_in_receivables_and_unexpected_instruction_from_early_stop_scan_rescheduling( |
There was a problem hiding this comment.
I understand that you've written the code with utmost confidence that we'll never hit the unreachable!() statements and I respect that. At the same time, I feel concerned that the testing team/user might encounter this and will end up panicking their Node. To prevent such cases, I'd appreciate if we avoid such panicking statements in the future if possible. 🙏
PS: This comments is not directly related to this case, but a generic statement.
Note
Adjust receivables scheduling and rescheduling on errors, replace/extend scanner error-hint enum, rename scan interval API, and propagate these changes across scanners, mocks, and tests.
ScanForReceivablesnow respects rescheduling hints;ReceivedPaymentsschedules the next receivable scan when no UI response.handle_start_scan_error; add support for Receivables in rescheduling logic.reset_scan_timer()call when initiating new payables.PayableSequenceScannerwithUnableToStartScanner(includesReceivables).time_until_next_scan()tocompute_time_to_next_scan()(real + mock + callers).StartScanError::nothing_to_process(response_skeleton_opt)in payable and pending-payable scanners.check_pending_payable_existence; adjust guard logic.build_and_provide_addresses()tobuild_with_addresses(); update call sites.Written by Cursor Bugbot for commit 8c5f218. This will update automatically on new commits. Configure here.