Make daemon startup reliable: validated config, atomic lockfile, waiting up

CLAUDE.md

@@ -81,18 +81,29 @@ 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.
 
 ### 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

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

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
+        )

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,35 +30,55 @@
 except ImportError:
     import pkg_resources
 import typer
+import yaml
 from Pyro5.errors import CommunicationError
 
 import pyrolab
 from pyrolab import (
     LOCKFILE,
     PYROLAB_LOGDIR,
     RUNTIME_CONFIG,
+    STARTUP_ERROR_FILE,
     UPDATE_CHECK_FILE,
     USER_CONFIG_FILE,
     updates,
 )
 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()