fix: idle timeout wiring in vertex path - #4937
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 (7)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change centralizes or repositions stream idle-timeout initialization across Gemini, Vertex, Cohere, Hugging Face, Mistral, Replicate, and vLLM streaming paths. Gemini handlers now accept the timeout value directly, while other providers set it earlier in successful streaming setup. ChangesStream idle-timeout updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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" Comment |
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
7d19f9a to
ca8fbd4
Compare
4d61b0e to
30ca2f5
Compare
30ca2f5 to
e2f9f2d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/providers/vertex/vertex.go (1)
977-977: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a regression test for the idle-timeout wiring fix.
This PR fixes a real gap where Vertex-Gemini streaming never had an idle timeout applied. A table-driven or unit test asserting
providerUtils.GetStreamIdleTimeout(ctx)is non-zero afterChatCompletionStream/ResponsesStreamdispatch (for the Gemini-family branch) would guard against regressing this fix.Also applies to: 1505-1505
🤖 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/providers/vertex/vertex.go` at line 977, Add a regression test for the Gemini streaming path in vertex.go to verify the idle-timeout wiring stays intact. Focus on the ChatCompletionStream and ResponsesStream dispatch flow in the Gemini-family branch, and assert that providerUtils.GetStreamIdleTimeout(ctx) is non-zero after the call. Use a table-driven or unit test that exercises the provider.networkConfig.StreamIdleTimeoutInSeconds path so the fix cannot regress.
🤖 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.
Nitpick comments:
In `@core/providers/vertex/vertex.go`:
- Line 977: Add a regression test for the Gemini streaming path in vertex.go to
verify the idle-timeout wiring stays intact. Focus on the ChatCompletionStream
and ResponsesStream dispatch flow in the Gemini-family branch, and assert that
providerUtils.GetStreamIdleTimeout(ctx) is non-zero after the call. Use a
table-driven or unit test that exercises the
provider.networkConfig.StreamIdleTimeoutInSeconds path so the fix cannot
regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd474357-70e7-4374-92cf-0973c82ff884
📒 Files selected for processing (2)
core/providers/gemini/gemini.gocore/providers/vertex/vertex.go
e2f9f2d to
7d0f706
Compare
Merge activity
|
## Summary The `SetStreamIdleTimeoutIfEmpty` call was being made before invoking the shared `HandleGemini*Stream` functions, meaning the timeout was set in the provider-level methods rather than inside the shared handlers. This caused the Vertex provider — which delegates directly to those shared handlers — to never set the stream idle timeout at all. Moving the call into `HandleGeminiChatCompletionStream` and `HandleGeminiResponsesStream`, and passing `streamIdleTimeoutInSeconds` as an explicit parameter, ensures the timeout is consistently applied regardless of which provider invokes the handler. ## Changes - Removed `SetStreamIdleTimeoutIfEmpty` calls from `GeminiProvider.ChatCompletionStream` and `GeminiProvider.ResponsesStream`. - Added `streamIdleTimeoutInSeconds int` parameter to `HandleGeminiChatCompletionStream` and `HandleGeminiResponsesStream`. - Moved `SetStreamIdleTimeoutIfEmpty` to the top of each shared handler so it is always executed. - Updated `VertexProvider.ChatCompletionStream` and `VertexProvider.ResponsesStream` to pass `provider.networkConfig.StreamIdleTimeoutInSeconds` to the shared handlers, fixing the missing timeout for Vertex streaming calls. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Verify that streaming requests through the Vertex provider respect the configured `StreamIdleTimeoutInSeconds` by setting a short timeout and confirming the stream is terminated after the idle period elapses. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary The `SetStreamIdleTimeoutIfEmpty` call was being made before invoking the shared `HandleGemini*Stream` functions, meaning the timeout was set in the provider-level methods rather than inside the shared handlers. This caused the Vertex provider — which delegates directly to those shared handlers — to never set the stream idle timeout at all. Moving the call into `HandleGeminiChatCompletionStream` and `HandleGeminiResponsesStream`, and passing `streamIdleTimeoutInSeconds` as an explicit parameter, ensures the timeout is consistently applied regardless of which provider invokes the handler. ## Changes - Removed `SetStreamIdleTimeoutIfEmpty` calls from `GeminiProvider.ChatCompletionStream` and `GeminiProvider.ResponsesStream`. - Added `streamIdleTimeoutInSeconds int` parameter to `HandleGeminiChatCompletionStream` and `HandleGeminiResponsesStream`. - Moved `SetStreamIdleTimeoutIfEmpty` to the top of each shared handler so it is always executed. - Updated `VertexProvider.ChatCompletionStream` and `VertexProvider.ResponsesStream` to pass `provider.networkConfig.StreamIdleTimeoutInSeconds` to the shared handlers, fixing the missing timeout for Vertex streaming calls. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Verify that streaming requests through the Vertex provider respect the configured `StreamIdleTimeoutInSeconds` by setting a short timeout and confirming the stream is terminated after the idle period elapses. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
The
SetStreamIdleTimeoutIfEmptycall was being made before invoking the sharedHandleGemini*Streamfunctions, meaning the timeout was set in the provider-level methods rather than inside the shared handlers. This caused the Vertex provider — which delegates directly to those shared handlers — to never set the stream idle timeout at all. Moving the call intoHandleGeminiChatCompletionStreamandHandleGeminiResponsesStream, and passingstreamIdleTimeoutInSecondsas an explicit parameter, ensures the timeout is consistently applied regardless of which provider invokes the handler.Changes
SetStreamIdleTimeoutIfEmptycalls fromGeminiProvider.ChatCompletionStreamandGeminiProvider.ResponsesStream.streamIdleTimeoutInSeconds intparameter toHandleGeminiChatCompletionStreamandHandleGeminiResponsesStream.SetStreamIdleTimeoutIfEmptyto the top of each shared handler so it is always executed.VertexProvider.ChatCompletionStreamandVertexProvider.ResponsesStreamto passprovider.networkConfig.StreamIdleTimeoutInSecondsto the shared handlers, fixing the missing timeout for Vertex streaming calls.Type of change
Affected areas
How to test
Verify that streaming requests through the Vertex provider respect the configured
StreamIdleTimeoutInSecondsby setting a short timeout and confirming the stream is terminated after the idle period elapses.go test ./...Breaking changes
Related issues
Security considerations
No security implications.
Checklist
docs/contributing/README.mdand followed the guidelines