diff --git a/emrg/__main__.py b/emrg/__main__.py index 86dbd225..db9ea418 100644 --- a/emrg/__main__.py +++ b/emrg/__main__.py @@ -386,6 +386,7 @@ def _run_daemon() -> None: logging.getLogger("httpx").setLevel(logging.WARNING) from emrg.config import load_config + from emrg.memory import NotARegularFile from emrg.tool_path import ensure_tool_dirs # Rant 2026-09-27T19:45:54 defect B — the same normalization the `emrgd` @@ -398,6 +399,13 @@ def _run_daemon() -> None: except FileNotFoundError as e: print(f"Error: {e}", file=sys.stderr) sys.exit(1) + except NotARegularFile as e: + # `load_config` refuses a config that is not a regular file (issue #2103) before + # opening it — a FIFO there would otherwise block this process forever, with no + # output on either channel. A refusal this command cannot act on is reported the + # same way a missing config is: one line, exit 1. + print(f"Error: {e}", file=sys.stderr) + sys.exit(1) from emrg.server.daemon import run_server diff --git a/emrg/config.py b/emrg/config.py index 29d9eb17..6b4380c5 100644 --- a/emrg/config.py +++ b/emrg/config.py @@ -14,6 +14,9 @@ import tomllib # Python 3.11+ stdlib +from emrg.memory import NotARegularFile +from emrg.tools.base import non_regular_kind + @dataclass class LlmConfig: @@ -204,6 +207,72 @@ def config_path() -> Path: +def _refuse_non_regular(path: Path) -> None: + """Refuse a config subject that is not a regular file, naming its kind (issue #2103). + + ``kind`` comes from the tool layer's path-taking reader + (:func:`emrg.tools.base.non_regular_kind` — the one ``#2062``, ``#2076`` and the + daemon's own readers share), and the refusal is + :class:`emrg.memory.NotARegularFile`, which is an ``OSError`` naming the kind. + + Being an ``OSError`` is the point rather than an implementation detail: every reader + in this module already answers a file it cannot read — a missing one (``load_config`` + raises ``FileNotFoundError``), a corrupt one (the two section loaders fall back to + their defaults) — so routing the refusal through that same class keeps each reader's + contract exactly as written. The only behaviour that changes is that the answer + *arrives*. + + Measured 2026-10-11 (``cyc20261011-134612``) on master ``3f18f6a6``, one child process + per arm under this parent's 8 s cap, the subject built under a ``TemporaryDirectory`` + with ``config_path`` re-pointed at it before any call: + + arm regular directory FIFO + load_config(path=…) returns IsADirectoryError no return, 8 s + load_config() returns IsADirectoryError no return, 8 s + load_sandbox_config() returns returns defaults no return, 8 s + load_update_config() returns returns defaults no return, 8 s + fingerprint(path) returns a hash returns None no return, 8 s + ConfigReloader(…, path=…) constructs constructs no return, 8 s + ensure_config() returns returns returns + + The **directory** column is the reading that fixes the design: the kind one over + already produces each site's answer, so the refusal adds no new answer — and the + ``ensure_config`` row is why this module guards three readers and not a writer: + ``if cfg_path.exists(): return`` is its first act, ``exists()`` is **true** for a FIFO, + so its ``write_text`` is never reached. That one is measured, not assumed. + + An **open is not a read**: on a FIFO it waits at the open until a writer appears and + raises nothing, so no ``except (OSError, …)`` around these call sites can bound the + consequence. Two of them run before the daemon has an event loop at all — they are + called from ``EmrgServer.__init__`` (``daemon.py:637``/``:643``/``:656``) — and the + reload tick reads the file on the loop every ``POLL_INTERVAL_SECONDS``. + """ + # A missing path is not a refusal: it raises `FileNotFoundError` from the `stat`, + # which is the answer callers of a missing config already handle. One syscall, + # on the path the open is about to follow. + kind = non_regular_kind(path) + if kind: + raise NotARegularFile( + f"{path} is {kind}, not a regular file. This reader opens the config file " + f"directly, and opening it blocks until a peer appears — a FIFO waits for a " + f"writer, a socket for a connection — so the call would never return, and " + f"neither would the daemon: two of these readers run in " + f"`EmrgServer.__init__`, before any handler exists to time them out." + ) + + +def _read_config_text(path: Path) -> str: + """``path.read_text()`` after asking its kind (``_refuse_non_regular``). + + Every read of the config file in this module goes through here, and + ``tests/test_config.py::test_no_read_text_reaches_the_config_unasked`` parses this + file to keep it that way: a reader that reaches its subject through ``Path``'s own + convenience method is the shape that keeps being missed (``#2095``/``#2096``). + """ + _refuse_non_regular(path) + return path.read_text(encoding="utf-8") + + def load_config(path: Optional[Path] = None) -> EmrgConfig: """Load EMRG configuration from ~/.emrg/config.toml. @@ -218,7 +287,7 @@ def load_config(path: Optional[Path] = None) -> EmrgConfig: f"config not found at {cfg_path} — create it with [llm] section" ) - content = cfg_path.read_text(encoding="utf-8") + content = _read_config_text(cfg_path) data = tomllib.loads(content) llm_data = data.get("llm", {}) @@ -293,7 +362,7 @@ def load_sandbox_config() -> SandboxConfig: cfg_path = config_path() if cfg_path.exists(): try: - data = tomllib.loads(cfg_path.read_text(encoding="utf-8")) + data = tomllib.loads(_read_config_text(cfg_path)) except (OSError, tomllib.TOMLDecodeError): data = {} cfg = _sandbox_from(data) @@ -310,7 +379,7 @@ def load_update_config() -> UpdateConfig: if not cfg_path.exists(): return UpdateConfig() try: - data = tomllib.loads(cfg_path.read_text(encoding="utf-8")) + data = tomllib.loads(_read_config_text(cfg_path)) except (OSError, tomllib.TOMLDecodeError): return UpdateConfig() update_data = data.get("update", {}) @@ -321,7 +390,14 @@ def load_update_config() -> UpdateConfig: def ensure_config() -> None: - """Create default config file if it doesn't exist.""" + """Create default config file if it doesn't exist. + + The one writer of the config file, and the one open in this module that needs no kind + guard — measured, not assumed (issue #2103): its first act is the ``exists()`` return + below, and ``exists()`` is **true** for a FIFO, a socket and a directory alike, so a + non-regular subject never reaches the ``write_text``. Guarding it would be code no + reading can reach. + """ cfg_path = config_path() if cfg_path.exists(): return diff --git a/emrg/server/config_reload.py b/emrg/server/config_reload.py index 85895744..3f4e1d42 100644 --- a/emrg/server/config_reload.py +++ b/emrg/server/config_reload.py @@ -56,6 +56,7 @@ from typing import Optional from emrg.config import EmrgConfig, LlmConfig, UpdateConfig, config_path, load_config +from emrg.tools.base import non_regular_kind logger = logging.getLogger("emrg.server") @@ -135,6 +136,15 @@ def fingerprint(path: Path) -> Optional[str]: coarse timestamps is a property of the mount, not of the OS. """ try: + if non_regular_kind(path): + # A FIFO (or a socket, or a device) opens and then waits: `read_bytes` on it + # never returns and raises nothing, so the `except OSError` below cannot fire + # and this poll would wedge the daemon's loop instead of skipping a tick. + # `None` is already this function's answer for a subject it cannot read, and + # `poll()` already answers `None` to that by keeping the live config — so the + # refusal needs no new outcome, only that it arrives (issue #2103, measured + # 2026-10-11 on `3f18f6a6`: a FIFO here did not return within an 8 s cap). + return None data = path.read_bytes() except OSError: return None diff --git a/tests/test_config.py b/tests/test_config.py index a8dbdfb1..bce0452c 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,5 +1,6 @@ """Tests for emrg.config.""" +import os import tempfile from pathlib import Path @@ -14,7 +15,10 @@ ensure_config, ) from emrg import config as cfg_mod +from emrg.memory import NotARegularFile from emrg.server import config_reload as cr +from emrg.tools.base import non_regular_kind +from tests.bounded_read import DEADLINE_SECONDS, within def test_config_dir(): @@ -300,3 +304,211 @@ def test_find_model_entry_skips_a_malformed_row_without_raising(): models = ["not-a-mapping", {"name": "good", "model": "x"}] assert find_model_entry(models, "good") is models[1] assert find_model_entry(models, "not-a-mapping") is None + + +# ── the kind-before-open guard (issue #2103) ───────────────────────── +# +# The reads below are bounded by `tests.bounded_read.within`, the one deadline this suite +# keeps. An unbounded read is the failure mode this section is about: a FIFO blocks at the +# **open** and raises nothing, so a test that calls the reader directly has only two +# outcomes — the reader returns, or the whole pytest run wedges — and a wedged run is +# unreadable as a failure (a cancelled CI job reads as `no_verdict`, whose remedy is to +# re-trigger the same head). The deadline is what turns a regression here into a failed +# test that names the cause. +# +# `_plant` is the only way a subject is made in this section, and it always builds it under +# the test's own `tmp_path`: the config path is the host's runtime state, so a test may +# only ever hand these readers a file it created itself. +# +# A FIFO needs `os.mkfifo`, so every FIFO arm is POSIX-gated; each one has a **FIFO-free** +# counterpart on a directory, which runs on both legs and is the *kind one over* whose +# answer the design copies. + + +def _plant(path: Path, kind: str) -> None: + """Put a non-regular subject where a config file is expected.""" + path.parent.mkdir(parents=True, exist_ok=True) + if kind == "dir": + path.mkdir() + else: + os.mkfifo(path) + + +def test_load_config_refuses_a_directory_subject(tmp_path, monkeypatch): + """The FIFO-free control: a directory is refused by name, not opened. + + This is the reading the whole design leans on. A directory at the config path already + produced an answer — `IsADirectoryError`, an `OSError`, out of `read_text` — so the + refusal is the same class of answer with the kind named rather than a new outcome, and + every caller's existing handler keeps working unchanged. + """ + cfg_path = tmp_path / ".emrg" / "config.toml" + _plant(cfg_path, "dir") + monkeypatch.setattr(cfg_mod, "config_path", lambda: cfg_path) + + with pytest.raises(NotARegularFile) as excinfo: + within(DEADLINE_SECONDS, load_config) + + assert isinstance(excinfo.value, OSError), ( + "callers answer this file's trouble with `except OSError`; the refusal has to be one" + ) + assert "a directory" in str(excinfo.value) + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="a FIFO needs os.mkfifo (POSIX)") +def test_load_config_refuses_a_fifo_subject(tmp_path, monkeypatch): + """A FIFO at the config path raises, where before the call never returned at all. + + Measured 2026-10-11 (`cyc20261011-134612`) in a detached worktree of `3f18f6a6`: this + call did **not return within 8 s**, because the open waits for a writer and raises + nothing, so no handler around it can fire. + """ + cfg_path = tmp_path / ".emrg" / "config.toml" + _plant(cfg_path, "fifo") + monkeypatch.setattr(cfg_mod, "config_path", lambda: cfg_path) + + with pytest.raises(NotARegularFile) as excinfo: + within(DEADLINE_SECONDS, load_config) + + assert "FIFO" in str(excinfo.value) + + +def test_the_section_loaders_degrade_on_a_directory_subject(tmp_path, monkeypatch): + """The readers whose contract is a default keep it: they degrade, never raise. + + `load_sandbox_config` and `load_update_config` both read through + `except (OSError, TOMLDecodeError)` and their docstrings promise defaults for a file + they cannot read. `NotARegularFile` **is** an `OSError`, so the refusal lands in the + handler already written there — the answer a directory already gets, one kind over. + A guard that raised instead would turn a documented default into a crash on a path + the daemon builds itself in `__init__`. + """ + cfg_path = tmp_path / ".emrg" / "config.toml" + _plant(cfg_path, "dir") + monkeypatch.setattr(cfg_mod, "config_path", lambda: cfg_path) + + assert within(DEADLINE_SECONDS, cfg_mod.load_sandbox_config) == cfg_mod.SandboxConfig() + assert within(DEADLINE_SECONDS, cfg_mod.load_update_config) == cfg_mod.UpdateConfig() + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="a FIFO needs os.mkfifo (POSIX)") +def test_the_section_loaders_degrade_on_a_fifo_subject(tmp_path, monkeypatch): + """The same two readers, on the subject that used to block: both return 0.00 s. + + Measured before the guard (`3f18f6a6`, detached worktree): both **did not return + within 8 s**. `EmrgServer.__init__` calls them (`daemon.py:637`/`:643`), so the block + was the daemon's construction, before any event loop or handler existed. + """ + cfg_path = tmp_path / ".emrg" / "config.toml" + _plant(cfg_path, "fifo") + monkeypatch.setattr(cfg_mod, "config_path", lambda: cfg_path) + + assert within(DEADLINE_SECONDS, cfg_mod.load_sandbox_config) == cfg_mod.SandboxConfig() + assert within(DEADLINE_SECONDS, cfg_mod.load_update_config) == cfg_mod.UpdateConfig() + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="a FIFO needs os.mkfifo (POSIX)") +def test_ensure_config_never_opens_a_non_regular_subject(tmp_path, monkeypatch): + """The writer is the one open in the module that needs no guard — and this pins why. + + Measured, not assumed (`3f18f6a6`, detached worktree): `ensure_config()` **returned** + with a FIFO at the config path, because its first act is `if cfg_path.exists(): return` + and `exists()` is true for a FIFO. So the subject is left exactly as it was and no open + happens — guarding this write would be code no reading can reach. + """ + cfg_path = tmp_path / ".emrg" / "config.toml" + _plant(cfg_path, "fifo") + monkeypatch.setattr(cfg_mod, "config_path", lambda: cfg_path) + + assert within(DEADLINE_SECONDS, cfg_mod.ensure_config) is None + assert non_regular_kind(cfg_path) == "a FIFO (named pipe)", ( + "the writer replaced a subject it was told to leave alone" + ) + + +def test_no_read_text_reaches_the_config_unasked(): + """The census leg: the shape that keeps being missed is `Path`'s own method. + + `emrg/config.py` had four opens and none of them asked. Asking the source rather than + a reviewer's memory is what `#2095`/`#2096` did for the scheduler, for the reason it + gave: a bare `read_text` is invisible to a grep for `open(`, and a reader added later + must not be able to skip the guard quietly. + + The write is the measured exemption, and it is an **enumerated list** rather than a + wildcard: `ensure_config` is the one writer, and the leg above is what holds its + reason. Any `read_text`/`write_text`/`open(` outside its home fails here. + """ + import ast + + source = Path(cfg_mod.__file__).read_text(encoding="utf-8") + tree = ast.parse(source) + homes = {"read_text": "_read_config_text", "write_text": "ensure_config", "open": None} + + def enclosing(stack: list[str]) -> str: + return stack[-1] if stack else "" + + found: dict[str, set[str]] = {} + stack: list[str] = [] + + class Visitor(ast.NodeVisitor): + def visit_FunctionDef(self, node): + stack.append(node.name) + self.generic_visit(node) + stack.pop() + + visit_AsyncFunctionDef = visit_FunctionDef + + def visit_Call(self, node): + func = node.func + if isinstance(func, ast.Attribute) and func.attr in homes: + found.setdefault(func.attr, set()).add(enclosing(stack)) + elif isinstance(func, ast.Name) and func.id == "open": + found.setdefault("open", set()).add(enclosing(stack)) + self.generic_visit(node) + + Visitor().visit(tree) + for shape, home in homes.items(): + callers = found.get(shape, set()) + assert callers <= {home}, ( + f"{shape} is called from {sorted(callers - {home})}, outside its home " + f"{home!r} — every open in this module asks its subject's kind first " + f"(issue #2103)" + ) + # A leg that finds no call at all would pass vacuously: the two shapes that exist must + # be seen, or this test has stopped reading the module it names. + assert found.get("read_text") == {"_read_config_text"} + assert found.get("write_text") == {"ensure_config"} + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="a FIFO needs os.mkfifo (POSIX)") +def test_the_foreground_daemon_command_reports_a_refused_config( + tmp_path, monkeypatch, capsys +): + """`emrg server` (foreground) says why and exits 1, instead of a traceback. + + The command's answer for a config it cannot read is one line and exit 1 — the shape it + already had for a missing file — and the refusal is a new way to be unreadable, so it + takes the same exit. Without the handler it escapes as an unhandled `NotARegularFile` + traceback, which this arm kills. + + Only two outcomes are reachable here and **neither starts a daemon**: the guard + refuses (the assertion below), or the guard is gone and `load_config` blocks at the + open, which the deadline turns into a failed test. The subject is a FIFO, never a + readable config, so `asyncio.run(run_server(...))` is unreachable by construction — + the safety of this test does not rest on the guard working. + + `ensure_tool_dirs` is stubbed because it rewrites this process's `PATH`, which no test + of a config reader should leave mutated for its neighbours. + """ + cfg_path = tmp_path / ".emrg" / "config.toml" + _plant(cfg_path, "fifo") + monkeypatch.setattr(cfg_mod, "config_path", lambda: cfg_path) + monkeypatch.setattr("emrg.tool_path.ensure_tool_dirs", lambda: None) + + from emrg.__main__ import _run_daemon + + with pytest.raises(SystemExit) as excinfo: + within(DEADLINE_SECONDS, _run_daemon) + + assert excinfo.value.code == 1 + assert "not a regular file" in capsys.readouterr().err diff --git a/tests/test_config_reload.py b/tests/test_config_reload.py index bcbd342f..e12018be 100644 --- a/tests/test_config_reload.py +++ b/tests/test_config_reload.py @@ -33,6 +33,7 @@ from emrg.server import config_reload as cr from emrg.server.config_reload import ConfigReloader, describe from emrg.server.daemon import EmrgServer +from tests.bounded_read import DEADLINE_SECONDS, within BASE = """[llm] base_url = "http://localhost:1/v1" @@ -692,3 +693,58 @@ def test_every_live_field_is_actually_reloadable(tmp_path): f.name for f in dataclasses.fields(LlmConfig) if f.name != "model" ) assert a.model == LlmConfig().model + + +# ── the fingerprint asks its subject's kind first (issue #2103) ────── +# +# `fingerprint` is the reader that gives the daemon its hot-reload baseline: `poll()` +# calls it once per `POLL_INTERVAL_SECONDS` tick on the event loop, and `ConfigReloader. +# __init__` calls it once at construction — which `EmrgServer.__init__` does. Its contract +# is already "a hash, or `None` when it cannot be read", and `poll()` already answers +# `None` to that by keeping the live configuration, so the guard adds no outcome: it is +# what makes the existing one arrive. +# +# Bounded by `tests/bounded_read.within` for the reason that harness states: a reader that +# blocks at the open raises nothing, so without a deadline the failure mode of this +# section is a wedged pytest run rather than a red test. Each FIFO arm has a FIFO-free +# counterpart on a directory (which `read_bytes` refuses at once on both legs). + + +def test_fingerprint_answers_none_for_a_directory(tmp_path): + """The FIFO-free control, and the answer the guard is built on. + + `read_bytes` on a directory raises `IsADirectoryError` *before* any guard — so `None` + is the answer this function already gives a subject it cannot read, and the change is + that a FIFO gets it too rather than never returning. + """ + d = tmp_path / "config.toml" + d.mkdir() + assert within(DEADLINE_SECONDS, lambda: cr.fingerprint(d)) is None + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="a FIFO needs os.mkfifo (POSIX)") +def test_fingerprint_answers_none_for_a_fifo_instead_of_blocking(tmp_path): + """Measured before the guard (`3f18f6a6`, detached worktree): no return within 8 s.""" + p = tmp_path / "config.toml" + os.mkfifo(p) + assert within(DEADLINE_SECONDS, lambda: cr.fingerprint(p)) is None + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="a FIFO needs os.mkfifo (POSIX)") +def test_a_reloader_built_on_a_fifo_constructs_and_keeps_the_live_config(tmp_path): + """Construction alone fingerprints the file — which is how the daemon was wedged. + + `EmrgServer.__init__` builds a `ConfigReloader` (`daemon.py:656`), so before the guard + a FIFO at the config path stopped the daemon from being *constructed*: no event loop, + no client, no log line. Measured on `3f18f6a6`: did not return within 8 s. Both the + baseline and the tick after it must now answer `None`, which is `poll()`'s "nothing + changed — keep the live config". + """ + p = tmp_path / "config.toml" + os.mkfifo(p) + live = LlmConfig() + + reloader = within(DEADLINE_SECONDS, lambda: ConfigReloader(live, path=p)) + + assert within(DEADLINE_SECONDS, reloader.poll) is None + assert reloader._seen is None, "the baseline is the unreadable answer, not a hash"