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 through HoldEngine::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

Command icon 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

mably created an issue. See original summary.

mably’s picture

Priority: Major » Critical

mably’s picture

Status: Active » Needs review

  • mably committed 2461d03e on 1.x
    fix: #3619446 A partial area reduction writes the booking's quantity...
mably’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.