fix(agent): recover from image-dominated 413 payloads - #88960
Open
brianbaldock wants to merge 1 commit into
Open
fix(agent): recover from image-dominated 413 payloads#88960brianbaldock wants to merge 1 commit into
brianbaldock wants to merge 1 commit into
Conversation
A 413 is a byte-size error, but the recovery path scores progress using estimate_messages_tokens_rough, which deliberately prices each image at a flat 1500 tokens so screenshots don't trigger premature compaction. When the payload is image-dominated that test can never be satisfied: in an affected session two vision_analyze results were 5,627,202 bytes (96.6% of the request body) but only ~3K of the ~80K token estimate, so compression reported no_progress, burned all three attempts, and the turn died with "max compression attempts (3) reached" at 13% context usage. The session then stayed dead — the existing image-strip fallback only mutates api_messages, so the megabytes were re-sent from stored history every turn and /compress and /retry failed identically. Add strip_oversized_image_parts(), which measures actual inline data-URL bytes, and call it in the 413 no-progress branch before declaring the turn dead. It mutates messages (persisted history) so the reduction survives into later turns, preserves tool_call_id linkage by replacing a fully stripped tool message with a placeholder rather than deleting it, protects the most recent messages so a just-attached image isn't pulled out from under the current turn, and leaves all text intact. Behavior is unchanged for non-image-dominated 413s: when nothing exceeds the byte budget the helper reports no change and the existing fallback and terminal error fire as before.
Contributor
Excellent root-cause analysis: the estimator's flat per-image pricing is exactly why text compression structurally cannot clear an image-dominated 413, and measuring actual payload bytes is the correct recovery driver. Persisting the strip into stored history (so the fix survives future turns), protecting the most recent N messages, and preserving tool_call_id linkage via placeholder are all thoughtful. One correctness gap:
No blocking issues found beyond item 1's alternation risk. |
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.
What does this PR do?
A session with plenty of context left dies permanently on
413:Status bar at the time:
122k/936k │ 13%. Once hit, the session is unrecoverable —/compressand/retryboth fail the same way, because the oversized content is re-sent from stored history on every subsequent turn.Root cause. A
413is a byte-size error, but the recovery path decides whether it made progress using the token estimate, which is deliberately blind to image bytes.agent/model_metadata.py:That flat cost is correct and intentional for context budgeting — without it a 1MB screenshot would estimate at ~250K tokens and trigger premature compaction. But
agent/conversation_loop.pyreuses that same estimate to score 413 recovery:When the payload is image-dominated, that test can never be satisfied. Real numbers from an affected session:
vision_analyzeresultsThe images are 96.6% of the request body and ~3.7% of the token estimate. Compression can summarize away every text turn and still not move the estimate by 5%, so it reports
no_progress, burns all three attempts, and gives up. Telemetry from the failing session:{"failure_class":"no_progress","commit_status":"aborted", "current_estimated_tokens":80453,"effective_threshold":234000, "main_context_limit":936000}Note
80453 < 234000— compression doesn't even believe it should be running.There is an existing image-strip fallback in the
elsebranch, but it doesn't resolve this for two reasons:api_messages(the transient per-call copy), so the megabytes return from stored history on the very next turnWhy this approach. The fix measures the thing the provider is actually rejecting (bytes) rather than a proxy that is designed to ignore it, and it mutates
messagesso the reduction survives into the next turn. It is additive and scoped to the no-progress branch, so every non-image-dominated 413 keeps its current behavior exactly.Related Issue
This PR was opened before I searched the tracker properly — that was my mistake, and the search turns up substantial prior art. Recording it honestly rather than quietly claiming the ground:
browser_visionscreenshots. Same bug class.conversation_loop.py, proposes reordering it so the image strip runs before text compression rather than after the budget is spent._strip_historical_mediamisses tool-result images and first-message images, so base64 accumulates inmessagesand re-ships every turn.Fixes #47339
Overlapping open PRs (maintainers: this needs a consolidation decision, not four parallel merges):
api_messagesonly, so the full-size base64 remains in stored history and the next turn rebuilds an oversized body — recovering the turn but paying a 413 round-trip plus a re-encode pass on every subsequent turn, whereas this PR mutatesmessagesso the reduction persists; (b) fix(413): recover from image-payload 413s instead of bricking the session #52444 leaves thenew_tokens < original_tokens * 0.95progress check unfixed and instead tries never to reach it, so if the shrink no-ops (both passes spent, Pillow unavailable, or images already small) it still falls through to the same false-negative that bricks the session. These are complementary, not competing — shrink first to preserve fidelity, strip as the correctness floor when shrinking can't get under the limit. Where a screenshot is still decision-relevant, fix(413): recover from image-payload 413s instead of bricking the session #52444's behavior is clearly better than mine.These are not all mutually exclusive, and none of them is a strict duplicate of another. #89965 is proactive (don't send it), #89776 is ordering (try the cheap fix first), #52444 is fidelity-preserving reduction (shrink the images), and this PR is the correctness floor (make the progress check measure bytes so recovery can't false-negative even when the others no-op). But all four touch the
is_payload_too_largebranch inconversation_loop.pyand will conflict textually, so this wants a consolidation decision rather than four independent merges. The coherent stack, in my view, is #89965 → #52444 → this PR (avoid the oversized body; if it happens, shrink; if shrinking can't get under the limit, strip and make the progress check honest). I'm happy to rebase this into a narrower change on top of whichever lands first, or to close it if a maintainer would rather foldstrip_oversized_image_parts()into one of the others — the byte-measurement tests are useful under any ordering. Say the word and I'll do the work.Type of Change
Changes Made
agent/message_sanitization.py— newstrip_oversized_image_parts(). Measures actual payload bytes of inlinedata:URLs and removes only image parts above the byte budget.tool-role message stripped to nothing becomes a plaintext placeholder rather than being deleted, keeping the pairedtool_call_idon the prior assistant message matched. Deleting it would produce a400. Same contract as the neighbouring_strip_images_from_messages.protect_last_n=4— an image the user just attached isn't removed out from under the current turn; recovery reaches for older history first.agent/conversation_loop.py— call it in the 413 no-progress branch before declaring the turn dead, mutatingmessages(not justapi_messages) so the reduction persists across turns.tests/agent/test_413_image_payload_recovery.py— 11 tests, new file.Behavior is unchanged for every non-image-dominated 413: if nothing exceeds the byte budget, the function reports no change and the existing fallback and terminal error fire exactly as before.
How to Test
Reproduction:
copilot).vision_analyzeon two full-resolution screenshots (~2MB PNGs, ~2.7MB each as base64).Request payload too large: max compression attempts (3) reachedwhile the status bar still reads ~13% context, and/compressand/retryboth re-fail because history re-ships the base64.messages, the retry succeeds, and the session continues with its text intact.Automated:
The tests assert invariants rather than snapshots:
TestTokenEstimateIsBlindToImageBytespins the root cause directly: a ~3000x increase in image bytes leaves the token estimate essentially unchanged, which is why text compression can't clear this class of 413.tool_call_id.protect_last_nshields the current turn.Verified these tests actually fail without the fix — sabotage run with the byte-measurement neutered gave 5 failed / 6 passed — rather than trusting a green run on code I just wrote.
Checklist
Code
fix(agent):)pytest tests/ -qand all tests pass — partial, see note belowNote on the full suite. I ran the 11 new tests (pass) and a clean upstream baseline of
tests/run_agent/(1694/1694 in 224s). I have not got a clean full-repopytest tests/ -qon the branch: my first attempt was run while switching branches underneath the process, which produced 254 uninterpretable failures for a tree that didn't even contain the fix. I discarded that result rather than report it. Flagging the gap honestly instead of checking the box. The branch is also currently based ~35 commits behindmainand I'll rebase on request.Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/A, no new config keys (threshold andprotect_last_nare internal constants; happy to promote them toconfig.yamlundercompression.if maintainers prefer)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A, no architecture changecopilot.Screenshots / Logs
Failing session, before the fix:
Measured payload composition of that same session, from
~/.hermes/state.db: