vatsrahul1001 commented on PR #68644: URL: https://github.com/apache/airflow/pull/68644#issuecomment-4845207674
Reviewed this and the core change looks solid. I verified the parts most likely to hide a bug and they're clean: - The `bash -c '…'` → `job_script='…'; bash -c "$job_script"` refactor produces a **byte-identical** job body — no quoting regression, and the `'"$log_file"'`-style breakouts still expand at wrapper-parse time exactly as before. - The new kill string is valid POSIX and dash/ash-clean; `[ "$p" -gt 1 ] 2>/dev/null` correctly rejects empty/non-numeric/`0`/`1`/negative/multi-word input with no stderr leaking back over the SSH channel, and the whole command exits `0` (best-effort). - Call sites are unchanged and `on_kill` already wraps execution in try/except with a 30s timeout. - The `$! == PGID` claim genuinely holds on the intended target (Linux remote, non-interactive exec with `get_pty=False`, job control off) — `setsid` doesn't fork an extra level there. Two things I'd suggest tightening before merge, both test coverage on the safety-critical logic: **1. The broadcast guard `[ "$p" -gt 1 ]` has no behavioral test.** Its entire purpose is to stop `kill -TERM -"$p"` from degenerating into `kill -TERM -0` / `-1` (a broadcast to every process the SSH account can signal). Today it's only pinned by a substring assert (`assert '[ "$p" -gt 1 ]' in cmd`), and the e2e test only ever runs the kill against a *valid* pgid. If the guard were dropped or `-gt` flipped to `-ge`/`-lt`, every test would still pass. Worth a small test that runs `build_posix_kill_command` against a pid file containing `0`/`1`/empty/garbage and asserts no broadcast / clean exit. **2. The `nohup` (no-`setsid`) fallback is never exercised at runtime.** `TestPosixKillBehaviour` is `skipif`'d unless `setsid` is present — but that's exactly the condition under which `build_posix_wrapper_command` takes the `setsid` branch, so the `else: nohup …` path can never run under this test on any host. The macOS/BSD fallback (where `$!` is not a group leader and the kill must degrade to single-PID) is only asserted as a substring. Some coverage that actually drives the fallback would protect the path the PR explicitly claims to support. A few smaller, non-blocking notes: - **PID-reuse blast radius (`build_posix_kill_command`).** If `on_kill` fires after the leader exited and its PID was recycled as a new, unrelated group leader, `kill -TERM -"$p"` now TERMs that whole group, whereas the old `kill <pid>` hit at most one process. Narrow race, but a genuine escalation versus the replaced code, and the `>1` guard doesn't cover number-space reuse — might be worth a line in "Known limitations." - **`set +m` hardening (wrapper).** The `$! == PGID` invariant relies on job control being off. It's safe today because submission is hardcoded `get_pty=False`, but if a future caller ever launches the wrapper under monitor mode, util-linux `setsid` forks, `$!` becomes a stale PID, and the orphaning behavior silently returns. Adding `set +m` before the launch would make the invariant self-enforcing. - **Test flake risk (`test_kill_terminates_whole_job_tree`).** The "alive before kill" assertion runs with no retry, while the post-kill check polls for 5s. On a loaded host `pgrep -g` can run before the just-detached leader is scheduled → spurious pre-condition failure. Mirroring the spin-wait on the pre-condition would remove the asymmetry. - **Structure pinning.** The substring-only assertions would still pass if the `setsid`/`nohup` branches were swapped, `echo $!` moved above the launch, or the `||` fallback order flipped. Asserting branch placement/ordering would catch those. Nothing blocking beyond 1–2. Nice fix overall — the process-group teardown is the right approach. --- Drafted-by: Claude Code (Opus 4.8) (no human review 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]
