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 selected for processing (42)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAsync request user-values are propagated into background async job execution: SubmitJob gained a contextValues parameter; the background worker restores those key/value pairs into a new BifrostContext (and sets the async flag) before running the operation. Call sites, tests, and related streaming/middleware plumbing were updated. Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Handler as Async Handler
participant Executor as AsyncJobExecutor
participant Store as LogStore/DB
participant Worker as Background Worker
participant Ctx as BifrostContext
Client->>Handler: HTTP request (with user values)
Handler->>Executor: SubmitJob(..., contextValues)
Executor->>Store: Persist async job record
Store-->>Executor: Job ID
Executor->>Worker: Schedule background execution (jobID + contextValues)
Worker->>Ctx: New BifrostContext()
Worker->>Ctx: Restore each contextValue via SetValue(k,v)
Worker->>Ctx: Set BifrostIsAsyncRequest = true
Worker->>Worker: Execute operation using restored context
Worker->>Store: Update job status / result / error / TTL
Handler-->>Client: 202 Accepted (job id)
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. |
|
|
Contributor
Confidence Score: 4/5Safe to merge after correcting the three status-code outliers; the core context-propagation and streaming fixes look correct. The context propagation fix and the streaming ctx-recycling fix are both sound. The one remaining P1 is the wrong HTTP status code returned by asyncTextCompletion, asyncRerank, and asyncOCR for SubmitJob client errors — fixing three lines before merge would make this 5/5. transports/bifrost-http/handlers/asyncinference.go — lines 134, 471, and 505 Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (1)
transports/bifrost-http/handlers/middlewares.go (1)
1018-1026:⚠️ Potential issue | 🟠 MajorThe new trace completer signature is not wired into the streaming handler.
This now stores
func([]schemas.PluginLogEntry), buttransports/bifrost-http/integrations/router.gostill doesctx.UserValue(schemas.BifrostContextKeyTraceCompleter).(func())at Line 2318 and invokes it with no args. That assertion will fail, sotraceCompleterstays nil and streaming traces never flush.🤖 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 1018 - 1026, The trace completer stored with ctx.SetUserValue using schemas.BifrostContextKeyTraceCompleter now has the signature func([]schemas.PluginLogEntry) but the streaming handler in transports/bifrost-http/integrations/router.go still type-asserts and calls it as func() causing a panic and never flushing traces; update the router streaming code to type-assert the user value as func([]schemas.PluginLogEntry) (or check both signatures if backward compatibility is needed) and invoke it with the transport/plugin logs available in the streaming path (or pass nil when none) so tracer.AttachPluginLogs and tracer.CompleteAndFlushTrace run as intended.
🧹 Nitpick comments (1)
framework/logstore/asyncjob_test.go (1)
55-55: Usebifrost.Ptr()instead of&for pointer construction.Please replace
strPtr(and direct&valuepatterns) withbifrost.Ptr(...)to stay consistent with repository conventions in tests too.Based on learnings: "In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically... including test utilities."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/asyncjob_test.go` at line 55, The test helper strPtr and any direct pointer constructions using & (e.g., &value) should be replaced with bifrost.Ptr(...) to match repo conventions; remove or replace the strPtr function and update all call sites in asyncjob_test.go (and any other tests) to call bifrost.Ptr("...") or bifrost.Ptr(variable) instead of strPtr(...) or &variable so tests use the canonical pointer helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/asyncjob_test.go`:
- Around line 203-206: Replace the brittle fixed sleep in the test with a
bounded polling loop: use a context with timeout (similar to
waitForJobCompletion) and repeatedly call
executor.logstore.FindAsyncJobByID(ctx, job.ID) until retrievedJob.Status equals
schemas.AsyncJobStatusFailed or the deadline expires, then require.NoError for
the final call and assert the status is Failed; reference the FindAsyncJobByID
call and the waitForJobCompletion pattern to implement the backoff/polling and
timeout behavior.
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 360-390: Move the lookup of the completer slot
(schemas.BifrostContextKeyTransportPostHookCompleter) before allocating
capturedReq/capturedResp so you only snapshot when a slot exists; i.e., check
ctx.UserValue(...).(*atomic.Value) first, return/skip work if nil, and only then
call lib.BuildHTTPRequestFromFastHTTP and lib.BuildHTTPResponseFromFastHTTP and
capture preHookLogs; ensure the ReleaseHTTPRequest/ReleaseHTTPResponse calls
remain in the completer closure (or add immediate releases if you early-return
after allocation) and keep runTransportPostHooksCaptured, completer, and
slot.Store(completer) logic unchanged.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 1018-1026: The trace completer stored with ctx.SetUserValue using
schemas.BifrostContextKeyTraceCompleter now has the signature
func([]schemas.PluginLogEntry) but the streaming handler in
transports/bifrost-http/integrations/router.go still type-asserts and calls it
as func() causing a panic and never flushing traces; update the router streaming
code to type-assert the user value as func([]schemas.PluginLogEntry) (or check
both signatures if backward compatibility is needed) and invoke it with the
transport/plugin logs available in the streaming path (or pass nil when none) so
tracer.AttachPluginLogs and tracer.CompleteAndFlushTrace run as intended.
---
Nitpick comments:
In `@framework/logstore/asyncjob_test.go`:
- Line 55: The test helper strPtr and any direct pointer constructions using &
(e.g., &value) should be replaced with bifrost.Ptr(...) to match repo
conventions; remove or replace the strPtr function and update all call sites in
asyncjob_test.go (and any other tests) to call bifrost.Ptr("...") or
bifrost.Ptr(variable) instead of strPtr(...) or &variable so tests use the
canonical pointer helper.
🪄 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: 621cfce3-a30d-40ff-9e84-3a19bdb25ed9
📒 Files selected for processing (6)
framework/logstore/asyncjob.goframework/logstore/asyncjob_test.gotransports/bifrost-http/handlers/asyncinference.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
7bd6f79 to
577707d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/inference.go`:
- Around line 1699-1711: The post-hook completer (closure-loaded via
completerSlot and invoked as postHookCompleter()) is currently called after the
stream termination and uses reader.SendError to emit errors, which can send
events after the terminal “[DONE]” marker; move the post-hook invocation so it
runs before emitting the final marker (i.e., call the loaded postHookCompleter
and handle its logs/errors prior to the code that writes the final
“[DONE]”/stream-close) and if you cannot move it ensure you do not call
reader.SendError after termination — instead log the error server-side (e.g.,
processLogger or append to transportLogs) so post-hook failures are recorded
without sending late SSE via reader.SendError.
- Around line 1671-1676: The spawned goroutine must not assume completerSlot is
already set; change the goroutine started in the handler to wait for
completerSlot.Load() to return a non-nil value (poll with a short sleep/backoff
and a bounded timeout) before invoking the completer, and log or handle the
timeout case so post-hooks aren’t silently skipped; reference the completerSlot
atomic.Value placed under schemas.BifrostContextKeyTransportPostHookCompleter
and the TransportInterceptorMiddleware behavior (which calls
slot.Store(completer) asynchronously) to locate where to add the wait/poll and
timeout handling.
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 1024-1028: The new callback stored under
schemas.BifrostContextKeyTraceCompleter has been changed to type
func([]schemas.PluginLogEntry) but the router-side consumer still asserts and
calls it as func() — update that consumer to assert the new signature and invoke
it with the transport plugin logs; specifically, locate the code that does a
ctx.Value(schemas.BifrostContextKeyTraceCompleter).(func()) (in the router
integration) and change it to assert func([]schemas.PluginLogEntry) and pass the
relevant transportLogs slice when calling so traces are completed/flushed
correctly.
- Around line 469-489: The cloned request used for post-hooks (created via
schemas.AcquireHTTPRequest and assigned to req) doesn't copy the request
payload, so post-hooks using runTransportPostHooks see an empty Body; update the
clone logic that copies capturedReq (the block that sets req.Method, req.Path,
req.Headers, req.Query, req.PathParams) to also copy the body: if
capturedReq.Body is non-nil, produce a safe clone (e.g., duplicate the byte
slice or buffer contents into a new reader/byte slice) and assign it to req.Body
so mutations by hooks won't affect the original, and ensure any
ownership/cleanup expectations of schemas.ReleaseHTTPRequest still hold.
🪄 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: e59c9514-7bd7-4c31-a068-b9ac38834019
📒 Files selected for processing (35)
core/changelog.mdcore/versionframework/changelog.mdframework/logstore/asyncjob.goframework/logstore/asyncjob_test.goframework/versionplugins/compat/changelog.mdplugins/compat/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/litellmcompat/changelog.mdplugins/litellmcompat/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/prompts/changelog.mdplugins/prompts/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.mdtransports/version
✅ Files skipped from review due to trivial changes (29)
- framework/version
- plugins/litellmcompat/changelog.md
- plugins/logging/changelog.md
- plugins/maxim/version
- core/version
- plugins/mocker/version
- plugins/semanticcache/changelog.md
- plugins/governance/changelog.md
- plugins/otel/changelog.md
- plugins/telemetry/changelog.md
- plugins/prompts/changelog.md
- plugins/governance/version
- plugins/prompts/version
- plugins/jsonparser/version
- plugins/logging/version
- plugins/jsonparser/changelog.md
- plugins/otel/version
- transports/version
- plugins/compat/version
- plugins/semanticcache/version
- plugins/litellmcompat/version
- plugins/mocker/changelog.md
- framework/changelog.md
- plugins/telemetry/version
- transports/changelog.md
- core/changelog.md
- plugins/compat/changelog.md
- plugins/maxim/changelog.md
- framework/logstore/asyncjob_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/handlers/asyncinference.go
- framework/logstore/asyncjob.go
akshaydeo
force-pushed
the
04-13-exception_fixes
branch
from
577707d to
cc97b6d
Compare
Contributor
Author
|
@coderabbitai review Pushed fixes for the 4 unresolved threads from the previous review:
Please re-review and resolve the threads if satisfied. |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/inference.go (1)
1704-1729:⚠️ Potential issue | 🟠 MajorOnly mark the completer as executed after you've actually obtained it.
On Line 1708,
completerRanis set beforecompleterSlot.Load()succeeds. If the 100ms wait expires and Line 1724 returns withloaded == nil, the deferredrunCompleter(false)path becomes a no-op, so transport post-hooks and their trace logs are still silently skipped on the timeout path. It also leaves the capturedHTTPRequest/HTTPResponsesnapshots allocated intransports/bifrost-http/handlers/middlewares.gounreleased, since theirRelease*calls only happen inside the completer closure.Suggested fix
runCompleter := func(sendSSEOnError bool) { if completerRan { return } - completerRan = true // Bounded wait for TransportInterceptorMiddleware to publish the completer. var loaded any deadline := time.Now().Add(100 * time.Millisecond) for { if loaded = completerSlot.Load(); loaded != nil { @@ if loaded == nil { return } postHookCompleter, ok := loaded.(func() ([]schemas.PluginLogEntry, error)) if !ok { return } + completerRan = true logs, err := postHookCompleter()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/inference.go` around lines 1704 - 1729, Move the completerRan = true assignment so it only happens after you successfully load and type-assert the completer from completerSlot; specifically, in runCompleter, do not set completerRan before calling completerSlot.Load() and validating loaded != nil and ok == true for postHookCompleter, instead set completerRan = true immediately after the type assertion succeeds (i.e., after confirming postHookCompleter is a valid func() ([]schemas.PluginLogEntry, error)) so the deferred runCompleter(false) path still runs the completer on timeout and releases any captured HTTPRequest/HTTPResponse snapshots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations_test.go`:
- Around line 1169-1189: The TestTriggerMigrations_Idempotent test opens sqlite
":memory:" directly which can create multiple isolated in-memory DBs when
connections are reset by triggerMigrations (it calls SetMaxOpenConns(0));
replace the direct gorm.Open setup with the helper setupFullMigrationDB(t) so
the test uses a single pinned connection for all operations, invoke
triggerMigrations(ctx, db) twice as before, and keep the same
db.Migrator().HasTable(...) assertions against that db to verify idempotency.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 1704-1729: Move the completerRan = true assignment so it only
happens after you successfully load and type-assert the completer from
completerSlot; specifically, in runCompleter, do not set completerRan before
calling completerSlot.Load() and validating loaded != nil and ok == true for
postHookCompleter, instead set completerRan = true immediately after the type
assertion succeeds (i.e., after confirming postHookCompleter is a valid func()
([]schemas.PluginLogEntry, error)) so the deferred runCompleter(false) path
still runs the completer on timeout and releases any captured
HTTPRequest/HTTPResponse snapshots.
🪄 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: 180cfc36-677d-4ffd-b5fb-3b9077b1fa50
📒 Files selected for processing (37)
core/changelog.mdcore/schemas/bifrost.gocore/versionframework/changelog.mdframework/configstore/migrations_test.goframework/logstore/asyncjob.goframework/logstore/asyncjob_test.goframework/versionplugins/compat/changelog.mdplugins/compat/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/litellmcompat/changelog.mdplugins/litellmcompat/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/prompts/changelog.mdplugins/prompts/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.mdtransports/version
✅ Files skipped from review due to trivial changes (27)
- plugins/telemetry/changelog.md
- plugins/semanticcache/changelog.md
- plugins/mocker/changelog.md
- plugins/jsonparser/changelog.md
- plugins/maxim/version
- plugins/governance/changelog.md
- plugins/mocker/version
- plugins/jsonparser/version
- plugins/prompts/changelog.md
- plugins/logging/version
- framework/version
- plugins/logging/changelog.md
- plugins/compat/version
- framework/changelog.md
- plugins/otel/version
- plugins/prompts/version
- plugins/maxim/changelog.md
- plugins/otel/changelog.md
- transports/version
- plugins/compat/changelog.md
- plugins/litellmcompat/version
- plugins/governance/version
- plugins/telemetry/version
- plugins/litellmcompat/changelog.md
- transports/changelog.md
- framework/logstore/asyncjob_test.go
- plugins/semanticcache/version
🚧 Files skipped from review as they are similar to previous changes (5)
- core/version
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/handlers/asyncinference.go
- framework/logstore/asyncjob.go
- core/changelog.md
akshaydeo
force-pushed
the
04-13-exception_fixes
branch
from
cc97b6d to
75cc3f2
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 (1)
transports/bifrost-http/integrations/router.go (1)
2317-2345:⚠️ Potential issue | 🟠 MajorRun the captured transport post-hooks before completing the trace.
transports/bifrost-http/handlers/middlewares.go:360-405now stores a deferred transport-post-hook completer for streaming responses, but this goroutine still hardcodestraceCompleter(nil). GenericRouter streams will therefore complete traces without ever invoking the captured post-hooks or attaching their plugin logs. This path should mirror the async streaming handler: run the stored completer in the goroutine and pass its 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 - 2345, The goroutine currently calls traceCompleter(nil) and thus skips the deferred transport post-hook completer stored by the middleware; retrieve the stored transport-post-hook completer from the request context (the same UserValue key used by transports/bifrost-http/handlers/middlewares.go), run that completer inside the goroutine before completing the trace, capture its returned plugin logs and pass them into traceCompleter (i.e. call transportPostHookCompleter(...) to get []schemas.PluginLogEntry and then call traceCompleter(logs)); update the defer block where traceCompleter(nil) is invoked (near reader.Done, schemas.ReleaseHTTPRequest) to run the transport completer and forward its logs to traceCompleter so plugin logs are attached to the trace.
♻️ Duplicate comments (1)
framework/configstore/migrations_test.go (1)
1169-1189:⚠️ Potential issue | 🟠 MajorReuse
setupFullMigrationDB()in the idempotency test.This test still opens
:memory:directly, so oncetriggerMigrations()resets SQLite’s pool settings, the second run and theHasTableassertions can land on a different empty in-memory database. That makes the idempotency check flaky and can hide real migration regressions.💡 Suggested fix
func TestTriggerMigrations_Idempotent(t *testing.T) { - db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{ - Logger: logger.Default.LogMode(logger.Silent), - }) - require.NoError(t, err) - + _, db := setupFullMigrationDB(t) ctx := context.Background() - // First run - err = triggerMigrations(ctx, db) - require.NoError(t, err, "first triggerMigrations should succeed") - - // Second run – must be a no-op, not an error. - err = triggerMigrations(ctx, db) + // Second run – must be a no-op, not an error. + err := triggerMigrations(ctx, db) require.NoError(t, err, "second triggerMigrations should succeed (idempotent)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations_test.go` around lines 1169 - 1189, Replace the direct in-memory sqlite gorm.Open in TestTriggerMigrations_Idempotent with the shared helper setupFullMigrationDB() so the test reuses the same DB connection/pool used by other migration tests; specifically, remove the sqlite.Open(":memory:")/gorm.Open block and call setupFullMigrationDB(ctx) to obtain db (and handle its returned error) before calling triggerMigrations(ctx, db) twice and running the HasTable assertions on tables.TableProvider, tables.TableKey, and tables.TableVirtualKey.
🧹 Nitpick comments (1)
framework/logstore/asyncjob_test.go (1)
85-114: Add a post-submit mutation case forcontextValues.This only proves propagation of a stable map literal. It won’t catch a bug where
SubmitJobretains the caller’scontextValuesmap by reference, which would let later request-context mutations leak the wrong virtual key or tracing metadata into the background job. Add a case that mutates the original map afterSubmitJobreturns and assert the async callback still sees the original values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/asyncjob_test.go` around lines 85 - 114, Add a post-submit mutation test to ensure SubmitJob copies contextValues rather than retaining the map by reference: after calling executor.SubmitJob in TestSubmitJob_PropagatesContextValues, mutate the original contextValues map (e.g., change the value for schemas.BifrostContextKeyVirtualKey and add/modify other keys) immediately after SubmitJob returns, then waitForJobCompletion and assert that the capturedCtx (from the operation closure) still sees the original values (referencing capturedCtx, executor.SubmitJob, waitForJobCompletion, and done) to verify SubmitJob made a defensive 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 `@framework/changelog.md`:
- Around line 1-4: Update the changelog bullets to explicitly list the PR’s
primary fixes: add an entry for "fix: restore async context values correctly
(async context value restoration)", an entry for "fix: make streaming
fasthttp.RequestCtx snapshots safe (streaming RequestCtx snapshot safety)", and
an entry for "change: trace completion callback now receives (context, error) or
updated input signature (trace completion callback input change)"; place these
lines alongside the existing bullets (refer to the current changelog entries and
add the three new lines so release notes accurately reflect fixes to async
context restoration, fasthttp.RequestCtx snapshot safety, and the trace
completion callback input change).
In `@plugins/prompts/version`:
- Line 1: The version file for the prompts plugin was bumped to 1.0.2 without
any corresponding code changes; either revert the file back to 1.0.1 or include
the actual functional changes and tests that justify the bump. If you choose to
keep 1.0.2, update the prompts plugin source (e.g., main.go) and any relevant
tests (helpers_test.go, plugin_test.go) to reflect the new behavior, and
add/update a brief changelog entry; if you revert, change the version file back
to 1.0.1 and ensure no other references to 1.0.2 remain.
In `@transports/bifrost-http/handlers/inference.go`:
- Around line 1704-1726: The issue is that completerRan is set to true before we
actually acquire the completer from completerSlot, which can make the fallback a
no-op if the slot remains empty; in runCompleter change the logic so you only
set completerRan = true after successfully loading a non-nil value from
completerSlot (i.e., move the completerRan assignment to immediately after the
loaded != nil check/return), keeping the existing 100ms wait loop and behavior
around sendSSEOnError; reference completerRan, runCompleter, completerSlot.Load
and sendSSEOnError when making the change.
In `@transports/bifrost-http/integrations/router.go`:
- Line 1507: Currently bifrostCtx.GetUserValues() passes the entire user-values
map into executor.SubmitJob(vkValue, resultTTL, operation, operationType,
bifrostCtx.GetUserValues()), which lets transport-internal fasthttp keys (e.g.,
FastHTTPUserValueBifrostContext, FastHTTPUserValueBifrostCancel,
FastHTTPUserValueLargeResponseMode) propagate into executeJob(); instead, build
and pass a filtered allowlist map containing only safe values (e.g., virtual key
and tracing headers) before calling executor.SubmitJob, and update the call-site
so executeJob receives only that allowlisted map rather than the full
bifrostCtx.GetUserValues() output.
---
Outside diff comments:
In `@transports/bifrost-http/integrations/router.go`:
- Around line 2317-2345: The goroutine currently calls traceCompleter(nil) and
thus skips the deferred transport post-hook completer stored by the middleware;
retrieve the stored transport-post-hook completer from the request context (the
same UserValue key used by transports/bifrost-http/handlers/middlewares.go), run
that completer inside the goroutine before completing the trace, capture its
returned plugin logs and pass them into traceCompleter (i.e. call
transportPostHookCompleter(...) to get []schemas.PluginLogEntry and then call
traceCompleter(logs)); update the defer block where traceCompleter(nil) is
invoked (near reader.Done, schemas.ReleaseHTTPRequest) to run the transport
completer and forward its logs to traceCompleter so plugin logs are attached to
the trace.
---
Duplicate comments:
In `@framework/configstore/migrations_test.go`:
- Around line 1169-1189: Replace the direct in-memory sqlite gorm.Open in
TestTriggerMigrations_Idempotent with the shared helper setupFullMigrationDB()
so the test reuses the same DB connection/pool used by other migration tests;
specifically, remove the sqlite.Open(":memory:")/gorm.Open block and call
setupFullMigrationDB(ctx) to obtain db (and handle its returned error) before
calling triggerMigrations(ctx, db) twice and running the HasTable assertions on
tables.TableProvider, tables.TableKey, and tables.TableVirtualKey.
---
Nitpick comments:
In `@framework/logstore/asyncjob_test.go`:
- Around line 85-114: Add a post-submit mutation test to ensure SubmitJob copies
contextValues rather than retaining the map by reference: after calling
executor.SubmitJob in TestSubmitJob_PropagatesContextValues, mutate the original
contextValues map (e.g., change the value for
schemas.BifrostContextKeyVirtualKey and add/modify other keys) immediately after
SubmitJob returns, then waitForJobCompletion and assert that the capturedCtx
(from the operation closure) still sees the original values (referencing
capturedCtx, executor.SubmitJob, waitForJobCompletion, and done) to verify
SubmitJob made a defensive 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: 3a0826b9-90f3-4bc2-b273-5a303ea0dc64
📒 Files selected for processing (37)
core/changelog.mdcore/schemas/bifrost.gocore/versionframework/changelog.mdframework/configstore/migrations_test.goframework/logstore/asyncjob.goframework/logstore/asyncjob_test.goframework/versionplugins/compat/changelog.mdplugins/compat/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/litellmcompat/changelog.mdplugins/litellmcompat/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/prompts/changelog.mdplugins/prompts/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.mdtransports/version
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
framework/configstore/migrations_test.go (1)
1166-1186:⚠️ Potential issue | 🟠 MajorUse
setupFullMigrationDBin idempotency test to avoid SQLite:memory:connection flakiness.Line 1167 reintroduces direct
sqlite.Open(":memory:"). This can make the second migration/assertion path non-deterministic when connection settings change during migration flow. Please reuse the shared-cache helper here.Suggested fix
func TestTriggerMigrations_Idempotent(t *testing.T) { - db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{ - Logger: logger.Default.LogMode(logger.Silent), - }) - require.NoError(t, err) - + _, db := setupFullMigrationDB(t) ctx := context.Background() - // First run - err = triggerMigrations(ctx, db) - require.NoError(t, err, "first triggerMigrations should succeed") - - // Second run – must be a no-op, not an error. - err = triggerMigrations(ctx, db) + // setupFullMigrationDB already performs the first run. + // Run once more to verify idempotency. + err := triggerMigrations(ctx, db) require.NoError(t, err, "second triggerMigrations should succeed (idempotent)") // Tables should still be intact assert.True(t, db.Migrator().HasTable(&tables.TableProvider{}), "TableProvider should still exist")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations_test.go` around lines 1166 - 1186, The test TestTriggerMigrations_Idempotent currently opens a raw in-memory SQLite via sqlite.Open(":memory:") which can be flaky; replace that setup with the shared helper setupFullMigrationDB so the test uses the stable shared-cache DB. Call setupFullMigrationDB to obtain the *gorm.DB (and context if returned), check for errors with require.NoError, then run triggerMigrations twice and assert the same tables (tables.TableProvider, tables.TableKey, tables.TableVirtualKey) still exist; keep the existing assertions and messages but remove the direct sqlite.Open usage.transports/bifrost-http/handlers/asyncinference.go (1)
124-132:⚠️ Potential issue | 🟠 MajorApply the same async-context allowlist here instead of
GetUserValues().All of these handlers now pass the full request-scoped user-values map into background execution. That replays the same transient transport state as the router path. Please route these through a shared allowlist helper so every async endpoint propagates only the intended keys.
🧩 Suggested direction
- bifrostCtx.GetUserValues(), + buildAsyncContextValues(bifrostCtx),Based on learnings, governance context keys in Go propagation are limited to VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys, and PluginName.
Also applies to: 163-171, 202-210, 237-245, 276-284, 315-323, 354-362, 393-401, 427-435, 461-469, 495-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/asyncinference.go` around lines 124 - 132, Replace the direct use of bifrostCtx.GetUserValues() when calling h.executor.SubmitJob with the shared async-context allowlist helper so only allowed governance keys are propagated; specifically, change the SubmitJob call site that passes bifrostCtx.GetUserValues() (the call that wraps h.client.TextCompletionRequest) to instead pass the result of the allowlist helper (e.g., ApplyAsyncContextAllowlist or similar), ensuring the helper filters for only VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys and PluginName before handing values to SubmitJob; do the same replacement for the other similar SubmitJob sites mentioned.transports/bifrost-http/integrations/router.go (1)
1507-1507:⚠️ Potential issue | 🟠 MajorDon't pass the full request user-values map into async jobs.
GetUserValues()is broader than the async worker should inherit. Passing it through verbatim here can keep transport-only callbacks/pointers alive after the request ends. Please build an explicit allowlist snapshot for async-safe keys and pass that instead.🧩 Suggested direction
- job, err := executor.SubmitJob(vkValue, resultTTL, operation, operationType, bifrostCtx.GetUserValues()) + job, err := executor.SubmitJob(vkValue, resultTTL, operation, operationType, buildAsyncContextValues(bifrostCtx))Based on learnings, governance context keys in Go propagation are limited to VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys, and PluginName.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/integrations/router.go` at line 1507, The code currently passes the entire request user-values map from bifrostCtx.GetUserValues() into executor.SubmitJob which can leak transport-only callbacks; instead, create an explicit allowlist snapshot map containing only the async-safe governance keys (VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys, PluginName) pulled from bifrostCtx.GetUserValues(), and pass that snapshot to executor.SubmitJob (replace the GetUserValues() argument with your new map) so only those whitelisted values are propagated to the async worker.
🧹 Nitpick comments (1)
framework/logstore/asyncjob.go (1)
114-126: SnapshotcontextValuesbefore starting the worker.
SubmitJobpasses the caller's map reference straight into a goroutine. If a future caller mutates or reuses that map afterSubmitJobreturns,executeJobcan race while iterating it. Take a shallow copy here so the executor owns an immutable snapshot.♻️ Suggested change
- go e.executeJob(job.ID, job.ResultTTL, operation, contextValues) + var contextValuesSnapshot map[any]any + if len(contextValues) > 0 { + contextValuesSnapshot = make(map[any]any, len(contextValues)) + for k, v := range contextValues { + contextValuesSnapshot[k] = v + } + } + + go e.executeJob(job.ID, job.ResultTTL, operation, contextValuesSnapshot)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/asyncjob.go` around lines 114 - 126, SubmitJob is passing the caller's contextValues map by reference into a goroutine, causing a race if the caller mutates the map; to fix, take a shallow copy of the contextValues map before spawning the worker (i.e., before the go e.executeJob(...) call) so the background worker owns an immutable snapshot; create a new map[any]any, copy all k,v pairs from the original contextValues into it, and pass that copy into executeJob (alternatively, perform the same shallow-copy at the start of executeJob) to eliminate concurrent iteration/mutation races.
🤖 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 500-505: The error wrap introduced in the HTTPTransportPostHook
error path uses an uppercase prefix; update the fmt.Errorf call in the post-hook
error branch so the new prefix is lowercase (e.g., "httpTransportPostHook plugin
%s: %w" or similar lowercase wording) to follow Go error string
conventions—locate the error construction near HTTPTransportPostHook handling
where pluginName, bifrostCtx.DrainPluginLogs(), and allLogs are used and replace
the current fmt.Errorf("HTTPTransportPostHook plugin %s: %w", pluginName, err)
with the same text but starting lowercase.
- Around line 382-390: Fix two issues in runTransportPostHooksCaptured: 1)
change the error construction string to start lowercase (e.g., replace
fmt.Errorf("HTTPTransportPostHook plugin %s: %w", ...) with a lowercase prefix
like "transport post-hook plugin %s: %w") so it follows Go error-string
conventions; 2) avoid delegating to the recycled parent context from the
deferred completer by snapshotting any user/context values needed by post-hooks
at defer time (capture them into local variables such as preHookLogs and any
specific keys), or create a fresh context without the original parent (e.g., use
context.Background() with copied values) before calling
runTransportPostHooksCaptured from the completer, and add a short comment near
completer/runTransportPostHooksCaptured/WithPluginScope noting that plugins must
not rely on values from the recycled request context if you choose to document
instead of snapshotting.
---
Duplicate comments:
In `@framework/configstore/migrations_test.go`:
- Around line 1166-1186: The test TestTriggerMigrations_Idempotent currently
opens a raw in-memory SQLite via sqlite.Open(":memory:") which can be flaky;
replace that setup with the shared helper setupFullMigrationDB so the test uses
the stable shared-cache DB. Call setupFullMigrationDB to obtain the *gorm.DB
(and context if returned), check for errors with require.NoError, then run
triggerMigrations twice and assert the same tables (tables.TableProvider,
tables.TableKey, tables.TableVirtualKey) still exist; keep the existing
assertions and messages but remove the direct sqlite.Open usage.
In `@transports/bifrost-http/handlers/asyncinference.go`:
- Around line 124-132: Replace the direct use of bifrostCtx.GetUserValues() when
calling h.executor.SubmitJob with the shared async-context allowlist helper so
only allowed governance keys are propagated; specifically, change the SubmitJob
call site that passes bifrostCtx.GetUserValues() (the call that wraps
h.client.TextCompletionRequest) to instead pass the result of the allowlist
helper (e.g., ApplyAsyncContextAllowlist or similar), ensuring the helper
filters for only VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys
and PluginName before handing values to SubmitJob; do the same replacement for
the other similar SubmitJob sites mentioned.
In `@transports/bifrost-http/integrations/router.go`:
- Line 1507: The code currently passes the entire request user-values map from
bifrostCtx.GetUserValues() into executor.SubmitJob which can leak transport-only
callbacks; instead, create an explicit allowlist snapshot map containing only
the async-safe governance keys (VirtualKey, Team, Customer, User, RoutingRule,
IncludeOnlyKeys, PluginName) pulled from bifrostCtx.GetUserValues(), and pass
that snapshot to executor.SubmitJob (replace the GetUserValues() argument with
your new map) so only those whitelisted values are propagated to the async
worker.
---
Nitpick comments:
In `@framework/logstore/asyncjob.go`:
- Around line 114-126: SubmitJob is passing the caller's contextValues map by
reference into a goroutine, causing a race if the caller mutates the map; to
fix, take a shallow copy of the contextValues map before spawning the worker
(i.e., before the go e.executeJob(...) call) so the background worker owns an
immutable snapshot; create a new map[any]any, copy all k,v pairs from the
original contextValues into it, and pass that copy into executeJob
(alternatively, perform the same shallow-copy at the start of executeJob) to
eliminate concurrent iteration/mutation races.
🪄 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: 23f19cd8-a531-4f45-9b9e-59bfd2491baf
📒 Files selected for processing (38)
core/changelog.mdcore/schemas/bifrost.gocore/versionframework/changelog.mdframework/configstore/migrations_test.goframework/configstore/rdb_test.goframework/logstore/asyncjob.goframework/logstore/asyncjob_test.goframework/versionplugins/compat/changelog.mdplugins/compat/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/litellmcompat/changelog.mdplugins/litellmcompat/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/prompts/changelog.mdplugins/prompts/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.mdtransports/version
✅ Files skipped from review due to trivial changes (29)
- plugins/maxim/version
- plugins/logging/changelog.md
- plugins/otel/version
- plugins/semanticcache/changelog.md
- plugins/logging/version
- plugins/maxim/changelog.md
- plugins/compat/version
- plugins/mocker/version
- plugins/telemetry/changelog.md
- plugins/prompts/version
- plugins/litellmcompat/changelog.md
- framework/changelog.md
- plugins/governance/changelog.md
- plugins/jsonparser/changelog.md
- plugins/telemetry/version
- plugins/governance/version
- framework/version
- plugins/litellmcompat/version
- plugins/jsonparser/version
- core/changelog.md
- core/version
- plugins/otel/changelog.md
- plugins/compat/changelog.md
- plugins/prompts/changelog.md
- transports/version
- plugins/semanticcache/version
- transports/changelog.md
- plugins/mocker/changelog.md
- framework/logstore/asyncjob_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/schemas/bifrost.go
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/handlers/inference.go
akshaydeo
force-pushed
the
04-13-exception_fixes
branch
from
75cc3f2 to
9a7503d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/configstore/migrations_test.go (1)
1166-1186:⚠️ Potential issue | 🟠 MajorReuse the shared-cache helper here.
Line 1167 still opens SQLite with
:memory:directly. AftertriggerMigrations()relaxes the pool settings, the rerun andHasTablechecks can land on a different empty in-memory database, so this idempotency test stays flaky.Suggested change
func TestTriggerMigrations_Idempotent(t *testing.T) { - db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{ - Logger: logger.Default.LogMode(logger.Silent), - }) - require.NoError(t, err) - + _, db := setupFullMigrationDB(t) ctx := context.Background() - // First run - err = triggerMigrations(ctx, db) - require.NoError(t, err, "first triggerMigrations should succeed") - - // Second run – must be a no-op, not an error. - err = triggerMigrations(ctx, db) - require.NoError(t, err, "second triggerMigrations should succeed (idempotent)") + // Re-run on the same shared in-memory DB. + err := triggerMigrations(ctx, db) + require.NoError(t, err, "triggerMigrations should succeed when rerun") // Tables should still be intact assert.True(t, db.Migrator().HasTable(&tables.TableProvider{}), "TableProvider should still exist") assert.True(t, db.Migrator().HasTable(&tables.TableKey{}), "TableKey should still exist")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations_test.go` around lines 1166 - 1186, TestTriggerMigrations_Idempotent opens SQLite with ":memory:" which can produce different in-memory databases across connections; replace that direct sqlite.Open(":memory:") usage with the shared-cache helper used elsewhere for tests (the helper that returns a gorm DB configured for shared-cache in-memory DB) so both triggerMigrations(ctx, db) runs and subsequent db.Migrator().HasTable checks operate on the same in-memory database; update the DB creation in TestTriggerMigrations_Idempotent to call that shared-cache helper instead of sqlite.Open(":memory:") and keep the rest of the test the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations_test.go`:
- Around line 1978-2003: Add an assertion that the legacy column
enable_litellm_fallbacks was removed after migration: after the existing mgr :=
db.Migrator() block (or before verifying row contents) call mgr.HasColumn for
the config client struct (tables.TableClientConfig) and assert it returns false
for "enable_litellm_fallbacks" (or otherwise query the DB schema to ensure that
column is absent) so the test fails if the old column remains.
---
Duplicate comments:
In `@framework/configstore/migrations_test.go`:
- Around line 1166-1186: TestTriggerMigrations_Idempotent opens SQLite with
":memory:" which can produce different in-memory databases across connections;
replace that direct sqlite.Open(":memory:") usage with the shared-cache helper
used elsewhere for tests (the helper that returns a gorm DB configured for
shared-cache in-memory DB) so both triggerMigrations(ctx, db) runs and
subsequent db.Migrator().HasTable checks operate on the same in-memory database;
update the DB creation in TestTriggerMigrations_Idempotent to call that
shared-cache helper instead of sqlite.Open(":memory:") and keep the rest of the
test the same.
🪄 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: fd519438-2bec-43f2-969b-682fce68204f
📒 Files selected for processing (39)
.github/workflows/scripts/run-migration-tests.shcore/changelog.mdcore/schemas/bifrost.gocore/versionframework/changelog.mdframework/configstore/migrations_test.goframework/configstore/rdb_test.goframework/logstore/asyncjob.goframework/logstore/asyncjob_test.goframework/versionplugins/compat/changelog.mdplugins/compat/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/litellmcompat/changelog.mdplugins/litellmcompat/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/prompts/changelog.mdplugins/prompts/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/handlers/asyncinference.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/ctx.gotransports/changelog.mdtransports/version
✅ Files skipped from review due to trivial changes (26)
- plugins/otel/changelog.md
- plugins/maxim/version
- plugins/jsonparser/version
- plugins/compat/version
- plugins/mocker/version
- plugins/litellmcompat/version
- plugins/prompts/changelog.md
- plugins/governance/changelog.md
- plugins/governance/version
- plugins/telemetry/version
- plugins/semanticcache/version
- plugins/logging/changelog.md
- plugins/jsonparser/changelog.md
- plugins/mocker/changelog.md
- plugins/prompts/version
- plugins/litellmcompat/changelog.md
- plugins/telemetry/changelog.md
- plugins/otel/version
- plugins/logging/version
- plugins/semanticcache/changelog.md
- core/changelog.md
- plugins/compat/changelog.md
- plugins/maxim/changelog.md
- framework/changelog.md
- framework/logstore/asyncjob_test.go
- transports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (9)
- core/version
- framework/version
- transports/version
- framework/configstore/rdb_test.go
- core/schemas/bifrost.go
- transports/changelog.md
- transports/bifrost-http/handlers/asyncinference.go
- framework/logstore/asyncjob.go
- transports/bifrost-http/handlers/middlewares.go
akshaydeo
force-pushed
the
04-13-exception_fixes
branch
from
9a7503d to
b208b52
Compare
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
Fixes context propagation for async job execution by ensuring that original request context values (virtual keys, tracing headers, etc.) are properly restored in background goroutines. This resolves issues where async operations would lose important request metadata needed for proper authentication, tracing, and plugin execution.
Changes
AsyncJobExecutor.SubmitJob()to accept acontextValuesparameter containing the original request's BifrostContext user valuesexecuteJob()to restore these context values on the background execution context before running the operationrunTransportPostHooksCaptured()for goroutine-safe plugin executionBuildHTTPResponseFromFastHTTP()utility function for creating response snapshotsType of change
Affected areas
How to test
Validate that async operations properly inherit request context:
Test async endpoints with virtual keys and custom headers to ensure context values are preserved in background execution.
Screenshots/Recordings
N/A
Breaking changes
Related issues
Addresses context loss in async job execution that could cause authentication failures and incomplete tracing data.
Security considerations
Improves security by ensuring virtual key validation and authentication context is properly maintained in async operations. No new security risks introduced as this only preserves existing context data.
Checklist
docs/contributing/README.mdand followed the guidelines