[Alpha162]

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 fix: release START_TXN nesting level on persist() SQLException path (#13905) #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)
• 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
• 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.
• fix: release START_TXN nesting level on persist() SQLException path (#13905) #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.

[codecov]

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.47%. Comparing base (e742aad) to head (8991f58).

Files with missing lines	Patch %	Lines
...c/main/java/com/cloud/utils/db/GenericDaoBase.java	93.33%	0 Missing and 1 partial ⚠️
...ain/java/com/cloud/utils/db/TransactionLegacy.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14511      +/-   ##
============================================
+ Coverage     16.43%   16.47%   +0.04%     
- Complexity    13676    13795     +119     
============================================
  Files          5669     5669              
  Lines        501631   501639       +8     
  Branches      60939    60943       +4     
============================================
+ Hits          82420    82669     +249     
+ Misses       409975   409690     -285     
- Partials       9236     9280      +44     

Flag	Coverage Δ
uitests	4.16% <ø> (ø)
unittests	17.34% <88.88%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.