Disable sidekiq unique jobs in test env

[mjankowski]

This is recommended by both Sidekiq - sidekiq/sidekiq#1269 (comment) - and the unique jobs gem - https://github.com/mhenrixon/sidekiq-unique-jobs#uniqueness

I believe this might fix the intermittent spec failure seen sometimes in the PollExpirationNotifyWorker and UpdateStatusService (maybe others?) specs. I've never been able to force these failures, but see them occasionally. Running just the failing specs in a loop before this change, I'd see ~10% of spec runs fail. With this change, looping through ~100 spec runs, I saw zero failures.

[ClearlyClaire]

I think we specifically wanted it for the ProcessStatusUpdateService because there once was a bug with poll notification workers not going off because they were incorrectly queued because of existing locks…

[mjankowski]

I think we specifically wanted it for the ProcessStatusUpdateService because there once was a bug with poll notification workers not going off because they were incorrectly queued because of existing locks…

Interesting -- I'll try to look through code and PR history and see if I can find reference to that and learn more.

[ClearlyClaire]

Found the change, it was #24311

[mjankowski]

Found the change, it was #24311

I read through this and the issue it was solving. Before that change, we were only removing/replacing poll notification jobs when votes were present; so a scheduled job for a poll which did not get any votes would still run at it's original time even if it's expiration time was moved back. The change there fixes that by removing the votes check. The actual code lines which remove the previously scheduled one and add new one were not changed in that PR. Separately, some coverage around the update status and poll expiration notify was added.

Possibly relevant to this current change, the unique lock config was changed from until_executed to until_executing, which I'm guessing is the change which solves the scheduling/locking issue.

All that said -- I think this change (disabling sidekiq unique jobs in test env) is fine and doesn't undo any of that. Most relevant is that the coverage for the lock strategy change (just a direct options value check) is still in place, and would still fail if someone were to change that config option, even while in test mode (I verified this locally).

So I guess ... having reviewed all that, I think this is fine - but also I might not be understanding the concern. If I'm missing the issue let me know and we can attempt to add more coverage that hits whatever scenario we're worried about and see if that is still ok as well in test mode.

[ClearlyClaire]

Possibly relevant to this current change, the unique lock config was changed from until_executed to until_executing, which I'm guessing is the change which solves the scheduling/locking issue.

Yes, the issue was that because of until_executed, it was not possible for PollExpirationNotifyWorker#requeue! to enqueue a new job.

All that said -- I think this change (disabling sidekiq unique jobs in test env) is fine and doesn't undo any of that. Most relevant is that the coverage for the lock strategy change (just a direct options value check) is still in place, and would still fail if someone were to change that config option, even while in test mode (I verified this locally).

Wouldn't the change in this PR make the test added in spec/services/update_status_service_spec.rb pass even if the lock strategy were to be changed back?

[mjankowski]

Wouldn't the change in this PR make the test added in spec/services/update_status_service_spec.rb pass even if the lock strategy were to be changed back?

I tried some combiations locally ... it looks like the 'requeues expiration notification' spec in update_status_service_spec...

• Passes both with this enabled setting true (implicitly anyway, prior to this PR) or set false (with this PR).
• Passes both with and without the lock strategy change.

I think that what might also be interacting here is from that same earlier PR we have sidekiq testing in fake mode as well -- https://github.com/mastodon/mastodon/pull/24311/files#diff-e7fd6507f78342aa03784a1860bd6a86ad1032971cbb60704ea9a355e5507b6eR123

I think the primary thing spec'ing the lock strategy is just the one direct check for that here - https://github.com/mastodon/mastodon/pull/24311/files#diff-a93192d57486bb4a7de26e1bd78d5c4aa9360cac894a32465ce71f87492e45caR27 - and the other things are mostly checking the behavior change.