ActiveJobExtensions clobbers Sidekiq tenancy

[atcruice]

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:

1. old code enqueuing jobs without passing through the new ActsAsTenant::ActiveJobExtensions#serialize (missing the "current_tenant" key)
2. old code ActsAsTenant::Sidekiq::Client#call saving job tenancy in "acts_as_tenant" hash
3. new code ActsAsTenant::Sidekiq::Server#call setting job tenancy from "acts_as_tenant" hash
4. new code ActsAsTenant::ActiveJobExtensions#deserialize then nullifying ActsAsTenant.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:

1. it was similarly opportunistic
   • this would resolve issues during the crossover period
2. perhaps short-circuit if ActsAsTenant.current_tenant is already set

Do these sound like reasonable improvements?

[tmaier]

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

[phinze]

@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. 💭