[Bug Report] Cancel only soft-flags the order (never removes it), so cancel + re-add of the same id is rejected and later corrupts the book into a panic

[OPTIONPOOL]

Found while integrating tome into an open matching-engine benchmark, the Matching Engine Performance Challenge — it cross-checks engines against the byte-identical consensus of other open-source engines. Thanks for the clean, well-commented codebase; this one is even flagged by your own // todo, so it may already be on your radar — writing it up with a concrete repro in case that helps.

Pinned at d3d81c4 (d3d81c4f78fa712455a2f817bd0ddd3cfd61fdeb, HEAD of master). Go 1.23, go test against the unmodified package; the relevant code is in order_book.go.

Bug 1 — Cancel never removes the order, so cancel + re-add of the same id is rejected and a later match panics

OrderBook.Cancel doesn't remove the order. It sets Cancelled = true and rewrites the activeOrders entry, but leaves the order's tracker in the price tree and the order in activeOrders (the trailing comment is // todo: remove from active orders); it's only pruned lazily if a later match happens to walk over it (order_book.go:268):

func (o *OrderBook) Cancel(id uint64) error {
    order, ok := o.activeOrders[id]
    if !ok {
        return nil
    }
    order.Cancel()                       // only sets Cancelled = true
    return o.updateActiveOrder(order)    // todo: remove from active orders  <-- tracker + activeOrders entry stay
}

Two consequences follow:

1. The id is never freed, so re-adding the same id (the natural way to implement a modify: cancel + re-insert) is rejected with order with ID <n> already exists.
2. That rejected Add does a partial rollback that strands the original tracker in the price tree with no trackers[] entry. A subsequent match over that side then deletes the order from activeOrders while the stranded tracker remains, and the next match over that side hits a tracker with no active order behind it and panics.

Walking the second consequence through the code: a cancel + re-insert of the same id does Add → submit, which rests the new order via addToBooks(tracker_new) (a second tracker for the id in the tree, trackers[id] = tracker_new). Then storeOrder → setActiveOrder finds activeOrders[id] still present (the cancelled order) and returns "order with ID %d already exists" (order_book.go:219). storeOrder's error path rolls back with o.orders.Remove(order.ID) (order_book.go:234), which removes tracker_new and deletes trackers[id] — but the original tracker_old is still in the price tree (Cancel never removed it). It is now stranded: in the tree, absent from trackers[].

The stranded node is then fatal on the matching path. matchOrder walks the opposite side; when it reaches tracker_old it sees IsCancelled() and defers removeFromBooks(id). But removeFromBooks → orderContainer.Remove(id) finds no trackers[id] and bails after logging cannot remove order: no tracker for id (order_container.go:36-40), so tracker_old stays in the tree — while removeFromBooks still does delete(o.activeOrders, id). The next match over that side reaches tracker_old, looks up the (now-deleted) active order, and panics (order_book.go:430):

oppositeOrder, ok := o.getActiveOrder(oppositeTracker.OrderID)
if !ok {
    panic("should NEVER happen - tracker exists but active order does not")
}

Repro.

ob := NewOrderBook("TEST", marketPrice, NewTradeBook("TEST"), NOPOrderRepository)

ob.Add(bid(1, 100, 5))     // rest BUY id=1 @100 x5
ob.Cancel(1)               // soft-flag only
// ob.GetBids() still returns order 1 here.

_, err := ob.Add(bid(1, 100, 5))   // re-add same id -> err = "order with ID 1 already exists"

ob.Add(ask(2, 100, 5))     // a crossing sell: skips the cancelled tracker,
                           // logs "cannot remove order: no tracker for id 1",
                           // and deletes activeOrders[1] while the tracker stays in the tree
ob.Add(ask(3, 100, 5))     // next match over the bids -> PANIC:
                           // "should NEVER happen - tracker exists but active order does not"

Expected: Cancel(1) removes order 1 so the book is empty and the id is reusable; the re-add succeeds and nothing panics. (Verified against d3d81c4. The panic needs the two same-id orders to carry distinct timestamps so they land at different tree keys — the normal case, since every realistic order, and the suite's own time.Now() helper, produces distinct timestamps; the first consequence — id never freed → re-add rejected — holds regardless.)

Fix. Make Cancel actually remove the order, reusing the engine's own removeFromBooks (the same routine that retires a fully-filled maker), so the tree and activeOrders are cleared and the id is freed:

func (o *OrderBook) Cancel(id uint64) error {
    o.orderMutex.RLock()
    order, ok := o.activeOrders[id]
    o.orderMutex.RUnlock()

    if !ok {
        return nil
    }
    order.Cancel()
    if err := o.updateActiveOrder(order); err != nil {
        return err
    }
    o.removeFromBooks(id) // actually remove it from the tree + activeOrders
    return nil
}

I confirmed this compiles, that the full TestOrderBook_* suite still passes (including TestOrderBook_Limit_To_Limit_Second_IOC_CancelCheck), and that the reproduction above no longer panics and re-adds the id cleanly. It preserves the existing orderMutex.RLock() around the activeOrders read.

Bug 2 — MinQty boundary rejects an order of exactly the minimum (very minor, possibly intentional)

Add rejects order.Qty <= MinQty with MinQty = 1 (order_book.go:290, :14), so an order of exactly the named minimum (1) is rejected with ErrInvalidQty. If MinQty is meant as the smallest accepted quantity, order.Qty < MinQty would admit it; if it's deliberately an exclusive floor then this is working as designed — just flagging the boundary in case it's the former. Trivial to reproduce: ob.Add(order_with_Qty_1) returns ErrInvalidQty.

---

This is just a reproducible snapshot of commit d3d81c4 from wiring the engine into a benchmark, offered back in case it's useful — the Cancel behavior is already marked // todo in your source, so this is mostly a concrete repro of where it bites. Happy to share the failing workload or the test file. Thanks again for the clear, well-documented code.

Respectfully submitted.