fix: make check-then-hold atomic against concurrent bookings (#4)

Two simultaneous requests for the same nights could both pass is_available()
before either wrote its held row, double-booking the dates. Wrap the
is_available -> create_booking pair in a per-room asyncio.Lock
(_room_locks[room_id]) in services.request_booking, which both the HTTP and
RPC doors funnel through. FX + invoice creation stay outside the lock, so it
covers only the DB critical section.

Single-loop scope (LNbits runs one worker); documented the multi-worker
caveat (needs a DB-level guard) in event-flow.md and the crud.is_available
note.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019VUQCfdqiLSsFS2jcGnaFD
This commit is contained in:
Padreug 2026-07-19 17:08:43 +02:00
commit 9d87c129ad
3 changed files with 48 additions and 19 deletions

View file

@ -120,17 +120,23 @@ 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. Required mitigation (marked `TODO(concurrency)` in
`views_api.py`):
`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.
- wrap check + insert in a DB transaction, **or**
- take a per-room `asyncio.Lock` around the section.
**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.
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.
**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.
## Cancellation & refunds (design intent)