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.

Co-authored-by: Samuel Searles-Bryant <sam.searles-bryant@kraken.tech>

Author: meshy

CHANGELOG.md

@@ -10,6 +10,11 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 ### Added
 
 - Django 6.0 has been added to the test matrix.
+- In tests, an error will now 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 error can be silenced by setting the `SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS_IN_TESTS` setting to `False`
+  to facilitate gradual adoption of this stricter rule.
 
 ### Fixed
 

docs/reference/settings.md

@@ -30,5 +30,25 @@ progressively enable after-commit callbacks in tests
 by using [`override_settings`][override_settings]
 on a per-test basis.
 
+## `SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS_IN_TESTS`
+
+(default: `True`)
+
+[`transaction`][django_subatomic.db.transaction]
+will raise `_UnhandledCallbacks` in tests
+if it detects any lingering unhandled after-commit callbacks
+when it's called.
+Note: because this exception represents a programming error,
+it starts with an underscore to discourage anyone from catching it.
+
+This highlights order-of-execution issues in tests
+caused by after-commit callbacks having not been run.
+This can only happen in tests without after-commit callback simulation
+(such as those using Django's `atomic` directly),
+because in live systems after commit callbacks are always handled or discarded.
+
+The error can be silenced by setting this to `False`,
+in which case, the lingering callbacks will be run by the transaction after it commits.
+
 [override_settings]: https://docs.djangoproject.com/en/stable/topics/testing/tools/#django.test.override_settings
 [django-settings]: https://docs.djangoproject.com/en/stable/topics/settings/

src/django_subatomic/db.py

@@ -194,11 +194,25 @@ 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_IN_TESTS", 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 +298,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,102 @@ 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_IN_TESTS=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