Skip to content

[Bug Report] ModifyOrder accepts a modify of a fully-filled order instead of rejecting it #23

Description

@OPTIONPOOL

Found while running go-trader's in-process order book through an open-source matching-engine benchmark, the Matching Engine Performance Challenge — it cross-checks engines against the byte-identical consensus of other open source engines. A random workload never hits this (it modifies live resting orders), so it stays latent; it surfaces the moment a modify arrives for an order that has already been fully executed.

When a resting order is fully filled during matching, the engine removes it from the price-level book and marks it Filled, but it is never removed from the owning session's orders map. ModifyOrder gates only on s.orders[orderId] membership — still satisfied for the filled order — so it proceeds, and when the follow-up ob.remove() fails (the order is no longer in the book) it reports the order's status and returns success instead of rejecting:

// internal/exchange/exchange.go  (ModifyOrder)
order, ok := s.orders[orderId]
if !ok {                        // membership only — a Filled order still passes
	return OrderNotFound
}
...
so := sessionOrder{client, order, time.Now()}
err := ob.remove(so)            // the order is no longer resting -> err != nil
if err != nil {
	client.SendOrderStatus(so)  // ...so this just reports the filled order
	return nil                  // and treats the modify as handled — never a reject
}

The order's own state already knows the truth — Order.IsActive() returns false for Filled/Cancelled/Rejected (pkg/common/orders.go:63) — it just isn't consulted.

Repro. On an empty book:

  1. NEW id=1 SELL 10 @100 — rests.
  2. NEW id=2 BUY 10 @100 — fully fills id 1 (one trade, qty 10); id 1 is now Filled and off the book.
  3. MODIFY id=1 → 5 @101.

Expected: a modify-reject for a no-longer-resting order. Actual: a ModifyAck / status for the filled order — ModifyOrder returns nil. A partial fill modifies correctly (the residual still rests), so the bug is specific to the fully-consumed order whose entry lingers in s.orders.

(A cancel of the same filled order is, by contrast, correctly rejected. CancelOrder runs the identical s.orders[orderId] membership check and the identical ob.remove(so) call — remove fails the same way, returning OrderNotFound from the price-level search (orderbook.go:171-172) because the level is gone — but CancelOrder propagates that error (return err) where ModifyOrder swallows it. So this report is ModifyOrder-specific: same failure, divergent error handling.)

Fix. Reject when the order is no longer active, before touching the book:

func (e *exchange) ModifyOrder(client exchangeClient, orderId OrderID, price Fixed, quantity Fixed) error {
	s := e.lockSession(client)
	defer s.Unlock()

	order, ok := s.orders[orderId]
	if !ok || !order.IsActive() {   // already filled/cancelled -> reject
		return OrderNotFound
	}
	...
}

That replaces the SendOrderStatus / return nil swallow with an actual reject. Optionally prune s.orders when an order goes inactive so the map doesn't retain dead entries. Happy to share the failing edge-case workload.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions