kaxil commented on code in PR #65738:
URL: https://github.com/apache/airflow/pull/65738#discussion_r3710985161
##########
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:
Verified the new revision: the parent-side `setpgid(pid, pid)` mirror closes
the `_on_child_started` race deterministically, and the own-group guard in
`_signal_subprocess()` covers the case where both `setpgid` calls failed. On
the follow-up question: sharing only the primitives (group setup + guarded
group-signal) and keeping the supervisor's escalation loop sounds right to me.
The supervisor has to keep draining the child's sockets while it dies, so
`reap_process_group`'s blocking `psutil.wait_procs` is the wrong shape here,
and an injectable wait-hook would complicate its other call sites for no gain.
Happy to review that follow-up when you get to it.
One small ask: the PR description still describes the earlier
`setsid()`/session-leader design and cites the old test name
(`test_child_is_session_leader`) -- mind refreshing it to match the current
revision before merge?
--
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]
