Raise error when encountering unhandled callbacks

Before now, exiting `transaction` would run all after-commit callbacks,
on the assumption that they were enqueued inside the transaction.
In tests this sometimes hid order-of-execution bugs, where pre-existing
unhandled after-commit callbacks would get called, but at the wrong
time.

When opening a transaction, we will now check for pre-existing unhandled
after-commit callbacks, and raise an error when they're found.

Author: meshy

CHANGELOG.md

@@ -7,6 +7,14 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
+### Added
+
+- An error will be raised when opening a transaction if there are pre-existing unhandled after-commit callbacks.
+  The pre-existing callbacks would previously run when `transaction` exits.
+  This helps catch order-of-execution bugs in tests.
+  The old behavior can be restored using `settings.SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS`
+  to facilitate gradual adoption of this stricter rule.
+
 ### Fixed
 
 - Ensure cleanup actions in `durable` always happen when the wrapped code raises an unexpected error.

src/django_subatomic/db.py

@@ -194,11 +194,23 @@ def _execute_on_commit_callbacks_in_tests(using: str | None = None) -> Generator
     - Django 4.2's `run_and_clear_commit_hooks` function:
         https://github.com/django/django/blob/stable/4.2.x/django/db/backends/base/base.py#L762-L779
     """
+    only_in_testcase_transaction = _innermost_atomic_block_wraps_testcase(using=using)
+
+    if (
+        getattr(settings, "SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS", True)
+        and only_in_testcase_transaction
+    ):
+        connection = django_transaction.get_connection(using)
+        callbacks = connection.run_on_commit
+        if callbacks:
+            raise _UnhandledCallbacks(tuple(callback for _, callback, _ in callbacks))
+
     yield
+
     if (
         # See Note [Running after-commit callbacks in tests]
         getattr(settings, "SUBATOMIC_RUN_AFTER_COMMIT_CALLBACKS_IN_TESTS", True)
-        and _innermost_atomic_block_wraps_testcase(using=using)
+        and only_in_testcase_transaction
     ):
         connection = django_transaction.get_connection(using)
         callbacks = connection.run_on_commit
@@ -284,6 +296,20 @@ class _UnexpectedDanglingTransaction(Exception):
     open_dbs: frozenset[str]
 
 
+@attrs.frozen
+class _UnhandledCallbacks(Exception):
+    """
+    Raised in tests when unhandled callbacks are found before opening a transaction.
+
+    This happens when after-commit callbacks are registered
+    but not run before trying to open a database transaction.
+
+    The best solution is to ensure the after-commit callbacks are run.
+    """
+
+    callbacks: tuple[Callable[[], object], ...]
+
+
 # Note [After-commit callbacks require a transaction]
 # ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 # After-commit callbacks may only be registered when a transaction is open.

tests/test_db.py

@@ -194,6 +194,100 @@ def raises() -> None:
         assert error_raised is True
         assert counter.count == 1
 
+    @pytest.mark.parametrize(
+        "transaction_manager",
+        (db.transaction, db.transaction_if_not_already),
+    )
+    def test_unhandled_callbacks_cause_error(
+        self, transaction_manager: DBContextManager
+    ) -> None:
+        """
+        If callbacks from a previous atomic context remain, raise an error.
+        """
+        counter = Counter()
+
+        # Django's `atomic` leaves unhandled after-commit actions on exit.
+        with django_transaction.atomic():
+            db.run_after_commit(counter.increment)
+
+        # `transaction` will raise when it finds the unhandled callback.
+        with pytest.raises(db._UnhandledCallbacks) as exc_info:  # noqa: SLF001
+            with transaction_manager():
+                ...
+
+        assert counter.count == 0
+        assert exc_info.value.callbacks == (counter.increment,)
+
+    @pytest.mark.parametrize(
+        "transaction_manager",
+        (db.transaction, db.transaction_if_not_already),
+    )
+    def test_unhandled_callbacks_check_can_be_disabled(
+        self, transaction_manager: DBContextManager
+    ) -> None:
+        """
+        We can disable the check for unhandled callbacks.
+        """
+        counter = Counter()
+
+        # Django's `atomic` leaves unhandled after-commit actions on exit.
+        with django_transaction.atomic():
+            db.run_after_commit(counter.increment)
+
+        # Run after-commit callbacks when `transaction` exits,
+        # even if that means running them later than is realistic.
+        with override_settings(SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS=False):
+            with transaction_manager():
+                assert counter.count == 0
+
+        assert counter.count == 1
+
+    @pytest.mark.parametrize(
+        "transaction_manager",
+        (db.transaction, db.transaction_if_not_already),
+    )
+    def test_handled_callbacks_are_not_an_error(
+        self, transaction_manager: DBContextManager
+    ) -> None:
+        """
+        Already-handled checks do not cause an error.
+        """
+        counter = Counter()
+
+        # Callbacks are handled by `transaction` and removed from the queue.
+        with db.transaction():
+            db.run_after_commit(counter.increment)
+            assert counter.count == 0
+        assert counter.count == 1
+
+        # The callbacks have been handled, so a second `transaction` does not raise.
+        with transaction_manager():
+            pass
+
+        # The callback was not run a second time.
+        assert counter.count == 1
+
+    @pytest.mark.parametrize(
+        "transaction_manager",
+        (db.transaction, db.transaction_if_not_already),
+    )
+    def test_callbacks_ignored_by_transaction_if_not_already(
+        self, transaction_manager: DBContextManager
+    ) -> None:
+        """
+        `transaction_if_not_already` ignores after-commit callbacks if a transaction already exists.
+        """
+        counter = Counter()
+
+        with transaction_manager():
+            db.run_after_commit(counter.increment)
+            with db.transaction_if_not_already():
+                assert counter.count == 0
+            assert counter.count == 0
+
+        # The callback is run when the outermost transaction exits.
+        assert counter.count == 1
+
 
 class TestTransactionRequired:
     @_parametrize_transaction_testcase