GenericDaoBase.persist() leaves the caller's transaction unbalanced when an insert throws
- Ngôn ngữ chính
- Java
- Star
- 3.1k
- Fork
- 1.4k
- Merge trung bình
- 6 ngày 19 giờ
- Pull request đã merge (30 ngày)
- 32
Mô tả
### 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:
```java
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 released
```
**Consequence:** 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
1. On 4.22.1.0 with the Usage Server enabled, deploy an instance. Its ROOT volume produces a `VOLUME.CREATE` usage event carrying `vm_id`.
2. `UsageManagerImpl.createVolumeHelperEvent()` performs two `persist()` calls sharing `(volume_id, created)`; the second violates `usage_volume`'s unique key (see #13399).
3. `createHelperRecord()` catches the resulting `EntityExistsException` and logs a warning, intending to continue.
4. Observe `txn: Commit called when it is not a transaction` shortly afterwards, and that the `processed` flags set for that batch of events in `cloud_usage.usage_event` were 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.
Hướng dẫn đóng góp
Hướng nghiên cứu
Bắt đầu bằng cách lần theo GenericDaoBase.persist() và hành vi lồng nhau cũng như commit của TransactionLegacy trong framework/db, sau đó kiểm tra UsageManagerImpl.createHelperRecord() và createVolumeHelperEvent() như một đường dẫn tái hiện. Xác nhận rằng lỗi insert không thể khiến transaction của bên gọi bị mất cân bằng hoặc khiến commit sau đó của nó âm thầm trở thành no-op, đồng thời các bên gọi xử lý EntityExistsException nhận được hành vi lỗi dự kiến.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Đánh giá
- Công nghệ
- java
- Lĩnh vực
- backend, database
- Loại issue
- Lỗi
- Độ khó
- 4/5
- Thời gian dự kiến
- 3-5 ngày
- Mức độ hoạt động
- Sôi nổi
- Độ rõ ràng
- Khá rõ ràng
- Mức phù hợp với người mới
- 48/100