From 52a2843bdf6985f23a731b25d496f5e0038b2432 Mon Sep 17 00:00:00 2001 From: mateaix <7333791@qq.com> Date: Sun, 13 Sep 2026 23:39:45 +0800 Subject: [PATCH] fix(goal): sync completion memory after transaction commit --- .../mate/goal/service/GoalServiceImpl.java | 58 +++++++++++++---- .../goal/GoalPersistenceIntegrationTest.java | 62 +++++++++++++++++++ 2 files changed, 107 insertions(+), 13 deletions(-) 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 5e3fcab4..6ac307c5 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 @@ -11,6 +11,11 @@ import org.springframework.dao.DuplicateKeyException; import org.springframework.context.ApplicationEventPublisher; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; +import org.springframework.transaction.support.TransactionTemplate; import vip.mate.audit.service.AuditEventService; import vip.mate.exception.MateClawException; import vip.mate.goal.config.GoalProperties; @@ -57,6 +62,7 @@ public class GoalServiceImpl implements GoalService { private final AuditEventService auditEventService; private final ObjectMapper objectMapper; private ApplicationEventPublisher applicationEventPublisher; + private PlatformTransactionManager transactionManager; /** * Optional — only set when the memory subsystem is wired. On goal @@ -89,6 +95,11 @@ public class GoalServiceImpl implements GoalService { this.applicationEventPublisher = publisher; } + @Autowired(required = false) + public void setTransactionManager(PlatformTransactionManager manager) { + this.transactionManager = manager; + } + // ==================== CRUD ==================== @Override @@ -431,24 +442,45 @@ public class GoalServiceImpl implements GoalService { writeEvent(id, GoalEventType.COMPLETED, null, detail); recordAudit("goal.completed", g, detail); - // Forward to long-term memory on completion. Best-effort: a failing - // memory pipeline must not roll back the DB transition. - if (memoryManager != null) { + syncCompletionMemoryAfterCommit(g, result); + return g; + } + + private void syncCompletionMemoryAfterCommit(GoalEntity goal, GoalEvaluationResult result) { + var target = memoryManager; + if (target == null) return; + // Snapshot values before returning the mutable entity to the caller. + Long agentId = goal.getAgentId(); + String conversationId = goal.getConversationId(); + String subject = "[goal completed] " + goal.getTitle(); + String summary = goal.getProgressSummary() != null && !goal.getProgressSummary().isBlank() + ? goal.getProgressSummary() : "Final score: " + (result != null ? result.score() : "—"); + Runnable sync = () -> { try { - String summary = g.getProgressSummary() != null && !g.getProgressSummary().isBlank() - ? g.getProgressSummary() - : "Final score: " + (result != null ? result.score() : "—"); - memoryManager.syncAll( - g.getAgentId(), - g.getConversationId(), - "[goal completed] " + g.getTitle(), - summary); + if (transactionManager != null) { + // afterCommit still has the old transaction's resources bound. + // Adapter DB writes need their own transaction to commit reliably. + TransactionTemplate independent = new TransactionTemplate(transactionManager); + independent.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + independent.executeWithoutResult(status -> target.syncAll(agentId, conversationId, subject, summary)); + } else { + target.syncAll(agentId, conversationId, subject, summary); + } } catch (Exception e) { log.debug("[GoalService] memory syncAll on goal completion failed: {}", e.getMessage()); } + }; + if (TransactionSynchronizationManager.isActualTransactionActive()) { + if (TransactionSynchronizationManager.isSynchronizationActive()) { + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + @Override public void afterCommit() { sync.run(); } + }); + } else { + log.debug("[GoalService] skipped completion memory: no commit synchronization available"); + } + } else { + sync.run(); // Direct/non-transactional callers retain best-effort behavior. } - - return g; } @Override 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 5c22140d..0a12cec7 100644 --- a/mateclaw-server/src/test/java/vip/mate/goal/GoalPersistenceIntegrationTest.java +++ b/mateclaw-server/src/test/java/vip/mate/goal/GoalPersistenceIntegrationTest.java @@ -229,6 +229,68 @@ class GoalPersistenceIntegrationTest { org.mockito.Mockito.verifyNoInteractions(memory); } + private GoalEntity readyForCompletion(String conversation, String title) { + GoalEntity goal = goalService.create(req(conversation, title), "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); + return goalService.getById(goal.getId()); + } + + @Test + void rolledBackCompletionDoesNotSyncMemory() { + GoalEntity goal = readyForCompletion("completion-memory-rollback", "report"); + new TransactionTemplate(transactionManager).executeWithoutResult(status -> { + goalService.markCompleted(goal.getId(), null); + status.setRollbackOnly(); + }); + assertEquals(GoalStatus.ACTIVE, goalService.getById(goal.getId()).getStatus()); + assertEquals(0L, goalService.listEvents(goal.getId(), 30).stream() + .filter(event -> "completed".equals(event.getEventType())).count()); + org.mockito.Mockito.verifyNoInteractions(memory); + } + + @Test + void completionMemoryRunsAfterCommitAndItsDatabaseWritesCommitIndependently() { + GoalEntity goal = readyForCompletion("completion-memory-after-commit", "original title"); + jdbc.execute("CREATE TABLE IF NOT EXISTS goal_memory_callback_probe(goal_id BIGINT PRIMARY KEY)"); + org.mockito.Mockito.doAnswer(call -> { + inIndependentTransaction(() -> assertEquals(GoalStatus.COMPLETED, + goalService.getById(goal.getId()).getStatus())); + assertEquals("[goal completed] original title", call.getArgument(2)); + jdbc.update("INSERT INTO goal_memory_callback_probe(goal_id) VALUES(?)", goal.getId()); + return null; + }).when(memory).syncAll(org.mockito.ArgumentMatchers.anyLong(), org.mockito.ArgumentMatchers.anyString(), + org.mockito.ArgumentMatchers.anyString(), org.mockito.ArgumentMatchers.anyString()); + new TransactionTemplate(transactionManager).executeWithoutResult(status -> { + GoalEntity returned = goalService.markCompleted(goal.getId(), null); + returned.setTitle("mutated after return"); + org.mockito.Mockito.verifyNoInteractions(memory); + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + @Override public void afterCommit() { + inIndependentTransaction(() -> assertEquals(1, jdbc.queryForObject( + "SELECT COUNT(*) FROM goal_memory_callback_probe WHERE goal_id=?", Integer.class, goal.getId()))); + } + }); + }); + org.mockito.Mockito.verify(memory, org.mockito.Mockito.times(1)).syncAll( + org.mockito.ArgumentMatchers.anyLong(), org.mockito.ArgumentMatchers.anyString(), + org.mockito.ArgumentMatchers.anyString(), org.mockito.ArgumentMatchers.anyString()); + } + + @Test + void memoryFailureCannotUndoCommittedCompletion() { + GoalEntity goal = readyForCompletion("completion-memory-failure", "report"); + org.mockito.Mockito.doThrow(new IllegalStateException("fixture failure")).when(memory).syncAll( + org.mockito.ArgumentMatchers.anyLong(), org.mockito.ArgumentMatchers.anyString(), + org.mockito.ArgumentMatchers.anyString(), org.mockito.ArgumentMatchers.anyString()); + goalService.markCompleted(goal.getId(), null); + assertEquals(GoalStatus.COMPLETED, goalService.getById(goal.getId()).getStatus()); + assertEquals(1L, goalService.listEvents(goal.getId(), 30).stream() + .filter(event -> "completed".equals(event.getEventType())).count()); + } + @Test @DisplayName("GoalStatus values persist as lowercase literals — load-bearing for uk_agent_goal_active_conv") void status_persistsAsLowercaseString() {