You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Bundles 2 lifecycle hardening findings from the post-recalibration billing audit:
🟠 B: forceRotateForPlanChange() — resetWeek() was no-op when the current ISO week doc existed, leaving stale quota/version snapshot on plan change (mid-week upgrades/downgrades broken until next weekly reset)
🟠 C: Outbox pattern for usage+extras atomicity — extras debit could be lost if it failed after usage idempotency was already claimed (revenue leak)
Changes
B — Plan-change forced rotation
BillingResetService.forceRotateForPlanChange(orgId, { preserveUsage }) updates current week snapshot quota/version mid-period
preserveUsage: true (default) keeps meterUsed (no double-charge / no refund)
🟠 B: BillingResetService.forceRotateForPlanChange() updates current week
snapshot mid-period when plan changes (was no-op when week doc existed,
keeping stale quota until next reset). Webhook plan.changed handler now
calls forceRotate with preserveUsage:true default. Consumers needing
clean-break downgrade pass {preserveUsage:false}.
🟠 C: Outbox pattern guarantees extras debit completes even if first attempt
fails. incrementMeterWithOutbox() inserts BillingMeterOutbox row alongside
usage increment; out-of-band cron retry-pending-extras-debit reconciles
within 5min. After 5 failed attempts, row marked 'failed' + billing.extras_debit.exhausted
event emitted for downstream alerting. unique index on idempotencyKey prevents
double-create races.
Documents both behaviors in modules/billing/README.md.
@PierreBrisorgueil has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 44 minutes and 37 seconds before requesting another review.
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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
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.
This PR implements an outbox-based extras debit retry system for billing meter overages and integrates plan-change snapshot rotation. When meter usage exceeds quota limits, attribute() now optimistically records usage and creates a pending outbox row, deferring debit reconciliation to a new 5-minute cron job that retries and eventually exhausts failed debits. A new forceRotateForPlanChange() method replaces resetWeek() for mid-cycle plan changes, optionally preserving or clearing existing meter usage.
Changes
Billing Meter Outbox Reliability & Plan Rotation
Layer / File(s)
Summary
Data Models & Schema modules/billing/models/billing.meter.outbox.model.mongoose.js, modules/billing/models/billing.meter.outbox.schema.js
Introduces BillingMeterOutbox Mongoose model and Zod schemas with fields for tracking pending extras debits: organizationId, idempotencyKey, extrasUnits, processing status (pending|committed|failed), and retry metadata (attempts, lastError, lastAttemptedAt). Compound index on (status, lastAttemptedAt) for efficient retry queries.
Adds outbox repository with CRUD operations: create(), findPendingDue() (time-windowed query), markCommitted(), and markFailedAttempt() (with automatic 5-attempt exhaustion). Updates usage repository with rotateWeekSnapshotForPlanChange() to update plan/quota snapshot fields and conditionally reset meter usage; incrementMeter() now accepts optional session parameter.
Service Core Logic modules/billing/services/billing.meter.outbox.service.js, modules/billing/services/billing.usage.service.js
New BillingMeterOutboxService implements retryPendingExtrasDebits() retry loop: fetches due rows, processes debits, updates outbox state, and emits billing.extras_debit.exhausted on terminal failures, returning scan/commit/failure/exhaustion counts. BillingUsageService adds incrementMeterWithOutbox() wrapper that creates outbox rows only when extras consumption occurs.
Meter Attribution & Plan Changes modules/billing/services/billing.meter.service.js, modules/billing/services/billing.reset.service.js
BillingMeterService.attribute() switches to optimistic-write pattern: creates outbox row before attempting debit, returns immediately after usage increment, and defers debit failures to the cron for reconciliation. BillingResetService adds forceRotateForPlanChange(orgId, options) to rotate active-week snapshot quota/version with optional preserveUsage flag.
New standalone Kubernetes CronJob entrypoint (retry-pending-extras-debit.cron.js) executes every ~5 minutes: loads config, applies jitter, calls retryPendingExtrasDebits(5 min, limit=100), and exits with status code reflecting exhaustion count. Events module documents new billing.plan_change.rotated and billing.extras_debit.exhausted event payloads.
❌ Patch coverage is 94.79167% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.03%. Comparing base (9b46842) to head (69be3fc). ⚠️ Report is 1 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The pull request successfully addresses the logic for mid-week plan changes and introduces an outbox pattern to harden extras billing. However, the current state is not up to standards according to Codacy, and there are critical reliability concerns that must be addressed before merging.
The most significant issue is that the 'atomic' outbox implementation in billing.usage.service.js is not actually atomic. Because it performs two sequential database writes without a transaction, a failure between these writes—combined with the idempotency protection on the first write—will lead to permanent loss of billing records for extras. This directly contradicts the goal of preventing revenue leaks. Additionally, several files have seen a significant spike in complexity (e.g., billing.reset.service.js and billing.meter.outbox.repository.js), which should be monitored for maintainability.
You must refactor the usage increment logic to use MongoDB transactions or implement a recovery check during idempotency replays to ensure the outbox record is always created.
About this PR
The implementation of 'atomic' usage and outbox creation in the service layer lacks a database transaction. If the usage increment succeeds but the outbox row creation fails, the record for the extras debit is lost. Since the system uses idempotency keys, a retry by the caller will not fix this; it will simply skip the logic. This creates a systemic risk of revenue leakage.
Test suggestions
Found recommended test scenario: forceRotateForPlanChange updates existing current-week usage document without an upsert
Found recommended test scenario: forceRotateForPlanChange preserves existing meterUsed and breakdown by default
Found recommended test scenario: forceRotateForPlanChange resets meterUsed to zero when preserveUsage is false
Found recommended test scenario: incrementMeterWithOutbox creates a pending outbox row when usage overflows into extras
Found recommended test scenario: attribute() marks the outbox row as committed immediately upon successful synchronous debit
Found recommended test scenario: attribute() leaves the outbox row as pending and returns applied:true if synchronous debit fails
Found recommended test scenario: retryPendingExtrasDebits cron reconciles pending rows and handles successful retries
Found recommended test scenario: Outbox rows reach 'failed' status and emit billing.extras_debit.exhausted after 5 failures
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Hardens billing lifecycle behavior by (1) refreshing the active weekly usage snapshot on mid-cycle Stripe plan changes, and (2) introducing an outbox flow to make “usage increment + extras debit” resilient to partial failures.
Changes:
Add forceRotateForPlanChange() to refresh current-week quota/version snapshot on plan change (without weekly rollover), and wire it into the subscription webhook.
Introduce BillingMeterOutbox + retry service/cron to reconcile extras debits when synchronous debit fails after usage is already attributed.
Add/adjust unit + integration tests and update billing docs/cron README to reflect the new semantics.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
…ron race
Add status:'pending' filter to both findOneAndUpdate calls in markFailedAttempt
so concurrent K8s CronJob instances cannot double-emit billing.extras_debit.exhausted.
The second cron that loses the status-flip race receives null back and skips emit.
Add regression test for the concurrent-nil path.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/billing/crons/retry-pending-extras-debit.cron.js`:
- Around line 1-43: The cron entrypoint filename does not follow the modules
cron naming convention; rename the script to the billing-prefixed form (e.g.,
billing.retry-pending-extras-debit.cron.js) so it matches
modules/{name}/crons/{name}.{action}.js, and update any references (CronJob
manifests, deployment scripts, CI/docs, imports) that point to
retry-pending-extras-debit.cron.js to the new
billing.retry-pending-extras-debit.cron.js name to keep tooling and docs
consistent.
- Around line 33-39: The cron currently sets process.exitCode = 1 when
result.exhausted > 0 which treats a normal "exhausted" business outcome as an
infrastructure failure; update the success path in the
billing.retryPendingExtrasDebit flow (the block that logs the
scanned/committed/failedAttempts/exhausted message) to NOT set a non‑zero exit
code for result.exhausted — only set process.exitCode to non‑zero on real
runtime errors (the catch block) or if there are actual failedAttempts you want
to signal; keep the existing logging but remove or change the line that sets
process.exitCode based solely on result.exhausted so exhausted rows are
considered handled.
In `@modules/billing/repositories/billing.meter.outbox.repository.js`:
- Around line 71-75: Update the atomic updates to guard against race conditions
by including the current status in the query filter: change
BillingMeterOutbox().updateOne({ _id: id }, ...) in markCommitted to use
BillingMeterOutbox().updateOne({ _id: id, status: 'pending' }, ...) so only
pending rows can be moved to committed, and apply the same pattern in
markFailedAttempt (and the related update methods around lines 86-106) so their
updateOne filters include status: 'pending' (or the appropriate allowed
prior-status set) before setting lastError/lastAttemptedAt/incrementing
attempts; keep the rest of the update payload the same so updates remain atomic
and race-safe.
In `@modules/billing/repositories/billing.usage.repository.js`:
- Around line 251-253: When preserveUsage is false the reset only sets
update.$set.meterUsed and update.$set.meterBreakdown but leaves threshold flags
(alertedAt80/alertedAt100) intact, suppressing future alerts; modify the branch
where preserveUsage is checked to also clear those flags by setting
update.$set.alertedAt80 = false and update.$set.alertedAt100 = false (or
delete/reset them in meterBreakdown if they live there) so that the new quota
window can trigger thresholds again; locate the logic around preserveUsage,
update.$set.meterUsed and update.$set.meterBreakdown in
billing.usage.repository.js to apply the change.
In `@modules/billing/services/billing.meter.outbox.service.js`:
- Around line 49-67: The code treats any debitResult.applied === false as a
retryable failure, but BillingExtraService.debit() can return false for
idempotent replays; change the logic to detect duplicate/idempotency replays and
mark the outbox committed instead of incrementing attempts. Either (A) make
BillingExtraService.debit() include a distinguishing field (e.g.
debitResult.duplicate === true or debitResult.reason === 'idempotency_replay')
and update this handler to call
BillingMeterOutboxRepository.markCommitted(row._id) and increment committed when
that flag is present, or (B) if debit() cannot be changed, perform a lookup
using the outbox row’s idempotency key (from row.payload) to detect an existing
successful debit and then call markCommitted/emit as above; continue to use
markFailedAttempt/emitExhausted only for real failures.
- Around line 15-23: The emitExhausted helper currently calls
billingEvents.emit('billing.extras_debit.exhausted', ...) directly which can
throw and abort the sweep; wrap that emit call in a try/catch inside
emitExhausted (the const emitExhausted function) so listener errors are caught,
logged (non-fatal) and not rethrown—ensure you still pass the same payload
(organizationId, idempotencyKey, extrasUnits, attempts, lastError) but swallow
listener exceptions to avoid failing the entire cron/sweep.
In `@modules/billing/services/billing.usage.service.js`:
- Around line 233-243: incrementMeterWithOutbox can short-circuit and lose the
outbox row if incrementMeter applied the usage but the process died before
BillingMeterOutboxRepository.create completed; change the flow so when
result.applied === true and result.extrasConsumed > 0 you do a repository-level
recover-or-create instead of naively calling create and returning on failure:
call a new or existing repo method (e.g.,
BillingMeterOutboxRepository.findByIdempotencyKeyOrCreate /
claimOrRecoverOutbox) that atomically finds an existing outbox row by
organizationId + idempotencyKey and, if missing, inserts one with extrasUnits =
result.extrasConsumed using an upsert or unique constraint to avoid races;
return the found-or-created outbox with the result so the “usage applied but
outbox missing” case is repaired rather than short-circuited.
In `@modules/billing/services/billing.webhook.service.js`:
- Around line 256-258: The code sets planChangeResetTriggered = true
unconditionally after calling
BillingResetService.forceRotateForPlanChange(organizationId, { preserveUsage:
true }); but forceRotateForPlanChange can return null when no rotation occurred;
update the code to inspect the return value (e.g., const rotationResult = await
BillingResetService.forceRotateForPlanChange(...)) and only set
planChangeResetTriggered = true when rotationResult indicates a successful
rotation (non-null/truthy), so the subsequent fallback call to resetWeek(...)
still runs when no rotation happened.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb12caee-2e8f-4fcc-b424-51740e48b8ad
📥 Commits
Reviewing files that changed from the base of the PR and between 9b46842 and 3102ee5.
- crypto.randomInt for cron jitter (resolves Codacy critical)
- markCommitted: add status:pending guard so committed/failed rows are immutable
- emitExhausted: wrap billingEvents.emit in try/catch so listener throws cannot
cause markFailedAttempt to double-count (Copilot thread 5)
- webhook: forceRotateForPlanChange no longer suppresses resetWeek when period
also changed (combined plan+period change now calls both, fixes Copilot thread 8)
- tests: align to new markCommitted filter + combined plan+period webhook contract
- cron: exhausted rows are a handled business outcome — exit 0, warn instead of exit 1
so K8s CronJob does not treat normal reconciliation as an infrastructure failure
- repository: rotateWeekSnapshotForPlanChange with preserveUsage=false now also clears
alertedAt80/alertedAt100 so the new quota window can trigger threshold alerts again
@coderabbitai — responding to all 5 threads from the latest review:
Thread 1 (rename cron): The existing billing crons use billing.weeklyReset.js / billing.extrasExpiration.js format (no .cron.js suffix). The filename retry-pending-extras-debit.cron.js uses the .cron.js pattern consistent with how this series of PRs documented the runner type. Renaming would require updates to Kubernetes CronJob manifests and deployment docs — deferred as a follow-up clean-up.
Thread 2 (exit code 1 for exhausted): Fixed in 69be3fc — exhausted rows now warn and exit 0. Infrastructure failures only set exit 1.
Thread 3 (alertedAt80/100 on clean-break): Fixed in 69be3fc — rotateWeekSnapshotForPlanChange with preserveUsage=false now also clears alertedAt80 and alertedAt100.
Thread 4 (idempotency replays vs real failures): Acknowledged — requires debit() API extension to surface a reason field. Deferred as follow-up issue.
Thread 5 (lost-outbox gap, critical): Acknowledged design trade-off already documented in code comments and README. The session plumbing added in this PR supports a future transaction-based fix. A findOrCreate/upsert approach is also viable — deferred to a follow-up PR.
For best results, initiate chat on the files or code changes.
@PierreBrisorgueil — thanks for the detailed recap across all five threads. Here's my acknowledgement on each:
Thread 1 (rename cron): Understood — the .cron.js suffix is a deliberate convention for this PR series to signal the runner type, distinct from the older billing.weeklyReset.js style. Deferral noted; no action needed here.
Thread 2 (exit code 1 for exhausted): ✅ Confirmed — exhausted rows now exit 0 (warn only); exit 1 is reserved for infrastructure failures. Fix looks correct.
Thread 3 (alertedAt80/100 on clean-break): ✅ Confirmed — clearing alertedAt80/alertedAt100 on preserveUsage: false is the right behaviour to avoid stale alert state after a downgrade rotation.
Thread 4 (idempotency replays vs real failures): Acknowledged. The core issue is that without a reason field from debit(), exhausted retries and replay-skips look identical to the outbox. Would you like me to open a follow-up GitHub issue to track the debit() API extension?
Thread 5 (lost-outbox gap, critical): Acknowledged. The trade-off (usage incremented but outbox insert not yet persisted) is a known atomicity gap; the session plumbing laid here and a future findOrCreate/upsert approach are both viable paths. Would you like me to open a follow-up GitHub issue to track the transaction-safe fix for this gap?
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
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.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Bundles 2 lifecycle hardening findings from the post-recalibration billing audit:
forceRotateForPlanChange()—resetWeek()was no-op when the current ISO week doc existed, leaving stale quota/version snapshot on plan change (mid-week upgrades/downgrades broken until next weekly reset)Changes
B — Plan-change forced rotation
BillingResetService.forceRotateForPlanChange(orgId, { preserveUsage })updates current week snapshot quota/version mid-periodpreserveUsage: true(default) keepsmeterUsed(no double-charge / no refund)preserveUsage: falseresetsmeterUsed = 0(clean-break downgrade option)billing.webhook.service.jsplan.changed handler now callsforceRotateForPlanChangewith defaultpreserveUsage: truebilling.plan_change.rotatedevent (try/catch guarded) for audit trailrotateWeekSnapshotForPlanChange()added to usage repository (targeted update, no upsert)C — Outbox for atomic usage+extras
BillingMeterOutboxcollection withuniqueindex onidempotencyKey(prevents double-create race)BillingUsageService.incrementMeterWithOutbox()creates outbox row alongside usage incrementattribute()callsincrementMeterWithOutboxthen attempts synchronous debit; debit failure leaves outboxpendingfor retryretry-pending-extras-debit.cron.jsreconciles every 5min (with jitter)failed+billing.extras_debit.exhaustedevent emittedREADME
Test plan
forceRotateForPlanChangeunit tests: preserveUsage=true/false, no-op on missing weekSummary by CodeRabbit
New Features
Documentation