fix: deduplicate notification events - #530
Merged
Merged
Conversation
The same logical notification event (e.g. a transaction reaching "completed") was delivered multiple times: the queue worker routes directly, TransactionModel.updateStatus() publishes the same payload to both the broadcast and per-transaction Redis channels that the notification worker subscribes to, and multiple worker instances each receive the pub/sub message. Deduplicate at the NotificationRouter choke point with an atomic Redis SET NX claim (TTL configurable via NOTIFICATION_DEDUP_TTL_SECONDS), falling back to an in-process store when Redis is unavailable. Event keys are status/entity-specific so distinct events are never conflated. Also merge the notification worker's identical message/pmessage handlers. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
fix: deduplicate notification events
|
@lekescrew22 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Closes #436
Closes #439
Closes #440
Closes #443
Summary
Duplicate notification events could be sent — e.g. a user could receive the same "transaction completed" SMS/email/push multiple times. This PR adds notification deduplication so each logical event is delivered exactly once within a dedup window.
Root cause
A single terminal status change (e.g.
completed/failed) triggered the notification router multiple times:src/queue/worker.tscallsnotificationRouter.routeTransactionNotification()afterupdateStatus().TransactionModel.updateStatus()publishes the same payload to two Redis channels (TRANSACTION_UPDATED:<id>andtransaction.updated), andNotificationWorkersubscribed to both (exactsubscribe+ patternpsubscribe), so each update was handled twice (message+pmessage).All notification paths funnel through
NotificationRouter.routeNotification(), so that is the choke point where dedup was added.Changes
src/services/notificationDeduplicator.ts(new): atomic per-event claim via RedisSET NX PX(reusing the existingredisClient), with an in-process fallback when Redis is unavailable, and fail-open behavior — a duplicate is preferable to a missed notification. TTL configurable viaNOTIFICATION_DEDUP_TTL_SECONDS(default 300s).src/services/notificationRouter.ts:routeNotification()claims a dedup key before fan-out and skips duplicates. Keys are event-specific so distinct events are never conflated:tx:{id}:{status}(completed / failed / retrying are distinct)dispute:{id}:{event}sys:{category}:{severity}:{entity}(falls back to a content hash)src/workers/notificationWorker.ts: merged the identicalmessage/pmessagehandlers into a singlehandleTransactionUpdate().notificationRouter.test.ts.Testing
notificationRouter.test.ts: 5/5 passsubscriptions.test.ts: 11/11 passhandleSubscriptionFailure.test.tsintegration: passtsc --noEmit: no new errors (20 pre-existing errors insrc/stellar/sep02.tsuntouched)Out of scope
batchPayoutWorkersends notifications via the channel services directly (bypassing the router), so it is not covered by this dedup. Happy to extend coverage there if batch payouts can be re-processed.