fix: surface when LlmCompaction briefings keep losing the race - #155
Merged
Conversation
Investigated with tracing on a live 25-turn run rather than by reading. Three of my own theories were wrong before the logs gave the answer. Not "the trigger window is too short": compact() is called every turn, so arming gets a chance early. Not "the history is too short to summarize": no HistoryTooShort line appeared. The actual sequence was summarizing messages[1..5) in background summary not ready, deterministic fallback <- compaction #1, 25 -> 3 msgs summary ready (3127 chars) <- arrived, too late history changed under summary, discarding <- of course: #1 rewrote it So the briefing lost the race, and the compaction that fired meanwhile destroyed the history it had been computed over, guaranteeing the fingerprint check would reject it even on arrival. The cause was the summarizer, and it is a documentation problem: LlmCompaction::from_config(loop_config) is the obvious call and the worst one. Same run, changing only that: loop's model (Sonnet 5) -> Deterministic, 3 msgs / 1.7K retained fast model (Haiku 4.5) -> Summarized, 22 msgs / 16.7K retained The module docs now lead with that table, and after two consecutive fallbacks the strategy warns once naming the likely cause, so a session paying for briefings it never uses is told. A successful splice resets the streak, so a mostly-working session is not misdirected. Deliberately NOT changed: compact_headroom_turns defaults to Some(30), which for tool-heavy work exceeds the budget and pins the target ratio to its 0.15 floor — the aggression that destroys in-flight summaries. That constant affects compaction for every user and wants measurement across growth rates, not a guess. #150 stays open for it. The streak test states its own scope: it pins the policy, not the wiring, because deleting the reset from compact's success path would not fail it. Said plainly rather than left to read as stronger than it is. 621 passed, clippy clean under -Dwarnings. Refs #150 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four review agents; every critical finding was real, and the worst two were in the mechanism this PR added. The streak reset ran on the "briefing produced, then discarded as too large" path, which exits through the same success return. It did not just under-count — it *cleared* the streak, so alternating that path with a plain miss sawtoothed 1,0,1,0 and the counter could never reach the threshold. Six consecutive deterministic compactions, three of them paid for, warned nobody. The reset now belongs to the briefing surviving into the result, and the discard counts. The warning fired when nothing had been spawned. In the inert configuration choose_cut never finds a split and no request is issued, so the caller was told they were "paying for briefings" having spent nothing — in the same session as warn_inert_once giving the opposite trigger_ratio advice. Gated on a request having been issued. Two-in-a-row was far too aggressive. A session measured at seven splices in nine compactions still contained a run of two, and the latch could never retract. Threshold is five, and a landed splice clears the latch. A briefing rejected on fingerprint mismatch reported summary: None, so cost accounting under-counted the most wasteful failure. types.rs states the contract and the sibling branch already honoured it. My causal claim was wrong, and contradicted this file's own Guarantees section: I wrote that the fallback rewrites the history the briefing was computed over, lifting "discarded on arrival" verbatim from the description of a bug that was already fixed. arm() fingerprints the history compact is about to *return* precisely so that cannot happen. Measured over 60 fed-back rounds with a slow summarizer: 13 "not ready" against 6 fingerprint rejections. Corrected in four places. Also corrected: "issues a request each time" overstated cost ~3x (arm starts none while one is in flight — 19 fallbacks, 7 billed requests); the measured table now discloses n=1 and the repro command; "25-turn" was 15; "do not reuse the loop's config" contradicted the file's own DeepSeek finding; RUST_LOG did nothing (the code reads YO_LOG, and env-filter is not compiled in); and the third orphaned doc block of this session left warn_inert_once undocumented. Both new assertions about the briefing's presence used context::message_text, which returns empty for anything but a ToolResult — so one passed vacuously and the other failed on a splice that had happened. They inspect content directly now. Five tests, each mutation-verified, including the two that previously escaped. 624 passed, clippy clean under -Dwarnings. Refs #150 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yuanhao
added a commit
that referenced
this pull request
Aug 23, 2026
…#156) RetryConfig::delay_for_attempt documents a 1-indexed attempt and computes attempt - 1. llm_compaction.rs passed the raw 0..=max_retries loop variable, so the first retry underflowed usize and panicked in debug. agent_loop.rs increments before calling and was correct; this was confined to compaction. The panic landed on a detached task, so nothing surfaced: the summarization vanished, no briefing arrived, and compaction fell back deterministically — one of the behaviours #150 is about. delay_for_attempt now saturates, so missing the contract costs the backoff, not the task. provider_failure_falls_back_deterministically was passing *because* of the panic: its FailingProvider returns a retryable error, and the instant death released the in-flight slot inside the test's 100ms window. It now configures no retries — it is about the drop guard, not backoff timing. Also closes a coverage hole from #155: deleting the only warn_losing_race_once call site disconnected the feature entirely while every test stayed green, because they all called the function directly. 627 passed, clippy clean under -Dwarnings. Refs #150
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
1 participant
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.
Partial fix for #150. Diagnosed with tracing on a live 25-turn run, after three of my own theories were wrong.
What it actually was
Not "the trigger window is too short" —
compact()is called every turn, so arming gets a chance early. Not "the history is too short to summarize" — noHistoryTooShortline ever appeared. The logs gave the real sequence:The briefing lost the race, and the compaction that fired meanwhile destroyed the history it had been computed over — so the fingerprint check would have rejected it even on arrival.
The cause was the summarizer, and it is a docs problem
LlmCompaction::from_config(loop_config)is the obvious call and the worst one. Same run, changing only that:DeterministicSummarizedAn order of magnitude in retained context, from one config line. The module docs now lead with that table.
The warning
After two consecutive fallbacks the strategy warns once, naming the likely cause. Two, not one — the first compaction of a session legitimately beats any summary, and warning on it would train callers to ignore the message. A successful splice resets the streak, so a session that mostly works is not misdirected into tuning something that is fine.
This matters because the silent case costs money: a run that always falls back still issues every summarization request, paying input tokens for the span and output tokens for a briefing it discards.
What I deliberately did not change
compact_headroom_turnsdefaults toSome(30). For any agent whose turns carry real tool output,30 × growthexceeds the budget outright and pinseffective_target_ratioto itsMIN_HEADROOM_RATIOfloor of 0.15 — which is the aggression that destroys in-flight summaries in the first place.That constant changes compaction for every user. Picking a new value wants measurement across growth rates via
examples/llm_compaction_live.rs, not a guess at the end of a release. #150 stays open for it.A note on the test
a_successful_splice_resets_the_streakstates its own scope in its doc: it pins the streak policy, not the wiring. Deleting the reset fromcompact's success path would not fail it — exercising that needs a summary staged inPhase::Readywith a matching fingerprint. Said plainly, because a test that reads stronger than it is, is worse than one that admits its limit.Also adds a
YO_LOGtracing subscriber toexamples/long_horizon.rs— without it, compaction decisions are invisible and this diagnosis was guesswork.cargo test --all-features: 621 passed, 0 failed. Clippy clean under-Dwarnings.🤖 Generated with Claude Code