Job record sometimes not visible in tests

[Earlopain]

Hi there,

I have some logic in my app that makes use of JobsFilter to display some stats about how many jobs are enqueued, what is currently running, etc.

I've written a test which inherits from ActiveSupport::TestCase that enqueues a bunch of jobs during record creation to check that I'm getting the correct numbers back. However that test is flaky and occasionally fails on CI (sadly on CI only, never gotten it locally). I've added a sleep right before the query but that didn't seem to have changed anything.

The jobs are enqeued in an after_commit hook, could that be part of the problem? It wouldn't surprise me if this doesn't have anything to do with GoodJob or I'm just doing something stupid but I thought I'd check here first.

[bensheldon]

Oof, there's a lot of variables here. (Aside: I'm really glad you're writing tests around usage of JobsFilter because I consider that a private API and subject to change 🙏🏻 )

I recently helped debug an issue where the problem was transactional fixtures/tests and threads. But, what you described could be an after_commit issue but I don't think that's been a use_transactional_tests problem in Rails for several versions: Rails is now smart enough to differentiate between the test-wrapper transaction and the transactions within the test-case and trigger an after_commit callback.

Can you tell me what execution mode GoodJob is using in your test? (Something like MyJob.queue_adapter.execution_mode) If it's inline I would expect that to be fairly deterministic. If it's async/external I would expect that to be the problem, because it's introducing threads.

Also, can you share more of the test code? I'm curious if you're able to isolate it down to what specifically is failing e.g. are no jobs ever running, or ever finishing, or is just one job that sometimes errors slightly more/less than expected.

[Earlopain]

Thanks for your reply, I really appreciate how in-depth you are going when trying to debug user issues with your gem.

Through trial and error I suspect that concurrency control plays a role here. For some reason sometimes jobs just don't get enqueued. After I removed the limit the test started to pass consistently on CI. I've somewhat confirmed this by inspecting the return value of perform_later which occasionally returns false.

I have 3 distinct job classes being enqueued, which all basically just use the model id as the concurrency key, nothing more. I suspect the concurrency key is not scoped to the job class. Job A with key "XYZ" enqueues, while job B after with the same key does not. That would also explain the CI failures. I don't have many tests and the chance of overlapping ids is, depending on the execution order, not that unlikely.

Here's some debug stuff to confirm:

# Format: model_id:job_class => good_job_concurrency_key
4:ScrapeArtistUrlJob => 4
5:ScrapeArtistUrlJob => 5
1:CreateSubmissionFileJob => 1-
2:CreateSubmissionFileJob => 2-
3:CreateSubmissionFileJob => 3-
1:E6IqdbQueryJob => 1
2:E6IqdbQueryJob => 2
3:E6IqdbQueryJob => 3
# Not enqueued because the key is occupied by the first two lines, even though this is a different job class
4:FalseClass => 
5:FalseClass => 

Since I am passing in model instances I should probably lock against the GID instead of just the plain ID. Can you confirm?

[bensheldon]

happy to help 😊 That definitely sounds plausible that the concurrency key is colliding and unable to differentiate between model primary keys. I'd try doing model.to_global_id.to_s as the concurrency key instead.

[Earlopain]

sidekiq-unique-jobs has the options unique_across_workers and unique_across_queues (both default to true). What do you think about adding similar options to GoodJob? I'd give it a shot trying to implement that myself.

Of course they'd be false by default, but perhaps for the next major release that could be changed? I believe the majority of cases are unique by job class. There are also some cases where you need uniqueness across jobs like when two are interacting with a ratelimited api but I believe those are the exception, not the rule.

That could also just be a me thing though. I'd love to hear your thoughts on this.

Edit: I made PR #1145 which adds these two values since the implementation is pretty simple.