From 036e95e93178f206e71c5d4eeb7da1730dce1a35 Mon Sep 17 00:00:00 2001 From: Ivan Yang Date: Tue, 27 Oct 2026 22:18:04 -0700 Subject: [PATCH 1/2] test(cloudxr): poll the run-flags cmdline read in test_background.py Popen() can return while the child is still mid-exec, when its /proc//cmdline reads empty. test_reads_real_cmdline_without_a_trailing_empty read the run flags at once and failed with `assert [] == ['--host-client']`. Replace the per-reader read_pid() poll with _await_exec(), which waits for the child's cmdline itself and fails clearly after 5s. Every test that reads a just-spawned child's cmdline now calls it first, including the two that expect no match: an empty cmdline made those pass without testing anything. Signed-off-by: Ivan Yang --- tests/AGENTS.md | 8 ++++-- tests/python/core/cloudxr/test_background.py | 30 +++++++++++++------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 5d1ea1a328..e87227907d 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -58,9 +58,11 @@ Shared C++ fixtures (not executables) live under `tests/cpp/viz/support/` in-tree configure: standalone wheel builds use the example as `CMAKE_SOURCE_DIR`, not the repo root. - A stand-in child process must stay alive on its own (`python -m ` - exits at once), and a `/proc//cmdline` check made right after `Popen()` - must poll, because the cmdline can still read empty - (`python/core/cloudxr/test_background.py` does both). + exits at once), and `Popen()` can return while the child's + `/proc//cmdline` still reads empty: wait for it to be non-empty before + any check, including one that expects no match, which an empty cmdline + passes without testing anything (`python/core/cloudxr/test_background.py` + does both). ## Adding tests diff --git a/tests/python/core/cloudxr/test_background.py b/tests/python/core/cloudxr/test_background.py index 92fd0185fe..81273a529f 100644 --- a/tests/python/core/cloudxr/test_background.py +++ b/tests/python/core/cloudxr/test_background.py @@ -8,6 +8,7 @@ import subprocess import sys import time +from pathlib import Path import pytest from unittest.mock import MagicMock, patch @@ -33,14 +34,18 @@ def stand_in_service(tmp_path, monkeypatch): monkeypatch.setattr(background, "_MODULE", "stand_in_service") -def _poll_read_pid(run_dir: str) -> int | None: - """read_pid(), polled: a new child's cmdline can be empty when Popen() returns.""" - deadline = time.monotonic() + 2.0 - pid = background.read_pid(run_dir) - while pid is None and time.monotonic() < deadline: - time.sleep(0.05) - pid = background.read_pid(run_dir) - return pid +def _await_exec(pid: int) -> None: + """Wait for *pid*'s cmdline: Popen() can return mid-exec, while it reads empty. + + An empty cmdline matches nothing, so a check made then fails a positive + test and passes a negative one without testing anything. + """ + cmdline = Path(f"/proc/{pid}/cmdline") + deadline = time.monotonic() + 5.0 + while not cmdline.read_bytes(): + if time.monotonic() > deadline: + pytest.fail(f"{cmdline} is still empty 5s after the spawn") + time.sleep(0.01) @_posix_only @@ -65,6 +70,7 @@ def test_none_when_pid_belongs_to_something_else(self, tmp_path): """Pid reuse: the recorded pid is alive, but is not our service.""" other = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"]) try: + _await_exec(other.pid) background.pid_path(str(tmp_path)).write_text(f"{other.pid}\n") assert background.read_pid(str(tmp_path)) is None finally: @@ -81,8 +87,9 @@ def test_recognises_a_service_under_any_shipped_module_name(self, tmp_path, modu [sys.executable, "-c", "import time; time.sleep(30)", module, "run"] ) try: + _await_exec(proc.pid) background.pid_path(str(tmp_path)).write_text(f"{proc.pid}\n") - assert _poll_read_pid(str(tmp_path)) == proc.pid + assert background.read_pid(str(tmp_path)) == proc.pid finally: proc.kill() proc.wait() @@ -99,6 +106,7 @@ def test_a_process_that_merely_names_the_module_is_not_the_service(self, tmp_pat ] ) try: + _await_exec(proc.pid) background.pid_path(str(tmp_path)).write_text(f"{proc.pid}\n") assert background.read_pid(str(tmp_path)) is None finally: @@ -192,6 +200,7 @@ def test_reads_real_cmdline_without_a_trailing_empty(self, tmp_path): ] ) try: + _await_exec(proc.pid) background.pid_path(str(tmp_path)).write_text(f"{proc.pid}\n") flags = background.read_run_flags(str(tmp_path)) assert flags == ["--host-client"] @@ -219,7 +228,8 @@ def test_sigterm_and_clears_the_pid_file(self, tmp_path, stand_in_service): pid, _ = background.spawn(["30"], run_dir, logs_dir) try: - assert _poll_read_pid(run_dir) == pid + _await_exec(pid) + assert background.read_pid(run_dir) == pid assert background.terminate(run_dir, timeout_sec=10) is True assert not background.pid_path(run_dir).exists() From 7ed7d58e4886754747eee3781e59f9784991bb33 Mon Sep 17 00:00:00 2001 From: Ivan Yang Date: Tue, 27 Oct 2026 23:00:50 -0700 Subject: [PATCH 2/2] test(cloudxr): wait for the child's own argv, not just a non-empty cmdline On the posix_spawn path the child shares our memory until exec swaps it, so /proc//cmdline can briefly read pytest's own argv. _await_exec then returned at once, and the next read could still miss the service. CPython 3.13 takes that path by default where os.POSIX_SPAWN_CLOSEFROM exists; uv's managed builds lack it, so CI is unaffected. Forced onto posix_spawn, the previous helper failed 7 of 10,000 runs of the cmdline tests; this one failed none. Signed-off-by: Ivan Yang --- tests/AGENTS.md | 8 ++++---- tests/python/core/cloudxr/test_background.py | 10 ++++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/AGENTS.md b/tests/AGENTS.md index e87227907d..27a847225a 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -59,10 +59,10 @@ Shared C++ fixtures (not executables) live under `tests/cpp/viz/support/` not the repo root. - A stand-in child process must stay alive on its own (`python -m ` exits at once), and `Popen()` can return while the child's - `/proc//cmdline` still reads empty: wait for it to be non-empty before - any check, including one that expects no match, which an empty cmdline - passes without testing anything (`python/core/cloudxr/test_background.py` - does both). + `/proc//cmdline` still reads empty or the parent's argv: wait for the + child's own argv before any check, including one that expects no match, + which either stale value passes without testing anything + (`python/core/cloudxr/test_background.py` does both). ## Adding tests diff --git a/tests/python/core/cloudxr/test_background.py b/tests/python/core/cloudxr/test_background.py index 81273a529f..d1d28b6f15 100644 --- a/tests/python/core/cloudxr/test_background.py +++ b/tests/python/core/cloudxr/test_background.py @@ -35,14 +35,16 @@ def stand_in_service(tmp_path, monkeypatch): def _await_exec(pid: int) -> None: - """Wait for *pid*'s cmdline: Popen() can return mid-exec, while it reads empty. + """Wait for *pid*'s own cmdline: Popen() can return mid-exec. - An empty cmdline matches nothing, so a check made then fails a positive - test and passes a negative one without testing anything. + Until exec finishes it reads empty or, on the posix_spawn path, briefly + this process's argv. Neither names the service, so a check made then fails + a positive test and passes a negative one without testing anything. """ cmdline = Path(f"/proc/{pid}/cmdline") + ours = Path("/proc/self/cmdline").read_bytes() deadline = time.monotonic() + 5.0 - while not cmdline.read_bytes(): + while cmdline.read_bytes() in (b"", ours): if time.monotonic() > deadline: pytest.fail(f"{cmdline} is still empty 5s after the spawn") time.sleep(0.01)