| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 382
Description
Describe the bug
For Redis user, the Modis "all index" rpush:notification:all will continue to grow even if they followed Wiki Using Redis - Database Cleanup:
This was previously pointed out in #525 (comment)
... but doesn't remove the id from the index that modis maintains (there is a set at
rpush:notifications:all)
I think the wiki solution has the same problem as well.
(We should probably also update the Wiki page)
Expire is probably the most efficient way to solve this problem but there is no reliable way to remove the item from the index at expiration. Maybe deleting the item from the index when the expire command is good enough? The cleanest way to do that would be to add an expire method to modis but you could fake it in your rpush callback.
To Reproduce
Steps to reproduce the behavior:
- Follow Wiki Using Redis - Database Cleanup to setup
- Keep using Rpush for a while
- Check the size taken by
rpush:notification:allfrom Redis
Expected behavior
The size of rpush:notification:all should be reclaimed when rpush_notification:<id> are removed.
Logs or other outpus
After running Rpush for a while, rpush:notification:all is taking 3.7GB out of our 5GB Redis
System configuration (please complete the following information):
- OS:
- OS version:
- Ruby version: 3.3.4
- Rails version: 8.0.1
- Rpush version: 9.2.0
Additional context
Modis #destroy does both del from rpush:notifications:<id> as well as srem from rpush:notifications:all.
Proposed Solutions
A: Opt out of rpush:notification:all with enable_all_index false
I'm experimenting with #743
I didn't find any usage of Rpush::Client::Redis::Notification.all for now, but it feels a bit too simple, so please let me know if I missed anything obvious.
B: Asynchronous cleanup with another daily job
If we also want rpush:notifications:all to be cleanup after at least 24 hours (for some reason?), we can add another code snippet to the wiki for cleaning up orphaned IDs from rpush:notifications:all.
This works for me:
def daily_rpush_notificatoin_index_cleanup
Rails.logger.info '[Rpush] Starting cleanup of rpush:notifications:all set...'
removed_count = 0
batch_size = 1000
Modis.with_connection do |redis|
initial_count = redis.scard('rpush:notifications:all')
Rails.logger.info "[Rpush] Initial count: #{initial_count} IDs in rpush:notifications:all set"
cursor = '0'
loop do
cursor, ids = redis.sscan('rpush:notifications:all', cursor, count: batch_size)
# Check existence of all keys in batch
existence_results = redis.pipelined do |pipeline|
ids.each do |id|
pipeline.exists?("rpush:notifications:#{id}")
end
end
ids_to_remove = ids.select.with_index { |id, idx| !existence_results[idx] }
if ids_to_remove.any?
removed = redis.srem('rpush:notifications:all', ids_to_remove)
removed_count += removed
Rails.logger.info "[Rpush] Removed #{removed} stale IDs from set (batch)"
end
break if cursor == '0'
end
final_count = redis.scard('rpush:notifications:all')
Rails.logger.info "[Rpush] Cleanup task completed. Total: #{initial_count}, Removed: #{removed_count}, Remained: #{final_count}"
end
endC: Synchronous cleanup with srem("rpush:notifications:all", [id])
Update Wiki Database Cleanup snippet to include srem("rpush:notifications:all", [id])
Something like:
redis.keys('rpush:notifications:*').each do |key|
next unless redis.ttl(key) == -1
next unless redis.type(key) == 'hash'
next if redis.hget(key, 'delivered').nil?
redis.expire(key, 24.hours.to_i)
+ id = key.split(':').last
+ redis.srem('rpush:notifications:all', [id])
endModis #destroy also use srem to update the all index (code).
This mean rpush:notifications:<id> is expired after 24h but <id> would be removed from all index immediately.
D: Recommend Modis #destroy for synchronous expiration
Like B, but replace both EXPIRE and SREM with just a Modis #destroy. However, I guess we didn't recommended this because we want asynchronous expiration for some reason?


