Issue #3616975: Put two bookers in flight at once, and guard the sort that stops them deadlocking

Adds the two-booker test #3616975 said should come first, and the rig it needs. Test-only: no production code changes.

What it asks

A hold opens a transaction, takes the slot row with SELECT FOR UPDATE and writes its lines inside it, and every save deletes that entity's row from the persistent entity cache without leaving the transaction. Nothing in the suite could ask what happens when two of those overlap: a kernel test runs its holds one after the other, and a second connection inside one process blocks on the first rather than racing it. So the bookers here are real HTTP requests, put in flight together.

Four shapes:

  • Different slots, the shape this issue is about.
  • The same slot, the control: the slot lock serialises them, which is the guarantee the engine is built on.
  • One basket asked twice, two tabs or a click that arrived twice. Both requests write the same order row and drop the same cache entry for it.
  • Two group holds naming the same slots in opposite orders, which is the only shape that can form a cycle at all, and the one that carries the weight.

The finding, and it is a positive one

The first three shapes are symmetric: both bookers do the same work in the same order, so they can queue behind one another but never wait on one another. That is why they are quiet, and on its own that quiet would be weak evidence. A cycle needs one side taking A then B while the other takes B then A, and a booker can only do that by holding a group naming several slots at once.

The engine sorts the slot ids before locking them for exactly this reason. That sort was verified by switching it off. With sort($slot_ids) commented out in BookingManager::hold(), the fourth shape deadlocked at round 7 of 12, MySQL killing one side on the second slot's locking read:

SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock
SELECT "t"."id" FROM "…yoyaku_slot" "t" WHERE "t"."id" = 1 FOR UPDATE

With the sort restored, the same pairs pass on the same machine. Same rig, opposite result. So two genuine concurrent bookers can deadlock here, one line of code is what stops them, and that line is now guarded: removing it turns a test red rather than turning into an intermittent lost click that looks like anything but a locking bug.

What it does not show

No collision on the entity cache deletes, which is the mechanism this issue was opened about. Twelve rounds a shape committed, and a one-off run at forty rounds, 240 concurrent pairs, equally clean.

That is a real result but a bounded one, and it should not be read as more than it is. It says the symmetric shapes do not cycle on those deletes; it does not prove no arrangement of bookers ever could. Worth recording alongside it: the collision seen on #3616956 was between a hold and a polling test, never between two bookers, and an attempt to re-stage that exact shape here did not reproduce it on an idle machine.

Untouched either way is the other half of the issue: those deletes remain avoidable work inside the one lock a rush contends on, which #3615593 measured as the queue. That case does not rest on a deadlock and should be argued on its own terms.

Why the quiet shapes are worth reading at all

A test reporting an absence is worth what its calibration is worth, so three things are asserted rather than assumed, every round:

  • The requests really overlapped, by the server's own clock. If they did not, the class skips rather than passes: an environment answering one request at a time cannot be asked this question. On the drupal.org runner they do overlap, and the tests run rather than skip.
  • The entity cache really was warm. A booker reads its order back the way every front does. Deleting a row that was never written takes a different lock from deleting one that was, so a rig that read nothing back would report a quiet it had not earned.
  • The window really was held open, asserted as a floor and never as an exact figure. Each request is kept inside its transaction for 250ms, widening the window by roughly three orders of magnitude over natural timing, which is a stronger instrument than the parallel load that made the #3616956 collision appear by luck.

And a fifth test calibrates the rest: it provokes a deadlock deliberately and insists it comes back named, as SQLSTATE 40001 through DatabaseExceptionWrapper. A class that has never been seen to report a collision cannot be told apart from one that is unable to.

Documented beside the claim it supports in docs/capacity-and-holds.md, under "What gets locked".

Edited by Frank Mably

Merge request reports

Loading
Loading