search

chaps-io / gush

Fast and distributed workflow runner using ActiveJob and Redis
MIT License
1.05k stars 106 forks source link

Fixed the workflow redis key format used in `Client#next_free_workflow_id` #120

Closed noahfpf closed 3 months ago

noahfpf commented 3 months ago

Previously, the wrong key prefix was used, which means that this method was not actually checking whether a workflow id was already in use.

natemontgomery commented 3 months ago

Nice catch!

A part of me wonders if we really need a check for collisions on a v4 UUID from a battle tested lib like SecureRandom, especially on any well-behaved random source from a Linux/Unix host, though. The probability of such an event is so small that I would be very interested if it ever did happen.

It does have the 'but why not check?' kind of feel though, so maybe a setting to turn the collision check off in the config would be helpful for some users? Given a few centuries, or a very interesting coincidence, who knows what could happen.

pokonski commented 3 months ago

@natemontgomery yeah the check is probably not needed like you correctly pointed out, but it's just an EXISTS command - so rather harmless

natemontgomery commented 3 months ago

Indeed, it seems entirely fair to be paranoid about collisions, especially for so cheap a price.