chore: add opt-in retry for rate-limited and transport-failed requests#278
Conversation
Adds RetryConfig(enabled, max_attempts, max_backoff) plumbed through Stream/AsyncStream via the same getattr-based sub-client pattern as the pool knobs. Disabled by default (single attempt, errors unchanged). When enabled, retries GET/HEAD on HTTP 429 (unless unrecoverable) or a transport error, honoring Retry-After when present, else full jitter over an exponential backoff. Reconciles with the CHA-2957 logging: the retry loop now owns http.request.failed, logging at DEBUG when it will retry and at ERROR only on a final transport failure (a final 429 stays uncovered by this event, as before, since http.response.received already logs it). Retried 429s omit error.type (transport-only enum); transport retries carry it. Deletes the tombstoned utils/retry.py.
The retry loops in _request_sync/_request_async dropped stream.endpoint_name and duration_ms from the loop-level http.request.failed log, breaking disabled-path logging parity with the pre-retry per-attempt log. Compute the endpoint once before the loop and time each attempt so both fields are carried on every DEBUG (retried) and ERROR (final) emission. Also strengthens test coverage: field-content assertions on the parity test, zero-http.request.failed assertions for non-retried/exhausted 429s (sync and async), async error.type present/absent assertions mirroring the sync ones, and a sync end-to-end retry_after clamp test.
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds opt-in retries for eligible GET/HEAD requests in synchronous and asynchronous clients, with configurable attempts, backoff, ChangesRequest retry behavior
Channel consistency test
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant StreamClient
participant RetryLoop
participant HTTPTransport
participant FailureLogger
StreamClient->>RetryLoop: send eligible GET or HEAD request
RetryLoop->>HTTPTransport: execute attempt
HTTPTransport-->>RetryLoop: 429 or transport failure
RetryLoop->>FailureLogger: log retry attempt at DEBUG
RetryLoop->>HTTPTransport: wait and execute next attempt
HTTPTransport-->>RetryLoop: success or final failure
RetryLoop->>FailureLogger: log final transport failure at ERROR
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
getstream/stream.py (1)
306-323: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
clone_for_tokendrops theretryconfig.Every other client-copying setting (
max_conns_per_host,idle_timeout,connect_timeout,user_agent,logger,log_bodies) is forwarded here, butretryis not. A client constructed withretry=RetryConfig(enabled=True, ...)silently reverts to no-retry behavior for any sibling produced byclone_for_token.🐛 Proposed fix
return self.__class__( api_key=self.api_key, token=token, base_url=self.base_url, request_timeout=self.request_timeout, max_conns_per_host=self.max_conns_per_host, idle_timeout=self.idle_timeout, connect_timeout=self.connect_timeout, user_agent=self.user_agent, logger=self.log, log_bodies=self.log_bodies, + retry=self.retry, )🤖 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 `@getstream/stream.py` around lines 306 - 323, Update clone_for_token to forward this client's retry configuration when constructing the sibling client, preserving the existing retry behavior alongside the other copied settings.
🧹 Nitpick comments (3)
getstream/base.py (1)
404-469: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSync/async retry loops duplicate the eligibility check and failed-log
extradict construction.
_request_syncand_request_asyncare structurally identical apart fromtime.sleepvsawait asyncio.sleep. Consider extracting a small helper (e.g. building theextradict for both the DEBUG/ERROR branches) shared by both, to avoid the two copies drifting out of sync on future changes (e.g. new log fields).Also applies to: 816-875
🤖 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 `@getstream/base.py` around lines 404 - 469, Refactor the duplicated retry failure logging in _request_sync and _request_async by extracting a shared helper for constructing the failed-request extra fields, including transport-specific error.type and timing/attempt data. Use the helper in both DEBUG retry logs and final ERROR transport logs, while preserving their existing eligibility checks and sleep behavior.tests/test_retry.py (2)
188-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing async mirrors for transport-exhaustion and disabled-logging-fidelity tests.
test_transport_exhaustion_logs_final_errorandtest_disabled_transport_failure_logs_identical_to_todayhave no async counterparts, even though_request_async/_attempt_asyncingetstream/base.pyduplicate this exact logic independently of the sync path. An async-only regression in either scenario (e.g. wrong log level on final transport exhaustion) wouldn't be caught.🤖 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 `@tests/test_retry.py` around lines 188 - 243, Add async counterparts to test_transport_exhaustion_logs_final_error and test_disabled_transport_failure_logs_identical_to_today, exercising the client’s async request path with the same transport failures and logging assertions. Use the async API and await the request, while preserving equivalent call counts, exception expectations, log levels, structured fields, and disabled-retry fidelity checks.
42-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign
test_retry.pywith the test conventions.Refactor this file to use pytest fixtures instead of
sync_client/async_client, group related retry tests under shared test classes, and route credentials through the existing.env-loaded fixtures.httpx.MockTransportis reasonable for deterministic HTTP sequencing here, but the helper/class organization still needs these updates.🤖 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 `@tests/test_retry.py` around lines 42 - 65, Refactor the retry tests around sync_client and async_client to use pytest fixtures, reusing the existing .env-loaded credential fixtures instead of hardcoded credentials. Group related synchronous and asynchronous retry cases into shared test classes, while retaining httpx.MockTransport for deterministic request sequencing and the current sleep monkeypatch behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In `@getstream/stream.py`:
- Around line 306-323: Update clone_for_token to forward this client's retry
configuration when constructing the sibling client, preserving the existing
retry behavior alongside the other copied settings.
---
Nitpick comments:
In `@getstream/base.py`:
- Around line 404-469: Refactor the duplicated retry failure logging in
_request_sync and _request_async by extracting a shared helper for constructing
the failed-request extra fields, including transport-specific error.type and
timing/attempt data. Use the helper in both DEBUG retry logs and final ERROR
transport logs, while preserving their existing eligibility checks and sleep
behavior.
In `@tests/test_retry.py`:
- Around line 188-243: Add async counterparts to
test_transport_exhaustion_logs_final_error and
test_disabled_transport_failure_logs_identical_to_today, exercising the client’s
async request path with the same transport failures and logging assertions. Use
the async API and await the request, while preserving equivalent call counts,
exception expectations, log levels, structured fields, and disabled-retry
fidelity checks.
- Around line 42-65: Refactor the retry tests around sync_client and
async_client to use pytest fixtures, reusing the existing .env-loaded credential
fixtures instead of hardcoded credentials. Group related synchronous and
asynchronous retry cases into shared test classes, while retaining
httpx.MockTransport for deterministic request sequencing and the current sleep
monkeypatch behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d862bde-f2c2-485e-9a4b-a1fd1d7f588f
📒 Files selected for processing (7)
README.mdgetstream/__init__.pygetstream/base.pygetstream/config.pygetstream/stream.pygetstream/utils/retry.pytests/test_retry.py
💤 Files with no reviewable changes (1)
- getstream/utils/retry.py
…hannel_partial
The channel echoed in an update_channel_partial response can be hydrated from a read replica that lags the write, so test_update_channel_partial intermittently observed the pre-update color ('blue' instead of 'red'). Re-read the channel with a short retry until the state converges instead of asserting on the write response.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_chat_channel.py (1)
109-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPoll with the read-only
channel.get()endpoint.
get_or_create()can create the channel when setup is missing, masking fixture or persistence regressions. Use the dedicatedchannel.get()method for each poll so this test only verifies persisted state.Proposed change
- custom = channel.get_or_create().data.channel.custom or {} + custom = channel.get().data.channel.custom or {}🤖 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 `@tests/test_chat_channel.py` around lines 109 - 110, Update the polling loop in the relevant test to call the read-only channel.get() method instead of channel.get_or_create(), while preserving the existing custom-data check and retry behavior so the test verifies persisted state without creating missing fixtures.
🤖 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 `@tests/test_chat_channel.py`:
- Around line 109-110: Update the polling loop in the relevant test to call the
read-only channel.get() method instead of channel.get_or_create(), while
preserving the existing custom-data check and retry behavior so the test
verifies persisted state without creating missing fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c0de80ca-9855-41db-a809-5775d60a44d9
📒 Files selected for processing (1)
tests/test_chat_channel.py
event_hooks is app-global and CI runs the Python matrix legs (3.10/3.11/3.12) in parallel against one shared app, so reading back the global hook count races another leg's in-flight update_app, which fully replaces the list. The test still exercises SDK serialization of the sqs/sns/pending hook variants (a bad payload raises) and the app round-trip; only the unverifiable cross-process count assertion is removed.
1 participant
Ticket
https://linear.app/stream/issue/CHA-2959/rate-limits-and-retry
Summary
Adds an opt-in auto-retry policy.
RetryConfigis disabled by default (maxAttempts=3,maxBackoff=30s). When enabled, retries ONLYGET/HEADrequests that fail with HTTP 429 or a transport error, honoringRetry-After(clamped tomaxBackoff) otherwise exponential backoff with full jitter. Never retries writes or 5xx; always honors the backendunrecoverableflag; surfaces the last attempt's error. Config surface:retry=RetryConfig(...)onStream/AsyncStream(sync + async)Notes
http.request.failedlog event with aretry.attemptfield (transport retries carryerror.type; 429 retries omit it, since that field is the transport-only enum).chore:intentionally so the merge does not auto-publish. Publish later via the manual Release workflow (workflow_dispatch,minor).Summary by CodeRabbit
GET/HEADrequests on HTTP 429 and eligible transport errors.Retry-Afterwhen available, otherwise full-jitter exponential backoff with a maximum delay.RetryConfig(exported from the top-level package) and supported on client/sub-clients.