cmettler commented on code in PR #65738:
URL: https://github.com/apache/airflow/pull/65738#discussion_r3523697512
##########
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:
Good catch — both failure paths were real. Fixed by doing both of your
suggestions: the process group is now set from both sides of the fork
(`os.setpgid(0, 0)` in the child, `os.setpgid(pid, pid)` in the parent right
after fork), so the group exists before `start()` returns and the
`_on_child_started` race is closed deterministically; and `kill()` now goes
through a `_signal_subprocess()` helper that refuses to `killpg` when the
resolved PGID equals our own (the same invariant as `reap_process_group`'s "I
refuse to kill myself" guard).
On the graceful path: that resolved itself upstream — #69034 removed the
SIGTERM forwarding entirely in favour of warm shutdown, so `kill()` is now the
only signal-delivery path and it's covered.
On porting `reap_process_group`/`set_new_process_group` into task-sdk:
agreed that one tested teardown beats two that can drift. I kept this PR
minimal but aligned the semantics with `set_new_process_group` (plain
`setpgid`, no new session), so a port becomes a drop-in. One design question
before attempting it as a follow-up: `reap_process_group` blocks in
`psutil.wait_procs` during escalation, while the supervisor must keep servicing
the child's sockets while it dies (last log lines, terminal state — and a full
pipe would deadlock otherwise). So I'd either share only the primitives (group
setup + guarded group-signal) and keep the supervisor's escalation loop, or
port `reap_process_group` with an injectable wait-hook. If the first, simpler
option sounds right to you I'm happy to pick that up once this PR lands.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @cmettler before posting
--
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]
