fix(restore-session): re-send AddInvoice for failed payments on session restore#754
fix(restore-session): re-send AddInvoice for failed payments on session restore#754codaMW wants to merge 5 commits into
Conversation
…on restore When a buyer performs restore-session after a Lightning payment fails in settled-hold-invoice state, Mostro returned the order without re-sending Action::AddInvoice. The buyer had no way to know a new invoice was needed. Fix: after sending the restore-session response, query orders with failed_payment = true and status = settled-hold-invoice for the restoring user's master_buyer_pubkey. For each match, re-queue Action::AddInvoice to the buyer. Manually tested on local regtest stack (bitcoind + LND + nostr-rs-relay): - Set order to settled-hold-invoice with failed_payment=1 - Ran restore-session via mostro-cli - Daemon logs confirm: Re-sent AddInvoice for order on restore-session - add-invoice payload delivered to buyer pubkey over the wire The existing payment safety model is unchanged - atomic state transitions and serial scheduler execution prevent double payments. Add three tests for find_failed_payment_for_master_key covering: - matching order returned - different master key ignored - non-failed order ignored Closes MostroP2P#601
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughPropagates master_key through restore-session handling, sends the restore response, queries DB for orders with failed payments for that master_key, and re-enqueues AddInvoice actions addressed to each order's buyer trade pubkey. ChangesFailed payment recovery in session restore
Sequence Diagram(s)sequenceDiagram
participant Handler as Restore Handler
participant DB as Database
participant Queue as Message Queue
Handler->>Queue: enqueue restore-session response (to trade_key)
Handler->>DB: find_failed_payment_for_master_key(master_key)
DB-->>Handler: Vec<Order> of failed payments
loop For each failed order
Handler->>Handler: parse order.get_buyer_pubkey()
Handler->>Queue: enqueue_order_msg_on_restore_queue(AddInvoice to buyer_trade_pubkey)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/app/restore_session.rs (1)
117-133: ⚡ Quick winLog when
get_buyer_pubkey()fails.The recovery loop silently skips orders where
get_buyer_pubkey()fails. While this defensive behavior prevents crashes, it reduces observability—operators won't know why some failed-payment orders weren't recovered. Adding a log entry whenget_buyer_pubkey()fails will help troubleshoot stuck orders.📊 Proposed logging for observability
Ok(failed_orders) => { for order in failed_orders { - if let Ok(buyer_pubkey) = order.get_buyer_pubkey() { + match order.get_buyer_pubkey() { + Ok(buyer_pubkey) => { enqueue_order_msg( None, Some(order.id), Action::AddInvoice, Some(Payload::Order(SmallOrder::from(order.clone()))), buyer_pubkey, None, ) .await; tracing::info!( "Re-sent AddInvoice for order {} on restore-session (failed payment)", order.id ); + } + Err(e) => { + tracing::warn!( + "Skipped re-sending AddInvoice for order {} (buyer_pubkey unavailable): {}", + order.id, + e + ); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/restore_session.rs` around lines 117 - 133, The loop over failed_orders currently ignores cases where order.get_buyer_pubkey() returns Err; modify the branch to log that failure (include order.id and the error) before skipping so operators can see why an order wasn’t re-enqueued. In practice, in restore_session.rs around the loop that calls get_buyer_pubkey(), add a tracing::warn/error when the Err variant occurs and include identifying context (order.id and the error message), keeping the existing successful path that calls enqueue_order_msg(None, Some(order.id), Action::AddInvoice, Some(Payload::Order(SmallOrder::from(order.clone()))), buyer_pubkey, None). Ensure the log level is appropriate for observability and does not change control flow.src/db.rs (2)
1721-1799: ⚡ Quick winAdd test case for wrong status.
The test suite verifies that non-failed orders are excluded and that different master keys are filtered, but there's no test confirming that orders with
failed_payment=truebut a different status (e.g.,'active') are excluded. The similar functionfind_failed_paymenthas this test case at lines 1696-1719.🧪 Proposed test
+ #[tokio::test] + async fn test_find_failed_payment_for_master_key_ignores_wrong_status() { + let pool = setup_orders_db().await.unwrap(); + let master_key = "a".repeat(64); + sqlx::query( + r#"INSERT INTO orders (id, kind, event_id, status, premium, payment_method, + amount, fiat_code, fiat_amount, created_at, expires_at, + failed_payment, payment_attempts, dev_fee, dev_fee_paid, + master_buyer_pubkey) + VALUES (?1, 'buy', 'ev1', 'active', 0, 'lightning', + 100000, 'USD', 100, 1700000000, 1700086400, + 1, 3, 0, 0, ?2)"#, + ) + .bind(uuid::Uuid::new_v4()) + .bind(&master_key) + .execute(&pool) + .await + .unwrap(); + let result = super::find_failed_payment_for_master_key(&pool, &master_key) + .await + .unwrap(); + assert!( + result.is_empty(), + "Should not return orders with wrong status" + ); + } +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 1721 - 1799, Add a test for find_failed_payment_for_master_key that ensures orders with failed_payment = 1 but a non-matching status are ignored: create a new #[tokio::test] (e.g., test_find_failed_payment_for_master_key_ignores_wrong_status) that uses setup_orders_db(), inserts an order with master_buyer_pubkey set to the target key and failed_payment = 1 but status set to something other than "settled-hold-invoice" (e.g., "active"), call super::find_failed_payment_for_master_key(&pool, &master_key).await.unwrap() and assert the result is empty; follow the pattern and bind usage from the existing tests to match insertion style.
574-592: ⚡ Quick winAdd input validation for
master_keyparameter.For consistency with similar DB functions like
find_user_orders_by_master_key(lines 1138-1141), validate thatmaster_keyis a 64-character hex string. While the caller validates the key, defensive coding at the DB layer prevents potential misuse and maintains consistency across the codebase.🛡️ Proposed validation
pub async fn find_failed_payment_for_master_key( pool: &SqlitePool, master_key: &str, ) -> Result<Vec<Order>, MostroError> { + // Validate public key format (32-bytes hex) + if !master_key.chars().all(|c| c.is_ascii_hexdigit()) || master_key.len() != 64 { + return Err(MostroCantDo(CantDoReason::InvalidPubkey)); + } + let orders = sqlx::query_as::<_, Order>(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 574 - 592, Add defensive validation at the start of find_failed_payment_for_master_key to ensure master_key is a 64-character hexadecimal string (same validation used by find_user_orders_by_master_key); if validation fails, return the same MostroError client/validation error used by find_user_orders_by_master_key (do not proceed to query). This keeps behavior consistent and prevents malformed keys from reaching the DB layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/restore_session.rs`:
- Around line 117-133: The loop over failed_orders currently ignores cases where
order.get_buyer_pubkey() returns Err; modify the branch to log that failure
(include order.id and the error) before skipping so operators can see why an
order wasn’t re-enqueued. In practice, in restore_session.rs around the loop
that calls get_buyer_pubkey(), add a tracing::warn/error when the Err variant
occurs and include identifying context (order.id and the error message), keeping
the existing successful path that calls enqueue_order_msg(None, Some(order.id),
Action::AddInvoice, Some(Payload::Order(SmallOrder::from(order.clone()))),
buyer_pubkey, None). Ensure the log level is appropriate for observability and
does not change control flow.
In `@src/db.rs`:
- Around line 1721-1799: Add a test for find_failed_payment_for_master_key that
ensures orders with failed_payment = 1 but a non-matching status are ignored:
create a new #[tokio::test] (e.g.,
test_find_failed_payment_for_master_key_ignores_wrong_status) that uses
setup_orders_db(), inserts an order with master_buyer_pubkey set to the target
key and failed_payment = 1 but status set to something other than
"settled-hold-invoice" (e.g., "active"), call
super::find_failed_payment_for_master_key(&pool, &master_key).await.unwrap() and
assert the result is empty; follow the pattern and bind usage from the existing
tests to match insertion style.
- Around line 574-592: Add defensive validation at the start of
find_failed_payment_for_master_key to ensure master_key is a 64-character
hexadecimal string (same validation used by find_user_orders_by_master_key); if
validation fails, return the same MostroError client/validation error used by
find_user_orders_by_master_key (do not proceed to query). This keeps behavior
consistent and prevents malformed keys from reaching the DB layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7d8bdaa-b0ad-4f40-a803-2b68c14b9fc8
📒 Files selected for processing (2)
src/app/restore_session.rssrc/db.rs
When a buyer's Lightning payment fails in settled-hold-invoice state, Mostro sends Action::AddInvoice requesting a new invoice. If the buyer disconnects before responding and later calls restore-session, the order was returned but AddInvoice was never re-sent. The buyer had no way to know action was required and the order would be permanently stuck. After sending the restore-session response, query for orders with failed_payment=true scoped to the restoring user's master key. For each match, re-enqueue Action::AddInvoice to the buyer. The existing payment safety model is unchanged -- atomic state transitions already prevent double payments. This fix only adds a notification. Added find_failed_payment_for_master_key DB helper with four unit tests. Added warn log when buyer pubkey is unavailable for an order. Added input validation to the DB helper for consistency. Closes MostroP2P#601
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db.rs (1)
582-595: Optional: consider indexing for master-key lookups.This query (and the sibling restore-session/pending-order queries) filters on
master_buyer_pubkey+status. If theorderstable grows, a composite index on(master_buyer_pubkey, status)would keep these lookups cheap. No change needed for this PR if order volume is small.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 582 - 595, The SELECT on the orders table filters by master_buyer_pubkey and status which will become slow at scale; add a composite database index on (master_buyer_pubkey, status) (or the equivalent migration that creates idx_orders_master_buyer_pubkey_status) so queries like the function that runs this sqlx::query_as::<_, Order>(...) against orders use the index; implement this as a DB migration (or ALTER TABLE/CREATE INDEX) and include a rollback if you use migrations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/db.rs`:
- Around line 582-595: The SELECT on the orders table filters by
master_buyer_pubkey and status which will become slow at scale; add a composite
database index on (master_buyer_pubkey, status) (or the equivalent migration
that creates idx_orders_master_buyer_pubkey_status) so queries like the function
that runs this sqlx::query_as::<_, Order>(...) against orders use the index;
implement this as a DB migration (or ALTER TABLE/CREATE INDEX) and include a
rollback if you use migrations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef1c1fea-a908-490a-b0a0-562539a4fbaf
📒 Files selected for processing (2)
src/app/restore_session.rssrc/db.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/restore_session.rs
|
Noted. This is a valid future optimization. Deferring to a separate PR to keep this fix minimal and focused. The order volume in current Mostro deployments does not warrant the index at this time. |
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I agree with the direction of this fix, but I do see one correctness issue that I would block on before merge.
Blocking issue:
- The recovery query is correctly scoped by
master_buyer_pubkey, but the re-sentAction::AddInvoiceis addressed toorder.get_buyer_pubkey(), i.e. the order's stored session/trade buyer pubkey, not the reconnecting identity that just performedrestore-session. That creates a real mismatch in the exact scenario this PR is trying to heal: if the buyer disconnected, rotated session keys, restored from another device, or otherwise no longer consumes messages on the old buyer pubkey, Mostro will still re-send the prompt to the stale destination and the restored user remains stuck with no visibleAddInvoicerequest.
The restore flow here is anchored on the reconnecting master_key, so the follow-up notification needs to be delivered to the currently restoring session/key, not to whatever buyer pubkey happened to be stored on the order when the first payment attempt failed. Otherwise we only fix the "same exact session key is still alive" case, which is not the failure mode restore-session is meant to recover.
Aside from that, the DB helper and the tests look reasonable, and I like that you added observability for missing buyer pubkeys. But I would still request changes until the notification is sent to the right restoring destination.
…tale buyer pubkey The previous code sent Action::AddInvoice to order.get_buyer_pubkey() — the session/trade key stored on the order when the payment first failed. If the buyer disconnected, rotated session keys, restored from another device, or otherwise no longer monitors that pubkey, the re-sent prompt was delivered to a stale destination and the restored user remained stuck. Fix: parse master_key (the pubkey that just sent restore-session) into a PublicKey and deliver AddInvoice to that key instead. The stored buyer_pubkey is still checked for observability — a warning is logged when it is missing — but delivery always goes to the reconnecting identity regardless. Tested on live stack: seeded order with failed_payment=1 and deliberately different master_buyer_pubkey vs buyer_pubkey, sent restore-session, confirmed daemon logged Re-sent AddInvoice to restoring identity and DM was delivered to master_key not stale buyer_pubkey. Addresses mostroneitor review request on PR MostroP2P#754.
|
Fixed in ddce729. The root cause was in The fix parses Tested on live stack: seeded order with |
|
Hi @Catrya, addressed in ddce729.
Tested on a live stack with deliberately mismatched Please let me know if there is anything else to address, thank you. |
There was a problem hiding this comment.
I re-reviewed the current head. The blocker I raised previously is resolved on this revision.
What changed and why this clears it:
- The failed-payment recovery is still scoped by
master_buyer_pubkey, but the re-sentAction::AddInvoiceis now delivered to the reconnectingmaster_keyidentity instead of the order's stale storedbuyer_pubkey. - That matches the real restore-session contract: the user who just restored is the one who needs to see the prompt, even if the original failed-payment session key is no longer active because of device switch, key rotation, or seed restore.
- Keeping the stored
buyer_pubkeycheck only for observability is fine. It preserves signal for inconsistent data without coupling delivery to a stale destination.
I also re-checked the surrounding change shape and it still looks reasonable: the helper query is narrowly scoped, the invalid-pubkey handling is explicit, and the additional tests around find_failed_payment_for_master_key cover the basic selector behavior.
Approved on the current revision.
|
Thank you for the thorough re-review. Glad the revised delivery logic addresses the concern anchoring on the restoring identity rather than the stored session key is the right contract for this flow. |
|
It still needs changes. On restore-session the re-sent AddInvoice is addressed to the master_buyer_pubkey instead of the trade key This is wrong :
It should be sent to the order's trade key. Additionally, the AddInvoice is enqueued/sent before the restore-session response; it should be sent after it. Look at this logs: Please test the PR after making the changes, keeping this in mind:
|
…re response Addresses two issues raised in review: 1. Recipient: the re-sent AddInvoice was addressed to the buyer's master/ identity key. Clients listen for order messages on trade keys, so the prompt never reached the user; and publishing the identity key as a gift-wrap recipient links it to the order, breaking trade-key unlinkability. find_failed_payment_for_master_key filters on master_buyer_pubkey, so every returned order has this user as buyer, so order.get_buyer_pubkey() (the buyer trade key) is the correct recipient. 2. Ordering: the AddInvoice was sent before the restore-session response because it went through queue_order_msg, which the scheduler drains before queue_restore_session_msg. It now routes through the restore- session queue (new enqueue_order_msg_on_restore_queue helper), preserving FIFO so the client receives the restore list first, then the AddInvoice. Verified on regtest with daemon logs: restore-session sent first, then AddInvoice to the buyer trade key (not the identity key).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/util.rs`:
- Around line 1124-1135: The restore-path currently drops the buyer's trade
index by calling Message::new_order(...) with trade_index = None; update
enqueue_order_msg_on_restore_queue to accept an additional trade_index parameter
and pass that through into Message::new_order instead of hardcoding None, and
then update the caller in restore_session (where it re-enqueues
Action::AddInvoice) to pass order.trade_index_buyer into
enqueue_order_msg_on_restore_queue so restored AddInvoice messages preserve
trade_index_buyer.
🪄 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: 8f6fd1a2-1b9e-4634-ab0c-cac4947b243e
📒 Files selected for processing (2)
src/app/restore_session.rssrc/util.rs
The restore-path AddInvoice was built with trade_index = None, giving it a different wire shape than the normal AddInvoice flow (src/util.rs), which passes order.trade_index_buyer. Clients that key the prompt back to the buyer session via trade_index could receive the DM but reply from the wrong key. enqueue_order_msg_on_restore_queue now takes a trade_index and the caller passes order.trade_index_buyer, matching the normal flow. Verified on regtest: restored AddInvoice payload now carries trade_index=3 (the buyer's index) instead of null. Addresses CodeRabbit review comment.
|
Hi @Catrya, both issues you raised are fixed and pushed.
CodeRabbit also caught a related issue I fixed in a follow-up commit: the restored AddInvoice was dropping trade_index_buyer (it was building the message with trade_index = None, unlike the normal AddInvoice flow). It now preserves it so the wire shape matches a normal AddInvoice. Verified on regtest with a seeded failed-payment order (deliberately different master_buyer_pubkey vs buyer_pubkey). Daemon logs confirm the send order and recipients: restore-session -> 3dccb248... (master) sent first builds clean, fmt + clippy clean, restore_session test passes, commits GPG-signed. I verified the daemon behavior via CLI since it shows the exact recipients/ordering deterministically. |
|
Hi @Catrya, I tested on the mobile client as you suggested. Summary: the daemon fix works correctly, and I've pinpointed why the prompt still doesn't show on mobile, it's the client-side restore behavior tracked in mobile#586, not this PR. Daemon side (this PR) verified:
Mobile side the phone receives it correctly but skips it: Received DM ... "action":"add-invoice" ... order e4da4124 ... buyer_trade_pubkey 873a76cd... but immediately logs: Skipping old message processing during restore: add-invoice So the client saves the message but does not render the Pay/Add-Invoice prompt instead it shows the older released state ("...has released the Sats... paying sats"). This is exactly the behavior described in mobile#586: during restore the client intentionally ignores gift-wrap messages and rebuilds state only from the restore-session payload. What this means: Daemon checks: builds clean, fmt + clippy clean, restore_session test passes, commits GPG-signed. Happy to pick up mobile#586 next to close the full loop if that's useful. |
2 participants
Problem
When a Lightning payment fails after a hold invoice is settled, Mostro sets
failed_payment = trueon the order and sends the buyer anAction::AddInvoicemessage asking them to provide a new invoice. This works correctly on first attempt.However, if the buyer disconnects before responding and later performs a
restore-sessionto recover their pending orders, Mostro returned the order list correctly but never re-sent theAddInvoicemessage. The buyer could see the order insettled-hold-invoicestatus but had no prompt to act on it and no way to know a new invoice was required. The order would be permanently stuck.This is a real money-at-risk bug: the seller's hold invoice has already been settled, the buyer's sats are allocated, but the payment cannot complete because the buyer has no invoice to submit.
Root Cause
The
restore-sessionhandler (src/app/restore_session.rs) only sends back the list of active orders and disputes. It had no logic to detect orders requiring active buyer participation, specifically orders insettled-hold-invoicestatus withfailed_payment = true, and re-trigger the necessary prompt.The original
AddInvoicemessage is ephemeral (in-memory queue). Once the buyer disconnects it is gone.restore-sessionwas the only reconnect mechanism and it did not reconcile this gap.Fix
After
send_restore_session_responsesends the order list back to the user, a reconciliation step is added:find_failed_payment_for_master_keya new targeted SQL query that fetches only orders wherefailed_payment = true AND status = 'settled-hold-invoice' AND master_buyer_pubkey = ?for the reconnecting userAction::AddInvoiceto the buyerThe fix is intentionally minimal:
get_db_pool()which references the same underlying pool as the rest of the daemonSafety
The existing payment safety model is completely unchanged. Atomic state transitions and serial scheduler execution already prevent double payments even if
AddInvoiceis sent multiple times. This fix only adds a notification. It does not touch any payment state.Manual Testing
Tested on a local regtest stack: bitcoind v25 + LND v0.18.5 + nostr-rs-relay + Mostro daemon (this branch) + mostro-cli.
Steps:
restore-sessionvia mostro-cli as the buyer identityDaemon logs confirming the fix works:
Verified existing restore-session is unaffected: A normal pending order restored correctly with no regression.
CLI note: The CLI showed
Received response with mismatched action. Expected: RestoreSession, Got: AddInvoice. The AddInvoice arrived correctly at the client but the CLI does not currently handle receiving AddInvoice interleaved with a restore-session response. This is a pre-existing CLI-side limitation tracked separately in mostro-cli#170, not introduced by this PR.Files Changed
src/app/restore_session.rsAddedmaster_keyparameter threading and AddInvoice re-send logic after restore responsesrc/db.rsAddedfind_failed_payment_for_master_keyhelper function with three unit testsUnit Tests Added
Three tests for
find_failed_payment_for_master_keyinsrc/db.rs:test_find_failed_payment_for_master_key_returns_matchingcorrect order is foundtest_find_failed_payment_for_master_key_ignores_different_keyother users' orders not returnedtest_find_failed_payment_for_master_key_ignores_non_failedorders withfailed_payment = falsenot returnedAll 330+ existing tests continue to pass.
Pull Request Checklist
Disclosure: I consulted Claude to understand the codebase structure but the solution, testing, and verification were done by myself.
Summary by CodeRabbit