[CAL-3023] Race condition in /api/book/event endpoint

[odicho]

Issue Summary

At the moment, there is a classic race condition in the handler of handleNewBooking.ts.

There is no db/application level locking or db transactions in place, so the following scenario can happen:

1. User 1 and 2 both load the same event type and select the same availability
2. When the request to /api/book/event is send, it will get all the availabilities and bookings from the DB and then calculate if the slot is within availability.
3. Both users will see that there is availability and naively insert a booking, causing duplicate bookings.

Steps to Reproduce

1. Copy the api/book/event request from network tab when clicking on "Confirm"
2. Insert into any tool that can make concurrent requests. Simple example:

for i in {1..10}
do
   <curl request here> & #make sure to add an ampersand at the end
done
wait

Actual Results

I'm able to create several bookings at the same exact time.

Expected Results

After the first booking, all subsequent requests to book should fail.

_CAL-3023

[htuerker]

@PeerRich, I'm looking into contributing to this one. A trivial application-level solution is; to run a two steps transactional operation instead of solely creating a db record:

• first, query availability
• then, insert the new booking

transaction([checkAvailability, createBooking]);

https://github.com/calcom/cal.com/blob/ebe6a60366e5cd7324bc93b1e3c921b91a7e047d/packages/features/bookings/lib/handleNewBooking.ts#L820

[htuerker]

Seems like there's no "easy" application-level solution. It also may not be possible to a full coverage at all.

With a quick research, I've figured out a solution using a simple db constraint. It's advised by experts to prevent row-level overbooking and overlapping records.

Here's a simple migration prototype:

ALTER TABLE "Booking" 
    ADD CONSTRAINT no_overlapping_accepted_bookings_for_user
        EXCLUDE USING GIST(
            "userId" WITH =, 
            "status" WITH =, 
            tsrange("startTime", "endTime") WITH &&
    ) WHERE("status" = 'accepted');

I don't like the idea of restricting overlapping events completely. Instead, I'd like to extend the booking table with "allowOverlap" and provide an option to overwrite this constraint. Users might want to have overlapping bookings by choice but in my opinion that must not be default.

I have been interacting with this project repository only for a day, there must be some edge cases that need to be checked properly in situations like recurring events, multiple-user events, team events, etc.

Lastly, I'm not a db-expert. I have no such knowledge of how this constraint would affect query performance and is it tolerable. I would love to hear some details.

Some references: https://www.pgcon.org/2010/schedule/attachments/136_exclusion_constraints2.pdf https://www.postgresql.org/docs/current/btree-gist.html https://www.postgresql.org/docs/current/gist-builtin-opclasses.html https://www.cybertec-postgresql.com/en/exclusion-constraints-in-postgresql-and-a-tricky-problem/ https://www.prisma.io/dataguide/datamodeling/correctness-constraints

[shirazdole]

@keithwillcode can we get this bumped back to medium priority? It's important for Montu

[keithwillcode]

We have already implemented an idempotency key on bookings. @emrysal this takes care of this, yeah?

[odicho]

Hi @shirazdole and @keithwillcode, the current idempotency key solution doesn't fix the issue above.

I am able to reproduce this issue with the following script:

 for i in {1..10}; do
  EMAIL="[email protected]"
  <POST /booking/event cURL> &
done

You current implementation looks like this:

const idempotencyKey = uuidv5(
            `${
              args.data.eventType?.connect?.id
            }.${args.data.startTime.valueOf()}.${args.data.endTime.valueOf()}.${uniqueEmailJoinInput.join(
              ","
            )}`,
            uuidv5.URL
          );

This value is different when the email differs, which is the realistic scenario here (two or more unique users trying to book at the same time). The solution only works if uniqueUser1 tries to book concurrently (using a script like above), but the realistic scenario is that uniqueUser1 and uniqueUser2 try to book concurrently.

Reference: