From ee4498f48a55fc0bfd97fea5ca51f459f03b5b0e Mon Sep 17 00:00:00 2001 From: Sequoia Ploeg Date: Sun, 4 Oct 2026 16:42:32 -0600 Subject: [PATCH 1/2] Config correctness: rename references, atomic writes, no silent settings - `pyrolab rename` (#67) renames an entity together with every reference to it (service daemon/nameservers, daemon nameservers, autolaunch, including the implicit "default" daemon), keeps entry order, refuses an existing name unless --force, reports what changed, says "Daemon/Service ... not found" correctly, and notes when a persistent nameserver's storage file will change. New configure.rename_entity() does the work. - Atomic writes (#69): update_config, export_config, save_config, `config export`, and the lockfile all go through the new utils.atomic_write_text (temp file, fsync, os.replace), so an interrupted write leaves the previous file intact. Fixes the "File does not:" message. - No silent settings (#72): the config models are plain pydantic models instead of BaseSettings, which filled unset fields from same-named environment variables (PORT=8080 moved every daemon). Unknown keys stay errors (now covered by tests). update_pyro_config logs what it applies and warns about anything it can't, unless the model declares the field as used elsewhere (NOT_PYRO_OPTIONS). - Daemon placement (#47): host, port, unixsocket, nathost, and natport are passed to the daemon's constructor, so `port` and `unixsocket` finally take effect. - PyroLabConfiguration names its nameservers on construction, so a config loaded with from_file() no longer computes storage paths like "ns_.sql". Closes #47 Closes #67 Closes #69 Closes #72 Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 12 ++- pyrolab/cli.py | 123 +++++++++++++++--------------- pyrolab/configure.py | 161 +++++++++++++++++++++++++++++++++------- pyrolab/manager.py | 13 +++- pyrolab/pyrolabd.py | 6 +- pyrolab/utils.py | 31 ++++++++ tests/test_cli.py | 93 +++++++++++++++++++---- tests/test_configure.py | 138 +++++++++++++++++++++++++++++++++- tests/test_manager.py | 57 ++++++++++++++ tests/test_utils.py | 24 ++++++ 10 files changed, 548 insertions(+), 110 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 92bc81d..37da0ea 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,7 +84,17 @@ 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. +Anything that writes a config must re-validate before saving; `rename_entity()` renames an entity together with +every reference to it. + +The models derive from `_ConfigModel` (plain pydantic `BaseModel`, `extra = "forbid"`), **not** `BaseSettings`: +settings models fill unset fields from same-named environment variables (`PORT`, `HOST`, ...), which silently +changed configurations. Unknown keys are errors. `update_pyro_config()` warns about any field it can't apply +unless the model lists it in `NOT_PYRO_OPTIONS`. Daemon placement (`host`, `port`, `unixsocket`, `nathost`, +`natport`) goes to the daemon's constructor, since Pyro5 has no global `PORT`/`UNIXSOCKET` options. +Nameservers get their names (used for storage paths) whenever a `PyroLabConfiguration` is constructed. + +Every config/lockfile write goes through `utils.atomic_write_text()` (temp file + fsync + `os.replace`). `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 diff --git a/pyrolab/cli.py b/pyrolab/cli.py index d9ec834..7c4787d 100644 --- a/pyrolab/cli.py +++ b/pyrolab/cli.py @@ -14,7 +14,6 @@ import fileinput import platform import re -import shutil import subprocess import sys import textwrap @@ -45,14 +44,16 @@ ) from pyrolab.api import Proxy from pyrolab.configure import ( + SECTIONS, PyroLabConfiguration, describe_config_error, export_config, + rename_entity, reset_config, update_config, ) from pyrolab.pyrolabd import PyroLabDaemon, read_lockfile -from pyrolab.utils import pid_is_running +from pyrolab.utils import atomic_write_text, pid_is_running def _print_config_problems(source, exc: Exception) -> None: @@ -376,7 +377,7 @@ def config_reset(): def config_export(filename: str): """Export the configuration file""" if USER_CONFIG_FILE.exists(): - shutil.copy(USER_CONFIG_FILE, filename) + atomic_write_text(filename, USER_CONFIG_FILE.read_text()) else: typer.secho("No configuration file found.", fg=typer.colors.RED) raise typer.Abort() @@ -597,89 +598,87 @@ def logs_export(filename: str): ############################################################################### -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: +def _rename(kind: str, old_name: str, new_name: str, force: bool) -> None: + """Rename an entity in the user configuration, with every reference.""" + if not USER_CONFIG_FILE.exists(): + typer.secho("No user configuration file found.", fg=typer.colors.RED) + raise typer.Exit(1) + config = _load_config_or_exit(USER_CONFIG_FILE) + entities = getattr(config, SECTIONS[kind]) + if old_name not in entities: + typer.secho(f"{kind.capitalize()} '{old_name}' not found.", fg=typer.colors.RED) + raise typer.Exit(1) + if new_name == old_name: + typer.secho(f"{kind.capitalize()} '{old_name}' already has that name.") + return + if new_name in entities and not force: typer.secho( - f"Can't rename '{old_name}': other entries still refer to it.", + f"A {kind} named '{new_name}' already exists. Use --force to replace " + "it (and point its references at the renamed one).", 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) + + renamed, changes = rename_entity(config, kind, old_name, new_name) + export_config(renamed, USER_CONFIG_FILE) + + typer.secho(f"Renamed {kind} '{old_name}' to '{new_name}'.", fg=typer.colors.GREEN) + if changes: + typer.echo("Updated references in: " + ", ".join(changes) + ".") + if kind == "nameserver": + before = config.nameservers[old_name] + if before.storage in ("sql", "dbm"): + after = renamed.nameservers[new_name] + typer.secho( + f"Note: its registrations are stored in " + f"{before.get_storage_location().split(':', 1)[1]}; under the new " + f"name it will use {after.get_storage_location().split(':', 1)[1]}. " + "Move the file before reloading to keep them.", + fg=typer.colors.YELLOW, + ) + if kind == "service": + typer.echo("Run 'pyrolab reload' for a running daemon to use the new name.") + else: + # reload restarts only what is running under a name still in the + # configuration; it can't tell the renamed entity is the same one. + typer.echo( + f"If '{old_name}' is running, 'pyrolab reload' will stop it; then " + f"start it under its new name with 'pyrolab start {kind} {new_name}'." + ) rename_app = typer.Typer() app.add_typer(rename_app, name="rename", help="Rename a nameserver, daemon or service.") +_FORCE = typer.Option( + False, "--force", help="Replace an existing entity that already has the new name." +) + @rename_app.command("nameserver") -def rename_nameserver( - old_name: str, - new_name: str, -): +def rename_nameserver(old_name: str, new_name: str, force: bool = _FORCE): """ - Rename a nameserver. + Rename a nameserver, updating every service, daemon, and autolaunch entry + that refers to it. """ - if USER_CONFIG_FILE.exists(): - config = PyroLabConfiguration.from_file(USER_CONFIG_FILE) - if old_name in config.nameservers: - config.nameservers[new_name] = config.nameservers.pop(old_name) - _save_renamed(config, old_name) - else: - typer.secho("Nameserver not found.", fg=typer.colors.RED) - raise typer.Exit() - else: - typer.secho("No user configuration file found.", fg=typer.colors.RED) - raise typer.Exit() + _rename("nameserver", old_name, new_name, force) @rename_app.command("daemon") -def rename_daemon( - old_name: str, - new_name: str, -): +def rename_daemon(old_name: str, new_name: str, force: bool = _FORCE): """ - Rename a daemon. + Rename a daemon, updating every service and autolaunch entry that refers + to it. """ - if USER_CONFIG_FILE.exists(): - config = PyroLabConfiguration.from_file(USER_CONFIG_FILE) - if old_name in config.daemons: - config.daemons[new_name] = config.daemons.pop(old_name) - _save_renamed(config, old_name) - else: - typer.secho("Nameserver not found.", fg=typer.colors.RED) - raise typer.Exit() - else: - typer.secho("No user configuration file found.", fg=typer.colors.RED) - raise typer.Exit() + _rename("daemon", old_name, new_name, force) @rename_app.command("service") -def rename_service( - old_name: str, - new_name: str, -): +def rename_service(old_name: str, new_name: str, force: bool = _FORCE): """ Rename a service. """ - if USER_CONFIG_FILE.exists(): - config = PyroLabConfiguration.from_file(USER_CONFIG_FILE) - if old_name in config.services: - config.services[new_name] = config.services.pop(old_name) - _save_renamed(config, old_name) - else: - typer.secho("Nameserver not found.", fg=typer.colors.RED) - raise typer.Exit() - else: - typer.secho("No user configuration file found.", fg=typer.colors.RED) - raise typer.Exit() + _rename("service", old_name, new_name, force) ############################################################################### diff --git a/pyrolab/configure.py b/pyrolab/configure.py index 38a5246..897abe1 100644 --- a/pyrolab/configure.py +++ b/pyrolab/configure.py @@ -37,10 +37,10 @@ import logging import uuid from pathlib import Path -from typing import IO, Any, Dict, List, Optional, Type, Union +from typing import IO, Any, ClassVar, Dict, List, Optional, Tuple, Type, Union import Pyro5 -from pydantic import BaseModel, BaseSettings, root_validator, validator +from pydantic import BaseModel, root_validator, validator from pydantic.fields import PrivateAttr from yaml import dump, load from yaml.constructor import ConstructorError @@ -54,7 +54,7 @@ from pyrolab import NAMESERVER_STORAGE, USER_CONFIG_FILE from pyrolab.server import Daemon from pyrolab.service import Service -from pyrolab.utils import generate_random_name, get_ip +from pyrolab.utils import atomic_write_text, generate_random_name, get_ip log = logging.getLogger(__name__) @@ -171,8 +171,15 @@ def construct_mapping(self, node, deep=False): class PyroConfigMixin: """ Mixin for pydantic models, updates fields that are Pyro5 configuration options. + + A field is applied when its uppercased name is a Pyro5 config option. Fields + that are used some other way are listed in ``NOT_PYRO_OPTIONS``; any other + field that can't be applied is logged as a warning rather than silently + dropped. """ + NOT_PYRO_OPTIONS: ClassVar[frozenset] = frozenset() + def update_pyro_config(self, values: dict = None) -> Dict[str, Any]: """ Sets all key-value attributes that are Pyro5 configuration options. @@ -203,15 +210,39 @@ def update_pyro_config(self, values: dict = None) -> Dict[str, Any]: values[key] = get_ip() pyroset = {} + ignored = [] for key, value in values.items(): - key = key.upper() - if key in Pyro5.config.__slots__: - # All Pyro config options are fully uppercased - setattr(Pyro5.config, key, value) - pyroset[key] = value + option = key.upper() # All Pyro config options are fully uppercased + if option in Pyro5.config.__slots__: + setattr(Pyro5.config, option, value) + pyroset[option] = value + elif key not in self.NOT_PYRO_OPTIONS: + ignored.append(key) + log.debug("%s applied Pyro5 settings: %s", type(self).__name__, pyroset) + if ignored: + log.warning( + "%s settings ignored (not Pyro5 options): %s", + type(self).__name__, + ", ".join(ignored), + ) return pyroset +class _ConfigModel(BaseModel): + """ + Base for PyroLab's configuration models. + + These are plain pydantic models rather than ``BaseSettings``: a settings + model also fills any field the file leaves out from an environment variable + of the same name (case-insensitively), so an unrelated ``PORT`` or ``HOST`` + in the environment would silently change the configuration. + """ + + class Config: + extra = "forbid" # a misspelled key is an error, not silently ignored + validate_all = True + + class YAMLMixin: def yaml( self, @@ -294,7 +325,7 @@ def from_file(cls, filename: Union[str, Path]) -> PyroLabConfiguration: raise FileNotFoundError(f"File does not exist: '{filename}'") -class NameServerConfiguration(BaseSettings, PyroConfigMixin, YAMLMixin): +class NameServerConfiguration(_ConfigModel, PyroConfigMixin, YAMLMixin): """ The NameServer Settings class. @@ -364,6 +395,9 @@ class NameServerConfiguration(BaseSettings, PyroConfigMixin, YAMLMixin): storage: str = "memory" _name: str = PrivateAttr("") + # Passed to the nameserver directly rather than through Pyro5.config. + NOT_PYRO_OPTIONS: ClassVar[frozenset] = frozenset({"broadcast", "storage"}) + @validator("storage") def valid_memory_format(cls, v: str): if v == "memory": @@ -424,7 +458,7 @@ def update_pyro_config(self) -> Dict[str, Any]: return super().update_pyro_config(values=values) -class DaemonConfiguration(BaseSettings, PyroConfigMixin, YAMLMixin): +class DaemonConfiguration(_ConfigModel, PyroConfigMixin, YAMLMixin): """ Server configuration object. @@ -495,6 +529,12 @@ class DaemonConfiguration(BaseSettings, PyroConfigMixin, YAMLMixin): servertype: str = "thread" nameservers: List[str] = [] + # Used to load and construct the daemon (host, port, unixsocket, nathost, + # and natport are passed to its constructor), not Pyro5 options. + NOT_PYRO_OPTIONS: ClassVar[frozenset] = frozenset( + {"module", "classname", "port", "unixsocket", "nameservers"} + ) + def _get_daemon(self) -> Type[Daemon]: """ Dynamically loads the class object for the daemon given by the configuration. @@ -517,7 +557,7 @@ def _get_daemon(self) -> Type[Daemon]: return obj -class ServiceConfiguration(BaseSettings, PyroConfigMixin, YAMLMixin): +class ServiceConfiguration(_ConfigModel, YAMLMixin): """ Groups together information about a PyroLab service. @@ -623,12 +663,12 @@ def _get_service(self) -> Type[Service]: return uobj -class AutolaunchSettings(BaseSettings, YAMLMixin): +class AutolaunchSettings(_ConfigModel, YAMLMixin): nameservers: List[str] = [] daemons: List[str] = [] -class PyroLabConfiguration(BaseSettings, YAMLMixin): +class PyroLabConfiguration(_ConfigModel, YAMLMixin): """ Global configuration options for PyroLab. @@ -637,10 +677,10 @@ class PyroLabConfiguration(BaseSettings, YAMLMixin): load trusted or untampered files! If in doubt, examine the file first. It's a short text file, and should not be hard to vet. - Please call ``initialize_nameservers()`` anytime after modifying the - nameservers dictionary. Nameservers themselves contain a private attribute - of their own name, which can only be given to them by the parent - configuration object. + Nameservers know their own name (used e.g. for their storage file) only + through this parent object. Names are assigned whenever a configuration is + created or loaded; call ``initialize_nameservers()`` again after changing + the nameservers dictionary in place. Every name a section refers to must be defined: each service's ``daemon`` and ``nameservers``, each daemon's ``nameservers``, and every @@ -684,6 +724,10 @@ def check(kind, names, defined, where): raise ValueError("; ".join(problems)) return values + def __init__(self, **data: Any) -> None: + super().__init__(**data) + self.initialize_nameservers() + def initialize_nameservers(self): for name, nscfg in self.nameservers.items(): nscfg.set_name(name) @@ -815,9 +859,7 @@ def save_config(self, filename: Union[str, Path]) -> None: filename : str or Path The path to save the configuration file to. """ - filename = Path(filename) - with filename.open("w") as f: - f.write(self.config.yaml()) + atomic_write_text(filename, self.config.yaml()) def set_config(self, cfg: PyroLabConfiguration) -> None: """ @@ -931,10 +973,9 @@ def update_config(filename: Union[str, Path]) -> None: """ filename = Path(filename) if not filename.exists(): - raise FileNotFoundError(f"File does not: '{filename}'") + raise FileNotFoundError(f"File does not exist: '{filename}'") config = PyroLabConfiguration.from_file(filename) - with open(USER_CONFIG_FILE, "w") as f: - f.write(config.yaml()) + atomic_write_text(USER_CONFIG_FILE, config.yaml()) def reset_config() -> None: @@ -959,5 +1000,75 @@ def export_config(config: PyroLabConfiguration, filename: Union[str, Path]) -> N filename : str or Path The path to the configuration file or directory to export to. """ - with Path(filename).open("w") as f: - f.write(config.yaml()) + atomic_write_text(filename, config.yaml()) + + +SECTIONS = {"nameserver": "nameservers", "daemon": "daemons", "service": "services"} + + +def rename_entity( + config: PyroLabConfiguration, kind: str, old: str, new: str +) -> Tuple[PyroLabConfiguration, List[str]]: + """ + Rename a nameserver, daemon, or service, and every reference to it. + + References are each service's ``daemon`` and ``nameservers``, each + daemon's ``nameservers``, and the ``autolaunch`` lists. If ``new`` already + exists it is replaced. Entry order is kept, so the saved file changes as + little as possible. + + Parameters + ---------- + config : PyroLabConfiguration + The configuration to rename within (not modified). + kind : str + "nameserver", "daemon", or "service". + old, new : str + The current and new names. + + Returns + ------- + config, changes + The renamed, validated configuration, and a description of each + reference that was updated. + + Raises + ------ + KeyError + If there is no ``kind`` named ``old``. + """ + data = config.dict() + section = data[SECTIONS[kind]] + if old not in section: + raise KeyError(old) + data[SECTIONS[kind]] = { + (new if name == old else name): entry + for name, entry in section.items() + if name != new or new == old + } + + changes = [] + + def swap(names: List[str], where: str) -> List[str]: + if old not in names: + return names + changes.append(where) + renamed = [new if name == old else name for name in names] + return list(dict.fromkeys(renamed)) # drop duplicates, keep order + + if kind == "nameserver": + for sname, svc in data["services"].items(): + svc["nameservers"] = swap(svc["nameservers"], f"service '{sname}'") + for dname, dcfg in data["daemons"].items(): + dcfg["nameservers"] = swap(dcfg["nameservers"], f"daemon '{dname}'") + auto = data["autolaunch"] + auto["nameservers"] = swap(auto["nameservers"], "autolaunch") + elif kind == "daemon": + for sname, svc in data["services"].items(): + if svc["daemon"] == old: + svc["daemon"] = new + changes.append(f"service '{sname}'") + auto = data["autolaunch"] + auto["daemons"] = swap(auto["daemons"], "autolaunch") + + return PyroLabConfiguration.parse_obj(data), changes diff --git a/pyrolab/manager.py b/pyrolab/manager.py index 83fc24c..a9af8a8 100644 --- a/pyrolab/manager.py +++ b/pyrolab/manager.py @@ -422,8 +422,17 @@ def setup_daemon(self) -> Tuple[Daemon, Dict[str, URI]]: The instantiated Daemon object and the URI for the hosted object, to be registered with the nameserver. """ - daemon = self.daemonconfig._get_daemon() - daemon = daemon() + cfg = self.daemonconfig + # Placement goes to the constructor: port and unixsocket have no + # Pyro5.config equivalent, so setting them globally did nothing. + daemon = cfg._get_daemon()( + host=get_ip() if cfg.host == "public" else cfg.host, + port=cfg.port, + unixsocket=cfg.unixsocket, + nathost=cfg.nathost, + # Pyro wants both NAT settings or neither; 0 means "same as port". + natport=cfg.natport if cfg.nathost else None, + ) uris = {} for sname, sconfig in self.serviceconfigs.items(): diff --git a/pyrolab/pyrolabd.py b/pyrolab/pyrolabd.py index 62c9b70..f266ae9 100644 --- a/pyrolab/pyrolabd.py +++ b/pyrolab/pyrolabd.py @@ -23,7 +23,7 @@ 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 +from pyrolab.utils import atomic_write_text, pid_is_running log = logging.getLogger("pyrolab.pyrolabd") @@ -45,9 +45,7 @@ def write_lockfile(info: InstanceInfo) -> None: 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) + atomic_write_text(LOCKFILE, info.json()) def read_lockfile() -> Optional[InstanceInfo]: diff --git a/pyrolab/utils.py b/pyrolab/utils.py index dbc3de6..dc675ec 100644 --- a/pyrolab/utils.py +++ b/pyrolab/utils.py @@ -119,3 +119,34 @@ def pid_is_running(pid: int) -> bool: except PermissionError: return True # exists, owned by another user return True + + +def atomic_write_text(path, text: str) -> None: + """ + Write ``text`` to ``path`` so readers see the old contents or the new, never + a partial file. + + The text goes to a temporary file in the same directory, which is flushed + to disk and then replaces ``path`` in one step (``os.replace`` is atomic on + POSIX and Windows). If anything fails, ``path`` is left untouched. + + Parameters + ---------- + path : str or Path + The file to write. + text : str + The new contents. + """ + from pathlib import Path + + path = Path(path) + tmp = path.with_name(f"{path.name}.{os.getpid()}.tmp") + try: + with open(tmp, "w") as f: + f.write(text) + f.flush() + os.fsync(f.fileno()) + os.replace(tmp, path) + except BaseException: + tmp.unlink(missing_ok=True) + raise diff --git a/tests/test_cli.py b/tests/test_cli.py index c6d82e0..3e59045 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -396,31 +396,96 @@ def test_rename(config_with_spare_daemon, kind, section, old): assert entities["renamed"] == before +def _load(path): + return PyroLabConfiguration.from_file(path) + + +def test_rename_nameserver_updates_references(user_config): + # Regression: renaming left every reference pointing at the old name (#67). + result = runner.invoke(cli.app, ["rename", "nameserver", "local", "lab"]) + assert result.exit_code == 0, result.output + assert "Renamed nameserver 'local' to 'lab'." in result.output + assert "service 'sample.echo', daemon 'lockable', autolaunch" in result.output + # reload alone won't start it under the new name; say what will. + assert "'pyrolab start nameserver lab'" in result.output + + config = _load(user_config) + assert list(config.nameservers) == ["lab", "persistent"] # order kept + assert config.services["sample.echo"].nameservers == ["lab"] + assert config.daemons["lockable"].nameservers == ["lab"] + assert config.autolaunch.nameservers == ["lab"] + + +def test_rename_daemon_updates_references(user_config): + result = runner.invoke(cli.app, ["rename", "daemon", "lockable", "bench"]) + assert result.exit_code == 0, result.output + config = _load(user_config) + assert config.services["sample.instrument"].daemon == "bench" + assert config.autolaunch.daemons == ["bench"] + assert config.services["sample.echo"].daemon == "plain" # untouched + + +def test_rename_default_daemon_updates_implicit_references(data_dir): + # A service with no `daemon:` refers to the one called "default". + data_dir.USER_CONFIG_FILE.write_text( + "daemons: {default: {}}\n" + "services:\n s: {module: pyrolab.drivers.sample, classname: SampleService}\n" + ) + result = runner.invoke(cli.app, ["rename", "daemon", "default", "main"]) + assert result.exit_code == 0, result.output + assert _load(data_dir.USER_CONFIG_FILE).services["s"].daemon == "main" + + +def test_rename_onto_existing_name_needs_force(user_config): + before = user_config.read_text() + result = runner.invoke(cli.app, ["rename", "nameserver", "local", "persistent"]) + assert result.exit_code == 1 + assert "already exists" in result.output + assert user_config.read_text() == before # nothing destroyed + + +def test_rename_with_force_replaces_and_merges_references(user_config): + config = _load(user_config) + config.services["sample.echo"].nameservers = ["local", "persistent"] + user_config.write_text(config.yaml()) + + result = runner.invoke( + cli.app, ["rename", "nameserver", "local", "persistent", "--force"] + ) + assert result.exit_code == 0, result.output + config = _load(user_config) + assert list(config.nameservers) == ["persistent"] + assert config.nameservers["persistent"].ns_port == 9090 # the renamed one + assert config.services["sample.echo"].nameservers == ["persistent"] + + @pytest.mark.parametrize( - "kind, old, referrer", - [ - ("nameserver", "local", "daemons.lockable"), - ("daemon", "plain", "service 'sample.echo'"), - ], + "kind, label", + [("nameserver", "Nameserver"), ("daemon", "Daemon"), ("service", "Service")], ) -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. +def test_rename_unknown(user_config, kind, label): + # Regression: daemon and service renames said "Nameserver not found" (#67). before = user_config.read_text() - result = runner.invoke(cli.app, ["rename", kind, old, "renamed"]) + result = runner.invoke(cli.app, ["rename", kind, "nope", "new"]) assert result.exit_code == 1 - assert "still refer to it" in result.output - assert f"'{old}'" in result.output + assert f"{label} 'nope' not found." in result.output assert user_config.read_text() == before -def test_rename_unknown_nameserver(user_config): +def test_rename_to_same_name(user_config): before = user_config.read_text() - result = runner.invoke(cli.app, ["rename", "nameserver", "nope", "new"]) - assert "Nameserver not found" in result.output + result = runner.invoke(cli.app, ["rename", "service", "sample.echo", "sample.echo"]) + assert result.exit_code == 0 assert user_config.read_text() == before +def test_rename_persistent_nameserver_mentions_storage(user_config): + result = runner.invoke(cli.app, ["rename", "nameserver", "persistent", "archive"]) + assert result.exit_code == 0, result.output + assert "ns_persistent.sql" in result.output + assert "ns_archive.sql" in result.output + + def test_rename_without_config(data_dir): result = runner.invoke(cli.app, ["rename", "daemon", "a", "b"]) assert "No user configuration file found" in result.output diff --git a/tests/test_configure.py b/tests/test_configure.py index cb8d521..50dceb0 100644 --- a/tests/test_configure.py +++ b/tests/test_configure.py @@ -1,3 +1,5 @@ +import logging + import Pyro5 import pytest from pydantic import ValidationError @@ -14,6 +16,7 @@ UniqueOrAutoKeyLoader, describe_config_error, export_config, + rename_entity, reset_config, uniquify_class, update_config, @@ -110,9 +113,10 @@ def test_service_requires_module_and_classname(): PyroLabConfiguration.from_yaml("services:\n svc:\n module: x\n") -def test_initialize_nameservers_assigns_names(sample_config_file): +def test_loading_assigns_nameserver_names(sample_config_file): + # Previously only GlobalConfiguration.load_config() named them, so a + # config loaded any other way computed storage paths like "ns_.sql". cfg = PyroLabConfiguration.from_file(sample_config_file) - cfg.initialize_nameservers() assert cfg.nameservers["local"].name == "local" assert cfg.nameservers["persistent"].name == "persistent" @@ -442,3 +446,133 @@ def test_export_config(tmp_path, sample_config_file): out = tmp_path / "exported.yaml" export_config(cfg, out) assert PyroLabConfiguration.from_file(out) == cfg + + +############################################################################### +# rename_entity (#67) +############################################################################### + + +def test_rename_entity_leaves_input_unchanged(sample_config_file): + config = PyroLabConfiguration.from_file(sample_config_file) + before = config.copy(deep=True) + renamed, changes = rename_entity(config, "nameserver", "local", "lab") + assert config == before + assert "lab" in renamed.nameservers and "local" not in renamed.nameservers + assert changes == ["service 'sample.echo'", "daemon 'lockable'", "autolaunch"] + assert renamed.nameservers["lab"].name == "lab" + + +def test_rename_entity_unknown(sample_config_file): + config = PyroLabConfiguration.from_file(sample_config_file) + with pytest.raises(KeyError): + rename_entity(config, "daemon", "nope", "new") + + +def test_rename_service_has_no_references(sample_config_file): + config = PyroLabConfiguration.from_file(sample_config_file) + renamed, changes = rename_entity(config, "service", "sample.echo", "echo") + assert changes == [] + assert list(renamed.services) == ["echo", "sample.instrument"] + + +############################################################################### +# Settings are never silently changed or dropped (#72) +############################################################################### + + +def test_environment_variables_do_not_leak_into_config(monkeypatch): + # These were BaseSettings models, which fill any field the file leaves out + # from an environment variable of the same name. + monkeypatch.setenv("PORT", "8080") + monkeypatch.setenv("SERVERTYPE", "multiplex") + monkeypatch.setenv("HOST", "elsewhere.example") + monkeypatch.setenv("STORAGE", "sql") + + daemon = DaemonConfiguration() + assert (daemon.port, daemon.servertype, daemon.host) == (0, "thread", "localhost") + assert NameServerConfiguration().storage == "memory" + + cfg = PyroLabConfiguration.from_yaml("daemons:\n d: {}\n") + assert cfg.daemons["d"].port == 0 + + +@pytest.mark.parametrize( + "yaml_text, field", + [ + ("daemons:\n d: {servertyp: multiplex}\n", "servertyp"), + ("nameservers:\n n: {nsport: 9000}\n", "nsport"), + ("services:\n s: {module: m, classname: C, paramaters: {}}\n", "paramaters"), + ("autolaunch: {deamons: []}\n", "deamons"), + ("nameserver: {}\n", "nameserver"), + ], +) +def test_misspelled_keys_are_rejected(yaml_text, field): + with pytest.raises(ValidationError) as excinfo: + PyroLabConfiguration.from_yaml(yaml_text) + (line,) = describe_config_error(excinfo.value) + assert field in line and "extra fields not permitted" in line + + +@pytest.mark.parametrize( + "cfg", + [NameServerConfiguration(storage="sql", broadcast=True), DaemonConfiguration()], +) +def test_update_pyro_config_logs_and_does_not_warn_for_known_fields(cfg, caplog): + with caplog.at_level(logging.DEBUG, logger="pyrolab.configure"): + applied = cfg.update_pyro_config() + assert "HOST" in applied + assert "applied Pyro5 settings" in caplog.text + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + + +def test_update_pyro_config_warns_about_settings_it_cannot_apply(caplog): + DaemonConfiguration().update_pyro_config( + values={"host": "127.0.0.1", "mystery_option": 1} + ) + assert "settings ignored (not Pyro5 options): mystery_option" in caplog.text + + +############################################################################### +# Configuration files are written atomically (#69) +############################################################################### + + +@pytest.fixture +def failing_replace(monkeypatch): + """Make the final step of an atomic write fail, as a crash would.""" + import pyrolab.utils + + def boom(src, dst): + raise OSError("disk full") + + monkeypatch.setattr(pyrolab.utils.os, "replace", boom) + + +def test_update_config_failure_keeps_previous_config( + data_dir, sample_config_file, tmp_path, failing_replace +): + data_dir.USER_CONFIG_FILE.write_text("daemons: {d: {}}\n") + with pytest.raises(OSError): + update_config(sample_config_file) + assert data_dir.USER_CONFIG_FILE.read_text() == "daemons: {d: {}}\n" + assert list(data_dir.root.glob("*.tmp")) == [] + + +def test_export_and_save_failure_keeps_existing_file( + global_config, sample_config_file, tmp_path, failing_replace +): + out = tmp_path / "out.yaml" + out.write_text("original\n") + cfg = PyroLabConfiguration.from_file(sample_config_file) + with pytest.raises(OSError): + export_config(cfg, out) + global_config.set_config(cfg) + with pytest.raises(OSError): + global_config.save_config(out) + assert out.read_text() == "original\n" + + +def test_update_config_missing_file_message(data_dir, tmp_path): + with pytest.raises(FileNotFoundError, match="does not exist"): + update_config(tmp_path / "missing.yaml") diff --git a/tests/test_manager.py b/tests/test_manager.py index b9a7d83..1963254 100644 --- a/tests/test_manager.py +++ b/tests/test_manager.py @@ -9,6 +9,7 @@ import multiprocessing import queue +import socket from datetime import datetime, timedelta import pytest @@ -253,3 +254,59 @@ def test_daemon_runner_setup_self_registers_with_nameservers(serve): hosted = daemon.objectsById[uris["sample.instrument"].object] assert issubclass(hosted, Lockable) + + +############################################################################### +# Daemon placement (#47) +############################################################################### + + +def _plain_runner(daemonconfig): + return _daemon_runner(daemonconfig, {}, {}) + + +def test_daemon_binds_configured_port(): + # Regression: port was only "applied" through Pyro5.config, which has no + # such option, so the daemon always picked a random port (#47). + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + port = s.getsockname()[1] + daemon, _ = _plain_runner( + DaemonConfiguration(host="127.0.0.1", port=port) + ).setup_daemon() + try: + assert daemon.locationStr == f"127.0.0.1:{port}" + finally: + daemon.close() + + +@pytest.mark.skipif(not hasattr(socket, "AF_UNIX"), reason="no Unix sockets") +def test_daemon_binds_configured_unixsocket(tmp_path): + path = tmp_path / "lab.sock" + daemon, _ = _plain_runner(DaemonConfiguration(unixsocket=str(path))).setup_daemon() + try: + assert daemon.locationStr == f"./u:{path}" + assert path.exists() + finally: + daemon.close() + + +def test_daemon_public_host_is_resolved(monkeypatch): + import pyrolab.manager + + monkeypatch.setattr(pyrolab.manager, "get_ip", lambda: "127.0.0.1") + daemon, _ = _plain_runner(DaemonConfiguration(host="public")).setup_daemon() + try: + assert daemon.locationStr.startswith("127.0.0.1:") + finally: + daemon.close() + + +def test_daemon_nat_settings_are_passed_through(): + daemon, _ = _plain_runner( + DaemonConfiguration(host="127.0.0.1", nathost="lab.example.org", natport=9999) + ).setup_daemon() + try: + assert daemon.natLocationStr == "lab.example.org:9999" + finally: + daemon.close() diff --git a/tests/test_utils.py b/tests/test_utils.py index ec9aead..64d0480 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -63,3 +63,27 @@ def test_pid_is_running_for_an_exited_process(): def test_pid_is_running_rejects_invalid_pids(): assert not utils.pid_is_running(0) assert not utils.pid_is_running(-1) + + +def test_atomic_write_text(tmp_path): + target = tmp_path / "file.txt" + utils.atomic_write_text(target, "first") + utils.atomic_write_text(target, "second") + assert target.read_text() == "second" + assert [f.name for f in tmp_path.iterdir()] == ["file.txt"] + + +def test_atomic_write_text_failure_leaves_original(tmp_path, monkeypatch): + target = tmp_path / "file.txt" + target.write_text("original") + + def boom(src, dst): + raise OSError("disk full") + + monkeypatch.setattr(utils.os, "replace", boom) + try: + utils.atomic_write_text(target, "new") + except OSError: + pass + assert target.read_text() == "original" + assert [f.name for f in tmp_path.iterdir()] == ["file.txt"] # no temp left From ef64b60ef77a6b4e54f41786180f5d1294a8e51c Mon Sep 17 00:00:00 2001 From: Sequoia Ploeg Date: Sun, 4 Oct 2026 17:16:39 -0600 Subject: [PATCH 2/2] Make reload start autolaunch entries reload restarted only what was running under names still in the configuration, so an autolaunched nameserver or daemon that had been renamed (or added to autolaunch) stayed stopped until started by hand. reload now also starts every autolaunch entry, nameservers first, and counts them in its result, so it matches a fresh `pyrolab up` plus anything started by hand. The rename hint now says plain `reload` is enough for autolaunched entities. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 ++ pyrolab/cli.py | 12 +++++++----- pyrolab/manager.py | 19 +++++++++++++------ tests/test_cli.py | 11 +++++++++-- tests/test_lifecycle.py | 22 ++++++++++++++++++++++ 5 files changed, 53 insertions(+), 13 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 37da0ea..9f21846 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -119,6 +119,8 @@ a hung instrument kills only its own process. Runners are controlled by sentinel - **Restart**: `checkup()` runs every second; a crash is restarted after `RESTART_BACKOFF` (5, 15, 60, 300s), and after `MAX_CONSECUTIVE_FAILURES` (5) the entity is `failed` until started by hand or `reload`. Running `STABLE_UPTIME` (10 min) before dying resets the count. `ps` shows status plus `LAST ERROR`. +- **Reload**: stops everything, then starts what was running (if still configured) plus every `autolaunch` + entry — nameservers first, each awaited — and returns True only if all of it came back. - **Ready**: runners publish `ready`/`error` in a shared dict. `launch_*(wait=True)` (used by `pyrolab start` and `reload`) returns `{"status", "error"}`; autolaunch and automatic restarts don't wait. - **Registration** (`Registrations` class, child side) is non-fatal and retried every `REGISTRATION_RETRY` (30s) diff --git a/pyrolab/cli.py b/pyrolab/cli.py index 7c4787d..5490fc5 100644 --- a/pyrolab/cli.py +++ b/pyrolab/cli.py @@ -304,7 +304,8 @@ def reload(): """ Reload the PyroLab daemon using the latest configuration file. - Useful if the configuration file has been updated. + Restarts everything that was running (if still configured) and starts every + autolaunch entry. """ daemon = get_daemon(suppress_reload_message=True) if not USER_CONFIG_FILE.exists(): @@ -636,11 +637,12 @@ def _rename(kind: str, old_name: str, new_name: str, force: bool) -> None: "Move the file before reloading to keep them.", fg=typer.colors.YELLOW, ) - if kind == "service": - typer.echo("Run 'pyrolab reload' for a running daemon to use the new name.") + if kind == "service" or new_name in getattr(renamed.autolaunch, SECTIONS[kind]): + # reload restarts the service's daemon, and starts autolaunch entries. + typer.echo("Run 'pyrolab reload' to apply the new name.") else: - # reload restarts only what is running under a name still in the - # configuration; it can't tell the renamed entity is the same one. + # reload restarts what was running under names still configured; it + # can't tell the renamed entity is the same one. typer.echo( f"If '{old_name}' is running, 'pyrolab reload' will stop it; then " f"start it under its new name with 'pyrolab start {kind} {new_name}'." diff --git a/pyrolab/manager.py b/pyrolab/manager.py index a9af8a8..2d617b3 100644 --- a/pyrolab/manager.py +++ b/pyrolab/manager.py @@ -1083,14 +1083,17 @@ def reload(self) -> bool: """ Restart every managed entity with the current configuration. - Entities no longer in the configuration are stopped and not restarted. - Crashed or failed ones are given a fresh start. + Everything running is stopped. Then everything that was running and is + still configured is started again, along with every ``autolaunch`` + entry (so the result matches a fresh ``pyrolab up``, plus anything + started by hand). Entities no longer in the configuration stay + stopped. Crashed or failed ones are given a fresh start. Returns ------- bool - True only if everything stopped cleanly and every relaunched - entity reported ready. + True only if everything stopped cleanly and every started entity + reported ready. """ log.info("Reloading all running entities.") self.stop_checkup_timer() @@ -1104,10 +1107,14 @@ def reload(self) -> bool: ok = all(stopped.values()) # Nameservers first, so daemons can register with them. - for kind, names in ((NAMESERVER, nameservers), (DAEMON, daemons)): + autolaunch = self.GLOBAL_CONFIG.config.autolaunch + for kind, names in ( + (NAMESERVER, nameservers + autolaunch.nameservers), + (DAEMON, daemons + autolaunch.daemons), + ): launched = [ (name, self._launch(kind, name, wait=False, timeout=READY_TIMEOUT)) - for name in names + for name in dict.fromkeys(names) # each once, in order if name in self._configured(kind) ] for name, result in launched: diff --git a/tests/test_cli.py b/tests/test_cli.py index 3e59045..0dcce8c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -406,8 +406,8 @@ def test_rename_nameserver_updates_references(user_config): assert result.exit_code == 0, result.output assert "Renamed nameserver 'local' to 'lab'." in result.output assert "service 'sample.echo', daemon 'lockable', autolaunch" in result.output - # reload alone won't start it under the new name; say what will. - assert "'pyrolab start nameserver lab'" in result.output + # It's in autolaunch, which reload starts, so reload alone is enough. + assert "Run 'pyrolab reload' to apply the new name." in result.output config = _load(user_config) assert list(config.nameservers) == ["lab", "persistent"] # order kept @@ -479,6 +479,13 @@ def test_rename_to_same_name(user_config): assert user_config.read_text() == before +def test_rename_outside_autolaunch_explains_restart(user_config): + # "persistent" isn't autolaunched, so reload won't start it under its new + # name; say what will. + result = runner.invoke(cli.app, ["rename", "nameserver", "persistent", "archive"]) + assert "'pyrolab start nameserver archive'" in result.output + + def test_rename_persistent_nameserver_mentions_storage(user_config): result = runner.invoke(cli.app, ["rename", "nameserver", "persistent", "archive"]) assert result.exit_code == 0, result.output diff --git a/tests/test_lifecycle.py b/tests/test_lifecycle.py index 66f8578..395aca8 100644 --- a/tests/test_lifecycle.py +++ b/tests/test_lifecycle.py @@ -390,6 +390,27 @@ def test_reload_restarts_and_reports_success(pm): assert len(pm.spawned["plain"]) == 2 +def test_reload_starts_autolaunch_entries(pm): + # The sample config autolaunches nameserver "local" and daemon "lockable"; + # only "plain" (not in autolaunch) is running before the reload. + pm.launch_daemon("plain", wait=True) + assert pm.reload() is True + assert set(pm.nameservers) == {"local"} + assert set(pm.daemons) == {"plain", "lockable"} + assert len(pm.spawned["plain"]) == 2 # restarted, not duplicated + + +def test_reload_starts_each_entity_once(pm): + pm.launch_nameserver("local", wait=True) # running and in autolaunch + assert pm.reload() is True + assert len(pm.spawned["local"]) == 2 # stopped once, started once + + +def test_reload_reports_failing_autolaunch_entry(pm): + pm.behavior["lockable"] = {"fail": "RuntimeError: no instrument"} + assert pm.reload() is False + + def test_reload_reports_failure(pm): pm.launch_daemon("plain", wait=True) pm.behavior["plain"] = {"fail": "ValueError: bad parameter"} @@ -401,6 +422,7 @@ def test_reload_drops_entities_removed_from_config(pm): del pm.GLOBAL_CONFIG.config.daemons["plain"] assert pm.reload() is True assert "plain" not in pm.daemons + assert len(pm.spawned["plain"]) == 1 def test_reload_revives_failed_entity(pm):