From 517af45e32535efe1494c0e1e59304a5a7771dba Mon Sep 17 00:00:00 2001 From: ThibG Date: Mon, 7 Sep 2020 18:00:15 +0200 Subject: [PATCH] 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 --- app/lib/feed_manager.rb | 20 +++++++++----------- spec/lib/feed_manager_spec.rb | 4 ++-- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index 9ab7b53be4..785009b526 100644 --- 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 5088d17429..d86dd79930 100644 --- 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