From 7f9485707d0183af0c9f04e1153e1ec9bd98aa68 Mon Sep 17 00:00:00 2001 From: Igor Ganapolsky Date: Fri, 10 Jul 2026 11:49:14 -0400 Subject: [PATCH] fix(compaction): judge the anti-thrash verdict on real usage, not in should_compress Third correction, and the load-bearing one. The previous commit put the "did compaction clear the threshold?" verdict inside should_compress(). But conversation_loop calls should_compress() TWICE per turn with two different measures (turn_context.py / conversation_loop.py:1033 and :4789): * pre-API : request_pressure_tokens -- a rough estimate that can dip BELOW the threshold * post-API: real prompt tokens -- which stay above it So the rough reading reset the strike every turn and the loop never stopped. Reproduced: 8 compactions in 8 turns under the real two-call pattern, even with the previous fix applied. (My earlier repro only called should_compress() once per turn, which is why it looked contained.) Move the verdict to update_from_response(), the one place that sees the provider's real prompt_tokens for the just-compacted conversation, guarded by the existing awaiting_real_usage_after_compression flag so it fires exactly once per compaction. Real-vs-real: it cannot be fooled by a rough sub-threshold reading, and (from the previous commit's lesson) never subtracts an estimate from a real count. should_compress() goes back to a plain threshold test plus the pre-existing cooldown and anti-thrash guards. New test test_rough_preflight_reading_does_not_reopen_the_loop drives the real two-call-per-turn pattern and fails on the prior should_compress()-based commit. Co-Authored-By: Claude Opus 4.8 (1M context) --- agent/context_compressor.py | 69 ++++++++-------- tests/agent/test_compaction_anti_thrash.py | 96 +++++++++++++++++----- 2 files changed, 112 insertions(+), 53 deletions(-) diff --git a/agent/context_compressor.py b/agent/context_compressor.py index 32e693bb923..27122ae0b20 100644 --- a/agent/context_compressor.py +++ b/agent/context_compressor.py @@ -1185,6 +1185,42 @@ class ContextCompressor(ContextEngine): self.last_rough_tokens_when_real_prompt_fit = self.last_compression_rough_tokens else: self.last_rough_tokens_when_real_prompt_fit = 0 + + # Anti-thrashing verdict, judged HERE because this is the only place + # that sees the provider's real prompt count for the just-compacted + # conversation. Effectiveness is "did the prompt get under the + # threshold?", not "did the message list shrink?": compaction can + # only shrink messages, while the system prompt and tool schemas are + # an incompressible floor (with 50+ tools, 20-30K tokens — see + # #14695). When that floor alone meets the threshold, every pass + # shrinks messages by a healthy margin yet leaves the prompt over the + # line, so the next turn compacts again, forever. + # + # It must NOT live in should_compress(): that runs twice per turn + # with two different measures (a rough preflight estimate and the + # real post-response count, #36718), and the rough one can dip below + # the threshold and reset the strike every turn, re-opening the loop. + # Keying on real usage compares like with like and fires exactly once + # per compaction. + if self._verify_compaction_cleared_threshold: + if self.last_prompt_tokens >= self.threshold_tokens: + self._ineffective_compression_count += 1 + if not self.quiet_mode: + logger.warning( + "Compaction did not clear the threshold: %d real " + "tokens still >= %d. The incompressible prompt " + "(system prompt + tool schemas) may already exceed " + "it, in which case shrinking messages cannot help. " + "ineffective_compression_count=%d", + self.last_prompt_tokens, self.threshold_tokens, + self._ineffective_compression_count, + ) + else: + self._ineffective_compression_count = 0 + # Consume the pending-verification flag once real usage arrives, whether + # or not prompt_tokens was reported, so a usage-less response can't leave + # it armed for a later, unrelated reading. + self._verify_compaction_cleared_threshold = False self.awaiting_real_usage_after_compression = False def should_defer_preflight_to_real_usage(self, rough_tokens: int) -> bool: @@ -1237,41 +1273,8 @@ class ContextCompressor(ContextEngine): where each pass removes only 1-2 messages. """ tokens = prompt_tokens if prompt_tokens is not None else self.last_prompt_tokens - - # Verify the previous compaction against the SAME measure this method - # gates on. Effectiveness is "did the prompt get under the threshold?", - # not "did the message list shrink?": compaction can only shrink - # messages, while the system prompt and tool schemas are an - # incompressible floor (with 50+ tools, 20-30K tokens — see #14695). - # When that floor alone meets the threshold, every pass can shrink - # messages by a healthy margin and still leave the prompt over the line, - # so the next turn compacts again, forever. - # - # Both sides of this comparison are the caller's own token count, so it - # is immune to the skew between a rough estimate and the provider's real - # tokenizer — an analytic "floor = real_prompt - rough_estimate" is not, - # and would misread that skew as an incompressible floor. if tokens < self.threshold_tokens: - # The prompt fits. Whatever happened before, we are not thrashing. - self._ineffective_compression_count = 0 - self._verify_compaction_cleared_threshold = False return False - - # Past this point the prompt is over the threshold. If a compaction just - # ran, it failed to clear it — one strike. Two strikes and we stop and - # tell the user, instead of compacting on every turn forever. - if self._verify_compaction_cleared_threshold: - self._verify_compaction_cleared_threshold = False - self._ineffective_compression_count += 1 - if not self.quiet_mode: - logger.warning( - "Compaction did not clear the threshold: %d tokens still " - ">= %d. The incompressible prompt (system prompt + tool " - "schemas) may already exceed it, in which case shrinking " - "messages cannot help. ineffective_compression_count=%d", - tokens, self.threshold_tokens, - self._ineffective_compression_count, - ) # Do not trigger compression while the summary LLM is in cooldown. # On a 429/transient failure _generate_summary() sets a cooldown and # returns None; compress() then inserts a static fallback marker and diff --git a/tests/agent/test_compaction_anti_thrash.py b/tests/agent/test_compaction_anti_thrash.py index f623cf47474..a219526c68f 100644 --- a/tests/agent/test_compaction_anti_thrash.py +++ b/tests/agent/test_compaction_anti_thrash.py @@ -15,14 +15,20 @@ Two defects made it dead code: Together these made a mis-sized context window present as a hung CLI rather than a warning. Effectiveness is now scored against the goal -- did the prompt get under the -threshold? -- and the check is made in ``should_compress()`` against the caller's own -token count on both sides. +threshold? -- judged in ``update_from_response()``, the one place that sees the +provider's real prompt count for the just-compacted conversation. -That last detail matters. An analytic ``floor = current_tokens - rough_estimate(messages)`` -looks equivalent and is not: it silently absorbs the skew between the rough estimator and -the provider's real tokenizer, reports it as incompressible overhead, and disables -compaction on a perfectly healthy session. ``test_no_false_positive_under_tokenizer_skew`` -pins that. +Two subtleties this pins: + +* The verdict must NOT live in ``should_compress()``. That runs twice per turn with + two different measures (a rough preflight estimate and the real post-response count, + #36718); the rough one can dip below the threshold and reset the strike every turn, + re-opening the loop. ``test_rough_preflight_reading_does_not_reopen_the_loop``. + +* It must not be computed analytically as ``floor = current_tokens - rough_estimate``. + That subtracts an estimate from a real count, absorbs the tokenizer skew into "floor", + and disables compaction on a healthy session. + ``test_no_false_positive_under_tokenizer_skew``. """ import pytest @@ -52,6 +58,22 @@ def _messages(n: int, size: int = 1500) -> list: return msgs +def _turn(cc, msgs, real_prompt_tokens): + """One agent turn as conversation_loop drives it. + + should_compress() gates on the real prompt count; if it fires, compress() + runs and the provider then reports real usage for the shorter conversation. + The anti-thrashing verdict is judged in update_from_response() against that + real count -- not inside should_compress(), which is called twice per turn + with two different measures. Returns (msgs, did_compact). + """ + if not cc.should_compress(real_prompt_tokens): + return msgs, False + msgs = cc.compress(msgs, current_tokens=real_prompt_tokens) + cc.update_from_response({"prompt_tokens": real_prompt_tokens}) + return msgs, True + + class TestSavingsBasis: def test_savings_does_not_depend_on_current_tokens(self): """Savings is a property of the messages, not of the caller's token count. @@ -88,22 +110,48 @@ class TestFutilityGuard: cc = _compressor(threshold_tokens=24_576) msgs = _messages(14) # Full prompt is far above threshold and dominated by system + tools, - # so it never drops no matter how much the message list shrinks. + # so the real count never drops no matter how the message list shrinks. real_prompt_tokens = 33_564 fired = 0 for _ in range(6): - if cc.should_compress(real_prompt_tokens): + msgs, did = _turn(cc, msgs, real_prompt_tokens) + if did: fired += 1 - msgs = cc.compress(msgs, current_tokens=real_prompt_tokens) msgs.append({"role": "user", "content": "next " + "w" * 4000}) assert cc._ineffective_compression_count >= 2 assert not cc.should_compress(real_prompt_tokens), ( - "compaction that cannot clear the threshold must stop and warn" + "compaction that cannot clear the threshold must stop" ) assert fired <= 3, f"expected the loop to break early, compacted {fired}x" + def test_rough_preflight_reading_does_not_reopen_the_loop(self): + """should_compress() runs twice per turn with two different measures. + + The pre-API gate uses a rough estimate that can dip below the threshold; + the post-response gate uses the real count that does not. If the verdict + lived in should_compress(), the rough reading would reset the strike + every turn and the loop would never stop. Judging it in + update_from_response() (real-vs-real) closes that hole. + """ + cc = _compressor(threshold_tokens=24_576) + msgs = _messages(13) + rough, real = 20_000, 33_564 # rough dips under; real never does + + fired = 0 + for _ in range(8): + cc.should_compress(rough) # pre-API gate (rough) + msgs, did = _turn(cc, msgs, real) # post-response gate (real) + usage + if did: + fired += 1 + msgs.append({"role": "user", "content": "more " + "w" * 3000}) + + assert fired <= 2, ( + f"a sub-threshold rough reading must not re-open the loop; " + f"compacted {fired}x" + ) + def test_effective_compaction_still_resets_the_counter(self): """A compaction that gets the prompt under the threshold is not thrashing.""" cc = _compressor(threshold_tokens=750_000) @@ -131,26 +179,34 @@ class TestFutilityGuard: real_prompt = floor + int(skew * estimate_messages_tokens_rough(msgs)) assert cc.should_compress(real_prompt) msgs = cc.compress(msgs, current_tokens=real_prompt) - real_prompt = floor + int(skew * estimate_messages_tokens_rough(msgs)) + real_after = floor + int(skew * estimate_messages_tokens_rough(msgs)) + cc.update_from_response({"prompt_tokens": real_after}) # provider's real count - assert real_prompt < cc.threshold_tokens, "compaction really did clear it" - assert not cc.should_compress(real_prompt) + assert real_after < cc.threshold_tokens, "compaction really did clear it" assert cc._ineffective_compression_count == 0, ( "tokenizer skew must not be mistaken for an incompressible floor" ) - def test_counter_resets_once_the_prompt_fits_again(self): - """One failed pass must not permanently disable compaction.""" + def test_a_failed_pass_records_exactly_one_strike(self): + """A compaction that leaves the real prompt over the threshold: one strike. + + The verdict is judged once, when the provider reports real usage — not on + every should_compress() reading. + """ cc = _compressor(threshold_tokens=24_576) msgs = _messages(14) - cc.should_compress(33_564) + assert cc.should_compress(33_564) cc.compress(msgs, current_tokens=33_564) - cc.should_compress(33_564) # still over -> 1 strike + assert cc._ineffective_compression_count == 0, "no verdict before real usage" + + cc.update_from_response({"prompt_tokens": 33_564}) # still over assert cc._ineffective_compression_count == 1 - cc.should_compress(1_000) # now under -> forgiven - assert cc._ineffective_compression_count == 0 + # A later reading, rough or real, must not add phantom strikes. + cc.should_compress(33_564) + cc.should_compress(20_000) + assert cc._ineffective_compression_count == 1 class TestMinimumMessagesBranch: