This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new e807d5e9ba3 Make SSH remote-job kill test diagnosable and stop it
reddening main (#70562)
e807d5e9ba3 is described below
commit e807d5e9ba34f8e809336d435cf37fba2c3e0fd3
Author: Jarek Potiuk <[email protected]>
AuthorDate: Mon Jul 27 23:12:58 2026 +0200
Make SSH remote-job kill test diagnosable and stop it reddening main
(#70562)
test_kill_terminates_whole_job_tree_under_job_control failed on main with
"job
never wrote its pid file". The run took 5.16s, so SUBMIT_DONE arrived
promptly and
the whole budget went on polling for a pid file that never appeared.
That symptom has two very different causes and the test cannot tell them
apart:
_run_bash_mc_under_pty returns silently when the marker never arrives, so a
launcher that died immediately (EOF, no marker, returns at once) and a job
that
died after being launched both surface later as the same empty pid file -
and both
produce the same ~5s runtime. Two changes separate them:
- assert the marker was actually seen, quoting what the pty did produce
- report the job directory contents when the pid file stays empty; the
wrapper
creates that directory and the log file before it backgrounds anything,
so a
missing directory means the launcher never got there and an empty one
means the
job was launched and died before its first statement
The teardown also has a real ordering hazard, closed here: the marker only
says the
launcher returned, which it does the moment it backgrounds setsid - before
that
child has forked, called setsid(2) and exec'd into the job. Closing the pty
master
hangs up the terminal, and pty.fork() makes the launcher the session
leader, so
hanging up inside that window could take the job down with the session. The
pty is
now held open until the job proves it left the session by recording its own
pid.
I could not reproduce the failure. macOS has no setsid(1) so the class
skips there;
under Linux in Docker the real wrapper ran through this exact harness 65
times, 25
idle and 40 with the container throttled to 0.35 CPU against six busy
loops, and
recorded its pid every time with and without the hold. So the hold is
hardening, not
a demonstrated fix. Reruns come back for that reason - #69384 had them on
the
sibling test until #69490 dropped the marker while writing this one - and
the first
attempt's assertion text still reaches the CI log, so the next occurrence
should
identify which half broke.
Generated-by: Claude Opus 5 (1M context)
---
.../ssh/tests/unit/ssh/utils/test_remote_job.py | 52 ++++++++++++++++++++--
1 file changed, 48 insertions(+), 4 deletions(-)
diff --git a/providers/ssh/tests/unit/ssh/utils/test_remote_job.py
b/providers/ssh/tests/unit/ssh/utils/test_remote_job.py
index 82b285d4587..be93502057e 100644
--- a/providers/ssh/tests/unit/ssh/utils/test_remote_job.py
+++ b/providers/ssh/tests/unit/ssh/utils/test_remote_job.py
@@ -25,6 +25,7 @@ import select
import shutil
import subprocess
import time
+from collections.abc import Callable
from pathlib import Path
import pytest
@@ -305,15 +306,39 @@ class TestPosixKillBehaviour:
if pid_text:
break
time.sleep(0.02)
- assert pid_text, "job never wrote its pid file"
+ # Say which half failed. The wrapper creates the job dir and log file
before it
+ # backgrounds anything, so their absence means the launcher never got
that far,
+ # while an empty job dir means the job was launched and then died
before its first
+ # statement. A bare "no pid file" cannot tell those apart.
+ job_dir = Path(paths.job_dir)
+ state = (
+ f"job dir holds {sorted(p.name for p in job_dir.iterdir())}"
+ if job_dir.exists()
+ else "job dir was never created (launcher did not run)"
+ )
+ assert pid_text, f"job never wrote its pid file; {state}"
return int(pid_text)
@staticmethod
- def _run_bash_mc_under_pty(script: str, marker: bytes, timeout: float =
8.0) -> None:
+ def _run_bash_mc_under_pty(
+ script: str,
+ marker: bytes,
+ detached: Callable[[], bool] | None = None,
+ timeout: float = 8.0,
+ ) -> None:
"""Run ``bash -mc script`` under a pty we own so job control genuinely
activates
(bash silently disables ``-m`` without a controlling terminal). Read
until the
marker, NOT to EOF: the detached job inherits the pty slave as its
stdin, so EOF
- would not arrive until the job itself exits (the full sleep
runtime)."""
+ would not arrive until the job itself exits (the full sleep runtime).
+
+ The marker only says the launcher returned, which it does the moment
it puts
+ ``setsid`` in the background - before that child has forked, called
setsid(2) and
+ exec'd into the job. Closing the master hangs the pty up, and the
launcher is the
+ session leader here (``pty.fork`` gives it the pty as controlling
terminal), so a
+ hangup inside that window can take the job down with the session
before it runs its
+ first statement. ``detached``, when given, is polled until the job has
proven it
+ left the session (it records its own pid), and only then do we hang up.
+ """
pid, fd = pty.fork()
if pid == 0:
try:
@@ -323,6 +348,7 @@ class TestPosixKillBehaviour:
try:
deadline = time.monotonic() + timeout
buf = b""
+ seen = False
while time.monotonic() < deadline:
r, _, _ = select.select([fd], [], [], 0.2)
if fd in r:
@@ -334,7 +360,13 @@ class TestPosixKillBehaviour:
break
buf += chunk
if marker in buf:
+ seen = True
break
+ # Without this the launcher timing out is indistinguishable from
the job dying:
+ # both surface later as an empty pid file.
+ assert seen, f"launcher never emitted {marker!r} within {timeout}s
(got {buf!r})"
+ while detached is not None and time.monotonic() < deadline and not
detached():
+ time.sleep(0.02)
finally:
os.close(fd) # hangs up the pty; the launcher (not the detached
job) exits
with contextlib.suppress(ChildProcessError):
@@ -365,6 +397,11 @@ class TestPosixKillBehaviour:
pgid = self._await_recorded_pid(paths)
self._assert_kill_tears_down(paths, pgid, marker)
+ # Same environment-dependent process-group race that #69384 added reruns
for on the
+ # sibling test; that marker was dropped in #69490 when this pty variant
was written.
+ # The launch still depends on how the runner schedules the setsid fork, so
keep main
+ # green on a fresh draw - the first attempt's assertion text stays in the
CI log.
+ @pytest.mark.flaky(reruns=5)
def test_kill_terminates_whole_job_tree_under_job_control(self, tmp_path):
"""With job control on, setsid(1) forks and the launcher's ``$!``
would name the
short-lived setsid parent, not the job -- the condition the old
wrapper orphaned
@@ -373,7 +410,14 @@ class TestPosixKillBehaviour:
marker = self._marker("2")
paths = RemoteJobPaths(job_id="killtree_jc", remote_os="posix",
base_dir=str(tmp_path / "jobs"))
wrapper = build_posix_wrapper_command(marker, paths)
- self._run_bash_mc_under_pty(wrapper + "\necho SUBMIT_DONE\n",
b"SUBMIT_DONE")
+ pid_path = Path(paths.pid_file)
+ self._run_bash_mc_under_pty(
+ wrapper + "\necho SUBMIT_DONE\n",
+ b"SUBMIT_DONE",
+ # The job records its pid only after setsid(2) has put it in its
own session, so
+ # a non-empty pid file is proof the pty hangup below can no longer
reach it.
+ detached=lambda: pid_path.exists() and
bool(pid_path.read_text().strip()),
+ )
pgid = self._await_recorded_pid(paths)
job_pids = subprocess.run(
