From cd498600bac051fd7437c31e186314c426dc825c Mon Sep 17 00:00:00 2001 From: arunderwood Date: Mon, 5 Jan 2026 21:59:35 -0800 Subject: [PATCH 1/2] fix(db): use TransactionTemplate for batched cleanup The previous @Transactional approach didn't work because deleteBatch() is called internally from executeCleanup() in the same class, bypassing Spring's proxy-based AOP (self-invocation problem). Switch to programmatic TransactionTemplate to ensure each batch commits in its own transaction, preventing long-running transactions when deleting millions of expired spots. --- .../scheduler/SpotCleanupService.java | 14 ++++++++--- .../scheduler/SpotCleanupServiceTest.java | 24 ++++++++++++++----- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/main/java/io/nextskip/spots/internal/scheduler/SpotCleanupService.java b/src/main/java/io/nextskip/spots/internal/scheduler/SpotCleanupService.java index e5c8f030..ff0982f8 100644 --- a/src/main/java/io/nextskip/spots/internal/scheduler/SpotCleanupService.java +++ b/src/main/java/io/nextskip/spots/internal/scheduler/SpotCleanupService.java @@ -7,7 +7,7 @@ import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.support.TransactionTemplate; import java.time.Duration; import java.time.Instant; @@ -37,12 +37,15 @@ public class SpotCleanupService { private static final int CLEANUP_BATCH_SIZE = 100_000; private final SpotRepository spotRepository; + private final TransactionTemplate transactionTemplate; private final Duration ttl; public SpotCleanupService( SpotRepository spotRepository, + TransactionTemplate transactionTemplate, @Value("${nextskip.spots.persistence.ttl:24h}") Duration ttl) { this.spotRepository = spotRepository; + this.transactionTemplate = transactionTemplate; this.ttl = ttl; } @@ -81,12 +84,17 @@ public int executeCleanup() { /** * Deletes a single batch of expired spots within its own transaction. * + *

Uses programmatic transaction management via {@link TransactionTemplate} + * to ensure each batch commits independently, even when called from within + * the same class (avoiding Spring's proxy-based AOP limitation). + * * @param cutoff delete spots created before this time * @return number of spots deleted in this batch */ - @Transactional int deleteBatch(Instant cutoff) { - return spotRepository.deleteExpiredSpotsBatch(cutoff, CLEANUP_BATCH_SIZE); + Integer deleted = transactionTemplate.execute( + status -> spotRepository.deleteExpiredSpotsBatch(cutoff, CLEANUP_BATCH_SIZE)); + return deleted != null ? deleted : 0; } /** diff --git a/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java b/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java index 8a6a52fb..b450370a 100644 --- a/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java +++ b/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java @@ -7,6 +7,8 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.transaction.support.TransactionCallback; +import org.springframework.transaction.support.TransactionTemplate; import java.time.Duration; import java.time.Instant; @@ -16,6 +18,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -34,11 +37,20 @@ class SpotCleanupServiceTest { @Mock private SpotRepository spotRepository; + @Mock + private TransactionTemplate transactionTemplate; + private SpotCleanupService cleanupService; @BeforeEach void setUp() { - cleanupService = new SpotCleanupService(spotRepository, DEFAULT_TTL); + // Configure TransactionTemplate mock to execute the callback directly + lenient().when(transactionTemplate.execute(any())) + .thenAnswer(invocation -> { + TransactionCallback callback = invocation.getArgument(0); + return callback.doInTransaction(null); + }); + cleanupService = new SpotCleanupService(spotRepository, transactionTemplate, DEFAULT_TTL); } // =========================================== @@ -105,7 +117,7 @@ void testExecuteCleanup_UsesTtlForCutoff() { @Test void testExecuteCleanup_CustomTtl_UsesConfiguredValue() { Duration customTtl = Duration.ofHours(12); - SpotCleanupService customService = new SpotCleanupService(spotRepository, customTtl); + SpotCleanupService customService = new SpotCleanupService(spotRepository, transactionTemplate, customTtl); Instant beforeCall = Instant.now(); when(spotRepository.deleteExpiredSpotsBatch(any(Instant.class), anyInt())) .thenReturn(0); @@ -149,7 +161,7 @@ void testGetTtl_DefaultTtl_Returns24Hours() { @Test void testGetTtl_CustomTtl_ReturnsConfiguredValue() { Duration customTtl = Duration.ofHours(48); - SpotCleanupService customService = new SpotCleanupService(spotRepository, customTtl); + SpotCleanupService customService = new SpotCleanupService(spotRepository, transactionTemplate, customTtl); assertThat(customService.getTtl()).isEqualTo(customTtl); } @@ -161,7 +173,7 @@ void testGetTtl_CustomTtl_ReturnsConfiguredValue() { @Test void testConstructor_NullTtl_DefaultsTo24Hours() { // The Spring @Value annotation provides the default, but we test the service works - SpotCleanupService service = new SpotCleanupService(spotRepository, Duration.ofHours(24)); + SpotCleanupService service = new SpotCleanupService(spotRepository, transactionTemplate, Duration.ofHours(24)); assertThat(service.getTtl()).isEqualTo(Duration.ofHours(24)); } @@ -169,7 +181,7 @@ void testConstructor_NullTtl_DefaultsTo24Hours() { @Test void testConstructor_ZeroTtl_Accepted() { // Edge case: immediate cleanup - SpotCleanupService service = new SpotCleanupService(spotRepository, Duration.ZERO); + SpotCleanupService service = new SpotCleanupService(spotRepository, transactionTemplate, Duration.ZERO); assertThat(service.getTtl()).isEqualTo(Duration.ZERO); } @@ -178,7 +190,7 @@ void testConstructor_ZeroTtl_Accepted() { void testConstructor_NegativeTtl_Accepted() { // Edge case: would delete future spots (unlikely but valid Duration) Duration negativeTtl = Duration.ofHours(-1); - SpotCleanupService service = new SpotCleanupService(spotRepository, negativeTtl); + SpotCleanupService service = new SpotCleanupService(spotRepository, transactionTemplate, negativeTtl); assertThat(service.getTtl()).isEqualTo(negativeTtl); } From 269221723bf6582a53fdc6fde66018618923e204 Mon Sep 17 00:00:00 2001 From: arunderwood Date: Mon, 5 Jan 2026 22:15:09 -0800 Subject: [PATCH 2/2] test(spots): add test for null TransactionTemplate return value Cover the null branch in SpotCleanupService.deleteBatch() to satisfy delta coverage requirements (80% branch coverage). The null case occurs when TransactionTemplate.execute() returns null during a rollback. --- .../internal/scheduler/SpotCleanupServiceTest.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java b/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java index b450370a..9b994f75 100644 --- a/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java +++ b/src/test/java/io/nextskip/spots/internal/scheduler/SpotCleanupServiceTest.java @@ -19,6 +19,7 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -149,6 +150,18 @@ void testExecuteCleanup_ExactBatchSize_ContinuesUntilPartialBatch() { verify(spotRepository, times(2)).deleteExpiredSpotsBatch(any(Instant.class), eq(BATCH_SIZE)); } + @Test + void testExecuteCleanup_TransactionReturnsNull_TreatedAsZero() { + // When TransactionTemplate.execute() returns null (e.g., rollback), treat as 0 deleted + // Reset the mock to clear the lenient stub and configure new behavior + reset(transactionTemplate); + when(transactionTemplate.execute(any())).thenReturn(null); + + int deleted = cleanupService.executeCleanup(); + + assertThat(deleted).isZero(); + } + // =========================================== // getTtl tests // ===========================================