Enable retry for POST requests in streaming error handling#523
Enable retry for POST requests in streaming error handling#523brunoluigi wants to merge 1 commit into
Conversation
- Add POST method to Faraday retry configuration (not retried by default) - Add tests verifying retry behavior for both Faraday 1.x and 2.x code paths - Add `expected_url_for` helper for URL assertions in error tests - Reduce retry intervals in test configuration for faster test execution
| RSpec.describe RubyLLM::Chat do | ||
| include_context 'with configured RubyLLM' | ||
|
|
||
| before do |
There was a problem hiding this comment.
I've done this since those tests running with retry enable were changing the VCR recorded requests and I wasn't sure if it was supposed to happen, need some help here =)
| config.retry_interval = 1 | ||
| config.retry_backoff_factor = 3 | ||
| config.retry_interval_randomness = 0.5 | ||
| config.max_retries = 2 |
There was a problem hiding this comment.
Once retry is enabled the tests at spec/ruby_llm/chat_streaming_spec.rb:57 starts to wait between retries and with the current settings it takes a lot of time so I adjusted to something more adequate for test environments.
One question is, should the faraday-retry middleware actually wait in the test environment or it could be set to simply skip the waiting and retry right away?
| end.to raise_error(expected_error_for(provider)) | ||
| end | ||
|
|
||
| it 'retries the request' do |
There was a problem hiding this comment.
Inspired by the test that @AlexVPopov suggested in the issue #417
|
Thanks for jumping on this and for digging into the Faraday v1 test hang. The root cause analysis around test backoff settings was very helpful. I verified the core issue locally and agree with enabling If you’re open to it, a minimal merge path could be:
Happy to jump in and do these changes myself if you can't. |
This follows up on #523 with a minimal patch set: - enable retries for POST while preserving Faraday default idempotent methods - map HTTP 504 to `RubyLLM::ServiceUnavailableError` (so retry classification is consistent) - reduce retry backoff settings in test config to avoid long sleeps in retry/error specs - add focused specs for retry config and 502/503/504 error mapping Related: #417
|
My apologies for the above PR @brunoluigi. I asked codex to integrate my changes in your PR but it went ahead and create a new PR and merged it 🤦♂. I'll credit you properly in the release notes. |
2 participants
What this does
expected_url_forhelper for URL assertions in error testsType of change
Scope check
Quality check
overcommit --installand all hooks passbundle exec rake vcr:record[provider_name]bundle exec rspecmodels.json,aliases.json)API changes
Related issues
Fixes #417