feat(billing): crons + dunning grace 7d (PR-N5)#3539
feat(billing): crons + dunning grace 7d (PR-N5)#3539PierreBrisorgueil merged 3 commits intomasterfrom
Conversation
Add 3 standalone k8s CronJob scripts (weeklyReset, extrasExpiration, dunningSweep), 7-day grace period in requireQuota middleware for past_due subs, pastDueSince stamping on invoice.payment_failed (idempotent), payment.failed event, and new repo methods (findStaleDunning, markUnpaid, findOrgsWithExpiringTopups). All paths gate on meterMode.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (21)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 32 minutes and 6 seconds.Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 3 high |
🟢 Metrics 139 complexity · 98 duplication
Metric Results Complexity 139 Duplication 98
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3539 +/- ##
==========================================
+ Coverage 87.68% 87.83% +0.15%
==========================================
Files 128 128
Lines 3540 3577 +37
Branches 1031 1048 +17
==========================================
+ Hits 3104 3142 +38
+ Misses 347 345 -2
- Partials 89 90 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
The pull request is currently not up to standards. The implementation introduces significant performance risks in high-traffic areas and potential memory exhaustion issues in repository queries. Specifically, the requireQuota middleware now executes a database lookup on every request, which will degrade system throughput, and the repository logic for expiring top-ups projects large arrays into memory.
Furthermore, while the dunning system is functional, the tiered logic (7 days grace, then 7 days blocked before downgrade) is not documented and could lead to maintenance confusion. Critical verification for the payment success webhook is missing from the test suite. Addressing the memory-intensive queries and ensuring atomic updates via a service layer are required to prevent data inconsistency and system instability.
About this PR
- The PR introduces significant boilerplate duplication across three new cron scripts. Identical environment setup, configuration gating, and connection logic increase the maintenance surface area and the risk of inconsistent behavior between crons.
Test suggestions
- requireQuota allows access and sets billingDegraded=true within 7 days of pastDueSince
- requireQuota returns 402 PAYMENT_PAST_DUE after 7 days of pastDueSince
- handleInvoicePaymentFailed sets pastDueSince only if it is currently null (idempotency)
- handleInvoicePaymentFailed emits the payment.failed event with organizationId
- handleInvoicePaymentSucceeded clears the pastDueSince marker and restores active status
- billing.dunningSweep.js transitions stale subscriptions to unpaid/free after 14 days
- findOrgsWithExpiringTopups filters for organizations with topups lacking expiration entries
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. handleInvoicePaymentSucceeded clears the pastDueSince marker and restores active status
Low confidence findings
- The dunning logic is split between a 7-day block in middleware and a 14-day downgrade in the cron. This tiered behavior (7 days grace, then 7 days blocked before downgrade) is not explicitly detailed in the PR documentation and might lead to future maintenance confusion.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| * NODE_ENV=production node scripts/crons/billing.dunningSweep.js | ||
| */ | ||
|
|
||
| process.env.NODE_ENV = process.env.NODE_ENV || 'development'; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The initialization logic in the new billing crons is highly repetitive. Extracting a shared cron wrapper or runner utility would centralize database connection management and configuration gating.
There was a problem hiding this comment.
By-design: each cron is an independent K8s CronJob entry point — sharing a wrapper would add indirection with no operational benefit. The boilerplate duplication is a Codacy static analysis finding; acceptable as-is.
| const orgId = req.organization._id.toString(); | ||
|
|
||
| // ── Degraded-mode gate (past_due grace period) ───────────────────── | ||
| const subscription = await SubscriptionRepository.findByOrganization(req.organization._id); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Executing a database lookup for the subscription on every request gated by this middleware can significantly impact throughput. Since req.organization is resolved upstream, consider pre-populating the subscription status there or using a local cache to avoid a per-request DB hit in this hot path.
There was a problem hiding this comment.
Noted for future optimization. The subscription lookup in meterMode is necessary for the grace-period gate (pastDueSince check). Caching at the resolveOrganization middleware level is tracked as a follow-up perf item.
| const docs = await BillingExtraBalance() | ||
| .find( |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The query for expired topups is imprecise and memory-intensive. Without using $elemMatch, the query may return incorrect documents. Furthermore, projecting the full ledger array for manual JS-side filtering will eventually cause heap out-of-memory errors as billing history grows.
Refactor this to use a MongoDB aggregation pipeline that filters the ledger array on the database side and returns only distinct organization IDs.
There was a problem hiding this comment.
Fixed in 17c2662: findOrgsWithExpiringTopups projects only ledger+organization (not full doc). Pre-filter behavior is intentional and documented in JSDoc — full aggregation pipeline deferred.
| const organizationId = String(existing.organization?._id || existing.organization); | ||
| try { | ||
| billingEvents.emit('payment.failed', { organizationId }); | ||
| } catch { /* listener errors must not disrupt webhook processing */ } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Avoid swallowing errors silently in the event emitter catch block. Logging the failure ensures that if downstream processes like notifications fail, there is a trace in the logs for troubleshooting.
There was a problem hiding this comment.
Fixed in 97e9662: both event emitter catch blocks (plan.changed and payment.failed) now log the error with console.error before swallowing, so failures are traceable in logs while webhook processing is not disrupted.
| await BillingSubscriptionRepository.markUnpaid(String(sub._id)); | ||
|
|
||
| // Sync Organization.plan to 'free' | ||
| const orgId = String(sub.organization); | ||
| if (mongoose.Types.ObjectId.isValid(orgId)) { | ||
| await Organization.findByIdAndUpdate(orgId, { plan: 'free' }, { runValidators: true }).exec(); | ||
| } | ||
|
|
||
| console.log(`[billing.dunningSweep] sub ${sub._id} → unpaid, org ${sub.organization} → free`); | ||
| processed += 1; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The manual update of both the subscription and the organization in this script is non-atomic and bypasses the service layer. If the organization update fails, the subscription remains 'unpaid' while the organization might still report a paid plan. This logic should be moved to a service method that ensures both updates occur together.
There was a problem hiding this comment.
Fixed in 97e9662: dunningSweep now uses OrganizationRepository.setPlan (atomic findByIdAndUpdate with runValidators) instead of direct DB access. Compensation path logs desyncErrors for manual reconciliation without reverting the Subscription state.
…nput guards
- Replace direct mongoose.model('Organization') in dunningSweep cron with
OrganizationRepository.setPlan — eliminates direct DB access from script layer
- Add OrganizationRepository.setPlan(orgId, plan): atomic findByIdAndUpdate with
ObjectId guard and runValidators
- Separate compensation path: if setPlan throws after markUnpaid succeeds, log
desyncErrors counter for manual reconciliation without reverting Subscription
- Add TypeError input guard on BillingSubscriptionRepository.findStaleDunning(threshold)
- Add TypeError input guard on BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now)
with JSDoc comment explaining pre-filter behavior
- Update README.md cron k8s image placeholder with clarifying comment
- 8 new unit tests (917 total, +8 from 909 baseline)
There was a problem hiding this comment.
Pull request overview
Adds operational billing automation for metered billing (guarded by config.billing.meterMode) by introducing Kubernetes-oriented cron entrypoints, dunning/grace-period behavior, and supporting repository/service changes across modules/billing/.
Changes:
- Added 3 standalone cron scripts (
weeklyReset,extrasExpiration,dunningSweep) plus k8s CronJob documentation. - Implemented dunning/grace handling: webhook stamps
pastDueSinceon first failure + emitspayment.failed;requireQuotaenforces a 7-day degraded-mode grace then blocks with 402. - Added new repository methods for dunning + expired topups discovery, with unit tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/crons/billing.weeklyReset.js | New CLI cron entrypoint for weekly reset sweep (meter mode only). |
| scripts/crons/billing.extrasExpiration.js | New CLI cron entrypoint for expiring extras topups (meter mode only). |
| scripts/crons/billing.dunningSweep.js | New CLI cron entrypoint to transition stale past_due to unpaid and sync org plan. |
| scripts/crons/README.md | Docs for running/deploying billing cron scripts via k8s CronJobs. |
| scripts/tests/billing.cron.weeklyReset.unit.tests.js | Unit tests around BillingResetService.resetAllDue() behavior/gating. |
| scripts/tests/billing.cron.extrasExpiration.unit.tests.js | Unit tests covering extras expiration sweep logic patterns. |
| scripts/tests/billing.cron.dunningSweep.unit.tests.js | Unit tests covering stale dunning queries + mark-unpaid behavior patterns. |
| modules/billing/services/billing.webhook.service.js | invoice.payment_failed now stamps pastDueSince (idempotent) + emits payment.failed. |
| modules/billing/middlewares/billing.requireQuota.js | Adds degraded-mode grace gate (7d) for past_due subscriptions in meter mode. |
| modules/billing/repositories/billing.subscription.repository.js | Adds findStaleDunning() + markUnpaid() repo methods. |
| modules/billing/repositories/billing.extraBalance.repository.js | Adds findOrgsWithExpiringTopups(now) helper for expiration sweeps. |
| modules/billing/lib/events.js | Documents new payment.failed billing event. |
| modules/billing/tests/billing.webhook.subscription.unit.tests.js | Adds coverage for pastDueSince idempotency + event emission. |
| modules/billing/tests/billing.service.unit.tests.js | Updates expectation to include pastDueSince on first failure. |
| modules/billing/tests/billing.quota.unit.tests.js | Adds test coverage for degraded-mode grace and 402 behavior. |
| modules/billing/tests/billing.subscription.repository.unit.tests.js | New unit tests for findStaleDunning / markUnpaid and a smoke test for reset query. |
| modules/billing/tests/billing.extraBalance.findOrgsWithExpiringTopups.unit.tests.js | New unit tests for findOrgsWithExpiringTopups. |
| await mongooseService.connect(); | ||
|
|
||
| try { | ||
| const { default: BillingResetService } = await import('../../modules/billing/services/billing.reset.service.js'); | ||
|
|
||
| const result = await BillingResetService.resetAllDue(); | ||
| console.log(`[billing.weeklyReset] done — processed: ${result.processed}, errors: ${result.errors}`); | ||
| process.exit(result.errors > 0 ? 1 : 0); | ||
| } catch (err) { | ||
| console.error('[billing.weeklyReset] fatal:', err); | ||
| process.exit(1); | ||
| } finally { | ||
| await mongooseService.disconnect?.(); | ||
| } |
There was a problem hiding this comment.
mongooseService.connect() runs before the try/catch, so connection failures won’t be reported via the script’s “fatal” handler and may skip cleanup logic. Consider moving connect (and loadModels) inside the try so errors are caught and the finally block can run.
| await mongooseService.connect(); | |
| try { | |
| const { default: BillingResetService } = await import('../../modules/billing/services/billing.reset.service.js'); | |
| const result = await BillingResetService.resetAllDue(); | |
| console.log(`[billing.weeklyReset] done — processed: ${result.processed}, errors: ${result.errors}`); | |
| process.exit(result.errors > 0 ? 1 : 0); | |
| } catch (err) { | |
| console.error('[billing.weeklyReset] fatal:', err); | |
| process.exit(1); | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| } | |
| let exitCode = 0; | |
| try { | |
| await mongooseService.connect(); | |
| const { default: BillingResetService } = await import('../../modules/billing/services/billing.reset.service.js'); | |
| const result = await BillingResetService.resetAllDue(); | |
| console.log(`[billing.weeklyReset] done — processed: ${result.processed}, errors: ${result.errors}`); | |
| exitCode = result.errors > 0 ? 1 : 0; | |
| } catch (err) { | |
| console.error('[billing.weeklyReset] fatal:', err); | |
| exitCode = 1; | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| } | |
| process.exit(exitCode); |
There was a problem hiding this comment.
Fixed in 97e9662: same pattern applied to billing.weeklyReset — connect moved inside try.
| process.exit(0); | ||
| } | ||
|
|
||
| await mongooseService.connect(); | ||
|
|
||
| try { | ||
| const { default: BillingResetService } = await import('../../modules/billing/services/billing.reset.service.js'); | ||
|
|
||
| const result = await BillingResetService.resetAllDue(); | ||
| console.log(`[billing.weeklyReset] done — processed: ${result.processed}, errors: ${result.errors}`); | ||
| process.exit(result.errors > 0 ? 1 : 0); | ||
| } catch (err) { | ||
| console.error('[billing.weeklyReset] fatal:', err); | ||
| process.exit(1); | ||
| } finally { | ||
| await mongooseService.disconnect?.(); |
There was a problem hiding this comment.
process.exit(...) inside the try/catch prevents the async finally block from reliably running, so mongooseService.disconnect() may be skipped. Prefer setting process.exitCode and returning/letting the script fall through, or explicitly disconnect before calling process.exit.
| process.exit(0); | |
| } | |
| await mongooseService.connect(); | |
| try { | |
| const { default: BillingResetService } = await import('../../modules/billing/services/billing.reset.service.js'); | |
| const result = await BillingResetService.resetAllDue(); | |
| console.log(`[billing.weeklyReset] done — processed: ${result.processed}, errors: ${result.errors}`); | |
| process.exit(result.errors > 0 ? 1 : 0); | |
| } catch (err) { | |
| console.error('[billing.weeklyReset] fatal:', err); | |
| process.exit(1); | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| process.exitCode = 0; | |
| } else { | |
| await mongooseService.connect(); | |
| try { | |
| const { default: BillingResetService } = await import('../../modules/billing/services/billing.reset.service.js'); | |
| const result = await BillingResetService.resetAllDue(); | |
| console.log(`[billing.weeklyReset] done — processed: ${result.processed}, errors: ${result.errors}`); | |
| process.exitCode = result.errors > 0 ? 1 : 0; | |
| } catch (err) { | |
| console.error('[billing.weeklyReset] fatal:', err); | |
| process.exitCode = 1; | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| } |
There was a problem hiding this comment.
Fixed in 97e9662: same pattern applied to billing.weeklyReset — process.exitCode used inside try, trailing process.exit after finally, connect moved inside try.
| await mongooseService.connect(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
This script connects before the try/catch, so a connection failure will bypass the “fatal” handler and skip the disconnect logic. Move connect (and loadModels, if added) inside the try so errors are handled consistently and the finally block can run.
| await mongooseService.connect(); | |
| try { | |
| try { | |
| await mongooseService.connect(); |
There was a problem hiding this comment.
Fixed in 97e9662: same pattern applied to billing.extrasExpiration — connect moved inside try.
| process.exit(0); | ||
| } | ||
|
|
||
| await mongooseService.connect(); | ||
|
|
||
| try { | ||
| const [{ default: BillingSubscriptionRepository }, { default: OrganizationRepository }] = await Promise.all([ | ||
| import('../../modules/billing/repositories/billing.subscription.repository.js'), | ||
| import('../../modules/organizations/repositories/organizations.repository.js'), | ||
| ]); | ||
|
|
||
| const now = new Date(); | ||
| const threshold = new Date(now.getTime() - 14 * 24 * 60 * 60 * 1000); | ||
|
|
||
| const staleSubs = await BillingSubscriptionRepository.findStaleDunning(threshold); | ||
| console.log(`[billing.dunningSweep] ${staleSubs.length} stale past_due subscription(s) found`); | ||
|
|
||
| let processed = 0; | ||
| let errors = 0; | ||
| let desyncErrors = 0; | ||
|
|
||
| for (const sub of staleSubs) { | ||
| try { | ||
| const subscription = await BillingSubscriptionRepository.markUnpaid(String(sub._id)); | ||
| if (!subscription) continue; // markUnpaid returns null on invalid id | ||
|
|
||
| try { | ||
| await OrganizationRepository.setPlan(String(sub.organization), 'free'); | ||
| } catch (orgErr) { | ||
| // Compensation: Subscription is now unpaid but Org.plan update failed. | ||
| // Log for manual reconciliation — do not revert Subscription status. | ||
| console.error('[billing.dunningSweep] Org plan sync failed (manual reconciliation required):', orgErr); | ||
| desyncErrors += 1; | ||
| } | ||
|
|
||
| console.log(`[billing.dunningSweep] sub ${sub._id} → unpaid, org ${sub.organization} → free`); | ||
| processed += 1; | ||
| } catch (err) { | ||
| errors += 1; | ||
| console.error(`[billing.dunningSweep] failed for sub ${sub._id}:`, err); | ||
| } | ||
| } | ||
|
|
||
| console.log(`[billing.dunningSweep] done — processed: ${processed}, errors: ${errors}, desyncErrors: ${desyncErrors}`); | ||
| process.exit(errors > 0 ? 1 : 0); | ||
| } catch (err) { | ||
| console.error('[billing.dunningSweep] fatal:', err); | ||
| process.exit(1); | ||
| } finally { | ||
| await mongooseService.disconnect?.(); |
There was a problem hiding this comment.
process.exit(...) is called before the async finally block can complete, so mongooseService.disconnect() may not run. Prefer process.exitCode + return, or explicitly disconnect before exit.
| process.exit(0); | |
| } | |
| await mongooseService.connect(); | |
| try { | |
| const [{ default: BillingSubscriptionRepository }, { default: OrganizationRepository }] = await Promise.all([ | |
| import('../../modules/billing/repositories/billing.subscription.repository.js'), | |
| import('../../modules/organizations/repositories/organizations.repository.js'), | |
| ]); | |
| const now = new Date(); | |
| const threshold = new Date(now.getTime() - 14 * 24 * 60 * 60 * 1000); | |
| const staleSubs = await BillingSubscriptionRepository.findStaleDunning(threshold); | |
| console.log(`[billing.dunningSweep] ${staleSubs.length} stale past_due subscription(s) found`); | |
| let processed = 0; | |
| let errors = 0; | |
| let desyncErrors = 0; | |
| for (const sub of staleSubs) { | |
| try { | |
| const subscription = await BillingSubscriptionRepository.markUnpaid(String(sub._id)); | |
| if (!subscription) continue; // markUnpaid returns null on invalid id | |
| try { | |
| await OrganizationRepository.setPlan(String(sub.organization), 'free'); | |
| } catch (orgErr) { | |
| // Compensation: Subscription is now unpaid but Org.plan update failed. | |
| // Log for manual reconciliation — do not revert Subscription status. | |
| console.error('[billing.dunningSweep] Org plan sync failed (manual reconciliation required):', orgErr); | |
| desyncErrors += 1; | |
| } | |
| console.log(`[billing.dunningSweep] sub ${sub._id} → unpaid, org ${sub.organization} → free`); | |
| processed += 1; | |
| } catch (err) { | |
| errors += 1; | |
| console.error(`[billing.dunningSweep] failed for sub ${sub._id}:`, err); | |
| } | |
| } | |
| console.log(`[billing.dunningSweep] done — processed: ${processed}, errors: ${errors}, desyncErrors: ${desyncErrors}`); | |
| process.exit(errors > 0 ? 1 : 0); | |
| } catch (err) { | |
| console.error('[billing.dunningSweep] fatal:', err); | |
| process.exit(1); | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| } else { | |
| await mongooseService.connect(); | |
| try { | |
| const [{ default: BillingSubscriptionRepository }, { default: OrganizationRepository }] = await Promise.all([ | |
| import('../../modules/billing/repositories/billing.subscription.repository.js'), | |
| import('../../modules/organizations/repositories/organizations.repository.js'), | |
| ]); | |
| const now = new Date(); | |
| const threshold = new Date(now.getTime() - 14 * 24 * 60 * 60 * 1000); | |
| const staleSubs = await BillingSubscriptionRepository.findStaleDunning(threshold); | |
| console.log(`[billing.dunningSweep] ${staleSubs.length} stale past_due subscription(s) found`); | |
| let processed = 0; | |
| let errors = 0; | |
| let desyncErrors = 0; | |
| for (const sub of staleSubs) { | |
| try { | |
| const subscription = await BillingSubscriptionRepository.markUnpaid(String(sub._id)); | |
| if (!subscription) continue; // markUnpaid returns null on invalid id | |
| try { | |
| await OrganizationRepository.setPlan(String(sub.organization), 'free'); | |
| } catch (orgErr) { | |
| // Compensation: Subscription is now unpaid but Org.plan update failed. | |
| // Log for manual reconciliation — do not revert Subscription status. | |
| console.error('[billing.dunningSweep] Org plan sync failed (manual reconciliation required):', orgErr); | |
| desyncErrors += 1; | |
| } | |
| console.log(`[billing.dunningSweep] sub ${sub._id} → unpaid, org ${sub.organization} → free`); | |
| processed += 1; | |
| } catch (err) { | |
| errors += 1; | |
| console.error(`[billing.dunningSweep] failed for sub ${sub._id}:`, err); | |
| } | |
| } | |
| console.log(`[billing.dunningSweep] done — processed: ${processed}, errors: ${errors}, desyncErrors: ${desyncErrors}`); | |
| process.exitCode = errors > 0 ? 1 : 0; | |
| } catch (err) { | |
| console.error('[billing.dunningSweep] fatal:', err); | |
| process.exitCode = 1; | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| } |
There was a problem hiding this comment.
Fixed in 97e9662: process.exit() inside try replaced with process.exitCode + trailing process.exit(process.exitCode ?? 0). mongooseService.connect() moved inside try so connection errors reach the fatal handler.
| const subscription = await SubscriptionRepository.findByOrganization(req.organization._id); | ||
| if (subscription?.status === 'past_due' && subscription.pastDueSince != null) { | ||
| const gracePeriodMs = 7 * 24 * 60 * 60 * 1000; | ||
| const elapsed = Date.now() - new Date(subscription.pastDueSince).getTime(); |
There was a problem hiding this comment.
In meterMode, this adds a per-request SubscriptionRepository.findByOrganization() call, but that repository method populates organization fields and returns a full Mongoose document. Since the degraded-mode gate only needs {status, pastDueSince}, consider adding a lean/projection repo method for this path to reduce query cost (and avoid populating unnecessarily).
There was a problem hiding this comment.
Noted. findByOrganization populates organization fields — future optimization: create a lean variant (findByOrganizationLean) returning only status+pastDueSince for the middleware path.
| try { | ||
| const [{ default: BillingExtraService }, { default: BillingExtraBalanceRepository }] = | ||
| await Promise.all([ | ||
| import('../../modules/billing/services/billing.extra.service.js'), | ||
| import('../../modules/billing/repositories/billing.extraBalance.repository.js'), | ||
| ]); |
There was a problem hiding this comment.
Like the other cron scripts, this relies on Mongoose models being registered, but it never calls mongooseService.loadModels(). Importing BillingExtraBalanceRepository / BillingExtraService will eventually call mongoose.model('BillingExtraBalance'), which will throw in a standalone CLI unless models were loaded first. Call loadModels() before importing billing modules.
There was a problem hiding this comment.
BillingExtraBalanceRepository and BillingExtraService register their models via import. No direct name-based mongoose.model() calls — loadModels() not required.
| process.exit(0); | ||
| } | ||
|
|
||
| await mongooseService.connect(); | ||
|
|
||
| try { | ||
| const [{ default: BillingExtraService }, { default: BillingExtraBalanceRepository }] = | ||
| await Promise.all([ | ||
| import('../../modules/billing/services/billing.extra.service.js'), | ||
| import('../../modules/billing/repositories/billing.extraBalance.repository.js'), | ||
| ]); | ||
|
|
||
| const now = new Date(); | ||
| const orgIds = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | ||
|
|
||
| let processed = 0; | ||
| let errors = 0; | ||
|
|
||
| for (const orgId of orgIds) { | ||
| try { | ||
| const added = await BillingExtraService.expireOldEntries(orgId); | ||
| console.log(`[billing.extrasExpiration] org ${orgId}: ${added} expiration entries added`); | ||
| processed += 1; | ||
| } catch (err) { | ||
| errors += 1; | ||
| console.error(`[billing.extrasExpiration] expireOldEntries failed for org ${orgId}:`, err); | ||
| } | ||
| } | ||
|
|
||
| console.log(`[billing.extrasExpiration] done — processed: ${processed}, errors: ${errors}`); | ||
| process.exit(errors > 0 ? 1 : 0); | ||
| } catch (err) { | ||
| console.error('[billing.extrasExpiration] fatal:', err); | ||
| process.exit(1); | ||
| } finally { | ||
| await mongooseService.disconnect?.(); |
There was a problem hiding this comment.
process.exit(...) is called before the finally block can complete, so mongooseService.disconnect() may not run. Prefer process.exitCode + return, or disconnect explicitly before exit, so the script shuts down cleanly.
| process.exit(0); | |
| } | |
| await mongooseService.connect(); | |
| try { | |
| const [{ default: BillingExtraService }, { default: BillingExtraBalanceRepository }] = | |
| await Promise.all([ | |
| import('../../modules/billing/services/billing.extra.service.js'), | |
| import('../../modules/billing/repositories/billing.extraBalance.repository.js'), | |
| ]); | |
| const now = new Date(); | |
| const orgIds = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | |
| let processed = 0; | |
| let errors = 0; | |
| for (const orgId of orgIds) { | |
| try { | |
| const added = await BillingExtraService.expireOldEntries(orgId); | |
| console.log(`[billing.extrasExpiration] org ${orgId}: ${added} expiration entries added`); | |
| processed += 1; | |
| } catch (err) { | |
| errors += 1; | |
| console.error(`[billing.extrasExpiration] expireOldEntries failed for org ${orgId}:`, err); | |
| } | |
| } | |
| console.log(`[billing.extrasExpiration] done — processed: ${processed}, errors: ${errors}`); | |
| process.exit(errors > 0 ? 1 : 0); | |
| } catch (err) { | |
| console.error('[billing.extrasExpiration] fatal:', err); | |
| process.exit(1); | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| process.exitCode = 0; | |
| } else { | |
| await mongooseService.connect(); | |
| try { | |
| const [{ default: BillingExtraService }, { default: BillingExtraBalanceRepository }] = | |
| await Promise.all([ | |
| import('../../modules/billing/services/billing.extra.service.js'), | |
| import('../../modules/billing/repositories/billing.extraBalance.repository.js'), | |
| ]); | |
| const now = new Date(); | |
| const orgIds = await BillingExtraBalanceRepository.findOrgsWithExpiringTopups(now); | |
| let processed = 0; | |
| let errors = 0; | |
| for (const orgId of orgIds) { | |
| try { | |
| const added = await BillingExtraService.expireOldEntries(orgId); | |
| console.log(`[billing.extrasExpiration] org ${orgId}: ${added} expiration entries added`); | |
| processed += 1; | |
| } catch (err) { | |
| errors += 1; | |
| console.error(`[billing.extrasExpiration] expireOldEntries failed for org ${orgId}:`, err); | |
| } | |
| } | |
| console.log(`[billing.extrasExpiration] done — processed: ${processed}, errors: ${errors}`); | |
| process.exitCode = errors > 0 ? 1 : 0; | |
| } catch (err) { | |
| console.error('[billing.extrasExpiration] fatal:', err); | |
| process.exitCode = 1; | |
| } finally { | |
| await mongooseService.disconnect?.(); | |
| } |
There was a problem hiding this comment.
Fixed in 97e9662: same pattern applied to billing.extrasExpiration — process.exitCode used inside try, trailing process.exit after finally, connect moved inside try.
| * Finds subscriptions in 'past_due' status whose pastDueSince is older than 14 days | ||
| * (i.e. the 7-day grace period has elapsed with no payment), transitions them to | ||
| * 'unpaid' + plan 'free', and syncs the Organization.plan field accordingly. |
There was a problem hiding this comment.
The comment says “pastDueSince is older than 14 days (i.e. the 7-day grace period has elapsed…)” which is internally inconsistent (14d != 7d). Clarify the intended policy in the comment (e.g., 7d request-level grace + 14d dunning sweep cutoff).
| * Finds subscriptions in 'past_due' status whose pastDueSince is older than 14 days | |
| * (i.e. the 7-day grace period has elapsed with no payment), transitions them to | |
| * 'unpaid' + plan 'free', and syncs the Organization.plan field accordingly. | |
| * Finds subscriptions in 'past_due' status whose pastDueSince is older than 14 days, | |
| * transitions them to 'unpaid' + plan 'free', and syncs the Organization.plan | |
| * field accordingly. |
There was a problem hiding this comment.
Fixed in 97e9662: doc comment now reads '14d = 7d grace + 7d blocked' and includes the full timeline (payment fails → pastDueSince set → 7d grace/degraded → 7d blocked/402 → day 14+ cron fires).
| await mongooseService.connect(); | ||
|
|
||
| try { | ||
| const [{ default: BillingSubscriptionRepository }, { default: OrganizationRepository }] = await Promise.all([ | ||
| import('../../modules/billing/repositories/billing.subscription.repository.js'), | ||
| import('../../modules/organizations/repositories/organizations.repository.js'), | ||
| ]); | ||
|
|
||
| const now = new Date(); | ||
| const threshold = new Date(now.getTime() - 14 * 24 * 60 * 60 * 1000); |
There was a problem hiding this comment.
This script imports BillingSubscriptionRepository and mongoose.model('Organization') but never calls mongooseService.loadModels(). In a standalone CronJob process, models are not registered unless loadModels() is called, so this will throw at runtime. Load models before importing repositories / calling mongoose.model(...).
There was a problem hiding this comment.
Resolved in 97e9662: the direct mongoose.model('Organization') access was removed — dunningSweep now imports OrganizationRepository which lazy-registers its model on import. loadModels() is only needed for name-based model lookups without importing the file; not required here.
| // Note: the MongoDB pre-filter `ledger.expiresAt: { $lt: now }` is a coarse pre-filter — | ||
| // some returned docs may have no unhandled expirations (already recorded expiration entries); | ||
| // the in-memory loop below performs the precise check. This is intentional for simplicity. | ||
| const docs = await BillingExtraBalance() | ||
| .find( | ||
| { | ||
| 'ledger.kind': 'topup', | ||
| 'ledger.expiresAt': { $lt: now }, |
There was a problem hiding this comment.
The Mongo filter on ledger uses separate predicates ('ledger.kind' and 'ledger.expiresAt') without $elemMatch, so MongoDB may match these conditions on different array elements. Using $elemMatch (kind='topup' AND expiresAt < now on the same entry) avoids false positives and is typically more index-friendly.
| // Note: the MongoDB pre-filter `ledger.expiresAt: { $lt: now }` is a coarse pre-filter — | |
| // some returned docs may have no unhandled expirations (already recorded expiration entries); | |
| // the in-memory loop below performs the precise check. This is intentional for simplicity. | |
| const docs = await BillingExtraBalance() | |
| .find( | |
| { | |
| 'ledger.kind': 'topup', | |
| 'ledger.expiresAt': { $lt: now }, | |
| // Use $elemMatch so `kind` and `expiresAt` are evaluated on the same ledger entry. | |
| // The in-memory loop below still performs the precise check for missing expiration entries. | |
| const docs = await BillingExtraBalance() | |
| .find( | |
| { | |
| ledger: { | |
| $elemMatch: { | |
| kind: 'topup', | |
| expiresAt: { $lt: now }, | |
| }, | |
| }, |
There was a problem hiding this comment.
Same as #3164066073 — the MongoDB pre-filter is coarse by design (documented in JSDoc). The JS-side loop performs the precise per-entry check. Ledger arrays are bounded per org in practice; aggregation pipeline deferred.
- billing.webhook.service: replace direct mongoose.model('Organization')
with OrganizationRepository.setPlan — keeps DB access in repo layer
- billing.webhook.service: add error logging to silent event emitter
catch blocks (plan.changed, payment.failed) for traceability
- billing.crons (dunningSweep/weeklyReset/extrasExpiration): move
mongooseService.connect inside try block so connection errors reach
the fatal handler; replace process.exit() in try with process.exitCode
+ trailing process.exit() so finally disconnect always runs
- billing.dunningSweep: clarify doc comment: 14d = 7d grace + 7d blocked
- tests: update 4 webhook test files to mock OrganizationRepository.setPlan
instead of mongoose.model('Organization').findByIdAndUpdate
* fix(test): eliminate Winston uncaughtException listener leak in jest test runs Each jest.resetModules() call in beforeEach re-imports lib/services/logger.js, creating a new Winston logger with handleExceptions:true. Winston registers a new process.uncaughtException listener on every import but never removes orphaned listeners from prior module registry generations. Across 1226 tests with resetModules in beforeEach, this leaked ~570 listeners and ~285-1100MB of heap. The billing PRs (#3535-#3539) amplified an existing pre-billing leak (162 instances) by +409 new instances (+252%) — pushing trawl_node past its 6144MB cap. Fix: guard handleExceptions with `process.env.NODE_ENV !== 'test'` on both the Console transport (logger creation) and the File transport (setupFileLogger / getLogOptions). Exception handling is preserved in production and development. Also add `forceExit: true` to jest.config.js to prevent Jest from hanging on open MongoDB handles from integration test afterAll disconnect races. Verified: MaxListenersExceededWarning fully eliminated. 99 suites / 1226 tests all pass. * docs(logger): replace volatile metrics with stable explanation in handleExceptions guard
Summary
scripts/crons/) for k8s CronJob orchestration — all gate onmeterMode === true(no-op when false):billing.weeklyReset.js— wrapsBillingResetService.resetAllDue()(~10 LOC)billing.extrasExpiration.js— sweeps expired topup ledger entries viaBillingExtraService.expireOldEntriesbilling.dunningSweep.js— transitionspast_duesubs older than 14d tounpaid+ syncsOrganization.plan = freeBillingExtraBalanceRepository.findOrgsWithExpiringTopups(now),BillingSubscriptionRepository.findStaleDunning(threshold),BillingSubscriptionRepository.markUnpaid(id)handleInvoicePaymentFailednow setspastDueSince = nowon first failure (idempotent — does not reset on retries) and emitsbillingEvents.emit('payment.failed', { organizationId })requireQuotadegraded-mode gate —past_due+pastDueSinceset: within 7d grace →res.locals.billingDegraded = true+ allow through; ≥7d → 402PAYMENT_PAST_DUEscripts/crons/README.mdwith k8s CronJob examples and scheduling recommendationsCloses
Part of Trawl Compute Pricing milestone — refs #3533 PR-N5
Test plan
npm run lint— greennpm run test:unit— 909 tests, 0 failuresmeterMode: false(default) → all cron scripts exit 0 immediatelybilling.requireQuotadegraded J+5 allows through withres.locals.billingDegraded = truebilling.requireQuotadegraded J+10 returns 402PAYMENT_PAST_DUEhandleInvoicePaymentFaileddoes not overwrite existingpastDueSinceon repeated calls