Skip to content

Commit 27cf66b

Browse files
committed
fix: Do not fail an action whose subprocess exits immediately
CI surfaced this on macOS 3.12 as a flaky failure in an unrelated wrap test, and it is the same root cause as the Windows-only flake that has been hitting one of six Windows jobs per run: posix: os.getpgid(pid) raises ProcessLookupError (ESRCH) windows: psutil raises NoSuchProcess ('process PID not found (pid=...)') Both are reached when a trivial command finishes before the runner has finished recording it. Both were raised on the run future, so a subprocess that ran to completion was reported as a failed action, and the session went to READY_ENDING -- which is why a wrap test asserting READY between tasks failed intermittently on the second of three back-to-back echo actions. - _subprocess.run: an already-reaped child has no process group to look up. Fall back to its own pid, which is the group we would have found since the child is the group leader. - _windows_process_killer._suspend_process_tree: a process that exits between being discovered and being walked has no children to suspend. Mirrors the NoSuchProcess handling already present elsewhere in that module. Tests: the posix guard is pinned by patching os.getpgid to raise (verified against a mutant that stops catching it); the Windows walk is pinned with a psutil mock, gated to Windows. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
1 parent 0bebd29 commit 27cf66b

3 files changed

Lines changed: 90 additions & 9 deletions

File tree

src/openjd/sessions/_subprocess.py

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,13 +177,22 @@ def run(self) -> None:
177177
# Would use is_posix(), but it doesn't short-circuit mypy which then complains
178178
# about os.getpgid not being a valid attribute.
179179
if not sys.platform == "win32":
180-
if not self._user or self._user.is_process_user():
181-
self._sudo_child_process_group_id = os.getpgid(self._process.pid)
182-
else:
183-
self._sudo_child_process_group_id = find_sudo_child_process_group_id(
184-
logger=self._logger,
185-
sudo_process=self._process,
186-
)
180+
# A trivial command can exit before we get here. Looking up the
181+
# process group of an already-reaped child raises ProcessLookupError
182+
# (ESRCH), which must not fail the action: the child ran, and its
183+
# exit code is still collected below. Fall back to its own pid --
184+
# the process group we would have found, since the child is the
185+
# group leader (start_new_session=True).
186+
try:
187+
if not self._user or self._user.is_process_user():
188+
self._sudo_child_process_group_id = os.getpgid(self._process.pid)
189+
else:
190+
self._sudo_child_process_group_id = find_sudo_child_process_group_id(
191+
logger=self._logger,
192+
sudo_process=self._process,
193+
)
194+
except ProcessLookupError:
195+
self._sudo_child_process_group_id = self._process.pid
187196

188197
self._logger.info(
189198
f"Command started as pid: {self._process.pid}",

src/openjd/sessions/_windows_process_killer.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,21 @@ def _suspend_process_tree(
6060
if not suspend_subprocesses:
6161
return
6262

63-
# Recursively suspend child processes.
64-
for child in process.children():
63+
# Recursively suspend child processes. A process that exited between being
64+
# discovered and being walked has no children to suspend, and must not fail
65+
# the cancel: psutil raises NoSuchProcess ("process PID not found") here, and
66+
# this runs on the run future, so the exception would surface as a failed
67+
# action for a subprocess that in fact completed.
68+
try:
69+
children = process.children()
70+
except NoSuchProcess:
71+
logger.info(
72+
f"Process {process.pid} exited before its children could be listed.",
73+
extra=LogExtraInfo(openjd_log_content=LogContent.PROCESS_CONTROL),
74+
)
75+
return
76+
77+
for child in children:
6578
_suspend_process_tree(
6679
logger, child, all_processes, procs_cannot_suspend, suspend_subprocesses
6780
)

test/openjd/sessions_v0/test_subprocess.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
from openjd.sessions._os_checker import is_posix, is_windows
1919
from openjd.sessions._session_user import PosixSessionUser, WindowsSessionUser
2020
from openjd.sessions._subprocess import LoggingSubprocess
21+
from openjd.sessions import _subprocess as subprocess_impl_mod
2122

2223
from .conftest import (
2324
build_logger,
@@ -1069,3 +1070,61 @@ def end_proc():
10691070
if num_children_running == 0:
10701071
break
10711072
assert num_children_running == 0
1073+
1074+
1075+
class TestFastExitingChild:
1076+
"""A trivial command can exit before the runner finishes recording it.
1077+
1078+
Looking up an already-reaped child's process group raises ProcessLookupError
1079+
on posix, and psutil raises NoSuchProcess when walking it on Windows. Neither
1080+
may fail the action: the child ran, and its exit code is still collected.
1081+
"""
1082+
1083+
@pytest.mark.skipif(not is_posix(), reason="posix-only: process groups")
1084+
@pytest.mark.usefixtures("message_queue", "queue_handler")
1085+
def test_getpgid_lookup_failure_does_not_fail_the_action(
1086+
self,
1087+
message_queue: SimpleQueue,
1088+
queue_handler: QueueHandler,
1089+
) -> None:
1090+
# GIVEN: the child is gone by the time its process group is looked up
1091+
logger = build_logger(queue_handler)
1092+
callback = MagicMock()
1093+
subproc = LoggingSubprocess(
1094+
logger=logger,
1095+
args=[sys.executable, "-c", "print('DONE')"],
1096+
callback=callback,
1097+
)
1098+
1099+
with patch.object(
1100+
subprocess_impl_mod.os, "getpgid", side_effect=ProcessLookupError(3, "No such process")
1101+
):
1102+
# WHEN
1103+
subproc.run()
1104+
1105+
# THEN: the action completed normally
1106+
assert subproc.exit_code == 0
1107+
assert subproc.failed_to_start is False
1108+
messages = collect_queue_messages(message_queue)
1109+
assert "DONE" in messages
1110+
1111+
@pytest.mark.skipif(not is_windows(), reason="Windows-only: psutil process walk")
1112+
def test_process_tree_walk_tolerates_exited_process(self) -> None:
1113+
# GIVEN: a process that disappears between discovery and the walk
1114+
from psutil import NoSuchProcess
1115+
1116+
from openjd.sessions._windows_process_killer import _suspend_process_tree
1117+
1118+
logger = MagicMock()
1119+
process = MagicMock()
1120+
process.pid = 4321
1121+
process.suspend.side_effect = NoSuchProcess(4321)
1122+
process.children.side_effect = NoSuchProcess(4321)
1123+
cannot_suspend: list = []
1124+
all_processes: list = []
1125+
1126+
# WHEN / THEN: no exception escapes
1127+
_suspend_process_tree(
1128+
logger, process, all_processes, cannot_suspend, suspend_subprocesses=True
1129+
)
1130+
assert process in all_processes

0 commit comments

Comments
 (0)