henry3260 commented on PR #65587: URL: https://github.com/apache/airflow/pull/65587#issuecomment-5194266538
> Have you been able to run the repro from #65482 against this branch? I applied it locally and the reported scenario still hangs, so I don't think this closes that issue. > > What I ran: the script from the issue, with `system_site_packages=True` and no PyPI requirements so the venv resolves to local sources, the variable stored only in the metastore, under `dag.test()`. > > On main the run hangs, and the log shows `Using Variable.get from 'airflow.models' is deprecated` coming from `airflow-core/src/airflow/models/variable.py:170` with a `correlation_id` attached, so the Execution API handler did take the Task SDK path. That part matches your diagnosis. > > With this branch applied the warning is gone and the handler resolves through the metastore, so `should_use_task_sdk_api_path()` does what you intend. The run still hangs. A faulthandler dump inside the venv child shows it parked here: > > ``` > File ".../sdk/execution_time/comms.py", line 332 in _read_frame > File ".../sdk/execution_time/comms.py", line 371 in _get_response > File ".../sdk/execution_time/comms.py", line 275 in send > File ".../sdk/execution_time/secrets/execution_api.py", line 119 in get_variable > ``` > > The child never gets a response, because `InProcessTestSupervisor.send_msg` (`task-sdk/src/airflow/sdk/execution_time/supervisor.py`, line 2133 on main) appends to the in-process deque rather than writing a response frame to the socket that `_setup_subprocess_socket()` handed the child: > > ```python > def send_msg(self, msg, request_id, error=None, **dump_opts): > """Override to use in-process comms.""" > self.comms.messages.append(msg) > ``` > > `_handle_request` answers every request through `send_msg` (`supervisor.py:1918`), and the only thing writing to that socket directly is `_send_new_log_fd`. That's why the logging-FD handshake added in #57212 works from a venv task while `GetVariable` and `GetConnection` never come back. > > If I route responses for socket-delivered requests back onto the socket, the repro passes on plain main, without this branch. Applying this branch on top produces the same value, minus one round trip through the SDK client and the deprecation warning. In the in-process path the nested call is already covered by `set_supervisor_comms(None)` in `InProcessSupervisorComms.send`, which is why the detour still resolves to the right value today. > > So as far as I can tell this is a robustness fix on that path rather than the fix for #65482. Thanks for the detailed diagnosis, @kaxil — you're right on all counts. I spent time reading through the code to confirm the root cause you pointed out, and I've reworked the PR around it. Summary of what changed in `0a6a3ad`: - **Root cause fix:** the real hang was `InProcessTestSupervisor.send_msg` appending responses to the in-process deque instead of writing a response frame back to the socket the subprocess reads from. The subprocess-spawning operators (`PythonVirtualenvOperator` / `ExternalPythonOperator`) now get their responses routed back onto the socket, so `GetVariable`/`GetConnection` actually return. - **Env var precedence:** dropped the env-var arm so the server short-circuit reads `_PROCESS_CONTEXT_OVERRIDE` only, leaving `SUPERVISOR_COMMS` winning over the env var. This keeps parity with `ensure_secrets_backend_loaded()` and avoids the `dag.test()` metastore fallback / lost `AirflowSecretsBackendAccessDenied` enforcement. - **`request_scoped_server_context`:** made it the default so the parameter goes away, which also covers the `triggerer_job_runner.in_process_api_server()` instance. - **`lru_cache` singleton:** no longer keying the cache on the new kwarg, so the singleton stays a singleton. - **Test coverage:** added a test that drives a request through the in-process path with `SUPERVISOR_COMMS` present and asserts the SDK path isn't re-entered, covering the actual #65482 scenario. PTAL when you have a moment, thanks! -- 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]
