Bulk Updates aren’t triggering notifications

[ryanrborn]

I have an issue where notifications aren’t being sent when running bulk updates. I’ve determined that it’s specifically in the :stream strategy with a data layer that supports transactions.

The specific problem is In lib/ash/actions/update/bulk.ex:1407 where a tmp_ref is created. This puts the notifications into a different key in the process dictionary than what is looked for later.

Is it important that there is a different ref here?

If so we could move the declaration of tmp_ref to outside the try block so that the after block can then grab the notifications in the tmp_ref key and send them.

If not, we could use the parent ref by changing the references to tmp_ref to ref.

I'm happy to do the work and submit the PR for this. I just feel I need some direction as I couldn't find a reason to have the tmp_ref but didn't trust myself to know either.

I haven't done an explicit test of bulk destroy, but it looks like it'll have the same issue.

[zachdaniel]

Hmm...I can't recall the justification for using a temporary ref there either 🤔 if removing it makes it work, that would be great. We're also going to want a test for doing a bulk update (using strategy: :stream) not sending notifications. A PR on this front would be very welcome 🙇

[ryanrborn]

I wasn't able to replicate it with ETS as it doesn't support transactions. Is there another data layer you'd prefer me using to replicate it in the test environment?

[ryanrborn]

Oh, I didn't realize Mnesia supports transactions, I'll use that.

[zachdaniel]

Just wanted to ping you on this, someone else is actually hitting this issue as well. Will you be able to make a PR in the next day or so?

[ryanrborn]

Yep. Right before I quit for the day I got it replicated with mnesia. I was going to do the same with bulk destroys too. But tomorrow I'll for sure have it ready.

[kernel-io]

I've tested this without the tmp_ref and it resolves the issue I am having, I have a made a PR, but a test would still be very welcome

#1263

[ryanrborn]

Sweet! I'll get the tests in tomorrow.

[zachdaniel]

Awesome :). I also found and fixed a fair few other transaction related notification issues related to this stuff thanks to @kernel-io, so make sure to test against latest main 🙇

[ryanrborn]

Created the PR that adds the regression tests #1266.

[zachdaniel]

Merged, thanks for the contribution!