Skip to content

Fix _retry_with_backoff never retrying rate-limit errors - #875

Open
jeszyman wants to merge 2 commits into
insanum:mainfrom
jeszyman:fix-rate-limit-backoff
Open

jeszyman wants to merge 2 commits into
insanum:mainfrom
jeszyman:fix-rate-limit-backoff

Conversation

@jeszyman

@jeszyman jeszyman commented Oct 17, 2026 •

Copy link
Copy Markdown

Fixes #289.

_retry_with_backoff compares the error code to the string '403', but the Calendar API returns it as a number ("code": 403), so the check never matches and a rate-limited request fails on its first attempt. This shows up when importing more than a few dozen events: some of them fail with "Rate Limit Exceeded" (also reported in #833).

Changes:

  • Compare the code as a number, and also accept 429, which the Calendar API returns with the same rateLimitExceeded reason (Calendar API error guide).
  • Re-raise on the last attempt instead of falling through to return None. That path could not be reached before; with retries working, ImportICS would call .get() on None when a request was still rate-limited after max_retries attempts.
  • Tests for the three rate-limit responses, for the raise after the last attempt, and for a non-rate-limit error (409 duplicate), which is still raised without a retry.

Testing: pytest 68 passed and ruff check is clean; the four new rate-limit tests fail on main. On a real calendar, importing the same 39 already-present events twice, 4.5.1 lost 4 and 12 events to rate limiting and the patched version lost 0 and 0.


Summary

Fixes _retry_with_backoff so that it actually retries rate-limit errors instead of falling through to an immediate re-raise.

Problem

The retry helper compared the API error code against the string '403', while the Calendar API returns the code as a number. As a result, the rate-limit branch was never taken and rate-limited requests were raised immediately rather than being retried with backoff. The helper also unconditionally indexed into error['errors'], which could fail on error bodies that do not include an errors list.

Changes

gcalcli/gcal.py

  • The error code is now compared against the numeric values 403 and 429, so rate-limit detection works with the actual API responses.
  • Rate-limit retries now apply to both 403 and 429 responses, matching the reasons rateLimitExceeded and userRateLimitExceeded.
  • The final attempt no longer sleeps; instead the error is re-raised so callers receive the actual HttpError rather than a silent None.
  • Access to the errors list is now null-safe, so error bodies without an errors list are re-raised unchanged instead of causing an indexing failure.

tests/test_gcalcli.py

  • Added an HttpError factory and a FailingRequest test double that raises a configurable number of times before succeeding.
  • Added tests covering: successful retry after rate-limit errors (403/429 with the rate-limit reasons), re-raising after the last allowed attempt, not retrying unrelated errors (e.g. 409 duplicate), and raising immediately for responses that have no errors list.

Known limitation (from review)

Rate-limit responses that omit the errors list (for example 429 with status: RESOURCE_EXHAUSTED) are still not retried and are re-raised immediately. This behavior is currently asserted by a test, so the fix does not yet cover the newer Google error format for those cases.

_retry_with_backoff compared the error code to the string '403', but the
Calendar API returns it as a number, so rate-limited requests were never
retried. Compare the number, also accept 429 (the same rateLimitExceeded
reason per the Calendar API error guide), and re-raise on the last attempt
so callers get the error instead of None.

Fixes insanum#289.
@kody-ai

kody-ai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Automatic Review: Kody reviews this PR automatically on every push.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ❌
Business Logic ✅

Access your configuration settings here.

@kody-ai

kody-ai Bot commented Oct 17, 2026

Copy link
Copy Markdown

kody code-review Business Logic medium

Business Rules Validation

Task: 211940595 - RetryWithBackoff
Task Link: #289

Status: Compliant
Confidence: high

Note: No structured acceptance criteria were provided. Validation is based on the issue description in FULL_TASK_CONTEXT.

Findings

INFO: Reported code-type mismatch is corrected

Requirement: "you check against '403' (string) while you should check to 403 (int) I believe" (issue description)
Missing in code: None.
Observation: The PR diff changes the comparison from error.get('code') == '403' to error.get('code') in (403, 429) in gcalcli/gcal.py (_retry_with_backoff). This directly addresses the reported root cause. The addition of 429 is an extension beyond the issue text, but it is consistent with the same rate-limit handling and is covered by tests.
Suggested action: None required for the reported requirement.

INFO: Last-attempt behavior is made explicit

Requirement: "I couldn't get upload an ics file due to rateLimitExceeded. Even though you are using backing off. It appears though, it doesn't work in your program" (issue description)
Missing in code: None.
Observation: The PR diff adds n < self.max_retries - 1 and re-raises on the final attempt. This prevents the retry path from falling through to return None when the rate limit persists, which is consistent with making the backoff actually usable for the reported import failure. This behavior is also covered by the new test test_retry_with_backoff_raises_after_last_attempt.
Suggested action: None required.

Requirements Verified

  • Issue requirement: "you check against '403' (string) while you should check to 403 (int)" → Implemented in gcalcli/gcal.py in _retry_with_backoff — the code now checks error.get('code') in (403, 429).
  • Issue context: rate-limit reasons rateLimitExceeded and userRateLimitExceeded must trigger retry → Implemented in gcalcli/gcal.py — the existing reason list is preserved and now reachable with numeric codes.
  • Issue context: backoff should work for rate-limited requests → Implemented in tests/test_gcalcli.py — new tests cover 403/429 rate-limit responses, retries, and final-attempt raise behavior.

Analysis performed by Kodus AI Business Rules Validator


💡 This validation runs automatically only on the first review of a pull request. To run it again, comment @kody -v business-logic.

Comment thread gcalcli/gcal.py Resolved
A 403 or 429 body with no 'errors' list made _retry_with_backoff raise
TypeError instead of HttpError. Default the missing list so such errors
are re-raised unchanged, and test a 429 RESOURCE_EXHAUSTED body.
Comment thread gcalcli/gcal.py
Comment on lines +146 to +148
if error.get('code') in (403, 429) and \
(error.get('errors') or [{}])[0].get('reason') \
in ['rateLimitExceeded', 'userRateLimitExceeded'] and \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Bug high

Revises an earlier Kody suggestion on gcalcli/gcal.py:145 (2026-10-17 15:42 UTC).

The retry decision depends on the legacy errors[0].reason field, so a rate-limit response carrying its reason in the newer status/details form (e.g. 429 with "status": "RESOURCE_EXHAUSTED" and no errors list) is re-raised on the first attempt, causing gcalcli to fail imports with "Failed to import event" (gcal.py:1674-1698) and the test at tests/test_gcalcli.py:462-473 to enshrine that behavior. Treat the HTTP status or status field as an additional rate-limit signal: OR (error.get('errors') or [{}])[0].get('reason') in [...] with error.get('status') == 'RESOURCE_EXHAUSTED' (or e.resp.status == 429), and scan all errors entries instead of only index 0.

Prompt for LLM

File gcalcli/gcal.py:

Line 146 to 148:

**Revises an earlier Kody suggestion** on `gcalcli/gcal.py:145` (2026-10-17 15:42 UTC).

The retry decision depends on the legacy `errors[0].reason` field, so a rate-limit response carrying its reason in the newer `status`/`details` form (e.g. 429 with `"status": "RESOURCE_EXHAUSTED"` and no `errors` list) is re-raised on the first attempt, causing gcalcli to fail imports with "Failed to import event" (gcal.py:1674-1698) and the test at tests/test_gcalcli.py:462-473 to enshrine that behavior. Treat the HTTP status or `status` field as an additional rate-limit signal: OR `(error.get('errors') or [{}])[0].get('reason') in [...]` with `error.get('status') == 'RESOURCE_EXHAUSTED'` (or `e.resp.status == 429`), and scan all `errors` entries instead of only index 0.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

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.

RetryWithBackoff

1 participant