🐛 Bugfix: Resolve the 502 Bad Gateway issue when calling multimodal tool#2811
🐛 Bugfix: Resolve the 502 Bad Gateway issue when calling multimodal tool#2811
Conversation
xuyaqist
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to fix intermittent 502 Bad Gateway errors when invoking the multimodal tool by adjusting how shared httpx clients are constructed.
Changes:
- Disable
httpxenvironment-derived configuration by settingtrust_env=Falseon shared sync clients. - Disable
httpxenvironment-derived configuration by settingtrust_env=Falseon shared async clients.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| limits=Limits( | ||
| max_connections=100, | ||
| max_keepalive_connections=20 | ||
| ) | ||
| ), | ||
| trust_env=False |
There was a problem hiding this comment.
base_url is used to key the client cache and the module docstring shows relative requests (e.g., client.get("/api/endpoint")), but the created httpx.Client isn't configured with base_url=.... That makes relative URL requests fail and also allows the cached client to be used for other hosts, undermining the per-base_url pooling intent. Consider passing base_url=base_url when constructing the client (and/or updating the docstring if only absolute URLs are supported).
| max_keepalive_connections=20 | ||
| ) | ||
| ), | ||
| trust_env=False |
There was a problem hiding this comment.
Setting trust_env=False globally changes behavior for all SDK HTTP calls by ignoring standard env configuration (e.g., HTTP(S)_PROXY / NO_PROXY and CA bundle env vars). If this is intended only to work around a specific 502/proxy issue, consider making it configurable (e.g., a trust_env parameter on get_*_client, or a library env flag) so proxy-based deployments don’t silently break.
| max_keepalive_connections=20 | ||
| ) | ||
| ), | ||
| trust_env=False |
There was a problem hiding this comment.
There are existing unit tests for HttpClientManager, but none assert the newly introduced trust_env=False behavior. Add a test that verifies httpx.Client(...) is constructed with trust_env=False (e.g., by patching httpx.Client and asserting call args) so future refactors don’t regress this fix.
| limits=Limits( | ||
| max_connections=100, | ||
| max_keepalive_connections=20 | ||
| ) | ||
| ), | ||
| trust_env=False |
There was a problem hiding this comment.
Same issue as the sync client: the httpx.AsyncClient is keyed by base_url but isn't constructed with base_url=..., so relative URL requests (as suggested by this module’s docstring) won’t work and the cached client can be used across hosts. Consider passing base_url=base_url here as well for consistency and correctness.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Uh oh!
There was an error while loading. Please reload this page.