fix(restore-session): re-send AddInvoice for failed payments on session restore#721
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe restore-session flow now forwards Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Restore as RestoreSessionHandler
participant DB as Database
participant Queue as ActionQueue
Client->>Restore: restore_session(master_key)
Restore->>Restore: Build set of restored_order_ids
Restore->>DB: find_failed_payment_for_master_key(master_key)
DB-->>Restore: failed_payment_orders
alt matching restored orders exist
loop per order in failed_payment_orders ∩ restored_order_ids
Restore->>Queue: Enqueue Action::AddInvoice(order_payload)
Queue-->>Client: Deliver AddInvoice action
Restore-->>Restore: Log re-send
end
end
Restore-->>Client: send_restore_session_response(payload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 26 minutes and 55 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/restore_session.rs (1)
111-113: Scope the failed-payment lookup to the restored orders.
find_failed_paymentpulls the full global set offailed_payment == true/settled-hold-invoiceorders and this branch filters it in memory withrestored_ids. That makes every restore-session cost proportional to the system backlog instead of the restored order set. A targeted helper keyed by restored IDs or the restoring master key would keep this path bounded and reduce the stale window beforeAddInvoiceis re-sent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 111 - 113, The lookup currently calling find_failed_payment(&pool).await returns all failed_payment orders and is then filtered in-memory by restored_ids; change this to a targeted DB query that is bounded to the restored set — e.g., add a helper like find_failed_payment_for_ids(pool: &DbPool, restored_ids: &[Uuid]) or find_failed_payment_for_master_key(pool, master_key: &str) and replace the call in restore_session.rs to call that helper with the restored_ids (or restoring master key) so only matching failed/settled-hold-invoice orders are returned, keeping the operation proportional to the restored set and enabling prompt AddInvoice re-send.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/restore_session.rs`:
- Around line 116-123: The resend is using the stale order.get_buyer_pubkey()
instead of the restored session key, so change the enqueue logic in
restore_session to target the current session key: pass trade_key (the incoming
event.sender key used for the restore payload) to enqueue_order_msg for the
AddInvoice action, or first set order.buyer_pubkey = trade_key (and persist if
required) and then call enqueue_order_msg; update references around
enqueue_order_msg, order.get_buyer_pubkey(), trade_key, and order.buyer_pubkey
accordingly so the follow-up message is delivered to the restored session key.
---
Nitpick comments:
In `@src/app/restore_session.rs`:
- Around line 111-113: The lookup currently calling
find_failed_payment(&pool).await returns all failed_payment orders and is then
filtered in-memory by restored_ids; change this to a targeted DB query that is
bounded to the restored set — e.g., add a helper like
find_failed_payment_for_ids(pool: &DbPool, restored_ids: &[Uuid]) or
find_failed_payment_for_master_key(pool, master_key: &str) and replace the call
in restore_session.rs to call that helper with the restored_ids (or restoring
master key) so only matching failed/settled-hold-invoice orders are returned,
keeping the operation proportional to the restored set and enabling prompt
AddInvoice re-send.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6979aef3-ddf4-4fad-9fc7-f2475ed810cb
📒 Files selected for processing (1)
src/app/restore_session.rs
codaMW
force-pushed
the
fix/restore-session-failed-payment-601
branch
from
befb508 to
ae264b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/restore_session.rs (1)
100-144: Please add a regression test for the new resend path.This path now has important behavior (ID intersection + failed-payment lookup +
Action::AddInvoiceenqueue). Add a co-located test that asserts only restored failed-payment orders are re-queued.As per coding guidelines: "
**/*.rs: Co-locate tests in their modules undermod testsblocks with descriptive names" and "src/app/**: Mirror test fixtures undersrc/app/where applicable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/restore_session.rs` around lines 100 - 144, Add a co-located regression test in restore_session.rs (under mod tests) that exercises the new resend path: simulate a restore session that returns a set of restored orders, stub/mock find_failed_payment_for_master_key to return a mix of failed-order records (some whose id is in restored_ids and some not), then call the restore-session handler and assert that enqueue_order_msg is invoked only for the intersection IDs with Action::AddInvoice and with Payload::Order(SmallOrder::from(...)) while non-restored failed orders are not enqueued; use test fixtures under src/app/ to mirror any required order/dispute setup and reference the functions/enums enqueue_restore_session_msg, find_failed_payment_for_master_key, enqueue_order_msg, Action::AddInvoice, and the restored_ids behavior in your assertions.src/db.rs (1)
571-589: Add docs and a focused test for this new public query helper.
find_failed_payment_for_master_keyis a non-obvious public API (buyer master key only +failed_payment+settled-hold-invoice). Please add a///doc comment and a dedicated test insrc/db.rsmod teststhat verifies filtering bymaster_buyer_pubkeyand status/flag combination.As per coding guidelines: "
**/*.rs: Document non-obvious public APIs with///doc comments" and "Co-locate tests in their modules undermod testsblocks with descriptive names".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db.rs` around lines 571 - 589, Add a /// doc comment above the public function find_failed_payment_for_master_key explaining that it returns orders for a given master_buyer_pubkey where failed_payment = true and status = "settled-hold-invoice", then add a focused async unit test in src/db.rs's mod tests that inserts several test orders (varying master_buyer_pubkey, status, and failed_payment), calls find_failed_payment_for_master_key with the target master key, and asserts only the orders with the matching master_buyer_pubkey AND failed_payment = true AND status = "settled-hold-invoice" are returned; make the test use the module's test DB setup (tokio::test), descriptive name (e.g., test_find_failed_payment_for_master_key_filters_correctly), and cleanup any test data as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/restore_session.rs`:
- Around line 117-118: The spawned background tasks must use the cloned pool
from ctx (the Arc<Pool<Sqlite>> created around line 38) instead of calling the
global get_db_pool(); update the code to pass that cloned pool into
handle_restore_session_results and send_restore_session_response (and any other
helpers that currently call get_db_pool(), e.g., the code paths touching
find_failed_payment_for_master_key) by adding an extra parameter for the pool to
their signatures and all call sites (also update callers for the occurrences
noted around lines 54–58 and 90–95), remove internal get_db_pool() usages inside
those functions, and use the passed-in pool when awaiting DB helpers so tests
using TestContextBuilder no longer rely on the global DB_POOL.
---
Nitpick comments:
In `@src/app/restore_session.rs`:
- Around line 100-144: Add a co-located regression test in restore_session.rs
(under mod tests) that exercises the new resend path: simulate a restore session
that returns a set of restored orders, stub/mock
find_failed_payment_for_master_key to return a mix of failed-order records (some
whose id is in restored_ids and some not), then call the restore-session handler
and assert that enqueue_order_msg is invoked only for the intersection IDs with
Action::AddInvoice and with Payload::Order(SmallOrder::from(...)) while
non-restored failed orders are not enqueued; use test fixtures under src/app/ to
mirror any required order/dispute setup and reference the functions/enums
enqueue_restore_session_msg, find_failed_payment_for_master_key,
enqueue_order_msg, Action::AddInvoice, and the restored_ids behavior in your
assertions.
In `@src/db.rs`:
- Around line 571-589: Add a /// doc comment above the public function
find_failed_payment_for_master_key explaining that it returns orders for a given
master_buyer_pubkey where failed_payment = true and status =
"settled-hold-invoice", then add a focused async unit test in src/db.rs's mod
tests that inserts several test orders (varying master_buyer_pubkey, status, and
failed_payment), calls find_failed_payment_for_master_key with the target master
key, and asserts only the orders with the matching master_buyer_pubkey AND
failed_payment = true AND status = "settled-hold-invoice" are returned; make the
test use the module's test DB setup (tokio::test), descriptive name (e.g.,
test_find_failed_payment_for_master_key_filters_correctly), and cleanup any test
data as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be9474a8-6bfc-4c2b-b451-862f4b36a8eb
📒 Files selected for processing (2)
src/app/restore_session.rssrc/db.rs
codaMW
force-pushed
the
fix/restore-session-failed-payment-601
branch
from
ae264b5 to
c04d0f8
Compare
…on restore When a buyer performs restore-session after a Lightning payment fails in settled-hold-invoice state, Mostro was returning the order without re-sending the AddInvoice action. The buyer had no way to know the payment failed and that a new invoice was required. Fix: after sending the restore-session response, query for orders with failed_payment == true and status == settled-hold-invoice. For any that match the restored order IDs, re-queue Action::AddInvoice to the buyer so they receive the prompt to submit a new invoice. The existing payment safety model is unchanged - atomic state transitions and serial scheduler execution already prevent double payments. Closes MostroP2P#601
codaMW
force-pushed
the
fix/restore-session-failed-payment-601
branch
from
c04d0f8 to
e9c1f29
Compare
|
|
@codaMW sorry this was merged by mistake, would you mind to create the PR again? sorry again |
|
@codaMW It doesn't work for me: when I do the restore-session, the order with failed payment doesn't even appear in the app. After restore |
3 participants
Closes #601
Problem
When a Lightning payment fails after
settled-hold-invoice, Mostrosends
Action::AddInvoiceto request a new invoice. But if the buyerperforms
restore-sessionbefore responding, Mostro returned the orderwithout re-sending
AddInvoice. The buyer saw the order as settled withno way to know a new invoice was needed.
Fix
After sending the restore-session response, query for orders with
failed_payment == trueandstatus == settled-hold-invoice. For anythat match the restored order IDs, re-queue
Action::AddInvoiceto thebuyer.
Safety
The existing payment safety model is unchanged atomic state transitions
in
payment_successand serial scheduler execution already prevent doublepayments. This fix only adds a notification, it does not change the
payment flow.
Files changed
src/app/restore_session.rsAdded AddInvoice re-queue logic aftersending restore response
Summary by CodeRabbit