Skip to content

fix: cap explicit page sizes and stop printing query results - #8

Merged
matthewberry merged 2 commits into
mainfrom
mjberry/fix/cap-page-size
Nov 4, 2026
Merged

matthewberry merged 2 commits into
mainfrom
mjberry/fix/cap-page-size

Conversation

@matthewberry

@matthewberry matthewberry commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Why

On 2026-11-04 this service was the container OOM-killed at 15:31 UTC while its node (mmli2-worker-cpu-03) went NotReady, evicting every pod on it. Two things in the /api/v1/data path make one request expensive:

  • An explicit limit had no maximum. Auto-pagination only applied when limit was omitted (app/routers/data.py), so ?limit=100000 loaded all 35,753 records.
  • Every result set was printed. get_filtered_data ran rich.print(f"{records=}") (app/db/queries.py:143). Measured locally with synthetic records shaped like the real ones, the print peaks at about 260 MiB for 5,000 records and about 1.6 GiB for the full table. It also wrote 33k log lines per page.

#7 adds a 1 GiB memory limit so this container, not the node, absorbs the next mistake. This PR removes the cause.

What

  • An explicit limit of AUTO_PAGINATION_THRESHOLD (5000) or more is now treated like a missing one: the query is limited to 5000, and the response sets auto_paginated with next/previous links. A limit below 5000 behaves as before.
  • Every page keeps its links. The next link carries limit=5000, which previously wasn't treated as paginated, so following next stopped after page 2 (true before this PR for clients that omitted limit).
  • A capped CSV response now says so: the links go in a Link header (rel="next", rel="prev") and the total in X-Total-Count. The link-building moved above the format branch so both formats share it.
  • The rich.print of query results is gone. (rich stays in pyproject.toml; the SQL tests use it.)
  • The API description and the /data docstring describe the cap and the CSV headers.

This changes behavior for clients that send limit above 5000: they get 5000 records and a next link instead of everything.

Testing

New tests in tests/test_auto_pagination.py, each failing without the fix:

  • ?limit=1000 with the threshold at 10 sends LIMIT 10 to the database and returns auto_paginated with a next link.
  • Query results are not written to stdout.
  • test_pagination_navigation (previously skipped) follows next from the first page to the last: offsets 0, 10, 20 with 25 records.
  • ?format=csv&limit=1000 returns X-Total-Count: 100 and a Link header with the next page, keeping format=csv.

Full suite: 23 passed. Before: 19 passed, 1 skipped.

Not handled: the app's CORSMiddleware sets no expose_headers, so browser JavaScript on another origin can't read Link or X-Total-Count. Scripts and plain downloads aren't affected.

Note: app.main's lifespan connects to a real database even though these tests mock every query, so they need a reachable Postgres. I ran them against a throwaway local postgres:16-alpine with PYTHONPATH=..

Without the print, a 5,000-record page peaks at about 21 MiB and the full table at about 71 MiB in plain Python (synthetic records, before FastAPI's encoding), well under #7's 1 GiB limit.

Deploying

Merging builds moleculemaker/mmli-oed-api:latest. The Deployment pulls :latest with imagePullPolicy: Always, but nothing in its spec changes, so the new code is live only after the pod restarts (kubectl -n openenzymedb rollout restart deploy/mmli-oed-api, after the image build finishes).

An explicit /api/v1/data limit had no maximum, so one request could load all 35,753
records, and get_filtered_data printed every record with rich.print, which peaks at
about 1.6 GiB for the full table. On 2026-11-04 this service was OOM-killed while its
node went NotReady. Cap an explicit limit at AUTO_PAGINATION_THRESHOLD the same way a
missing one already is, with next/previous links, and drop the print.

@matthewberry matthewberry left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced the PR's claims against a throwaway Postgres: the full suite gives 21 passed, 1 skipped, and with app/ reverted to origin/main both new tests fail.

Two problems worth fixing before merge. Both are in code this PR doesn't touch, but the PR now sends every client asking for limit > 5000 through it.

1. Following next stops after page 2 (reproduced). The next link carries limit=5000. On that request, params.limit > settings.AUTO_PAGINATION_THRESHOLD (app/routers/data.py:198) is false, so the response has no auto_paginated and no next. With the threshold monkeypatched to 10:

PAGE1 next: http://testserver/api/v1/data?format=json&offset=10&limit=10
PAGE2 keys: ['limit', 'offset', 'total']  next: None

This was already broken for clients that omit limit. A client that follows next gets two pages and then a response that looks like the end of the data, unless it compares offset + limit to total. The PR body's "auto_paginated with next/previous links" only holds for the first page.

  • Suggested fix: params.limit >= settings.AUTO_PAGINATION_THRESHOLD, so pages after the first keep their links.
  • Test: one that follows next twice. The skipped test_pagination_navigation was meant to cover this.

2. CSV results are cut off with no signal (reproduced). ?format=csv&limit=1000 with the threshold at 10 returned 10 rows. The response headers were only content-disposition, content-type, vary and content-encoding, because the CSV branch never adds pagination metadata. Before this PR, an explicit large limit was how a CSV client got everything from the API. Now it gets 5,000 rows and can't tell anything is missing. The full-dataset .gz downloads in the API description reduce the impact.

  • Suggested fix: add a Link: <…>; rel="next" or X-Total-Count header to the CSV response, or at least document that CSV output is capped.

Didn't investigate: who calls /data with limit above 5000. If the OED frontend or a known script relies on uncapped limits, this change breaks it. Worth checking before merging.

…es them too

The next link carries limit=threshold, and a limit equal to the threshold was not
treated as paginated, so following next stopped after page 2. Use >=.

A CSV response has no body metadata, so a capped one gave no sign anything was
missing. Send the next/prev links in a Link header and the total in X-Total-Count.
The link-building moves above the format branch so both formats share it.

Implements the skipped test_pagination_navigation.
@matthewberry

Copy link
Copy Markdown
Contributor Author

Both fixed in ede0f1c.

1. next stopping after page 2. The threshold check is now params.limit >= settings.AUTO_PAGINATION_THRESHOLD, so a page requested with limit=5000 is still paginated and has its own links. test_pagination_navigation is no longer skipped: with 25 records and the threshold at 10, it follows next from the first page and sees offsets 0, 10, 20, then a last page with previous and no next. It fails without the change ([0, 10]). Side effect: an explicit limit=5000 now also gets auto_paginated and links; the rows returned are the same.

2. Capped CSV. CSV responses now carry X-Total-Count, plus a Link header with rel="next"/rel="prev" when the result is paginated (the links keep format=csv). To share them between formats, the link-building moved above the format branch; the JSON fields are unchanged. New test test_capped_csv_says_there_is_more checks the headers on ?format=csv&limit=1000. The API description and the /data docstring describe the headers. One gap, noted in the PR body: CORSMiddleware sets no expose_headers, so browser JavaScript on another origin can't read the new headers.

Who calls /data with a large limit. Within moleculemaker, the only reference to openenzymedb-api.platform.moleculemaker.org outside this repo is a link to the API docs on the OpenEnzymeDB frontend's About page (src/app/about-page/about-page.component.html), from a GitHub code search. oed-toolkit doesn't call the API. So no known internal caller depends on uncapped limits; the only caller I know of is the external client paging through /data on 2026-11-04, and I can't tell who that is because traefik has no access log.

Full suite: 23 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant