Skip to content

perf(memory): stop pinning request-sized buffers for the stream lifetime#489

Merged
SantiagoDePolonia merged 1 commit into
mainfrom
perf/stream-memory-footprint
Jul 6, 2026
Merged

perf(memory): stop pinning request-sized buffers for the stream lifetime#489
SantiagoDePolonia merged 1 commit into
mainfrom
perf/stream-memory-footprint

Conversation

@SantiagoDePolonia

@SantiagoDePolonia SantiagoDePolonia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Problem

Follow-up to #488. Large streamed requests (coding-agent workloads) keep roughly 4-6x their body size live until stream close, which sets the RSS high-water mark that Go's GC then holds. This PR removes the per-stream pins that were cheap to fix without architectural change — four verified pin points from the memory investigation.

Changes

  • RouteHints (internal/core/semantic.go): gjson result strings alias the full parsed body, and the two short selector strings stored on the request context pinned a request-sized backing string for the whole (possibly streaming) request. strings.Clone unpins it.
  • ExtraFields (internal/core/json_fields.go): the unknown-fields buffer was pre-sized to len(body), so one small unknown field retained a body-sized array per decoded object — request, message, tool call (13 call sites share the helper). The buffer now starts at 256B and re-copies only when it over-grew (>1KB slack), bounding retained capacity near the extras' own length. New regression test asserts a 1MB body with tiny extras retains <4KB.
  • llmclient.DoStream (internal/llmclient/client.go): GetBody closes over the fully marshaled request payload and stays reachable through resp.Request while the stream is open. It is dropped once the response arrives.
  • Anthropic chat stream (internal/providers/anthropic/chat_stream.go): the converter accumulated every tool-call argument delta into a strings.Builder nothing on the chat path reads (the Responses converter uses its own ResponsesOutputToolCallState, which legitimately accumulates). For tool calls carrying file contents this doubled tool-payload memory per stream. Deleted.

Design notes for review

  • Why GetBody is cleared after Do, not before: nil-ing it pre-send would disable net/http's transparent retry of requests that fail on a stale pooled connection — a reliability regression for streaming, since DoStream never retries at our layer. Redirects and transport retries only consult GetBody inside Do, so post-response clearing is a pure reference release with no behavior change. resp.Request.Body is deliberately left untouched (the h1 write loop can still reference it on early-response paths — clearing it would race). Known limitation: on HTTP/2 the transport's stream state can still hold the drained request reader until stream end; that part is Go internals.
  • Perf-guard baselines (tests/perf/hotpath_test.go): the strings.Clone adds +1 small alloc per request on the chat hot paths. Main already measured at cap−1 from drift, so the two alloc ceilings move up by 2 with a comment documenting the intentional trade. Byte ceilings unchanged.

Measured impact

  • BenchmarkExtractUnknownJSONFieldsObject_Chat: 1792 → 1152 B/op, allocs unchanged (2), time flat.
  • BenchmarkExtractUnknownJSONFieldsObject_Responses: 896 → 704 B/op, allocs unchanged.
  • Hot-path guard: +1 alloc, ~+50 B/op on the chat paths (the Clone); all thresholds pass.
  • Retention per active stream drops by roughly one full marshaled request copy (GetBody), one full-body string (RouteHints), body-sized arrays per decoded object with unknown fields (ExtraFields), and the accumulated tool-call payload (Anthropic chat).

Verification

Full suite passes (61 packages); pre-commit race tests, lint, and the hot-path perf guard all pass. Anthropic stream converter behavior is covered by the existing chat-stream tests (argument deltas are relayed verbatim; the deferred-placeholder logic at chat_stream.go:292 is preserved).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Reduced memory usage when handling requests with extra JSON fields, helping prevent large request bodies from being kept in memory longer than needed.
    • Improved streaming request handling so request payload data is released earlier during long-lived streams.
    • Fixed tool-call streaming behavior to avoid unnecessary accumulation of intermediate argument text.
  • Performance

    • Lowered memory retention in routing and selector handling, which can improve stability under heavier traffic.
  • Tests

    • Added regression coverage for memory retention behavior.

Large streamed requests kept ~4-6x their body size live until stream
close. This trims the per-stream footprint at four verified pin points:

- RouteHints: gjson result strings alias the full parsed body; the two
  short selector strings stored on the request context pinned a
  request-sized backing string. strings.Clone unpins it (+1 tiny alloc
  per request, reflected in the perf-guard baselines).

- ExtraFields: the unknown-fields buffer was pre-sized to the full body,
  so one small unknown field retained a body-sized array per decoded
  object (request, message, tool call, ...). The buffer now starts at
  256B and re-copies only when it over-grew, bounding retained capacity
  near the extras' own length without adding allocations on the hot path.

- llmclient.DoStream: GetBody closes over the fully marshaled request
  payload and stays reachable through resp.Request while the stream is
  open. Redirects and transparent transport retries only consult GetBody
  inside Do, so it is dropped once the response arrives - a pure
  reference release with no behavior change.

- Anthropic chat stream: the converter accumulated every tool-call
  argument delta into a strings.Builder that nothing on the chat path
  ever reads (the Responses converter uses its own state). For coding
  agents whose tool calls carry file contents this doubled tool-payload
  memory per stream. Deleted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 46b20b77-c942-497c-9628-638debae1933

📥 Commits

Reviewing files that changed from the base of the PR and between a069a2c and a68f59b.

📒 Files selected for processing (6)
  • internal/core/json_fields.go
  • internal/core/json_fields_test.go
  • internal/core/semantic.go
  • internal/llmclient/client.go
  • internal/providers/anthropic/chat_stream.go
  • tests/perf/hotpath_test.go

📝 Walkthrough

Walkthrough

Changes reduce retained backing memory across the gateway hot path: unknown JSON field extraction shrinks buffer growth and conditionally clones raw bytes, selector hint strings are cloned to avoid body aliasing, streaming client drops request GetBody closures, Anthropic tool-call state removes an argument accumulator, and perf guard allocation thresholds are adjusted upward.

Changes

Memory Retention Reduction

Layer / File(s) Summary
Unknown JSON field buffer sizing
internal/core/json_fields.go, internal/core/json_fields_test.go
extractUnknownJSONFields grows a buffer capped at 256 bytes and clones raw bytes only when capacity significantly exceeds length; a new regression test verifies retained capacity stays small for large bodies.
Selector hint string cloning
internal/core/semantic.go
deriveSnapshotSelectorHintsGJSON clones extracted model/provider strings instead of aliasing gjson result strings.
Streaming client payload release
internal/llmclient/client.go
DoStream clears resp.Request.GetBody after establishing the stream to release the marshaled request payload.
Anthropic tool-call state simplification
internal/providers/anthropic/chat_stream.go
streamToolCallState removes the accumulated Arguments builder; placeholder detection and transition logic no longer buffer/append argument text.
Perf guard threshold updates
tests/perf/hotpath_test.go
maxAllocs ceilings for two hot-path benchmarks are raised (110→112, 128→130) with updated baseline comments.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • ENTERPILOT/GoModel#163: Both PRs modify extractUnknownJSONFields/UnknownJSONFields raw storage behavior in internal/core/json_fields.go.
  • ENTERPILOT/GoModel#201: Both PRs change extractUnknownJSONFields behavior and extend internal/core/json_fields_test.go around unknown JSON field retention.

Poem

A buffer once bloated, now trim and neat,
Cloned strings no longer to bodies compete,
Streams let go of closures they held too tight,
Tool calls drop builders, keeping state light,
Hop hop, less memory pinned tonight! 🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main memory-retention fix for streamed requests and matches the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/stream-memory-footprint

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/core/json_fields.go 66.66% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

All four changes are pure reference-release or copy-on-write adjustments with no behavioral changes on the happy path; the regression test and benchmark guard document both the fix and the small allocation trade-off.

The GetBody nil-ing is correctly placed after Do returns (transport retries are finished), the nil-guard is present, and callers only receive resp.Body so the cleared field is unreachable. The gjson Clone is a textbook unpin of an aliased substring. The bytes.Buffer change is conservative: the 256 B floor and 1 KB slack threshold keep retained capacity tightly bounded, and the new regression test directly asserts the cap invariant. The Anthropic Arguments removal is safe because the field was write-only on the chat path—its value was never consulted before or after the delta was emitted.

No files require special attention; the hotpath perf guard in tests/perf/hotpath_test.go explicitly documents the +2 alloc ceiling adjustments, and the new regression test in json_fields_test.go directly verifies the capacity bound.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Client
    participant DoStream
    participant Transport
    participant Stream

    Client->>DoStream: DoStream(ctx, req)
    DoStream->>Transport: buildRequest sets GetBody closing over bodyBytes
    Transport-->>DoStream: http.Response with resp.Request.GetBody holding bodyBytes
    Note over DoStream: resp.Request.GetBody = nil releases marshaled payload ref
    DoStream-->>Client: resp.Body as io.ReadCloser
    Note over Client,Stream: Stream open for minutes, bodyBytes now GC-eligible
    Client->>Stream: Read SSE events
    Stream-->>Client: tool_call arg deltas relayed verbatim, no accumulation
    Client->>Stream: Close()
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Client
    participant DoStream
    participant Transport
    participant Stream

    Client->>DoStream: DoStream(ctx, req)
    DoStream->>Transport: buildRequest sets GetBody closing over bodyBytes
    Transport-->>DoStream: http.Response with resp.Request.GetBody holding bodyBytes
    Note over DoStream: resp.Request.GetBody = nil releases marshaled payload ref
    DoStream-->>Client: resp.Body as io.ReadCloser
    Note over Client,Stream: Stream open for minutes, bodyBytes now GC-eligible
    Client->>Stream: Read SSE events
    Stream-->>Client: tool_call arg deltas relayed verbatim, no accumulation
    Client->>Stream: Close()
Loading

Reviews (1): Last reviewed commit: "perf(memory): stop pinning request-sized..." | Re-trigger Greptile

@SantiagoDePolonia SantiagoDePolonia merged commit 92db958 into main Jul 6, 2026
21 checks passed
SantiagoDePolonia added a commit that referenced this pull request Jul 7, 2026
…r reads (#497)

* fix(lifecycle): close leaked goroutines, sessions, and unbounded error reads

Four lifecycle leaks from the memory investigation (follow-up to #488/#489):

- llmclient now closes RawBodyReader on every error return that precedes
  the HTTP transport (circuit breaker open, request build failure),
  mirroring net/http's Do contract. Pipe-backed uploads (files, audio
  transcription) previously leaked their producer goroutine and the full
  upload buffer forever when the breaker rejected the request — exactly
  the agent retry-storm-during-outage pattern.

- The audit middleware publishes the terminal live event when a handler
  panic unwinds through it (then re-panics for the outer recover).
  Previously the panic skipped all terminal events, permanently orphaning
  the entry in the live-dashboard active-snapshot state.

- The realtime websocket relay gains a heartbeat (30s ping, 10s timeout,
  both peers). A silently dead peer — NAT timeout, power loss, no RST —
  left both copy loops blocked in Read forever, leaking two goroutines
  and both connections per session. A ping that fails because the
  connection is already locally closed defers to the copy loop's close
  cause instead of racing to report a secondary error.

- Upstream error bodies are read through a 64KB LimitReader (matching
  core's audit capture cap) in DoStream's non-200 path and doRequest's
  error-status path, so a misbehaving provider answering errors with an
  endless body is no longer buffered whole. Successful response reads
  are unchanged.

Also fixes TestProxyRelaysLargeFrame leaking its hijacked session past
the test boundary (Accept hijacks the connection, so httptest's Close
does not wait for the relay goroutines).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(realtime): give each heartbeat ping its own timeout budget

Review follow-ups on #497: the shared pingBoth deadline let a
slow-but-alive client ping leave the upstream ping a nearly expired
budget, spuriously blaming a healthy upstream. Each peer now gets its
own full heartbeatTimeout. Also widen the responsive-session test's
pong timeout (150ms -> 2s) and loop window so scheduler stalls on
loaded CI runners cannot fail a healthy session.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants