handle slow payments to buyer#844
Conversation
|
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:
WalkthroughThe PR adds structured LN payment status checks, updates invoice guards to treat confirmed payments as blocking or healable, refactors pending-payment retries around status checks, and introduces a shared success-completion helper with tests for the new flows. ChangesPayment Status Hardening and Success Reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 2
🤖 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 `@jobs/pending_payments.ts`:
- Around line 86-131: The condition at the start of the confirmed payment block
uses AND logic which means if is_confirmed is true but payment is falsy, the
entire block is skipped and execution falls through to re-payment logic.
Instead, change the primary skip condition to check only is_confirmed, then nest
the notification messages (toAdminChannelPendingPaymentSuccessMessage,
toBuyerPendingPaymentSuccessMessage, and rateUserMessage) inside a separate
check for when payment is present. This prevents re-payment of confirmed
invoices when payment data is missing. Apply the same fix pattern to the
attemptCommunitiesPendingPayments function which has the identical
vulnerability.
In `@ln/pay_request.ts`:
- Around line 245-256: The catch block in getPaymentStatus is returning a
safe-looking response for all errors, including unexpected ones like transient
connectivity issues, which can mask real failures and cause callers to unsafely
proceed with payment. Modify the error handling to only return the
is_confirmed/is_pending false response for legitimate "not found" cases (errors
containing SentPaymentNotFound, PaymentNotFound, or 404), and rethrow all other
unrecognized errors so that callers like payToBuyer and attemptPendingPayments
can properly abort before attempting payment instead of treating an unknown
error state as safe.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e68f4c3-ad4c-43c0-bf7e-ce206d4b7095
📒 Files selected for processing (5)
bot/modules/community/scenes.tsbot/scenes.tsjobs/pending_payments.tsln/index.tsln/pay_request.ts
|
What does is_confirmed mean? are we introducing on-chain (non-lightning) payments? If the answer is no, the word "confirmed" might be measleading (i.e. "confirmations"). |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review
Direction is correct and it fixes two real bugs along the way:
- Good fix: the old
attemptCommunitiesPendingPaymentsdidif (isPending) return;— areturnthat aborted the whole community payout batch, not just that item. Now it'scontinue. - Good fix:
attempts++is no longer incremented on every poll while a payment is in-flight; it's incremented only right before an actual pay attempt. sellerUser.save()→await sellerUser.save().
I don't see a critical fund-loss bug: for the same payment_request, LND's payment-hash idempotency backstops the check (a second payViaPaymentRequest on an in-flight/settled hash fails rather than double-paying). Build (tsc --noEmit), eslint, and the test suite (133 passing) are all green locally. That said, a few things should be addressed before merge — details inline.
Summary
- 🟠 The "healing" branches mark the order
SUCCESSbut skip buyer notification,trades_completed, rating androuting_fee, unlike the full success routine — inconsistent completed-order state. - 🟡
getPaymentStatusfails open on unknown/transient LND errors (returns all-false → caller proceeds to pay). For a double-pay guard this should fail closed (treat unknown error as pending/skip). - 🟡 Not-found detection via
error.toString().includes('404' | 'SentPaymentNotFound')is fragile; inspect the structured error instead. - 🟡
payment?: anydrops SDK typing; the success routine is now triplicated and the heal branches are already diverging from it (see first point). - 🟡 N+1 LND RPCs per job run;
previousPendingPaymentsquery isn't filtered. - 🟢 No tests for the new, fund-sensitive logic.
Verdict: request changes. The must-haves before merge are (1) unify notification/reputation in the heal branches and (2) make getPaymentStatus fail-closed on unknown errors. The rest are recommended but non-blocking.
| logger.info( | ||
| `Order ${order._id}: original buyer invoice already confirmed, marking SUCCESS and skipping retry`, | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
Healing branch doesn't notify or credit. When order.buyer_invoice is already confirmed the buyer did receive the sats, but here we only set status='SUCCESS' + pending.paid=true — no buyer message, no trades_completed++ for either party, no routing_fee, no rateUserMessage. Same for the previousPendingPayments-confirmed branch below and the /setinvoice heal in bot/scenes.ts. Only the currentStatus.is_confirmed branch runs the full routine. Result: "phantom" completed orders with no reputation/rating and an uninformed buyer. Run the same success routine when healing by confirmation.
| _id: { $ne: pending._id }, | ||
| order_id: order._id, | ||
| is_invoice_expired: false, | ||
| }); |
There was a problem hiding this comment.
N+1 LND RPCs per job run: this now does getPaymentStatus(order.buyer_invoice) + one per previousPendingPayments + one for the current request, for every pending, every 5 min. Acceptable at current scale but worth noting. This query also doesn't filter paid:false/attempts, so it scans more rows than needed.
| if (alreadyPaid) { | ||
| order.status = 'SUCCESS'; | ||
| await order.save(); | ||
| return await messages.invoiceAlreadyUpdatedMessage(ctx); |
There was a problem hiding this comment.
Same consistency gap as the job: heals to SUCCESS but the buyer (who is here precisely because they think they weren't paid) gets invoiceAlreadyUpdatedMessage, which is confusing. If the original invoice is confirmed, tell them they were already paid and credit the success routine.
fix: address grunch review on handle-delayed-payment PR
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
bot/scenes.ts (1)
181-200: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t persist partial
SUCCESSoutside the success helper.The fallback sets only
order.status = 'SUCCESS'. That bypassescompleteOrderAsSuccess, so routing fee, trade counters, and success/rating messages can be skipped; once status isSUCCESS, the helper’s compare-and-set cannot heal those side effects later.Suggested guard split
if (originalStatus?.is_confirmed) { const i18nCtx = await getUserI18nContext(buyer); + if (!originalStatus.payment) { + logger.error( + `Order ${order._id}: confirmed buyer invoice has no payment details`, + ); + await messages.invoiceAlreadyUpdatedMessage(ctx); + return ctx.scene.leave(); + } const sellerUser = await User.findOne({ _id: order.seller_id }); - if (originalStatus.payment && sellerUser !== null) { - // Shared, idempotent success routine: if the pending-payments job - // already closed this order the compare-and-set inside makes this a - // no-op so the buyer is not notified twice. - await completeOrderAsSuccess( - bot, - order, - originalStatus.payment, - buyer, - sellerUser, - i18nCtx, - ); - } else { - order.status = 'SUCCESS'; - await order.save(); - await messages.invoiceAlreadyUpdatedMessage(ctx); - } + if (sellerUser === null) throw Error('sellerUser was not found in DB'); + await completeOrderAsSuccess( + bot, + order, + originalStatus.payment, + buyer, + sellerUser, + i18nCtx, + ); return ctx.scene.leave(); }🤖 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 `@bot/scenes.ts` around lines 181 - 200, The fallback path in scenes.ts is only setting order.status to SUCCESS, which skips the side effects handled by completeOrderAsSuccess. Update the originalStatus?.is_confirmed branch in the same flow that calls completeOrderAsSuccess so the order cannot be partially persisted as SUCCESS without routing fee, trade counter, and rating/message updates; if the helper cannot run, keep the order in its prior state or gate the fallback on the same conditions used by completeOrderAsSuccess rather than saving SUCCESS directly.
🧹 Nitpick comments (1)
tests/bot/pending-payments.spec.ts (1)
111-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the CAS call, not just the side effects.
These cases currently pass as long as
order.statusis mutated in memory. They never prove that the success path used the intended atomicfindOneAndUpdate(...status != SUCCESS...)guard, so a regression back to a plainsave()would slip through. Keep that stub in a variable and assert its filter/update in one happy-path case.Also applies to: 172-218, 258-291
🤖 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 `@tests/bot/pending-payments.spec.ts` around lines 111 - 117, The pending-payments tests are only checking side effects and not proving the atomic compare-and-set path is used. In the happy-path cases in pending-payments.spec.ts, keep the Order.findOneAndUpdate stub in a named variable and assert it was called with the expected filter guarding against already-SUCCESS orders and the intended update payload, so completeOrderAsSuccess cannot regress to a plain save() without failing these tests.
🤖 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 `@bot/scenes.ts`:
- Around line 132-133: The healed-payment flow is pulling the bot from
ctx.wizard.bot, but that property is not available in the wizard context and can
leave completeOrderAsSuccess with undefined. Update the scene logic in scenes.ts
to accept and use the bot instance explicitly from the surrounding
handler/context, and pass that concrete bot through to completeOrderAsSuccess so
the completion messages can always be sent.
In `@tests/ln/get-payment-status.spec.ts`:
- Around line 20-23: The test setup in get-payment-status.spec.ts derives
destination from an unchecked random private key via ecc.pointFromScalar(),
which can intermittently fail when makeInvoice() runs. Update the test fixture
near the priv/destination setup to use a deterministic valid key pair or add
validation and retry logic until pointFromScalar() returns a non-null point.
Keep the change localized to the test helper setup so the invoice creation path
is always fed a valid destination.
In `@util/completeOrder.ts`:
- Around line 28-61: The completion flow in completeOrder is not retryable
because Order.findOneAndUpdate writes SUCCESS before buyerUser.save,
sellerUser.save, and the message sends, so a later failure leaves the order
partially completed. Update completeOrder to make the DB updates and completion
side effects retry-safe by using a transaction/atomic completion marker or an
outbox pattern, and ensure the status transition, trade counters, and
pending/payment notifications can be resumed if any await fails.
- Around line 40-43: The `trades_completed` updates in `completeOrder` are still
using read-modify-write via `buyerUser.save()` and `sellerUser.save()`, which
can lose increments under concurrent order completions. Update the counter
writes to use atomic `$inc` operations on the user records instead of mutating
the loaded documents, and if the order status change must stay in sync with both
counter updates, group the `findOneAndUpdate` logic and the user counter updates
in a transaction.
---
Duplicate comments:
In `@bot/scenes.ts`:
- Around line 181-200: The fallback path in scenes.ts is only setting
order.status to SUCCESS, which skips the side effects handled by
completeOrderAsSuccess. Update the originalStatus?.is_confirmed branch in the
same flow that calls completeOrderAsSuccess so the order cannot be partially
persisted as SUCCESS without routing fee, trade counter, and rating/message
updates; if the helper cannot run, keep the order in its prior state or gate the
fallback on the same conditions used by completeOrderAsSuccess rather than
saving SUCCESS directly.
---
Nitpick comments:
In `@tests/bot/pending-payments.spec.ts`:
- Around line 111-117: The pending-payments tests are only checking side effects
and not proving the atomic compare-and-set path is used. In the happy-path cases
in pending-payments.spec.ts, keep the Order.findOneAndUpdate stub in a named
variable and assert it was called with the expected filter guarding against
already-SUCCESS orders and the intended update payload, so
completeOrderAsSuccess cannot regress to a plain save() without failing these
tests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d36da1fb-86fe-4571-a4cd-aa5c2a67c93d
📒 Files selected for processing (7)
bot/scenes.tsjobs/pending_payments.tsln/index.tsln/pay_request.tstests/bot/pending-payments.spec.tstests/ln/get-payment-status.spec.tsutil/completeOrder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- jobs/pending_payments.ts
| const priv = crypto.randomBytes(32); | ||
| const destination = Buffer.from(ecc.pointFromScalar(priv, true)).toString( | ||
| 'hex', | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file and any local references to makeInvoice / pointFromScalar.
git ls-files 'tests/ln/get-payment-status.spec.ts' 'package.json' 'package-lock.json' 'pnpm-lock.yaml' 'yarn.lock' 2>/dev/null || true
printf '\n--- FILE ---\n'
sed -n '1,220p' tests/ln/get-payment-status.spec.ts
printf '\n--- SEARCH ---\n'
rg -n "pointFromScalar|makeInvoice|randomBytes\\(32\\)|Buffer\\.from\\(" tests/ln/get-payment-status.spec.ts tests -g '!**/node_modules/**' || true
printf '\n--- PACKAGE INFO ---\n'
sed -n '1,220p' package.json 2>/dev/null || trueRepository: lnp2pBot/bot
Length of output: 8878
🌐 Web query:
tiny-secp256k1 pointFromScalar null invalid scalar documentation
💡 Result:
In the tiny-secp256k1 library, the pointFromScalar(d[, compressed]) function handles scalar inputs to derive a point on the secp256k1 elliptic curve [1][2]. According to the library's documentation, the function behaves as follows regarding null returns and invalid scalars: 1. Null Returns: The function returns null if the resulting point is at infinity [1][3]. This occurs when the scalar provided is invalid or specifically results in the point at infinity (for example, if the scalar is zero or a multiple of the curve order) [1][2]. 2. Invalid Scalars: If the provided scalar input (d) is invalid, the function throws an error [1][2]. Specifically, it throws an 'Expected Private' error if the input fails the isPrivate(d) check, which verifies that the scalar is a valid private key (i.e., it must be a 32-byte buffer and numerically greater than 0 and less than the curve order) [1][3]. In summary, the function distinguishes between mathematically valid scalars that result in the point at infinity (which return null) and inputs that are not valid scalars (which throw an error) [1][2].
Citations:
- 1: https://github.com/bitcoinjs/tiny-secp256k1
- 2: https://github.com/bitcoinjs/tiny-secp256k1/blob/master/README.md
- 3: https://www.npmjs.com/package/tiny-secp256k1
🌐 Web query:
Buffer.from(null) TypeError Node.js documentation
💡 Result:
In Node.js, calling Buffer.from(null) throws a TypeError because null is not a valid input type for this method [1]. The Buffer.from method requires its input to be one of the following types: string, Buffer, ArrayBuffer, Array, or an Array-like object [1][2]. Because null does not meet these criteria, Node.js explicitly validates the input and throws an ERR_INVALID_ARG_TYPE error (which is an instance of TypeError) to prevent incorrect memory allocation or unexpected behavior [1][3]. This strict validation is intended to ensure that the method only receives data that can be correctly converted into a Buffer [3]. If you encounter this error in your code, it usually means a variable you expected to contain data was unexpectedly null at the time Buffer.from was called [1]. To resolve it, ensure the input is sanitized or checked for nullity before passing it to Buffer.from.
Citations:
- 1: Buffer.from doesn't allow "null" chunk nock/nock#1944
- 2: https://github.com/nodejs/node/blob/main/doc/api/buffer.md
- 3: nodejs/node@75eaf25e78
Avoid deriving the test invoice from an unchecked random private key. tiny-secp256k1.pointFromScalar() can throw on invalid scalars and return null in edge cases, so makeInvoice() can fail intermittently here. Use a fixed fixture or retry until a valid point is produced.
🤖 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 `@tests/ln/get-payment-status.spec.ts` around lines 20 - 23, The test setup in
get-payment-status.spec.ts derives destination from an unchecked random private
key via ecc.pointFromScalar(), which can intermittently fail when makeInvoice()
runs. Update the test fixture near the priv/destination setup to use a
deterministic valid key pair or add validation and retry logic until
pointFromScalar() returns a non-null point. Keep the change localized to the
test helper setup so the invoice creation path is always fed a valid
destination.
| const won = await Order.findOneAndUpdate( | ||
| { _id: order._id, status: { $ne: 'SUCCESS' } }, | ||
| { $set: { status: 'SUCCESS', routing_fee: payment.fee } }, | ||
| ); | ||
| if (won === null) return false; | ||
| // Keep the in-memory document consistent for any later save by the caller. | ||
| order.status = 'SUCCESS'; | ||
| order.routing_fee = payment.fee; | ||
| if (pending) { | ||
| pending.paid = true; | ||
| pending.paid_at = new Date(); | ||
| } | ||
| buyerUser.trades_completed++; | ||
| await buyerUser.save(); | ||
| sellerUser.trades_completed++; | ||
| await sellerUser.save(); | ||
| if (pending) { | ||
| await messages.toAdminChannelPendingPaymentSuccessMessage( | ||
| bot, | ||
| buyerUser, | ||
| order, | ||
| pending, | ||
| payment, | ||
| i18nCtx, | ||
| ); | ||
| } | ||
| await messages.toBuyerPendingPaymentSuccessMessage( | ||
| bot, | ||
| buyerUser, | ||
| order, | ||
| payment, | ||
| i18nCtx, | ||
| ); | ||
| await messages.rateUserMessage(bot, buyerUser, order, i18nCtx); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the completion path retryable
status: 'SUCCESS' is written before the user saves and notifications. If any later await throws, retries exit on the CAS (won === null), leaving a SUCCESS order with incomplete counters or missing messages. Wrap the DB writes in a transaction or persist an outbox/completion marker so the remaining side effects can be retried.
🤖 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 `@util/completeOrder.ts` around lines 28 - 61, The completion flow in
completeOrder is not retryable because Order.findOneAndUpdate writes SUCCESS
before buyerUser.save, sellerUser.save, and the message sends, so a later
failure leaves the order partially completed. Update completeOrder to make the
DB updates and completion side effects retry-safe by using a transaction/atomic
completion marker or an outbox pattern, and ensure the status transition, trade
counters, and pending/payment notifications can be resumed if any await fails.
| buyerUser.trades_completed++; | ||
| await buyerUser.save(); | ||
| sellerUser.trades_completed++; | ||
| await sellerUser.save(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files util/completeOrder.ts util | sed -n '1,120p'
printf '\n== outline util/completeOrder.ts ==\n'
ast-grep outline util/completeOrder.ts --view expanded || true
printf '\n== relevant lines util/completeOrder.ts ==\n'
nl -ba util/completeOrder.ts | sed -n '1,220p'
printf '\n== search trades_completed usages ==\n'
rg -n "trades_completed" -S . || true
printf '\n== search transaction/session usage in util ==\n'
rg -n "startSession|withTransaction|session:" util models jobs . -g '!node_modules' || trueRepository: lnp2pBot/bot
Length of output: 549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== util/completeOrder.ts ==\n'
cat -n util/completeOrder.ts | sed -n '1,220p'
printf '\n== trades_completed references ==\n'
rg -n "trades_completed" -S util models jobs . -g '!node_modules' || true
printf '\n== transaction/session references near complete order flow ==\n'
rg -n "startSession|withTransaction|session:" util models jobs . -g '!node_modules' || true
printf '\n== related order helpers ==\n'
rg -n "completeOrderAsSuccess|completeOrder" util models jobs . -g '!node_modules' || trueRepository: lnp2pBot/bot
Length of output: 7676
Use atomic increments for trades_completed. findOneAndUpdate only guards the order transition; these save() calls are still read-modify-write and can drop increments if the same user completes multiple orders concurrently. Switch to $inc here, and wrap the order/status + counter updates in a transaction if they must stay coupled.
🤖 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 `@util/completeOrder.ts` around lines 40 - 43, The `trades_completed` updates
in `completeOrder` are still using read-modify-write via `buyerUser.save()` and
`sellerUser.save()`, which can lose increments under concurrent order
completions. Update the counter writes to use atomic `$inc` operations on the
user records instead of mutating the loaded documents, and if the order status
change must stay in sync with both counter updates, group the `findOneAndUpdate`
logic and the user counter updates in a transaction.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review
Blocking issues remain in the payment recovery path:
payToBuyer()now fails closed on status-lookup errors, but it has no retry/pending-payment fallback. A transient LND lookup failure will silently drop the payout instead of deferring it safely.- The healing branch still requires
paymentto be present whenis_confirmedis true. Thelightningtypes allow confirmed responses without apaymentpayload, and that path can still fall through to a fresh pay attempt.
This is a sensitive path; I would not merge until both are made crash-safe and retry-safe.
| }; | ||
|
|
||
| export { payRequest, payToBuyer, isPendingPayment }; | ||
| const isPendingOrConfirmed = async (request: string) => { |
There was a problem hiding this comment.
Fail-closed here is the right instinct for preventing double-pays, but payToBuyer() has no retry/pending-payment fallback. This branch is only reached from the hold-invoice settlement flow, so a transient getPaymentStatus() error now drops the payout entirely. Please persist a retryable state or route this case through the existing pending-payment machinery instead of returning silently.
… extra defensive guard on pendingPayments
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ln/pay_request.ts (1)
237-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't coerce invoice-parse failures into
is_pending.parsePaymentRequest()is inside thetry, so a malformed stored invoice falls through to{ is_pending: true }.jobs/pending_payments.tstreats that as in-flight and skips retries, so the order can stall indefinitely instead of surfacing the bad invoice. Parse first, then only wrap the LND lookup/transport call.🤖 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 `@ln/pay_request.ts` around lines 237 - 265, The getPaymentStatus flow is swallowing parsePaymentRequest failures and incorrectly returning is_pending for malformed invoices. Move parsePaymentRequest({ request }) out of the try/catch in getPaymentStatus so invoice parsing failures are not treated like transient LND lookup errors, and keep the catch only around getPayment({ lnd, id }) and related transport errors. Preserve the existing not-found handling for the LND response, but ensure bad stored invoices surface as failures instead of being coerced into pending by jobs/pending_payments.ts.
🤖 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.
Outside diff comments:
In `@ln/pay_request.ts`:
- Around line 237-265: The getPaymentStatus flow is swallowing
parsePaymentRequest failures and incorrectly returning is_pending for malformed
invoices. Move parsePaymentRequest({ request }) out of the try/catch in
getPaymentStatus so invoice parsing failures are not treated like transient LND
lookup errors, and keep the catch only around getPayment({ lnd, id }) and
related transport errors. Preserve the existing not-found handling for the LND
response, but ensure bad stored invoices surface as failures instead of being
coerced into pending by jobs/pending_payments.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0481fe4e-0a78-46c0-a40d-96dd717cdb80
📒 Files selected for processing (3)
ln/pay_request.tstests/bot/pending-payments.spec.tsutil/completeOrder.ts
💤 Files with no reviewable changes (1)
- util/completeOrder.ts
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
I re-read the PR conversation and the current head. The direction is good, but I still see two funds-safety issues that need to be fixed before merge:
Blocking
jobs/pending_payments.ts: the retry gate still requirescurrentStatus.paymentwhenis_confirmedis true. A confirmed payment without the optional payload should not be treated as retryable — that creates a re-pay window in a funds-sensitive flow.bot/scenes.ts: when the original payment is confirmed but the payment payload is missing, the code marks the orderSUCCESSand exits. In a loss-of-funds edge case, I would rather see a safe manual/operational resolution than a partial success path that can hide an inconsistent settlement state.
Why this is blocking
- These are exactly the kinds of edge cases where we should fail closed and force manual resolution if the code cannot prove the payment state safely.
- The rest of the recovery logic is moving in the right direction, but these two branches still leave room for unsafe ambiguity.
If you want, I can re-check the next iteration once these branches are tightened.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review. The architecture (double-pay prevention after restart) is sound and well-tested — I built the branch and ran the new suites (15/15 pass). I'm requesting changes for one real bug in the new code.
🔴 HIGH — bot is undefined in the PHI-wizard heal path
bot/scenes.ts:133 (in addInvoicePHIWizard):
const bot = ctx.wizard.bot; // undefined at runtimeThe bot is passed into scene state — bot/commands.ts:684:
ctx.scene.enter('ADD_INVOICE_PHI_WIZARD_SCENE_ID', { order, buyer, bot });so it lives at ctx.wizard.state.bot, exactly how the sibling addInvoiceWizard reads it (bot/scenes.ts:53: let { bot, buyer, seller, order } = ctx.wizard.state).
ctx.wizard.bot only compiles because CommunityWizard declares an aspirational bot: Telegraf<...> field (communityContext.ts:60) — but nothing in the codebase ever assigns ctx.wizard.bot (the only reference is this new line), and Telegraf does not populate it.
Failure scenario: the buyer's original invoice was already settled when they re-run /setinvoice (the restart-mid-payment case this PR targets) → originalStatus.is_confirmed is true → completeOrderAsSuccess(bot = undefined, …) runs. Inside, the order is CAS-flipped to SUCCESS and both users' trades_completed are incremented and saved, then messages.toBuyerPendingPaymentSuccessMessage(undefined, …) calls bot.telegram.sendMessage → TypeError, which the wizard's try/catch swallows (just logger.error). Net result: the order silently closes but the buyer is never told they were paid and gets no rating prompt. No fund loss / no double-pay, but it breaks the user-facing half of the exact feature being added — and the new tests don't cover this wizard path, so CI stays green.
Fix: const { bot } = ctx.wizard.state; (match addInvoiceWizard). A test that drives the is_confirmed heal branch and asserts the buyer notification fires would lock it in.
🟡 MEDIUM — fail-closed can wedge a pending payment indefinitely (by design, but unobserved)
getPaymentStatus returns is_pending: true on any unknown LND error (correct — avoids double-pay), so the job skips the retry without incrementing attempts. For a persistent non-404 fault (LND misconfig, cert/permission error), that pending payment retries every cycle forever, never expires, and never alerts an admin. Right safety tradeoff, but consider a cap/alert after N consecutive unknown-error skips so a stuck payout becomes visible. Not blocking.
🟢 What's correct / good
completeOrderAsSuccessidempotency: the atomicfindOneAndUpdate({ _id, status: { $ne: 'SUCCESS' } })CAS correctly gates the side-effects (notify, rating,trades_completed) so a job/wizard race can't double-notify or double-count. The unconditionalorder.save()in the job'sfinallyonly re-writesstatus/routing_feewith the same value, so it doesn't defeat the CAS.attempts++moved to just before the realpayRequest— in-flight/healed skips no longer burn retry budget.- Real pre-existing bug fixed: the community loop's
if (isPending) return;(which aborted the whole batch) is now a per-itemcontinue. getPaymentStatuserror parsing handles thelightninglib's array-shaped errors[code, message, details]and treats only 404 /PaymentNotFoundas "safe to pay". Well covered by the new fail-closed / not-found / happy-path tests.- Community confirmed/expired branches correctly rely on
finally { await pending.save() }to persistpaid/is_invoice_expired— verified.
Once the ctx.wizard.bot fix lands (ideally with a covering test), this is an easy approve.
…admin channel This commit adds a new ERROR state for orders. Orders in ERROR state may require manual resolution.
There was a problem hiding this comment.
Code Review Summary
Verdict: Request changes (funds-safety consistency)
I re-read the full PR conversation and verified the current head (9f7cc2b) against the prior blockers from the last two review rounds.
Previous blockers — resolved on this head
- ✅
bot/scenes.ts:133(grunch's HIGH):ctx.wizard.botis gone. The PHI wizard now readsconst { bot } = ctx.wizard.state;(same pattern as the siblingaddInvoiceWizard). No moreundefinedbot on the heal path. - ✅
jobs/pending_payments.tscurrent-payment branch (ermeme): theis_confirmedgate no longer requirescurrentStatus.payment. When confirmed but payload is missing, the code now moves the order toERRORand logs to admin instead of re-paying. Double-pay window closed on that branch. - ✅
bot/scenes.ts:181-215(ermeme): theoriginalStatus.is_confirmed && !paymentcase inaddInvoicePHIWizardnow fails closed toERROR+logOrderError+ user-facing error instead of silently markingSUCCESS.
Remaining blocker — inconsistent fail-closed policy in the job
The same funds-safety fix was applied to scenes.ts and to the current pending payment branch, but the two sibling branches earlier in jobs/pending_payments.ts still silently mark SUCCESS when confirmed-without-payload — creating a partial-success state (no routing_fee, no trades_completed++, no buyer/admin notifications, no rating prompt) and inconsistent policy across paths reachable from the same job.
jobs/pending_payments.ts:60-66—originalStatus.is_confirmed && !originalStatus.paymentonorder.buyer_invoice: still doesorder.status = 'SUCCESS'; pending.paid = true; logger.warn(...).jobs/pending_payments.ts:138-143—prevStatus.is_confirmed && !prevStatus.paymenton a previous pending payment: still doesorder.status = 'SUCCESS'; logger.warn(...).
Please align these two branches with the policy already used at jobs/pending_payments.ts:162-178 and bot/scenes.ts:196-213: mark the order as ERROR, call logOrderError() so the admin channel is notified, and leave it for manual/operational resolution. That way every "confirmed but no payload" path fails closed the same way and no code path can produce a "phantom" completed order.
Non-blocking
- No new blockers on the current head beyond the consistency fix above.
There was a problem hiding this comment.
Code Review Summary
Verdict: Request changes
I re-read the full PR conversation and verified the current head (f0c1d9a).
Previous blockers — resolved on this head
- ✅
jobs/pending_payments.ts:60-73— theoriginalStatus.is_confirmed && !paymentbranch (buyer_invoice) now fails closed: moves the order toERROR, callslogOrderError, and skips the retry. No more silent partial-SUCCESS. - ✅
jobs/pending_payments.ts:145-158— theprevStatus.is_confirmed && !paymentbranch (previous pending payment) applies the same ERROR +logOrderErrorpolicy. - ✅ Both new branches match the policy already used at
jobs/pending_payments.ts:174-192andbot/scenes.ts:196-213. Consistency achieved on the buyer flow.
Remaining blocker — same double-pay pattern still open in attemptCommunitiesPendingPayments
attemptCommunitiesPendingPayments (jobs/pending_payments.ts:366) still gates the confirmed branch on both conditions:
if (status.is_confirmed && status.payment) { ... continue; }
if (status.is_pending) { ... continue; }
// falls through to payRequest(...)If LND returns is_confirmed: true with a missing/undefined payment payload, line 366 is false, line 395 (is_pending) is false, and execution falls through to pending.attempts++ and then to payRequest(...) at line 412 — re-paying a community withdrawal that LND already confirmed. This is the exact same double-pay pattern that CodeRabbit originally flagged for both attemptPendingPayments and attemptCommunitiesPendingPayments back on 2026-06-23; the fix was applied to the buyer flow (and later tightened to ERROR) but the community flow was left in the pre-fix shape.
Please apply the same fail-closed policy here as well: gate the skip on is_confirmed alone, and when payment is missing, do not re-pay — flag the pending as terminal (for example pending.is_invoice_expired = true + pending.paid = true with last_error populated, or an equivalent "needs manual resolution" marker), notify the admin channel, and continue. Otherwise the whole point of this PR (preventing double-pays after restart) still has a hole on the community withdrawal path.
Non-blocking
- No other blockers on the current head.
Luquitasjeffrey
force-pushed
the
handle_delayed_payment
branch
from
56c967f to
c09a587
Compare
There was a problem hiding this comment.
Strict review
Verdict: Request changes
Two blocking issues remain on the current head:
util/completeOrder.tsstill marks the orderSUCCESSbefore all follow-up side effects finish. If a later await throws, retries stop at the CAS and the order can stay SUCCESS with missing accounting/notifications.ln/pay_request.tsstill treats transient/unknown LND lookup failures asis_pending, andpayToBuyer()returns early without persisting any retryable state. That can strand a hold-invoice payout afterpayHoldInvoice().
The earlier confirmed-but-no-payload double-pay path in the pending-payment healer looks fixed on this head, but these two blockers still need another pass.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Review estricto
La dirección del PR es correcta: cerrar la ventana de doble pago tras un reinicio con hold invoices liquidadas es un problema real, la semántica fail-closed de getPaymentStatus es una decisión defendible, el CAS de completeOrderAsSuccess es la primitiva adecuada, y los tests nuevos son valiosos. Sin embargo, el review confirmó 5 bugs concretos (2 operativamente serios) y una regresión de backoff — ver comentarios inline. Los hallazgos fueron verificados adversarialmente contra el código del branch, con líneas citadas.
Resumen de hallazgos
| # | Severidad | Dónde | Qué |
|---|---|---|---|
| 1 | 🔴 Alta | ln/pay_request.ts (payToBuyer) |
Un error transitorio de LND durante el release deja el pago varado: no se crea PendingPayment, el retry job nunca lo ve |
| 2 | 🔴 Alta | jobs/pending_payments.ts (ramas is_error) |
Spam al canal de admin cada 5 min por pending afectado durante una caída de LND, indefinidamente |
| 3 | 🔴 Media | bot/scenes.ts:213 |
ctx.user es undefined en el wizard → el comprador nunca recibe el error (fix de una línea: usar buyer) |
| 4 | 🔴 Media | util/completeOrder.ts |
Mutación pre-CAS + order.save() del finally del job: el perdedor de la carrera pisa el routing_fee del ganador |
| 5 | 🔴 Media | jobs/pending_payments.ts (comunidades) |
Retiro confirmado-sin-payload retirado en silencio: sin alerta a admin ni usuario, contradice su propio log |
| 6 | 🟠 Regresión | jobs/pending_payments.ts (skips in-flight) |
Se perdió el backoff exponencial: sondeo gRPC a LND cada 5 min sin tope |
| 7 | 🟡 Limpieza | jobs/pending_payments.ts |
Bloque de curación copiado 4-5 veces, ya con divergencias entre copias |
| 8 | 🟡 Limpieza | util/index.ts |
logOrderError: inglés hardcodeado fuera de bot/messages.ts/i18n, y textos de log que no corresponden a la condición real |
Verificado y descartado (para ahorrar trabajo a otros reviewers)
is_invoice_expired = truecomo kill-switch de reintentos: ningún consumidor envía "invoice expired" al usuario por ese flag; es solo un olor semántico (el archivo ya lo usaba así para AMOUNT_MISMATCH).- «Órdenes ERROR con fondos retenidos sin salida»: los casos sin ruta automática son todos
is_confirmed(fondos ya entregados, solo mal etiquetados); los casosis_pendingconservan un pending reintentable que puede sanar ERROR→SUCCESS./cancelorderde admin además no filtra por status. volume_tradedno se incrementa encompleteOrderAsSuccess: inconsistencia real pero preexistente (la rama vieja del job tampoco llamaba ahandleReputationItems); no es regresión de este PR.- Tipado de
wizard.state.bot→Telegraf<CommunityContext>: verificado correcto contra el runtime; es un fix legítimo. - Bloqueo de
/setinvoicey del retiro de earnings durante un blip de LND: real pero menor; se autocorrige al reintentar (a diferencia del hallazgo 1).
Los hallazgos 3 y 4 tienen fixes de una línea; el 1 y el 2 merecen decidirse antes del merge porque cambian el comportamiento operativo ante caídas de LND.
Generated by Claude Code
| const isPaymentPendingOrConfirmed = await isPendingOrConfirmed( | ||
| order.buyer_invoice, | ||
| ); | ||
| if (isPaymentPendingOrConfirmed) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Bug 1 (crítico): un fallo transitorio de LND durante la liberación deja el pago al comprador varado sin reintento automático.
Con la nueva semántica fail-closed, isPendingOrConfirmed también devuelve true ante cualquier error desconocido de LND (503, timeout, connection refused — ver el catch de getPaymentStatus, que retorna is_pending: true). Este return temprano se ejecuta entonces con un simple blip del nodo.
El problema: el registro PendingPayment solo se crea en la rama de fallo de payRequest, más abajo en esta misma función — el return temprano la salta. Y attemptPendingPayments solo itera sobre documentos PendingPayment; nunca escanea órdenes por status. Trazando los callers (payHoldInvoice en subscribe_invoice.ts y /paytobuyer en start.ts), ninguno crea un pending antes de llamar aquí.
Resultado: un único error transitorio de getPayment en el momento del release deja la orden en PAID_HOLD_INVOICE para siempre, sin pending, sin retry job, hasta que un admin corra /paytobuyer manualmente. Con el código anterior (fail-open) esto no ocurría.
Sugerencia: cuando getPaymentStatus devuelva is_error, no retornar silenciosamente: crear el PendingPayment (sin pagar) para que el job lo reconcilie cuando LND se recupere, o al menos notificar al canal de admin.
Generated by Claude Code
There was a problem hiding this comment.
Si bien esa sugerencia haría el flujo mas autómatico (se requiere menos intervencion manual), expone al bot a una vulnerabilidad de doble pago. La razón para dejar la orden en stand by congelando pagos pendientes de esa orden y pasandola a status "ERROR", es porque, aún en un estado de error transitorio, si LND devuelve un error no manejado entonces el codigo no puede estar 100% seguro de que el pago no se envio.
Ej:
Si se corta la conexion con LND mientras se esta consultando que un PendingPayment haya sido enviado o este al vuelo, LND va a devolver el mismo resultado tanto para pagos enviados/al vuelo, como pagos fallidos. Entonces tratar errores no catcheados LND como "retryable" deja abierta una ventana de exploit.
La premisa de este PR es que ante una ventana que pueda dejar abierta una vulnerabilidad, es preferible una resolucion manual antes que una perdida de fondos.
There was a problem hiding this comment.
De acuerdo con la premisa — reintentar a ciegas tras un error no manejado sería reabrir la ventana de doble pago, y no propongo eso. El matiz es que crear el PendingPayment no implica reintentar a ciegas: el job que lo consume es, con este mismo PR, fail-closed — antes de pagar vuelve a llamar a getPaymentStatus y solo ejecuta payRequest cuando LND responde un not-found definitivo (404/SentPaymentNotFound); ante is_error o is_pending salta sin pagar. Cuando la conexión se recupera, getPayment sí distingue enviado/al-vuelo/fallido de forma autoritativa — es exactamente la misma fuente en la que confían las ramas de curación de este PR. O sea: el escenario que describís (corte de conexión → respuesta indistinguible) bloquea el pago mientras dura el corte, que es lo correcto, pero no obliga a que el caso quede varado para siempre.
Dicho eso, si preferís sostener la resolución 100% manual para este path, lo mínimo que pediría es no retornar en silencio: este early-return deja la orden en PAID_HOLD_INVOICE sin marcar ERROR y sin logOrderError, a diferencia de todas las demás ramas is_error del PR. Nadie se entera de que hay que resolver nada. Con un logOrderError acá, el admin sabe que debe correr /paytobuyer (que funciona en este caso justamente porque la orden sigue en PAID_HOLD_INVOICE). Con eso el hilo queda resuelto para mí.
Generated by Claude Code
| if (originalStatus.is_pending) { | ||
| if (originalStatus.is_error) { | ||
| await logOrderError( | ||
| bot.telegram, | ||
| order, | ||
| 'getPaymentStatus returned error for invoice: ' + | ||
| order.buyer_invoice, | ||
| ); | ||
| order.status = 'ERROR'; | ||
| await order.save(); | ||
| } | ||
| logger.info( | ||
| `Order ${order._id}: original buyer invoice is pending (in-flight), skipping retry without incrementing attempts`, | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔴 Bug 2: spam ilimitado al canal de admin durante una caída de LND (12 mensajes/hora por cada pending afectado).
Esta rama is_error (y sus gemelas: confirmado-sin-payload en las líneas ~60 y ~186, y currentStatus.is_error en ~215) hace order.status = 'ERROR' + logOrderError (mensaje a ADMIN_CHANNEL) y luego continue — sin tocar pending.paid, is_invoice_expired, attempts ni next_retry.
La query de selección del job (paid: false, attempts < MAX, is_invoice_expired: false, next_retry <= now) no excluye órdenes en ERROR, así que el mismo pending se reselecciona en cada corrida (~5 min): durante una caída de LND, cada pago pendiente genera un mensaje ERROR en cada ciclo (el finally además re-guarda y re-emite orderUpdated).
Sugerencia: en las ramas de error, avanzar next_retry con el backoff existente (o marcar el pending con un flag terminal tras notificar una vez). Con eso el admin recibe una alerta, no mil.
Generated by Claude Code
| if (status.is_confirmed) { | ||
| if (!status.payment) { | ||
| logger.error( | ||
| `Community ${pending.community_id} pending payment confirmed but LND returned no details; ` + | ||
| `not marking SUCCESS — needs manual resolution`, | ||
| ); | ||
| pending.paid = true; | ||
| pending.is_invoice_expired = true; | ||
| pending.last_error = 'CONFIRMED_NO_PAYLOAD'; | ||
| await pending.save(); | ||
| // Notify admin (using Telegram bot's admin capability if possible, | ||
| // or just generic bot.telegram.sendMessage to an admin ID if available. | ||
| // For now, mirroring the logic of failing and continuing.) | ||
| continue; |
There was a problem hiding this comment.
🔴 Bug 5: el retiro comunitario "confirmado sin payload" se entierra en silencio — nadie se entera nunca.
El log dice not marking SUCCESS — needs manual resolution, pero el código hace exactamente lo contrario a permitir esa resolución: pending.paid = true + is_invoice_expired = true lo retiran permanentemente de la cola (la query filtra paid: false), y no se envía ningún mensaje ni al admin ni al usuario. La "notificación al admin" es solo un comentario TODO que no describe código existente — a diferencia de la rama análoga del flujo de órdenes (línea ~186), que sí llama a logOrderError.
Resultado: un retiro de earnings que según el propio código "necesita resolución manual" desaparece sin que ningún humano sea alertado; la única traza es un logger.error.
Sugerencia: enviar la alerta al ADMIN_CHANNEL antes del continue (como hace la rama de órdenes) y eliminar el comentario placeholder. También reconsiderar paid = true en esta rama: contradice la intención fail-closed declarada.
Generated by Claude Code
| } | ||
| logger.info( | ||
| `Order ${order._id}: current payment is already in-flight, skipping retry without incrementing attempts`, | ||
| ); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 Regresión: se perdió el backoff exponencial para pagos en vuelo — sondeo a LND cada 5 minutos, para siempre.
El código anterior incrementaba attempts y avanzaba next_retry al tope del loop, incluso al saltar una iteración: un HTLC atascado in-flight se consultaba con backoff creciente (hasta 1 h) y tras PAYMENT_ATTEMPTS corridas salía de la query. Ahora las ramas de skip (esta, y la del invoice original en ~88) no tocan nada, así que un pago perpetuamente in-flight genera un gRPC getPayment en cada corrida de 5 minutos, sin backoff ni tope.
No reintentar mientras está en vuelo es correcto (evita el doble pago); la regresión es solo el sondeo sin backoff. Sugerencia: en las ramas de skip in-flight, avanzar next_retry (sin incrementar attempts, para no consumir reintentos reales).
Generated by Claude Code
| if (order.buyer_invoice) { | ||
| const originalStatus = await getPaymentStatus(order.buyer_invoice); | ||
| if (originalStatus.is_confirmed) { | ||
| logger.info( | ||
| `Order ${order._id}: original buyer invoice already confirmed, running success routine`, | ||
| ); | ||
| const buyerUser = await User.findOne({ _id: order.buyer_id }); |
There was a problem hiding this comment.
🟡 Mantenibilidad: el bloque de "curación" está copiado 4-5 veces con manejo divergente en cada copia.
El patrón «buscar buyer/seller + i18n → si hay payload completeOrderAsSuccess, si no ERROR + logOrderError» aparece aquí, en ~121 (previous pending), ~174 (current pending), en la variante comunitaria (~366) y en bot/scenes.ts (~181). Las copias ya divergieron: la comunitaria olvidó la notificación al admin y la del wizard usa ctx.user en vez de buyer (ver comentarios respectivos).
Sugerencia: extraer un único helper (p. ej. healConfirmedOrder(bot, order, status, pending?)) que encapsule el contrato fail-closed completo. Así el próximo call site no puede reabrir la ventana de doble pago que este PR cierra.
Generated by Claude Code
| const logOrderError = async ( | ||
| telegram: Telegram, | ||
| order: IOrder, | ||
| details: string, | ||
| ): Promise<void> => { | ||
| logger.error( | ||
| `Order ${order._id}: payment skipped due to unknown errors; payout may be stuck`, | ||
| ); | ||
| try { | ||
| await telegram.sendMessage( | ||
| String(process.env.ADMIN_CHANNEL), | ||
| `⚠️ Order ${order._id} transitioned to ERROR state \n` + | ||
| `The payout may be stuck — please check the order. \n` + | ||
| `The order may require manual resolution. \n` + | ||
| `Details: ${details}`, | ||
| ); | ||
| } catch (error) { | ||
| logger.error(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 Convenciones + textos engañosos en logOrderError.
Tres problemas en esta función:
- Ubicación/patrón: es un notificador de
ADMIN_CHANNELhardcodeado en inglés dentro deutil/. Todos sus hermanos (toAdminChannelPendingPaymentFailedMessage, etc.) viven enbot/messages.tsy usan plantillas i18n delocales/. Sugerencia: moverla comotoAdminChannelOrderErrorMessagejunto al resto. - El
logger.errorfijo miente según el caller: dicepayment skipped due to unknown errorsaunque varias ramas que la llaman corresponden a pagos confirmados (confirmado-sin-payload), no a errores desconocidos. detailscopiado mal en varios call sites:bot/scenes.ts:210yjobs/pending_payments.ts:~187pasangetPaymentStatus returned error for pending paymenten ramas cuya condición real es «confirmado pero sin payload». Quien haga triage de una ordenERRORleerá una causa falsa.
Generated by Claude Code
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Re-review del commit 7b3563e ("Handle points 2 to 5"): los Bugs 3 (ctx.user → buyer) y 4 (mutación movida después del CAS) quedan correctamente resueltos — resuelvo esos hilos. El Bug 2 queda acotado por el nuevo guard de status, con un follow-up operativo comentado inline (órdenes ERROR quedan sin vía de resolución ni automática ni por /paytobuyer). Pendientes: Bug 5 (rama comunitaria sin notificación), la regresión de backoff para pagos in-flight, y el Lint de CI (nit inline).
Generated by Claude Code
| } else if (order.status === 'ERROR') { | ||
| logger.info( | ||
| `attemptPendingPayments: order ${order._id} is in ERROR status, skipping`, | ||
| ); | ||
| //Increment attempts so it is expired | ||
| pending.attempts++; | ||
| await pending.save(); | ||
| continue; |
There was a problem hiding this comment.
Este guard acota bien el spam del Bug 2 (un solo aviso al admin, luego el pending consume attempts y sale de la query) 👍 — pero introduce un dead-end operativo que antes no existía:
-
Una orden
ERRORya no puede resolverse ni automática ni manualmente vía el bot. Antes de este commit, si elis_errorfue un blip transitorio de LND, una corrida posterior del job re-consultabagetPaymentStatusy podía curar la orden (completeOrderAsSuccessusa$ne: 'SUCCESS', así que ERROR→SUCCESS era posible) o pagar ante un not-found definitivo. Con este skip, eso ya no ocurre nunca. Y la vía manual tampoco existe:/paytobuyersolo aceptaFROZENoPAID_HOLD_INVOICE(start.ts), así que rechaza órdenesERROR— la "resolución manual" que el mensaje del admin promete hoy requiere editar la DB a mano. Sugerencia mínima: permitirERRORen el gate de/paytobuyer, así el flujo manual queda operable de punta a punta. -
Nit: el Lint de CI está fallando exactamente acá — línea 36,
//Incrementnecesita espacio tras//(spaced-comment);eslint --fixlo resuelve.
Generated by Claude Code
4 participants
Summary by CodeRabbit