Make PostOnly and FOK decisions atomic with the sweep (#112) by joaquinbejar · Pull Request #130 · joaquinbejar/PriceLevel

joaquinbejar · 2026-07-14T13:48:34Z

Summary

PostOnly's pre-check and FOK's dry-run inspected the queue and then ran a
separate sweep, so concurrent add_order / cancel / update could change
matchable depth between the phases: a PostOnly taker could trade against depth
added after its check, and an FOK taker could start with sufficient depth and
return partially filled. PostOnly is now structurally incapable of trading;
FOK is all-or-nothing under every interleaving.

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 10/10, the top)
Base branch: stack/9-117-atomic-stats
Stacked on: Make execution-statistics failures non-partial and observable (#117) #129 — the diff vs main is cumulative; review against the base branch.

Changes

PostOnly, structural (no lock): a positive post-only either crosses
(rejected) or rests (NotFilled), returning before any queue touch — it
never enters the sweep, so "PostOnly emits zero trades" holds under every
interleaving. The old fall-through also let a non-crossing probe
garbage-collect an unmatchable zero-visible reserve; a read-only probe no
longer mutates resting liquidity. Reject-vs-rest is linearized at the
depth-check read; post-only is evaluated before TIF, so it never takes
liquidity even under Fok / Ioc.
FOK, level guard: all-or-nothing across N makers is a cross-maker
transactional property per-entry atomics cannot express, so a positive FOK
takes the exclusive side of a level RwLock across dry-run + sweep;
add_order / update_order take the shared side. The Gtc/Ioc/Day sweep is
unguarded. Guard always acquired before any entry lock (no ordering cycle);
poisoned guards recover via into_inner; the guard is acquired exactly once
per public update_order call — the same-price Replace /
UpdatePriceAndQuantity branches delegate to a guard-free inner fn, so no
recursive read can deadlock behind a queued FOK writer.
Lock-free headline claims in lib.rs / mod.rs / level.rs / README
qualified honestly (common paths lock-free; FOK takes a level-exclusive
guard for the duration of its feasibility check and sweep).

Technical decisions

Reviewed by architect, concurrency-auditor, and
microstructure-critic. The auditor reproduced a P1 deadlock in the
first iteration (recursive shared-guard read in update_order's same-price
branches vs a queued FOK writer, std RwLock being writer-preferring) —
fixed with the guard-free inner delegate, and the race test fails
deterministically via a bounded timeout instead of hanging if it ever
regresses. The critic confirmed SKIP/kill semantics, IOC correctly
unguarded, and MatchResult shapes per Validate MatchResult invariants during decoding (#116) #123's validator.

Performance

Criterion with the untouched Gtc sweep as drift control: add_order +0.9%,
cancel +1.4% (both within noise — one uncontended shared acquisition);
post-only −1.5% (skips the vacuous sweep); FOK pays one uncontended exclusive
acquisition on a cold path.

Testing

Barrier race tests (2000 iterations each): add-vs-PostOnly (zero trades
every iteration), cancel-vs-FOK and same-price-replace-vs-FOK
(complete-or-killed, never partial, counters == queue)
Deadlock regression test bounded by timeout (fails deterministically
pre-fix)
MatchResult invariants for Rejected / Killed per the Validate MatchResult invariants during decoding (#116) #123 validator
Pre-push checks + examples clean locally

507 tests passed; 0 failed (488 lib + 9 + 1 + 9 doctests).

Checklist

Code follows rules/global_rules.md and CLAUDE.md
No .unwrap() / .expect() / unchecked indexing in production code
(poison recovery via into_inner)
The lock is justified per the rules (no lock-free structure expresses
cross-maker atomicity; contention-rare path; documented on the field)
No new production unsafe, no new dependencies (std::sync::RwLock)
README regenerated; lock-free claims qualified

Closes #112

codecov-commenter · 2026-07-14T13:53:59Z

Codecov Report

❌ Patch coverage is 94.36620% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/price_level/statistics.rs	94.26%	7 Missing ⚠️
src/price_level/level.rs	96.59%	3 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/level.rs	`90.75% <96.59%> (+0.72%)`	⬆️
src/price_level/statistics.rs	`91.29% <94.26%> (+0.59%)`	⬆️

🚀 New features to boost your workflow:

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

joaquinbejar

The exclusive FOK guard does not make the dry run equivalent to the sweep, and the public lock free claims contradict the new shared guard on add and cancel. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The FOK guard fixes the reviewed cancel and same price update races, but the dry run can still diverge at replenishment overflow, the PostOnly depth scan is not linearizable, snapshots do not participate in the guard, and the public lock free claims no longer match the implementation. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The current head fixes the reviewed cancel and same price update races, but the FOK dry run can still diverge from the sweep, the PostOnly decision is not linearizable, snapshots do not participate in the guard, and the public lock free claims contradict the shared mutator locks. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The guard closes several decision races, but FOK can still partially fill at the replenishment overflow boundary, PostOnly lacks a linearizable depth decision, snapshots can capture an intermediate FOK state, and the public lock free claims remain false. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

joaquinbejar

The guard closes several decision races, but FOK can still partially fill at the replenishment overflow boundary, PostOnly lacks a linearizable depth decision, snapshots can capture an intermediate FOK state, poisoned guards can reopen a partially mutated level, and the public lock free claims remain false. GitHub requires this self review to be submitted as COMMENT, but the intended effective verdict is REQUEST_CHANGES.

joaquinbejar

The rewritten head adds the shared topology fixes and all exact head checks pass, but the existing FOK partial fill, non linearizable PostOnly scan, intermediate snapshot, and lock free contract findings remain unresolved. The new concurrency tests also do not deterministically force the decision to sweep race window required by issue 112. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

…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 shared guard fixes the reviewed cancel and same price update races and all current checks pass, but the six existing FOK, PostOnly, snapshot isolation, poison recovery, lock free contract, and deterministic race coverage findings remain unresolved on this exact head. 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

PostOnly's pre-check and FOK's dry-run inspected the queue and then ran a separate sweep, so concurrent add_order / cancel / update could change matchable depth between the phases: a PostOnly taker could trade against depth added after its check, and an FOK taker could start with sufficient depth and return partially filled. PostOnly is now structural: a positive post-only either crosses (rejected) or rests (NotFilled), returning before any queue touch -- it never enters the sweep, so "PostOnly emits zero trades" holds under every interleaving with no lock. The old fall-through additionally let a non-crossing probe garbage-collect an unmatchable zero-visible reserve; a read-only probe no longer mutates resting liquidity. The reject-vs- rest decision is linearized at the depth-check read, and post-only is evaluated before TimeInForce, so a post-only taker never takes liquidity even under Fok/Ioc. FOK all-or-nothing is a cross-maker transactional property that per-entry atomics cannot express, so a positive FOK takes the exclusive side of a level RwLock across its feasibility check and sweep while add_order / update_order take the shared side; the Gtc/Ioc/Day sweep and cancel-through-update take no new ordering risk (guard always acquired before any entry lock; the sweep never re-enters a mutator). The guard is acquired exactly once per public update_order call -- the same-price Replace / UpdatePriceAndQuantity branches delegate to a guard-free inner function, so no recursive read can deadlock behind a queued FOK writer (reproduced pre-fix; the Barrier race test fails deterministically via a bounded timeout instead of hanging). Poisoned guards recover via into_inner. Lock-free headline claims in lib.rs/mod.rs/level.rs/README are qualified accordingly. Benches: uncontended shared acquisition on add/cancel within noise of the untouched Gtc control; post-only slightly faster (skips the vacuous sweep). Race tests (2000 iters each): add-vs-PostOnly never trades; cancel-vs-FOK and same-price-replace-vs-FOK are complete-or-killed, never partial, counters == queue every iteration. Closes #112

joaquinbejar

The issue patch is unchanged from the previously reviewed head and all six current inline threads remain unresolved, including the partial FOK path and the PostOnly, snapshot, poison recovery, lock free contract, and deterministic test gaps. The intended effective verdict remains REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

joaquinbejar

The issue patch is unchanged from the last reviewed head and the six existing FOK correctness, PostOnly linearizability, snapshot isolation, poison recovery, lock free contract, and deterministic race coverage findings remain unresolved. The shared guard also lacks contended tail latency evidence for the hot add, update, and cancel paths. The intended effective verdict is REQUEST_CHANGES. GitHub requires COMMENT because this is a self review.

…ast poison - the guarded fill-or-kill dry run now simulates the sweep's counter evolution exactly (mutators are frozen by the write guard): pure consumes decrement the projected visible counter, replenishments apply their checked net delta, and a would-abort headroom overflow terminates the projection where the real sweep would stop -- so a FOK the sweep cannot complete is killed with zero trades instead of partially filling (reviewer's boundary construction is a test) - a mutation epoch (bumped by every committed admission / update) brackets the post-only depth scan with a stable-epoch retry, giving the rest-vs-reject verdict a linearization point - snapshot() takes the shared side of the FOK guard for its materialization, so a checksummed snapshot can never capture a multi-maker fill-or-kill mid-transaction - a poisoned guard no longer silently reopens the level: recovery trips a sticky poisoned flag (one ERROR log on transition); admissions and updates fail fast and matching refuses while set, snapshots stay available for diagnostics -- defense in depth, the production sweep has no unwind path - lock-free claims reworded honestly: the Gtc/Ioc/Day match is lock-free; admissions and updates are shared-lock mutators that can block behind an O(depth) fill-or-kill writer (README regenerated) - a cfg(test) decision-boundary seam makes the add-after-post-only- check race deterministic; the FOK cancel window is documented as structurally closed by the write guard

joaquinbejar · 2026-07-14T17:08:03Z

Thanks for the exhaustive scrutiny across the whole stack. All eight findings are closed in b0995c5, exact FOK feasibility under the guard, a linearizable post-only verdict, snapshot isolation from FOK transactions, fail-fast poison handling, honest shared-lock contracts, and a deterministic decision-boundary seam.

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

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

Merged

11 tasks