Issue #3619446: Give a reduction the engine locks

Reducing a pooled area wrote the booking's quantity from the controller, which was the only place in the module a booking was written from outside the engine. The read of the current quantity and the write of the reduced one were serialized against nothing, so two bookers reducing the same area at once both saw five, both wrote four, and the area kept a unit nobody held.

BookingManager::reduce() goes through the engine's mutateUnderLock(), which takes the scopes the booking counts against and then its row, in the order a hold takes them.

The two things the issue left to decide

The argument is the units to give back, never the new total. A caller passing a total would be handing over a number worked out from a read the lock does not cover, which is the defect itself expressed as an API. This is the half that makes the bug unwriteable rather than merely fixed.

Giving back the last unit releases rather than writing a quantity of zero. A consuming row holding nothing is a shape the engine produces nowhere else, so every reader of a held booking would have to learn it. That release announces RELEASED, because to a listener it is one.

The guard, and the proof it is one

A scenario on TwoBookersAtOnceTest drives two reductions of one booking at once, through a new endpoint on the concurrency test module.

It asserts what the slot has free, not either reducer's answer: both requests report success whichever way the race falls, and only the capacity says whether both decrements survived.

Seen to fail against the old behaviour and pass against this one, by putting the read-then-write back into the endpoint and running the same test:

  • with the unsafe write: Failed asserting that 495 is identical to 496 — one of the two decrements lost
  • with the manager doing it: OK (1 test, 555 assertions)

The other four scenarios in that class passed in both runs, so the new one is specific rather than incidentally red.

Also covered

Three kernel cases in BookingManagerTest: a partial reduction frees exactly what it gave back, a reduction to nothing releases, and over-reducing is refused rather than clamped. Clamping would hide the stale read this exists to prevent. testReduceToNothingReleases was checked the same way, by making reduce() write a zero quantity and watching it fail.

phpcs, phpstan and the translation check are clean.

Merge request reports

Loading
Loading