From eaf6f66d57dca7469d8c32fee4c3c205b522588b Mon Sep 17 00:00:00 2001 From: Chao Wang <26245345+ChaoWao@users.noreply.github.com> Date: Sun, 26 Jul 2026 08:23:04 -0700 Subject: [PATCH] Fix: read the orphan test's child pids from a file, not the parent's log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_orphan_child_reaping` scraped the forked children's pids out of the parent subprocess's combined stdout+stderr, taking every digit-only token it found. That stream also carries log lines and warnings, so any bare number in them was read as a pid. On CI it collected a fourth entry: expected exactly 3 forked sub-worker pids, parent reported [6778, 6779, 6780, 3] The three real pids were correct — `Worker._sub_pids` is not at fault — and the run was otherwise healthy, so the test failed on main for a parsing artifact rather than a defect. It also meant the `finally` block would have sent SIGKILL to whatever process that stray number named. Give the parent a dedicated output file, named on its command line, and keep stdout+stderr as a separate log used only for diagnostics when an assertion fails. Parsing a channel that carries nothing but the pids removes the class of failure rather than filtering harder against it, which is the same mistake in a different shape. Also drop the `isdigit()` filter, now that a non-numeric token in the pid file would be a real defect worth surfacing rather than noise to skip. 15/15 local runs pass; red-to-green still holds against the pre-#1495 `worker.py` ("3 of 3 children outlived their SIGKILLed parent by 20s", now with exactly three pids and no phantom entry). Co-Authored-By: Claude Opus 5 --- .../test_worker/test_orphan_child_reaping.py | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/ut/py/test_worker/test_orphan_child_reaping.py b/tests/ut/py/test_worker/test_orphan_child_reaping.py index 5c7d2a221..0ad438e8f 100644 --- a/tests/ut/py/test_worker/test_orphan_child_reaping.py +++ b/tests/ut/py/test_worker/test_orphan_child_reaping.py @@ -40,6 +40,10 @@ # enumerating children externally also catches the transient shell that runs # pgrep, and inside a container's shallow pid namespace it picks up unrelated # low pids — which the test would then wait on and, worse, SIGKILL. +# +# The pids go to a dedicated file named on the command line, not to stdout: +# the parent's stdout and stderr carry log lines and warnings, and scraping +# integers out of that stream picks up any bare number they happen to contain. _PARENT_SRC = textwrap.dedent( """ import os, sys @@ -48,8 +52,10 @@ worker = Worker(level=3, num_sub_workers=%d) worker.register(lambda args: None) worker.init() - print(" ".join(str(p) for p in sorted(worker._sub_pids)), flush=True) - sys.stdout.flush() + with open(sys.argv[1], "w") as f: + f.write(" ".join(str(p) for p in sorted(worker._sub_pids))) + f.flush() + os.fsync(f.fileno()) os.kill(os.getpid(), 9) """ ) @@ -66,11 +72,14 @@ def _alive(pid: int) -> bool: def test_children_exit_when_parent_is_killed(tmp_path): - out_path = tmp_path / "children.txt" + pid_path = tmp_path / "child_pids.txt" + log_path = tmp_path / "parent_output.txt" reported: list[int] = [] try: - with out_path.open("w") as out: - parent = subprocess.Popen([sys.executable, "-c", _PARENT_SRC % NUM_SUB_WORKERS], stdout=out, stderr=out) + with log_path.open("w") as log: + parent = subprocess.Popen( + [sys.executable, "-c", _PARENT_SRC % NUM_SUB_WORKERS, str(pid_path)], stdout=log, stderr=log + ) returncode = parent.wait(timeout=PARENT_EXIT_TIMEOUT_S) # The parent is supposed to die from its own SIGKILL. Any other exit @@ -79,13 +88,13 @@ def test_children_exit_when_parent_is_killed(tmp_path): # got []" that reads like a defect in the code under test. assert returncode == -signal.SIGKILL, ( f"parent exited {returncode}, expected -{int(signal.SIGKILL)} (its own SIGKILL): " - f"it failed before it could fork\nparent output:\n{out_path.read_text()}" + f"it failed before it could fork\nparent output:\n{log_path.read_text()}" ) - reported = [int(tok) for tok in out_path.read_text().split() if tok.isdigit()] + reported = [int(tok) for tok in pid_path.read_text().split()] assert len(reported) == NUM_SUB_WORKERS, ( f"expected exactly {NUM_SUB_WORKERS} forked sub-worker pids, parent reported {reported}\n" - f"parent output:\n{out_path.read_text()}" + f"parent output:\n{log_path.read_text()}" ) deadline = time.monotonic() + REAP_TIMEOUT_S