Repository navigation
Allow opting into fork+exec task processes via execute_tasks_new_python_interpreter - #72164
Conversation
seanmuth
left a comment
There was a problem hiding this comment.
This review was drafted by Claude at @seanmuth's request, drawing on a multi-day debugging session together on the underlying inherited-lock deadlock (working with Gang on reproducing and confirming the mechanism from the OpenSSL/pthread_rwlock source, and validating the fork+exec fix's effect independently). Sharing that context here since it's directly relevant to evaluating this PR.
Overall: this is a sound, minimally-scoped fix. It reuses the existing _FORK_EXEC_PLATFORMS mechanism rather than inventing a parallel one. Traced through the source to confirm it correctly extends to all three fork points that need it — task execution (supervisor.py), the Dag File Processor (dag_processing/processor.py), and the triggerer (jobs/triggerer_job_runner.py) — since all three call the same shared _should_use_exec() gate that this PR modifies.
A few notes, none blocking:
-
Config-key reuse is safe but slightly overloaded (nit).
[core] execute_tasks_new_python_interpreteralready had a distinct meaning forCeleryExecutorpre-3.0 (celery_executor_utils.py's_execute_in_fork/_execute_in_subprocessbranch — full subprocess respawn for plugin-reload freshness, not lock-safety). That branch is dead on Airflow 3 (guarded byif not AIRFLOW_V_3_0_PLUS:), so there's no functional collision, but the same option name now carries two unrelated rationales depending on major version. Might be worth a one-line clarifying note in the config docs, not a code change. -
Test coverage looks right for the change. The
TestShouldUseExecparametrization (darwin regardless of config; linux off by default; linux on via explicitTrue) covers the new branch precisely, and correctly patches the task-sdk's own conf object rather than airflow-core's — the exact gotcha called out as a fix vs. #71723. -
The production evidence here is strong — 16/13,930 bare-fork deadlocks vs. 0/5,195 with exec, plus a sustained zero-hang run afterward. Worth weighing heavily given this touches core fork behavior.
-
Opt-in default is the right call given the measured ~9s/task overhead on a plugin-heavy image — letting KubernetesExecutor users (who already eat a cold fork per task) opt in makes sense rather than changing default behavior for everyone.
No blocking issues found from this pass.
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
7639e6b to
1d65a2c
Compare
bb3553e to
8902fd0
Compare
…on_interpreter On Linux the supervisor bare-forks the task process. If any supervisor thread (OTel exporter, google-auth refresh, ...) holds a C-level lock such as 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 with no timeout able to fire (apache#71707). exec()-ing a fresh interpreter right after fork (what _FORK_EXEC_PLATFORMS already forces on macOS) makes the inherited-lock state unreachable. Extend the existing [core] execute_tasks_new_python_interpreter config to opt into that path on any platform, and document the new effect. Kept opt-in because the fresh interpreter re-imports the SDK and plugins (measured ~9s per task on a plugin-heavy production image; it shows up in task duration, not queued duration). Supersedes apache#71723; the config approach and platform gate are unchanged from it. The test here patches the configuration through the environment variable so it reaches the task-sdk conf object the supervisor actually reads. Co-authored-by: Pulak Saha <sahapulak557@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
execve resets the dumpable flag that supervise_task() set before the fork, so an exec'd child was readable through /proc/<pid>/environ by a same-UID sibling. Re-apply it as the first step of _child_exec_main(), update the security docs that described the flag as purely inherited, and pin it with a Linux-only fork+exec test.
_should_use_exec() is also consulted by the Dag processor (one child per file per parse loop) and the triggerer runner, so reading the option there would exec a fresh interpreter per parse. Keep it as the platform gate and read the option in ActivitySubprocess.start() only, which is also the shape v3-2/v3-3 can carry (they check darwin inline at that spot). Document the tasks-only scope, that the option was a no-op for tasks on Airflow 3 until now, and the Edge worker's second interpreter.
conf_vars already patches the task-sdk conf once the supervisor module is imported, so the env-var route was only an inconsistency.
"exec'd" is neither a dictionary word nor in the wordlist; say "a child started through exec" instead.
…iring in tests The exec'd child was dumpable from execve until _child_exec_main() ran, i.e. for the whole Airflow import; a /proc/<pid>/mem descriptor or ptrace attach taken then survives the later prctl. The -c bootstrap (now _CHILD_EXEC_BOOTSTRAP) restores the flag with ctypes before importing Airflow, leaving interpreter start as the only window; _child_exec_main() keeps the logged fallback. Tests now pin what the review found unpinned: the real prelude string is exec'd and asserted to flip the flag 1 -> 0 (Linux), _child_exec_main() is checked to call _make_process_nondumpable() before _fork_main(), and ActivitySubprocess.start() is checked to pass the task-process decision as use_exec (stub targets stay bare fork). The forked test branch exits via os._exit in a finally so a failed execv cannot run pytest in the child. Docstrings no longer describe exec as macOS-only.
…rk, describe the bootstrap prctl ``exec``s left the literal open until the next backticks in the Configuration Reference; reworded. Task callbacks run from a closure the exec path cannot name, so they stay on bare fork -- say so in the config text and the newsfragment. The security docs now describe the prctl in the exec bootstrap and name ptrace_scope >= 1 as what covers the interpreter-start window.
… end in tests The prelude imported ctypes outside its try, so an interpreter built without _ctypes would have died before _child_exec_main() could reach the logged fallback; the import now sits inside the try like _make_process_nondumpable(). test_fork_exec_bootstrap_runs_an_importable_target_end_to_end drives ActivitySubprocess.start through the real os.execv: the fresh interpreter runs _CHILD_EXEC_BOOTSTRAP, rebuilds FDs 0-3, rehydrates the target by name and its stdout comes back through the supervisor. It uses an importable probe standing in for _subprocess_main rather than the task runner, because the suite stubs _get_plugins in-process -- inherited by a bare-forked child, not by a fresh interpreter, which then fails with 'Plugins folder is not set'. Also pinned: the bootstrap starts with the prelude and ends in _child_exec_main(), and the prelude survives a missing _ctypes.
… global option ptrace_scope >= 1 gates PTRACE_MODE_ATTACH only, so it covers /proc/<pid>/mem and ptrace attach for the interpreter-start window but not environ/maps (which show the supervisor's environment every task under the worker already has). The task process reads the global value of the option; Edge workers resolve it per team.
8902fd0 to
b51701c
Compare
|
@kaxil PTAL |
kaxil
left a comment
There was a problem hiding this comment.
Approving, all three rounds are addressed. The prelude guards its ctypes import now and test_prelude_survives_an_interpreter_without_ctypes pins it; test_fork_exec_bootstrap_runs_an_importable_target_end_to_end drives the real os.execv bootstrap: dropping _child_exec_main() from the bootstrap, dropping the _AIRFLOW_CHILD_TARGET env or losing sys.executable as argv[0] each fails it now, and dropping the prelude fails the shape check, which is what the round-3 comment was after. The Yama wording and the global-read clause read right. Yes please to the follow-up issue for an importable callback entry point (#72164 (comment)). On the v3-2/v3-3 backport: those branches have no _should_use_exec, no target rehydration and an inline -c string, so it needs a hand-crafted cherry-pick and the end-to-end test cannot come along as-is.
Both were already available at module scope (subprocess is imported at the top of the file and task_sdk is on the path for the other imports), so the local imports in test_fork_exec_bootstrap_runs_an_importable_target_end_to_end and test_prelude_survives_an_interpreter_without_ctypes only added noise.
|
Thanks! Both nits in f2aedc3 (imports hoisted to module level). Opened #73143 for the importable callback entry point. The failing |
execute_tasks_new_python_interpreter (apache#72164) and the macOS-forced exec path both still call os.fork() before execv(), and CPython's os.fork() runs every os.register_at_fork(after_in_child=...) callback synchronously inside the fork() call itself, before any Python-level code -- including the planned execv() -- gets control back. A third-party library's own fork handler that blocks there (confirmed live: a customer's task process hung inside datadog's dogstatsd client, which registers such a handler by default) hangs the child before exec is ever reached, regardless of how soon the caller tries to exec. This isn't just Airflow's own known OpenSSL provider-store case (apache#71707) -- it's any library that registers an at-fork handler that isn't async-signal-safe, which fork+exec cannot protect against structurally, no matter how the call sites are ordered. os.posix_spawn() doesn't have this gap: CPython's binding never calls PyOS_AfterFork_Child(), and glibc's own posix_spawn (2.24+) uses clone(CLONE_VM|CLONE_VFORK) rather than fork(), so registered os.register_at_fork()/pthread_atfork() handlers are structurally unreachable, not just less likely to hang. It's also not a new cost on top of the existing exec path -- benchmarked against a real Airflow import, posix_spawn is measurably not more expensive than the fork+exec it replaces (slightly cheaper, from skipping fork()'s own copy-on-write setup before the exec). No new config surface: wherever use_exec was already True (the platform gate or execute_tasks_new_python_interpreter), the spawn mechanism underneath is now always posix_spawn. That decision was already made by existing config; this only changes how "give me a fresh interpreter" is implemented once it's been decided, matching the existing dup2/env/ process-group semantics via posix_spawn's file_actions/env/setpgroup parameters instead of imperative code in a forked child.
execute_tasks_new_python_interpreter (apache#72164) and the macOS-forced exec path both still call os.fork() before execv(), and CPython's os.fork() runs every os.register_at_fork(after_in_child=...) callback synchronously inside the fork() call itself, before any Python-level code -- including the planned execv() -- gets control back. A third-party library's own fork handler that blocks there (confirmed live: a customer's task process hung inside datadog's dogstatsd client, which registers such a handler by default) hangs the child before exec is ever reached, regardless of how soon the caller tries to exec. This isn't just Airflow's own known OpenSSL provider-store case (apache#71707) -- it's any library that registers an at-fork handler that isn't async-signal-safe, which fork+exec cannot protect against structurally, no matter how the call sites are ordered. os.posix_spawn() doesn't have this gap: CPython's binding never calls PyOS_AfterFork_Child(), and glibc's own posix_spawn (2.24+) uses clone(CLONE_VM|CLONE_VFORK) rather than fork(), so registered os.register_at_fork()/pthread_atfork() handlers are structurally unreachable, not just less likely to hang. It's also not a new cost on top of the existing exec path -- benchmarked against a real Airflow import, posix_spawn is measurably not more expensive than the fork+exec it replaces (slightly cheaper, from skipping fork()'s own copy-on-write setup before the exec). No new config surface: wherever use_exec was already True (the platform gate or execute_tasks_new_python_interpreter), the spawn mechanism underneath is now always posix_spawn. That decision was already made by existing config; this only changes how "give me a fresh interpreter" is implemented once it's been decided, matching the existing dup2/env/ process-group semantics via posix_spawn's file_actions/env/setpgroup parameters instead of imperative code in a forked child.
…73503) * Use os.posix_spawn instead of fork+exec in the task-sdk supervisor execute_tasks_new_python_interpreter (#72164) and the macOS-forced exec path both still call os.fork() before execv(), and CPython's os.fork() runs every os.register_at_fork(after_in_child=...) callback synchronously inside the fork() call itself, before any Python-level code -- including the planned execv() -- gets control back. A third-party library's own fork handler that blocks there (confirmed live: a customer's task process hung inside datadog's dogstatsd client, which registers such a handler by default) hangs the child before exec is ever reached, regardless of how soon the caller tries to exec. This isn't just Airflow's own known OpenSSL provider-store case (#71707) -- it's any library that registers an at-fork handler that isn't async-signal-safe, which fork+exec cannot protect against structurally, no matter how the call sites are ordered. os.posix_spawn() doesn't have this gap: CPython's binding never calls PyOS_AfterFork_Child(), and glibc's own posix_spawn (2.24+) uses clone(CLONE_VM|CLONE_VFORK) rather than fork(), so registered os.register_at_fork()/pthread_atfork() handlers are structurally unreachable, not just less likely to hang. It's also not a new cost on top of the existing exec path -- benchmarked against a real Airflow import, posix_spawn is measurably not more expensive than the fork+exec it replaces (slightly cheaper, from skipping fork()'s own copy-on-write setup before the exec). No new config surface: wherever use_exec was already True (the platform gate or execute_tasks_new_python_interpreter), the spawn mechanism underneath is now always posix_spawn. That decision was already made by existing config; this only changes how "give me a fresh interpreter" is implemented once it's been decided, matching the existing dup2/env/ process-group semantics via posix_spawn's file_actions/env/setpgroup parameters instead of imperative code in a forked child. * Update stale fork+exec docstrings to describe posix_spawn Fixes docstrings left describing the old os.fork()+os.execv() mechanism after it was replaced with os.posix_spawn(): the module docstring's os.set_inheritable/FD_CLOEXEC description, _child_exec_main's "placed there via dup2" description, and the dup2-ordering safety invariant that was dropped along with the old imperative dup2 code.
Resolves #71707. Supersedes #71723 (same approach, @pulaksaha143 credited as co-author; it had been idle since Aug 17 with static checks failing).
Why
On Linux the supervisor bare-forks the task process. If any supervisor thread 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_timeoutnever fires because the hang precedesexecute()(details in #71707).exec()-ing a fresh interpreter right after the fork — what_FORK_EXEC_PLATFORMSalready forces on macOS — makes inherited-lock state unreachable. This lets a deployment opt into that path for the task process with the existing[core] execute_tasks_new_python_interpreteroption.Scope and cost
_should_use_exec()stays the platform gate (it is also consulted by the Dag processor and triggerer); the option is read inActivitySubprocess.start()via_task_process_uses_exec(). That is also the shape v3-2/v3-3 can carry, since they check darwin inline at that spot.Truefrom Airflow 2 get the exec path on upgrade —72164.significant.rstsays so, as does the config description, together with the Edge worker's second interpreter.execveresetsPR_SET_DUMPABLE; the-cbootstrap (_CHILD_EXEC_BOOTSTRAP) restores it with ctypes before importing Airflow, so the only window is interpreter start (ptrace_scope >= 1covers it);_child_exec_main()keeps the logged fallback. Pinned by an end-to-end test that drives the realos.execvbootstrap with an importable probe target, a Linux-only test that execs the real prelude, a_child_exec_mainordering spy, and anActivitySubprocess.startuse_exectest.Production validation
Running as an image patch on Airflow 3.2.2 / KubernetesExecutor: a TLS-stress probe deadlocked 16 of 13,930 bare-forked children (
pthread_rwlock_wrlockunderSSL_CTX_new_ex) vs 0 of 5,195 with exec. Since Aug 26, seven production deployments: 466k+ task starts, zero hangs (≈450 expected at the pre-fix rate); deferrals, retries, XCom and log shipping normal. Data: #71723 (comment)Changes vs #71723
Config read scoped to the task process, dumpable flag re-applied in the exec'd child, config/newsfragment/security docs, tests via
conf_vars(the earlier claim that #71723's test patched the wrong conf was mistaken —conf_varscovers the SDK conf), static checks passing.A backport to v3-2/v3-3 would be very welcome — every KubernetesExecutor user on those lines forks cold per task.