fix: kill the whole process tree on Windows spawn-test timeout (#13)
CI / Lint (ruff) (pull_request) Successful in 7s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 8s
CI / Tests (py3.12 / windows-latest) (pull_request) Successful in 20s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 9s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 8s
CI / Lint (ruff) (pull_request) Successful in 7s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 8s
CI / Tests (py3.12 / windows-latest) (pull_request) Successful in 20s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 9s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 8s
Popen.kill() only terminated the direct child, so runner-style commands (npx -> node -> server) leaked the real server process on every Windows spawn test. taskkill /PID <pid> /T /F walks the descendant tree. Also sets CREATE_NO_WINDOW on the spawned test process so the windowed exe doesn't flash a console per test. Closes #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+22
-1
@@ -1581,6 +1581,23 @@ def server_log_path(name: str) -> Path | None:
|
|||||||
_STDERR_CAP = 4096 # bytes
|
_STDERR_CAP = 4096 # bytes
|
||||||
|
|
||||||
|
|
||||||
|
def _kill_process_tree_windows(pid: int) -> None:
|
||||||
|
"""
|
||||||
|
Kill `pid` and its whole descendant tree via `taskkill /T /F` (issue #13).
|
||||||
|
|
||||||
|
Popen.kill() only terminates the direct child; runner-style commands
|
||||||
|
(npx → node → server, cmd → real process) leave the actual server alive,
|
||||||
|
leaking a process on every Windows spawn test. taskkill walks the tree.
|
||||||
|
"""
|
||||||
|
flags = getattr(subprocess, "CREATE_NO_WINDOW", 0) # no console flash from the GUI exe
|
||||||
|
with contextlib.suppress(OSError):
|
||||||
|
subprocess.run(
|
||||||
|
["taskkill", "/PID", str(pid), "/T", "/F"],
|
||||||
|
capture_output=True,
|
||||||
|
creationflags=flags,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def spawn_test(data: dict, timeout: float = 3.0) -> dict:
|
def spawn_test(data: dict, timeout: float = 3.0) -> dict:
|
||||||
"""
|
"""
|
||||||
Attempt to start a stdio server and observe it for `timeout` seconds.
|
Attempt to start a stdio server and observe it for `timeout` seconds.
|
||||||
@@ -1675,6 +1692,10 @@ def _spawn_test_impl(data: dict, timeout: float) -> dict:
|
|||||||
)
|
)
|
||||||
if os.name != "nt":
|
if os.name != "nt":
|
||||||
popen_kwargs["start_new_session"] = True # own process group → clean kill
|
popen_kwargs["start_new_session"] = True # own process group → clean kill
|
||||||
|
else:
|
||||||
|
# The packaged app is windowed (console=False); without this every
|
||||||
|
# spawn test of a console server flashes a console window.
|
||||||
|
popen_kwargs["creationflags"] = getattr(subprocess, "CREATE_NO_WINDOW", 0)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
proc = subprocess.Popen(args_list, **popen_kwargs)
|
proc = subprocess.Popen(args_list, **popen_kwargs)
|
||||||
@@ -1696,7 +1717,7 @@ def _spawn_test_impl(data: dict, timeout: float) -> dict:
|
|||||||
if os.name != "nt":
|
if os.name != "nt":
|
||||||
os.killpg(os.getpgid(proc.pid), _signal.SIGKILL)
|
os.killpg(os.getpgid(proc.pid), _signal.SIGKILL)
|
||||||
else:
|
else:
|
||||||
proc.kill() # best-effort on Windows
|
_kill_process_tree_windows(proc.pid)
|
||||||
except OSError:
|
except OSError:
|
||||||
pass
|
pass
|
||||||
with contextlib.suppress(subprocess.TimeoutExpired):
|
with contextlib.suppress(subprocess.TimeoutExpired):
|
||||||
|
|||||||
@@ -329,6 +329,55 @@ def test_spawn_test_large_stderr_does_not_deadlock():
|
|||||||
assert len(r["stderr"]) <= c._STDERR_CAP
|
assert len(r["stderr"]) <= c._STDERR_CAP
|
||||||
|
|
||||||
|
|
||||||
|
def test_windows_tree_kill_uses_taskkill(monkeypatch):
|
||||||
|
"""The Windows kill path must walk the process tree (taskkill /T /F);
|
||||||
|
Popen.kill() alone orphans grandchildren (issue #13)."""
|
||||||
|
calls = []
|
||||||
|
|
||||||
|
def fake_run(cmd, **kwargs):
|
||||||
|
calls.append(cmd)
|
||||||
|
return _FakeRC()
|
||||||
|
|
||||||
|
class _FakeRC:
|
||||||
|
returncode = 0
|
||||||
|
|
||||||
|
monkeypatch.setattr(c.subprocess, "run", fake_run)
|
||||||
|
c._kill_process_tree_windows(1234)
|
||||||
|
assert calls == [["taskkill", "/PID", "1234", "/T", "/F"]]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.skipif(os.name != "nt", reason="Windows-only: real process-tree kill")
|
||||||
|
def test_spawn_test_windows_kills_child_process_tree(tmp_path):
|
||||||
|
"""A spawn-tested parent that has its own child (npx -> node style) must
|
||||||
|
not leave the child running after the timeout kill (issue #13)."""
|
||||||
|
import subprocess as sp
|
||||||
|
import time as _t
|
||||||
|
|
||||||
|
pid_file = tmp_path / "child.pid"
|
||||||
|
parent_script = (
|
||||||
|
"import subprocess, sys, time\n"
|
||||||
|
"p = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(60)'])\n"
|
||||||
|
f"open({str(pid_file)!r}, 'w').write(str(p.pid))\n"
|
||||||
|
"time.sleep(60)\n"
|
||||||
|
)
|
||||||
|
r = c.spawn_test({"command": sys.executable, "args": ["-c", parent_script]}, timeout=2.0)
|
||||||
|
assert r["outcome"] == "ok" # parent was still running at timeout, then tree-killed
|
||||||
|
assert pid_file.is_file(), "parent never spawned its child"
|
||||||
|
child_pid = int(pid_file.read_text())
|
||||||
|
|
||||||
|
deadline = _t.time() + 5.0
|
||||||
|
alive = True
|
||||||
|
while _t.time() < deadline:
|
||||||
|
out = sp.run(
|
||||||
|
["tasklist", "/FI", f"PID eq {child_pid}"], capture_output=True, text=True
|
||||||
|
).stdout
|
||||||
|
alive = str(child_pid) in out
|
||||||
|
if not alive:
|
||||||
|
break
|
||||||
|
_t.sleep(0.25)
|
||||||
|
assert not alive, f"child pid {child_pid} survived the spawn-test kill"
|
||||||
|
|
||||||
|
|
||||||
def test_spawn_test_non_string_env_value():
|
def test_spawn_test_non_string_env_value():
|
||||||
# Pasted JSON can carry numeric env values ("env": {"PORT": 8080}) that never
|
# Pasted JSON can carry numeric env values ("env": {"PORT": 8080}) that never
|
||||||
# round-trip through the editor. Popen rejects non-str env; spawn_test must
|
# round-trip through the editor. Popen rejects non-str env; spawn_test must
|
||||||
|
|||||||
Reference in New Issue
Block a user