fix: add default HTTP request timeout to prevent indefinite hangs#938
fix: add default HTTP request timeout to prevent indefinite hangs#938Alfredo Garcia (agarctfi) wants to merge 3 commits intomainfrom
Conversation
When an HTTP server accepts a connection but never responds (or stalls mid-transfer), the requests library will block indefinitely if no timeout is set. This causes connector syncs to hang permanently with no log output, no error, and no retry. This adds a default timeout of (30s connect, 300s read) to all HTTP requests made via HttpClient.send_request(). The timeout is only applied when no explicit timeout is provided in request_kwargs, so callers can still override it. ConnectTimeout and ReadTimeout exceptions are already handled as TRANSIENT_EXCEPTIONS in rate_limiting.py and will be retried automatically with exponential backoff. Root cause: observed in source-intercom where a sync hung permanently after an Intercom API 500 error. The retry succeeded but a subsequent request hung indefinitely because no timeout was configured. Co-Authored-By: alfredo.garcia@airbyte.io <freddy.garcia7.fg@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1772830465-add-default-http-timeout#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1772830465-add-default-http-timeoutPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
Pull request overview
Adds a default requests timeout to outbound HTTP calls made through airbyte_cdk’s HttpClient.send_request() to prevent indefinite hangs when servers stall mid-request/response.
Changes:
- Introduce default connect/read timeout constants on
HttpClient. - Apply a default
(connect, read)timeout tuple torequest_kwargswhen no explicittimeoutis provided.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| request_kwargs = {**request_kwargs, **env_settings} | ||
|
|
||
| if "timeout" not in request_kwargs: | ||
| request_kwargs["timeout"] = (self._DEFAULT_CONNECT_TIMEOUT, self._DEFAULT_READ_TIMEOUT) | ||
|
|
||
| response: requests.Response = self._send_with_retry( | ||
| request=request, | ||
| request_kwargs=request_kwargs, |
There was a problem hiding this comment.
request_kwargs is typed as Mapping[str, Any], but this code mutates it via request_kwargs["timeout"] = .... With the repo's strict mypy settings, this will raise a type error because Mapping is read-only. Consider copying into a Dict[str, Any]/MutableMapping[str, Any] local (or changing the parameter type) before setting defaults so mypy passes.
| request_kwargs = {**request_kwargs, **env_settings} | |
| if "timeout" not in request_kwargs: | |
| request_kwargs["timeout"] = (self._DEFAULT_CONNECT_TIMEOUT, self._DEFAULT_READ_TIMEOUT) | |
| response: requests.Response = self._send_with_retry( | |
| request=request, | |
| request_kwargs=request_kwargs, | |
| mutable_request_kwargs: Dict[str, Any] = {**request_kwargs, **env_settings} | |
| if "timeout" not in mutable_request_kwargs: | |
| mutable_request_kwargs["timeout"] = (self._DEFAULT_CONNECT_TIMEOUT, self._DEFAULT_READ_TIMEOUT) | |
| response: requests.Response = self._send_with_retry( | |
| request=request, | |
| request_kwargs=mutable_request_kwargs, |
| if "timeout" not in request_kwargs: | ||
| request_kwargs["timeout"] = (self._DEFAULT_CONNECT_TIMEOUT, self._DEFAULT_READ_TIMEOUT) | ||
|
|
There was a problem hiding this comment.
This change introduces new default-timeout behavior but there isn't a unit test asserting (a) the default timeout is applied when request_kwargs has no timeout, and (b) an explicit timeout is respected. Adding a focused test would help prevent regressions (especially since this behavior is safety-critical for avoiding indefinite hangs).
- Use Dict[str, Any] (mutable) instead of mutating Mapping[str, Any] for request_kwargs to satisfy mypy type checking - Add test_send_request_applies_default_timeout_when_not_provided - Add test_send_request_respects_explicit_timeout Co-Authored-By: alfredo.garcia@airbyte.io <freddy.garcia7.fg@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded two class-level defaults to HttpClient ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-Authored-By: alfredo.garcia@airbyte.io <freddy.garcia7.fg@gmail.com>
|
Closing this PR — the connector is pinned to a dev image from PR #870, so this fix needs to be applied on top of that branch instead. A new PR will be created targeting the |
Summary
Adds a default timeout of (30s connect, 300s read) to all HTTP requests made via
HttpClient.send_request(). Currently, no timeout is set on outbound HTTP requests, which means if a server accepts a TCP connection but never responds (or stalls mid-transfer), the connector process hangs indefinitely — no error, no log output, no retry.Root cause: Observed in production on
source-intercomwhere a sync hung permanently after an Intercom API 500 error. The CDK's retry logic succeeded, but a subsequent request to the Intercom API hung with zero output for 80+ minutes. Therequests.Session.send()call blocked forever because notimeoutparameter was passed.The timeout is only applied when no explicit
timeoutis already present inrequest_kwargs, so callers can still override it.ConnectTimeoutandReadTimeoutare already handled asTRANSIENT_EXCEPTIONSinrate_limiting.pyand will be retried automatically with exponential backoff.Review & Testing Checklist for Human
HttpRequester) so connectors can tune it?send_requestis the right interception point: The timeout is added insend_request()only. If any code path calls_send_with_retry()or_send()directly, those requests won't get a default timeout.Notes
Summary by CodeRabbit