Make execution-statistics failures non-partial and observable (#117) by joaquinbejar · Pull Request #129 · joaquinbejar/PriceLevel

joaquinbejar · 2026-07-14T11:55:00Z

Summary

record_execution mutated several independent atomics with fallible checked
additions: a later overflow left the earlier counters advanced (partial
contribution), and match_order discarded the returned error — a trade
succeeded with silently inconsistent statistics. Statistics recording is now
all-or-nothing in the committed state, and a dropped execution is observable.

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 9/10)
Base branch: stack/8-115-live-update
Stacked on: Apply UpdateQuantity to the live maker state (#115) #128 — the diff vs main is cumulative; review against the base branch.

Changes

Additive aggregates commit via fetch_update(checked_add); on any later
failure the already-committed prefix rolls back with fetch_sub of exactly
this call's contribution (the price-level: Reject quantity overflow before mutating level state #111/price-level: Reject duplicate order IDs atomically #113 call-backed argument, documented as
contingent on reset()'s new quiescence contract). last_execution_time
stores only after every additive commit succeeds.
match_order no longer discards the error: WARN log (the match is not
aborted — the committed trade is never unwound) + new sticky
stats_degraded flag on PriceLevelStatistics, queryable and carried
through Clone / Display / FromStr / serde.
Snapshot compatibility preserved: the flag serializes only when true,
so a pre-price-level: Make execution-statistics failures non-partial and observable #117 v2 package re-serializes byte-identically and its SHA-256
still validates — old snapshots restore unchanged (guarded by an old-format
compatibility test). No snapshot version bump needed.
record_order_added / record_order_removed audited: single-atomic, no
partiality possible.

Technical decisions

Validate-then-commit across 8 independent atomics is not linearizable
without a lock; CAS-per-counter + prefix rollback keeps the path lock-free
and matches the crate's rollback precedent. The transient prefix window
(visible before rollback) is documented honestly — the guarantee is on the
committed state.
architect caught the HIGH checksum break (unconditional 9th field would
have failed validate() on every pre-price-level: Make execution-statistics failures non-partial and observable #117 package) — fixed with
skip-when-false serialization. concurrency-auditor (0 P1) flagged the
reset()-vs-rollback wrap (P2, unreachable today) — closed by contract:
reset() documented quiescent-only. All LOW items applied (WARN level,
Barrier in the concurrent test, config-specific exactly-K note, torn-read
caveats on the average accessors).

Public API impact

New additive accessor PriceLevelStatistics::stats_degraded(); serde field
emitted only when true. No signature changes, lib.rs untouched.

Testing

Forced overflow per aggregate: no aggregate advances, flag set, Err
Barrier-controlled concurrent boundary: exactly K symmetric-headroom
successes, quantity_executed / value_executed in lockstep
match_order integration: trade + MatchResult intact on stats
overflow, flag observable and round-tripped through snapshot JSON
Old-format (8-field statistics) package passes validate();
omit-when-false serde guards
Pre-push checks clean locally

495 tests passed; 0 failed (476 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 — no saturating_* / wrapping_*
tracing only (WARN per the logging taxonomy)
Snapshot round-trip + previous-format compatibility test
No new production unsafe, no new dependencies

Closes #117

Blocks: Make PostOnly and FOK decisions atomic with the sweep (#112) #130

codecov-commenter · 2026-07-14T11:59:49Z

Codecov Report

❌ Patch coverage is 89.14027% with 24 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/price_level/level.rs	85.48%	9 Missing ⚠️
src/price_level/statistics.rs	94.26%	7 Missing ⚠️
src/price_level/order_queue.rs	82.35%	6 Missing ⚠️
src/price_level/snapshot.rs	33.33%	2 Missing ⚠️

Files with missing lines	Coverage Δ
src/price_level/snapshot.rs	`79.63% <33.33%> (+0.09%)`	⬆️
src/price_level/order_queue.rs	`92.26% <82.35%> (-2.65%)`	⬇️
src/price_level/statistics.rs	`91.29% <94.26%> (+0.59%)`	⬆️
src/price_level/level.rs	`91.77% <85.48%> (-0.04%)`	⬇️

... 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

This change fixes the original partial update path, but the reset race, unchecked execution count, unchanged v2 wire identifier, and repeated warning path need correction before merge. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self-review.

joaquinbejar

One additional concurrency gap remains after the earlier exact-head review because an in-flight statistics prefix can be persisted as a stable snapshot. The intended effective verdict remains REQUEST_CHANGES.

joaquinbejar

The rebased head leaves the existing reset rollback race, unchecked execution count, warning flood, snapshot format compatibility break, and torn statistics snapshot findings 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 reset rollback race, unchecked execution count, repeated warning flood, unchanged snapshot format compatibility break, and torn statistics snapshot. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The current head fixes the original single recorder partial update, but the existing reset race, unchecked execution count, warning flood, snapshot format compatibility break, and torn statistics snapshot findings remain unresolved. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The current head fixes the original single recorder partial update, but the existing reset rollback race, unchecked execution count, warning flood, unchanged v2 compatibility break, and persistable torn statistics snapshot remain unresolved. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The degraded statistics signal improves observability, but reset can race rollback into a wrapped counter, orders executed still overflows unchecked, snapshots can persist torn statistics, and the changed payload remains mislabeled as format v2. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

joaquinbejar

The degraded statistics signal improves observability, but reset can race rollback into a wrapped counter, orders executed still overflows unchecked, snapshots can persist torn statistics, and the changed payload remains mislabeled as format v2. 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 rewritten head adds the shared topology fixes and all exact head checks pass, but the existing statistics rollback, overflow, logging, snapshot compatibility, and torn read findings remain unresolved, and concurrent recorders can still move the latest execution timestamp backwards. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

- 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

…observable record_execution validated the multiplication and timestamps first but then mutated several independent atomics with fallible checked additions: a later counter overflow left the earlier counters already advanced (orders_executed and quantity_executed could advance while value_executed rejected), and match_order discarded the returned error, so a trade succeeded with silently inconsistent statistics. The additive aggregates are now committed with fetch_update(checked_add) CAS loops and, on any later failure, the already-committed prefix is rolled back with fetch_sub of exactly this call's contribution (the crate's #111/#113 call-backed rollback argument -- documented as contingent on reset()'s new quiescence contract). last_execution_time is stored only after every additive commit succeeds. An accepted execution therefore contributes to ALL aggregates or NONE in the committed state; the transient prefix window is documented honestly. match_order no longer discards the error: it logs at WARN (the match is not aborted -- the trade is already committed and is never unwound) and the new sticky stats_degraded flag records that an execution's statistics were dropped. The flag is queryable, carried through Clone / Display / FromStr / serde, and is serialized ONLY when true so a pre-#117 v2 snapshot package re-serializes byte-identically and its SHA-256 still validates -- old snapshots restore unchanged (guarded by an old-format compatibility test). Tests: forced overflow per aggregate (all-or-nothing, flag set, Err), Barrier-controlled concurrent boundary (exactly K symmetric-headroom successes, quantity/value lockstep), match_order integration (trade intact, flag observable and round-tripped), serde omit-when-false guards, torn-read caveats on the average accessors. Closes #117

joaquinbejar

The issue specific partial update and ignored error paths are fixed and all current checks pass, but the six existing statistics threads remain unresolved on this exact head, including rollback safety, checked overflow, snapshot compatibility and consistency, warning behavior, and timestamp monotonicity. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

- an AtomicU64 sequence brackets every statistics WRITE (record_execution and reset, via a RAII guard that closes the section on every early return) and Clone / Serialize / Display retry their multi-field copy until no write overlaps: a checksummed snapshot can no longer capture an in-flight recording prefix, and reset can no longer interleave inside a rollback (it is a seqlock writer now, enforced rather than documented); reader liveness is argued from the writer-serialization contract and the guard's panic-safe Drop - orders_executed joins the checked all-or-nothing transaction (a FromStr-seeded usize::MAX rejects instead of wrapping while the other aggregates advance) - the degraded WARN fires only on the false-to-true transition of the sticky flag (compare_exchange decides the witness) -- a burst of drops logs once - snapshot format bumps to v3: new packages write v3, validate() accepts v2 (legacy 8-field statistics, flag defaults false) and rejects v1; statistics keep skip-when-false serialization so checksum recomputation stays version-agnostic and legacy v2 checksums still match; migration guide + README updated - last_execution_time commits via fetch_max, so an out-of-order recorder can never move the latest-execution timestamp backwards

joaquinbejar · 2026-07-14T16:36:15Z

Thanks for pushing on the statistics guarantees. All six findings closed in 05cda8e, seqlock-consistent reads with enforced reset serialization, checked orders_executed, transition-only WARN, format v3 with v2 migration, and a monotonic execution timestamp.

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

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

Merged

11 tasks