fix: Honor explicit timeout timedelta larger than timeout_max - #962
fix: Honor explicit timeout timedelta larger than timeout_max#962vdusek wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #962 +/- ##
==========================================
+ Coverage 94.64% 94.66% +0.01%
==========================================
Files 58 58
Lines 5263 5263
==========================================
+ Hits 4981 4982 +1
+ Misses 282 281 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Pijukatel
left a comment
There was a problem hiding this comment.
Can you please verify in the API code that a timeout larger than 360 s has any meaning?
If there is some hard-coded or implementation limit on the API side, it makes no sense to lift the limit in the client.
(But a warning might be good in those cases)
|
Probably, there are mostly idle timeouts, not wall-clock timeouts, so this could make sense. See:
I don't want to waste much time here. IMO, silently truncating it is definitely wrong. If we don't want to allow longer timeouts, we should at least log a warning. However, allowing longer timeouts wouldn't be harmful anyway. I'll wait for your response, but I don't have a strong preference here. |
An explicit per-call
timeouttimedelta larger thantimeout_max(default 360 s) was silently clamped totimeout_maxon every attempt, contradicting the documented behavior "Atimedeltaoverrides it for this call". For example,dataset.get_items_as_bytes(timeout=timedelta(minutes=30))ran every attempt with a 360 s timeout, causing repeatedimpit.TimeoutExceptionand failing the call despite the explicit 30-minute request.Root cause:
_compute_timeoutusedmin(resolved * 2**(attempt-1), timeout_max), somin()shrank the resolved base timeout totimeout_maxeven on attempt 1, before any exponential growth. The fix caps growth atmax(timeout_max, resolved)instead, so the base timeout is never shrunk below itself while retry growth stays bounded.✍️ Drafted by Claude Code