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

Batch callbacks not run when job fails, then succeeds #1239

Closed
mikehale opened this issue Feb 9, 2024 · 9 comments · Fixed by #1243
Closed

Batch callbacks not run when job fails, then succeeds #1239

mikehale opened this issue Feb 9, 2024 · 9 comments · Fixed by #1243

Comments

@mikehale
Copy link

mikehale commented Feb 9, 2024

Given the following code, the success callback is never invoked. Is that expected?

class CronCheckInJob < ApplicationJob
  class SuccessCallback < ApplicationJob
    def perform(batch, _params)
      # record success
    end
  end

  def perform(**kwargs)
    klass = kwargs[:class]
    GoodJob::Batch.enqueue(on_success: CronCheckInJob::SuccessCallback, check_in_keys: check_in_keys) do
       klass.constantize.perform_later
    end
  end
end

class TestJob < ApplicationJob
  def perform
    raise 'failing' if executions < 2
  end
end

CronCheckInJob.perform_later(klass: 'TestJob')
@bensheldon
Copy link
Owner

Thanks for opening this issue! I just poked at it and I think there's a bug with how a job is treated if it errors, but then later succeeds. I will dig into this more.

@bensheldon
Copy link
Owner

oh, let me ask: Is your batch.on_success = 'CronCheckInJob::SuccessCallback' a job? It must be a job.

@mikehale
Copy link
Author

mikehale commented Feb 10, 2024

yes, and the callback works in the case where there are no errors on the original job, updating the original job

@bensheldon
Copy link
Owner

Are you seeing this in production, or just in test with :inline execution? (I ask because the issue I discovered is specifically with :inline)

@mikehale
Copy link
Author

Good question. I'm running locally in development mode. It's running under the puma web process, but I'm not sure if that means it is :inline or not.

@mikehale
Copy link
Author

I just tried changing :execution_mode to :external, and still seeing the same result.

@bensheldon
Copy link
Owner

Just wanted to briefly follow up: I think I've found the problem:

When #928 was introduced, I think I overlooked how that would interact with this line:

execution_discarded = execution && execution.error.present? && execution.retried_good_job_id.nil?

I think the simple fix is to add && execution.finished_at there, but I want to add a few more tests and make sure that's truly the issue. But happy to have narrowed it down with your help.

@bensheldon
Copy link
Owner

bensheldon commented Feb 12, 2024

The fix is released in https://github.com/bensheldon/good_job/releases/tag/v3.24.0

Thank you so much for reporting this!

@mikehale
Copy link
Author

Wow, thanks for fixing this so quickly!

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

Successfully merging a pull request may close this issue.

2 participants