Resequence quantity increases without vacating the id (#119) by joaquinbejar · Pull Request #127 · joaquinbejar/PriceLevel

joaquinbejar · 2026-07-14T10:58:21Z

Summary

The quantity-increase demotion did remove(id) + push(new): during that gap
a concurrent cancel was lost while the update resurrected the maker, a
concurrent same-id admission could be blindly overwritten (the P1 flagged in
#125's concurrency audit), and the transient absence was the second
side-topology race window documented in #126. Demotion is now an atomic
in-place re-sequencing — the id never leaves the map.

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 7/10)
Base branch: stack/6-120-topology-invariants
Stacked on: Enforce level admission and trade topology invariants (#120) #126 — the diff vs main is cumulative; review against the base branch.

Changes

OrderQueue::resequence_to_tail: under one continuous hold of the DashMap
entry lock — mint tail seq, swap the stored (seq, order) pair in place,
re-key the index. Allocation-free beyond the caller's Arc.
update_order's increase branch uses it; counter deltas keyed off the
returned replaced order exactly as before. Demotion semantics unchanged
(fresh tail seq, original timestamp — the Preserve queue-priority demotion across snapshot restore #109/Preserve queue-priority demotion across snapshot restore (#109) #110 tests still pass).
match_front stale-seq guard: if the stored seq no longer equals the
index-selected seq (concurrent demotion in the gap), drop the stale key and
retry — sequences are monotonic and never reused, so the removal is provably
safe. A demoted maker can no longer be consumed via its stale front position.
BREAKING: OrderQueue::push (blind overwrite) retired to
#[cfg(test)] — no production caller remains. Migration guide + README
updated; the now-closed price-level: Reject duplicate order IDs atomically #113/price-level: Enforce level admission and trade topology invariants #120 doc caveats retired.

Technical decisions

concurrency-auditor verdict: lock-free safe, commit safe, 0 P1/P2 —
walked reseq-vs-cancel, reseq-vs-match, reseq-vs-reseq, and
reseq-vs-snapshot interleavings; the cancel-removes-old-key orphan is
unreachable because the map swap and both index ops happen under one held
entry lock. Two doc-precision notes applied (the index re-key still has a
transient two-op window — a missed fill at worst, same as ReplaceAtTail
has always had and strictly narrower than the old remove+push map absence;
and the guard-retry liveness caveat).
This closes: the lost-cancel/resurrection hazard, the Reject duplicate order ids atomically (#113) #125-audit P1
(duplicate admission through the upsize window — now always rejected), the
Enforce level admission and trade topology invariants (#120) #126 second topology window, and the stale-front FIFO hazard.

Performance

Update-heavy bench (Update Orders/update_quantity, 50 upsizes/iter):
26.02 µs [25.88, 26.18] new vs 25.72 µs [25.65, 25.78] old — neutral within
noise (~1%); the queue op is a small fraction of the full update_order path.
The change is a correctness fix, not a throughput claim.

Public API impact

OrderQueue::push removed from the public surface (breaking) — migration
guide + README regenerated. resequence_to_tail is pub(crate).

Testing

Barrier race tests, 1500 iterations each: upsize-vs-cancel (no lost
cancel, no resurrection, counters == queue), upsize-vs-duplicate-admission
(always DuplicateOrderId, no drift — fails 3/3 under the old
remove+push), concurrent upsize+match stress (consistent drain, no
phantom consumption)
Preserve queue-priority demotion across snapshot restore #109 upsize demotion + snapshot round-trip tests unchanged and passing
Pre-push checks + examples clean locally

484 tests passed; 0 failed (465 lib + 9 + 1 + 9 doctests).

Checklist

Code follows rules/global_rules.md and CLAUDE.md
No .unwrap() / .expect() / unchecked indexing in production code
Non-SeqCst orderings justified (next_seq Relaxed: only
uniqueness/monotonicity ride on it; happens-before carried by the entry lock)
Allocation-free mutation path (auditor-verified)
No new production unsafe, no new dependencies
Migration guide + README updated

Closes #119

Blocks: Apply UpdateQuantity to the live maker state (#115) #128

codecov-commenter · 2026-07-14T11:03:12Z

Codecov Report

❌ Patch coverage is 88.75740% with 19 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/price_level/level.rs	87.05%	18 Missing ⚠️
src/price_level/order_queue.rs	96.66%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/price_level/order_queue.rs	`94.91% <96.66%> (+0.83%)`	⬆️
src/price_level/level.rs	`91.80% <87.05%> (-0.45%)`	⬇️

🚀 New features to boost your workflow:

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

joaquinbejar

The map residency change closes the cancel and duplicate admission window, and the migration docs are in sync, but the two step index rekey still permits a missed fill and the public destructive pop can return a demoted maker out of FIFO order. The intended effective verdict is REQUEST_CHANGES.

joaquinbejar

The existing review covers the re-key and pop races. One additional FIFO violation remains because concurrent resizes choose priority from a stale quantity. The intended effective verdict remains REQUEST_CHANGES, submitted as COMMENT because the active account owns this PR.

joaquinbejar

The rebased head is patch equivalent to the previously reviewed change, and the existing non linearizable rekey, sequence blind destructive pop, and stale priority decision findings remain unresolved. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The current head still has the existing non linearizable index rekey, sequence blind destructive pop, and stale priority decision findings. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The current head closes the map vacancy window, but the existing non linearizable index rekey, sequence blind destructive pop, and stale priority decision findings remain unresolved. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

add_order accepted any order without checking that its price equals the level price or that all makers share one side. A wrong-price maker traded at PriceLevel::price rather than its own stored price, and mixed maker sides produced contradictory taker sides across a single MatchResult (the taker side is derived per-maker). The maker != taker guard was a debug_assert, so release builds could emit self-trades. Admission now validates topology before any counter reservation: a mismatching price or an incompatible side is rejected with InvalidOperation and the level is left byte-identical. The level side is derived from the resting orders (first admitted maker defines it; a drained level accepts either side again) -- a correctness invariant on single-logical-writer admission, with both residual race windows documented (empty-level admission race, and the upsize remove+push transient that #119's in-place re-sequencing closes). Self-trades are now structurally impossible in every build profile: a front maker whose id equals the taker id is parked like a set-aside maker (no trade, no counter movement, makers behind it still match in FIFO). This is order-id identity -- an order can never match itself; account-level STP via user_id remains the composing book's job. BREAKING: matchable_quantity takes the taker id so the fill-or-kill dry run applies the identical skip and can never diverge from the sweep (migration guide updated, README regenerated). from_snapshot validates the same price/side topology; the infallible From<&PriceLevelSnapshot> is documented as the trusted-input path that skips validation. Closes #120

joaquinbejar

The current head closes the map vacancy window, but the existing non linearizable index rekey, sequence blind destructive pop, and stale priority decision remain unresolved. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The in place update removes the map absence window, but rekeying still creates an index absence window, pop can consume a demoted maker through a stale sequence, and the priority decision uses an unlocked pre read. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

- side and order count now live in one packed atomic word: admission establishes the side / joins / rejects in a single CAS, and removal decrements and un-pins in the same CAS when the count reaches zero, so the two races derive-from-queue left open (opposite-side admissions into an empty level; an opposite-side admission racing an upsize's transient queue gap) are closed by construction -- and every admission drops the per-call queue iteration, cutting add_order latency ~39% in the admission bench - a topology epoch bumps on every pin / un-pin and snapshot() retries its materialization if the epoch moves, so a checksummed snapshot can never capture a torn old-side/new-side view across a valid drain-then-re-admit transition (single-side backstop bounds the retry) - a taker whose id already rests at the level is now rejected terminally before any sweep for every TIF -- zero trades, level byte-identical -- per the issue's no-mutation self-match contract; the in-sweep skip remains as defense-in-depth for admissions racing the sweep - the set-aside bookkeeping insert moved out from under the shard lock (no allocation under the entry lock; no eager pre-reserve either -- the common zero-set-aside match stays allocation-free)

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 in place re-sequence closes the map vacancy window, but the existing non linearizable index rekey, sequence blind destructive pop, and stale priority decision findings remain unresolved. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

joaquinbejar

The new key first rekey and sequence validated destructive pop resolve the two queue race findings. The quantity update still chooses increase versus decrease from an unlocked pre read, so the remaining FIFO priority Critical is unresolved. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

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

Thanks. Re-key is new-before-old at every site, pop_entry validates sequences, and the allocation claim is narrowed (e4cfe70); the stale pre-read decision is closed by #128 (the #115 rewrite) immediately above in this stack.

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

Enforce level admission and trade topology invariants (#120) #126

Merged

10 tasks