Transaction Commit vs. Delivery 🙏

[jon-sully]

Greetings! I'm trying to find the 'right' way to accomplish gracefully sending notifications from background jobs. I'm using this tiny test harness locally to convey my point:

class ExampleJob < ApplicationJob
  def perform
    b = Book.last

    b.with_lock do
      ExampleNotification.with(book: b).deliver_later(b.owner)

      raise "This is going to rollback"
    end
  end
end

And the difficulty I'm running into is that even though the job ultimately fails and rolls back, the underlying email is still sent to the user. Which causes all kinds of havoc since the job will likely be retried and thus another email sent 😅. And .deliver vs. .deliver_later doesn't change the semantics here (though I'd been curious if .deliver_later ran its own after_commit on the Notification record to do the deliveries; I don't think that's how it works though)

I used with_lock here to ensure that the Notification record will not be persisted — the whole DB transaction gets rolled back.

I know the docs mention to be careful and use after_commit for deliver_later so that the delivery jobs don't run before a transaction closes but I think that's not applicable here (the job that kicks off the notification is already after_commit for the same reason 😛)

So what's the best way to safely go about sending notifications from background jobs (vis a vis creating Notification records) while making sure the transaction commits before sending the email/delivery?

[jon-sully]

Oh, I should note that for testing this locally in-console I was also using require "sidekiq/testing" + Sidekiq::Testing.inline! just to make sure I could see the async delivery jobs running

[jon-sully]

The route I've ended up trying to walk down is to use after_commit :deliver_later, on: :create on my Notification model:

class Notification < ApplicationRecord
  include Noticed::Model

  after_commit :deliver_later, on: :create

  belongs_to :recipient, polymorphic: true
end

But of course this will require some knowledge from developers to keep in mind that if their notification delivers via database, they should not call .deliver*_later, and if their notification doesn't deliver via database, they'll need to call .deliver*_later themselves and won't get transaction lock guarantees 🤔

(EDIT: Will take some leg-work to make this work; having to figure out how to pass around the recipients that are usually passed to deliver_later and/or get them broken down and stored may be tricky)

[jon-sully]

That ^ track didn't end up going well yesterday 😆

Today wondering if instead I can go down a different route: unique_by: :foo in a Notification 🤔 e.g. maybe an API like:

class CheckedOutNotification < Noticed::Base
  deliver_by :database
  deliver_by :email #etc.

  param :book

  unique_by :book, :recipient
  # No combination of [this book, this recipient] can receive more than one CheckedOutNotification?
end

[jon-sully]

None of these trails ended up going anywhere helpful. After quite a bit of time burned here, I found https://github.com/Envek/after_commit_everywhere which will allow me to use the after_commit hooks from my background job (universally) to fire off Notifications so that a Notification will only even attempt to run if the job's transaction is successful 👍 this works great for me!

[jon-sully]

@excid3 since you mentioned in another issue thinking about a next big version of Noticed, I'd love to see some kind of transaction-based locking included. No idea how you'd accomplish that, but ultimately while I suppose notification delivery is like any other HTTP call and not bound to the success of the database transaction in which it was fired, it' be really neat if Noticed vNext had some means of only firing the notification if the transaction commits successfully.

I imagine that'll be admittedly quite tricky given that Notification records may or may not even be part of that transaction depending on whether or not deliver_by :database is set.

Just food for thought!

[excid3]

Definitely tricky! I think the after_create_commit is probably the best approach in general, but maybe there's something we can do.

We're adding a polymorphic association to the Notification model, so that could be the trigger to raise an error and not deliver notifications. 🤔

[jon-sully]

Just posting back here as I've (quite late) realized that this request is roughly equivalent to Sidekiq's "transactional push" concept and likely would indeed take the same route as just wrapping the notification stuff in after_commit_everywhere like Sidekiq opted to. Mike said they've had essentially zero issues with the setup since launch ("which means that either nobody is using it, or we just nailed it on the first try.... and I know people are using it.") Seems like a good pattern for Noticed v2 to consider as well! 💯

Any way I could help or contribute to/for that idea, @excid3?

[excid3]

v2 is going to always store the event in the database and that should solve this issue along with several others. The deliver method now just writes the notification to the database and enqueues a job to process the event.

[jon-sully]

Awesome! Can't wait to see the full list of revisions and migrate 😁 essentially just deliver_by :database being true?

[excid3]

Check out #313 for more details, shipping it this morning I think. It's a pretty massive update (I actually started the gem from scratch 🙃) but it solves so many issues and simplifies things, it's going to be so much nicer.

[jon-sully]

Amazing. I'll check it out. Thanks for all the work you put into this! 🙌

[jon-sully]

Hey @excid3, just out of curiosity, are you open to PR's against #313 (or main once it merges), specifically for docs/writing?

[jon-sully]

Ope, there it goes! 😆

[excid3]

Documentation improvements are always welcome. 👍