kaxil opened a new pull request, #69490:
URL: https://github.com/apache/airflow/pull/69490

   When an `SSHRemoteJobOperator` task is cancelled, `on_kill` signals the 
process group recorded in the job's pid file so the whole job tree is torn 
down. The detached job launches under `setsid` to lead its own process group, 
and the wrapper recorded `$!` as that group's leader.
   
   `$!` is the group leader only when `setsid(1)` runs in place. If job control 
is on in the launching shell, `setsid(1)` forks (`setsid(2)` cannot create a 
new session from an existing process-group leader), so `$!` names the 
short-lived `setsid` parent instead of the job. Cancellation then signals a 
dead group, the real command keeps running as an orphan, and because the 
wrapper subshell is what writes the `exit_code` file, that file never appears 
and the task stays deferred until the trigger times out.
   
   ## Fix
   
   Record the job's own pid instead of the launcher's `$!`. Right after 
`setsid(2)`, POSIX guarantees `pid == pgid == sid` for the caller, and that 
identity survives the following `exec`, so `$$` written from inside the job 
script is always the true process-group id, whether or not `setsid` forked. The 
launcher no longer records `$!` at all, and `build_posix_kill_command` is 
unchanged.
   
   The pid file is read only at cancellation time, in 
[`on_kill`](https://github.com/astronomer/airflow/blob/eedbf78c7d822ea3da0cb276363a805262598896/providers/ssh/src/airflow/providers/ssh/operators/ssh_remote_job.py#L486-L518),
 which runs long after submission, so the job's own write is reliably in place 
before any reader and the launcher does not need to wait for it.
   
   ## Relationship to #68644
   
   #68644 added the `setsid` + process-group-kill behavior but assumed `$!` was 
always the group leader. That holds only when the launching shell is not 
already a process-group leader. Submission runs with `get_pty=False`, so the 
default path is unaffected. The orphaning surfaces on remotes where job control 
ends up on: a controlling terminal, a `ForceCommand` wrapper, or `set -m` in 
the remote user's shell init.
   
   ## Regression test (follow-up to #69384)
   
   The end-to-end kill test was marked `@pytest.mark.flaky(reruns=5)` in #69384 
because it depended on incidental process-group timing. It now forces job 
control through a real controlling terminal (`pty.fork()` + `bash -mc`), 
exercising the exact fork path that orphaned the job, and asserts the recorded 
pid equals the job's true process-group id before checking teardown. The 
`flaky` marker is removed; test markers are unique per test and per worker so 
the suite stays isolated under `pytest -n auto`.
   
   ## Gotcha
   
   On hosts without `setsid` (some macOS/BSD images) the job is not a 
process-group leader, so `$$` is just its own pid and cancellation degrades to 
the previous single-process kill, unchanged from before.
   


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