From 7dbd77a1f426a32fe0a975febadce3d2a85b365d Mon Sep 17 00:00:00 2001 From: LauAubert Date: Fri, 2 Oct 2026 01:57:02 -0300 Subject: [PATCH] fix: [TESIS-999019] check the product version under the stock lock PUT /products with If-Match verified the version with a FOR UPDATE on the products row, and took the stock advisory lock only afterwards, to write. Sales (DeductStock) and transfers never touch the products row, they only take the advisory lock, so a sale could commit between the check and the lock: the version already validated did not include it, and the absolute quantities of the request erased it without a trace. That is the exact race the product version exists to catch. When the request carries stocks, the advisory lock is now taken before the version check and covers it and the write; a concurrent sale waits or the PUT answers 409. Updates without stocks still take no stock lock. Co-Authored-By: Claude Opus 5.5 --- app/poros/products/update_product.rb | 43 ++++++----- spec/poros/products/update_product_spec.rb | 85 ++++++++++++++++++++++ spec/requests/api/v1/products_spec.rb | 4 +- 3 files changed, 111 insertions(+), 21 deletions(-) create mode 100644 spec/poros/products/update_product_spec.rb diff --git a/app/poros/products/update_product.rb b/app/poros/products/update_product.rb index ff8d1e59..cd0d399f 100644 --- a/app/poros/products/update_product.rb +++ b/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! diff --git a/spec/poros/products/update_product_spec.rb b/spec/poros/products/update_product_spec.rb new file mode 100644 index 00000000..6a9b1b3b --- /dev/null +++ b/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 diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 2c1a67a5..32d30c80 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/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