fix: reject quantity<=0 at order admission — silent loss + LIMIT_MAKER poison pill (#91)#103
Merged
Merged
Conversation
…R poison pill (#91) 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mechanism
Engine.processCreate/processUpdatevalidated price (validateLimitPrice,notionalOverflows) but never checkedquantity > 0.quantityis signed int64 on the wire, so0/negative are representable via any non-REST client (e.g. gRPC), which the OMS REST layer does not guard. The deterministic state machine must not trust its input, so the fix rejects at admission:OrderRejectReason.INVALID_QUANTITY = 6(+describe()text). A distinct code, not a reused one — reject codes become user-visible wire values in the upcoming v6 schema.quantity <= 0into thevaliditycomputation before the overflow check, soINVALID_QUANTITYwins overOVERFLOWfor garbage input. LoudREJECTED, nothing rests.<= 0, sell qty<= 0) →REJECTEDwithINVALID_QUANTITY, skipping the matching sweep entirely rather than misattributing the cause to the no-liquidity branch.quantity <= 0added to the existing pre-cancel validation block, so anUPDATEto qty<= 0is a REJECT (never an implicit cancel) and the OLD order survives — same pattern as the existing price-validation guard.ArrayOrderBook.addOrderandDirectIndexOrderBook.addOrderreturnINVALID_QUANTITYfor qty<= 0at the top (above DirectIndexOrderBook's fix: DirectIndexOrderBook requote exhausted the 64/level cap via unfreed tombstones (#94) #101 compaction block). The existingrestReason/addResultplumbing turns a non-NONEreturn into a loud terminal status.invalidQuantityRejectCountdiag counter mirroringoverflowRejectCount(EGRESS-DIAG line +match_invalid_qty_rejects_totalmetric).Checks are pure functions of the command; WARN logging via
OrderRejectReason.describemirrors the existing reject paths. Determinism holds.What this fixes (from the issue)
processCreate's status branches fired — noNEW/REJECTED/FILLEDever published. The OMS never learned the order's fate. NowREJECTEDis always published.matchAtLevelcomputesmatchQty=min(taker, makerQty), somakerQty=0 → matchQty<=0 → break, abandoning the WHOLE level even though real orders sit behind it in FIFO (it survived snapshot/restore). Now it can never rest.What changed
OrderRejectReason.javaINVALID_QUANTITY = 6+describe()Engine.javaArrayOrderBook.java,DirectIndexOrderBook.javaAppClusteredService.javainvalidQtyRej(metric + EGRESS-DIAG)Tests
REJECTED(closes the silent-egress gap); qty=0 LIMIT_MAKERREJECTED, nothing rests; MARKET buy budget=0 and MARKET sell qty=0REJECTEDwithINVALID_QUANTITY(counter asserted, resting liquidity untouched); UPDATE to qty=0REJECTED, old order survives and still matches a later market sell.addOrderreturnsINVALID_QUANTITYfor qty<= 0; a positive quantity still rests.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 — proving the level no longer bricks). Lands in the A/B-convergent set (DeterminismAbEquivalenceTest); no existing goldens change (verified viagit diff).EmbeddedClusterTest,ArchiveHousekeepingTest).Closes #91
🤖 Generated with Claude Code