From 80e115b600a4d90a9a40e3b50a88ecc8bca02f11 Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Sun, 26 Nov 2017 10:31:28 -0600
Subject: [PATCH] favgroups: fix race condition when adding posts to favgroups.

Adding or removing a post id to a favgroup's post_ids string is
non-atomic. Lock it to prevent simultaneous updates to the same favgroup
from clobbering each other.

Same bug as #3091.
---
 app/models/favorite_group.rb | 20 ++++++++++++--------
 test/unit/post_test.rb       | 19 +++++++++++++++++++
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/app/models/favorite_group.rb b/app/models/favorite_group.rb
index f6047e9e2..2d87845f3 100644
--- a/app/models/favorite_group.rb
+++ b/app/models/favorite_group.rb
@@ -173,11 +173,13 @@ class FavoriteGroup < ApplicationRecord
   end
 
   def add!(post_id)
-    post_id = post_id.id if post_id.is_a?(Post)
-    return if contains?(post_id)
+    with_lock do
+      post_id = post_id.id if post_id.is_a?(Post)
+      return if contains?(post_id)
 
-    clear_post_id_array
-    update_attributes(:post_ids => add_number_to_string(post_id, post_ids))
+      clear_post_id_array
+      update_attributes(:post_ids => add_number_to_string(post_id, post_ids))
+    end
   end
 
   def self.purge_post(post_id)
@@ -188,11 +190,13 @@ class FavoriteGroup < ApplicationRecord
   end
 
   def remove!(post_id)
-    post_id = post_id.id if post_id.is_a?(Post)
-    return unless contains?(post_id)
+    with_lock do
+      post_id = post_id.id if post_id.is_a?(Post)
+      return unless contains?(post_id)
 
-    clear_post_id_array
-    update_attributes(:post_ids => remove_number_from_string(post_id, post_ids))
+      clear_post_id_array
+      update_attributes(:post_ids => remove_number_from_string(post_id, post_ids))
+    end
   end
 
   def add_number_to_string(number, string)
diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb
index 42deb0b7f..15ca6f194 100644
--- a/test/unit/post_test.rb
+++ b/test/unit/post_test.rb
@@ -813,6 +813,25 @@ class PostTest < ActiveSupport::TestCase
           end
         end
 
+        context "for a favgroup" do
+          setup do
+            @favgroup = FactoryGirl.create(:favorite_group, creator: @user)
+            @post = FactoryGirl.create(:post, :tag_string => "aaa favgroup:#{@favgroup.id}")
+          end
+
+          should "add the post to the favgroup" do
+            assert_equal(1, @favgroup.reload.post_count)
+            assert_equal(true, !!@favgroup.contains?(@post.id))
+          end
+
+          should "remove the post from the favgroup" do
+            @post.update(:tag_string => "-favgroup:#{@favgroup.id}")
+
+            assert_equal(0, @favgroup.reload.post_count)
+            assert_equal(false, !!@favgroup.contains?(@post.id))
+          end
+        end
+
         context "for a pool" do
           setup do
             mock_pool_archive_service!