chore: Integrate confirmation booking audit

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#10

[claude]

🔴 TOCTOU race condition in recurring booking rejection: The refactored code splits the original atomic updateMany (which had status: BookingStatus.PENDING in the WHERE clause) into a separate findMany + updateMany by uid only. If a booking transitions from PENDING to ACCEPTED between the two queries (e.g., via a concurrent payment webhook), the updateMany will blindly overwrite it back to REJECTED. Add status: BookingStatus.PENDING to the updateMany WHERE clause at line 420 to restore the original atomic safety.

Extended reasoning...

What the bug is

In confirm.handler.ts lines 409-430, the recurring booking rejection path was refactored from a single atomic updateMany with { recurringEventId, status: BookingStatus.PENDING } in the WHERE clause to a two-step process: first a findMany (with the status check), then an updateMany (by uid only, without the status check). This removes the atomicity guarantee that only PENDING bookings can be rejected.

The specific code path

Line 409: findMany correctly filters for status: BookingStatus.PENDING, returning UIDs of pending bookings.
Line 420: updateMany uses only uid: { in: ... } in its WHERE clause — no status filter.

The original code before this PR had:

await prisma.booking.updateMany({
  where: { recurringEventId, status: BookingStatus.PENDING },
  data: { status: BookingStatus.REJECTED, rejectionReason },
});

This was atomic: the database itself guaranteed that only rows with status = PENDING at the moment of the UPDATE would be affected.

Why existing code does not prevent it

There is no database-level lock or transaction wrapping the findMany and updateMany. Between these two queries, any concurrent process can modify the booking status. Payment webhooks (handlePaymentSuccess) are a realistic concurrent actor — they run asynchronously and can accept bookings via handleConfirmation.

Concrete step-by-step proof

1. User A initiates rejection of a recurring event with recurringEventId = "rec-123".
2. findMany at line 409 returns bookings [B1, B2, B3], all with status: PENDING.
3. Between line 409 and line 420, a Stripe/PayPal payment webhook fires for booking B2. The handlePaymentSuccess function calls handleConfirmation, which updates B2 from PENDING to ACCEPTED and creates calendar events.
4. updateMany at line 420 executes: WHERE uid IN (B1, B2, B3) — no status check.
5. B2 is overwritten from ACCEPTED back to REJECTED, losing the payment confirmation. The attendee has paid but their booking is now rejected.

Impact

This is a data integrity regression. A booking that was legitimately accepted (potentially with a successful payment) can be silently reverted to REJECTED. The user loses their confirmed booking and the payment may not be properly handled.

Additionally, the audit trail will record incorrect oldStatus values. Lines 432-446 read the status after the updateMany, so oldStatus will always be REJECTED (the new value) rather than the actual previous status. However, this is a secondary concern compared to the TOCTOU race.

How to fix

Add status: BookingStatus.PENDING back to the updateMany WHERE clause:

await prisma.booking.updateMany({
  where: {
    uid: {
      in: unconfirmedRecurringBookings.map((booking) => booking.uid),
    },
    status: BookingStatus.PENDING,  // Restore atomic safety
  },
  data: {
    status: BookingStatus.REJECTED,
    rejectionReason,
  },
});

This restores the original atomic behavior: the database will only update rows that are still PENDING at the time the UPDATE executes, preventing any concurrently-accepted bookings from being overwritten.

[claude]

🔴 Bug: Line 413 hardcodes oldStatus: BookingStatus.ACCEPTED for non-recurring booking acceptance, but the booking is transitioning from PENDING to ACCEPTED. This causes the audit log to record {old: ACCEPTED, new: ACCEPTED} instead of {old: PENDING, new: ACCEPTED}, making the status change invisible in the audit trail. Should use booking.status instead, consistent with the recurring path at line 263.

Extended reasoning...

What the bug is

In handleConfirmation.ts, the non-recurring booking acceptance path constructs an acceptedBookings array at line 411-416 that is later passed to fireBookingAcceptedEvent. This array includes an oldStatus field meant to capture the booking status before the acceptance update. However, line 413 hardcodes oldStatus: BookingStatus.ACCEPTED (the new status) instead of using booking.status which holds the actual pre-update status (PENDING).

The specific code path

When a non-recurring pending booking is confirmed:

1. The confirmHandler in confirm.handler.ts verifies the booking is not already ACCEPTED (throwing if it is), then calls handleConfirmation.
2. Inside handleConfirmation, at line 383-409, the booking is updated via prisma.booking.update to set status: BookingStatus.ACCEPTED.
3. Immediately after, at lines 411-416, the code builds the acceptedBookings array with oldStatus: BookingStatus.ACCEPTED.
4. This array is passed to fireBookingAcceptedEvent (line 435-440), which feeds oldStatus into the audit data as status: { old: acceptedBooking.oldStatus, new: BookingStatus.ACCEPTED }.

The booking parameter still holds the original pre-update status (PENDING) because the Prisma update returns a new object and does not mutate the booking input parameter.

Why existing code does not prevent it

The confirmHandler already validates that booking.status !== BookingStatus.ACCEPTED before reaching handleConfirmation, guaranteeing the booking is PENDING at this point. However, nothing in handleConfirmation itself checks or corrects the hardcoded value. The recurring path at line 263-265 correctly uses booking.status from the fetched records, but the non-recurring path was written with the hardcoded enum value, likely a copy-paste error.

Step-by-step proof

1. User creates a booking that requires confirmation. Booking status is PENDING.
2. Organizer clicks Confirm in the webapp. confirmHandler runs.
3. confirmHandler checks booking.status === BookingStatus.ACCEPTED. It is PENDING, so no error is thrown.
4. handleConfirmation is called with booking.status = PENDING.
5. The Prisma update changes the DB status to ACCEPTED, but the booking parameter still has status: PENDING.
6. Line 413 sets oldStatus: BookingStatus.ACCEPTED (hardcoded).
7. fireBookingAcceptedEvent receives { old: ACCEPTED, new: ACCEPTED } and records this in the audit log.
8. The audit log shows no status change occurred, which is incorrect.

Impact

The audit trail for every non-recurring booking acceptance will show {old: ACCEPTED, new: ACCEPTED}, making it appear that no status change happened. This defeats the purpose of the audit log for this action. For organizations relying on audit logs for compliance or debugging, this renders the status change data for non-recurring confirmations meaningless.

Fix

Change line 413 from oldStatus: BookingStatus.ACCEPTED to oldStatus: booking.status. This matches the pattern used in the recurring path at line 263-265 and correctly captures the pre-update PENDING status.

[claude]

🔴 Bug: In the recurring booking rejection path (lines 432-447), oldStatus is read from updatedRecurringBookings which is fetched AFTER the updateMany sets status to REJECTED. The audit log will record {old: REJECTED, new: REJECTED} instead of {old: PENDING, new: REJECTED}. Fix: use unconfirmedRecurringBookings (line 409) instead of updatedRecurringBookings (line 432) and remove the unnecessary second findMany.

Extended reasoning...

What the bug is

In confirm.handler.ts, the recurring booking rejection path has an incorrect read-after-write that causes the audit log to record the wrong oldStatus. The code performs three steps:

1. Step 1 (lines 409-418): Finds all PENDING bookings for the recurring event into unconfirmedRecurringBookings. At this point, status = PENDING.
2. Step 2 (lines 420-430): Updates all those bookings to REJECTED via prisma.booking.updateMany.
3. Step 3 (lines 432-442): Re-reads the same bookings into updatedRecurringBookings. Since step 2 already changed the status, these bookings now have status = REJECTED.
4. Step 4 (lines 444-447): Maps updatedRecurringBookings to build the rejectedBookings array, using recurringBooking.status as oldStatus.

Why this is wrong

Because step 3 reads the bookings after step 2 has already updated them to REJECTED, recurringBooking.status at line 446 will be BookingStatus.REJECTED, not BookingStatus.PENDING. The rejectedBookings array will contain { oldStatus: REJECTED } for every booking.

Concrete proof by example

Consider an organizer rejecting a recurring event with 3 PENDING bookings:

1. unconfirmedRecurringBookings finds 3 bookings, each with status: PENDING.
2. updateMany sets all 3 to status: REJECTED.
3. updatedRecurringBookings re-reads all 3 — each now has status: REJECTED.
4. rejectedBookings is mapped as [{ oldStatus: REJECTED, uid: ... }, ...].
5. fireRejectionEvent receives rejectedBookings and constructs audit data: { status: { old: REJECTED, new: REJECTED } }.

The audit log records a no-op transition REJECTED → REJECTED instead of the correct PENDING → REJECTED.

Contrast with the correct paths

The non-recurring rejection path (lines 458-473) correctly uses booking.status from the original booking query (fetched before any update), so oldStatus is correctly PENDING. The acceptance path in handleConfirmation.ts (line 263) also correctly uses unconfirmedRecurringBookings (pre-update data) to build acceptedBookings with the original status.

Impact

The booking audit trail for recurring booking rejections will be inaccurate. Every rejected recurring booking will show a status transition of REJECTED → REJECTED instead of PENDING → REJECTED, making the audit log misleading and potentially unusable for compliance or debugging purposes.

Fix

Replace updatedRecurringBookings with unconfirmedRecurringBookings at line 444, and remove the unnecessary second findMany at lines 432-442:

rejectedBookings = unconfirmedRecurringBookings.map((recurringBooking) => ({
  uid: recurringBooking.uid,
  oldStatus: recurringBooking.status,
}));

This mirrors the pattern already used in the acceptance path.

[claude]

🟡 In handleConfirmation.ts, the recurring booking acceptance path calls fireBookingAcceptedEvent (line ~279) BEFORE the database updates at Promise.all(updateBookingsPromise) (line ~287+). If the DB update fails, a phantom audit event will have already been dispatched claiming bookings were accepted while they remain PENDING. The non-recurring path (line ~434) correctly fires the event AFTER the DB update. Move the fireBookingAcceptedEvent call to after await Promise.all(updateBookingsPromise) completes.

Extended reasoning...

What the bug is

In handleConfirmation.ts, the fireBookingAcceptedEvent function is called at different points depending on whether the booking is recurring or not. For the recurring path, it fires BEFORE the database is updated. For the non-recurring path, it fires AFTER. This ordering inconsistency means the recurring path can emit an audit event that claims bookings were accepted even if the subsequent DB update fails.

Code path walkthrough

In the recurring acceptance branch (if (recurringEventId)):

1. unconfirmedRecurringBookings are fetched from the DB (line ~258)
2. acceptedBookings is built from these fetched bookings
3. fireBookingAcceptedEvent is called with these bookings (line ~279) — audit event dispatched
4. updateBookingsPromise is constructed and awaited via Promise.all (line ~287+) — DB update happens here

Compare with the non-recurring branch:

1. prisma.booking.update() is called and awaited (line ~350-409) — DB update first
2. acceptedBookings is built from the result
3. After the if/else block, fireBookingAcceptedEvent is called (line ~434) — audit event dispatched after DB success

Concrete failure scenario

Consider a recurring booking with 5 instances, all PENDING. The organizer clicks Accept All:

1. The code fetches all 5 unconfirmed bookings
2. fireBookingAcceptedEvent dispatches an audit task via bookingEventHandlerService.onBulkBookingsAccepted, recording all 5 bookings as transitioning from PENDING to ACCEPTED
3. One of the 5 prisma.booking.update() calls fails (e.g., due to a constraint violation or DB connectivity issue)
4. The audit system now has a record saying all 5 bookings were accepted, but some (or all) are still PENDING in the database

Impact

The practical impact is low because: (1) the audit goes through an async Tasker queue, so there is inherent eventual consistency; (2) DB failures at this point in the flow are rare for simple status updates; (3) fireBookingAcceptedEvent wraps everything in try-catch so it won't block the main flow. However, the ordering is incorrect compared to both the non-recurring acceptance path and the rejection path in confirm.handler.ts, which both correctly fire their audit events AFTER the DB mutations.

Fix

Move the fireBookingAcceptedEvent call in the recurring path to after const updatedBookingsResult = await Promise.all(updateBookingsPromise) completes successfully. This aligns it with the non-recurring path pattern and ensures audit events only fire for bookings that were actually updated in the database.