From 55b9928909f54cbc3f8228e86e7dee4932ce20e5 Mon Sep 17 00:00:00 2001 From: Jan Date: Sat, 13 Jun 2026 17:29:00 +0000 Subject: [PATCH 1/2] fix(stock): make transfer reserved-stock guard explicit; document stock invariants (closes audit-M-stock) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stock/concurrency cluster (bkhk). Most findings were already resolved by prior work; this makes the remaining guard explicit + tested and documents the rest: 1. Transfer-of-allocated-stock: dispatch availability already nets the source warehouse's per-(product,warehouse) reservedQty (kept in sync with order allocations), so a transfer can't strand an order. Extracted that rule into a pure availableForTransfer/canDispatchTransferQty helper (used by dispatch) so it's explicit and regression-tested. 2. Manual receipt + WMS booked-in double-layer: VERIFIED resolved — reconcileBookedInQuantities nets localReceivedQty (read under the PO FOR UPDATE lock); already covered by mintsoft-phase6-booked-in tests. 3. Opening-stock race: VERIFIED resolved — applyOpeningStock takes a FOR UPDATE lock on the stock level before the existing-opening-layer check, serialising concurrent calls. 4. Transfer FIFO ordering at destination: documented + accepted (cosmetic; per layer cost basis preserved). 5. stock_levels CHECK >= 0: VERIFIED present (migration 20260512100000: quantity>=0, reservedQty>=0, reservedQty<=quantity). Documented all five in docs/workflows.md under Stock / concurrency invariants. Tests: 5 transfer-availability cases; type-check + lint clean. Co-Authored-By: Claude Fable 5 --- app/actions/transfers.ts | 7 ++-- docs/workflows.md | 23 +++++++++++++ lib/domain/inventory/transfer-availability.ts | 32 +++++++++++++++++++ .../inventory/transfer-availability.test.ts | 28 ++++++++++++++++ 4 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 lib/domain/inventory/transfer-availability.ts create mode 100644 tests/domain/inventory/transfer-availability.test.ts diff --git a/app/actions/transfers.ts b/app/actions/transfers.ts index 5ac9d075..bc752056 100644 --- a/app/actions/transfers.ts +++ b/app/actions/transfers.ts @@ -17,6 +17,7 @@ import { } from '@/lib/cost-layers' import { sliceTransferSnapshotForReceipt } from '@/lib/domain/wms/asn-reconciliation' import { toInventoryConstraintMessage } from '@/lib/domain/inventory/prisma-errors' +import { availableForTransfer, canDispatchTransferQty } from '@/lib/domain/inventory/transfer-availability' import { addMoney, multiplyMoney, roundQuantity, toDecimal } from '@/lib/domain/math/decimal' import { serializeCostLayerSnapshot } from '@/lib/cost-layer-snapshots' import { @@ -388,8 +389,10 @@ export async function dispatchTransfer(id: string): Promise { where: { productId_warehouseId: { productId: line.productId, warehouseId: transfer.fromWarehouseId } }, select: { quantity: true, reservedQty: true }, }) - const available = level ? Number(level.quantity) - Number(level.reservedQty) : 0 - if (available < qty) { + // audit-M-stock #1: net the source warehouse's reserved (allocated) + // quantity so a transfer can't drain stock an order is holding there. + const available = availableForTransfer(level?.quantity, level?.reservedQty) + if (!canDispatchTransferQty(level?.quantity, level?.reservedQty, qty)) { throw new Error(`Insufficient stock for ${line.sku}: ${available} available, ${qty} requested`) } } diff --git a/docs/workflows.md b/docs/workflows.md index e24d2df6..35a026b9 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -151,3 +151,26 @@ refund payments. Stock transfer status tracks inter-warehouse movement. `IN_TRANSIT` means stock has left the source warehouse and is not yet available at the destination. + +**Stock / concurrency invariants (audit-M-stock).** A few cross-cutting guards +worth knowing: + +- **Transfers don't strand allocations.** Dispatch availability is the source + warehouse's on-hand minus its `reservedQty` (`availableForTransfer`), and + `StockLevel.reservedQty` is per-(product, warehouse) and kept in sync with + order allocations — so a transfer can never drain stock an order is holding in + that warehouse. +- **Manual receipt + WMS booked-in don't double-count.** `reconcileBookedInQuantities` + nets the locally-received quantity (read under the PO's `FOR UPDATE` lock) so a + manual receipt already covering a line is not re-added when the Mintsoft + booked-in webhook approves the same ASN. +- **Opening stock can't duplicate.** `applyOpeningStock` takes a `FOR UPDATE` + lock on the stock level before checking for an existing opening cost layer, so + concurrent calls serialise and the second is rejected. +- **Non-negative stock is enforced in the DB** via CHECK constraints + (`stock_levels_quantity_nonnegative`, `reservedQty >= 0`, `reservedQty <= quantity`). +- **FIFO ordering at the destination** of a received transfer follows the + dispatch-time cost-layer snapshot order. If transfers are received out of + dispatch order the recreated layers can be marginally out of strict FIFO order + at the destination — this is cosmetic for reporting and accepted (the cost + basis per layer is preserved). diff --git a/lib/domain/inventory/transfer-availability.ts b/lib/domain/inventory/transfer-availability.ts new file mode 100644 index 00000000..ef40185a --- /dev/null +++ b/lib/domain/inventory/transfer-availability.ts @@ -0,0 +1,32 @@ +// decimal-boundary-ok: server-action-boundary (numeric stock availability check) +import { decimalToNumber, type DecimalLike } from '@/lib/decimal' + +// --------------------------------------------------------------------------- +// Transfer-dispatch availability (audit-M-stock #1) +// +// A transfer must not drain stock that an order has already reserved in the +// SOURCE warehouse, or the order is stranded. StockLevel.reservedQty is +// per-(product, warehouse) and kept in sync with order allocations, so the +// available-to-transfer quantity is the warehouse's on-hand minus its reserved +// — netting out exactly the allocations pointing at that warehouse. Centralised +// here so the rule is explicit and regression-tested rather than an inline +// subtraction. +// --------------------------------------------------------------------------- + +/** On-hand minus reserved for a single (product, warehouse) stock level. Never negative. */ +export function availableForTransfer( + quantity: DecimalLike | null | undefined, + reservedQty: DecimalLike | null | undefined, +): number { + const available = decimalToNumber(quantity ?? 0) - decimalToNumber(reservedQty ?? 0) + return available > 0 ? available : 0 +} + +/** Whether `requestedQty` can be dispatched without eating into reserved (allocated) stock. */ +export function canDispatchTransferQty( + quantity: DecimalLike | null | undefined, + reservedQty: DecimalLike | null | undefined, + requestedQty: number, +): boolean { + return requestedQty <= availableForTransfer(quantity, reservedQty) +} diff --git a/tests/domain/inventory/transfer-availability.test.ts b/tests/domain/inventory/transfer-availability.test.ts new file mode 100644 index 00000000..7968d450 --- /dev/null +++ b/tests/domain/inventory/transfer-availability.test.ts @@ -0,0 +1,28 @@ +import assert from 'node:assert/strict' +import test from 'node:test' + +import { Prisma } from '@/app/generated/prisma/client' +import { availableForTransfer, canDispatchTransferQty } from '@/lib/domain/inventory/transfer-availability' + +test('availableForTransfer nets reserved (allocated) qty out of on-hand', () => { + assert.equal(availableForTransfer(new Prisma.Decimal('100'), new Prisma.Decimal('30')), 70) +}) + +test('availableForTransfer never goes negative', () => { + assert.equal(availableForTransfer(new Prisma.Decimal('10'), new Prisma.Decimal('15')), 0) +}) + +test('availableForTransfer treats missing level as zero', () => { + assert.equal(availableForTransfer(null, null), 0) + assert.equal(availableForTransfer(undefined, undefined), 0) +}) + +test('canDispatchTransferQty: cannot dispatch into reserved stock', () => { + // 100 on hand, 30 reserved for an order → only 70 transferable. + assert.equal(canDispatchTransferQty(new Prisma.Decimal('100'), new Prisma.Decimal('30'), 70), true) + assert.equal(canDispatchTransferQty(new Prisma.Decimal('100'), new Prisma.Decimal('30'), 71), false) +}) + +test('canDispatchTransferQty: full unreserved stock is transferable', () => { + assert.equal(canDispatchTransferQty(new Prisma.Decimal('50'), new Prisma.Decimal('0'), 50), true) +}) From 0ae2085febb2cdc9a3d9a01adfc5885552bc6080 Mon Sep 17 00:00:00 2001 From: Jan Date: Sat, 13 Jun 2026 17:36:33 +0000 Subject: [PATCH 2/2] fix(stock): preserve over-reservation diagnostic + precise docs (adversarial review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex adversarial pass on PR #206 (verified all three 'already-resolved' claims are substantively correct; no refuted claims): MEDIUM — the extracted helper clamped a negative (over-reserved) available to 0, so the dispatch error message showed '0 available' instead of the real negative delta, hiding a data-integrity signal. The error now reports the raw (unclamped) delta while the clamped value still gates dispatch. MEDIUM — docs overstated the reservedQty<=quantity constraint: it's NOT VALID (new-writes-only, never validated against historical rows), unlike the fully VALIDATEd quantity>=0 / reservedQty>=0. Docs corrected, and note the transfer guard (not just the constraint) is what protects allocations. LOW — docs now mention both the PO and stock-transfer FOR UPDATE locks for the WMS reconcile; JSDoc warns availableForTransfer's clamp must not be reused for data-integrity checks; added an over-reserved canDispatchTransferQty test. 6/6 helper tests; type-check + lint clean. Co-Authored-By: Claude Fable 5 --- app/actions/transfers.ts | 8 +++++--- docs/workflows.md | 13 ++++++++----- lib/domain/inventory/transfer-availability.ts | 7 ++++++- .../domain/inventory/transfer-availability.test.ts | 4 ++++ 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/app/actions/transfers.ts b/app/actions/transfers.ts index bc752056..762b7cb5 100644 --- a/app/actions/transfers.ts +++ b/app/actions/transfers.ts @@ -17,7 +17,7 @@ import { } from '@/lib/cost-layers' import { sliceTransferSnapshotForReceipt } from '@/lib/domain/wms/asn-reconciliation' import { toInventoryConstraintMessage } from '@/lib/domain/inventory/prisma-errors' -import { availableForTransfer, canDispatchTransferQty } from '@/lib/domain/inventory/transfer-availability' +import { canDispatchTransferQty } from '@/lib/domain/inventory/transfer-availability' import { addMoney, multiplyMoney, roundQuantity, toDecimal } from '@/lib/domain/math/decimal' import { serializeCostLayerSnapshot } from '@/lib/cost-layer-snapshots' import { @@ -391,9 +391,11 @@ export async function dispatchTransfer(id: string): Promise { }) // audit-M-stock #1: net the source warehouse's reserved (allocated) // quantity so a transfer can't drain stock an order is holding there. - const available = availableForTransfer(level?.quantity, level?.reservedQty) if (!canDispatchTransferQty(level?.quantity, level?.reservedQty, qty)) { - throw new Error(`Insufficient stock for ${line.sku}: ${available} available, ${qty} requested`) + // Report the raw (unclamped) delta so an over-reservation (negative) + // stays a visible diagnostic, not silently shown as 0. + const rawAvailable = Number(level?.quantity ?? 0) - Number(level?.reservedQty ?? 0) + throw new Error(`Insufficient stock for ${line.sku}: ${rawAvailable} available, ${qty} requested`) } } diff --git a/docs/workflows.md b/docs/workflows.md index 35a026b9..0455ac8e 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -161,14 +161,17 @@ worth knowing: order allocations — so a transfer can never drain stock an order is holding in that warehouse. - **Manual receipt + WMS booked-in don't double-count.** `reconcileBookedInQuantities` - nets the locally-received quantity (read under the PO's `FOR UPDATE` lock) so a - manual receipt already covering a line is not re-added when the Mintsoft - booked-in webhook approves the same ASN. + nets the locally-received quantity (read under a `FOR UPDATE` lock on the PO or + the stock transfer) so a manual receipt already covering a line is not re-added + when the Mintsoft booked-in webhook approves the same ASN. - **Opening stock can't duplicate.** `applyOpeningStock` takes a `FOR UPDATE` lock on the stock level before checking for an existing opening cost layer, so concurrent calls serialise and the second is rejected. -- **Non-negative stock is enforced in the DB** via CHECK constraints - (`stock_levels_quantity_nonnegative`, `reservedQty >= 0`, `reservedQty <= quantity`). +- **Non-negative stock in the DB.** `quantity >= 0` and `reservedQty >= 0` are + fully VALIDATEd CHECK constraints (every existing row checked). `reservedQty <= + quantity` is a `NOT VALID` constraint — enforced on new writes but not yet + validated against historical rows (that cleanup is deferred), so the transfer + guard above (not just the constraint) is what protects allocations at dispatch. - **FIFO ordering at the destination** of a received transfer follows the dispatch-time cost-layer snapshot order. If transfers are received out of dispatch order the recreated layers can be marginally out of strict FIFO order diff --git a/lib/domain/inventory/transfer-availability.ts b/lib/domain/inventory/transfer-availability.ts index ef40185a..87502da5 100644 --- a/lib/domain/inventory/transfer-availability.ts +++ b/lib/domain/inventory/transfer-availability.ts @@ -13,7 +13,12 @@ import { decimalToNumber, type DecimalLike } from '@/lib/decimal' // subtraction. // --------------------------------------------------------------------------- -/** On-hand minus reserved for a single (product, warehouse) stock level. Never negative. */ +/** + * On-hand minus reserved for a single (product, warehouse) stock level, clamped + * to >= 0. Correct for dispatch gating; do NOT reuse for data-integrity checks — + * the clamp hides an over-reservation (reservedQty > quantity), which raw + * subtraction would surface as a negative. + */ export function availableForTransfer( quantity: DecimalLike | null | undefined, reservedQty: DecimalLike | null | undefined, diff --git a/tests/domain/inventory/transfer-availability.test.ts b/tests/domain/inventory/transfer-availability.test.ts index 7968d450..69a7ffff 100644 --- a/tests/domain/inventory/transfer-availability.test.ts +++ b/tests/domain/inventory/transfer-availability.test.ts @@ -26,3 +26,7 @@ test('canDispatchTransferQty: cannot dispatch into reserved stock', () => { test('canDispatchTransferQty: full unreserved stock is transferable', () => { assert.equal(canDispatchTransferQty(new Prisma.Decimal('50'), new Prisma.Decimal('0'), 50), true) }) + +test('canDispatchTransferQty: rejects when over-reserved (reservedQty > quantity)', () => { + assert.equal(canDispatchTransferQty(new Prisma.Decimal('10'), new Prisma.Decimal('15'), 1), false) +})