AbstractReactiveTransactionManager throws IllegalStateException when rollback fails after commit attempt

[SledgeHammer01]

Hi,

I'm working on a Spring / Hibernate Reactive integration since r2dbc isn't very useful at the moment without ORM support. My integration does use Mutiny / Vert.x under the covers due to that being a Hibernate Reactive thing, but that's irrelevant for this issue.

It's pretty much done and working, but during building out my unit tests, I ran across a bug in AbstractReactiveTransactionManager error handling that also affects R2dbcTransactionManager.

Scenario 1 "happy path" -- works as expected

doBegin() ok
do work ok
doCommit() ok

Scenario 2 "fails in 'work'" -- works as expected, exception bubbles up to controller

doBegin() ok
do work fails
doCommit() is NOT called
doRollback() IS called and is ok

Scenario 3 "fails in doCommit()" -- works as expected, exception bubbled up to controller

covered here: https://github.com/spring-projects/spring-framework/blob/main/spring-r2dbc/src/test/java/org/springframework/r2dbc/connection/R2dbcTransactionManagerTests.java#L223

doBegin() ok
do work ok
doCommit() fails
doRollback() IS called and is ok

Scenario 4 "fails in doCommit() AND doRollback()" -- doesn't work, wrong exception IllegalStateException gets bubbled up

doBegin() ok
do work ok
doCommit() fails
doRollback() fails

This scenario causes the code to end up here:

https://github.com/spring-projects/spring-framework/blob/main/spring-tx/src/main/java/org/springframework/transaction/reactive/AbstractReactiveTransactionManager.java#L619

and eventually we get an exception here when completionStatus == 2. This occurs because the syncs have already been cleared and TransactionSynchronizationManager::getSynchronizations() expects the syncs to still be there or it throws an exception.

https://github.com/spring-projects/spring-framework/blob/main/spring-tx/src/main/java/org/springframework/transaction/reactive/TransactionSynchronizationManager.java#L229

We get the incorrect IllegalStateException bubbling up to the controller.

Seems like maybe the intention was to cover this failure in one of these tests? But neither really cover both the commit and rollback failing:

https://github.com/spring-projects/spring-framework/blob/main/spring-r2dbc/src/test/java/org/springframework/r2dbc/connection/R2dbcTransactionManagerTests.java#L273-L316

EDIT: As a test hack, I used AspectJ to bypass AbstractReactiveTransactionManager::triggerAfterCompletion() and that lets the correct exception bubble up.

I'm thinking, in AbstractReactiveTransactionManager::triggerAfterCompletion() you need to add another check for completionStatus != 2?