fix: [TESIS-999019] check the product version under the stock lock

app/poros/products/update_product.rb

@@ -14,14 +14,13 @@ def initialize(product:, params:, stocks:, expected_version: nil)
 
     def call
       Product.transaction do
-        verify_version!
-        @product.update!(@params)
-
-        if @stocks_params.present?
-          write_stocks!
-          reload_for_render!
+        within_stock_lock do
+          verify_version!
+          @product.update!(@params)
+          write_stocks! if @stocks_params.present?
         end
 
+        reload_for_render! if @stocks_params.present?
         @product
       end
     end
@@ -54,27 +53,33 @@ def verify_version!
     # bloquear. Además la operación abarca varias filas del mismo producto,
     # así que se serializa por producto y no fila por fila.
     #
-    # Ojo con el alcance: esto ordena las escrituras, no detecta ediciones
-    # concurrentes. Como la cantidad llega absoluta desde el request, dos
-    # operadores que editan el mismo producto siguen pisándose — el último
-    # gana. Detectar eso pide locking optimista (lock_version / If-Match) y
-    # un cambio de contrato; ver ADR-009.
+    # Con stocks en el request, el lock se toma ANTES de verificar la versión y
+    # cubre las dos cosas. Antes sólo envolvía la escritura: el `lock!` de la
+    # versión bloquea la fila de `products`, pero las ventas (`DeductStock`) y
+    # las transferencias no la tocan, sólo toman este advisory lock. Entre el
+    # chequeo y el lock entraba una venta, la versión ya validada no la veía y
+    # la cantidad absoluta del request la borraba sin rastro (hallazgo de la
+    # auditoría de TESIS-89). Bajo el mismo lock, la venta espera o el PUT
+    # responde 409.
     #
-    # El lock envuelve sólo la escritura de stocks y no el update! del
-    # producto: editar el nombre no compite por stock con nadie, y con
-    # wait: false envolver todo el #call devolvería 409 espurios mientras un
-    # job de sincronización toca los stocks en paralelo.
+    # Sin stocks, nada: editar el nombre no compite por stock con nadie, y con
+    # wait: false envolver ese caso devolvería 409 espurios mientras un job de
+    # sincronización toca los stocks en paralelo.
     #
     # wait: false porque esto corre en el ciclo de un request HTTP: conviene
     # devolver 409 enseguida (ApplicationController mapea el
     # Catalog::LockTimeoutError) antes que colgar un thread de Puma esperando.
     # El modo wait: true queda para los jobs de background.
+    def within_stock_lock(&)
+      return yield if @stocks_params.blank?
+
+      Catalog::WithStockLock.new(product_id: @product.id, wait: false).call(&)
+    end
+
+    # Corre ya dentro del advisory lock (ver `within_stock_lock`).
     def write_stocks!
       validate_warehouses_belong_to_company!
-
-      Catalog::WithStockLock.new(product_id: @product.id, wait: false).call do
-        upsert_stocks_for_product!
-      end
+      upsert_stocks_for_product!
     end
 
     def upsert_stocks_for_product!

spec/poros/products/update_product_spec.rb

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Products::UpdateProduct, type: :poro do
+  def company = @company ||= Company.create!(name: 'Acme', tax_id: '20-12345678-9')
+  def product = @product ||= Product.create!(company: company, sku: 'CEL-1', name: 'Celular')
+
+  def warehouse
+    @warehouse ||= Warehouse.create!(company: company, name: 'Central', zip_code: '1900',
+                                     address: 'Av 1')
+  end
+
+  before do
+    Current.company_id = company.id
+    Stock.create!(product: product, warehouse: warehouse, quantity: 10)
+  end
+
+  def current_version = Catalog::ProductVersion.new(product: product.reload).call
+
+  def update(stocks:, expected_version: current_version)
+    described_class.new(product: product, params: { name: 'Celular X' }, stocks: stocks,
+                        expected_version: expected_version).call
+  end
+
+  # Desde otra sesión de Postgres, como lo vería una venta: el advisory lock es
+  # reentrante dentro de la misma conexión, así que preguntarlo desde la del
+  # test diría que está libre aunque no lo esté.
+  def stock_lock_free_for_others?
+    key = Catalog::WithStockLock.new(product_id: product.id).lock_key
+    config = ActiveRecord::Base.connection_db_config.configuration_hash
+    other = PG.connect(host: config[:host], port: config[:port], dbname: config[:database],
+                       user: config[:username], password: config[:password])
+    other.exec('BEGIN')
+    other.exec("SELECT pg_try_advisory_xact_lock(#{key})").getvalue(0, 0) == 't'
+  ensure
+    other&.exec('ROLLBACK')
+    other&.close
+  end
+
+  # Qué tan libre estaba el lock de stock en el momento en que se calculó la
+  # versión del producto para compararla con el If-Match.
+  def lock_free_while_checking_the_version
+    observed = nil
+    allow(Catalog::ProductVersion).to receive(:new).and_wrap_original do |original, **kwargs|
+      observed = stock_lock_free_for_others?
+      original.call(**kwargs)
+    end
+    yield
+    observed
+  end
+
+  # Hallazgo de auditoría (TESIS-89). Si el lock se tomaba recién para escribir,
+  # una venta entraba entre el chequeo y la escritura, la versión ya validada no
+  # la veía y la cantidad absoluta del request la borraba sin rastro.
+  it 'holds the stock lock while it checks the version, so no sale slips in between' do
+    version = current_version
+
+    free = lock_free_while_checking_the_version do
+      update(stocks: [{ warehouse_id: warehouse.id, quantity: 3 }], expected_version: version)
+    end
+
+    expect(free).to be(false)
+  end
+
+  it 'takes no stock lock for a change that does not touch stocks' do
+    version = current_version
+
+    free = lock_free_while_checking_the_version { update(stocks: [], expected_version: version) }
+
+    expect(free).to be(true)
+  end
+
+  it 'writes the stock it was sent' do
+    update(stocks: [{ warehouse_id: warehouse.id, quantity: 3 }])
+
+    expect(Stock.find_by(product: product, warehouse: warehouse).quantity).to eq(3)
+  end
+
+  it 'refuses a stale version and writes nothing', :aggregate_failures do
+    expect { update(stocks: [{ warehouse_id: warehouse.id, quantity: 3 }], expected_version: 'stale') }
+      .to raise_error(Catalog::StaleProductError)
+    expect(Stock.find_by(product: product, warehouse: warehouse).quantity).to eq(10)
+  end
+end

spec/requests/api/v1/products_spec.rb

@@ -869,8 +869,8 @@ def put_stock(quantity)
         expect(stock.reload.quantity).to eq(5)
       end
 
-      # El lock sólo envuelve la escritura de stocks (ver comentario en
-      # Products::UpdateProduct#write_stocks!): un PATCH que sólo cambia
+      # El lock sólo se toma cuando el request trae stocks (ver comentario en
+      # Products::UpdateProduct#within_stock_lock): un PATCH que sólo cambia
       # `name` no compite por él. Este ejemplo documenta esa decisión de
       # diseño de no envolver todo el update en el advisory lock.
       it 'still returns 200 for an update that does not touch stocks' do