Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Job running on the wrong tanent #27

Open
Frank004 opened this issue Jan 5, 2018 · 18 comments
Open

Job running on the wrong tanent #27

Frank004 opened this issue Jan 5, 2018 · 18 comments

Comments

@Frank004
Copy link

Frank004 commented Jan 5, 2018

I'm using

gem 'apartment', '~> 2.0'
gem 'apartment-sidekiq', github: 'tmster/apartment-sidekiq'
gem 'sidekiq', '~> 5.0.0'

ruby '2.4.2'
gem 'rails', '5.0.6'
gem 'activerecord-import', '~> 0.20.0'

I got a job that creates a batch of records on the sidekiq dashboard I get this information.

Job
BuildFooComponents

Extras
{"apartment"=>"other_tanent"}

Error Class

ActiveRecord::RecordNotUnique

Error Message

PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "foo_pkey" DETAIL: Key (id)=(3880) already exists. : INSERT INTO "foo"(column) VALUES (nextval('demo.foo_id_seq')

if you can see the tanent VALUES (nextval('demo.foo_id_seq') is not the same from the extras "apartment"=>"other_tanent"

the job

class BuildFooComponents
  include Sidekiq::Worker
  def perform(components, section_id, project_id)
    group_components=[]
    ...
   FooComponent.import group_components
  end

end
@jdrago999
Copy link

Yes I have run into this problem myself.

I haven't yet looked into solving the problem, but I have sidekiq-web running on a non-multitenant Rails app, and when I retry jobs from the sidekiq-web dashboard there, they get run on the wrong tenant within my multitenant Rails apps.

This happens even though the job itself contains the apartment argument.

@jdrago999
Copy link

For my Future Self, for anyone else from $MY_TEAMS coming here, and for everyone else:

If you're using Rails 5, do not use apartment-sidekiq until it is fixed for Rails 5.

See this blog: http://fuzzyblog.io/blog/rails/2017/08/21/rails-apartment-tenancy-and-sidekiq.html

Your welcome.

@Frank004
Copy link
Author

@jdrago999 I need to make time to figure this out. :/

@Frank004
Copy link
Author

@jdrago999 bookmarking it thanx

@alejandrodevs
Copy link

I checked this issue and I was not able to replicate it. Does someone know if this is still happening in newer versions of rails?

@alejandrodevs
Copy link

Nevermind, it is still happening.

@alejandrodevs
Copy link

alejandrodevs commented Nov 11, 2019

I was able to solve this without this gem. This is the approach:

This is for active jobs (app/jobs/application_job.rb):

class ApplicationJob < ActiveJob::Base
  before_enqueue do |job|
    tenant = Apartment::Tenant.current
    job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
    job.arguments.last.merge!(tenant: tenant)
  end

  before_perform do |job|
    tenant = job.arguments.last[:tenant]
    Apartment::Tenant.switch!(tenant)
  end
end

This is for action mailer config/initializers/mail_delivery_job.rb:

module ActionMailer
  class MailDeliveryJob < ActiveJob::Base
    before_enqueue do |job|
      tenant = Apartment::Tenant.current
      args = job.arguments.last[:args]
      args.push({}) unless args.last.is_a?(Hash)
      args.last.merge!(tenant: tenant)
      job.arguments.last[:args] = args
    end

    before_perform do |job|
      args = job.arguments.last[:args]
      tenant = args.last[:tenant]
      Apartment::Tenant.switch!(tenant)
    end
  end
end

With this you can continue using your jobs and mailers without passing extra params and you get the correct tenant in your jobs.

I hope it is useful for someone.

@Frank004
Copy link
Author

I was able to solve this without this gem. This is the approach:

This is for active jobs (app/jobs/application_job.rb):

class ApplicationJob < ActiveJob::Base
  before_enqueue do |job|
    tenant = Apartment::Tenant.current
    job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
    job.arguments.last.merge!(tenant: tenant)
  end

  before_perform do |job|
    tenant = job.arguments.last[:tenant]
    Apartment::Tenant.switch!(tenant)
  end
end

This is for action mailer config/initializers/mail_delivery_job.rb:

module ActionMailer
  class MailDeliveryJob < ActiveJob::Base
    before_enqueue do |job|
      tenant = Apartment::Tenant.current
      args = job.arguments.last[:args]
      args.push({}) unless args.last.is_a?(Hash)
      args.last.merge!(tenant: tenant)
      job.arguments.last[:args] = args
    end

    before_perform do |job|
      args = job.arguments.last[:args]
      tenant = args.last[:tenant]
      Apartment::Tenant.switch!(tenant)
    end
  end
end

With this you can continue using your jobs and mailers without passing extra params and you get the correct tenant in your jobs.

I hope it is useful for someone.

Thank you, I will try this and let you know. this will be a game-changer for me.

@Frank004
Copy link
Author

Frank004 commented Nov 11, 2019

@alejandrodevs Did you added anything else for normal jobs?? like processing background jobs (images, forms, and data process)?

@alejandrodevs
Copy link

No, I didn't add anything else. Just your job classes must inherit from ApplicationJob and that's it. Are you having an issue?

@Frank004
Copy link
Author

No at the moment, just checking the configurations before starting the test. This is a problem I been trying to fix for some time now. Gracias.

@Frank004
Copy link
Author

No, I didn't add anything else. Just your job classes must inherit from ApplicationJob and that's it. Are you having an issue? Oh I can't use include Sidekiq::Worker when I inherit from ApplicationJob. mmmMM..

@alejandrodevs
Copy link

My solution works using active job with sidekiq. I don't know how to do it directly with sidekiq workers.

@Frank004
Copy link
Author

My solution works using active job with sidekiq. I don't know how to do it directly with sidekiq workers.

Using this solution you can't use perform_now or you will get a tenant | nil as this code never gets run

    before_enqueue do |job|
      tenant = Apartment::Tenant.current
      job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
      job.arguments.last.merge!(tenant: tenant)
   end

@alejandrodevs
Copy link

You can make it works just by adding checking for the current tenant in the before_perform callback:

before_perform do |job|
  args = job.arguments.last[:args]
  tenant = args.last[:tenant] || Apartment::Tenant.current
  Apartment::Tenant.switch!(tenant)
end

@Frank004
Copy link
Author

Frank004 commented Dec 2, 2019

@alejandrodevs This what I end up using.

  before_perform do |job|
    job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
    if job.arguments.last.has_key?(:tenant)
      tenant = job.arguments.last[:tenant]
    else
      tenant = Apartment::Tenant.current
    end
    Apartment::Tenant.switch!(tenant)
  end

@h0jeZvgoxFepBQ2C
Copy link

So this is still not fixed? Is it fixable at all?

@tlkiong
Copy link

tlkiong commented Jun 30, 2021

I think I found the issue.
It should be most probably because this did not get run or it didn't run in the correct order

You can try adding the middleware directly yourself in your sidekiq initializer. Eg:

in config/initializers/sidekiq.rb:

Sidekiq.configure_client do |config|
  config.client_middleware do |chain|
    chain.add ::Apartment::Sidekiq::Middleware::Client
  end
end
        
Sidekiq.configure_server do |config|
  config.client_middleware do |chain|
    chain.add ::Apartment::Sidekiq::Middleware::Client
  end
  config.server_middleware do |chain|
    if defined?(::Sidekiq::Batch::Server)
      chain.insert_before ::Sidekiq::Batch::Server, ::Apartment::Sidekiq::Middleware::Server
    else
      chain.add ::Apartment::Sidekiq::Middleware::Server
    end
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants