fix(notifications): archive the oldest active notification at the per-source cap instead of dropping the newest - #685
Merged
Conversation
…of dropping the newest A create for a (user, source) pair already holding ten active notifications threw, so the eleventh and every later item was lost while the ten stale ones stayed put. Callers that swallow the failure per item made the loss invisible outside a log. The cap now archives the source's oldest active notifications as Superseded, and the new one is always written. The repository returns the source's active rows oldest first in place of a bare count, and the create endpoint's 429 branch goes with the throw it caught. Claude-Session: https://claude.ai/code/session_01NwnN3ND2eSXKAxJteNWSXb
Contributor
Preview Container ImagesPublished for commit
This comment is updated on each push to this PR. |
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.
Problem
InAppNotificationService.CreateNotificationAsyncthrew once a(userId, source)pair held 10 active notifications (inline literal). The cap is per source, not per subject, so a user with 11+ simultaneously pending items from one source lost everything past the tenth — and sinceMealMatchingServicecatches per entry, the drop was invisible outside an error log.NotificationsControlleradditionally caught the throw by message substring (Contains("Rate limit")) to return 429.Fix
MaxActiveNotificationsPerSource = 10.NotificationArchiveReason.Superseded; the column is string-converted, so no migration) through the existingArchiveNotificationAsyncpath — archive broadcast and ownership guard included — and then always succeeds. The newest item is kept; the stalest is displaced.GetActiveCountBySourceAsync(single caller, now useless) is replaced byGetActiveBySourceAsync, orderedCreatedAtthenIdfor a deterministic oldest.ProducesResponseType(429)are removed; nothing throws for the cap any more.The constant is a
public constrather than an options binding: no notification options surface exists today (NotificationCleanupServicehardcodes its intervals the same way). Happy to bind it to configuration if preferred.Tests
Three new tests: the eleventh create supersedes exactly the oldest and keeps the newest; one-below-cap supersedes nothing; over-cap (12 active) archives three to land on the cap. Mutation-proven: off-by-one flips in the overflow arithmetic fail 2–3 of them.
~Notificationssuite: 103 passed / 0 failed.Follow-up from #571.
https://claude.ai/code/session_01NwnN3ND2eSXKAxJteNWSXb