Skip to content

fix: sync buyer_invoice to the paid invoice on successful retry#866

Closed
grunch wants to merge 1 commit into
mainfrom
fix/864-buyer-invoice-on-retry
Closed

fix: sync buyer_invoice to the paid invoice on successful retry#866
grunch wants to merge 1 commit into
mainfrom
fix/864-buyer-invoice-on-retry

Conversation

@grunch

@grunch grunch commented Jul 7, 2026

Copy link
Copy Markdown
Member

Closes #864.

Problem

When a payout to a buyer fails and the buyer runs /setinvoice, the retry pays a new invoice that is stored only on the PendingPayment (payment_request), while order.buyer_invoice keeps pointing at the original, failed invoice. So any order that ended up SUCCESS via a retry recorded an invoice that was never paid — the real payment lived only in pendingpayments.payment_request.

This breaks reconciliation and auditing against the node (verifying the SUCCESS by the payment hash of order.buyer_invoice yields a false "payment not found"), and misleads anyone inspecting the order.

Fix

In the payment.confirmed_at block of attemptPendingPayments (jobs/pending_payments.ts), set:

order.buyer_invoice = pending.payment_request;

so the order faithfully records the invoice that was actually paid.

Doing it on confirmation (and not in /setinvoice) is deliberate: it preserves the double-payment defense at jobs/pending_payments.ts:41 (isPendingOldPayment), which needs the old invoice around to detect a previous HTLC still in flight. Touching the field only on the success path does not interfere with that logic.

Also logs the real payment hash (payment.id) instead of pending.hash — the latter stores the hold invoice hash, which made the Invoice with hash ... paid log line misleading. This matches the existing log style in payToBuyer.

Scope note

This fixes the data going forward. Orders already completed via a retry keep the old inconsistency, so external DB consumers (e.g. the stats dashboard) still need their pendingpayments fallback for the historical backlog. The two are complementary.

Tests

Adds tests/jobs/pending_payments.spec.ts with two cases:

  • successful retry → order.buyer_invoice is synced to the paid invoice, status SUCCESS, routing fee recorded;
  • non-confirming payment → buyer_invoice is left untouched and the pending payment is not marked paid.

Both pass; npx tsc compiles cleanly and Prettier reports no changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01X5uNqd26cC88p5HVqErRDU


Generated by Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • When a pending payment retry succeeds, the order now records the actual paid invoice instead of the original one.
    • Success logs now show the real payment hash, making payment records easier to trace.
    • Added regression coverage to ensure successful retries update payment status and fees correctly, while failed retries leave the order unchanged.

When a payout to a buyer fails and they run /setinvoice, the retry pays a
new invoice stored only on the pending payment, while order.buyer_invoice
kept pointing at the original (failed) invoice. Orders that ended up
SUCCESS via a retry therefore recorded an invoice that was never paid,
breaking reconciliation and auditing against the node.

Set order.buyer_invoice = pending.payment_request in the payment-confirmed
block of attemptPendingPayments so the order faithfully records the invoice
that was actually paid. Doing it on confirmation (not in /setinvoice)
preserves the double-payment defense that relies on the old invoice to
detect an in-flight HTLC.

Also log the real payment hash (payment.id) instead of pending.hash, which
stores the hold invoice hash and made the "Invoice with hash ... paid" log
misleading.

Adds a regression spec covering both the successful-retry and
failed-payment paths.

Closes #864.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X5uNqd26cC88p5HVqErRDU
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ed4552eb-d208-4ba2-8f82-54e7dc64635f

📥 Commits

Reviewing files that changed from the base of the PR and between e45b8ec and 54b308a.

📒 Files selected for processing (2)
  • jobs/pending_payments.ts
  • tests/jobs/pending_payments.spec.ts

Walkthrough

Modifies attemptPendingPayments to update order.buyer_invoice with the pending payment's payment_request on successful retry confirmation, and to log payment.id instead of pending.hash. Adds a new regression test file covering both successful and failed payment retry scenarios.

Changes

Pending payment retry fix and tests

Layer / File(s) Summary
Update buyer_invoice and logging on payment confirmation
jobs/pending_payments.ts
On successful retry, order.buyer_invoice is set to pending.payment_request instead of the original invoice, and success logging now uses payment.id instead of pending.hash.
Regression tests for retry success and failure
tests/jobs/pending_payments.spec.ts
New test file with a proxyquire-based mock harness; verifies buyer_invoice, routing_fee, pending.paid, and order.save on success, and unchanged state when payment is not confirmed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

A hop, a skip, an invoice fix so neat,
Now buyer_invoice matches what was paid, complete!
The logs no longer fib about the hash,
Tests stand guard against a future crash.
🐰✨ Thump thump — ship it, quick as a dash!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main fix: syncing buyer_invoice to the paid invoice on successful retry.
Linked Issues check ✅ Passed The changes match issue #864 by updating buyer_invoice on confirmation, improving the hash log, and adding regression tests.
Out of Scope Changes check ✅ Passed The PR stays within the stated scope and only adds the retry fix, logging improvement, and focused tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/864-buyer-invoice-on-retry

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.

@grunch grunch requested a review from Luquitasjeffrey July 7, 2026 13:52

@Luquitasjeffrey Luquitasjeffrey left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is a good idea to persist which invoice was actually paid in the order.
I disagree with the idea of erasing the first buyer invoice, i think it will be optimally to persist the invoice paid (according to the bot) to facilitate concilliation, but I dont like the idea of the original invoice to be lost. Debugging can get harder

Comment thread jobs/pending_payments.ts
// on the pending payment, while order.buyer_invoice still points at the
// original (failed) invoice. Sync it here so the order is a faithful
// record of the payment (needed for reconciliation/auditing). See #864.
order.buyer_invoice = pending.payment_request;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Original buyer_invoice is lost permanently.
Instead what I would do is to persist the paid invoice in a new Order field 'buyer_invoice_paid'.
With that approach, conciliation is easier but also no data is lost if more traceabillity and debigging may be required

@Luquitasjeffrey

Copy link
Copy Markdown
Collaborator

I think we should close this PR in favor of #876

@grunch grunch closed this Jul 10, 2026
@grunch grunch deleted the fix/864-buyer-invoice-on-retry branch July 10, 2026 18:04
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.

buyer_invoice is left stale after /setinvoice when the payout is retried with a different invoice

3 participants