diff --git a/app/models/notifier.rb b/app/models/notifier.rb index f00987ab8..f6f356fa4 100644 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@ -23,6 +23,12 @@ class Notifier end unless notification.previously_new_record? + # Always include source_type in the update to prevent a race condition between + # concurrent Event and Mention notifier jobs: without this, Rails' dirty tracking + # may skip source_type when it hasn't changed from the stale in-memory value, + # even though another job has since modified it in the database, leaving + # source_type and source_id mismatched. + notification.source_type_will_change! notification.update!(source: source, creator: creator, read_at: nil, unread_count: notification.unread_count + 1) end diff --git a/test/models/notifier/mention_notifier_test.rb b/test/models/notifier/mention_notifier_test.rb index 833f55ec8..2d0b274c0 100644 --- a/test/models/notifier/mention_notifier_test.rb +++ b/test/models/notifier/mention_notifier_test.rb @@ -24,4 +24,47 @@ class Notifier::EventNotifierTest < ActiveSupport::TestCase Notifier.for(events(:layout_commented)).notify end end + + test "updates source_type correctly even when a concurrent job modifies it between load and save" do + # Start with a notification sourced from a Mention for kevin on the layout card + notifications(:layout_commented_kevin).destroy + mention = users(:kevin).mentioned_by(users(:david), at: comments(:layout_overflowing_david)) + notification = Notification.create!( + user: users(:kevin), card: cards(:layout), + source: mention, creator: users(:david), unread_count: 1 + ) + + # Override create_or_find_by to simulate a concurrent EventNotifier updating + # source_type in the database after the notification is loaded but before + # the MentionNotifier's update! runs — reproducing the race condition where + # Rails' dirty tracking skips source_type because it hasn't changed from + # the stale in-memory value. + Notification.class_eval do + class << self + alias_method :original_create_or_find_by, :create_or_find_by + + def create_or_find_by(...) + original_create_or_find_by(...).tap do |record| + unless record.previously_new_record? + where(id: record.id).update_all(source_type: "Event") + end + end + end + end + end + + new_mention = users(:kevin).mentioned_by(users(:jz), at: comments(:layout_overflowing_david)) + Notifier.for(new_mention).notify + + notification.reload + assert_equal "Mention", notification.source_type + assert_equal new_mention, notification.source + ensure + Notification.class_eval do + class << self + alias_method :create_or_find_by, :original_create_or_find_by + remove_method :original_create_or_find_by + end + end + end end