Repository navigation
fix(memory): keep per-request parse garbage off the session arena (#124 slice 2b) - #176
Merged
Merged
Conversation
… slice 2b) The measured leak was path-specific: slice 2a (PR #162) moved the openai per-SSE-event parse trees to the scratch arena, but the codex/Responses and Anthropic paths still parsed EVERY stream event onto the process-lifetime session arena, and tool-argument JSON + oauth-refresh internals did too. Measured on the codex SSE path (local Responses mock, ReleaseFast, 30 turns): session arena +28 KB/turn monotonic before, +0.0 after (flat at 409 KB); RSS slope +8.8 -> +3.6 KB/turn (residual is history-scale, not leak). - parseResponses: events parse on the scratch arena; only what escapes the request is detached onto the session arena (output items via dupeJsonValue — stepResponses appends them to history verbatim — plus usage/id/error). Covers both codex transports (the WS path funnels frames into the same function as synthetic SSE lines). - assembleAnthropic: full assembly (event trees, delta accumulators, stitched message) on scratch; ONE deep copy of the finished message onto the session arena at return replaces the old whole-stream-on-arena scheme. - stepResponses/stepOpenAI tool-argument parses -> scratch (execution-only; the history message carries the arguments string, not the tree). KB-scale per file-edit tool call. - oauth.refreshOAuthKey call sites: internals (file read + JSON parse, ~1-2KB EVERY request on kimi/xai login sessions) -> scratch; the token is duped to survive. The per-request scratch reset moves to the top of request() so the refresh can use it. - util.dupeJsonValue: deep-copy helper + reset-and-clobber test. - GRAFF_MEM_DEBUG=1: per-turn 'mem' event (--json) reporting session/scratch arena capacity — allocator-level leak telemetry; RSS sampling alone cannot tell an arena leak from gpa noise (and a Debug-allocator build inflates RSS slope ~50x, which is what made earlier numbers look scarier). - benchmarks/README: document memory_session.py and the leak-vs-context distinction. Co-Authored-By: blackfloofie <265516171+blackfloofie@users.noreply.github.com>
4 tasks
…scratch The forced-refresh fallback stored the scratch-backed token slice when the session-arena dupe failed — a would-be UAF at the next request's scratch reset. Keep the previous key instead (the retry then fails cleanly), same policy as the near-expiry refresh site. Co-Authored-By: blackfloofie <265516171+blackfloofie@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What the fresh measurement showed
Re-measuring #124 at v0.0.200 with
benchmarks/memory_session.pyagainst local mocks (and a new allocator-level probe) rewrote the story:parseResponses) and the Anthropic path (assembleAnthropic) still parsed every SSE event of every stream onto the process-lifetime session arena. Measured on a codex SSE mock (ReleaseFast, 30 turns): session arena +28 KB/turn, monotonic — matching the +36-45 KB/turn from the original report.The fix
parseResponses: events parse on the per-request scratch arena; only what escapes the request is detached onto the session arena — output items via a newutil.dupeJsonValuedeep-copy (stepResponsesappends them to history verbatim), plus usage/id/error scalars. Covers both codex transports (WS frames funnel into the same function).assembleAnthropic: the whole assembly (event trees, delta accumulators, stitched message) lives on scratch; ONE deep copy of the finished message lands on the session arena at return.stepResponses/stepOpenAI) → scratch: execution-only, KB-scale per file-edit call; the history message carries the arguments string, not the tree.oauth.refreshOAuthKeyinternals (~1-2 KB every request on kimi/xai login sessions) → scratch; the token is duped to survive. The scratch reset moves to the top ofrequest()to allow this.GRAFF_MEM_DEBUG=1: per-turnmemevent (--json) reporting session/scratch arena capacity — RSS sampling alone can't tell an arena leak from gpa noise.benchmarks/README.md: documentsmemory_session.py(issue item 3).Measured A/B (ReleaseFast, 30 turns, local mocks)
Residual ~2-3 KB/turn RSS is history-scale (a conversation must grow), not leak.
Every moved site passed an adversarial lifetime review (multi-agent audit of the request path): the two real aliasing hazards found — Responses output items and the assembled Anthropic message both escaping into history — are exactly what the deep-copies detach.
util.dupeJsonValueships with a reset-and-clobber test;zig build+zig build testclean.Remaining for a later slice (small, catalogued): the
--jsonrequest-line parse tree (~1 KB/turn on the session arena), per-turn autosave/title allocPrints (~100 B/turn), eval-loop allocations (eval sessions only), and the REPL render churn (issue item 2).Part of #124.