kaxil commented on code in PR #65738:
URL: https://github.com/apache/airflow/pull/65738#discussion_r3501286381
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -1007,7 +1017,18 @@ def kill(
for sig in escalation_path:
try:
- self._process.send_signal(sig)
+ # Signal the whole process group so subprocesses the
+ # task-runner spawned (venv children, Docker exec, bash
+ # shells, etc.) are also reached. Requires the task-runner to
+ # have been placed in its own session via os.setsid() at fork
+ # time (see start()). See issue #65505.
+ try:
+ os.killpg(os.getpgid(self._process.pid), sig)
Review Comment:
`os.killpg(os.getpgid(self._process.pid), sig)` trusts that the child has
already run `os.setsid()`. Two paths break that assumption and make this signal
the supervisor's *own* process group:
1. `setsid()` is wrapped in `with suppress(OSError)` in `start()`, so if it
ever fails the child stays in the supervisor's group.
2. `_on_child_started` calls `self.kill(signal.SIGKILL)` on any exception
from `task_instances.start()` (line 1385). `setsid()` runs in the forked child
and the parent doesn't synchronize on it, so a synchronous failure there can
reach `kill()` before the child has run `setsid()`. (A 409 over the network is
fine, since the round-trip gives the child time to run it; a local/synchronous
failure isn't.)
In both cases `os.getpgid(child)` returns the supervisor's PGID and
`os.killpg(..., SIGKILL)` hits the supervisor and every sibling in its group.
The `except (ProcessLookupError, PermissionError)` fallback doesn't catch this
because nothing is raised. `test_child_is_session_leader` asserts this exact
invariant ("so os.killpg() does not signal the supervisor itself"), but the
production path has no guard.
Either set the group race-free from the parent too (`with suppress(OSError):
os.setpgid(pid, pid)` right after the fork) or guard the kill site:
```python
pgid = os.getpgid(self._process.pid)
if pgid == os.getpgid(0):
self._process.send_signal(sig)
else:
os.killpg(pgid, sig)
```
Separately: this group-signal only runs on the `kill()` path. The graceful
path in `wait()` (`_forward_signal` -> `os.kill(self.pid, signum)`) still
signals the task-runner alone, so the orphan leak this PR targets persists on
graceful SIGTERM (e.g. K8s pod termination). Now that the child is a session
leader, that path could use `killpg` too.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -690,6 +690,16 @@ def start(
pid = os.fork()
if pid == 0:
+ # Put the task-runner into its own session so its PGID == its own
+ # PID. The supervisor can then deliver signals to the whole tree
+ # via os.killpg() in kill(), reaching every subprocess the
+ # task-runner spawned (e.g. venv children from
+ # PythonVirtualenvOperator). Without this, a SIGTERM from kill()
+ # only hits the task-runner and any Popen children are reparented
+ # to PID 1 and leak as orphans. See issue #65505.
+ with suppress(OSError):
+ os.setsid()
Review Comment:
This `setsid()` (and the `killpg` in `kill()`) lands in the base
`WatchedSubprocess`, and `kill()` is defined only here with no subclass
override, so it applies to every subprocess type (`DagFileProcessorProcess`,
`TriggerRunnerSupervisor`, `CallbackSubprocess`), not just the
`ActivitySubprocess` task-runner this PR describes.
The triggerer's long-lived async subprocess installs its own SIGINT/SIGTERM
handlers and expects a graceful shutdown; making it a session leader and
group-signalling it changes that. Detaching these children into their own
session also means a terminal Ctrl-C (foreground-group SIGINT) no longer
reaches them directly. The neighbouring `use_exec` handles exactly this by
being opt-in per subclass (its docstring even notes the DAG processor and
triggerer as a follow-up).
Suggest the same here: a `new_session: bool = False` param on `start()` set
`True` only in `ActivitySubprocess`, with the `killpg` branch in `kill()` gated
on it. That keeps the change scoped to the task-runner, and confines the
self-signal risk flagged on the `killpg` line to the one path you've actually
tested.
--
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]
