misc(payment): idempotent retry

app/jobs/invoices/payments/create_job.rb

@@ -10,8 +10,8 @@ class CreateJob < ApplicationJob
       retry_on Invoices::Payments::ConnectionError, wait: :polynomially_longer, attempts: 6
       retry_on Invoices::Payments::RateLimitError, wait: :polynomially_longer, attempts: 6
 
-      def perform(invoice:, payment_provider:, payment: nil)
-        Invoices::Payments::CreateService.call!(invoice:, payment_provider:, payment:)
+      def perform(invoice:, payment_provider:)
+        Invoices::Payments::CreateService.call!(invoice:, payment_provider:)
       end
 
       def lock_key_arguments

app/models/payment.rb

@@ -3,6 +3,8 @@
 class Payment < ApplicationRecord
   include PaperTrailTraceable
 
+  PAYABLE_PAYMENT_STATUS = %w[pending processing succeeded failed].freeze
+
   belongs_to :payable, polymorphic: true
   belongs_to :payment_provider, optional: true, class_name: 'PaymentProviders::BaseProvider'
   belongs_to :payment_provider_customer, class_name: 'PaymentProviderCustomers::BaseCustomer'
@@ -12,6 +14,8 @@ class Payment < ApplicationRecord
 
   delegate :customer, to: :payable
 
+  enum payable_payment_status: PAYABLE_PAYMENT_STATUS.map { |s| [s, s] }.to_h
+
   def should_sync_payment?
     return false unless payable.is_a?(Invoice)
 
@@ -26,6 +30,7 @@ def should_sync_payment?
 #  id                           :uuid             not null, primary key
 #  amount_cents                 :bigint           not null
 #  amount_currency              :string           not null
+#  payable_payment_status       :enum
 #  payable_type                 :string           default("Invoice"), not null
 #  provider_payment_data        :jsonb
 #  status                       :string           not null
@@ -40,6 +45,7 @@ def should_sync_payment?
 # Indexes
 #
 #  index_payments_on_invoice_id                    (invoice_id)
+#  index_payments_on_payable_id_and_payable_type   (payable_id,payable_type) UNIQUE WHERE (payable_payment_status = ANY (ARRAY['pending'::payment_payable_payment_status, 'processing'::payment_payable_payment_status]))
 #  index_payments_on_payable_type_and_payable_id   (payable_type,payable_id)
 #  index_payments_on_payment_provider_customer_id  (payment_provider_customer_id)
 #  index_payments_on_payment_provider_id           (payment_provider_id)

app/services/invoices/payments/create_service.rb

@@ -5,9 +5,8 @@ module Payments
     class CreateService < BaseService
       include Customers::PaymentProviderFinder
 
-      def initialize(invoice:, payment_provider: nil, payment: nil)
+      def initialize(invoice:, payment_provider: nil)
         @invoice = invoice
-        @payment = payment
         @provider = payment_provider&.to_sym
 
         super
@@ -22,40 +21,53 @@ def call
           return result
         end
 
+        if processing_payment
+          # Payment is being processed, return the existing payment
+          # Status will be updated via webhooks
+          result.payment = processing_payment
+          return result
+        end
+
         invoice.update!(payment_attempts: invoice.payment_attempts + 1)
 
-        @payment ||= Payment.create!(
-          payable: invoice,
+        payment ||= Payment.create_with(
           payment_provider_id: current_payment_provider.id,
           payment_provider_customer_id: current_payment_provider_customer.id,
           amount_cents: invoice.total_amount_cents,
-          amount_currency: invoice.currency,
+          amount_currency: invoice.currency
+        ).find_or_create_by!(
+          payable: invoice,
+          payable_payment_status: "pending",
           status: "pending"
         )
+
         result.payment = payment
 
         payment_result = ::PaymentProviders::CreatePaymentFactory.new_instance(provider:, payment:).call!
 
-        deliver_error_webhook(payment_result) if payment_result.error_message.present?
+        # Keep payment in a pending state. Used manly for `amount_too_small` in stripe service
+        return result if payment_result.payment.payable_payment_status.nil?
 
-        if payment_result.payment_status.present?
-          update_invoice_payment_status(
-            payment_status: payment_result.payment_status,
-            processing: payment_result.payment.status == "processing"
-          )
+        payment_status = payment_result.payment.payable_payment_status
+        update_invoice_payment_status(
+          payment_status: (payment_status == "processing") ? :pending : payment_status,
+          processing: payment_status == "processing"
+        )
 
-          if ["pending", "success"].include?(payment_result.payment_status)
-            # TODO: better handling of payment status, with a `processing` status on the payment
-            Integrations::Aggregator::Payments::CreateJob.perform_later(payment:) if result.payment.should_sync_payment?
-          end
-        end
+        Integrations::Aggregator::Payments::CreateJob.perform_later(payment:) if result.payment.should_sync_payment?
 
         result
       rescue BaseService::ServiceFailure => e
         deliver_error_webhook(e.result)
-        update_invoice_payment_status(payment_status: e.result.payment_status) if e.result.payment_status.present?
 
-        raise
+        if e.result.payment.payable_payment_status.present?
+          update_invoice_payment_status(payment_status: e.result.payment.payable_payment_status)
+        end
+
+        # Some errors should be investigated and need to be raised
+        raise if e.result.reraise
+
+        result
       end
 
       def call_async
@@ -114,6 +126,17 @@ def deliver_error_webhook(payment_result)
           }
         })
       end
+
+      def processing_payment
+        @processing_payment ||= Payment.find_by(
+          payable_id: invoice,
+          payment_provider_id: current_payment_provider.id,
+          payment_provider_customer_id: current_payment_provider_customer.id,
+          amount_cents: invoice.total_amount_cents,
+          amount_currency: invoice.currency,
+          payable_payment_status: "processing"
+        )
+      end
     end
   end
 end

app/services/invoices/payments/stripe_service.rb

@@ -80,13 +80,15 @@ def create_payment(stripe_payment, invoice: nil)
 
         increment_payment_attempts
 
-        Payment.new(
+        Payment.find_or_create_by!(
           payable: @invoice,
           payment_provider_id: stripe_payment_provider.id,
           payment_provider_customer_id: customer.stripe_customer.id,
           amount_cents: @invoice.total_amount_cents,
           amount_currency: @invoice.currency,
-          provider_payment_id: stripe_payment.id
+          provider_payment_id: stripe_payment.id,
+          payable_payment_status: "pending",
+          status: stripe_payment.status
         )
       end
 

app/services/payment_providers/adyen/payments/create_service.rb

@@ -22,29 +22,24 @@ def call
           adyen_result = create_adyen_payment
 
           if adyen_result.status > 400
-            result.error_message = adyen_result.response["message"]
-            result.error_code = adyen_result.response["errorType"]
-            return result
+            return prepare_failed_result(::Adyen::AdyenError.new(
+              nil, nil, adyen_result.response["message"], adyen_result.response["errorType"]
+            ))
           end
 
-          payment.provider_payment_data = adyen_result.response["pspReference"]
+          payment.provider_payment_id = adyen_result.response["pspReference"]
           payment.status = adyen_result.response["resultCode"]
+          payment.payable_payment_status = payment_status_mapping(payment.status)
           payment.save!
 
-          result.payment_status = payment_status_mapping(payment.status)
           result.payment = payment
           result
         rescue ::Adyen::AuthenticationError, ::Adyen::ValidationError => e
-          result.error_message = e.msg
-          result.error_code = e.code
-          result.payment_status = :failed
-          result
+          prepare_failed_result(e)
         rescue ::Adyen::AdyenError => e
-          result.error_message = e.msg
-          result.error_code = e.code
-          result.payment_status = :failed
-          result.service_failure!(code: "adyen_error", message: "#{e.code}: #{e.msg}")
+          prepare_failed_result(e, reraise: true)
         rescue Faraday::ConnectionFailed => e
+          # Allow auto-retry with idempotency key
           raise Invoices::Payments::ConnectionError, e
         end
 
@@ -94,8 +89,8 @@ def payment_method_params
         def payment_params
           prms = {
             amount: {
-              currency: invoice.currency.upcase,
-              value: invoice.total_amount_cents
+              currency: payment.amount_currency.upcase,
+              value: payment.amount_cents
             },
             reference: invoice.number,
             paymentMethod: {
@@ -118,6 +113,16 @@ def payment_status_mapping(payment_status)
 
           payment_status
         end
+
+        def prepare_failed_result(error, reraise: false)
+          result.error_message = error.msg
+          result.error_code = error.code
+          result.reraise = reraise
+
+          payment.update!(status: :failed, payable_payment_status: :failed)
+
+          result.service_failure!(code: "adyen_error", message: "#{error.code}: #{error.msg}")
+        end
       end
     end
   end

app/services/payment_providers/gocardless/payments/create_service.rb

@@ -37,21 +37,15 @@ def call
 
           payment.provider_payment_id = gocardless_result.id
           payment.status = gocardless_result.status
+          payment.payable_payment_status = payment_status_mapping(payment.status)
           payment.save!
 
-          result.payment_status = payment_status_mapping(payment.status)
           result.payment = payment
           result
         rescue GoCardlessPro::ValidationError => e
-          result.error_message = e.message
-          result.error_code = e.code
-          result.payment_status = :failed
-          result
+          prepare_failed_result(e)
         rescue MandateNotFoundError, GoCardlessPro::Error => e
-          result.error_message = e.message
-          result.error_code = e.code
-          result.payment_status = :failed
-          result.service_failure!(code: "gocardless_error", message: "#{e.code}: #{e.message}")
+          prepare_failed_result(e, reraise: true)
         end
 
         attr_reader :payment, :invoice, :provider_customer
@@ -86,8 +80,8 @@ def mandate_id
         def create_gocardless_payment
           client.payments.create(
             params: {
-              amount: invoice.total_amount_cents,
-              currency: invoice.currency.upcase,
+              amount: payment.amount_cents,
+              currency: payment.amount_currency.upcase,
               retry_if_possible: false,
               metadata: {
                 lago_customer_id: customer.id,
@@ -111,6 +105,16 @@ def payment_status_mapping(payment_status)
 
           payment_status
         end
+
+        def prepare_failed_result(error, reraise: false)
+          result.error_message = error.message
+          result.error_code = error.code
+          result.reraise = reraise
+
+          payment.update!(status: :failed, payable_payment_status: :failed)
+
+          result.service_failure!(code: "gocardless_error", message: "#{error.code}: #{error.message}")
+        end
       end
     end
   end

app/services/payment_providers/stripe/payments/create_service.rb

@@ -24,35 +24,32 @@ def call
 
           payment.provider_payment_id = stripe_result.id
           payment.status = stripe_result.status
+          payment.payable_payment_status = payment_status_mapping(payment.status)
           payment.provider_payment_data = stripe_result.next_action if stripe_result.status == "requires_action"
           payment.save!
 
           handle_requires_action(payment) if payment.status == "requires_action"
 
-          result.payment_status = payment_status_mapping(payment.status)
           result.payment = payment
           result
 
         # TODO: global refactor of the error handling
-        # reprocessable errors shoud let the payment status to pending
-        # identified errors should mark it as failed to allow reprocess via a new payment
+        # identified processing errors should mark it as failed to allow reprocess via a new payment
+        # other should be reprocessed
         rescue ::Stripe::AuthenticationError, ::Stripe::CardError, ::Stripe::InvalidRequestError, ::Stripe::PermissionError => e
           # NOTE: Do not mark the invoice as failed if the amount is too small for Stripe
           #       For now we keep it as pending, the user can still update it manually
           return result if e.code == "amount_too_small"
 
-          result.error_message = e.message
-          result.error_code = e.code
-          result.payment_status = :failed
-          result
+          prepare_failed_result(e)
         rescue ::Stripe::RateLimitError => e
+          # Allow auto-retry with idempotency key
           raise Invoices::Payments::RateLimitError, e
         rescue ::Stripe::APIConnectionError => e
+          # Allow auto-retry with idempotency key
           raise Invoices::Payments::ConnectionError, e
         rescue ::Stripe::StripeError => e
-          result.error_message = e.message
-          result.error_code = e.code
-          result.service_failure!(code: "stripe_error", message: e.message)
+          prepare_failed_result(e, reraise: true)
         end
 
         private
@@ -125,8 +122,8 @@ def create_payment_intent
 
         def payment_intent_payload
           {
-            amount: invoice.total_amount_cents,
-            currency: invoice.currency.downcase,
+            amount: payment.amount_cents,
+            currency: payment.amount_currency.downcase,
             customer: provider_customer.provider_customer_id,
             payment_method: stripe_payment_method,
             payment_method_types: provider_customer.provider_payment_methods,
@@ -163,6 +160,16 @@ def error_on_requires_action?
         def description
           "#{invoice.organization.name} - Invoice #{invoice.number}"
         end
+
+        def prepare_failed_result(error, reraise: false)
+          result.error_message = error.message
+          result.error_code = error.code
+          result.reraise = reraise
+
+          payment.update!(status: :failed, payable_payment_status: :failed)
+
+          result.service_failure!(code: "stripe_error", message: error.message)
+        end
       end
     end
   end

db/migrate/20241216110525_add_payable_payment_status_to_payment.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+class AddPayablePaymentStatusToPayment < ActiveRecord::Migration[7.1]
+  disable_ddl_transaction!
+
+  def change
+    create_enum :payment_payable_payment_status,
+      %w[
+        pending
+        processing
+        succeeded
+        failed
+      ]
+
+    add_column :payments,
+      :payable_payment_status,
+      :enum,
+      enum_type: 'payment_payable_payment_status',
+      null: true
+
+    add_index :payments,
+      %i[payable_id payable_type],
+      where: "payable_payment_status in ('pending', 'processing')",
+      unique: true,
+      algorithm: :concurrently
+  end
+end