fix(413): recover from image-payload 413s instead of bricking the session - #52444
fix(413): recover from image-payload 413s instead of bricking the session#52444Caoxuyang wants to merge 1 commit into
Conversation
…sion A 413 "Request Entity Too Large" is a request-BODY-size error measured in bytes, not a token-count overflow. When the body is bloated by several base64 images embedded via native vision (each individually under the 5 MB single-image ceiling, but collectively megabytes), the existing `payload_too_large` handler only ran TEXT compression — which can't touch image bytes. It would report "cannot compress further" and abort the turn while the real payload (the images) was never reduced. Observed on GitHub Copilot: a session that is only ~20-40 KB of text but carries 4-5 ~500 KB `vision_analyze` screenshots (~2 MB of base64) 413s immediately, then dies at "Payload too large and cannot compress further." Meanwhile a pure-text session at 100 K tokens sends fine — proving the binding constraint is body bytes, not context length. The model's context window resolves correctly (Copilot reports claude-opus-4.8 at 936 K via /models); context-window resolution was never the problem. The image-shrink recovery that already exists for Anthropic's per-image 5 MB ceiling (`FailoverReason.image_too_large`) never fired here because a 413 classifies as `payload_too_large`, and the shrink helper's 4 MB per-image budget no-ops on ~500 KB images anyway. Fix: - `try_shrink_image_parts_in_messages` gains a `target_bytes` override so the aggregate-payload path can demand a much smaller per-image budget than the single-image 4 MB default. - The `payload_too_large` handler now shrinks embedded images BEFORE text compression, in two PROGRESSIVE passes (<=512 KB, then <=256 KB per image) driven by two one-shot guards on `TurnRetryState`. It mirrors the proven `image_too_large` sibling: in-place mutate `api_messages` + `continue` (re-runs `_build_api_kwargs` with the shrunk payload). Only after both image passes are spent — i.e. the body is genuinely text-dominated — does it fall through to the existing text-compression path. Verified end-to-end with 5 real ~300-400 KB screenshots: 1.90 MB payload -> pass 1 -> 1.51 MB -> pass 2 -> 0.89 MB, every image <=256 KB, then clean fall-through to text compression (no infinite loop). Tests: new TestAggregatePayloadShrink covers the default-budget no-op, the small-target-bytes shrink, and the target_bytes<=0 fallback. TurnRetryState field-set + all-False contract tests updated for the two new bool guards.
|
Local validation on PR head python -m pytest tests/agent/test_turn_retry_state.py tests/run_agent/test_image_shrink_recovery.py -q -o 'addopts='
# 29 passed, 1 warning
python -m py_compile agent/conversation_compression.py agent/conversation_loop.py agent/turn_retry_state.py run_agent.py tests/agent/test_turn_retry_state.py tests/run_agent/test_image_shrink_recovery.py
# passedReviewed the retry path. The new 413 branch runs before text compression and targets aggregate image bytes ( |
|
Thanks @jakepresent for the thorough local validation — much appreciated that you ran the full suite ( Since you didn't find any blockers, would you be willing to convert your comment into a formal approving review? The merge is currently gated by branch protection waiting on an approval. Glad to address anything else if something turns up — thanks again for taking the time! |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the aggregate-image 413 separately from the per-image ceiling. The premise remains valid on current main: agent/conversation_loop.py:3374-3430 still attempts text compression and then only strips tool-message images, while agent/conversation_compression.py:1162-1217 ignores individually sub-4 MiB inline images.
Problems
- The new recovery mutates only
api_messages. Current main builds that request copy atagent/conversation_loop.py:792-835and prompt caching can deep-copy it atagent/prompt_caching.py:894-899; canonicalmessagesretain the original data URL. A later turn can rebuild the oversized payload. The linked #62005 identifies this same persistence boundary. - The added tests cover the helper but not the retry branch.
tests/run_agent/test_image_shrink_recovery.py:12-15explicitly excludes retry-loop wiring; use the request-level 413 pattern intests/run_agent/test_413_compression.py:230-298to prove the repaired request is retried before text compaction.
Suggested changes
- Propagate successful replacements to canonical history and persist it before retrying.
- Add an end-to-end 413 regression covering several individually sub-4 MiB inline images, the shrunk retry payload, and subsequent-turn persistence.
Automated hermes-sweeper review.
| _shrink_target = 256 * 1024 | ||
| if _shrink_target is not None: | ||
| if agent._try_shrink_image_parts_in_messages( | ||
| api_messages, |
There was a problem hiding this comment.
This mutates only the provider-facing api_messages copy. Please also repair and persist the canonical messages image parts before retrying; otherwise the next turn rebuilds the original oversized payload. See current agent/conversation_loop.py:792-835 and the persistence gap tracked by linked #62005.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address image-driven HTTP 413 recovery before text compression, but at different scopes: #42509 adds a one-shot shrink for an individually oversized inline image, while #52444 adds progressive per-image byte targets for aggregate payloads containing several individually sub-limit images.
Related pull requests
- #42509 [closed]
related— (+106/-0) — closed reference implementation: the diff places a guarded image-shrink attempt before 413 text compression and preserves no-image fallthrough, but its tests cover classification, state, and helper behavior rather than exercising the retry loop. Despite the maintainer-bot keep_open/high-salvageability verdict, the PR was subsequently abandoned at the project owner's direction; it should remain closed while serving as the narrower implementation reference. - #52444
related— (+186/-1) — keep open with a salvage path: the diff extends shrinking with 512 KB and 256 KB targets, so aggregate image payloads can be reduced even when each image is below the default 4 MiB threshold. This agrees with the contributor keep_open/medium-salvageability review, whose blockers remain: replacements affect only the request copy rather than canonical messages, and the added tests do not drive the actual 413 retry branch before text compression.
Duplicates
#42509 and #52444 overlap in routing image-driven 413s through shrinking before text compression, but they are not exact duplicates: #42509 handles the narrower single-image case, whereas #52444 adds progressive byte targets for aggregate sub-limit images.
Suggested consolidation
Keep #52444 open with a salvage path: retain its aggregate-payload target_bytes mechanism and progressive 512 KB/256 KB passes, propagate successful replacements across the documented request-copy boundary into canonical message history, and add a request-level regression proving that a first 413 produces a smaller retried request before any text compaction, plus a no-image compression control. Keep #42509 closed rather than reopening it; use its simpler guarded fallthrough as a reference while consolidating the non-duplicative aggregate-image behavior in #52444.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 21 kB of PR diffs, 8 kB of issue/PR text, 5 kB of discussion (8 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
What
Recover from image-payload 413 (Request Entity Too Large) errors by
shrinking embedded images, instead of bricking the turn with "cannot
compress further."
The bug
A 413 is a request-body-size error measured in bytes, not a
token-count overflow. When the body is bloated by several base64 images
embedded via native vision (each individually under the 5 MB single-image
ceiling, but collectively megabytes), the
payload_too_largehandler onlyran text compression — which can't touch image bytes. It reported
"cannot compress further" and aborted the turn while the real payload (the
images) was never reduced.
Reproduction (GitHub Copilot, claude-opus-4.8)
A session that is only ~20–40 KB of text but carries 4–5 ~500 KB
vision_analyzescreenshots (~2 MB of base64) 413s immediately, then dies:Meanwhile a pure-text session at 100 K tokens sends fine — proving the
binding constraint is body bytes, not context length. (Confirmed the
model's context window resolves correctly: Copilot reports
claude-opus-4.8atmax_prompt_tokens=936000via/models.Context-window resolution was never the problem.)
Why the existing image-shrink recovery didn't catch it
The reactive shrink that handles Anthropic's per-image 5 MB ceiling
(
FailoverReason.image_too_large) never fired here, because:payload_too_large, notimage_too_large, so theshrink branch is skipped entirely.
try_shrink_image_parts_in_messagesuses a 4 MBper-image budget — it no-ops on ~500 KB images. The problem is their
sum, not any single image.
The fix
try_shrink_image_parts_in_messagesgains atarget_bytesoverride sothe aggregate-payload path can demand a much smaller per-image budget than
the single-image 4 MB default.
payload_too_largehandler now shrinks embedded images beforetext compression, in two progressive passes (≤512 KB, then ≤256 KB per
image) driven by two one-shot guards on
TurnRetryState. It mirrors theproven
image_too_largesibling: in-place mutateapi_messages+continue(re-runs_build_api_kwargswith the shrunk payload). Onlyafter both image passes are spent — i.e. the body is genuinely
text-dominated — does it fall through to the existing text-compression
path. No new cache-breaking, no mid-conversation toolset/system-prompt
changes.
Verification
End-to-end with 5 real ~300–400 KB screenshots:
New
TestAggregatePayloadShrinkcovers the default-budget no-op, thesmall-
target_bytesshrink, and thetarget_bytes<=0fallback.TurnRetryStatefield-set + all-False contract tests updated for the twonew bool guards.
Files
agent/conversation_compression.pytarget_bytesparam on the shrink helperagent/conversation_loop.pyagent/turn_retry_state.pyrun_agent.pytarget_bytestests/run_agent/test_image_shrink_recovery.pyTestAggregatePayloadShrinktests/agent/test_turn_retry_state.py