Copilot commented on code in PR #7997:
URL: https://github.com/apache/incubator-seata/pull/7997#discussion_r2969542963
##########
common/src/main/java/org/apache/seata/core/model/GlobalStatus.java:
##########
@@ -235,4 +235,17 @@ public static boolean isTwoPhaseHeuristic(GlobalStatus
status) {
}
return false;
}
+
+ /**
+ * Is one phase prepare failed boolean.
+ *
+ * @param status the status
+ * @return the boolean
+ */
+ public static boolean isOnePhasePrepareFailed(GlobalStatus status) {
+ if (status == GlobalStatus.RollbackRetrying) {
+ return true;
+ }
+ return false;
Review 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.
##########
tm/src/main/java/org/apache/seata/tm/api/TransactionalTemplate.java:
##########
@@ -373,9 +375,15 @@ private void commitTransaction(GlobalTransaction tx,
TransactionInfo txInfo)
statusException = new TmTransactionException(
TransactionExceptionCode.TransactionTimeout,
String.format("Global transaction[%s] is timeout and
will be rollback[TC].", tx.getXid()));
+ } else if
(GlobalStatus.isOnePhasePrepareFailed(afterCommitStatus)) {
+ statusException = new TmTransactionException(
+ TransactionExceptionCode.BranchPrepareFailed,
+ String.format(
+ "Global transaction[%s] is branch prepare
failure and will be rollback[TC].",
Review 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.
```suggestion
TransactionExceptionCode.Unknown,
String.format(
"Global transaction[%s] is in
rollback-retrying status after commit and will be rollback[TC].",
```
##########
core/src/main/java/org/apache/seata/core/model/BranchStatus.java:
##########
@@ -112,7 +112,13 @@ public enum BranchStatus {
* Stop retry
* description:user operate to stop retry
*/
- STOP_RETRY(14);
+ STOP_RETRY(14),
+
+ /**
+ * The Phase one prepare failed.
+ * description:Branch logic is prepare Failed at phase one, need to notify
TC of global rollback
Review 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.
```suggestion
* description:Branch prepare failed in phase one; notify TC to roll
back the global transaction.
```
##########
server/src/main/java/org/apache/seata/server/transaction/xa/XACore.java:
##########
@@ -44,7 +46,18 @@ public void branchReport(
BranchType branchType, String xid, long branchId, BranchStatus
status, String applicationData)
throws TransactionException {
super.branchReport(branchType, xid, branchId, status, applicationData);
- if (BranchStatus.PhaseOne_Failed == status) {}
+ 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;
Review 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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
