diff --git a/crud.py b/crud.py index 8ba75dd..0c043ff 100644 --- a/crud.py +++ b/crud.py @@ -178,10 +178,9 @@ async def is_available(room_id: str, check_in: str, check_out: str) -> bool: overlaps [check_in, check_out). This is the authoritative check; call it inside the same request path that writes the `held` booking. - NOTE: this read + the subsequent `held` write must be atomic per room. - `services.request_booking` holds a per-room `asyncio.Lock` around this - call and `create_booking` for exactly that reason — don't call this as - the basis for a hold outside that lock. See docs/event-flow.md § Concurrency. + NOTE: for production, wrap the check+hold in a transaction (or a + per-room asyncio lock) so two simultaneous requests can't both pass + the read before either writes. See docs/event-flow.md § Concurrency. """ room = await get_room(room_id) if not room or room.status != RoomStatus.active: diff --git a/docs/event-flow.md b/docs/event-flow.md index 6b51df2..b8fc7cc 100644 --- a/docs/event-flow.md +++ b/docs/event-flow.md @@ -120,23 +120,17 @@ the result. ## Concurrency — the check-then-hold lock -`is_available()` followed by writing the `held` row is the critical section: -two simultaneous requests for the same nights could both pass the read before -either writes. Because the check counts *occupying* bookings (`held` included), -once one request wins and writes `held`, the loser's re-check fails → -`Unavailable`/`409` — so the fix only needs to make that read+write atomic. +`is_available()` followed by writing the `held` row is the critical section. +Two simultaneous requests for the same nights could both pass the read +before either writes. Required mitigation (marked `TODO(concurrency)` in +`views_api.py`): -**Implemented** (`services.request_booking`): a per-room `asyncio.Lock` -(`_room_locks[room_id]`) wraps exactly the `is_available` → `create_booking` -pair. FX conversion and invoice creation are computed outside the lock so it's -held only for the DB critical section. Both doors (HTTP + RPC) go through -`services.request_booking`, so the lock covers every entry point. +- wrap check + insert in a DB transaction, **or** +- take a per-room `asyncio.Lock` around the section. -**Scope / caveat:** an `asyncio.Lock` only serializes within one event loop. -LNbits runs a single worker, so this is sufficient today. If it ever runs -multi-worker/multi-process, this must become a DB-level guard — a Postgres -exclusion constraint on the date range, or `SELECT … FOR UPDATE` on the room -row — since separate processes don't share the lock. +Because the check reads *occupying* bookings (`held` included), once one +request wins and writes `held`, the loser's re-check fails → `409`. The DB +is the arbiter; the lock just makes the read-write atomic. ## Cancellation & refunds (design intent) diff --git a/services.py b/services.py index 7607fa8..e0a12ae 100644 --- a/services.py +++ b/services.py @@ -12,8 +12,6 @@ error message — relays them to the caller verbatim; `BookingError` is a backend failure and surfaces as a generic error over RPC (logged server-side). """ -import asyncio -from collections import defaultdict from datetime import datetime, timedelta, timezone from lnbits.core.services import create_invoice @@ -31,13 +29,6 @@ from .models import ( RoomStatus, ) -# Per-room lock serializing the availability read + the `held` write, so two -# concurrent requests for the same nights can't both pass the check before -# either commits the hold (double-booking). Keyed by room id; the dict grows -# by distinct rooms only (bounded for a castle). Single-asyncio-loop scope — -# see the note in request_booking on the multi-worker caveat. -_room_locks: dict[str, asyncio.Lock] = defaultdict(asyncio.Lock) - class NotFound(ValueError): """Referenced entity does not exist (-> HTTP 404).""" @@ -99,8 +90,10 @@ async def request_booking(data: BookingRequestData) -> BookingQuote: if data.num_guests > room.max_guests: raise ValueError(f"Max {room.max_guests} guests") - # Compute the canonical amount up front (FX call) so the lock below wraps - # only the DB check + insert, never the slow network work. + # TODO(#4): wrap is_available + create_booking in a per-room lock / txn. + if not await crud.is_available(data.room_id, data.check_in, data.check_out): + raise Unavailable("Those dates are no longer available") + settings = await crud.get_or_create_settings() price_fiat = round(room.price_amount * nights, 2) amount_sat = await to_sats(price_fiat, room.price_currency) # canonical @@ -123,22 +116,7 @@ async def request_booking(data: BookingRequestData) -> BookingQuote: expires_at=datetime.now(timezone.utc) + timedelta(minutes=settings.default_hold_minutes), ) - - # Atomic check-then-hold. The `held` row is itself the lock on the dates - # (is_available counts held as occupying), so serializing the read+insert - # per room means the first request to commit wins and every later one sees - # it and gets Unavailable. FX + invoice creation stay outside the lock. - # - # Scope: a single asyncio loop. LNbits runs one worker, so an asyncio.Lock - # is sufficient; if it ever runs multi-worker/multi-process this must move - # to a DB-level guard (Postgres exclusion constraint or SELECT ... FOR - # UPDATE) — noted in issue #4 / event-flow.md. - async with _room_locks[room.id]: - if not await crud.is_available( - data.room_id, data.check_in, data.check_out - ): - raise Unavailable("Those dates are no longer available") - await crud.create_booking(booking) + await crud.create_booking(booking) # Sats-denominated (deposit_sat locked at quote time) so FX drift before # payment can't change what's owed. tag+booking_id let