Repository navigation
Return full CSV search results up to 100,000 rows; cap JSON limit at 5,000 - #20
Conversation
Without an explicit limit, a CSV search was cut at AUTO_PAGINATION_THRESHOLD (5000) with no sign it was incomplete. CSV now streams every match through a server-side cursor, up to CSV_MAX_ROWS (default 100,000). A query that matches more, or an explicit limit above the cap, is rejected with a 400 instead of returning a partial file. CSV no longer runs the COUNT query, whose result it never used. Closes #18
Streaming straight from the cursor held a pooled DB connection, inside a transaction, for as long as the client took to read the response. Ten slow clients exhausted the pool and stalled every endpoint. Write the CSV to a SpooledTemporaryFile (in memory up to 8 MB, then on disk) and release the connection before sending. A DB error now returns a 500 instead of a truncated file. The file is closed in a BackgroundTask, which also runs when the client disconnects.
matthewberry
left a comment
matthewberry
left a comment
There was a problem hiding this comment.
The spooling change fixes the connection-holding problem, and errors now come back as JSON 500s. All 8 tests pass at c990f02. I also checked that the background close runs on disconnect with what the Dockerfile ships: uvicorn 0.34.2 reports ASGI spec 2.3, so Starlette takes the task-group path, which still runs background (starlette/responses.py:262-273).
Three comments below belong in this PR. The fourth one, on write_csv, doesn't block it. The rich.print debug output on this path predates the PR, so it's filed separately as #22.
One open question I can't check from here: no bytes are sent until the full query and the whole file are done, so a proxy with a read timeout in front of production (nginx defaults to 60s) could cut off a slow full-cap export before anything is sent. Worth knowing the ingress timeout before this ships.
| yield record | ||
|
|
||
|
|
||
| async def has_more_rows_than(db: Database, params: CLEANSearchQueryParams, n: int) -> bool: |
There was a problem hiding this comment.
The cap check and the export are still two separate queries. If the data changes between has_more_rows_than and the cursor opening (for example, a materialized view refresh), the export's LIMIT cap can silently drop rows, which is the problem #18 is about. Every result under the cap is also scanned twice (36k matches for ec_number=1.1.1.1).
Spooling makes this cheap to fix: nothing is sent until write_csv returns, so the cap can be enforced during the write. Query LIMIT cap + 1, and if row cap+1 arrives, close the file and raise the 400. That removes has_more_rows_than, the race, and the second scan.
There was a problem hiding this comment.
Agreed, and done in 12eb4f0. When no limit is given, the export now requests cap + 1 rows and write_csv returns the 400 if the extra row arrives, so the cap is checked against the same rows that get written. has_more_rows_than is gone, and so is the OFFSET clamp it needed after #25.
Both row generators are now closed with contextlib.aclosing, so a 400 halfway through releases the cursor's connection right away instead of when the garbage collector runs. test_write_csv_closes_rows_when_over_cap and test_closing_iter_filtered_data_closes_the_cursor each fail if their aclosing is removed.
On Postgres 16 with 100,001 seeded rows: exactly 100,000 matches returns all of them; 100,001 returns the 400; 15 rejected exports in parallel all returned 400, and afterwards all 10 pool connections were idle.
Trade-off: rejecting an over-cap query now costs a full read of 100,001 rows (1.6 s locally) instead of the old 0.12 s EXISTS check. That's the same cost as a legitimate limit=100000 export, so it doesn't add a new kind of expensive request (numbers in the write_csv thread).
| detail=f"Query matches more than {max_rows} records, the most a CSV " | ||
| "response can contain. Narrow the filters, or page with limit and offset.", | ||
| ) | ||
| elif params.limit > max_rows: |
There was a problem hiding this comment.
A negative limit or offset returns a 500 containing the raw Postgres error ("LIMIT must not be negative"). Only limit > max_rows is rejected here. Adding ge=0 to the limit/offset Query(...) declarations in parse_query_params would turn these into 422 validation errors for both CSV and JSON. main behaves the same way, but this PR adds the validation branch, so it fits here.
| output = StringIO() | ||
| max_rows = settings.CSV_MAX_ROWS | ||
| if params.limit is None: | ||
| params.limit = max_rows |
There was a problem hiding this comment.
This overwrites the caller's limit with the cap. After this line, "no limit given" and "limit=100000 given" look the same, which is the same pattern as the old AUTO_PAGINATION_THRESHOLD default that made a capped file look complete. If the cap moves into the write (see the queries.py:194 comment), passing the cap to iter_filtered_data as its own argument would keep params as the caller sent it.
There was a problem hiding this comment.
Agreed, done in 12eb4f0. The CSV branch no longer assigns to params.limit; it passes the export params.model_copy(update={"limit": MAX_SEARCH_LIMIT + 1}), so the caller's params keep their limit of None.
| output, fieldnames=[c.value for c in CLEANColumn], extrasaction="ignore" | ||
| ) | ||
| writer.writeheader() | ||
| async for row in rows: |
There was a problem hiding this comment.
Non-blocking: once the output passes CSV_SPOOL_MAX_MEMORY and moves to disk, each writer.writerow is a blocking file write inside async code. The cursor hands control back to the event loop every 1000 rows, so the impact is small. If large exports turn out to slow down other requests, move the write into a thread (run_in_threadpool) or write in batches.
There was a problem hiding this comment.
Declining the threadpool change here, but this matters more than I first thought. I measured it on Postgres 16 with 100,001 seeded rows, sending a JSON request 2 s into a burst of 15 parallel requests:
| Burst of 15 | JSON request during burst |
|---|---|
CSV limit=100000 |
6.9 s |
| CSV with no limit, over the cap (400) | 10.4 s |
JSON limit=100000 (allowed on main since #25) |
30.4 s |
Most of that time is Python CPU (building rows and formatting the CSV, or Pydantic models for JSON) on the event loop, not file writes, so moving the write into a thread wouldn't help much because of the GIL. JSON is the worst case and isn't touched by this PR. The fix I'd suggest is a limit on how many large /search requests run at once, for JSON as well as CSV; I'll follow up on that separately.
# Conflicts: # tests/test_search_csv.py
Replace the configurable CSV_MAX_ROWS with MAX_SEARCH_LIMIT, the same bound #25 put on limit. An explicit limit above it already gets a 422, so drop the CSV-only 400 for that case and its test. Clamp the cap check's OFFSET to MAX_BIGINT: with offset allowed up to the bigint maximum, offset + cap overflowed and Postgres returned a 500.
The cap check and the export were two queries, so data changing between them could let the export's LIMIT silently drop rows, and every result under the cap was scanned twice. Fetch cap + 1 rows and return the 400 from write_csv if the extra row arrives. This removes has_more_rows_than and its OFFSET clamp, and leaves the caller's params unchanged. Close the row generators explicitly with aclosing, so stopping early releases the cursor's connection immediately rather than whenever the garbage collector runs.
|
Thanks, and replies are inline. Since this review, Proxy timeout: the ingress is Traefik ( #22: already closed. |
JSON is paginated, so an explicit JSON limit above AUTO_PAGINATION_THRESHOLD (5000) now gets a 422 shaped like the le= error on limit. CSV, the bulk download, keeps the 100,000 cap. A JSON request builds a Pydantic object per row and keeps the whole response in memory, so 100,000-row JSON requests were the most expensive /search requests.
matthewberry
changed the title
Closes #18.
Problem
A CSV search without
limitwas cut atAUTO_PAGINATION_THRESHOLD(5000) with no total and no next link, so the file looked complete when it wasn't. In productionec_number=1.1.1.1alone matches 36,682 records. Every CSV request also ran theCOUNTquery and discarded the result.Changes
limit: CSV now returns every matching row, up toMAX_SEARCH_LIMIT(100,000), the same bound Bound limit and offset on search and typeahead #25 put onlimit. If the query matches more than that, it returns a 400 asking the caller to narrow the filters or page withlimit/offset; it never returns a partial file. The export requestscap + 1rows andwrite_csvreturns the 400 if the extra row arrives, so the cap is checked against the same rows that are written, in one query.Database.iterate, prefetch 1000) into aSpooledTemporaryFile(in memory up to 8 MB, then on disk). The DB connection is released before the response starts, so a slow client never holds it; the file is closed by aBackgroundTask, which also runs when the client disconnects. The row generators are closed withaclosing, so stopping early (a 400 or an error) releases the cursor's connection immediately. A DB error now returns a 500 instead of a truncated file.limit: returned as before; Bound limit and offset on search and typeahead #25 already rejects one above 100,000 with a 422.limitcapped at the page size: an explicit JSONlimitaboveAUTO_PAGINATION_THRESHOLD(5,000) now gets a 422, shaped like Bound limit and offset on search and typeahead #25'sle=error. JSON is paginated, so nothing becomes unreachable; CSV, the bulk download, keeps 100,000. This updates Bound limit and offset on search and typeahead #25's boundary test (JSON accepts 5,000 and rejects 5,001). The frontend only sends 10, 20, or 50; API clients sending a larger JSONlimitwill get a 422.COUNTfor CSV. The JSON path is unchanged.get_filtered_data's query building is pulled intobuild_data_query, shared with the newiter_filtered_data.HTTPExceptionnow passes through the endpoint's catch-all instead of being rewrapped as a 500.main.pyexplain how CSV behaves.Testing
tests/test_search_csv.py: 9 tests with the DB calls mocked, covering the cap, the explicit-limit path, the header-only empty result, the row generators being closed when the cap is passed (both fail if theiraclosingis removed), a DB error partway through the rows (500), output large enough to spill to disk, and that CSV skipsget_total_countandget_filtered_data. Full suite (with Bound limit and offset on search and typeahead #25's tests): 25 passed.cleandbschema (250 seeded records, and 100,001 for the cap):offset=1the 100,000 remaining rows are returnedoffsetat the bigint maximum, and at maximum − 99,999, returns an empty CSV (an earlier version of the cap check returned a 500 "bigint out of range" here)idlelimit=100001gets Bound limit and offset on search and typeahead #25's 422limit/offset, the EC wildcard filter withordering, an empty result, and the JSON path all behaved correctlycurl --limit-rate 2k, more than the pool's 10 connections) downloading 20 MB responses (with an earlier configurable cap set to 100): all DB connections stayedidleand a JSON request answered in 12 ms. Each download had one spill file open, and all 15 closed within 2 s of the clients disconnecting, with no other trafficTrade-off
The first byte is sent only after the query has finished, since the whole CSV is written before the response starts. That is what keeps client speed from deciding how long a DB connection is held.
An earlier version of this PR streamed straight from the cursor. With 10 slow clients connected, all 10 pool connections sat
idle in transactionand a JSON request got no response within 15 s.Rejecting an over-cap query reads 100,001 rows before answering (1.6 s locally), the same cost as a legitimate
limit=100000export. Large requests are CPU-bound on the event loop: a JSON request sent during a burst of 15 parallellimit=100000CSV exports took 6.9 s, and during 15 JSONlimit=100000requests it took 30.4 s, which the JSON cap above removes. Limiting how many large/searchrequests run at once is #26.