A partial reduction of a pooled area writes the booking's quantity directly, from a controller, outside the locks the engine takes for every other change to a booking. Quantity is capacity, so two bookers reducing the same area at once can lose a decrement.
Problem/Motivation
PlaceSelectionController::releaseArea() gives units back to a pooled area. It has two paths, and they do not agree with each other:
- Giving up a whole booking calls
BookingManager::release(), which goes throughHoldEngine::mutateUnderLock(): it takes the lock anchors bounding the slot and then the booking's own row. - Giving back part of a booking writes the new quantity itself:
$booking->set('quantity', $quantity - $left)followed by$booking->save(). It takes neither lock.
So the read of the current quantity and the write of the reduced one are not serialized against anything. Two requests reducing the same pooled area by one both read a quantity of five, both write four, and the area keeps a unit that nobody holds. The lock-anchor counters are still maintained, because the presave hooi fires on any save, but they are updated outside the lock that makes a counter safe to read, which is the arrangement #3616896 put in place.
It is also the only place in the module where a booking is written from outside BookingManager or HoldEngine. Everything else that changes a booking's state or quantity goes through the manager, and the reason it does is this one.
Steps to reproduce
Two concurrent requests, each reducing the same pooled area of one slot by one unit, against a booking holding several. Both observe the same starting quantity and the second write lands on the first, so the order gives back one unit instead of two while the anchor counters record two.
Proposed resolution
Give the manager the operation, so a reduction is serialized exactly like a release. A reduce() on BookingManagerInterface implemented through mutateUnderLock(), reading the quantity inside the lock rather than passing in one read outside it, and the controller asks for it. It hands back BookingInterface, like the other manager methods, so a caller cannot carry on writing the row.
Two things need deciding rather than assuming. Reducing to zero should almost certainly release the booking instead of writing a quantity of zero, since a zero-quantity consuming row is a shape nothing else in the engine produces. And the units to give back should be the argument, not the new total, so the caller cannot hand over a total it computed from a stale read.
Remaining tasks
- Add the manager method and route the controller through it.
- Decide what a reduction to zero means, and make the engine say so in one place.
- A concurrency case on the load rig: two reducers on one pooled area, asserting the area's free count afterwards rather than either booking's own quantity.
Noted while reviewing, not part of this: FeeRecordSubscriber::recordBooking() also saves a booking from outside the engine, but it writes only the unit_fee and unit_fee_absorbed columns while an order changes state, so it cannot move capacity or the anchor counters.
AI-Generated: Yes (Claude Code found this while auditing which booking writes happen outside the engine, and drafted this summary. I reviewed it. No fix or test is written yet.)
Issue fork yoyaku-3619446
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
mably commentedComment #4
mably commentedComment #6
mably commented