Skip to content

Only retry the Drive 403 usage limits that clear by waiting - #1603

Open
pavlosambur wants to merge 1 commit into
burnash:masterfrom
pavlosambur:fix/backoff-non-retryable-usage-limits
Open

pavlosambur wants to merge 1 commit into
burnash:masterfrom
pavlosambur:fix/backoff-non-retryable-usage-limits

Conversation

@pavlosambur

Copy link
Copy Markdown

BackOffHTTPClient decides whether a Drive 403 is retryable from the error domain alone:

if code == HTTPStatus.FORBIDDEN and error["errors"][0]["domain"] == "usageLimits":
    return within_max_backoff

Google's Drive error guide puts four reasons in the usageLimits domain, and only two of them clear by waiting:

reason domain what Google says to do
rateLimitExceeded usageLimits use exponential backoff to retry
userRateLimitExceeded usageLimits use exponential backoff to retry
dailyLimitExceeded usageLimits remove the usage cap on "Queries per day"
UrlLeaseLimitExceeded usageLimits complete or cancel the pending uploads

The 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.sleep patched so the wait is counted rather than spent:

reason                        domain        attempts   slept
rateLimitExceeded             usageLimits          8    254s   correct
userRateLimitExceeded         usageLimits          8    254s   correct
dailyLimitExceeded            usageLimits          8    254s   should raise at once
UrlLeaseLimitExceeded         usageLimits          8    254s   should raise at once
sharingRateLimitExceeded      global               1      0s   correct
insufficientFilePermissions   global               1      0s   correct
storageQuotaExceeded          global               1      0s   correct

So a call that hit the daily quota spends 254 seconds across 8 attempts before the APIError reaches the caller.

The change

Match on reason and retry only the two rate limits. This is the check described in #1268:

I found the following article from google drive doc that mentions a keyword we can match userRateLimitExceeded located in the error message under error.errors[0].reason if we find the error code 403 we should look for that value, if present then delay the request if not just keep raising the error.

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 usageLimits later 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_backoff bound 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 with dailyLimitExceeded or UrlLeaseLimitExceeded must raise after one attempt without sleeping. On master it fails with AssertionError: 8 != 1.
  • test_retries_user_rate_limit_exceeded — a 403 with userRateLimitExceeded still backs off and then succeeds. This one passes both before and after; it is here to catch a future narrowing that goes too far.
  • The shared _error_response helper takes an optional reason and 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 with domain="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 with reason="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 the google.rpc.Status shape (code/message/status, sometimes details) with no errors array. Every 4xx body recorded in tests/cassettes/ is that shape, including a real 403 PERMISSION_DENIED.

Verification

  • tox -e lint,py,build,doc — all green (168 tests passed).
  • The suite also passes on Python 3.8 and 3.9 (168 tests each), the ends of the CI matrix.
  • The new regression test fails on master and passes on this branch.

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: global and 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.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant