[ISD-3759] Change the threads to daemons, and cleanup processes (#584)

Author: yhaliaw

docs/changelog.md

@@ -2,6 +2,10 @@
 
 This changelog documents user-relevant changes to the GitHub runner charm.
 
+## 2025-06-26
+
+- Fix a process leak internal to the charm.
+
 ## 2025-06-24
 
 - Fix a bug where deleted GitHub Actions Job would cause an endless loop of retries.

github-runner-manager/src/github_runner_manager/cli.py

@@ -82,9 +82,11 @@ def main(
     http_server_args = FlaskArgs(host=host, port=port, debug=debug)
 
     thread_manager = ThreadManager()
-    thread_manager.add_thread(target=partial(start_http_server, config, lock, http_server_args))
     thread_manager.add_thread(
-        target=partial(start_reconcile_service, config, python_path_config, lock)
+        target=partial(start_http_server, config, lock, http_server_args), daemon=True
+    )
+    thread_manager.add_thread(
+        target=partial(start_reconcile_service, config, python_path_config, lock), daemon=True
     )
     thread_manager.start()
 

src/manager_client.py

@@ -64,6 +64,8 @@ def func_with_error_handling(*args: Any, **kwargs: Any) -> Any:
             ) from err
         except requests.ConnectionError as err:
             raise RunnerManagerServiceConnectionError(CONNECTION_ERROR_MESSAGE) from err
+        except requests.ReadTimeout as err:
+            raise RunnerManagerServiceConnectionError(CONNECTION_ERROR_MESSAGE) from err
 
     return func_with_error_handling
 
@@ -122,7 +124,7 @@ def check_runner(self) -> dict[str, str]:
             The information on the runners.
         """
         self.wait_till_ready()
-        response = self._request(_HTTPMethod.GET, "/runner/check")
+        response = self._request(_HTTPMethod.GET, "/runner/check", timeout=600)
         runner_info = json.loads(response.text)
         runner_info["runners"] = tuple(runner_info["runners"])
         runner_info["busy_runners"] = tuple(runner_info["busy_runners"])
@@ -138,7 +140,7 @@ def flush_runner(self, busy: bool = False) -> None:
         """
         self.wait_till_ready()
         params = {"flush-busy": str(busy)}
-        self._request(_HTTPMethod.POST, "/runner/flush", params=params)
+        self._request(_HTTPMethod.POST, "/runner/flush", params=params, timeout=600)
 
     def health_check(self) -> None:
         """Request a health check on the runner manager service.
@@ -151,7 +153,7 @@ def health_check(self) -> None:
                 API requests.
         """
         try:
-            response = self._request(_HTTPMethod.GET, "/health")
+            response = self._request(_HTTPMethod.GET, "/health", timeout=60)
         except requests.HTTPError as err:
             raise RunnerManagerServiceNotReadyError(NOT_READY_ERROR_MESSAGE) from err
         except requests.ConnectionError as err:

src/manager_service.py

@@ -38,6 +38,7 @@
 JOB_MANAGER_PACKAGE_PATH = "./jobmanager/client"
 GITHUB_RUNNER_MANAGER_SERVICE_NAME = "github-runner-manager"
 GITHUB_RUNNER_MANAGER_SERVICE_LOG_DIR = Path("/var/log/github-runner-manager")
+GITHUB_RUNNER_MANAGER_SERVICE_EXECUTABLE_PATH = "/usr/local/bin/github-runner-manager"
 
 _INSTALL_ERROR_MESSAGE = "Unable to install github-runner-manager package from source"
 _SERVICE_SETUP_ERROR_MESSAGE = "Unable to enable or start the github-runner-manager application"
@@ -62,6 +63,20 @@ def setup(state: CharmState, app_name: str, unit_name: str) -> None:
             systemd.service_stop(GITHUB_RUNNER_MANAGER_SERVICE_NAME)
     except SystemdError as err:
         raise RunnerManagerApplicationStartError(_SERVICE_SETUP_ERROR_MESSAGE) from err
+    # Currently, there is some multiprocess issues that cause leftover processes.
+    # This is a temp patch to clean them up.
+    output, code = execute_command(
+        ["/usr/bin/pkill", "-f", GITHUB_RUNNER_MANAGER_SERVICE_EXECUTABLE_PATH], check_exit=False
+    )
+    if code == 1:
+        logger.info("No leftover github-runner-manager process to clean up.")
+    elif code == 0:
+        logger.warning("Clean up leftover processes.")
+    else:
+        logger.warning(
+            "Unexpected return code %s of pkill for cleanup processes: %s", code, output
+        )
+
     config = create_application_configuration(state, app_name, unit_name)
     config_file = _setup_config_file(config)
     GITHUB_RUNNER_MANAGER_SERVICE_LOG_DIR.mkdir(parents=True, exist_ok=True)

tests/integration/test_reactive.py

@@ -250,6 +250,7 @@ async def test_reactive_mode_scale_down(
 
     # we assume that the runner got deleted while running the job, so we expect a failed job
     await wait_for_completion(run, conclusion="failure")
+    await wait_for_reconcile(app)
     assert_queue_is_empty(mongodb_uri, app.name)
 
     # 2. Spawn a job.

tests/unit/test_manager_service.py

@@ -42,7 +42,7 @@ def mock_systemd_fixture(monkeypatch):
 
 @pytest.fixture(name="mock_execute_command")
 def mock_execute_command_fixture(monkeypatch):
-    mock_execute_command = MagicMock()
+    mock_execute_command = MagicMock(return_value=("Mock", 0))
     monkeypatch.setattr("manager_service.execute_command", mock_execute_command)
     return mock_execute_command
 
@@ -63,6 +63,7 @@ def test_setup_started(
     mock_service_path: Path,
     mock_home_path: Path,
     mock_systemd: MagicMock,
+    mock_execute_command: MagicMock,
 ):
     """
     arrange: Mock the service to be running.
@@ -93,7 +94,10 @@ def test_setup_started(
 
 
 def test_setup_no_started(
-    patch_file_paths: None, complete_charm_state: CharmState, mock_systemd: MagicMock
+    patch_file_paths: None,
+    complete_charm_state: CharmState,
+    mock_systemd: MagicMock,
+    mock_execute_command: MagicMock,
 ):
     """
     arrange: Mock the service to be not running.
@@ -108,7 +112,10 @@ def test_setup_no_started(
 
 
 def test_setup_systemd_error(
-    patch_file_paths: None, complete_charm_state: CharmState, mock_systemd: MagicMock
+    patch_file_paths: None,
+    complete_charm_state: CharmState,
+    mock_systemd: MagicMock,
+    mock_execute_command: MagicMock,
 ):
     """
     arrange: Mock the systemd to raise error.