From 6ec833edb3dba2bb914f818f2db06041d502c055 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 1 Jun 2026 11:01:54 +0200 Subject: [PATCH 1/2] fix(billing): re-apply lost C.2 review nits (warn-on-unknown-priceId + 4 hardening) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5 Copilot review findings from PR #3743 (#1250 P0) were silently lost because a controller-side fix commit was never pushed to origin before the squash-merge. Re-applied fixes: 1. JSDoc for buildPriceIdToPlanMap: accurate description with @returns tag 2. resolvePlan: warn log when priceId not in map and metadata empty (operationally critical — same silent-free-downgrade shape as the P0 it was meant to prevent) 3. cancel_at truthy check → typeof === 'number' (prevents cancel_at=0 being skipped) 4. previousPlan: validatePlan() before comparison (rejects stale/invalid metadata values) 5. Test comment: corrects misleading "retryWithBackoff setTimeout suppression" claim Refs: #3743, plan 2026-05-30-trawl-devkit-perfect-alignment.md --- .../services/billing.webhook.service.js | 32 ++++++++++++++----- .../billing.webhook.priceid-map.unit.tests.js | 4 ++- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index 8cd48101d..c68f2cbad 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -58,12 +58,15 @@ const validatePlan = (plan) => { const planRanks = Object.fromEntries((config.billing?.plans || []).map((p, i) => [p, i])); /** - * Reverse-map from Stripe price ID → plan name, built from config.stripe.prices at boot. - * config.stripe.prices = { growth: { monthly: 'price_xxx', annual: 'price_yyy' }, pro: { ... } } - * This avoids relying on price.metadata.planId (empty on Stripe webhook payloads — planId - * lives on the Product, not the Price) and avoids a Stripe API call per webhook. + * Build a reverse-map from Stripe price ID → plan name, sourced from `config.stripe.prices` + * at module load. Shape: `{ growth: { monthly: 'price_xxx', annual: 'price_yyy' }, pro: {...} }`. * - * Fix: resolvePlan no longer reads price.metadata (always empty in real Stripe webhook payloads). + * Why: `price.metadata.planId` is empty on real Stripe webhook payloads — `planId` lives on + * the Product, not the Price exposed by `customer.subscription.updated`. The reverse-map gives + * a robust priceId→plan lookup without an extra Stripe API call per webhook. `resolvePlan` + * keeps `price.metadata.planId` as a legacy fallback (test fixtures, manual Stripe edits). + * + * @returns {Record} priceId → planId map (built once at module init) */ const buildPriceIdToPlanMap = () => { const map = {}; @@ -95,7 +98,17 @@ const resolvePlan = (subscription) => { } // Legacy fallback: price metadata set explicitly (e.g. test fixtures or manual Stripe edits) const rawMeta = item?.price?.metadata?.planId || item?.plan?.metadata?.planId; - return validatePlan(rawMeta) || 'free'; + const fromMeta = validatePlan(rawMeta); + if (fromMeta) return fromMeta; + // Last-resort fallback — warn so misconfigured config.stripe.prices is visible + // (otherwise this silently downgrades paid orgs to 'free', which is the exact bug #1250 fixed). + if (priceId) { + logger.warn('[billing.webhook] resolvePlan: priceId not in priceIdToPlan map and metadata empty — falling back to free', { + priceId, + stripeSubscriptionId: subscription?.id, + }); + } + return 'free'; }; /** @@ -421,7 +434,7 @@ const handleSubscriptionUpdated = async (subscription, event) => { if (typeof subscription.cancel_at_period_end === 'boolean') { fields.cancelAtPeriodEnd = subscription.cancel_at_period_end; } - if (subscription.cancel_at) { + if (typeof subscription.cancel_at === 'number') { fields.cancelAt = new Date(subscription.cancel_at * 1000); } else if (subscription.cancel_at === null) { fields.cancelAt = null; @@ -464,10 +477,13 @@ const handleSubscriptionUpdated = async (subscription, event) => { let planChangeResetTriggered = false; if (previousItems) { const previousPriceId = previousItems[0]?.price?.id; - const previousPlan = (previousPriceId && priceIdToPlan[previousPriceId]) + const previousRaw = (previousPriceId && priceIdToPlan[previousPriceId]) || previousItems[0]?.price?.metadata?.planId || previousItems[0]?.plan?.metadata?.planId || null; + // Validate against the canonical plan enum — legacy metadata can carry stale or invalid + // values that would otherwise emit plan.changed + trigger forceRotateForPlanChange with junk. + const previousPlan = validatePlan(previousRaw); if (previousPlan && previousPlan !== newPlan) { const prevRank = planRanks[previousPlan]; const newRank = planRanks[newPlan]; diff --git a/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js b/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js index 2bcd05cf9..9a02b7c26 100644 --- a/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js +++ b/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js @@ -68,7 +68,9 @@ const makeBaseSetup = (configOverride = {}) => { model: () => ({}), }, })); - // Suppress retryWithBackoff delays in tests (replace setTimeout with immediate resolve) + // Mock the failed-backfill repository so the retry recorder is a no-op in unit tests + // (does not suppress the retryWithBackoff setTimeout delays themselves — tests that exercise + // retry pathways still pay the real delay, so keep retry attempt counts low in fixtures). jest.unstable_mockModule('../repositories/billing.failedBackfill.repository.js', () => ({ default: { record: jest.fn().mockResolvedValue({}) }, })); From b25da05d7e7cba050b6686a3e5e93e7333b96d6a Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 1 Jun 2026 11:05:37 +0200 Subject: [PATCH 2/2] fix(billing): gate resolvePlan warn to metadata-absent case only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review finding on PR #3758: the previous warn fired whenever priceId was present and metadata didn't validate, but the log message said "metadata empty" — misleading when metadata IS present but invalid (validatePlan already warns in that case → double-warn). Gate: only emit the warn when rawMeta is also absent (!rawMeta), and update the message from "metadata empty" to "no metadata" to be precise. --- modules/billing/services/billing.webhook.service.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index c68f2cbad..cdbe741b0 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -100,10 +100,12 @@ const resolvePlan = (subscription) => { const rawMeta = item?.price?.metadata?.planId || item?.plan?.metadata?.planId; const fromMeta = validatePlan(rawMeta); if (fromMeta) return fromMeta; - // Last-resort fallback — warn so misconfigured config.stripe.prices is visible - // (otherwise this silently downgrades paid orgs to 'free', which is the exact bug #1250 fixed). - if (priceId) { - logger.warn('[billing.webhook] resolvePlan: priceId not in priceIdToPlan map and metadata empty — falling back to free', { + // Last-resort fallback — warn only when metadata is also absent so misconfigured + // config.stripe.prices is visible (otherwise this silently downgrades paid orgs to 'free', + // which is the exact bug #1250 fixed). When metadata is present but invalid, validatePlan() + // above already emitted an "unrecognized planId" warning — no double-warn needed. + if (priceId && !rawMeta) { + logger.warn('[billing.webhook] resolvePlan: priceId not in priceIdToPlan map and no metadata — falling back to free', { priceId, stripeSubscriptionId: subscription?.id, });