fix(google-calendar): retry PATCH requests and 403 rate-limit errors#29599
fix(google-calendar): retry PATCH requests and 403 rate-limit errors#29599shafi-VM wants to merge 2 commits into
Conversation
Google Calendar relied on gaxios defaults, which do not retry PATCH requests or HTTP 403 responses. Event creation performs a POST followed by a PATCH; a transient `rateLimitExceeded` (403) on the PATCH was never retried, causing calendar events to become out of sync with bookings. Add an explicit retry config to retry PATCH requests and HTTP 403 responses. POST requests remain non-retriable to avoid duplicate events. Fixes calcom#28834.
|
Welcome to Cal.diy, @shafi-VM! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app-store/googlecalendar/lib/__tests__/CalendarService.auth.test.ts (1)
327-331: ⚡ Quick winAdd an assertion for
408retry coverage in the retry-policy test.This suite should also assert timeout retries (HTTP 408) so policy regressions are caught when
retryConfigis edited.Suggested test addition
expect(retryConfig?.httpMethodsToRetry).toContain("PATCH"); expect(retryConfig?.httpMethodsToRetry).not.toContain("POST"); expect(retryConfig?.statusCodesToRetry).toContainEqual([403, 403]); + expect(retryConfig?.statusCodesToRetry).toContainEqual([408, 408]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app-store/googlecalendar/lib/__tests__/CalendarService.auth.test.ts` around lines 327 - 331, In the retry-policy test section, add an assertion to verify that HTTP status code 408 (timeout) is included in the retryConfig?.statusCodesToRetry array, similar to the existing assertion for 403. This will ensure that timeout retries are covered in the retry policy configuration and help catch regressions if the retryConfig is modified in the future. Add the assertion after the existing statusCodesToRetry test line to maintain consistency with the test structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app-store/googlecalendar/lib/CalendarAuth.ts`:
- Around line 39-44: The statusCodesToRetry configuration in CalendarAuth.ts is
missing HTTP status code 408 (Request Timeout), which is part of gaxios's
default retry behavior. Add a new entry [408, 408] to the statusCodesToRetry
array to include request timeout errors in the retry logic and preserve the
default resilience behavior.
---
Nitpick comments:
In
`@packages/app-store/googlecalendar/lib/__tests__/CalendarService.auth.test.ts`:
- Around line 327-331: In the retry-policy test section, add an assertion to
verify that HTTP status code 408 (timeout) is included in the
retryConfig?.statusCodesToRetry array, similar to the existing assertion for
403. This will ensure that timeout retries are covered in the retry policy
configuration and help catch regressions if the retryConfig is modified in the
future. Add the assertion after the existing statusCodesToRetry test line to
maintain consistency with the test structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 045fdac1-97c5-47d1-904f-f38e8ef6e6bf
📒 Files selected for processing (2)
packages/app-store/googlecalendar/lib/CalendarAuth.tspackages/app-store/googlecalendar/lib/__tests__/CalendarService.auth.test.ts
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What does this PR do?
Google Calendar event creation performs an
insert(POST) followed by aPATCHto add metadata such as description, location, and conference data.The Google Calendar client previously relied on gaxios's default retry configuration, which does not retry
PATCHrequests or HTTP403responses. Since Google'srateLimitExceedederrors can be returned as either403or429, a transient403during the follow-upPATCHwas never retried, causing the calendar event to become out of sync with the booking while the booking itself still appeared successful.This PR adds an explicit
retryConfigto the Google Calendar client that:PATCHtohttpMethodsToRetry403tostatusCodesToRetryPOSTrequests remain intentionally excluded to avoid duplicate event creation.Visual Demo (For contributors especially)
No UI changes. This is a backend integration fix.
Before
After
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Expected result:
PATCHrequests.403responses are included in retryable status codes.POSTrequests are not retried.Checklist
N/A — contributing guide followed, project style adhered to, no new warnings introduced, and PR size is small.