From 7e976cd2eaefe2ae5563d67f4cea6c8ea0fae62c Mon Sep 17 00:00:00 2001 From: Sequoia Ploeg Date: Sun, 4 Oct 2026 14:26:47 -0600 Subject: [PATCH] Make daemon startup reliable: validated config, atomic lockfile, waiting up - Config (#54): PyroLabConfiguration rejects dangling cross-references (service daemon/nameservers, daemon nameservers, autolaunch entries), reporting every problem at once. `config update` and `info` print them readably; `rename` refuses to write a config it would invalidate. - Startup (#54, #56): pyrolabd.main() validates the config before starting anything, publishes the lockfile as soon as the daemon can answer, then autolaunches in a background thread with each entry isolated. A startup failure is written to STARTUP_ERROR_FILE. - Lockfile (#56): written atomically (temp file + os.replace), read tolerantly, and judged stale by whether its PID is running (new utils.pid_is_running, using ctypes on Windows) rather than by a failed connection. - `pyrolab up` waits until the daemon responds (--timeout, default 30s) and otherwise reports why it exited; it refuses to start a second daemon while one is running, even an unresponsive one. - ProcessManager methods share a reentrant lock, now that autolaunch, the checkup timer, and Pyro request threads run concurrently. - Importing pyrolab.api no longer fails on an invalid user config, one with no nameservers, or an unresolvable "public" host; this would otherwise have locked users out of every CLI command. Closes #54 Closes #56 Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 21 +++- pyrolab/__init__.py | 2 + pyrolab/api.py | 18 +++- pyrolab/cli.py | 194 +++++++++++++++++++++++++---------- pyrolab/configure.py | 64 +++++++++++- pyrolab/manager.py | 27 +++++ pyrolab/pyrolabd.py | 160 ++++++++++++++++++++++------- pyrolab/utils.py | 57 +++++++++++ tests/conftest.py | 1 + tests/test_cli.py | 220 +++++++++++++++++++++++++++++++++++++++- tests/test_configure.py | 70 +++++++++++++ tests/test_imports.py | 29 ++++++ tests/test_pyrolabd.py | 149 ++++++++++++++++++++++++++- tests/test_utils.py | 19 ++++ 14 files changed, 929 insertions(+), 102 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 20d38db..a488249 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,6 +81,11 @@ subclass, sets the Pyro instance mode, and stashes `parameters` on `_autoconnect exists because autoconnect parameters live as *class* attributes — two hardware units of the same model would otherwise clobber each other. +`PyroLabConfiguration` validates **cross-references** on load: every service's `daemon`/`nameservers`, every +daemon's `nameservers`, and every `autolaunch` entry must name something defined (a service with no `daemon:` +refers to one called `default`). `describe_config_error()` turns a load error into readable lines for the CLI. +Anything that writes a config (e.g. `pyrolab rename`) must re-validate before saving. + `GlobalConfiguration` is a singleton and **must only be touched from the main process**. Child processes read the frozen `RUNTIME_CONFIG` file instead, which is why config changes require `pyrolab reload` rather than taking effect live. @@ -88,11 +93,17 @@ taking effect live. ### Process model `pyrolab/pyrolabd.py` is the background daemon (`PyroLabDaemon`, `instance_mode="single"`), launched by -`pyrolab up`, which runs `pyrolabd.py` as a detached subprocess (`pythonw.exe` on Windows). It writes pid+URI to `LOCKFILE`, and the -CLI reaches it by reading that file. It owns a `ProcessManager` (`pyrolab/manager.py`) singleton that spawns +`pyrolab up`, which runs `pyrolabd.py` as a detached subprocess (`pythonw.exe` on Windows) and **waits until it +responds** (or reports why it exited, read from `STARTUP_ERROR_FILE`). `pyrolabd.main()` loads and validates the +config, registers itself, then publishes pid+URI to `LOCKFILE` — written atomically (`write_lockfile`: temp +file + `os.replace`) — and only then runs autolaunch in a background thread, each entry independently. The CLI +reaches the daemon through `read_lockfile()`; a lockfile whose PID isn't running (`utils.pid_is_running`, which +uses ctypes on Windows because `os.kill` would kill the process) is stale. It owns a `ProcessManager` (`pyrolab/manager.py`) singleton that spawns each nameserver and each daemon as its own `multiprocessing.Process` (`NameServerRunner`, `DaemonRunner`), so a hung instrument kills only its own process. Runners are controlled by sentinel `None` on a -`multiprocessing.Queue`; `ProcessManager.checkup()` restarts dead ones on a timer. +`multiprocessing.Queue`; `ProcessManager.checkup()` restarts dead ones on a timer. Pyro request threads, the checkup timer, and +autolaunch all use the `ProcessManager` concurrently, so methods touching its process tables are +`@_synchronized` on a reentrant class-level lock. Everything spawn-related lives in `manager.py` specifically because child processes re-import the target module — keeping process creation out of that import path prevents recursive spawning. @@ -121,7 +132,9 @@ Other modules bind these constants by value (`from pyrolab import LOCKFILE, ...` which is what the `data_dir` test fixture does. `pyrolab/api.py` is the intended public surface — it re-exports the Pyro5 names plus PyroLab's, and on import -applies the first configured nameserver's settings to `Pyro5.config` so `locate_ns()` just works. It is +applies the first configured nameserver's settings to `Pyro5.config` so `locate_ns()` just works. That step is +best-effort: **importing `pyrolab.api` must never fail** because of the local config (invalid, no nameservers, +unresolvable `public` host), or no CLI command — including the ones that fix the config — could run. It is excluded from import sorting (a ruff per-file ignore); import order there is load-bearing. ## Conventions diff --git a/pyrolab/__init__.py b/pyrolab/__init__.py index 9fbb8cb..8ad02d4 100644 --- a/pyrolab/__init__.py +++ b/pyrolab/__init__.py @@ -92,6 +92,8 @@ USER_CONFIG_FILE = PYROLAB_DATA_DIR / "user_configuration.yaml" RUNTIME_CONFIG = PYROLAB_DATA_DIR / "runtime_config.yaml" UPDATE_CHECK_FILE = PYROLAB_DATA_DIR / "update_check.json" +# Why the background daemon exited before it could serve requests, for `up`. +STARTUP_ERROR_FILE = PYROLAB_DATA_DIR / "pyrolabd_startup_error.txt" # Set up logging to file diff --git a/pyrolab/api.py b/pyrolab/api.py index fec58c0..9151611 100644 --- a/pyrolab/api.py +++ b/pyrolab/api.py @@ -48,7 +48,19 @@ # If a user config file exists, load the first listed nameserver by default, # so that locate_ns "just works." +# This is a convenience only: importing the API must never fail because the +# local configuration is invalid, has no nameservers, or names a "public" +# host that can't be resolved -- otherwise neither client scripts nor the +# CLI commands that would fix the configuration could even start. if USER_CONFIG_FILE.exists(): - cfg = PyroLabConfiguration.from_file(USER_CONFIG_FILE) - nscfg = next(iter(cfg.nameservers.values())) - nscfg.update_pyro_config() + try: + cfg = PyroLabConfiguration.from_file(USER_CONFIG_FILE) + nscfg = next(iter(cfg.nameservers.values()), None) + if nscfg is not None: + nscfg.update_pyro_config() + except Exception as e: + import logging + + logging.getLogger(__name__).warning( + "Not applying nameserver defaults from %s: %s", USER_CONFIG_FILE, e + ) diff --git a/pyrolab/cli.py b/pyrolab/cli.py index 0e17a4a..bef660c 100644 --- a/pyrolab/cli.py +++ b/pyrolab/cli.py @@ -18,6 +18,7 @@ import subprocess import sys import textwrap +import time from pathlib import Path from time import sleep, strptime from typing import Callable, Iterable, Optional @@ -29,6 +30,7 @@ except ImportError: import pkg_resources import typer +import yaml from Pyro5.errors import CommunicationError import pyrolab @@ -36,6 +38,7 @@ LOCKFILE, PYROLAB_LOGDIR, RUNTIME_CONFIG, + STARTUP_ERROR_FILE, UPDATE_CHECK_FILE, USER_CONFIG_FILE, updates, @@ -43,21 +46,39 @@ from pyrolab.api import Proxy from pyrolab.configure import ( PyroLabConfiguration, + describe_config_error, export_config, reset_config, update_config, ) -from pyrolab.pyrolabd import InstanceInfo, PyroLabDaemon +from pyrolab.pyrolabd import PyroLabDaemon, read_lockfile +from pyrolab.utils import pid_is_running + + +def _print_config_problems(source, exc: Exception) -> None: + typer.secho(f"Invalid configuration in {source}:", fg=typer.colors.RED) + for line in describe_config_error(exc): + typer.secho(f" - {line}", fg=typer.colors.RED) + + +def _load_config_or_exit(path: Path) -> PyroLabConfiguration: + try: + return PyroLabConfiguration.from_file(path) + except (ValueError, yaml.YAMLError) as e: + _print_config_problems(path, e) + raise typer.Exit(1) def get_daemon(abort=True, suppress_reload_message=False) -> PyroLabDaemon: - if LOCKFILE.exists(): - ii = InstanceInfo.parse_file(LOCKFILE) + info = read_lockfile() + if info is not None and pid_is_running(info.pid): try: - DAEMON = Proxy(ii.uri) + DAEMON = Proxy(info.uri) DAEMON._pyroBind() except CommunicationError: - raise ConnectionRefusedError("Could not connect to daemon.") + raise ConnectionRefusedError( + f"PyroLab daemon (pid {info.pid}) is running but not responding." + ) if ( not suppress_reload_message and RUNTIME_CONFIG.exists() @@ -142,60 +163,101 @@ def main( ) +def _spawn_daemon(port: Optional[int]) -> subprocess.Popen: + """Launch pyrolabd.py as a detached background process.""" + try: + rsrc = pkg_resources.files(pyrolab) / "pyrolabd.py" + except AttributeError: + rsrc = Path(pkg_resources.resource_filename("pyrolab", "pyrolabd.py")) + + args = [sys.executable, str(rsrc)] + if port: + args.append(str(port)) + + if platform.system() == "Windows": + DETACHED_PROCESS = 0x00000008 + # Replace python.exe with pythonw.exe on Windows, usually in the + # same directory (certainly true for conda installations). + args[0] = str(Path(sys.exec_prefix) / "pythonw.exe") + return subprocess.Popen( + args, + close_fds=True, + start_new_session=True, + creationflags=DETACHED_PROCESS, + ) + return subprocess.Popen(args, close_fds=True, start_new_session=True) + + +def _daemon_responds(uri: str) -> bool: + try: + with Proxy(uri) as proxy: + proxy._pyroTimeout = 2 + proxy._pyroBind() + return True + except CommunicationError: + return False + + @app.command() def up( port: int = typer.Option( None, "--port", "-p", help="Port to use for the PyroLab daemon." ), + timeout: float = typer.Option( + 30.0, "--timeout", help="Seconds to wait for the daemon to respond." + ), ): """ - Start the background PyroLab daemon. + Start the background PyroLab daemon and wait until it responds. - Only use the `--force` flag if you're sure the daemon is dead, or you may - orphan the process. + Nameservers and daemons listed under autolaunch keep starting in the + background after this returns; check on them with `pyrolab ps`. """ - try: - daemon = get_daemon(abort=False, suppress_reload_message=True) - force = False - except ConnectionRefusedError: - daemon = None - force = True - if daemon is None or force: - if force and LOCKFILE.exists(): - LOCKFILE.unlink() - - try: - rsrc = pkg_resources.files(pyrolab) / "pyrolabd.py" - except AttributeError: - rsrc = Path(pkg_resources.resource_filename("pyrolab", "pyrolabd.py")) - - if port: - args = [sys.executable, str(rsrc), str(port)] + existing = read_lockfile() + if existing is not None and pid_is_running(existing.pid): + if _daemon_responds(existing.uri): + typer.secho("PyroLab daemon is already running!", fg=typer.colors.RED) else: - args = [sys.executable, str(rsrc)] - - if platform.system() == "Windows": - DETACHED_PROCESS = 0x00000008 - # Replace python.exe with pythonw.exe on Windows, usually in the - # same directory (certainly true for conda installations). - executable = Path(sys.exec_prefix) / "pythonw.exe" - args[0] = str(executable) - subprocess.Popen( - args, - close_fds=True, - start_new_session=True, - creationflags=DETACHED_PROCESS, + typer.secho( + f"A PyroLab daemon (pid {existing.pid}) is running but not " + "responding. It may still be starting; otherwise stop that " + "process before running 'pyrolab up' again.", + fg=typer.colors.RED, ) - else: - subprocess.Popen( - args, close_fds=True, start_new_session=True - ) # stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, - - typer.secho("PyroLab daemon launched.", fg=typer.colors.GREEN) - else: - typer.secho("PyroLab daemon is already running!", fg=typer.colors.RED) raise typer.Abort() + # Any lockfile left now is stale: its process is gone. + LOCKFILE.unlink(missing_ok=True) + STARTUP_ERROR_FILE.unlink(missing_ok=True) + process = _spawn_daemon(port) + + deadline = time.monotonic() + timeout + while True: + info = read_lockfile() + if info is not None and _daemon_responds(info.uri): + typer.secho("PyroLab daemon is running.", fg=typer.colors.GREEN) + return + if process.poll() is not None: + reason = ( + STARTUP_ERROR_FILE.read_text().strip() + if STARTUP_ERROR_FILE.exists() + else f"it exited with code {process.returncode}; see the logs in " + f"{PYROLAB_LOGDIR}" + ) + typer.secho( + f"PyroLab daemon failed to start: {reason}", fg=typer.colors.RED + ) + raise typer.Exit(1) + if time.monotonic() > deadline: + typer.secho( + f"PyroLab daemon did not respond within {timeout:g} seconds. It may " + "still be starting; check with 'pyrolab ps', or see the logs in " + f"{PYROLAB_LOGDIR}.", + fg=typer.colors.RED, + ) + raise typer.Exit(1) + sleep(0.2) + @app.command() def down(): @@ -258,7 +320,16 @@ def ps(): @config_app.command("update") def config_update(filename: str): """Update the configuration file""" - update_config(filename) + try: + update_config(filename) + except FileNotFoundError as e: + typer.secho(str(e), fg=typer.colors.RED) + raise typer.Exit(1) + except (ValueError, yaml.YAMLError) as e: + _print_config_problems(filename, e) + typer.secho("The installed configuration was not changed.") + raise typer.Exit(1) + typer.secho("Configuration updated.", fg=typer.colors.GREEN) @config_app.command("reset") @@ -359,9 +430,9 @@ def info(): Show details about the current PyroLab configuration. """ if RUNTIME_CONFIG.exists(): - config = PyroLabConfiguration.from_file(RUNTIME_CONFIG) + config = _load_config_or_exit(RUNTIME_CONFIG) elif USER_CONFIG_FILE.exists(): - config = PyroLabConfiguration.from_file(USER_CONFIG_FILE) + config = _load_config_or_exit(USER_CONFIG_FILE) else: typer.secho("No configuration installed.", fg=typer.colors.RED) raise typer.Exit() @@ -474,6 +545,25 @@ def logs_export(filename: str): # pyrolab rename ############################################################################### + +def _save_renamed(config: PyroLabConfiguration, old_name: str) -> None: + # Renaming only moves the entry; anything that still refers to the old + # name would leave an invalid configuration (which the daemon refuses to + # start with), so check before writing. + try: + PyroLabConfiguration.parse_obj(config.dict()) + except ValueError as e: + typer.secho( + f"Can't rename '{old_name}': other entries still refer to it.", + fg=typer.colors.RED, + ) + for line in describe_config_error(e): + typer.secho(f" - {line}", fg=typer.colors.RED) + typer.secho("Update those references first. Nothing was changed.") + raise typer.Exit(1) + export_config(config, USER_CONFIG_FILE) + + rename_app = typer.Typer() app.add_typer(rename_app, name="rename", help="Rename a nameserver, daemon or service.") @@ -490,7 +580,7 @@ def rename_nameserver( config = PyroLabConfiguration.from_file(USER_CONFIG_FILE) if old_name in config.nameservers: config.nameservers[new_name] = config.nameservers.pop(old_name) - export_config(config, USER_CONFIG_FILE) + _save_renamed(config, old_name) else: typer.secho("Nameserver not found.", fg=typer.colors.RED) raise typer.Exit() @@ -511,7 +601,7 @@ def rename_daemon( config = PyroLabConfiguration.from_file(USER_CONFIG_FILE) if old_name in config.daemons: config.daemons[new_name] = config.daemons.pop(old_name) - export_config(config, USER_CONFIG_FILE) + _save_renamed(config, old_name) else: typer.secho("Nameserver not found.", fg=typer.colors.RED) raise typer.Exit() @@ -532,7 +622,7 @@ def rename_service( config = PyroLabConfiguration.from_file(USER_CONFIG_FILE) if old_name in config.services: config.services[new_name] = config.services.pop(old_name) - export_config(config, USER_CONFIG_FILE) + _save_renamed(config, old_name) else: typer.secho("Nameserver not found.", fg=typer.colors.RED) raise typer.Exit() diff --git a/pyrolab/configure.py b/pyrolab/configure.py index 605f751..38a5246 100644 --- a/pyrolab/configure.py +++ b/pyrolab/configure.py @@ -40,7 +40,7 @@ from typing import IO, Any, Dict, List, Optional, Type, Union import Pyro5 -from pydantic import BaseModel, BaseSettings, validator +from pydantic import BaseModel, BaseSettings, root_validator, validator from pydantic.fields import PrivateAttr from yaml import dump, load from yaml.constructor import ConstructorError @@ -642,6 +642,10 @@ class PyroLabConfiguration(BaseSettings, YAMLMixin): of their own name, which can only be given to them by the parent configuration object. + Every name a section refers to must be defined: each service's ``daemon`` + and ``nameservers``, each daemon's ``nameservers``, and every + ``autolaunch`` entry. A dangling reference is rejected when the file is + loaded, rather than failing later inside a running process. """ version: str = "1.0" @@ -650,6 +654,36 @@ class PyroLabConfiguration(BaseSettings, YAMLMixin): services: Dict[str, ServiceConfiguration] = {} autolaunch: AutolaunchSettings = AutolaunchSettings() + @root_validator(skip_on_failure=True) + def references_are_defined(cls, values): + nameservers = values["nameservers"] + daemons = values["daemons"] + problems = [] + + def check(kind, names, defined, where): + for name in names: + if name not in defined: + problems.append( + f"{where} refers to {kind} '{name}', which is not defined" + ) + + for sname, svc in values["services"].items(): + where = f"service '{sname}'" + if svc.daemon not in daemons: + hint = "" if "daemon" in svc.__fields_set__ else " (the default)" + problems.append( + f"{where} refers to daemon '{svc.daemon}'{hint}, which is not defined" + ) + check("nameserver", svc.nameservers, nameservers, where) + for dname, dcfg in daemons.items(): + check("nameserver", dcfg.nameservers, nameservers, f"daemon '{dname}'") + check("nameserver", values["autolaunch"].nameservers, nameservers, "autolaunch") + check("daemon", values["autolaunch"].daemons, daemons, "autolaunch") + + if problems: + raise ValueError("; ".join(problems)) + return values + def initialize_nameservers(self): for name, nscfg in self.nameservers.items(): nscfg.set_name(name) @@ -661,6 +695,34 @@ def get_daemon_settings(self, daemon: str) -> DaemonConfiguration: return self.daemons[daemon] +def describe_config_error(exc: Exception) -> List[str]: + """ + Turn a configuration loading error into one readable line per problem. + + Parameters + ---------- + exc : Exception + Typically a pydantic ``ValidationError`` from loading a configuration; + anything else is described by its message. + + Returns + ------- + List[str] + Human-readable problem descriptions, e.g. + ``"daemons.lab.port: value is not a valid integer"``. + """ + from pydantic import ValidationError + + if not isinstance(exc, ValidationError): + return [str(exc)] + lines = [] + for err in exc.errors(): + loc = ".".join(str(part) for part in err["loc"] if part != "__root__") + for msg in err["msg"].split("; "): + lines.append(f"{loc}: {msg}" if loc else msg) + return lines + + class GlobalConfiguration: """ A Singleton global configuration object that can read and write configuration files. diff --git a/pyrolab/manager.py b/pyrolab/manager.py index 844e581..a65b6a7 100644 --- a/pyrolab/manager.py +++ b/pyrolab/manager.py @@ -24,6 +24,7 @@ from __future__ import annotations +import functools import logging import multiprocessing import threading @@ -354,6 +355,17 @@ def __init__( self.shared_uris = shared_uris +def _synchronized(method): + """Run a ProcessManager method while holding the manager's lock.""" + + @functools.wraps(method) + def wrapper(self, *args, **kwargs): + with self._lock: + return method(self, *args, **kwargs) + + return wrapper + + class ProcessManager: """ A manager class for running a set of PyroLab processes. @@ -363,6 +375,11 @@ class ProcessManager: """ _instance = None + # Pyro request threads, the checkup timer, and the background autolaunch + # all use the singleton concurrently; methods that read or change the + # process tables hold this lock. Reentrant, since e.g. reload() calls + # shutdown_daemon(). + _lock = threading.RLock() nameservers: Dict[str, NameServerProcessGroup] daemons: Dict[str, DaemonProcessGroup] GLOBAL_CONFIG: GlobalConfiguration @@ -428,6 +445,7 @@ def stop_checkup_timer(self) -> None: if hasattr(self, "_timer"): self._timer.cancel() + @_synchronized def launch_nameserver(self, nameserver: str) -> bool: """ Launch a nameserver. @@ -444,6 +462,7 @@ def launch_nameserver(self, nameserver: str) -> bool: runner.start() return True + @_synchronized def get_nameserver_process_info(self, nameserver: str) -> Dict[str, str]: """ Gets info on a nameserver process. @@ -482,6 +501,7 @@ def get_nameserver_process_info(self, nameserver: str) -> Dict[str, str]: "uri": "", } + @_synchronized def launch_daemon(self, daemon: str) -> bool: """ Launch a daemon and all its associated services. @@ -505,6 +525,7 @@ def launch_daemon(self, daemon: str) -> bool: runner.start() return True + @_synchronized def get_daemon_process_info(self, daemon: str) -> Dict[str, str]: """ Return the process group for a daemon. @@ -535,6 +556,7 @@ def get_daemon_process_info(self, daemon: str) -> Dict[str, str]: "uri": "", } + @_synchronized def get_service_process_info(self, service: str) -> Dict[str, str]: """ Return the process info for a service. @@ -557,6 +579,7 @@ def get_service_process_info(self, service: str) -> Dict[str, str]: "uri": "", } + @_synchronized def checkup(self, continuous: bool = True) -> None: """ Checkup the processes. @@ -587,6 +610,7 @@ def checkup(self, continuous: bool = True) -> None: if continuous: self.start_checkup_timer() + @_synchronized def shutdown_nameserver(self, nameserver: str) -> bool: """ Stop a running nameserver. @@ -606,6 +630,7 @@ def shutdown_nameserver(self, nameserver: str) -> bool: time.sleep(2 * polling) return True + @_synchronized def shutdown_daemon(self, daemon: str) -> bool: """ Stop a running daemon and its services. @@ -625,6 +650,7 @@ def shutdown_daemon(self, daemon: str) -> bool: time.sleep(2 * polling) return True + @_synchronized def reload(self) -> bool: """ Reload all entities. @@ -651,6 +677,7 @@ def reload(self) -> bool: return True + @_synchronized def shutdown_all(self) -> None: """ Shutdown all entities. diff --git a/pyrolab/pyrolabd.py b/pyrolab/pyrolabd.py index 5b2bb02..06d5a8d 100644 --- a/pyrolab/pyrolabd.py +++ b/pyrolab/pyrolabd.py @@ -12,15 +12,18 @@ import logging import os import shutil -from typing import NamedTuple +import sys +import threading +from typing import NamedTuple, Optional import Pyro5.api as api from pydantic import BaseModel from tabulate import tabulate -from pyrolab import LOCKFILE, RUNTIME_CONFIG, USER_CONFIG_FILE -from pyrolab.configure import GlobalConfiguration +from pyrolab import LOCKFILE, RUNTIME_CONFIG, STARTUP_ERROR_FILE, USER_CONFIG_FILE +from pyrolab.configure import GlobalConfiguration, describe_config_error from pyrolab.manager import ProcessManager +from pyrolab.utils import pid_is_running log = logging.getLogger("pyrolab.pyrolabd") @@ -34,6 +37,33 @@ class InstanceInfo(BaseModel): uri: str +def write_lockfile(info: InstanceInfo) -> None: + """ + Atomically write the lockfile. + + The contents go to a temporary file in the same directory, which then + replaces the lockfile in one step, so readers see either no lockfile or a + complete one, never a partial write. + """ + tmp = LOCKFILE.with_name(f"{LOCKFILE.name}.{os.getpid()}.tmp") + tmp.write_text(info.json()) + os.replace(tmp, LOCKFILE) + + +def read_lockfile() -> Optional[InstanceInfo]: + """ + Return the running daemon's details from the lockfile. + + Returns None if there is no lockfile or it cannot be parsed. The PID may + belong to a process that has since exited; check with + :py:func:`pyrolab.utils.pid_is_running`. + """ + try: + return InstanceInfo.parse_file(LOCKFILE) + except (OSError, ValueError): # missing, unreadable, or not valid JSON + return None + + class NameServerInfo(NamedTuple): """ Named tuple for storing information about a running nameserver. @@ -98,22 +128,36 @@ class PyroLabDaemon: def __init__(self): log.info("Starting PyroLab daemon.") - self.manager = ProcessManager.instance() - + # Load (and validate) the configuration before starting the process + # manager, so an invalid configuration fails fast and leaves nothing + # running. + self.gconfig = GlobalConfiguration.instance() if USER_CONFIG_FILE.exists(): - self.gconfig = GlobalConfiguration.instance() self.gconfig.load_config(USER_CONFIG_FILE) self.gconfig.save_config(RUNTIME_CONFIG) - else: - self.gconfig = GlobalConfiguration.instance() + self.manager = ProcessManager.instance() + + def autolaunch(self) -> None: + """ + Starts every nameserver and daemon listed under ``autolaunch``. + Each entry is started independently: one that fails is logged and the + rest still start. Run in a background thread at startup, so the daemon + answers requests while the entities come up. + """ log.info("Autolaunching PyroLab entities.") autodetails = self.gconfig.config.autolaunch for ns in autodetails.nameservers: - self.start_nameserver(ns) + try: + self.start_nameserver(ns) + except Exception: + log.exception("Autolaunch of nameserver '%s' failed", ns) for daemon in autodetails.daemons: - self.start_daemon(daemon) - log.info("PyroLab background daemon started.") + try: + self.start_daemon(daemon) + except Exception: + log.exception("Autolaunch of daemon '%s' failed", daemon) + log.info("Autolaunch complete.") def reload(self) -> bool: """ @@ -272,30 +316,78 @@ def shutdown(self) -> None: log.info("Daemon shutdown complete.") -if __name__ == "__main__": - if LOCKFILE.exists(): - raise RuntimeError(f"Lockfile already exists. Is another instance running?") - else: - try: - LOCKFILE.touch(exist_ok=False) +def _report_startup_error(message: str) -> None: + """Log why the daemon could not start, and leave the reason for `up`.""" + log.error("PyroLab daemon failed to start: %s", message) + try: + STARTUP_ERROR_FILE.write_text(message) + except OSError: + log.exception("Could not write %s", STARTUP_ERROR_FILE) + + +def main(port: int = 0) -> int: + """ + Runs the background daemon until it is shut down. - import sys + The lockfile is published as soon as the daemon can answer requests; + autolaunched entities start afterwards, in the background. - if len(sys.argv) > 1: - port = int(sys.argv[1]) - else: - port = 0 + Parameters + ---------- + port : int, optional + The port to serve on (default 0: any free port). - daemon = api.Daemon(port=port) + Returns + ------- + int + The process exit code. + """ + existing = read_lockfile() + if existing is not None and pid_is_running(existing.pid): + _report_startup_error( + f"another PyroLab daemon is already running (pid {existing.pid})" + ) + return 1 + + daemon = None + published = False + try: + daemon = api.Daemon(port=port) + try: pyrolabd = PyroLabDaemon() - uri = daemon.register(pyrolabd, "pyrolabd") - ii = InstanceInfo(pid=os.getpid(), uri=str(uri)) - with LOCKFILE.open("w") as f: - f.write(ii.json()) - daemon.requestLoop() - finally: - LOCKFILE.unlink() - try: - RUNTIME_CONFIG.unlink() - except FileNotFoundError: - print("Runtime configuration not found (and therefore not removed).") + except ValueError as e: # includes pydantic's ValidationError + problems = "\n".join(f" - {line}" for line in describe_config_error(e)) + _report_startup_error( + f"invalid configuration in {USER_CONFIG_FILE}:\n{problems}" + ) + return 1 + uri = daemon.register(pyrolabd, "pyrolabd") + write_lockfile(InstanceInfo(pid=os.getpid(), uri=str(uri))) + published = True + log.info("PyroLab daemon listening at %s", uri) + + threading.Thread( + target=pyrolabd.autolaunch, name="autolaunch", daemon=True + ).start() + daemon.requestLoop() + return 0 + except Exception as e: + log.exception("PyroLab daemon stopped unexpectedly") + if not published: + _report_startup_error(f"{type(e).__name__}: {e}") + return 1 + finally: + # A normal shutdown already stopped everything; this covers failures. + manager = ProcessManager._instance + if manager is not None: + manager.shutdown_all() + if daemon is not None: + daemon.close() + current = read_lockfile() + if current is not None and current.pid == os.getpid(): + LOCKFILE.unlink(missing_ok=True) + RUNTIME_CONFIG.unlink(missing_ok=True) + + +if __name__ == "__main__": + sys.exit(main(int(sys.argv[1]) if len(sys.argv) > 1 else 0)) diff --git a/pyrolab/utils.py b/pyrolab/utils.py index 9bf9a71..dbc3de6 100644 --- a/pyrolab/utils.py +++ b/pyrolab/utils.py @@ -9,8 +9,10 @@ Convenience functions for working with the pyrolab package. """ +import os import secrets import socket +import sys try: import importlib.resources as pkg_resources @@ -62,3 +64,58 @@ def generate_random_name(count: int = 3) -> str: wordlist = f.read().splitlines() return "-".join([secrets.choice(wordlist) for _ in range(count)]) + + +def pid_is_running(pid: int) -> bool: + """ + Return True if a process with the given PID currently exists. + + PIDs are reused, so True only means *some* process has this PID; callers + that need certainty should also check that it answers as expected. + + Parameters + ---------- + pid : int + The process ID to check. + + Returns + ------- + bool + Whether the process exists. + """ + if pid <= 0: + return False + if sys.platform == "win32": + # os.kill() on Windows terminates the process instead of probing it. + import ctypes + + PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 + STILL_ACTIVE = 259 + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + # Declare types so 64-bit HANDLEs aren't truncated to a C int. + kernel32.OpenProcess.argtypes = [ctypes.c_ulong, ctypes.c_int, ctypes.c_ulong] + kernel32.OpenProcess.restype = ctypes.c_void_p + kernel32.GetExitCodeProcess.argtypes = [ + ctypes.c_void_p, + ctypes.POINTER(ctypes.c_ulong), + ] + kernel32.CloseHandle.argtypes = [ctypes.c_void_p] + handle = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, pid) + if not handle: + # Access denied means it exists but belongs to someone else. + ERROR_ACCESS_DENIED = 5 + return ctypes.get_last_error() == ERROR_ACCESS_DENIED + try: + exit_code = ctypes.c_ulong() + if not kernel32.GetExitCodeProcess(handle, ctypes.byref(exit_code)): + return True + return exit_code.value == STILL_ACTIVE + finally: + kernel32.CloseHandle(handle) + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + return True # exists, owned by another user + return True diff --git a/tests/conftest.py b/tests/conftest.py index e58641e..99a9800 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -85,6 +85,7 @@ def data_dir(tmp_path, monkeypatch): RUNTIME_CONFIG=tmp_path / "runtime_config.yaml", LOCKFILE=tmp_path / "pyrolabd.lock", UPDATE_CHECK_FILE=tmp_path / "update_check.json", + STARTUP_ERROR_FILE=tmp_path / "pyrolabd_startup_error.txt", PYROLAB_LOGDIR=tmp_path / "logs", NAMESERVER_STORAGE=tmp_path / "nameserver", ) diff --git a/tests/test_cli.py b/tests/test_cli.py index 3d673f7..edb3fdd 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,11 +1,13 @@ import os +import subprocess +import sys import pytest from Pyro5.errors import CommunicationError from typer.testing import CliRunner from pyrolab import __version__, cli -from pyrolab.configure import PyroLabConfiguration +from pyrolab.configure import DaemonConfiguration, PyroLabConfiguration from pyrolab.pyrolabd import InstanceInfo runner = CliRunner() @@ -47,7 +49,7 @@ def running_daemon(data_dir, monkeypatch): FakeDaemonProxy.returns = {} monkeypatch.setattr(cli, "Proxy", FakeDaemonProxy) data_dir.LOCKFILE.write_text( - InstanceInfo(pid=1, uri="PYRO:pyrolabd@localhost:1").json() + InstanceInfo(pid=os.getpid(), uri="PYRO:pyrolabd@localhost:1").json() ) return FakeDaemonProxy @@ -79,6 +81,36 @@ def test_get_daemon_without_abort_returns_none(data_dir): assert cli.get_daemon(abort=False) is None +@pytest.fixture +def dead_pid(): + """The PID of a process that has already exited.""" + proc = subprocess.run( + [sys.executable, "-c", "import os; print(os.getpid())"], + capture_output=True, + text=True, + check=True, + ) + return int(proc.stdout) + + +def test_stale_lockfile_means_not_running(data_dir, dead_pid): + data_dir.LOCKFILE.write_text( + InstanceInfo(pid=dead_pid, uri="PYRO:pyrolabd@localhost:1").json() + ) + result = runner.invoke(cli.app, ["ps"]) + assert result.exit_code == 1 + assert "not running" in result.output + + +@pytest.mark.parametrize("contents", ["", "{", '{"pid": "x"}']) +def test_unreadable_lockfile_means_not_running(data_dir, contents): + # Regression: a partially written lockfile raised a ValidationError (#56). + data_dir.LOCKFILE.write_text(contents) + result = runner.invoke(cli.app, ["ps"]) + assert result.exit_code == 1 + assert "not running" in result.output + + def test_get_daemon_unreachable(running_daemon): running_daemon.fail_bind = True with pytest.raises(ConnectionRefusedError): @@ -213,6 +245,33 @@ def test_commands_work_after_config_reset(running_daemon, data_dir): ############################################################################### +def test_config_update_rejects_dangling_references(user_config, tmp_path): + # Regression: a bad autolaunch entry was accepted, then killed the + # background daemon at startup (#54). + before = user_config.read_text() + bad = tmp_path / "bad.yaml" + bad.write_text("daemons:\n d: {}\nautolaunch:\n daemons: [d, typo]\n") + + result = runner.invoke(cli.app, ["config", "update", str(bad)]) + assert result.exit_code == 1 + assert "autolaunch refers to daemon 'typo', which is not defined" in result.output + assert "not changed" in result.output + assert user_config.read_text() == before + + +def test_config_update_missing_file(data_dir, tmp_path): + result = runner.invoke(cli.app, ["config", "update", str(tmp_path / "x.yaml")]) + assert result.exit_code == 1 + assert "does not" in result.output + + +def test_info_with_invalid_config(data_dir): + data_dir.RUNTIME_CONFIG.write_text("services:\n s: {module: m, classname: C}\n") + result = runner.invoke(cli.app, ["info"]) + assert result.exit_code == 1 + assert "daemon 'default' (the default), which is not defined" in result.output + + def test_config_update(data_dir, sample_config_file): result = runner.invoke(cli.app, ["config", "update", str(sample_config_file)]) assert result.exit_code == 0, result.output @@ -278,15 +337,25 @@ def test_info_prefers_runtime_config(user_config, data_dir): ############################################################################### +@pytest.fixture +def config_with_spare_daemon(user_config): + """The sample config plus a daemon that nothing refers to.""" + config = PyroLabConfiguration.from_file(user_config) + config.daemons["spare"] = DaemonConfiguration() + user_config.write_text(config.yaml()) + return user_config + + @pytest.mark.parametrize( "kind, section, old", [ - ("nameserver", "nameservers", "local"), - ("daemon", "daemons", "plain"), + ("nameserver", "nameservers", "persistent"), + ("daemon", "daemons", "spare"), ("service", "services", "sample.echo"), ], ) -def test_rename(user_config, kind, section, old): +def test_rename(config_with_spare_daemon, kind, section, old): + user_config = config_with_spare_daemon before = getattr(PyroLabConfiguration.from_file(user_config), section)[old] result = runner.invoke(cli.app, ["rename", kind, old, "renamed"]) @@ -297,6 +366,24 @@ def test_rename(user_config, kind, section, old): assert entities["renamed"] == before +@pytest.mark.parametrize( + "kind, old, referrer", + [ + ("nameserver", "local", "daemons.lockable"), + ("daemon", "plain", "service 'sample.echo'"), + ], +) +def test_rename_refuses_to_orphan_references(user_config, kind, old, referrer): + # Renaming moves only the entry itself; with references left pointing at + # the old name the config would be invalid, so nothing is written. + before = user_config.read_text() + result = runner.invoke(cli.app, ["rename", kind, old, "renamed"]) + assert result.exit_code == 1 + assert "still refer to it" in result.output + assert f"'{old}'" in result.output + assert user_config.read_text() == before + + def test_rename_unknown_nameserver(user_config): before = user_config.read_text() result = runner.invoke(cli.app, ["rename", "nameserver", "nope", "new"]) @@ -350,3 +437,126 @@ def test_logs_clean(data_dir): result = runner.invoke(cli.app, ["logs", "clean"]) assert result.exit_code == 0 assert list(data_dir.PYROLAB_LOGDIR.iterdir()) == [] + + +############################################################################### +# pyrolab up +############################################################################### + + +class FakeDaemonProcess: + """ + Stands in for the spawned pyrolabd. After ``ready_after`` polls it + publishes a lockfile (or, if ``exit_code`` is set, exits instead). + """ + + def __init__(self, data_dir, ready_after=2, exit_code=None, error=None): + self.data_dir = data_dir + self.polls_left = ready_after + self.exit_code = exit_code + self.error = error + self.returncode = None + + def poll(self): + self.polls_left -= 1 + if self.polls_left > 0: + return None + if self.exit_code is None: + self.data_dir.LOCKFILE.write_text( + InstanceInfo(pid=os.getpid(), uri="PYRO:pyrolabd@localhost:1").json() + ) + return None + if self.error: + self.data_dir.STARTUP_ERROR_FILE.write_text(self.error) + self.returncode = self.exit_code + return self.exit_code + + +@pytest.fixture +def spawn(data_dir, monkeypatch): + """ + Replace process spawning; the daemon "responds" once its lockfile exists. + Set ``spawn.process`` before invoking `up`; ``spawn.count`` records spawns. + """ + + class Spawn: + process = FakeDaemonProcess(data_dir) + count = 0 + + def fake_spawn(port): + Spawn.count += 1 + return Spawn.process + + monkeypatch.setattr(cli, "_spawn_daemon", fake_spawn) + monkeypatch.setattr(cli, "_daemon_responds", lambda uri: data_dir.LOCKFILE.exists()) + monkeypatch.setattr(cli, "sleep", lambda seconds: None) + return Spawn + + +def test_up_waits_until_daemon_responds(spawn): + result = runner.invoke(cli.app, ["up"]) + assert result.exit_code == 0, result.output + assert "PyroLab daemon is running." in result.output + assert spawn.count == 1 + + +def test_up_replaces_stale_lockfile(spawn, data_dir, dead_pid): + data_dir.LOCKFILE.write_text( + InstanceInfo(pid=dead_pid, uri="PYRO:pyrolabd@localhost:1").json() + ) + result = runner.invoke(cli.app, ["up"]) + assert result.exit_code == 0, result.output + assert spawn.count == 1 + + +def test_up_when_already_running(spawn, data_dir): + data_dir.LOCKFILE.write_text( + InstanceInfo(pid=os.getpid(), uri="PYRO:pyrolabd@localhost:1").json() + ) + result = runner.invoke(cli.app, ["up"]) + assert result.exit_code == 1 + assert "already running" in result.output + assert spawn.count == 0 + + +def test_up_when_running_but_not_responding(spawn, data_dir, monkeypatch): + data_dir.LOCKFILE.write_text( + InstanceInfo(pid=os.getpid(), uri="PYRO:pyrolabd@localhost:1").json() + ) + monkeypatch.setattr(cli, "_daemon_responds", lambda uri: False) + result = runner.invoke(cli.app, ["up"]) + assert result.exit_code == 1 + assert f"(pid {os.getpid()}) is running but not responding" in result.output + assert spawn.count == 0 # never starts a second daemon + assert data_dir.LOCKFILE.exists() + + +def test_up_reports_startup_error(spawn, data_dir): + # Regression: `up` reported success while the daemon died at startup (#54). + spawn.process = FakeDaemonProcess( + data_dir, exit_code=1, error="invalid configuration in user.yaml" + ) + result = runner.invoke(cli.app, ["up"]) + assert result.exit_code == 1 + assert "failed to start: invalid configuration in user.yaml" in result.output + + +def test_up_reports_unexplained_exit(spawn, data_dir): + spawn.process = FakeDaemonProcess(data_dir, exit_code=3) + result = runner.invoke(cli.app, ["up"]) + assert result.exit_code == 1 + assert "exited with code 3" in result.output + + +def test_up_ignores_previous_startup_error(spawn, data_dir): + data_dir.STARTUP_ERROR_FILE.write_text("left over from last time") + spawn.process = FakeDaemonProcess(data_dir, exit_code=3) + result = runner.invoke(cli.app, ["up"]) + assert "left over" not in result.output + + +def test_up_times_out(spawn, data_dir): + spawn.process = FakeDaemonProcess(data_dir, ready_after=10**9) + result = runner.invoke(cli.app, ["up", "--timeout", "0"]) + assert result.exit_code == 1 + assert "did not respond within 0 seconds" in result.output diff --git a/tests/test_configure.py b/tests/test_configure.py index f8f2484..cb8d521 100644 --- a/tests/test_configure.py +++ b/tests/test_configure.py @@ -12,6 +12,7 @@ PyroLabConfiguration, ServiceConfiguration, UniqueOrAutoKeyLoader, + describe_config_error, export_config, reset_config, uniquify_class, @@ -116,6 +117,75 @@ def test_initialize_nameservers_assigns_names(sample_config_file): assert cfg.nameservers["persistent"].name == "persistent" +############################################################################### +# Cross-references between sections (#54) +############################################################################### + + +@pytest.mark.parametrize( + "yaml_text, problem", + [ + ( + "services:\n s: {module: m, classname: C, daemon: nope}\n", + "service 's' refers to daemon 'nope', which is not defined", + ), + ( + "services:\n s: {module: m, classname: C}\n", + "service 's' refers to daemon 'default' (the default), which is not defined", + ), + ( + "daemons: {d: {}}\n" + "services:\n s: {module: m, classname: C, daemon: d, nameservers: [x]}\n", + "service 's' refers to nameserver 'x', which is not defined", + ), + ( + "daemons:\n d: {nameservers: [x]}\n", + "daemon 'd' refers to nameserver 'x', which is not defined", + ), + ( + "autolaunch: {nameservers: [x]}\n", + "autolaunch refers to nameserver 'x', which is not defined", + ), + ( + "autolaunch: {daemons: [x]}\n", + "autolaunch refers to daemon 'x', which is not defined", + ), + ], +) +def test_dangling_references_are_rejected(yaml_text, problem): + with pytest.raises(ValidationError) as excinfo: + PyroLabConfiguration.from_yaml(yaml_text) + assert describe_config_error(excinfo.value) == [problem] + + +def test_every_dangling_reference_is_reported(): + yaml_text = ( + "daemons:\n d: {nameservers: [a]}\n" + "autolaunch: {nameservers: [b], daemons: [c]}\n" + ) + with pytest.raises(ValidationError) as excinfo: + PyroLabConfiguration.from_yaml(yaml_text) + assert len(describe_config_error(excinfo.value)) == 3 + + +def test_describe_field_errors(): + with pytest.raises(ValidationError) as excinfo: + PyroLabConfiguration.from_yaml("daemons:\n d: {port: lots}\n") + (line,) = describe_config_error(excinfo.value) + assert line.startswith("daemons.d.port: ") + + +def test_describe_other_errors(): + assert describe_config_error(FileNotFoundError("gone")) == ["gone"] + + +def test_global_load_rejects_dangling_references(global_config, tmp_path): + bad = tmp_path / "bad.yaml" + bad.write_text("autolaunch: {daemons: [x]}\n") + with pytest.raises(ValidationError): + global_config.load_config(bad) + + ############################################################################### # NameServerConfiguration ############################################################################### diff --git a/tests/test_imports.py b/tests/test_imports.py index 62ef468..4484a8b 100644 --- a/tests/test_imports.py +++ b/tests/test_imports.py @@ -78,3 +78,32 @@ def test_source_compiles_without_warnings(path): with warnings.catch_warnings(): warnings.simplefilter("error") compile(path.read_text(encoding="utf-8"), str(path), "exec") + + +@pytest.mark.parametrize( + "contents", + [ + "autolaunch: {daemons: [ghost]}\n", # fails validation + "nameservers: {\n", # not valid YAML + "daemons: {d: {}}\n", # valid, but no nameservers + ], +) +def test_api_import_survives_any_user_config(data_dir, contents): + # Regression: importing pyrolab.api loaded the user config strictly, so an + # invalid one (or one with no nameservers) broke every client script and + # every CLI command, including the ones that would fix it. + import pyrolab.api + + data_dir.USER_CONFIG_FILE.write_text(contents) + importlib.reload(pyrolab.api) + + +def test_api_import_applies_first_nameserver(data_dir, pyro_config): + import pyrolab.api + + data_dir.USER_CONFIG_FILE.write_text( + "nameservers:\n first: {host: 127.0.0.1, ns_port: 9555}\n" + " second: {ns_port: 9666}\n" + ) + importlib.reload(pyrolab.api) + assert pyro_config.NS_PORT == 9555 diff --git a/tests/test_pyrolabd.py b/tests/test_pyrolabd.py index e1bc436..5a4d638 100644 --- a/tests/test_pyrolabd.py +++ b/tests/test_pyrolabd.py @@ -1,8 +1,17 @@ +import os +import threading + import pytest +from Pyro5.api import Proxy from pyrolab import pyrolabd from pyrolab.configure import PyroLabConfiguration -from pyrolab.pyrolabd import InstanceInfo, PyroLabDaemon +from pyrolab.pyrolabd import ( + InstanceInfo, + PyroLabDaemon, + read_lockfile, + write_lockfile, +) class FakeManager: @@ -10,12 +19,20 @@ class FakeManager: def __init__(self): self.calls = [] + self.broken = set() # names whose launch raises def launch_nameserver(self, name): self.calls.append(("launch_nameserver", name)) + if name in self.broken: + raise KeyError(name) def launch_daemon(self, name): self.calls.append(("launch_daemon", name)) + if name in self.broken: + raise KeyError(name) + + def shutdown_all(self): + self.calls.append(("shutdown_all",)) def shutdown_nameserver(self, name): self.calls.append(("shutdown_nameserver", name)) @@ -42,7 +59,13 @@ def get_service_process_info(self, name): @pytest.fixture def fake_manager(monkeypatch): fake = FakeManager() - monkeypatch.setattr(pyrolabd.ProcessManager, "instance", lambda: fake) + fake.instance_calls = 0 + + def instance(): + fake.instance_calls += 1 + return fake + + monkeypatch.setattr(pyrolabd.ProcessManager, "instance", instance) return fake @@ -59,13 +82,38 @@ def test_startup_writes_runtime_config(pld, data_dir): ) == PyroLabConfiguration.from_file(data_dir.USER_CONFIG_FILE) -def test_startup_autolaunches_nameservers_then_daemons(pld, fake_manager): +def test_startup_does_not_autolaunch(pld, fake_manager): + # Autolaunch runs separately, in the background, once the daemon can + # already answer requests. + assert fake_manager.calls == [] + + +def test_autolaunch_starts_nameservers_then_daemons(pld, fake_manager): + pld.autolaunch() assert fake_manager.calls == [ ("launch_nameserver", "local"), ("launch_daemon", "lockable"), ] +def test_autolaunch_continues_past_a_failing_entry(pld, fake_manager): + # Regression: one failing entry stopped the rest, and killed the daemon + # when it ran during startup (#54). + fake_manager.broken = {"local"} + pld.autolaunch() + assert ("launch_daemon", "lockable") in fake_manager.calls + + +def test_invalid_user_config_fails_before_starting_anything( + data_dir, global_config, fake_manager +): + data_dir.USER_CONFIG_FILE.write_text("autolaunch:\n daemons: [ghost]\n") + with pytest.raises(ValueError, match="ghost"): + PyroLabDaemon() + assert fake_manager.instance_calls == 0 # process manager never started + assert not data_dir.RUNTIME_CONFIG.exists() + + def test_startup_without_user_config(data_dir, global_config, fake_manager): PyroLabDaemon() assert fake_manager.calls == [] @@ -132,3 +180,98 @@ def test_ps_returns_plain_string(pld): def test_instance_info_round_trip(): ii = InstanceInfo(pid=1234, uri="PYRO:pyrolabd@localhost:5555") assert InstanceInfo.parse_raw(ii.json()) == ii + + +############################################################################### +# Lockfile +############################################################################### + + +def test_lockfile_round_trip(data_dir): + info = InstanceInfo(pid=os.getpid(), uri="PYRO:pyrolabd@localhost:5555") + write_lockfile(info) + assert read_lockfile() == info + # The temporary file was moved into place, not left behind. + assert list(data_dir.root.glob("*.tmp")) == [] + + +def test_write_lockfile_replaces_existing(data_dir): + write_lockfile(InstanceInfo(pid=1, uri="PYRO:old@localhost:1")) + write_lockfile(InstanceInfo(pid=2, uri="PYRO:new@localhost:2")) + assert read_lockfile().uri == "PYRO:new@localhost:2" + + +@pytest.mark.parametrize("contents", [None, "", "{", '{"pid": 1}']) +def test_read_lockfile_missing_or_invalid(data_dir, contents): + if contents is not None: + data_dir.LOCKFILE.write_text(contents) + assert read_lockfile() is None + + +############################################################################### +# main(): the background daemon process +############################################################################### + + +@pytest.fixture +def run_main(data_dir, global_config, fake_manager): + """Run pyrolabd.main() in a thread; returns (thread, result dict).""" + started = [] + + def start(): + result = {} + thread = threading.Thread( + target=lambda: result.update(code=pyrolabd.main(port=0)), daemon=True + ) + thread.start() + started.append(thread) + return thread, result + + yield start + for thread in started: + thread.join(timeout=10) + + +def test_main_publishes_lockfile_then_autolaunches_and_shuts_down( + run_main, data_dir, fake_manager, sample_config_file, wait_for +): + data_dir.USER_CONFIG_FILE.write_text(sample_config_file.read_text()) + thread, result = run_main() + + info = wait_for(read_lockfile) + assert info is not None and info.pid == os.getpid() + assert wait_for(lambda: ("launch_daemon", "lockable") in fake_manager.calls) + + with Proxy(info.uri) as proxy: + assert "at" in proxy.whoami() + proxy.shutdown() + + thread.join(timeout=10) + assert result["code"] == 0 + assert ("shutdown_all",) in fake_manager.calls + assert not data_dir.LOCKFILE.exists() + assert not data_dir.RUNTIME_CONFIG.exists() + + +def test_main_reports_invalid_config(run_main, data_dir, fake_manager): + data_dir.USER_CONFIG_FILE.write_text("autolaunch:\n nameservers: [ghost]\n") + thread, result = run_main() + thread.join(timeout=10) + + assert result["code"] == 1 + message = data_dir.STARTUP_ERROR_FILE.read_text() + assert "invalid configuration" in message + assert "autolaunch refers to nameserver 'ghost'" in message + assert not data_dir.LOCKFILE.exists() + assert fake_manager.instance_calls == 0 + + +def test_main_refuses_to_start_twice(run_main, data_dir): + other = InstanceInfo(pid=os.getpid(), uri="PYRO:pyrolabd@localhost:1") + write_lockfile(other) + thread, result = run_main() + thread.join(timeout=10) + + assert result["code"] == 1 + assert "already running" in data_dir.STARTUP_ERROR_FILE.read_text() + assert read_lockfile() == other # the running daemon's lockfile is untouched diff --git a/tests/test_utils.py b/tests/test_utils.py index d569d95..ec9aead 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,3 +1,7 @@ +import os +import subprocess +import sys + import pyrolab from pyrolab import utils @@ -44,3 +48,18 @@ def close(self): ip = utils.get_ip() assert ip == "10.1.2.3" assert calls["closed"] + + +def test_pid_is_running_for_this_process(): + assert utils.pid_is_running(os.getpid()) + + +def test_pid_is_running_for_an_exited_process(): + proc = subprocess.Popen([sys.executable, "-c", "pass"]) + proc.wait() + assert not utils.pid_is_running(proc.pid) + + +def test_pid_is_running_rejects_invalid_pids(): + assert not utils.pid_is_running(0) + assert not utils.pid_is_running(-1)