Enforce level admission and trade topology invariants (#120)

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "pricelevel"
-version = "0.8.5"
+version = "0.9.0"
 edition = "2024"
 authors = ["Joaquin Bejar <jb@taunais.com>"]
 description = "A high-performance, lock-free price level implementation for limit order books in Rust. This library provides the building blocks for creating efficient trading systems with support for multiple order types and concurrent access patterns."

README.md

@@ -513,6 +513,74 @@ binaries may prefer `.expect(...)`); the returned `Arc` is unchanged on
 success. Admissions that stay within `u64` (all normal use) behave exactly
 as before.
 
+`add_order` also now **rejects a duplicate id**: publishing is an
+insert-if-absent, so reusing the id of an order already resting at the level
+returns the new [`PriceLevelError::DuplicateOrderId`] variant (again leaving
+the level unchanged) instead of overwriting the live order and leaving the
+id-keyed map and the ordered index disagreeing. Snapshot restore
+([`PriceLevel::from_snapshot`] and the JSON / package forms) likewise
+rejects an orders vector that repeats an id rather than silently
+overwriting. Submitting genuinely distinct ids (all normal use) is
+unaffected.
+
+### Migration Guide (v0.9 — duplicate-id safety on restore + queue surface)
+
+Three intentional breaking changes remove infallible / overwriting paths
+that could desync a level's counters from its queue:
+
+- **`impl From<&PriceLevelSnapshot> for PriceLevel` is removed; use
+  [`TryFrom`].** The old `From` swallowed aggregate-overflow errors and built
+  the queue keep-first, so a snapshot repeating an id restored counters
+  computed over every copy while the queue kept one. Replace
+  `PriceLevel::from(&snapshot)` / `let lvl: PriceLevel = (&snapshot).into();`
+  with `PriceLevel::try_from(&snapshot)?` (or `.expect(...)` in tests). It
+  delegates to [`PriceLevel::from_snapshot`], returning
+  [`PriceLevelError::DuplicateOrderId`] on a repeated id and the
+  per-order / level aggregate-overflow errors instead of hiding them.
+- **`OrderQueue::push` is now `pub(crate)`.** Unconditional overwriting
+  publication is never safe for an external caller (reusing a live id would
+  silently replace the resting order and strand its old index entry).
+  Admission goes through `add_order` (or, at the queue layer, the
+  insert-if-absent `try_push`); there is no public overwriting insert.
+- **`OrderQueue::from_vec` is now `pub(crate)`.** It is a keep-first
+  constructor that drops duplicates silently; the public restore path is
+  [`PriceLevel::from_snapshot`], which rejects them.
+### Migration Guide (level topology invariants — breaking)
+
+A [`PriceLevel`] now enforces that every resting order sits at the level's
+price and shares a single side (the first admitted maker pins the side; a
+fully drained level accepts either side again). [`PriceLevel::add_order`]
+returns [`PriceLevelError::InvalidOperation`] for an order whose price does
+not match the level, or whose side is incompatible with the resting side,
+and [`PriceLevel::from_snapshot`] rejects a snapshot that violates either
+(previously such orders were admitted, trading at the level price rather
+than their own and producing contradictory taker sides in one
+[`MatchResult`]). Callers that composed a level from mixed-price or
+mixed-side orders must route each order to the correct level.
+
+Single-side coherence is a **correctness invariant**, not an
+eventually-consistent one like the advisory counters: it holds only when a
+given level's admissions arrive from a single logical writer (the composing
+order book routes each price to one admission path). The side is derived
+from the live queue, so under genuinely concurrent multi-writer admission a
+narrow race — an opposite side slipping into a momentarily empty level — can
+still admit a mixed side; see the note on the [`PriceLevel`] type.
+
+[`PriceLevel::matchable_quantity`] gains a `taker_id` parameter:
+`matchable_quantity(incoming_quantity)` becomes
+`matchable_quantity(incoming_quantity, taker_id)`. A resting maker sharing
+the taker id is skipped (self-trade prevention), matching the sweep, so a
+fill-or-kill dry run and the real sweep agree. `match_order` applies the
+same **self-trade skip** deterministically in every build profile (it used
+to be a debug-only assertion): a resting maker whose id equals the taker's
+is skipped — no self-trade is emitted and the other makers still match.
+
+This self-trade guard is **order-id identity** — an order can never match
+itself. It is NOT account/owner-level self-trade prevention: two distinct
+order ids owned by the same `user_id` will still trade. Account-level STP is
+the responsibility of the order book composing these levels, which owns the
+account relationships a single price level does not.
+
 
  ## Setup Instructions
 

examples/src/bin/contention_test.rs

@@ -100,12 +100,15 @@ fn test_read_write_ratio() {
                         // Write operation
                         match local_counter % 3 {
                             0 => {
-                                // Add a new order
+                                // Add a new order. Ids overlap across threads
+                                // once a counter exceeds 10000 (see the taker
+                                // comment below) and across ratio phases, so
+                                // since issue #113 a rejected duplicate is an
+                                // expected outcome here — deliberately ignored
+                                // to keep the write pressure going.
                                 let order_id = thread_id as u64 * 10000 + local_counter;
                                 let order = create_standard_order(order_id, 10000, 10);
-                                thread_price_level
-                                    .add_order(order)
-                                    .expect("add_order should succeed");
+                                let _ = thread_price_level.add_order(order);
                             }
                             1 => {
                                 // Match order. Takers live in a disjoint high id
@@ -304,11 +307,12 @@ fn test_hot_spot_contention() {
                     // Perform an operation based on iteration
                     match local_counter % 3 {
                         0 => {
-                            // Add a new order with same ID (this will likely fail, but creates contention)
+                            // Add a new order with same ID. Since issue #113 a
+                            // duplicate id is rejected with DuplicateOrderId —
+                            // an expected outcome in this contention pattern,
+                            // so the result is deliberately ignored.
                             let order = create_standard_order(order_idx, 10000, 10);
-                            thread_price_level
-                                .add_order(order)
-                                .expect("add_order should succeed");
+                            let _ = thread_price_level.add_order(order);
                         }
                         1 => {
                             // Cancel an order

examples/src/bin/hft_simulation.rs

@@ -82,7 +82,10 @@ fn main() {
             let mut local_counter = 0;
             while thread_running.load(Ordering::Relaxed) {
                 // Generate a unique order ID
-                let order_id = (thread_id as u64) * 1_000_000 + local_counter;
+                // +1 offset keeps maker ids clear of the 0..initial_order_count
+                // seed range: admission now rejects a duplicate id (issue
+                // #113) instead of silently overwriting.
+                let order_id = (thread_id as u64 + 1) * 1_000_000 + local_counter;
 
                 // Create and add an order
                 let order_type = match local_counter % 5 {
@@ -135,8 +138,18 @@ fn main() {
 
             let mut local_counter = 0;
             while thread_running.load(Ordering::Relaxed) {
-                // Generate a unique taker order ID
-                let taker_id = Id::from_u64((thread_id as u64) * 1_000_000 + local_counter);
+                // Generate a unique taker order ID in a DISJOINT high range so a
+                // taker id can never collide with a resting maker id. Makers use
+                // `(thread_id + 1) * 1_000_000 + counter`, so taker thread 10's
+                // base (`10 * 1_000_000`) would otherwise land exactly on maker
+                // thread 9's base (`(9 + 1) * 1_000_000`). A taker sharing a
+                // resting maker's id is a self-fill — impossible for a real
+                // order and caught by match_order's debug-only self-fill
+                // assertion. Offsetting by `1 << 40` (~1.1e12, far above any
+                // reachable maker id) keeps the two id spaces apart, matching
+                // contention_test.
+                let taker_id =
+                    Id::from_u64((1u64 << 40) + (thread_id as u64) * 1_000_000 + local_counter);
 
                 // Match varying quantities
                 let quantity = (local_counter % 5) + 1; // Match 1-5 units

examples/src/bin/simple.rs

@@ -52,7 +52,10 @@ fn main() {
                     thread_barrier.wait(); // Wait for all threads to be ready
 
                     for i in 0..50 {
-                        let order_id = thread_id as u64 * 1000 + i;
+                        // Offset past the ids seeded by setup_initial_orders
+                        // (0..240): admission now rejects a duplicate id
+                        // (issue #113) instead of silently overwriting.
+                        let order_id = 10_000 + thread_id as u64 * 1000 + i;
                         let order = create_order(thread_id, order_id);
                         thread_price_level
                             .add_order(order)

src/errors/tests/types.rs

@@ -60,6 +60,7 @@ mod tests {
             PriceLevelError::InvalidFormat,
             PriceLevelError::UnknownOrderType("TestOrder".to_string()),
             PriceLevelError::MissingField("id".to_string()),
+            PriceLevelError::DuplicateOrderId("42".to_string()),
             PriceLevelError::InvalidFieldValue {
                 field: "side".to_string(),
                 value: "MIDDLE".to_string(),

src/errors/types.rs

@@ -45,6 +45,14 @@ pub enum PriceLevelError {
     /// The string parameter specifies which field is missing.
     MissingField(String),
 
+    /// Error indicating an order id already rests at the price level.
+    ///
+    /// Admission (or a duplicate-bearing restore) is rejected atomically rather
+    /// than overwriting the live order, which would leave the level's id-keyed
+    /// map and its ordered index disagreeing (two sequences for one id) and its
+    /// counters double-counted. The string parameter is the offending id.
+    DuplicateOrderId(String),
+
     /// Error indicating a field has an invalid value.
     ///
     /// This error occurs when a field's value is present but doesn't meet validation criteria.
@@ -96,6 +104,7 @@ impl Display for PriceLevelError {
                 write!(f, "Unknown order type: {order_type}")
             }
             PriceLevelError::MissingField(field) => write!(f, "Missing field: {field}"),
+            PriceLevelError::DuplicateOrderId(id) => write!(f, "Duplicate order id: {id}"),
             PriceLevelError::InvalidFieldValue { field, value } => {
                 write!(f, "Invalid value for field {field}: {value}")
             }
@@ -128,6 +137,7 @@ impl Debug for PriceLevelError {
                 write!(f, "Unknown order type: {order_type}")
             }
             PriceLevelError::MissingField(field) => write!(f, "Missing field: {field}"),
+            PriceLevelError::DuplicateOrderId(id) => write!(f, "Duplicate order id: {id}"),
             PriceLevelError::InvalidFieldValue { field, value } => {
                 write!(f, "Invalid value for field {field}: {value}")
             }

src/lib.rs

@@ -505,6 +505,74 @@
 //! success. Admissions that stay within `u64` (all normal use) behave exactly
 //! as before.
 //!
+//! `add_order` also now **rejects a duplicate id**: publishing is an
+//! insert-if-absent, so reusing the id of an order already resting at the level
+//! returns the new [`PriceLevelError::DuplicateOrderId`] variant (again leaving
+//! the level unchanged) instead of overwriting the live order and leaving the
+//! id-keyed map and the ordered index disagreeing. Snapshot restore
+//! ([`PriceLevel::from_snapshot`] and the JSON / package forms) likewise
+//! rejects an orders vector that repeats an id rather than silently
+//! overwriting. Submitting genuinely distinct ids (all normal use) is
+//! unaffected.
+//!
+//! ## Migration Guide (v0.9 — duplicate-id safety on restore + queue surface)
+//!
+//! Three intentional breaking changes remove infallible / overwriting paths
+//! that could desync a level's counters from its queue:
+//!
+//! - **`impl From<&PriceLevelSnapshot> for PriceLevel` is removed; use
+//!   [`TryFrom`].** The old `From` swallowed aggregate-overflow errors and built
+//!   the queue keep-first, so a snapshot repeating an id restored counters
+//!   computed over every copy while the queue kept one. Replace
+//!   `PriceLevel::from(&snapshot)` / `let lvl: PriceLevel = (&snapshot).into();`
+//!   with `PriceLevel::try_from(&snapshot)?` (or `.expect(...)` in tests). It
+//!   delegates to [`PriceLevel::from_snapshot`], returning
+//!   [`PriceLevelError::DuplicateOrderId`] on a repeated id and the
+//!   per-order / level aggregate-overflow errors instead of hiding them.
+//! - **`OrderQueue::push` is now `pub(crate)`.** Unconditional overwriting
+//!   publication is never safe for an external caller (reusing a live id would
+//!   silently replace the resting order and strand its old index entry).
+//!   Admission goes through `add_order` (or, at the queue layer, the
+//!   insert-if-absent `try_push`); there is no public overwriting insert.
+//! - **`OrderQueue::from_vec` is now `pub(crate)`.** It is a keep-first
+//!   constructor that drops duplicates silently; the public restore path is
+//!   [`PriceLevel::from_snapshot`], which rejects them.
+//! ## Migration Guide (level topology invariants — breaking)
+//!
+//! A [`PriceLevel`] now enforces that every resting order sits at the level's
+//! price and shares a single side (the first admitted maker pins the side; a
+//! fully drained level accepts either side again). [`PriceLevel::add_order`]
+//! returns [`PriceLevelError::InvalidOperation`] for an order whose price does
+//! not match the level, or whose side is incompatible with the resting side,
+//! and [`PriceLevel::from_snapshot`] rejects a snapshot that violates either
+//! (previously such orders were admitted, trading at the level price rather
+//! than their own and producing contradictory taker sides in one
+//! [`MatchResult`]). Callers that composed a level from mixed-price or
+//! mixed-side orders must route each order to the correct level.
+//!
+//! Single-side coherence is a **correctness invariant**, not an
+//! eventually-consistent one like the advisory counters: it holds only when a
+//! given level's admissions arrive from a single logical writer (the composing
+//! order book routes each price to one admission path). The side is derived
+//! from the live queue, so under genuinely concurrent multi-writer admission a
+//! narrow race — an opposite side slipping into a momentarily empty level — can
+//! still admit a mixed side; see the note on the [`PriceLevel`] type.
+//!
+//! [`PriceLevel::matchable_quantity`] gains a `taker_id` parameter:
+//! `matchable_quantity(incoming_quantity)` becomes
+//! `matchable_quantity(incoming_quantity, taker_id)`. A resting maker sharing
+//! the taker id is skipped (self-trade prevention), matching the sweep, so a
+//! fill-or-kill dry run and the real sweep agree. `match_order` applies the
+//! same **self-trade skip** deterministically in every build profile (it used
+//! to be a debug-only assertion): a resting maker whose id equals the taker's
+//! is skipped — no self-trade is emitted and the other makers still match.
+//!
+//! This self-trade guard is **order-id identity** — an order can never match
+//! itself. It is NOT account/owner-level self-trade prevention: two distinct
+//! order ids owned by the same `user_id` will still trade. Account-level STP is
+//! the responsibility of the order book composing these levels, which owns the
+//! account relationships a single price level does not.
+//!
 
 mod orders;
 mod price_level;