Skip to content

Commit 871419b

Browse files
cruseakshaydominikhei
authored andcommitted
fix: fab deserialize issue (apache#62153)
1 parent a9eadfa commit 871419b

2 files changed

Lines changed: 70 additions & 1 deletion

File tree

providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
from __future__ import annotations
1919

2020
import warnings
21+
from contextlib import suppress
2122
from functools import cached_property
2223
from pathlib import Path
2324
from typing import TYPE_CHECKING, Any
@@ -30,7 +31,7 @@
3031
from flask import Blueprint, current_app, g
3132
from flask_appbuilder.const import AUTH_LDAP
3233
from sqlalchemy import select
33-
from sqlalchemy.exc import NoResultFound
34+
from sqlalchemy.exc import NoResultFound, SQLAlchemyError
3435
from sqlalchemy.orm import Session, joinedload
3536

3637
from airflow.api_fastapi.app import AUTH_MANAGER_FASTAPI_APP_PREFIX
@@ -290,6 +291,12 @@ def deserialize_user(self, token: dict[str, Any]) -> User:
290291
return self.session.scalars(select(User).where(User.id == int(token["sub"]))).one()
291292
except NoResultFound:
292293
raise ValueError(f"User with id {token['sub']} not found")
294+
except SQLAlchemyError:
295+
# Discard the poisoned scoped session so the next request gets a
296+
# fresh connection from the pool instead of a PendingRollbackError.
297+
with suppress(Exception):
298+
self.session.remove()
299+
raise
293300

294301
def serialize_user(self, user: User) -> dict[str, Any]:
295302
return {"sub": str(user.id)}

providers/fab/tests/unit/fab/auth_manager/test_fab_auth_manager.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import pytest
2727
from flask import g
2828
from flask_appbuilder.const import AUTH_DB, AUTH_LDAP
29+
from sqlalchemy.exc import OperationalError, PendingRollbackError
2930

3031
from airflow.api_fastapi.app import AUTH_MANAGER_FASTAPI_APP_PREFIX
3132
from airflow.api_fastapi.common.types import MenuItem
@@ -959,6 +960,67 @@ def test_resetdb(
959960
mock_init.assert_called_once()
960961

961962

963+
@pytest.mark.db_test
964+
class TestDeserializeUserSessionCleanup:
965+
"""Test that deserialize_user cleans up the FAB scoped session on database errors.
966+
967+
Problem:
968+
When the database connection drops (e.g., PostgreSQL's
969+
``idle_in_transaction_session_timeout`` fires), the underlying connection
970+
becomes invalid. SQLAlchemy raises ``OperationalError`` on the first request
971+
that hits the dead connection. The scoped session then enters an invalid
972+
state. Any subsequent request that reuses the same thread-local session
973+
raises ``PendingRollbackError`` — permanently breaking the API server until
974+
it is restarted.
975+
"""
976+
977+
@staticmethod
978+
def _patched_session(auth_manager, mock_session):
979+
"""Replace the ``session`` property on *auth_manager* with *mock_session*."""
980+
return mock.patch.object(
981+
type(auth_manager), "session", new_callable=mock.PropertyMock, return_value=mock_session
982+
)
983+
984+
@pytest.mark.parametrize(
985+
"raised_exc",
986+
[
987+
OperationalError("server closed the connection unexpectedly", None, Exception()),
988+
PendingRollbackError(
989+
"Can't reconnect until invalid transaction is rolled back. "
990+
"Please rollback() fully before proceeding"
991+
),
992+
],
993+
ids=["operational_error", "pending_rollback_error"],
994+
)
995+
def test_db_error_calls_session_remove(self, auth_manager_with_appbuilder, raised_exc):
996+
"""session.remove() is called on SQLAlchemy errors so the next request recovers."""
997+
mock_session = MagicMock(spec=["scalars", "remove"])
998+
mock_session.scalars.side_effect = raised_exc
999+
auth_manager_with_appbuilder.cache.pop(99997, None)
1000+
1001+
with self._patched_session(auth_manager_with_appbuilder, mock_session):
1002+
with pytest.raises(type(raised_exc)):
1003+
auth_manager_with_appbuilder.deserialize_user({"sub": "99997"})
1004+
1005+
mock_session.remove.assert_called_once()
1006+
1007+
def test_db_error_propagates_when_session_remove_raises(self, auth_manager_with_appbuilder):
1008+
"""The original SQLAlchemyError propagates even if session.remove() itself raises."""
1009+
# Arrange — session.scalars raises the original DB error;
1010+
# session.remove raises a secondary error that must be suppressed.
1011+
original_exc = OperationalError("connection dropped", None, Exception())
1012+
mock_session = MagicMock(spec=["scalars", "remove"])
1013+
mock_session.scalars.side_effect = original_exc
1014+
mock_session.remove.side_effect = AttributeError("appbuilder gone")
1015+
auth_manager_with_appbuilder.cache.pop(99997, None)
1016+
1017+
with self._patched_session(auth_manager_with_appbuilder, mock_session):
1018+
with pytest.raises(OperationalError):
1019+
auth_manager_with_appbuilder.deserialize_user({"sub": "99997"})
1020+
1021+
mock_session.remove.assert_called_once()
1022+
1023+
9621024
class TestFabAuthManagerSessionCleanup:
9631025
"""Test session cleanup middleware in FAB auth manager FastAPI app.
9641026

0 commit comments

Comments
 (0)