bugfix:Branch logic is prepare Failed at phase one, need to notify TC…#7997
bugfix:Branch logic is prepare Failed at phase one, need to notify TC…#7997xiaoxiangyeyu0 wants to merge 7 commits into
Conversation
… of global rollback
| * @return the boolean | ||
| */ | ||
| public static boolean isOnePhasePrepareFailed(GlobalStatus status) { | ||
| if (status == GlobalStatus.RollbackRetrying) { |
There was a problem hiding this comment.
Why is a phase‑one failure determined by the global status rather than the branch status, and why is the check limited to whether the status is "RollbackRetrying"?
There was a problem hiding this comment.
This is the global transaction status returned by TC when TM submits a global transaction. The newly added branch status will update the global transaction status to RollbackRetraining
这是TM提交全局事务时,TC返回的全局事务状态,新加的分支状态会把全局事务状态更新为RollbackRetrying
| // Branch Report to TC: Failed | ||
| reportStatusToTC(BranchStatus.PhaseOne_Failed); | ||
| // Branch Report to TC: Exception | ||
| reportStatusToTC(BranchStatus.PhaseOne_PrepareFailed); |
There was a problem hiding this comment.
When the database is MariaDB or MySQL 8.0.29 (or earlier), if an END is executed and then an exception occurs during PREPARE, with the changes introduced by this PR: after the TM makes the two-phase commit decision, the server will directly remove this branch. However, the client may continue to hold onto that connection. In this case, will the resources held by the client connection be properly released?
There was a problem hiding this comment.
After the modification of the pre submission fails, call the rollback method and close the original connection
修改预提交失败后,调用回滚方法并关闭原始连接
|
|
||
| BranchStatus currentStatus = branchSession.getStatus(); | ||
| if (currentStatus == BranchStatus.PhaseOne_Failed) { | ||
| if (currentStatus == BranchStatus.PhaseOne_Failed || currentStatus == BranchStatus.PhaseOne_PrepareFailed) { |
| branchSession -> { | ||
| BranchStatus currentBranchStatus = branchSession.getStatus(); | ||
| if (currentBranchStatus == BranchStatus.PhaseOne_Failed) { | ||
| if (currentBranchStatus == BranchStatus.PhaseOne_Failed || currentBranchStatus == BranchStatus.PhaseOne_PrepareFailed) { |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 2.x #7997 +/- ##
============================================
+ Coverage 72.23% 72.28% +0.04%
Complexity 876 876
============================================
Files 1310 1310
Lines 49975 49999 +24
Branches 5945 5947 +2
============================================
+ Hits 36099 36141 +42
+ Misses 10924 10904 -20
- Partials 2952 2954 +2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a correctness gap in XA mode where exceptions during the XA close/prepare phase can be swallowed by Spring, allowing TM/TC to proceed with a global commit even though a branch effectively failed phase-one. It introduces a dedicated “prepare failed” branch status and propagates that condition back to TC/TM to drive a global rollback instead.
Changes:
- Add
BranchStatus.PhaseOne_PrepareFailedand report it from RM when XA close/prepare fails. - Update TC commit/rollback processing and XA branch reporting to treat prepare-failed branches as terminal and move the global into rollback-retry flow.
- Update TM commit handling to surface a new
TransactionExceptionCode.BranchPrepareFailedwhen the global ends up rollback-retrying after commit.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tm/src/main/java/org/apache/seata/tm/api/TransactionalTemplate.java | Detect rollback-retrying after commit and throw a prepare-failed-related ExecutionException. |
| server/src/main/java/org/apache/seata/server/transaction/xa/XACore.java | On PhaseOne_PrepareFailed branch report, transition the global session toward rollback-retry processing. |
| server/src/main/java/org/apache/seata/server/coordinator/DefaultCore.java | Skip/remove prepare-failed branches during global commit/rollback iteration. |
| rm-datasource/src/main/java/org/apache/seata/rm/datasource/xa/ConnectionProxyXA.java | Report prepare-failed status (and rollback) when XA close/prepare throws; adjust connection holding behavior on exceptions. |
| rm-datasource/src/test/java/org/apache/seata/rm/datasource/xa/ConnectionProxyXATest.java | Update assertion to expect PhaseOne_PrepareFailed reporting. |
| core/src/main/java/org/apache/seata/core/model/BranchStatus.java | Add the new PhaseOne_PrepareFailed enum constant. |
| core/src/main/java/org/apache/seata/core/exception/TransactionExceptionCode.java | Add BranchPrepareFailed exception code for TM-side signaling. |
| common/src/main/java/org/apache/seata/core/model/GlobalStatus.java | Add helper isOnePhasePrepareFailed(...) used by TM commit logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (BranchStatus.PhaseOne_PrepareFailed == status) { | ||
| GlobalSession globalSession = SessionHolder.findGlobalSession(xid); | ||
| // just lock changeStatus | ||
| SessionHolder.lockAndExecute(globalSession, () -> { | ||
| globalSession.close(); // Highlight: Firstly, close the session, then no more branch can be registered. | ||
| if (globalSession.getStatus() == GlobalStatus.Begin) { | ||
| globalSession.changeGlobalStatus(GlobalStatus.RollbackRetrying); | ||
| return true; |
There was a problem hiding this comment.
SessionHolder.findGlobalSession(xid) can return null, but the result is used immediately in lockAndExecute(globalSession, ...) and dereferenced inside the lambda. If the global session is concurrently finished/removed after super.branchReport(...), this can cause an NPE. Consider reusing the already-validated session (e.g., by moving the XA-specific logic into the AbstractCore.branchReport path) or at least null-checking globalSession before calling lockAndExecute and before dereferencing it in the callback.
| public static boolean isOnePhasePrepareFailed(GlobalStatus status) { | ||
| if (status == GlobalStatus.RollbackRetrying) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
GlobalStatus.isOnePhasePrepareFailed() currently returns true for GlobalStatus.RollbackRetrying, which is a generic rollback-retry state used in multiple scenarios (e.g., normal rollback retries). This makes the method name/contract misleading and can cause TM-side logic to report “prepare failed” when the real reason is an unrelated rollback retry. Consider either introducing a dedicated GlobalStatus for prepare-failed scenarios, or renaming the helper to reflect what it actually checks, or tightening the check based on additional state that uniquely identifies prepare failures.
| TransactionExceptionCode.BranchPrepareFailed, | ||
| String.format( | ||
| "Global transaction[%s] is branch prepare failure and will be rollback[TC].", |
There was a problem hiding this comment.
The new BranchPrepareFailed exception is triggered purely by afterCommitStatus matching isOnePhasePrepareFailed(...), but that predicate currently maps to RollbackRetrying (a non-specific status). This risks throwing TransactionExceptionCode.BranchPrepareFailed with a misleading message when the global is rollback-retrying for reasons other than XA prepare failure. If you can’t introduce a dedicated global status, consider using a more generic exception/message for RollbackRetrying, or attaching a distinct marker (e.g., applicationData) so the TM can distinguish prepare-failed rollbacks from other rollback-retry situations.
| TransactionExceptionCode.BranchPrepareFailed, | |
| String.format( | |
| "Global transaction[%s] is branch prepare failure and will be rollback[TC].", | |
| TransactionExceptionCode.Unknown, | |
| String.format( | |
| "Global transaction[%s] is in rollback-retrying status after commit and will be rollback[TC].", |
|
|
||
| /** | ||
| * The Phase one prepare failed. | ||
| * description:Branch logic is prepare Failed at phase one, need to notify TC of global rollback |
There was a problem hiding this comment.
Javadoc has grammatical/capitalization issues (“prepare Failed”). Consider rephrasing to something like “Branch prepare failed in phase one; notify TC to roll back the global transaction” for clarity and consistency with the other enum descriptions.
| * description:Branch logic is prepare Failed at phase one, need to notify TC of global rollback | |
| * description:Branch prepare failed in phase one; notify TC to roll back the global transaction. |
… of global rollback
Ⅰ. Describe what this PR did
When an error occurs during the close phase of XA mode, Spring will swallow the exception and TM can initiate global transaction commit normally. TC can also submit global transactions normally, which is unreasonable. Therefore, a prepare Failed status is added to report to TC and roll back globally
Ⅱ. Does this pull request fix one issue?
fixes #7975
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews