refactor: notification event handling#4141
Merged
Merged
Conversation
Signed-off-by: Krisztian Gacsal <chrisgacsal@users.noreply.github.com>
Signed-off-by: Krisztian Gacsal <chrisgacsal@users.noreply.github.com>
chrisgacsal
force-pushed
the
refactor/notification-events
branch
from
a223896 to
d8c8b2a
Compare
Contributor
📝 WalkthroughWalkthroughThis PR refactors notification event handler lifecycle management. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
openmeter/notification/adapter/event.go (1)
215-219: Consider using a singlenowvariable for consistency.
clock.Now()is called twice here (lines 217 and 219). While the time difference would be negligible in practice, using a singlenowvariable would be cleaner and ensure temporal consistency - same pattern used inListRules.♻️ Suggested fix
+ now := clock.Now() + ruleQuery := a.db.NotificationRule.Query(). Where(ruledb.Namespace(params.Namespace)). Where(ruledb.ID(params.RuleID)). Where(ruledb.Or( ruledb.DeletedAtIsNil(), - ruledb.DeletedAtGT(clock.Now()), + ruledb.DeletedAtGT(now), )). - WithChannels(EagerLoadActiveChannels(clock.Now())) + WithChannels(EagerLoadActiveChannels(now))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/notification/adapter/event.go` around lines 215 - 219, Capture clock.Now() once into a local variable (e.g., now) and reuse it in the query to ensure temporal consistency: replace the two clock.Now() calls used in Where(... DeletedAtGT(...)) and WithChannels(EagerLoadActiveChannels(...)) with that single now variable; update the same block where Where, ruledb.Or, ruledb.DeletedAtIsNil, ruledb.DeletedAtGT and WithChannels/EagerLoadActiveChannels are used (same pattern as ListRules).cmd/server/main.go (1)
278-301: Minor redundancy:Close()is called twice.The
deferat lines 280-284 andeventHandleStopat lines 292-298 both callapp.NotificationEventHandler.Close(). While this is safe (the handler usessync.OnceFuncinternally), it's a bit redundant.Comparing to the telemetry and API server blocks - those use
defer Close()for hard-close on panic/exit andShutdown(ctx)for graceful shutdown in the callback. SinceEventHandler.Close()is already idempotent and immediate, you could remove the defer block entirely and rely solely on the run group's stop function.That said, the defensive defer doesn't hurt anything - just a style nit.
♻️ Optional: Remove redundant defer
// Set notification event handler { - defer func() { - if err = app.NotificationEventHandler.Close(); err != nil { - logger.Warn("failed to close notification event handler", "error", err) - } - }() - eventHandlerStart := func() error { logger.Info("starting notification event handler") return app.NotificationEventHandler.Start() } eventHandleStop := func(err error) { logger.Debug("shutting down notification event handler gracefully...", "error", err) if err = app.NotificationEventHandler.Close(); err != nil { logger.Warn("failed to shutdown notification event handler", "error", err) } } group.Add(eventHandlerStart, eventHandleStop) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/server/main.go` around lines 278 - 301, Remove the redundant deferred Close call for app.NotificationEventHandler: delete the outer defer block that calls app.NotificationEventHandler.Close(), and keep the existing graceful shutdown callback (eventHandleStop) passed to group.Add which performs the Close; this removes the duplicate Close invocation while leaving eventHandlerStart, eventHandleStop and group.Add intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/notification/eventhandler/handler.go`:
- Around line 73-101: The test environment hangs because Handler.Start() blocks
until <-h.stopCh and NewTestEnv starts it synchronously; change NewTestEnv to
start the handler in a goroutine instead of calling Start() directly so setup
can complete and the registered cleanup can later call Close() to signal stopCh.
Specifically, locate where NewTestEnv invokes Handler.Start() and replace the
direct call with a goroutine (e.g., go func() { if err := h.Start(); err != nil
{ /* capture/log to an error channel or test logger */ } }()), ensuring any
start errors are propagated back to the testenv via a channel or logger so they
aren’t lost.
In `@openmeter/notification/internal/rule.go`:
- Around line 162-173: The fixture sets CreatedAt to now while UpdatedAt is
earlier (updatedAt = now - 12h), producing created_at > updated_at; change the
timestamps so UpdatedAt is >= CreatedAt (either set updatedAt to now or make
CreatedAt an earlier time) — update the variables used for CreatedAt and
UpdatedAt in the fixture (references: CreatedAt, UpdatedAt, now, updatedAt) so
the timestamps are in valid chronological order.
---
Nitpick comments:
In `@cmd/server/main.go`:
- Around line 278-301: Remove the redundant deferred Close call for
app.NotificationEventHandler: delete the outer defer block that calls
app.NotificationEventHandler.Close(), and keep the existing graceful shutdown
callback (eventHandleStop) passed to group.Add which performs the Close; this
removes the duplicate Close invocation while leaving eventHandlerStart,
eventHandleStop and group.Add intact.
In `@openmeter/notification/adapter/event.go`:
- Around line 215-219: Capture clock.Now() once into a local variable (e.g.,
now) and reuse it in the query to ensure temporal consistency: replace the two
clock.Now() calls used in Where(... DeletedAtGT(...)) and
WithChannels(EagerLoadActiveChannels(...)) with that single now variable; update
the same block where Where, ruledb.Or, ruledb.DeletedAtIsNil, ruledb.DeletedAtGT
and WithChannels/EagerLoadActiveChannels are used (same pattern as ListRules).
🪄 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: CHILL
Plan: Pro
Run ID: 7b9c421d-1eb0-4af5-bce6-a1d574422ef4
📒 Files selected for processing (16)
app/common/notification.gocmd/balance-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/notification-service/wire_gen.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/notification/adapter/event.goopenmeter/notification/adapter/rule.goopenmeter/notification/consumer/entitlementbalancethreshold.goopenmeter/notification/consumer/entitlementreset.goopenmeter/notification/eventhandler/handler.goopenmeter/notification/internal/rule.goopenmeter/notification/rule.goopenmeter/notification/service/event.goopenmeter/notification/service/service.go
💤 Files with no reviewable changes (3)
- openmeter/notification/consumer/entitlementreset.go
- openmeter/notification/rule.go
- openmeter/notification/consumer/entitlementbalancethreshold.go
* dispatch event if there are active channels to sent to * make running event handler blocking * run event handler in backend service only
chrisgacsal
force-pushed
the
refactor/notification-events
branch
from
d8c8b2a to
aad8cb5
Compare
gergely-kurucz-konghq
approved these changes
Apr 15, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor