Skip to content

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

Closed
LauAubert wants to merge 1 commit into
masterfrom
TESIS-999019-product-update-lock-before-version
Closed

LauAubert wants to merge 1 commit into
masterfrom
TESIS-999019-product-update-lock-before-version

Conversation

@LauAubert

Copy link
Copy Markdown
Member

Ticket de Jira

https://proyectofinalfrlp.atlassian.net/browse/TESIS-999019

ID provisorio: se reemplaza por la clave real al cargar la card en Jira. Card: cards/019.md. Sale de una auditoría de código (TESIS-89).


Descripción

Products::UpdateProduct verificaba el If-Match (TESIS-101) con lock! —FOR UPDATE sobre la fila de products— y tomaba el advisory lock de stock recién después, para escribir. Pero las ventas (DeductStock, que usan el alta manual, la ingesta por webhook y la edición de órdenes) y las transferencias no tocan la fila del producto: sólo toman el advisory lock. Quedaba una ventana:

  1. El PUT bloquea la fila del producto, lee stock 10, la versión coincide.
  2. Un job de webhook toma el advisory lock, descuenta 10 → 5 y commitea.
  3. El PUT toma el advisory lock (ya libre) y escribe la cantidad absoluta del request: 10.
  4. La venta de 5 unidades desaparece sin rastro.

Es exactamente la carrera que la versión del producto existe para detectar. La ventana es chica, pero real (si la venta todavía tiene el lock, el PUT ya recibía 409, que es lo correcto).

El cambio: cuando el request trae stocks, el advisory lock se toma antes de verificar la versión y cubre el chequeo, el update! y la escritura. Una venta concurrente espera, o el PUT responde 409. Sin stocks no se toma: editar el nombre no compite por stock con nadie, y con wait: false envolver ese caso devolvería 409 espurios (se mantiene la decisión que documentaba el comentario anterior).

  • UpdateProduct#call envuelve versión, update y escritura en within_stock_lock, que toma el lock sólo si hay stocks.
  • write_stocks! ya no toma el lock: corre dentro de él.
  • Agrega spec/poros/products/update_product_spec.rb (no había spec de este PORO): verifica desde otra sesión de Postgres que el lock está tomado mientras se calcula la versión —el advisory lock es reentrante dentro de la misma conexión, así que preguntarlo desde la del test daría «libre» por la razón equivocada—; que sin stocks no se toma; que escribe; y que una versión vieja no escribe nada.
  • Actualiza el comentario del spec de request que remitía a write_stocks!.

Evidencia visual

N/A


Cómo probar

La carrera no se reproduce a mano con facilidad; el spec la fija:

  1. bundle exec rspec spec/poros/products/update_product_spec.rb → 4 ejemplos en verde.
  2. Con app/poros/products/update_product.rb de master, falla «holds the stock lock while it checks the version»: el lock está libre en ese momento.

Verificación: bundle exec rspec (1571 ejemplos, 0 fallas, cobertura 99.89% de línea), rubocop y brakeman limpios.


Impacto y consideraciones

¿Introduce breaking changes?
No. Un PUT con stocks que choca con una venta en curso responde 409 (ya podía pasar), en lugar de pisarla.

¿Requiere nuevas variables de entorno?
No

¿Afecta la arquitectura o genera un nuevo patrón?
No. Ajusta el alcance del lock dentro del criterio de ADR-009.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@TomasMartin2004

Copy link
Copy Markdown
Contributor

Revisado. No lo mergearía para esta entrega.

El análisis es correcto: UpdateProduct verifica el If-Match con lock! sobre la fila de products, y las ventas y las transferencias no tocan esa fila —sólo toman el advisory lock de stock—, así que entre la verificación de versión y la escritura hay una ventana real. Toca RNF-04, que es alcance comprometido, y la corrección se ve bien.

Lo dejo afuera por una razón de riesgo, no de calidad: es un cambio en el orden de los locks del camino de escritura de productos, el lugar donde un error se paga con un deadlock o con stock mal escrito, y son 111 líneas sobre el código más delicado del sistema. Para ganar eso hay que poder probarlo con concurrencia de verdad, y a esta altura del calendario no es donde pondría el riesgo.

Segunda línea: si queda tiempo después de QA, lo retomaría antes que cualquier otro de los que dejé afuera.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants