fix(restore-session): re-send AddInvoice for failed payments on session restore#754
Open
codaMW wants to merge 2 commits into
Open
fix(restore-session): re-send AddInvoice for failed payments on session restore#754codaMW wants to merge 2 commits into
codaMW wants to merge 2 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
Contributor
WalkthroughPropagates master_key through restore-session, queries DB for orders with failed payments for that master_key, and re-enqueues AddInvoice actions using each order's buyer public key. 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->>DB: find_failed_payment_for_master_key(master_key)
DB-->>Handler: Vec<Order> of failed payments
loop For each failed order
Handler->>Handler: extract buyer_pubkey
Handler->>Queue: enqueue_order_msg(AddInvoice with buyer_pubkey)
end
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Contributor
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
Contributor
Author
|
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}
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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