Copilot commented on code in PR #65235:
URL: https://github.com/apache/airflow/pull/65235#discussion_r3081147391
##########
airflow-core/tests/conftest.py:
##########
@@ -161,3 +161,13 @@ def requests_mock() -> RequestsMockFixture: ...
# time-machine
@pytest.fixture # type: ignore[no-redef]
def time_machine() -> TimeMachineFixture: ...
+
+
[email protected](autouse=True)
+def _clear_in_process_api_cache():
+ """Clear the cached InProcessExecutionAPI after each test to prevent state
leakage."""
+ yield
+ # Local import required: top-level conftest.py asserts no airflow modules
are imported at load time.
+ from airflow.sdk.execution_time.supervisor import in_process_api_server
+
+ in_process_api_server.cache_clear()
Review Comment:
This autouse fixture unconditionally imports
`airflow.sdk.execution_time.supervisor` at teardown for every test, which can
introduce unnecessary import overhead and side effects for tests that never
touch the in-process API server. Consider avoiding the import by checking
`sys.modules` for `airflow.sdk.execution_time.supervisor` and only calling
`in_process_api_server.cache_clear()` if the module is already loaded.
```suggestion
supervisor_module =
sys.modules.get("airflow.sdk.execution_time.supervisor")
if supervisor_module is not None:
supervisor_module.in_process_api_server.cache_clear()
```
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -1692,15 +1694,18 @@ def start( # type: ignore[override]
@staticmethod
def _api_client(dag=None):
api = in_process_api_server()
+ from airflow.api_fastapi.common.dagbag import dag_bag_from_app
+
if dag is not None:
- from airflow.api_fastapi.common.dagbag import dag_bag_from_app
from airflow.models.dagbag import DBDagBag
# This is needed since the Execution API server uses the DBDagBag
in its "state".
# This `app.state.dag_bag` is used to get some Dag properties like
`fail_fast`.
dag_bag = DBDagBag()
api.app.dependency_overrides[dag_bag_from_app] = lambda: dag_bag
+ else:
+ api.app.dependency_overrides.pop(dag_bag_from_app, None)
Review Comment:
The new behavior that clears `dag_bag_from_app` from
`api.app.dependency_overrides` when `dag` is `None` isn’t covered by tests
here. Please add a regression test that sets the override via
`_api_client(dag=...)`, then calls `_api_client(dag=None)` and asserts the
override was removed, to ensure stale overrides don’t leak into trigger runs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
