fix(catalog): stop an admin edit reverting a reservation - #88
Merged
Conversation
Every widget write went through one UpdateAsync that set every column from the caller's in-memory object, including quantity_reserved. Three handlers use the load-change-save shape, so each of them wrote back the stock it had read moments earlier: admin loads widget, reserved 0 customer checks out, reserved 3 admin saves a name change, reserved written back as 0 The order still exists and still expects those goods, but the stock is on sale again. That is an oversell - the precise outcome the guarded reservation exists to prevent - reached with no attacker, by a manager fixing a typo during a busy minute, and silently. It defeated the invariant the rest of the inventory work depends on. The write path is now split by intent, so each caller touches only what it owns: UpdateDetailsAsync name, description, image, price, listed AdjustStockAsync a signed delta to on-hand ArchiveAsync retirement AdjustStockAsync does its arithmetic and both guards inside the statement and returns the resulting availability, so a reservation taken between reading and writing is accounted for rather than overwritten. That also fixes a second lost update in the same shape: two concurrent restocks each computed a new total from a stale row, so one of them vanished. As deltas applied by the database, both land. The handler keeps its own checks in front of the write. They are advisory - the statement is what enforces - but they name the reason accurately, and a restock that would go negative is a different mistake from one that would strand a reservation. Collapsing them into a single message was a regression the existing tests caught. The in-memory fake copies the editable fields onto the stored row instead of replacing the object, because swapping it wholesale reproduces the defect in miniature and would have let these tests pass over a broken repository. ClockSkew is set to thirty seconds while here. The default five minutes quietly turned a fifteen-minute access token into a twenty-minute one; thirty seconds covers ordinary drift between hosts and makes the configured lifetime true. Six integration tests cover it against real PostgreSQL: an edit during a checkout keeps the reservation, an adjustment counts stock reserved in the gap, two adjustments both land, stock cannot go below what is reserved or below zero, and an archived widget refuses one. 489 backend tests and 234 frontend tests pass; build clean under -warnaserror; dotnet format clean. 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
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
Every widget write went through one
UpdateAsyncthat set every column from the caller's in-memory object —quantity_reservedincluded. Three handlers use the load-change-save shape, so each wrote back the stock it had read moments earlier:The order still exists and still expects those goods, but the stock is on sale again. That is an oversell — the precise outcome the guarded reservation exists to prevent — reached with no attacker, by a manager fixing a typo during a busy minute, and silently. It defeated the invariant the rest of the inventory work depends on.
The fix
The write path is split by intent, so each caller touches only what it owns:
UpdateDetailsAsyncAdjustStockAsyncArchiveAsyncAdjustStockAsyncdoes its arithmetic and both guards inside the statement, returning the resulting availability:That also fixes a second lost update in the same shape: two concurrent restocks each computed a new total from a stale row, so one vanished. As deltas applied by the database, both land.
Two details worth flagging
The handler keeps its own checks in front of the write. They're advisory — the statement enforces — but they name the reason accurately: a restock that would go negative is a different mistake from one that would strand a reservation. Collapsing them into one message was a regression, and the existing tests caught it.
The in-memory fake copies fields onto the stored row rather than replacing the object. Swapping it wholesale reproduces the defect in miniature and would have let these tests pass over a broken repository.
Also here
ClockSkewset to 30 seconds. The default 5 minutes quietly turned a 15-minute access token into a 20-minute one.Tests
Six integration tests against real PostgreSQL: an edit during a checkout keeps the reservation, an adjustment counts stock reserved in the gap, two adjustments both land, stock cannot go below reserved or below zero, and an archived widget refuses one.
-warnaserror;dotnet formatcleanGenerated by Claude Code