Cart service has no atomicity guarantees — concurrent AddItem calls lose updates, and checkout's EmptyCart can silently discard items added during checkout

[bhuvan-somisetty]

Version: verified against commit 6c4b663a8a2c243f60f6b1a7d93e52467b3124f1 on main (2026-08-10).

Summary

ValkeyCartStore stores each user's cart as a single serialized blob under one Redis/Valkey hash field and mutates it with an unprotected read → modify → write sequence (no WATCH/MULTI, no Lua script, no per-field atomic ops, no optimistic-concurrency/version check). Because of this, concurrent writes to the same cart are not serialized in any way, which produces two distinct, reproducible failure modes:

1. Lost updates on concurrent AddItem — two "add to cart" calls for the same user that overlap in time silently clobber each other; one increment is lost and the user is undercharged/under-fulfilled with no error surfaced anywhere (not in the response, not in a span, not in a log).
2. Silent item loss during checkout — checkoutservice.PlaceOrder calls cartservice.GetCart and then, after building the order, calls cartservice.EmptyCart (src/checkout/main.go:520 and :528). If an AddItem for that same user lands in the window between those two calls (e.g. a second browser tab, a retried request, or the load generator), EmptyCartAsync unconditionally overwrites the hash field with an empty cart, silently discarding an item that was never included in the placed order and is never billed — a silent data/revenue-consistency bug with no telemetry signal indicating anything went wrong.

Both symptoms trace back to the same root cause in src/cart/src/cartstore/ValkeyCartStore.cs, so this is one subsystem-level defect, not two unrelated ones.

Reproduction steps

A. Lost update on concurrent AddItem

1. docker compose up (or run cartservice against Valkey directly).
2. Fire two concurrent AddItem gRPC calls for the same user_id/product_id (e.g. two grpcurl invocations launched in parallel, or two goroutines calling the generated client):

   grpcurl -plaintext -d '{"user_id":"race-user","item":{"product_id":"OLJCESPC7Z","quantity":1}}' localhost:5070 oteldemo.CartService/AddItem &
   grpcurl -plaintext -d '{"user_id":"race-user","item":{"product_id":"OLJCESPC7Z","quantity":1}}' localhost:5070 oteldemo.CartService/AddItem &
   wait
   

3. Call GetCart for race-user.
4. Expected: quantity 2. Actual (intermittently, under real network/scheduling jitter): quantity 1 — one of the two increments is lost because both requests read the same pre-update blob before either writes it back (ValkeyCartStore.cs:140-165).

B. Silent item loss racing with checkout

1. Add an item to a cart for user_id=checkout-race-user.
2. Call checkoutservice.PlaceOrder for that user, but pause/delay the request (or add a temporary sleep) between its GetCart call and its EmptyCart call (src/checkout/main.go:520-528) to simulate realistic latency under load.
3. During that window, call cartservice.AddItem again for the same user with a different product.
4. Inspect the placed order (it will not contain the item added in step 3 — it wasn't in the cart when GetCart ran) and then call GetCart again.
5. Expected: the item added in step 3 should either be in a fresh cart or surfaced as a conflict. Actual: the cart is empty — EmptyCartAsync (ValkeyCartStore.cs:178-194) does an unconditional HashSetAsync overwrite with no check against the state PlaceOrder originally read, so the item vanishes: not ordered, not billed, not in the cart, and nothing logs or traces this as an error.

Root cause

src/cart/src/cartstore/ValkeyCartStore.cs:

• AddItemAsync (lines 127-176): db.HashGetAsync → deserialize → mutate in memory → db.HashSetAsync. Two concurrent calls both read the same starting state and the second write wins, discarding the first.
• EmptyCartAsync (lines 178-194): unconditional HashSetAsync with an empty cart blob — never conditioned on the version/state that the caller (e.g. checkout) originally observed.
• GetCartAsync (lines 196-227): plain read with no version/ETag returned to callers, so callers have no way to detect that the cart changed underneath them.

There is no WATCH/MULTI-EXEC transaction, no Lua script, and no per-item atomic field (e.g. HINCRBY on a productId field) anywhere in this file — the entire cart is treated as one opaque blob guarded only by a connection-level lock (_locker) that protects Redis connection setup, not cart data mutations.

Compounding this: cart mutation logic currently has zero effective test coverage. All three tests in src/cart/tests/CartServiceTests.cs (GetItem_NoAddItemBefore_EmptyCartReturned, AddItem_ItemExists_Updated, AddItem_New_Inserted) are marked [Fact(Skip = ...)] referencing PR #746 and have been disabled since that PR merged, so there is no regression safety net over AddItemAsync/EmptyCartAsync/GetCartAsync today, and no concurrency test exists for this path at all.

Impact

• Data/revenue consistency: items can be silently dropped from an order during checkout under realistic concurrent load (double-tab, client retry, load generator) — the worst class of bug for a checkout flow, since it fails silently rather than loudly.
• Correctness under concurrency: AddItem is not idempotent/atomic, so quantities are simply wrong under contention, with no error returned to the caller and no exception/log/span marking the loss.
• Observability credibility: this is a project whose entire purpose is to demonstrate correct, observable microservice behavior. A silent, un-instrumented data-loss bug in the reference checkout flow undermines that goal — anyone using this demo to practice diagnosing production issues will find no trace, log, or metric pointing at the actual defect.
• No regression protection: because the existing cart tests are skipped, this class of bug can be reintroduced or worsened by future changes without CI ever catching it.

Affected files/components

• src/cart/src/cartstore/ValkeyCartStore.cs — core fix: replace blob read-modify-write with atomic operations (e.g. per-product hash fields with HashIncrementAsync, or a Lua script / WATCH+MULTI+EXEC transaction with retry-on-conflict for the whole-cart representation).
• src/cart/src/cartstore/ICartStore.cs — may need a version/ETag-aware signature (e.g. optional expected-version parameter or a compare-and-swap style API) so callers like checkout can detect concurrent modification instead of blindly overwriting.
• src/cart/src/services/CartService.cs — surface conflict/retry behavior (e.g. return a specific gRPC status) if a CAS-based store reports contention.
• src/checkout/main.go (PlaceOrder, lines ~520-528) — must not unconditionally call EmptyCart after GetCart without accounting for concurrent modification; needs either a conditional/versioned empty-cart call or an explicit "remove exactly these items" operation instead of "clear everything."
• src/cart/tests/CartServiceTests.cs — re-enable the disabled tests (fixing whatever made them skip in Avoid calling obsolete methods #746) and add concurrency-focused tests reproducing both failure modes above.
• Documentation: any architecture/README notes describing the cart storage contract should note the atomicity guarantees once fixed.

Acceptance criteria

1. Two concurrent AddItem calls for the same user/product each register their full increment — no lost updates under concurrent load (verified by an automated concurrency test, e.g. N parallel AddItem calls resulting in the correct summed quantity).
2. EmptyCart (or whatever replaces the checkout-side cart-clearing call) cannot silently discard an item that was added after the state PlaceOrder/the caller last observed; the system either preserves the new item or surfaces an explicit, observable conflict (span exception, error status, or documented compensating behavior) — no silent loss.
3. The three currently-skipped tests in CartServiceTests.cs are re-enabled and passing, plus new tests cover the concurrent-AddItem and checkout/AddItem-race scenarios described above.
4. No behavioral regression to the existing single-writer cart flow (add/get/empty) used throughout the rest of the demo (frontend, load generator, checkout, recommendation).

High-level implementation plan

1. Redesign the Valkey cart schema in ValkeyCartStore.cs to make mutations atomic — most direct option: store each cart item as its own hash field (HINCRBY for quantity) instead of one serialized blob, eliminating the read-modify-write pattern for AddItem entirely; alternatively, keep the blob but wrap read+modify+write in a WATCH/MULTI/EXEC transaction with bounded retry on conflict, or a Lua script for atomicity.
2. Update EmptyCartAsync/checkout's clear-cart call so it can't blindly wipe state that has changed since it was last read — e.g. accept the expected item set/version and only clear what was actually ordered, or fail/report conflict rather than silently overwrite.
3. Update ICartStore/CartService gRPC surface as needed to propagate conflict signals, keeping backward-compatible behavior for the common non-concurrent path.
4. Update checkoutservice.PlaceOrder in src/checkout/main.go to use the new conflict-aware clear operation.
5. Re-enable and fix the three skipped tests in CartServiceTests.cs; add targeted concurrency tests for both reproduction scenarios above.
6. Verify manually with the reproduction steps in this issue, and note the fix in any relevant architecture docs describing cart storage guarantees.

[soumadipcodeEnthusiast]

Nice writeup, really thorough repro steps. Are you planning on submitting the fix yourself or is this up for grabs? If you've got the core atomicity fix covered, happy to help out with re-enabling those skipped tests in CartServiceTests.cs and adding the concurrency test cases — seems like a good chunk to split off.

[bhuvan-somisetty]

Thanks! Yeah, I'm working on the fix myself right now - have the atomicity/CAS fix for AddItemAsync and the checkout race in progress, and re-enabling the skipped tests plus concurrency test cases as part of the same PR. Appreciate the offer though, will tag you if I end up splitting anything off. Should have a PR up soon.