You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "gang-zh (via GitHub)" <gi...@apache.org> on 2026/08/27 17:22:56 UTC
[PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
gang-zh opened a new pull request, #72164:
URL: https://github.com/apache/airflow/pull/72164
Resolves #71707. Supersedes #71723 — same approach and platform gate, credited to @pulaksaha143 as co-author; that PR has been idle since Aug 17 with static checks failing, so this picks it up and finishes it.
## Why
On Linux the supervisor bare-forks the task process. If any supervisor thread (OTel exporter, google-auth refresh, …) holds a C-level lock like OpenSSL's at the instant of the fork, the child inherits it permanently locked and deadlocks at its first TLS call — typically the DAG bundle download — stuck in RUNNING forever. `execution_timeout` never fires because the hang precedes `execute()` (details in #71707).
`exec()`-ing a fresh interpreter right after fork — what `_FORK_EXEC_PLATFORMS` already forces on macOS — makes inherited-lock state unreachable. This extends the existing `[core] execute_tasks_new_python_interpreter` config to opt into that path on any platform.
It stays opt-in because the fresh interpreter re-imports the SDK and plugins: we measured **~+9s per task** on a plugin-heavy production image (it lands in task duration, not queued duration — the TI is marked running before the new interpreter boots).
## Production validation
We've run this exact behavior (image-patched) on Airflow 3.2.2 / KubernetesExecutor in production: a TLS-stress probe deadlocked **16 of 13,930 bare-forked children** (`pthread_rwlock_wrlock` under `SSL_CTX_new_ex`) vs **0 of 5,195 with exec**; four production deployments since, thousands of task starts, zero hangs, deferral resume / retries / XCom / log shipping all normal. Full data: https://github.com/apache/airflow/pull/71723#issuecomment-5432889600
## Changes vs #71723
- Test patches the config through `AIRFLOW__CORE__EXECUTE_TASKS_NEW_PYTHON_INTERPRETER` so it reaches the **task-sdk** conf object the supervisor reads (`conf_vars` patches airflow-core's conf, which the supervisor never consults) — and imports at top level per review feedback there.
- Static checks pass (ruff format + lint clean).
- `config.yml` description documents the new effect of the flag.
A backport to v3-2/v3-3 would be very welcome — every KubernetesExecutor user on those lines forks cold per task.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3973082952
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -223,6 +223,13 @@ core:
* ``False``: Execute via forking of the parent process
* ``True``: Spawning a new python process, slower than fork, but means plugin changes picked
up by tasks straight away
+
+ On Airflow 3 this applies to the task process only. When ``True`` the supervisor ``exec``s
Review Comment:
Fixed in f6f4740 — reworded so no literal is followed by a letter ("starts the task in a fresh interpreter with `exec`"), and present tense for the reference page.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4363,6 +4363,32 @@ def _drop_root_if_needed():
os.setuid(_NOBODY_UID)
+@pytest.mark.skipif(sys.platform != "linux", reason="PR_SET_DUMPABLE is Linux-only")
+def test_exec_child_reapplies_nondumpable():
+ """execve resets PR_SET_DUMPABLE to 1; the exec'd child must set it to 0 again."""
+ probe = (
+ "import ctypes\n"
+ "from airflow.sdk.execution_time.supervisor import _PR_GET_DUMPABLE, _make_process_nondumpable\n"
+ "libc = ctypes.CDLL(None, use_errno=True)\n"
+ "after_exec = libc.prctl(_PR_GET_DUMPABLE, 0, 0, 0, 0)\n"
+ "_make_process_nondumpable()\n"
Review Comment:
Done in faa7ce7: the prelude is now a module constant (`_CHILD_EXEC_PRELUDE`, part of `_CHILD_EXEC_BOOTSTRAP`) and the Linux test execs the real string; a `TestChildExecMain` spy asserts `_make_process_nondumpable()` runs before `_fork_main`; the forked branch exits through `finally: os._exit(1)`.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4479,6 +4505,25 @@ def test_api_client_clears_dag_bag_override_when_dag_is_none():
in_process_api_server.cache_clear()
+class TestTaskProcessUsesExec:
+ """The config opt-in for fork+exec of the task process where the platform does not force it."""
+
+ @pytest.mark.parametrize(
+ ("platform", "config_value", "expected"),
+ [
+ ("darwin", None, True),
+ ("darwin", "False", True),
+ ("linux", None, False),
+ ("linux", "False", False),
+ ("linux", "True", True),
+ ],
+ )
+ def test_task_process_uses_exec(self, monkeypatch, platform, config_value, expected):
+ monkeypatch.setattr(supervisor.sys, "platform", platform)
+ with conf_vars({("core", "execute_tasks_new_python_interpreter"): config_value}):
+ assert supervisor._task_process_uses_exec() is expected
Review Comment:
Added in faa7ce7: `test_activity_start_opts_into_fork_exec`, same shape as the processor/triggerer tests (patched gate, mocked `WatchedSubprocess.start`, asserts `use_exec`; stub target expects `False`).
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -533,13 +549,16 @@ def _resolve_child_target(dotted: str) -> Callable[[], None]:
def _child_exec_main():
"""
- Entry point for the child process when using fork+exec (macOS).
+ Entry point for the child process when using fork+exec.
After exec, FDs 0/1/2/3 are the requests/stdout/stderr/log sockets the parent
placed there via dup2. The target to run is named in ``_AIRFLOW_CHILD_TARGET``
(``module:qualname``); it is rehydrated and handed to :func:`_fork_main`, which
sets up the structured log channel from FD 3 exactly as the bare-fork path does.
"""
+ # execve resets PR_SET_DUMPABLE to 1, so re-apply what supervise_task() set before the
+ # fork; otherwise a same-UID sibling could read this child's /proc/<pid>/environ.
+ _make_process_nondumpable()
Review Comment:
Good point — faa7ce7 moves the prctl into the `-c` bootstrap via ctypes, before the Airflow import, so the window is interpreter start only; this call stays as the logged fallback. Both security docs now describe it that way and name `ptrace_scope >= 1` for the remainder (f6f4740).
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,21 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker read it). When set to
+``True``, the task supervisor now ``exec``\ s a fresh interpreter right after forking the task process,
+which prevents the fork from inheriting a lock held by a supervisor thread (a permanent hang at the
+task's first TLS call). Deployments that kept the option ``True`` from Airflow 2 get this behaviour,
+and its per-task interpreter start-up cost, on upgrade without further action; set it to ``False`` to
+keep bare fork. Edge workers with the option ``True`` already start a fresh interpreter for the
+supervisor and will now start a second one for the task. The Dag processor and triggerer are not
+affected.
Review Comment:
Went with saying it: f6f4740 notes in the newsfragment and the config text that task callbacks (closure target, cannot be named for an exec'd child) keep bare fork. An importable callback entry point feels like its own follow-up; happy to open an issue for it.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -517,6 +517,22 @@ def _should_use_exec() -> bool:
return sys.platform in _FORK_EXEC_PLATFORMS
+def _task_process_uses_exec() -> bool:
+ """
+ Whether the task process should ``exec`` a fresh interpreter after the fork.
+
+ Forced where bare fork is unsafe (macOS); elsewhere a deployment opts in with
+ ``[core] execute_tasks_new_python_interpreter``. exec replaces the child's address
+ space, so it cannot inherit a lock a supervisor thread held at fork time (e.g.
+ OpenSSL's, which otherwise hangs the task at its first TLS call; #71707). Only the
+ task process reads the option: the Dag processor and triggerer fork far more often
+ and keep the platform gate alone.
Review Comment:
Right — reworded in faa7ce7 to lean on the option having always described task execution (with the per-parse fork rate noted for the Dag processor only), and the `use_exec` docstring and child comment no longer say macOS-only.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3972714813
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -223,6 +223,13 @@ core:
* ``False``: Execute via forking of the parent process
* ``True``: Spawning a new python process, slower than fork, but means plugin changes picked
up by tasks straight away
+
+ On Airflow 3 this applies to the task process only. When ``True`` the supervisor ``exec``s
Review Comment:
The closing backticks in ``exec``s are followed by a letter, so rst does not treat them as an end-string and the literal runs on to the ``True`` on line 232: the Configuration Reference renders everything from `exec` to `True` as one monospace block, and docutils raises no warning (the `--docs-only` job passed on b653700 with this text). The newsfragment already has the fix, ``` ``exec``\ s ```. Small wording thing while here: 'will now start a second one' is release-note tense for a permanent reference page; 'starts a second one' reads right there.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4363,6 +4363,32 @@ def _drop_root_if_needed():
os.setuid(_NOBODY_UID)
+@pytest.mark.skipif(sys.platform != "linux", reason="PR_SET_DUMPABLE is Linux-only")
+def test_exec_child_reapplies_nondumpable():
+ """execve resets PR_SET_DUMPABLE to 1; the exec'd child must set it to 0 again."""
+ probe = (
+ "import ctypes\n"
+ "from airflow.sdk.execution_time.supervisor import _PR_GET_DUMPABLE, _make_process_nondumpable\n"
+ "libc = ctypes.CDLL(None, use_errno=True)\n"
+ "after_exec = libc.prctl(_PR_GET_DUMPABLE, 0, 0, 0, 0)\n"
+ "_make_process_nondumpable()\n"
Review Comment:
This pins execve resetting the flag and the helper restoring it, but the probe calls `_make_process_nondumpable()` itself, so nothing checks that `_child_exec_main` does: deleting supervisor.py:561 leaves the suite green. Exec'ing the real bootstrap string (factored into a module constant) or a `monkeypatch` spy on `_make_process_nondumpable` in `TestChildExecMain`, asserted to run before `_fork_main`, would pin the wiring. The forked branch also wants a `try/finally: os._exit(...)` like `test_nondumpable_blocks_child_memory_read`, or a failing `execv` runs the rest of pytest in the child while the parent blocks on the pipe.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -533,13 +549,16 @@ def _resolve_child_target(dotted: str) -> Callable[[], None]:
def _child_exec_main():
"""
- Entry point for the child process when using fork+exec (macOS).
+ Entry point for the child process when using fork+exec.
After exec, FDs 0/1/2/3 are the requests/stdout/stderr/log sockets the parent
placed there via dup2. The target to run is named in ``_AIRFLOW_CHILD_TARGET``
(``module:qualname``); it is rehydrated and handed to :func:`_fork_main`, which
sets up the structured log channel from FD 3 exactly as the bare-fork path does.
"""
+ # execve resets PR_SET_DUMPABLE to 1, so re-apply what supervise_task() set before the
+ # fork; otherwise a same-UID sibling could read this child's /proc/<pid>/environ.
+ _make_process_nondumpable()
Review Comment:
This restores the flag for the task's lifetime, but the child is dumpable from `execve` until this line runs, which is interpreter start plus `import airflow.sdk.execution_time.supervisor` (about a second on a dev checkout here, more with plugins), and a `/proc/<pid>/mem` fd or `PTRACE_ATTACH` taken in that window survives the prctl because the kernel checks access once, at open or attach. It needs same-UID co-tenancy and `kernel.yama.ptrace_scope=0`, so not blocking, but running the prctl in the `-c` bootstrap before the Airflow import shrinks the window to interpreter start at no cost, with this call kept as the logged fallback. The two security docs would then describe it that way rather than 'as its first step', naming `ptrace_scope>=1` as what covers the remainder.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4479,6 +4505,25 @@ def test_api_client_clears_dag_bag_override_when_dag_is_none():
in_process_api_server.cache_clear()
+class TestTaskProcessUsesExec:
+ """The config opt-in for fork+exec of the task process where the platform does not force it."""
+
+ @pytest.mark.parametrize(
+ ("platform", "config_value", "expected"),
+ [
+ ("darwin", None, True),
+ ("darwin", "False", True),
+ ("linux", None, False),
+ ("linux", "False", False),
+ ("linux", "True", True),
+ ],
+ )
+ def test_task_process_uses_exec(self, monkeypatch, platform, config_value, expected):
+ monkeypatch.setattr(supervisor.sys, "platform", platform)
+ with conf_vars({("core", "execute_tasks_new_python_interpreter"): config_value}):
+ assert supervisor._task_process_uses_exec() is expected
Review Comment:
This binds the helper, but nothing asserts `ActivitySubprocess.start` passes its result as `use_exec`, so reverting line 1437 to `_should_use_exec()` also stays green. The Dag processor and triggerer each have a `test_start_opts_into_fork_exec` (`test_processor.py:493`, `test_triggerer_job.py:288`) that patches the gate, mocks `WatchedSubprocess.start` and asserts `call_args.kwargs["use_exec"]`; a mirror here, including the stub-target case expecting `False`, would close it.
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,21 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker read it). When set to
+``True``, the task supervisor now ``exec``\ s a fresh interpreter right after forking the task process,
+which prevents the fork from inheriting a lock held by a supervisor thread (a permanent hang at the
+task's first TLS call). Deployments that kept the option ``True`` from Airflow 2 get this behaviour,
+and its per-task interpreter start-up cost, on upgrade without further action; set it to ``False`` to
+keep bare fork. Edge workers with the option ``True`` already start a fresh interpreter for the
+supervisor and will now start a second one for the task. The Dag processor and triggerer are not
+affected.
Review Comment:
Callbacks are the other thing these workers fork: `ExecuteCallback` goes `run_workload` to `supervise_callback` to `CallbackSubprocess.start`, whose closure target can never take the exec path (`WatchedSubprocess.start` rejects closures under `use_exec`), so an `on_failure_callback` that does TLS keeps the inherited-lock exposure after an operator flips this option to cure hangs. Tasks-only is the agreed scope; I'm not sure whether this line (and the config text) should say callbacks stay on bare fork, or whether a follow-up for an importable callback entry point is the better home.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -517,6 +517,22 @@ def _should_use_exec() -> bool:
return sys.platform in _FORK_EXEC_PLATFORMS
+def _task_process_uses_exec() -> bool:
+ """
+ Whether the task process should ``exec`` a fresh interpreter after the fork.
+
+ Forced where bare fork is unsafe (macOS); elsewhere a deployment opts in with
+ ``[core] execute_tasks_new_python_interpreter``. exec replaces the child's address
+ space, so it cannot inherit a lock a supervisor thread held at fork time (e.g.
+ OpenSSL's, which otherwise hangs the task at its first TLS call; #71707). Only the
+ task process reads the option: the Dag processor and triggerer fork far more often
+ and keep the platform gate alone.
Review Comment:
`TriggerRunnerSupervisor.start` runs once per triggerer job (`triggerer_job_runner.py:271`), so 'fork far more often' only holds for the Dag processor; the reason that covers both is that the option has always described task execution. The base `start()` docstring for `use_exec` (line 712) and the child comment at 764 also still say macOS only, though they now describe the Linux opt-in path too.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3963303760
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
Review Comment:
`_should_use_exec()` is also what `DagFileProcessorProcess.start()` and `TriggerRunnerSupervisor.start()` consult, so on Linux this flag now also execs a fresh interpreter per Dag file per parse loop (skipping `_pre_import_airflow_modules`) and once for the triggerer runner, while the title, this docstring and the config text only describe the task process. Is the Dag processor part intended? The same boot you measured at ~9s per task lands in the parse loop, counts against `dag_file_processor_timeout`, and the Helm chart's `config:` renders into the one `airflow.cfg` every component mounts, so scoping it to workers means a per-component `env:` override instead. If tasks are the intent, moving the conf read to `ActivitySubprocess.start()` (next to the `target is _subprocess_main` check) keeps this as the platform gate and matches what a v3-2/v3-3 backport can carry anyway, since those branches have no `_should_use_exec()` and check darwin inline there. If all three are intended, t
he config description needs to say so. (slice-soupam's latest comment on #71707 makes the fork-rate case for splitting these.)
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4479,6 +4479,31 @@ def test_api_client_clears_dag_bag_override_when_dag_is_none():
in_process_api_server.cache_clear()
+class TestShouldUseExec:
+ """The config opt-in for fork+exec on platforms where it is not forced on."""
+
+ @pytest.mark.parametrize(
+ ("platform", "config_value", "expected"),
+ [
+ ("darwin", None, True),
+ ("darwin", "False", True),
+ ("linux", None, False),
+ ("linux", "False", False),
+ ("linux", "True", True),
+ ],
+ )
+ def test_should_use_exec(self, monkeypatch, platform, config_value, expected):
+ monkeypatch.setattr(supervisor.sys, "platform", platform)
+ # The supervisor reads the task-sdk conf; the env var reaches it regardless
Review Comment:
`conf_vars` already patches the SDK conf too (it picks up `airflow.sdk.configuration.conf` whenever that module is in `sys.modules`, which it is once `supervisor` is imported), so the PR-body rationale for going through the env var doesn't hold, and `with conf_vars({("core", "execute_tasks_new_python_interpreter"): config_value}):` would match the rest of this file. The env var route works as well, since env is first in the lookup order, so this is only about consistency.
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -223,6 +223,10 @@ core:
* ``False``: Execute via forking of the parent process
* ``True``: Spawning a new python process, slower than fork, but means plugin changes picked
up by tasks straight away
+
+ On workers this also makes the task process ``exec`` a fresh interpreter right after the
Review Comment:
Two things this paragraph should cover. The option has been a no-op for tasks on Airflow 3 until now (Celery's reader is behind `if not AIRFLOW_V_3_0_PLUS`, `settings.py` only has a removed-attribute shim, and the Edge worker is the one component that reads it), so anyone who kept it `True` from Airflow 2 gets the exec path and its per-task cost on upgrade with no action; a `72164.significant.rst` newsfragment would flag that. And the Edge worker already `Popen`s a fresh interpreter to run the supervisor when this is `True` (`providers/edge3/.../cli/worker.py`, `_launch_job`), so those deployments will now boot two interpreters per task. Both seem acceptable, but they should be written down here rather than discovered in task duration.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
+
+ Always on for platforms where bare fork is unsafe (macOS). Elsewhere it can be
+ opted into with ``[core] execute_tasks_new_python_interpreter``: exec replaces
+ the child's address space, so it cannot inherit a lock a supervisor thread held
+ at fork time (e.g. OpenSSL's, which otherwise deadlocks the task at its first
+ TLS call — see #71707).
+ """
+ if sys.platform in _FORK_EXEC_PLATFORMS:
+ return True
+ return conf.getboolean("core", "execute_tasks_new_python_interpreter", fallback=False)
Review Comment:
Turning this on for Linux drops the memory protection #62523 added. `supervise_task()` calls `_make_process_nondumpable()` (`prctl(PR_SET_DUMPABLE, 0)`) in the supervisor and relies on `fork()` inheriting the flag, but `execve` gives the child a fresh mm with dumpable reset to 1, and `_child_exec_main()` never re-applies it. Reproduced in a `python:3.12-slim` container as uid 65534: supervisor 0, bare-fork child 0, exec'd child 1, and a same-UID sibling could read the exec'd child's `/proc/<pid>/environ` and `maps` while the bare-fork child's raised `PermissionError`. On Celery, LocalExecutor and Edge workers, where several tasks share one UID, that means task A's user code can read task B's environment (the supervisor's env at exec time) and, where `ptrace_scope` allows, its memory. On macOS this never showed because the prctl is a no-op there. Calling `_make_process_nondumpable()` first thing in `_child_exec_main()` closes it for both paths; `security/workload.rst` and `security
_model.rst` ("this flag is inherited by the forked child") need a matching edit, and a Linux-only test beside `test_nondumpable_blocks_sibling_proc_read` that fork+execs and asserts `PR_GET_DUMPABLE == 0` would pin it.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on PR #72164:
URL: https://github.com/apache/airflow/pull/72164#issuecomment-5668678996
Thanks! Both nits in f2aedc37dc (imports hoisted to module level). Opened #73143 for the importable callback entry point. The failing `Compat 2.11.1:P3.10` job stalled in "Prepare provider distributions: wheel" for 46 min and was killed before any test ran (no log blob); the same job passed on the previous head, so the push should clear it.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r4009428819
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,23 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker read it). When set to
Review Comment:
```suggestion
On Airflow >=3.0,<=3.3 the option had no effect on task execution (only the Edge worker read it). When set to
```
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,23 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker read it). When set to
+``True``, the task supervisor now ``exec``\ s a fresh interpreter right after forking the task process,
+which prevents the fork from inheriting a lock held by a supervisor thread (a permanent hang at the
+task's first TLS call). Deployments that kept the option ``True`` from Airflow 2 get this behaviour,
+and its per-task interpreter start-up cost, on upgrade without further action; set it to ``False`` to
+keep bare fork. The task process reads the global value, so a team-scoped Edge setting alone does not
+turn it on. Edge workers with the option ``True`` already start a fresh interpreter for the supervisor
Review Comment:
Is this for the Edge provider?
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,23 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker read it). When set to
+``True``, the task supervisor now ``exec``\ s a fresh interpreter right after forking the task process,
+which prevents the fork from inheriting a lock held by a supervisor thread (a permanent hang at the
+task's first TLS call). Deployments that kept the option ``True`` from Airflow 2 get this behaviour,
+and its per-task interpreter start-up cost, on upgrade without further action; set it to ``False`` to
+keep bare fork. The task process reads the global value, so a team-scoped Edge setting alone does not
Review Comment:
```suggestion
keep bare fork. The task process reads the global value, so a team-scoped setting alone does not
```
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on PR #72164:
URL: https://github.com/apache/airflow/pull/72164#issuecomment-5605835689
@kaxil all four points addressed in 9711663 / 0fe8931 / b653700 (dumpable flag re-applied in the exec'd child, option scoped to the task process, config text + significant newsfragment, test via `conf_vars`); PR body corrected accordingly.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3981941402
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -531,15 +547,37 @@ def _resolve_child_target(dotted: str) -> Callable[[], None]:
return pkgutil.resolve_name(dotted)
+# Runs in the exec'd child before anything else. execve reset PR_SET_DUMPABLE (4 in
+# <linux/prctl.h>); restore it before the Airflow import so the window in which a same-UID
+# sibling can open /proc/<pid>/mem or ptrace-attach is interpreter start only (a descriptor
+# or attach taken in that window survives a later prctl -- the kernel checks once, at open).
+# _child_exec_main() repeats the call as the logged fallback.
+_CHILD_EXEC_PRELUDE = """\
+import sys
+if sys.platform == "linux":
+ import ctypes
Review Comment:
Fixed in 3f40f69749 — the import is inside the `try` now, matching `_make_process_nondumpable()`; `test_prelude_survives_an_interpreter_without_ctypes` runs the prelude with `sys.modules['_ctypes'] = None`.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -531,15 +547,37 @@ def _resolve_child_target(dotted: str) -> Callable[[], None]:
return pkgutil.resolve_name(dotted)
+# Runs in the exec'd child before anything else. execve reset PR_SET_DUMPABLE (4 in
+# <linux/prctl.h>); restore it before the Airflow import so the window in which a same-UID
+# sibling can open /proc/<pid>/mem or ptrace-attach is interpreter start only (a descriptor
+# or attach taken in that window survives a later prctl -- the kernel checks once, at open).
+# _child_exec_main() repeats the call as the logged fallback.
+_CHILD_EXEC_PRELUDE = """\
+import sys
+if sys.platform == "linux":
+ import ctypes
+ try:
+ ctypes.CDLL(None, use_errno=True).prctl(4, 0, 0, 0, 0)
+ except Exception:
+ pass
+"""
+_CHILD_EXEC_BOOTSTRAP = _CHILD_EXEC_PRELUDE + (
Review Comment:
Added in 3f40f69749: `test_fork_exec_bootstrap_runs_an_importable_target_end_to_end` drives `ActivitySubprocess.start` through the real `os.execv` — fresh interpreter runs `_CHILD_EXEC_BOOTSTRAP`, FDs 0-3 rebuilt, target rehydrated by name, stdout asserted through the supervisor — plus the `startswith(_CHILD_EXEC_PRELUDE)` floor. It uses an importable probe standing in for `_subprocess_main` instead of a `test_run_simple_dag` row: the suite stubs `_get_plugins` in-process (task-sdk conftest), which a bare-forked child inherits and a fresh interpreter does not, so the exec'd task runner fails with 'Plugins folder is not set' (reproduced outside pytest in both fork modes; it is also why `test_run_simple_dag` fails on macOS today).
##########
airflow-core/docs/security/workload.rst:
##########
@@ -67,8 +67,11 @@ Worker process memory protection (Linux)
''''''''''''''''''''''''''''''''''''''''
On Linux, the supervisor process calls ``prctl(PR_SET_DUMPABLE, 0)`` at the start of
-``supervise_task()`` before forking the task process. This flag is inherited by the forked
-child. Marking processes as non-dumpable prevents same-UID sibling processes from reading
+``supervise_task()`` before forking the task process. A bare-forked child inherits the flag;
+a child started through ``exec`` (macOS, or ``[core] execute_tasks_new_python_interpreter``)
+restores it in its bootstrap, before importing Airflow, because ``execve`` resets it; the remaining
+window is interpreter start, which ``kernel.yama.ptrace_scope >= 1`` covers. Marking processes as
Review Comment:
Thanks — both docs now say it covers `/proc/<pid>/mem` and `ptrace` attach for that window (bb3553efad).
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,22 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker read it). When set to
+``True``, the task supervisor now ``exec``\ s a fresh interpreter right after forking the task process,
+which prevents the fork from inheriting a lock held by a supervisor thread (a permanent hang at the
+task's first TLS call). Deployments that kept the option ``True`` from Airflow 2 get this behaviour,
+and its per-task interpreter start-up cost, on upgrade without further action; set it to ``False`` to
+keep bare fork. Edge workers with the option ``True`` already start a fresh interpreter for the
+supervisor and will now start a second one for the task. The Dag processor, the triggerer and task
Review Comment:
Added in bb3553efad: config text and newsfragment say the task process reads the global value (Edge resolves it per team, so a team-scoped `True` alone does not turn it on). Forwarding the resolved value in `_launch_job_subprocess` sounds right as an edge3 follow-up.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r4008238518
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4620,8 +4703,86 @@ def test_start_rejects_non_importable_target_under_exec(self):
@pytest.mark.usefixtures("disable_capturing")
+def test_fork_exec_bootstrap_runs_an_importable_target_end_to_end(
+ captured_logs, time_machine, monkeypatch, client_with_ti_start
+):
+ """
+ Drive the real ``os.execv`` bootstrap: the fresh interpreter runs ``_CHILD_EXEC_BOOTSTRAP``,
+ rebuilds FDs 0-3, rehydrates the target by name and hands it to ``_fork_main``.
+
+ The probe stands in for ``_subprocess_main`` rather than running the task runner: the
+ suite stubs plugin loading in-process (conftest ``_get_plugins``), which a bare-forked
+ child inherits and a fresh interpreter cannot.
+ """
+ from task_sdk.execution_time import exec_probe_target
Review Comment:
Done in f2aedc37dc.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4620,8 +4703,86 @@ def test_start_rejects_non_importable_target_under_exec(self):
@pytest.mark.usefixtures("disable_capturing")
+def test_fork_exec_bootstrap_runs_an_importable_target_end_to_end(
+ captured_logs, time_machine, monkeypatch, client_with_ti_start
+):
+ """
+ Drive the real ``os.execv`` bootstrap: the fresh interpreter runs ``_CHILD_EXEC_BOOTSTRAP``,
+ rebuilds FDs 0-3, rehydrates the target by name and hands it to ``_fork_main``.
+
+ The probe stands in for ``_subprocess_main`` rather than running the task runner: the
+ suite stubs plugin loading in-process (conftest ``_get_plugins``), which a bare-forked
+ child inherits and a fresh interpreter cannot.
+ """
+ from task_sdk.execution_time import exec_probe_target
+
+ tests_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+ monkeypatch.setenv(
+ "PYTHONPATH", os.pathsep.join(p for p in (tests_dir, os.environ.get("PYTHONPATH", "")) if p)
+ )
+ monkeypatch.setattr(supervisor, "_task_process_uses_exec", lambda: True)
+ # ActivitySubprocess.start only execs its own entry point; let the probe be that entry point.
+ monkeypatch.setattr(supervisor, "_subprocess_main", exec_probe_target.exec_probe_main)
+ time_machine.move_to(timezone.datetime(2024, 11, 7, 12, 34, 56, 78901), tick=False)
+
+ proc = ActivitySubprocess.start(
+ dag_rel_path=os.devnull,
+ bundle_info=FAKE_BUNDLE,
+ what=TaskInstance(
+ id="4d828a62-a417-4936-a7a6-2b3fabacecab",
+ task_id="b",
+ dag_id="c",
+ run_id="d",
+ try_number=1,
+ dag_version_id=uuid7(),
+ queue="default",
+ ),
+ client=client_with_ti_start,
+ target=exec_probe_target.exec_probe_main,
+ )
+
+ assert proc.wait() == 0, captured_logs
+ assert {
+ "logger": "task.stdout",
+ "event": "exec-probe-ok",
+ "level": "info",
+ "timestamp": "2024-11-07T12:34:56.078901Z",
+ } in captured_logs
+
+
class TestChildExecMain:
- """Test the macOS fork+exec child entry point."""
+ """Test the fork+exec child entry point."""
+
+ def test_bootstrap_is_prelude_then_entry_point(self):
+ assert supervisor._CHILD_EXEC_BOOTSTRAP.startswith(supervisor._CHILD_EXEC_PRELUDE)
+ assert supervisor._CHILD_EXEC_BOOTSTRAP.rstrip().endswith("_child_exec_main()")
+ compile(supervisor._CHILD_EXEC_BOOTSTRAP, "<bootstrap>", "exec")
+
+ def test_prelude_survives_an_interpreter_without_ctypes(self):
+ """Without _ctypes the prelude must fall through to the logged fallback, not kill the task."""
+ import subprocess
Review Comment:
Done in f2aedc37dc.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3971124958
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
+
+ Always on for platforms where bare fork is unsafe (macOS). Elsewhere it can be
+ opted into with ``[core] execute_tasks_new_python_interpreter``: exec replaces
+ the child's address space, so it cannot inherit a lock a supervisor thread held
+ at fork time (e.g. OpenSSL's, which otherwise deadlocks the task at its first
+ TLS call — see #71707).
+ """
+ if sys.platform in _FORK_EXEC_PLATFORMS:
+ return True
+ return conf.getboolean("core", "execute_tasks_new_python_interpreter", fallback=False)
Review Comment:
Good catch — fixed in 9711663: `_child_exec_main()` now calls `_make_process_nondumpable()` first, both security docs updated, and a Linux-only `test_exec_child_reapplies_nondumpable` fork+execs and asserts the flag goes 1 → 0.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
Review Comment:
Agreed — tasks only. 0fe8931 keeps `_should_use_exec()` as the platform gate and reads the option in `ActivitySubprocess.start()` via `_task_process_uses_exec()`; Dag processor and triggerer are unchanged. Same shape a v3-2/v3-3 backport can carry.
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -223,6 +223,10 @@ core:
* ``False``: Execute via forking of the parent process
* ``True``: Spawning a new python process, slower than fork, but means plugin changes picked
up by tasks straight away
+
+ On workers this also makes the task process ``exec`` a fresh interpreter right after the
Review Comment:
Done in 0fe8931: the description now says tasks-only, that the option was a no-op for tasks on 3.x until now, and notes the Edge worker's second interpreter; added `72164.significant.rst`.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4479,6 +4479,31 @@ def test_api_client_clears_dag_bag_override_when_dag_is_none():
in_process_api_server.cache_clear()
+class TestShouldUseExec:
+ """The config opt-in for fork+exec on platforms where it is not forced on."""
+
+ @pytest.mark.parametrize(
+ ("platform", "config_value", "expected"),
+ [
+ ("darwin", None, True),
+ ("darwin", "False", True),
+ ("linux", None, False),
+ ("linux", "False", False),
+ ("linux", "True", True),
+ ],
+ )
+ def test_should_use_exec(self, monkeypatch, platform, config_value, expected):
+ monkeypatch.setattr(supervisor.sys, "platform", platform)
+ # The supervisor reads the task-sdk conf; the env var reaches it regardless
Review Comment:
You're right — `conf_vars` covers the SDK conf; my PR-body rationale was wrong and I've corrected it. b653700 switches the test to `conf_vars`.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on PR #72164:
URL: https://github.com/apache/airflow/pull/72164#issuecomment-5623008208
@kaxil third round addressed in 3f40f69749 (ctypes import guarded; end-to-end exec bootstrap test with an importable probe, plus bootstrap floors and a missing-_ctypes probe) and bb3553efad (Yama scope wording; task process reads the global option).
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r4007637689
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4620,8 +4703,86 @@ def test_start_rejects_non_importable_target_under_exec(self):
@pytest.mark.usefixtures("disable_capturing")
+def test_fork_exec_bootstrap_runs_an_importable_target_end_to_end(
+ captured_logs, time_machine, monkeypatch, client_with_ti_start
+):
+ """
+ Drive the real ``os.execv`` bootstrap: the fresh interpreter runs ``_CHILD_EXEC_BOOTSTRAP``,
+ rebuilds FDs 0-3, rehydrates the target by name and hands it to ``_fork_main``.
+
+ The probe stands in for ``_subprocess_main`` rather than running the task runner: the
+ suite stubs plugin loading in-process (conftest ``_get_plugins``), which a bare-forked
+ child inherits and a fresh interpreter cannot.
+ """
+ from task_sdk.execution_time import exec_probe_target
Review Comment:
Take or leave: `task_sdk` is already imported at the top of this file, so this can move up with the other imports.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4620,8 +4703,86 @@ def test_start_rejects_non_importable_target_under_exec(self):
@pytest.mark.usefixtures("disable_capturing")
+def test_fork_exec_bootstrap_runs_an_importable_target_end_to_end(
+ captured_logs, time_machine, monkeypatch, client_with_ti_start
+):
+ """
+ Drive the real ``os.execv`` bootstrap: the fresh interpreter runs ``_CHILD_EXEC_BOOTSTRAP``,
+ rebuilds FDs 0-3, rehydrates the target by name and hands it to ``_fork_main``.
+
+ The probe stands in for ``_subprocess_main`` rather than running the task runner: the
+ suite stubs plugin loading in-process (conftest ``_get_plugins``), which a bare-forked
+ child inherits and a fresh interpreter cannot.
+ """
+ from task_sdk.execution_time import exec_probe_target
+
+ tests_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+ monkeypatch.setenv(
+ "PYTHONPATH", os.pathsep.join(p for p in (tests_dir, os.environ.get("PYTHONPATH", "")) if p)
+ )
+ monkeypatch.setattr(supervisor, "_task_process_uses_exec", lambda: True)
+ # ActivitySubprocess.start only execs its own entry point; let the probe be that entry point.
+ monkeypatch.setattr(supervisor, "_subprocess_main", exec_probe_target.exec_probe_main)
+ time_machine.move_to(timezone.datetime(2024, 11, 7, 12, 34, 56, 78901), tick=False)
+
+ proc = ActivitySubprocess.start(
+ dag_rel_path=os.devnull,
+ bundle_info=FAKE_BUNDLE,
+ what=TaskInstance(
+ id="4d828a62-a417-4936-a7a6-2b3fabacecab",
+ task_id="b",
+ dag_id="c",
+ run_id="d",
+ try_number=1,
+ dag_version_id=uuid7(),
+ queue="default",
+ ),
+ client=client_with_ti_start,
+ target=exec_probe_target.exec_probe_main,
+ )
+
+ assert proc.wait() == 0, captured_logs
+ assert {
+ "logger": "task.stdout",
+ "event": "exec-probe-ok",
+ "level": "info",
+ "timestamp": "2024-11-07T12:34:56.078901Z",
+ } in captured_logs
+
+
class TestChildExecMain:
- """Test the macOS fork+exec child entry point."""
+ """Test the fork+exec child entry point."""
+
+ def test_bootstrap_is_prelude_then_entry_point(self):
+ assert supervisor._CHILD_EXEC_BOOTSTRAP.startswith(supervisor._CHILD_EXEC_PRELUDE)
+ assert supervisor._CHILD_EXEC_BOOTSTRAP.rstrip().endswith("_child_exec_main()")
+ compile(supervisor._CHILD_EXEC_BOOTSTRAP, "<bootstrap>", "exec")
+
+ def test_prelude_survives_an_interpreter_without_ctypes(self):
+ """Without _ctypes the prelude must fall through to the logged fallback, not kill the task."""
+ import subprocess
Review Comment:
`subprocess` is already imported at module level (line 28), so this one can go.
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on PR #72164:
URL: https://github.com/apache/airflow/pull/72164#issuecomment-5608844695
@kaxil second round addressed in faa7ce7 (prctl in the exec bootstrap before the Airflow import; wiring tests for `_child_exec_main`, `ActivitySubprocess.start`, and the real prelude) and f6f4740 (rst literal, callbacks note, ptrace_scope wording).
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "gang-zh (via GitHub)" <gi...@apache.org>.
gang-zh commented on PR #72164:
URL: https://github.com/apache/airflow/pull/72164#issuecomment-5666551450
@kaxil PTAL
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #72164:
URL: https://github.com/apache/airflow/pull/72164
--
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
Re: [PR] Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3979588416
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -531,15 +547,37 @@ def _resolve_child_target(dotted: str) -> Callable[[], None]:
return pkgutil.resolve_name(dotted)
+# Runs in the exec'd child before anything else. execve reset PR_SET_DUMPABLE (4 in
+# <linux/prctl.h>); restore it before the Airflow import so the window in which a same-UID
+# sibling can open /proc/<pid>/mem or ptrace-attach is interpreter start only (a descriptor
+# or attach taken in that window survives a later prctl -- the kernel checks once, at open).
+# _child_exec_main() repeats the call as the logged fallback.
+_CHILD_EXEC_PRELUDE = """\
+import sys
+if sys.platform == "linux":
+ import ctypes
Review Comment:
`import ctypes` sits outside the `try` here, unlike `_make_process_nondumpable()` which guards the import as well, so on an interpreter built without `_ctypes` the bootstrap dies with `ModuleNotFoundError` before `_child_exec_main` runs and the logged fallback never gets its turn: every opted-in task exits 1 where bare fork degrades to a warning today. Moving the import inside the `try` matches the helper; a probe with `sys.modules['_ctypes'] = None` reproduces the failure at HEAD and passes with the import moved.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -531,15 +547,37 @@ def _resolve_child_target(dotted: str) -> Callable[[], None]:
return pkgutil.resolve_name(dotted)
+# Runs in the exec'd child before anything else. execve reset PR_SET_DUMPABLE (4 in
+# <linux/prctl.h>); restore it before the Airflow import so the window in which a same-UID
+# sibling can open /proc/<pid>/mem or ptrace-attach is interpreter start only (a descriptor
+# or attach taken in that window survives a later prctl -- the kernel checks once, at open).
+# _child_exec_main() repeats the call as the logged fallback.
+_CHILD_EXEC_PRELUDE = """\
+import sys
+if sys.platform == "linux":
+ import ctypes
+ try:
+ ctypes.CDLL(None, use_errno=True).prctl(4, 0, 0, 0, 0)
+ except Exception:
+ pass
+"""
+_CHILD_EXEC_BOOTSTRAP = _CHILD_EXEC_PRELUDE + (
Review Comment:
Nothing runs this string end to end: the Linux test execs `_CHILD_EXEC_PRELUDE`, the one `use_exec=True` test raises at the closure guard before the fork, and CI has no macOS runner, so dropping `_child_exec_main()` from here, dropping the prelude, or losing `sys.executable` as argv[0] in the `execv` call all stay green while failing every opted-in task. A parametrized row of `test_run_simple_dag` with `_task_process_uses_exec` patched to `True` would cover the assembled path on Linux CI: it goes through `supervise_task` with the default target and so reaches the real `os.execv`, and the asserted `task.stdout` line is re-logged by the supervisor, so the frozen timestamp still holds. `assert _CHILD_EXEC_BOOTSTRAP.startswith(_CHILD_EXEC_PRELUDE)` is a cheap floor alongside it.
##########
airflow-core/newsfragments/72164.significant.rst:
##########
@@ -0,0 +1,22 @@
+``[core] execute_tasks_new_python_interpreter`` now applies to Airflow 3 task processes
+
+On Airflow 3 the option had no effect on task execution (only the Edge worker read it). When set to
+``True``, the task supervisor now ``exec``\ s a fresh interpreter right after forking the task process,
+which prevents the fork from inheriting a lock held by a supervisor thread (a permanent hang at the
+task's first TLS call). Deployments that kept the option ``True`` from Airflow 2 get this behaviour,
+and its per-task interpreter start-up cost, on upgrade without further action; set it to ``False`` to
+keep bare fork. Edge workers with the option ``True`` already start a fresh interpreter for the
+supervisor and will now start a second one for the task. The Dag processor, the triggerer and task
Review Comment:
One narrow inaccuracy here: the Edge worker reads this option through `ExecutorConf(team_name)`, and a team-scoped read consults only the `<team>___core` section (and the `AIRFLOW__<TEAM>___CORE__...` env var) with no fallback to the global value, while the supervisor's new read is global. In a multi-team Edge deployment a team-scoped `True` therefore gives a fresh supervisor interpreter and a bare-forked task, and a global `True` the reverse; a clause that the task process reads the global value would keep this sentence and the config text accurate, and forwarding the resolved value into the supervisor's env in `_launch_job_subprocess` looks like an edge3 follow-up.
##########
airflow-core/docs/security/workload.rst:
##########
@@ -67,8 +67,11 @@ Worker process memory protection (Linux)
''''''''''''''''''''''''''''''''''''''''
On Linux, the supervisor process calls ``prctl(PR_SET_DUMPABLE, 0)`` at the start of
-``supervise_task()`` before forking the task process. This flag is inherited by the forked
-child. Marking processes as non-dumpable prevents same-UID sibling processes from reading
+``supervise_task()`` before forking the task process. A bare-forked child inherits the flag;
+a child started through ``exec`` (macOS, or ``[core] execute_tasks_new_python_interpreter``)
+restores it in its bootstrap, before importing Airflow, because ``execve`` resets it; the remaining
+window is interpreter start, which ``kernel.yama.ptrace_scope >= 1`` covers. Marking processes as
Review Comment:
Following on from https://github.com/apache/airflow/pull/72164#discussion_r3972714843, one precision point on this sentence, since the paragraph goes on to list `/proc/<pid>/environ` and `maps` among what is blocked: Yama only gates `PTRACE_MODE_ATTACH` (`yama_ptrace_access_check` tests `mode & PTRACE_MODE_ATTACH`), so `ptrace_scope >= 1` does close `/proc/<pid>/mem` and `ptrace` attach for the interpreter-start window, but `environ_open` and `proc_maps_open` use `PTRACE_MODE_READ` and stay openable in it, and `environ_read` never re-checks. What that shows is the supervisor's environment, which every task under the same worker already inherits, so nothing new leaks; the sentence would be exact as "``kernel.yama.ptrace_scope >= 1`` covers ``/proc/<pid>/mem`` and ``ptrace`` attach for that window", and `security_model.rst` line 583 has the same wording.
--
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