fix(billing): resolve Stripe plan via the priceId map in admin sync and reconcile (#3969)

resolveStripePlan in billing.admin.service.js and billing.reconcile.service.js
read only price/plan.metadata.planId and fell back to 'free' — the exact defect
config.stripe.prices, never propagated here. Real Stripe Price objects carry no
metadata.planId, so:
  - syncOrgFromStripe (a DB write) could silently downgrade a paying org to free
    on the very tool ops runs to resolve a billing divergence.
  - runReconciliation (log-only) emitted a false planMismatch alert for every
    paid org, drowning real divergences.

Extracted the webhook's resolvePlan/buildPriceIdToPlanMap into a shared
modules/billing/lib/billing.planResolver.js so all three call sites resolve via
ONE implementation. The shared resolver returns null (never 'free') when
unresolved; the admin path now ABORTS the write (409) instead of guessing, and
the reconcile path skips the plan comparison instead of alerting.

Replaced the unrealistic metadata.planId-only test fixtures with priceId-based
ones matching real Stripe payloads, and added regression coverage: a paid
subscription with no metadata.planId resolves correctly in both admin and
reconcile, and admin sync never writes 'free' for it.

Closes #3964

Claude-Session: https://claude.ai/code/session_01WfNC8bt1TgL4AsiYgCEGup

Author: PierreBrisorgueil

ERRORS.md

@@ -31,3 +31,4 @@ Use this file as a compact memory of recurring AI mistakes.
 - [2026-06-04] repository: top-level `const Foo = mongoose.model('Foo')` in a repository file -> this is evaluated at import time; safe in an HTTP server (loadModels() runs first) but silently crashes standalone scripts (crons, migrations) with `MissingSchemaError` when import order differs; tests miss it because jest mocks intercept the module entirely; fix = lazy getter `const Foo = () => mongoose.model('Foo')` (call sites: `Foo().find(...)`) or dynamic import after `loadModels()` in the entrypoint; see pierreb-devkit/Node#3789
 - [2026-06-15] deps/audit: leaving `npm audit` advisories unaddressed on the assumption they need a major bump -> run `npm audit fix` (never `--force`) first; the runtime-tree DoS/ReDoS items (`qs`, `path-to-regexp`, `brace-expansion`) all fixed via in-range bumps, no residual. These are DoS-class but NOT attacker-reachable in this stack: Express route patterns are static (no user-controlled `path-to-regexp` input) and `qs`/`brace-expansion` only parse server-side query strings under fixed code paths — still bump them to keep the tree clean and avoid scanner noise.
 - [2026-07-16] security: `users.repository.js findByIdAndUpdatePopulated()` does `.populate()` with no `.select()`, so `organizations.controller.js switchOrganization` serialized the raw doc (password hash + OAuth tokens + reset/verification tokens) straight to the client; `users.account.controller.js me()` separately forwarded `providerData` (OAuth tokens) verbatim -> any endpoint returning a Mongoose user doc must go through `UserService.removeSensitive()` (whitelist, `modules/users/utils/sanitizeUser.js`) at the response boundary, never serialize `req.user`/a populated doc directly; a local-signup fixture's `providerData` defaults to `{}` so a naive falsy check won't catch this — seed a fake OAuth token in tests to prove the leak is actually closed; see pierreb-devkit/Node#3963
+- [2026-07-16] billing/stripe: the #3742 priceId-map fix (`resolvePlan`/`buildPriceIdToPlanMap`) was applied only to the webhook handler; `billing.admin.service.js` `resolveStripePlan` (admin force-sync, a DB WRITE path) and `billing.reconcile.service.js` `resolveStripePlan` (LOG-ONLY divergence check) kept their own copies reading only `metadata.planId` -> a paid org's Stripe subscription (which never carries `metadata.planId`) resolved to `'free'` on admin sync (silently downgrading a paying org) and produced a false `planMismatch` alert on every reconcile run; fix = one shared resolver (`modules/billing/lib/billing.planResolver.js`) used by all three call sites, and a null-unresolved sentinel (never guess `'free'`) so the admin write path ABORTS instead of downgrading and the reconcile log path skips the comparison instead of alerting; see pierreb-devkit/Node#3964

modules/billing/controllers/billing.admin.controller.js

@@ -139,7 +139,11 @@ const adminSyncFromStripe = async (req, res) => {
     return responses.success(res, 'subscription synced from Stripe')(result);
   } catch (err) {
     const status = err.status ?? 500;
-    const title = status === 404 ? 'Not Found' : status === 422 ? 'Unprocessable Entity' : status === 502 ? 'Bad Gateway' : 'Internal Server Error';
+    const title = status === 404 ? 'Not Found'
+      : status === 422 ? 'Unprocessable Entity'
+      : status === 409 ? 'Conflict'
+      : status === 502 ? 'Bad Gateway'
+      : 'Internal Server Error';
     return responses.error(res, status, title, 'Failed to sync from Stripe')(err);
   }
 };

modules/billing/lib/billing.planResolver.js

@@ -0,0 +1,91 @@
+/**
+ * Module dependencies
+ */
+import config from '../../../config/index.js';
+import logger from '../../../lib/services/logger.js';
+
+/**
+ * Valid plan names from config (immutable set for O(1) lookups).
+ */
+const validPlans = new Set(config.billing?.plans || ['free', 'starter', 'pro', 'enterprise']);
+
+/**
+ * Build a reverse-map from Stripe price ID → plan name, sourced from `config.stripe.prices`
+ * at module load. Shape: `{ starter: { monthly: 'price_xxx', annual: 'price_yyy' }, pro: {...} }`.
+ *
+ * Why: `price.metadata.planId` is empty on real Stripe payloads — `planId` lives on the
+ * Product, not the Price exposed by webhook/subscription objects. The reverse-map gives a
+ * robust priceId→plan lookup without an extra Stripe API call.
+ *
+ * Shared by the webhook handler, the admin force-sync tool, and the reconcile cron so all
+ * three resolve identically (#3964 / #1250 — the admin + reconcile copies had drifted into
+ * a metadata-only resolver that silently fell back to 'free' for every real subscription).
+ *
+ * @returns {Record<string, string>} priceId → planId map (built once at module init)
+ */
+export const buildPriceIdToPlanMap = () => {
+  const map = {};
+  const stripePrices = config.stripe?.prices || {};
+  for (const [planId, intervals] of Object.entries(stripePrices)) {
+    if (!validPlans.has(planId)) continue;
+    if (intervals?.monthly) map[intervals.monthly] = planId;
+    if (intervals?.annual) map[intervals.annual] = planId;
+  }
+  return map;
+};
+
+const priceIdToPlan = buildPriceIdToPlanMap();
+
+/**
+ * @description Look up the plan for a raw Stripe price ID (no subscription object needed).
+ * Used when only a bare price ID is available (e.g. reading `previous_attributes` on a
+ * webhook diff, where the previous line item is a partial object, not a full subscription).
+ * @param {string|undefined} priceId - Stripe price ID (price_xxx).
+ * @returns {string|undefined} plan name, or undefined when the price ID is not mapped.
+ */
+export const lookupPlanByPriceId = (priceId) => (priceId ? priceIdToPlan[priceId] : undefined);
+
+/**
+ * @description Resolve the plan name from a Stripe subscription object.
+ * Strategy (most-specific first):
+ *   1. config priceId map (price_xxx → planId) — robust, no metadata dependency.
+ *   2. price.metadata.planId / plan.metadata.planId legacy fallback (test fixtures, manual
+ *      Stripe edits) — validated against the known plan enum.
+ *   3. `null` when nothing resolves — deliberately NOT 'free'. Silently downgrading an
+ *      unresolvable paid subscription to 'free' is the exact defect this resolver replaces;
+ *      the caller decides the safe behavior for its context (log-only vs a DB write).
+ * @param {Object} subscription - Stripe subscription object.
+ * @param {Object} [opts]
+ * @param {string} [opts.logPrefix] - Log tag for the caller module (e.g. '[billing.admin]').
+ * @returns {string|null} plan name, or null when unresolved.
+ */
+export const resolvePlanFromSubscription = (subscription, { logPrefix = '[billing]' } = {}) => {
+  const item = subscription?.items?.data?.[0];
+  const priceId = item?.price?.id;
+  if (priceId && priceIdToPlan[priceId]) {
+    return priceIdToPlan[priceId];
+  }
+
+  // Legacy fallback: price/plan metadata set explicitly (e.g. test fixtures or manual Stripe edits).
+  const rawMeta = item?.price?.metadata?.planId || item?.plan?.metadata?.planId;
+  if (rawMeta) {
+    if (validPlans.has(rawMeta)) return rawMeta;
+    logger.warn(`${logPrefix} resolvePlanFromSubscription: unrecognized planId in metadata`, {
+      raw: rawMeta,
+      validPlans: [...validPlans],
+    });
+    return null;
+  }
+
+  // Nothing resolved — warn so a misconfigured config.stripe.prices (or an unmapped plan
+  // such as a manually-sold enterprise price) is visible instead of silently downgrading.
+  if (priceId) {
+    logger.warn(`${logPrefix} resolvePlanFromSubscription: priceId not in priceIdToPlan map and no metadata`, {
+      priceId,
+      stripeSubscriptionId: subscription?.id,
+    });
+  }
+  return null;
+};
+
+export default { buildPriceIdToPlanMap, resolvePlanFromSubscription, lookupPlanByPriceId };

modules/billing/services/billing.admin.service.js

@@ -1,7 +1,6 @@
 /**
  * Module dependencies
  */
-import config from '../../../config/index.js';
 import getStripe from '../lib/stripe.js';
 import logger from '../../../lib/services/logger.js';
 import { bumpEventMarkers } from '../lib/billing.markerBump.js';
@@ -11,11 +10,7 @@ import OrganizationRepository from '../../organizations/repositories/organizatio
 import BillingExtraBalanceRepository from '../repositories/billing.extraBalance.repository.js';
 import BillingWebhookService from './billing.webhook.service.js';
 import { getDollarsToUnitRatio } from '../lib/billing.constants.js';
-
-/**
- * Valid plan names from config (immutable set for O(1) lookups).
- */
-const validPlans = new Set(config.billing?.plans || ['free', 'starter', 'pro', 'enterprise']);
+import { resolvePlanFromSubscription } from '../lib/billing.planResolver.js';
 
 /**
  * @function getCustomerStatus
@@ -93,6 +88,29 @@ const syncOrgFromStripe = async (orgId) => {
 
   const stripeSub = await stripe.subscriptions.retrieve(existing.stripeSubscriptionId);
   const newPlan = resolveStripePlan(stripeSub);
+
+  // Safety guard (#3964): never silently downgrade a paying org to 'free'. If the Stripe
+  // subscription's price doesn't map via config.stripe.prices AND carries no valid
+  // metadata.planId, the plan is genuinely unresolvable (misconfigured price map, or an
+  // unmapped price such as a manually-sold enterprise plan) — abort the write entirely
+  // rather than guess. Ops must fix the config.stripe.prices mapping (or set the plan via
+  // bumpOrgPlan) before retrying sync.
+  if (!newPlan) {
+    logger.error('[billing.admin] syncOrgFromStripe — cannot resolve plan from Stripe subscription, aborting sync to avoid downgrading org', {
+      orgId,
+      stripeSubscriptionId: existing.stripeSubscriptionId,
+      stripePriceId: stripeSub.items?.data?.[0]?.price?.id ?? null,
+      currentDbPlan: existing.plan,
+    });
+    throw Object.assign(
+      new Error(
+        `cannot resolve plan for Stripe subscription ${existing.stripeSubscriptionId} (orgId=${orgId}) — `
+        + 'no priceId→plan mapping and no valid metadata.planId; aborting sync to avoid corrupting the org\'s plan',
+      ),
+      { status: 409 },
+    );
+  }
+
   const newStatus = stripeSub.status;
   // Stripe API ≥ 2025-08-27 moved current_period_start to items.data[0].
   // Read from items first, fall back to top-level for older API versions (mirrors webhook handler).
@@ -373,17 +391,19 @@ const creditDisputeReinstated = async (chargeId, amountCents, reason, refundRequ
 
 /**
  * @function resolveStripePlan
- * @description Resolve the plan name from a Stripe subscription object.
- *              Same heuristic as billing.webhook.service.js (price.metadata.planId first).
+ * @description Resolve the plan name from a Stripe subscription object, via the shared
+ *              priceId→plan resolver (`../lib/billing.planResolver.js` — also used by the
+ *              webhook handler and the reconcile cron; #3964/#1250).
+ *              Unlike the webhook resolver, this does NOT fall back to 'free' when
+ *              unresolved — `syncOrgFromStripe` is a direct DB write on the admin escape-hatch
+ *              path, and silently downgrading an unresolvable paid subscription to 'free' is
+ *              the exact defect this fixes. Returns `null` so the caller can abort instead.
  * @param {Object} subscription - Stripe subscription object.
- * @returns {string} plan name.
+ * @returns {string|null} plan name, or null when unresolved (caller must abort the write).
  */
 // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik
-const resolveStripePlan = (subscription) => {
-  const item = subscription.items?.data?.[0];
-  const raw = item?.price?.metadata?.planId || item?.plan?.metadata?.planId;
-  return validPlans.has(raw) ? raw : 'free';
-};
+const resolveStripePlan = (subscription) =>
+  resolvePlanFromSubscription(subscription, { logPrefix: '[billing.admin]' });
 
 /**
  * @function dispatchWebhookEvent

modules/billing/services/billing.reconcile.service.js

@@ -6,6 +6,7 @@ import getStripe from '../lib/stripe.js';
 import logger from '../../../lib/services/logger.js';
 import billingEvents from '../lib/events.js';
 import { currentWeekKey, weekStartDate } from '../lib/billing.isoWeek.js';
+import { resolvePlanFromSubscription } from '../lib/billing.planResolver.js';
 
 /**
  * Page size for the reconciliation cursor — fetch in batches to avoid long-running queries.
@@ -17,23 +18,20 @@ const RECONCILE_PAGE_SIZE = 100;
  */
 const RECONCILE_STATUSES = ['active', 'past_due', 'trialing', 'unpaid'];
 
-/**
- * Valid plan names from config.
- */
-const validPlans = new Set(config.billing?.plans || ['free', 'starter', 'pro', 'enterprise']);
-
 /**
  * @function resolveStripePlan
- * @description Resolve the plan name from a Stripe subscription object.
+ * @description Resolve the plan name from a Stripe subscription object, via the shared
+ *              priceId→plan resolver (`../lib/billing.planResolver.js` — also used by the
+ *              webhook handler and the admin force-sync tool; #3964/#1250). Returns `null`
+ *              when unresolved (LOG-ONLY caller — never guesses 'free', which previously
+ *              produced a false `planMismatch` divergence alert for every paid org whose
+ *              price lacked `metadata.planId`, i.e. every real Stripe subscription).
  * @param {Object} subscription - Stripe subscription object.
- * @returns {string} plan name.
+ * @returns {string|null} plan name, or null when unresolved.
  */
 // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik
-const resolveStripePlan = (subscription) => {
-  const item = subscription.items?.data?.[0];
-  const raw = item?.price?.metadata?.planId || item?.plan?.metadata?.planId;
-  return validPlans.has(raw) ? raw : 'free';
-};
+const resolveStripePlan = (subscription) =>
+  resolvePlanFromSubscription(subscription, { logPrefix: '[billing.reconcile]' });
 
 /**
  * @function runReconciliation
@@ -193,7 +191,19 @@ const _reconcileOne = async (stripe, sub) => {
   const stripePlan = resolveStripePlan(stripeSub);
 
   const statusMismatch = sub.status !== stripeStatus;
-  const planMismatch = sub.plan !== stripePlan;
+  // stripePlan === null means the price is unresolvable (no priceId mapping, no valid
+  // metadata) — skip the plan comparison rather than risk a false divergence against a
+  // guessed 'free'. Log distinctly so a misconfigured config.stripe.prices stays visible.
+  if (stripePlan === null) {
+    logger.warn('[billing.reconcile] cannot resolve Stripe plan — skipping plan comparison for this subscription', {
+      organizationId: orgId,
+      subscriptionId: String(sub._id),
+      stripeSubscriptionId: subId,
+      stripePriceId: stripeSub.items?.data?.[0]?.price?.id ?? null,
+      dbPlan: sub.plan,
+    });
+  }
+  const planMismatch = stripePlan !== null && sub.plan !== stripePlan;
 
   // Check meter↔extras divergence for this org (Opus C1 detection layer).
   // This runs regardless of status/plan match — a healthy subscription can still

modules/billing/services/billing.webhook.service.js

@@ -16,6 +16,7 @@ import billingEvents from '../lib/events.js';
 import { SENTINEL_PENDING } from '../lib/billing.constants.js';
 import { retryWithBackoff } from '../lib/billing.retry.js';
 import { isNonTransientStripeError } from '../lib/billing.stripe-errors.js';
+import { resolvePlanFromSubscription, lookupPlanByPriceId } from '../lib/billing.planResolver.js';
 import AnalyticsService from '../../../lib/services/analytics.js';
 
 /**
@@ -58,61 +59,18 @@ const validatePlan = (plan) => {
  */
 const planRanks = Object.fromEntries((config.billing?.plans || []).map((p, i) => [p, i]));
 
-/**
- * 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: {...} }`.
- *
- * 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<string, string>} priceId → planId map (built once at module init)
- */
-const buildPriceIdToPlanMap = () => {
-  const map = {};
-  const stripePrices = config.stripe?.prices || {};
-  for (const [planId, intervals] of Object.entries(stripePrices)) {
-    if (!validPlans.has(planId)) continue;
-    if (intervals.monthly) map[intervals.monthly] = planId;
-    if (intervals.annual) map[intervals.annual] = planId;
-  }
-  return map;
-};
-const priceIdToPlan = buildPriceIdToPlanMap();
-
 /**
  * @description Resolve the plan name from a Stripe subscription object.
- * Strategy (most-specific first):
- *   1. config price-ID map (price_xxx → planId) — robust, no metadata dependency.
- *   2. price.metadata.planId legacy fallback (works only if metadata was explicitly set).
- *   3. plan.metadata.planId further legacy fallback.
- *   4. 'free' when nothing resolves.
+ * Delegates to the shared priceId→plan resolver (see `../lib/billing.planResolver.js`,
+ * also used by the admin force-sync tool and the reconcile cron — #3964/#1250). Strategy
+ * (most-specific first): config priceId map → price/plan.metadata.planId legacy fallback →
+ * 'free' when nothing resolves (webhook-specific last resort: an event still needs a plan
+ * written; unlike the admin/reconcile call sites, there is no "abort" option mid-webhook).
  * @param {Object} subscription - Stripe subscription object
  * @returns {string} plan name
  */
-const resolvePlan = (subscription) => {
-  const item = subscription.items?.data?.[0];
-  const priceId = item?.price?.id;
-  if (priceId && priceIdToPlan[priceId]) {
-    return priceIdToPlan[priceId];
-  }
-  // Legacy fallback: price metadata set explicitly (e.g. test fixtures or manual Stripe edits)
-  const rawMeta = item?.price?.metadata?.planId || item?.plan?.metadata?.planId;
-  const fromMeta = validatePlan(rawMeta);
-  if (fromMeta) return fromMeta;
-  // 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,
-    });
-  }
-  return 'free';
-};
+const resolvePlan = (subscription) =>
+  resolvePlanFromSubscription(subscription, { logPrefix: '[billing.webhook]' }) ?? 'free';
 
 /**
  * @description Sync the organization plan field to match the subscription plan.
@@ -480,7 +438,7 @@ const handleSubscriptionUpdated = async (subscription, event) => {
   let planChangeResetTriggered = false;
   if (previousItems) {
     const previousPriceId = previousItems[0]?.price?.id;
-    const previousRaw = (previousPriceId && priceIdToPlan[previousPriceId])
+    const previousRaw = lookupPlanByPriceId(previousPriceId)
       || previousItems[0]?.price?.metadata?.planId
       || previousItems[0]?.plan?.metadata?.planId
       || null;