Conversation
🦋 Changeset detectedLatest commit: 0f204ad The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Redis + BullMQ sliding-window evaluation utility ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Queue as BullMQ Queue
participant Worker as BullMQ Worker
participant Redis
participant Evaluator as Evaluator
participant Sentry
Client->>Queue: enqueue report(event, timestamp)
Queue->>Worker: process report job
Worker->>Redis: ZADD partition ZSET (member, score=timestamp)
Worker->>Queue: schedule expire job (deterministic jobId, delayed)
Worker->>Queue: enqueue throttled check job
Worker->>Sentry: start span / add breadcrumb
Queue->>Worker: process check job
Worker->>Redis: ZRANGEBYSCORE (now-window .. now)
Worker->>Redis: SISMEMBER triggered-set
Worker->>Evaluator: evaluate(deserialized events)
Evaluator-->>Worker: result { trigger: bool }
alt trigger transitioned true
Worker->>Client: call onTrigger(partition, result)
Worker->>Redis: SADD triggered-set
else trigger transitioned true->false
Worker->>Client: call onTriggerExpire(partition)
Worker->>Redis: SREM triggered-set
end
Worker->>Sentry: finish span / add breadcrumb
Queue->>Worker: process expire job
Worker->>Redis: ZREM partition ZSET member
Worker->>Client: call onEventExpire(partition, event) (if deserializable)
Worker->>Queue: enqueue check job
Worker->>Sentry: span / breadcrumb
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust and scalable sliding-window event evaluation system. It allows for defining rules that track events over a specified time period, automatically expiring old events, and triggering actions when certain thresholds are met. The system leverages Redis for efficient data storage and BullMQ for reliable asynchronous processing, ensuring that event evaluation is performed consistently and with proper error handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #915 +/- ##
==========================================
+ Coverage 71.69% 72.01% +0.31%
==========================================
Files 228 229 +1
Lines 8277 8400 +123
Branches 2661 2695 +34
==========================================
+ Hits 5934 6049 +115
- Misses 2113 2114 +1
- Partials 230 237 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1270af3 to
bf6bbe1
Compare
server/utils/windowRule.ts
Outdated
| const jobOptions = { | ||
| attempts: 5, | ||
| backoff: { delay: config.backoffDelay ?? 1000, type: "exponential" as const }, | ||
| removeOnComplete: true, | ||
| }; | ||
|
|
||
| const queue = new Queue(queueName, { connection: redis, defaultJobOptions: jobOptions }); |
There was a problem hiding this comment.
🚩 jobOptions is a single-use extracted variable
The jobOptions constant at server/utils/windowRule.ts:34-38 is used only once at line 40 as defaultJobOptions. AGENTS.md's extraction rule says "single-use = inline." This could be inlined into the Queue constructor. However, the object is moderately complex (3 properties with a nested backoff config), and this is a borderline case given the project's "maximum compactness" formatting rule would let prettier handle the line breaking. Flagging for reviewer awareness rather than as a definitive violation.
Was this helpful? React with 👍 or 👎 to provide feedback.
973de02 to
f478f78
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f204ad397
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (now - timestamp > window) { | ||
| await redis.zrem(getKey(partition), member); | ||
| break; |
There was a problem hiding this comment.
Re-evaluate trigger state after stale report cleanup
In the stale-report branch, the worker removes the member and exits without scheduling check, which leaves wr:<name>:triggered stale when a prior partial attempt inserted that member and set the partition as triggered. This can happen if zadd succeeded but another promise in the same Promise.all failed and the retry runs after the window; the orphan is removed here, but the trigger flag is never recomputed, so the next above-threshold event can be treated as already-triggered and skip onTrigger. After zrem in this path, schedule a check for the partition.
Useful? React with 👍 / 👎.
| if (result.trigger) { | ||
| if (onTrigger) await onTrigger(partition, result); | ||
| await redis.sadd(triggeredKey, partition); |
There was a problem hiding this comment.
Bug: The onTrigger callback is called before its state is persisted to Redis. If the Redis write fails, the job retries and calls the callback again, leading to multiple executions.
Severity: MEDIUM
Suggested Fix
To ensure the callback is only fired once, the state should be committed to Redis before the callback is executed. For example, move the await redis.sadd(triggeredKey, partition) call to before the await onTrigger(partition, result) call.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: server/utils/windowRule.ts#L166-L168
Potential issue: The `onTrigger` and `onTriggerExpire` callbacks are executed before
their corresponding state change is committed to Redis. For example, `onTrigger` is
called before `redis.sadd`. If the Redis operation fails due to a transient issue, the
job will retry. On retry, the system checks the state from Redis, finds that the trigger
has not been recorded, and executes the callback a second time. This can lead to
duplicate notifications or actions for a single trigger event. The issue is confirmed by
tests that explicitly expect the callbacks to be called twice upon Redis failure.
Did we get this right? 👍 / 👎 to inform future reviews.
Summary by CodeRabbit