Merge pull request #4136 from mkllnk/4018-synchronise-checkout

Lock variants during checkout to avoid race condition

app/controllers/checkout_controller.rb

@@ -3,6 +3,10 @@ require 'open_food_network/address_finder'
 class CheckoutController < Spree::CheckoutController
   layout 'darkswarm'
 
+  # We need pessimistic locking to avoid race conditions.
+  # Otherwise we fail on duplicate indexes or end up with negative stock.
+  prepend_around_filter CurrentOrderLocker, only: :update
+
   prepend_before_filter :check_hub_ready_for_checkout
   prepend_before_filter :check_order_cycle_expiry
   prepend_before_filter :require_order_cycle

app/services/current_order_locker.rb

@@ -0,0 +1,40 @@
+# Locks a controller's current order including its variants.
+#
+# It should be used when making major changes like checking out the order.
+# It can keep stock checking in sync and prevent overselling of an item.
+class CurrentOrderLocker
+  # This interface follows the ActionController filters convention:
+  #
+  #   https://guides.rubyonrails.org/action_controller_overview.html#filters
+  #
+  def self.around(controller)
+    lock_order_and_variants(controller.current_order) { yield }
+  end
+
+  # Locking will not prevent all access to these rows. Other processes are
+  # only waiting if they try to lock one of these rows as well.
+  #
+  #   https://api.rubyonrails.org/classes/ActiveRecord/Locking/Pessimistic.html
+  #
+  def self.lock_order_and_variants(order)
+    return yield if order.nil?
+
+    order.with_lock do
+      lock_variants_of(order)
+      yield
+    end
+  end
+  private_class_method :lock_order_and_variants
+
+  # There are many places in which stock is stored in the database. Row locking
+  # on variant level ensures that there are no conflicts even when an item is
+  # sold through multiple shops.
+  def self.lock_variants_of(order)
+    variant_ids = order.line_items.select(:variant_id)
+
+    # Ordering the variants by id prevents deadlocks. Plucking the ids sends
+    # the locking query without building Spree::Variant objects.
+    Spree::Variant.where(id: variant_ids).order(:id).lock.pluck(:id)
+  end
+  private_class_method :lock_variants_of
+end

spec/controllers/checkout_controller_concurrency_spec.rb

@@ -0,0 +1,89 @@
+require 'spec_helper'
+
+# This is the first example of testing concurrency in the Open Food Network.
+# If we want to do this more often, we should look at:
+#
+#   https://github.com/forkbreak/fork_break
+#
+# The concurrency flag enables multiple threads to see the same database
+# without isolated transactions.
+describe CheckoutController, concurrency: true, type: :controller do
+  let(:order_cycle) { create(:order_cycle) }
+  let(:distributor) { order_cycle.distributors.first }
+  let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor) }
+  let(:address) { create(:address) }
+  let(:payment_method) { create(:payment_method, distributors: [distributor]) }
+  let(:breakpoint) { Mutex.new }
+
+  before do
+    # Create a valid order ready for checkout:
+    create(:shipping_method, distributors: [distributor])
+    variant = order_cycle.variants_distributed_by(distributor).first
+    order.line_items << create(:line_item, variant: variant)
+
+    # Set up controller environment:
+    session[:order_id] = order.id
+    allow(controller).to receive(:spree_current_user).and_return(order.user)
+    allow(controller).to receive(:current_distributor).and_return(order.distributor)
+    allow(controller).to receive(:current_order_cycle).and_return(order.order_cycle)
+
+    # New threads start running straight away. The breakpoint is after loading
+    # the order and before advancing the order's state and making payments.
+    breakpoint.lock
+    allow(controller).to receive(:check_order_for_phantom_fees) do
+      breakpoint.synchronize {}
+    end
+  end
+
+  it "waits for concurrent checkouts" do
+    # Basic data the user submits during checkout:
+    address_params = address.attributes.except("id")
+    order_params = {
+      "payments_attributes" => [
+        {
+          "payment_method_id" => payment_method.id,
+          "amount" => order.total
+        }
+      ],
+      "bill_address_attributes" => address_params,
+      "ship_address_attributes" => address_params,
+    }
+
+    # Starting two checkout threads. The controller code will determine if
+    # these two threads are synchronised correctly or run into a race condition.
+    #
+    # 1. If the controller synchronises correctly:
+    #    The first thread locks required resources and then waits at the
+    #    breakpoint. The second thread waits for the first one.
+    # 2. If the controller fails to prevent the race condition:
+    #    Both threads load required resources and wait at the breakpoint to do
+    #    the same checkout action. This will lead to (random) errors.
+    #
+    #    I observed:
+    #    ActiveRecord::RecordNotUnique: duplicate key value violates unique
+    #    constraint "index_spree_shipments_on_order_id"
+    #    on `INSERT INTO "spree_shipments" ...`.
+    #
+    #    Or:
+    #    ActiveRecord::InvalidForeignKey: insert or update on table
+    #    "spree_orders" violates foreign key constraint
+    #    "spree_orders_customer_id_fk"
+    threads = [
+      Thread.new { spree_post :update, format: :json, order: order_params },
+      Thread.new { spree_post :update, format: :json, order: order_params },
+    ]
+    # Let the threads run again. They should not be in a race condition.
+    breakpoint.unlock
+    # Wait for both threads to finish.
+    threads.each(&:join)
+    order.reload
+
+    # When the spec passes, both threads have the same result. The user should
+    # see the order page. This is basically verifying a "double click"
+    # scenario.
+    expect(response.status).to eq(200)
+    expect(response.body).to eq({ path: spree.order_path(order) }.to_json)
+    expect(order.payments.count).to eq 1
+    expect(order.completed?).to be true
+  end
+end

spec/controllers/checkout_controller_spec.rb

@@ -130,6 +130,7 @@ describe CheckoutController, type: :controller do
     context 'when completing the order' do
       before do
         order.state = 'complete'
+        order.save!
         allow(order).to receive(:update_attributes).and_return(true)
         allow(order).to receive(:next).and_return(true)
         allow(order).to receive(:set_distributor!).and_return(true)

spec/spec_helper.rb

@@ -100,6 +100,7 @@ RSpec.configure do |config|
   config.before(:suite)          { DatabaseCleaner.clean_with :deletion, except: ['spree_countries', 'spree_states'] }
   config.before(:each)           { DatabaseCleaner.strategy = :transaction }
   config.before(:each, js: true) { DatabaseCleaner.strategy = :deletion, { except: ['spree_countries', 'spree_states'] } }
+  config.before(:each, concurrency: true) { DatabaseCleaner.strategy = :deletion, { except: ['spree_countries', 'spree_states'] } }
   config.before(:each)           { DatabaseCleaner.start }
   config.after(:each)            { DatabaseCleaner.clean }
   config.after(:each, js: true) do