From f66abcd4e9ab864544fae0b18b7f56e64a972d3d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 22:44:32 +0000 Subject: [PATCH] fix: widen the order number and stop a cart quantity wrapping negative Two defects found reading the checkout path after the hardening pass. Order numbers collided at commerce scale. The number is WW-{date}-{suffix} and the suffix was six hex characters of the order's Guid - 24 bits, scoped to a single day. order_number carries a unique index, so a collision was never a data leak, but it was an INSERT that violated the constraint and rolled the whole placement back: a customer meeting a hard failure at checkout. Collisions arrive by the birthday bound, not when the space runs out, so this bites far earlier than 16.7 million. At a thousand orders in a day the chance of at least one collision is around three per cent; at five thousand it is a coin flip; at ten thousand it is near certain. The suffix is now ten characters - 40 bits - which stays under a rounding error past a million orders a day. The cost is four characters on a number people read out. The failure was at least in the safe order: TryPlaceAsync runs before the payment is charged, so a collision cost the customer an error rather than money. A test pins the width so it cannot be shortened again for tidiness. Cart quantity wrapped instead of clamping. AddCartItemHandler summed the existing line and the requested amount in int arithmetic before clamping to available stock. A quantity near int.MaxValue wrapped negative, Math.Min then chose the negative, and the shopper was told the item was out of stock - a misleading answer rather than a dangerous one, since nothing negative reached the cart. The sum is now widened to long before the clamp, so the clamp does the clamping and five available means five in the cart. 482 backend tests pass locally against PostgreSQL 16; dotnet format clean. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Carts/AddItem/AddCartItemHandler.cs | 6 +++- .../Checkout/PlaceOrder/OrderDraft.cs | 14 +++++++-- .../WidgetWorks.UnitTests/CartHandlerTests.cs | 29 +++++++++++++++++++ .../OrderStateMachineTests.cs | 20 +++++++++++-- 4 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs b/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs index 80841f9..8228506 100644 --- a/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs +++ b/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs @@ -24,7 +24,11 @@ public async Task> Handle(AddCartItemCommand command, Cancellat var cart = await ResolveCartAsync(command.CartId, command.UserId, ct); var existing = cart.Items.FirstOrDefault(i => i.WidgetId == command.WidgetId); - var desired = Math.Min((existing?.Quantity ?? 0) + command.Quantity, widget.QuantityAvailable); + // Widened to long before adding. In int arithmetic a quantity near int.MaxValue wraps + // negative, Math.Min then picks the negative, and the shopper is told the item is out of + // stock - a misleading answer to a bad request rather than a dangerous one, but the clamp + // should do the clamping. + var desired = (int)Math.Min((long)(existing?.Quantity ?? 0) + command.Quantity, widget.QuantityAvailable); if (desired <= 0) { return Result.Fail("This widget is out of stock."); diff --git a/src/WidgetWorks.Application/Checkout/PlaceOrder/OrderDraft.cs b/src/WidgetWorks.Application/Checkout/PlaceOrder/OrderDraft.cs index 538c162..43b48ee 100644 --- a/src/WidgetWorks.Application/Checkout/PlaceOrder/OrderDraft.cs +++ b/src/WidgetWorks.Application/Checkout/PlaceOrder/OrderDraft.cs @@ -59,7 +59,17 @@ public static Order Create( }; } - /// Human-quotable order number: WW-{date}-{6 chars}, e.g. WW-20260501-A1B2C3. + /// + /// Human-quotable order number: WW-{date}-{10 chars}, e.g. WW-20260501-A1B2C3D4E5. + /// + /// The suffix is the head of the order's own v4 Guid, so it is random rather than sequential - + /// an order number cannot be incremented to reach the next customer's. Its width is the part + /// that matters: order_number carries a unique index, so a collision is not a data leak but it + /// is a failed checkout, and collisions arrive by the birthday bound rather than when the space + /// runs out. Six characters is 24 bits, which is a coin flip at roughly five thousand orders in + /// a single day; ten characters is 40 bits, which stays under a rounding error past a million. + /// The cost of the extra four characters is four characters. + /// public static string NumberFor(DateTimeOffset now, Guid orderId) - => $"WW-{now:yyyyMMdd}-{orderId.ToString("N")[..6].ToUpperInvariant()}"; + => $"WW-{now:yyyyMMdd}-{orderId.ToString("N")[..10].ToUpperInvariant()}"; } diff --git a/tests/WidgetWorks.UnitTests/CartHandlerTests.cs b/tests/WidgetWorks.UnitTests/CartHandlerTests.cs index 6874e0e..8b50c62 100644 --- a/tests/WidgetWorks.UnitTests/CartHandlerTests.cs +++ b/tests/WidgetWorks.UnitTests/CartHandlerTests.cs @@ -184,4 +184,33 @@ public async Task Merge_never_absorbs_another_users_cart() Assert.Equal("Cart not found.", result.Error); Assert.NotNull(await carts.GetAsync(victimCart.Value!.Id, CancellationToken.None)); // untouched } + + [Fact] + public async Task An_absurd_quantity_clamps_to_stock_instead_of_wrapping_negative() + { + var widgets = new InMemoryWidgetRepository(); + var carts = new InMemoryCartRepository(); + var widgetId = Guid.NewGuid(); + widgets.Store[widgetId] = new Widget + { + Id = widgetId, + Sku = "WW-001", + Name = "Standard Widget Block Cobalt", + Price = 9.99m, + IsActive = true, + QuantityOnHand = 5, + QuantityReserved = 0, + }; + + var handler = new AddCartItemHandler(carts, widgets, new FakeTimeProvider( + new DateTimeOffset(2026, 1, 1, 0, 0, 0, TimeSpan.Zero))); + + var result = await handler.Handle( + new AddCartItemCommand(null, null, widgetId, int.MaxValue), CancellationToken.None); + + // In int arithmetic this wrapped negative and answered "out of stock". The clamp should + // clamp: five are available, so five is what lands in the cart. + Assert.True(result.IsSuccess); + Assert.Equal(5, result.Value!.Items.Single().Quantity); + } } diff --git a/tests/WidgetWorks.UnitTests/OrderStateMachineTests.cs b/tests/WidgetWorks.UnitTests/OrderStateMachineTests.cs index 5b45131..423e09f 100644 --- a/tests/WidgetWorks.UnitTests/OrderStateMachineTests.cs +++ b/tests/WidgetWorks.UnitTests/OrderStateMachineTests.cs @@ -216,11 +216,27 @@ public void The_quote_and_the_charge_are_the_same_calculation() [Fact] public void An_order_number_is_dated_and_short_enough_to_read_out() { - var id = Guid.Parse("a1b2c3d4-0000-0000-0000-000000000000"); + var id = Guid.Parse("a1b2c3d4-e5f6-0000-0000-000000000000"); var number = OrderDraft.NumberFor(new DateTimeOffset(2026, 5, 1, 0, 0, 0, TimeSpan.Zero), id); - Assert.Equal("WW-20260501-A1B2C3", number); + Assert.Equal("WW-20260501-A1B2C3D4E5", number); + } + + [Fact] + public void An_order_number_carries_enough_of_the_id_to_make_a_collision_negligible() + { + var number = OrderDraft.NumberFor( + new DateTimeOffset(2026, 5, 1, 0, 0, 0, TimeSpan.Zero), + Guid.NewGuid()); + + // order_number is uniquely indexed, so a collision is a failed checkout rather than a leak, + // and collisions arrive by the birthday bound. Ten hex characters is 40 bits; six was 24, + // which is a coin flip at about five thousand orders in one day. This pins the width so it + // cannot be shortened back for tidiness. + var suffix = number.Split('-')[2]; + Assert.Equal(10, suffix.Length); + Assert.Equal(suffix.ToUpperInvariant(), suffix); } [Fact]