feat(reliability): fd-leak rails — self-watchdog, poll-overlap guard, fd tests
Guardrails so the fd-leak class of bug (Errno 24 lockup; recent SNMP + UniFi fixes) surfaces early or can't compound, instead of silently killing the app. - Self-fd watchdog (steward/core/self_monitor.py): records open_fds and open_fds_pct (% of soft RLIMIT_NOFILE) as "steward"/"process" metrics each minute through the normal alert pipeline, so the operator can alert on them via the existing alert-rules UI. Built-in WARNING floor at 80% gives a zero-config early signal. Stdlib-only (/proc + resource); degrades to a no-op off Linux. Registered as a core ScheduledTask in app.py. - Poll-overlap guard (steward/core/scheduler.py): extract a pure _DueTracker that skips a tick while a task's prior run is still in flight, so a hung poll can't stack overlapping runs (which amplify per-poll resource/fd use). A skipped task isn't penalised — it retries the next tick after it completes. - fd-stability tests (tests/core/): _DueTracker overlap policy, the watchdog metric/warning/degradation paths, and a real-fd canary that hammers tcp_check and asserts /proc/self/fd doesn't grow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,46 @@
|
||||
"""Real-fd regression canary.
|
||||
|
||||
Drives a real socket code path (the TCP reachability probe) in a tight loop and
|
||||
asserts the process's open-fd count doesn't grow. If someone reintroduces a
|
||||
socket leak in the probe path — the class of bug behind the Errno 24 lockups —
|
||||
this fails in CI instead of in production. Uses real descriptors, not fakes, so
|
||||
the assertion has teeth.
|
||||
"""
|
||||
import asyncio
|
||||
import os
|
||||
|
||||
import pytest
|
||||
|
||||
from steward.monitors.ping import tcp_check
|
||||
|
||||
_ITERATIONS = 100
|
||||
|
||||
|
||||
def _fd_count() -> int | None:
|
||||
try:
|
||||
return len(os.listdir("/proc/self/fd"))
|
||||
except OSError:
|
||||
return None
|
||||
|
||||
|
||||
async def _hammer_tcp_check() -> None:
|
||||
# 127.0.0.1:1 has no listener → connection refused immediately. Each call must
|
||||
# fully release its socket; a leak would add ~one fd per iteration.
|
||||
for _ in range(_ITERATIONS):
|
||||
await tcp_check("127.0.0.1", 1)
|
||||
|
||||
|
||||
def test_tcp_check_does_not_leak_fds():
|
||||
if _fd_count() is None:
|
||||
pytest.skip("/proc/self/fd unavailable on this platform")
|
||||
|
||||
asyncio.run(_hammer_tcp_check()) # warm up (lazy imports, caches)
|
||||
before = _fd_count()
|
||||
asyncio.run(_hammer_tcp_check())
|
||||
after = _fd_count()
|
||||
|
||||
# Small slack for interpreter-internal fds; a genuine leak over 100 iterations
|
||||
# would be far larger than this.
|
||||
assert after - before <= 5, (
|
||||
f"open fds grew {before}->{after} over {_ITERATIONS} tcp_check calls "
|
||||
"— possible socket leak")
|
||||
@@ -0,0 +1,58 @@
|
||||
"""Unit tests for the scheduler's poll-overlap guard (_DueTracker).
|
||||
|
||||
The tracker is pure (clock passed in) so we can assert the overlap policy
|
||||
deterministically: a task whose prior run is still in flight is never re-fired,
|
||||
and a skipped task isn't penalised — it runs on the next tick once it finishes.
|
||||
This is the rail that stops a hung poll from stacking overlapping runs (which
|
||||
would amplify any per-poll resource/fd use).
|
||||
"""
|
||||
from steward.core.scheduler import ScheduledTask, _DueTracker
|
||||
|
||||
|
||||
def _task(name: str, interval: int) -> ScheduledTask:
|
||||
return ScheduledTask(name=name, coro_factory=lambda: None, interval_seconds=interval)
|
||||
|
||||
|
||||
def test_due_only_after_interval_elapses():
|
||||
t = _task("a", 60)
|
||||
tr = _DueTracker()
|
||||
assert tr.due([t], now=59) == [] # 59 < 60
|
||||
assert tr.due([t], now=60) == [t] # interval elapsed
|
||||
|
||||
|
||||
def test_in_flight_task_is_skipped_even_when_due():
|
||||
t = _task("a", 0) # due every tick
|
||||
tr = _DueTracker()
|
||||
tr.mark_started(t, now=0)
|
||||
assert tr.due([t], now=100) == [] # still running → skipped
|
||||
tr.mark_done("a")
|
||||
assert tr.due([t], now=100) == [t] # finished → eligible again
|
||||
|
||||
|
||||
def test_skip_does_not_advance_last_run_so_it_retries():
|
||||
t = _task("a", 10)
|
||||
tr = _DueTracker()
|
||||
tr.mark_started(t, now=0)
|
||||
assert tr.due([t], now=100) == [] # due but in flight
|
||||
assert tr.last_run["a"] == 0 # not advanced by the skip
|
||||
tr.mark_done("a")
|
||||
assert tr.due([t], now=100) == [t] # retried promptly after completion
|
||||
|
||||
|
||||
def test_mark_started_advances_last_run_and_marks_in_flight():
|
||||
t = _task("a", 10)
|
||||
tr = _DueTracker()
|
||||
tr.mark_started(t, now=50)
|
||||
assert tr.last_run["a"] == 50
|
||||
assert tr.in_flight == {"a"}
|
||||
assert tr.due([t], now=55) == [] # 5 < 10 (not yet due)
|
||||
assert tr.due([t], now=61) == [] # due by interval, but still in flight
|
||||
tr.mark_done("a")
|
||||
assert tr.due([t], now=61) == [t]
|
||||
|
||||
|
||||
def test_independent_tasks_do_not_block_each_other():
|
||||
a, b = _task("a", 0), _task("b", 0)
|
||||
tr = _DueTracker()
|
||||
tr.mark_started(a, now=0) # a hangs
|
||||
assert tr.due([a, b], now=10) == [b] # b still fires
|
||||
@@ -0,0 +1,115 @@
|
||||
"""Unit tests for the self-fd watchdog (no DB, no network).
|
||||
|
||||
Exercises the fd accounting, the percentage/warning floor, and that the
|
||||
recorder degrades to a no-op where fd accounting isn't available. The DB session
|
||||
and the alert pipeline are faked — we only assert that the right metrics get
|
||||
handed to record_metric.
|
||||
"""
|
||||
import asyncio
|
||||
import logging
|
||||
|
||||
import steward.core.alerts as alerts
|
||||
import steward.core.self_monitor as sm
|
||||
|
||||
|
||||
# ── fd accounting ────────────────────────────────────────────────────────────
|
||||
|
||||
def test_count_open_fds_sane_or_none():
|
||||
n = sm.count_open_fds()
|
||||
# On the Linux CI image /proc exists; stdin/stdout/stderr are always open.
|
||||
assert n is None or n >= 3
|
||||
|
||||
|
||||
def test_fd_soft_limit_positive_or_none():
|
||||
soft = sm.fd_soft_limit()
|
||||
assert soft is None or soft > 0
|
||||
|
||||
|
||||
# ── fake session / app plumbing ──────────────────────────────────────────────
|
||||
|
||||
class _Ctx:
|
||||
async def __aenter__(self):
|
||||
return self
|
||||
|
||||
async def __aexit__(self, *_):
|
||||
return False
|
||||
|
||||
|
||||
class _FakeSession(_Ctx):
|
||||
def begin(self):
|
||||
return _Ctx()
|
||||
|
||||
|
||||
class _FakeApp:
|
||||
def db_sessionmaker(self):
|
||||
return _FakeSession()
|
||||
|
||||
|
||||
def _patch_recorder(monkeypatch):
|
||||
recorded: list[tuple] = []
|
||||
|
||||
async def fake_record_metric(session, source_module, resource_name, metric_name, value):
|
||||
recorded.append((source_module, resource_name, metric_name, value))
|
||||
|
||||
monkeypatch.setattr(alerts, "record_metric", fake_record_metric)
|
||||
return recorded
|
||||
|
||||
|
||||
# ── record_self_metrics ──────────────────────────────────────────────────────
|
||||
|
||||
def test_records_both_fd_metrics(monkeypatch):
|
||||
recorded = _patch_recorder(monkeypatch)
|
||||
monkeypatch.setattr(sm, "count_open_fds", lambda: 50)
|
||||
monkeypatch.setattr(sm, "fd_soft_limit", lambda: 1000)
|
||||
|
||||
asyncio.run(sm.record_self_metrics(_FakeApp()))
|
||||
|
||||
assert ("steward", "process", "open_fds", 50.0) in recorded
|
||||
assert ("steward", "process", "open_fds_pct", 5.0) in recorded
|
||||
|
||||
|
||||
def test_no_pct_metric_when_limit_unknown(monkeypatch):
|
||||
recorded = _patch_recorder(monkeypatch)
|
||||
monkeypatch.setattr(sm, "count_open_fds", lambda: 50)
|
||||
monkeypatch.setattr(sm, "fd_soft_limit", lambda: None)
|
||||
|
||||
asyncio.run(sm.record_self_metrics(_FakeApp()))
|
||||
|
||||
metric_names = [m for (_s, _r, m, _v) in recorded]
|
||||
assert "open_fds" in metric_names
|
||||
assert "open_fds_pct" not in metric_names # meaningless without a ceiling
|
||||
|
||||
|
||||
def test_warns_past_floor(monkeypatch, caplog):
|
||||
_patch_recorder(monkeypatch)
|
||||
monkeypatch.setattr(sm, "count_open_fds", lambda: 900)
|
||||
monkeypatch.setattr(sm, "fd_soft_limit", lambda: 1000) # 90% ≥ FD_WARN_PCT
|
||||
monkeypatch.setattr(sm, "_warned", False)
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger=sm.logger.name):
|
||||
asyncio.run(sm.record_self_metrics(_FakeApp()))
|
||||
|
||||
assert any("soft limit" in r.message for r in caplog.records)
|
||||
|
||||
|
||||
def test_no_warning_below_floor(monkeypatch, caplog):
|
||||
_patch_recorder(monkeypatch)
|
||||
monkeypatch.setattr(sm, "count_open_fds", lambda: 100)
|
||||
monkeypatch.setattr(sm, "fd_soft_limit", lambda: 1000) # 10%
|
||||
monkeypatch.setattr(sm, "_warned", False)
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger=sm.logger.name):
|
||||
asyncio.run(sm.record_self_metrics(_FakeApp()))
|
||||
|
||||
assert not any("soft limit" in r.message for r in caplog.records)
|
||||
|
||||
|
||||
def test_noop_when_fd_count_unavailable(monkeypatch):
|
||||
# /proc absent → must return before touching the DB.
|
||||
monkeypatch.setattr(sm, "count_open_fds", lambda: None)
|
||||
|
||||
class _Boom:
|
||||
def db_sessionmaker(self):
|
||||
raise AssertionError("must not open a session when fds are unknown")
|
||||
|
||||
asyncio.run(sm.record_self_metrics(_Boom())) # no raise
|
||||
Reference in New Issue
Block a user