fix(trading): page order history by a real keyset cursor, not the same "0:" forever

[cardosofede]

Problem

Paging POST /trading/orders/search looped forever. An agent asking for order history with limit=3 got page one, followed next_cursor, and got most of page one back, with the same cursor, on every later call.

Root cause. The route built each order's cursor from timestamp and client_order_id, but OrderRepository.to_dict returns created_at and order_id. Every row got the cursor "0:" and sorted as a tie. The page after "0:" was the list minus its first row, and it handed "0:" back.

A second bug behind it. Each account was read as its newest limit * 2 rows at offset 0, then merged and sliced in memory. So even a correct cursor could never reach an older order, and has_more said history had ended. Filters with more than one connector_names/trading_pairs value ran in memory after that cap, and total_count counted the rows fetched, not the orders that match.

Fix

Pagination now happens in the database, keyset on (created_at, client_order_id):

• OrderRepository.get_orders takes the account, connector and pair filters as lists, plus a before cursor. It orders by created_at DESC, client_order_id DESC and returns only orders strictly after the cursor, so orders with the same created_at are never skipped or served twice. The cut is a row-value comparison, (created_at, client_order_id) < (t, id). PostgreSQL derives created_at <= t from it as a range bound on ix_orders_created_at, so a deep page costs the same as page one. The equivalent OR would run as a filter that walks every newer order first. The new count_orders uses the same filters, so total_count is real.
• The route runs one query across all accounts and fetches limit + 1 rows; the extra row is how it knows has_more. next_cursor is "<created_at ISO>|<order id>", split back on the first |, which an ISO timestamp never contains.
• A malformed cursor is refused with a 400, including the old "0:". It used to silently restart at page one, which a client walking the history can't tell apart from older orders. Only the format is checked. A well-formed cursor is a keyset position and pages back from that point, whether or not the order it names exists.
• TradingHistoryService.get_orders → search_orders, which returns the page and the total from one session. The route was its only caller.

routers/trading.py also clears the flake8 findings it already had on main: unused imports, inline import logging in except blocks, untyped connector_service = Depends(...) parameters, and over-long lines. The pre-commit flake8 hook lints the whole file, so no commit touching it could pass without this. It's formatting only; nothing outside the order search route behaves differently.

Compatibility

• The response shape is unchanged: data, pagination.{limit, has_more, next_cursor, total_count}.
• Cursors keep their contract (pass back next_cursor), but their format changed. A client holding an old "0:" cursor now gets a 400 instead of a repeated page.
• total_count is now every order that matches the filters, not the size of the fetched window.

Testing

• The new test/test_order_history_cursor.py runs the real route, service and repository against a real SQL table: sync SQLite behind an async shim, since the env has no aiosqlite. It follows the cursor the way a client does, at page sizes 1, 2, 3, 4, 10, 11 and 100. On the old route every walk looped on "0:". Now each order comes back exactly once, newest first. The tests also cover orders past 2 × limit, filters with more than one value, total_count, and the 400 for unknown cursors.
• Full suite: 846 passed (829 before, plus 17 new). flake8 (--max-line-length=130) and isort are clean on every file touched, and the pre-commit gate passed without --no-verify.
• PostgreSQL 16, 1M seeded orders. The first version's OR cut discarded 950k rows on a page 95% deep, taking 107 ms. The row-value cut plans as Index Cond: created_at <= … and discards 5 rows, taking 0.06–0.08 ms at 5%, 50% and 95% depth. A 2,000-order walk at limit=7 over tied created_at values matched a direct ordered read exactly, with no repeats. A composite (created_at, client_order_id) index was tried, but the planner didn't use it, so it isn't added.

Follow-ups (found here, not fixed)

1. Units for start_time and end_time disagree. hummingbot-api-client documents them as Unix seconds. OrderRepository divides by 1000, so it expects milliseconds. A seconds value filters from 1970.
2. POST /trading/trades has the same limit * 2 window. Its cursor is built from fields the rows actually have, so it doesn't loop, but it still can't page past the window.

🤖 Generated with Claude Code

https://claude.ai/code/session_015UhbCLq9uRjJ1bWYDr59da

[greptile-apps]

Confidence Score: 4/5

The PR is not yet safe to merge because the outstanding cursor-validation defect allows fabricated cursors to silently skip newer matching orders.

Findings

1.  Unknown cursors are accepted ▶

Summary

• Pages orders by (created_at, client_order_id) across the selected accounts.
• Fetches one extra row to determine has_more and reports the full matching count.
• Adds route-through-repository tests for cursor walking, tie-breaking, filters, counts, and malformed cursors.
• Changes since the previous review optimize the keyset predicate with a SQL row-value comparison but deliberately continue accepting well-formed cursors that the route never issued.

Diagram

sequenceDiagram
    participant Client
    participant Route as Orders Search Route
    participant Service as TradingHistoryService
    participant Repo as OrderRepository
    participant DB

    Client->>Route: filters, limit, optional cursor
    Route->>Route: Parse cursor into created_at and order_id
    Route->>Service: search_orders(filters, limit + 1, before)
    Service->>Repo: get_orders(filters, before)
    Repo->>DB: Keyset query ordered by created_at and client_order_id DESC
    DB-->>Repo: At most limit + 1 orders
    Service->>Repo: count_orders(filters)
    Repo->>DB: COUNT matching orders
    DB-->>Repo: total_count
    Service-->>Route: orders and total_count
    Route-->>Client: page, has_more, next_cursor, total_count

Loading

_{Reviews (2) · Last reviewed commit: "fix(trading): bound the order cursor in ..."}

[greptile-apps]

Unknown cursors are accepted

The parser accepts any cursor containing a valid ISO timestamp and a nonempty ID, even if the route never issued it. For example, 2026-09-11T10:00:00|unknown is used directly as the database boundary, so the response can silently omit newer orders instead of returning the promised 400. Validate that the cursor identifies an existing order in the applicable result set, or use an opaque authenticated cursor.

[cardosofede]

Fixed in 4dab50a by narrowing the documented contract, not by validating existence. A cursor is a keyset position, not a token: a well-formed <created_at ISO>|<order id> pages back from that point whether or not the order exists, the same way the repository's before bound works. Only a malformed cursor gets a 400. Looking the order up would cost a query per page and add nothing for a client that follows next_cursor. The route and _parse_order_cursor docstrings now say this, and test_a_well_formed_cursor_is_a_position_even_for_an_order_that_does_not_exist pins it.

[david-hummingbot]

The keyset cursor is applied as a filter, not an index bound — deep pages cost ~5× page one

Testing this branch at volume: pagination is correct throughout (the cursor walk is clean at every page size I tried, including against real PostgreSQL timestamptz), but the cursor cut in OrderRepository.get_orders is written as an OR:

or_(
    Order.created_at < created_at,
    and_(Order.created_at == created_at, Order.client_order_id < client_order_id),
)

PostgreSQL applies that as a filter over an index scan rather than as an index range bound, so each page walks backward from the newest row and discards everything newer than the cursor before it can return limit rows. The cost is linear in how deep the client has paged, which is the O(offset) behaviour keyset pagination is meant to remove.

Measured on 1,000,000 orders:

cursor depth	execution time	rows discarded by the filter
5%	4.6 ms	50,061
25%	23.7 ms	250,061
50%	46.6 ms	500,061
75%	58.9 ms	750,061
95%	88.5 ms	950,061

End to end through POST /trading/orders/search at limit=100, five runs each: page 1 ≈ 38 ms, deep page ≈ 190 ms.

Two smaller things noticed alongside it: count_orders re-counts the full match set on every page (25–38 ms unfiltered at 1M rows, ~50 ms with a connector filter — most of page one's cost), and there is no composite index on the sort columns; only single-column ix_orders_created_at and the unique ix_orders_client_order_id.

---

Steps to reproduce

1. Bring up the stack (from this branch) and seed 1M orders:

INSERT INTO orders (
    client_order_id, exchange_order_id, created_at, updated_at,
    account_name, connector_name, trading_pair,
    trade_type, order_type, amount, price, status, filled_amount
)
SELECT
    format('vol-%s', lpad(i::text, 8, '0')),
    format('ex-%s', i),
    TIMESTAMPTZ '2025-01-01 00:00:00+00' + ((i / 5) * INTERVAL '2 seconds'),
    TIMESTAMPTZ '2025-01-01 00:00:00+00' + ((i / 5) * INTERVAL '2 seconds'),
    'acct-' || (i % 4),
    (ARRAY['binance','kucoin','okx','bybit'])[1 + (i % 4)],
    (ARRAY['BTC-USDT','ETH-USDT','SOL-USDT'])[1 + (i % 3)],
    CASE WHEN i % 2 = 0 THEN 'BUY' ELSE 'SELL' END,
    'LIMIT', 1.0, 100 + (i % 1000),
    (ARRAY['FILLED','CANCELLED','OPEN'])[1 + (i % 3)],
    0
FROM generate_series(1, 1000000) AS i;

ANALYZE orders;

That spans 2025-01-01 00:00:00+00 to 2025-01-05 15:06:40+00, five orders per timestamp.

2. Plan the query the route issues, with a cursor deep in the history:

EXPLAIN (ANALYZE, BUFFERS) SELECT * FROM orders
WHERE account_name IN ('acct-0','acct-1','acct-2','acct-3')
  AND (created_at < TIMESTAMPTZ '2025-01-01 05:33:20+00'
       OR (created_at = TIMESTAMPTZ '2025-01-01 05:33:20+00'
           AND client_order_id < 'vol-00000001'))
ORDER BY created_at DESC, client_order_id DESC LIMIT 11;

Observed:

Index Scan Backward using ix_orders_created_at on orders
  Rows Removed by Filter: 950061
Execution Time: 88.499 ms

Sweep the cursor timestamp across the seeded range to see the cost track depth linearly.

3. Confirm it end to end. Page one:

curl -s -u admin:PASSWORD -X POST http://localhost:8000/trading/orders/search \
  -H 'Content-Type: application/json' \
  -d '{"limit":100,"account_names":["acct-0","acct-1","acct-2","acct-3"]}'

then the same request with "cursor":"2025-01-01T05:33:20+00:00|vol-00000001". Page 1 lands ~38 ms, the deep page ~190 ms.

One measurement note that cost me an hour: when comparing two containers, address them in-network by container name rather than through published host ports. On WSL2 a published port answered from a different container than the one bound to it, which produced a complete set of plausible but wrong numbers. Worth an identity check (assert on something only the build under test produces) before trusting any timing.

Correctness is unaffected — every walk I ran returned each order exactly once, newest first, including with ties on created_at to the microsecond.

[cardosofede]

@david-hummingbot thanks for the numbers and the repro. Fixed in 4dab50a.

The cut is now a row-value comparison, tuple_(Order.created_at, Order.client_order_id) < tuple_(t, id). PostgreSQL derives created_at <= t from it as an index condition, so the scan starts at the cursor. Re-run on PostgreSQL 16 with your 1M-order seed and your query shape:

cursor depth	old OR cut	row-value cut
5%	5.1 ms, 50,001 rows removed	0.06 ms, 5 removed
50%	50.8 ms, 500,001 rows removed	0.06 ms, 5 removed
95%	106.8 ms, 950,001 rows removed	0.06 ms, 5 removed

Through the repository at limit=101, page 1 and a 95%-deep page both take about 2 ms. A 2,000-order walk at limit=7 over tied created_at values matched a direct ordered read exactly, with no repeats.

On the two side notes:

• Composite index: tried (created_at, client_order_id). The planner kept ix_orders_created_at with the row-value cut, and an incremental sort handles the ties, so the index isn't added.
• count_orders on every page: left as is for now. Its cost doesn't grow with depth, and the only way to drop it is to return total_count only on page one, which changes the response contract. Happy to do that as a follow-up if we want it.