ActiveJobExtensions clobbers Sidekiq tenancy
atcruice opened this issue · 2 comments
We recently encountered some difficulty trying to update to v1. The cause is an undesirable interaction between ActsAsTenant::Sidekiq::Server
and the new ActsAsTenant::ActiveJobExtensions
(introduced in #319) - due to their different tenancy control approaches:
- https://github.com/ErwinM/acts_as_tenant/blob/v1.0.1/lib/acts_as_tenant/sidekiq.rb#L30 is opportunistic
- only influence tenancy if the expected pieces are present
- https://github.com/ErwinM/acts_as_tenant/blob/v1.0.1/lib/acts_as_tenant/active_job_extensions.rb#L9 is pessimistic
- clobber any tenancy if the expected pieces aren't present
Context
- Ruby 3.1.4
- Rails 7.1.2
- Sidekiq Pro 7.2.0
- codebase has both ActiveJob and Sidekiq::Job derived jobs
We use a rolling deploy strategy, so have both old and new code in active use for a period during release. The exceptions we encountered were due to:
- old code enqueuing jobs without passing through the new
ActsAsTenant::ActiveJobExtensions#serialize
(missing the"current_tenant"
key) - old code
ActsAsTenant::Sidekiq::Client#call
saving job tenancy in"acts_as_tenant"
hash - new code
ActsAsTenant::Sidekiq::Server#call
setting job tenancy from"acts_as_tenant"
hash - new code
ActsAsTenant::ActiveJobExtensions#deserialize
then nullifyingActsAsTenant.current_tenant
because the"current_tenant"
key was absent (clobbering the existing tenancy context)
Proposed Solution
I'd like to work on refactoring ActsAsTenant::ActiveJobExtensions#deserialize
such that:
- it was similarly opportunistic
- this would resolve issues during the crossover period
- perhaps short-circuit if
ActsAsTenant.current_tenant
is already set
Do these sound like reasonable improvements?
I think #deserialize
should be designed similar to how #call
of ActsAsTenant::Sidekiq
looks like.
This means, it should use ActsAsTenant.with_tenant
def deserialize(job_data)
tenant_global_id = job_data.delete("current_tenant")
ActsAsTenant.current_tenant = tenant_global_id ? GlobalID::Locator.locate(tenant_global_id) : nil
super
end
Should become
def deserialize(job_data)
tenant_global_id = job_data.delete("current_tenant")
tenant = tenant_global_id ? GlobalID::Locator.locate(tenant_global_id) : nil
ActsAsTenant.with_tenant tenant do
super
end
end
@tmaier I've attempted your suggestion to resolve my issues losing current_tenant
in a Rake task after switching from Sucker Punch to GoodJob, but it doesn't seem to help.
Thinking through the change, I don't think with_tenant
for only the scope of the deserialize
method will have the intended effect, as I believe the idea is to set current_tenant
for the full duration of job execution.
I think the underlying issue with the ActiveJobExtensions implementation is still a problem and would benefit from the improvements suggested by @atcruice. I'm planning to spend a little more time thinking through this and #335 and then hopefully submit a PR. 💭