From bc1df4920188ebee7f8897338f30e80fe6954729 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Mon, 22 Jun 2026 16:07:19 +0800 Subject: [PATCH] fix(gateway): handle lifecycle edge cases --- nanobot/gateway/runtime.py | 26 +++++++++++++------------- nanobot/gateway/service.py | 6 +++--- tests/gateway/test_gateway_service.py | 24 ++++++++++++++++++++++++ tests/gateway/test_runtime.py | 24 +++++++++++++++++++++++- 4 files changed, 63 insertions(+), 17 deletions(-) diff --git a/nanobot/gateway/runtime.py b/nanobot/gateway/runtime.py index fd79ac60..2627c0ac 100644 --- a/nanobot/gateway/runtime.py +++ b/nanobot/gateway/runtime.py @@ -178,7 +178,8 @@ class GatewayRuntime: self._clear_state() return RuntimeResult(False, "gateway_state_stale", self.status(reason="stale_state")) - self._terminate(status.pid, timeout_s=timeout_s) + if not self._terminate(status.pid, timeout_s=timeout_s): + return RuntimeResult(False, "gateway_stop_timeout", self.status(reason="stop_timeout")) self._clear_state() return RuntimeResult(True, "gateway_stopped", self.status(reason="stopped")) @@ -263,13 +264,12 @@ class GatewayRuntime: return {"creationflags": flags} return {"start_new_session": True} - def _terminate(self, pid: int, *, timeout_s: int) -> None: + def _terminate(self, pid: int, *, timeout_s: int) -> bool: if self.platform_name == "Windows": - self._terminate_windows(pid, timeout_s=timeout_s) - else: - self._terminate_posix(pid, timeout_s=timeout_s) + return self._terminate_windows(pid, timeout_s=timeout_s) + return self._terminate_posix(pid, timeout_s=timeout_s) - def _terminate_posix(self, pid: int, *, timeout_s: int) -> None: + def _terminate_posix(self, pid: int, *, timeout_s: int) -> bool: try: pgid = os.getpgid(pid) except OSError: @@ -280,28 +280,28 @@ class GatewayRuntime: else: os.kill(pid, signal.SIGTERM) except ProcessLookupError: - return + return True if self._wait_for_exit(pid, timeout_s): - return + return True with suppress(ProcessLookupError): if pgid is not None: os.killpg(pgid, signal.SIGKILL) else: os.kill(pid, signal.SIGKILL) - self._wait_for_exit(pid, 2) + return self._wait_for_exit(pid, 2) - def _terminate_windows(self, pid: int, *, timeout_s: int) -> None: + def _terminate_windows(self, pid: int, *, timeout_s: int) -> bool: ctrl_break = getattr(signal, "CTRL_BREAK_EVENT", None) if ctrl_break is not None: with suppress(ProcessLookupError): os.kill(pid, ctrl_break) if self._wait_for_exit(pid, timeout_s): - return + return True self._subprocess_run(["taskkill", "/PID", str(pid), "/T"], check=False) if self._wait_for_exit(pid, 2): - return + return True self._subprocess_run(["taskkill", "/PID", str(pid), "/T", "/F"], check=False) - self._wait_for_exit(pid, 2) + return self._wait_for_exit(pid, 2) def _wait_for_exit(self, pid: int, timeout_s: int | float) -> bool: deadline = time.monotonic() + max(float(timeout_s), 0.0) diff --git a/nanobot/gateway/service.py b/nanobot/gateway/service.py index 2bc8eecf..5a39a89b 100644 --- a/nanobot/gateway/service.py +++ b/nanobot/gateway/service.py @@ -141,7 +141,7 @@ class GatewayServiceInstaller: "Label": label, "ProgramArguments": build_gateway_command(options.python_executable, options.start), "WorkingDirectory": _working_directory_text(options.start), - "RunAtLoad": bool(options.start_now), + "RunAtLoad": bool(options.enable), "KeepAlive": {"SuccessfulExit": False}, "StandardOutPath": str(stdout_path), "StandardErrorPath": str(stderr_path), @@ -149,7 +149,7 @@ class GatewayServiceInstaller: content = plistlib.dumps(payload, sort_keys=False).decode("utf-8") domain = _launchd_domain() commands: list[tuple[str, ...]] = [] - if options.enable or options.start_now: + if options.start_now: commands.append(("launchctl", "bootstrap", domain, str(path))) if options.enable: commands.append(("launchctl", "enable", f"{domain}/{label}")) @@ -162,7 +162,7 @@ class GatewayServiceInstaller: path.parent.mkdir(parents=True, exist_ok=True) stdout_path.parent.mkdir(parents=True, exist_ok=True) path.write_text(content, encoding="utf-8") - if options.enable or options.start_now: + if options.start_now: self._run_best_effort(("launchctl", "bootout", domain, str(path))) for command_args in commands: self._subprocess_run(list(command_args), check=True) diff --git a/tests/gateway/test_gateway_service.py b/tests/gateway/test_gateway_service.py index 6df5ec92..076ec422 100644 --- a/tests/gateway/test_gateway_service.py +++ b/tests/gateway/test_gateway_service.py @@ -103,6 +103,7 @@ def test_launchd_install_dry_run_renders_plist(tmp_path): "/Users/test/.nanobot/config.json", ] assert payload["KeepAlive"] == {"SuccessfulExit": False} + assert payload["RunAtLoad"] is True assert ("launchctl", "bootstrap", _expected_launchd_domain(), str(result.path)) in result.commands @@ -118,11 +119,34 @@ def test_launchd_no_enable_start_still_bootstraps(tmp_path): dry_run=True, ) + assert result.content is not None + payload = plistlib.loads(result.content.encode("utf-8")) + assert payload["RunAtLoad"] is False assert result.commands[0][:2] == ("launchctl", "bootstrap") assert not any(command[1] == "enable" for command in result.commands) assert any(command[1] == "kickstart" for command in result.commands) +def test_launchd_enable_without_start_sets_run_at_load_without_bootstrap(tmp_path): + installer = GatewayServiceInstaller(platform_name="Darwin", home=tmp_path) + + result = installer.install( + GatewayServiceOptions( + start=GatewayStartOptions(port=18790), + enable=True, + start_now=False, + ), + dry_run=True, + ) + + assert result.content is not None + payload = plistlib.loads(result.content.encode("utf-8")) + assert payload["RunAtLoad"] is True + assert not any(command[1] == "bootstrap" for command in result.commands) + assert any(command[1] == "enable" for command in result.commands) + assert not any(command[1] == "kickstart" for command in result.commands) + + def test_launchd_no_enable_start_reinstall_boots_out_existing_label(tmp_path): commands: list[list[str]] = [] installer = GatewayServiceInstaller( diff --git a/tests/gateway/test_runtime.py b/tests/gateway/test_runtime.py index c91b25d0..b3646b8b 100644 --- a/tests/gateway/test_runtime.py +++ b/tests/gateway/test_runtime.py @@ -139,10 +139,32 @@ def test_stop_terminates_recorded_process(tmp_path, monkeypatch): monkeypatch.setattr(runtime, "_is_pid_running", lambda _pid: True) monkeypatch.setattr(runtime, "_process_identity", lambda _pid: 12345) terminated: list[int] = [] - monkeypatch.setattr(runtime, "_terminate", lambda pid, timeout_s: terminated.append(pid)) + + def fake_terminate(pid, timeout_s): + terminated.append(pid) + return True + + monkeypatch.setattr(runtime, "_terminate", fake_terminate) result = runtime.stop() assert result.ok is True assert terminated == [12345] assert not runtime.paths.state_path.exists() + + +def test_stop_keeps_state_when_process_survives_timeout(tmp_path, monkeypatch): + runtime = GatewayRuntime(paths=_paths(tmp_path), platform_name="Linux") + runtime.paths.run_dir.mkdir(parents=True) + runtime.paths.state_path.write_text('{"pid": 12345, "identity": 12345}', encoding="utf-8") + monkeypatch.setattr(runtime, "_is_pid_running", lambda _pid: True) + monkeypatch.setattr(runtime, "_process_identity", lambda _pid: 12345) + monkeypatch.setattr(runtime, "_terminate", lambda _pid, timeout_s: False) + + result = runtime.stop(timeout_s=0) + + assert result.ok is False + assert result.message == "gateway_stop_timeout" + assert result.status.running is True + assert result.status.reason == "stop_timeout" + assert runtime.paths.state_path.exists()