refactor(notification-service): drop unused Svix client from wiring - #4739
Draft
tothandras wants to merge 1 commit into
Draft
refactor(notification-service): drop unused Svix client from wiring#4739tothandras wants to merge 1 commit into
tothandras wants to merge 1 commit into
Conversation
The notification-service binary only consumes Kafka system events and persists pending notification events via Service.CreateEvent; webhook delivery and reconciliation run exclusively in cmd/server, and the webhook handler is only reachable through channel/rule CRUD served by the HTTP API. The real Svix webhook handler was constructed solely to satisfy NewNotificationService, which also coupled worker startup to Svix availability via construction-time event type registration. Switch to the common.NotificationService wire set (noop webhook handler) so the worker no longer constructs a Svix client it cannot use. Event type registration still happens at cmd/server startup. Also document the wire set split in the notification skill and remove the stale EventHandler.Dispatch flow description (no call sites exist; delivery is reconcile-loop driven). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EsKVSPEKRuNu3xc2Tww6Ae
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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
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
cmd/notification-servicewired a real Svix client and webhook handler into itsnotification.Service, but the binary can never use it:Service.CreateEvent, which validates the rule and persists pending events + delivery statuses to Postgres.openmeter/notification/service/channel.go,rule.go), which is reachable only through the HTTP API served bycmd/server.openmeter/notification/eventhandler/, which is wired and started only incmd/server(wire already prunedNewNotificationEventHandlerin this binary since nothing consumes it).This PR switches the binary to the existing
common.NotificationServicewire set (noop webhook handler), which was defined for exactly this case but previously unused.Behavior change: constructing the real Svix handler is not side-effect-free —
webhooksvix.Newregisters notification event types with the Svix API at startup and (by default) fails startup on error. This PR removes that startup-time Svix dependency from notification-service, meaning the worker no longer fail-fasts on bad Svix config and can now start (and keep persisting pending events) while Svix is unreachable. Event type registration still happens atcmd/serverstartup (as well ascmd/balance-workerandcmd/jobs, which retain the full wiring), andcmd/serveris required for delivery anyway.Also included:
cmd/serveruses the full set, not the noop one), and the describedEventHandler.Dispatchasync path has no call sites — delivery is purely reconcile-loop driven.gofmtof pre-existing struct field misalignment incmd/notification-service/wire.go.Notes for reviewer
go generate ./cmd/notification-service/...(wire) reproduces the committedwire_gen.gowith zero diff, andgo vet -tags=dynamic/go build -tags=dynamic ./cmd/notification-service/...pass against librdkafka v2.15.0 (built from the same source pin the flake uses).cmd/balance-workerandcmd/jobsconstruct the same unused Svix webhook handler; they are intentionally left untouched here and could be follow-ups if desired.🤖 Generated with Claude Code
https://claude.ai/code/session_01EsKVSPEKRuNu3xc2Tww6Ae