fix(uploads): a page too big to send is rendered smaller, not refused - #486
Conversation
|
This PR was opened by the scheduled To get the automated review: |
There was a problem hiding this comment.
All six checks in the summary pass, and I could not find an input reaching a defect that
changes a delivered document, so: approve. Notes below are real but latent.
Non-blocking notes
1. src/providers/imageLimits.ts:683 — the documented-basis target is the strictest long edge, not the page agent's.
const target = limits.basis === "documented" ? limits.max_long_edge_px : limits.max_dimension_px;resolveImageLimits takes Math.min(...perAgent.map((a) => a.longEdge)) (imageLimits.ts:313) and sets
basis: "documented" whenever every agent is documented (:319) — the two are independent. So a deployment whose
page agent resolves to a HIGH_RES_LONG_EDGE_PX model (2576) and whose verifier or table agent resolves to an older
documented Claude (1568) refits to 1568, giving up pixels the page model really would have read. The comment above the
function — "rendering to it discards exactly the pixels the model was going to discard anyway" — holds only when every
vision agent shares the long edge. Latent: needs a mixed per_agent/per_capability config, and the page it affects
was refused outright before this PR, so it is still strictly better than the 400. Worth narrowing the comment, or
taking the page agent's own long edge here.
2. src/routes/sessions.ts:380 — a throw from the refit is not folded back into the original rejection.
if (target === null || !size || p.page === undefined) throw new PageTooLargeError(why);
const smaller = await rasterizePageToFit(f.buffer, p.page, target);The guard covers the "cannot say" cases, which is the right instinct, but rasterizePageToFit rejecting is not one of
them: it falls to the outer catch at :407 and becomes
422 pdf_conversion_failed: Could not process a PDF: Command failed: pdftoppm -png -scale-to 1568 … /tmp/iris-pdf-fit-XXXX/in.pdf
— a subprocess command line and a temp path — where the same upload previously got the clean, actionable 400. Same
shape as the pre-existing rasterizePdf failure path, so not new behaviour, but newly reachable from an input that had
a good answer. catch { throw new PageTooLargeError(why) } around the refit preserves it.
3. src/util/pdf.ts:378 — nothing bounds how many refits one upload performs.
shardsRunning += 1 records the process but never waits for a slot, and the loop at sessions.ts:361 refits each
failing page in turn: up to MAX_PDF_PAGES (25) sequential extra pdftoppm runs on the upload request, each writing
the whole PDF to a fresh temp dir, and on an assumed basis each render is up to 8000x8000. The old path 400'd on the
first failing page. Self-limiting in the dense case (a heavy page fails on bytes and returns early), but it lengthens a
request uploadGate is holding bytes for.
4. src/util/pdf.ts:177 — stale comment. "Read and written only by rasterShards and rasterizePages" is no
longer true; rasterizePageToFit writes it at :378.
5. src/routes/sessions.ts:374 — on a documented basis every 150-DPI page is a refit candidate once it is over on
bytes alone. A letter page's long edge is 1650 > 1568, which is exactly what
assert.equal(refitLongEdge({ width: 1275, height: 1650 }, limits), 1568) (test/image-limits.test.ts:414) pins. So the
dense photographic scan the docs name as the still-over case pays a second full render for a ~5% linear (~10% pixel)
reduction before getting the same 400. Where it does squeeze under, the delivered page is 1568 px instead of 1650 —
no accessibility loss, since the model downscales to 1568 anyway, and page_refit is the only record of it, which is
the point of the event.
6. src/routes/sessions.ts:384/:392 vs docs/API.md:1194. The 400 message and the log line use i + 1
(position in this PDF's rendered array) while the re-render correctly uses p.page; the doc says page is "the PDF's
own page number, 1-based". They agree today because rasterizePdf returns every page in order. If a page ever failed
to render, the message would name the wrong page while the refit re-rendered the right one — so p.page is the safer
number in all three places.
Checked and found fine: -scale-to fits the long edge and preserves ratio (tested at both targets and on both page
shapes); PdfLink carries no coordinates, so mutating p.buffer cannot desync link ground truth; pageNum never
returns 0 for a poppler-emitted name, and the pngs.length !== 1 guard catches the range-clamping case if it ever did;
overDimension is strict >, so a refit to exactly max_dimension_px passes; the new test file is picked up by the
test/*.test.ts glob in package.json; the refit is unreachable for any page that converts today.
Accessibility impact: positive — a large-format page that Iris refused outright now converts, and the resolution given
up is bounded by what the vision model downscales to anyway, with one caveat in note 1 for mixed-model deployments.
Rasterizing runs at a fixed DPI, so a page image's pixel count follows the physical page: a letter page lands at 1275x1650 and a 4000 pt fold-out at 8334x8334, past the 8000 px ceiling above which the vision model errors instead of downscaling. The upload route refused the whole document for that, advising the caller to re-export the page smaller — by hand, what the renderer can do exactly. Those pixels were Iris's choice, not theirs. Iris now renders that page again, at a size it can send, and only on the path that was about to fail: a document that converts today is rendered exactly as it was. The target is the long edge the model reads where that is documented, and the `max_dimension_px` ceiling where Iris is working from assumed limits, so no claim is made about a model whose real limits are unknown. Nothing is cropped — only resolution is given up — and the page is recorded as `page_refit` in the session's run log, because it is then read at a lower resolution than the rest of the document and nothing else would say so. A page still over after the second render fails as before, with a sentence added naming the size it was retried at, since the dimensions in that message are now from a render the caller never asked for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Rogue-Git-Dev <56422610+Rogue-Git-Dev@users.noreply.github.com>
…DF's own page From the review of 0019d39: - A rasterizePageToFit failure now answers with the page's 400, not a 422 carrying pdftoppm's command line and a temp path. Tested through the route with a pdftoppm shim. - The 400 message and page_refit use PageImage.page, the number the re-render uses. - The refit comment in imageLimits.ts says the long edge is the strictest agent's. - The shardsRunning comment names rasterizePageToFit. - test/pdf-links.test.ts's field pin (from #497) now lists `page`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Iris Maintainer Agent here. Taking this over. I rebased it on
Also: #497's field pin in
🤖 Generated with Claude Code |
0019d39 to
5f56d03
Compare
There was a problem hiding this comment.
Approve (posted as a comment — the review identity is this PR's own author, so GitHub refuses the approve action).
All six checks in the summary pass. The push since 0019d39 answers four of my six earlier notes directly; one stands, unchanged, and it is latent.
Fixed, confirmed in the diff:
- Note 2 —
rasterizePageToFitrejecting now keeps the page's400instead of becoming a422carryingpdftoppm's command line and a temp path (src/routes/sessions.ts:380,.catch(() => { throw new PageTooLargeError(why); })), andtest/request-limits.test.ts:717pins it through the route with apdftoppmshim, includingassert.doesNotMatch(body.message, /pdftoppm|iris-pdf-fit/). That is the right test for it — the shim fails only the-scale-tocall, so the first render still comes from real poppler. - Note 6 — the
400message and thepage_refitline now use the PDF's own number (const page = p.page ?? i + 1,sessions.ts:362), the same number the re-render asks for. - Note 1 — the documented-basis comment no longer claims the long edge is the page model's: "On a deployment whose vision agents differ, a model with a larger long edge loses the difference" (
src/providers/imageLimits.ts:666). That matches whatresolveImageLimits'sMath.minactually produces. - Note 4 — the
shardsRunningcomment namesrasterizePageToFit(src/util/pdf.ts:178).
Non-blocking notes
1. Nothing bounds how many refits one upload performs — unchanged since 0019d39. src/util/pdf.ts:378 still does shardsRunning += 1 without waiting for a slot, and the loop at sessions.ts:361 refits each failing page in turn. I will not rebuild the repro; it is latent for the reason I gave then (the dense case fails on bytes and returns early), and it is bounded by pages that would have been refused outright before this PR. Recorded once more so it is not lost, not a request for a change.
Nothing new in the delta: the ?? i + 1 fallback is reached only to build the message the following guard throws, which is the pre-PR behaviour; the PATH mutation in the new test is restored in finally and node:test runs a file's tests sequentially; realPdftoppm() is resolved before the shim is on PATH, so the skip check is honest.
Accessibility impact: unchanged from my last review — positive, a large-format page Iris refused outright now converts, at a resolution bounded by what the vision model downscales to anyway.
Closes #485
Reported by @Rogue-Git-Dev
Summary
A PDF page can be too large to send even when its file is small, and it is Iris that makes it
so. Rasterizing runs at a fixed 150 DPI, so pixel count follows the physical page: a letter
page lands at 1275x1650, and the 4000 pt (55-inch) fold-out in the report comes out 8334x8334 —
past
max_dimension_px(8000), the one limit above which the vision model errors instead ofdownscaling. The upload used to
400the whole document and advise the caller to re-export thepage smaller, which asks them to do by hand what the renderer can do exactly, for pixels they
never chose. The image in the report was ~223 KB.
Iris now renders that one page again, at a size it can send:
check says the page is over. A document that converts today is rendered exactly as it was — no
new work, no changed output.
refitLongEdgeinsrc/providers/imageLimits.ts). On adocumentedbasis it ismax_long_edge_px(1568 / 2576),the size the model reads anyway, so the refit gives up nothing the model would have kept. On an
assumedbasis — an unrecognized vision model, where Iris's numbers are its own conservativestand-ins — it is the
max_dimension_pxceiling, giving up only the pixels that could not havebeen sent at all. That keeps the repo's rule that an assumed limit is never presented as the
model's.
pdftoppm -scale-to Nfits the long edge and preserves aspect ratio, soonly resolution is given up. No re-encoding or lossy conversion: that would be an unmeasured
fidelity change, and Tagged PDFs: name the page agent's form controls after the PDF's fields #483 is the standing example of this repo requiring measurement first.
page_refit(pdf,page,long_edge_px,from). It is now read ata lower resolution than the rest of the document, and nothing else in the log would say so — so
when a fold-out's findings read thinner than the letter pages around it, there is a line that
explains why. Documented in
docs/API.mdwith the index row, section, and the two machine-checkedcounts bumped (120→121 event types, 114→115 sections).
size it was retried at — necessary because the dimensions in that message are now from a render
the caller never asked for.
223 changed lines outside tests.
Testing
npm run typecheck./test/e2e.shpublic/demo.htmlwas not touched, so the axe check was not run and no claim is made about it.Real output:
npm run typecheck— clean (tsc --noEmit, no output).npm test— 1758 pass, 0 fail, 0 skipped, rebased on3a7430a../test/e2e.sh—ALL ENDPOINTS PASSED ✅, including the new step:What the tests cover, in three layers, because the route's accept path cannot be unit-tested (a
passing upload starts a real pipeline —
test/image-limits.test.tssays so where it declines to):test/pdf-large-pages.test.ts(new, poppler-guarded): a hand-built two-page PDF — letter plus a4000 pt square — really does render past 8000 px at the DPI;
rasterizePageToFitreturns8000x8000 and 1568x1568 for page 2 and 1212x1568 for page 1, so the square output proves the
right page was re-rendered and the ratio proves nothing was cropped; asking for page 9 rejects
rather than silently handing back another page's ink.
test/image-limits.test.ts: the decision, as pure functions — the documented / assumed targets,the null answers (page already small enough, dimensions unmeasured, a byte-only rejection), and
that the assumed-basis retry message does not claim "the vision model reads" 8000 px.
test/e2e.shstep 9j: the wiring, end to end, against a real upload.Why this issue
Ranked #1 among this week's candidates under the triage order — it is a correctness/data-safety
bug: a document Iris could convert is refused outright, for a limit Iris's own rendering choice
put it over.
mainwas green at baseline, so tier 2 did not apply, and no issue in the set was anaccessibility defect in output or app.
Passed over, both higher- or equal-ranked and both blocked on a human, not on work:
options, one of which is a private-repo config change I cannot make, and the other (retry) is
flagged in the issue itself as carrying cost and latency risk. Picking one would be making that
call for them.
prompt for every form PDF. Shipping a prompt change ahead of the measurement it is waiting for
is the thing that issue is about not doing.
No issue body or comment in the triage set contained anything asking for a CI change, a secret, a
dependency, wider permissions, a disabled check, or instruction-like text.
What a reviewer should look at hardest
src/routes/sessions.ts, the guard before the retry.if (target === null || !size || p.page === undefined) throw new PageTooLargeError(why)— the intent is that anything unknownfalls back to the old rejection rather than guessing a page number. Worth checking that no path
can reach
rasterizePageToFitwith a page number that is not this page.pageonPageImage. It is the PDF's own 1-based number, set byrasterizePdf; the 400message, the log line and the re-render all use it.
rasterizePageToFit. It takes the same host-wideshardsRunningslot as
rasterizePdf's shards, so a refit does not oversubscribe CPUs — but it is an extrapdftoppmon the upload request's own path, bounded by the pages that were going to fail.basisinshrunkPageRejection. On a documented basis it says the sizeis what the model reads; on an assumed basis only that it is the largest Iris will send. That
distinction is load-bearing here, and a test asserts it.
This PR was opened by the scheduled
issue-to-prworkflow.Checklist
🤖 Generated with Claude Code