[fix]: map upstream connection failures to 502 instead of 400 - #3929
Merged
akshaydeo merged 5 commits intoJun 5, 2026
Merged
Conversation
## Summary Removes the redundant changelog header and version label from `cli/changelog.md` and fixes a missing newline at the end of the file. ## Changes - Removed the `# Bifrost CLI Changelog` heading and `## v0.10.5` version label from the top of the changelog - Added a trailing newline to the final line of the file to resolve the "no newline at end of file" issue ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Verify the changelog renders correctly and that no trailing newline warning appears in diff tooling. ```sh cat -A cli/changelog.md | tail -5 ``` The last line should end with `$` (indicating a proper newline terminator). ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Documentation** * Updated changelog to document CLI command overlay improvements enabling arrow key navigation in tab popups. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
When the upstream provider fails to return a response body (connection reset before the first byte, DNS lookup failure, connection refused), makeRequestWithDoFunc returned a BifrostError with StatusCode: nil and IsBifrostError: false. In SendBifrostError that fell through to HTTP 400, misleading downstream consumers into treating a retriable upstream connectivity failure as a non-retriable client bad-request. Mirrors the timeout-mapping fix in maximhq#2412 / maximhq#2413 applied to the connection-failure branches. Changes: - core/schemas/bifrost.go - new ProviderConnectionFailed error type constant ("provider_connection_failed"), alongside the existing RequestCancelled / RequestTimedOut. - core/providers/utils/utils.go - new NewBifrostUpstreamConnectionError constructor mirroring NewBifrostTimeoutError; sets StatusCode to 502 (Bad Gateway) and Error.Type to ProviderConnectionFailed. IsBifrostError is false (the upstream is the cause). Replaced both StatusCode-less return paths in makeRequestWithDoFunc with the new constructor: the net.OpError / net.DNSError branch, and the generic do-error branch. - core/providers/utils/makerequest_test.go - new TestNewBifrostUpstreamConnectionError; updated TestMakeRequestWithContext_ClientError to assert 502 + ProviderConnectionFailed. - core/changelog.md - entry at top. Closes maximhq#3927 References maximhq#2412 / maximhq#2413 (precedent: timeout -> 504 mapping)
|
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMaps upstream connectivity failures to HTTP 502 (provider_connection_failed), adds ChangesUpstream Connection Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
akshaydeo
previously approved these changes
May 31, 2026
Contributor
|
❤️ for the PR @chris-colinsky - should go with our next release |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/changelog.md`:
- Line 1: Update the changelog entry "[fix]: map upstream connection failures to
502 instead of 400 [`@chris-colinsky`]" to include the PR number in the same
format as other entries by appending " (`#3929`)" to the end of the line so it
matches the existing pattern (e.g., include the PR identifier next to the author
handle).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36aef3e7-b9fc-48bc-aeab-61c22d2d8aaf
📒 Files selected for processing (4)
core/changelog.mdcore/providers/utils/makerequest_test.gocore/providers/utils/utils.gocore/schemas/bifrost.go
Contributor
Confidence Score: 4/5The core code change is correct and safe; the changelog files contain accidental content that should be fixed before merging. The core/changelog.md and cli/changelog.md — the former drops five existing entries and the latter adds twelve unrelated CLI entries; both appear to be rebase/merge artifacts. Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'dev' into fix/upstream-con..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses review feedback — matches the existing entry format (e.g. `(maximhq#3900)`) for traceability. Changes: - core/changelog.md - append (maximhq#3929) to the upstream connection failures entry.
akshaydeo
force-pushed
the
dev
branch
2 times, most recently
from
June 4, 2026 10:02
6711ce3 to
a1beab5
Compare
Signed-off-by: Akshay Deo <akshay@akshaydeo.com>
18 tasks
akshaydeo
added a commit
that referenced
this pull request
Jun 6, 2026
## Summary This PR bumps the Go toolchain version from `1.26.3` to `1.26.4` across all modules and CI workflows, and cuts a new release (`core` v1.5.17, `framework` v1.3.17, `transports` v1.5.9, `plugins/compat` v0.1.16, `plugins/governance` v1.5.17, and associated plugin versions) incorporating a large batch of features and fixes accumulated since the previous release. ## Changes - **Go 1.26.4** — Updated `go-version` in all GitHub Actions workflows (`e2e-tests`, `helm-release`, `pr-tests`, `release-cli`, `release-pipeline`, `snyk`) and all `go.mod` files (core, framework, transports, cli, all plugins, examples, and test modules). - **Core (v1.5.17)** — OpenAI compaction support, multi-customer logs and usage tracking, multiple team/business unit support, `request_headers` wildcard pattern capture for OTel and Maxim plugins, xAI `x_search` tool, fetch URL validation with SSRF hardening, `file://` pricing URL scheme, virtual key provider fan-out filtering, and a broad set of fixes including Anthropic prompt cache key, empty thinking block stripping, OpenAI stream usage event cleanup, Gemini numeric schema constraints, stale connection retries, Azure Claude diagnostic strip, and passthrough budget handling. - **Framework (v1.3.17)** — Scope-aware budgets and limits wired from model configs, provider-level governance, multiple customer budget support with `calendar_aligned` windows, paginated virtual key fetch, `config.json` source-of-truth flow, FTS index cap reduction, sync worker drift fix, cascade deletes for model configs, and high-scale virtual key flow improvements. - **Transports (v1.5.9)** — Full changelog covering all of the above plus UI improvements (log navigation, customer detail sheet, `BudgetDisplay` component, inline loading shell, materialized view alias), SCIM provisioning fields, Helm/config schema additions (`roles`, `per_user_oauth`), client IP resolution from forwarded headers, and dependency upgrades (`recharts` to 3.8.1, `golang.org/x` CVE remediation). - **Plugins** — `governance` v1.5.17 adds team budget/rate-limit exporters, ghost node reconciliation fix, and VK double usage counting fix; `logging` v1.5.17 adds wildcard header capture and file attachment rendering; `otel` v1.2.17 adds `disable_content_logging` and multiple collectors support; `maxim` v1.6.17 adds `request_headers` wildcard capture; `compat` v0.1.16 fixes `max_tokens` preservation during param filtering. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Verify Go version go version # should report go1.26.4 # Run core tests cd core && go test ./... # Run framework tests cd framework && go test ./... # Run transports tests cd transports && go test ./... # Run plugin tests cd plugins/governance && go test ./... cd plugins/logging && go test ./... cd plugins/otel && go test ./... # UI cd ui pnpm i pnpm build pnpm test ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues #4053, #4066, #4041, #4012, #3976, #3947, #3991, #4045, #3957, #3938, #3937, #3939, #3981, #3998, #3997, #4092, #4091, #4079, #4080, #4086, #3929, #3994, #4028, #3970, #3919, #3861, #3664, #3999, #4088, #4070, #4051, #4043, #4057, #4023, #3941, #3955, #4024, #3956, #3967, #3925, #3992, #3900 ## Security considerations - Fetch URL validation hardened against SSRF by tightening IP checks for private networks and link-local addresses (#4092, #3947, #3991). - Transitive `golang.org/x` dependencies (crypto, net, sys, text) bumped to address Docker Scout CVEs (#3900). ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * OpenAI compaction, multi-customer/team logstore support, request-header wildcard capture, enhanced governance (provider-level & scope-aware limits), disable-content-logging option, support for multiple OpenTelemetry collectors, SSRF hardening and URL validation. * **Chores** * Bumped Go toolchain across modules and updated component/plugin version releases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Merged
akshaydeo
added a commit
that referenced
this pull request
Jun 7, 2026
* changelogs * [fix]: map upstream connection failures to 502 instead of 400 When the upstream provider fails to return a response body (connection reset before the first byte, DNS lookup failure, connection refused), makeRequestWithDoFunc returned a BifrostError with StatusCode: nil and IsBifrostError: false. In SendBifrostError that fell through to HTTP 400, misleading downstream consumers into treating a retriable upstream connectivity failure as a non-retriable client bad-request. Mirrors the timeout-mapping fix in #2412 / #2413 applied to the connection-failure branches. Changes: - core/schemas/bifrost.go - new ProviderConnectionFailed error type constant ("provider_connection_failed"), alongside the existing RequestCancelled / RequestTimedOut. - core/providers/utils/utils.go - new NewBifrostUpstreamConnectionError constructor mirroring NewBifrostTimeoutError; sets StatusCode to 502 (Bad Gateway) and Error.Type to ProviderConnectionFailed. IsBifrostError is false (the upstream is the cause). Replaced both StatusCode-less return paths in makeRequestWithDoFunc with the new constructor: the net.OpError / net.DNSError branch, and the generic do-error branch. - core/providers/utils/makerequest_test.go - new TestNewBifrostUpstreamConnectionError; updated TestMakeRequestWithContext_ClientError to assert 502 + ProviderConnectionFailed. - core/changelog.md - entry at top. Closes #3927 References #2412 / #2413 (precedent: timeout -> 504 mapping) * [chore]: add PR number to changelog entry Addresses review feedback — matches the existing entry format (e.g. `(#3900)`) for traceability. Changes: - core/changelog.md - append (#3929) to the upstream connection failures entry. --------- Signed-off-by: Akshay Deo <akshay@akshaydeo.com> Co-authored-by: akshaydeo <akshay@akshaydeo.com>
akshaydeo
added a commit
that referenced
this pull request
Jun 7, 2026
## ✨ Features - **OpenAI Compaction** — Added OpenAI conversation compaction support across core, framework, logging, and the API surface (#4053) - **Multi-Customer & Org Hierarchy** — Logs and usage tracking now support multiple customers, teams, and business units, including business unit CRUD, team assignment, and governance endpoints in the OpenAPI spec (#4066, #4041, #4082) - **Provider-Level Governance** — Budgets & limits are now scope-aware and can be applied at the virtual-key top level and per provider, wired from the model configs table, with UI filters for scope and providers (#3938, #3937, #3939, #3981, #3962) - **Customer Budgets** — Customers support multiple budgets and `calendar_aligned` budget windows (#3998, #3997) - **Virtual Key Attribution & Controls** — Added a `created_by` user attribution column and a `blacklisted_models` column for virtual key provider configs (#3672, #3653) - **Request Header Capture** — OTel and Maxim observability plugins capture `request_headers` by pattern, with wildcard support (e.g. `x-custom-*`); logging gained the same wildcard header capture (#4012, #3958) - **OTel Content Controls & Collectors** — New `disable_content_logging` option drops message/tool content from exported spans, plus support for multiple OTel collectors (#4064, #3894) - **xAI x_search** — Added xAI `x_search` tool support (#3976) - **URL Validation** — Added fetch URL validation with private-network configuration and link-local blocking (#3947, #3991) - **File Scheme Pricing URLs** — Pricing source URLs now accept the `file://` scheme for air-gapped and self-hosted deployments (#4045) - **Paginated Virtual Keys** — Virtual key fetching is paginated to handle deployments with very large numbers of keys (#3957) - **Client IP Resolution** — Resolve client IP from `X-Forwarded-For`/`X-Real-IP` headers - **SCIM Provisioning** — Added `attributeType`/`attributeValue` SCIM provisioning fields - **Helm/Config Schema** — Added `roles` RBAC governance config and `per_user_oauth` MCP auth to the Helm chart and config schema (#4004, #4009) - **Log Navigation UI** — Added a "View logs" menu item to customer, team, and virtual key tables, clickable links in log detail views, a customer detail sheet, and a reusable `BudgetDisplay` component (#4073, #4054, #4026, #4055) - **Faster First Paint** — Added an inline loading shell to `#root` before React mounts (#4063) - **Materialized View Alias** — Added an `alias` column to the materialized view with filter support (#4078) ## 🐞 Fixed - **Fetch URL IP Checks** — Hardened fetch URL IP checks against SSRF (#4092) - **Mantle Model Matching** — Broadened Mantle model matching to all `gpt` variants (#4091) - **Empty Thinking Blocks** — Strip thinking blocks when the signature is empty (#4079) - **OpenAI Stream Usage** — Removed usage from the `responses.created` event in the OpenAI stream (#4080) - **Prompt Cache Key** — Set the prompt cache key from the Anthropic integration (#4086) - **Upstream Failure Status** — Map upstream connection failures to 502 instead of 400 (#3929) (thanks [@chris-colinsky](https://github.com/chris-colinsky)!) - **Gemini Schema Constraints** — Accept numeric schema integer constraints for Gemini (#3994) (thanks [@yanhao98](https://github.com/yanhao98)!) - **Files Provider Param** — Accept the `?provider=` query param on `GET /v1/files` (#3971) (thanks [@alexef](https://github.com/alexef)!) - **Optional Batch Model** — Made the `model` field optional on `POST /v1/batches` (#3973) (thanks [@alexef](https://github.com/alexef)!) - **Helm Azure Config** — Added missing `azure_key_config` fields to the Helm schema (#3996) (thanks [@axelray-dev](https://github.com/axelray-dev)!) - **Text Completion Chunk Model** — Added the missing `Model` field to `TextCompletionChunkResponse` (#3970) (thanks [@kuishou68](https://github.com/kuishou68)!) - **MCP Inline stdio Env** — MCP stdio server configs accept inline environment variable assignments (#3861) (thanks [@Shushmitaaaa](https://github.com/Shushmitaaaa)!) - **Orphaned Tool Results** — Orphaned tool results in the OpenAI to Anthropic conversion flow are no longer rejected by the Anthropic API (#3919) - **Node Usage Reconciliation** — Added a monotonic `inc_number` log cursor so node usage reconciliation does not skip late async log writes (#3664) - **Bedrock Output Assessments** — Corrected the type of `outputAssessments` in Bedrock responses (#4028) - **Model Pool Pricing Reloads** — Preserve non-pricing model pool entries across pricing reloads (#3999) - **Ghost Node Reconciliation** — Replicate the VK hierarchy flow for ghost node reconciliation (#4088) - **VK Double Usage Counting** — Fixed double usage counting when creating a virtual key (#4070) - **Model Config Lifecycle** — Cascade deletes for model configs and removal of stale in-memory model configs (#4051, #4043) - **FTS Index Cap** — Reduced the FTS index `left()` cap from 800k to 250k chars to stay within the tsvector limit (#4057) - **Sync Worker Drift** — Reduced the sync worker ticker period to 5m to prevent threshold drift (#4023) - **Passthrough** — Fixed passthrough budgets, gated passthrough models per VK, model extraction for Azure passthrough, and restricted fallbacks/provider selection to the VK boundary (#3941, #3988, #3983, #3924) - **Provider Response Headers** — Strip provider response headers and add a content-type filter (#3955, #4024) - **Stream Handling** — Drain non-SSE stream readers and retry stale connections (#3956, #3967) - **Azure Claude** — Strip Azure diagnostic property for Claude models (#3925) - **Compat max_tokens** — Preserve chat `max_tokens` during param filtering (#3992) - **Raw Request Flag** — Removed the raw request flag from providers that don't support it (#4058) - **UI Fixes** — Standardized page container layout, virtual key model configs UI, and dashboard chart tooltips (#4046, #4052, #4044) ## 🔧 Maintenance - **Dependency Upgrades** — Bumped transitive `golang.org/x` dependencies (crypto, net, sys, text) for Docker Scout CVE remediation and `recharts` to 3.8.1; cascaded version bumps across all modules (#3900, #4003)
Closed
2 tasks
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.
Description
When the upstream provider fails to return a response body (connection reset before the first byte, DNS lookup failure, connection refused),
makeRequestWithDoFuncincore/providers/utils/utils.goreturned aBifrostErrorwithStatusCode: nilandIsBifrostError: false. InSendBifrostErrorthat fell through to HTTP 400, misleading downstream consumers into treating a retriable upstream connectivity failure as a non-retriable client bad-request.This mirrors the timeout-mapping fix in #2412 / #2413, applied to the connection-failure branches that weren't covered there. 502 (Bad Gateway) is the right semantic: Bifrost successfully dispatched to the upstream; the upstream just failed to respond.
Type of Change
Affected Packages
core/schemas/— newProviderConnectionFailederror type constantcore/providers/utils/— newNewBifrostUpstreamConnectionErrorconstructor; two branches inmakeRequestWithDoFuncupdated; tests addedChanges Made
ProviderConnectionFailed = "provider_connection_failed"to the existing error-type const block incore/schemas/bifrost.go.NewBifrostUpstreamConnectionError(message, err)incore/providers/utils/utils.go, mirroringNewBifrostTimeoutErrorfrom fix timeout status code #2413. SetsStatusCode: 502,Error.Type: ProviderConnectionFailed.IsBifrostError: falsebecause the upstream is the cause.StatusCode-less return paths inmakeRequestWithDoFuncwith the new constructor:*net.OpError/*net.DNSErrorbranch (usesschemas.ErrProviderNetworkError).schemas.ErrProviderDoRequest) — this is the branch that catches the "server closed connection before returning the first response byte" case from fasthttp.TestNewBifrostUpstreamConnectionErrorincore/providers/utils/makerequest_test.go(mirrorsTestNewBifrostTimeoutError).TestMakeRequestWithContext_ClientErrorto assertStatusCode == 502andError.Type == ProviderConnectionFailed.core/changelog.md.Total: 4 files, +53 / -16.
Testing
make test-allrun locally — Go toolchain not available in my environment; relying on CI. The change is mechanical (replaces inline struct literals with a constructor call of identical shape) and the new test assertions mirror the precedent'sTestNewBifrostTimeoutError.Checklist
NewBifrostTimeoutErrorprecedent)make test-all) — see note above[type]: descriptionRelated Issues
Closes #3927
References #2412 / #2413 (precedent: timeout → 504 mapping)
Summary by CodeRabbit
New Features
Bug Fixes