From b855f86bc8e5ced904fe0be97ab8db4be87fca00 Mon Sep 17 00:00:00 2001 From: Brian Date: Tue, 18 Aug 2026 06:19:01 +0000 Subject: [PATCH] fix(agent): 413 recovery measures bytes, not token estimates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 413 is a byte-size error, but the recovery loop scored compression progress with estimate_messages_tokens_rough, which deliberately prices every image at a flat per-image token cost (so screenshots don't trigger premature compaction). When the payload is image-dominated that check can never pass: in the reporting session two vision_analyze results were 5,627,202 bytes (96.6% of the request body) but ~3K of the ~80K token estimate, so every attempt reported no_progress, the budget burned, and the session wedged permanently with 'max compression attempts (3) reached' at 13% context usage. Post-#97160, the 413 path already routes into compaction and compaction's historical-media aging genuinely frees the image bytes — but the token-scored yardstick could not see the megabytes it freed. Add serialized_messages_bytes() (exact serialized payload size, measured identically before and after each pass — a measurement, not an estimate) and score the 413 progress check with it. Tokens remain for status display only; the context-overflow branch keeps its token yardstick, because that error IS a token-budget error. Images are never evicted from live history outside compaction (cache invariant); the original strip-from-history mechanism in this PR was superseded by #97160's compaction-time aging and is dropped in salvage. Salvaged from #88960. Fixes #47339. --- agent/conversation_loop.py | 35 ++- agent/message_sanitization.py | 35 +++ .../agent/test_413_image_payload_recovery.py | 217 ++++++++++++++++++ 3 files changed, 282 insertions(+), 5 deletions(-) create mode 100644 tests/agent/test_413_image_payload_recovery.py diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index fca8666690..0a8e7cd511 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -62,6 +62,7 @@ from agent.message_sanitization import ( _looks_like_image_content_rejection, _strip_images_from_messages, _strip_non_ascii, + serialized_messages_bytes, ) # Must mirror _STALE_TOOL_CALL_MARKER_RE in hermes_state.py — kept local # to avoid importing hermes_state at module load time (its module-level @@ -5659,7 +5660,19 @@ def run_conversation( agent._buffer_status(f"⚠️ Request payload too large (413) — compression attempt {compression_attempts}/{max_compression_attempts}...") original_len = len(messages) - original_tokens = estimate_messages_tokens_rough(messages) + # A 413 is a BYTE-size error, so this branch scores + # progress in BYTES of the serialized messages payload — + # exact and free — never the token estimate. The + # estimator prices every image at a flat per-image token + # cost (see estimate_messages_tokens_rough) so screenshots + # don't trigger premature compaction; that deliberate + # byte-blindness means compaction can free megabytes of + # base64 (real case: two vision results = 96.6% of the + # request body but ~3.7% of the estimate) while the token + # delta stays under any threshold. Token-scored progress + # here burned all attempts on "no progress" and wedged + # the session permanently. (#88960 / #47339) + original_bytes = serialized_messages_bytes(messages) _overflow_input = messages # Option A (LCM issue 441): overhead-aware request size so recovery arms on the # true request (msgs + tools + system), not the tool-blind message count. @@ -5684,18 +5697,30 @@ def run_conversation( agent, messages, conversation_history ) - # Re-estimate tokens after compression. Same-message-count + # Re-measure after compression. Same-message-count # compression (tool-result pruning, in-place summarization) # can materially reduce request size without reducing the - # message array. (#39550) + # message array (#39550), and — the image-dominated case — + # compaction's historical-media aging (#97160) can free + # megabytes of base64 that the token estimate never + # counted. Bytes are the yardstick for a 413; tokens are + # kept only for status display. new_tokens = estimate_messages_tokens_rough(messages) approx_tokens = new_tokens # update for downstream logging + new_bytes = serialized_messages_bytes(messages) - if len(messages) < original_len or (new_tokens > 0 and new_tokens < original_tokens * 0.95): + made_progress = ( + len(messages) < original_len + or (new_bytes > 0 and new_bytes < original_bytes * 0.95) + ) + if made_progress: if len(messages) < original_len: agent._buffer_status(COMPRESSION_RETRY_MESSAGES_STATUS_TEMPLATE.format(before=original_len, after=len(messages))) else: - agent._buffer_status(COMPRESSION_RETRY_TOKENS_STATUS_TEMPLATE.format(before=original_tokens, after=new_tokens)) + agent._buffer_status( + f"🗜️ Compressed {original_bytes:,} → {new_bytes:,} " + f"payload bytes, retrying..." + ) time.sleep(2) # Brief pause between compression retries _retry.restart_with_compressed_messages = True break diff --git a/agent/message_sanitization.py b/agent/message_sanitization.py index 32064c0185..6a4c4cbbbd 100644 --- a/agent/message_sanitization.py +++ b/agent/message_sanitization.py @@ -400,6 +400,41 @@ def _sanitize_tools_non_ascii(tools: list) -> bool: return _sanitize_structure_non_ascii(tools) +def serialized_messages_bytes(messages: list) -> int: + """Exact serialized size, in bytes, of the ``messages`` request payload. + + Recovery path for HTTP 413 (payload too large). A 413 is a *byte*-size + error, but Hermes' context estimator deliberately prices an image at a + flat per-image token cost so that a screenshot does not trigger premature + compaction (see ``estimate_messages_tokens_rough``). That makes the + token estimate structurally unable to *score* recovery from an + image-dominated 413: compaction can free megabytes of base64 while the + estimate barely moves, so a token-scored progress check reports + "no progress" and the turn dies permanently. + + This measures the thing the provider actually rejected — serialized + bytes — exactly and for free. It is a faithful proxy for the request + body's ``messages`` field (the only part recovery can shrink) and is + measured identically before and after each compression pass, so the + before/after ratio is exact. It is NOT an estimate. + + Non-serializable values fall back to ``str()`` so a malformed message + can never crash the 413 recovery path. + """ + if not isinstance(messages, list) or not messages: + return 0 + try: + return len( + json.dumps( + messages, ensure_ascii=False, separators=(",", ":"), default=str + ).encode("utf-8") + ) + except (TypeError, ValueError): + # Extremely defensive — ``default=str`` already covers exotic + # values. Never let byte accounting take down error recovery. + return sum(len(str(m)) for m in messages) + + def _strip_images_from_messages(messages: list) -> bool: """Remove image_url content parts from all messages in-place. diff --git a/tests/agent/test_413_image_payload_recovery.py b/tests/agent/test_413_image_payload_recovery.py new file mode 100644 index 0000000000..f1f4daa98f --- /dev/null +++ b/tests/agent/test_413_image_payload_recovery.py @@ -0,0 +1,217 @@ +"""Regression tests: HTTP 413 recovery must score progress in BYTES. + +Bug (#88960 / #47339): a 413 is a *byte*-size error, but the recovery loop in +``agent/conversation_loop.py`` scored compression progress with +``estimate_messages_tokens_rough``, which deliberately prices every image at +a flat per-image token cost so screenshots don't trigger premature +compaction. When the payload is image-dominated that progress test can never +be satisfied: in the reporting session two ``vision_analyze`` results were +5,627,202 bytes — 96.6% of the request body — while contributing only ~3K of +the ~80K token estimate. Compaction (post-#97160) frees those megabytes, but +the token-scored check reported "no progress", burned all three attempts, and +wedged the session permanently at 13% context usage. + +The fix: the 413 no-progress check measures ``serialized_messages_bytes`` +(exact, free) before and after each compression pass, never the token +estimate. These tests assert that invariant directly. +""" + +import pytest + +from agent.message_sanitization import serialized_messages_bytes +from agent.model_metadata import estimate_messages_tokens_rough + + +def _data_url_image(size_bytes: int) -> dict: + """An image part whose inline data URL is ~``size_bytes`` long.""" + return { + "type": "image_url", + "image_url": {"url": "data:image/png;base64," + ("A" * size_bytes)}, + } + + +def _tool_msg_with_image(size_bytes: int, text: str = "screenshot captured") -> dict: + return { + "role": "tool", + "tool_call_id": "call_abc123", + "content": [ + {"type": "text", "text": text}, + _data_url_image(size_bytes), + ], + } + + +def _image_aged_out(msg: dict) -> dict: + """The message after compaction replaced its image with a placeholder.""" + out = dict(msg) + out["content"] = [ + p for p in msg["content"] if p.get("type") != "image_url" + ] + [{"type": "text", "text": "[image removed during compaction]"}] + return out + + +class TestSerializedMessagesBytes: + def test_counts_inline_data_url_payloads(self): + small = serialized_messages_bytes([_tool_msg_with_image(1_000)]) + huge = serialized_messages_bytes([_tool_msg_with_image(3_000_000)]) + assert huge - small == pytest.approx(3_000_000 - 1_000, abs=64) + + def test_is_exact_not_an_estimate(self): + """Same input, same answer — a measurement, not a heuristic.""" + messages = [_tool_msg_with_image(50_000), {"role": "user", "content": "hi"}] + assert serialized_messages_bytes(messages) == serialized_messages_bytes( + messages + ) + + def test_utf8_bytes_not_codepoints(self): + ascii_msgs = [{"role": "user", "content": "aaaa"}] + utf8_msgs = [{"role": "user", "content": "éééé"}] # 2 bytes each in UTF-8 + assert serialized_messages_bytes(utf8_msgs) > serialized_messages_bytes( + ascii_msgs + ) + + def test_degenerate_input(self): + assert serialized_messages_bytes([]) == 0 + assert serialized_messages_bytes("not-a-list") == 0 # type: ignore[arg-type] + + def test_never_raises_on_non_serializable_content(self): + class Weird: + pass + + messages = [{"role": "tool", "content": Weird()}] + assert serialized_messages_bytes(messages) > 0 + + +class TestTokenEstimateIsBlindToImageBytes: + """The root cause, asserted directly. + + This is why a token-scored progress check can never clear an + image-dominated 413: the estimate barely moves regardless of how many + megabytes are on the wire. + """ + + def test_estimate_barely_moves_as_image_bytes_explode(self): + small = [_tool_msg_with_image(1_000)] + huge = [_tool_msg_with_image(3_000_000)] + + small_tokens = estimate_messages_tokens_rough(small) + huge_tokens = estimate_messages_tokens_rough(huge) + + # ~3000x more bytes on the wire... + assert len(huge[0]["content"][1]["image_url"]["url"]) > 2_000_000 + + # ...but the token estimate is essentially unchanged, so a + # "did compression make progress?" check scored in tokens + # (new < original * 0.95) can never be satisfied by freeing images. + assert huge_tokens < small_tokens * 2 + + +class TestByteScoredProgressCheck: + """The invariant the fix installs: 413 progress is judged in bytes. + + Mirrors the exact decision expression in the 413 handler: + ``len(messages) < original_len or new_bytes < original_bytes * 0.95``. + """ + + def _decision(self, before: list, after: list, *, metric: str) -> bool: + if len(after) < len(before): + return True + if metric == "tokens": + o = estimate_messages_tokens_rough(before) + n = estimate_messages_tokens_rough(after) + else: + o = serialized_messages_bytes(before) + n = serialized_messages_bytes(after) + return n > 0 and n < o * 0.95 + + def _image_dominated_session(self): + """The real-world shape that wedged a session: ~190 substantive text + turns (~77K token estimate), two multi-MB vision results = 96%+ of + the serialized body but a tiny slice of the token estimate.""" + messages = [ + { + "role": "user" if i % 2 == 0 else "assistant", + "content": f"turn {i} " + ("x" * 900), + } + for i in range(190) + ] + messages.insert(50, _tool_msg_with_image(2_756_000)) + messages.insert(80, _tool_msg_with_image(2_871_000)) + return messages + + def test_token_scoring_wedges_on_image_dominated_payload(self): + """BEFORE-behavior pin: compaction frees megabytes, token check + still says no-progress -> attempts burn -> session wedges.""" + before = self._image_dominated_session() + # Compaction ages out the older tool image (#97160) — same message + # count, ~2.7MB freed. + after = [ + _image_aged_out(m) + if isinstance(m.get("content"), list) and m is before[50] + else m + for m in before + ] + freed = serialized_messages_bytes(before) - serialized_messages_bytes(after) + assert freed > 2_000_000, "compaction really freed megabytes" + assert self._decision(before, after, metric="tokens") is False, ( + "token yardstick is blind to the freed bytes — this is the bug" + ) + + def test_byte_scoring_sees_the_same_reduction(self): + before = self._image_dominated_session() + after = [ + _image_aged_out(m) + if isinstance(m.get("content"), list) and m is before[50] + else m + for m in before + ] + assert self._decision(before, after, metric="bytes") is True, ( + "byte yardstick must recognize a multi-MB reduction as progress" + ) + + def test_text_only_compression_still_scores_progress_in_bytes(self): + """Non-image 413s keep working: summarizing text shrinks bytes too.""" + before = [ + {"role": "user", "content": "x" * 10_000} for _ in range(50) + ] + after = [ + {"role": "user", "content": "x" * 10_000} for _ in range(10) + ] + # message-count branch fires first, but the byte branch alone would + # also pass: + assert serialized_messages_bytes(after) < ( + serialized_messages_bytes(before) * 0.95 + ) + assert self._decision(before, after, metric="bytes") is True + + def test_true_no_progress_is_still_terminal(self): + """When nothing actually shrank, byte scoring must NOT fake progress.""" + before = self._image_dominated_session() + after = list(before) # identical payload + assert self._decision(before, after, metric="bytes") is False + + +class TestConversationLoopWiring: + """The handler really uses the byte metric (source-level contract).""" + + def test_413_branch_scores_bytes_not_tokens(self): + import inspect + + import agent.conversation_loop as loop + + src = inspect.getsource(loop) + # The byte measurement is taken before and after the 413 compression + # pass and drives the progress decision. + assert "original_bytes = serialized_messages_bytes(messages)" in src + assert "new_bytes = serialized_messages_bytes(messages)" in src + assert "new_bytes < original_bytes * 0.95" in src + # The old token-scored expression is gone from the 413 branch's + # decision. Isolate the 413 handler region: from its status line to + # its terminal error. (Token scoring survives in the + # context-overflow branches, which ARE token-budget errors.) + start = src.index("Request payload too large (413) — compression attempt") + end = src.index("Payload too large and cannot compress further") + branch = src[start:end] + assert "new_tokens < original_tokens * 0.95" not in branch + assert "original_bytes = serialized_messages_bytes" in branch + assert "new_bytes = serialized_messages_bytes" in branch