feat(billing): hygiene polish — stack trace, req.id, $slice ledger, TTL archived, cache utils#3614
Merged
Merged
Conversation
…TL archived, cache utils Étape 5 — Polish (devkit-only): A — lastError + req.id correlation - incrementAttempts stores err.stack (not just message); JSON fallback for plain-object throws - withIdempotency accepts optional ctx.requestId; logs entry/skip/retry/dead-letter - Webhook controller reads req.id and threads it into all withIdempotency calls B — Performance hygiene - listLedger: new listLedgerPage() repo method uses $slice aggregation — only the page is transferred over the wire; perf integration test asserts < 50ms on 1000 entries - BillingUsage: TTL index on archivedAt (1-year, partialFilterExpression $type date) so archived weekly periods are auto-purged without touching active docs C — Tier 3 nits - Removed module-scope METER_RUN_BASE const (stale at load time); getMeterRunBase() function kept and re-exported from billing.meter.service.js - Exported clearPlansCache() from billing.plans.service.js for test isolation - fetchPlansFromStripe: explicit warn log when Stripe price ID mapped to multiple plans
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3614 +/- ##
==========================================
+ Coverage 88.17% 88.34% +0.16%
==========================================
Files 134 134
Lines 4432 4462 +30
Branches 1366 1380 +14
==========================================
+ Hits 3908 3942 +34
+ Misses 411 408 -3
+ Partials 113 112 -1
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR applies a “production hygiene” polish to billing by improving webhook observability (stack traces + requestId correlation), reducing ledger list payloads via server-side slicing, and making a few test/ops-oriented refinements (TTL purge for archived usage, cache reset utilities, and config resolution lazily).
Changes:
- Webhook idempotency: persist full error stack traces, add requestId correlation, and update controller + tests accordingly.
- Ledger pagination: introduce
listLedgerPage()using Mongo aggregation$sortArray+$slice, update service + add a perf integration test. - Billing maintenance utilities: TTL for archived usage docs, export
clearPlansCache(), swap stale module-scope meter constant for lazygetMeterRunBase().
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/billing/services/billing.webhook.service.js | Adds requestId-aware logging and passes full Error objects into attempt tracking. |
| modules/billing/controllers/billing.webhook.controller.js | Propagates req.id into webhook processing and standardizes log payloads. |
| modules/billing/repositories/billing.processedStripeEvent.repository.js | Stores stack traces / robust error serialization in incrementAttempts. |
| modules/billing/repositories/billing.extraBalance.repository.js | Adds listLedgerPage() aggregation to fetch only a ledger slice. |
| modules/billing/services/billing.extra.service.js | Switches listLedger to repository pagination with safe page/limit handling. |
| modules/billing/services/billing.plans.service.js | Adds duplicate priceId warning + exports clearPlansCache() for test isolation. |
| modules/billing/models/billing.usage.model.mongoose.js | Adds TTL index to purge archived usage docs after 1 year. |
| modules/billing/lib/billing.constants.js | Removes eager METER_RUN_BASE constant; keeps lazy getMeterRunBase(). |
| modules/billing/services/billing.meter.service.js | Stops exporting constant and re-exports getMeterRunBase on the facade. |
| modules/billing/tests/billing.webhook.idempotency.unit.tests.js | Updates assertions for Error-object attempt tracking + requestId ctx acceptance. |
| modules/billing/tests/billing.webhook.hardening.unit.tests.js | Updates attempt tracking assertion to expect Error objects. |
| modules/billing/tests/billing.processedStripeEvent.repository.unit.tests.js | Adds coverage for stack trace capture and serialization fallbacks. |
| modules/billing/tests/billing.extra.service.unit.tests.js | Updates tests to reflect repository-level pagination API. |
| modules/billing/tests/billing.extraBalance.listLedger.perf.integration.tests.js | Adds a perf gate test for slice-based ledger pagination. |
| modules/billing/tests/billing.plans.unit.tests.js | Adds tests for clearPlansCache() and duplicate priceId warning. |
| modules/billing/tests/billing.meter.service.unit.tests.js | Updates tests to validate new getMeterRunBase export. |
| modules/billing/tests/billing.controller.unit.tests.js | Updates expectations for the new withIdempotency(..., ctx) signature and log shape. |
Comment on lines
+288
to
+305
| $project: { | ||
| _id: 0, | ||
| cachedBalance: 1, | ||
| total: { $size: '$ledger' }, | ||
| // Sort descending by `at` then slice the requested page | ||
| ledgerPage: { | ||
| $slice: [ | ||
| { | ||
| $sortArray: { | ||
| input: '$ledger', | ||
| sortBy: { at: -1 }, | ||
| }, | ||
| }, | ||
| skip, | ||
| limit, | ||
| ], | ||
| }, | ||
| }, |
Comment on lines
+53
to
+68
| const start = performance.now(); | ||
| const result = await BillingExtraBalanceRepository.listLedgerPage(String(orgId), 0, 20); | ||
| const elapsed = performance.now() - start; | ||
|
|
||
| expect(result).not.toBeNull(); | ||
| expect(result.total).toBe(1000); | ||
| expect(result.ledgerPage).toHaveLength(20); | ||
| expect(result.cachedBalance).toBe(1000000); | ||
|
|
||
| // Entries should be sorted descending by `at` (newest first) | ||
| const firstAt = new Date(result.ledgerPage[0].at).getTime(); | ||
| const secondAt = new Date(result.ledgerPage[1].at).getTime(); | ||
| expect(firstAt).toBeGreaterThanOrEqual(secondAt); | ||
|
|
||
| // Performance gate: < 50ms for a 1000-entry ledger page fetch | ||
| expect(elapsed).toBeLessThan(50); |
|
|
||
| expect(mockBillingWebhookService.handleSubscriptionUpdated).toHaveBeenCalledWith({ id: 'sub_123' }, eventData); | ||
| expect(mockBillingWebhookService.withIdempotency).toHaveBeenCalledWith(eventData, expect.any(Function)); | ||
| expect(mockBillingWebhookService.withIdempotency).toHaveBeenCalledWith(eventData, expect.any(Function), expect.objectContaining({})); |
| @@ -195,10 +195,11 @@ describe('Billing webhook idempotency unit tests:', () => { | |||
| BillingWebhookService.withIdempotency(event, handler), | |||
| ).rejects.toThrow('handler blew up'); | |||
|
|
|||
| @@ -404,7 +404,11 @@ describe('withIdempotency — replay-storm dead-letter protection:', () => { | |||
|
|
|||
| await expect(BillingWebhookService.withIdempotency(event, handler)).rejects.toThrow('transient'); | |||
|
|
|||
Comment on lines
25
to
32
| /** | ||
| * @function getMeterRunBase | ||
| * @description Resolve the configured base units charged when no costs are present. | ||
| * Evaluated lazily on each call so test overrides and runtime config | ||
| * changes are always reflected (avoids the stale module-scope const pattern). | ||
| * @returns {number} Meter run base units. | ||
| */ | ||
| export const getMeterRunBase = () => |
…ield - Add $ifNull(['$ledger', []]) guard in $size and $sortArray expressions so listLedgerPage is robust against legacy/partial docs with missing ledger - Add toHaveBeenCalledTimes(1) assertions before mock.calls[0] access in webhook idempotency + hardening tests for clearer failure diagnostics - Add dedicated controller test asserting req.id is propagated as requestId into withIdempotency third arg
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Étape 5 — final polish layer for billing prod-ready (devkit Node only). Builds on Étapes 1–4 (#3611 #3612 #3613).
A — Dead-letter observability + req.id correlation
incrementAttemptsnow storeserr.stack(full trace) rather than justerr.message; JSON-serializes plain-object throws to avoid[object Object]withIdempotencyaccepts optionalctx.requestId; logs handler entry / skip / retry / dead-letter with the request ID for end-to-end traceabilityreq.id(injected by globalrequestIdmiddleware) and propagates it to allwithIdempotencycallsB — Performance hygiene
listLedgerrefactored to use newlistLedgerPage()repo method — MongoDB aggregation$sortArray+$slicetransfers only the requested page over the wire instead of the full document< 50mson a 1000-entry ledger page fetchBillingUsage: TTL index onarchivedAt(expireAfterSeconds: 365 days,partialFilterExpression: { archivedAt: { $type: 'date' } }) — auto-purge archived weekly periods after 1 year; active docs (archivedAt null/absent) are excludedC — Tier-3 nits
METER_RUN_BASEconst (evaluated stale at load time);getMeterRunBase()function kept and re-exported frombilling.meter.service.jsfor lazy evaluationclearPlansCache()frombilling.plans.service.jsfor test isolation (resets all module-scope cache state between test files)fetchPlansFromStripe: explicitwarnlog when a Stripe price ID is mapped to multiple plans — was previously silent (config error causing billing misrouting)Pre-push review
DeepSeek critical review verdict: OK with nits — 0 critical, 0 high. Two nits addressed:
$exists: truefrom TTLpartialFilterExpression(kept$type: 'date'which is sufficient)lastErrorserialization for non-Error object throws (JSON.stringify fallback)Test plan
npm run test:unit)billing.extraBalance.listLedger.perf.integration.tests.js— asserts< 50mson 1000-entry ledgerincrementAttemptsclearPlansCacheexport + duplicate price warn assertions