Repository navigation
fix: cap in-process PDF URL downloads and clarify trust_urls docs - #1679
Closed
HarshRajSinghania wants to merge 4 commits into
Closed
HarshRajSinghania wants to merge 4 commits into
HarshRajSinghania wants to merge 4 commits into
Conversation
added 4 commits
October 12, 2026 13:08
Add a default 32 MB limit (configurable via max_bytes) to in-process URL downloads in encode(), streaming the response in chunks instead of a single read() to prevent unbounded memory use. Update modalities.md to accurately describe that trust_urls=False only covers railtracks' own fetches; some provider/litellm paths may still fetch certain image URLs. Closes the immediate items in RailtownAI#1646. The broader single-fetch-policy work remains in RailtownAI#1347.
Replace placeholder with full implementation: stream URL downloads with 32MB default max_bytes limit in encode(). Update modalities.md for accurate trust_urls description. Add regression test for size limit.
Collaborator
|
Thanks for picking this up, @HarshRajSinghania! There was already a draft for this issue, #1677, opened before yours. It covers the same two items and also passes the cap through Attachment/UserMessage as attachment_max_mb, which #1646 asked for. So I'm closing this one as a duplicate to avoid two competing fixes. Sorry for the overlap; the draft wasn't linked on the issue, so it was easy to miss. If you'd like another one, the issues labelled good first issue are open, and please leave a comment on the one you pick so we can assign it to you. Cheers! |
Collaborator
|
The PR will be closed in 1 day, unless you have a different opinion that you wish to discuss =D |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the two concrete items in #1646:
Size-limited streaming download for URL attachments.
encode()(used for in-process PDF URL downloads whentrust_urls=True) now reads the response in 8 KiB chunks and enforces a default maximum of 32 MB (DEFAULT_MAX_DOWNLOAD_BYTES). The limit is configurable via the newmax_byteskeyword argument. Exceeding it raisesValueErrorbefore the full body is materialised in memory.Accurate
trust_urlsdocumentation. Updateddocs/documentation/agent_design/llms/modalities.mdso it no longer claims that image URLs never perform in-process network I/O. It now states thattrust_urls=Falseonly covers railtracks' own fetches (PDF downloads and HEAD probes), while certain litellm/provider paths may still fetch some image URLs.The broader single-fetch-policy work that would also cover litellm remains tracked under #1347.
Motivation
Issue #1646 (help wanted / good first issue) notes that the PDF download path does an unbounded
url.read()and that the docs overstate the SSRF protection provided bytrust_urls=False.Implementation notes
Testing
Result: 39 passed.
No other files were modified. Existing functionality for local files, data URIs, and error paths is preserved.