fix(extension_manager): apply configured timeout to StreamableHttp reqwest client - #9101
fix(extension_manager): apply configured timeout to StreamableHttp reqwest client#9101clouatre wants to merge 3 commits into
Conversation
…qwest client Issues aaif-goose#9100 and aaif-goose#9022: create_streamable_http_client built its reqwest::Client without .timeout(), so the extension timeout from config.yaml was resolved but silently discarded at the HTTP transport layer. The OS TCP default (~30s on Linux) fired instead. - Compute timeout_duration via resolve_timeout() before both reqwest::Client builds - Add .timeout(timeout_duration) to primary and OAuth fallback HTTP clients - Make resolve_timeout pub(crate) for test access - Add unit tests for resolve_timeout behavior and ExtensionConfig serde roundtrip Fixes aaif-goose#9100 Fixes aaif-goose#9022 Signed-off-by: Hugues Clouâtre <hugues@linux.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6845efbf37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… serde test external The two resolve_timeout unit tests require access to a private function. Move them into the existing #[cfg(test)] module in extension_manager.rs where they can call it directly. Keep streamable_http_config_timeout_roundtrips in tests/ as it only uses public API (ExtensionConfig + serde_json). Reverts pub(crate) visibility added in previous commit; resolve_timeout remains private. Fixes inline review comment on PR aaif-goose#9101. Signed-off-by: Hugues Clouâtre <hugues@linux.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7df084c85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… HTTP clients Using .timeout() on the shared reqwest::Client caps all requests including the long-lived SSE GET stream, causing periodic disconnects for streamable HTTP extensions that run longer than the configured timeout (default 300s). Switch to .connect_timeout() which bounds only TCP connection establishment, leaving streaming responses unaffected. The MCP-layer timeout passed to McpClient::connect() already governs RPC call duration. Addresses review comment on PR aaif-goose#9101. Signed-off-by: Hugues Clouâtre <hugues@linux.com>
|
Thanks for looking into this, but this PR doesn't actually fix the bug described in #9022. The issue is that reqwest defaults
The tests are also trivial (testing If you'd like to take another pass at this, the fix is straightforward: add |
2 participants
Summary
create_streamable_http_clientbuilt itsreqwest::Clientwithout a connection timeout, so the extension timeout fromconfig.yamlwas correctly resolved byresolve_timeout()but silently discarded at the HTTP transport layer. The OS TCP default (~30s on Linux) fired instead, regardless of the configured value.This is the root cause of #9022 (streamable HTTP calls reset after ~30s on Linux). The session persistence path (
EnabledExtensionsState) is correct; the bug was exclusively in HTTP client construction.Changes
crates/goose/src/agents/extension_manager.rs: computetimeout_durationviaresolve_timeout()once before bothreqwest::Client::builder()calls; add.connect_timeout(timeout_duration)to the primary client and the OAuth fallback client..connect_timeout()bounds TCP connection establishment without capping long-lived SSE streams (using.timeout()would disconnect SSE streams every 300s).crates/goose/tests/extension_timeout_test.rs(new): serde roundtrip test forExtensionConfig::StreamableHttptimeout field (public API only).crates/goose/src/agents/extension_manager.rs#[cfg(test)]module: two unit tests forresolve_timeoutwith a per-extension value and withNone(default fallback). Placed here sinceresolve_timeoutis private.The fix follows the existing pattern used in
download_manager.rs(which also uses.connect_timeout()). Note:.tcp_user_timeout()suggested in #9022 is not available in reqwest 0.13.Test plan
cargo test -p goose)cargo clippy --all-targets -- -D warnings)aptu scan_security)Fixes #9022