fix(billing): weekKey index becomes a true partial — legacy usage tracking works past month one - #4000
Conversation
… one doc per org (#3991) The (organizationId, weekKey) unique index declared sparse: true on a COMPOUND index — MongoDB only excludes a doc from a compound sparse index when ALL indexed fields are missing, and organizationId is always present. Every legacy (weekKey-less) usage document was indexed too (weekKey: null), so a second legacy month for the same org collided as a duplicate key and increment()'s retry silently lost the write. - Model: partialFilterExpression { weekKey: { $exists: true } }, explicit distinct name (avoids an IndexOptionsConflict boot-crash against the still-live old-named index on already-deployed DBs). - New migration: creates the new index alongside the old one, then drops the old one — dup pre-check, idempotent, skip-window fast path, E11000-catch abort, mirroring the #3990 migration's safety patterns. - Service: increment() now logs loud (with context) instead of silently swallowing a lost write on the (now anomalous) null case. - Regression + migration integration tests, MIGRATIONS.md entry.
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR replaces the compound sparse billing usage index with a named partial unique index, adds an idempotent migration with duplicate detection and race handling, and surfaces anomalous null results from legacy usage increments through service logging. ChangesBilling usage index and increment handling
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 #4000 +/- ##
=======================================
Coverage 93.42% 93.43%
=======================================
Files 170 170
Lines 5688 5693 +5
Branches 1826 1828 +2
=======================================
+ Hits 5314 5319 +5
Misses 304 304
Partials 70 70
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js`:
- Around line 208-214: Update the drop loop in the migration around
usages.dropIndex to catch and ignore MongoDB IndexNotFound errors (code 27),
while rethrowing all other failures. Preserve the existing index selection and
logging behavior, allowing concurrent instances to remove the same legacy index
without failing the migration.
- Around line 179-204: Update the index reconciliation flow around
isExactTargetIndex and NEW_INDEX_NAME so an existing same-name index with
divergent options is dropped before createIndex, allowing the expected unique
partial index to converge. Preserve the existing exact-index skip behavior, and
ensure the drop occurs before the create attempt rather than being excluded by
the later cleanup step.
In `@modules/billing/repositories/billing.usage.repository.js`:
- Around line 34-40: Update the return contract near BillingUsageRepository’s
usage update method to document both null outcomes: invalid organizationId
rejected before any write and duplicate-key retry finding no exact-match
document. In BillingUsageService.increment, distinguish these cases in logging
so invalid input is not reported as a lost write.
🪄 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 Plus
Run ID: 56e18274-e502-442d-8b13-585ba6f029fa
📒 Files selected for processing (9)
MIGRATIONS.mdmodules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.jsmodules/billing/models/billing.usage.model.mongoose.jsmodules/billing/repositories/billing.usage.repository.jsmodules/billing/services/billing.usage.service.jsmodules/billing/tests/billing.usage.bootIndexReady.integration.tests.jsmodules/billing/tests/billing.usage.repository.integration.tests.jsmodules/billing/tests/billing.usage.service.unit.tests.jsmodules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js
#3991) - Migration: drop a pre-existing NEW_INDEX_NAME index with a divergent spec before createIndex, so a hand-fixed/earlier-iteration index doesn't wedge every future boot in an IndexOptionsConflict crash loop (step (c) never drops NEW_INDEX_NAME, so it could never self-heal otherwise). - Repository: document that increment() also returns null on an invalid organizationId (no write attempted), not only on the anomalous duplicate-key-retry-miss case. - Add a regression test for the divergent-NEW_INDEX_NAME recovery path.
1 participant
What — The
{organizationId, weekKey}unique index moves fromsparse: true(which never excluded weekKey-less docs: compound-sparse skips only when ALL fields are missing) to a real partial filter (weekKey: {$exists: true}) under a new explicit name, with a coexistence migration (create-new → drop-old) and a loud log on the previously silent null-return in the legacyincrement()path.Why — With
meterMode: false(the default), a second month's legacy usage increment for the same organization collided on the mis-scoped index and silently returned null — legacy usage tracking stopped after the first month, with no error anywhere.Design notes: same-name drop/recreate was deliberately avoided — the old index is live on deployed DBs and
awaitIndexBuilds()(#3993) runs before migrations, so a same-name spec change wouldIndexOptionsConflict-crash boot; the distinct-name coexistence technique follows the existing users email-CI migration precedent. Migration: zero-write dup pre-check → create new → drop old, idempotent, skip-window fast path, E11000-catch.increment()null nowlogger.errors with full context (not thrown — public API, no caller guarantees non-null).Review gate: Claude fallback (kimi down, infra#60 fix in flight) — OK, 0 findings, all merge-blocker questions verified incl. both cited precedents. Suites: unit 2272, integration 529, lint clean. Simplify pass kept (shared dup-check helper).
Closes #3991
Summary by CodeRabbit
Bug Fixes
Reliability