Generalize process tree termination/killing to all controllers

In particular, Solanum needs it in some circumstances.

Author: progval

irctest/basecontrollers.py

@@ -6,6 +6,7 @@
 import os
 from pathlib import Path
 import shutil
+import signal
 import socket
 import subprocess
 import tempfile
@@ -112,7 +113,7 @@ def _used_ports(self) -> Iterator[Set[Tuple[str, int]]]:
     def get_hostname_and_port(self) -> Tuple[str, int]:
         with self._used_ports() as used_ports:
             while True:
-                (hostname, port) = find_hostname_and_port()
+                hostname, port = find_hostname_and_port()
                 if (hostname, port) not in used_ports:
                     # double-checking in self._used_ports to prevent collisions
                     # between controllers starting at the same time.
@@ -129,15 +130,41 @@ def check_is_alive(self) -> None:
         if self.proc.returncode is not None:
             raise ProcessStopped(f"process returned {self.proc.returncode}")
 
+    def _terminate_process_group(self, sig: signal.Signals) -> bool:
+        """Try to send a signal to the entire process group.
+
+        Returns True if successful, False if we should fall back to single process.
+        """
+        assert self.proc
+        try:
+            os.killpg(os.getpgid(self.proc.pid), sig)
+            return True
+        except (ProcessLookupError, PermissionError, OSError):
+            # Process group doesn't exist or we don't have permission
+            return False
+
     def kill_proc(self) -> None:
         """Terminates the controlled process, waits for it to exit, and
-        eventually kills it."""
+        eventually kills it.
+
+        This method kills the entire process group to ensure child processes
+        (e.g., Solanum's authd and ssld helpers) are also terminated.
+        """
         assert self.proc
-        self.proc.terminate()
+
+        # Try to terminate the entire process group first
+        if not self._terminate_process_group(signal.SIGTERM):
+            # Fall back to terminating just this process
+            self.proc.terminate()
+
         try:
             self.proc.wait(5)
         except subprocess.TimeoutExpired:
-            self.proc.kill()
+            # If still running, send SIGKILL to the process group
+            if not self._terminate_process_group(signal.SIGKILL):
+                # Fall back to killing just this process
+                self.proc.kill()
+            self.proc.wait(timeout=10)  # Wait for it to actually die
         self.proc = None
 
     def kill(self) -> None:
@@ -154,6 +181,10 @@ def execute(
         self, command: Sequence[Union[str, Path]], **kwargs: Any
     ) -> subprocess.Popen:
         output_to = None if self.debug_mode else subprocess.DEVNULL
+        # Start in a new process group so we can kill all children together
+        # Note: start_new_session and preexec_fn cannot be used together
+        if "start_new_session" not in kwargs and "preexec_fn" not in kwargs:
+            kwargs["start_new_session"] = True
         return subprocess.Popen(command, stderr=output_to, stdout=output_to, **kwargs)
 
 
@@ -176,7 +207,9 @@ def kill(self) -> None:
     def terminate(self) -> None:
         """Stops the process gracefully, and does not clean its config."""
         assert self.proc
-        self.proc.terminate()
+        # Terminate the entire process group to kill child processes too
+        if not self._terminate_process_group(signal.SIGTERM):
+            self.proc.terminate()
         self.proc.wait()
         self.proc = None
 

irctest/controllers/mammon.py

@@ -1,4 +1,5 @@
 import shutil
+import signal
 from typing import Optional, Set, Type
 
 from irctest.basecontrollers import (
@@ -76,9 +77,14 @@ def create_config(self) -> None:
             pass
 
     def kill_proc(self) -> None:
-        # Mammon does not seem to handle SIGTERM very well
+        # Mammon does not seem to handle SIGTERM very well, so use SIGKILL
         assert self.proc
-        self.proc.kill()
+        # Kill the entire process group to handle child processes
+        if not self._terminate_process_group(signal.SIGKILL):
+            # Fall back to killing just this process
+            self.proc.kill()
+        self.proc.wait()
+        self.proc = None
 
     def run(
         self,

irctest/controllers/sable.py

@@ -1,7 +1,6 @@
 import os
 from pathlib import Path
 import shutil
-import signal
 import subprocess
 import tempfile
 import time
@@ -410,10 +409,8 @@ def run(
                 self.directory / "configs/network_config.conf",
             ],
             cwd=self.directory,
-            preexec_fn=os.setsid,
             env={"RUST_BACKTRACE": "1", **os.environ},
         )
-        self.pgroup_id = os.getpgid(self.proc.pid)
 
         if run_services:
             self.services_controller = SableServicesController(self.test_config, self)
@@ -423,10 +420,6 @@ def run(
                 server_port=services_port,
             )
 
-    def kill_proc(self) -> None:
-        os.killpg(self.pgroup_id, signal.SIGKILL)
-        super().kill_proc()
-
     def registerUser(
         self,
         case: BaseServerTestCase,  # type: ignore
@@ -488,14 +481,8 @@ def run(self, protocol: str, server_hostname: str, server_port: int) -> None:
                 self.server_controller.directory / "configs/network.conf",
             ],
             cwd=self.server_controller.directory,
-            preexec_fn=os.setsid,
             env={"RUST_BACKTRACE": "1", **os.environ},
         )
-        self.pgroup_id = os.getpgid(self.proc.pid)
-
-    def kill_proc(self) -> None:
-        os.killpg(self.pgroup_id, signal.SIGKILL)
-        super().kill_proc()
 
     def wait_for_services(self) -> None:
         # by default, wait_for_services() connects a user that sends a HELP command

irctest/controllers/unrealircd.py

@@ -3,6 +3,7 @@
 import functools
 from pathlib import Path
 import shutil
+import signal
 import subprocess
 import textwrap
 from typing import Callable, ContextManager, Iterator, Optional, Type
@@ -288,7 +289,10 @@ def kill_proc(self) -> None:
         assert self.proc
 
         with _STARTSTOP_LOCK():
-            self.proc.kill()
+            # Kill the entire process group to handle child processes
+            if not self._terminate_process_group(signal.SIGKILL):
+                # Fall back to killing just this process
+                self.proc.kill()
             self.proc.wait(5)  # wait for it to actually die
             self.proc = None