Add support for a discard option in concurrency controls #594
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Remove 'duplicate' verbiage and use concurrency limits instead, simplify control flow Fix race condition vulnerability by changing logic to enqueue Add assertions when bulk enqueuing jobs with concurrency controls Dispatch jobs in the order they were enqueued Set ActiveJob successfully_enqueued for both enqueued/blocked and discarded jobs Change concurrency 'at_limit' -> 'on_conflict' Update discard logic to trigger an ActiveRecord rollback when attempting dispatch to prevent discarded job creation Change default on_conflict concurrency option to old behaviour (blocking execution) Add concurrent on_conflict documentation to README Add test for discarding grouped concurrent jobs Fix tests which expect raising enqueue errors Add test to confirm scheduled jobs are also discarded
Otherwise we'd get inconsistent behaviour with other adapters like Sidekiq, where only jobs that are actually enqueued get assigned a provider_job_id. Turns out, issuing a DELETE query for a record that got inserted within a transaction works perfectly, even with bulk INSERT, so let's just avoid creating those jobs altogether.
This was referenced Jul 10, 2025
| Awesome! 👏 In the future, I also see an option to: |
| It looks great, @rosa! Thank you for wrapping it up for us; I am happy to have contributed some ideas. Look forward to seeing this released. ❤️ |
| This looks incredible! It was a honor to get to talk to you and Mike at Rails Conf and discuss Active Job's |
This was referenced Jul 13, 2025
Closed
chaadow reviewed Jul 14, 2025
| | ||
| def handle_concurrency_conflict | ||
| if concurrency_on_conflict.discard? | ||
| destroy |
There was a problem hiding this comment.
Parent method is not defined with a bang, so should this one call a bang operation?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This PR builds on #523 and #586, after having great discussions in those with @joelzwarrington and @mhenrixon. Thanks a lot for your work on that and for bearing with me 🙏 In the beginning, I wasn't sure how to approach this because of the way Solid Queue enqueues jobs in two steps (first, the job is created, then an execution for it is created), but after going through those two PRs again and looking into how Sidekiq's unique jobs feature behaves, I think the behaviour here is the right one.
The way this works is quite simple: a new option,
on_conflict, that defaults to:blockbut can be set to:discard.Basically, when a job needs to be discarded because of a conflict, we delete it in the same transaction where it was created. We don't mark it as finished as #586 does because I don't want to have a provider job ID in this case. I became convinced of this after reviewing how Sidekiq's unique jobs feature behaves. I was super lucky I got the chance to speak with @mperham yesterday at RailsConf (🙏 🙇♀️ ), so I could make sure I got this right. Chatting with him and other super nice and helpful attendees (I'm sorry I didn't catch your GitHub handle, but if you're reading this, you know who you are so let me know!), made me realise Active Job's behaviour with regards to
successfully_enqueued?is inconsistent betweenperform_laterandperform_all_later. Withperform_later, unless the adapter raises, the job is considered successfully enqueued. This doesn't matchperform_all_laterwhere raising doesn't make sense because you need to enqueue multiple jobs, so you don't want the whole thing to fail if a single job fails to be enqueued. In Sidekiq's case, when a job fails to be enqueued an exception is not raised: the return result is not a job ID (jid) butnilinstead. This is reflected in the bulk enqueue implementation but not in the individual enqueue. I think this should be fixed in Active Job, giving the adapter a chance to setsucessfully_enqueuedinstead of having to raise an exception to prevent it from being set totruehere. I think the inconsistency might simply be becauseperform_all_laterwas added much later and the exception raising behaviour couldn't be used there 🤔Anyway, here I chose to do the same as Sidekiq, and just not enqueue the job at all (and not have a job ID) when we need to discard a job because of concurrency limits.