fix(billing): cumulative review — refund correlation + cleanup (#3543)

* fix(billing): apply cumulative review — refund correlation + cleanup

CRITICAL: backfill PaymentIntent metadata with real cs_* session ID after
creditPack succeeds in handleCheckoutPaymentCompleted, so charge.refunded
events can correlate the charge back to the correct ledger entry (the
__pending__ placeholder was breaking refundPartial lookups). Add 3 unit
tests covering PI update, absent PI, and non-fatal failure path.

HIGH: replace stale TODO PR-N3 in refundPartial with accurate comment —
heuristic is still intentionally needed since refundPartial does not
receive packId from the webhook call-site.

HIGH: add Mixed type caveat JSDoc to billing.usage.model (meterBreakdown
and counters fields) matching the existing note in billing.extraBalance.model.

MEDIUM: document BillingExtraBalance.organization vs BillingUsage.organizationId
asymmetry in billing.controller.js.

MEDIUM: add startup priceUsd validation warning in billing.init.js so
misconfigured packs are caught before a refund event exposes the gap.

* fix(billing): correct organization field type comment (both ObjectId)

Author: PierreBrisorgueil

modules/billing/billing.init.js

@@ -1,6 +1,7 @@
 /**
  * Module dependencies
  */
+import config from '../../config/index.js';
 import AnalyticsService from '../../lib/services/analytics.js';
 import billingEvents from './lib/events.js';
 
@@ -13,6 +14,15 @@ import billingEvents from './lib/events.js';
  */
 // eslint-disable-next-line no-unused-vars
 export default async (app) => {
+  // Warn at startup if any pack is missing a valid priceUsd — refundPartial fallback will be inaccurate
+  if (config.billing?.packs?.length) {
+    for (const pack of config.billing.packs) {
+      if (typeof pack.priceUsd !== 'number' || pack.priceUsd <= 0) {
+        console.warn(`[billing] pack '${pack.packId}' missing valid priceUsd; refundPartial fallback will be inaccurate`);
+      }
+    }
+  }
+
   // Update analytics group properties when a subscription plan changes
   billingEvents.on('plan.changed', ({ organizationId, newPlan }) => {
     try {

modules/billing/controllers/billing.controller.js

@@ -9,6 +9,9 @@ import BillingUsageService from '../services/billing.usage.service.js';
 import BillingExtraService from '../services/billing.extra.service.js';
 import BillingExtraBalanceRepository from '../repositories/billing.extraBalance.repository.js';
 
+// NOTE: BillingExtraBalance uses field name 'organization', BillingUsage uses 'organizationId' — both are Schema.ObjectId refs to Organization.
+// Only the field name differs (historical reasons) — keep queries consistent with each model's own convention.
+
 /**
  * @desc Endpoint to create a Stripe Checkout session
  * @param {Object} req - Express request object

modules/billing/models/billing.usage.model.mongoose.js

@@ -12,6 +12,12 @@ const Schema = mongoose.Schema;
  * Meter fields (weekKey, meterUsed, etc.) are sparse/optional — only
  * populated when config.billing.meterMode is true. This ensures full
  * backward compatibility for non-meter downstream projects.
+ *
+ * NOTE — Mixed type caveats (applies to 'counters' and 'meterBreakdown' fields):
+ *   Mongoose validators are NOT executed for in-place mutations on Mixed fields
+ *   (doc.field.x = y; doc.save() silently skips validators).
+ *   Always use atomic MongoDB operators ($inc, $set via findOneAndUpdate)
+ *   or Model.create() which runs validators on the full document.
  */
 const UsageMongoose = new Schema(
   {

modules/billing/services/billing.extra.service.js

@@ -87,10 +87,12 @@ const refundPartial = async (orgId, stripeSessionId, amountRefundedCents) => {
     return { doc, applied: false, refundUnits: 0 };
   }
 
-  // Find the pack config to compute proportion.
+  // Find the pack config to compute proportion using the topup entry's meterUnits.
   // Ambiguity guard: if 0 or >1 packs share the same meterUnits, fall back to
   // applied=false rather than using a wrong priceUsd.
-  // TODO PR-N3: webhook handler will pass packId from session metadata, removing the need for this heuristic.
+  // NOTE: packId is not passed to refundPartial — charge.refunded carries it in
+  // charge.metadata but the webhook call-site only passes (orgId, stripeSessionId, amountCents).
+  // This heuristic is therefore intentionally retained; the ambiguity guard is the safety net.
   const packs = config?.billing?.packs ?? [];
   const matchingPacks = packs.filter(
     (p) => (p.meterUnits ?? p.computeUnits) === topupEntry.amount,

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

@@ -4,6 +4,7 @@
 import mongoose from 'mongoose';
 
 import config from '../../../config/index.js';
+import getStripe from '../lib/stripe.js';
 import SubscriptionRepository from '../repositories/billing.subscription.repository.js';
 import ProcessedStripeEventRepository from '../repositories/billing.processedStripeEvent.repository.js';
 import OrganizationRepository from '../../organizations/repositories/organizations.repository.js';
@@ -152,14 +153,38 @@ const handleCheckoutCompleted = async (session) => {
 const handleCheckoutPaymentCompleted = async (session) => {
   if (session.payment_status !== 'paid') return;
 
-  const { metadata, id: stripeSessionId } = session;
+  const { metadata, id: stripeSessionId, payment_intent: paymentIntentId } = session;
   const { organizationId, packId, kind } = metadata ?? {};
 
   if (kind !== 'extras') return;
   if (!organizationId || !mongoose.Types.ObjectId.isValid(organizationId)) return;
   if (!packId) return;
 
   await BillingExtraService.creditPack(organizationId, packId, stripeSessionId);
+
+  // Backfill PaymentIntent metadata with the real session ID so that charge.refunded
+  // events can correlate the charge back to this ledger entry.
+  // At session.create time stripeSessionId was set to '__pending__' (Stripe forbids
+  // self-reference). Propagating the real cs_* ID here ensures charge.metadata carries
+  // it when a refund is issued later.
+  if (paymentIntentId) {
+    const stripe = getStripe();
+    if (stripe) {
+      try {
+        await stripe.paymentIntents.update(paymentIntentId, {
+          metadata: {
+            organizationId,
+            packId,
+            kind: 'extras',
+            stripeSessionId,  // real cs_* ID
+          },
+        });
+      } catch (err) {
+        // Log but don't fail — refund correlation may need fallback path
+        console.warn('[billing.webhook] PaymentIntent metadata update failed:', err.message);
+      }
+    }
+  }
 };
 
 /**

modules/billing/tests/billing.webhook.checkout.unit.tests.js

@@ -14,6 +14,7 @@ describe('Billing webhook checkout unit tests:', () => {
   let mockSubscriptionRepository;
   let mockOrganizationRepository;
   let mockExtraService;
+  let mockStripeInstance;
 
   const orgId = '507f1f77bcf86cd799439011';
   const subId = '607f1f77bcf86cd799439022';
@@ -39,6 +40,12 @@ describe('Billing webhook checkout unit tests:', () => {
       refundPartial: jest.fn(),
     };
 
+    mockStripeInstance = {
+      paymentIntents: {
+        update: jest.fn().mockResolvedValue({}),
+      },
+    };
+
     jest.unstable_mockModule('../repositories/billing.subscription.repository.js', () => ({
       default: mockSubscriptionRepository,
     }));
@@ -66,6 +73,10 @@ describe('Billing webhook checkout unit tests:', () => {
       default: { emit: jest.fn() },
     }));
 
+    jest.unstable_mockModule('../lib/stripe.js', () => ({
+      default: jest.fn(() => mockStripeInstance),
+    }));
+
     jest.unstable_mockModule('../../../config/index.js', () => ({
       default: {
         billing: { plans: ['free', 'starter', 'pro', 'enterprise'] },
@@ -220,6 +231,59 @@ describe('Billing webhook checkout unit tests:', () => {
 
       expect(mockExtraService.creditPack).not.toHaveBeenCalled();
     });
+
+    test('should call stripe.paymentIntents.update with real sessionId after creditPack succeeds (CRITICAL: refund correlation)', async () => {
+      const paymentIntentId = 'pi_test_abc123';
+
+      await BillingWebhookService.handleCheckoutPaymentCompleted({
+        id: stripeSessionId,
+        payment_status: 'paid',
+        payment_intent: paymentIntentId,
+        metadata: { organizationId: orgId, packId: 'pack_500k', kind: 'extras' },
+      });
+
+      expect(mockExtraService.creditPack).toHaveBeenCalledWith(orgId, 'pack_500k', stripeSessionId);
+      expect(mockStripeInstance.paymentIntents.update).toHaveBeenCalledWith(
+        paymentIntentId,
+        {
+          metadata: {
+            organizationId: orgId,
+            packId: 'pack_500k',
+            kind: 'extras',
+            stripeSessionId,  // real cs_* ID (not '__pending__')
+          },
+        },
+      );
+    });
+
+    test('should skip paymentIntents.update when payment_intent is absent', async () => {
+      await BillingWebhookService.handleCheckoutPaymentCompleted({
+        id: stripeSessionId,
+        payment_status: 'paid',
+        // payment_intent omitted — e.g. in test fixtures without PI
+        metadata: { organizationId: orgId, packId: 'pack_500k', kind: 'extras' },
+      });
+
+      expect(mockExtraService.creditPack).toHaveBeenCalled();
+      expect(mockStripeInstance.paymentIntents.update).not.toHaveBeenCalled();
+    });
+
+    test('should not throw when paymentIntents.update fails (non-fatal fallback)', async () => {
+      const paymentIntentId = 'pi_test_failing';
+      mockStripeInstance.paymentIntents.update.mockRejectedValue(new Error('Stripe API error'));
+
+      await expect(
+        BillingWebhookService.handleCheckoutPaymentCompleted({
+          id: stripeSessionId,
+          payment_status: 'paid',
+          payment_intent: paymentIntentId,
+          metadata: { organizationId: orgId, packId: 'pack_500k', kind: 'extras' },
+        }),
+      ).resolves.toBeUndefined();
+
+      // creditPack should still have run despite the PI update failure
+      expect(mockExtraService.creditPack).toHaveBeenCalledWith(orgId, 'pack_500k', stripeSessionId);
+    });
   });
 
   describe('handleCheckoutCompleted (mode=subscription)', () => {