Fix multiple boosts of a same toot erroneously appearing in TL (#14759)

* Check for and record reblog info atomically

Instead of using ZREVRANK to determine whether a reblog is a new reblog or not,
use ZADD's NX option to perform the check/addition option atomically.

* Replace ZREVRANK call with ZSCORE key which is more efficient

* Make tests a bit stricter

* Fix off-by-one

diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb
index 9ab7b53be43165a31c5bb861354931a20f23771e..785009b5267af22f822c454c3aecb14eff570adf 100644 (file)
--- a/app/lib/feed_manager.rb
+++ b/app/lib/feed_manager.rb
@@ -77,9 +77,11 @@ class FeedManager
 
     # Get the score of the REBLOG_FALLOFF'th item in our feed, and stop
     # tracking anything after it for deduplication purposes.
-    falloff_rank  = FeedManager::REBLOG_FALLOFF - 1
+    falloff_rank  = FeedManager::REBLOG_FALLOFF
     falloff_range = redis.zrevrange(timeline_key, falloff_rank, falloff_rank, with_scores: true)
-    falloff_score = falloff_range&.first&.last&.to_i || 0
+    falloff_score = falloff_range&.first&.last&.to_i
+
+    return if falloff_score.nil?
 
     # Get any reblogs we might have to clean up after.
     redis.zrangebyscore(reblog_key, 0, falloff_score).each do |reblogged_id|
@@ -279,14 +281,12 @@ class FeedManager
 
       return false if !rank.nil? && rank < FeedManager::REBLOG_FALLOFF
 
-      reblog_rank = redis.zrevrank(reblog_key, status.reblog_of_id)
-
-      if reblog_rank.nil?
+      # The ordered set at `reblog_key` holds statuses which have a reblog
+      # in the top `REBLOG_FALLOFF` statuses of the timeline
+      if redis.zadd(reblog_key, status.id, status.reblog_of_id, nx: true)
         # This is not something we've already seen reblogged, so we
-        # can just add it to the feed (and note that we're
-        # reblogging it).
+        # can just add it to the feed (and note that we're reblogging it).
         redis.zadd(timeline_key, status.id, status.id)
-        redis.zadd(reblog_key, status.id, status.reblog_of_id)
       else
         # Another reblog of the same status was already in the
         # REBLOG_FALLOFF most recent statuses, so we note that this
@@ -300,9 +300,7 @@ class FeedManager
       # delay of the worker deliverying the original status, the late addition
       # by merging timelines, and other reasons.
       # If such a reblog already exists, just do not re-insert it into the feed.
-      rank = redis.zrevrank(reblog_key, status.id)
-
-      return false unless rank.nil?
+      return false unless redis.zscore(reblog_key, status.id).nil?
 
       redis.zadd(timeline_key, status.id, status.id)
     end

diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb
index 5088d174299bf0b3ead2f0d2f895b2a06b979403..d86dd79930987eaf66621a4506424caf00787541 100644 (file)
--- a/spec/lib/feed_manager_spec.rb
+++ b/spec/lib/feed_manager_spec.rb
@@ -444,8 +444,8 @@ RSpec.describe FeedManager do
       expect(Redis.current.exists?(reblog_set_key)).to be true
       expect(Redis.current.zrange(reblogs_key, 0, -1)).to eq [reblogged.id.to_s]
 
-      # Push everything off the end of the feed.
-      FeedManager::MAX_ITEMS.times do
+      # Push everything past the reblog falloff.
+      FeedManager::REBLOG_FALLOFF.times do
         FeedManager.instance.push_to_home(receiver, Fabricate(:status))
       end