mirror of
https://github.com/arsvendg/Stirling-PDF.git
synced 2026-07-14 10:34:06 +02:00
saas: harden CreditService Stripe ordering + lint @AutoJobPostMapping weights (#6458)
# 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
This commit is contained in:
ConnorYoh
+5
-1
@@ -77,6 +77,10 @@ public @interface AutoJobPostMapping {
|
||||
/**
|
||||
* Relative resource weight (1-100). See {@link
|
||||
* stirling.software.common.enumeration.ResourceWeight} for the standard tiers.
|
||||
*
|
||||
* <p>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;
|
||||
}
|
||||
|
||||
+5
-1
@@ -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",
|
||||
|
||||
+16
-4
@@ -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 =
|
||||
|
||||
+10
-4
@@ -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",
|
||||
|
||||
+5
-1
@@ -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",
|
||||
|
||||
+132
@@ -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}.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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<String> 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<String> findOffendingMethods() throws IOException, ClassNotFoundException {
|
||||
List<String> 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<Class<?>> 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<Class<?>> 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<Class<?>> candidates) {
|
||||
return candidates.stream().map(Class::getName).collect(Collectors.joining(", "));
|
||||
}
|
||||
}
|
||||
+5
-1
@@ -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 =
|
||||
|
||||
@@ -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);
|
||||
idempotencyKey,
|
||||
creditAmount,
|
||||
freeCreditsUsed);
|
||||
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.");
|
||||
}
|
||||
}
|
||||
|
||||
// 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.
|
||||
*
|
||||
* <p>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<UserCredit> 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);
|
||||
|
||||
boolean reported =
|
||||
stripeUsageReportingService.reportUsageToStripe(
|
||||
supabaseId.toString(), creditAmount, idempotencyKey);
|
||||
|
||||
if (reported) {
|
||||
creditsConsumedCounter.increment(creditAmount);
|
||||
|
||||
log.info(
|
||||
"[WATERFALL] Reported {} overage credits to Stripe for user: {}",
|
||||
scheduleStripeReportAfterCommit(
|
||||
supabaseId.toString(),
|
||||
creditAmount,
|
||||
user.getUsername());
|
||||
idempotencyKey,
|
||||
creditAmount,
|
||||
/* freeCreditsUsed= */ 0);
|
||||
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());
|
||||
}
|
||||
} else if (user.getRolesAsString().contains("ROLE_PRO_USER")) {
|
||||
// Pro user without metered billing enabled; reject with helpful message
|
||||
log.warn(
|
||||
|
||||
+109
@@ -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<String> 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<String> 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();
|
||||
}
|
||||
}
|
||||
-132
@@ -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.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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;
|
||||
}
|
||||
}
|
||||
}
|
||||
+62
@@ -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");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user