From ee9fdeed6b354b1878e930f6f8a7363509d3175d Mon Sep 17 00:00:00 2001 From: ConnorYoh <40631091+ConnorYoh@users.noreply.github.com> Date: Thu, 11 Jun 2026 20:42:40 +0100 Subject: [PATCH] fix(payg): run the entitlement guard before the charge interceptor (#6622) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem When the `EntitlementGuard` refuses a request with **402** (team is over its free allowance / spending cap, or has no subscription to bill), the handler never runs — so it must not charge. But it did: the guard (order **1100**) ran *after* the charge interceptor (**1000**), so `openProcess` had already written the charge before the 402, and `afterCompletion` then billed it as "customer paid for the attempt" — a ledger debit, and a **Stripe meter for a subscribed-over-cap team**. ## Fix **Run the guard first** (order **900**, before the charge interceptor at 1000). Spring runs interceptors in ascending order on the way in and **skips a later interceptor's `preHandle` (and `afterCompletion`) entirely once an earlier one returns `false`** — so a refused request short-circuits with its 402 *before* the charge interceptor runs at all. A blocked request never opens a process, materialises inputs, or writes a charge. This replaces the earlier attribute-flag + `afterCompletion`-refund approach with a simpler reorder (per review): the no-charge-on-block guarantee is now **structural**, and it also avoids the wasted open-then-refund churn (no temp-file write, no debit/refund pair) for refused requests. ### Why the reorder is safe - `EntitlementGuard` reads no `PaygChargeInterceptor` state and has **no `afterCompletion`** (only `preHandle`), so reverse-order teardown is a non-issue. - The legacy `UnifiedCreditInterceptor` (default order **0**, and only registered under the `legacy-credits` profile) still runs first, so any legacy rejection wins. - For *admitted* requests both interceptors still run (guard then charge) — behaviour is unchanged; only refused requests now short-circuit before the charge. ## Tests `PaygWebMvcConfigTest` locks the `ENTITLEMENT_GUARD_ORDER < INTERCEPTOR_ORDER` invariant (if it's ever reversed, refused requests would bill again — this fails first). Existing `EntitlementGuardTest` already proves the guard returns 402 on a degraded/billable request. `:saas:test` + spotless green. ## Related (separate, in progress) The **fail-cleanly + fire-the-modal** half (suppress the error toast, trigger the subscribe/raise-cap modal via the existing `subscribed`/`category` signal — catching the 402 centrally so direct API usage is handled, and propagating the entitlement reason through the async policy-run status) lands **with Ethan's modal** so we don't remove the toast before there's a popup to replace it. --- .../saas/payg/filter/PaygWebMvcConfig.java | 26 +++++++++++-------- .../payg/filter/PaygWebMvcConfigTest.java | 22 ++++++++++++++++ 2 files changed, 37 insertions(+), 11 deletions(-) create mode 100644 app/saas/src/test/java/stirling/software/saas/payg/filter/PaygWebMvcConfigTest.java diff --git a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygWebMvcConfig.java b/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygWebMvcConfig.java index 30dd5a4ba..6397bfc27 100644 --- a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygWebMvcConfig.java +++ b/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygWebMvcConfig.java @@ -42,22 +42,26 @@ public class PaygWebMvcConfig implements WebMvcConfigurer { } /** - * Interceptor ordering: the legacy {@code UnifiedCreditInterceptor} (registered with default - * order = 0 in {@code CreditInterceptorConfig}) must run BEFORE this one so credit rejections - * short-circuit before we hash inputs. Explicit positive order guarantees this regardless of - * {@code WebMvcConfigurer} bean discovery order. + * The {@code PaygChargeInterceptor} runs after the {@link #ENTITLEMENT_GUARD_ORDER guard} (and + * after the legacy {@code UnifiedCreditInterceptor}, default order 0), so {@code openProcess} + * only fires for requests the guard has admitted. See {@link #ENTITLEMENT_GUARD_ORDER} for the + * full ordering rationale. */ public static final int INTERCEPTOR_ORDER = 1000; /** - * The entitlement guard runs AFTER the charge interceptor so cap-rejected requests still leave - * the charge interceptor's preHandle state in the consistent open-or-bypassed shape (and so the - * guard's 402 reaches the client without the charge interceptor needing to know about it). - * Spring runs interceptors in registration order on the way in, reverse order on the way out; - * the guard's preHandle short-circuit ({@code return false}) prevents the handler from running - * but Spring still invokes the charge interceptor's afterCompletion to clean up temp files. + * The {@code EntitlementGuard} runs BEFORE the charge interceptor. Spring runs interceptors in + * ascending order on the way in and skips a later interceptor's {@code preHandle} (and its + * {@code afterCompletion}) entirely once an earlier one returns {@code false} — so a request the + * guard refuses (over its free allowance / spending cap, or with no subscription to bill) + * short-circuits with its 402 before the charge interceptor ever runs. A blocked request + * therefore never opens a process, materialises inputs, or writes a charge: a refused operation + * must not bill, and running the guard first guarantees that structurally rather than by + * compensating after the fact. Stays above the legacy {@code UnifiedCreditInterceptor} (default + * order 0, only registered under the {@code legacy-credits} profile) so a legacy rejection still + * wins. */ - public static final int ENTITLEMENT_GUARD_ORDER = 1100; + public static final int ENTITLEMENT_GUARD_ORDER = 900; @Override public void addInterceptors(InterceptorRegistry registry) { diff --git a/app/saas/src/test/java/stirling/software/saas/payg/filter/PaygWebMvcConfigTest.java b/app/saas/src/test/java/stirling/software/saas/payg/filter/PaygWebMvcConfigTest.java new file mode 100644 index 000000000..b766fec4a --- /dev/null +++ b/app/saas/src/test/java/stirling/software/saas/payg/filter/PaygWebMvcConfigTest.java @@ -0,0 +1,22 @@ +package stirling.software.saas.payg.filter; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +/** + * Locks the interceptor ordering that makes "a blocked request is never charged" hold structurally: + * the {@link stirling.software.saas.payg.entitlement.EntitlementGuard} must run BEFORE the {@link + * PaygChargeInterceptor}. Spring skips a later interceptor's {@code preHandle} once an earlier one + * returns {@code false}, so guard-first means a refused (402) request never reaches {@code + * openProcess}. If these constants are ever reordered the wrong way, refused requests would start + * billing again — this test fails first. + */ +class PaygWebMvcConfigTest { + + @Test + void entitlementGuardRunsBeforeChargeInterceptor() { + assertThat(PaygWebMvcConfig.ENTITLEMENT_GUARD_ORDER) + .isLessThan(PaygWebMvcConfig.INTERCEPTOR_ORDER); + } +}