From a0e0e88f0731aa86d76fac84e3f5f4dd16cc3af3 Mon Sep 17 00:00:00 2001 From: ConnorYoh <40631091+ConnorYoh@users.noreply.github.com> Date: Thu, 28 May 2026 15:57:59 +0100 Subject: [PATCH] saas: harden CreditService Stripe ordering + lint @AutoJobPostMapping weights (#6458) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description of Changes Two narrowly-scoped hardening changes to the credits engine. ## 1. CreditService — move Stripe meter call to `afterCommit` The Stripe metered-usage call sits inside the surrounding `@Transactional`, holding the `user_credits` row lock for the duration of an HTTP round-trip to Supabase. Under load this starves concurrent debits; a transient Stripe blip rolls back a (correct) free-credit consumption and forces the caller to retry. The Stripe call now runs in a `TransactionSynchronization.afterCommit` hook — DB commits first, Stripe fires immediately after. If Stripe fails after commit, we log + increment a new `credits.stripe_report.failures` counter; the idempotency key is stable, so a manual replay recovers without double-charging. Applied to both `consumeCreditBySupabaseId` and `consumeCreditWithWaterfall`. **Dead-code removed:** - Unreachable UUID fallback for MDC `requestId` — `CorrelationIdFilter` already guarantees the key on every request. - The `"Unable to report usage to Stripe"` `RuntimeException` and its catch block — the afterCommit refactor eliminates the throw path. - `StripeRollbackOnFailureTest` — pinned the rollback-on-Stripe-fail behaviour this refactor replaces. ## 2. `@AutoJobPostMapping` — build-time lint for `resourceWeight` `UnifiedCreditInterceptor` multiplies `resourceWeight` into the per-call charge. An endpoint that falls through to the annotation default produces a charge derived from a value nobody chose. - Annotation default flipped from `1` to `Integer.MIN_VALUE` (sentinel). Both runtime readers (`UnifiedCreditInterceptor`, `AutoJobAspect`) already clamp into `[1, 100]` so behaviour is unchanged. - New `AutoJobPostMappingWeightTest` scans the classpath and fails the build if any method leaves the sentinel. - Initial run caught 11 endpoints relying on the default. Explicit weights now declared, chosen by comparing to peer endpoints: - `EditTextController` — LARGE - `EmailController#sendEmailWithAttachment` — SMALL - `ConvertPDFToMarkdown` — MEDIUM - `AttachmentController` (extract/list/rename/delete) — SMALL × 4 - `ConvertImgPDFController` (cbr/cbz ↔ pdf) — MEDIUM × 2, LARGE × 2 ## Tests - `StripeUsageIdempotencyKeyTest` — pins the `(supabaseId, overage, requestId)` idempotency key shape so Stripe always dedupes a retry. - `StripeAfterCommitOrderingTest` — pins that `afterCommit` fires after commit and NOT on rollback. - `AutoJobPostMappingWeightTest` — the lint itself, plus a self-check that the classpath scan finds at least 10 `@AutoJobPostMapping` methods (guards against the lint passing vacuously). Build verified: `ENABLE_SAAS=true ./gradlew :stirling-pdf:test :saas:test`. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) — no translation changes - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) — internal-billing change, no public docs impact - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) — N/A ### Translations (if applicable) - [ ] Not applicable ### UI Changes (if applicable) - [ ] Not applicable ### Testing (if applicable) - [x] I have run `task check` (via `./gradlew :stirling-pdf:test :saas:test` with `ENABLE_SAAS=true`) — passes - [x] I have tested my changes locally --- .../annotations/AutoJobPostMapping.java | 6 +- .../controller/api/EditTextController.java | 6 +- .../converters/ConvertImgPDFController.java | 20 +- .../api/misc/AttachmentController.java | 14 +- .../api/converters/ConvertPDFToMarkdown.java | 6 +- .../config/AutoJobPostMappingWeightTest.java | 132 +++++++++++ .../controller/api/EmailController.java | 6 +- .../software/saas/service/CreditService.java | 205 ++++++++++-------- .../StripeAfterCommitOrderingTest.java | 109 ++++++++++ .../service/StripeRollbackOnFailureTest.java | 132 ----------- .../StripeUsageIdempotencyKeyTest.java | 62 ++++++ 11 files changed, 464 insertions(+), 234 deletions(-) create mode 100644 app/core/src/test/java/stirling/software/SPDF/config/AutoJobPostMappingWeightTest.java create mode 100644 app/saas/src/test/java/stirling/software/saas/service/StripeAfterCommitOrderingTest.java delete mode 100644 app/saas/src/test/java/stirling/software/saas/service/StripeRollbackOnFailureTest.java create mode 100644 app/saas/src/test/java/stirling/software/saas/service/StripeUsageIdempotencyKeyTest.java diff --git a/app/common/src/main/java/stirling/software/common/annotations/AutoJobPostMapping.java b/app/common/src/main/java/stirling/software/common/annotations/AutoJobPostMapping.java index d3d79760d..0d91fedf1 100644 --- a/app/common/src/main/java/stirling/software/common/annotations/AutoJobPostMapping.java +++ b/app/common/src/main/java/stirling/software/common/annotations/AutoJobPostMapping.java @@ -77,6 +77,10 @@ public @interface AutoJobPostMapping { /** * Relative resource weight (1-100). See {@link * stirling.software.common.enumeration.ResourceWeight} for the standard tiers. + * + *

The default is a sentinel ({@link Integer#MIN_VALUE}); {@code + * AutoJobPostMappingWeightTest} fails the build if any endpoint leaves it unset. Runtime + * readers clamp the value into {@code [1, 100]}. */ - int resourceWeight() default 1; + int resourceWeight() default Integer.MIN_VALUE; } diff --git a/app/core/src/main/java/stirling/software/SPDF/controller/api/EditTextController.java b/app/core/src/main/java/stirling/software/SPDF/controller/api/EditTextController.java index 84f4c8f6e..ec4420fb1 100644 --- a/app/core/src/main/java/stirling/software/SPDF/controller/api/EditTextController.java +++ b/app/core/src/main/java/stirling/software/SPDF/controller/api/EditTextController.java @@ -32,6 +32,7 @@ import stirling.software.SPDF.model.json.PdfJsonTextElement; import stirling.software.SPDF.service.PdfJsonConversionService; import stirling.software.common.annotations.AutoJobPostMapping; import stirling.software.common.annotations.api.GeneralApi; +import stirling.software.common.enumeration.ResourceWeight; import stirling.software.common.model.api.general.EditTextOperation; import stirling.software.common.util.ExceptionUtils; import stirling.software.common.util.GeneralUtils; @@ -75,7 +76,10 @@ public class EditTextController { new StringToArrayListPropertyEditor<>(EditTextOperation.class)); } - @AutoJobPostMapping(consumes = "multipart/form-data", value = "/edit-text") + @AutoJobPostMapping( + consumes = "multipart/form-data", + value = "/edit-text", + resourceWeight = ResourceWeight.LARGE_WEIGHT) @StandardPdfResponse @Operation( summary = "Edit text in a PDF via find and replace", diff --git a/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertImgPDFController.java b/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertImgPDFController.java index 19e564418..1255a2a22 100644 --- a/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertImgPDFController.java +++ b/app/core/src/main/java/stirling/software/SPDF/controller/api/converters/ConvertImgPDFController.java @@ -275,7 +275,10 @@ public class ConvertImgPDFController { GeneralUtils.generateFilename(file[0].getOriginalFilename(), "_converted.pdf")); } - @AutoJobPostMapping(consumes = MediaType.MULTIPART_FORM_DATA_VALUE, value = "/cbz/pdf") + @AutoJobPostMapping( + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + value = "/cbz/pdf", + resourceWeight = ResourceWeight.MEDIUM_WEIGHT) @Operation( summary = "Convert CBZ comic book archive to PDF", description = @@ -301,7 +304,10 @@ public class ConvertImgPDFController { return WebResponseUtils.pdfFileToWebResponse(pdfFile, filename); } - @AutoJobPostMapping(consumes = MediaType.MULTIPART_FORM_DATA_VALUE, value = "/pdf/cbz") + @AutoJobPostMapping( + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + value = "/pdf/cbz", + resourceWeight = ResourceWeight.LARGE_WEIGHT) @Operation( summary = "Convert PDF to CBZ comic book archive", description = @@ -324,7 +330,10 @@ public class ConvertImgPDFController { return WebResponseUtils.zipFileToWebResponse(cbzFile, filename); } - @AutoJobPostMapping(consumes = MediaType.MULTIPART_FORM_DATA_VALUE, value = "/cbr/pdf") + @AutoJobPostMapping( + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + value = "/cbr/pdf", + resourceWeight = ResourceWeight.MEDIUM_WEIGHT) @Operation( summary = "Convert CBR comic book archive to PDF", description = @@ -350,7 +359,10 @@ public class ConvertImgPDFController { return WebResponseUtils.bytesToWebResponse(pdfBytes, filename); } - @AutoJobPostMapping(consumes = MediaType.MULTIPART_FORM_DATA_VALUE, value = "/pdf/cbr") + @AutoJobPostMapping( + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + value = "/pdf/cbr", + resourceWeight = ResourceWeight.LARGE_WEIGHT) @Operation( summary = "Convert PDF to CBR comic book archive", description = diff --git a/app/core/src/main/java/stirling/software/SPDF/controller/api/misc/AttachmentController.java b/app/core/src/main/java/stirling/software/SPDF/controller/api/misc/AttachmentController.java index 924013aba..0e272678c 100644 --- a/app/core/src/main/java/stirling/software/SPDF/controller/api/misc/AttachmentController.java +++ b/app/core/src/main/java/stirling/software/SPDF/controller/api/misc/AttachmentController.java @@ -141,7 +141,8 @@ public class AttachmentController { @AutoJobPostMapping( consumes = MediaType.MULTIPART_FORM_DATA_VALUE, - value = "/extract-attachments") + value = "/extract-attachments", + resourceWeight = ResourceWeight.SMALL_WEIGHT) @Operation( summary = "Extract attachments from PDF", description = @@ -176,7 +177,10 @@ public class AttachmentController { } } - @AutoJobPostMapping(consumes = MediaType.MULTIPART_FORM_DATA_VALUE, value = "/list-attachments") + @AutoJobPostMapping( + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + value = "/list-attachments", + resourceWeight = ResourceWeight.SMALL_WEIGHT) @Operation( summary = "List attachments in PDF", description = @@ -193,7 +197,8 @@ public class AttachmentController { @AutoJobPostMapping( consumes = MediaType.MULTIPART_FORM_DATA_VALUE, - value = "/rename-attachment") + value = "/rename-attachment", + resourceWeight = ResourceWeight.SMALL_WEIGHT) @StandardPdfResponse @Operation( summary = "Rename attachment in PDF", @@ -228,7 +233,8 @@ public class AttachmentController { @AutoJobPostMapping( consumes = MediaType.MULTIPART_FORM_DATA_VALUE, - value = "/delete-attachment") + value = "/delete-attachment", + resourceWeight = ResourceWeight.SMALL_WEIGHT) @StandardPdfResponse @Operation( summary = "Delete attachment from PDF", diff --git a/app/core/src/main/java/stirling/software/SPDF/model/api/converters/ConvertPDFToMarkdown.java b/app/core/src/main/java/stirling/software/SPDF/model/api/converters/ConvertPDFToMarkdown.java index d44233425..ce5a61078 100644 --- a/app/core/src/main/java/stirling/software/SPDF/model/api/converters/ConvertPDFToMarkdown.java +++ b/app/core/src/main/java/stirling/software/SPDF/model/api/converters/ConvertPDFToMarkdown.java @@ -13,6 +13,7 @@ import lombok.RequiredArgsConstructor; import stirling.software.SPDF.config.swagger.MarkdownConversionResponse; import stirling.software.common.annotations.AutoJobPostMapping; import stirling.software.common.annotations.api.ConvertApi; +import stirling.software.common.enumeration.ResourceWeight; import stirling.software.common.model.api.PDFFile; import stirling.software.common.util.PDFToFile; import stirling.software.common.util.TempFileManager; @@ -23,7 +24,10 @@ public class ConvertPDFToMarkdown { private final TempFileManager tempFileManager; - @AutoJobPostMapping(consumes = MediaType.MULTIPART_FORM_DATA_VALUE, value = "/pdf/markdown") + @AutoJobPostMapping( + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + value = "/pdf/markdown", + resourceWeight = ResourceWeight.MEDIUM_WEIGHT) @MarkdownConversionResponse @Operation( summary = "Convert PDF to Markdown", diff --git a/app/core/src/test/java/stirling/software/SPDF/config/AutoJobPostMappingWeightTest.java b/app/core/src/test/java/stirling/software/SPDF/config/AutoJobPostMappingWeightTest.java new file mode 100644 index 000000000..2b675650e --- /dev/null +++ b/app/core/src/test/java/stirling/software/SPDF/config/AutoJobPostMappingWeightTest.java @@ -0,0 +1,132 @@ +package stirling.software.SPDF.config; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.jupiter.api.Test; +import org.springframework.core.io.Resource; +import org.springframework.core.io.support.PathMatchingResourcePatternResolver; +import org.springframework.core.io.support.ResourcePatternResolver; +import org.springframework.core.type.classreading.CachingMetadataReaderFactory; +import org.springframework.core.type.classreading.MetadataReader; +import org.springframework.core.type.classreading.MetadataReaderFactory; +import org.springframework.core.type.filter.TypeFilter; + +import stirling.software.common.annotations.AutoJobPostMapping; + +/** + * Build-time guardrail: every {@link AutoJobPostMapping} method must declare an explicit {@code + * resourceWeight}. + * + *

The credits interceptor multiplies {@code resourceWeight} into the per-call charge. An + * endpoint that falls through to the annotation default produces a charge derived from a value + * nobody chose — silently under- or over-billing depending on the endpoint's true cost. Forcing + * each method to pick a value from {@link stirling.software.common.enumeration.ResourceWeight} + * keeps the choice deliberate. + * + *

The annotation's default is {@link Integer#MIN_VALUE} (a sentinel). Runtime readers clamp the + * value into {@code [1, 100]}, so a missed declaration can't crash production — this test is the + * contract, the clamp is the safety net. + * + *

Lives in {@code :stirling-pdf} (core) because that's the module whose compile classpath + * transitively sees every other module's controllers ({@code :common}, {@code :proprietary}, and + * {@code :saas} when enabled). + */ +class AutoJobPostMappingWeightTest { + + private static final String SCAN_BASE_PACKAGE = "stirling.software"; + + @Test + void everyAutoJobPostMappingDeclaresExplicitResourceWeight() throws Exception { + List offenders = findOffendingMethods(); + + assertTrue( + offenders.isEmpty(), + () -> + "The following @AutoJobPostMapping methods do not declare an explicit" + + " resourceWeight. Pick a value from" + + " stirling.software.common.enumeration.ResourceWeight (SMALL," + + " MEDIUM, LARGE, XLARGE) and add it to the annotation:\n - " + + String.join("\n - ", offenders)); + } + + private List findOffendingMethods() throws IOException, ClassNotFoundException { + List offenders = new ArrayList<>(); + for (Class candidate : scanForCandidateClasses()) { + for (Method method : candidate.getDeclaredMethods()) { + AutoJobPostMapping annotation = method.getAnnotation(AutoJobPostMapping.class); + if (annotation == null) { + continue; + } + if (annotation.resourceWeight() == Integer.MIN_VALUE) { + offenders.add(candidate.getName() + "#" + method.getName()); + } + } + } + return offenders; + } + + /** + * Returns every class under {@link #SCAN_BASE_PACKAGE} that has an @AutoJobPostMapping method. + */ + private List> scanForCandidateClasses() throws IOException, ClassNotFoundException { + ResourcePatternResolver resolver = new PathMatchingResourcePatternResolver(); + MetadataReaderFactory metadataReaderFactory = new CachingMetadataReaderFactory(resolver); + + String pattern = "classpath*:" + SCAN_BASE_PACKAGE.replace('.', '/') + "/**/*.class"; + Resource[] resources = resolver.getResources(pattern); + + // Pre-filter by reading annotation metadata from the class file so we don't have to load + // every class on the test classpath just to find the few that are annotated. + TypeFilter mentionsAutoJobPostMapping = + (reader, factory) -> + reader.getAnnotationMetadata() + .getAnnotatedMethods(AutoJobPostMapping.class.getName()) + .size() + > 0; + + List> matches = new ArrayList<>(); + for (Resource resource : resources) { + if (!resource.isReadable()) { + continue; + } + MetadataReader reader = metadataReaderFactory.getMetadataReader(resource); + if (!mentionsAutoJobPostMapping.match(reader, metadataReaderFactory)) { + continue; + } + matches.add(Class.forName(reader.getClassMetadata().getClassName())); + } + return matches; + } + + /** + * Sanity check that the classpath scan returns non-empty; otherwise the main test passes + * vacuously. + */ + @Test + void scannerFindsAtLeastOneAutoJobPostMapping() throws Exception { + long count = + scanForCandidateClasses().stream() + .flatMap(c -> java.util.Arrays.stream(c.getDeclaredMethods())) + .filter(m -> m.isAnnotationPresent(AutoJobPostMapping.class)) + .count(); + + assertTrue( + count > 10, + () -> + "Expected the classpath scan to find many @AutoJobPostMapping methods but" + + " found only " + + count + + ". Scanner regression?"); + } + + @SuppressWarnings("unused") + private static String describeCandidates(List> candidates) { + return candidates.stream().map(Class::getName).collect(Collectors.joining(", ")); + } +} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/EmailController.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/EmailController.java index 3fa41907a..213e08ea0 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/EmailController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/EmailController.java @@ -17,6 +17,7 @@ import lombok.extern.slf4j.Slf4j; import stirling.software.common.annotations.AutoJobPostMapping; import stirling.software.common.annotations.api.GeneralApi; +import stirling.software.common.enumeration.ResourceWeight; import stirling.software.proprietary.security.model.api.Email; import stirling.software.proprietary.security.service.EmailService; @@ -39,7 +40,10 @@ public class EmailController { * attachment. * @return ResponseEntity with success or error message. */ - @AutoJobPostMapping(consumes = MediaType.MULTIPART_FORM_DATA_VALUE, value = "/send-email") + @AutoJobPostMapping( + consumes = MediaType.MULTIPART_FORM_DATA_VALUE, + value = "/send-email", + resourceWeight = ResourceWeight.SMALL_WEIGHT) @Operation( summary = "Send an email with an attachment", description = diff --git a/app/saas/src/main/java/stirling/software/saas/service/CreditService.java b/app/saas/src/main/java/stirling/software/saas/service/CreditService.java index 426c70ae7..775c97d8d 100644 --- a/app/saas/src/main/java/stirling/software/saas/service/CreditService.java +++ b/app/saas/src/main/java/stirling/software/saas/service/CreditService.java @@ -15,6 +15,8 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.Gauge; @@ -55,6 +57,7 @@ public class CreditService { private final Counter creditsConsumedCounter; private final Counter creditConsumptionFailuresCounter; private final Counter cycleResetCounter; + private final Counter stripeReportFailuresCounter; public CreditService( UserCreditRepository userCreditRepository, @@ -90,6 +93,10 @@ public class CreditService { Counter.builder("credits.cycle_reset") .description("Number of credit cycle resets performed") .register(meterRegistry); + this.stripeReportFailuresCounter = + Counter.builder("credits.stripe_report.failures") + .description("Stripe meter post failed after the DB debit committed") + .register(meterRegistry); // Active gauges for current credit levels Gauge.builder("credits.total_available", this, CreditService::getTotalAvailableCredits) @@ -296,7 +303,8 @@ public class CreditService { return true; } } else { - // Partial or full overage: consume free credits and report overage to Stripe + // Partial or full overage: consume free credits in this tx, report the overage + // to Stripe after commit (see scheduleStripeReportAfterCommit). int freeCreditsUsed = userCredits.getCycleCreditsRemaining() != null ? userCredits.getCycleCreditsRemaining() @@ -328,55 +336,27 @@ public class CreditService { } } - // Stable idempotency key per (user, amount, operation) so retries dedupe. String operationId = MDC.get("requestId"); - if (operationId == null || operationId.isBlank()) { - operationId = UUID.randomUUID().toString(); - } String idempotencyKey = stripeUsageReportingService.generateIdempotencyKey( supabaseId, overageCredits, operationId); - log.info( - "[CREDIT-CONSUME] Calling Stripe reporting service - User: {}, Overage credits: {}, Idempotency key: {}", + scheduleStripeReportAfterCommit( supabaseId, overageCredits, - idempotencyKey); - - boolean reported = - stripeUsageReportingService.reportUsageToStripe( - supabaseId, overageCredits, idempotencyKey); - - log.info( - "[CREDIT-CONSUME] Stripe reporting result: {} for user: {}", - reported ? "SUCCESS" : "FAILED", - supabaseId); - - if (reported) { - creditsConsumedCounter.increment(creditAmount); - log.info( - "[USAGE-BASED] User {} consumed {} free + {} overage credits (total: {})", - supabaseId, - freeCreditsUsed, - overageCredits, - creditAmount); - return true; - } else { - log.error( - "[USAGE-BASED] Failed to report {} overage credits to Stripe for user: {}", - overageCredits, - supabaseId); - log.error( - "[USAGE-BASED] Throwing exception to fail the operation; metering must succeed"); - creditConsumptionFailuresCounter.increment(); - throw new RuntimeException( - "Unable to report usage to Stripe. Operation cannot proceed without metering. Please try again or contact support if the issue persists."); - } + idempotencyKey, + creditAmount, + freeCreditsUsed); + return true; } - // Free credits were sufficient; already consumed and returned above - // If we reach here, there's a logic error - log.error("[USAGE-BASED] Unexpected code path reached for user: {}", supabaseId); + // Lost a concurrent-debit race: the in-memory balance check passed but the atomic + // UPDATE found insufficient credits. Surface the failure so the caller can retry. + log.warn( + "[USAGE-BASED] Concurrent-debit race lost the free-tier consumption for" + + " user {}; caller should retry.", + supabaseId); + creditConsumptionFailuresCounter.increment(); return false; } @@ -411,17 +391,6 @@ public class CreditService { creditConsumptionFailuresCounter.increment(); return false; } catch (RuntimeException e) { - // Metering failures are critical and should fail the operation. - // This ensures users aren't charged for operations that weren't metered. - if (e.getMessage() != null - && e.getMessage().contains("Unable to report usage to Stripe")) { - log.error( - "[CREDIT-CONSUME] Metering failure; rethrowing exception to fail operation"); - throw e; - } - - // Other runtime exceptions are logged but don't fail the operation. - // This prevents transient errors from blocking user operations. log.error( "[CREDIT-CONSUME] Unexpected runtime error consuming credits for user: {} - {}", supabaseId, @@ -451,6 +420,87 @@ public class CreditService { return saasUserExtensionService.isMeteredBillingEnabled(user); } + /** + * Posts the Stripe meter event for an overage debit in a {@code TransactionSynchronization} + * afterCommit hook, so the DB row lock is released before the HTTP call to Stripe. + * + *

If no transaction is active (e.g. a test calling consume directly) the report runs + * synchronously instead, so the meter event still fires. + */ + private void scheduleStripeReportAfterCommit( + String supabaseId, + int overageCredits, + String idempotencyKey, + int creditAmount, + int freeCreditsUsed) { + + Runnable reportToStripe = + () -> { + log.info( + "[CREDIT-CONSUME] Posting Stripe meter event - User: {}, Overage: {}," + + " Idempotency: {}", + supabaseId, + overageCredits, + idempotencyKey); + + boolean reported; + try { + reported = + stripeUsageReportingService.reportUsageToStripe( + supabaseId, overageCredits, idempotencyKey); + } catch (RuntimeException e) { + // Don't let a Stripe exception unwind the afterCommit chain — the DB + // debit has already committed. + log.error( + "[CREDIT-CONSUME] Stripe meter post threw for user {} (overage {});" + + " usage owed-but-unbilled until a retry succeeds", + supabaseId, + overageCredits, + e); + stripeReportFailuresCounter.increment(); + return; + } + + if (reported) { + creditsConsumedCounter.increment(creditAmount); + log.info( + "[USAGE-BASED] User {} consumed {} free + {} overage credits" + + " (total: {}); Stripe meter posted.", + supabaseId, + freeCreditsUsed, + overageCredits, + creditAmount); + } else { + // DB has the debit, Stripe doesn't. The idempotency key is stable, so a + // replay with the same key recovers the meter event without + // double-charging. + stripeReportFailuresCounter.increment(); + log.error( + "[USAGE-BASED] Failed to post Stripe meter event for user {}" + + " (overage {}); usage owed-but-unbilled. Idempotency key" + + " is stable: replay with key '{}' to recover.", + supabaseId, + overageCredits, + idempotencyKey); + } + }; + + if (TransactionSynchronizationManager.isSynchronizationActive()) { + TransactionSynchronizationManager.registerSynchronization( + new TransactionSynchronization() { + @Override + public void afterCommit() { + reportToStripe.run(); + } + }); + } else { + log.warn( + "[CREDIT-CONSUME] No active transaction; reporting Stripe usage synchronously." + + " Expected only in tests."); + reportToStripe.run(); + } + } + /** Check if a user has credits available by Supabase ID (unified approach). */ public boolean hasCreditsAvailableBySupabaseId(String supabaseId) { Optional credits = getUserCreditsBySupabaseId(supabaseId); @@ -1054,48 +1104,23 @@ public class CreditService { // STEP 4: Try metered billing (check flag, not role) if (saasUserExtensionService.isMeteredBillingEnabled(user)) { log.info( - "[WATERFALL] User {} has metered billing enabled; reporting {} credits to Stripe", + "[WATERFALL] User {} has metered billing enabled; scheduling {} credits for" + + " Stripe report (after commit)", user.getUsername(), creditAmount); - try { - String operationId = MDC.get("requestId"); - if (operationId == null || operationId.isBlank()) { - operationId = UUID.randomUUID().toString(); - } - String idempotencyKey = - stripeUsageReportingService.generateIdempotencyKey( - supabaseId.toString(), creditAmount, operationId); + String operationId = MDC.get("requestId"); + String idempotencyKey = + stripeUsageReportingService.generateIdempotencyKey( + supabaseId.toString(), creditAmount, operationId); - boolean reported = - stripeUsageReportingService.reportUsageToStripe( - supabaseId.toString(), creditAmount, idempotencyKey); - - if (reported) { - creditsConsumedCounter.increment(creditAmount); - - log.info( - "[WATERFALL] Reported {} overage credits to Stripe for user: {}", - creditAmount, - user.getUsername()); - return CreditConsumptionResult.success("METERED_SUBSCRIPTION"); - } else { - log.error( - "[WATERFALL] Failed to report usage to Stripe for user: {}", - user.getUsername()); - creditConsumptionFailuresCounter.increment(); - return CreditConsumptionResult.failure("Failed to report usage to Stripe"); - } - } catch (Exception e) { - log.error( - "[WATERFALL] Exception while reporting to Stripe for user {}: {}", - user.getUsername(), - e.getMessage(), - e); - creditConsumptionFailuresCounter.increment(); - return CreditConsumptionResult.failure( - "Error reporting usage to Stripe: " + e.getMessage()); - } + scheduleStripeReportAfterCommit( + supabaseId.toString(), + creditAmount, + idempotencyKey, + creditAmount, + /* freeCreditsUsed= */ 0); + return CreditConsumptionResult.success("METERED_SUBSCRIPTION"); } else if (user.getRolesAsString().contains("ROLE_PRO_USER")) { // Pro user without metered billing enabled; reject with helpful message log.warn( diff --git a/app/saas/src/test/java/stirling/software/saas/service/StripeAfterCommitOrderingTest.java b/app/saas/src/test/java/stirling/software/saas/service/StripeAfterCommitOrderingTest.java new file mode 100644 index 000000000..6454dfe3b --- /dev/null +++ b/app/saas/src/test/java/stirling/software/saas/service/StripeAfterCommitOrderingTest.java @@ -0,0 +1,109 @@ +package stirling.software.saas.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; + +/** + * Pins the contract {@code CreditService.scheduleStripeReportAfterCommit} relies on: a {@link + * TransactionSynchronization#afterCommit()} hook fires after a successful commit and never on + * rollback. + */ +class StripeAfterCommitOrderingTest { + + @AfterEach + void clearSynchronization() { + if (TransactionSynchronizationManager.isSynchronizationActive()) { + TransactionSynchronizationManager.clear(); + } + } + + @Test + void afterCommitRunsAfterCommit_notDuringTransaction() { + List order = new ArrayList<>(); + + TransactionSynchronizationManager.initSynchronization(); + try { + order.add("inside-tx-before-register"); + TransactionSynchronizationManager.registerSynchronization( + new TransactionSynchronization() { + @Override + public void afterCommit() { + order.add("after-commit-hook"); + } + }); + order.add("inside-tx-after-register"); + + // Simulate commit by firing afterCommit on every registered synchronization. + order.add("commit-triggered"); + for (TransactionSynchronization s : + TransactionSynchronizationManager.getSynchronizations()) { + s.afterCommit(); + } + } finally { + TransactionSynchronizationManager.clearSynchronization(); + } + + assertThat(order) + .containsExactly( + "inside-tx-before-register", + "inside-tx-after-register", + "commit-triggered", + "after-commit-hook"); + } + + @Test + void afterCommitDoesNotRun_onRollback() { + List order = new ArrayList<>(); + + TransactionSynchronizationManager.initSynchronization(); + try { + TransactionSynchronizationManager.registerSynchronization( + new TransactionSynchronization() { + @Override + public void afterCommit() { + order.add("after-commit-hook-MUST-NOT-FIRE"); + } + + @Override + public void afterCompletion(int status) { + if (status == TransactionSynchronization.STATUS_ROLLED_BACK) { + order.add("after-completion-rollback"); + } + } + }); + + // Simulate rollback: afterCompletion fires, afterCommit must not. + for (TransactionSynchronization s : + TransactionSynchronizationManager.getSynchronizations()) { + s.afterCompletion(TransactionSynchronization.STATUS_ROLLED_BACK); + } + } finally { + TransactionSynchronizationManager.clearSynchronization(); + } + + assertThat(order) + .containsExactly("after-completion-rollback") + .doesNotContain("after-commit-hook-MUST-NOT-FIRE"); + } + + @Test + void isSynchronizationActive_reflectsSpringTransactionalContext() { + assertThat(TransactionSynchronizationManager.isSynchronizationActive()).isFalse(); + + TransactionSynchronizationManager.initSynchronization(); + try { + assertThat(TransactionSynchronizationManager.isSynchronizationActive()).isTrue(); + } finally { + TransactionSynchronizationManager.clearSynchronization(); + } + + assertThat(TransactionSynchronizationManager.isSynchronizationActive()).isFalse(); + } +} diff --git a/app/saas/src/test/java/stirling/software/saas/service/StripeRollbackOnFailureTest.java b/app/saas/src/test/java/stirling/software/saas/service/StripeRollbackOnFailureTest.java deleted file mode 100644 index 03bb8fa7d..000000000 --- a/app/saas/src/test/java/stirling/software/saas/service/StripeRollbackOnFailureTest.java +++ /dev/null @@ -1,132 +0,0 @@ -package stirling.software.saas.service; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertThrows; - -import java.util.concurrent.atomic.AtomicInteger; - -import org.junit.jupiter.api.Test; -import org.springframework.transaction.PlatformTransactionManager; -import org.springframework.transaction.support.AbstractPlatformTransactionManager; -import org.springframework.transaction.support.DefaultTransactionDefinition; -import org.springframework.transaction.support.DefaultTransactionStatus; -import org.springframework.transaction.support.TransactionTemplate; - -/** - * Verifies finding #5 (CreditService Stripe ordering / DB divergence) end-to-end. - * - *

Connor's claim: free credits are deducted before the Stripe overage call; if Stripe fails the - * code throws but the deduction has already committed. Earlier analysis flagged this BOGUS because - * the class is {@code @Transactional} and Spring rolls back on uncaught RuntimeException — but the - * subtlety I missed last time (with {@code @PreAuthorize hasRole}) means I want a real test rather - * than another argument-from-docs. - * - *

This test reproduces the exact Spring transaction wiring: a method annotated as transactional - * does (1) an in-transaction "deduct credits" write, then (2) throws a RuntimeException. We assert - * the transaction manager observes the throw and triggers {@code rollback()}, not {@code commit()}. - */ -class StripeRollbackOnFailureTest { - - @Test - void runtimeExceptionTriggersRollback_notCommit() { - AtomicInteger commits = new AtomicInteger(); - AtomicInteger rollbacks = new AtomicInteger(); - - PlatformTransactionManager tm = - new AbstractPlatformTransactionManager() { - @Override - protected Object doGetTransaction() { - return new Object(); - } - - @Override - protected void doBegin( - Object transaction, - org.springframework.transaction.TransactionDefinition def) { - // no-op - } - - @Override - protected void doCommit(DefaultTransactionStatus status) { - commits.incrementAndGet(); - } - - @Override - protected void doRollback(DefaultTransactionStatus status) { - rollbacks.incrementAndGet(); - } - }; - - TransactionTemplate template = - new TransactionTemplate(tm, new DefaultTransactionDefinition()); - - // This is the exact shape of CreditService.consumeCreditBySupabaseId when Stripe fails: - // 1. deduct free credits (already happened, line 318-320 in production) - // 2. call Stripe → returns false (mocked) - // 3. throw new RuntimeException("Unable to report usage to Stripe...") - // The throw escapes through the catch at line 413-420 (which re-throws metering failures). - RuntimeException thrown = - assertThrows( - RuntimeException.class, - () -> - template.executeWithoutResult( - status -> { - // Step 1: imaginary credit deduction happens here. - // Step 2: Stripe returns false. - // Step 3: throw — same wording as production line 372. - throw new RuntimeException( - "Unable to report usage to Stripe. Operation cannot proceed without metering."); - })); - - assertThat(thrown.getMessage()).contains("Unable to report usage to Stripe"); - assertThat(commits.get()) - .as("commit() must NOT be called when the method throws a RuntimeException") - .isZero(); - assertThat(rollbacks.get()) - .as("rollback() must be called when the method throws a RuntimeException") - .isEqualTo(1); - } - - @Test - void runtimeExceptionIsRethrown_notSwallowed_throughCatchBlock() { - // Sanity check that the actual catch logic at CreditService.java:413-420 re-throws the - // Stripe-failure RuntimeException rather than swallowing it. If it didn't re-throw, the - // transaction would commit. We rebuild the same try/catch shape here. - RuntimeException thrown = - assertThrows( - RuntimeException.class, - () -> consumeCreditMimicry(/* stripeReports= */ false)); - assertThat(thrown.getMessage()).contains("Unable to report usage to Stripe"); - } - - @Test - void runtimeExceptionIsSwallowed_forNonMeteringErrors() { - // Unrelated runtime exceptions are caught at CreditService.java:425-431 and swallowed - // (return false). This is per the existing behaviour so we just lock it in. - Boolean result = consumeCreditMimicry(/* stripeReports= */ true); - assertThat(result).isTrue(); - } - - /** Tiny inline mock of the catch chain in CreditService.consumeCreditBySupabaseId. */ - private static Boolean consumeCreditMimicry(boolean stripeReports) { - try { - // Step 1: deduct free credits (would have been DB write). - // Step 2: Stripe call. - if (!stripeReports) { - throw new RuntimeException( - "Unable to report usage to Stripe. Operation cannot proceed without metering."); - } - return true; - } catch (IllegalArgumentException e) { - return false; - } catch (RuntimeException e) { - if (e.getMessage() != null - && e.getMessage().contains("Unable to report usage to Stripe")) { - throw e; // re-thrown so @Transactional rolls back - } - return false; - } catch (Exception e) { - return false; - } - } -} diff --git a/app/saas/src/test/java/stirling/software/saas/service/StripeUsageIdempotencyKeyTest.java b/app/saas/src/test/java/stirling/software/saas/service/StripeUsageIdempotencyKeyTest.java new file mode 100644 index 000000000..8c3414e94 --- /dev/null +++ b/app/saas/src/test/java/stirling/software/saas/service/StripeUsageIdempotencyKeyTest.java @@ -0,0 +1,62 @@ +package stirling.software.saas.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import stirling.software.saas.billing.service.StripeUsageReportingService; +import stirling.software.saas.config.SupabaseConfigurationProperties; + +/** + * Pins the Stripe meter-event idempotency key as a deterministic function of (Supabase user, + * overage amount, request id). Stripe collapses duplicates by this key, so a regression here means + * customers get double-billed on a retry. + */ +class StripeUsageIdempotencyKeyTest { + + private final StripeUsageReportingService service = + new StripeUsageReportingService(Mockito.mock(SupabaseConfigurationProperties.class)); + + @Test + void sameInputs_produceSameKey() { + String first = service.generateIdempotencyKey("user-123", 10, "req-abc"); + String second = service.generateIdempotencyKey("user-123", 10, "req-abc"); + + assertThat(first) + .as("Idempotency key must be stable across calls with identical inputs.") + .isEqualTo(second); + } + + @Test + void differentRequestIds_produceDifferentKeys() { + String reqA = service.generateIdempotencyKey("user-123", 10, "req-abc"); + String reqB = service.generateIdempotencyKey("user-123", 10, "req-xyz"); + + assertThat(reqA).isNotEqualTo(reqB); + } + + @Test + void differentOverageAmounts_produceDifferentKeys() { + String tenCredits = service.generateIdempotencyKey("user-123", 10, "req-abc"); + String elevenCredits = service.generateIdempotencyKey("user-123", 11, "req-abc"); + + assertThat(tenCredits).isNotEqualTo(elevenCredits); + } + + @Test + void differentUsers_produceDifferentKeys() { + String alice = service.generateIdempotencyKey("user-alice", 10, "req-abc"); + String bob = service.generateIdempotencyKey("user-bob", 10, "req-abc"); + + assertThat(alice).isNotEqualTo(bob); + } + + @Test + void keyShapeIncludesAllThreeDimensions() { + // Format: usage_{supabaseId}_{credits}_{operationId} + String key = service.generateIdempotencyKey("user-123", 42, "req-abc"); + + assertThat(key).contains("user-123").contains("42").contains("req-abc"); + } +}