fix(resources): control de concurrencia optimista en PUT /resources/:id/inventory (#294) - #335
fix(resources): control de concurrencia optimista en PUT /resources/:id/inventory (#294)#335nopestack wants to merge 2 commits into
Conversation
…ventory (GlobalEmergency#294) PUT /resources/:id/inventory replaced the whole declared inventory with an unconditional delete+reinsert, so a line merged concurrently via Resource.receiveInventory (POST /resources/:id/inventory-entries, the donation intake worker) between the owner's form load and their save was silently discarded. Adds an inventoryVersion counter on resources (migration 0055), bumped on every replaceInventory/receiveInventory. GET /resources/:id/inventory now returns { items, version }; PUT requires expectedVersion and is rejected with 409 (InventoryVersionConflictError) when it no longer matches — checked both fail-fast in the use case and atomically at the storage level via a conditional UPDATE ... WHERE inventory_version = $expected, so a real concurrent writer can't slip through between the two checks. The mis-puntos inventory form carries the version in a hidden field and surfaces a clear "reload and retry" message on 409.
|
@nopestack is attempting to deploy a commit to the GlobalEmergency Team on Vercel. A member of the Team first needs to authorize it. |
vgpastor
left a comment
vgpastor
left a comment
There was a problem hiding this comment.
¡Gran trabajo con esto! 🙌 El control de concurrencia optimista está muy bien planteado: el UPDATE ... WHERE id=$1 AND inventory_version=$2 RETURNING es el check-and-set atómico correcto, el mirror in-memory replica la misma semántica race-free, el guard de no-op en receiveInventory está cuidado, y la cobertura de tests es ejemplar (dominio + aplicación reproduciendo el escenario exacto del issue + integración contra Postgres real + e2e con el caso 409). i18n en ambos locales, filtro de excepciones y Swagger actualizados, y gen:api regenerado. Migración idempotente e inmutable-safe. Se nota el curro. 👏
Dejo un par de observaciones para que las valores cuando puedas (ninguna la veo bloqueante):
- La principal (comentario en
drizzle-resource.repository.ts): el candado solo cubre la ruta del PUT.save()sigue pisando items y ahora además revierteinventory_version, lo que en un escenario con una mutación no-inventario concurrente puede reabrir el propio lost-update de #294. Creo que da para un ticket de seguimiento. - Nit (comentario en
actions.ts): el fallback a0cuando faltaexpectedVersionno es estrictamente "siempre obsoleto" para un recurso recién registrado.
¡Gracias por el detalle en los comentarios y los tests, se revisa genial así! 🚀
Generated by Claude Code
| disputed: s.disputed ?? false, | ||
| disputedAt: s.disputedAt ?? null, | ||
| disputeDismissedAt: s.disputeDismissedAt ?? null, | ||
| inventoryVersion: s.inventoryVersion ?? 0, |
There was a problem hiding this comment.
Aquí save() fija inventory_version al valor que trae el snapshot en memoria, y no es solo un no-op: puede revertir la versión y, con ello, reabrir la carrera que este PR cierra. Como save() sigue borrando+reinsertando resource_items incondicionalmente, cualquier mutación NO-inventario (cambio de estado público, verify, disputa…) que se cargara antes de un receiveInventory concurrente pisa los items y baja la versión:
v5, items[A,B]- El dueño abre el form PUT → lee
v5 - Un coordinador abre "cambiar estado" → carga
v5,[A,B] - El worker de donaciones fusiona
C→v6,[A,B,C],save() - El coordinador guarda el estado →
save()escribev5e items[A,B](¡Cperdido y versión revertida a 5!) - El dueño hace PUT con
expectedVersion=5→ elWHERE inventory_version=5coincide y el write pasa → justo el lost-update de [Resources] Lost update en PUT /resources/:id/inventory frente a merges concurrentes (receiveInventory) #294
El clobber de items por save() es preexistente, pero la reversión de la versión es nueva. No lo veo bloqueante para este PR (es un problema estructural del save() last-write-wins, más amplio que el "dos receiveInventory" que ya dejáis fuera de alcance), pero creo que merece su propio ticket de seguimiento. La solución limpia sería que save() no fije la versión al valor en memoria (que la preserve/incremente a nivel de storage, o directamente no toque items/versión cuando la mutación no es de inventario). 👍
Generated by Claude Code
| // stale" (0 never matches a resource whose inventory has been touched more | ||
| // than once) so the API rejects it with 409 rather than the write silently | ||
| // going through with an undefined expectedVersion. | ||
| const expectedVersion = Number(formData.get('expectedVersion') ?? NaN); |
There was a problem hiding this comment.
Nit: el comentario dice que un valor ausente/no numérico es "definitely stale" porque "0 never matches a resource whose inventory has been touched more than once". Pero en un recurso recién registrado cuya versión sigue siendo 0 (nunca tocado), este fallback a 0 sí coincidiría y dejaría pasar el write con una versión que en realidad se desconocía. Es un borde muy benigno (habría que perder el hidden field), pero la afirmación del comentario no es estrictamente cierta.
Si queréis un centinela realmente imposible, un -1 como expectedVersion de fallback nunca casaría (@Min(0) lo rechazaría con 400, o el WHERE inventory_version = -1 daría 0 filas → 409), y así "no sé la versión" nunca se confunde con "versión 0". No es imprescindible.
Generated by Claude Code
|
Heads-up de coordinación 🔀: acabo de mergear #338 (fix #296), que toca los mismos ficheros que este PR — Dos cosas cuando puedas:
Gracias por el curro 🙌 Generated by Claude Code |
2 participants
Closes #294
Problema
PUT /resources/:id/inventorysobrescribía el inventario completo sin control de concurrencia, perdiendo en silencio líneas fusionadas porPOST /resources/:id/inventory-entrieso el worker de donaciones mientras el formulario del dueño estaba abierto.Solución
0055_resource_inventory_version.sql: columnaresources.inventory_version.ResourcetrackeainventoryVersion, incrementada porreplaceInventoryyreceiveInventory.saveIfInventoryVersionMatches— UPDATE condicional atómico en la misma transacción que el delete+insert de items.PUTahora requiereexpectedVersion; mismatch →409 Conflict(InventoryVersionConflictError).GET /resources/:id/inventorydevuelve{ items, version }.pnpm gen:apiregenerado.expectedVersiony muestra un mensaje claro en caso de 409 (recarga automática completa queda como mejora futura).Tests
pnpm --filter api test: 248 suites / 1603 tests. Build, lint (--max-warnings=0), prettier OK.Fuera de alcance (seguimiento futuro)
receiveInventoryentre sí (worker vs inventory-entries) siguen siendo read-modify-write simple — no es lo que reporta este issue, pero podría necesitar su propio ticket si se manifiesta.