From a8b4d2e82c9f7baa3dfc0a2c6e6b7b6dc5b78423 Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Thu, 15 Feb 2024 06:21:03 +0800
Subject: [PATCH] DEV: Drop distributed mutex
 from`SidebarSiteSettingsBackfiller#backfill!` (#25674)

Why this change?

Backfilling can take a long time on a site with many users. As a
result, the `DistriburedMutex` will warn in the logs when the block takes
more than 60 seconds to complete. We can increase the lock validity but
the method is currently only called from a job which has a
`cluster_concurrency` set to `1`. Therefore, it is actually not
necessary for us to hold a `DistributedMutex` here.

What does this change do?

1. Removes the distributed mutex and adds a comment about the risk of
   calling the `SidebarSiteSettingsBackfiller#backfill!` method.a

2. Delete old sidebar category section links in batches for all users
   to avoid a single long running query.
---
 .../sidebar_site_settings_backfiller.rb       | 48 ++++++++++---------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/app/services/sidebar_site_settings_backfiller.rb b/app/services/sidebar_site_settings_backfiller.rb
index 3fbcd51506c..350ec887b76 100644
--- a/app/services/sidebar_site_settings_backfiller.rb
+++ b/app/services/sidebar_site_settings_backfiller.rb
@@ -14,7 +14,7 @@ class SidebarSiteSettingsBackfiller
     @linkable_klass, previous_ids, new_ids =
       case setting_name
       when "default_navigation_menu_categories"
-        [Category, previous_value.split("|"), new_value.split("|")]
+        [Category, previous_value.split("|").map(&:to_i), new_value.split("|").map(&:to_i)]
       when "default_navigation_menu_tags"
         klass = Tag
 
@@ -31,33 +31,37 @@ class SidebarSiteSettingsBackfiller
     @removed_ids = previous_ids - new_ids
   end
 
+  # This should only be called from the `Jobs::BackfillSidebarSiteSettings` job as the job is ran with a cluster
+  # concurrency of 1 to ensure that only one process is running the backfill at any point in time.
   def backfill!
-    DistributedMutex.synchronize("backfill_sidebar_site_settings_#{@setting_name}") do
-      SidebarSectionLink.where(
-        linkable_type: @linkable_klass.to_s,
-        linkable_id: @removed_ids,
-      ).delete_all
+    User
+      .real
+      .where(staged: false)
+      .select(:id)
+      .find_in_batches do |users|
+        rows = []
+        user_ids = users.map(&:id)
 
-      User
-        .real
-        .where(staged: false)
-        .select(:id)
-        .find_in_batches do |users|
-          rows = []
-
-          users.each do |user|
-            @added_ids.each do |linkable_id|
-              rows << {
-                user_id: user[:id],
-                linkable_type: @linkable_klass.to_s,
-                linkable_id: linkable_id,
-              }
-            end
+        user_ids.each do |user_id|
+          @added_ids.each do |linkable_id|
+            rows << {
+              user_id: user_id,
+              linkable_type: @linkable_klass.to_s,
+              linkable_id: linkable_id,
+            }
           end
+        end
+
+        SidebarSectionLink.transaction do
+          SidebarSectionLink.where(
+            user_id: user_ids,
+            linkable_id: @removed_ids,
+            linkable_type: @linkable_klass.to_s,
+          ).delete_all
 
           SidebarSectionLink.insert_all(rows) if rows.present?
         end
-    end
+      end
   end
 
   def number_of_users_to_backfill