fix(bookings): set booking to PENDING when calendar sync fails#28916
fix(bookings): set booking to PENDING when calendar sync fails#28916pmartin-dev wants to merge 2 commits intocalcom:mainfrom
Conversation
|
Welcome to Cal.diy, @pmartin-dev! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughAdds a Vitest test suite for 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/bookings/lib/service/RegularBookingService.ts`:
- Around line 2082-2093: The code sets booking.status = BookingStatus.PENDING
when allCalendarEventsFailed but execution continues down the
isConfirmedByDefault branch, causing ACCEPTED-only side effects to still run;
update the flow in RegularBookingService so that after detecting
allCalendarEventsFailed you either (a) switch to the pending/requested path
(e.g., return or jump to the pending handler) or (b) add guards on downstream
side effects (the BOOKING_CREATED emit, BOOKING_REQUESTED queueing, and no-show
scheduling) to check booking.status === BookingStatus.ACCEPTED before executing;
specifically adjust the block that handles allCalendarEventsFailed (uses
booking, allCalendarEventsFailed, BookingStatus.PENDING) and protect the later
side-effect code paths that reference BOOKING_CREATED, BOOKING_REQUESTED, and
no-show scheduling to only run for BookingStatus.ACCEPTED.
🪄 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: 240ba73d-edbd-4309-9713-55de30d2cf6e
📒 Files selected for processing (2)
packages/features/bookings/lib/service/RegularBookingService.test.tspackages/features/bookings/lib/service/RegularBookingService.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/bookings/lib/service/RegularBookingService.ts`:
- Around line 2082-2086: The current flow persists bookings as ACCEPTED before
EventManager.create() runs, causing a transient ACCEPTED state; change the
createBooking flow in RegularBookingService (the code that calls
EventManager.create() and uses deps.prismaClient.booking.update on booking) to
initially persist the booking record with status BookingStatus.PENDING, then
call EventManager.create(); after at least one calendar write succeeds,
explicitly update that booking row to BookingStatus.ACCEPTED (via
deps.prismaClient.booking.update), and only use the compensating rollback to
PENDING if all calendar writes fail. Locate the logic around createBooking(),
the booking variable, EventManager.create(), and the update call to
BookingStatus.PENDING/ACCEPTED and switch the order of operations so promotion
to ACCEPTED happens after successful calendar writes.
🪄 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: c533875b-a0f4-4d88-87cc-1b690927c31d
📒 Files selected for processing (1)
packages/features/bookings/lib/service/RegularBookingService.ts
| if (allCalendarEventsFailed) { | ||
| await deps.prismaClient.booking.update({ | ||
| where: { id: booking.id }, | ||
| data: { status: BookingStatus.PENDING }, | ||
| }); |
There was a problem hiding this comment.
This still leaves a transient ACCEPTED booking in the DB.
By the time this runs, createBooking() has already persisted the confirmed-by-default booking earlier in the flow, so this is only a compensating update after EventManager.create() returns. Anything observing the row in that gap can still see the phantom ACCEPTED state the issue is trying to remove. Consider persisting these bookings as PENDING first and only promoting them to ACCEPTED after at least one calendar write succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/bookings/lib/service/RegularBookingService.ts` around lines
2082 - 2086, The current flow persists bookings as ACCEPTED before
EventManager.create() runs, causing a transient ACCEPTED state; change the
createBooking flow in RegularBookingService (the code that calls
EventManager.create() and uses deps.prismaClient.booking.update on booking) to
initially persist the booking record with status BookingStatus.PENDING, then
call EventManager.create(); after at least one calendar write succeeds,
explicitly update that booking row to BookingStatus.ACCEPTED (via
deps.prismaClient.booking.update), and only use the compensating rollback to
PENDING if all calendar writes fail. Locate the logic around createBooking(),
the booking variable, EventManager.create(), and the update call to
BookingStatus.PENDING/ACCEPTED and switch the order of operations so promotion
to ACCEPTED happens after successful calendar writes.
There was a problem hiding this comment.
Acknowledged => the transient ACCEPTED window is a known trade-off. Persisting as PENDING first would require changing createBooking() which conditions all downstream side effects (notifications, payments, workflows). That's a much larger refactor with higher regression risk. The compensating update runs before any side effect fires, so no consumer observes the phantom state in practice.
There was a problem hiding this comment.
@pmartin-dev, that's a fair and well-reasoned trade-off. Looking at the code flow confirms your claim: fireBookingEvents, the webhook trigger, scheduleNoShowTriggers, and the email dispatch are all gated on booking.status === BookingStatus.ACCEPTED or !allCalendarEventsFailed, and they all execute after the compensating booking.status = BookingStatus.PENDING mutation. So within this handler, no side effects can observe the phantom ACCEPTED state.
The only residual exposure is external DB readers (e.g., a separate process or replica polling the booking table) that happen to query between createBooking() completing and eventManager.create() + the compensating update finishing. That window is genuinely narrow and, for the scope of this fix vs. the regression risk of refactoring createBooking(), the trade-off is defensible.
Happy to close this out.
✏️ Learnings added
Learnt from: pmartin-dev
Repo: calcom/cal.diy PR: 28916
File: packages/features/bookings/lib/service/RegularBookingService.ts:2082-2086
Timestamp: 2026-04-16T13:00:11.708Z
Learning: In `packages/features/bookings/lib/service/RegularBookingService.ts`, when all calendar integrations fail (`allCalendarEventsFailed`), the booking is compensated to `BookingStatus.PENDING` via a `prismaClient.booking.update` call immediately after `eventManager.create()` returns. This is intentionally a compensating-update pattern (not an optimistic-PENDING-then-promote pattern), because `createBooking()` conditions downstream side effects on the initial status; refactoring that is considered too high-regression-risk. All side effects (emails, webhooks, no-show triggers) are gated on `booking.status === ACCEPTED` and fire after the in-memory mutation, so no consumer within the handler observes the transient `ACCEPTED` state. The only residual risk is external DB readers during the narrow window between `createBooking()` and the compensating update — accepted as a known trade-off.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
What does this PR do?
When external calendar event creation fails (Google Calendar, Outlook, etc.), bookings were silently saved as
ACCEPTEDwith no corresponding calendar event — creating "phantom bookings". This fix downgrades the booking toPENDINGwhen all calendar integrations fail, and prevents misleading confirmation emails and webhook payloads.Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
No special environment variables needed. Run the unit tests:
3 test cases:
To reproduce the original bug manually:
Checklist