MM-69458 Improving notification deduplication logic#1321
MM-69458 Improving notification deduplication logic#1321avasconcelos114 wants to merge 2 commits into
Conversation
- Moving encryption guard up to prevent mistakenly treating the raw admin API token as an encrypted value
avasconcelos114
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesNotification deduplication and encryption guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/webhook.go`:
- Around line 223-237: Keep the in-flight dedup claim from expiring before the
DM send completes. The pre-send claim in PostNotifications uses
notificationDedupKey with the same short notificationDedupTTL, but the
CreateBotDMPost path can take longer than that and allow a second delivery to
reclaim the key and post a duplicate. Update the claim in PostNotifications to
use a longer-lived expiry for the in-flight send, and after CreateBotDMPost
succeeds, optionally restore the expiry back to notificationDedupTTL if you
still want the final dedup window to remain short.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3e9a6084-5413-4497-b8d8-3ea286c2db29
📒 Files selected for processing (3)
server/plugin.goserver/webhook.goserver/webhook_parser_misc_test.go
| if err != nil { | ||
| p.errorf("PostNotifications: failed to create notification post, err: %v", err) | ||
| // Release the claim so the notification can be retried. | ||
| if claimed { |
There was a problem hiding this comment.
CreateBotDMPost can persist the post and still return an error. Releasing the claim in that case defeats the point of this PR as retry will duplicate. Suggest not releasing on error and letting the TTL expire the key.
post, err := p.CreateBotDMPost(instance.GetID(), mattermostUserID, notification.message, notification.postType)
if err != nil {
p.errorf("PostNotifications: failed to create notification post, err: %v", err)
// Dont release the dedup claim
// TTL will clear the claim
continue
}
| raw := fmt.Sprintf("%s_%s_%s", | ||
| func notificationDedupKey(instanceID types.ID, wh *webhook, recipientID types.ID, message string) string { | ||
| raw := fmt.Sprintf("%s_%s_%s_%s", | ||
| string(instanceID), |
There was a problem hiding this comment.
Including instanceID fixes cross instance key collisions. Good catch :))
nang2049
left a comment
nang2049
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114 just one minor improvement!
|
@ogi-m This should be ready for testing now! If possible it would be great to have this tested on an HA environment as some reports show that having an |
2 participants
Summary
This PR is meant to pay some technical debt that was identified during a review in a backport PR. This is aimed to make 3 general improvements:
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-69458
Change Impact: 🟠 Medium
Reasoning: The PR hardens admin API token handling (encryption guard ordering) and materially changes webhook notification de-duplication semantics under concurrency (atomic claim, instance-scoped keys, longer TTL), both of which affect correctness of sensitive flows but remain contained to server-side plugin/webhook logic.
Regression Risk: Moderate. Token decryption/config-path behavior and user-facing DM notification delivery/dedup behavior are both impacted; however, the change is localized, has accompanying test updates for the dedup key, and rollback should be straightforward (revert server/plugin/webhook logic).
QA Recommendation: Run targeted manual QA focused on (1) admin API token configuration cases (with/without encryption key) and failure logging/error behavior, and (2) webhook notification delivery under concurrent deliveries to confirm duplicates are suppressed only within the intended instance scope. Skipping manual QA is not recommended for these user-visible correctness paths.
Generated by CodeRabbitAI