DEV: ensure rebaking works even when some users have inconsistent data (#30261)

* DEV: add db consistency check for UserEmail

* DEV: add db consistency check for UserAvatar

* DEV: ignore inconsistent data related to user avatars when deciding whether to rebake old posts


Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

---------

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

app/jobs/scheduled/ensure_db_consistency.rb

@@ -22,6 +22,7 @@ module Jobs
         CategoryTagStat,
         User,
         UserAvatar,
+        UserEmail,
         Category,
         TopicThumbnail,
       ].each do |klass|

app/jobs/scheduled/periodical_updates.rb

@@ -25,7 +25,10 @@ module Jobs
 
       # Forces rebake of old posts where needed, as long as no system avatars need updating
       if !SiteSetting.automatically_download_gravatars ||
-           !UserAvatar.where("last_gravatar_download_attempt IS NULL").limit(1).first
+           !UserAvatar
+             .joins(user: :user_emails)
+             .where(user_emails: { primary: true }, last_gravatar_download_attempt: nil)
+             .exists?
         problems = Post.rebake_old(SiteSetting.rebake_old_posts_count, priority: :ultra_low)
         problems.each do |hash|
           post_id = hash[:post].id

app/models/user_avatar.rb

@@ -152,6 +152,13 @@ class UserAvatar < ActiveRecord::Base
   end
 
   def self.ensure_consistency!(max_optimized_avatars_to_remove: 20_000)
+    DB.exec <<~SQL
+      DELETE FROM user_avatars
+      USING user_avatars ua
+      LEFT JOIN users u ON ua.user_id = u.id
+      WHERE user_avatars.id = ua.id AND u.id IS NULL
+    SQL
+
     DB.exec <<~SQL
       UPDATE user_avatars
       SET gravatar_upload_id = NULL

app/models/user_email.rb

@@ -22,6 +22,23 @@ class UserEmail < ActiveRecord::Base
   before_save -> { destroy_email_tokens(self.email_was) }, if: :will_save_change_to_email?
 
   after_destroy { destroy_email_tokens(self.email) }
+  def self.ensure_consistency!
+    user_ids_without_primary_email = DB.query_single <<~SQL
+      SELECT u.id
+      FROM users u
+      LEFT JOIN user_emails ue ON u.id = ue.user_id AND ue.primary = true
+      WHERE ue.id IS NULL;
+    SQL
+
+    user_ids_without_primary_email.each do |user_id|
+      UserEmail.create!(
+        user_id: user_id,
+        # 64 max character length of local-part for the email address https://datatracker.ietf.org/doc/html/rfc5321#section-4.5.3.1.1
+        email: "#{SecureRandom.alphanumeric(64)}@missing-primary-email.invalid",
+        primary: true,
+      )
+    end
+  end
 
   def normalize_email
     self.normalized_email =

spec/jobs/periodical_updates_spec.rb

@@ -1,6 +1,13 @@
 # frozen_string_literal: true
 
 RSpec.describe Jobs::PeriodicalUpdates do
+  fab!(:admin)
+  before do
+    UserAvatar.where(last_gravatar_download_attempt: nil).update_all(
+      last_gravatar_download_attempt: Time.now,
+    )
+  end
+
   it "works" do
     # does not blow up, no mocks, everything is called
     Jobs::PeriodicalUpdates.new.execute(nil)
@@ -8,7 +15,7 @@ RSpec.describe Jobs::PeriodicalUpdates do
 
   it "can rebake old posts when automatically_download_gravatars is false" do
     SiteSetting.automatically_download_gravatars = false
-    post = create_post
+    post = create_post(user: admin)
     post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1)
 
     Sidekiq::Testing.fake! do
@@ -31,4 +38,52 @@ RSpec.describe Jobs::PeriodicalUpdates do
     post.reload
     expect(post.baked_at).to eq_time(baked)
   end
+
+  it "does not rebake old posts when automatically_download_gravatars is true and a valid user avatar needs updating" do
+    SiteSetting.automatically_download_gravatars = true
+    UserAvatar.last.update!(last_gravatar_download_attempt: nil)
+    post = create_post(user: admin)
+    post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1)
+
+    Sidekiq::Testing.fake! do
+      Jobs::ProcessPost.jobs.clear
+      Jobs::PeriodicalUpdates.new.execute
+      expect(Jobs::ProcessPost.jobs).to be_empty
+    end
+  end
+
+  it "does not rebake old posts when there are user avatars that need updating" do
+    SiteSetting.automatically_download_gravatars = true
+
+    post = create_post(user: admin)
+    post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1)
+    UserAvatar.last.update!(last_gravatar_download_attempt: nil)
+
+    Sidekiq::Testing.fake! do
+      Jobs::ProcessPost.jobs.clear
+      Jobs::PeriodicalUpdates.new.execute
+      expect(Jobs::ProcessPost.jobs).to be_empty
+    end
+  end
+
+  # inconsistent data will be fixed by ensure_consistency! of the relevant models
+  it "rebakes old posts when there are user avatars that need updating but have inconsistent data" do
+    SiteSetting.automatically_download_gravatars = true
+
+    user_avatar_without_user = Fabricate(:user_avatar, last_gravatar_download_attempt: Time.now)
+    user_avatar_without_user.user.delete
+    user_without_any_email = Fabricate(:user)
+    user_without_any_email.user_emails.delete_all
+    user_without_primary_email = Fabricate(:user)
+    user_without_primary_email.primary_email.update_column(:primary, false)
+
+    post = create_post(user: admin)
+    post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1)
+
+    Sidekiq::Testing.fake! do
+      Jobs::ProcessPost.jobs.clear
+      Jobs::PeriodicalUpdates.new.execute
+      expect(Jobs::ProcessPost.jobs.length).to eq(1)
+    end
+  end
 end

spec/models/user_avatar_spec.rb

@@ -234,5 +234,16 @@ RSpec.describe UserAvatar do
       expect(user_avatar.gravatar_upload_id).to eq(nil)
       expect(user_avatar.custom_upload_id).to eq(nil)
     end
+
+    it "deletes avatars without users and does not remove avatars with users" do
+      user_avatar_with_user = Fabricate(:user_avatar)
+      user_avatar_without_user = Fabricate(:user_avatar)
+      user_avatar_without_user.user.delete
+
+      UserAvatar.ensure_consistency!
+
+      expect(UserAvatar.exists?(user_avatar_with_user.id)).to eq true
+      expect(UserAvatar.exists?(user_avatar_without_user.id)).to eq false
+    end
   end
 end

spec/models/user_email_spec.rb

@@ -63,4 +63,26 @@ RSpec.describe UserEmail do
       expect(user.user_emails.count).to eq 3
     end
   end
+
+  describe ".ensure_consistency!" do
+    context "when some users have no primary emails" do
+      it "creates primary emails for the users without a primary email" do
+        user_with_primary_email = Fabricate(:user)
+        user_without_primary_email = Fabricate(:user)
+        user_without_any_email = Fabricate(:user)
+
+        user_without_primary_email.primary_email.update_column(:primary, false)
+        user_without_any_email.user_emails.delete_all
+        original_email_of_user_with_primary_email = user_with_primary_email.primary_email.email
+
+        described_class.ensure_consistency!
+
+        expect(user_without_primary_email.reload.primary_email).to be_present
+        expect(user_without_any_email.reload.primary_email).to be_present
+        expect(
+          user_with_primary_email.reload.primary_email.email,
+        ).to eq original_email_of_user_with_primary_email
+      end
+    end
+  end
 end