Review: Order Book and Matching Engine Written by an AI Assistant

Published 10 Nov, 2026

Your trading systems team asked an AI coding assistant to rewrite the matching core of an internal crossing engine. It opened this pull request. You are the reviewer, and nothing merges without your approval.

The assistant's description:

Price-time priority order book with O(1) cancel through an id index, a lock-free SPSC queue between the feed handler and the matcher, and lock-free top-of-book reads for market data. All book mutations are guarded by a single mutex.

Where it runs: One instrument per book. A feed-handler thread decodes orders into the SpscQueue; the matching thread pops them and calls add() and cancel(); a market-data thread calls best_bid() and best_ask() every 100 µs. Strategies receive fills through on_fill and often cancel or re-quote from inside the callback. Busy price levels hold thousands of resting orders. Built with -O3 for x86-64 production servers and ARM64 simulation hosts.

Your task

Flag every problem you would block the merge for. Click a line number to comment, and Shift-click to cover a range.
For each one, choose the issue type and severity, and explain what fails and how to fix it.
You are scored on the problems you catch, the right issue type and a clear explanation. Missing a critical problem fails the round.

Aim for 25 minutes, as in a real review round.

In SpscQueue::push(), what guarantees the consumer sees buf_[h] once it sees the new head_?

index_ stores raw pointers into a std::vector. What happens to them on push_back and on erase?

A buy order clears an entire ask level. What does ++lvl act on after side.erase(lvl)?

best_bid() runs on another thread with no lock. What is it walking while the matcher inserts a new level?

A strategy cancels its resting quote from inside on_fill. Which lock does cancel() need, and who holds it?

The structure looks textbook, but three bugs are undefined behaviour that corrupt or crash the engine in normal trading, and two more appear as soon as the market-data thread and strategy callbacks are connected.

Missing release in SPSC push (line 29, critical). The relaxed store to head_ does not order the earlier write to buf_[h], so the consumer's acquire load has nothing to pair with. On ARM64, or after compiler reordering on x86, the matcher can pop a slot before the order is written and trade on a stale or half-written order. ThreadSanitizer reports the race. Store head_ with memory_order_release, as the tail store already does.
Index pointers into a vector dangle (lines 58–60, critical). &level.back() points into a std::vector. A later push_back that reallocates frees the old storage, and every erase in match() or remove() shifts later orders down, so index_ entries dangle or point at a different order. cancel() then reads freed memory at line 67 (AddressSanitizer reports heap-use-after-free) or uses the wrong price and side. Use storage with stable addresses: std::list with stored iterators, or an order pool with intrusive per-level lists.
Erased map iterator is incremented (line 96, critical). side.erase(lvl) invalidates lvl, then the loop runs ++lvl on it. This is undefined behaviour every time an aggressive order clears a level, which is routine. Write lvl = side.erase(lvl) when the level empties and increment only otherwise.
Top of book read without synchronisation (lines 74–76, major). std::map is not safe to read while another thread inserts or erases. The market-data thread can follow a node that is being rebalanced or freed, and crash or publish garbage; the comment saying no lock is needed is wrong. Have the matcher publish best bid and ask in std::atomic<int64_t> values after each change, or take the lock.
Fill callback runs under the lock (line 86, major). on_fill_ is called with mu_ held and while iterating the level. A strategy that cancels or re-quotes calls cancel() or add() on the same thread and locks a non-recursive std::mutex again: deadlock, formally undefined behaviour. A recursive mutex would only swap that for modifying the containers mid-iteration. Collect fills and invoke callbacks after the book is consistent and the lock is released, or queue strategy actions.
Front erase is linear (line 91, minor). Fills always consume the front of the level, and vector::erase at the front shifts every remaining order. Sweeping a level of thousands of orders is quadratic and causes latency spikes. Use a std::deque or an intrusive list.
Head and tail share a cache line (lines 41–42, minor). head_ and tail_ sit next to each other, so the producer and consumer threads bounce the same cache line on every operation. Align each to 64 bytes (alignas(64)) and keep a cached copy of the other index on each side.

#pragma once
#include <algorithm>
#include <atomic>
#include <cstddef>
#include <cstdint>
#include <functional>
#include <map>
#include <mutex>
#include <unordered_map>
#include <vector>

struct Order {
    uint64_t id;
    bool     is_buy;
    int64_t  price;  // in ticks
    uint32_t qty;
};

// Lock-free single-producer, single-consumer ring.
// The feed handler thread pushes, the matching thread pops.
template <typename T, size_t N>
class SpscQueue {
public:
    bool push(const T& v) {
        size_t h = head_.load(std::memory_order_relaxed);
        size_t next = (h + 1) % N;
        if (next == tail_.load(std::memory_order_acquire)) return false;  // full
        buf_[h] = v;
        head_.store(next, std::memory_order_relaxed);
        return true;
    }
    bool pop(T& out) {
        size_t t = tail_.load(std::memory_order_relaxed);
        if (t == head_.load(std::memory_order_acquire)) return false;  // empty
        out = buf_[t];
        tail_.store((t + 1) % N, std::memory_order_release);
        return true;
    }
private:
    T buf_[N];
    std::atomic<size_t> head_{0};
    std::atomic<size_t> tail_{0};
};

// Price-time priority book for one instrument.
class OrderBook {
public:
    using FillFn = std::function<void(uint64_t maker, uint64_t taker, int64_t px, uint32_t qty)>;
    explicit OrderBook(FillFn on_fill) : on_fill_(std::move(on_fill)) {}

void add(Order o) {
        std::lock_guard<std::mutex> lk(mu_);
        if (o.is_buy)
            match(o, asks_, [](int64_t level, int64_t px) { return level <= px; });
        else
            match(o, bids_, [](int64_t level, int64_t px) { return level >= px; });
        if (o.qty == 0) return;
        std::vector<Order>& level = o.is_buy ? bids_[o.price] : asks_[o.price];
        level.push_back(o);
        index_[o.id] = &level.back();
    }

bool cancel(uint64_t id) {
        std::lock_guard<std::mutex> lk(mu_);
        auto it = index_.find(id);
        if (it == index_.end()) return false;
        Order* o = it->second;
        if (o->is_buy) remove(bids_, o->price, id);
        else           remove(asks_, o->price, id);
        index_.erase(it);
        return true;
    }

// Read-only snapshot for the market data publisher thread; no lock needed.
    int64_t best_bid() const { return bids_.empty() ? 0 : bids_.begin()->first; }
    int64_t best_ask() const { return asks_.empty() ? 0 : asks_.begin()->first; }

private:
    template <typename Side, typename Crosses>
    void match(Order& taker, Side& side, Crosses crosses) {
        for (auto lvl = side.begin(); lvl != side.end() && taker.qty > 0; ++lvl) {
            if (!crosses(lvl->first, taker.price)) break;
            std::vector<Order>& queue = lvl->second;
            for (auto it = queue.begin(); it != queue.end() && taker.qty > 0;) {
                uint32_t q = std::min(it->qty, taker.qty);
                on_fill_(it->id, taker.id, lvl->first, q);
                it->qty -= q;
                taker.qty -= q;
                if (it->qty == 0) {
                    index_.erase(it->id);
                    it = queue.erase(it);
                } else {
                    ++it;
                }
            }
            if (queue.empty()) side.erase(lvl);
        }
    }

template <typename Side>
    void remove(Side& side, int64_t price, uint64_t id) {
        auto lvl = side.find(price);
        auto& q = lvl->second;
        q.erase(std::remove_if(q.begin(), q.end(),
                               [id](const Order& x) { return x.id == id; }),
                q.end());
        if (q.empty()) side.erase(lvl);
    }

std::mutex mu_;
    std::map<int64_t, std::vector<Order>, std::greater<int64_t>> bids_;
    std::map<int64_t, std::vector<Order>> asks_;
    std::unordered_map<uint64_t, Order*> index_;
    FillFn on_fill_;
};

{"issues":[
{"id":"release","file":"order_book.hpp","lines":[29,29],"category":"concurrency","severity":"critical","title":"SPSC head published with a relaxed store","explanation":"Without release ordering the consumer can observe the new head before buf_[h] is written and read a stale or partly written order.","keywords":["release","acquire","relaxed","memory order","memory_order","reorder","visib","happens-before","fence"],"accept":["bug"],"fix":"head_.store(next, std::memory_order_release);"},
{"id":"dangle","file":"order_book.hpp","lines":[58,60],"category":"bug","severity":"critical","title":"index_ holds pointers into vectors that reallocate and shift","explanation":"push_back reallocation frees old storage and erase shifts later elements, so index_ entries dangle or point at the wrong order; cancel reads freed memory.","keywords":["invalidat","dangl","realloc","pointer","use after free","use-after-free","shift","stable","std::list","push_back"],"accept":["design"],"fix":"Store orders where addresses are stable (std::list with iterators, or a pool with intrusive lists) and index by handle."},
{"id":"mapit","file":"order_book.hpp","lines":[96,96],"category":"bug","severity":"critical","title":"Map iterator incremented after erase","explanation":"side.erase(lvl) invalidates lvl and the loop then increments it, which is undefined behaviour whenever a level is fully consumed.","keywords":["invalidat","erase","iterator","++lvl","lvl = side.erase","undefined","use after free","dangl"],"accept":["edge"],"fix":"if (queue.empty()) lvl = side.erase(lvl); else ++lvl; (remove ++lvl from the for header)."},
{"id":"mdrace","file":"order_book.hpp","lines":[74,76],"category":"concurrency","severity":"major","title":"best_bid and best_ask read the map without synchronisation","explanation":"The market-data thread walks std::map while the matcher inserts and erases nodes: a data race that can crash or return garbage.","keywords":["race","lock","thread","atomic","synchron","concurren","not thread-safe","not thread safe","data race"],"accept":["bug"],"fix":"Publish best prices via std::atomic<int64_t> updated by the matcher, or lock the mutex."},
{"id":"reent","file":"order_book.hpp","lines":[86,86],"category":"design","severity":"major","title":"Fill callback invoked with the mutex held mid-iteration","explanation":"A strategy calling cancel() or add() from on_fill relocks a non-recursive mutex on the same thread and deadlocks; a recursive mutex would mutate containers being iterated.","keywords":["callback","reentran","re-entran","deadlock","recursive","under the lock","holding","defer","after unlock"],"accept":["concurrency"],"fix":"Buffer fills and invoke callbacks after releasing the lock, or queue strategy actions."},
{"id":"fronterase","file":"order_book.hpp","lines":[91,91],"category":"perf","severity":"minor","title":"Erasing from the front of a vector is O(n) per fill","explanation":"Each filled maker shifts the rest of the level, so sweeping a deep level is quadratic.","keywords":["o(n)","linear","quadratic","shift","deque","list","front","latency"],"accept":["design"],"fix":"Use std::deque or an intrusive list for each price level."},
{"id":"falseshare","file":"order_book.hpp","lines":[41,42],"category":"perf","severity":"minor","title":"head_ and tail_ share a cache line","explanation":"Producer and consumer write adjacent atomics, so the cache line ping-pongs between cores on every push and pop.","keywords":["false sharing","cache line","alignas","64","padding","ping-pong","hardware_destructive"],"accept":["concurrency"],"fix":"alignas(64) on each index and keep a cached copy of the other side's index."}
]}

Topics #019 AI Code Review Algo Trading C++ Concurrency Hard Low Latency