rails / solid_cable

A database backed ActionCable adapter
MIT License
217 stars 14 forks source link

SolidCable::TrimJob#trim? seems a bit wonky! #25

Closed shettytejas closed 1 month ago

shettytejas commented 1 month ago

So, I was going through the codebase and I came across the TrimJob. While reading the #trim? method, I just found a few cases where the method would always return false.

# Current Implementation
def trim?
  expires_per_write = (1 / trim_batch_size.to_f) * ::SolidCable.trim_chance

  rand < (expires_per_write - expires_per_write.floor)
end

Considering the SolidCable.trim_chance is hard coded to 10 (which it currently is). This effectively builds the forumla to:

expires_per_write = (1 / trim_batch_size.to_f) * 10
chance = (expires_per_write - expires_per_write.floor)
rand < chance

Now, if trim_batch_size is something like 100 (which is a default value) this would make expires_per_write equals 0.1, which would result in a chance of 0.1 (10%).

Similarly, if we keep trim_batch_size to 20, expires_per_write becomes 0.5, resulting in a chance of 0.5 (50%).

However, if the trim_batch_size is kept to 10, the expires_per_write would become 1.0, resulting in the chance to become 0.0 (0%).

This would also happen when trim_batch_size is kept to something like 5:

expires_per_write = (1 / 5.0) * 10 = 0.2 * 10 = 2.0
chance = expires_per_write - expires_per_write.floor = 2.0 - 2.0.floor = 0.0
rand < 0.0 # which basically would always be false.

In my opinion, this can be mitigated by changing the SolidCable.trim_chance to be some decimal number to never let the chance hit 0%. I can raise a PR for that if you think we can solve it in this way 😄

npezza93 commented 1 month ago

Fixed in https://github.com/rails/solid_cable/commit/ae1102a323e4e039b2cb2f54ce04ea03394743fa