From 263f58159659927c2b4fe2f95ff09b8046ca427e Mon Sep 17 00:00:00 2001 From: Charlie Denton Date: Thu, 9 Oct 2025 12:42:44 +0100 Subject: [PATCH] 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. --- CHANGELOG.md | 8 ++++ src/django_subatomic/db.py | 28 +++++++++++- tests/test_db.py | 94 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e95dd0..bac120c 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/django_subatomic/db.py b/src/django_subatomic/db.py index 25a3a77..eebe00b 100644 --- a/src/django_subatomic/db.py +++ b/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. diff --git a/tests/test_db.py b/tests/test_db.py index 0ff83a5..199ad52 100644 --- a/tests/test_db.py +++ b/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