Skip to content

fix: DirectIndexOrderBook requote exhausted the 64/level cap via unfreed tombstones (#94)#101

Merged
emrebulutlar merged 1 commit into
mainfrom
fix/match94-tombstone-compaction
Jul 7, 2026
Merged

fix: DirectIndexOrderBook requote exhausted the 64/level cap via unfreed tombstones (#94)#101
emrebulutlar merged 1 commit into
mainfrom
fix/match94-tombstone-compaction

Conversation

@emrebulutlar

Copy link
Copy Markdown
Member

Mechanism

DirectIndexOrderBook.cancelOrder tombstones an order (sets qty=0) instead of unlinking it, keeping the slot in the singly-linked FIFO chain. A tombstone's slot is reclaimed only when:

  1. the whole level empties (orderCount==0 triggers freeAllSlotsAtLevel), or
  2. the lazy head-advance inside getHeadOrder* walks past it during a matching sweep at that price.

A price level pinned live by another user's order never trades, so its head never advances. Every cancel-then-replace at that price (Engine.processUpdate = cancel old + re-add new) permanently consumes one of the 64 MAX_ORDERS_PER_LEVEL slots. After ~62 requotes the level returns LEVEL_FULL despite only two orders genuinely resting. The default ArrayMatchingEngine has no per-level cap and rests them fine, so the direct engine (MATCH_ENGINE_IMPL=direct) diverges from array, breaking the A/B behavioral-parity invariant. Medium severity, fallback engine only.

Approach: compact on allocation pressure (not unlink-on-cancel)

The FIFO chain is singly linked (ORDER_FIELDS=4, no prev pointer), so O(1) unlink-on-cancel is impossible without a layout change (a prev pointer would grow every level's footprint). Instead, when addOrder finds the level's free stack exhausted, it calls a new compactLevel(priceIdx):

  • walk the chain once (≤64 nodes), tracking the last surviving live node as we go;
  • unlink and free every tombstone (qty==0), re-stitching live orders into a compact chain in their original FIFO order;
  • rebuild head/tail; then retry the allocation.

LEVEL_FULL is now returned only if 64 genuinely-live orders remain after compaction.

Why this is safe:

  • Slots do not move, so orderLocations entries for survivors stay valid.
  • Every tombstone already had its orderLocations entry cleared (in cancelOrder/reduceOrderQuantity), so freeing its slot cannot orphan a lookup.
  • Post-compaction the chain holds no tombstones, so the lazy head-advance in getHeadOrder* frees nothing further — no double-free.
  • Compaction is a pure function of book state (no time/randomness), so determinism holds.
  • The documented level_full_reject divergence (64 real live orders) is preserved.

Tests

DirectIndexOrderBookTest (4 new):

  • testRequotePinnedLevel_NoLevelFull — issue trigger: B pins the level, A cancel-then-replaces 100×, every replace succeeds; only 2 orders rest.
  • testLevelFull_64LiveOrders_CompactionCannotHelp — 64 genuinely-live orders → 65th still LEVEL_FULL (boundary preserved).
  • testFifoPreservedAcrossCompaction — interleaved live/tombstones, forced compaction, survivors match in exact FIFO order.
  • testCompactionNoDoubleFreeAndCleanNotFound — level totals/getActiveOrders consistent after 200 compaction cycles; a compacted-away id reports a clean not-found; level still fills to true 64 capacity.

Determinism suite:

  • New requote_level_slots.scenario (B pins the $60k level, A requotes 70×) added to the convergent set — DeterminismAbEquivalenceTest now asserts direct == array for it (it would have diverged on the old code). Golden generated via -Dupdate.golden=true; no existing golden changed.

Full match-cluster suite (excluding the embedded-cluster/archive-housekeeping infra tests that need live Aeron): 396 tests, 0 failures.

Closes #94

🤖 Generated with Claude Code

…mpact tombstones on allocation pressure (#94)

DirectIndexOrderBook.cancelOrder tombstones (qty=0) instead of unlinking; a
tombstone's slot is reclaimed only when the whole level empties, or lazily as
getHeadOrder* advances the head during a matching sweep. A level pinned live by
another user's order never trades, so its head never advances: every
cancel-then-replace at that price permanently consumes one of the 64
MAX_ORDERS_PER_LEVEL slots. After ~62 requotes the level returns LEVEL_FULL
despite only a couple of orders genuinely resting, and the array engine (no
per-level cap) does not, breaking the A/B behavioral-parity invariant.

The FIFO chain is singly linked (ORDER_FIELDS=4, no prev pointer), so O(1)
unlink-on-cancel is not possible without a layout change. Instead we compact on
allocation pressure: when addOrder finds the level's free stack exhausted, walk
the chain once (<=64 nodes), unlink and free every tombstone while re-stitching
live orders into a compact chain in their original FIFO order, then retry.
LEVEL_FULL is returned only if 64 genuinely-live orders remain. Slots do not
move, so orderLocations stays valid; tombstones already had their location
cleared, so freeing them cannot orphan a lookup and the post-compaction chain
has no tombstones for the lazy head-advance to double-free.

Compaction is a pure function of book state (no time/randomness), so
determinism holds. The documented level_full_reject divergence (64 real live
orders) is preserved.

Tests:
- DirectIndexOrderBookTest: issue trigger (B pins, A requotes 100x, no
  LEVEL_FULL); 64-live boundary still rejects the 65th; FIFO preserved across
  compaction; level totals/getActiveOrders consistent + clean not-found for a
  compacted-away id (no double-free).
- New determinism scenario requote_level_slots.scenario, asserted NON-divergent
  in DeterminismAbEquivalenceTest's convergent set; golden generated via
  -Dupdate.golden=true.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@emrebulutlar
emrebulutlar merged commit 343175d into main Jul 7, 2026
2 checks passed
@emrebulutlar
emrebulutlar deleted the fix/match94-tombstone-compaction branch July 7, 2026 19:20
emrebulutlar added a commit that referenced this pull request Jul 7, 2026
…R poison pill (#91) (#103)

Engine.processCreate/processUpdate validated price (validateLimitPrice,
notionalOverflows) but never checked quantity > 0. quantity is signed int64 on
the wire, so 0/negative are representable via any non-REST client (gRPC), which
the OMS REST layer does not guard. Two failures resulted:

- Silent egress gap (LIMIT): a qty=0 LIMIT never matched and never rested, so
  none of processCreate's status branches fired — no NEW/REJECTED/FILLED ever
  published. The OMS never learned the order's fate.
- Poison pill (LIMIT_MAKER, default array engine): a qty=0 LIMIT_MAKER that did
  not cross rested at the head of its price level. matchAtLevel computes
  matchQty=min(taker, makerQty); makerQty=0 -> matchQty<=0 -> break, abandoning
  the WHOLE level even though real orders sit behind it in FIFO. It survived
  snapshot/restore (PriceRules.validate only checks price).

Mechanism (deterministic state machine must not trust its input):
- New reject code OrderRejectReason.INVALID_QUANTITY = 6 (+ describe()). A
  distinct code, not a reused one — reject codes become user-visible wire values.
- processCreate LIMIT/LIMIT_MAKER: fold quantity<=0 into the validity
  computation BEFORE the overflow check, so INVALID_QUANTITY wins over OVERFLOW
  for garbage input. Loud REJECTED status, nothing rests.
- processCreate MARKET: explicit admission reject (buy budget<=0, sell qty<=0)
  -> REJECTED with INVALID_QUANTITY, skipping the matching sweep entirely rather
  than misattributing the cause to the no-liquidity branch.
- processUpdate: quantity<=0 added to the existing pre-cancel validation block,
  so an UPDATE to qty<=0 is a REJECT (never an implicit cancel) and the OLD
  order survives — same pattern as the existing price-validation guard.
- Belt-and-suspenders: ArrayOrderBook.addOrder and DirectIndexOrderBook.addOrder
  return INVALID_QUANTITY for qty<=0 at the top (above DirectIndexOrderBook's
  #101 compaction block); the existing restReason/addResult plumbing turns a
  non-NONE return into a loud terminal status.
- New invalidQuantityRejectCount diag counter mirroring overflowRejectCount
  (EGRESS-DIAG line + match_invalid_qty_rejects_total metric).

Checks are pure functions of the command; WARN logging via
OrderRejectReason.describe mirrors the existing reject paths. Determinism holds.

Tests:
- EngineTest: qty=0 and qty=-1 LIMIT publish REJECTED (closes the silent-egress
  gap); qty=0 LIMIT_MAKER REJECTED, nothing rests; MARKET buy budget=0 and
  MARKET sell qty=0 REJECTED with INVALID_QUANTITY (counter asserted, resting
  liquidity untouched); UPDATE to qty=0 REJECTED, old order survives and still
  matches a later market sell.
- ArrayOrderBookTest + DirectIndexOrderBookTest: addOrder returns
  INVALID_QUANTITY for qty<=0; a positive quantity still rests.
- New determinism scenario reject_zero_qty.scenario + golden replaying the
  issue's poison-pill sequence (qty=0 LIMIT_MAKER now rejected; a real
  LIMIT_MAKER behind it; MARKET SELL matches the real order — the level no
  longer bricks). A/B-convergent (DeterminismAbEquivalenceTest); no existing
  goldens change.

Full match-cluster suite green (408 tests) minus the embedded-infra classes
(EmbeddedClusterTest, ArchiveHousekeepingTest).

Closes #91

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DirectIndexOrderBook: tombstone slots never freed without a match → requote exhausts the 64/level cap

1 participant