diff --git a/services/ha-diag-agent/src/ha_diag/config.py b/services/ha-diag-agent/src/ha_diag/config.py index 6eb8be8..32d1491 100644 --- a/services/ha-diag-agent/src/ha_diag/config.py +++ b/services/ha-diag-agent/src/ha_diag/config.py @@ -4,11 +4,18 @@ import os from pathlib import Path import yaml -from pydantic import field_validator +from pydantic import Field, field_validator from pydantic_settings import BaseSettings _CONFIG_YAML = Path("/config/ha-diag-agent.yaml") +# node_name values that are unsafe to run with. A node_name of "unknown" (or +# empty) silently produces evt-unknown-* event files. Such a file once landed in +# events/piha/ and, being lexically greater than every evt-piha-* name, pinned +# the observer's (then lexical) checkpoint and blocked PIHA for ~34 days — see +# fix d5139c9. We refuse to start rather than ever emit that poison again. +_FORBIDDEN_NODE_NAMES = {"", "unknown"} + class Settings(BaseSettings): # HA connection @@ -16,8 +23,11 @@ class Settings(BaseSettings): ha_token: str = "" ha_timeout: float = 10.0 - # Node identity - node_name: str = "unknown" + # Node identity. + # validate_default=True so the "unknown" default itself is rejected: if + # NODE_NAME never reaches the process the agent must fail fast, not run as + # "unknown" and poison another node's event directory (see d5139c9). + node_name: str = Field(default="unknown", validate_default=True) location_tag: str = "default" # Intervals (seconds) @@ -69,6 +79,26 @@ class Settings(BaseSettings): def strip_trailing_slash(cls, v: str) -> str: return v.rstrip("/") + @field_validator("node_name") + @classmethod + def reject_unknown_node(cls, v: str) -> str: + """Fail-fast: never let the agent run as "unknown"/empty. + + A node_name of "unknown" makes EventEmitter write evt-unknown-* files. + Landing in another node's events dir, such a file poisoned the observer + checkpoint and blocked PIHA for ~34 days (d5139c9). Refusing to start is + strictly safer than silently emitting the poison. + """ + normalized = (v or "").strip() + if normalized.lower() in _FORBIDDEN_NODE_NAMES: + raise ValueError( + "NODE_NAME not set — refusing to start, would poison event " + "pipeline with evt-unknown-* files (see d5139c9). " + "Set NODE_NAME to the node's canonical inventory name " + "(e.g. piha, chelsty-infra)." + ) + return normalized + @classmethod def load(cls) -> "Settings": """Load settings: YAML file provides defaults; env vars override.""" diff --git a/services/ha-diag-agent/src/ha_diag/event_emitter.py b/services/ha-diag-agent/src/ha_diag/event_emitter.py index 3254d7a..e59ec19 100644 --- a/services/ha-diag-agent/src/ha_diag/event_emitter.py +++ b/services/ha-diag-agent/src/ha_diag/event_emitter.py @@ -16,8 +16,19 @@ class EventEmitter: def __init__( self, events_dir: Path, node_name: str, location_tag: str = "" ) -> None: + # Defense in depth (config.py already fails fast on this): the event id + # embeds node_name into the filename, so "unknown"/empty here is exactly + # what produces evt-unknown-* poison files. Refuse at the emit boundary + # too, so no future call path can quietly reintroduce it (see d5139c9). + normalized = (node_name or "").strip() + if normalized.lower() in ("", "unknown"): + raise ValueError( + f"EventEmitter refuses node_name={node_name!r} — would write " + "evt-unknown-* files that poison the observer checkpoint " + "(see d5139c9). node_name must be a real node identity." + ) self._events_dir = events_dir - self._node_name = node_name + self._node_name = normalized self._location_tag = location_tag self._seq = 0 events_dir.mkdir(parents=True, exist_ok=True) diff --git a/services/ha-diag-agent/src/ha_diag/main.py b/services/ha-diag-agent/src/ha_diag/main.py index 23b9d53..31540b9 100644 --- a/services/ha-diag-agent/src/ha_diag/main.py +++ b/services/ha-diag-agent/src/ha_diag/main.py @@ -3,12 +3,14 @@ from __future__ import annotations import asyncio import json import logging +import sys import time from datetime import datetime import structlog import uvicorn from apscheduler.schedulers.asyncio import AsyncIOScheduler +from pydantic import ValidationError from .api import app, register_checks, register_ws_monitor from .checks.automation_failures import AutomationFailuresCheck @@ -196,7 +198,18 @@ async def run(settings: Settings) -> None: def main() -> None: - settings = Settings.load() + try: + settings = Settings.load() + except ValidationError as exc: + # Fail fast and loud rather than run mis-identified. The most important + # case is node_name="unknown"/empty, which would emit evt-unknown-* + # poison files into another node's events dir (see d5139c9). + print( + "FATAL: refusing to start ha-diag-agent — invalid configuration:\n" + f"{exc}", + file=sys.stderr, + ) + raise SystemExit(1) asyncio.run(run(settings)) diff --git a/services/ha-diag-agent/tests/test_config.py b/services/ha-diag-agent/tests/test_config.py new file mode 100644 index 0000000..939d4c6 --- /dev/null +++ b/services/ha-diag-agent/tests/test_config.py @@ -0,0 +1,141 @@ +"""Tests for Settings / node_name fail-fast. + +Regression coverage for the evt-unknown-* poison bug: a node_name of "unknown" +(or empty) produced event files that pinned the observer checkpoint and blocked +PIHA for ~34 days (see fix d5139c9). The agent must now refuse to start rather +than ever emit such a file. +""" +from __future__ import annotations + +import os +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from ha_diag import config +from ha_diag.config import Settings + + +@pytest.fixture(autouse=True) +def _isolate_environ(): + """Snapshot os.environ — Settings.load() mutates it via setdefault().""" + saved = dict(os.environ) + yield + os.environ.clear() + os.environ.update(saved) + + +# --------------------------------------------------------------------------- +# node_name loaded from env NODE_NAME +# --------------------------------------------------------------------------- + + +def test_node_name_from_env(monkeypatch): + monkeypatch.setenv("NODE_NAME", "piha") + settings = Settings() + assert settings.node_name == "piha" + + +def test_node_name_from_env_case_insensitive_var(monkeypatch): + # BaseSettings is case-insensitive: lowercase env var maps to the field too. + monkeypatch.delenv("NODE_NAME", raising=False) + monkeypatch.setenv("node_name", "chelsty-infra") + settings = Settings() + assert settings.node_name == "chelsty-infra" + + +def test_node_name_whitespace_is_stripped(monkeypatch): + monkeypatch.setenv("NODE_NAME", " piha ") + settings = Settings() + assert settings.node_name == "piha" + + +# --------------------------------------------------------------------------- +# fail-fast: unknown / empty node_name refuses to start +# --------------------------------------------------------------------------- + + +def test_explicit_unknown_node_name_refuses(monkeypatch): + monkeypatch.delenv("NODE_NAME", raising=False) + with pytest.raises(ValidationError) as exc: + Settings(node_name="unknown") + assert "refusing to start" in str(exc.value) + + +def test_unknown_case_insensitive_refuses(monkeypatch): + monkeypatch.delenv("NODE_NAME", raising=False) + with pytest.raises(ValidationError): + Settings(node_name="UNKNOWN") + + +def test_empty_node_name_refuses(monkeypatch): + monkeypatch.delenv("NODE_NAME", raising=False) + with pytest.raises(ValidationError): + Settings(node_name="") + + +def test_whitespace_only_node_name_refuses(monkeypatch): + monkeypatch.delenv("NODE_NAME", raising=False) + with pytest.raises(ValidationError): + Settings(node_name=" ") + + +def test_env_unknown_refuses(monkeypatch): + monkeypatch.setenv("NODE_NAME", "unknown") + with pytest.raises(ValidationError): + Settings() + + +def test_default_without_env_refuses(monkeypatch): + # No NODE_NAME anywhere → the "unknown" default must be rejected, not used. + monkeypatch.delenv("NODE_NAME", raising=False) + monkeypatch.delenv("node_name", raising=False) + with pytest.raises(ValidationError): + Settings() + + +# --------------------------------------------------------------------------- +# precedence: env wins over YAML for node_name +# --------------------------------------------------------------------------- + + +def test_env_wins_over_yaml(monkeypatch, tmp_path: Path): + yaml_file = tmp_path / "ha-diag-agent.yaml" + yaml_file.write_text("node_name: solaria\n") + monkeypatch.setattr(config, "_CONFIG_YAML", yaml_file) + monkeypatch.setenv("NODE_NAME", "piha") + + settings = Settings.load() + # env value must win; YAML only supplies a default when env is absent. + assert settings.node_name == "piha" + + +def test_yaml_supplies_node_name_when_env_absent(monkeypatch, tmp_path: Path): + yaml_file = tmp_path / "ha-diag-agent.yaml" + yaml_file.write_text("node_name: chelsty-infra\n") + monkeypatch.setattr(config, "_CONFIG_YAML", yaml_file) + monkeypatch.delenv("NODE_NAME", raising=False) + monkeypatch.delenv("node_name", raising=False) + + settings = Settings.load() + assert settings.node_name == "chelsty-infra" + + +def test_yaml_unknown_still_refuses(monkeypatch, tmp_path: Path): + # Even if YAML carries the poison value, fail-fast must still fire. + yaml_file = tmp_path / "ha-diag-agent.yaml" + yaml_file.write_text("node_name: unknown\n") + monkeypatch.setattr(config, "_CONFIG_YAML", yaml_file) + monkeypatch.delenv("NODE_NAME", raising=False) + monkeypatch.delenv("node_name", raising=False) + + with pytest.raises(ValidationError): + Settings.load() + + +def test_load_valid_node_name(monkeypatch, tmp_path: Path): + monkeypatch.setattr(config, "_CONFIG_YAML", tmp_path / "does-not-exist.yaml") + monkeypatch.setenv("NODE_NAME", "piha") + settings = Settings.load() + assert settings.node_name == "piha" diff --git a/services/ha-diag-agent/tests/test_event_emitter.py b/services/ha-diag-agent/tests/test_event_emitter.py index e70e02b..c86981f 100644 --- a/services/ha-diag-agent/tests/test_event_emitter.py +++ b/services/ha-diag-agent/tests/test_event_emitter.py @@ -76,6 +76,21 @@ def test_location_tag_empty_not_in_payload(tmp_events_dir: Path): assert "location_tag" not in data["payload"] +@pytest.mark.parametrize("bad_node", ["unknown", "UNKNOWN", "", " "]) +def test_emitter_refuses_unknown_or_empty_node(tmp_events_dir: Path, bad_node: str): + # No emit path may ever produce evt-unknown-* poison files (see d5139c9). + with pytest.raises(ValueError): + EventEmitter(tmp_events_dir, node_name=bad_node) + + +def test_emitter_never_writes_unknown_node_in_filename(tmp_events_dir: Path): + emitter = EventEmitter(tmp_events_dir, node_name="piha") + event_id = emitter.emit("ha_websocket_dead", "error", "homeassistant", "msg") + assert "unknown" not in event_id + data = json.loads((tmp_events_dir / f"{event_id}.json").read_text()) + assert data["node"] == "piha" + + def test_location_tag_does_not_override_explicit_payload_key(tmp_events_dir: Path): emitter = EventEmitter(tmp_events_dir, node_name="piha", location_tag="ken") event_id = emitter.emit(