Clean up reblog tracking keys, related improvements (#5428)

* Clean up reblog-tracking sets from FeedManager

Builds on #5419, with a few minor optimizations and cleanup of sets
after they are no longer needed.

* Update tests, fix multiply-reblogged case

Previously, we would have lost the fact that a given status was
reblogged if the displayed reblog of it was removed, now we don't.

Also added tests to make sure FeedManager#trim cleans up our reblog
tracking keys, fixed up FeedCleanupScheduler to use the right loop,
and fixed the test for it.

2 files changed, 65 insertions, 11 deletions

diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb
index 454c3afec..0f97a579e 100644
--- a/spec/lib/feed_manager_spec.rb
+++ b/spec/lib/feed_manager_spec.rb
@@ -211,6 +211,22 @@ RSpec.describe FeedManager do
         expect(FeedManager.instance.push('type', account, reblogs.last)).to be false
       end
 
+      it 'does not save a new reblog of a multiply-reblogged-then-unreblogged status' do
+        account   = Fabricate(:account)
+        reblogged = Fabricate(:status)
+        reblogs = 3.times.map { Fabricate(:status, reblog: reblogged) }
+
+        # Accept the reblogs
+        FeedManager.instance.push('type', account, reblogs[0])
+        FeedManager.instance.push('type', account, reblogs[1])
+
+        # Unreblog the first one
+        FeedManager.instance.unpush('type', account, reblogs[0])
+
+        # The last reblog should still be ignored
+        expect(FeedManager.instance.push('type', account, reblogs.last)).to be false
+      end
+
       it 'saves a new reblog of a long-ago-reblogged status' do
         account = Fabricate(:account)
         reblogged = Fabricate(:status)
@@ -230,6 +246,38 @@ RSpec.describe FeedManager do
     end
   end
 
+  describe '#trim' do
+    let(:receiver) { Fabricate(:account) }
+
+    it 'cleans up reblog tracking keys' do
+      reblogged      = Fabricate(:status)
+      status         = Fabricate(:status, reblog: reblogged)
+      another_status = Fabricate(:status, reblog: reblogged)
+      reblogs_key    = FeedManager.instance.key('type', receiver.id, 'reblogs')
+      reblog_set_key = FeedManager.instance.key('type', receiver.id, "reblogs:#{reblogged.id}")
+
+      FeedManager.instance.push('type', receiver, status)
+      FeedManager.instance.push('type', receiver, another_status)
+
+      # We should have a tracking set and an entry in reblogs.
+      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
+        FeedManager.instance.push('type', receiver, Fabricate(:status))
+      end
+
+      # `trim` should be called automatically, but do it anyway, as
+      # we're testing `trim`, not side effects of `push`.
+      FeedManager.instance.trim('type', receiver.id)
+
+      # We should not have any reblog tracking data.
+      expect(Redis.current.exists(reblog_set_key)).to be false
+      expect(Redis.current.zrange(reblogs_key, 0, -1)).to be_empty
+    end
+  end
+
   describe '#unpush' do
     let(:receiver) { Fabricate(:account) }
 
@@ -265,20 +313,22 @@ RSpec.describe FeedManager do
       expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to be_empty
     end
 
-    it 'leaves a reblogged status if another reblog was in feed' do
-      reblogged      = Fabricate(:status)
-      status         = Fabricate(:status, reblog: reblogged)
-      another_status = Fabricate(:status, reblog: reblogged)
+    it 'leaves a multiply-reblogged status if another reblog was in feed' do
+      reblogged = Fabricate(:status)
+      reblogs   = 3.times.map { Fabricate(:status, reblog: reblogged) }
 
-      FeedManager.instance.push('type', receiver, status)
-      FeedManager.instance.push('type', receiver, another_status)
+      reblogs.each do |reblog|
+        FeedManager.instance.push('type', receiver, reblog)
+      end
 
       # The reblogging status should show up under normal conditions.
-      expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [status.id.to_s]
+      expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [reblogs.first.id.to_s]
 
-      FeedManager.instance.unpush('type', receiver, status)
+      reblogs[0...-1].each do |reblog|
+        FeedManager.instance.unpush('type', receiver, reblog)
+      end
 
-      expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [another_status.id.to_s]
+      expect(Redis.current.zrange("feed:type:#{receiver.id}", 0, -1)).to eq [reblogs.last.id.to_s]
     end
 
     it 'sends push updates' do
diff --git a/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb b/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb
index b8487b03f..7fae680ba 100644
--- a/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb
+++ b/spec/workers/scheduler/feed_cleanup_scheduler_spec.rb
@@ -9,14 +9,18 @@ describe Scheduler::FeedCleanupScheduler do
   it 'clears feeds of inactives' do
     Redis.current.zadd(feed_key_for(inactive_user), 1, 1)
     Redis.current.zadd(feed_key_for(active_user), 1, 1)
+    Redis.current.zadd(feed_key_for(inactive_user, 'reblogs'), 2, 2)
+    Redis.current.sadd(feed_key_for(inactive_user, 'reblogs:2'), 3)
 
     subject.perform
 
     expect(Redis.current.zcard(feed_key_for(inactive_user))).to eq 0
     expect(Redis.current.zcard(feed_key_for(active_user))).to eq 1
+    expect(Redis.current.exists(feed_key_for(inactive_user, 'reblogs'))).to be false
+    expect(Redis.current.exists(feed_key_for(inactive_user, 'reblogs:2'))).to be false
   end
 
-  def feed_key_for(user)
-    FeedManager.instance.key(:home, user.account_id)
+  def feed_key_for(user, subtype = nil)
+    FeedManager.instance.key(:home, user.account_id, subtype)
   end
 end