Skip to content

Commit 7da16ea

Browse files
committed
fix(test): the orphan-reap assertion asked for an ordering the engine does not guarantee
`test_worker_kill_reaps_the_whole_process_tree` asserted the LATER of two events at the instant the EARLIER one landed, and it is currently reding a REQUIRED context: PR #458's ubuntu leg failed at tests/test_sandbox.py:998, "the grandchild survived the worker kill", and the same test failed on `main` in run 32206563674. THE DOCSTRING WAS THE DEFECT, NOT ONLY THE TIMING. It claimed pipe-EOF is equivalent to "grandchild reaped". It is not, and the two observables are not the same event: - pipe EOF fires when the last holder of the write end releases its fds -- at process EXIT; - `os.kill(pid, 0)` raises ESRCH only once the pid is REAPED; - the grandchild is ORPHANED, because the sandbox worker (its parent) is killed and `proc.wait()`ed first, so pytest cannot `waitpid` it and reaping falls to PID 1 or the nearest subreaper, asynchronously. So t_reap is STRICTLY AFTER t_exit == t_EOF, always, and the test asserted the later one with no wait, poll or deadline between them. It passed only when the reap won a race it was never entitled to win. TIMING-DEPENDENT AND WRONG ARE NOT ALTERNATIVES HERE -- the ordering is guaranteed by the mechanism. A sleep or a retry would have made it green while leaving the false equivalence in place for the next reader to rely on again, which is why the docstring is part of the change rather than a footnote to it. The two senses of "reap" that collide here are now stated: `_reap_process_tree` reaps in this codebase's sense -- TERMINATE every process in the tree, which is what the engine guarantees and what the pipe-EOF assertion genuinely proves -- while POSIX `waitpid` reaping is a different act on a different schedule, and the test now waits for TERMINATION only. It asks for nothing the engine does not promise. VERIFIED: tests/test_sandbox.py -> 25 passed; the false-equivalence sentence is gone (grep returns 0); ruff 0.15.22 clean; no cp1252-unsafe character introduced. ITS ADVERSARIAL REVIEW HAD NOT REPORTED WHEN THIS WAS COMMITTED -- committed to protect the work across a usage-window boundary, with any finding to be fixed forward. Nothing is pushed. The platform split matters for whoever reads this next: the ordering argument is POSIX-shaped, and this box is Windows, so the CI red is the behavioural evidence rather than a local reproduction.
1 parent ca6e86e commit 7da16ea

1 file changed

Lines changed: 100 additions & 17 deletions

File tree

tests/test_sandbox.py

Lines changed: 100 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -904,8 +904,40 @@ def _orphan_graph(tmp_path: Path) -> tuple[Registry, str, Path]:
904904
return load_config(tmp_path), str(tmp_path), pidfile
905905

906906

907-
def _pid_alive(pid: int) -> bool:
908-
"""Whether ``pid`` names a live process — cross-platform, no third-party deps."""
907+
def _proc_state(pid: int) -> str | None:
908+
"""The Linux ``/proc/<pid>/stat`` process-state field, or ``None`` where ``/proc`` is absent.
909+
910+
Only ``Z`` is load-bearing here: exited, every fd already released, but the pid is still in the
911+
table because nobody has ``waitpid``-ed it yet."""
912+
try:
913+
with open(f"/proc/{pid}/stat", encoding="utf-8") as fh:
914+
text = fh.read()
915+
# UnicodeDecodeError is not an OSError, and a non-Linux /proc need not be text at all — a
916+
# platform we cannot read is a "cannot tell", never a crash in a helper the asserts depend on.
917+
except (OSError, UnicodeDecodeError):
918+
return None
919+
if ")" not in text: # pragma: no cover - a Linux /proc always parenthesises comm
920+
return None
921+
# Field 2 (`comm`) can contain spaces and parens, so everything after the LAST ')' is field 3
922+
# onward and field N sits at index N-3 — the idiom `_posix_stat_ppid_starttime` already uses in
923+
# harness/load/connscale/probe.py. State is field 3, hence index 0.
924+
after = text.rpartition(")")[2].split()
925+
return after[0] if after else None
926+
927+
928+
def _pid_running(pid: int) -> bool | None:
929+
"""Whether ``pid`` names a process that is still RUNNING. ``None`` = this platform cannot tell a
930+
runner from an exited-but-unreaped pid.
931+
932+
Deliberately NOT "does this pid exist", because on POSIX those are different questions and the
933+
difference is the whole point of the caller below. ``os.kill(pid, 0)`` keeps succeeding for a
934+
ZOMBIE: a process that has already exited and released every fd it held, but whose pid stays in
935+
the table until its reaper calls ``waitpid``. Terminating a process is something a caller can
936+
demand; retiring its pid afterwards is the reaper's business and on its schedule.
937+
938+
Windows has no zombie state — there is no exited-but-unretired pid to be fooled by, and
939+
``GetExitCodeProcess`` stops reporting ``STILL_ACTIVE`` once the process has terminated — so the
940+
plain liveness check already IS the running/not-running answer there."""
909941
if sys.platform == "win32":
910942
import ctypes
911943

@@ -914,7 +946,7 @@ def _pid_alive(pid: int) -> bool:
914946
kernel32 = ctypes.WinDLL("kernel32", use_last_error=True)
915947
handle = kernel32.OpenProcess(process_query_limited_information, False, pid)
916948
if not handle:
917-
return False # no such pid (or already fully reaped)
949+
return False # no such pid (or already fully gone)
918950
try:
919951
code = ctypes.c_ulong()
920952
if not kernel32.GetExitCodeProcess(handle, ctypes.byref(code)):
@@ -925,10 +957,28 @@ def _pid_alive(pid: int) -> bool:
925957
try:
926958
os.kill(pid, 0)
927959
except ProcessLookupError:
928-
return False
960+
return False # reaped: out of the process table entirely
929961
except PermissionError:
930-
return True # exists but owned by someone else — still "alive"
931-
return True
962+
return True # exists but owned by someone else — still "running" as far as we can see
963+
state = _proc_state(pid)
964+
if state is None: # POSIX without /proc (e.g. macOS): a zombie and a runner look identical
965+
return None
966+
return state != "Z"
967+
968+
969+
def _wait_until_not_running(pid: int, timeout: float) -> bool | None:
970+
"""Poll :func:`_pid_running` until ``pid`` stops running, returning its last answer.
971+
972+
Bounded rather than instantaneous because a POSIX exit is not atomic: the kernel closes the
973+
dying process's fds — which is what releases a pipe — BEFORE it marks the task a zombie, so a
974+
single check taken at the instant of pipe-EOF can still land inside that tail. This waits for
975+
TERMINATION only and never for the reap, so it asks for nothing the engine does not guarantee."""
976+
deadline = time.monotonic() + timeout
977+
while True:
978+
running = _pid_running(pid)
979+
if running is not True or time.monotonic() >= deadline:
980+
return running
981+
time.sleep(0.01)
932982

933983

934984
def _best_effort_kill_pid(pid: int) -> None:
@@ -953,21 +1003,42 @@ def _best_effort_kill_pid(pid: int) -> None:
9531003

9541004

9551005
def test_worker_kill_reaps_the_whole_process_tree(tmp_path: Path) -> None:
956-
"""Killing the worker must reap the WHOLE tree, not just the immediate child (BACKLOG #342).
1006+
"""Killing the worker must take down the WHOLE tree, not just the immediate child.
9571007
9581008
A Handler spawns a grandchild that inherits fd 1 (the response pipe). Before the fix a bare
9591009
``proc.kill()`` terminated only the worker, leaving the grandchild alive — an orphan still holding
960-
the pipe, so the pipe never reached EOF and the kill was incomplete. The fix reaps the tree: a
961-
Windows ``JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`` job object (exercised locally, this host is
1010+
the pipe, so the pipe never reached EOF and the kill was incomplete. The fix takes down the tree:
1011+
a Windows ``JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`` job object (exercised locally, this host is
9621012
Windows) or a POSIX new-session process group killed with ``killpg`` (exercised by the CI
9631013
ubuntu-latest leg).
9641014
965-
The observable is platform-neutral: for THIS grandchild — which holds fd 1 until it exits —
966-
pipe-EOF is equivalent to "grandchild reaped", so the primary assert covers BOTH halves of the
967-
defect (pipe released AND no lingering process). ``_pid_alive`` re-checks the process half
968-
directly. See the FALSIFICATION recorded in the lane report: forcing
969-
``_assign_kill_on_close_job`` to return ``None`` degrades the Windows path to a bare
970-
``proc.kill()``, the grandchild survives, the pipe never EOFs, and this test goes red."""
1015+
Note the two senses of "reap" that meet here. ``_reap_process_tree`` reaps in this codebase's
1016+
sense — TERMINATE every process in the tree. POSIX ``waitpid`` reaping is a different act —
1017+
RETIRE an already-exited pid from the process table — and the engine neither performs nor can
1018+
bound it for an orphan. The asserts below are split along exactly that line:
1019+
1020+
* PRIMARY, pipe-EOF: proves every holder of fd 1 has EXITED and released it — the worker AND the
1021+
grandchild. That is precisely what ``_reap_process_tree`` guarantees, so it is the pass/fail
1022+
proof of the fix.
1023+
* SECONDARY, the process half: proves the grandchild is NOT RUNNING. It must not ask whether the
1024+
pid was ``waitpid``-ed, which an earlier version of this test did by asserting
1025+
``os.kill(pid, 0)`` raises the instant EOF landed. That ordering is not available: this
1026+
grandchild is an ORPHAN — its parent was killed and ``proc.wait()``-ed first, so pytest cannot
1027+
``waitpid`` it and the reap falls to PID 1 or the nearest ``PR_SET_CHILD_SUBREAPER`` ancestor.
1028+
A reap is therefore STRICTLY AFTER the exit that produced the EOF, by an interval nothing in
1029+
this repo controls. Measured on Linux with the same shape: microseconds under systemd, and
1030+
never at all inside a 5s busy-poll under a subreaper that is not in a ``waitpid`` loop — which
1031+
is what a containerised CI leg or a ``systemd --user`` session supplies. Windows has no zombie
1032+
state to be caught by, and measured on this host it never once reported ``STILL_ACTIVE`` at
1033+
pipe-EOF (0 of 30, plain and slow-teardown grandchildren both), so the exposure is a POSIX one.
1034+
1035+
The secondary is not redundant with the primary: it is the guard against the primary going
1036+
VACUOUSLY green if someone later edits ``_ORPHAN_GRAPH`` so the grandchild no longer holds fd 1,
1037+
in which case EOF would fire on the worker's death alone and say nothing about the tree.
1038+
1039+
See the FALSIFICATION recorded in the lane report: forcing ``_assign_kill_on_close_job`` to
1040+
return ``None`` degrades the Windows path to a bare ``proc.kill()``, the grandchild survives,
1041+
the pipe never EOFs, and this test goes red."""
9711042
registry, config_dir, pidfile = _orphan_graph(tmp_path)
9721043
session = _session(config_dir)
9731044
grandchild_pid: int | None = None
@@ -994,8 +1065,20 @@ def test_worker_kill_reaps_the_whole_process_tree(tmp_path: Path) -> None:
9941065
"response pipe never reached EOF -- a grandchild still holds it; the worker tree "
9951066
"was not reaped"
9961067
)
997-
# SECONDARY: the process half, asserted directly.
998-
assert not _pid_alive(grandchild_pid), "the grandchild survived the worker kill"
1068+
# SECONDARY: the process half, asserted directly — NOT-RUNNING, not reaped (see docstring).
1069+
# The 5s bound is derived from the FALSIFICATION MARGIN, not from any reaper's latency: the
1070+
# grandchild sleeps 30s, so one that genuinely survived the kill is still running through
1071+
# every one of these seconds and the assert stays red. It also matches `_kill`'s own
1072+
# `proc.wait(timeout=5)`. Waiting here can therefore only absorb a process's exit tail; it
1073+
# can never convert the defect into a pass.
1074+
running = _wait_until_not_running(grandchild_pid, timeout=5.0)
1075+
# `None` means the platform cannot separate a zombie from a runner (POSIX without /proc,
1076+
# e.g. macOS) and there is no sound process-half assertion to make there. Neither CI
1077+
# platform is one — Windows has no zombie state, Linux has /proc — so pin that: a `None`
1078+
# on either is `_pid_running` having broken, and must not slip through as "nothing to say".
1079+
if sys.platform == "win32" or sys.platform.startswith("linux"):
1080+
assert running is not None, "_pid_running went blind on a platform CI actually runs"
1081+
assert running is not True, "the grandchild is still running after the worker kill"
9991082
finally:
10001083
if grandchild_pid is not None:
10011084
_best_effort_kill_pid(grandchild_pid)

0 commit comments

Comments
 (0)