Skip to content

Commit 3ee6d6d

Browse files
authored
fix: prevent session cleanup from masking original exceptions (#704)
## Summary - Replace `BaseHTTPMiddleware` with pure ASGI `SessionMiddleware` in Starlette/FastAPI extension — fixes timing issue where `response_status` wasn't set before generator cleanup - Protect session cleanup (commit/rollback/close) across **all** framework extensions so cleanup failures never mask the original route handler exception - Each cleanup operation is individually wrapped; when an original exception exists, cleanup errors are logged but suppressed **Affected extensions:** FastAPI, Starlette, Sanic, Litestar (async + sync) **Reported issue:** FastAPI validation errors overridden by internal session attribute name `advanced_alchemy_async_session_db_session_response_status` (Close #705)
1 parent 45e7ab0 commit 3ee6d6d

10 files changed

Lines changed: 674 additions & 157 deletions

File tree

advanced_alchemy/extensions/fastapi/providers.py

Lines changed: 62 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import contextlib
99
import datetime
1010
import inspect
11+
import logging
1112
from collections.abc import AsyncGenerator, Generator
1213
from typing import (
1314
TYPE_CHECKING,
@@ -53,6 +54,8 @@
5354
from advanced_alchemy.utils.singleton import SingletonMeta
5455
from advanced_alchemy.utils.text import camelize
5556

57+
logger = logging.getLogger("advanced_alchemy.extensions.fastapi")
58+
5659
if TYPE_CHECKING:
5760
from advanced_alchemy.extensions.fastapi import SQLAlchemyAsyncConfig, SQLAlchemySyncConfig
5861

@@ -201,7 +204,7 @@ def provide_service(
201204
) -> Callable[..., Generator[SyncServiceT_co, None, None]]: ...
202205

203206

204-
def provide_service( # noqa: PLR0915
207+
def provide_service( # noqa: C901, PLR0915
205208
service_class: type[Union["AsyncServiceT_co", "SyncServiceT_co"]],
206209
/,
207210
extension: AdvancedAlchemy,
@@ -256,27 +259,41 @@ async def provide_async_service(
256259
response_status = getattr(request.state, f"{session_key}_response_status", None)
257260
commit_mode = async_config.commit_mode if async_config else "manual"
258261

259-
try:
260-
should_commit = (
261-
exc_info is None
262-
and response_status is not None
263-
and _should_commit_for_status(response_status, commit_mode)
264-
)
262+
should_commit = (
263+
exc_info is None
264+
and response_status is not None
265+
and _should_commit_for_status(response_status, commit_mode)
266+
)
265267

268+
# Each cleanup operation is individually protected so that failures
269+
# in one step do not prevent subsequent steps or mask the original exception.
270+
try:
266271
if should_commit:
267272
await db_session.commit()
268273
else:
269274
await db_session.rollback()
270-
finally:
275+
except Exception:
276+
if exc_info is not None:
277+
logger.debug("Session commit/rollback failed during cleanup", exc_info=True)
278+
else:
279+
raise
280+
281+
try:
271282
await db_session.close()
272-
# Clean up request state
273-
for attr in [
274-
session_key,
275-
f"{session_key}_generator_managed",
276-
f"{session_key}_response_status",
277-
]:
278-
with contextlib.suppress(AttributeError):
279-
delattr(request.state, attr)
283+
except Exception:
284+
if exc_info is not None:
285+
logger.debug("Session close failed during cleanup", exc_info=True)
286+
else:
287+
raise
288+
289+
# Clean up request state
290+
for attr in [
291+
session_key,
292+
f"{session_key}_generator_managed",
293+
f"{session_key}_response_status",
294+
]:
295+
with contextlib.suppress(Exception):
296+
delattr(request.state, attr)
280297

281298
return provide_async_service
282299

@@ -312,27 +329,41 @@ def provide_sync_service(
312329
response_status = getattr(request.state, f"{session_key}_response_status", None)
313330
commit_mode = sync_config.commit_mode if sync_config else "manual"
314331

315-
try:
316-
should_commit = (
317-
exc_info is None
318-
and response_status is not None
319-
and _should_commit_for_status(response_status, commit_mode)
320-
)
332+
should_commit = (
333+
exc_info is None
334+
and response_status is not None
335+
and _should_commit_for_status(response_status, commit_mode)
336+
)
321337

338+
# Each cleanup operation is individually protected so that failures
339+
# in one step do not prevent subsequent steps or mask the original exception.
340+
try:
322341
if should_commit:
323342
db_session.commit()
324343
else:
325344
db_session.rollback()
326-
finally:
345+
except Exception:
346+
if exc_info is not None:
347+
logger.debug("Session commit/rollback failed during cleanup", exc_info=True)
348+
else:
349+
raise
350+
351+
try:
327352
db_session.close()
328-
# Clean up request state
329-
for attr in [
330-
session_key,
331-
f"{session_key}_generator_managed",
332-
f"{session_key}_response_status",
333-
]:
334-
with contextlib.suppress(AttributeError):
335-
delattr(request.state, attr)
353+
except Exception:
354+
if exc_info is not None:
355+
logger.debug("Session close failed during cleanup", exc_info=True)
356+
else:
357+
raise
358+
359+
# Clean up request state
360+
for attr in [
361+
session_key,
362+
f"{session_key}_generator_managed",
363+
f"{session_key}_response_status",
364+
]:
365+
with contextlib.suppress(Exception):
366+
delattr(request.state, attr)
336367

337368
return provide_sync_service
338369

advanced_alchemy/extensions/litestar/plugins/init/config/asyncio.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import logging
12
from collections.abc import AsyncGenerator, Coroutine
23
from contextlib import asynccontextmanager
34
from dataclasses import dataclass, field
@@ -22,6 +23,8 @@
2223
from advanced_alchemy.extensions.litestar.plugins.init.config.engine import EngineConfig
2324
from advanced_alchemy.routing.context import reset_routing_context
2425

26+
logger = logging.getLogger("advanced_alchemy.extensions.litestar")
27+
2528
if TYPE_CHECKING:
2629
from collections.abc import AsyncGenerator, Coroutine
2730

@@ -117,9 +120,14 @@ async def handler(message: "Message", scope: "Scope") -> None:
117120
await session.commit()
118121
else:
119122
await session.rollback()
123+
except Exception: # noqa: BLE001
124+
logger.debug("Session commit/rollback failed during cleanup", exc_info=True)
120125
finally:
121126
if session and message["type"] in SESSION_TERMINUS_ASGI_EVENTS:
122-
await session.close()
127+
try:
128+
await session.close()
129+
except Exception: # noqa: BLE001
130+
logger.debug("Session close failed during cleanup", exc_info=True)
123131
delete_aa_scope_state(scope, session_scope_key)
124132

125133
return handler

advanced_alchemy/extensions/litestar/plugins/init/config/sync.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import logging
12
from contextlib import asynccontextmanager
23
from dataclasses import dataclass, field
34
from typing import TYPE_CHECKING, Any, Callable, Literal, Optional, Union, cast
@@ -22,6 +23,8 @@
2223
from advanced_alchemy.extensions.litestar.plugins.init.config.engine import EngineConfig
2324
from advanced_alchemy.routing.context import reset_routing_context
2425

26+
logger = logging.getLogger("advanced_alchemy.extensions.litestar")
27+
2528
if TYPE_CHECKING:
2629
from collections.abc import AsyncGenerator
2730

@@ -118,9 +121,14 @@ def handler(message: "Message", scope: "Scope") -> None:
118121
session.commit()
119122
else:
120123
session.rollback()
124+
except Exception: # noqa: BLE001
125+
logger.debug("Session commit/rollback failed during cleanup", exc_info=True)
121126
finally:
122127
if session and message["type"] in SESSION_TERMINUS_ASGI_EVENTS:
123-
session.close()
128+
try:
129+
session.close()
130+
except Exception: # noqa: BLE001
131+
logger.debug("Session close failed during cleanup", exc_info=True)
124132
delete_aa_scope_state(scope, session_scope_key)
125133

126134
return handler

advanced_alchemy/extensions/sanic/config.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import asyncio
88
import contextlib
9+
import logging
910
from dataclasses import dataclass, field
1011
from typing import Any, Callable, Optional, cast
1112

@@ -35,6 +36,8 @@
3536
from advanced_alchemy.config.sync import SQLAlchemySyncConfig as _SQLAlchemySyncConfig
3637
from advanced_alchemy.service import schema_dump
3738

39+
logger = logging.getLogger("advanced_alchemy.extensions.sanic")
40+
3841

3942
def _make_unique_context_key(app: "Sanic[Any, Any]", key: str) -> str: # pragma: no cover
4043
"""Generates a unique context key for the Sanic application.
@@ -246,8 +249,13 @@ async def session_handler(
246249
await session.commit()
247250
else:
248251
await session.rollback()
252+
except Exception: # noqa: BLE001
253+
logger.debug("Session commit/rollback failed during cleanup", exc_info=True)
249254
finally:
250-
await session.close()
255+
try:
256+
await session.close()
257+
except Exception: # noqa: BLE001
258+
logger.debug("Session close failed during cleanup", exc_info=True)
251259
with contextlib.suppress(AttributeError, KeyError):
252260
delattr(request.ctx, self.session_key)
253261

@@ -455,8 +463,13 @@ async def session_handler(
455463
await loop.run_in_executor(None, session.commit)
456464
else:
457465
await loop.run_in_executor(None, session.rollback)
466+
except Exception: # noqa: BLE001
467+
logger.debug("Session commit/rollback failed during cleanup", exc_info=True)
458468
finally:
459-
await loop.run_in_executor(None, session.close)
469+
try:
470+
await loop.run_in_executor(None, session.close)
471+
except Exception: # noqa: BLE001
472+
logger.debug("Session close failed during cleanup", exc_info=True)
460473
with contextlib.suppress(AttributeError, KeyError):
461474
delattr(request.ctx, self.session_key)
462475

0 commit comments

Comments
 (0)