Conversation
|
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTrace completion and transport post-hook handling were made goroutine-safe: trace completers now accept transport plugin logs, transport post-hook completers are stored in an Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant Producer as ProducerGoroutine
participant PostHooks as PostHookRunner
participant Tracer as Tracer
Client->>Router: open streaming request
Router->>Producer: start producer goroutine (capture traceCompleter, store completerSlot)
Producer->>Client: stream SSE chunks...
Producer-->>Producer: defer cleanup -> load completer from completerSlot
Producer->>PostHooks: invoke completer -> runTransportPostHooksCaptured(snapshot)
PostHooks-->Producer: transportLogs or error
Producer->>Tracer: call traceCompleter(transportLogs)
Tracer-->>Producer: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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: 2
🧹 Nitpick comments (1)
transports/bifrost-http/lib/ctx.go (1)
620-624: Clear reused response fields before building the snapshot.This helper only fills part of a pooled
schemas.HTTPResponse. Clearing any omitted fields up front makes the “status + headers only” contract independent of pool cleanup and avoids stale data bleeding into later hook consumers.As per coding guidelines, "Always reset ALL fields of pooled objects before calling
pool.Put()to prevent stale data from leaking to subsequent users."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 620 - 624, The pooled resp returned by schemas.AcquireHTTPResponse() must be fully reset before populating just status and headers to avoid leaking stale pooled data; update the code around resp (the schemas.AcquireHTTPResponse() result) to zero-out or reinitialize all fields on the HTTPResponse struct (e.g., clear Body, Trailer, Cookies, any Proto/Reason/Flags, and replace or clear the Headers map) and then set resp.StatusCode and copy ctx.Response.Header into resp.Headers so the snapshot contains only the intended fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 364-390: The deferred completer is being stored into the atomic
slot after next(ctx) returns, allowing a racing goroutine to load a nil/absent
value and leak capturedReq/capturedResp; move the atomic publish so the slot is
populated before the handler returns. Specifically, locate the
ctx.UserValue(schemas.BifrostContextKeyTransportPostHookCompleter) slot and call
slot.Store(completer) (or otherwise publish the completer) prior to invoking
next(ctx) (or any point before the handler can return), ensuring the
closure-created completer (which wraps capturedReq/capturedResp and calls
runTransportPostHooksCaptured) is visible atomically to the cleanup goroutine to
avoid dropped post-hooks and leaked snapshots.
- Around line 462-506: The cloned request used in runTransportPostHooksCaptured
omits copying capturedReq.Body so post-hooks see wrong payloads for streamed
requests; fix by copying the body from capturedReq into the pooled req (e.g., if
capturedReq.Body != nil create a deep copy/byte buffer and assign it to
req.Body/any related body fields) so plugins operate on the same payload as the
captured snapshot (ensure you copy, not alias, so plugins can mutate req
safely).
---
Nitpick comments:
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 620-624: The pooled resp returned by schemas.AcquireHTTPResponse()
must be fully reset before populating just status and headers to avoid leaking
stale pooled data; update the code around resp (the
schemas.AcquireHTTPResponse() result) to zero-out or reinitialize all fields on
the HTTPResponse struct (e.g., clear Body, Trailer, Cookies, any
Proto/Reason/Flags, and replace or clear the Headers map) and then set
resp.StatusCode and copy ctx.Response.Header into resp.Headers so the snapshot
contains only the intended fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53cdba1f-023a-46fa-8447-55ac62ebbb32
📒 Files selected for processing (5)
core/schemas/bifrost.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.go
akshaydeo
force-pushed
the
04-13-exception_fixes
branch
from
83b7999 to
d8fe420
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/azure/azure.go (1)
2864-2909:⚠️ Potential issue | 🔴 CriticalCapture the post-hook finalizer before starting the goroutine.
The EOF path still does a late
ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer)lookup and invokes hooks from inside the stream goroutine. That can preserve the same lifetime bug this stack is fixing if the stored closure still closes over transport-scoped state. Resolve the finalizer/captured hook once beforego func()and pass the captured function(s) into the goroutine instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/azure/azure.go` around lines 2864 - 2909, The EOF path currently looks up and invokes the post-hook finalizer inside the streaming goroutine, which can keep transport-scoped state alive; fix this by reading and storing the finalizer (ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) cast to func(context.Context)) into a local variable before starting the goroutine and passing that captured finalizer into the goroutine; then, in the EOF handling path inside the loop (where postHookRunner is called and the finalizer is invoked), call the pre-captured finalizer if not nil instead of doing a fresh ctx.Value lookup, ensuring you still call postHookRunner(ctx, finalResp, nil) and invoke the captured finalizer(ctx) when present.transports/bifrost-http/integrations/router.go (1)
2317-2343:⚠️ Potential issue | 🟠 MajorCapture and run the transport post-hook completer here too.
This path now captures
traceCompletersafely, but it still hardcodestraceCompleter(nil)and never loadsBifrostContextKeyTransportPostHookCompleter. SinceRegisterRoutesruns these handlers through the shared middleware stack, streaming integration routes can skip the captured transport post-hooks or lose their plugin logs in the final trace. Please mirror thetransports/bifrost-http/handlers/inference.gocleanup flow here: capture the atomic completer before the goroutine starts, run it in the deferred teardown, and pass the returned logs intotraceCompleter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/integrations/router.go` around lines 2317 - 2343, Capture the transport post-hook completer from context (using schemas.BifrostContextKeyTransportPostHookCompleter) before starting the goroutine (just like traceCompleter is captured) and store it (e.g., transportPostHookCompleter). In the deferred teardown inside the producer goroutine (where reader.Done() and schemas.ReleaseHTTPRequest(httpReq) are deferred), call the transportPostHookCompleter (if non-nil) to obtain transportLogs and then pass those logs into traceCompleter(transportLogs) instead of traceCompleter(nil); ensure both completers are nil-checked and reference the existing symbols used here (traceCompleter, NewSSEStreamReader/reader, BuildHTTPRequestFromFastHTTP, GetStreamChunkInterceptor, schemas.ReleaseHTTPRequest) so the change mirrors the cleanup flow in transports/bifrost-http/handlers/inference.go.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
462-476:⚠️ Potential issue | 🟠 MajorRequest body is still not copied to the cloned request object.
The past review comment on this section remains unaddressed. The clone copies
Method,Path,Headers,Query, andPathParamsfromcapturedReq, but omitsBody. Any transport post-hook that logs, redacts, or validates the request payload will see an empty body for streaming requests.🐛 Proposed fix to copy the body
req.Method = capturedReq.Method req.Path = capturedReq.Path + if len(capturedReq.Body) > 0 { + req.Body = make([]byte, len(capturedReq.Body)) + copy(req.Body, capturedReq.Body) + } for k, v := range capturedReq.Headers { req.Headers[k] = v }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 462 - 476, In runTransportPostHooksCaptured the cloned request omits the request body so plugins see an empty payload for streaming requests; copy and deep-clone capturedReq.Body into the pooled req (e.g., set req.Body to a new byte slice copy of capturedReq.Body) before returning to ensure plugins can read/mutate safely (refer to runTransportPostHooksCaptured, schemas.AcquireHTTPRequest, and the schemas.HTTPRequest.Body field).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 1029-1030: Validation branches that return early for OCR requests
are not attaching model context, so populate OriginalModelRequested and
ResolvedModelUsed on the returned error responses; in the branches that build
the response for schemas.OCRRequest (where RequestType: schemas.OCRRequest and
Provider: req.Provider are set) add OriginalModelRequested:
req.OriginalModelRequested and ResolvedModelUsed: req.ResolvedModelUsed (or the
resolved model variable) so all three early-return error paths include the same
model metadata as other validators.
In `@core/providers/azure/azure.go`:
- Around line 2854-2857: The passthrough stream response metadata currently
constructs extraFields without preserving the requested model; update the
construction of schemas.BifrostResponseExtraFields (the extraFields variable
created near provider.GetProviderKey() and schemas.PassthroughStreamRequest) to
include OriginalModelRequested set from the incoming request model (e.g.,
req.Model or the function's request parameter) so chunk/final-response hooks and
traces retain the requested model information.
In `@core/providers/mistral/errors.go`:
- Around line 22-24: ParseMistralError currently drops provider/request
metadata; restore the metadata inputs or make the helper internal. Specifically,
either revert ParseMistralError to accept the metadata parameters (e.g.,
provider string, requestType string, originalModelRequested string) and populate
bifrostErr.ExtraFields.Provider, .RequestType and .OriginalModelRequested before
returning, or mark ParseMistralError unexported and update tests to use
core/bifrost.go's PopulateExtraFields path; reference the ParseMistralError
function and (*schemas.BifrostError).PopulateExtraFields to ensure the metadata
is set consistently.
In `@core/schemas/chatcompletions.go`:
- Around line 165-172: The conversion in ToTextCompletionResponse() drops model
metadata—add OriginalModelRequested and ResolvedModelUsed to the
BifrostResponseExtraFields struct literal so they are copied from cr.ExtraFields
(e.g., ExtraFields: BifrostResponseExtraFields{ RequestType: ..., ChunkIndex:
cr.ExtraFields.ChunkIndex, Provider: cr.ExtraFields.Provider, Latency:
cr.ExtraFields.Latency, RawResponse: cr.ExtraFields.RawResponse, CacheDebug:
cr.ExtraFields.CacheDebug, OriginalModelRequested:
cr.ExtraFields.OriginalModelRequested, ResolvedModelUsed:
cr.ExtraFields.ResolvedModelUsed }). Make the same addition in every return
branch inside ToTextCompletionResponse() so the model identity is preserved
across all code paths.
---
Outside diff comments:
In `@core/providers/azure/azure.go`:
- Around line 2864-2909: The EOF path currently looks up and invokes the
post-hook finalizer inside the streaming goroutine, which can keep
transport-scoped state alive; fix this by reading and storing the finalizer
(ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) cast to
func(context.Context)) into a local variable before starting the goroutine and
passing that captured finalizer into the goroutine; then, in the EOF handling
path inside the loop (where postHookRunner is called and the finalizer is
invoked), call the pre-captured finalizer if not nil instead of doing a fresh
ctx.Value lookup, ensuring you still call postHookRunner(ctx, finalResp, nil)
and invoke the captured finalizer(ctx) when present.
In `@transports/bifrost-http/integrations/router.go`:
- Around line 2317-2343: Capture the transport post-hook completer from context
(using schemas.BifrostContextKeyTransportPostHookCompleter) before starting the
goroutine (just like traceCompleter is captured) and store it (e.g.,
transportPostHookCompleter). In the deferred teardown inside the producer
goroutine (where reader.Done() and schemas.ReleaseHTTPRequest(httpReq) are
deferred), call the transportPostHookCompleter (if non-nil) to obtain
transportLogs and then pass those logs into traceCompleter(transportLogs)
instead of traceCompleter(nil); ensure both completers are nil-checked and
reference the existing symbols used here (traceCompleter,
NewSSEStreamReader/reader, BuildHTTPRequestFromFastHTTP,
GetStreamChunkInterceptor, schemas.ReleaseHTTPRequest) so the change mirrors the
cleanup flow in transports/bifrost-http/handlers/inference.go.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 462-476: In runTransportPostHooksCaptured the cloned request omits
the request body so plugins see an empty payload for streaming requests; copy
and deep-clone capturedReq.Body into the pooled req (e.g., set req.Body to a new
byte slice copy of capturedReq.Body) before returning to ensure plugins can
read/mutate safely (refer to runTransportPostHooksCaptured,
schemas.AcquireHTTPRequest, and the schemas.HTTPRequest.Body field).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 030d94a0-93a5-4327-a412-c113a6b7d095
📒 Files selected for processing (13)
core/bifrost.gocore/bifrost_test.gocore/providers/azure/azure.gocore/providers/mistral/custom_provider_test.gocore/providers/mistral/errors.gocore/providers/mistral/mistral.gocore/providers/mistral/ocr_test.gocore/schemas/bifrost.gocore/schemas/chatcompletions.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/handlers/inference.go
Contributor
Confidence Score: 4/5The core streaming race-condition fix is sound, but one streaming path silently drops traces and should be addressed before merging. The inference.go and handleStreaming paths are correctly implemented. One P1 gap remains: handlePassthroughStream sets the deferred-trace flag but never captures or calls traceCompleter, causing traces for all passthrough streaming requests to be silently dropped. Resolving the passthrough trace gap is a one-line capture plus one-line call that mirrors the already-correct handleStreaming pattern. transports/bifrost-http/integrations/router.go — handlePassthroughStream goroutine is missing traceCompleter capture and call Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
04-13-exception_fixes
branch
from
d8fe420 to
8b370cb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/integrations/router.go (1)
2317-2343:⚠️ Potential issue | 🟠 MajorInstall and execute the deferred transport post-hook completer here too.
This streaming path only captures
traceCompleter, but it never creates/loadsschemas.BifrostContextKeyTransportPostHookCompleter. Because these routes still run the shared middleware chain,TransportInterceptorMiddlewarewill skip the deferred post-hooks unless that*atomic.Valueslot exists. The result is that streaming integration routes lose transport post-hooks entirely, traces always miss transport plugin logs, and the captured request/response snapshots intransports/bifrost-http/handlers/middlewares.go:355-395never get released through the completer path.transports/bifrost-http/handlers/inference.go:1676-1720already has the correct pattern; this handler needs the same handoff/load flow instead of hard-codingtraceCompleter(nil).Suggested fix
+// also add `sync/atomic` to the imports + // Capture trace completer BEFORE goroutine — ctx may be recycled inside goroutine. // Signature: func(transportLogs []schemas.PluginLogEntry) + var completerSlot atomic.Value + ctx.SetUserValue(schemas.BifrostContextKeyTransportPostHookCompleter, &completerSlot) traceCompleter, _ := ctx.UserValue(schemas.BifrostContextKeyTraceCompleter).(func([]schemas.PluginLogEntry)) @@ defer func() { - // Complete the trace after streaming finishes - // This ensures all spans (including llm.call) are properly ended before the trace is sent to OTEL. - // Router path has no transport post-hooks, so pass nil for transport logs. + var transportLogs []schemas.PluginLogEntry + if fn, ok := completerSlot.Load().(func() ([]schemas.PluginLogEntry, error)); ok && fn != nil { + logs, err := fn() + transportLogs = logs + if err != nil { + g.logger.Warn("transport post-hooks failed: %v", err) + } + } if traceCompleter != nil { - traceCompleter(nil) + traceCompleter(transportLogs) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/integrations/router.go` around lines 2317 - 2343, The streaming handler currently only captures traceCompleter but never loads or invokes the transport post-hook completer, so transport post-hooks and plugin logs are skipped; fix this by reading schemas.BifrostContextKeyTransportPostHookCompleter from the ctx (same pattern as TransportInterceptorMiddleware / the inference handler): capture a local variable like transportCompleterVal, check it != nil, do v := transportCompleterVal.(*atomic.Value) and if v.Load() != nil cast to a func([]schemas.PluginLogEntry) and call that completer (passing collected transport plugin logs or nil if none) in the same deferred cleanup block before/alongside traceCompleter(nil) so transport post-hooks are executed and logs are handed off correctly.
🧹 Nitpick comments (1)
framework/modelcatalog/pricing_test.go (1)
193-196: Inconsistent pointer creation style.The file now mixes
bifrost.Ptr(...)(e.g., lines 23-24, 132-133) withnew(float64_literal)(lines 193-196, 213-216, 318, etc.). While both are valid, consider unifying tobifrost.Ptr()for consistency with repository conventions.Based on learnings: "prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/pricing_test.go` around lines 193 - 196, The test uses raw new(float64) pointer creation for pricing fields (e.g., p.InputCostPerTokenAbove200kTokens, p.OutputCostPerTokenAbove200kTokens, p.InputCostPerTokenAbove272kTokens, p.OutputCostPerTokenAbove272kTokens); replace these with the repository-convention helper bifrost.Ptr(float64) so all pointer float literals in pricing_test.go follow the existing bifrost.Ptr(...) style (match other instances around lines where bifrost.Ptr is already used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/lib/pricing_integration_test.go`:
- Around line 514-516: The test calls mc.SetShouldSyncGate immediately after
modelcatalog.Init which can dereference mc if Init returned an error; change the
order so you assert require.NoError(t, err) right after calling
modelcatalog.Init(ctx, cfg, store, clg) and only then call
mc.SetShouldSyncGate(noSyncFunc). Apply the same change for all occurrences that
call modelcatalog.Init followed by mc.SetShouldSyncGate (the instances around
Init lines referenced) so Init error is checked before using mc.
---
Outside diff comments:
In `@transports/bifrost-http/integrations/router.go`:
- Around line 2317-2343: The streaming handler currently only captures
traceCompleter but never loads or invokes the transport post-hook completer, so
transport post-hooks and plugin logs are skipped; fix this by reading
schemas.BifrostContextKeyTransportPostHookCompleter from the ctx (same pattern
as TransportInterceptorMiddleware / the inference handler): capture a local
variable like transportCompleterVal, check it != nil, do v :=
transportCompleterVal.(*atomic.Value) and if v.Load() != nil cast to a
func([]schemas.PluginLogEntry) and call that completer (passing collected
transport plugin logs or nil if none) in the same deferred cleanup block
before/alongside traceCompleter(nil) so transport post-hooks are executed and
logs are handed off correctly.
---
Nitpick comments:
In `@framework/modelcatalog/pricing_test.go`:
- Around line 193-196: The test uses raw new(float64) pointer creation for
pricing fields (e.g., p.InputCostPerTokenAbove200kTokens,
p.OutputCostPerTokenAbove200kTokens, p.InputCostPerTokenAbove272kTokens,
p.OutputCostPerTokenAbove272kTokens); replace these with the
repository-convention helper bifrost.Ptr(float64) so all pointer float literals
in pricing_test.go follow the existing bifrost.Ptr(...) style (match other
instances around lines where bifrost.Ptr is already used).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa23ee03-b25a-4ca1-a76e-3b746006420f
⛔ Files ignored due to path filters (1)
plugins/prompts/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
core/bifrost.gocore/bifrost_test.gocore/providers/azure/azure.gocore/providers/mistral/custom_provider_test.gocore/providers/mistral/errors.gocore/providers/mistral/mistral.gocore/providers/mistral/ocr_test.gocore/schemas/bifrost.gocore/schemas/chatcompletions.goframework/modelcatalog/config.goframework/modelcatalog/main.goframework/modelcatalog/pricing.goframework/modelcatalog/pricing_test.goframework/modelcatalog/utils.goframework/oauth2/main.goplugins/prompts/go.modtransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/pricing_integration_test.go
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/providers.go
✅ Files skipped from review due to trivial changes (4)
- plugins/prompts/go.mod
- framework/modelcatalog/utils.go
- core/providers/mistral/ocr_test.go
- core/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/schemas/chatcompletions.go
- core/providers/mistral/errors.go
- core/providers/mistral/mistral.go
- transports/bifrost-http/lib/ctx.go
- core/providers/azure/azure.go
- transports/bifrost-http/handlers/middlewares.go
- transports/bifrost-http/handlers/inference.go
Pratham-Mishra04
force-pushed
the
04-13-exception_fixes
branch
from
8b370cb to
82b2d6c
Compare
Pratham-Mishra04
force-pushed
the
04-13-exception_fixes
branch
from
82b2d6c to
36cf4ba
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/integrations/router.go (1)
2317-2343:⚠️ Potential issue | 🟠 MajorIntegration streaming still skips deferred transport post-hooks.
This cleanup only captures
BifrostContextKeyTraceCompleter. Because these routes run through the shared middleware chain,TransportInterceptorMiddlewarewill switch to deferred mode for streamed responses, but there is noBifrostContextKeyTransportPostHookCompleterslot here to publish/load. That means streamed integration routes never run transport post-hooks, and their transport plugin logs never reach tracing when transport plugins are enabled.Please mirror the
handlers/inference.gopattern here: create/capture the post-hook completer slot before spawning the goroutine, run it in the deferred cleanup, and pass the returned logs intotraceCompleter(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/integrations/router.go` around lines 2317 - 2343, The streaming producer currently only captures BifrostContextKeyTraceCompleter but misses the transport post-hook completer, so deferred transport post-hooks never run for streamed routes; before spawning the goroutine capture the post-hook completer from ctx (e.g., via ctx.UserValue(schemas.BifrostContextKeyTransportPostHookCompleter).(func() []schemas.PluginLogEntry) or similar) into a variable (name it transportPostHookCompleter), then in the goroutine's deferred cleanup call that completer to obtain transport logs and pass those logs into the existing traceCompleter(nil) call (i.e., call traceCompleter(transportLogs)), ensuring you still call reader.Done() and schemas.ReleaseHTTPRequest(httpReq) in their defers and preserve LIFO order.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
462-476:⚠️ Potential issue | 🟠 MajorCopy
capturedReq.Bodyinto the cloned request.
runTransportPostHooksCaptured()still rebuilds the mutable request without the body, so streamed post-hooks see a different request than non-streaming ones. Any plugin that logs, redacts, or validates payloads will silently lose request content on streamed routes.Proposed fix
req.Method = capturedReq.Method req.Path = capturedReq.Path + if len(capturedReq.Body) > 0 { + req.Body = append(req.Body[:0], capturedReq.Body...) + } for k, v := range capturedReq.Headers { req.Headers[k] = v } for k, v := range capturedReq.Query {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 462 - 476, In runTransportPostHooksCaptured, the cloned mutable request (req acquired via schemas.AcquireHTTPRequest) is missing the body from capturedReq, causing streamed post-hooks to see an empty payload; update the function to copy capturedReq.Body into req.Body (making a deep copy of the byte slice or otherwise cloning the buffer) after copying headers/query/path params so plugins that log/redact/validate payloads see the same request content as non-streamed routes; ensure you use the same request variable (req) and release semantics already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transports/bifrost-http/integrations/router.go`:
- Around line 2317-2343: The streaming producer currently only captures
BifrostContextKeyTraceCompleter but misses the transport post-hook completer, so
deferred transport post-hooks never run for streamed routes; before spawning the
goroutine capture the post-hook completer from ctx (e.g., via
ctx.UserValue(schemas.BifrostContextKeyTransportPostHookCompleter).(func()
[]schemas.PluginLogEntry) or similar) into a variable (name it
transportPostHookCompleter), then in the goroutine's deferred cleanup call that
completer to obtain transport logs and pass those logs into the existing
traceCompleter(nil) call (i.e., call traceCompleter(transportLogs)), ensuring
you still call reader.Done() and schemas.ReleaseHTTPRequest(httpReq) in their
defers and preserve LIFO order.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 462-476: In runTransportPostHooksCaptured, the cloned mutable
request (req acquired via schemas.AcquireHTTPRequest) is missing the body from
capturedReq, causing streamed post-hooks to see an empty payload; update the
function to copy capturedReq.Body into req.Body (making a deep copy of the byte
slice or otherwise cloning the buffer) after copying headers/query/path params
so plugins that log/redact/validate payloads see the same request content as
non-streamed routes; ensure you use the same request variable (req) and release
semantics already present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 753b7b91-784e-4d1f-ae25-c98dd7eeee6a
⛔ Files ignored due to path filters (1)
plugins/prompts/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
core/bifrost.gocore/bifrost_test.gocore/providers/azure/azure.gocore/providers/mistral/custom_provider_test.gocore/providers/mistral/errors.gocore/providers/mistral/mistral.gocore/providers/mistral/ocr_test.gocore/schemas/bifrost.gocore/schemas/chatcompletions.goframework/modelcatalog/config.goframework/modelcatalog/main.goframework/modelcatalog/pricing.goframework/modelcatalog/pricing_test.goframework/modelcatalog/utils.goframework/oauth2/main.goplugins/prompts/go.modtransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/pricing_integration_test.go
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/providers.go
✅ Files skipped from review due to trivial changes (5)
- plugins/prompts/go.mod
- core/providers/mistral/ocr_test.go
- framework/modelcatalog/config.go
- framework/modelcatalog/utils.go
- transports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- core/bifrost_test.go
- transports/bifrost-http/handlers/config.go
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/handlers/asyncinference.go
- core/providers/mistral/mistral.go
- core/providers/mistral/custom_provider_test.go
- transports/bifrost-http/lib/config.go
- core/schemas/chatcompletions.go
- transports/bifrost-http/handlers/inference.go
- framework/modelcatalog/pricing_test.go
- transports/bifrost-http/lib/pricing_integration_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/azure/azure.go (1)
2844-2895:⚠️ Potential issue | 🔴 CriticalNormal EOF still bypasses the captured post-hook path.
The timeout/error branches now use the safe helpers, but the success path still calls
postHookRunner(ctx, finalResp, nil)directly and readsschemas.BifrostContextKeyPostHookSpanFinalizerfromctxinside the goroutine. That leaves normal passthrough completion exposed to the same recycled-request-context race this PR is fixing. Please route the EOF branch through the same captured post-hook/finalizer flow before spawning the goroutine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/azure/azure.go` around lines 2844 - 2895, The EOF success path is still directly calling postHookRunner and reading the finalizer from ctx inside the goroutine, exposing a recycled-context race; before launching the goroutine capture the postHookRunner and the post-hook finalizer (e.g., read provider.postHookRunner into a local like capturedPostHookRunner and extract ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) into capturedFinalizer) and then inside the goroutine use those capturedPostHookRunner(capturedCtx, finalResp, nil) and capturedFinalizer(capturedCtx) (if non-nil) for the EOF branch instead of accessing ctx and postHookRunner directly so the EOF path uses the same safe captured post-hook/finalizer flow as the timeout/error branches.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/middlewares.go (1)
463-476:⚠️ Potential issue | 🟠 MajorCopy the captured request body into the cloned request.
The captured path still clones method/path/headers/query/path params only.
capturedReq.Bodynever makes it intoreq, so streamed post-hooks see an empty payload while the non-streaming path passes the request body through.Possible fix
req.Method = capturedReq.Method req.Path = capturedReq.Path + if len(capturedReq.Body) > 0 { + req.Body = append(req.Body[:0], capturedReq.Body...) + } for k, v := range capturedReq.Headers { req.Headers[k] = v }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 463 - 476, The cloned request misses copying the body, so streamed post-hooks see no payload; when creating req from capturedReq (in the AcquireHTTPRequest/ReleaseHTTPRequest block), copy the body into req using a deep copy rather than sharing the underlying buffer: if capturedReq.Body is a []byte set req.Body = append([]byte(nil), capturedReq.Body...), or if it's an io.ReadCloser wrap a new reader (e.g., ioutil.NopCloser(bytes.NewReader(...))) so the cloned req.Body contains the same bytes and can be consumed independently; update the cloning code around req, capturedReq, AcquireHTTPRequest and ReleaseHTTPRequest to perform this deep copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/chatcompletions.go`:
- Around line 171-180: The fallback branch constructing
BifrostResponseExtraFields in ToTextCompletionResponse (or the function building
the TextCompletionResponse) omits ProviderResponseHeaders, causing loss of
request-id and rate-limit metadata; update the struct literal in that branch to
include ProviderResponseHeaders: cr.ExtraFields.ProviderResponseHeaders so the
fallback path preserves headers just like the other return paths (look for the
ExtraFields: BifrostResponseExtraFields{...} block and add the
ProviderResponseHeaders field).
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 365-390: The capturedReq and capturedResp snapshots are only
released inside the completer closure, so if
ctx.UserValue(schemas.BifrostContextKeyTransportPostHookCompleter) is missing or
the type assertion fails the snapshots leak; update the branch after the type
assertion on slot to release the pooled snapshots (call
schemas.ReleaseHTTPRequest(capturedReq) and
schemas.ReleaseHTTPResponse(capturedResp)) when ok == false and avoid silently
dropping post-hooks (optionally invoke
runTransportPostHooksCaptured(capturedReq, capturedResp, plugins, bifrostCtx)
synchronously or log the missing slot) so resources are cleaned up and post-hook
results are not lost.
---
Outside diff comments:
In `@core/providers/azure/azure.go`:
- Around line 2844-2895: The EOF success path is still directly calling
postHookRunner and reading the finalizer from ctx inside the goroutine, exposing
a recycled-context race; before launching the goroutine capture the
postHookRunner and the post-hook finalizer (e.g., read provider.postHookRunner
into a local like capturedPostHookRunner and extract
ctx.Value(schemas.BifrostContextKeyPostHookSpanFinalizer) into
capturedFinalizer) and then inside the goroutine use those
capturedPostHookRunner(capturedCtx, finalResp, nil) and
capturedFinalizer(capturedCtx) (if non-nil) for the EOF branch instead of
accessing ctx and postHookRunner directly so the EOF path uses the same safe
captured post-hook/finalizer flow as the timeout/error branches.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 463-476: The cloned request misses copying the body, so streamed
post-hooks see no payload; when creating req from capturedReq (in the
AcquireHTTPRequest/ReleaseHTTPRequest block), copy the body into req using a
deep copy rather than sharing the underlying buffer: if capturedReq.Body is a
[]byte set req.Body = append([]byte(nil), capturedReq.Body...), or if it's an
io.ReadCloser wrap a new reader (e.g., ioutil.NopCloser(bytes.NewReader(...)))
so the cloned req.Body contains the same bytes and can be consumed
independently; update the cloning code around req, capturedReq,
AcquireHTTPRequest and ReleaseHTTPRequest to perform this deep copy.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13223da3-92f9-4815-b7f7-7878d0fbc74d
⛔ Files ignored due to path filters (1)
plugins/prompts/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
core/bifrost.gocore/bifrost_test.gocore/providers/azure/azure.gocore/providers/mistral/custom_provider_test.gocore/providers/mistral/errors.gocore/providers/mistral/mistral.gocore/providers/mistral/ocr_test.gocore/schemas/bifrost.gocore/schemas/chatcompletions.goframework/modelcatalog/config.goframework/modelcatalog/main.goframework/modelcatalog/pricing.goframework/modelcatalog/pricing_test.goframework/modelcatalog/utils.goframework/oauth2/main.goplugins/prompts/go.modtransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/pricing_integration_test.go
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/providers.go
✅ Files skipped from review due to trivial changes (7)
- framework/modelcatalog/utils.go
- framework/oauth2/main.go
- core/providers/mistral/ocr_test.go
- framework/modelcatalog/config.go
- core/bifrost.go
- plugins/prompts/go.mod
- transports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- core/bifrost_test.go
- transports/bifrost-http/handlers/config.go
- core/providers/mistral/mistral.go
- core/providers/mistral/custom_provider_test.go
- core/providers/mistral/errors.go
- framework/modelcatalog/pricing_test.go
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/lib/ctx.go
- core/schemas/bifrost.go
- transports/bifrost-http/handlers/inference.go
- framework/modelcatalog/pricing.go
- transports/bifrost-http/integrations/router.go
Pratham-Mishra04
force-pushed
the
04-13-exception_fixes
branch
from
36cf4ba to
e614eb1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
transports/bifrost-http/handlers/middlewares.go (2)
462-476:⚠️ Potential issue | 🟠 MajorClone the captured request body for deferred post-hooks.
This clone preserves method/path/headers/query/path params, but
req.Bodystays empty. Any transport post-hook that logs, redacts, or validates the payload will see different input for streamed requests than for non-streamed ones.Possible fix
req.Method = capturedReq.Method req.Path = capturedReq.Path +if len(capturedReq.Body) > 0 { + req.Body = append(req.Body[:0], capturedReq.Body...) +} for k, v := range capturedReq.Headers { req.Headers[k] = v }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 462 - 476, runTransportPostHooksCaptured clones headers/query/pathparams but doesn't copy the captured request payload, so deferred post-hooks see an empty req.Body for streamed requests; update the function (runTransportPostHooksCaptured) to deep-copy capturedReq.Body into the pooled req.Body (preserving the same type/contents as capturedReq.Body) when acquiring the new schemas.HTTPRequest so post-hook plugins read the original payload (use a defensive copy or appropriate clone for the body representation used in capturedReq.Body to avoid sharing/mutation).
364-390:⚠️ Potential issue | 🟠 MajorSynchronize the deferred completer handoff.
next(ctx)returns only after the handler has already started the producer goroutine. If the upstream stream closes immediately, that goroutine can hit its deferredLoad()beforeslot.Store(completer)happens here, so transport post-hooks/logs are skipped and the captured snapshots never get released. Please add a regression test with an immediately-closing stream and a transport post-hook to verify this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 364 - 390, The deferred completer can be raced by the producer goroutine; to fix, ensure the completer is published to the atomic slot before the upstream goroutine can Load() it: create and Store a placeholder (or Store the actual completer) into the ctx UserValue slot keyed by BifrostContextKeyTransportPostHookCompleter immediately after constructing completer (use slot.Store(completer) before returning from the middleware or before calling next(ctx) where applicable), and keep the existing capturedReq/capturedResp release in the completer; update runTransportPostHooksCaptured usage and ensure slot.Store is used (atomic.Value) so the producer never observes a nil Load. Also add a regression test that opens a stream which closes immediately and registers a transport post-hook to verify post-hooks/logs run and captured snapshots are released.
🧹 Nitpick comments (1)
framework/modelcatalog/pricing_test.go (1)
193-196: Consider usingbifrost.Ptr()consistently.The test uses
new(0.000006)syntax here while other tests in this file (e.g., lines 107, 132-133) usebifrost.Ptr(). For consistency with the repository convention of preferringbifrost.Ptr()over raw pointer creation, consider standardizing on one approach. Based on learnings,bifrost.Ptr()is the preferred pattern in this codebase.✨ Example for consistency
- p.InputCostPerTokenAbove200kTokens = new(0.000006) - p.OutputCostPerTokenAbove200kTokens = new(0.00003) - p.InputCostPerTokenAbove272kTokens = new(0.000009) - p.OutputCostPerTokenAbove272kTokens = new(0.000045) + p.InputCostPerTokenAbove200kTokens = bifrost.Ptr(0.000006) + p.OutputCostPerTokenAbove200kTokens = bifrost.Ptr(0.00003) + p.InputCostPerTokenAbove272kTokens = bifrost.Ptr(0.000009) + p.OutputCostPerTokenAbove272kTokens = bifrost.Ptr(0.000045)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/pricing_test.go` around lines 193 - 196, The test sets pointer fields using new(...) for p.InputCostPerTokenAbove200kTokens, p.OutputCostPerTokenAbove200kTokens, p.InputCostPerTokenAbove272kTokens, and p.OutputCostPerTokenAbove272kTokens; replace those raw pointer usages with the repository-preferred helper bifrost.Ptr(value) for consistency (e.g., call bifrost.Ptr(0.000006) etc.) so the test matches other examples like the usages at lines ~107 and 132-133 and to follow the codebase convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 462-476: runTransportPostHooksCaptured clones
headers/query/pathparams but doesn't copy the captured request payload, so
deferred post-hooks see an empty req.Body for streamed requests; update the
function (runTransportPostHooksCaptured) to deep-copy capturedReq.Body into the
pooled req.Body (preserving the same type/contents as capturedReq.Body) when
acquiring the new schemas.HTTPRequest so post-hook plugins read the original
payload (use a defensive copy or appropriate clone for the body representation
used in capturedReq.Body to avoid sharing/mutation).
- Around line 364-390: The deferred completer can be raced by the producer
goroutine; to fix, ensure the completer is published to the atomic slot before
the upstream goroutine can Load() it: create and Store a placeholder (or Store
the actual completer) into the ctx UserValue slot keyed by
BifrostContextKeyTransportPostHookCompleter immediately after constructing
completer (use slot.Store(completer) before returning from the middleware or
before calling next(ctx) where applicable), and keep the existing
capturedReq/capturedResp release in the completer; update
runTransportPostHooksCaptured usage and ensure slot.Store is used (atomic.Value)
so the producer never observes a nil Load. Also add a regression test that opens
a stream which closes immediately and registers a transport post-hook to verify
post-hooks/logs run and captured snapshots are released.
---
Nitpick comments:
In `@framework/modelcatalog/pricing_test.go`:
- Around line 193-196: The test sets pointer fields using new(...) for
p.InputCostPerTokenAbove200kTokens, p.OutputCostPerTokenAbove200kTokens,
p.InputCostPerTokenAbove272kTokens, and p.OutputCostPerTokenAbove272kTokens;
replace those raw pointer usages with the repository-preferred helper
bifrost.Ptr(value) for consistency (e.g., call bifrost.Ptr(0.000006) etc.) so
the test matches other examples like the usages at lines ~107 and 132-133 and to
follow the codebase convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e398f48a-6630-455a-b876-fccd0e65848c
⛔ Files ignored due to path filters (1)
plugins/prompts/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
core/bifrost.gocore/bifrost_test.gocore/providers/azure/azure.gocore/providers/mistral/custom_provider_test.gocore/providers/mistral/errors.gocore/providers/mistral/mistral.gocore/providers/mistral/ocr_test.gocore/schemas/bifrost.gocore/schemas/chatcompletions.goframework/modelcatalog/config.goframework/modelcatalog/main.goframework/modelcatalog/pricing.goframework/modelcatalog/pricing_test.goframework/modelcatalog/utils.goframework/oauth2/main.goplugins/prompts/go.modtransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/pricing_integration_test.go
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/providers.go
✅ Files skipped from review due to trivial changes (7)
- core/providers/mistral/ocr_test.go
- plugins/prompts/go.mod
- framework/oauth2/main.go
- framework/modelcatalog/config.go
- framework/modelcatalog/utils.go
- core/schemas/bifrost.go
- transports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- transports/bifrost-http/handlers/config.go
- core/bifrost_test.go
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/lib/config.go
- core/schemas/chatcompletions.go
- transports/bifrost-http/lib/pricing_integration_test.go
- core/providers/mistral/custom_provider_test.go
akshaydeo
force-pushed
the
04-13-exception_fixes
branch
from
e614eb1 to
302f634
Compare
Contributor
Author
Merge activity
|
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
3 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
Fixes a race condition in streaming response handling where transport post-hooks could access recycled fasthttp RequestCtx objects, leading to potential crashes or data corruption. The fix ensures thread-safe execution by capturing request/response data before goroutine execution and using atomic operations for coordination.
Changes
BuildHTTPResponseFromFastHTTPfunction to create response snapshotsrunTransportPostHooksCapturedfunction that operates on pre-captured data instead of live contextThe key design decision was to eagerly capture all needed data from the fasthttp context while it's still valid, then pass this data through closures to avoid any context access after the response stream begins.
Type of change
Affected areas
How to test
Test streaming endpoints with transport plugins enabled to verify no race conditions occur:
Verify that transport plugin logs appear correctly in traces for both streaming and non-streaming responses.
Screenshots/Recordings
N/A - Internal concurrency fix with no UI changes.
Breaking changes
Related issues
Addresses race conditions in streaming response handling where fasthttp context recycling could cause crashes in transport post-hooks.
Security considerations
This fix prevents potential memory corruption and crashes that could occur due to race conditions, improving overall system stability and security.
Checklist
docs/contributing/README.mdand followed the guidelines