message_send: Update do_send_messages codepath to send event on commit.

[prakhar1144]

CZO Discussion

Screenshots and screen captures:

Self-review checklist

• Self-reviewed the changes for clarity and maintainability
  (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

• Explains differences from previous plans (e.g., issue description).
• Highlights technical choices and bugs encountered.
• Calls out remaining decisions and concerns.
• Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

• Each commit is a coherent idea.
• Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

• Visual appearance of the changes.
• Responsiveness and internationalization.
• Strings and tooltips.
• End-to-end functionality of buttons, interactions and flows.
• Corner cases, error conditions, and easily imagined bugs.

[timabbott]

I think we probably need a docstring or something that explains how a caller should decide whether to pass skip_capture_on_commit_callbacks ... possibly something brief linking to a new section in docs/subsystems/events-system.md

[prakhar1144]

Added docstring.

I think it's not a big enough change to highlight in docs?

[timabbott]

Yeah I think the new section would be on capturing on-commit callbacks in general, not this override.

[prakhar1144]

This would be a bit clumsy to review, so I'd just write down the flow in which I made the changes -- that should make it a bit easier to review:

• In the do_send_messages codepath, change all the send_event (used directly within the function or via call to another function) to send_event_on_commit .
• Make sure that all the send_event_on_commit are within transaction.
• Fix tests

[prakhar1144]

One more important change I have made is:

Earlier, actions function were in the format of:

• do database related work within a transaction
• then send events (outside transaction)

Now we need to ensure that the send events calls are also within a transaction. So, I just removed the transaction.atomic over the database queries and add it as a decorator to the action function itself (both db queries & events will be within a transaction).

[timabbott]

Is there any harm in having @transaction.atomic(savepoint=False) on this? That usage model is nestable.

[prakhar1144]

I was just trying to avoid nested transactions as they are not of any help in these cases? Let me know if they help in some way.

[timabbott]

@transaction.atomic(savepoint=False) is a noop if we're already in a transaction -- that's effectively what savepoint=False does.

So the reason to do it here is just to avoid the risk that code reuses this function incorrectly, and there's little downside. Overall it's a lot better to have a function that is hard to use badly than a function with a comment that says "The function calling this needs to do X" -- less non-local information is required to convince oneself that it's correct.

[prakhar1144]

Yeah, my thought was to remove as it is a noop.

But your reasoning makes sense.

[timabbott]

Pushed a change for #30101 (comment) and marked this to merge once CI passes. Thanks @prakhar1144!