Re: [PR] Fix: handle cross-event-loop RuntimeError in ExecutionAPISecretsBackend [airflow]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #64238:
URL: https://github.com/apache/airflow/pull/64238#discussion_r3025329717


##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -66,25 +66,26 @@ def get_connection(self, conn_id: str, team_name: str | None = None) -> Connecti
             # Convert ExecutionAPI response to SDK Connection
             return _process_connection_result_conn(msg)
         except RuntimeError as e:
-            # TriggerCommsDecoder.send() uses async_to_sync internally, which raises RuntimeError
-            # when called within an async event loop. In greenback portal contexts (triggerer),
-            # we catch this and use greenback to call the async version instead.
-            if str(e).startswith("You cannot use AsyncToSync in the same thread as an async event loop"):
-                import asyncio
+            import asyncio
+            import greenback
 
-                import greenback
+            msg = str(e)
 
+            if (
+                "You cannot use AsyncToSync in the same thread as an async event loop" in msg
+                or "attached to a different loop" in msg
+            ):
                 task = asyncio.current_task()
-                if greenback.has_portal(task):
+                if task and greenback.has_portal(task):
                     import warnings
 
                     warnings.warn(
-                        "You should not use sync calls here -- use `await aget_connection` instead",
+                        "Sync connection access failed due to event loop mismatch. "
+                        "Falling back to async connection retrieval.",

Review Comment:
   The new warning message explains the fallback, but it no longer tells callers what to do instead (previously it suggested using `await aget_connection`). Consider including that actionable guidance so users hitting this in triggerer/async contexts know how to avoid the sync call entirely.
   ```suggestion
                           "If you are running in an async or triggerer context, use "
                           "`await aget_connection(conn_id)` instead of `get_connection` "
                           "to avoid this sync call. Falling back to async connection retrieval.",
   ```



##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -66,25 +66,26 @@ def get_connection(self, conn_id: str, team_name: str | None = None) -> Connecti
             # Convert ExecutionAPI response to SDK Connection
             return _process_connection_result_conn(msg)
         except RuntimeError as e:
-            # TriggerCommsDecoder.send() uses async_to_sync internally, which raises RuntimeError
-            # when called within an async event loop. In greenback portal contexts (triggerer),
-            # we catch this and use greenback to call the async version instead.
-            if str(e).startswith("You cannot use AsyncToSync in the same thread as an async event loop"):
-                import asyncio
+            import asyncio
+            import greenback
 
-                import greenback
+            msg = str(e)
 
+            if (
+                "You cannot use AsyncToSync in the same thread as an async event loop" in msg
+                or "attached to a different loop" in msg
+            ):

Review Comment:
   This change introduces a second RuntimeError message trigger ("attached to a different loop"), but there is no regression test covering that new branch. Please add a test analogous to `test_runtime_error_triggers_greenback_fallback` that raises the cross-loop RuntimeError and asserts the greenback fallback path is used.



##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -66,25 +66,26 @@ def get_connection(self, conn_id: str, team_name: str | None = None) -> Connecti
             # Convert ExecutionAPI response to SDK Connection
             return _process_connection_result_conn(msg)
         except RuntimeError as e:
-            # TriggerCommsDecoder.send() uses async_to_sync internally, which raises RuntimeError
-            # when called within an async event loop. In greenback portal contexts (triggerer),
-            # we catch this and use greenback to call the async version instead.
-            if str(e).startswith("You cannot use AsyncToSync in the same thread as an async event loop"):
-                import asyncio
+            import asyncio
+            import greenback
 

Review Comment:
   `asyncio`/`greenback` are imported for every `RuntimeError`, even when the message doesn't match the event-loop cases. This adds unnecessary work in the common path for unrelated runtime errors and also keeps imports inside a method body. Consider moving these imports to module scope, or at least deferring them until after the message check so they only happen when the greenback fallback branch is actually taken.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org