Repository navigation
Fix: Clone the model name off gjson's body copy - #1604
Conversation
gjson.ParseBytes parses a copy of the whole body, and Result.String() returns a substring of that copy. requestModel handed that substring to its callers, so every place a model name lives longer than the request — RequestedModel in the inference extension, the model_rewritten invocation details — pinned the entire request body. One full body copy per inference request, invisible to the interner: measured at 95MB (59% of a 160MB in-use heap) on a live laptop proxy, via inferencerouter.chooseModel -> SetRequestModel -> requestModel. strings.Clone at the single return covers every caller at once. Pinned by TestRequestModelDoesNotPinTheBody: 10 names kept from 4MiB bodies pinned exactly 40MiB before the fix. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
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. Comment |
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
Correct, minimal fix. gjson.ParseBytes does Parse(string(json)), so every Result.String() from it is a substring of a full body copy; cloning at requestModel's single return covers RequestModel, SetRequestModel and the inference router at once. I checked the rest of core/ and this is the only ParseBytes/Parse call site in non-test code. The GetBytes callers (limits, strip, toolprune, snapshot) are unaffected because GetBytes already copies its result strings off the input.
Verified locally (go1.26.10): go test ./pipeline/ passes, go vet and gofmt are clean, and with the clone reverted the new regression test fails with heap = 40MiB after keeping 10 model names from 4MiB bodies. So the test really does pin the property.
One optional nit inline about making the heap assertion delta-based.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (core/pipeline), tests, security
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing (a few Go CI jobs still pending at review time; none failing)
| runtime.ReadMemStats(&ms) | ||
| // 10 bodies x 4MiB = 40MiB pinned under the bug; live heap must stay well | ||
| // under that even with noise from the rest of the package's tests. | ||
| if got := ms.HeapAlloc; got > 20<<20 { |
There was a problem hiding this comment.
nit: HeapAlloc is the whole package's live heap, so this threshold depends on whatever earlier tests in pipeline leave reachable. It's fine today (0 t.Parallel() in the package, and the bug case shows a clean 40MiB against a 20MiB bar), but taking a ReadMemStats baseline after a runtime.GC() before the loop and asserting on the delta would keep the test independent of package growth. Optional.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
Two gaps the pprof pass on a live proxy caught, both still open after the memory fixes in #1604 and #1606: requestModel copied every routed request body. gjson.ParseBytes is Parse(string(body)), so reading the top-level keys for "model" made one full copy of the body per request — ~1GB/hour of churn on a proxy that was mostly idle, third behind tool-prune's copies. #1604 stopped the copy being pinned; this stops it being made. The scan now runs over unsafe.String aliasing the body: safe because gjson never writes what it parses, the body is not written concurrently on the request path, and the only string that escapes the scan is the name, which #1604 already clones. TestRequestModelDoesNotCopyTheBody pins the property: one call on a 4MiB body allocates under 64KiB, where the old code allocated the 4MiB. /debug/pprof/block and /mutex were permanently empty. The handlers from #1603 exist, but the runtime records contention only when asked, and nothing ever called SetBlockProfileRate or SetMutexProfileFraction — the two endpoints answered 200 with zero samples, on the box and on every deployment with loopback-only listeners. WithPprof now turns the recorders on with the handlers, under the same loopback gate. Both cost nothing unless the event they measure happens. TestStatServerPprofEnablesContentionProfiles reads the rates back process-globally: both were left at zero before. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
What
requestModelreturned gjson'sResult.String()directly.gjson.ParseBytesparses a copy of the whole body, andString()on a string node is a substring of that copy — so the ~15-byte model name handed to callers pinned the entire request body.Every place a model name outlives the request retains that copy:
ext.RequestedModelin the inference extension (lives in the session event)details["from"]in themodel_rewritteninvocation recordThe inference-router calls
RequestModel()on every inference request, so this is one full body copy per request, invisible to the interner (it dedupes by content, but the first insert still carries the big backing array).Evidence
A live laptop proxy (7 min uptime, 160MB in-use heap) pinned 95MB (59%) in
gjson.ParseBytes, retained via:The regression test pins the property: 10 names kept from 4MiB bodies held exactly 40MiB before the fix, ~0 after (GC'd).
Fix
strings.CloneatrequestModel's single return — every caller (RequestModel,SetRequestModel, the router) is covered at once.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com