feat: send HTTP/2 PING keepalives on the Bedrock provider - #5213
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an optional HTTP/2 PING interval to Bedrock network configuration. The interval is serialized, validated, clamped, tested, and applied only when HTTP/2 is enforced and the interval is positive. Backend schemas, Helm values, frontend schemas, and frontend types include the setting. ChangesBedrock HTTP/2 keepalive
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NetworkConfig
participant NewBedrockProvider
participant HTTP2Transport
participant Intermediary
NetworkConfig->>NewBedrockProvider: HTTP2PingIntervalInSeconds
NewBedrockProvider->>HTTP2Transport: configure SendPingTimeout
HTTP2Transport->>Intermediary: send HTTP/2 PING
Intermediary-->>HTTP2Transport: acknowledge HTTP/2 PING
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (8): Last reviewed commit: "fix: make Bedrock HTTP/2 keepalive ping ..." | Re-trigger Greptile |
504e3f9 to
6356afb
Compare
|
Addressed the schema finding: added |
6356afb to
9e85f2e
Compare
|
Question for Bifrost devs: Initially when I created the PR, I enabled the HTTP/2 PING keepalives by default with a reasonable interval, but I backed off that to preserve the current behavior but allowed configurability. The downside of that though is that other users could potentially experience these expected EOF stream timeout errors, debug, and then discover that there's a config knob. Would you prefer enabling these keepalives by default? |
5491cac to
52e2db0
Compare
|
Rebased onto the latest |
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/providers/bedrock/bedrock.go`:
- Around line 128-131: Update the UI NetworkConfig type in config.ts to declare
the http2_ping_interval_in_seconds property used by the Bedrock transport
configuration, with the appropriate numeric/optional contract so UI
serialization preserves the HTTP/2 keepalive value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 514a3de3-06cf-4bf4-aba7-663b70272b76
📒 Files selected for processing (6)
core/changelog.mdcore/providers/bedrock/bedrock.gocore/schemas/provider.gocore/schemas/serialization_test.gohelm-charts/bifrost/values.schema.jsontransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
- helm-charts/bifrost/values.schema.json
- core/schemas/serialization_test.go
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 `@ui/lib/schemas/providerForm.ts`:
- Line 46: Update the http2_ping_interval_in_seconds field in the provider form
schema to include the shared 3600-second maximum, preserving its integer,
non-negative, and optional constraints and using the same validation message as
the shared network schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a757472f-0479-4772-aaad-004e0ac04bef
📒 Files selected for processing (3)
ui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
dee7dd3 to
ec1229a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/changelog.md (1)
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the HTTP/2 keepalive configuration gates.
TestBedrockTransportHTTP2Configonly checksForceAttemptHTTP2. Add cases for enforced HTTP/2 with a positive ping interval, enforced HTTP/2 withHTTP2PingIntervalInSeconds = 0, and non-enforced HTTP/2 with a positive interval; asserttransport.HTTP2is configured or nil accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/changelog.md` at line 1, Extend TestBedrockTransportHTTP2Config to cover the three HTTP/2 keepalive configuration gates: enforced HTTP/2 with a positive HTTP2PingIntervalInSeconds must configure transport.HTTP2, enforced HTTP/2 with an interval of 0 must leave it nil, and non-enforced HTTP/2 with a positive interval must also leave it nil. Retain the existing ForceAttemptHTTP2 assertion.
🤖 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 `@transports/config.schema.json`:
- Around line 3701-3705: Cap HTTP/2 ping intervals at 9,223,372,036 seconds in
CheckAndSetDefaults before converting HTTP2PingIntervalInSeconds to
time.Duration, rejecting larger values to prevent overflow. Add maximum:
9223372036 to both http2_ping_interval_in_seconds entries in
transports/config.schema.json while preserving the existing minimum and
disabled-value behavior.
---
Nitpick comments:
In `@core/changelog.md`:
- Line 1: Extend TestBedrockTransportHTTP2Config to cover the three HTTP/2
keepalive configuration gates: enforced HTTP/2 with a positive
HTTP2PingIntervalInSeconds must configure transport.HTTP2, enforced HTTP/2 with
an interval of 0 must leave it nil, and non-enforced HTTP/2 with a positive
interval must also leave it nil. Retain the existing ForceAttemptHTTP2
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6ac9f7f-993d-4832-a0d2-b71d78c99879
📒 Files selected for processing (9)
core/changelog.mdcore/providers/bedrock/bedrock.gocore/schemas/provider.gocore/schemas/serialization_test.gohelm-charts/bifrost/values.schema.jsontransports/config.schema.jsonui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/lib/types/config.ts
- ui/lib/schemas/providerForm.ts
- helm-charts/bifrost/values.schema.json
- core/schemas/serialization_test.go
- core/schemas/provider.go
- ui/lib/types/schemas.ts
- core/providers/bedrock/bedrock.go
The merge-base changed after approval.
When enforce_http2 is set, configure http.HTTP2Config.SendPingTimeout so the client sends PING frames while a streaming or unary response is idle, keeping a long-lived connection from being closed by an intermediary idle timeout (which the AWS EventStream decoder would otherwise surface as an "unexpected EOF"). The interval is configurable via network_config http2_keepalive_interval_in_seconds (default 30s).
Mirror http2_ping_interval_in_seconds into the chart values.schema.json networkConfig def so Helm deployments can set it.
There was no HTTP/2 ping keepalive before this change, so the 30s default-fill was an arbitrary imposition. Treat 0 as disabled (matching net/http's own SendPingTimeout semantics) and only send pings when a positive interval is configured, keeping enforce_http2 orthogonal to keepalive.
The TS NetworkConfig interface and its three Zod mirrors (providerForm's NetworkConfigSchema, and schemas.ts's networkConfigSchema / networkFormConfigSchema) were missing the field added alongside enforce_http2 in the Go schema, so a value round-tripped through those validators would be silently stripped.
Matches the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds bounds in the same schema, and the 3600s ceiling already enforced in ui/lib/types/schemas.ts. Without it, a value above 3600 could pass this schema but fail the shared one later.
http2_ping_interval_in_seconds is converted to time.Duration via * time.Second in the Bedrock transport; a value above ~9.2 billion seconds overflows int64 silently. Clamp in CheckAndSetDefaults (same pattern as MaxConnsPerHost) and cap the config.schema.json / values.schema.json bounds accordingly. Also extends TestBedrockTransportHTTP2Config to assert transport.HTTP2 across all three enforce_http2 x interval gate combinations, which the existing test never exercised.
… fields The prior overflow-safety fix (5c45ef3) capped this at the raw int64 overflow boundary (9223372036) in the Go constant and both JSON schemas, but the UI's three Zod mirrors already capped it at 3600 — matching the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds fields, which cap at 3600 everywhere including config.schema.json. Align all six copies on 3600: a config value between 3601 and 9223372036 passed the backend but would fail the UI's validation on round-trip.
a9a3a07
710ae9e to
a9a3a07
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
* feat: send HTTP/2 PING keepalives on the Bedrock provider When enforce_http2 is set, configure http.HTTP2Config.SendPingTimeout so the client sends PING frames while a streaming or unary response is idle, keeping a long-lived connection from being closed by an intermediary idle timeout (which the AWS EventStream decoder would otherwise surface as an "unexpected EOF"). The interval is configurable via network_config http2_keepalive_interval_in_seconds (default 30s). * fix: expose Bedrock HTTP/2 keepalive interval in Helm values schema Mirror http2_ping_interval_in_seconds into the chart values.schema.json networkConfig def so Helm deployments can set it. * fix: make Bedrock HTTP/2 keepalive ping opt-in (0 = disabled) There was no HTTP/2 ping keepalive before this change, so the 30s default-fill was an arbitrary imposition. Treat 0 as disabled (matching net/http's own SendPingTimeout semantics) and only send pings when a positive interval is configured, keeping enforce_http2 orthogonal to keepalive. * fix: declare http2_ping_interval_in_seconds in UI NetworkConfig types The TS NetworkConfig interface and its three Zod mirrors (providerForm's NetworkConfigSchema, and schemas.ts's networkConfigSchema / networkFormConfigSchema) were missing the field added alongside enforce_http2 in the Go schema, so a value round-tripped through those validators would be silently stripped. * fix: cap http2_ping_interval_in_seconds at 3600s in provider form schema Matches the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds bounds in the same schema, and the 3600s ceiling already enforced in ui/lib/types/schemas.ts. Without it, a value above 3600 could pass this schema but fail the shared one later. * fix: clamp Bedrock HTTP/2 ping interval to avoid int64 overflow http2_ping_interval_in_seconds is converted to time.Duration via * time.Second in the Bedrock transport; a value above ~9.2 billion seconds overflows int64 silently. Clamp in CheckAndSetDefaults (same pattern as MaxConnsPerHost) and cap the config.schema.json / values.schema.json bounds accordingly. Also extends TestBedrockTransportHTTP2Config to assert transport.HTTP2 across all three enforce_http2 x interval gate combinations, which the existing test never exercised. * fix: align http2_ping_interval_in_seconds ceiling with UI and sibling fields The prior overflow-safety fix (5c45ef3) capped this at the raw int64 overflow boundary (9223372036) in the Go constant and both JSON schemas, but the UI's three Zod mirrors already capped it at 3600 — matching the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds fields, which cap at 3600 everywhere including config.schema.json. Align all six copies on 3600: a config value between 3601 and 9223372036 passed the backend but would fail the UI's validation on round-trip.
* feat: send HTTP/2 PING keepalives on the Bedrock provider When enforce_http2 is set, configure http.HTTP2Config.SendPingTimeout so the client sends PING frames while a streaming or unary response is idle, keeping a long-lived connection from being closed by an intermediary idle timeout (which the AWS EventStream decoder would otherwise surface as an "unexpected EOF"). The interval is configurable via network_config http2_keepalive_interval_in_seconds (default 30s). * fix: expose Bedrock HTTP/2 keepalive interval in Helm values schema Mirror http2_ping_interval_in_seconds into the chart values.schema.json networkConfig def so Helm deployments can set it. * fix: make Bedrock HTTP/2 keepalive ping opt-in (0 = disabled) There was no HTTP/2 ping keepalive before this change, so the 30s default-fill was an arbitrary imposition. Treat 0 as disabled (matching net/http's own SendPingTimeout semantics) and only send pings when a positive interval is configured, keeping enforce_http2 orthogonal to keepalive. * fix: declare http2_ping_interval_in_seconds in UI NetworkConfig types The TS NetworkConfig interface and its three Zod mirrors (providerForm's NetworkConfigSchema, and schemas.ts's networkConfigSchema / networkFormConfigSchema) were missing the field added alongside enforce_http2 in the Go schema, so a value round-tripped through those validators would be silently stripped. * fix: cap http2_ping_interval_in_seconds at 3600s in provider form schema Matches the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds bounds in the same schema, and the 3600s ceiling already enforced in ui/lib/types/schemas.ts. Without it, a value above 3600 could pass this schema but fail the shared one later. * fix: clamp Bedrock HTTP/2 ping interval to avoid int64 overflow http2_ping_interval_in_seconds is converted to time.Duration via * time.Second in the Bedrock transport; a value above ~9.2 billion seconds overflows int64 silently. Clamp in CheckAndSetDefaults (same pattern as MaxConnsPerHost) and cap the config.schema.json / values.schema.json bounds accordingly. Also extends TestBedrockTransportHTTP2Config to assert transport.HTTP2 across all three enforce_http2 x interval gate combinations, which the existing test never exercised. * fix: align http2_ping_interval_in_seconds ceiling with UI and sibling fields The prior overflow-safety fix (5c45ef3) capped this at the raw int64 overflow boundary (9223372036) in the Go constant and both JSON schemas, but the UI's three Zod mirrors already capped it at 3600 — matching the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds fields, which cap at 3600 everywhere including config.schema.json. Align all six copies on 3600: a config value between 3601 and 9223372036 passed the backend but would fail the UI's validation on round-trip.
* feat: send HTTP/2 PING keepalives on the Bedrock provider When enforce_http2 is set, configure http.HTTP2Config.SendPingTimeout so the client sends PING frames while a streaming or unary response is idle, keeping a long-lived connection from being closed by an intermediary idle timeout (which the AWS EventStream decoder would otherwise surface as an "unexpected EOF"). The interval is configurable via network_config http2_keepalive_interval_in_seconds (default 30s). * fix: expose Bedrock HTTP/2 keepalive interval in Helm values schema Mirror http2_ping_interval_in_seconds into the chart values.schema.json networkConfig def so Helm deployments can set it. * fix: make Bedrock HTTP/2 keepalive ping opt-in (0 = disabled) There was no HTTP/2 ping keepalive before this change, so the 30s default-fill was an arbitrary imposition. Treat 0 as disabled (matching net/http's own SendPingTimeout semantics) and only send pings when a positive interval is configured, keeping enforce_http2 orthogonal to keepalive. * fix: declare http2_ping_interval_in_seconds in UI NetworkConfig types The TS NetworkConfig interface and its three Zod mirrors (providerForm's NetworkConfigSchema, and schemas.ts's networkConfigSchema / networkFormConfigSchema) were missing the field added alongside enforce_http2 in the Go schema, so a value round-tripped through those validators would be silently stripped. * fix: cap http2_ping_interval_in_seconds at 3600s in provider form schema Matches the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds bounds in the same schema, and the 3600s ceiling already enforced in ui/lib/types/schemas.ts. Without it, a value above 3600 could pass this schema but fail the shared one later. * fix: clamp Bedrock HTTP/2 ping interval to avoid int64 overflow http2_ping_interval_in_seconds is converted to time.Duration via * time.Second in the Bedrock transport; a value above ~9.2 billion seconds overflows int64 silently. Clamp in CheckAndSetDefaults (same pattern as MaxConnsPerHost) and cap the config.schema.json / values.schema.json bounds accordingly. Also extends TestBedrockTransportHTTP2Config to assert transport.HTTP2 across all three enforce_http2 x interval gate combinations, which the existing test never exercised. * fix: align http2_ping_interval_in_seconds ceiling with UI and sibling fields The prior overflow-safety fix (5c45ef3) capped this at the raw int64 overflow boundary (9223372036) in the Go constant and both JSON schemas, but the UI's three Zod mirrors already capped it at 3600 — matching the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds fields, which cap at 3600 everywhere including config.schema.json. Align all six copies on 3600: a config value between 3601 and 9223372036 passed the backend but would fail the UI's validation on round-trip.
* feat: send HTTP/2 PING keepalives on the Bedrock provider When enforce_http2 is set, configure http.HTTP2Config.SendPingTimeout so the client sends PING frames while a streaming or unary response is idle, keeping a long-lived connection from being closed by an intermediary idle timeout (which the AWS EventStream decoder would otherwise surface as an "unexpected EOF"). The interval is configurable via network_config http2_keepalive_interval_in_seconds (default 30s). * fix: expose Bedrock HTTP/2 keepalive interval in Helm values schema Mirror http2_ping_interval_in_seconds into the chart values.schema.json networkConfig def so Helm deployments can set it. * fix: make Bedrock HTTP/2 keepalive ping opt-in (0 = disabled) There was no HTTP/2 ping keepalive before this change, so the 30s default-fill was an arbitrary imposition. Treat 0 as disabled (matching net/http's own SendPingTimeout semantics) and only send pings when a positive interval is configured, keeping enforce_http2 orthogonal to keepalive. * fix: declare http2_ping_interval_in_seconds in UI NetworkConfig types The TS NetworkConfig interface and its three Zod mirrors (providerForm's NetworkConfigSchema, and schemas.ts's networkConfigSchema / networkFormConfigSchema) were missing the field added alongside enforce_http2 in the Go schema, so a value round-tripped through those validators would be silently stripped. * fix: cap http2_ping_interval_in_seconds at 3600s in provider form schema Matches the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds bounds in the same schema, and the 3600s ceiling already enforced in ui/lib/types/schemas.ts. Without it, a value above 3600 could pass this schema but fail the shared one later. * fix: clamp Bedrock HTTP/2 ping interval to avoid int64 overflow http2_ping_interval_in_seconds is converted to time.Duration via * time.Second in the Bedrock transport; a value above ~9.2 billion seconds overflows int64 silently. Clamp in CheckAndSetDefaults (same pattern as MaxConnsPerHost) and cap the config.schema.json / values.schema.json bounds accordingly. Also extends TestBedrockTransportHTTP2Config to assert transport.HTTP2 across all three enforce_http2 x interval gate combinations, which the existing test never exercised. * fix: align http2_ping_interval_in_seconds ceiling with UI and sibling fields The prior overflow-safety fix (5c45ef3) capped this at the raw int64 overflow boundary (9223372036) in the Go constant and both JSON schemas, but the UI's three Zod mirrors already capped it at 3600 — matching the sibling stream_idle_timeout_in_seconds / keep_alive_timeout_in_seconds fields, which cap at 3600 everywhere including config.schema.json. Align all six copies on 3600: a config value between 3601 and 9223372036 passed the backend but would fail the UI's validation on round-trip.
Summary
When a Bedrock stream goes silent for tens of seconds (model "thinking"), an intermediary (reverse proxy / load balancer / service mesh) with an idle timeout shorter than the gap can sever the connection, surfacing as
io.ErrUnexpectedEOF. This adds client-initiated HTTP/2 PING keepalives on the Bedrock provider transport so a quiet stream stays warm.Changes
core/providers/bedrock/bedrock.go: whennetwork_config.enforce_http2is set and a positivehttp2_ping_interval_in_secondsis configured, set the transport'shttp.HTTP2Config{SendPingTimeout}(Go 1.24+ stdlib) to send a PING after that idle interval. Off by default (no ping) to keepenforce_http2orthogonal to keepalive.core/schemas/provider.go: addhttp2_ping_interval_in_secondstoNetworkConfig(opt-in;0= disabled, matching net/http's ownSendPingTimeoutsemantics; mirrored in the Marshal/Unmarshal aliases).core/schemas/serialization_test.go: unit test for the field round-trip + opt-in (0 = off) behavior.Type of change
How to test
cd core && go test ./schemas/ -run TestNetworkConfig_HTTP2PingInterval -count=1. Also validated end-to-end against real Bedrock behind an intermediary with a short idle timeout: HTTP/2 negotiated, PING frames emitted at the interval, stream survives a long thinking gap.Affected areas
core / providers / bedrock; core / schemas
Breaking changes
None — gated on the existing
enforce_http2flag; default off.Related issues
Closes #5211
Security considerations
None.