streaming memory improvements - #4678
Merged
Merged
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds atomic stream-close ownership claims, forced accumulator cleanup backstops, response chunk builder refactoring, related tests, and a formatting-only UI update. ChangesStreaming cleanup and response assembly
UI formatting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
akshaydeo
marked this pull request as ready for review
June 24, 2026 18:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/utils/utils.go (1)
2425-2433: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRe-panic unexpected reader panics.
Line 2427 now recovers every panic, but only assigns an error for idle-timeout/closed cases. Any unrelated panic from
r.reader.Readis silently converted into(0, nil), which can hide corruption or spin callers.Restore the previous fail-fast behavior for unexpected panics
func (r *idleTimeoutReader) Read(p []byte) (n int, err error) { defer func() { if recovered := recover(); recovered != nil { if r.fired.Load() || r.connectionClosed() { n = 0 err = r.closedReadError() return } + panic(recovered) } }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/utils/utils.go` around lines 2425 - 2433, The idleTimeoutReader.Read recovery logic is swallowing unrelated panics from r.reader.Read and turning them into a successful read, so restore fail-fast behavior for non-idle/closed cases. Update the defer/recover handling in idleTimeoutReader.Read so it only converts the panic to closedReadError when r.fired.Load() or r.connectionClosed() is true, and otherwise re-panic the recovered value to preserve unexpected failures.framework/streaming/responses.go (1)
624-652: 🎯 Functional Correctness | 🟠 MajorDefault
ContentIndexbefore dropping deltascore/providers/bedrock/bedrock.goemitsResponsesStreamResponseTypeOutputTextDeltawithoutContentIndex, so this branch can discard real text and leave a placeholder empty message. Fall back to0whenContentIndexis nil, or defer creating the empty message until a delta is actually applied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/streaming/responses.go` around lines 624 - 652, The OutputTextDelta and RefusalDelta handling in responses.go drops real streamed content when resp.ContentIndex is nil, because it creates an empty message but never applies the delta; update the delta accumulation path in the ResponsesStreamResponseTypeOutputTextDelta and ResponsesStreamResponseTypeRefusalDelta cases to default a missing ContentIndex to 0, or delay createNewMessage until a delta is actually appended, so builderFor/ensureContentBlock still writes into the intended block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/utils/idle_timeout_reader_test.go`:
- Around line 638-646: The race test in
NewIdleTimeoutReader/SetupStreamCancellation is nondeterministic because it
relies on a fixed 3ms sleep before stop() and cleanup(), which can miss both
close paths on slow CI. Replace the sleep with a bounded wait that polls or
waits until closeCount becomes greater than 0 before performing final cleanup
and assertions, using the existing cancel(), stop(), cleanup(), and closeCount
state to keep the test deterministic.
In `@core/schemas/tracer.go`:
- Around line 116-120: The new ForceCleanupStreamAccumulator method should not
be added to the public schemas.Tracer interface because it breaks custom
implementations. Move this behavior into a separate optional secondary interface
near Tracer, keep Tracer unchanged, and update the call site to type-assert for
the optional interface before invoking ForceCleanupStreamAccumulator. Use the
existing Tracer and ForceCleanupStreamAccumulator symbols to locate and refactor
the affected contract.
In `@framework/streaming/responses_test.go`:
- Around line 13-15: `testResponsesAccumulator` creates an `Accumulator` that
starts background work, but callers are not consistently cleaning it up. Change
the helper to accept `testing.TB`, create the accumulator as before, and
register `Cleanup()` on the passed test handle so every test/benchmark using
`testResponsesAccumulator` is covered. Update the existing callers in
`responses_test.go` to pass their `*testing.T`/`*testing.B` values.
---
Outside diff comments:
In `@core/providers/utils/utils.go`:
- Around line 2425-2433: The idleTimeoutReader.Read recovery logic is swallowing
unrelated panics from r.reader.Read and turning them into a successful read, so
restore fail-fast behavior for non-idle/closed cases. Update the defer/recover
handling in idleTimeoutReader.Read so it only converts the panic to
closedReadError when r.fired.Load() or r.connectionClosed() is true, and
otherwise re-panic the recovered value to preserve unexpected failures.
In `@framework/streaming/responses.go`:
- Around line 624-652: The OutputTextDelta and RefusalDelta handling in
responses.go drops real streamed content when resp.ContentIndex is nil, because
it creates an empty message but never applies the delta; update the delta
accumulation path in the ResponsesStreamResponseTypeOutputTextDelta and
ResponsesStreamResponseTypeRefusalDelta cases to default a missing ContentIndex
to 0, or delay createNewMessage until a delta is actually appended, so
builderFor/ensureContentBlock still writes into the intended block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e13613e-a798-48f4-8387-dbd6574e198e
📒 Files selected for processing (9)
core/bifrost.gocore/providers/utils/idle_timeout_reader_test.gocore/providers/utils/utils.gocore/schemas/tracer.goframework/streaming/accumulator.goframework/streaming/responses.goframework/streaming/responses_test.goframework/tracing/tracer.goui/app/clientLayout.tsx
Contributor
Confidence Score: 4/5Safe to merge pending the pre-existing open comment about silent panic swallowing in idleTimeoutReader.Read. The CAS-based close ownership, force-reap backstop, and strings.Builder refactor are all correct and well-tested. The atomic-claim fix is mechanically sound — replacing racy read-then-write with a single locked compare-and-swap in three coordinated sites, backed by targeted race-detector tests. The prior review's concern about the removed panic(recovered) fallback in Read() — where unrelated panics are now silently converted to (0, nil) returns, producing a busy-loop instead of a visible crash — remains unaddressed in this revision. core/providers/utils/utils.go — the Read() recover block still has no fallback for panics unrelated to idle-timeout or connection-closed state. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CG as Cancellation Goroutine
participant IT as Idle-Timeout Timer
participant RS as ReleaseStreamingResponse
participant CAS as GetAndSetValue (CAS)
participant FS as fasthttp requestStream
note over CG,FS: All three racers compete for close ownership
CG->>CAS: GetAndSetValue(ConnectionClosed, true)
CAS-->>CG: "prev=false, wins claim"
IT->>CAS: GetAndSetValue(ConnectionClosed, true)
CAS-->>IT: "prev=true, skips"
RS->>CAS: GetAndSetValue(ConnectionClosed, true)
CAS-->>RS: "prev=true, skips"
CG->>FS: CloseWithError(ctx.Err()) [exactly once]
note over CG,FS: After stream ends
participant MC as TracingMiddleware TraceCompleter
MC->>MC: CompleteAndFlushTrace(traceID)
MC->>MC: ForceCleanupStreamAccumulator(traceID)
%%{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 CG as Cancellation Goroutine
participant IT as Idle-Timeout Timer
participant RS as ReleaseStreamingResponse
participant CAS as GetAndSetValue (CAS)
participant FS as fasthttp requestStream
note over CG,FS: All three racers compete for close ownership
CG->>CAS: GetAndSetValue(ConnectionClosed, true)
CAS-->>CG: "prev=false, wins claim"
IT->>CAS: GetAndSetValue(ConnectionClosed, true)
CAS-->>IT: "prev=true, skips"
RS->>CAS: GetAndSetValue(ConnectionClosed, true)
CAS-->>RS: "prev=true, skips"
CG->>FS: CloseWithError(ctx.Err()) [exactly once]
note over CG,FS: After stream ends
participant MC as TracingMiddleware TraceCompleter
MC->>MC: CompleteAndFlushTrace(traceID)
MC->>MC: ForceCleanupStreamAccumulator(traceID)
Reviews (4): Last reviewed commit: "streaming memory improvements" | Re-trigger Greptile |
akshaydeo
force-pushed
the
06-24-streaming_memory_improvements
branch
from
June 24, 2026 18:27
5dc6090 to
4304def
Compare
akshaydeo
force-pushed
the
06-24-streaming_memory_improvements
branch
from
June 24, 2026 19:53
4304def to
3fe2b41
Compare
akshaydeo
force-pushed
the
06-24-streaming_memory_improvements
branch
from
June 24, 2026 21:45
3fe2b41 to
0d3ebf4
Compare
Contributor
Author
Merge activity
|
This was referenced Jun 26, 2026
Merged
18 tasks
1 task
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
## Summary This PR fixes two classes of bugs: a race condition that caused fasthttp's pooled `requestStream` to be double-released (producing `slice bounds out of range` panics on concurrent requests), and a stream accumulator memory leak where accumulators for aborted or non-cleanly-terminated streams were not freed until the TTL sweep. It also replaces O(n²) string concatenation in the Responses API stream builder with a `strings.Builder`-based accumulator pattern. ## Changes - **Atomic stream close ownership (`utils.go`):** Replaced the racy `Value`-then-`SetValue` guard on `BifrostContextKeyConnectionClosed` with a single `GetAndSetValue` compare-and-swap in `SetupStreamCancellation`, `NewIdleTimeoutReader`, and `ReleaseStreamingResponse`. Previously, the cancellation goroutine, the idle-timeout timer, and `ReleaseStreamingResponse` could each observe `false` and all proceed to call `CloseWithError`, double-releasing the pooled fasthttp `requestStream` and corrupting a concurrent request's slice bounds. Now exactly one owner wins the claim and performs the close. - **Removed re-panic in idle timeout reader (`utils.go`):** Dropped the unconditional `panic(recovered)` in `idleTimeoutReader.Read`'s recover block, which was re-panicking on legitimate post-close reads after the atomic-claim fix moved `r.fired.Store(true)` to the winning-owner path only. - **Force-reap stream accumulator backstop (`accumulator.go`, `tracer.go`, `bifrost.go`):** Added `ForceCleanupStreamAccumulator` to the `Tracer` interface, `NoOpTracer`, `Accumulator`, and `Tracer` implementations. The stream's terminal lifecycle hook (the provider goroutine's `postHookSpanFinalizer`) now calls this after releasing the plugin pipeline, guaranteeing that accumulators for streams that end without a clean terminal chunk (client abort, broken SSE write, multi-plugin refcount imbalance) are freed immediately rather than waiting for the TTL sweep. The trace ID is captured as an immutable string before the pooled request object is recycled. - **O(n²) → O(n) Responses stream builder (`responses.go`):** Replaced per-delta `*field += delta` string concatenation in `buildCompleteMessageFromResponsesStreamChunks` with `strings.Builder` accumulators keyed by message and content-block index. Builders are materialized into final strings once after the chunk walk. The five helper methods (`appendTextDeltaToResponsesMessage`, `appendRefusalDeltaToResponsesMessage`, `appendFunctionArgumentsDeltaToResponsesMessage`, `appendReasoningDeltaToResponsesMessage`, `appendReasoningSignatureToResponsesMessage`) are removed and their logic is inlined into the walk + materialization pass. - **Tests (`idle_timeout_reader_test.go`, `responses_test.go`):** Added `TestStreamClose_ExactlyOnceAcrossOwners` (races the timer and cancellation goroutine 100 times, asserts `CloseWithError` fires exactly once), `TestConnectionClosedClaim_SerializesOwners` (64 concurrent goroutines, asserts exactly one wins), `TestReleaseStreamingResponse_SkipsWhenAlreadyClaimed`, `TestBuildResponsesMessageConcatenatesTextDeltas`, `TestBuildResponsesMessageRoutesParallelToolArgs`, `TestBuildResponsesMessageAccumulatesReasoningSummary`, `TestForceCleanupStreamAccumulatorReapsRegardlessOfRefcount`, and `BenchmarkBuildResponsesMessageTextDeltas`. - **UI formatting (`clientLayout.tsx`):** Reformatted from 2-space to tab indentation and collapsed some multi-line imports; no behavioral changes. ## Type of change - [x] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Run with race detector to exercise the atomic-claim fix go test -race ./core/providers/utils/... -run TestStreamClose_ExactlyOnceAcrossOwners -count=10 go test -race ./core/providers/utils/... -run TestConnectionClosedClaim_SerializesOwners go test -race ./core/providers/utils/... -run TestReleaseStreamingResponse_SkipsWhenAlreadyClaimed # Force-reap accumulator backstop go test ./framework/streaming/... -run TestForceCleanupStreamAccumulatorReapsRegardlessOfRefcount -v # Responses stream builder correctness and performance go test ./framework/streaming/... -run TestBuildResponsesMessage go test -bench=BenchmarkBuildResponsesMessageTextDeltas -benchmem ./framework/streaming/... # Full suite go test ./... # UI cd ui pnpm i pnpm build ``` ## Breaking changes - [x] No The `Tracer` interface gains `ForceCleanupStreamAccumulator`. Any external implementations of `Tracer` must add this method. The `NoOpTracer` implementation is provided as a reference. ## Related issues Fixes the `slice bounds out of range [:-680]` crash caused by double-release of fasthttp's pooled `requestStream`. Fixes stream accumulator leaks on client-aborted or non-cleanly-terminated streams. ## Security considerations No auth, secrets, or PII implications. The fix closes a memory-corruption vector (double-pool-release) that could cause one request to alias another's in-flight data. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
## Summary This PR fixes two classes of bugs: a race condition that caused fasthttp's pooled `requestStream` to be double-released (producing `slice bounds out of range` panics on concurrent requests), and a stream accumulator memory leak where accumulators for aborted or non-cleanly-terminated streams were not freed until the TTL sweep. It also replaces O(n²) string concatenation in the Responses API stream builder with a `strings.Builder`-based accumulator pattern. ## Changes - **Atomic stream close ownership (`utils.go`):** Replaced the racy `Value`-then-`SetValue` guard on `BifrostContextKeyConnectionClosed` with a single `GetAndSetValue` compare-and-swap in `SetupStreamCancellation`, `NewIdleTimeoutReader`, and `ReleaseStreamingResponse`. Previously, the cancellation goroutine, the idle-timeout timer, and `ReleaseStreamingResponse` could each observe `false` and all proceed to call `CloseWithError`, double-releasing the pooled fasthttp `requestStream` and corrupting a concurrent request's slice bounds. Now exactly one owner wins the claim and performs the close. - **Removed re-panic in idle timeout reader (`utils.go`):** Dropped the unconditional `panic(recovered)` in `idleTimeoutReader.Read`'s recover block, which was re-panicking on legitimate post-close reads after the atomic-claim fix moved `r.fired.Store(true)` to the winning-owner path only. - **Force-reap stream accumulator backstop (`accumulator.go`, `tracer.go`, `bifrost.go`):** Added `ForceCleanupStreamAccumulator` to the `Tracer` interface, `NoOpTracer`, `Accumulator`, and `Tracer` implementations. The stream's terminal lifecycle hook (the provider goroutine's `postHookSpanFinalizer`) now calls this after releasing the plugin pipeline, guaranteeing that accumulators for streams that end without a clean terminal chunk (client abort, broken SSE write, multi-plugin refcount imbalance) are freed immediately rather than waiting for the TTL sweep. The trace ID is captured as an immutable string before the pooled request object is recycled. - **O(n²) → O(n) Responses stream builder (`responses.go`):** Replaced per-delta `*field += delta` string concatenation in `buildCompleteMessageFromResponsesStreamChunks` with `strings.Builder` accumulators keyed by message and content-block index. Builders are materialized into final strings once after the chunk walk. The five helper methods (`appendTextDeltaToResponsesMessage`, `appendRefusalDeltaToResponsesMessage`, `appendFunctionArgumentsDeltaToResponsesMessage`, `appendReasoningDeltaToResponsesMessage`, `appendReasoningSignatureToResponsesMessage`) are removed and their logic is inlined into the walk + materialization pass. - **Tests (`idle_timeout_reader_test.go`, `responses_test.go`):** Added `TestStreamClose_ExactlyOnceAcrossOwners` (races the timer and cancellation goroutine 100 times, asserts `CloseWithError` fires exactly once), `TestConnectionClosedClaim_SerializesOwners` (64 concurrent goroutines, asserts exactly one wins), `TestReleaseStreamingResponse_SkipsWhenAlreadyClaimed`, `TestBuildResponsesMessageConcatenatesTextDeltas`, `TestBuildResponsesMessageRoutesParallelToolArgs`, `TestBuildResponsesMessageAccumulatesReasoningSummary`, `TestForceCleanupStreamAccumulatorReapsRegardlessOfRefcount`, and `BenchmarkBuildResponsesMessageTextDeltas`. - **UI formatting (`clientLayout.tsx`):** Reformatted from 2-space to tab indentation and collapsed some multi-line imports; no behavioral changes. ## Type of change - [x] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Run with race detector to exercise the atomic-claim fix go test -race ./core/providers/utils/... -run TestStreamClose_ExactlyOnceAcrossOwners -count=10 go test -race ./core/providers/utils/... -run TestConnectionClosedClaim_SerializesOwners go test -race ./core/providers/utils/... -run TestReleaseStreamingResponse_SkipsWhenAlreadyClaimed # Force-reap accumulator backstop go test ./framework/streaming/... -run TestForceCleanupStreamAccumulatorReapsRegardlessOfRefcount -v # Responses stream builder correctness and performance go test ./framework/streaming/... -run TestBuildResponsesMessage go test -bench=BenchmarkBuildResponsesMessageTextDeltas -benchmem ./framework/streaming/... # Full suite go test ./... # UI cd ui pnpm i pnpm build ``` ## Breaking changes - [x] No The `Tracer` interface gains `ForceCleanupStreamAccumulator`. Any external implementations of `Tracer` must add this method. The `NoOpTracer` implementation is provided as a reference. ## Related issues Fixes the `slice bounds out of range [:-680]` crash caused by double-release of fasthttp's pooled `requestStream`. Fixes stream accumulator leaks on client-aborted or non-cleanly-terminated streams. ## Security considerations No auth, secrets, or PII implications. The fix closes a memory-corruption vector (double-pool-release) that could cause one request to alias another's in-flight data. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
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
2 participants
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.
Summary
This PR fixes two classes of bugs: a race condition that caused fasthttp's pooled
requestStreamto be double-released (producingslice bounds out of rangepanics on concurrent requests), and a stream accumulator memory leak where accumulators for aborted or non-cleanly-terminated streams were not freed until the TTL sweep. It also replaces O(n²) string concatenation in the Responses API stream builder with astrings.Builder-based accumulator pattern.Changes
Atomic stream close ownership (
utils.go): Replaced the racyValue-then-SetValueguard onBifrostContextKeyConnectionClosedwith a singleGetAndSetValuecompare-and-swap inSetupStreamCancellation,NewIdleTimeoutReader, andReleaseStreamingResponse. Previously, the cancellation goroutine, the idle-timeout timer, andReleaseStreamingResponsecould each observefalseand all proceed to callCloseWithError, double-releasing the pooled fasthttprequestStreamand corrupting a concurrent request's slice bounds. Now exactly one owner wins the claim and performs the close.Removed re-panic in idle timeout reader (
utils.go): Dropped the unconditionalpanic(recovered)inidleTimeoutReader.Read's recover block, which was re-panicking on legitimate post-close reads after the atomic-claim fix movedr.fired.Store(true)to the winning-owner path only.Force-reap stream accumulator backstop (
accumulator.go,tracer.go,bifrost.go): AddedForceCleanupStreamAccumulatorto theTracerinterface,NoOpTracer,Accumulator, andTracerimplementations. The stream's terminal lifecycle hook (the provider goroutine'spostHookSpanFinalizer) now calls this after releasing the plugin pipeline, guaranteeing that accumulators for streams that end without a clean terminal chunk (client abort, broken SSE write, multi-plugin refcount imbalance) are freed immediately rather than waiting for the TTL sweep. The trace ID is captured as an immutable string before the pooled request object is recycled.O(n²) → O(n) Responses stream builder (
responses.go): Replaced per-delta*field += deltastring concatenation inbuildCompleteMessageFromResponsesStreamChunkswithstrings.Builderaccumulators keyed by message and content-block index. Builders are materialized into final strings once after the chunk walk. The five helper methods (appendTextDeltaToResponsesMessage,appendRefusalDeltaToResponsesMessage,appendFunctionArgumentsDeltaToResponsesMessage,appendReasoningDeltaToResponsesMessage,appendReasoningSignatureToResponsesMessage) are removed and their logic is inlined into the walk + materialization pass.Tests (
idle_timeout_reader_test.go,responses_test.go): AddedTestStreamClose_ExactlyOnceAcrossOwners(races the timer and cancellation goroutine 100 times, assertsCloseWithErrorfires exactly once),TestConnectionClosedClaim_SerializesOwners(64 concurrent goroutines, asserts exactly one wins),TestReleaseStreamingResponse_SkipsWhenAlreadyClaimed,TestBuildResponsesMessageConcatenatesTextDeltas,TestBuildResponsesMessageRoutesParallelToolArgs,TestBuildResponsesMessageAccumulatesReasoningSummary,TestForceCleanupStreamAccumulatorReapsRegardlessOfRefcount, andBenchmarkBuildResponsesMessageTextDeltas.UI formatting (
clientLayout.tsx): Reformatted from 2-space to tab indentation and collapsed some multi-line imports; no behavioral changes.Type of change
Affected areas
How to test
Breaking changes
The
Tracerinterface gainsForceCleanupStreamAccumulator. Any external implementations ofTracermust add this method. TheNoOpTracerimplementation is provided as a reference.Related issues
Fixes the
slice bounds out of range [:-680]crash caused by double-release of fasthttp's pooledrequestStream. Fixes stream accumulator leaks on client-aborted or non-cleanly-terminated streams.Security considerations
No auth, secrets, or PII implications. The fix closes a memory-corruption vector (double-pool-release) that could cause one request to alias another's in-flight data.
Checklist
docs/contributing/README.mdand followed the guidelines