From ef60218c4c751f356092c5f440a7ac4336f3af07 Mon Sep 17 00:00:00 2001
From: mohammad <onetoofour3@gmail.com>
Date: Mon, 2 Mar 2026 00:27:04 +0300
Subject: [PATCH] fix: make booking overlap check atomic with select_for_update

Wrap the overlap query and Booking.objects.create() in a single
transaction.atomic() block inside BookingCreateSerializer.create().
Lock the StaffProfile row with select_for_update() so concurrent
requests for the same staff slot are serialized at the DB level;
only one writer can hold the lock at a time, eliminating the race
window between validate() and save().

The early check in validate() is kept for fast user feedback in
the common non-concurrent case. The locked re-check in create()
is the correctness guarantee.

On SQLite (dev/tests) FOR UPDATE is silently ignored but writes
are still serialized. PostgreSQL (production) gets row-level locking.

Update docs/risks.md to mark the race condition as fixed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---
 backend/apps/bookings/serializers.py | 49 +++++++++++++++++++++-------
 docs/risks.md                        |  2 +-
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/backend/apps/bookings/serializers.py b/backend/apps/bookings/serializers.py
index 4ad920f..129e919 100644
--- a/backend/apps/bookings/serializers.py
+++ b/backend/apps/bookings/serializers.py
@@ -1,3 +1,4 @@
+from django.db import transaction
 from django.utils.translation import gettext_lazy as _
 from rest_framework import serializers
 from apps.bookings.models import Booking, BookingStatus
@@ -74,14 +75,40 @@ class BookingCreateSerializer(serializers.ModelSerializer):
     def create(self, validated_data):
         request = self.context["request"]
         service = validated_data["service"]
-        return Booking.objects.create(
-            salon=service.salon,
-            customer=request.user,
-            service=service,
-            staff=validated_data.get("staff"),
-            start_time=validated_data["start_time"],
-            end_time=validated_data["end_time"],
-            notes=validated_data.get("notes", ""),
-            price_amount=service.price_amount,
-            currency=service.currency,
-        )
+        staff = validated_data.get("staff")
+        start_time = validated_data["start_time"]
+        end_time = validated_data["end_time"]
+
+        with transaction.atomic():
+            # Lock the staff row so concurrent booking requests for the same staff
+            # member are serialized. Without this, two requests that both pass the
+            # overlap check in validate() can race and both commit overlapping
+            # bookings. On SQLite (dev/tests) the FOR UPDATE clause is silently
+            # ignored but the transaction still serializes writes; PostgreSQL
+            # (production) gets true row-level locking.
+            StaffProfile.objects.select_for_update().get(pk=staff.pk)
+
+            # Re-run the overlap check inside the lock so the check and the insert
+            # are atomic with respect to other writers.
+            overlap = Booking.objects.filter(
+                staff=staff,
+                status__in=[BookingStatus.PENDING, BookingStatus.CONFIRMED],
+                start_time__lt=end_time,
+                end_time__gt=start_time,
+            ).exists()
+            if overlap:
+                raise serializers.ValidationError(
+                    {"start_time": _("Booking overlaps an existing appointment")}
+                )
+
+            return Booking.objects.create(
+                salon=service.salon,
+                customer=request.user,
+                service=service,
+                staff=staff,
+                start_time=start_time,
+                end_time=end_time,
+                notes=validated_data.get("notes", ""),
+                price_amount=service.price_amount,
+                currency=service.currency,
+            )
diff --git a/docs/risks.md b/docs/risks.md
index c56cd40..3661198 100644
--- a/docs/risks.md
+++ b/docs/risks.md
@@ -11,7 +11,7 @@ This file tracks known gaps and risks to address in future iterations.
 
 ## Booking Integrity
 - Availability checks and overlap prevention are now enforced for staff bookings.
-- **Race condition — booking overlap check is not atomic:** `validate_booking_request` runs the overlap query and returns; the view then calls `serializer.save()` in a separate step with no lock held. Two concurrent POST `/api/bookings/` requests for the same staff slot will both pass validation and both commit. Fix: wrap the overlap check and booking insert in a `select_for_update()` query (or use serializable transaction isolation) so only one request can hold the lock at a time.
+- **Race condition — fixed:** `BookingCreateSerializer.create()` now locks the staff row with `select_for_update()` inside `transaction.atomic()` and re-runs the overlap check before inserting. Concurrent requests for the same staff slot are serialized at the DB level. Requires PostgreSQL in production (SQLite ignores `FOR UPDATE` but still serializes writes).
 - No timezone handling or business hours enforcement.
 - No cancellation rules or refund logic.