Repository navigation
fix: schedule bounded cleanup of expired rate-limit buckets - #301
amanthanvi wants to merge 2 commits into
Conversation
Expired buckets kept raw client identifiers and nothing deleted them. Register a one-minute internal cron that removes them in capped batches and schedules a bounded number of follow-ups while any remain. Co-authored-by: Aman Thanvi <amanthanvi@users.noreply.github.com>
|
@coderabbitai review @greptileai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Summary
Merge Risk: ⚪ Minimal · up to The scheduled cleanup appears ready to merge after normal checks. No concrete issue introduced by this change was established. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: scheduling bounded cleanup for expired rate-limit buckets. Description check The description includes the required What, Why, and Checklist sections. It explains the cleanup behavior, motivation, scope, and relevant validation results. The ingestion checks remain unchecked bec… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
-
- Commit to this branch
- Create a new PR
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the ticking clock
Expired buckets hop away
Small batches clear the burrow
More follow when the budget says
Each minute brings another sweep
The live ones safely stay
Comment @coderabbitai help to get the list of available commands.
Reviewer's GuideAdds a one-minute cron for internal, clock-driven rate-limit bucket cleanup, with transaction-safe batches capped at 500 rows and a maximum of 40 immediate follow-ups before the next cron tick resumes remaining work; comprehensive tests and changelog documentation are included. Sequence diagram for bounded rate-limit bucket cleanupsequenceDiagram
participant Cron
participant Cleanup as rateLimit.cleanupExpired
participant DB as rateLimitBuckets
participant Scheduler
Cron->>Cleanup: cleanupExpired(maxBuckets, followupsRemaining)
Cleanup->>DB: query expired buckets with by_expiresAt
DB-->>Cleanup: up to maxBuckets + 1 rows
Cleanup->>DB: delete up to maxBuckets rows
alt expired rows remain and followupsRemaining > 0
Cleanup->>Scheduler: runAfter(0, cleanupExpired, maxBuckets, followupsRemaining - 1)
end
Cleanup-->>Cron: deleted, hasMore, scheduledFollowup
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="convex/rateLimit.ts" line_range="55" />
<code_context>
+// One mutation stays under Convex's per-transaction document limits.
+// The cron asks for the cap; a caller that omits maxBuckets deletes a smaller page.
+export const CLEANUP_BATCH_DEFAULT = 100;
+export const CLEANUP_BATCH_LIMIT = 500;
+// Further batches one wave may schedule after the current mutation. The next
+// cron tick continues any remainder, so a stuck hasMore cannot loop forever.
</code_context>
<issue_to_address>
**Expired identifiers accumulate**
When expired buckets are created faster than cleanup can delete 20,500 per minute over a sustained period, `cleanupExpired` cannot keep pace, so the backlog grows and client identifiers remain stored past their expiration.
Increase cleanup capacity enough to keep pace with sustained bucket creation, or ensure bucket creation stays below that capacity.
Also at `convex/rateLimit.ts:58`, `convex/crons.ts:12`, `convex/crons.ts:16`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and the cron permanently deletes expired rate-limit bucket rows containing raw client identifiers, so a mistaken one-minute retention policy would remove data that reverting the change cannot restore. The cleanup is bounded per transaction but continues indefinitely across cron runs, making the irreversible effect potentially broad while providing no failure signal if the retention decision is wrong.
Blocking findings: convex/rateLimit.ts:55
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Expired identifiers stay until a later bounded batch when a backlog exceeds one cron wave. Say that in the changelog and the cron comment. Co-authored-by: Aman Thanvi <amanthanvi@users.noreply.github.com>
|
@coderabbitai review @greptileai review |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
convex/crons.tsto run internalrateLimit.cleanupExpiredevery minute.cleanupExpiredstays aninternalMutationand still reads the clock itself.Why
rateLimitBucketskeys embed the client identifier.cleanupExpiredalready existed, but nothing called it, so expired rows and those identifiers were kept indefinitely. A separate change is locking downrateLimit.enforce; this PR only schedules and bounds the cleanup.Checklist
pnpm next:lint,pnpm next:grammar,pnpm next:test(Next.js or Convex changes)pnpm ingest:lint,pnpm ingest:test(ingestion changes)CHANGELOG.mdupdated under Unreleased when behavior changesLocal checks:
pnpm convex:test(127 passed),pnpm convex:typecheck,scripts/check-convex-public-api.mjs,pnpm next:grammar,pnpm next:lint, andpnpm next:test(86 passed). The localconvex devwatcher accepted the cron module, andcleanupExpiredis deployed as an internal mutation.Summary by Sourcery
Schedule bounded recurring cleanup of expired rate-limit buckets while preserving the internal mutation and server-side clock handling.
Bug Fixes:
Enhancements:
Tests: