excid3 / noticed

Notifications for Ruby on Rails applications
MIT License
2.44k stars 173 forks source link

[BUG] Events created in nested transactions can fail to deliver #452

Closed ryanshellberg closed 5 months ago

ryanshellberg commented 5 months ago

Bug Report

Describe the Bug: Originally called out as a concern in the V2 PR. When an event is created inside of a transaction, the corresponding EventJob fires before the event itself is created. This leads to a DeserializationError which is then discarded, so the failure was quite tricky to track down.

To Reproduce: A little tricky to reproduce here because it is a race condition, but our code looks like this:

Person.transaction do 
  ... other changes ...
  ReportComplete.with(report: report).deliver(report.user)
end

Expected Behavior: EventJob's are created after the event is committed to the database.

Actual Behavior: EventJob's are fired off before the event is committed to the database.

Environment:

Possible Fix: Seems like using an after_commit callback to fire the job should work.

Related Issues: https://github.com/excid3/noticed/pull/313#discussion_r1452563955

Checklist:

excid3 commented 5 months ago

I wouldn't consider this a bug in Noticed. You should deliver the notification after your transaction is completed.

By wrapping Noticed in another transaction, it negates the transaction Noticed uses internally.

Rails 7.2 is going to help with this by enqueuing after transaction: https://edgeguides.rubyonrails.org/7_2_release_notes.html#prevent-jobs-from-being-scheduled-within-transactions

ryanshellberg commented 5 months ago

Good point, thanks for the info on the upcoming Rails change!