fix(billing): stop the webhook from forcing free on an unresolvable price (#3974)

* fix(billing): stop the webhook from forcing free on an unresolvable price

customer.subscription.{updated,created} and invoice.payment_succeeded
resolved an unmappable Stripe price (no config.stripe.prices entry AND
no valid metadata.planId — e.g. a manually-sold enterprise price) to a
hardcoded 'free' fallback, silently downgrading a paying org on the
next webhook delivery. #3964 fixed the same defect on the admin
(409 abort) and reconcile (skip-comparison) call sites but explicitly
left the webhook's fallback in place, since a webhook cannot abort
mid-flight.

resolvePlan now retains the org/subscription's already-loaded current
plan on an unresolvable price instead of forcing 'free', and emits
billing.webhook.plan_unresolved (mirrors the billing.refund.unresolved
alert pattern already used in this file) so ops can review. Only a
subscription with zero prior plan reference anywhere (a genuinely new
org/customer) still defaults to 'free' — the least-harmful default for
a fresh signup. A genuinely-free priceId/metadata mapping still
resolves 'free' normally, unaffected.

Closes #3970

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

* fix(billing): wire the plan_unresolved ops alert listener

billing.webhook.plan_unresolved (#3970) was emitted from
billing.webhook.service.js but had no billingEvents.on() listener —
EventEmitter.emit silently no-ops with zero listeners, so the
manual-review alert never reached the ops/ntfy channel. Registers the
listener in billing.init.js mirroring the billing.refund.unresolved /
billing.reconciliation.divergence pattern exactly (logger.error with
ntfyPriority: 4 — same "manual review" priority as those two siblings).
Adds the matching unit test case.

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

Author: PierreBrisorgueil

ERRORS.md

@@ -34,3 +34,4 @@ Use this file as a compact memory of recurring AI mistakes.
 - [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
 - [2026-07-16] architecture: `users.service.js remove()`'s sole-owner cascade called `OrganizationsRepository.remove()` directly instead of `organizations.crud.service.js#remove()`, silently skipping `runOrganizationRemovedHandlers` (the `onOrganizationRemoved` seam `modules/tasks/tasks.init.js` registers for org-scoped task cleanup) -> deleting an org must always route through the owning module's SERVICE method, never straight to its Repository, even for a "bare" cascade delete from another module — a direct-repository shortcut silently bypasses any registered removal hook; a stale cross-test fixture in `tasks.integration.tests.js` (an orphaned-but-still-existing task reused across unrelated test cases) was inadvertently relying on this bug and needed a properly isolated fixture once the cascade actually cleaned up; see pierreb-devkit/Node#3965
 - [2026-07-17] error handling: shared helpers (`lib/helpers/mailer/index.js#sendMail`, `modules/audit/services/audit.service.js#log`) wrapped the failing call in their own try/catch, logged a single generic line, and resolved `null` -> every caller's own context-rich `.catch()` (action/userId/orgId) became dead code on a real outage, since the promise never rejected; a central helper must let the underlying call's rejection propagate and let each CALL SITE own the "never break the main flow" `.catch()` with its own context — never swallow centrally just because most callers currently attach a catch; see pierreb-devkit/Node#3966
+- [2026-07-17] billing/stripe: #3964 fixed the admin (409 abort) and reconcile (skip-comparison) call sites of the shared plan resolver but left `billing.webhook.service.js resolvePlan`'s `?? 'free'` fallback in place -> a webhook for an existing paid org whose Stripe price is unresolvable (unmapped `priceId`, no valid `metadata.planId` — e.g. a manually-sold enterprise price) silently downgraded the org to `'free'` on the next `customer.subscription.updated`/`.created`/`invoice.payment_succeeded` event; a webhook cannot 409-abort mid-flight like the admin path, so the fix instead retains the org's/subscription's already-loaded `.plan` (never a hardcoded `'free'`) and emits `billing.webhook.plan_unresolved` for manual review — only a brand-new subscription with zero prior plan reference (no DB row anywhere for that org) still defaults to `'free'`; see pierreb-devkit/Node#3970

modules/billing/billing.init.js

@@ -234,6 +234,22 @@ export default async (app) => {
     });
   });
 
+  // billing.webhook.plan_unresolved — priority 4 (high): webhook couldn't resolve a Stripe
+  // subscription price to a plan; the write retained the last-known plan instead of forcing
+  // 'free' (#3970), and this alert is the manual-review half of that guard.
+  billingEvents.on('billing.webhook.plan_unresolved', (payload) => {
+    const { organizationId, eventId, stripeSubscriptionId, priceId, retainedPlan, hadKnownPlan } = payload;
+    logger.error('[billing.init] ALERT: webhook plan unresolved — manual review required', {
+      organizationId,
+      eventId,
+      stripeSubscriptionId,
+      priceId,
+      retainedPlan,
+      hadKnownPlan,
+      ntfyPriority: 4,
+    });
+  });
+
   // Prevent accidental crash if any future code emits 'error' with no listener
   // (Node default behaviour: throws if no 'error' listener is registered).
   // Registered here (after config is ready) so events.js stays config-free and importable without ordering hazards.

modules/billing/lib/events.js

@@ -18,6 +18,10 @@ import { EventEmitter } from 'events';
  *   - `billing.refund.unresolved` — emitted when a charge.refunded event cannot be correlated
  *     to a known session/org (missing metadata on charge AND PaymentIntent, or ambiguous pack).
  *     Payload: { chargeId, paymentIntentId, refundAmount } | { reason, orgId, stripeSessionId, amountRefundedCents }
+ *   - `billing.webhook.plan_unresolved` — emitted when a webhook's Stripe subscription price
+ *     cannot be resolved to a plan (unmapped priceId AND no valid metadata.planId) — the write
+ *     retains the last-known plan (or 'free' for a genuinely new org) instead of forcing 'free'.
+ *     Payload: { organizationId, stripeSubscriptionId, priceId, retainedPlan, hadKnownPlan, eventId }
  *
  * NOTE: The 'error' event listener is registered in billing.init.js (after config is ready)
  * to avoid module-load-time config reads in this low-level singleton.

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

@@ -60,17 +60,55 @@ const validatePlan = (plan) => {
 const planRanks = Object.fromEntries((config.billing?.plans || []).map((p, i) => [p, i]));
 
 /**
- * @description Resolve the plan name from a Stripe subscription object.
- * 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).
+ * @description Resolve the plan name from a Stripe subscription object for a webhook write path.
+ * 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 → unresolvable.
+ *
+ * On unresolvable (unmapped priceId AND no valid metadata.planId — e.g. a manually-sold
+ * enterprise price, or a misconfigured `config.stripe.prices`), this does NOT force 'free'.
+ * Unlike the admin tool (aborts the write with a 409) or the reconcile cron (skips the plan
+ * comparison, log-only), a webhook cannot abort mid-flight — an event still needs a plan
+ * written. Instead it retains `fallbackPlan` (the org/subscription's last-known plan, passed by
+ * the caller from the already-loaded DB doc) so an ambiguous price never silently downgrades a
+ * paying org, and emits an alert for manual review (mirrors the `billing.refund.unresolved`
+ * alert pattern used elsewhere in this file). When `fallbackPlan` is itself unknown (no prior
+ * plan reference — a genuinely brand-new subscription/org), defaults to 'free': the
+ * least-harmful default for a fresh signup, an ops/config error either way, and still surfaced
+ * via the alert (#3970).
  * @param {Object} subscription - Stripe subscription object
+ * @param {string|null} [fallbackPlan] - Last-known plan to retain on ambiguity; null/undefined → 'free'.
+ * @param {Object} [ctx={}] - Alert context merged into the log/emit payload (e.g. organizationId, eventId).
  * @returns {string} plan name
  */
-const resolvePlan = (subscription) =>
-  resolvePlanFromSubscription(subscription, { logPrefix: '[billing.webhook]' }) ?? 'free';
+const resolvePlan = (subscription, fallbackPlan = null, ctx = {}) => {
+  const resolved = resolvePlanFromSubscription(subscription, { logPrefix: '[billing.webhook]' });
+  if (resolved !== null) return resolved;
+
+  const hadKnownPlan = fallbackPlan !== null && fallbackPlan !== undefined;
+  const retainedPlan = hadKnownPlan ? fallbackPlan : 'free';
+  const priceId = subscription?.items?.data?.[0]?.price?.id ?? null;
+
+  logger.error(
+    '[billing.webhook] resolvePlan — unresolvable price, NOT forcing free (retaining last-known plan)',
+    { ...ctx, stripeSubscriptionId: subscription?.id, priceId, retainedPlan, hadKnownPlan },
+  );
+  try {
+    billingEvents.emit('billing.webhook.plan_unresolved', {
+      ...ctx,
+      stripeSubscriptionId: subscription?.id,
+      priceId,
+      retainedPlan,
+      hadKnownPlan,
+    });
+  } catch (evtErr) {
+    logger.error('[billing.webhook] billing.webhook.plan_unresolved listener error (non-fatal)', {
+      error: evtErr?.message ?? String(evtErr),
+      stack: evtErr?.stack,
+    });
+  }
+  return retainedPlan;
+};
 
 /**
  * @description Sync the organization plan field to match the subscription plan.
@@ -369,7 +407,10 @@ const handleSubscriptionUpdated = async (subscription, event) => {
   const existing = await SubscriptionRepository.findByStripeSubscriptionId(subscription.id);
   if (!existing) return;
 
-  const newPlan = resolvePlan(subscription);
+  // Hoisted so both resolvePlan's alert context and the rest of the handler share one value.
+  const organizationId = String(existing.organization?._id || existing.organization);
+
+  const newPlan = resolvePlan(subscription, existing.plan, { organizationId, eventId: event?.id });
   // Stripe API ≥ 2025-08-27 moved current_period_start/end to items.data[0].
   // Read from items first, fall back to top-level for older API versions.
   const rawPeriodStart = subscription.items?.data?.[0]?.current_period_start ?? subscription.current_period_start;
@@ -407,7 +448,6 @@ const handleSubscriptionUpdated = async (subscription, event) => {
     return;
   }
 
-  const organizationId = String(existing.organization?._id || existing.organization);
   await syncOrganizationPlan(organizationId, effectivePlan);
 
   if (isIncompleteExpired) {
@@ -633,13 +673,18 @@ const handleInvoicePaymentSucceeded = async (invoice, event) => {
   const isPastDue = existing.pastDueSince !== null && existing.pastDueSince !== undefined;
   const fields = isPastDue ? { pastDueSince: null, status: 'active' } : {};
 
+  // Hoisted so both resolvePlan's alert context and the rest of the handler share one value.
+  const organizationId = String(existing.organization?._id || existing.organization);
+
   let resolvedPlan = null;
   if (isPastDue) {
     try {
       const stripe = getStripe();
       if (!stripe) throw new Error('Stripe not configured');
       const stripeSub = await stripe.subscriptions.retrieve(stripeSubscriptionId);
-      resolvedPlan = resolvePlan(stripeSub);
+      // Restore path: an unresolvable price here must NOT re-write 'free' over a dunning-downgraded
+      // sub — retain existing.plan (the last-known plan) and alert instead (#3970).
+      resolvedPlan = resolvePlan(stripeSub, existing.plan, { organizationId, stripeSubscriptionId, eventId: event?.id });
       fields.plan = resolvedPlan;
     } catch (stripeErr) {
       logger.warn('[billing.webhook] invoice.payment_succeeded — Stripe re-fetch failed, plan not restored', {
@@ -649,8 +694,6 @@ const handleInvoicePaymentSucceeded = async (invoice, event) => {
     }
   }
 
-  const organizationId = String(existing.organization?._id || existing.organization);
-
   const updated = await SubscriptionRepository.updateIfEventNewer(
     String(existing._id),
     event.created,
@@ -1120,7 +1163,6 @@ const handleChargeDisputeFundsWithdrawn = async (dispute, event) => {
  */
 // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik
 const handleSubscriptionCreated = async (subscription, event) => {
-  const newPlan = resolvePlan(subscription);
   const rawPeriodStart = subscription.items?.data?.[0]?.current_period_start ?? subscription.current_period_start;
   const newPeriodStart = rawPeriodStart ? new Date(rawPeriodStart * 1000) : undefined;
 
@@ -1145,6 +1187,10 @@ const handleSubscriptionCreated = async (subscription, event) => {
     }
 
     const organizationId = String(orgSub.organization?._id || orgSub.organization);
+    // orgSub was found via stripeCustomerId, so the org already carries a plan (e.g. a Dashboard
+    // second-subscription edge case) — retain it on an unresolvable price rather than defaulting
+    // to 'free', since syncOrganizationPlan below writes the shared org-level plan field (#3970).
+    const newPlan = resolvePlan(subscription, orgSub.plan, { organizationId, eventId: event?.id });
     logger.info('[billing.webhook] subscription.created — creating DB row for Dashboard/Payment Link subscription', {
       stripeSubscriptionId: subscription.id,
       organizationId,
@@ -1179,6 +1225,9 @@ const handleSubscriptionCreated = async (subscription, event) => {
     return;
   }
 
+  const organizationId = String(existing.organization?._id || existing.organization);
+  const newPlan = resolvePlan(subscription, existing.plan, { organizationId, eventId: event?.id });
+
   const fields = {
     plan: newPlan,
     status: subscription.status,
@@ -1197,7 +1246,6 @@ const handleSubscriptionCreated = async (subscription, event) => {
     return;
   }
 
-  const organizationId = String(existing.organization?._id || existing.organization);
   await syncOrganizationPlan(organizationId, newPlan);
 };
 

modules/billing/tests/billing.init.ops-listeners.unit.tests.js

@@ -11,6 +11,7 @@ import { jest, describe, test, afterEach, expect } from '@jest/globals';
  *   - billing.dispute.lost    (priority 5)
  *   - billing.refund.unresolved (priority 4)
  *   - billing.reconciliation.divergence (priority 4)
+ *   - billing.webhook.plan_unresolved (priority 4)
  *   - error event listener on billingEvents singleton
  *
  * Each listener must:
@@ -90,6 +91,7 @@ describe('billing.init ops-listeners unit tests:', () => {
       realBillingEvents.removeAllListeners('billing.dispute.lost');
       realBillingEvents.removeAllListeners('billing.refund.unresolved');
       realBillingEvents.removeAllListeners('billing.reconciliation.divergence');
+      realBillingEvents.removeAllListeners('billing.webhook.plan_unresolved');
       realBillingEvents.removeAllListeners('error');
     }
     jest.restoreAllMocks();
@@ -249,6 +251,55 @@ describe('billing.init ops-listeners unit tests:', () => {
     });
   });
 
+  // ---------------------------------------------------------------------------
+  // billing.webhook.plan_unresolved
+  // ---------------------------------------------------------------------------
+  describe('billing.webhook.plan_unresolved listener:', () => {
+    test('fires logger.error with full payload spread + ntfyPriority 4', async () => {
+      await setup();
+
+      const payload = {
+        organizationId: '507f1f77bcf86cd799439055',
+        eventId: 'evt_plan_unresolved',
+        stripeSubscriptionId: 'sub_live_bbb',
+        priceId: 'price_unmapped',
+        retainedPlan: 'growth',
+        hadKnownPlan: true,
+      };
+
+      realBillingEvents.emit('billing.webhook.plan_unresolved', payload);
+      await new Promise((resolve) => setImmediate(resolve));
+
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        '[billing.init] ALERT: webhook plan unresolved — manual review required',
+        expect.objectContaining({
+          organizationId: '507f1f77bcf86cd799439055',
+          eventId: 'evt_plan_unresolved',
+          stripeSubscriptionId: 'sub_live_bbb',
+          priceId: 'price_unmapped',
+          retainedPlan: 'growth',
+          hadKnownPlan: true,
+          ntfyPriority: 4,
+        }),
+      );
+    });
+
+    test('plan_unresolved: emitting the event does not throw synchronously', async () => {
+      await setup();
+
+      const payload = {
+        organizationId: 'org',
+        eventId: 'evt_ok',
+        stripeSubscriptionId: 'sub_ok',
+        priceId: 'price_ok',
+        retainedPlan: 'free',
+        hadKnownPlan: false,
+      };
+      expect(() => realBillingEvents.emit('billing.webhook.plan_unresolved', payload)).not.toThrow();
+      await new Promise((resolve) => setImmediate(resolve));
+    });
+  });
+
   // ---------------------------------------------------------------------------
   // Multiple listeners all fire (sanity)
   // ---------------------------------------------------------------------------
@@ -259,11 +310,12 @@ describe('billing.init ops-listeners unit tests:', () => {
     realBillingEvents.emit('billing.dispute.lost', { disputeId: 'dp_b', chargeId: 'ch_b', organizationId: 'o', stripeSessionId: 'cs', amount: 200 });
     realBillingEvents.emit('billing.refund.unresolved', { chargeId: 'ch_c' });
     realBillingEvents.emit('billing.reconciliation.divergence', { organizationId: 'o', subscriptionId: 's', stripeSubscriptionId: 'sub_x', db: {}, stripe: {}, statusMismatch: true, planMismatch: false });
+    realBillingEvents.emit('billing.webhook.plan_unresolved', { organizationId: 'o', eventId: 'evt_d', stripeSubscriptionId: 'sub_y', priceId: 'price_y', retainedPlan: 'free', hadKnownPlan: false });
 
     await new Promise((resolve) => setImmediate(resolve));
 
-    // 4 listeners, 1 logger.error call each = 4 total
-    expect(mockLogger.error).toHaveBeenCalledTimes(4);
+    // 5 listeners, 1 logger.error call each = 5 total
+    expect(mockLogger.error).toHaveBeenCalledTimes(5);
   });
 
   // ---------------------------------------------------------------------------

modules/billing/tests/billing.webhook.integration.tests.js

@@ -297,9 +297,11 @@ describe('Billing webhook integration tests:', () => {
       expect(mockOrganizationRepository.setPlan).not.toHaveBeenCalled();
     });
 
-    test('should fall back to free when plan metadata is invalid', async () => {
-      const existing = { _id: subId, organization: orgId };
+    test('#3970: retains current plan (does NOT force free) when plan metadata is invalid', async () => {
+      // Existing paid org — an unrecognized metadata.planId is unresolvable, must not downgrade.
+      const existing = { _id: subId, organization: orgId, plan: 'starter' };
       mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing);
+      const { default: billingEvents } = await import('../lib/events.js');
 
       await WebhookService.handleSubscriptionUpdated(
         {
@@ -316,9 +318,14 @@ describe('Billing webhook integration tests:', () => {
         subId,
         expect.any(Number),
         expect.any(String),
-        expect.objectContaining({ plan: 'free' }),
+        expect.objectContaining({ plan: 'starter' }),
         'subscription',
       );
+      expect(mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'starter');
+      expect(billingEvents.emit).toHaveBeenCalledWith(
+        'billing.webhook.plan_unresolved',
+        expect.objectContaining({ organizationId: orgId, retainedPlan: 'starter', hadKnownPlan: true }),
+      );
     });
   });