From 5c930dc0807a7c1c2c5f53ec2a192dd23f34c726 Mon Sep 17 00:00:00 2001 From: Jakob Bornecrantz Date: Fri, 23 Oct 2026 13:30:08 +0100 Subject: [PATCH] test(cloudxr): fix flaky terminate race in test_background.py test_sigterm_and_clears_the_pid_file intermittently failed with "assert False is True" from terminate(). Its stand-in for the service, `python -m time run 30`, never slept: a builtin module has no code object, so the child exited within milliseconds. The test passed only if terminate() read /proc//cmdline before then; a short stall on a loaded CI runner found a zombie with an empty cmdline instead, so read_pid() returned None. Unrelated to the CloudXR runtime, which this code path never loads. Add a stand_in_service fixture that runs a module sleeping for its last argument, and use it in the three tests that relied on `-m time`. Move the cmdline poll from #1209 into _poll_read_pid() and wait on it before terminate(), since a fresh child's cmdline can still be empty when Popen() returns. Record both pitfalls in tests/AGENTS.md. Reproduced by stalling 50 ms after spawn(): the old test fails as on CI, the new one passes. Signed-off-by: Jakob Bornecrantz --- tests/AGENTS.md | 4 ++ tests/python/core/cloudxr/test_background.py | 45 +++++++++++++------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/tests/AGENTS.md b/tests/AGENTS.md index e777b9c8f5..5d1ea1a328 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -57,6 +57,10 @@ Shared C++ fixtures (not executables) live under `tests/cpp/viz/support/` - If CMake under `examples/` reads a file under `tests/`, guard it on the 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). ## Adding tests diff --git a/tests/python/core/cloudxr/test_background.py b/tests/python/core/cloudxr/test_background.py index 6f94a0c687..d135ce1334 100644 --- a/tests/python/core/cloudxr/test_background.py +++ b/tests/python/core/cloudxr/test_background.py @@ -19,6 +19,30 @@ ) +@pytest.fixture +def stand_in_service(tmp_path, monkeypatch): + """Have spawn() run a module that sleeps for its last argument. + + Stands in for the real service, which needs a GPU. Not ``-m time``: a + builtin has no code object, so that child exits within milliseconds. + """ + (tmp_path / "stand_in_service.py").write_text( + "import sys, time\ntime.sleep(float(sys.argv[-1]))\n" + ) + monkeypatch.setenv("PYTHONPATH", str(tmp_path)) + 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 + + @_posix_only class TestReadPid: """A pid file outlives its process, and pids get reused.""" @@ -58,15 +82,7 @@ def test_recognises_a_service_under_any_shipped_module_name(self, tmp_path, modu ) try: background.pid_path(str(tmp_path)).write_text(f"{proc.pid}\n") - # A freshly spawned pid's /proc//cmdline isn't guaranteed to be - # populated the instant Popen() returns, especially on a loaded CI - # runner - poll briefly instead of asserting on the very first read. - deadline = time.monotonic() + 2.0 - pid = background.read_pid(str(tmp_path)) - while pid is None and time.monotonic() < deadline: - time.sleep(0.05) - pid = background.read_pid(str(tmp_path)) - assert pid == proc.pid + assert _poll_read_pid(str(tmp_path)) == proc.pid finally: proc.kill() proc.wait() @@ -94,13 +110,11 @@ def test_a_process_that_merely_names_the_module_is_not_the_service(self, tmp_pat class TestSpawn: """Tests for the detached spawn itself.""" - def test_child_leads_its_own_session(self, tmp_path, monkeypatch): + def test_child_leads_its_own_session(self, tmp_path, stand_in_service): """The whole point: a signal aimed at our process group must miss it.""" run_dir = str(tmp_path / "run") logs_dir = tmp_path / "logs" - # Stand in for `-m isaaccapture.cloudxr.service run`, which needs a GPU. - monkeypatch.setattr(background, "_MODULE", "time") pid, log = background.spawn(["30"], run_dir, logs_dir) try: assert os.getsid(pid) == pid # session leader @@ -139,14 +153,13 @@ def test_the_log_socket_is_not_inherited(self, tmp_path, monkeypatch): assert "ISAACCAPTURE_LOG_SOCKET" not in env assert env["PYTHONUNBUFFERED"] == "1" - def test_log_is_appended_not_truncated(self, tmp_path, monkeypatch): + def test_log_is_appended_not_truncated(self, tmp_path, stand_in_service): """A restart must not erase the log that explains the last crash.""" run_dir = str(tmp_path / "run") logs_dir = tmp_path / "logs" logs_dir.mkdir(parents=True) background.log_path(logs_dir).write_text("earlier run\n") - monkeypatch.setattr(background, "_MODULE", "time") pid, log = background.spawn(["30"], run_dir, logs_dir) try: assert log.read_text().startswith("earlier run") @@ -200,12 +213,12 @@ def test_removes_a_stale_pid_file(self, tmp_path): assert background.terminate(str(tmp_path)) is False assert not background.pid_path(str(tmp_path)).exists() - def test_sigterm_and_clears_the_pid_file(self, tmp_path, monkeypatch): + def test_sigterm_and_clears_the_pid_file(self, tmp_path, stand_in_service): run_dir = str(tmp_path / "run") logs_dir = tmp_path / "logs" - monkeypatch.setattr(background, "_MODULE", "time") pid, _ = background.spawn(["30"], run_dir, logs_dir) + assert _poll_read_pid(run_dir) == pid assert background.terminate(run_dir, timeout_sec=10) is True assert not background.pid_path(run_dir).exists()