Repository navigation
Only retry the Drive 403 usage limits that clear by waiting - #1603
Open
pavlosambur wants to merge 1 commit into
Open
pavlosambur wants to merge 1 commit into
pavlosambur wants to merge 1 commit into
Conversation
BackOffHTTPClient decides a 403 is retryable from the error domain alone, so every reason in the 'usageLimits' domain is retried. Two of the four reasons Google puts in that domain cannot clear by waiting: 'dailyLimitExceeded' needs the usage cap raised and resets at midnight Pacific, and 'UrlLeaseLimitExceeded' needs the pending uploads finished or cancelled. Both burn the full backoff (8 attempts, 254 seconds) before surfacing an error that was never going to go away. Match on the error reason instead, retrying only 'rateLimitExceeded' and 'userRateLimitExceeded'. This is the check described in issue burnash#1268 before the domain-level one landed in burnash#1415. The shared test helper now takes an optional reason, and test_gives_up_after_max_backoff_on_usage_limits builds its 403 with 'rateLimitExceeded' so it still covers a retryable error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BackOffHTTPClientdecides whether a Drive 403 is retryable from the error domain alone:Google's Drive error guide puts four reasons in the
usageLimitsdomain, and only two of them clear by waiting:rateLimitExceededusageLimitsuserRateLimitExceededusageLimitsdailyLimitExceededusageLimitsUrlLeaseLimitExceededusageLimitsThe daily quota resets at midnight Pacific and the URL lease limit needs the caller to finish or cancel its uploads. Neither is affected by sleeping, so the retries only postpone an error that was always going to be raised.
Reproduced on master
A session that always returns the given 403, with
time.sleeppatched so the wait is counted rather than spent:So a call that hit the daily quota spends 254 seconds across 8 attempts before the
APIErrorreaches the caller.The change
Match on
reasonand retry only the two rate limits. This is the check described in #1268:That issue was closed by 46d7538 ("Improve back-off client", #1415), which introduced the
domain == "usageLimits"check that is in the code today. Before #1415 every 403 was retried, so this is a further step in the same direction rather than a change of approach.The retryable reasons are an allow-list, so a reason Google adds to
usageLimitslater would raise immediately until it is listed. That seemed better than a deny-list, which would silently keep burning the full backoff on any new terminal reason — and it is what #1268 describes.The
within_max_backoffbound from #1599 is untouched: a retryable reason still stops after 8 attempts.Tests
tests/http_client_test.py:test_does_not_retry_terminal_usage_limits— a 403 withdailyLimitExceededorUrlLeaseLimitExceededmust raise after one attempt without sleeping. On master it fails withAssertionError: 8 != 1.test_retries_user_rate_limit_exceeded— a 403 withuserRateLimitExceededstill backs off and then succeeds. This one passes both before and after; it is here to catch a future narrowing that goes too far._error_responsehelper takes an optionalreasonand omits the key when it is not given, so every existing error body is unchanged.One existing test needed a change, which is worth flagging rather than leaving to be noticed:
test_gives_up_after_max_backoff_on_usage_limits(added in #1599) built its 403 withdomain="usageLimits"and no reason at all, which under a reason-based check is no longer a retryable error, so it saw 1 attempt instead of 8. It now builds the error withreason="rateLimitExceeded"and still covers exactly what it was written for — the backoff bound on a retryable usage limit.The Sheets API is not affected: this branch is guarded by
if "errors" in error, and Sheets returns thegoogle.rpc.Statusshape (code/message/status, sometimesdetails) with noerrorsarray. Every 4xx body recorded intests/cassettes/is that shape, including a real 403PERMISSION_DENIED.Verification
tox -e lint,py,build,doc— all green (168 tests passed).One follow-up I would rather send separately: the class docstring still lists as a known issue that it "will retry exponentially even when the error should raise instantly. Due to the Drive API that raises 403 (Forbidden) errors for forbidden access". Forbidden access carries
domain: globaland has been filtered out since #1415, and this PR removes the remaining case, so the note no longer describes the code. Happy to open that as a small docs-only PR once this is settled.Worked on this with Claude as an assistant; the reasoning above is mine and I ran everything locally.