feat: persistent deduplication keys for scheduled notifications - #899
Open
Vvictor-commits wants to merge 2 commits into
Open
Vvictor-commits wants to merge 2 commits into
Vvictor-commits wants to merge 2 commits into
Conversation
…EOUT_MS) - Add webhookTimeoutMs field to RetrySchedulerOptions and RetrySchedulerConfig - Read WEBHOOK_DELIVERY_TIMEOUT_MS env var in loadRetrySchedulerConfig() (default 10 000 ms) - Pass timeout to WebhookDeliveryService constructor in RetryScheduler - Document new env var in .env.example Fix pre-existing merge artifacts: - request-id.ts: restore missing closing brace on generateCorrelationId() - security-headers.ts: fix invalid 'http.ServerResponse' import type syntax - discord-notification.ts: remove dead unreachable return in scvString case; restore missing closing brace on sanitizeForDiscord() - index.ts: collapse duplicate healthMonitor construction, remove duplicate subscriber declaration, fix broken shutdown try/catch block - events-server.ts: remove duplicate TemplateService/handleTemplateRoutes imports; add missing handleApiError and applyRequestIdMiddleware imports - config.ts: remove duplicate types import line; add missing validateSecrets import - event-subscriber.ts: remove duplicate processableEvents declaration; collapse duplicate getContractEvents request block; add missing backfillStartLedger property
- Add migration 003: ALTER TABLE adds deduplication_key TEXT with a UNIQUE partial index on scheduled_notifications - Update schema.sql with the new column and index for fresh installs - Extend ScheduledNotification, ScheduledNotificationRow, and CreateScheduledNotificationInput types with deduplicationKey field - Update repository.create(): inserts deduplication_key; on UNIQUE constraint violation returns the existing notification id so callers are idempotent without throwing — covers concurrent requests at the DB layer Fix pre-existing type errors surfaced by the workflow check: - migration-system.ts: cast db.all() result through unknown before mapping; stringify error in logger.error call - discord-notification.ts: use local variable instead of accessing embed.title / embed.footer after optional narrowing - notification-retry-queue.ts: annotate entry param type in .map() - schema/compatibility-check.ts: use contractEvent.dataFields instead of undeclared dataFields; guard expectedTopics?.length comparison - benchmark-utils.ts: use ReturnType<typeof setInterval> instead of NodeJS.Timer - notification-stats-cache.ts: replace cache.has() with cache.get() check (NodeCache typings do not expose has())
|
@Vvictor-commits 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
1 participant
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 #838
Summary
Introduces explicit deduplication keys for notifications so the same logical notification is never delivered more than once, even under concurrent requests or process restarts.
Changes
Core feature (4 files)
listener/src/migrations/003-notification-deduplication-key.ts(new)Adds
deduplication_key TEXTtoscheduled_notificationsviaALTER TABLE, with aUNIQUEpartial index scoped to non-null values. The partial index means null keys (notifications without dedup) are unaffected.listener/src/database/schema.sqlColumn + index added to the
CREATE TABLEstatement so fresh installs get the same schema without running the migration.listener/src/types/scheduled-notification.tsdeduplicationKey?: string | nulladded toScheduledNotificationdeduplicationKey?: stringadded toCreateScheduledNotificationInputdeduplication_key: string | nulladded toScheduledNotificationRowlistener/src/services/scheduled-notification-repository.tscreate()includesdeduplication_keyin theINSERTidis returned instead of throwing — safe under concurrent inserts (two racing requests both get the same id back)rowToNotification()mapsdeduplication_key→deduplicationKeyAcceptance criteria
create()catches constraint violation and returns existing idBug fixes (pre-existing type errors)
migration-system.tsdb.all()result throughunknownbefore mapping; stringify error inlogger.errordiscord-notification.tsembed.title/embed.footerafter optional narrowingnotification-retry-queue.tsentryparam type in.map()schema/compatibility-check.tscontractEvent.dataFieldsinstead of undeclareddataFields; guardexpectedTopics?.lengthcomparisonbenchmark-utils.tsReturnType<typeof setInterval>instead ofNodeJS.Timernotification-stats-cache.tscache.has()withcache.get() !== undefined(NodeCache typings don't exposehas())