Skip to content

Commit 132e8fc

Browse files
committed
test: give the Windows descendant probe its own budget
When a suite leader has already exited, taskkill /T cannot walk its tree, so run-test-wave.py proves cleanup by asking Windows whether anything is still parented to the dead PID. That probe was timed with --kill-grace, which the parallel gate passes as 1s. A cold powershell.exe + CIM start routinely costs more than that on a runner, so TimeoutExpired -- a fact about interpreter start-up, not about the process tree -- was read as "assume the worst" and failed an already-clean shard with exit 2. Give the probe its own stable-state budget (15s) plus one retry, keeping the fail-closed semantics: an exhausted or failed probe still counts as "cannot prove absence". The refusal now names which of the two happened, an unfinished probe or a counted set of live descendants, instead of reporting both as a leaked tree. The contract test pins this structurally -- no sleeps, no timing thresholds: the probe must be bounded by the module constant rather than by the kill-grace argument, must retry a timed-out probe, must never read an unusable probe as absence, and must report the two failures distinctly. Its Windows leader-exit case now waits on the scheduler's refusal rather than on a deadline shorter than the new budget. Refs #1741 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
1 parent ffb5ebe commit 132e8fc

2 files changed

Lines changed: 156 additions & 26 deletions

File tree

‎scripts/run-test-wave.py‎

Lines changed: 58 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,20 @@
2828
SLOW_SUITES = frozenset(("incremental", "store_arch", "daemon_runtime"))
2929
POLL_SECONDS = 0.05
3030

31+
# WHY: the Windows descendant probe below is a cold `powershell.exe` + CIM
32+
# start. On a GitHub Windows runner that routinely costs seconds -- interpreter
33+
# start-up, module autoload, CIM service warm-up -- and that cost is unrelated
34+
# to the state of the tree being proven. --kill-grace bounds how long a
35+
# *process* may resist termination and CI passes 1s, so timing the probe with
36+
# it made the proof a function of interpreter latency instead of the tree: a
37+
# cold start blew the 1s budget, TimeoutExpired became "assume the worst", and
38+
# an already-clean shard exited 2. This is a stable-state budget, not a race
39+
# tune -- the answer does not change with waiting, the budget only has to cover
40+
# a cold start, and a probe that still cannot finish is reported as an
41+
# unfinished probe rather than as a leaked tree.
42+
WINDOWS_DESCENDANT_PROBE_SECONDS = 15
43+
WINDOWS_DESCENDANT_PROBE_ATTEMPTS = 2
44+
3145

3246
@dataclass
3347
class ActiveSuite:
@@ -123,37 +137,54 @@ def start_suite(
123137
)
124138

125139

126-
def windows_descendants(pid: int, timeout: int) -> bool:
127-
"""True if any live process still claims `pid` as its parent.
140+
def windows_tree_cleanup_blocker(pid: int) -> str | None:
141+
"""Why `pid`'s tree cannot be called clean, or None when it provably is.
128142
129143
Used only when the suite leader has already exited: `taskkill /T` cannot
130144
walk a tree from a dead PID, so cleanup is proven by asking whether anything
131145
is still parented to it. One level deep on purpose -- Windows does not
132146
reparent orphans, so a grandchild keeps pointing at its own (dead) parent
133147
and would not be found here. That is a weaker proof than taskkill /T, which
134148
is why it is reserved for the case where the strong proof is impossible.
149+
150+
Fail-closed: a probe that times out, cannot start, or reports failure is
151+
never read as absence. The reason names WHICH of the two happened -- a probe
152+
that did not finish, or a counted set of live descendants -- because those
153+
are different defects and used to be reported with the same sentence.
135154
"""
136-
try:
137-
completed = subprocess.run(
138-
[
139-
"powershell.exe",
140-
"-NoProfile",
141-
"-NonInteractive",
142-
"-Command",
143-
"@(Get-CimInstance Win32_Process -Filter "
144-
f"'ParentProcessId={pid}').Count",
145-
],
146-
check=False,
147-
stdin=subprocess.DEVNULL,
148-
capture_output=True,
149-
text=True,
150-
timeout=timeout,
151-
)
152-
except (OSError, subprocess.TimeoutExpired):
153-
return True # cannot prove absence -> assume the worst
154-
if completed.returncode != 0:
155-
return True
156-
return (completed.stdout or "").strip() not in ("0", "")
155+
unproven = "descendant probe did not run"
156+
for _ in range(WINDOWS_DESCENDANT_PROBE_ATTEMPTS):
157+
try:
158+
completed = subprocess.run(
159+
[
160+
"powershell.exe",
161+
"-NoProfile",
162+
"-NonInteractive",
163+
"-Command",
164+
"@(Get-CimInstance Win32_Process -Filter "
165+
f"'ParentProcessId={pid}').Count",
166+
],
167+
check=False,
168+
stdin=subprocess.DEVNULL,
169+
capture_output=True,
170+
text=True,
171+
timeout=WINDOWS_DESCENDANT_PROBE_SECONDS,
172+
)
173+
except subprocess.TimeoutExpired:
174+
unproven = (
175+
"descendant probe could not complete in "
176+
f"{WINDOWS_DESCENDANT_PROBE_SECONDS}s"
177+
)
178+
continue
179+
except OSError as exc:
180+
return f"descendant probe could not run: {exc}"
181+
if completed.returncode != 0:
182+
return f"descendant probe failed (rc={completed.returncode})"
183+
count = (completed.stdout or "").strip()
184+
if count in ("0", ""):
185+
return None
186+
return f"{count} live descendant(s)"
187+
return unproven
157188

158189

159190
def terminate_process_tree(active: ActiveSuite, kill_grace: int) -> None:
@@ -167,9 +198,11 @@ def terminate_process_tree(active: ActiveSuite, kill_grace: int) -> None:
167198
# how a deliberately-hanging fixture suite reddened a release run.
168199
# taskkill /T cannot walk a tree from a dead PID, so prove cleanup
169200
# the only way still available -- nothing is parented to it.
170-
if windows_descendants(process.pid, kill_grace):
201+
blocker = windows_tree_cleanup_blocker(process.pid)
202+
if blocker is not None:
171203
raise RuntimeError(
172-
f"suite {active.name!r} leader exited leaving live descendants"
204+
f"suite {active.name!r} leader exited and tree cleanup "
205+
f"could not be proven: {blocker}"
173206
)
174207
return
175208
try:

‎tests/test_parallel_harness_contract.sh‎

Lines changed: 98 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,99 @@ if ! grep -Fq 'run-test-wave.py' "$driver"; then
3030
exit 1
3131
fi
3232

33+
# The Windows descendant proof must not be timed by --kill-grace. That argument
34+
# bounds how long a *process* may resist termination (this file runs the
35+
# scheduler with 1s); the probe is a cold PowerShell + CIM start that routinely
36+
# costs seconds on a runner. Binding one to the other made the verdict a
37+
# function of interpreter latency: a slow start became "assume the worst" and
38+
# reddened an already-clean shard. Asserted structurally -- no sleeps, no timing
39+
# thresholds -- so the contract stays deterministic on every platform.
40+
# (Command substitution, not `| grep -q`: under pipefail an early-exiting
41+
# reader can hand the writer EPIPE and turn a satisfied match into status 141.)
42+
probe_sites=$(grep -n 'windows_tree_cleanup_blocker(' "$scheduler" || true)
43+
if [[ "$probe_sites" == *kill_grace* ]]; then
44+
echo "FAIL: the Windows descendant probe is still timed by --kill-grace" >&2
45+
exit 1
46+
fi
47+
48+
python3 - "$scheduler" <<'PROBE'
49+
from __future__ import annotations
50+
51+
import importlib.util
52+
import subprocess
53+
import sys
54+
55+
56+
spec = importlib.util.spec_from_file_location("cbm_run_test_wave", sys.argv[1])
57+
module = importlib.util.module_from_spec(spec)
58+
# @dataclass resolves its own module out of sys.modules; register before exec.
59+
sys.modules[spec.name] = module
60+
spec.loader.exec_module(module)
61+
62+
budget = getattr(module, "WINDOWS_DESCENDANT_PROBE_SECONDS", None)
63+
if not isinstance(budget, int) or budget < 15:
64+
raise SystemExit(
65+
"FAIL: the descendant probe has no independent budget "
66+
f"(WINDOWS_DESCENDANT_PROBE_SECONDS={budget!r})"
67+
)
68+
69+
probe = module.windows_tree_cleanup_blocker
70+
original_run = subprocess.run
71+
observed: list[object] = []
72+
73+
74+
def timing_out(*args: object, **kwargs: object) -> object:
75+
observed.append(kwargs.get("timeout"))
76+
raise subprocess.TimeoutExpired(cmd="probe", timeout=kwargs.get("timeout"))
77+
78+
79+
class _Completed:
80+
def __init__(self, stdout: str) -> None:
81+
self.returncode = 0
82+
self.stdout = stdout
83+
84+
85+
try:
86+
subprocess.run = timing_out
87+
timed_out_reason = probe(4321)
88+
subprocess.run = lambda *a, **k: _Completed("3\n")
89+
live_reason = probe(4321)
90+
subprocess.run = lambda *a, **k: _Completed("0\n")
91+
clean_reason = probe(4321)
92+
finally:
93+
subprocess.run = original_run
94+
95+
if observed != [budget] * len(observed):
96+
raise SystemExit(
97+
"FAIL: the descendant probe is not bounded by its own budget "
98+
f"(timeouts={observed})"
99+
)
100+
if len(observed) < 2:
101+
raise SystemExit(
102+
"FAIL: the descendant probe does not retry a timed-out probe "
103+
f"(attempts={len(observed)})"
104+
)
105+
if timed_out_reason is None or live_reason is None:
106+
raise SystemExit(
107+
"FAIL: the descendant probe stopped failing closed "
108+
f"(timed_out={timed_out_reason!r}, live={live_reason!r})"
109+
)
110+
if clean_reason is not None:
111+
raise SystemExit(f"FAIL: a clean tree was not proven clean ({clean_reason!r})")
112+
if "could not complete" not in timed_out_reason:
113+
raise SystemExit(
114+
f"FAIL: an unfinished probe is not named as one ({timed_out_reason!r})"
115+
)
116+
if "3 live descendant" not in live_reason:
117+
raise SystemExit(
118+
f"FAIL: proven descendants are not reported with their count ({live_reason!r})"
119+
)
120+
if timed_out_reason == live_reason:
121+
raise SystemExit(
122+
"FAIL: an unfinished probe and a leaked tree are reported identically"
123+
)
124+
PROBE
125+
33126
cat >"$fixture/fake_runner.py" <<'PY'
34127
from __future__ import annotations
35128
@@ -370,7 +463,11 @@ try:
370463
raise SystemExit("FAIL: scheduler did not observe the forced leader exit")
371464
time.sleep(0.02)
372465
release.write_text("release\n", encoding="utf-8")
373-
stdout, stderr = process.communicate(timeout=8)
466+
# Generous on purpose: the scheduler's refusal is the asserted state, and
467+
# on Windows it now spends up to the descendant-probe budget (twice --
468+
# once in the wave loop, once in the cleanup pass) before refusing. This
469+
# bound only has to exceed that worst case; it never decides the verdict.
470+
stdout, stderr = process.communicate(timeout=120)
374471
375472
if os.name == "nt":
376473
# Assert the PROPERTY, not the wording. This used to require the phrase

0 commit comments

Comments
 (0)