fix: move inventory when an order ships or is cancelled - #85
Merged
Conversation
quantity_reserved went up in TryPlaceAsync and came down in exactly one
place, MarkPaymentFailedAsync. Nothing released it along the fulfilment
path, so against the real transition table:
Paid -> Cancelled kept the reservation, and the stock became permanently
unsellable with no route back but an admin edit.
Paid -> Shipped decremented nothing, so quantity_on_hand kept counting
goods that had left the warehouse and quantity_reserved
grew without bound.
The shipped case hid itself. Availability is on_hand - reserved, and both
stayed wrong by the same amount, so the storefront number looked right while
the warehouse number drifted - visible only at a stock count.
UpdateStatusAsync now takes the transitioned order and writes the status
together with the movement it implies, in one transaction: shipping turns the
reservation into a real decrement of both columns, cancelling releases the
hold, delivery moves nothing because shipping already did. Splitting the two
writes would let a crash between them leave a shipped order still holding its
stock, which is the drift being fixed. The guards keep both columns off
negative, and MarkPaymentFailedAsync now shares the release statement instead
of repeating it.
The in-memory fake mirrors the same movement, skipping widgets absent from
its store so existing lifecycle tests are unaffected.
Five tests cover it: ship, cancel and deliver against real Postgres, and the
handler path for ship and cancel.
Not verified locally - this container has no .NET SDK and no Postgres, so CI
is the first execution of any of it.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
quantity_reservedgoes up inTryPlaceAsyncand comes down in exactly one place:MarkPaymentFailedAsync. Nothing released it along the fulfilment path. Against the real transition table (Paid → Shipped|Cancelled,Shipped → Delivered):on_hand -N,reserved -Non_handcounts goods that left the warehouse;reservedgrows unboundedThe shipped case hid itself: availability is
on_hand - reserved, and both stayed wrong by the same N — so the storefront number looked correct while the warehouse number drifted, visible only at a physical stock count.The fix
UpdateStatusAsyncnow takes the transitioned order and writes the status together with the inventory movement it implies, in one transaction:Splitting those two writes would let a crash between them leave a shipped order still holding its stock — precisely the drift being fixed. Guards keep both columns off negative, and
MarkPaymentFailedAsyncnow shares the release statement rather than repeating it.The in-memory fake mirrors the same movement, skipping widgets absent from its store so existing lifecycle tests are unaffected.
Tests
Five new, covering the behaviour rather than the implementation:
UpdateOrderStatusHandlerNot run locally. This container has no .NET SDK and no Postgres, so CI is the first execution of any of this. Given it touches inventory on the payment path, it deserves a human read before it reaches production.
Not in this PR
The reservation reaper for orders stranded in
AwaitingPayment(a settlement webhook that never arrives holds stock forever). That needs a hosted service, options binding and DI — this repo has noBackgroundServiceyet — so it lands as its own change rather than riding along untested here.Generated by Claude Code