FINERACT-2304: Avoid retrying after the command succeed

2025-06-14 14:08:35 +02:00
parent e77f783609
commit 2c77f6a277
4 changed files with 237 additions and 49 deletions
@@ -25,6 +25,7 @@ import io.github.resilience4j.retry.RetryRegistry;
 import java.util.Arrays;
 import lombok.AllArgsConstructor;
 import org.apache.fineract.batch.service.BatchExecutionException;
+import org.apache.fineract.commands.exception.CommandResultPersistenceException;
 import org.apache.fineract.infrastructure.core.config.FineractProperties;
 import org.apache.fineract.infrastructure.core.domain.FineractRequestContextHolder;
 import org.springframework.stereotype.Service;
@@ -35,6 +36,7 @@ public class RetryConfigurationAssembler {

    public static final String EXECUTE_COMMAND = "executeCommand";
    public static final String BATCH_RETRY = "batchRetry";
+    public static final String COMMAND_RESULT_PERSISTENCE = "commandResultPersistence";
    private static final String LAST_EXECUTION_EXCEPTION_KEY = "LAST_EXECUTION_EXCEPTION";
    private final RetryRegistry registry;
    private final FineractProperties fineractProperties;
@@ -107,4 +109,13 @@ public class RetryConfigurationAssembler {

        return configBuilder;
    }
+
+    public Retry getRetryConfigurationForCommandResultPersistence() {
+        RetryConfig.Builder configBuilder = buildCommonExecuteCommandConfiguration();
+
+        configBuilder.retryOnException(e -> e instanceof RuntimeException && !(e instanceof CommandResultPersistenceException));
+
+        RetryConfig config = configBuilder.build();
+        return registry.retry(COMMAND_RESULT_PERSISTENCE, config);
+    }
 }
@@ -0,0 +1,26 @@
+/**
+ * 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.fineract.commands.exception;
+
+public class CommandResultPersistenceException extends RuntimeException {
+
+    public CommandResultPersistenceException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}
@@ -24,6 +24,7 @@ import static org.apache.http.HttpStatus.SC_OK;

 import com.google.gson.Gson;
 import com.google.gson.reflect.TypeToken;
+import io.github.resilience4j.retry.Retry;
 import java.lang.reflect.Type;
 import java.time.Instant;
 import java.util.ArrayList;
@@ -37,6 +38,7 @@ import org.apache.fineract.commands.configuration.RetryConfigurationAssembler;
 import org.apache.fineract.commands.domain.CommandProcessingResultType;
 import org.apache.fineract.commands.domain.CommandSource;
 import org.apache.fineract.commands.domain.CommandWrapper;
+import org.apache.fineract.commands.exception.CommandResultPersistenceException;
 import org.apache.fineract.commands.exception.UnsupportedCommandException;
 import org.apache.fineract.commands.handler.NewCommandSourceHandler;
 import org.apache.fineract.commands.provider.CommandHandlerProvider;
@@ -132,43 +134,74 @@ public class SynchronousCommandProcessingService implements CommandProcessingSer

            setIdempotencyKeyStoreFlag(true);

-            final CommandProcessingResult result;
-            try {
-                result = commandSourceService.processCommand(findCommandHandler(wrapper), command, commandSource, user,
-                        isApprovedByChecker);
-            } catch (Throwable t) { // NOSONAR
-                RuntimeException mappable = ErrorHandler.getMappable(t);
-                ErrorInfo errorInfo = commandSourceService.generateErrorInfo(mappable);
-                Integer statusCode = errorInfo.getStatusCode();
-                commandSource.setResultStatusCode(statusCode);
-                commandSource.setResult(errorInfo.getMessage());
-                if (statusCode != SC_OK) {
-                    commandSource.setStatus(ERROR);
-                }
-                if (!isEnclosingTransaction) { // TODO: temporary solution
-                    commandSource = commandSourceService.saveResultNewTransaction(commandSource);
-                }
-                // must not throw any exception; must persist in new transaction as the current transaction was already
-                // marked as rollback
-                publishHookErrorEvent(wrapper, command, errorInfo);
-                throw mappable;
-            }
-
-            commandSource.setResultStatusCode(SC_OK);
-            commandSource.updateForAudit(result);
-            commandSource.setResult(toApiResultJsonSerializer.serializeResult(result));
-            commandSource.setStatus(PROCESSED);
-            commandSource = commandSourceService.saveResultSameTransaction(commandSource);
-            storeCommandIdInContext(commandSource); // Store command id as a request attribute
-
-            result.setRollbackTransaction(null);
-            publishHookEvent(wrapper.entityName(), wrapper.actionName(), command, result); // TODO must be performed in
-                                                                                           // a
-            // new transaction
-            return result;
+            return executeCommand(wrapper, command, isApprovedByChecker, commandSource, user, isEnclosingTransaction);
        });
    }

+    private CommandProcessingResult executeCommand(final CommandWrapper wrapper, final JsonCommand command,
+            final boolean isApprovedByChecker, CommandSource commandSource, AppUser user, boolean isEnclosingTransaction) {
+
+        final CommandProcessingResult result;
+        try {
+            result = commandSourceService.processCommand(findCommandHandler(wrapper), command, commandSource, user, isApprovedByChecker);
+        } catch (Throwable t) { // NOSONAR
+            RuntimeException mappable = ErrorHandler.getMappable(t);
+            ErrorInfo errorInfo = commandSourceService.generateErrorInfo(mappable);
+            Integer statusCode = errorInfo.getStatusCode();
+            commandSource.setResultStatusCode(statusCode);
+            commandSource.setResult(errorInfo.getMessage());
+            if (statusCode != SC_OK) {
+                commandSource.setStatus(ERROR);
+            }
+            if (!isEnclosingTransaction) { // TODO: temporary solution
+                commandSourceService.saveResultNewTransaction(commandSource);
+            }
+            // must not throw any exception; must persist in new transaction as the current transaction was already
+            // marked as rollback
+            publishHookErrorEvent(wrapper, command, errorInfo);
+            throw mappable;
+        }
+
+        Retry persistenceRetry = retryConfigurationAssembler.getRetryConfigurationForCommandResultPersistence();
+
+        try {
+            CommandSource finalCommandSource = commandSource;
+            CommandSource savedCommandSource = persistenceRetry.executeSupplier(() -> {
+                // Get metrics for logging
+                long attemptNumber = persistenceRetry.getMetrics().getNumberOfFailedCallsWithRetryAttempt() + 1;
+
+                // Critical: Refetch on retry attempts (not on first attempt)
+                CommandSource currentSource = finalCommandSource;
+                if (attemptNumber > 1) {
+                    log.info("Retrying command result save - attempt {} for command ID {}", attemptNumber, finalCommandSource.getId());
+                    currentSource = commandSourceService.getCommandSource(finalCommandSource.getId());
+                }
+
+                // Update command source with results
+                currentSource.setResultStatusCode(SC_OK);
+                currentSource.updateForAudit(result);
+                currentSource.setResult(toApiResultJsonSerializer.serializeResult(result));
+                currentSource.setStatus(PROCESSED);
+
+                // Return saved command source
+                return commandSourceService.saveResultSameTransaction(currentSource);
+            });
+
+            // Command successfully saved
+            storeCommandIdInContext(savedCommandSource);
+
+        } catch (Exception e) {
+            // After all retries have been exhausted
+            log.error("Failed to persist command result after multiple retries for command ID {}", commandSource.getId(), e);
+            throw new CommandResultPersistenceException("Failed to persist command result after multiple retries", e);
+        }
+
+        result.setRollbackTransaction(null);
+        publishHookEvent(wrapper.entityName(), wrapper.actionName(), command, result); // TODO must be performed in a
+        // new transaction
+        return result;
+    }
+
    private void storeCommandIdInContext(CommandSource savedCommandSource) {
        if (savedCommandSource.getId() == null) {
            throw new IllegalStateException("Command source not saved");
@@ -20,8 +20,11 @@ package org.apache.fineract.commands.service;

 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.atLeast;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
@@ -35,11 +38,13 @@ import io.github.resilience4j.retry.RetryRegistry;
 import jakarta.servlet.http.HttpServletRequest;
 import java.time.Duration;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.fineract.batch.exception.ErrorInfo;
 import org.apache.fineract.commands.configuration.RetryConfigurationAssembler;
 import org.apache.fineract.commands.domain.CommandProcessingResultType;
 import org.apache.fineract.commands.domain.CommandSource;
 import org.apache.fineract.commands.domain.CommandWrapper;
+import org.apache.fineract.commands.exception.CommandResultPersistenceException;
 import org.apache.fineract.commands.handler.NewCommandSourceHandler;
 import org.apache.fineract.commands.provider.CommandHandlerProvider;
 import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService;
@@ -52,6 +57,7 @@ import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidati
 import org.apache.fineract.infrastructure.core.serialization.ToApiJsonSerializer;
 import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext;
 import org.apache.fineract.useradministration.domain.AppUser;
+import org.jetbrains.annotations.NotNull;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -128,6 +134,9 @@ public class SynchronousCommandProcessingServiceTest {
        var impl = new RetryConfigurationAssembler(retryRegistry, fineractProperties, fineractRequestContextHolder);
        var retry = impl.getRetryConfigurationForExecuteCommand();
        when(retryConfigurationAssembler.getRetryConfigurationForExecuteCommand()).thenReturn(retry);
+
+        var persistenceRetry = impl.getRetryConfigurationForCommandResultPersistence();
+        when(retryConfigurationAssembler.getRetryConfigurationForCommandResultPersistence()).thenReturn(persistenceRetry);
    }

    @AfterEach
@@ -145,11 +154,7 @@ public class SynchronousCommandProcessingServiceTest {

    @Test
    public void testExecuteCommandSuccessAfter2Fails() {
-        CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class);
-        when(commandWrapper.isDatatableResource()).thenReturn(false);
-        when(commandWrapper.isNoteResource()).thenReturn(false);
-        when(commandWrapper.isSurveyResource()).thenReturn(false);
-        when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false);
+        CommandWrapper commandWrapper = getCommandWrapper();

        long commandId = 1L;
        JsonCommand jsonCommand = Mockito.mock(JsonCommand.class);
@@ -371,11 +376,7 @@ public class SynchronousCommandProcessingServiceTest {

    @Test
    public void testExecuteCommandSuccess() {
-        CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class);
-        when(commandWrapper.isDatatableResource()).thenReturn(false);
-        when(commandWrapper.isNoteResource()).thenReturn(false);
-        when(commandWrapper.isSurveyResource()).thenReturn(false);
-        when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false);
+        CommandWrapper commandWrapper = getCommandWrapper();

        long commandId = 1L;
        JsonCommand jsonCommand = Mockito.mock(JsonCommand.class);
@@ -415,11 +416,7 @@ public class SynchronousCommandProcessingServiceTest {

    @Test
    public void testExecuteCommandFails() {
-        CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class);
-        when(commandWrapper.isDatatableResource()).thenReturn(false);
-        when(commandWrapper.isNoteResource()).thenReturn(false);
-        when(commandWrapper.isSurveyResource()).thenReturn(false);
-        when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false);
+        CommandWrapper commandWrapper = getCommandWrapper();
        JsonCommand jsonCommand = Mockito.mock(JsonCommand.class);
        Long commandId = jsonCommand.commandId();

@@ -457,6 +454,16 @@ public class SynchronousCommandProcessingServiceTest {
        verify(commandSourceService).generateErrorInfo(runtimeException);
    }

+    @NotNull
+    private static CommandWrapper getCommandWrapper() {
+        CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class);
+        when(commandWrapper.isDatatableResource()).thenReturn(false);
+        when(commandWrapper.isNoteResource()).thenReturn(false);
+        when(commandWrapper.isSurveyResource()).thenReturn(false);
+        when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false);
+        return commandWrapper;
+    }
+
    @Test
    public void publishHookEventHandlesInvalidJson() {
        String entityName = "entity";
@@ -473,4 +480,115 @@ public class SynchronousCommandProcessingServiceTest {

    private static final class RetryException extends RuntimeException {}

+    @Test
+    public void testExecuteCommandWithRetry() {
+        CommandWrapper commandWrapper = getCommandWrapper();
+        when(commandWrapper.isInterestPauseResource()).thenReturn(false);
+
+        long commandId = 1L;
+        JsonCommand jsonCommand = Mockito.mock(JsonCommand.class);
+        when(jsonCommand.commandId()).thenReturn(commandId);
+
+        NewCommandSourceHandler commandHandler = Mockito.mock(NewCommandSourceHandler.class);
+        CommandProcessingResult commandProcessingResult = Mockito.mock(CommandProcessingResult.class);
+        when(commandProcessingResult.isRollbackTransaction()).thenReturn(false);
+        when(commandHandler.processCommand(jsonCommand)).thenReturn(commandProcessingResult);
+        when(commandHandlerProvider.getHandler(Mockito.any(), Mockito.any())).thenReturn(commandHandler);
+
+        when(configurationDomainService.isMakerCheckerEnabledForTask(Mockito.any())).thenReturn(false);
+        String idempotencyKey = "test-idempotency-key";
+        when(idempotencyKeyResolver.resolve(commandWrapper)).thenReturn(idempotencyKey);
+
+        CommandSource commandSource = Mockito.mock(CommandSource.class);
+        when(commandSource.getId()).thenReturn(commandId);
+        when(commandSourceService.findCommandSource(commandWrapper, idempotencyKey)).thenReturn(null);
+
+        AppUser appUser = Mockito.mock(AppUser.class);
+        when(appUser.getId()).thenReturn(1L);
+        when(context.authenticatedUser(Mockito.any(CommandWrapper.class))).thenReturn(appUser);
+
+        when(commandSourceService.saveInitialNewTransaction(commandWrapper, jsonCommand, appUser, idempotencyKey))
+                .thenReturn(commandSource);
+        when(commandSourceService.processCommand(commandHandler, jsonCommand, commandSource, appUser, false))
+                .thenReturn(commandProcessingResult);
+
+        final AtomicInteger saveAttempts = new AtomicInteger(0);
+
+        doAnswer(invocation -> {
+            int attempt = saveAttempts.incrementAndGet();
+            if (attempt == 1) {
+                throw new RuntimeException("Database error on first attempt");
+            }
+            return commandSource;
+        }).when(commandSourceService).saveResultSameTransaction(any(CommandSource.class));
+
+        // When fetching the command source after failure, return the same mock
+        when(commandSourceService.getCommandSource(commandId)).thenReturn(commandSource);
+
+        // Execute the command
+        CommandProcessingResult result = underTest.executeCommand(commandWrapper, jsonCommand, false);
+
+        assertEquals(2, saveAttempts.get(), "Expected 2 save attempts");
+
+        verify(commandSource, atLeast(1)).setResultStatusCode(200);
+        verify(commandSource, atLeast(1)).updateForAudit(commandProcessingResult);
+        verify(commandSource, atLeast(1)).setResult(any());
+        verify(commandSource, atLeast(1)).setStatus(CommandProcessingResultType.PROCESSED);
+
+        assertEquals(commandProcessingResult, result);
+    }
+
+    @Test
+    public void testExecuteCommandWithMaxRetryFailure() {
+        CommandWrapper commandWrapper = getCommandWrapper();
+        when(commandWrapper.isInterestPauseResource()).thenReturn(false);
+
+        long commandId = 1L;
+        JsonCommand jsonCommand = Mockito.mock(JsonCommand.class);
+        when(jsonCommand.commandId()).thenReturn(commandId);
+
+        NewCommandSourceHandler commandHandler = Mockito.mock(NewCommandSourceHandler.class);
+        CommandProcessingResult commandProcessingResult = Mockito.mock(CommandProcessingResult.class);
+        when(commandProcessingResult.isRollbackTransaction()).thenReturn(false);
+        when(commandHandler.processCommand(jsonCommand)).thenReturn(commandProcessingResult);
+        when(commandHandlerProvider.getHandler(Mockito.any(), Mockito.any())).thenReturn(commandHandler);
+
+        when(configurationDomainService.isMakerCheckerEnabledForTask(Mockito.any())).thenReturn(false);
+        String idempotencyKey = "test-idempotency-key";
+        when(idempotencyKeyResolver.resolve(commandWrapper)).thenReturn(idempotencyKey);
+
+        CommandSource commandSource = Mockito.mock(CommandSource.class);
+        when(commandSource.getId()).thenReturn(commandId);
+        when(commandSourceService.findCommandSource(commandWrapper, idempotencyKey)).thenReturn(null);
+
+        AppUser appUser = Mockito.mock(AppUser.class);
+        when(appUser.getId()).thenReturn(1L);
+        when(context.authenticatedUser(Mockito.any(CommandWrapper.class))).thenReturn(appUser);
+
+        when(commandSourceService.saveInitialNewTransaction(commandWrapper, jsonCommand, appUser, idempotencyKey))
+                .thenReturn(commandSource);
+        when(commandSourceService.processCommand(commandHandler, jsonCommand, commandSource, appUser, false))
+                .thenReturn(commandProcessingResult);
+
+        final AtomicInteger saveAttempts = new AtomicInteger(0);
+
+        // Simulate persistent save failure - all retry attempts fail
+        RuntimeException persistentException = new RuntimeException("Database error persists");
+        doAnswer(invocation -> {
+            // Count the number of attempts
+            saveAttempts.incrementAndGet();
+            // Always throw the exception to trigger retry
+            throw persistentException;
+        }).when(commandSourceService).saveResultSameTransaction(any(CommandSource.class));
+
+        when(commandSourceService.getCommandSource(commandId)).thenReturn(commandSource);
+
+        CommandResultPersistenceException exception = assertThrows(CommandResultPersistenceException.class, () -> {
+            underTest.executeCommand(commandWrapper, jsonCommand, false);
+        });
+
+        assertEquals(persistentException, exception.getCause());
+
+        assertTrue(saveAttempts.get() >= 3, "Expected at least 3 save attempts, but got: " + saveAttempts.get());
+    }
 }