feat: handle stream error codes 401, 409, 429 - #353
Conversation
- 401 (unauthorized): disable auto-reconnect, dispatch LoggedOut - 409 (conflict): disable auto-reconnect, dispatch StreamReplaced - 429 (rate limited): keep auto-reconnect, increase backoff by 5 steps
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdded handling for three stream error codes (401 unauthorized, 409 conflict, 429 rate limited) in the client with specific recovery actions: 401 and 409 disable auto-reconnect and dispatch respective events, while 429 keeps auto-reconnect enabled and increases backoff delay. Includes tests for all three codes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (2)
src/client.rs (2)
4639-4654: Consolidate duplicated test-client helpers.
create_test_client()duplicatescreate_offline_sync_test_client()setup. Reusing one helper will keep test setup changes centralized.♻️ Suggested simplification
- async fn create_test_client() -> Arc<Client> { - let backend = crate::test_utils::create_test_backend().await; - let pm = Arc::new( - PersistenceManager::new(backend) - .await - .expect("persistence manager should initialize"), - ); - let (client, _rx) = Client::new( - pm, - Arc::new(crate::transport::mock::MockTransportFactory::new()), - Arc::new(MockHttpClient), - None, - ) - .await; - client - } + // Reuse existing create_offline_sync_test_client()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client.rs` around lines 4639 - 4654, create_test_client() duplicates the setup in create_offline_sync_test_client(); replace the duplicate by reusing the existing helper to centralize test setup. Locate the create_test_client function and change it to call create_offline_sync_test_client() (or extract the shared setup into a new helper used by both) instead of repeating the PersistenceManager::new(...) and Client::new(...) logic; ensure the returned type remains Arc<Client> and any required mocks (MockTransportFactory, MockHttpClient) are provided by the reused helper or moved into the shared helper used by both functions.
4656-4704: Strengthen stream-error tests for event semantics and exact backoff delta.These tests cover reconnect flags well, but they currently don’t assert emitted events for 401/409, and
429uses a>=check that can hide regressions.✅ Minimal assertion hardening for 429
- assert!( - after >= before + 5, - "429 should increase backoff: before={before}, after={after}" - ); + assert_eq!( + after, + before + 5, + "429 should increase backoff by exactly 5: before={before}, after={after}" + );Consider also adding event-bus assertions:
401dispatchesEvent::LoggedOut409dispatchesEvent::StreamReplaced🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client.rs` around lines 4656 - 4704, The tests calling client.handle_stream_error (test_stream_error_401_disables_reconnect, test_stream_error_409_disables_reconnect, test_stream_error_429_keeps_reconnect_with_backoff) should also assert the expected events and tighten the 429 backoff check: after invoking client.handle_stream_error(&node) add an assertion that the client's event bus emits Event::LoggedOut for code "401" and Event::StreamReplaced for code "409" (use the test client's event receiver/queue to pop the emitted event), and change the 429 backoff assertion to require an exact increment of +5 to client.auto_reconnect_errors (i.e., assert after == before + 5) while keeping the enable_auto_reconnect checks using client.enable_auto_reconnect.load(Ordering::Relaxed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client.rs`:
- Around line 4639-4654: create_test_client() duplicates the setup in
create_offline_sync_test_client(); replace the duplicate by reusing the existing
helper to centralize test setup. Locate the create_test_client function and
change it to call create_offline_sync_test_client() (or extract the shared setup
into a new helper used by both) instead of repeating the
PersistenceManager::new(...) and Client::new(...) logic; ensure the returned
type remains Arc<Client> and any required mocks (MockTransportFactory,
MockHttpClient) are provided by the reused helper or moved into the shared
helper used by both functions.
- Around line 4656-4704: The tests calling client.handle_stream_error
(test_stream_error_401_disables_reconnect,
test_stream_error_409_disables_reconnect,
test_stream_error_429_keeps_reconnect_with_backoff) should also assert the
expected events and tighten the 429 backoff check: after invoking
client.handle_stream_error(&node) add an assertion that the client's event bus
emits Event::LoggedOut for code "401" and Event::StreamReplaced for code "409"
(use the test client's event receiver/queue to pop the emitted event), and
change the 429 backoff assertion to require an exact increment of +5 to
client.auto_reconnect_errors (i.e., assert after == before + 5) while keeping
the enable_auto_reconnect checks using
client.enable_auto_reconnect.load(Ordering::Relaxed).
Bencher Report
1 participant
Fixes #342
Summary
LoggedOut— session is invalidStreamReplaced— another client connectedCross-referenced against WA Web
WAWebHandleStreamErrorbehavior.Test plan
cargo fmt --allcleancargo clippy --all --testscleanSummary by CodeRabbit