fix(acp): bound the tool-call payload a session snapshot carries - #712
Open
Adam-Dalloul wants to merge 1 commit into
Open
fix(acp): bound the tool-call payload a session snapshot carries#712Adam-Dalloul wants to merge 1 commit into
Adam-Dalloul wants to merge 1 commit into
Conversation
`active_tool_calls` is cleared in exactly one place, `TurnComplete`, so a turn that keeps working accumulates an entry for every tool call it has ever made, and `to_snapshot` copied all of them whole. Issue xintaofei#380 sampled a turn that had not reached a `TurnComplete`: 1814 entries / 23.6 MB, then 1983 entries / 26.7 MB. Each entry holds up to the 64 KiB per-event output cap plus the agent's rendered content and any base64 image data, so the payload the attach path serves had no bound, and the desktop client that parses it on every attach stopped responding. Nothing could simply drop the finished entries: `denormalizeSnapshot` resolves each `ToolCallRef` in the live message through this list and skips a block whose id is missing, so a removed entry is a tool card missing from the middle of the in-flight turn. Every call still ships, in the same order, with its id, kind, label, status and meta. What is now bounded is the result payload (input / output / content / locations / images), and only on calls that already reached a terminal status: the newest keep everything until a 2 MiB budget is spent, older ones ship without it, and their results stay durable in the agent's transcript. Pending and in-progress calls are never trimmed at any size, since their partial output exists nowhere else and their count tracks live concurrency rather than turn length. Sizing reuses the escape-aware, allocation-free estimator the per-event cap already uses, so a payload means the same number of bytes on both paths. Measured on the reported shape (one turn, prose interleaved, ~12 KB of output and rendered content per call): 1983 calls went from 24.06 MiB to 2.62 MiB, 4000 calls from 48.53 MiB to 3.24 MiB, with the newest 170 calls keeping their full payload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the unbounded part of #380.
active_tool_callsis cleared in exactly one place,TurnComplete. A turn that keeps working therefore accumulates an entry for every tool call it has ever made, andto_snapshotcopied all of them whole. The reporter sampled a turn that had not reached aTurnComplete: 1814 entries / 23.6 MB, then 1983 entries (1964 completed, 18 failed, 1 running) / 26.7 MB, with the server answering in 0.134s and the desktop client unable to send, stop, or switch sessions afterwards.Nothing could simply drop the finished entries.
denormalizeSnapshotresolves eachToolCallRefin the live message through this list and skips a block whose id is missing, so a removed entry is a tool card missing from the middle of the in-flight turn.So every call still ships, in the same order, with its id, kind, label, status and meta. What is now bounded is the result payload (
input/output/content/locations/images), and only on calls that already reached a terminal status: the newest keep everything until a 2 MiB budget is spent, and older ones ship without it. A finished call's result stays durable in the agent's own transcript. Pending and in-progress calls are never trimmed at any size, since their partial output exists nowhere else and their count tracks live concurrency, not turn length.Sizing reuses the escape-aware, allocation-free estimator
event_streamalready uses for the per-event cap, split so it can take a plain slice.Measurement, on the reported shape (one long turn, prose interleaved between the calls, ~12 KB of output plus rendered content per call), serializing the same state before and after:
Before is linear and unbounded in the number of calls. After, the newest 170 keep their full payload and what remains grows only with the per-call identity fields the refs need.
Unchanged: the in-memory
active_tool_callsmap (latest_live_reply, the delegation meta writer and the launch-id probe all still read it exactly as before), the snapshot's id-sorted order, every other snapshot field, and the wire shape of any turn whose tool calls fit the budget, which is byte-identical to today.Not covered here: rendering 1983 tool cards is its own cost, and this PR does not touch the render path.
Three tests in
session_state.rs: a normal turn ships byte-identical, a 300-call turn stays near the budget with every ref still resolving and the oldest calls keeping their identity fields, and a running call larger than the whole budget still ships whole.Does not touch anything in #705, #708 or #703. #703 edits
session_state.rstoo, but onlybackground_keepalive_max_age.