[Django] #37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when deferred SQL raises

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------
Reporter: Gavin Wahl(vendor) | Type:
| Uncategorized
Status: new | Component: Database
| layer (models, ORM)
Version: 6.1 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
`BaseDatabaseSchemaEditor` (normally accessed through
`connection.schema_editor`) implements the context manager protocol and
wraps `transaction.atomic()` in its `__enter__` and `__exit__`. In
`__exit__`, it runs the deferred SQL and then calls
`self.atomic.__exit__`. If the deferred SQL raises, the second call never
happens and the atomic block stays open.

After that the connection is stuck. `in_atomic_block` is True with no
atomic block on the stack, so nothing will ever commit. Once anything sets
`needs_rollback` you get `TransactionManagementError` on every query.
`close()` doesn't help because inside an atomic block it keeps the dead
connection object around and waits for an `Atomic.__exit__` to reset it.

This doesn't affect manage.py migrate from a shell, since the process
exits and the server rolls back. It matters if you catch the error and
keep going, e.g. tests, `call_command("migrate")`, or a tool using
MigrationExecutor directly.

There is an existing `test_migrations_not_applied_on_deferred_sql_failure`
but it has `atomic = False` so it doesn't hit this.

Minimal reproducer attached.

Related #29721
--
Ticket URL: <https://code.djangoproject.com/ticket/37406>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------

Reporter: Gavin Wahl(vendor) | Owner: (none)
Type: Uncategorized | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Gavin Wahl(vendor)):

* Attachment "deferred_fail_atomic.py" added.

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------
Reporter: Gavin Wahl(vendor) | Owner: (none)
Type: Uncategorized | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Gavin Wahl(vendor)):

* Attachment "deferred_fail_atomic.py" removed.

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------
Reporter: Gavin Wahl(vendor) | Owner: (none)
Type: Uncategorized | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Gavin Wahl(vendor)):

* Attachment "deferred_fail_atomic.py" added.

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------
Reporter: Gavin Wahl(vendor) | Owner: (none)
Type: Uncategorized | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Gavin Wahl(vendor)):

* Attachment "deferred_fail_atomic.2.py" added.

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------
Reporter: Gavin Wahl(vendor) | Owner: (none)
Type: Uncategorized | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Gavin Wahl(vendor)):

* Attachment "deferred_fail_atomic.2.py" removed.

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------
Reporter: Gavin Wahl(vendor) | Owner: (none)
Type: Uncategorized | Status: new
Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Gavin Wahl(vendor):

Old description:

> `BaseDatabaseSchemaEditor` (normally accessed through
> `connection.schema_editor`) implements the context manager protocol and
> wraps `transaction.atomic()` in its `__enter__` and `__exit__`. In
> `__exit__`, it runs the deferred SQL and then calls
> `self.atomic.__exit__`. If the deferred SQL raises, the second call never
> happens and the atomic block stays open.
>
> After that the connection is stuck. `in_atomic_block` is True with no
> atomic block on the stack, so nothing will ever commit. Once anything
> sets `needs_rollback` you get `TransactionManagementError` on every
> query. `close()` doesn't help because inside an atomic block it keeps the
> dead connection object around and waits for an `Atomic.__exit__` to reset
> it.
>
> This doesn't affect manage.py migrate from a shell, since the process
> exits and the server rolls back. It matters if you catch the error and
> keep going, e.g. tests, `call_command("migrate")`, or a tool using
> MigrationExecutor directly.
>
> There is an existing
> `test_migrations_not_applied_on_deferred_sql_failure` but it has `atomic
> = False` so it doesn't hit this.
>
> Minimal reproducer attached.
>
> Related #29721

New description:

`BaseDatabaseSchemaEditor` (normally accessed through
`connection.schema_editor`) implements the context manager protocol and
wraps `transaction.atomic()` in its `__enter__` and `__exit__`. In
`__exit__`, it runs the deferred SQL and then calls
`self.atomic.__exit__`. If the deferred SQL raises, the second call never
happens and the atomic block stays open.

After that the connection is stuck. `in_atomic_block` is True with no
atomic block on the stack, so nothing will ever commit. Once anything sets
`needs_rollback` you get `TransactionManagementError` on every query.
`close()` doesn't help because inside an atomic block it keeps the dead
connection object around and waits for an `Atomic.__exit__` to reset it.

This doesn't affect manage.py migrate from a shell, since the process
exits and the server rolls back. It matters if you catch the error and
keep going, e.g. tests, `call_command("migrate")`, or a tool using

schema_editor directly.

There is an existing `test_migrations_not_applied_on_deferred_sql_failure`
but it has `atomic = False` so it doesn't hit this.

Minimal reproducer attached.

Related #29721

--

--
Ticket URL: <https://code.djangoproject.com/ticket/37406#comment:1>

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------
Reporter: Gavin Wahl(vendor) | Owner: (none)

Type: Bug | Status: new

Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Unreviewed => Accepted
* type: Uncategorized => Bug

Comment:

Thank you, reproduced:
{{{#!diff
--- a/tests/schema/tests.py
+++ b/tests/schema/tests.py
@@ -6078,3 +6078,23 @@ class SchemaTests(TransactionTestCase):
if connection.vendor == "postgresql":
with connection.cursor() as cursor:
cursor.execute("DROP COLLATION IF EXISTS
case_insensitive")
+
+
+class SchemaEditorTransactionTests(TransactionTestCase):
+ available_apps = []
+
+ @skipUnlessDBFeature("can_rollback_ddl")
+ def test_deferred_sql_failure_exits_atomic_block(self):
+ editor = connection.schema_editor()
+ try:
+ with self.assertRaises(DatabaseError):
+ with editor:
+ editor.deferred_sql.append(
+ "SELECT * FROM django_nonexistent_table"
+ )
+
+ self.assertFalse(connection.in_atomic_block)
+ finally:
+ # Keep the test database usable while the bug is present.
+ if editor.atomic in connection.atomic_blocks:
+ editor.atomic.__exit__(DatabaseError, DatabaseError(),
None)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37406#comment:2>

[Django]

#37406: BaseDatabaseSchemaEditor.__exit__ doesn't exit its atomic block when
deferred SQL raises
-------------------------------------+-------------------------------------

Reporter: Gavin Wahl(vendor) | Owner: Vishy
Type: Bug | Status: assigned

Component: Database layer | Version: 6.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Vishy):

* owner: (none) => Vishy
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37406#comment:3>