Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
While working on #3406985: Convert all transactions in core to use explicit ::commitOrRelease(), I realized the changes in #3384997: Rolling back to a savepoint should leave the savepoint in the transaction stack are incorrect.
Right now, you can't rollback a root transaction if you just rolled back a later savepoint. The later savepoint remains active both on the DB and on the TransactionManager stack, and trying to roll back the outer root transaction end in an out of sequence exception. This is because the savepoint is not released, just rolled back to.
However Drupal expects to start a transaction without caring whether it's a root or a savepoint; and to roll it back without knowing the stack state.
Proposed resolution
Upon rollback of a Transaction object, besides the DB savepoint to rollback, also do a DB savepoint release and void the item on the transaction stack.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3407080
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:
- 3407080-leaving-the-savepoint
changes, plain diff MR !5743
Comments
Comment #3
mondrakeComment #4
mondrakeAdding an additional test case, repeated rollback.
Comment #5
mondrakeComment #6
alexpottI'm a little confused by the idea that the changes in #3384997: Rolling back to a savepoint should leave the savepoint in the transaction stack were "partially incorrect". Doesn't this issue completely revert the nature of the changes made there?
I think this fix is correct but I think that we should have a re-read of the docs that #3384997: Rolling back to a savepoint should leave the savepoint in the transaction stack was based on
We specifically prevent
Savepoints that were set at a later time than the named savepoint are deleted.from happening and we throw out of order errors. Are we sure the behaviour we've implemented here is correct. I'm happy to leave this unresolved here as I think this issue is returning us to the behaviour before all the recent transaction changes.Comment #7
mondrakeRemoved 'partially' from the IS.
What we do here is revert the behaviour that the savepoint rolled back to was left alive, both on the DB and as an item on the TransactionManager stack. After this patch, we RELEASE SAVEPOINT the rolled back savepoint on the DB, and void the item on the TransactionManager so it cannot be committed or rolled back again.
As far as I can remember, I was quite confused by Drupal logic - you could commit any savepoint/root at discretion, but you only could rollback LIFO, i.e. without breaking the order. However, I made efforts to keep this logic in the TransactionManager, too.
On this basis,
I think (hope) it's correct. It's implicit in the LIFO behavior. It's now covered by
DriverSpecificTransactionTestBase::testRollbackSavepointWithLaterSavepoint.Happy to loosen the LIFO behaviour if I was mistaken, and allow rolling back to savepoint/root coming earlier than the last active, if that's the correct expected behavior. It was a grey zone for me, but I remember I found barriers in the previous testing that were indicating to me strict LIFO.
Comment #8
alexpottLet's do this fix as it is really important. I think we can think about loosening the LIFO behaviour in a future issue.
Comment #9
mondrakeThanks. Filed #3407676: Loosen TransactionManager LIFO behaviour on rollback as a follow-up.
Comment #12
catchCommitted/pushed to 11.x and cherry-picked to 10.2.x, thanks!