emrg: the abort-run store and the skill publisher ask their subject's kind

emrg/server/abort_runs.py

@@ -42,6 +42,11 @@
 from emrg.config import config_dir
 from emrg.server.atomic import atomic_write_bytes
 
+# The "is this path a regular file" rule is the tool layer's, reached at a path. Imported
+# rather than restated, for the reason `emrg/sessions_index.py` gives at the same import:
+# the boundary is one fact, and a second spelling of it is a second boundary that can drift.
+from emrg.tools.base import non_regular_kind
+
 logger = logging.getLogger(__name__)
 
 
@@ -147,6 +152,46 @@ def _load(self) -> dict[str, dict]:
         return self._state
 
     def _read(self) -> dict[str, dict]:
+        """The stored state; unreadable, missing and non-regular all read as "nothing open".
+
+        The subject's **kind** is asked before it is opened, because opening it is what decides
+        whether this call returns at all: `read_text` on a FIFO waits at the *open* for a writer,
+        raises nothing, and so the handlers below are unreachable — no exception ever reaches
+        them and the call simply never ends. Measured 2026-10-11 (`cyc20261011-130932`) on master
+        `3f18f6a6`, one child process per arm under an 8 s cap: a `mkfifo` at the state path left
+        `AbortRuns.snapshot()` and `AbortRuns.note()` **not returned in 8 s**, where the same
+        fixture with a regular file returned in 0.00 s.
+
+        Why it matters more here than elsewhere in this family: `note()` is called from the
+        content-filter path — `emrg/server/daemon.py:4352` / `:4410`, the moment a turn has just
+        lost its answer and the retry ladder is spent — as the argument of the `logger.error` that
+        reports the run, with the `_broadcast` of the error frame *after* it. The block therefore
+        hangs the daemon exactly where the client is waiting for the explanation of a failed turn.
+
+        Degraded rather than raised, which is this module's own contract rather than a preference
+        — see the module docstring: *"Every read and write here is best-effort. An unreadable file
+        reads as 'no run open'"*. A refusal would make the abort path raise while it is trying to
+        *report* an abort. It is safe for the reason `emrg/sessions_index.py::_load` gives at the
+        same answer: the state is re-derived on the next `note()`, and `_save` is
+        `atomic_write_bytes` (`mkstemp` + `os.replace`), which renames over the destination
+        instead of opening it — measured, a FIFO destination returns at once — so there is no
+        block for a reader-side guard to move onto the writer.
+
+        The kind is named in the log so the omission is diagnosable rather than silent.
+        """
+        try:
+            kind = non_regular_kind(self._path)
+        except OSError:
+            # Absent (or unstattable) — the empty state a missing file has always got. The
+            # `FileNotFoundError` branch this replaces is now this one: `non_regular_kind`
+            # *stats*, so absence arrives as an OSError rather than as a `None` kind.
+            return {}
+        if kind:
+            logger.warning(
+                "abort-run state at %s is %s, not a regular file — counting from scratch",
+                self._path, kind,
+            )
+            return {}
         try:
             raw = json.loads(self._path.read_text(encoding="utf-8"))
         except FileNotFoundError:

emrg/skills/installer.py

@@ -44,12 +44,18 @@
 from typing import Awaitable, Callable, Optional
 
 from emrg.config import config_dir
+from emrg.memory import NotARegularFile
 from emrg.skills.registry import (
     find_catalog_skill,
     read_state,
     write_state,
 )
 
+# The "is this path a regular file" rule is the tool layer's, reached at a path. Imported
+# rather than restated, for the reason `emrg/skills/registry.py` gives at the same import:
+# the boundary is one fact, and a second spelling of it is a second boundary that can drift.
+from emrg.tools.base import non_regular_kind
+
 logger = logging.getLogger(__name__)
 
 CLI_NAME = "browser-harness"
@@ -168,6 +174,27 @@ async def _publish_skill(entry: dict, runner: Optional[Runner]) -> dict:
     dest.mkdir(parents=True, exist_ok=True)
     target = dest / f"{name}.md"
     try:
+        # The subject's kind is asked before it is opened, because the open is what decides
+        # whether this call returns at all: `Path.write_text` is `open(path, "w")`, and opening
+        # a FIFO for **writing** waits for a *reader* to appear — raises nothing, so the
+        # `except OSError` below would never be reached and the install would never finish.
+        # Measured 2026-10-11 (`cyc20261011-130932`) on master `3f18f6a6`, one child process per
+        # arm under an 8 s cap: a `mkfifo` at `<dest>/probe.md` left `_publish_skill` **not
+        # returned in 8 s**, where the same fixture with a regular file returned in 0.00 s with
+        # `{"ok": True, ...}`.
+        #
+        # Refused rather than degraded, and named: `NotARegularFile` **is** an `OSError`, so it
+        # lands in the handler already written for a failed write and produces the reading this
+        # function already gives one — `{"error": "cannot write skill file: <the kind>"}` —
+        # which `install_skill` returns to the tool and the client. The `FileNotFoundError`
+        # branch is not optional: a writer's normal case is a destination that does not exist
+        # yet, and this function exists to create it.
+        try:
+            kind = non_regular_kind(target)
+        except FileNotFoundError:
+            kind = None
+        if kind:
+            raise NotARegularFile(f"{target} is {kind}, not a regular file")
         target.write_text(skill_text + "\n", encoding="utf-8")
     except OSError as e:
         return {"error": f"cannot write skill file: {e}"}

tests/test_abort_runs.py

@@ -11,8 +11,13 @@
 from __future__ import annotations
 
 import json
+import logging
+import os
 from datetime import datetime, timedelta
 
+import pytest
+
+from tests.bounded_read import DEADLINE_SECONDS, within
 import emrg.server.abort_runs as mod
 from emrg.server.abort_runs import RUN_TTL_DAYS, AbortRuns, default_path
 
@@ -249,3 +254,108 @@ def test_the_test_suite_is_not_pointed_at_the_hosts_state(tmp_path):
     # reading the sibling above wants, and the only one that can see the
     # resolution rather than the redirect.
     assert mod.default_path() == tmp_path / "abort-runs.json"
+
+
+class TestTheStateFileIsOpenedByKind:
+    """The state file itself is asked its kind before it is opened (issue #2101).
+
+    `_read` opened its subject inside `except (OSError, ValueError)` / `except FileNotFoundError`
+    — neither of which can bound a *block*: `read_text` on a FIFO waits at the open for a writer,
+    raises nothing, so no handler runs and the call never returns. Measured 2026-10-11
+    (`cyc20261011-130932`) on master `3f18f6a6`: a `mkfifo` at `~/.emrg/logs/abort-runs.json`
+    left `AbortRuns.snapshot()` and `AbortRuns.note()` **not returned in 8 s**, where the same
+    fixture with a regular file returned in 0.00 s.
+
+    `note()` is the reason this one is not merely a hang: the daemon calls it from the
+    content-filter path (`daemon.py:4352`, `:4410`) as the argument of the `logger.error` that
+    reports the run, and the error frame to the client is broadcast *after* it — so the block
+    lands exactly where a client is waiting to be told why its turn failed.
+
+    The answer is the module's own: an unreadable file reads as "no run open" (module docstring),
+    so a non-regular one does too.
+
+    The FIFO subjects carry an `os.mkfifo` gate, so they skip where there are no FIFOs; the arms
+    that cover those platforms are the FIFO-free ones — and one of them,
+    `test_a_directory_is_named_rather_than_merely_unreadable`, is **discriminating** as well.
+    That distinction is the point rather than a detail: a FIFO-free arm that passes both with
+    and without the guard does not cover the leg it appears to cover, which is what an earlier
+    version of this paragraph claimed it did.
+    """
+
+    @pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="this platform has no FIFOs")
+    def test_a_fifo_state_file_reads_as_nothing_open(self, tmp_path):
+        state = tmp_path / "abort-runs.json"
+        os.mkfifo(state)
+        runs = AbortRuns(state)
+        assert within(DEADLINE_SECONDS, lambda: runs.snapshot()) == {}
+
+    def test_the_control_with_a_regular_state_file(self, tmp_path):
+        """The kind is the axis: the same reader still reads a regular file."""
+        state = tmp_path / "abort-runs.json"
+        state.write_text(json.dumps({"k: s": {"cause": "k", "session_id": "s", "count": 1}}),
+                         encoding="utf-8")
+        assert list(AbortRuns(state).snapshot()) == ["k: s"]
+
+    @pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="this platform has no FIFOs")
+    def test_the_abort_path_notes_an_abort_rather_than_hanging(self, tmp_path):
+        """The daemon's own call, not just the reader: `note()` returns and the count is kept.
+
+        It returns `count == 1` rather than raising, because the module's contract is that this
+        store may never fail a turn that has already lost its answer.
+        """
+        state = tmp_path / "abort-runs.json"
+        os.mkfifo(state)
+        runs = AbortRuns(state)
+        record = within(DEADLINE_SECONDS, lambda: runs.note("content_filter", "s_1"))
+        assert record["count"] == 1, record
+
+    def test_a_missing_state_file_is_still_empty(self, tmp_path):
+        """The contract the kind check must not narrow: absent reads as `{}`, not as a raise.
+
+        FIFO-free on purpose — this one runs on every platform.
+        """
+        assert AbortRuns(tmp_path / "absent.json").snapshot() == {}
+
+    def test_a_directory_is_named_rather_than_merely_unreadable(self, tmp_path, caplog):
+        """**FIFO-free, and it discriminates** — what the Windows leg needs.
+
+        The two FIFO arms above are gated on `os.mkfifo`, so they skip wherever there are
+        none (Windows), and every other arm in this class passes with *and* without the
+        guard: measured 2026-10-11 on this file with `emrg/server/abort_runs.py` reverted to
+        `3f18f6a6`, the FIFO-free half is green either way (60 passed, 3 deselected across
+        this file and `tests/test_skills_registry.py`). The claim that "the Windows leg runs
+        the discriminating half" was therefore false, and the fix is to give that leg an arm
+        that discriminates rather than to weaken the sentence.
+
+        A directory is that arm, and it is why this one reads the **log**: both versions
+        answer `{}` for a directory — the module's "no run open" is the same answer on both
+        — so the return value cannot tell them apart. What can is the sentence they leave
+        behind, which is this class's whole subject:
+
+        ==================  =================================================================
+        base `3f18f6a6`     ``abort-run state at … is unreadable (IsADirectoryError: …)``
+        this tree           ``abort-run state at … is a directory, not a regular file — …``
+        ==================  =================================================================
+
+        What is pinned is this family's own **phrase** and the path, the same way the FIFO
+        arms pin `"a FIFO (named pipe)"` and `"not a regular file"` separately — not the
+        sentence around them, which is free to be reworded (measured: the equivalent
+        reword ``is a directory (not a regular file)`` leaves this arm green). The base
+        cannot produce the phrase at all: its own wording is the exception's, and the
+        exception's text differs by platform (``Is a directory`` here, ``Permission
+        denied`` on Windows) — which is why pinning *that* would not have survived Windows.
+        """
+        state = tmp_path / "abort-runs.json"
+        state.mkdir()
+
+        with caplog.at_level(logging.WARNING, logger="emrg.server.abort_runs"):
+            assert AbortRuns(state).snapshot() == {}
+
+        assert "not a regular file" in caplog.text, caplog.text
+        assert str(state) in caplog.text, caplog.text
+
+    def test_an_unreadable_state_file_still_counts_from_scratch(self, tmp_path):
+        """The branch the kind check must not shadow: a corrupt file still degrades, still logs."""
+        state = tmp_path / "abort-runs.json"
+        state.write_text("{not json", encoding="utf-8")
+        assert AbortRuns(state).snapshot() == {}

tests/test_skills_registry.py

@@ -465,3 +465,89 @@ def test_a_missing_subject_is_still_the_empty_answer(self, tmp_home):
         assert load_catalog_skills() == []
         assert read_state() == {}
         assert skill_is_managed("anything") is False
+
+
+# ── the publisher asks the target's kind (issue #2101) ───────────────
+
+class TestThePublisherAsksTheKindBeforeOpening:
+    """`_publish_skill` wrote its target without asking what it was.
+
+    `Path.write_text` is `open(path, "w")`, and opening a FIFO for **writing** waits for a
+    *reader* — the opposite peer to the read case, so no guard elsewhere in this family would
+    have covered it. Measured 2026-10-11 (`cyc20261011-130932`) on master `3f18f6a6`, one child
+    process per arm under an 8 s cap: a `mkfifo` at `<dest>/probe.md` left `_publish_skill`
+    **not returned in 8 s**, where the same fixture with a regular file returned in 0.00 s with
+    `{"ok": True, ...}`. `skills_install` runs on the daemon's event loop, so the block is the
+    server.
+
+    The answer is the refusal this function already gives a failed write — `NotARegularFile` is
+    an `OSError`, so it lands in the `except OSError` already written there and comes back as
+    `{"error": "cannot write skill file: …"}` — now with the kind named.
+
+    The FIFO arm carries an `os.mkfifo` gate, so it skips where there are no FIFOs; the arm
+    that covers those platforms is `test_a_directory_target_is_refused_by_kind_on_every_platform`,
+    which is FIFO-free **and discriminating** (see its docstring for the two-way measurement).
+    """
+
+    @pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="this platform has no FIFOs")
+    def test_a_fifo_target_is_refused_rather_than_blocking(self, tmp_home, with_cli):
+        ensure_catalog_file()
+        os.mkfifo(tmp_home / ".emrg" / "skills" / "browser-harness.md")
+        runner = FakeRunner(skill_output=VALID_SKILL_MD)
+
+        result = within(DEADLINE_SECONDS, lambda: _run(installer.install_skill(
+            "browser-harness", confirmed=True, runner=runner, http_get=FakeHttp("v0.1.8")
+        )))
+
+        assert "error" in result, result
+        assert "cannot write skill file" in result["error"], result["error"]
+        assert "regular file" in result["error"], result["error"]
+        assert read_state() == {}, "nothing may be recorded when the publish did not happen"
+
+    def test_the_control_with_a_regular_target(self, tmp_home, with_cli):
+        """The kind is the axis: the same call with no FIFO anywhere still publishes."""
+        ensure_catalog_file()
+        runner = FakeRunner(skill_output=VALID_SKILL_MD)
+
+        result = _run(installer.install_skill(
+            "browser-harness", confirmed=True, runner=runner, http_get=FakeHttp("v0.1.8")
+        ))
+
+        assert result["ok"] is True, result
+        assert (tmp_home / ".emrg" / "skills" / "browser-harness.md").is_file()
+
+    def test_a_directory_target_is_refused_by_kind_on_every_platform(self, tmp_home, with_cli):
+        """**FIFO-free, and it discriminates** — the arm the Windows leg was missing.
+
+        The FIFO arm above is gated on `os.mkfifo`, so it skips wherever there are none
+        (Windows), and the other arms here pass with *and* without the guard: measured
+        2026-10-11 on this file with `emrg/skills/installer.py` reverted to `3f18f6a6`,
+        the FIFO-free half is green either way — so the suite's earlier claim that "the
+        Windows leg runs the discriminating half" was false, and the fix is to give that
+        leg an arm that really does discriminate rather than to weaken the sentence.
+
+        A **directory** is the arm that works on every platform, and it is the one
+        `tests/test_config.py` uses for the same reason (issue #2103 / PR #2104): both
+        versions refuse the write, and only one of them refuses it **by name**. The
+        assertion is therefore on the name, not on the failure:
+
+        ==================  =========================================================
+        base `3f18f6a6`     ``cannot write skill file: [Errno 21] Is a directory: …``
+        this tree           ``cannot write skill file: … is a directory, not a regular file``
+        ==================  =========================================================
+
+        The `IsADirectoryError`/`PermissionError` text differs by platform, which is
+        exactly why pinning it would not have survived Windows; ``not a regular file`` is
+        this family's own wording and is what the base cannot produce.
+        """
+        ensure_catalog_file()
+        (tmp_home / ".emrg" / "skills" / "browser-harness.md").mkdir()
+        runner = FakeRunner(skill_output=VALID_SKILL_MD)
+
+        result = _run(installer.install_skill(
+            "browser-harness", confirmed=True, runner=runner, http_get=FakeHttp("v0.1.8")
+        ))
+
+        assert "error" in result, result
+        assert "cannot write skill file" in result["error"], result["error"]
+        assert "not a regular file" in result["error"], result["error"]