routing: make sure attempts are always resolved after a timeout
What the developer wrote
Authored by ziggie
routing: make sure attempts are always resolved after a timeout
We check the context of the payment lifecycle at the beginning of
the `resumepayment` loop. This will make sure we have always the
latest state of the payment before deciding on the next steps in
the function `decideNextStep`.
What changed, and why it matters
This change moves a timeout/context check to the very start of each loop iteration in LND's payment lifecycle logic. The goal is to ensure that when a payment attempt times out or the context is cancelled, any remaining in-flight HTLC attempts are resolved and the payment state is updated before the loop exits. The commit message frames this as a correctness fix for resolving attempts after a timeout, but does not describe it as a security vulnerability.
Treat as a reliability/correctness fix rather than an urgent security patch. Reviewers should verify that `checkContext` correctly marks payments as failed and that `decideNextStep` handles the post-timeout state so no HTLC attempts are left in an unresolved state. Consider whether this change could cause payments to be marked failed prematurely if `checkContext` is called before state reload in edge cases.
Security signals we found
Payment lifecycle state machine change
Timeout/context cancellation handling moved earlier in loop
Potential for in-flight HTLC attempts to be left unresolved before fix
No explicit security framing by vendor
Evidence from the diff
In routing/payment_lifecycle.go, the checkContext(ctx) call is moved from after reloadPayment() to before it inside the resumePayment loop. The accompanying test is simplified to expect an immediate exit when the router quit channel is closed, without reloading the payment. The change means that on each iteration, the lifecycle first checks whether the payment attempt timeout has expired or the context was cancelled; if so, it marks the payment as failed and reloads the latest state. This addresses a case where attempts could remain unresolved if the timeout/context check happened only after state reload and route selection.
Changed components
routing/payment_lifecycle.gorouting/payment_lifecycle_test.goInspect captured patch +21 / −25
diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go
index 7a6443a..a42b21d 100644
--- a/routing/payment_lifecycle.go
+++ b/routing/payment_lifecycle.go
@@ -226,6 +226,19 @@ func (p *paymentLifecycle) resumePayment(ctx context.Context) ([32]byte,
// critical error during path finding.
lifecycle:
for {
+ // Before we attempt any new shard, we'll check to see if we've
+ // gone past the payment attempt timeout or if the context was
+ // canceled. If the context is done, the payment is marked as
+ // failed and we reload the latest payment state to reflect
+ // this.
+ //
+ // NOTE: This can be called several times if there are more
+ // attempts to be resolved after the timeout or context is
+ // cancelled.
+ if err := p.checkContext(ctx); err != nil {
+ return exitWithErr(err)
+ }
+
// We update the payment state on every iteration.
currentPayment, ps, err := p.reloadPayment()
if err != nil {
@@ -241,19 +254,11 @@ lifecycle:
// We now proceed our lifecycle with the following tasks in
// order,
- // 1. check context.
- // 2. request route.
- // 3. create HTLC attempt.
- // 4. send HTLC attempt.
- // 5. collect HTLC attempt result.
+ // 1. request route.
+ // 2. create HTLC attempt.
+ // 3. send HTLC attempt.
+ // 4. collect HTLC attempt result.
//
- // Before we attempt any new shard, we'll check to see if we've
- // gone past the payment attempt timeout, or if the context was
- // cancelled, or the router is exiting. In any of these cases,
- // we'll stop this payment attempt short.
- if err := p.checkContext(ctx); err != nil {
- return exitWithErr(err)
- }
// Now decide the next step of the current lifecycle.
step, err := p.decideNextStep(payment)
diff --git a/routing/payment_lifecycle_test.go b/routing/payment_lifecycle_test.go
index 0ee7511..a4f4550 100644
--- a/routing/payment_lifecycle_test.go
+++ b/routing/payment_lifecycle_test.go
@@ -868,25 +868,16 @@ func TestResumePaymentFailOnTimeoutErr(t *testing.T) {
// Create a test paymentLifecycle with the initial two calls mocked.
p, m := setupTestPaymentLifecycle(t)
- paymentAmt := lnwire.MilliSatoshi(10000)
-
- // We now enter the payment lifecycle loop.
- //
- // 1. calls `FetchPayment` and return the payment.
- m.control.On("FetchPayment", p.identifier).Return(m.payment, nil).Once()
-
- // 2. calls `GetState` and return the state.
- ps := &channeldb.MPPaymentState{
- RemainingAmt: paymentAmt,
- }
- m.payment.On("GetState").Return(ps).Once()
+ // We now enter the payment lifecycle loop, we will check the router
+ // quit channel in the beginning and quit immediately without reloading
+ // the payment.
// NOTE: GetStatus is only used to populate the logs which is
// not critical so we loosen the checks on how many times it's
// been called.
m.payment.On("GetStatus").Return(channeldb.StatusInFlight)
- // 3. quit the router to return an error.
+ // Quit the router to return an error.
close(p.router.quit)
// Send the payment and assert it failed when router is shutting down.
Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.