hooiv commented on PR #70163:
URL: https://github.com/apache/airflow/pull/70163#issuecomment-5083989837

   My sincere apologies! I led myself completely astray in my previous comment.
   
   I did a deep dive into the Celery/billiard source code today and realized my 
assumption about billiard was **completely wrong**. Billiard does **not** 
install a global SIGCHLD handler that blindly reaps arbitrary child processes 
with waitpid(-1). Instead, it tracks specific PIDs. The reproducer I provided 
earlier was flawed because I explicitly injected a global handler into the 
script to force the failure, incorrectly assuming that's what billiard did 
under the hood.
   
   The original diagnosis in #70117 was entirely correct: **dumb-init** (or 
whichever process is running as PID 1 in the container) is the real culprit. 
dumb-init is designed to aggressively reap orphaned child processes using 
waitpid(-1). When the Airflow task subprocess exits, dumb-init races the 
Airflow Task SDK supervisor. If dumb-init wins and reaps the child, the OS 
discards the child's exit status. 
   
   When our supervisor subsequently calls psutil.wait(timeout=0), the 
underlying OS call os.waitpid(pid, WNOHANG) raises ECHILD (No child processes). 
psutil catches this ChildProcessError and, realizing the process no longer 
exists, gracefully returns None. 
   
   In our supervisor loop (_check_subprocess_exit), we were not handling this 
None return value. As a result, self._exit_code remained None, meaning the loop 
in _monitor_subprocess never satisfied the condition if self._exit_code is not 
None, preventing SOCKET_CLEANUP_TIMEOUT from ever triggering.
   
   I have reverted my SIGCHLD changes and restored the correct fix: when 
psutil.wait() returns None, we now explicitly map it to an exit code of -1 (and 
log a warning that the exit status was consumed externally). This allows the 
supervisor to exit its loop gracefully. I have also added a dedicated unit test 
in        est_supervisor.py to cover this exact scenario.
   
   apologize for the confusion , branch has been force pushed with the correct 
fix and test.


-- 
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]

Reply via email to