Skip to content

fix(serve): cap on-demand renders per /search request - #164

Merged
ASuresh0524 merged 1 commit into
StarTrail-org:mainfrom
dex0shubham:fix/serve-ondemand-render-budget
Oct 1, 2026
Merged

ASuresh0524 merged 1 commit into
StarTrail-org:mainfrom
dex0shubham:fix/serve-ondemand-render-budget

Conversation

@dex0shubham

Copy link
Copy Markdown
Contributor

Problem

With render-on-demand enabled (--kiwix-url), /search has no bound on how
much work one request can queue. In the hit loop:

elif req.include_images and _state.get("ondemand") is not None:
    img_b64 = await asyncio.to_thread(_ondemand_chunk_b64, aid, ti, ci, th)

For every hit whose tile isn't on disk that reaches
OnDemandTiles.chunk_path → _render_and_chunk, which launches a Chrome page
render
, serialized on a process-global _render_lock and bounded only by
PIXELRAG_RENDER_TIMEOUT (120s by default).

Nothing capped how many of those a single request may ask for. At this
endpoint's own limits — 32 queries (#154) and n_docs up to 1000 (#122) — one
{"include_images": true} request can queue tens of thousands of serialized
renders. The handler awaits them one at a time, so the request occupies a task
for as long as that takes, and because the lock is process-wide, every other
caller's renders stall behind it. The endpoint is public and unauthenticated.

This is the same family as the n_docs and query-image bounds, but a bound on
time rather than memory — the amplification those two didn't close.

Fix

A per-request render budget (16). Past it the hit is still returned, just
without image_base64 — already what a failed render or a missing tile yields,
so no new client contract — and the client can fetch it from
/tile/{article_id}/{tile_index}/{chunk_index}. A real caller asks for
n_docs=10, so a legitimate request never reaches the cap.

Cache hits must not spend the budget. In a warm deployment most calls are
hits that cost a single stat, and charging them would withhold images from
already-rendered articles for no reason. OnDemandTiles grows
cached_chunk_path, which reports an already-rendered chunk without
rendering, so the handler can tell a hit from a render before committing to one.
chunk_path now routes through it, with the chunk filename built in one place
(_chunk_file) instead of two.

Tests

tests/test_serve_ondemand_budget.py drives the real handler through
TestClient with pre-computed query embeddings — no model, no index, no Chrome —
against a counting stand-in for OnDemandTiles:

  • renders stop at the budget, and every hit is still returned;
  • the budget covers the whole request, not one per query;
  • an all-cached corpus spends zero budget.

Removing the budget makes them fail with the amplification itself: 40 != 16
for one query, 160 != 16 for four — and that is at n_docs=40, far below what
the endpoint permits.

Full suite: 165 passed, 1 skipped (--extra serve --extra qdrant).

Not addressed

The global lock still serializes renders across requests, so a queue of
modest requests can pile up even with each one budgeted. Bounding queue depth,
or making renders concurrent, is a larger design change and I'd rather not
half-build it here — happy to follow up if you'd like a particular shape.

Also left alone: _MAX_ONDEMAND_RENDERS is a module constant, not an env knob
like PIXELRAG_RENDER_TIMEOUT. Easy to change if you'd prefer operators tune it.

With render-on-demand enabled (--kiwix-url), every hit whose tile is not
on disk triggers a Chrome page render inside OnDemandTiles.chunk_path —
serialized on a process-global lock and bounded only by
PIXELRAG_RENDER_TIMEOUT (120s by default). Nothing capped how many one
request could ask for. At this endpoint's own limits (32 queries from
StarTrail-org#154, n_docs <= 1000 from StarTrail-org#122) a single include_images request could
queue tens of thousands of renders, and because the lock is process-wide
it stalls every other caller's renders too. No auth stands in front of it.

Budget the renders per request (16). Past it a hit is returned without
its image, which is already what a failed render or a missing tile
yields, and the client can still fetch it from /tile.

Cache hits must not spend the budget — in a warm deployment most calls
are hits and cost only a stat. OnDemandTiles grows `cached_chunk_path`,
which reports an already-rendered chunk without rendering, so the handler
can tell the two apart before committing; `chunk_path` now routes through
it, with the filename built in one place (`_chunk_file`).

Not addressed: the global lock still serializes renders across requests,
so a queue of modest requests can still pile up. Bounding queue depth or
making renders concurrent is a larger design change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Sep 29, 2026

Copy link
Copy Markdown

@dex0shubham is attempting to deploy a commit to the andylizf's projects Team on Vercel.

A member of the Team first needs to authorize it.

@ASuresh0524
ASuresh0524 merged commit c6abea3 into StarTrail-org:main Oct 1, 2026
4 of 5 checks 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.

2 participants