fix(compaction): address the round-3 review of the compaction stack by baptmont · Pull Request #1316 · google/adk-go

baptmont · 2026-08-12T13:35:12Z

Seventh PR in the context-compaction stack, and the only one that is not a
feature slice: it carries the fixes for @karolpiotrowicz's review of #1231 to
#1236.

These were briefly pushed onto #1236, which made that PR look like 54 files of
end-to-end test. They are split out here so #1236 is the 3-file test it should
be, and so this round of fixes can be read as one thing against one review.

What is here

Fourteen commits, each self-contained and each with a regression test I
confirmed fails without its fix.

Two design changes, both proposed in the review and both free only while this
stack is unmerged:

961f3f9a A compaction now names the events it replaces. Coverage was an
inclusive timestamp interval while the window was chosen by position and then
filtered, so an interval covering the ends also covered the gaps the filters
left, and an event in a gap was dropped from every later prompt having been
summarized by nothing. Four separate findings were that one mismatch.
b38ebcca A Summarizer returns the summary rather than a finished event.
Returning an event let third-party code set the authorship, an app-scoped
state delta, an agent transfer and its own covered range, all of which the
framework appended verbatim.

Correctness: the progress gate disarming a long turn, the transcript budget
measured in bytes against a rune cap, a truncated generation stored as a
summary, the sliding window stalling on a branch change, the live turn's own
question being summarized out of the prompt answering it, tail retention
stopping for good on one unanswered tool call, and the token count being read
from another branch.

Serving: a compaction failure no longer fails the turn that produced it on
any of the four surfaces, and a config the server cannot serve is refused at
startup rather than 500ing every request.

Telemetry: a discarded compaction no longer reports success with a result
event that exists in no session.

Not fixed here

Span parenting with no ambient caller span (#1233 finding 2). Closing it needs
the invocation's span context to reach the runner, and the agent derives it
internally and passes it only to its own children. The test states it as a
known gap rather than manufacturing a parent, and the reachable half, the
correlation attribute naming the wrong turn under concurrency, is fixed.

Testing plan

go build, go test -race -count=1 -shuffle=on, golangci-lint run and
go mod tidy -diff all clean in both modules. Every fix has a regression test
verified to fail against the unfixed code. Two known-unrelated failures in this
tree: TestConfigureExporters (a stale-base artifact, green on main and in CI)
and the flake filed as #1313.

Long conversations grow session history without bound. Every turn re-sends the whole history, so cost and latency climb with conversation length and a long enough session eventually exceeds the model's context window. Compaction replaces ranges of older events with a model-generated summary so the substance survives but the token count does not. This adds the data model and the library. Nothing calls it yet; the runner and the contents processor are wired up in a follow-up so that change can be reviewed on its own. - session.EventCompaction records the timestamp range a summary covers plus the summary content, attached to a new event via EventActions.Compaction. Covered events are never modified or deleted, so a session stays complete and auditable. - session/compaction holds what a user touches: Config, the Summarizer extension point, and the default LLMSummarizer. - internal/compactioninternal holds the algorithms: window selection, summary substitution and function-call recovery. Keeping them unexported leaves them free to change. The summarizer renders thoughts and tool traffic as well as text, since those carry the reasoning and evidence a text-only summary would lose. Testing: unit tests throughout, using fake summarizers and models so nothing touches the network.

Four of these are data-loss or silent-failure defects in the core model, so they are fixed here rather than layered over in the follow-ups. Coverage is recorded as an inclusive timestamp range, but the window was built by filtering while walking backwards, which could skip an event that still fell inside the resulting range. Such an event was dropped from the prompt without ever being summarized. The window is now a contiguous slice between invocation boundaries, so a hole is unexpressible, and the trim is pulled back off a timestamp tie for the same reason. A test asserts the invariant directly: everything inside a summary's range was in the set summarized. A response carrying content with no parts was accepted as a summary, which erased the covered turns and replaced them with nothing. That shape is what a blocked or candidate-less generation produces, so this was reachable rather than theoretical. Empty summaries are now rejected, and a summarizer that produces nothing usable returns an error carrying the finish reason instead of reporting "nothing to compact" -- which had made a summarizer failing every call indistinguishable from an idle one. On the Agent Engine backend a summary reached no storage slot at all: it carries its content only on Actions.Compaction, and eventNeedsRawEvent did not list it. The summarizer was called and billed, the result was discarded, and the same range was re-summarized on every later trigger. The shared session suite now asserts the round trip so no backend can regress this quietly again. Also: - A function call with no ID opened no obligation, so the trim that keeps a call with its response silently did not fire. It is now an unconditional obligation, erring toward not summarizing. - A tool call that never gets a response no longer stops compaction for the rest of the session. The window steps past the blocked head and summarizes the remainder, leaving the pending call raw. - A Summarizer returning an event with no compaction record is rejected rather than appended as a conversational turn. - Tool output can no longer forge turns in the summarizer transcript. - Package docs no longer imply the sliding window bounds prompt growth. It is a constant-factor reduction; tail retention is what bounds it. Testing: every fix has a regression test, and each was confirmed to fail against the previous behaviour rather than passing vacuously. Build, race tests, lint and tidy are clean. One unrelated pre-existing failure in telemetry.TestConfigureExporters (an OpenTelemetry schema version conflict from the GCP detector bump) reproduces on the integration branch with no compaction code present.

…'s role Review of the interface surfaced three things an implementer could not learn from the documentation. SummarizeEvents documented that a nil event means "skip this compaction" but never said which of the two nil returns signals it. The distinction matters: (nil, nil) is a decline and leaves history alone, while (nil, err) is a failure that gets reported and traced. A summarizer that reports failures as declines is indistinguishable from an idle one while the prompt keeps growing. NewSummaryEvent read as a validator, because its documentation led with the chronology check. It is a constructor. Say what it builds first, and say that implementations are expected to call it rather than assemble the event themselves. Nothing explained that the returned event comes back without an ID, invocation ID or timestamp, which from outside looks like a bug rather than the framework's job. Documenting it also answers why this constructor takes no context.Context when session.NewEvent does. Comments only, no behavior change.

Wires the compaction library into the run loop, so it now does something. Two halves: - The contents processor substitutes each stored summary for the events it covers when assembling a prompt. Compaction events are exempt from the empty-content filter because they carry their summary on Actions.Compaction rather than on Content. - The runner runs sliding-window compaction once an invocation has finished and all of its events have been persisted, then appends the summary. Enabled with runner.Config.EventsCompactionConfig, off by default. The window covers every invocation since the last compaction, plus OverlapSize earlier ones so consecutive summaries overlap and context is not lost at the seam. It is trimmed so a summary can never split a function call from its response. The summary is appended but not yielded: it is bookkeeping for the next prompt, not part of the conversation. A compaction failure is returned rather than swallowed, since silently never compacting would let a session grow until some later turn fails against the context limit, far from the cause. Testing: prompt-assembly tests covering substitution and the paused long-running-tool case, plus runner tests for the trigger, config validation and error propagation.

The most serious finding is not about compaction at all. A compaction record instructs prompt assembly to drop a span of history and put content of the record's choosing in its place. Two separate paths let that record come from outside the framework: the REST create-session body mapped Actions.Compaction verbatim onto the stored event, and EventActions is handed to tool code and to callbacks, whole, and then copied onto the event that gets persisted. Either one was an erase-and-inject primitive over the conversation. It is fixed in layers, because the two halves fail differently. Prompt assembly now ignores compaction records entirely unless the run has compaction configured, which covers every application that never opted in -- the default. For the applications that did opt in, gating is not enough, so the record is also cleared wherever caller-supplied actions become an event, and the REST layer no longer maps it inbound at all. Reads still return records, so a client can see summaries it did not write. Compaction also ran only when the consumer drained the whole stream. Breaking out of the range loop on the terminal event is the ordinary streaming idiom and is what the A2A executor does, so for those callers compaction silently never happened. The hook now runs from a defer via an idempotent closure, in both the agent and node paths. On an early exit the error is logged rather than yielded, because yield must not be called after it has returned false. Two concurrent invocations on one session lost conversation. The window was selected from the session snapshot the invocation began with, so events another invocation had appended were inside the recorded range but were never summarized, and every later prompt dropped them. Not a data race, so -race stayed clean while turns went missing. The session is re-read before the window is chosen, and again after summarizing, because the model call is itself long enough for an append to land inside the chosen range. A summary whose range was raced is discarded. The window is now bounded at CompactionInterval new invocations rather than running to the end of the session. Uncapped it was O(session) instead of O(interval), so enabling compaction on an existing deployment handed a whole live conversation to one model call, and it compounded: a summarizer error records nothing, so the next turn recomputed from the same start over a strictly larger window and was more likely to fail again. A retry is now the same size as the attempt that failed, and a backlog drains one bounded window per turn. Also: - The summarization call ignored the application's GenerateContentConfig, so the one call that sees the entire transcript fell back to provider defaults for safety settings and output limits. It now inherits the root agent's config, minus the system instruction, tools and tool config. - A summary carried no branch or isolation scope, so it passed every branch filter and leaked a sub-agent's content to the parent. It now inherits both from the window, and a window is trimmed to a single scope before being summarized. - An invocation that ended in an error is no longer summarized. The window would be a question with no answer, and that summary is stored permanently. - Compaction failures are wrapped in an exported ErrCompaction, so a caller can tell a bookkeeping failure from a failed turn instead of matching on an error string. The A2A executor currently turns the former into a task-failed event for the end user. - Non-text parts are dropped from a summary. A hallucinated FunctionCall in one would have reached the model unpaired. A part that survives is copied whole rather than rebuilt from its text, so metadata the model expects back with it, a thought signature above all, is not lost. A summary left with no prose at all is refused. - Prompt assembly no longer sorts the result by timestamp, which could put a function response ahead of the call it answers whenever clock skew or the SQL backend's microsecond truncation left the two out of order. Raw events keep their stream order, and each summary is placed where the first event it covers sat, so it still precedes any turn it does not cover. - OverlapSize is documented as repeating content rather than sharing it. The repetition is the point of the option, and by the time the summaries exist it lives in their prose, not in their ranges, so it cannot be trimmed away afterwards. Testing: every fix has a regression test, each confirmed to fail against the previous behaviour rather than passing vacuously. The gaps the review named are covered too. A non-LLM root agent, so the hook in Runner.Run is exercised rather than only runNode's. The branch that reports a failure to store a summary. An OverlapSize case that tells overlap from no overlap by comparing stored ranges. And a compaction event authored by another agent, which pins the foreign-reply clause. Build, race tests, lint and tidy are clean in both modules. One unrelated pre-existing failure in telemetry.TestConfigureExporters (an OpenTelemetry schema version conflict from the GCP detector bump) reproduces on the integration branch with no compaction code present.

Five findings from the review of #1231 that had no design question attached. The code they concern was introduced there, but it is amended here rather than in that commit so the reviewed history stays readable. A compaction record carrying no content could evict a real summary. Subsumption keyed on "declares a compaction" while substitution kept only records with content, so the weaker predicate won where it mattered: the contentless record subsumed the usable one, both were then dropped, and nothing represented the range while the boundary calculation went on pointing at the useless record. Subsumption now uses IsCompactionEvent, so only a record that could stand in for the range may displace one that already does. A streamed fragment could be stored as the whole summary. The loop returned on the first response carrying content without checking Partial, so a chunking model yielded a summary of its first fragment and lost the usage metadata that only the final response carries. This summarizer asks for a non-streaming call, so a well-behaved model never does it, but model.LLM is an exported interface and Partial exists to mark exactly this. Three nil dereferences were reachable from exported entry points: a nil element in the event list panicked both NewSummaryEvent and Apply, and a nil part panicked the transcript builder. NewSummaryEvent now reports the offending index, and the other two skip. Being total over the input is the right posture for a package whose callers include third-party Summarizer and model.LLM implementations. An event whose parts are all inline data, file data, executable code or a code-execution result rendered as nothing at all, so after compaction there was no record the turn had happened. Each now renders a short placeholder naming the kind. Dropping the bytes is deliberate; dropping the turn is not, and the same argument already justifies rendering thoughts and tool traffic. Finally, the Config doc claimed that enabling neither strategy disables compaction entirely, which Validate contradicts by rejecting exactly that. The doc was the wrong half: a Config that costs a configuration step and does nothing is almost always a mistake, and a nil Config already says "disabled" unambiguously. Testing: five regression tests, each confirmed to fail against the previous behaviour rather than passing vacuously, two of them by panic. Build, race tests, lint and tidy are clean in both modules.

… events Two review findings on #1231 that needed a decision rather than a patch. A stored compaction event satisfied IsFinalResponse. It carries a record and no content, which is exactly the shape that clause treats as a final answer, so a streaming consumer deciding what to surface would show an empty final response every time compaction ran. A compaction event is bookkeeping and now reports false. The summarization call had no bound on the transcript it built. Only tool arguments and responses were truncated, so the same payload cost 2,710 characters as a tool response and 100,663 as a text part, decided purely by which kind of part it arrived in. Text is not the more trustworthy of the two: it carries pasted documents and tool results re-emitted as text. The per-part cap now applies to every rendered part. A whole-transcript budget sits above that, and it declines rather than trims. Dropping the oldest turns would be the obvious fix and is the wrong one: every event in the window is inside the range the compaction records as covered, so removing them from the transcript while still deleting them from history would lose them with nothing standing in their place. Exceeding the budget means the window is too large, so that is what the error says. One intermediate step shrinks oversized parts first, so a couple of large payloads do not refuse an otherwise reasonable window. Testing: three regression tests, each confirmed to fail against the previous behaviour.

Four items that needed work rather than a decision. Chronology is now checked across the whole window instead of only its ends. The range is the interval between the first and last event and prompt assembly deletes everything inside it, so an interior event stamped past the last one was summarized, fell outside the range, and survived in the prompt as well: the model saw that turn twice. Widening the range to cover the true span would have been the wrong repair, since a window is a contiguous slice and stretching its range past its own ends can swallow an event that was never summarized, turning a duplicate into a deletion. I tried that first and a test caught it. A thought-only summary is refused. The transcript builder skips thought parts of a stored summary, so one rendered as nothing: the covered turns were deleted and replaced by an empty line. The author and tool names in a transcript line are escaped, not just the free text around them. Both are attacker-influenced, Author over the REST surface and tool names through a dynamically loaded tool set, and both were interpolated raw into a line whose shape the summarizer trusts. The REST mapping has a direct test. It pins the direction: a client-supplied record is dropped on the way in, a stored one is returned on the way out. Testing: four regression tests, each confirmed to fail against the previous behaviour.

…ime out Two decisions from the review, both settled by looking at what the reference implementation does. Summaries bypassed the plugin pipeline. Every other event the runner persists passes through the event callback, which is where a plugin sees, rewrites or rejects what enters a session, and a derived summary is exactly the kind of content a redaction plugin cares about. adk-python reaches the same place from the other direction: its sliding window yields the event and lets the runner append it, so persistence stays at the runtime's synchronisation point. Go now offers the summary to plugins before storing it, and stores whatever comes back. The summarizer call had no bound. It is synchronous inside the run loop, so one that hangs holds up the turn behind it with nothing to show for it. LLMSummarizerConfig gains a Timeout, defaulting to zero, which means no timeout and matches the behaviour of every ADK implementation today. It can only shorten the wait, never extend it, since the caller's own deadline still applies. Testing: the plugin test rewrites the summary and asserts the rewrite is what gets stored, so it pins that the returned event is the one persisted, not merely that a callback ran. Both confirmed to fail against the previous behaviour.

The part filter and the nil-element check arrive with this change, so the constructor's contract has to grow with them. Dropping non-prose parts is a security control rather than tidying: whatever the summarizer returns is injected into later prompts verbatim, so a function call it invented or was tricked into emitting would arrive unpaired and a model may act on it. An implementer needs to know that before wondering where their part went. Comments only, no behavior change.

Compaction previously left no trace, so there was no way to tell from production whether it was firing, how much it was shrinking, or whether the summarizer was failing. Each summarization now runs inside a "compact_events <trigger>" span carrying the trigger, session, summarizer type, event count, configured thresholds and the resulting range. Three states are distinguishable in a trace: - no span: the trigger was evaluated and declined - span with no result attributes: the summarizer ran and declined - span with error status: the summarizer failed The span name and the gen_ai.compaction.* attribute keys are part of ADK's cross-language telemetry contract, so dashboards written against one implementation work against the others. Testing: span assertions via an in-memory exporter, covering attribute values, the omission of the strategy that did not fire, and all three states above.

Five findings from the review of #1233 with no contract question attached. The parity items, which depend on what adk-python emits, are left alone. A failed compaction produced a span that was simultaneously an error and a success. A Summarizer may return a usable event alongside an error, and the caller discards that event, but the span recorded its identity and range anyway, so a trace showed a failure naming an event no session ever held. Both sides are fixed: TraceCompactionResult returns once it has recorded the error, and summarizeTraced no longer stamps a result it is about to throw away. That second half also stops a discarded event consuming a UUID. A panicking summarizer left the span status Unset, which reads as success, so third-party code blowing up looked like a healthy compaction that happened to produce nothing. The span now ends through a deferred recover that sets an error status before re-panicking. The panic itself still propagates. The telemetry tests pinned that attributes existed rather than what they contained, which left the wire format unprotected. Reproduced before fixing: switching the timestamp layout to RFC1123, and sourcing the start bound from EndTimestamp, both left the suite green. The assertions now compare rendered values, so both mutations fail, and result_event_id is required to be non-empty so a trace can be joined to the stored summary. Finally, a comment claimed a span shows only the strategy that fired. Both strategies can be configured at once and the branches gate on configuration, not on firing. The comment now says what is true and points at the trigger attribute, which is what actually names the strategy. Testing: two new tests, both confirmed to fail against the previous behaviour, one of them exercising a summarizer that returns an event and an error together, which the existing failure test could not reach since its fake returns a nil event. Build, race tests, lint and tidy are clean in both modules.

…butes Two of the three parity findings on #1233, settled by reading adk-python rather than leaving them open. Both are contract keys that a consumer joining traces across the two implementations has to match on. The range bounds were emitted as RFC 3339 strings. adk-python declares EventCompaction.start_timestamp as a float in seconds and puts that float straight on the span under the same key, so the two implementations disagreed on the type behind a shared key. They are now epoch seconds. That also removes both side defects the review found, since the host zone offset reaching the wire and the fractional zeros breaking sort order were consequences of rendering a string in the first place. The summarizer type was Sprintf("%T"), which emits a Go type expression rather than a name: "*compaction.LLMSummarizer" where the reference implementation, which uses type(summarizer).__name__, has "LlmEventSummarizer". Go now emits the bare type name. The third finding, the missing gen_ai.system, is confirmed real but left alone. The key is easy; the value is a choice between two semconv generations, since adk-python emits "gemini" or "vertex_ai" while this repo is on semconv v1.36.0 where the constants are "gcp.gemini" and "gcp.vertex_ai". No Go span sets the attribute today, and the compaction path has no backend handle to derive it from, so it is not compaction's decision to make alone. Testing: the assertions pin the attribute type as well as the value, so regressing the timestamps to strings fails with a type mismatch rather than passing on a coincidence. Both fixes were confirmed to fail when reverted. Build, race tests, lint and tidy are clean in both modules.

The reference implementation sets gen_ai.system on every compaction span and Go set it on none, so a trace could not say which system produced a summary. The backend comes from the summarizer through the same optional interface the rest of the framework already uses to tell Vertex AI from the Gemini API, rather than a new parameter threaded down from the runner. A third-party Summarizer with no model, or one that does not care to say, leaves the attribute unset instead of being made to invent a value. One deliberate difference from the reference. The values here come from this repo's semconv version, which prefixes them "gcp.", while adk-python is on an older generation emitting bare "gemini" and "vertex_ai". Mixing semconv generations inside one implementation is worse than differing from another implementation on a value, and the gap is repo-wide rather than compaction's to close. Testing: a table covering both backends and a summarizer that reports neither, which must leave the attribute absent rather than guess.

Two gaps from the #1233 review, plus a stale comment. The span carried no invocation id. It is not a child of the turn's span, so there was no way to ask which turn a compaction belonged to. It now carries the invocation of the newest event in the session, which is the turn that triggered it. The newest event rather than the newest one in the window, deliberately: the window is what is being summarized, and tail retention excludes the turn in progress from it, which is exactly the turn worth naming. The summarizer's token usage was dropped. Compaction spends a model call to save tokens later, so a span that does not record what it spent cannot show whether it paid for itself. Input and output tokens are now on the span when the summarizer reports them. Also corrected a test comment that still said a span shows the strategy that "fired". Both strategies can be configured at once, and the branches gate on configuration; the trigger attribute is what names the strategy. The production comment was fixed earlier and its twin in the test was missed.

… covers The individual assertions only check the keys they name, so adding, renaming or dropping one passed unnoticed. The whole key set is now compared, since these keys are shared with adk-python and a dashboard written against one implementation depends on them. Also narrowed two claims in the doc comment that were larger than the truth. The parity with adk-python is real and was read from source, but nothing enforces it, and adk-kotlin has no compaction telemetry at all, so "cross-language contract" meant two implementations rather than all three. Investigated and deliberately not changed: the span still wraps the summarizer call rather than the whole compaction. Starting it earlier would emit a span for every evaluation that declines, which breaks the property that a span means compaction really ran, and what it would add is an in-memory scan measured in microseconds against a model call. The comment now says so instead of leaving the scope unexplained. Also reverted a change I had made and could not justify: passing the invocation context to the compaction hook so the span would nest under the turn. It made no difference, because the outer context already carries the caller's span, and the turn's own span has ended by the time compaction runs so it cannot be the parent anyway. A test now pins what is actually true: compaction shares the caller's trace and names the invocation it followed.

…ken threshold Sliding-window compaction only runs between turns, so it cannot help a single invocation whose own tool traffic inflates the prompt past the context limit. Loading a large skill file, or a long chain of tool calls, can exhaust the window before the turn ends. Tail retention runs immediately before each model call. Once the most recently reported prompt token count reaches TokenThreshold, it summarizes everything except the most recent EventRetentionSize events, which stay raw so the model keeps immediate continuity. Each new summary is seeded with the previous one, so history stays as a single rolling summary plus a raw tail rather than an ever-growing chain of summaries. The two strategies are independent. Either alone is a valid configuration and both can be armed together. Unlike the post-invocation pass, a failure here is surfaced immediately. Compaction fires precisely because the prompt is near the context limit, so continuing would likely fail the model call anyway with a far less informative error. The request processor needs the config and the session service, neither of which agent.InvocationContext exposes. Adding them would break every external implementation of that interface, so they travel on the context, as parentmap, runconfig and plugininternal already do. Testing: window selection including the same-timestamp cut and the rolling-summary seed, token counting with and without observed usage metadata, plus runner tests for the mid-turn trigger, the threshold gate, error propagation and both strategies running together.

…ow has Review of #1234 found that the mid-turn tail-retention path never inherited the hardening the post-invocation sliding-window path received. The two paths run the same algorithm against the same session, so every defect fixed on one side was still live on the other. Two of them were worse mid-turn than they had been post-invocation. Appending the summary to the session on the invocation context fails outright when an agent has wrapped it. A coordinator hands a sub-agent a session wrapped to carry a synthetic first turn, and every session service type-asserts on its own concrete type, so the append is rejected. Nothing surfaced: the failure became a tool-error response and the coordinator answered on top of a broken delegation. The processor now compacts against the session underneath any wrapper. Unwrapping rather than re-reading is what makes this work, because the wrapper reads through to the session it decorates on every call, so a summary appended underneath reaches the prompt this processor deliberately runs ahead of. A freshly read session is a different object and the summary misses it. A summarizer error that wrapped context.Canceled was swallowed whole. The error rides the flow's error channel into the workflow scheduler, which tests for a context.Canceled chain before anything else and drops what it finds, so a cancelled summarizer ended the turn with no answer, no events and no error. The ErrCompaction sentinel added in #1232 does not help, because the scheduler never consults it. The cause is now rendered with %v, which keeps it in the message and out of the chain. Callers lose errors.Is against the cause and keep it against ErrCompaction, which is the right way round for bookkeeping. The remaining three are ports of code that already existed in runner.go: - The session is re-read after summarizing and the summary is discarded when RangeRaced reports that another invocation appended inside the chosen range. The read is only a comparison here, because the append has to keep object identity with the session the prompt is built from. - ctx.Err() is checked before the summarizer call and before the append, so a cancelled turn neither spends a model call nor writes a summary nobody is waiting for. - The window is trimmed to a single branch and isolation scope, and the rolling seed carries the previous summary's scope rather than only its branch. A window spanning a boundary produced one summary that misattributed half its content and, stamped with the first event's empty scope, was readable by agents the filters exist to keep it from. Also corrects an assertion added alongside the tail-retention span test: it checked for the absence of "gen_ai.compaction.interval", but the key is "gen_ai.compaction.compaction_interval", so it could never have failed whatever the span carried. Testing: internal/llminternal/compaction_processor_test.go is new, and covers the wrapper, cancellation and raced-summary paths; the file had no tests at all before. Every fix has a regression test confirmed to fail against the previous behaviour, and the wrapper one reproduces the reviewer's error verbatim. Build, race tests, lint and tidy are clean in both modules.

…lling turns Five findings on #1234, and one wiring bug found while fixing them. A turn compacted mid-flight was compacted again the moment it ended. The two strategies are independent triggers on the same history, so a turn that crossed the token threshold paid for a second model call to re-summarize what had just been summarized, and left two ranges over overlapping spans. The reference implementation avoids this by evaluating both in one place and returning early; the same effect is reached here by recording, on the per-invocation compaction runtime, that a compaction already ran. Wiring bug found by the test for that: in the node path the runtime was allocated inside newNodeInvocationContext, so the mid-turn processor and the post-invocation hook held different instances and no hand-off could ever arrive. One runtime is now attached to the invocation's context before either is built from it. The token scan read the summary's own usage metadata. A summary carries the usage of the summarizer's call, which measures the transcript it was handed rather than the agent's prompt, and that number sits far above the threshold. Every later turn therefore saw the threshold crossed and compacted again. Compaction events are skipped in the scan. A mid-turn compaction failure aborted the turn. Tail retention runs before a model call, inside an invocation whose tools may already have run and committed their side effects, so aborting cost the user an answer while the side effects stood and any summary already written was orphaned, all to report that an optimisation did not happen. Failures are now logged and the turn continues with a larger prompt, which usually still fits since the threshold sits well below the real context limit, and which otherwise fails with the provider's own error. The failure is not lost: the compaction span records it with an error status. A config with a token threshold and no retention size is now rejected. At zero the window extends to the newest event, which is the question being answered, so the turn in progress was summarized out of its own prompt. The reference implementation shares this behaviour; it has no legitimate use in either. Finally, a compaction that fired and could do nothing now emits a span saying so. A trigger that never fires stays silent, so a span still means compaction was wanted, but the case where the threshold is crossed and nothing may be summarized looked exactly like an idle session while the prompt grew on every turn. Testing: the hand-off test failed vacuously at first, passing whether or not the gate was present because the configuration never made both strategies want the same turn; it is now tuned so they collide, and it catches both the missing gate and the split-runtime wiring. Two tests that pinned the old abort behaviour were rewritten to pin the new contract rather than deleted.

Six items from the #1234 review that needed work rather than a decision. An event stamped exactly at the previous compaction's end was lost. Candidates were filtered on "strictly after that instant", while the new range, seeded with the previous summary, starts back at the previous start and so covers it. Such an event went into no window and inside the next recorded range: summarized by nothing, and dropped from every prompt afterwards. Candidates are now taken by stream position, which has no ties. The estimator counted bytes while its constant and its documentation both said characters, so the same ten characters cost 2, 7 or 10 depending on the script. It counts runes now. A failed session re-read still killed the turn. Everything else on that path degrades, for the good reason that tools may already have run; this one path did not. It does now, so no mid-turn compaction failure can fail a user's turn. RangeRaced ignored compaction events entirely, so two invocations could each summarize the same span and both record it. It now treats a compaction event that is new since the window was chosen as a race, while still ignoring the one the window was built from. The rolling seed is labelled rather than anonymous, so a summary is not mistaken for something the agent said. compactionctx had no tests at all. It has them now, including the nil receiver every caller relies on and the concurrent marking that is the reason the flag is atomic. One change was reverted after a test disagreed with it. Making LatestCompactionEvent require usable content looked like the obvious way to unify the two predicates, but a contentless record still marks how far compaction reached, and requiring content there makes the next window re-summarize everything the broken record covered. The existing test says so. The asymmetry is deliberate and is now documented where it lives.

Three decisions from the review, and one tidy-up. Compaction could fire before every model call in a tool loop and achieve nothing. If the retained tail alone already exceeds the threshold, each round summarizes a little more and leaves the prompt exactly as far over, so every round pays for a summarizer call that changes nothing. Measured: six calls inside one seven-call invocation. The gate is a progress check rather than a cap. A compaction is allowed when the prompt is smaller than it was at the last one in this turn, so a long turn can still compact repeatedly while that is helping, and stops as soon as it is not. Neither adk-python nor adk-kotlin has any brake here, so this is a deliberate divergence. The token count no longer lags by a whole turn. A reported count describes the prompt of an earlier call, so everything appended since was invisible and the call that first crossed the threshold was missed. An estimate for the events since that report is added to it. Runtime's fields are unexported behind accessors and a constructor. One Runtime is shared by every goroutine in an invocation and the config it points at is shared across every invocation of the runner, so an exported pointer field invited a mutation that would leak between turns.

…rface Compaction could only be enabled by constructing a runner directly, so an agent served through the launcher, the REST server, A2A or Agent Engine had no way to turn it on. Anyone following the quickstart, which uses the launcher, could not use the feature at all. EventsCompactionConfig is now accepted by launcher.Config and adkrest.ServerConfig, and reaches the console, the web UI, A2A, Agent Engine and the pub/sub and Eventarc trigger endpoints. The A2A executor needed no change, since it copies the whole runner.Config. Three REST controller constructors take their dependencies positionally and had no config struct to extend. Rather than change their signatures, each gained a trailing variadic option. Every existing caller compiles unchanged, which tests pin. The alternative of hanging the field off triggers.TriggerConfig would have been a smaller diff but semantically wrong: that type carries retry and concurrency policy, not prompt management. Also adds examples/compaction, which follows the same launcher shape as examples/quickstart so the only thing that stands out is the compaction config itself. Testing: a REST integration test driving real HTTP through httptest.NewServer, proving the chain from ServerConfig to a compacted prompt and that leaving the field unset changes nothing, plus backward-compatibility pins on the three variadic constructors.

Three findings from the review of #1235 that do not depend on how the constructor-signature question is resolved. Only the CLI launcher validated the compaction config. The REST server and the Agent Engine handler did not, and both build a runner per request, where the config is validated. An invalid config therefore produced a server that started cleanly and then failed every request with a 500 naming nothing the operator could act on. Both now refuse to construct, naming the field. A nil option panicked all three trigger and runtime constructors. Options are commonly assembled by a helper that returns nil when it has nothing to apply, and a variadic parameter makes passing one easy, so panicking during construction is a poor way to report it. Nil options are skipped, and a nil one no longer prevents a later real option from applying. The example's doc was wrong in both directions about which surfaces the launcher serves, and the command it printed does not run. Reproduced rather than inferred: "go run ./examples/compaction web" exits with "no active sublaunchers found". The doc now shows "web webui", says what the bare form does, and lists what full.NewLauncher actually serves, which includes the Pub/Sub and Eventarc triggers and the REST API and does not include Agent Engine. Agent Engine reads the same config field but is served by its own handler. Testing: two regression tests, both confirmed to fail against the previous behaviour, one of them by panic. Build, race tests, lint and tidy are clean in both modules.

Adding a trailing variadic parameter to an exported function changes its type. Ordinary call sites keep compiling, which is why this looked safe, but anything that referenced the constructor as a value or stored it in a field of that function type stops compiling. Three constructors were affected, and they are in a released API: v2.2.0 was tagged shortly before the commit that changed them, the routers package is internal so calling these directly is the only supported way to mount the controllers, and examples/bidi does exactly that in this repository. Each signature is restored byte for byte and a WithOptions sibling carries the optional settings. The in-repo call sites use the sibling. The test that claimed to pin backward compatibility could not: it was a plain call expression, and a call expression still compiles after a variadic parameter is added, so it passed throughout. It now assigns each constructor to an explicit function type, which is the form that actually fails when the signature changes. Verified by re-adding the variadic and watching it fail, and by compiling a file that holds the constructor as a value against the commit before this one, where it does not build.

The REST server and the Agent Engine handler already validated. The launcher did not, so `web a2a`, `web pubsub` and `web eventarc` started healthy and then failed every request, because the config is checked inside runner.New and a runner is built per request. launcher.Config gains a Validate, called from each Execute entry point, so the failure lands at startup naming the field instead of arriving as a 500 per request.

Two decisions from the #1235 review. A trigger controller runs each delivery in a session of its own, so history never accumulates across messages and the sliding window, which counts completed invocations within one session, can never reach its interval. Configuring only a sliding window there is almost certainly a mistake, and it used to be a silent one: the operator sets compaction and believes it is working. It is now logged, and the option's documentation explains which strategy does work there, since tail retention measures the prompt inside a single run and is unaffected. The REST server's compaction config is documented as server-wide. One server can serve many applications through its agent loader, and they all get the same config and the same Summarizer instance, so the same model. Scoping it per application is a larger change; saying so is the honest minimum. Also corrected the last two copies of the claim that compaction keeps prompts small, which had been fixed at the tip of the stack but not in the files these two pull requests own.

Every other compaction test uses a fake summarizer or a fake model, and a fake accepts any prompt handed to it. So nothing so far establishes that a compacted prompt is actually well formed: that role alternation holds, that no function response is left without its call, and that the API accepts the result. This drives three turns with a tool call through a real model, replayed from an httprr cassette. The recorded run confirms compaction firing after two invocations, tool traffic reaching the summarizer transcript, the summary replacing the raw turns, and the model still answering correctly from the summary alone. The summary's wording is deliberately not asserted. It is model output, and pinning it would fail on any model or prompt revision without indicating a real problem. The test skips when the cassette is absent, so an unrecorded checkout still has a green suite.

Review of #1236 found that the test's own documentation claimed more than the test could deliver, and that several of its settings and omissions were justified by reasoning that does not hold. None of this needed a re-recording: the cassette replays unchanged. The central claim was that only a real request can show a compacted prompt is well formed. Replay does not work that way. It keys on exact request bytes, so a structural defect introduced later shows up as a cassette miss that aborts the run on the turn loop, before any assertion is reached. The properties are now asserted directly and offline, over the prompts the agent actually sent: every function response must be preceded by the call it answers, checked across every captured prompt. One gap is real and is now documented as a gap rather than claimed as coverage. The recorded conversation issues no tool call after the compaction point, so the final prompt carries no function traffic and pairing across a summary is not exercised. Pairing itself is, on the two pre-compaction prompts. Closing the rest needs a re-record whose fourth turn calls a tool after the summary exists, which needs credentials. Also: - A missing cassette failed the test rather than skipping it. The cassette is committed, so its absence means it was lost or renamed, and skipping turned that into a silent pass. - OverlapSize is removed rather than re-justified. It was inert by construction, not merely unexercised: the first compaction of a session starts at the first invocation whatever the overlap is. - The recorded recall answer was discarded into a blank identifier, so the memory property the test exists to demonstrate went unasserted. It is now checked. - The second compacted turn was never asserted absent, leaving half the compacted range unchecked. - The package-level go:generate directive already matches every cassette here, so the third directive only added another way to re-record all of them by accident. Removed, with a comment saying the absence is deliberate. - The doc said the summary wording is not asserted while an assertion depended on it. The two now say different things: no particular wording is expected, and whatever the summarizer produced must be what reaches the next prompt. Testing: the three new assertions were each confirmed to fail when inverted or stubbed, so none of them passes vacuously. The cassette is unchanged and still replays. Build, race tests, lint and tidy are clean in both modules.

…ording The recorded conversation stopped calling tools after the compaction point, so the only post-summary prompt carried no function traffic and the call-pairing and call-recovery paths were inert against the cassette. That was the one property this test is better placed to check than an offline test, and it was the one property it did not check. A fourth turn calls a tool after the summary exists. The recording now holds a prompt with six contents carrying a summary, a function call and its response together, which is the arrangement pairing has to survive. Recorded against gemini-3.5-flash. The cassette carries no credentials: the request scrubber drops the API key header, and the tree was checked for key material afterwards. Two assertions had to move with the extra turn, both because they were reading the wrong thing rather than because the property changed. The recall check now reads the answer to the turn that asks about the colour rather than whichever turn is last, and the summary check now uses the first stored summary rather than the last, since with four turns the last compaction is written after the final model call and cannot appear in any recorded prompt. The new arrangement is asserted rather than assumed: at least one prompt must carry a summary and function traffic together, so a later re-record whose conversation stops calling tools after the compaction point fails instead of quietly losing the coverage again.

The reviewer asked for a check that the summary stands for the range it claims, not just that two named turns disappeared. Every event inside the first compaction's range is now required to be absent from the final prompt, which is the contract rather than a sample of it. Also removed two paragraphs still describing the skip-on-missing-cassette behaviour that was replaced with a hard failure, and the recording instructions that told the reader the test skips until a cassette exists.

karolpiotrowicz

Reviewed at f3cd4a4.

Most of this lands. I re-ran the original reproduction for each finding rather than reading the diff, and the ones that mattered most are genuinely closed:

The non-ASCII paste that used to brick a session now completes — turn 1 error nil, 0 of 5 following turns failing, 6 compactions stored, against 5/5 failing and 0 stored before.
The chronology check no longer poisons a session — 0 of 4 deterministic trials and 0 of 12 concurrency trials, against 4/4 and 5–7/12.
trimToOneScope advances across five distinct windows instead of repeating one.
The tied-timestamp coverage gaps are gone, because inRange now requires the ID rather than the interval.
The prose filter blocks a transfer_funds call returned by a summarizer from reaching the prompt.
ErrCompaction has four consumers, so a bookkeeping failure no longer discards an answer the agent already produced.

Both design changes are implemented with more care than I asked for. Excluding ID-less events from the covered set with the duplication-over-loss trade written down, giving the rolling seed the previous record's own ID so subsumption is clean, and keeping an honest "not fixed here" section for the span-parenting gap are all good calls.

What blocks is that the coverage fix removed something that was holding up the roof. There are now three ways compaction silently stops while the prompt grows without bound, and each is quieter than the bug it replaced — the old failure was loud data loss, the new one is a session that looks healthy and grows forever.

1. Tail retention never reconsiders a retained tail. selectTailRetentionWindow picks candidates by stream position, meaning everything after the previous compaction event. Each round's retained tail sits before that event, so it is never a candidate again. Under the interval model the next record's widened range swallowed those events and deleted them. That deletion was the bug this PR fixes, and it was also the only thing bounding growth.

On an identical 300-turn run at TokenThreshold: 1000, EventRetentionSize: 2, the prompt reaches 602,700 characters against 4,030 on the base branch, still climbing about 2,000 a turn.
Minimal version: after round 1 retains two events, round 2's window is empty and those two are in no summary and in every later prompt.
The package doc at compaction.go:31-34 says tail retention is what bounds prompt size and can be enabled on its own. That is no longer true of the "on its own" case.

2. The sliding window can return nil forever. endID is still chosen by position in order, and the new staleAt skip was added to fix the branch-change stall. Together they can put the first uncovered event of startID after the last event of endID, at which point selection returns nil and nothing ever advances coverage.

Reproduced deterministically at the documented CompactionInterval >= 2 with a late-resuming invocation, the shape a HITL approval or a long-running tool result produces: eight consecutive turns with an empty window while prompt size goes 4, 6, 8, 10, 12, 14, 16, 18. No error and no telemetry. This one is a regression from the branch-change fix rather than something the coverage change exposed.

3. A concurrent append during summarization is skipped permanently. It lands before the compaction event, so it is absent from CoveredEventIDs — it did not exist when the set was built — and it also sits before start, so it is excluded from every future window.

One change closes all three: select candidates as "every event no surviving compaction names", which is the predicate selectSlidingWindow already has in coveredByAny, rather than by stream position. A test asserting prompt size stays flat over fifty tail-retention rounds would have caught all three, and nothing in the suite currently does.

On Vertex AI, compaction is now worse than leaving it off. The design makes prompt correctness depend on session.Event.ID surviving a round trip, and on that backend it does not:

The append envelope carries no ID field.
On read, event.ID = id overwrites it from the server resource name, under a comment saying identity fields are authoritative on the envelope.
The repo already knows. session/vertexai/service_test.go:52 sets ProvidesServerAssignedEventID: true, and the shared suite skips ID equality for it.

Because the set is then non-empty but dangling, inRange does not fall back to the interval and returns false for everything. Nothing is dropped, so the prompt after a round trip carries every summarized turn plus the summary, the summary is placed after the turns it summarizes, and the window is re-selected every turn, billing a summarizer call forever.

Related: 51d6f38 assigns a missing event ID in the in-memory and database services and skips Vertex AI. The shared suite has a case for exactly that, and on Vertex AI it skips for want of a replay recording, as does compaction_record_round_trips. Those two skips mean the backend this design depends on is the one backend with no coverage of it.

The Summarizer refactor removes the declarative control but not the effective one. compaction.go:180-186 justifies the breaking change on the grounds that a summarizer returning a whole event "could also set the authorship, the state delta, an agent transfer, and the range of history to delete". It still can, indirectly:

collect copies the slice but not the events, so third-party code receives the session's live *session.Event pointers.
There is no defensive copy before the call.
newSummaryEvent then derives the range, the branch and the isolation scope from those same objects afterwards, at summary_event.go:78-86 and :108-113.

So a summarizer that returns innocent prose but first walks its input rewrites the stored conversation in place, dictates the covered range by moving every timestamp, and escapes the isolation scope by clearing Branch on the window. The interface doc says the events passed in are never modified, and nothing enforces it. Snapshotting {ID, Timestamp, Branch, IsolationScope} before the call and deriving the record from the snapshot closes the range and scope halves cheaply. Only a deep copy closes the history rewrite.

A plugin's replacement summary is appended with no re-validation. At runner.go:328-333 a non-nil modified from RunOnEventCallback replaces the summary and goes straight to AppendEvent. newSummaryEvent does not run again, so neither does the prose filter it exists for. A plugin returning CompactedContent with a text part plus a FunctionCall gets that unpaired call into a real model prompt.

A discarded summary permanently disarms the progress gate. RecordAt's own doc says to call it once the summary is in hand, never before, precisely so a transient failure cannot disarm compaction for the rest of the invocation. It is called at tail_retention.go:138-140 as soon as the summarizer returns, but the caller can still discard that summary four ways afterwards — cancelled turn, failed re-read, RangeRaced, failed append. Each leaves the gate armed with nothing stored, and Recovered() cannot re-arm it because the prompt never drops. Same failure the doc says was fixed, reached through the discard paths rather than the error path.

The covered-ID set is unbounded, and lookup is now quadratic. A rolling summary inherits the previous record's IDs, so the set is one entry per conversation event forever and each turn persists a fresh copy:

400 entries and about 25 KB in a single record after 200 turns.
40,200 entries and 1.68 MB across the session, with nothing pruning or capping it.
Apply then does a linear slices.Contains per event on the per-model-call path, making prompt assembly O(events × covered).

Worth considering before the format is persisted in real sessions: record the exceptions rather than the membership. The interval was only ever wrong about the holes window selection leaves, and holes are rare — zero in the single-agent case, one in the multi-agent case I measured — so an interval plus a short exclusion list carries the same information in bounded space.

A legacy seed silently undoes compaction. Inheriting a record written before this change gives the new record a partial set, the new events only, which is non-empty. That disables the interval fallback, and the old record is then subsumed by ID and dropped. With a modern seed the prompt is [rolled]. With a legacy seed it is [old-1, old-2, rolled]. Nothing is lost, but compaction is undone and a paid-for summary is discarded, on the first pass after upgrading, which is when a long session is closest to its limit. Either propagate the interval semantics onto the merged record, or drop the fallback and say pre-change records are unsupported.

Two app-level generation settings now break the summarizer. The allow-list inversion is right, but MaxOutputTokens, StopSequences and CandidateCount are still inherited:

An ordinary app-level MaxOutputTokens fails every summarization, so compaction never runs.
A StopSequences hit reports STOP, which the new finish-reason check accepts, so a truncated summary is stored and the covered turns are then deleted.
CandidateCount: 4 bills four generations for a summary of which one is read.

Smaller things:

The rewritten RangeRaced and its overlaps helper have no test at all, and f4dce0b ships three of its four fixed surfaces untested.
Moving RecordAt past the summarizer fixed the transient-failure latch but widened an existing race from nanoseconds to the length of a model call.
Config.Validate's tail-retention branch is pinned only from cmd/launcher.
OverlapSize is still unbounded, so the window cap bounds one end only.

Two things outside the code:

7633fe0, whose subject is the sliding-window branch fix, carries 714 of its 813 lines as a console line-editing feature and adds a new direct dependency, github.com/ergochat/readline. Neither is mentioned in the commit message or the PR body, and dependency additions want calling out per CONTRIBUTING.md. That belongs in its own PR.
On the self-contained-commits claim, 51d6f38 and 01d2eba are both red in isolation, with 01d2eba's breakage described in the following commit's message. The PR has fifteen commits, not fourteen.

Happy to look again once candidate selection moves off stream position. That one change is most of this.

The window is trimmed to one branch and one isolation scope, so when the branch changes inside an invocation the cut stops short of that invocation's last event. Two things then conspired to freeze it there. Progress was measured against the newest compaction's end timestamp, so the partly-summarized invocation stayed "new" for ever, and the slice was taken from that invocation's first event however much of it had already been summarized. Every later turn recomputed a byte-identical window and paid for a model call that changed nothing. Both now ask what is covered rather than comparing timestamps, which the covered-event set makes exact. A window that stops halfway through an invocation resumes from where it stopped on the next pass: measured across four passes, [a b] then [c] then [d e] then nothing left, against the same window three times before. Overlap still works. It re-summarizes whole earlier invocations deliberately, so only events belonging to the new invocations are skipped as already covered. A first attempt skipped every covered event and silently disabled overlap, which two existing tests caught. Forking a child branch inside one invocation is the ordinary multi-agent shape, so this affected any workflow using the dynamic scheduler or a parallel worker. Also pins an explicit summarizer in the end-to-end test. The default summarizer now carries a timeout, and a deadline on that call reaches the wire as an X-Server-Timeout header, so it became part of what the recording has to match. Naming the summarizer in the test keeps the cassette independent of a number that belongs to the runner rather than to compaction. Addresses finding 3 of the #1232 review.

The compaction span ended when the summarizer returned, before the caller had decided anything. Five paths then discarded the result: a cancelled turn, a failed re-read, a competing compaction, a plugin rejecting the summary, and a failed append. Every one left a span reporting success and carrying a result_event_id naming an event that exists in no session, so a trace could not tell a compaction that shrank a prompt from one that spent a model call and changed nothing. The span now stays open until the caller reports the outcome, and a discarded summary is recorded on the same key as a decline: to anything reading the trace the two mean the same thing. Four smaller corrections in the same area: - A panicking summarizer set an error status but recorded no exception event, so an alert keyed on exception.type never saw one. - Output tokens counted candidates only, where TraceGenerateContentResult in this same package counts candidates plus thoughts and cites semconv for it. Two spans in one trace meant different things by one key, and a thinking model's summary was under-reported. - gen_ai.system had a second copy of the backend mapping. It now uses the one the rest of telemetry uses, so a future change reaches both. The two divergences from the reference implementation are unchanged and are now documented at the test that pins them, rather than the test simply freezing the current behaviour. - The invocation the span names was read from the newest event in the session, which is a guess that goes wrong exactly when it matters: with two invocations in flight, both compactions read the same event and at least one named a turn that did not cause it. The caller passes it now. Not fixed, and now stated as a known gap in the test: with no ambient caller span the compaction span is a root of its own rather than joining the turn. Closing it needs the invocation's span context to reach the runner, and the agent derives it internally and passes it only to its own children. An attempt to parent from the InvocationContext was a no-op and is not in this change. Addresses findings 1 and 3 of the #1233 review, its two smaller notes, and part of finding 2.

…erve NewServer's own comment says it validates the compaction config there so a bad one cannot "start cleanly and then fail every request with a 500 that names nothing the operator can act on". That is what happened anyway, because Validate() only checks the config against itself. A config with no Summarizer is perfectly well-shaped, and over a root agent that is not an LLM agent there is no model to build the default summarizer from, so every request 500'd with an error naming an internal type. Reproduced with a single-app loader, so this was not a multi-tenancy problem. The trigger controllers had it worse: they returned only a controller, so they had no way to refuse anything. An empty compaction config constructed fine and then failed every delivery, and on Pub/Sub push a 500 is a NACK, so the message came back, failed again, and the subscription span. Both WithOptions constructors now return an error. They are new in this stack, so nothing existing has to change, and the two older constructors keep their signatures. Validation is a dry run of runner.New per app rather than a second copy of its checks, so it cannot drift from the path a request takes. It runs twice, once without the compaction config and once with it, and reports only a failure the compaction config introduced: everything else a runner needs may legitimately be absent at construction time, and refusing on that would reject setups that work. Addresses findings 1 and 2 of the #1235 review.

Three defects, all in tail retention, and all worst in long tool-using multi-agent sessions, which is the shape this strategy is for. One unanswered tool call stopped it for good. The window is anchored to the last compaction boundary, so a call awaiting human approval, or one whose backend died, sat at the head of every later attempt. The sliding window already steps past a blocked head; tail retention gave up, and gave up silently, because "no self-contained prefix" and "nothing to do" both come back as nil. Measured with 38 compactable events stuck behind one pending call. Stepping past a blocked head could split a call from its response. The prefix scan only tracks obligations opened inside the slice it is handed, so a response whose call sits in the skipped head looked unremarkable: the response was summarized, the call stayed raw, and the model was shown a call it had already answered with the answer gone. Refusing every unmatched response would be too strong and would stall the ordinary long-running-tool resume, where the call is behind the compaction boundary and legitimately already summarized, so the test is specifically whether the call is in the head this pass skipped. Fixing this had to come before reusing the fallback, or tail retention would have inherited it. The token count was read from anywhere in the session. It decides whether this turn's prompt is too large, and that prompt is built with branch and isolation-scope filtering, so a sub-agent whose own prompt is a few tokens read its parent's 200,000 and compacted history it had no business touching. The three things a pass needs to know about its turn are now one TurnScope value rather than a growing list of strings, and the branch rule moves to internal/utils so prompt assembly and the code reasoning about prompt size share one definition. The estimator counted only part.Text, which is precisely what a tool loop does not grow by: 400,000 characters of function responses moved it by nothing. It now counts tool names and payloads too. This one mattered more than it looks, because the lag estimate added earlier in this review feeds on it, so the mechanism for catching a growing tool loop was blind to tool loops. An existing test asserted that a function call contributes nothing to the estimate, which is the bug written down as an expectation. Addresses the "a few others" paragraph of the #1234 review and finding 2 of #1231.

Four symbols were exported that nothing outside the framework has a use for. Each one is a promise we would have to keep. DefaultPromptTemplate was the worst of them. adk-python keeps the equivalent private (_DEFAULT_PROMPT_TEMPLATE on LlmEventSummarizer) and lets callers replace it through a constructor argument without reading it, which is the right shape: the wording of one default is not a contract. Exported, and now that the apidiff guard has landed, every later improvement to that prompt is a breaking change with a label to match. Callers can still supply their own template, which is the part that matters. DefaultMaxToolContentChars and DefaultMaxTranscriptChars go the same way, and for the same reason python keeps its own cap private. The numbers are in the field documentation where a caller reads them. IsCompactionEvent had no caller outside internal packages. It also carried a distinction subtle enough to have caused a bug: it answers "is there a usable summary here", not "is this event bookkeeping", and mixing the two let a contentless record evict a real summary and authorise deleting the events it claimed to cover. It moves to compactioninternal as HasUsableSummary, named for the question it actually answers. The public surface of session/compaction is now Config, Summarizer, LLMSummarizer, LLMSummarizerConfig, NewLLMSummarizer, ErrCompaction and ConversationHistoryPlaceholder. The placeholder stays because a custom template is invalid without it, and hardcoding the literal is worse. LLMSummarizer.GetGoogleLLMVariant also stays. It is reached by interface assertion from another package, follows the pattern the rest of the framework uses to tell Vertex AI from the Gemini API, and lets a third-party summarizer report its own backend to telemetry.

…covered Naming every event a summary replaces was the wrong half of the pair, and it took the bound on prompt growth with it. EventCompaction now records the range plus the events inside it the summary does NOT stand in for. Same information, because holes are what window selection actually creates, and there are normally none: a membership list carried one entry per conversation event, for ever, recopied onto every rolling summary, reaching 40,000 entries across a session and turning prompt assembly into a linear scan per event. It also fails in the safe direction. An ID that matches nothing excludes nothing, so coverage falls back to the plain range, where the inverse list matched nothing, covered nothing, and left compaction paying for summaries that never shrank a prompt. The growth was the real damage. Candidates were chosen by stream position, so each round's retained tail, which sits before the record written after it, was never offered again. While coverage was a plain interval the next record's widened range swallowed those events and deleted them. That deletion was the bug the covered-ID set fixed, and it was also the only thing bounding the prompt: 66,409 characters at 300 turns and still climbing, against 256 and flat. Selection now asks what is covered, so the tail comes back round and is summarized instead of either deleted or accumulated. That also picks up an event a concurrent invocation appended mid-summarization, which was excluded by position for ever after. coveredByAny needed the stream-position guard coveredBy already had, or selection skipped events prompt assembly keeps, which an existing tied-boundary test caught. Two separate ways the sliding window could return nil for ever, both silent because an empty window and "nothing to do yet" are the same answer. The slice was anchored on endID's own events, so once that invocation was fully covered the bounds inverted; it is bounded by invocation position now. And the interval counted covered invocations, so one invocation that can never be compacted, a call awaiting approval being the ordinary case, pinned the start and held the end one step behind it. It counts only invocations that still need summarizing. The prompt-size-stays-flat test is the one the suite never had, and it fails against position-based selection.

… far Three separate holes, all reachable by code the framework invites you to write. A Summarizer received the session's live event pointers. The slice was copied and the events were not, and the record is derived from those same objects after the call, so narrowing the return type stopped a summarizer declaring an authorship or a covered range and left it able to impose both by writing to its input. Reproduced: the user's original instruction rewritten in the store, the range forged by moving timestamps, the isolation scope escaped by clearing Branch, and a compaction record planted on a live event, which puts arbitrary content and an unpaired function call into a later prompt. It gets copies now, content included, which is what the interface always said it got. A plugin's replacement summary went to the session unexamined. Routing summaries through the plugin pipeline is deliberate, so a redaction plugin can see them, but the replacement never passed through the builder that filters a summarizer's output. A plugin returning a text part plus a FunctionCall got that unpaired call into a real model prompt. Replacements are filtered now, and one left with nothing usable is discarded rather than stored empty. The progress gate was armed by an attempt, not a result. Moving the call past the summarizer fixed the transient-error case and left four more, because the caller can still discard a good summary: a cancelled turn, a failed re-read, a competing compaction, a failed append. Each closed the gate on a summary that never existed, and Recovered cannot reopen it because the prompt never drops. Recording now rides on the same callback that closes the span, so it happens only when the summary is really stored. Addresses findings A, B and C of the #1316 review.

Inverting the deny-list to an allow-list was right, and three of the settings it let through mean something different for a summary than for a reply. MaxOutputTokens is sized for the agent's own output. A summary of a whole window is longer than a reply, so an ordinary app-level cap fails the summarization outright and compaction silently never runs. StopSequences are chosen for the agent's output format. A hit reports finish reason STOP, which is indistinguishable from finishing normally, so the truncation check added earlier in this review waves it through: a summary cut off at the first occurrence of the token is stored, and the covered turns are then dropped in favour of it. CandidateCount bills one generation per candidate and only the first is read, so an application asking for four pays four times for one summary. Addresses finding F of the #1316 review.

The exclusion list named events by ID, and one backend does not keep them: the Vertex AI service replaces a client-assigned event ID with the server resource name on read. Every reference then matched nothing, so a summary covered the holes it was supposed to leave alone and deleted them from the prompt. It now refers to an event by invocation and timestamp, both of which survive that round trip. No storage change and no re-recorded fixtures, and the format stops depending on a field that is not stable everywhere. This works because exclusions are safe to be imprecise about, in the direction that matters. A reference that matches nothing excludes nothing, so coverage falls back to the plain range. A reference that matches two events of one invocation sharing a timestamp leaves an extra event raw beside a summary of it. Both are visible and recoverable. Naming members rather than holes had the opposite failure: a key that did not match meant covering nothing at all. One consequence, and the reason RangeRaced guards ordinary turns again. The hole list is computed from what the framework can see when the summary is built, so an event a concurrent invocation appends afterwards is inside the range and named by nothing, which reads as covered. Naming members made that case safe by omission; naming holes does not, so the race check has to cover it. An existing test caught this the moment the key changed. Whether session.Event.ID should survive a Vertex AI round trip at all is a real bug, since anything that remembers an ID is broken by it, but it is a Vertex bug rather than a compaction one and wants its own change and its own re-record.

Tail retention seeds its window with the previous summary so the new record spans the old range and supersedes it. It did not, and the prompt grew with the length of the conversation, which is the one thing this strategy exists to stop. The exclusion list is built by scanning every stored event inside the new range and recording the ones the window left out. The events the previous summary already covered are exactly that shape: inside the range, and absent from a window that carries the summary in place of them. So every pass declared the previous pass's content a set of holes. A record that leaves out what an older one covered cannot subsume it, so the older record survived into the prompt, and the pass after that inherited both sets of holes and added its own. An event an earlier summary covers is covered by this one too, transitively, through the seed the window carries. Skipping those leaves the ordinary case with no exclusions at all, which is what it should always have been: one summary reaches the model, and the record stays a range plus the few genuine holes window selection left behind. Measured over sixty turns with a 520-character summary: 24,991 prompt characters carrying forty-eight summaries before, 551 carrying one after. The exclusion list on the last record went from ninety-eight references to none, on a session holding a hundred and sixty-eight events. Both tests that should have caught this were green. TestSelectTailRetentionWindowSeedsPreviousSummary passed the window itself as the list to scan for holes, so the scan could not see anything the window had left out, and the test agreed with itself. TestTailRetentionKeepsThePromptBounded used a seven-character summary, and forty-eight of those is still a small prompt, so the size assertion held while the property it names was broken. Both are fixed here, the second by summarizing at a realistic length and by counting the summaries that reach the model rather than only their total size. Both fail without the change. The sliding window is unaffected: it does not seed, its records carry no exclusions, and a summary per completed block is the constant-factor reduction it documents.

Four leftovers from the #1231 to #1236 round, none of them large. UnwrapSession followed a chain of session decorators with no bound. The interface is matched structurally, so any session carrying an Unwrap method qualifies, including one written outside this repository, and a wrapper returning itself spun the loop rather than failing the invocation. It now gives up after a fixed depth and returns the last session it saw, which the caller can still use. agentengine.NewHandler reached for the compaction field instead of asking the config to check itself, so a check added to launcher.Config.Validate later would have reached every launcher surface except that one. It delegates now, with a test that the handler refuses a config it cannot serve. launcher.Config said nothing about the setting being process-wide, which adkrest.ServerConfig already documents. One launcher serving several applications through its agent loader gives them all the same config and the same summarizer, and so the same model. NewRuntimeAPIController had no signature pin, unlike the two trigger constructors that gained one after the same review. The test that claimed to protect it was an ordinary call expression, which is exactly the form a trailing variadic does not break, so it pinned nothing. It now assigns the constructor to an explicit function type, and re-adding the variadic fails to compile. Also a test on the launcher entry point that is actually reachable. Validate is called from three Execute methods, and two of them cannot be called at all: console.NewLauncher and web.NewLauncher return a launcher.SubLauncher, whose interface has Run and no Execute, and the universal launcher dispatches to Run. That leaves the universal launcher as the only place the check runs, so that is where it is now tested.

…anded The snapshot copied the event struct and the Part struct and left every pointer inside them shared with the store, so a Summarizer that wrote to a member rather than to a field reached stored history regardless. Sixteen fields were aliased; the doc comment claimed two. The compaction record is the one that matters. Tail retention seeds its window with the previous summary and puts the stored record on it, so the pointer is genuinely reachable, and that record decides what every later prompt drops. Writing content into it put an unpaired function call into a real model prompt, past both existing defences: the prose filter inspects what a summarizer returns, and SanitizeSummary re-checks what a plugin substitutes, and neither looks at what the summarizer wrote to its input. A tool call's arguments and a tool response's payload were reachable the same way, which rewrites the stored conversation rather than the prompt. The snapshot is now built field by field from what a summarizer is for, rather than copied and then unpicked. Copying and severing does not hold: a field added to session.Event or genai.Part upstream is silently shared until somebody notices, and this is the second time that has happened. Naming the fields inverts the default, so a new field is absent from the summarizer's view until it is deliberately added. What survives is the conversation: who spoke, when, what was said, and a tool call's name and arguments, because a transcript renders those. The record survives as scalars only, enough to tell a summary apart from a turn and to see what it spanned. The text of a previous summary is still readable, because the seed carries it as ordinary content. Maps inside a tool payload are copied recursively. A shallow clone protects the top level and leaves a nested map shared, which is the same hole one level down.

…e key Three comments claimed a reference that fails to match fails safe, and the direction is inverted. Coverage is the range minus the exclusions, so the exclusion list is what protects an event that was never summarized. A reference that stops matching un-protects its event, which is then covered by a summary that never described it and dropped from every later prompt. That reasoning would hold for a membership list. For an exclusion list it is backwards, and the exclusion design was argued for partly on its strength. The producer had the failure the comments described as impossible. Two events of one invocation can share a timestamp, which the key cannot tell apart, and EventRef's own documentation says so. Window membership was read from that same key, so an event outside the window colliding with one inside it was taken for summarized and no hole was recorded at all, which deletes it. Membership is now decided by identity. The window holds the very pointers the session holds, so this is exact where the key is not. The synthetic seed is the one window element absent from the session and it matches nothing, which is correct: it stands for events rather than being one. The cost is the over-naming case, and that is the one to want. The recorded reference still matches both events of a colliding pair, so the event that was summarized is left raw beside a summary of it. Visible and recoverable, where the deletion was silent. The comments now say which direction is safe, and say plainly that a backend which does not round-trip these timestamps exactly will delete conversation. That is a live question for Vertex AI, whose conformance cases for this are skipped.

One unanswered call among parallel siblings stopped both strategies for the rest of the session, silently. The standard long-running shape is one model turn emitting a call to an ordinary tool alongside one to a long-running tool that never produces a response. The answered sibling's response necessarily sits after both calls, so every resume point the scan would consider had that response in the tail and a call for it still open in the head, and skipBlockedHead refused all of them. Refusing is correct on its own terms: summarizing a response whose call stays raw shows the model a call it has already answered with the answer gone. The resume point that works is the one just after the response. The head then holds the ordinary call and its answer, only the long-running call is still open, and the tail answers nothing. The scan could not reach it because it resumed only after an event that opened an obligation, and a response closes one. It now resumes wherever the set of open obligations changed. Silent because "no window" and "nothing to do yet" are both nil, so nothing logged and nothing failed. Measured end to end, 20 turns after one such event at CompactionInterval 2: 1 compaction record and 41 contents in the prompt before, 20 records and 23 contents after. The existing test for this path asserted only that the response is not summarized, which giving up entirely also satisfies. It gains a sibling that asserts a usable window comes back.

The guard re-read the session and compared it against the state the window was chosen from, which is the right check, done too early. Plugins run between it and the append, that is arbitrary code, and an event landing in that gap sits inside the recorded range while being named by nothing, so every later prompt drops it. The check now runs again immediately before the append. The early one stays, so a summary that is already doomed does not cost a plugin pass. Reaching the window does not need a hostile plugin, or even a concurrent invocation. An event carries the timestamp it was created at rather than the one it was stored at, so parallel tool responses, which are merged and appended when the slowest tool finishes, and sub-agent events funnelled through a channel are routinely created before a range ends and stored after it. On a networked backend the append itself widens the gap to tens of milliseconds. What is left between the second check and the append is not closed. Doing that needs the append to be conditional on a session version, and session.Service has no way to express one. This narrows the window to the smallest the interface allows rather than removing it. The mid-turn path needs no equivalent change: it does not run plugins, so its check and its append are already adjacent. The regression test appends a straggler from inside a plugin and asserts no stored summary covers an event it never summarized. Asserting the text is still in the prompt does not work, because the next window legitimately summarizes it, which is the same observation that makes this failure invisible in production.

A compaction record is not content. It names which stored events every later prompt drops and what stands in for them, so planting one erases real history and substitutes text of the planter's choosing, and that text does not go through the filter a summary's does. One returned event was enough to inject content as prior history, erase the conversation around it, and put an unpaired function call into the next prompt. session.EventActions.Compaction says the framework writes this field, and for tools and callbacks it is enforced in three places: agent.eventActionsFrom, the tool path in base_flow, and workflow.ToolNode all clear it. Plugins were the one hook whose returned event was persisted exactly as given, on all three of the general event paths. The record the framework put on the original is now restored onto whatever comes back, rather than the field being cleared, so an ordinary event carries none and a summary carries the real one under a single rule. The post-invocation summary path is deliberately untouched. A plugin is invited to rewrite a summary there, which a redaction plugin needs, and SanitizeSummary re-checks the result. Also two comment defects found in the same review. The doc comments on resolveCompactionConfig and compactAfterInvocation were each separated from their declaration by an intervening one, so compactAfterInvocation, the most consequential function in the file, rendered with no doc comment at all. And a nil check was justified by a third party calling an exported NewSummaryEvent that does not exist; the check is right, the reason was not.

Compaction runs after the agent has answered and after its events are persisted, so a failure there means only that a later prompt will be larger. REST, the two triggers and A2A all recognise ErrCompaction, log it and carry on. Agent Engine emitted it to the client and broke out of the stream, so a client that had already received its answer was then told the request failed, on both of its streaming methods. Also documents two obligations context compaction puts on session.Service. The shared conformance suite already enforces them, so a third-party implementation goes red on upgrade with nothing on the interface explaining why: an event arriving without an ID must be assigned one in place, and EventActions.Compaction must survive the round trip. A summary carries its content only on that field, with no content and no deltas, so a backend that decides what to persist by looking at content or deltas drops it silently and the same range is summarized and billed again every turn.

A reference is written from an event held in memory and compared against the same event read back, and the two do not always carry the same number of digits. The SQL backend truncates event timestamps to microseconds while the compaction record travels beside them as JSON at full nanosecond precision. The Vertex AI service takes the event timestamp from the server envelope while the reference comes from the client-written payload, with no normalisation anywhere. Comparing exactly then answers no for an event the reference names, and because coverage is the range minus the exclusions, answering no does not leave that event alone. It hands it to a summary that never saw it, and the conversation is gone from every later prompt with something plausible standing where it was. Both sides of the exclusion test are now truncated to microseconds, the coarsest precision any backend here keeps, which makes the comparison independent of who stored what. Only the exclusion test. Widening a hole leaves an extra event raw beside a summary of it, which is visible and recoverable. Widening the range would pull in an event sitting just past the end that was summarized by nothing, which is the deletion this mechanism exists to prevent, so inRange still compares exactly and the test says so. On SQL the bug is currently masked, and only by accident: AppendEvent truncates the caller's event struct in place, so a reference built afterwards from either copy agrees. That is an undocumented mutation of an argument the caller still owns, and removing it as a tidy-up would silently start deleting conversation. This removes the dependency on it. Vertex AI remains unverified against a live project, which is worth doing, but the comparison no longer depends on the answer.

…n bug The compaction record case truncated its timestamps to milliseconds, so it could not tell a backend that keeps the record faithfully from one that rounds it. It now uses nanosecond values, which is the precision a real clock hands over. A second case covers the property that actually loses conversation, which the first one does not reach. A hole names an event by invocation and timestamp, and the record and the events have to keep agreeing about which event that is. A hole that stops matching is read as no hole at all, so the summary covers an event it never saw and that turn disappears from every later prompt. The new case is deliberate about what it catches. A backend that rounds the event and every reference derived from it to one resolution stays consistent and passes, whatever that resolution is, and that is correct rather than a gap: it is why the SQL backend is safe today. What fails is divergence, a backend keeping the two in different precision domains, or rounding the event on read while the record keeps what the client wrote. That is the shape of the open Vertex AI question. Both cases still skip on Vertex AI for want of a recording, so that backend is covered by this only once someone regenerates against a live project.

A summarizer that ignores ctx holds the turn open for as long as it runs, and the caller cannot get out: cancelling its context does not make Run return, because Run is waiting on the summarizer. Post-invocation compaction is driven from a deferred call, so this outlasts even a consumer that has stopped reading events. The framework puts a deadline on the summarizer it installs by default and has no way to put one on a summarizer it is handed, so the obligation belongs on the interface where an implementer will see it. Documented rather than enforced. Wrapping every call in a fixed timeout would also cut short a summarizer that legitimately takes longer than the default, and picking that bound for someone else's model call is not obviously the framework's to make.

The package documentation advised enabling tail retention "either on its own or alongside the sliding window", and the example armed both and called the two triggers independent. Alongside does not work, and the example shipped the configuration that does not bound anything. They fire at different points, tail retention mid-invocation and the sliding window after one completes, but they share a candidate rule. Tail retention summarizes the events no compaction already covers, and the sliding window covers everything it reaches every CompactionInterval invocations, so what is left over never reaches EventRetentionSize and tail retention never fires. It cannot consolidate the summaries instead, because those are compaction events and no strategy re-summarizes one. Measured over 160 turns with a 520-character summary: tail retention alone holds the prompt flat at about 550 characters, the two together grow it linearly to 41,000. Documented rather than fixed, and rather than rejected in Validate. adk-python starves its own token-threshold strategy exactly the same way: driving its selector with a sliding window running every two turns returns an empty window at 10, 20 and 40 turns, and 10, 30 and 70 events with the sliding window off. So this is the shared design, AGENTS.md makes that the source of truth for behaviour, and diverging here would be a decision to take with adk-python rather than alone. Validate keeps accepting the combination because it is well-formed and because rejecting it would break configurations that exist. The example now arms the sliding window and carries the tail-retention pair commented out, with what to delete in order to swap.

baptmont · 2026-08-19T15:20:37Z

Closing. Every fix that was here has been folded into the slice it belongs to, so there is no longer a PR whose job is to correct the ones before it.

This PR existed because three rounds of review landed after the stack was cut, and parking the fixes at the end kept them reviewable as one response. The cost was that #1231 to #1236 each showed code with known defects and a reply pointing four PRs downstream, which is the wrong thing to ask a reviewer to hold in their head.

The stack is now five PRs, re-cut so each one is correct on its own:

PR	content
#1231	session model and the storage contract
#1232	library, engine, telemetry, sliding window, runner
#1234	tail retention and the mid-turn processor
#1235	serving surfaces
#1236	end-to-end test and the example

Two things worth stating plainly.

Nothing changed in the code. The tip of #1236 has the same tree hash as the tip of this PR did, 734b2d03. The re-cut moved code between commits and nothing else, so what merges is exactly what was tested and reviewed here.

Each slice was verified on its own, not just the tip: build, go vet, and the full test suite at every one of the five, with -race -shuffle=on at the end. The only failure at any slice is TestConfigureExporters, a known stale-base artifact that is green on main.

#1233 is closed too. Telemetry stopped being separable once the compactor started calling it directly, so it now lands with the engine in #1232.

The old boundaries no longer fitted the code: the fixes changed the Summarizer signature, extracted the summary builder, moved session/compaction internals to internal/compactioninternal, and wove telemetry through the compactor. The new slices are drawn around what the code actually depends on.

Review threads here remain readable.

baptmont added 30 commits July 31, 2026 13:21

baptmont mentioned this pull request Aug 12, 2026

test(llmagent): drive context compaction against a recorded real model #1236

Open

karolpiotrowicz reviewed Aug 12, 2026

View reviewed changes

baptmont added 5 commits August 13, 2026 08:41

baptmont force-pushed the baptmont/compaction-07-review-fixes branch from f3cd4a4 to 6a4478d Compare August 13, 2026 09:10

baptmont added 2 commits August 13, 2026 09:28

baptmont force-pushed the baptmont/compaction-07-review-fixes branch from 43c752b to 9e75da6 Compare August 13, 2026 10:34

baptmont added 13 commits August 13, 2026 10:43

baptmont requested a review from karolpiotrowicz August 19, 2026 12:56

baptmont force-pushed the baptmont/compaction-06-e2e branch from 06f453c to 4fe3602 Compare August 19, 2026 15:19

baptmont closed this Aug 19, 2026

Navigation Menu

Uh oh!

fix(compaction): address the round-3 review of the compaction stack - #1316

fix(compaction): address the round-3 review of the compaction stack#1316
baptmont wants to merge 62 commits into
baptmont/compaction-06-e2efrom
baptmont/compaction-07-review-fixes

baptmont commented Aug 12, 2026

Uh oh!

karolpiotrowicz left a comment

Uh oh!

baptmont commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

baptmont commented Aug 12, 2026

What is here

Not fixed here

Testing plan

Uh oh!

karolpiotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

baptmont commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants