Manage webhook state from background job #14
Conversation
We want to know whether the correct state transitions take place, and whether the actual processing of the webhook gets done correctly
julik
changed the title
lib/munster/jobs/processing_job.rb
Outdated
| @@ -5,14 +5,23 @@ | |||
| module Munster | |||
| class ProcessingJob < ActiveJob::Base | |||
| def perform(webhook) | |||
| webhook.class.transaction do | |||
There was a problem hiding this comment.
issue: #with_lock opens transaction internally. Is there a need to open another transaction?
See rails code:
https://github.com/rails/rails/blob/36c1591bcb5e0ee3084759c7f42a706fe5bb7ca7/activerecord/lib/active_record/locking/pessimistic.rb#L92
There was a problem hiding this comment.
None - I didn't know it does! thanks!
lib/munster/jobs/processing_job.rb
Outdated
| else | ||
| Rails.logger.info { "Webhook #{webhook.inspect} did not pass validation and was skipped" } | ||
| Rails.logger.info { "#{webhook.class} #{webhook.id} did not pass validation and was skipped" } |
There was a problem hiding this comment.
issue: I would consider this to be an error-level severity and would like to see it in AppSignal, instead of just as a message in the logs (which I have a 99% chance of missing).
| webhook.handler.process(webhook) | ||
| webhook.processed! if webhook.processing? |
There was a problem hiding this comment.
question: could this if webhook.processing? check be a bit redundant?
There was a problem hiding this comment.
No, because we still assume a handler could have set the state to something else than processing.
Instead of requiring the webhook handler to manage the state of the webhook, manage it from the background job. Atomically lock the webhook for processing, and skip the job if the webhook has already been taken by a different worker. This removes the need for handlers to manage the processing state, and also enables locking - removing the need for us to have locks on the level of the job.