Skip to content

fix: make tracing span lookup nil-safe to prevent panic on streaming errors - #4896

Merged
akshaydeo merged 2 commits into
maximhq:devfrom
eyeveil:fix/3455-bedrock-nil-deref
Jul 12, 2026
Merged

akshaydeo merged 2 commits into
maximhq:devfrom
eyeveil:fix/3455-bedrock-nil-deref

Conversation

@eyeveil

@eyeveil eyeveil commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Problem

A Bedrock streaming error can panic the whole Bifrost process while finalizing tracing spans, instead of forwarding the stream error to the caller (SIGSEGV in schemas.(*Trace).GetSpan, reported in production while streaming Anthropic models via Bedrock).

Root cause

The streaming error path finalizes deferred tracing spans and sets span attributes through Trace.GetSpan. The trace/span helpers assumed the trace receiver and every entry in Trace.Spans are non-nil. When trace cleanup/reuse leaves a nil span entry (or a nil span/trace reaches the finalizer, as visible in the reported stack where completeDeferredSpan receives a nil argument), the tracing code dereferences nil and crashes before the stream error is delivered.

Fix

Make the trace/span helpers defensive no-ops for nil data, keeping tracing best-effort:

  • Trace.GetSpan returns nil for nil traces, empty span IDs, and skips nil span entries.
  • Trace.AddSpan ignores nil traces/spans.
  • Span.SetAttribute, Span.AddEvent, and Span.End ignore nil receivers.

Tracing edge cases can no longer crash provider streaming error handling; the stream error is forwarded gracefully.

Testing

  • Added offline unit tests: nil-safe GetSpan (nil trace, nil span entry, hit/miss) and nil-receiver mutators no-op instead of panicking — pass.
  • CGO_ENABLED=0 go build ./... and go vet in core/ — pass.
  • Note: go test ./schemas has one pre-existing failure (TestResponsesMessageToolCallArguments/real_tool_search_call_frames_from_openai) that reproduces identically on a clean dev checkout (6484317); unrelated to this change.

Fixes #3455

…eaming error panic

A Bedrock streaming error finalizer could hit a nil span entry (or nil
receiver) in the tracing path (completeDeferredSpan -> Trace.GetSpan),
panicking the process instead of forwarding the stream error.

Affected packages:
- core/schemas/trace.go

Fixes maximhq#3455

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

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5914f1f9-3bbd-4890-8412-a6a663fc914d

📥 Commits

Reviewing files that changed from the base of the PR and between 9324451 and 38259c5.

📒 Files selected for processing (1)
  • core/schemas/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/schemas/trace_test.go

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved trace and span handling with defensive nil-safety to prevent panics.
    • Span lookups now return no result for nil traces, empty span IDs, or missing spans.
    • Common span actions now behave as safe no-ops when invoked with invalid/uninitialized receivers.
  • Tests
    • Added unit tests covering nil-safe trace/span accessors and mutators.

Walkthrough

Trace and Span helpers now return early on nil receivers and invalid inputs, and new unit tests cover the nil-safe behavior for span lookup and mutators.

Changes

Nil-safety fixes for Trace and Span

Layer / File(s) Summary
Nil-safe guards on Trace and Span methods
core/schemas/trace.go
AddSpan, GetSpan, SetAttribute, AddEvent, and End now return early on nil receivers and invalid inputs before locking or mutating state.
Tests for nil-safe behavior
core/schemas/trace_test.go
New tests verify GetSpan nil handling and confirm nil Trace/Span mutators are safe no-ops.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: danpiths, akshaydeo

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main fix: making tracing span lookup nil-safe to avoid streaming panics.
Description check ✅ Passed The description covers the problem, root cause, fix, and testing, even though several template sections are omitted.
Linked Issues check ✅ Passed The changes address #3455 by preventing nil dereferences in Core tracing helpers so Bedrock streaming errors can be forwarded safely.
Out of Scope Changes check ✅ Passed The diff stays within the tracing nil-safety fix and added tests, with no unrelated changes visible.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


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

❤️ Share

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

@coderabbitai
coderabbitai Bot requested review from akshaydeo and danpiths July 3, 2026 23:56
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 3, 2026
@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Safe to merge — the fix is narrowly scoped to nil-guard helpers in the tracing schema, directly addresses the reported production SIGSEGV, and is covered by new tests.

The crash path (nil span entry in Trace.Spans dereferenced inside GetSpan during streaming error finalization) is correctly fixed. The remaining Trace methods without nil-receiver guards (SetAttribute, GetAttribute, AppendPluginLogs, etc.) are all called exclusively through TraceStore, which already guards against nil before dispatching — so there is no unprotected path left on the streaming error route. Tests cover all four new guards including the nil-entry iteration case that triggered the production crash.

No files require special attention — both changed files are straightforward and the fix is well-contained.

Important Files Changed

Filename Overview
core/schemas/trace.go Nil-safety guards added to GetSpan (nil trace, empty spanID, nil slice entries), AddSpan (nil trace or span), and Span mutators (nil receiver). All other Trace methods remain unguarded but are reached exclusively through TraceStore, which already checks for nil before dispatching.
core/schemas/trace_test.go New test file covering nil-trace GetSpan, empty spanID, nil span entry in slice, hit/miss on a valid span, nil-span/trace AddSpan no-ops, and nil-receiver Span mutators (SetAttribute, AddEvent, End) — comprehensive for all guards introduced.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant P as Provider (Bedrock streaming)
    participant CDS as completeDeferredSpan
    participant TR as Tracer
    participant TS as TraceStore
    participant T as Trace
    participant S as Span

    P->>CDS: streaming error occurs
    CDS->>TR: SetAttribute(handle, key, value)
    TR->>TS: GetTrace(traceID)
    TS-->>TR: "*Trace (non-nil)"
    TR->>T: GetSpan(spanID)
    Note over T: BEFORE: dereferences nil entry in Spans → SIGSEGV
    Note over T: AFTER: skips nil entries, returns nil if not found
    T-->>TR: "*Span or nil"
    TR->>S: SetAttribute(key, value)
    Note over S: nil-safe: no-op if s==nil
    CDS->>CDS: error forwarded to caller ✓
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant P as Provider (Bedrock streaming)
    participant CDS as completeDeferredSpan
    participant TR as Tracer
    participant TS as TraceStore
    participant T as Trace
    participant S as Span

    P->>CDS: streaming error occurs
    CDS->>TR: SetAttribute(handle, key, value)
    TR->>TS: GetTrace(traceID)
    TS-->>TR: "*Trace (non-nil)"
    TR->>T: GetSpan(spanID)
    Note over T: BEFORE: dereferences nil entry in Spans → SIGSEGV
    Note over T: AFTER: skips nil entries, returns nil if not found
    T-->>TR: "*Span or nil"
    TR->>S: SetAttribute(key, value)
    Note over S: nil-safe: no-op if s==nil
    CDS->>CDS: error forwarded to caller ✓
Loading

Reviews (2): Last reviewed commit: "[test]: core/schemas - cover empty span ..." | Re-trigger Greptile

Comment thread core/schemas/trace_test.go
Addresses greptile review on PR maximhq#4896.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@akshaydeo
akshaydeo merged commit 1d4fb54 into maximhq:dev Jul 12, 2026
6 checks passed
tcx4c70 added a commit to tcx4c70/bifrost that referenced this pull request Jul 13, 2026
* origin/dev: (79 commits)
  chore: add `helm-update` Claude skill for syncing Helm chart with `config.schema.json` (maximhq#5144)
  fix: web search options to google search mapping in gemini api (maximhq#5139)
  feat: add `postgresql.external.port` string support and `bifrost.mcp.toolGroups[*].id` to Helm chart (maximhq#5143)
  fix: parse `SecretVar` JSON with `ref`/`env_var` fields even when `value` is absent (maximhq#5146)
  Revert "fix: less strict unmarshalling for secret var (maximhq#4723)" (maximhq#5145)
  fix: max reasoning effort in openai (maximhq#5130)
  chore: replace manual `helm registry login` steps with `step-security/docker-login-action` (maximhq#5132)
  fix: support GA transcription-type sessions in POST /v1/realtime/client_secrets (maximhq#5092)
  community: add Xquik to MCP library (maximhq#5069)
  fix: warn callers not to truncate the #t= temp-token fragment on MCP inline-auth links (maximhq#5104)
  chore: build fix in core (maximhq#5129)
  fix: never persist masked provider key previews (maximhq#5106)
  Filter out provider-level keys from selector in prompt manager (maximhq#5018)
  fix: show user popover when `userInfo` exists and include `preferred_username` as display name fallback (maximhq#5098)
  fix: use `AutoMigrate` and add `runner_id`/`created_by_user_id` columns to sidekiq table migration (maximhq#5085)
  dds new harness skill and updates based on merged PRs (maximhq#5126)
  dds new harness skill and updates based on merged PRs (maximhq#5123)
  Add Trendshift badge to README (maximhq#5124)
  fix: make tracing span lookup nil-safe to prevent panic on streaming errors (maximhq#4896)
  Revert "fix: synthesize per-query rerank usage for Bedrock and Vertex (maximhq#4322)" (maximhq#5122)
  ...
akshaydeo pushed a commit that referenced this pull request Jul 14, 2026
…errors (#4896)

* [fix]: core/schemas - make trace/span helpers nil-safe to prevent streaming error panic

A Bedrock streaming error finalizer could hit a nil span entry (or nil
receiver) in the tracing path (completeDeferredSpan -> Trace.GetSpan),
panicking the process instead of forwarding the stream error.

Affected packages:
- core/schemas/trace.go

Fixes #3455

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

* [test]: core/schemas - cover empty span ID guard in GetSpan

Addresses greptile review on PR #4896.

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

---------

Co-authored-by: nnNyx <64274427+nnNyx@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
akhsaul pushed a commit to akhsaul/bifrost that referenced this pull request Aug 27, 2026
…errors (maximhq#4896)

* [fix]: core/schemas - make trace/span helpers nil-safe to prevent streaming error panic

A Bedrock streaming error finalizer could hit a nil span entry (or nil
receiver) in the tracing path (completeDeferredSpan -> Trace.GetSpan),
panicking the process instead of forwarding the stream error.

Affected packages:
- core/schemas/trace.go

Fixes maximhq#3455

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

* [test]: core/schemas - cover empty span ID guard in GetSpan

Addresses greptile review on PR maximhq#4896.

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

---------

Co-authored-by: nnNyx <64274427+nnNyx@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
occcat pushed a commit to occcat/bifrost that referenced this pull request Sep 2, 2026
…errors (maximhq#4896)

* [fix]: core/schemas - make trace/span helpers nil-safe to prevent streaming error panic

A Bedrock streaming error finalizer could hit a nil span entry (or nil
receiver) in the tracing path (completeDeferredSpan -> Trace.GetSpan),
panicking the process instead of forwarding the stream error.

Affected packages:
- core/schemas/trace.go

Fixes maximhq#3455

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

* [test]: core/schemas - cover empty span ID guard in GetSpan

Addresses greptile review on PR maximhq#4896.

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

---------

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Segfault/nil dereference panic in Bedrock provider

2 participants