Apply UpdateQuantity to the live maker state (#115) by joaquinbejar · Pull Request #128 · joaquinbejar/PriceLevel

joaquinbejar · 2026-07-14T11:27:28Z

Summary

UpdateQuantity derived the replacement order and its priority policy from a
stale pre-read, so a concurrent match could execute quantity that the update
then silently restored — including hidden iceberg depth — with numerically
consistent counters describing resurrected liquidity, durably persisted by
snapshots. The read/derive/decide/replace sequence is now one atomic operation
against the live stored order.

Stack

Stack: orders: Reduce every supported order variant after a partial fill #118 → execution: Parse malformed UTF-8 without panicking #114 → execution: Validate MatchResult invariants during decoding #116 → price-level: Reject quantity overflow before mutating level state #111 → price-level: Reject duplicate order IDs atomically #113 → price-level: Enforce level admission and trade topology invariants #120 → price-level: Resequence quantity increases without exposing an absent order #119 → price-level: Apply UpdateQuantity to the live maker state #115 → price-level: Make execution-statistics failures non-partial and observable #117 → price-level: Make PostOnly and FOK decisions atomic with the sweep #112 (this is 8/10)
Base branch: stack/7-119-atomic-reseq
Stacked on: Resequence quantity increases without vacating the id (#119) #127 — the diff vs main is cumulative; review against the base branch.

Changes

OrderQueue::update_entry(order_id, decide) + UpdateDecision enum
(mirrors the match_front decision-closure precedent): decide runs
against the live stored order under the per-entry lock; its decision
(in-place swap or inline tail re-sequence) commits in the same critical
section.
The level closure resizes from the live order (new_quantity → live
visible tranche, hidden preserved as live — resurrection structurally
impossible), picks increase-vs-decrease from the live total, and reserves
the level-counter delta with fetch_update(checked_add) before the
queue commits — an overflowing delta rejects the update with the level
untouched (closes the fetch_add(new − old) wrap flagged in Reject quantity overflow before mutating level state (#111) #124's audit).
update_in_place and resequence_to_tail subsumed and removed
(pub(crate) — no public impact). UpdatePriceAndQuantity's same-price
branch routes through the same primitive.

Technical decisions

concurrency-auditor: lock-free safe, commit safe, 0 P1/P2 — single-lock
design cannot deadlock (only one lock exists; atomics/SkipMap are
lock-free), the production closure is panic-free with all fallible work
before the first counter write, and the price-level: Resequence quantity increases without exposing an absent order #119 orphan-free re-key argument
transfers to the inlined body. Its test-adequacy finding (the consistency
helper compared snapshot folds — consistent by construction) is fixed: the
helper now also asserts the live advisory counters converge to the
queue sums when quiescent. architect's #[must_use] and id-guard
(debug_assert) notes applied.

Performance

Update-heavy bench (Update Orders/update_quantity, 50 upsizes/iter):
24.12 µs [24.05, 24.19] vs 24.97 µs [24.89, 25.04] base — ~3% faster
(one entry-lock acquisition instead of a pre-read plus a separate commit).

Testing

Barrier race tests (1000 iterations each): competing same-id updates
(one winner, counters == queue), decrease-vs-cancel (never resurrected),
update-vs-match on replenishing iceberg (hidden never exceeds pre-race
value, counters == queue == snapshot)
Deterministic level-counter overflow rejection (old fetch_add would
have wrapped and succeeded)
Live-derivation sanity: resize after a partial fill keeps the live
hidden, never the pre-fill value
Consistency helper strengthened to assert live atomics, not just
snapshot folds
Pre-push checks + examples clean locally

489 tests passed; 0 failed (470 lib + 9 + 1 + 9 doctests).

Checklist

Code follows rules/global_rules.md and CLAUDE.md
No .unwrap() / .expect() / unchecked indexing in production code
Checked arithmetic — validate-before-mutate on the counter delta
Non-SeqCst orderings justified (advisory counters; happens-before on
the entry lock)
No new production unsafe, no new dependencies
No public surface change — no migration / README edit needed

Closes #115

Blocks: Make execution-statistics failures non-partial and observable (#117) #129

codecov-commenter · 2026-07-14T11:31:35Z

Codecov Report

❌ Patch coverage is 84.61538% with 16 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/price_level/level.rs	84.48%	9 Missing ⚠️
src/price_level/order_queue.rs	84.78%	7 Missing ⚠️

Files with missing lines	Coverage Δ
src/price_level/order_queue.rs	`92.26% <84.78%> (-2.64%)`	⬇️
src/price_level/level.rs	`90.03% <84.48%> (-1.59%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joaquinbejar

The live entry decision fixes the stale quantity and priority race from #127, and the focused #115 tests pass, but the new counter decrease can wrap during a replenish race and the resurrection stress test can pass without exercising that interleaving. The inherited index rekey and destructive pop Critical findings already filed on #127 also remain unresolved. The intended effective verdict is REQUEST_CHANGES, submitted as COMMENT because the active account owns this PR.

joaquinbejar

The live maker derivation remains correct, but the existing counter underflow and race coverage warnings plus the inherited queue rekey and pop blockers remain unresolved. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The live maker derivation resolves the stale priority decision, but the existing counter underflow race and weak interleaving test remain, along with the inherited index rekey and destructive pop blockers. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The current head fixes live maker derivation, but the existing counter underflow and race coverage warnings remain, along with inherited queue rekey and destructive pop blockers. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The current head fixes live maker derivation and stale priority selection, but the existing counter underflow race and ineffective interleaving test remain, together with the inherited index rekey and destructive pop blockers. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

Deriving the update from the live maker fixes the stale priority decision, but the inherited rekey and pop races remain, the unchecked counter decrease can wrap during replenishment, and the race test does not force that interleaving. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

The quantity-increase branch demoted via remove(id) + push(new): during that gap the id was absent from the queue map, so a concurrent cancel reported no removal while the update re-inserted the maker (lost cancel / resurrection), a concurrent same-id admission could win try_push and then be blindly overwritten (the P1 flagged in the #113 audit), and the transient absence was the second side-topology race window documented in #120. match_front also never verified that the index-selected sequence still owned the live map entry, so a stale front selection could act on a just-demoted maker. New OrderQueue::resequence_to_tail extends the ReplaceAtTail shape: under one continuous hold of the DashMap entry lock it mints a tail sequence, swaps the stored (seq, order) pair in place, and re-keys the index -- the id never leaves the map. A concurrent cancel now either fully precedes (update reports not-found) or fully follows (it removes the re-sequenced order); a same-id admission is always rejected as DuplicateOrderId. match_front gains a stale-seq guard: if the stored sequence no longer equals the index-selected one, the stale key is dropped and the scan retries -- sequences are monotonic and never reused, so the removal is provably safe. Demotion semantics unchanged (fresh tail seq, original timestamp; #109 tests untouched). BREAKING: OrderQueue::push (blind overwrite) has no production caller left and is retired to #[cfg(test)] -- admission uses try_push, demotion uses resequence_to_tail. Migration guide + README updated; the now-closed #113/#120 doc caveats are retired. Barrier race tests (1500 iterations each): upsize-vs-cancel never loses a cancel or resurrects; upsize-vs-duplicate-admission is always rejected with no counter drift (fails 3/3 under the old remove+push); concurrent upsize+match stress drains consistently. Perf: update-heavy bench 26.0us vs 25.7us median -- neutral within noise; the win is correctness. Closes #119

- resequence_to_tail and the match sweep's ReplaceAtTail now publish the new index key BEFORE removing the old one, so a continuously resident maker is never transiently absent from the ordered index and a concurrent front scan can never report Empty past resting liquidity; the transient two-key window is discarded on selection by the stale-front guard (the stored sequence is already the new one) - pop_entry validates, under the map entry, that the stored sequence equals the popped index key; a demoted maker's stale old key is dropped and the scan retries, so the destructive pop can neither return a just-demoted maker ahead of older makers nor strand its new key - the allocation-free claim on the re-key path is narrowed: the SkipMap insert allocates the new index node Stress + deterministic tests: 200k front scans against a continuous demoter never observe Empty; a demoted maker drains last and exactly once under a racing pop; map/index end empty and consistent.

joaquinbejar

The live maker derivation fixes stale priority selection, but the existing counter underflow race and ineffective interleaving test remain, together with the inherited index rekey and destructive pop blockers. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

UpdateQuantity derived the replacement and its priority policy from a stale pre-read: a concurrent match could consume or replenish the maker between the read and the swap, so the stale replacement resurrected already-executed visible or hidden quantity (iceberg hidden depth included), and the stale total could pick the wrong priority branch. Counters stayed numerically consistent while describing resurrected liquidity, and a snapshot persisted the corruption durably. New OrderQueue::update_entry runs the decide closure against the LIVE stored order under the per-entry lock and commits its UpdateDecision (in-place swap, or inline tail re-sequence -- a separate call would double-lock the shard) in the same critical section. The level closure resizes from the live order (new_quantity applies to the live visible tranche, hidden preserved as live -- resurrection is structurally impossible), decides increase-vs-decrease from the live total, and reserves the level-counter delta with fetch_update(checked_add) BEFORE the queue commits: an update whose delta would overflow a level counter is rejected with InvalidOperation leaving maker, position, and counters untouched. That closes the fetch_add(new - old) wrap flagged in the issue-111 audit. update_in_place and resequence_to_tail are subsumed and removed. Race tests (Barrier, 1000 iterations each): competing same-id updates leave exactly one winner with consistent counters; decrease-vs-cancel never resurrects; update-vs-match on a replenishing iceberg never raises hidden above its pre-race value; plus a deterministic level-counter overflow rejection and a live-derivation sanity case. The shared consistency helper now also asserts the LIVE advisory counters converge to the queue sums when quiescent (previously it only compared snapshot folds, which are consistent by construction). Perf: update-heavy bench 24.12us vs 24.97us median -- ~3% faster (one entry-lock acquisition instead of a pre-read plus a separate commit). Closes #115

joaquinbejar

The shared queue fixes resolve the inherited rekey and destructive pop blockers, and deriving the update from the live maker fixes stale priority selection. The counter decrease can still underflow during a replenish race, and the stress test still does not force that interleaving. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

- the match sweep's replenish branch now publishes its level-counter transition inside the locked decision closure -- one checked fetch_update applying the net visible delta plus the hidden draw -- so the counters can never transiently lag a committed replenishment and a concurrent UpdateQuantity decrease can no longer underflow; the post-lock sweep body skips the already-committed step (flag threaded through StepData) and the update path's decrease reserve gains a checked_sub defense documented as unreachable once the structural ordering holds - the old order Arc is moved out with mem::replace in update_entry and match_front's in-place / re-key arms and dropped only after the shard guard releases, so a last-reference deallocation never runs inside the critical section - the update-vs-replenish race test now asserts strictly non-increasing hidden depth while the maker rests (the pre-#115 stale write-back fails it), guarantees matcher progress, and runs the drain across the whole race window

joaquinbejar · 2026-07-14T15:59:18Z

Thanks. All three findings closed in a97a3f9 (entry-locked replenish counters plus checked decrease, deferred Arc drops, and the strict monotonic race test); the inherited re-key and pop findings were already closed by the #127 fix now in this PR's base.

joaquinbejar added bug Something isn't working price-level Engine: level/order_queue/snapshot/statistics + execution results labels Jul 14, 2026

joaquinbejar self-assigned this Jul 14, 2026

joaquinbejar mentioned this pull request Jul 14, 2026

Resequence quantity increases without vacating the id (#119) #127

Merged

9 tasks