Test settings drop ATOMIC_REQUESTS, so the suite never runs a view inside the request transaction production uses #572

New issue

Closed

opened 2026-10-01 10:47:44 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-10-01 10:47:44 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, comparing the test settings' database with production's.

Checked again on 2026-10-01 against main at 0a5cad587: still present.

config/settings/base.py:140 turns ATOMIC_REQUESTS on, and Postulo's handling of slow work (#206, #220, #247) is written around it. config/settings/test.py then replaces DATABASES wholesale, in both the in-memory branch and the POSTULO_TEST_DATABASE_URL (PostgreSQL) branch, and sets it neither time. Under the test settings connections["default"].settings_dict["ATOMIC_REQUESTS"] is False. Most tests do not notice, because pytest-django wraps each test in a transaction of its own; the tests marked django_db(transaction=True) run views with no transaction at all.

Where: config/settings/test.py:13 and :22-24 (the two DATABASES assignments); config/settings/base.py:140 (DATABASES["default"].setdefault("ATOMIC_REQUESTS", True)); tests/test_slow_requests.py:1-13 (its docstring states that ATOMIC_REQUESTS is on); tests/test_backup.py:33 (pytest.mark.django_db(transaction=True)).
What happens:
- A view that writes and then raises keeps its writes in a test, where production rolls them back to the request's savepoint; a test cannot observe what a person actually sees after a failure.
- In transaction=True tests there is no enclosing transaction. core/backup.py:98-105 refuses to run inside one (_not_in_a_transaction), so a Server settings view for backups (#242) that forgot non_atomic_requests would pass in tests/test_backup.py and answer "A backup cannot be taken or restored from inside a transaction" in production.
- tests/test_slow_requests.py checks only each view's _non_atomic_requests attribute, so a change that stopped base.py setting ATOMIC_REQUESTS would not fail it either.
Why it matters: the per-request transaction decides what fails together and which views must opt out of it; tests that run a different arrangement cannot catch a mistake in either.
Fix: after both assignments in test.py, add DATABASES["default"]["ATOMIC_REQUESTS"] = True, and add a test in tests/test_slow_requests.py asserting that connection.settings_dict["ATOMIC_REQUESTS"] is true under the test settings and in the production settings read through the existing subprocess fixture in tests/security/conftest.py.

The reported scenario, a caught IntegrityError leaving a PostgreSQL transaction aborted only in production, does not hold: the test's own transaction reproduces that state.

Done when

A test in tests/test_slow_requests.py asserting ATOMIC_REQUESTS is on for the default connection under the test settings, which fails today.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, comparing the test settings' database with production's. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. `config/settings/base.py:140` turns `ATOMIC_REQUESTS` on, and Postulo's handling of slow work (#206, #220, #247) is written around it. `config/settings/test.py` then replaces `DATABASES` wholesale, in both the in-memory branch and the `POSTULO_TEST_DATABASE_URL` (PostgreSQL) branch, and sets it neither time. Under the test settings `connections["default"].settings_dict["ATOMIC_REQUESTS"]` is `False`. Most tests do not notice, because pytest-django wraps each test in a transaction of its own; the tests marked `django_db(transaction=True)` run views with no transaction at all. - **Where:** `config/settings/test.py:13` and `:22-24` (the two `DATABASES` assignments); `config/settings/base.py:140` (`DATABASES["default"].setdefault("ATOMIC_REQUESTS", True)`); `tests/test_slow_requests.py:1-13` (its docstring states that `ATOMIC_REQUESTS` is on); `tests/test_backup.py:33` (`pytest.mark.django_db(transaction=True)`). - **What happens:** - A view that writes and then raises keeps its writes in a test, where production rolls them back to the request's savepoint; a test cannot observe what a person actually sees after a failure. - In `transaction=True` tests there is no enclosing transaction. `core/backup.py:98-105` refuses to run inside one (`_not_in_a_transaction`), so a Server settings view for backups (#242) that forgot `non_atomic_requests` would pass in `tests/test_backup.py` and answer "A backup cannot be taken or restored from inside a transaction" in production. - `tests/test_slow_requests.py` checks only each view's `_non_atomic_requests` attribute, so a change that stopped base.py setting `ATOMIC_REQUESTS` would not fail it either. - **Why it matters:** the per-request transaction decides what fails together and which views must opt out of it; tests that run a different arrangement cannot catch a mistake in either. - **Fix:** after both assignments in `test.py`, add `DATABASES["default"]["ATOMIC_REQUESTS"] = True`, and add a test in `tests/test_slow_requests.py` asserting that `connection.settings_dict["ATOMIC_REQUESTS"]` is true under the test settings and in the production settings read through the existing subprocess fixture in `tests/security/conftest.py`. The reported scenario, a caught `IntegrityError` leaving a PostgreSQL transaction aborted only in production, does not hold: the test's own transaction reproduces that state. ## Done when - A test in `tests/test_slow_requests.py` asserting `ATOMIC_REQUESTS` is on for the default connection under the test settings, which fails today.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:47:44 +00:00

tiagoagueda added the

tier

enhancement

labels

2026-10-01 10:47:44 +00:00

tiagoagueda referenced this issue from a commit

2026-10-05 23:54:30 +00:00

Run the suite's views inside the request transaction production uses

tiagoagueda referenced this issue from a commit

2026-10-06 00:27:34 +00:00