fix(monitors): a failed ping or TCP check says why (#5430)

tcp_check swallowed every exception and icmp_check returned on a
non-zero exit without a reason, so every down ICMP/TCP monitor stored
error_msg NULL and the Monitors page and MCP showed it down with no why.

Each failure now names its cause: "No reply within 2 s", "Connection
refused on port N", "No answer on port N within 5 s", "Can't resolve
<name>: ...", the OS error, or ping's own first stderr line. An ICMP
check that fell back to TCP says so, and a ping that outlives its
timeout is killed rather than left running.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

steward/monitors/ping.py

@@ -1,17 +1,38 @@
-"""ICMP / TCP reachability checks — pure probes returning a result dict."""
+"""ICMP / TCP reachability checks — pure probes returning a result dict.
+
+A failed check says why in `error_msg` (#5430): a down monitor that can't
+say whether nothing answered or something refused leaves the operator to
+guess.
+"""
 from __future__ import annotations
 import asyncio
 import logging
+import socket
 import time
 
 logger = logging.getLogger(__name__)
 
 TCP_TIMEOUT = 5.0
 DEFAULT_TCP_PORT = 80
+PING_WAIT_S = 2
+PING_TIMEOUT = 5.0
+
+
+def tcp_failure(exc: BaseException, address: str, port: int) -> str:
+    """Why a TCP connect failed, in a phrase."""
+    if isinstance(exc, (asyncio.TimeoutError, TimeoutError)):
+        return f"No answer on port {port} within {TCP_TIMEOUT:g} s"
+    if isinstance(exc, ConnectionRefusedError):
+        return f"Connection refused on port {port}"
+    if isinstance(exc, socket.gaierror):
+        return f"Can't resolve {address}: {exc.strerror or exc}"
+    if isinstance(exc, OSError):
+        return f"Can't connect to port {port}: {exc.strerror or exc}"
+    return str(exc) or type(exc).__name__
 
 
 async def tcp_check(address: str, port: int) -> dict:
-    """TCP connect probe. Returns {is_up, response_ms}."""
+    """TCP connect probe. Returns {is_up, response_ms, error_msg}."""
     start = time.monotonic()
     try:
         reader, writer = await asyncio.wait_for(
@@ -20,28 +41,53 @@ async def tcp_check(address: str, port: int) -> dict:
         )
         writer.close()
         await writer.wait_closed()
-        return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2)}
-    except Exception:
-        return {"is_up": False, "response_ms": None}
+        return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2),
+                "error_msg": None}
+    except Exception as exc:
+        return {"is_up": False, "response_ms": None, "error_msg": tcp_failure(exc, address, port)}
+
+
+def ping_failure(returncode: int, stderr: str) -> str:
+    """Why `ping -c 1` failed: exit 1 is no reply; anything else is ping's
+    own complaint (an unknown name, no route), first line of it."""
+    if returncode == 1:
+        return f"No reply within {PING_WAIT_S} s"
+    said = stderr.strip().splitlines()[0] if stderr.strip() else ""
+    return said or f"ping failed (exit {returncode})"
 
 
 async def icmp_check(address: str) -> dict:
     """ICMP echo via the system ping binary (setuid; no raw-socket privilege).
 
     Falls back to a TCP connect on the default port if the ping binary is
-    unavailable or errors, so a host with ICMP filtered still gets a signal.
+    unavailable or errors, so a host with ICMP filtered still gets a signal;
+    a failure then says the TCP check stood in.
     """
     start = time.monotonic()
     try:
         proc = await asyncio.create_subprocess_exec(
-            "ping", "-c", "1", "-W", "2", address,
+            "ping", "-c", "1", "-W", str(PING_WAIT_S), address,
             stdout=asyncio.subprocess.DEVNULL,
-            stderr=asyncio.subprocess.DEVNULL,
+            stderr=asyncio.subprocess.PIPE,
         )
-        await asyncio.wait_for(proc.wait(), timeout=5.0)
-        if proc.returncode == 0:
-            return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2)}
-        return {"is_up": False, "response_ms": None}
     except Exception as exc:
         logger.warning("ICMP ping failed for %s: %s, falling back to TCP", address, exc)
-        return await tcp_check(address, DEFAULT_TCP_PORT)
+        result = await tcp_check(address, DEFAULT_TCP_PORT)
+        if not result["is_up"]:
+            result["error_msg"] = f"ping unavailable, TCP check instead: {result['error_msg']}"
+        return result
+    try:
+        _, stderr = await asyncio.wait_for(proc.communicate(), timeout=PING_TIMEOUT)
+    except asyncio.TimeoutError:
+        proc.kill()
+        try:
+            await asyncio.wait_for(proc.wait(), timeout=PING_TIMEOUT)
+        except asyncio.TimeoutError:
+            logger.warning("ping for %s did not exit after kill", address)
+        return {"is_up": False, "response_ms": None,
+                "error_msg": f"No reply within {PING_TIMEOUT:g} s"}
+    if proc.returncode == 0:
+        return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2),
+                "error_msg": None}
+    return {"is_up": False, "response_ms": None,
+            "error_msg": ping_failure(proc.returncode, (stderr or b"").decode(errors="replace"))}

tests/core/test_ping_checks.py

@@ -0,0 +1,65 @@
+"""A failed ping or TCP check says why (#5430): no reply, refused, no answer
+in time, a name that doesn't resolve, or ping's own complaint; an ICMP check
+that fell back to TCP says so."""
+import asyncio
+import socket
+
+import pytest
+
+from steward.monitors import ping
+
+
+@pytest.mark.parametrize("exc,want", [
+    (asyncio.TimeoutError(), "No answer on port 22 within 5 s"),
+    (ConnectionRefusedError(111, "Connection refused"), "Connection refused on port 22"),
+    (socket.gaierror(-2, "Name or service not known"), "Can't resolve nas.lan: Name or service not known"),
+    (OSError(113, "No route to host"), "Can't connect to port 22: No route to host"),
+    (RuntimeError("odd"), "odd"),
+])
+def test_a_tcp_failure_says_why(monkeypatch, exc, want):
+    async def boom(*a, **kw):
+        raise exc
+    monkeypatch.setattr(ping.asyncio, "open_connection", boom)
+    got = asyncio.run(ping.tcp_check("nas.lan", 22))
+    assert got == {"is_up": False, "response_ms": None, "error_msg": want}
+
+
+class _Proc:
+    def __init__(self, returncode, stderr=b""):
+        self.returncode, self._stderr = returncode, stderr
+
+    async def communicate(self):
+        return b"", self._stderr
+
+
+@pytest.mark.parametrize("code,stderr,want", [
+    (1, b"", "No reply within 2 s"),
+    (2, b"ping: nas.lan: Name or service not known\n", "ping: nas.lan: Name or service not known"),
+    (2, b"", "ping failed (exit 2)"),
+])
+def test_a_ping_failure_says_why(monkeypatch, code, stderr, want):
+    async def spawn(*a, **kw):
+        return _Proc(code, stderr)
+    monkeypatch.setattr(ping.asyncio, "create_subprocess_exec", spawn)
+    got = asyncio.run(ping.icmp_check("nas.lan"))
+    assert got == {"is_up": False, "response_ms": None, "error_msg": want}
+
+
+def test_a_reply_is_up_with_no_error(monkeypatch):
+    async def spawn(*a, **kw):
+        return _Proc(0)
+    monkeypatch.setattr(ping.asyncio, "create_subprocess_exec", spawn)
+    got = asyncio.run(ping.icmp_check("nas.lan"))
+    assert got["is_up"] is True and got["error_msg"] is None
+
+
+def test_without_ping_the_tcp_stand_in_says_so(monkeypatch):
+    async def no_ping(*a, **kw):
+        raise FileNotFoundError("ping")
+
+    async def refused(*a, **kw):
+        raise ConnectionRefusedError(111, "Connection refused")
+    monkeypatch.setattr(ping.asyncio, "create_subprocess_exec", no_ping)
+    monkeypatch.setattr(ping.asyncio, "open_connection", refused)
+    got = asyncio.run(ping.icmp_check("nas.lan"))
+    assert got["error_msg"] == "ping unavailable, TCP check instead: Connection refused on port 80"