From 542f075759e9db66061487d747132c89ffe8754d Mon Sep 17 00:00:00 2001 From: mateaix <7333791@qq.com> Date: Sun, 13 Sep 2026 23:10:45 +0800 Subject: [PATCH] fix: avoid completion side effects on goal no-op transitions --- .../mate/goal/service/GoalServiceImpl.java | 5 +++ .../goal/GoalPersistenceIntegrationTest.java | 33 ++++++++++++++++++- .../mate/goal/service/GoalServiceTest.java | 18 ++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java b/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java index 20758c3c..5e3fcab4 100644 --- a/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java +++ b/mateclaw-server/src/main/java/vip/mate/goal/service/GoalServiceImpl.java @@ -378,7 +378,10 @@ public class GoalServiceImpl implements GoalService { } private GoalEntity completeGoal(Long id, GoalEvaluationResult result, boolean evaluated) { + boolean[] transitioned = {false}; GoalEntity g = retryOptimistic(id, "markCompleted", fresh -> { + // A failed CAS may retry against another worker's completed row. + transitioned[0] = false; if (fresh.getStatus().isTerminal()) { if (evaluated && fresh.getStatus() != GoalStatus.COMPLETED) { throw new MateClawException("err.goal.completion_not_verified", 409, @@ -416,8 +419,10 @@ public class GoalServiceImpl implements GoalService { w.set(GoalEntity::getCriteria, GoalCriteriaCodec.serialize(allPassed, objectMapper)); } bumpVersionAndTime(w); + transitioned[0] = true; return w; }); + if (!transitioned[0]) return g; Map detail = new LinkedHashMap<>(); detail.put("finalScore", result != null ? result.score() : null); detail.put("agentLlmCallsUsed", g.getAgentLlmCallsUsed()); diff --git a/mateclaw-server/src/test/java/vip/mate/goal/GoalPersistenceIntegrationTest.java b/mateclaw-server/src/test/java/vip/mate/goal/GoalPersistenceIntegrationTest.java index 1f85e420..5c22140d 100644 --- a/mateclaw-server/src/test/java/vip/mate/goal/GoalPersistenceIntegrationTest.java +++ b/mateclaw-server/src/test/java/vip/mate/goal/GoalPersistenceIntegrationTest.java @@ -4,6 +4,8 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import vip.mate.memory.spi.MemoryManager; import org.springframework.context.ApplicationEventPublisher; import org.springframework.dao.DuplicateKeyException; import org.springframework.jdbc.core.JdbcTemplate; @@ -57,10 +59,12 @@ import static org.junit.jupiter.api.Assertions.fail; "spring.datasource.url=jdbc:h2:mem:goal_persistence_${random.uuid};MODE=MySQL;DATABASE_TO_LOWER=TRUE;CASE_INSENSITIVE_IDENTIFIERS=TRUE;DB_CLOSE_DELAY=-1", "spring.ai.dashscope.api-key=test-key", "spring.main.web-application-type=none", - "mateclaw.goal.enabled=false" + "mateclaw.goal.enabled=false", "mateclaw.plugin.enabled=false", "mateclaw.skill.workspace.auto-init=false", + "mateclaw.skill.workspace.root=${java.io.tmpdir}/mateclaw-goal-persistence-skills-${random.uuid}" }) class GoalPersistenceIntegrationTest { + @MockBean private MemoryManager memory; @Autowired private GoalService goalService; @Autowired private JdbcTemplate jdbc; @Autowired private GoalContinuationStore continuations; @@ -198,6 +202,33 @@ class GoalPersistenceIntegrationTest { assertEquals(1L, saved.getEvaluationRevision()); } + @Test + void repeatedCompletionWritesOneEventAndSyncsMemoryOnce() { + GoalEntity goal = goalService.create(req("completion-event-idempotence", "report"), "alice"); + goalService.appendCriterion(goal.getId(), "report", "alice"); + var passed = new vip.mate.goal.model.GoalEvaluationResult(1.0, "", "completed", true, "fixture", 1, 0, + java.util.List.of(new vip.mate.goal.model.GoalChecklistVerdict.CriterionVerdict("C1", true, "report evidence")), null); + goalService.recordEvaluation(goal.getId(), passed, 0, 1); + goalService.markEvaluatedCompleted(goal.getId(), passed); + goalService.markEvaluatedCompleted(goal.getId(), passed); + goalService.markCompleted(goal.getId(), null); + assertEquals(1L, goalService.listEvents(goal.getId(), 30).stream() + .filter(event -> "completed".equals(event.getEventType())).count()); + org.mockito.Mockito.verify(memory, org.mockito.Mockito.times(1)).syncAll( + org.mockito.ArgumentMatchers.eq(1L), org.mockito.ArgumentMatchers.eq(goal.getConversationId()), + org.mockito.ArgumentMatchers.anyString(), org.mockito.ArgumentMatchers.anyString()); + } + + @Test + void abandonedGoalCannotBeRecordedAsCompletedByAnIdempotentCall() { + GoalEntity goal = goalService.create(req("abandoned-no-completion-event", "report"), "alice"); + goalService.abandon(goal.getId(), "alice"); + assertEquals(GoalStatus.ABANDONED, goalService.markCompleted(goal.getId(), null).getStatus()); + assertEquals(0L, goalService.listEvents(goal.getId(), 30).stream() + .filter(event -> "completed".equals(event.getEventType())).count()); + org.mockito.Mockito.verifyNoInteractions(memory); + } + @Test @DisplayName("GoalStatus values persist as lowercase literals — load-bearing for uk_agent_goal_active_conv") void status_persistsAsLowercaseString() { diff --git a/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java b/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java index a0ff786c..b23b6d01 100644 --- a/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/goal/service/GoalServiceTest.java @@ -37,6 +37,8 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -514,11 +516,27 @@ class GoalServiceTest { @Test void markCompleted_isIdempotent_onTerminal() { + var memory = mock(vip.mate.memory.spi.MemoryManager.class); + service.setMemoryManager(memory); GoalEntity g = persisted(1L, GoalStatus.COMPLETED); when(goalMapper.selectById(1L)).thenReturn(g); GoalEntity result = service.markCompleted(1L, null); assertEquals(GoalStatus.COMPLETED, result.getStatus()); verify(goalMapper, never()).update(any(), any(LambdaUpdateWrapper.class)); + verifyNoInteractions(eventMapper, auditEventService, memory); + } + + @Test + void completionCasLoserDoesNotRepeatWinnerSideEffects() { + var memory = mock(vip.mate.memory.spi.MemoryManager.class); + service.setMemoryManager(memory); + GoalEntity active = persisted(1L, GoalStatus.ACTIVE); + GoalEntity completed = statusFlipped(active, GoalStatus.COMPLETED); + when(goalMapper.selectById(1L)).thenReturn(active, completed); + when(goalMapper.update(any(), any(LambdaUpdateWrapper.class))).thenReturn(0); + assertEquals(GoalStatus.COMPLETED, service.markCompleted(1L, null).getStatus()); + verify(goalMapper, times(1)).update(any(), any(LambdaUpdateWrapper.class)); + verifyNoInteractions(eventMapper, auditEventService, memory); } @Test