fix(compaction): bound what a summary request is asked to read - #2072
Open
rogercloud wants to merge 5 commits into
Open
fix(compaction): bound what a summary request is asked to read#2072rogercloud wants to merge 5 commits into
rogercloud wants to merge 5 commits into
Conversation
One oversized message could make compaction impossible rather than merely expensive, and leave the context with no way to shrink at all. A summary is written by reading the history. When a single tool result is large enough to exhaust the window on its own, the request cannot be sent -- and the backstop cannot rescue it either: a context that short has a tail window wide enough to keep every message, so nothing is dropped, ``removed_count`` is 0, and the next turn arrives in the same state. Stuck, not slow. The threshold already bounds ordinary growth. At the default ratio of 0.75, a 32k window compacts at 24000 tokens and asks for at most 6000 back, so input plus output sits inside the window with room to spare; history reaches that size gradually and is compacted on the way. What is missing is a bound on the outlier that arrives in one step. So this caps a single message, not the total: ``threshold // 4``, with a floor and deliberately no ceiling. It shares the summary output budget's denominator because no one message should be able to claim more of the request than the whole summary may produce. The floor says only that a message that small was never what exhausted a window, and keeps a tiny threshold from capping everything to nothing. No ceiling, because the reason ``COMPACT_SUMMARY_MAX_TOKENS`` exists -- providers cap output far below input -- has no counterpart here: nothing limits one input message except the window it must fit in, so scaling with the window cannot outgrow a provider limit. Oversized content is replaced whole, never sliced. A byte-slice can land inside a structured value, and the model completes the severed token by guessing, which reads as data and is silently wrong; that failure is the reason xorbitsai#1598 exists and it would have been reintroduced here. The stand-in names the tool, gives the size, and says the work happened and where to re-read it. The notice is derived only from the message -- never the clock, never a request id -- so the budget ladder's retries send a byte-identical request instead of defeating prefix caching. Verified: reverting that determinism turns its test red. ``omitted_messages`` and the cap ride on the request metadata into the compact trace event, so a thin summary has a visible cause. Counts and sizes only, never the omitted content. The read path that replays a stored summary looks at three keys and none of them is this one, so none of it reaches a replayed prefix. Each guarantee was mutation-tested: removing the cap, slicing instead of replacing, putting a clock value in the notice, and dropping the scaling each turn exactly their own tests red.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
qinxuye
reviewed
Sep 3, 2026
qinxuye
reviewed
Sep 5, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 5, 2026
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.
Follows #2025. That one stopped compaction being redone every turn; this one
stops a single oversized message from making compaction impossible in the
first place.
The failure
A summary is written by reading the history. When one tool result is large
enough to exhaust the window on its own, the request cannot be sent — and the
backstop cannot rescue it either: a context that short has a tail window wide
enough to keep every message, so nothing is dropped,
removed_countis 0, andthe next turn arrives in exactly the same state.
Stuck, not slow. The fallback that exists to guarantee forward progress
happens not to make any here.
Why a per-message cap and not a total input budget
I originally framed this as "the summary request's input is unbounded". That
framing is wrong, and worth correcting because it points at the wrong fix.
The threshold already bounds ordinary growth. At the default ratio of 0.75, a
32k window compacts at 24000 tokens and asks for at most 6000 back — input
plus output sits inside the window with room to spare, and history reaches
that size gradually, being compacted on the way:
What is missing is a bound on the outlier that arrives in one step.
The cap
threshold // 4, with a floor and deliberately no ceiling.message should be able to claim more of the request than the whole summary
is allowed to produce.
COMPACT_TRANSCRIPT_MESSAGE_MIN_TOKENS) says only that a messagethat small was never what exhausted a window. It stops a tiny threshold from
capping everything to nothing, and stops binding once the threshold clears
8192 — i.e. for every realistic model.
COMPACT_SUMMARY_MAX_TOKENSexists —providers cap output far below input — has no counterpart on this side.
Nothing limits one input message except the window it has to fit in, so
scaling with the window cannot outgrow a provider limit.
Replaced whole, never sliced
A byte-slice can land inside a structured value, and the model completes the
severed token by guessing — which reads as data and is silently wrong. That is
the failure #1598 exists to fix, and slicing here would have reintroduced it
one layer over. The stand-in names the tool, gives the size, and says the work
happened and where to re-read it:
It is derived only from the message — never the clock, never a request id — so
the budget ladder's retries send a byte-identical request instead of defeating
prefix caching. Reverting that determinism turns its test red.
Observability
omitted_messagesand the cap ride on the request metadata into the compacttrace event, so a thin summary has a visible cause. Counts and sizes only,
never the omitted content — that is the whole reason it was left out.
The read path added in #2025 looks at three keys (
summary,watermark_message_id,summary_context_refs) and none of them is this one,so none of this reaches a replayed prefix.
Verification
tests/core+tests/web(excludingRAG_toolsand the Node-dependentpptx suite): the failure set is identical to the merge base — no regressions.
pre-commit run --all-files: all Python hooks pass. The two npm hooks exit127 (
eslint/tscnot found) because frontend dependencies are notinstalled locally; this change touches no frontend files.
Mutation-tested, one test per mutation:
omits_a_message_too_large_to_readoversized_content_is_replaced_whole_not_slicedoversized_content_is_replaced_whole_not_slicedWhat this unblocks
Deleting the
truncatebackstop needs the summary path to stop failing forreasons it can prevent. This removes the one that is entirely within our
control. It does not remove the backstop — a summary can still come back
unusable — but it narrows what is left to genuine provider failure.