Skip to content

fix: retry rate limits and transient errors on Google read requests - #24

Open
boss477 wants to merge 5 commits into
CopilotKit:mainfrom
boss477:fix/google-rate-limit-retry
Open

boss477 wants to merge 5 commits into
CopilotKit:mainfrom
boss477:fix/google-rate-limit-retry

Conversation

@boss477

@boss477 boss477 commented Sep 22, 2026 •

Copy link
Copy Markdown

What changed

  • Added bounded retries with exponential backoff, jitter, and Retry-After parsing for idempotent Google GET reads.
  • Reads retry up to two times on HTTP 429, 500, 502, 503, 504, Google 403 rateLimitExceeded / userRateLimitExceeded, and network failures. Default backoff starts at 1000ms; provider delays are capped at 30 seconds.
  • Malformed or oversized error JSON and successful malformed read responses are not retried.
  • Writes remain single-attempt and preserve OutcomeUnknownError for uncertain outcomes.
  • Preserved upstream charset, attachment filename, calendar-zone, and calendar-range fixes.
  • Added regression coverage for capped numeric/date Retry-After, invalid-header backoff, and network retry exhaustion without a final sleep.
  • Fixed the calendar conversation regression's stale single-attempt assumption: it now checks each failure class's request count and verifies network failure reaches the model without claiming an empty successful result.

Verification

  • Google suite: 49 passed (node --import tsx/esm --test tests/google.test.ts).
  • Calendar conversation suite after fixing its retry-count assertion: 12 passed (node --import tsx/esm --import ./tests/setup.ts --test tests/conversation-calendar.test.ts).
  • pnpm lint, pnpm typecheck, pnpm build:server, web/iOS/Android exports, and worker typecheck passed.
  • git diff --check passed.

Full-suite limitation: the normal pnpm test launcher could not create its Unix IPC socket in this environment. Running the same fixtures directly through Node passed 459 tests but failed five test-file processes. Those five files were rerun sequentially: 78 of 79 tests passed, revealing the stale calendar assertion fixed above; the affected calendar file then passed all 12 tests. A single clean full-suite invocation is still for CI to confirm.

Tests use controlled fixtures; live Google credentials and native device behavior were not exercised.

Add resilient retry logic with exponential backoff and Retry-After header parsing for idempotent GET requests in GoogleClient. Retries apply up to MAX_READ_RETRIES (2) on HTTP 429, HTTP 503, and transient network errors. Write operations (POST, PATCH, DELETE) remain strictly single-attempt without retrying to guarantee mutation safety.

kvnloo commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

The read/write split here looks good. I think the retryable-read predicate may be a little too narrow, though.

Right now idempotent GETs retry only 429 and 503. Google's guidance for transient server failures also covers the usual retryable server-error cases such as 500, 502, and 504, while writes correctly remain non-retryable here because their outcome may be uncertain.

Would it be cleaner to encode that as one explicit read-only retry predicate rather than special-casing 503?

For example, an explicit set such as 429 | 500 | 502 | 503 | 504 keeps the policy auditable without accidentally retrying definite client failures. Then a regression for one additional transient 5xx (say 502) can pin the same backoff and bounded-attempt behavior.

That keeps the important invariant simple: retry eligibility is determined by operation safety first, then transient response classification.

AI-use note: I used an AI assistant to help inspect the retry boundary and draft this review; I verified the cited behavior against the current PR diff and existing Google write-safety tests before posting.

…, 502, 504

Address review feedback by defining an explicit retryable-read status predicate (429, 500, 502, 503, 504). Write operations remain non-retryable and fail fast with OutcomeUnknownError on 5xx.
@boss477

boss477 commented Sep 24, 2026

Copy link
Copy Markdown
Author

Thanks for the thoughtful review and suggestion, @kvnloo!

I've pushed commit \42755f5\ addressing this feedback:

  1. Explicit Retryable-Read Predicate: Extracted and exported \RETRYABLE_READ_STATUS_CODES\ (\Set([429, 500, 502, 503, 504])) and \isRetryableReadStatus(status: number)\ in [\packages/integrations/src/google.ts](https://github.com/CopilotKit/openmuse/blob/fix/google-rate-limit-retry/packages/integrations/src/google.ts) to auditably capture Google's guidance for transient server failures and rate limits.
  2. Operation Safety First: Retries strictly apply only to idempotent GET reads (!write && isRetryableReadStatus(response.status)). All mutations (\POST, \PATCH, \DELETE) continue to fail immediately with \OutcomeUnknownError\ on any 5xx/408/network failure without retrying.
  3. Test Coverage: Added test cases in [\ ests/google.test.ts](https://github.com/CopilotKit/openmuse/blob/fix/google-rate-limit-retry/tests/google.test.ts) covering:
    • \isRetryableReadStatus\ predicate validation across transient codes and client rejection codes.
    • Idempotent GET retry recovery on transient \502 Bad Gateway.
    • Bounded attempts and backoff progression up to \MAX_READ_RETRIES\ on transient \504 Gateway Timeout.
    • Verification that write mutations across all transient 5xx codes (\500, \502, \503, \504) remain strictly single-attempt without retrying.

@davidmckayv

Copy link
Copy Markdown
Contributor

Needs changes before merge:

  • Google's guidance for Gmail and Calendar is to wait at least one second before the first retry, with exponential backoff. Set DEFAULT_RETRY_DELAY_MS to 1000 and add random jitter. listMail fetches 30 messages in parallel, so without jitter a 429 burst retries in lockstep.
  • Also retry 403 when the error reason is rateLimitExceeded or userRateLimitExceeded, which both docs list as retryable.
  • Do not retry when readJson fails. An oversized or malformed body fails the same way again.
  • Revert the charset regex change (["'] to ['"]). It is unrelated.
  • The description says 429 and 503. The code retries 429, 500, 502, 503 and 504. Update it.

Refs: https://developers.google.com/workspace/gmail/api/guides/handle-errors and https://developers.google.com/workspace/calendar/api/guides/errors

@jerelvelarde jerelvelarde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Value: bounded retries improve Google read reliability. Template/security: read retries remain bounded and writes retain single-attempt OutcomeUnknownError semantics; 33 Google tests passed, no actionable security defect found. Please integrate current main before approval: packages/integrations/src/google.ts and tests/google.test.ts conflict with the merged charset, filename, calendar-zone and calendar-range fixes. Preserve those behaviors and rerun combined regressions. Also update the description's retry-status list to include the implemented 500/502/504 statuses.

@boss477

boss477 commented Oct 7, 2026

Copy link
Copy Markdown
Author

Updated in 1f5c3c1:

  • merged current main and resolved the conflicts in packages/integrations/src/google.ts and tests/google.test.ts
  • changed default read backoff to 1000ms plus jitter
  • added retry support for Google 403 rateLimitExceeded / userRateLimitExceeded
  • stopped retrying malformed/oversized error JSON and successful malformed read responses
  • kept upstream charset, attachment filename, calendar-zone, and calendar-range fixes
  • updated the PR description retry-status list

Verification: git diff --check and node --import tsx/esm --test tests/google.test.ts (46 passed).

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.

4 participants