instantout: harden instant out error handling

docs/release-notes/release-notes-next.md

@@ -13,6 +13,10 @@
 
 #### Bug Fixes
 
+* Newly initiated Instant Out swaps are now indexed by their actual swap hash,
+  and normal payment-stream closure no longer aborts a swap before the server
+  reports that it accepted the payment.
+
 * Instant Out now attempts to cancel server-side swaps when client
   initialization fails, allowing locked reservations to be released without
   waiting for the server timeout.

instantout/actions.go

@@ -299,22 +299,42 @@ func (f *FSM) PollPaymentAcceptedAction(ctx context.Context,
 
 	// We want to poll quickly the first time.
 	timer := time.NewTimer(time.Second)
+	defer timer.Stop()
+
 	for {
 		select {
-		case payRes := <-payChan:
+		case payRes, ok := <-payChan:
+			if !ok {
+				// The router closes both payment channels after the
+				// payment stream reaches a terminal state. Stop selecting
+				// on this channel and let the server poll determine whether
+				// it accepted the payment.
+				payChan = nil
+				continue
+			}
+
 			f.Debugf("payment result: %v", payRes)
 			if payRes.State == lnrpc.Payment_FAILED {
 				return f.handleErrorAndUnlockReservations(
 					ctx, fmt.Errorf("payment failed: %v",
 						payRes.FailureReason),
 				)
 			}
-		case err := <-paymentErrChan:
+		case err, ok := <-paymentErrChan:
+			if !ok {
+				// Channel closure is the normal end-of-stream signal.
+				paymentErrChan = nil
+				continue
+			}
+			if err == nil {
+				err = errors.New("payment error channel returned nil")
+			}
+
 			f.Errorf("error sending payment: %v", err)
 			return f.handleErrorAndUnlockReservations(ctx, err)
 
 		case <-ctx.Done():
-			return f.handleErrorAndUnlockReservations(ctx, nil)
+			return f.handleErrorAndUnlockReservations(ctx, ctx.Err())
 
 		case <-timer.C:
 			res, err := f.cfg.InstantOutClient.PollPaymentAccepted(

instantout/manager.go

@@ -166,14 +166,12 @@ func (m *Manager) NewInstantOut(ctx context.Context,
 		sweepAddress:    sweepAddr,
 		maxSwapFee:      maxSwapFee,
 	}
+	m.Unlock()
 
 	instantOut, err := NewFSM(m.cfg, ProtocolVersionFullReservation)
 	if err != nil {
-		m.Unlock()
 		return nil, err
 	}
-	m.activeInstantOuts[instantOut.InstantOut.SwapHash] = instantOut
-	m.Unlock()
 
 	// Start the instantout FSM.
 	go func() {
@@ -193,6 +191,10 @@ func (m *Manager) NewInstantOut(ctx context.Context,
 		return nil, err
 	}
 
+	m.Lock()
+	m.activeInstantOuts[instantOut.InstantOut.SwapHash] = instantOut
+	m.Unlock()
+
 	return instantOut, nil
 }
 

instantout/payment_test.go

@@ -0,0 +1,148 @@
+package instantout
+
+import (
+	"context"
+	"testing"
+
+	"github.com/lightninglabs/lndclient"
+	"github.com/lightninglabs/loop/fsm"
+	"github.com/lightninglabs/loop/instantout/reservation"
+	"github.com/lightninglabs/loop/swapserverrpc"
+	"github.com/lightningnetwork/lnd/lntypes"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
+)
+
+type paymentTestRouter struct {
+	lndclient.RouterClient
+
+	statusChan chan lndclient.PaymentStatus
+	errorChan  chan error
+}
+
+func (r *paymentTestRouter) SendPayment(context.Context,
+	lndclient.SendPaymentRequest) (chan lndclient.PaymentStatus, chan error,
+	error) {
+
+	return r.statusChan, r.errorChan, nil
+}
+
+type paymentTestReservationManager struct {
+	ReservationManager
+}
+
+func (m *paymentTestReservationManager) LockReservation(context.Context,
+	reservation.ID) error {
+
+	return nil
+}
+
+func (m *paymentTestReservationManager) UnlockReservation(context.Context,
+	reservation.ID) error {
+
+	return nil
+}
+
+type paymentTestInstantOutClient struct {
+	swapserverrpc.InstantSwapServerClient
+
+	accepted bool
+	polls    int
+}
+
+func (c *paymentTestInstantOutClient) PollPaymentAccepted(context.Context,
+	*swapserverrpc.PollPaymentAcceptedRequest, ...grpc.CallOption) (
+	*swapserverrpc.PollPaymentAcceptedResponse, error) {
+
+	c.polls++
+	return &swapserverrpc.PollPaymentAcceptedResponse{
+		Accepted: c.accepted,
+	}, nil
+}
+
+func (c *paymentTestInstantOutClient) CancelInstantSwap(context.Context,
+	*swapserverrpc.CancelInstantSwapRequest, ...grpc.CallOption) (
+	*swapserverrpc.CancelInstantSwapResponse, error) {
+
+	return &swapserverrpc.CancelInstantSwapResponse{}, nil
+}
+
+func newPaymentTestFSM(router lndclient.RouterClient,
+	client swapserverrpc.InstantSwapServerClient) *FSM {
+
+	return &FSM{
+		StateMachine: &fsm.StateMachine{},
+		cfg: &Config{
+			RouterClient:       router,
+			InstantOutClient:   client,
+			ReservationManager: &paymentTestReservationManager{},
+		},
+		InstantOut: &InstantOut{
+			SwapHash: lntypes.Hash{1},
+			Reservations: []*reservation.Reservation{
+				{ID: reservation.ID{1}},
+			},
+		},
+	}
+}
+
+// TestPollPaymentAcceptedIgnoresClosedStreams verifies that the normal router
+// end-of-stream signal doesn't race with the server's acceptance response.
+func TestPollPaymentAcceptedIgnoresClosedStreams(t *testing.T) {
+	statusChan := make(chan lndclient.PaymentStatus)
+	errorChan := make(chan error)
+	close(statusChan)
+	close(errorChan)
+
+	router := &paymentTestRouter{
+		statusChan: statusChan,
+		errorChan:  errorChan,
+	}
+	client := &paymentTestInstantOutClient{accepted: true}
+	instantOutFSM := newPaymentTestFSM(router, client)
+
+	event := instantOutFSM.PollPaymentAcceptedAction(t.Context(), nil)
+
+	require.Equal(t, OnPaymentAccepted, event)
+	require.Equal(t, 1, client.polls)
+}
+
+// TestPollPaymentAcceptedRejectsNilError verifies that an invalid nil error
+// value can't make the FSM report a successful failure path.
+func TestPollPaymentAcceptedRejectsNilError(t *testing.T) {
+	errorChan := make(chan error, 1)
+	errorChan <- nil
+
+	router := &paymentTestRouter{
+		statusChan: make(chan lndclient.PaymentStatus),
+		errorChan:  errorChan,
+	}
+	instantOutFSM := newPaymentTestFSM(
+		router, &paymentTestInstantOutClient{},
+	)
+
+	event := instantOutFSM.PollPaymentAcceptedAction(t.Context(), nil)
+
+	require.Equal(t, fsm.OnError, event)
+	require.ErrorContains(
+		t, instantOutFSM.LastActionError,
+		"payment error channel returned nil",
+	)
+}
+
+// TestPollPaymentAcceptedPreservesContextError verifies that cancellation is
+// recorded as the action error rather than becoming a nil FSM error.
+func TestPollPaymentAcceptedPreservesContextError(t *testing.T) {
+	ctx, cancel := context.WithCancel(t.Context())
+	cancel()
+
+	router := &paymentTestRouter{}
+	instantOutFSM := newPaymentTestFSM(
+		router, &paymentTestInstantOutClient{},
+	)
+
+	event := instantOutFSM.PollPaymentAcceptedAction(ctx, nil)
+
+	require.Equal(t, fsm.OnError, event)
+	require.ErrorIs(t, instantOutFSM.LastActionError, context.Canceled)
+}