Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

noahfpf
Copy link
Contributor

@noahfpf noahfpf commented Aug 6, 2024

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

…w_id`

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
Copy link

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
Copy link
Contributor

pokonski commented Aug 8, 2024

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

@pokonski pokonski merged commit 407578e into chaps-io:master Aug 8, 2024
12 checks passed
@natemontgomery
Copy link

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

@noahfpf noahfpf deleted the fix-next-free-workflow-id-key branch August 19, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants