Repository navigation
GenericDaoBase.persist() leaves the caller's transaction unbalanced when an insert throws #13905
Description
problem
Split out of #13399 at @DaanHoogland's request.
GenericDaoBase.persist() does not own a transaction, it joins the caller's via TransactionLegacy.currentTxn() and calls txn.start(), which pushes a START_TXN nesting level. On the SQLException path it never reaches txn.commit(), and there is no finally, so that nesting level is leaked:
final TransactionLegacy txn = TransactionLegacy.currentTxn(); // the CALLER's transaction
try {
txn.start(); // pushes a START_TXN nesting level
...
pstmt.executeUpdate(); // throws
...
txn.commit(); // never reached
} catch (final SQLException e) {
logger.error("DB Exception on: " + pstmt, e);
handleEntityExistsException(e); // throws EntityExistsException
throw new CloudRuntimeException("Unable to persist on DB, due to: " + e.getLocalizedMessage());
}
// no finally, the pushed nesting level is never releasedConsequence: the caller's own commit() then finds the transaction unbalanced and silently no-ops, logging only:
WARN [db.Transaction.Transaction] txn: Commit called when it is not a transaction:
(TransactionLegacy.commit() — if (!_txn) { LOGGER.warn(...); return false; })
Everything in that transaction is discarded while the caller believes it committed.
Why it matters beyond one call site: callers that deliberately catch EntityExistsException in order to log-and-continue cannot actually continue, because the enclosing transaction is already unrecoverable. UsageManagerImpl.createHelperRecord() is one such caller, and in #13399 this is what converts a single constraint violation into permanent usage-aggregation failure rather than one skipped record.
versions
Observed on CloudStack 4.22.1.0 (EL9 packages), MySQL 8.x / InnoDB.
This is a code-level defect in framework/db rather than an environment-specific one; the code path is not version-specific and hypervisor/storage/network are not relevant.
The steps to reproduce the bug
- On 4.22.1.0 with the Usage Server enabled, deploy an instance. Its ROOT volume produces a
VOLUME.CREATEusage event carryingvm_id. UsageManagerImpl.createVolumeHelperEvent()performs twopersist()calls sharing(volume_id, created); the second violatesusage_volume's unique key (see Usage Server repeatedly reprocesses historical usage and creates duplicate cloud_usage records for usage_type=1 #13399).createHelperRecord()catches the resultingEntityExistsExceptionand logs a warning, intending to continue.- Observe
txn: Commit called when it is not a transactionshortly afterwards, and that theprocessedflags set for that batch of events incloud_usage.usage_eventwere never committed.
Any caller that hits a constraint violation inside a transaction it owns should show the same behaviour, #13399 is simply a case where it happens on every VM deployment.
What to do about it?
Release the nesting level in a finally, and/or mark the transaction rollback-only so callers receive a real failure instead of a silent no-op.
Either way this needs someone familiar with TransactionLegacy's nesting semantics, since GenericDaoBase backs every DAO in the codebase. I'm raising it rather than proposing a patch.
Activity
boring-cyborg commentedon Aug 18, 2026
Thanks for opening your first issue here! Be sure to follow the issue template!
DaanHoogland commentedon Oct 19, 2026
@Alpha162 I lost track of this, is #13925 a good fix (if so, should we back-port it; the author is irresponsive)
Alpha162 commentedon Oct 26, 2026
@DaanHoogland #13925 fixes the imbalance, but I wouldn't backport it as it stands.
TransactionLegacy.rollback()isn't nesting-aware, as the review thread on #13925 notes: it strips everySTART_TXNoff the stack and does one physical rollback. So whenpersist()fails inside a transaction the caller already started, the newfinallyrolls back the caller's work too. The caller's latercommit()then logs "Commit called when it is not a transaction", and anything it does after catching the exception runs outside a transaction.The callers that hit this are the ones that catch
EntityExistsExceptioninside their own transaction and carry on, which is how this issue started. InUsageManagerImpl.parse(), one duplicate helper insert would roll back the processed flags and helper rows of the failing event and every event before it in the batch, and the rest of the batch would then commit row by row.I'd make the failure path nesting-aware. If
persist()opened the outermost transaction, roll back as #13925 does. If it's nested, unwind only its own level, so the failed insert is a no-op and the caller's transaction carries on.setSavepoint()androllback(Savepoint)already do that, though nothing else calls them today, so it'd need unit tests for both the nested and outermost cases. With that in place, yes, 4.20 makes sense. #13926 uses the same approach in seven sibling methods, and it has conflicts now.DaanHoogland commentedon Oct 27, 2026
@Alpha162 can you scavange the PRs and create a new one, or a set? I fear this is being orphaned otherwise.
Alpha162 commentedon Oct 28, 2026
@DaanHoogland No problem, I'll pick it up. I'll start with a PR against 4.20 for 'persist()', using the nesting-aware rollback with unit tests for the nested and outermost cases, and follow up with the sibling methods from #13926 once the approach is agreed.
I'll credit waterWang's PRs in the description.