Copilot commented on code in PR #8027:
URL: https://github.com/apache/incubator-seata/pull/8027#discussion_r2992107959
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/ActionInterceptorHandler.java:
##########
@@ -53,6 +55,9 @@ public class ActionInterceptorHandler {
private static final Logger LOGGER =
LoggerFactory.getLogger(ActionInterceptorHandler.class);
+ private static final boolean ENABLE_ACTION_STATUS_REPORT =
ConfigurationFactory.getInstance()
+
.getBoolean(ConfigurationKeys.CLIENT_SAGA_ACTION_STATUS_REPORT_ENABLE, false);
Review Comment:
`ENABLE_ACTION_STATUS_REPORT` is read once at class-load time. This prevents
config hot-reload from taking effect and can also capture the wrong value if
`ConfigurationFactory` isn’t initialized yet when this class is first loaded.
Prefer reading the flag from `ConfigurationFactory.getInstance()` at execution
time (or caching via a config listener) instead of a `static final` snapshot.
```suggestion
private static boolean isActionStatusReportEnabled() {
return ConfigurationFactory.getInstance()
.getBoolean(ConfigurationKeys.CLIENT_SAGA_ACTION_STATUS_REPORT_ENABLE, false);
}
```
##########
saga/seata-saga-annotation/src/main/java/org/apache/seata/saga/rm/SagaAnnotationResourceManager.java:
##########
@@ -131,6 +137,7 @@ public BranchStatus branchRollback(
LOGGER.error(msg, ExceptionUtil.unwrap(t));
return BranchStatus.PhaseTwo_RollbackFailed_Retryable;
} finally {
+ doAfterSagaAnnotationRollback(xid, branchId,
resource.getActionName(), businessActionContext);
Review Comment:
`doAfterSagaAnnotationRollback(...)` is invoked in the `finally` block even
when `businessActionContext` was never created (e.g., if
`getBusinessActionContext(...)` throws). In that case hooks will receive a null
`context`, which can cause NPEs inside hook implementations. Guard the hook
invocation (or build a minimal context) when `businessActionContext == null`.
```suggestion
if (businessActionContext != null) {
doAfterSagaAnnotationRollback(xid, branchId,
resource.getActionName(), businessActionContext);
}
```
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/ActionInterceptorHandler.java:
##########
@@ -325,4 +343,32 @@ protected Map<String, Object>
fetchActionRequestContext(Method method, Object[]
}
return context;
}
+
+ /**
+ * Report action status to TC
+ *
+ * @param actionContext the action context
+ * @param status the action status (success/failed)
+ */
+ protected void reportActionStatus(BusinessActionContext actionContext,
String status) {
+ try {
+ actionContext.setActionStatus(status);
+ actionContext.setUpdated(true);
+ BusinessActionContextUtil.reportContext(actionContext);
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug(
+ "Report action status: xid={}, branchId={}, status={}",
+ actionContext.getXid(),
+ actionContext.getBranchId(),
+ status);
+ }
+ } catch (Exception e) {
+ LOGGER.warn(
+ "Report action status failed: xid={}, branchId={},
status={}, error={}",
+ actionContext.getXid(),
+ actionContext.getBranchId(),
+ status,
+ e.getMessage());
Review Comment:
`reportActionStatus(...)` swallows exceptions from
`BusinessActionContextUtil.reportContext(...)`, but it leaves
`actionContext.updated=true`. The `finally` block later calls
`BusinessActionContextUtil.reportContext(actionContext)` again and may throw,
so the “swallow” here won’t actually prevent the prepare from failing. Either
make status reporting best-effort by clearing/resetting `updated` when
catching, or don’t catch and let failures propagate (and log the
exception/stacktrace accordingly).
```suggestion
// Make status reporting best-effort: roll back the updated flag
on failure
actionContext.setUpdated(false);
LOGGER.warn(
"Report action status failed: xid={}, branchId={},
status={}",
actionContext.getXid(),
actionContext.getBranchId(),
status,
e);
```
##########
saga/seata-saga-annotation/src/test/java/org/apache/seata/saga/rm/SagaAnnotationResourceManagerTest.java:
##########
@@ -0,0 +1,333 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.saga.rm;
+
+import org.apache.seata.core.model.BranchStatus;
+import org.apache.seata.core.model.BranchType;
+import org.apache.seata.integration.tx.api.fence.hook.TccHook;
+import org.apache.seata.integration.tx.api.fence.hook.TccHookManager;
+import org.apache.seata.integration.tx.api.remoting.TwoPhaseResult;
+import org.apache.seata.rm.tcc.api.BusinessActionContext;
+import org.apache.seata.rm.tcc.api.BusinessActionContextUtil;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Unit tests for SagaAnnotationResourceManager.
+ *
+ * Focus areas:
+ * 1. branchRollback with TccHook before/after callbacks
+ * 2. Hook exception handling (should not break rollback)
+ * 3. Different compensation return types (boolean, TwoPhaseResult, null)
+ * 4. Compensation exception handling
+ */
+public class SagaAnnotationResourceManagerTest {
+
+ static {
+ System.setProperty("config.type", "file");
+ System.setProperty("config.file.name", "file.conf");
+ }
Review Comment:
This test sets global `System` properties in a static initializer and never
restores the previous values. That can leak state into other tests when the
full suite runs in the same JVM. Consider moving this to `@BeforeAll` and
clearing/restoring the properties in `@AfterAll`.
##########
saga/seata-saga-annotation/src/test/java/org/apache/seata/saga/rm/SagaAnnotationResourceManagerTest.java:
##########
@@ -0,0 +1,333 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.saga.rm;
+
+import org.apache.seata.core.model.BranchStatus;
+import org.apache.seata.core.model.BranchType;
+import org.apache.seata.integration.tx.api.fence.hook.TccHook;
+import org.apache.seata.integration.tx.api.fence.hook.TccHookManager;
+import org.apache.seata.integration.tx.api.remoting.TwoPhaseResult;
+import org.apache.seata.rm.tcc.api.BusinessActionContext;
+import org.apache.seata.rm.tcc.api.BusinessActionContextUtil;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Unit tests for SagaAnnotationResourceManager.
+ *
+ * Focus areas:
+ * 1. branchRollback with TccHook before/after callbacks
+ * 2. Hook exception handling (should not break rollback)
+ * 3. Different compensation return types (boolean, TwoPhaseResult, null)
+ * 4. Compensation exception handling
+ */
+public class SagaAnnotationResourceManagerTest {
+
+ static {
+ System.setProperty("config.type", "file");
+ System.setProperty("config.file.name", "file.conf");
+ }
+
+ private SagaAnnotationResourceManager resourceManager;
+
+ @BeforeEach
+ void setUp() {
+ TccHookManager.clear();
+ resourceManager = new SagaAnnotationResourceManager();
+ }
+
+ @AfterEach
+ void tearDown() {
+ TccHookManager.clear();
+ BusinessActionContextUtil.clear();
+ }
+
+ // ---- Helper classes ----
+
+ public static class TestCompensationTarget {
+ public boolean compensate(BusinessActionContext context) {
+ return true;
+ }
+
+ public boolean compensateFail(BusinessActionContext context) {
+ return false;
+ }
+
+ public Boolean compensateReturnNull(BusinessActionContext context) {
+ return null;
+ }
+
+ public TwoPhaseResult
compensateWithResultSuccess(BusinessActionContext context) {
+ return new TwoPhaseResult(true, "ok");
+ }
+
+ public TwoPhaseResult compensateWithResultFail(BusinessActionContext
context) {
+ return new TwoPhaseResult(false, "fail");
+ }
+
+ public boolean compensateThrow(BusinessActionContext context) {
+ throw new RuntimeException("compensation error");
+ }
+ }
+
+ public static class TrackingTccHook implements TccHook {
+ boolean beforeRollbackCalled = false;
+ boolean afterRollbackCalled = false;
+ boolean shouldThrowInBefore = false;
+ boolean shouldThrowInAfter = false;
+
+ @Override
+ public void beforeTccPrepare(String xid, Long branchId, String
actionName, BusinessActionContext context) {}
+
+ @Override
+ public void afterTccPrepare(String xid, Long branchId, String
actionName, BusinessActionContext context) {}
+
+ @Override
+ public void beforeTccCommit(String xid, Long branchId, String
actionName, BusinessActionContext context) {}
+
+ @Override
+ public void afterTccCommit(String xid, Long branchId, String
actionName, BusinessActionContext context) {}
+
+ @Override
+ public void beforeTccRollback(String xid, Long branchId, String
actionName, BusinessActionContext context) {
+ beforeRollbackCalled = true;
+ if (shouldThrowInBefore) {
+ throw new RuntimeException("hook error in beforeTccRollback");
+ }
+ }
+
+ @Override
+ public void afterTccRollback(String xid, Long branchId, String
actionName, BusinessActionContext context) {
+ afterRollbackCalled = true;
+ if (shouldThrowInAfter) {
+ throw new RuntimeException("hook error in afterTccRollback");
+ }
+ }
+ }
+
+ private SagaAnnotationResource createResource(String actionName, String
methodName) throws NoSuchMethodException {
+ SagaAnnotationResource resource = new SagaAnnotationResource();
+ resource.setActionName(actionName);
+ resource.setTargetBean(new TestCompensationTarget());
+ resource.setCompensationMethod(
+ TestCompensationTarget.class.getDeclaredMethod(methodName,
BusinessActionContext.class));
+ resource.setCompensationArgsClasses(new Class<?>[]
{BusinessActionContext.class});
+ resource.setPhaseTwoCompensationKeys(new String[] {"unused"});
+ return resource;
+ }
+
+ // ---- Tests for hook invocation in branchRollback ----
+
+ @Test
+ void testBranchRollbackWithHooksInvoked() throws Exception {
+ TrackingTccHook hook = new TrackingTccHook();
+ TccHookManager.registerHook(hook);
+
+ SagaAnnotationResource resource = createResource("testAction",
"compensate");
+ resourceManager.getManagedResources().put("testAction", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 1L, "testAction", null);
+
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ assertTrue(hook.beforeRollbackCalled, "beforeTccRollback should be
called");
+ assertTrue(hook.afterRollbackCalled, "afterTccRollback should be
called");
+ }
+
+ @Test
+ void testBranchRollbackHookExceptionInBeforeDoesNotBreakRollback() throws
Exception {
+ TrackingTccHook hook = new TrackingTccHook();
+ hook.shouldThrowInBefore = true;
+ TccHookManager.registerHook(hook);
+
+ SagaAnnotationResource resource = createResource("testAction2",
"compensate");
+ resourceManager.getManagedResources().put("testAction2", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 2L, "testAction2", null);
+
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ assertTrue(hook.beforeRollbackCalled);
+ assertTrue(hook.afterRollbackCalled, "afterTccRollback should still be
called even if before throws");
+ }
+
+ @Test
+ void testBranchRollbackHookExceptionInAfterDoesNotBreakRollback() throws
Exception {
+ TrackingTccHook hook = new TrackingTccHook();
+ hook.shouldThrowInAfter = true;
+ TccHookManager.registerHook(hook);
+
+ SagaAnnotationResource resource = createResource("testAction3",
"compensate");
+ resourceManager.getManagedResources().put("testAction3", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 3L, "testAction3", null);
+
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ assertTrue(hook.beforeRollbackCalled);
+ assertTrue(hook.afterRollbackCalled);
+ }
+
+ @Test
+ void testBranchRollbackWithoutHooks() throws Exception {
+ SagaAnnotationResource resource = createResource("testAction4",
"compensate");
+ resourceManager.getManagedResources().put("testAction4", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 4L, "testAction4", null);
+
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ }
+
+ // ---- Tests for different compensation return types ----
+
+ @Test
+ void testBranchRollbackCompensationReturnsFalse() throws Exception {
+ SagaAnnotationResource resource = createResource("testAction5",
"compensateFail");
+ resourceManager.getManagedResources().put("testAction5", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 5L, "testAction5", null);
+
+ assertEquals(BranchStatus.PhaseTwo_RollbackFailed_Retryable, status);
+ }
+
+ @Test
+ void testBranchRollbackCompensationReturnsNull() throws Exception {
+ SagaAnnotationResource resource = createResource("testAction6",
"compensateReturnNull");
+ resourceManager.getManagedResources().put("testAction6", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 6L, "testAction6", null);
+
+ // null return is treated as success
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ }
+
+ @Test
+ void testBranchRollbackCompensationReturnsTwoPhaseResultSuccess() throws
Exception {
+ SagaAnnotationResource resource = createResource("testAction7",
"compensateWithResultSuccess");
+ resourceManager.getManagedResources().put("testAction7", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 7L, "testAction7", null);
+
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ }
+
+ @Test
+ void testBranchRollbackCompensationReturnsTwoPhaseResultFail() throws
Exception {
+ SagaAnnotationResource resource = createResource("testAction8",
"compensateWithResultFail");
+ resourceManager.getManagedResources().put("testAction8", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 8L, "testAction8", null);
+
+ assertEquals(BranchStatus.PhaseTwo_RollbackFailed_Retryable, status);
+ }
+
+ @Test
+ void testBranchRollbackCompensationThrowsException() throws Exception {
+ SagaAnnotationResource resource = createResource("testAction9",
"compensateThrow");
+ resourceManager.getManagedResources().put("testAction9", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 9L, "testAction9", null);
+
+ assertEquals(BranchStatus.PhaseTwo_RollbackFailed_Retryable, status);
+ }
+
+ // ---- Tests for hook invocation with compensation failure ----
+
+ @Test
+ void testBranchRollbackWithHooksWhenCompensationFails() throws Exception {
+ TrackingTccHook hook = new TrackingTccHook();
+ TccHookManager.registerHook(hook);
+
+ SagaAnnotationResource resource = createResource("testAction10",
"compensateFail");
+ resourceManager.getManagedResources().put("testAction10", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 10L, "testAction10", null);
+
+ assertEquals(BranchStatus.PhaseTwo_RollbackFailed_Retryable, status);
+ assertTrue(hook.beforeRollbackCalled, "beforeTccRollback should be
called even when compensation fails");
+ assertTrue(hook.afterRollbackCalled, "afterTccRollback should be
called in finally block");
+ }
+
+ @Test
+ void testBranchRollbackWithHooksWhenCompensationThrows() throws Exception {
+ TrackingTccHook hook = new TrackingTccHook();
+ TccHookManager.registerHook(hook);
+
+ SagaAnnotationResource resource = createResource("testAction11",
"compensateThrow");
+ resourceManager.getManagedResources().put("testAction11", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 11L, "testAction11", null);
+
+ assertEquals(BranchStatus.PhaseTwo_RollbackFailed_Retryable, status);
+ assertTrue(hook.beforeRollbackCalled);
+ assertTrue(hook.afterRollbackCalled, "afterTccRollback should be
called in finally block even on exception");
+ }
+
+ // ---- Tests for multiple hooks ----
+
+ @Test
+ void testBranchRollbackWithMultipleHooks() throws Exception {
+ TrackingTccHook hook1 = new TrackingTccHook();
+ TrackingTccHook hook2 = new TrackingTccHook();
+ TccHookManager.registerHook(hook1);
+ TccHookManager.registerHook(hook2);
+
+ SagaAnnotationResource resource = createResource("testAction12",
"compensate");
+ resourceManager.getManagedResources().put("testAction12", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 12L, "testAction12", null);
+
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ assertTrue(hook1.beforeRollbackCalled);
+ assertTrue(hook1.afterRollbackCalled);
+ assertTrue(hook2.beforeRollbackCalled);
+ assertTrue(hook2.afterRollbackCalled);
+ }
+
+ @Test
+ void testBranchRollbackFirstHookThrowsSecondStillCalled() throws Exception
{
+ TrackingTccHook hook1 = new TrackingTccHook();
+ hook1.shouldThrowInBefore = true;
+ TrackingTccHook hook2 = new TrackingTccHook();
+ TccHookManager.registerHook(hook1);
+ TccHookManager.registerHook(hook2);
+
+ SagaAnnotationResource resource = createResource("testAction13",
"compensate");
+ resourceManager.getManagedResources().put("testAction13", resource);
+
+ BranchStatus status =
+ resourceManager.branchRollback(BranchType.SAGA_ANNOTATION,
"xid123", 13L, "testAction13", null);
+
+ assertEquals(BranchStatus.PhaseTwo_Rollbacked, status);
+ assertTrue(hook1.beforeRollbackCalled);
+ // hook2.beforeRollbackCalled is NOT guaranteed because hook1 throws
+ // the loop iterates hooks sequentially, and the exception breaks the
loop
Review Comment:
The comment says hook1’s exception breaks the hook loop and therefore
hook2’s `beforeTccRollback` isn’t guaranteed, but
`SagaAnnotationResourceManager#doBeforeSagaAnnotationRollback` catches
exceptions per-hook and continues iterating. This comment is misleading; update
it (and/or assert `hook2.beforeRollbackCalled` if that’s the intended behavior).
```suggestion
// Even if the first hook throws in beforeTccRollback, subsequent
hooks should still be invoked
assertTrue(hook2.beforeRollbackCalled);
```
--
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]
