Alpha162 opened a new pull request, #14511: URL: https://github.com/apache/cloudstack/pull/14511
### Description This PR fixes `GenericDaoBase.persist()` so that a failed insert only undoes its own work. Today, when the insert throws after `txn.start()`, the `START_TXN` that `persist()` pushed stays on the stack. The caller's later `commit()` then finds an open level and skips the commit without any error (#13905). #13925 balances the stack by calling `TransactionLegacy.rollback()` in a `finally`, but `rollback()` strips every `START_TXN` and rolls back the caller's whole transaction. That hurts the callers that catch `EntityExistsException` inside their own transaction and carry on, such as `UsageManagerImpl.createHelperRecord()`. With this change: - When `persist()` runs inside a caller's transaction, it takes a savepoint, releases it on success and rolls back to it on failure. The failed insert is undone and the caller's transaction carries on. - When `persist()` runs on its own, it starts, commits and rolls back its own transaction, as #13925 does. - `TransactionLegacy.setSavepoint()` and `releaseSavepoint()` now update the stack only after the JDBC call succeeds. Before, a failed `SAVEPOINT` left a plain `START_TXN` behind, and a failed `RELEASE SAVEPOINT` removed the entry the rollback needed. Nothing else calls these methods today. This builds on @waterWang's #13925, which had the flag and `finally` structure. #13926 applies the same pattern to the sibling methods (`update()`, `remove()`, `expunge()` and so on), and I'll follow up with those once the approach here is agreed. Fixes: #13905 ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] build/CI - [ ] test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Feature/Enhancement Scale - [ ] Major - [ ] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [x] Major - [ ] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? The new unit test `GenericDaoBasePersistTransactionTest` (6 tests) runs `persist()` against a mocked `Connection` through `TransactionLegacy.transitToUserManagedConnection()`. It covers a failing and a succeeding insert, both inside a caller's transaction and on their own, plus a failing `SAVEPOINT` and `RELEASE SAVEPOINT`. To check the tests catch the problem, I also ran them against: - the current 4.20 code: 5 of 6 fail. The one that passes is the unchanged case of a successful insert outside a transaction. - #13925's change: 4 of 6 fail, including the failed insert inside a caller's transaction, where `conn.rollback()` is called instead of rolling back to the savepoint. I also ran a full build of 4.20 with this change (`mvn -P developer -Dsimulator install`) on Ubuntu 24.04, OpenJDK 11.0.32.1 and Maven 3.8.7. All 9,295 unit tests across 85 modules passed. The only failure was the apidoc packaging step, because the build box had no `bzip2`. The new test sits in `com.cloud.utils` next to `DbUtilTest`, because framework/db's surefire configuration excludes `com/cloud/utils/db/*Test*`. Most of the tests behind that exclusion fail when run by hand (12 failures and 3 errors on 4.20), which is probably worth its own issue. #### How did you try to break this feature and the system with this change? - Made `SAVEPOINT` and `RELEASE SAVEPOINT` fail. The stack stays consistent in both cases, and the caller's `commit()` still commits. - If a call inside `persist()` leaves its own `START_TXN` behind (for example `insertElementCollection()`), `rollback(savepoint)` still unwinds it, because `removeTxn()` drops every `START_TXN` above the savepoint. - Cost: a nested `persist()` now sends `SAVEPOINT` and `RELEASE SAVEPOINT`. If that's a concern, the savepoint could be limited to inserts that span more than one table or write an element collection, since InnoDB already undoes a single failed statement. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
