routing: mark payment as failed when no route can be found

routing/payment_lifecycle.go

@@ -266,9 +266,14 @@ lifecycle:
 			return exitWithErr(err)
 		}
 
-		// NOTE: might cause an infinite loop, see notes in
-		// `requestRoute` for details.
+		// We may not be able to find a route for current attempt. In
+		// that case, we continue the loop and move straight to the
+		// next iteration in case there are results for inflight HTLCs
+		// that still need to be collected.
 		if rt == nil {
+			log.Errorf("No route found for payment %v",
+				p.identifier)
+
 			continue lifecycle
 		}
 
@@ -363,42 +368,30 @@ func (p *paymentLifecycle) requestRoute(
 	log.Warnf("Failed to find route for payment %v: %v", p.identifier, err)
 
 	// If the error belongs to `noRouteError` set, it means a non-critical
-	// error has happened during path finding and we might be able to find
-	// another route during next HTLC attempt. Otherwise, we'll return the
-	// critical error found.
+	// error has happened during path finding and we will mark the payment
+	// failed with this reason. Otherwise, we'll return the critical error
+	// found to abort the lifecycle.
 	var routeErr noRouteError
 	if !errors.As(err, &routeErr) {
 		return nil, fmt.Errorf("requestRoute got: %w", err)
 	}
 
-	// There is no route to try, and we have no active shards. This means
-	// that there is no way for us to send the payment, so mark it failed
-	// with no route.
-	//
-	// NOTE: if we have zero `numShardsInFlight`, it means all the HTLC
-	// attempts have failed. Otherwise, if there are still inflight
-	// attempts, we might enter an infinite loop in our lifecycle if
-	// there's still remaining amount since we will keep adding new HTLC
-	// attempts and they all fail with `noRouteError`.
-	//
-	// TODO(yy): further check the error returned here. It's the
-	// `paymentSession`'s responsibility to find a route for us with best
-	// effort. When it cannot find a path, we need to treat it as a
-	// terminal condition and fail the payment no matter it has inflight
+	// It's the `paymentSession`'s responsibility to find a route for us
+	// with best effort. When it cannot find a path, we need to treat it as
+	// a terminal condition and fail the payment no matter it has inflight
 	// HTLCs or not.
-	if ps.NumAttemptsInFlight == 0 {
-		failureCode := routeErr.FailureReason()
-		log.Debugf("Marking payment %v permanently failed with no "+
-			"route: %v", p.identifier, failureCode)
+	failureCode := routeErr.FailureReason()
+	log.Warnf("Marking payment %v permanently failed with no route: %v",
+		p.identifier, failureCode)
 
-		err := p.router.cfg.Control.FailPayment(
-			p.identifier, failureCode,
-		)
-		if err != nil {
-			return nil, fmt.Errorf("FailPayment got: %w", err)
-		}
+	err = p.router.cfg.Control.FailPayment(p.identifier, failureCode)
+	if err != nil {
+		return nil, fmt.Errorf("FailPayment got: %w", err)
 	}
 
+	// NOTE: we decide to not return the non-critical noRouteError here to
+	// avoid terminating the payment lifecycle as there might be other
+	// inflight HTLCs which we must wait for their results.
 	return nil, nil
 }
 

routing/payment_lifecycle_test.go

@@ -419,13 +419,8 @@ func TestRequestRouteHandleCriticalErr(t *testing.T) {
 func TestRequestRouteHandleNoRouteErr(t *testing.T) {
 	t.Parallel()
 
-	p := createTestPaymentLifecycle()
-
-	// Create a mock payment session.
-	paySession := &mockPaymentSession{}
-
-	// Mount the mocked payment session.
-	p.paySession = paySession
+	// Create a paymentLifecycle with mockers.
+	p, m := newTestPaymentLifecycle(t)
 
 	// Create a dummy payment state.
 	ps := &channeldb.MPPaymentState{
@@ -437,68 +432,22 @@ func TestRequestRouteHandleNoRouteErr(t *testing.T) {
 	// Mock remainingFees to be 1.
 	p.feeLimit = ps.FeesPaid + 1
 
-	// Mock the paySession's `RequestRoute` method to return an error.
-	paySession.On("RequestRoute",
+	// Mock the paySession's `RequestRoute` method to return a NoRouteErr
+	// type.
+	m.paySession.On("RequestRoute",
 		mock.Anything, mock.Anything, mock.Anything, mock.Anything,
 	).Return(nil, errNoTlvPayload)
 
+	// The payment should be failed with reason no route.
+	m.control.On("FailPayment",
+		p.identifier, channeldb.FailureReasonNoRoute,
+	).Return(nil).Once()
+
 	result, err := p.requestRoute(ps)
 
 	// Expect no error is returned since it's not critical.
 	require.NoError(t, err, "expected no error")
 	require.Nil(t, result, "expected no route returned")
-
-	// Assert that `RequestRoute` is called as expected.
-	paySession.AssertExpectations(t)
-}
-
-// TestRequestRouteFailPaymentSucceed checks that `requestRoute` fails the
-// payment when received an `noRouteError` returned from payment session while
-// it has no inflight attempts.
-func TestRequestRouteFailPaymentSucceed(t *testing.T) {
-	t.Parallel()
-
-	p := createTestPaymentLifecycle()
-
-	// Create a mock payment session.
-	paySession := &mockPaymentSession{}
-
-	// Mock the control tower's `FailPayment` method.
-	ct := &mockControlTower{}
-	ct.On("FailPayment",
-		p.identifier, errNoTlvPayload.FailureReason(),
-	).Return(nil)
-
-	// Mount the mocked control tower and payment session.
-	p.router.cfg.Control = ct
-	p.paySession = paySession
-
-	// Create a dummy payment state with zero inflight attempts.
-	ps := &channeldb.MPPaymentState{
-		NumAttemptsInFlight: 0,
-		RemainingAmt:        1,
-		FeesPaid:            100,
-	}
-
-	// Mock remainingFees to be 1.
-	p.feeLimit = ps.FeesPaid + 1
-
-	// Mock the paySession's `RequestRoute` method to return an error.
-	paySession.On("RequestRoute",
-		mock.Anything, mock.Anything, mock.Anything, mock.Anything,
-	).Return(nil, errNoTlvPayload)
-
-	result, err := p.requestRoute(ps)
-
-	// Expect no error is returned since it's not critical.
-	require.NoError(t, err, "expected no error")
-	require.Nil(t, result, "expected no route returned")
-
-	// Assert that `RequestRoute` is called as expected.
-	paySession.AssertExpectations(t)
-
-	// Assert that `FailPayment` is called as expected.
-	ct.AssertExpectations(t)
 }
 
 // TestRequestRouteFailPaymentError checks that `requestRoute` returns the