Issue #3618543: Report a rollback that cannot be performed instead of raising it over the reason for it

A write path that abandons its work rolls its transaction back and raises what stopped it. If that rollback fails, the failure comes from inside the catch block and replaces the exception being handled, so the booker sees a failure of the site and the log holds a line about transaction plumbing with nothing about the cause.

A rollback fails in two ways here. Out of order: core allows one only on the innermost item of the transaction stack, so a nested transaction still above this one is enough. Already ended: the server rolls a deadlock victim back on its own and deletes its savepoints, after which core is releasing savepoints that are gone.

The reporting

Rollbacks::discard() asks for the rollback, and when the rollback itself throws it records one line naming both what the work was abandoned for and what the rollback said. The caller then raises what it caught. The three catch-and-rollback sites go through it: the hold transaction and lockedMutate() in BookingManager, and TransactionManager::withRowLock().

SlotSeats::repairStep() had no failure path at all. Since a Transaction destroyed without an explicit commit is committed rather than abandoned, an exception escaping a repair step kept the seats it had written and the settlement it had made, and reported neither. It now rolls back through the same service.

Skipping the rollback is not the fix, for the same reason: a catch block that merely raises keeps every write the work had made.

The answer a booker gets

With the cause visible it can be recognized, which is what the rest of this does. Contention::endedTheTransaction() tells the database settling a conflict from something being wrong, off the driver's codes rather than its sentences: SQLSTATE 40001, PostgreSQL's 40P01 and 3B001, and MySQL's own error number 1305 for a savepoint that is gone. That last one is read as a number because MySQL files it under 42000, which is also a syntax error and a denied permission, and refusing every 42000 as contention would answer a genuine bug with an invitation to keep trying.

Recognized, it becomes a refusal carrying a sentence for the booker rather than a fault: nothing was written, they lost nothing, and asking again ordinarily works. Both paths a claim can reach it by are covered, which took measuring to find: the hold path through refusalFor(), the give-back path through lockedMutate(), and then OfferClaim::reduceTarget(), which called release() with no handling at all, so even a properly worded refusal escaped to the top as a 500. A refused give-back now stops the reduction and tells the booker, instead of reporting units as given back while the line is still held.

Measured

Load harness, forty concurrent bookers, ninety seconds, one slot of eight thousand places in sixteen parts, on this branch alone, so every deadlock the engine can have is present.

holds refused failed what the log held
1.x 4209 0 1360 1324 deadlocks, and 53 refused rollbacks with no cause named
this branch 4614 807 0 54 reports of a rollback that could not be performed

Before, the 53 masked cases logged only TransactionOutOfOrderException: Error attempting rollback of ...\drupal_transaction. Active stack: ...\drupal_transaction > ...\savepoint_1. That exception no longer appears anywhere, no request fails, and every contention failure reaches its booker as a sentence saying to try again.

Tests

RollbacksTest builds the out-of-order case for real: a nested transaction kept alive, then the outer one discarded. It asserts that nothing is raised and that the record names both halves, and it was confirmed to fail against a naive rollback with exactly the exception seen in production. A second test asserts that an ordinary rollback still undoes its writes, which is what stops the report being fixed by swallowing the rollback instead.

ContentionTest covers both directions, including the trap: 42000 with error 1305 is contention, 42000 with error 1064, a syntax error, is not.

RollbacksTest, ContentionTest, ModuleBoundariesTest, BookingManagerTest, BookerRefusalTest, SlotClaimCostTest and SlotSeatsRepairedTest pass against MySQL. phpcs with the CI standard, phpstan and cspell are clean, and the translation check reports every shipped string translated.

Audited before merge, and it found two things

A refusal read off a failure was dropping that failure. Turning a deadlock into a booker refusal kept only the message text, so the log lost the SQLSTATE and the trace, which is the same fault as the one this issue is about one layer further in. BookingException::refusal() now takes the cause, and testRefusalKeepsWhatItWasRaisedFor asserts it survives and is still recognizable as contention.

Because a refusal now carries its cause, both passes of a hold would match that cause and wrap a refusal in another refusal, saying the same sentence twice. refusalFor() returns anything already worded for a booker untouched.

No performance regression

There is no mechanism for one: every call site of the new code sits inside a catch block, so nothing added here runs when a hold succeeds. Measured anyway, arms alternated, with cache tag checksums on memcache so the cachetags convoy of [#3618572] could not mask the difference.

1.x this branch
quota'd house, 40 bookers 43.6 and 45.9 holds/s 43.8 and 49.0
stadium of 80,000 seats in 80 blocks, 80 bookers 220.4 holds/s 240.1

No refusals and no failures on either arm at any point, and the ninety-fifth percentile is lower on this branch in every run.

Edited by Frank Mably

Merge request reports

Loading
Loading