fix(unifi): close the cached client before discarding it (fd leak)
The UniFi poller caches a UnifiClient holding a long-lived httpx.AsyncClient (a real socket pool). On a login failure or a mid-poll error it set `_client = None` to force re-auth, but never called close() — orphaning the connection pool until GC. Under a flapping/unreachable controller that leaks a file descriptor every failure tick: the same class of bug as the SNMP engine leak (OSError: [Errno 24] Too many open files). Add _drop_client() that aclose()s the client (best-effort) before nulling the cache, and route both discard paths through it. Regression tests cover both failure paths plus the noop/close-error contracts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -15,6 +15,25 @@ logger = logging.getLogger(__name__)
|
|||||||
_client = None # UnifiClient instance, initialised on first scrape
|
_client = None # UnifiClient instance, initialised on first scrape
|
||||||
|
|
||||||
|
|
||||||
|
async def _drop_client() -> None:
|
||||||
|
"""Close and discard the cached client.
|
||||||
|
|
||||||
|
The client holds a long-lived ``httpx.AsyncClient`` (a connection pool over
|
||||||
|
real sockets). Nulling the reference without ``aclose()`` orphans that pool
|
||||||
|
until GC, so every failure tick that re-auths would leak file descriptors —
|
||||||
|
the same class of bug that took the app down via SNMP (Errno 24, "Too many
|
||||||
|
open files"). Close first, then drop. Best-effort: a failed close must not
|
||||||
|
break the poll loop.
|
||||||
|
"""
|
||||||
|
global _client
|
||||||
|
if _client is not None:
|
||||||
|
try:
|
||||||
|
await _client.close()
|
||||||
|
except Exception:
|
||||||
|
pass
|
||||||
|
_client = None
|
||||||
|
|
||||||
|
|
||||||
def make_poll_task(app: "Quart") -> ScheduledTask:
|
def make_poll_task(app: "Quart") -> ScheduledTask:
|
||||||
cfg = app.config["PLUGINS"]["unifi"]
|
cfg = app.config["PLUGINS"]["unifi"]
|
||||||
interval = int(cfg.get("poll_interval_seconds", 60))
|
interval = int(cfg.get("poll_interval_seconds", 60))
|
||||||
@@ -51,7 +70,7 @@ async def _do_poll(app: "Quart") -> None:
|
|||||||
await _client.login()
|
await _client.login()
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
logger.warning("UniFi initial login failed: %s", exc)
|
logger.warning("UniFi initial login failed: %s", exc)
|
||||||
_client = None
|
await _drop_client()
|
||||||
return
|
return
|
||||||
|
|
||||||
try:
|
try:
|
||||||
@@ -60,7 +79,7 @@ async def _do_poll(app: "Quart") -> None:
|
|||||||
devices = await _client.get_devices()
|
devices = await _client.get_devices()
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
logger.warning("UniFi poll failed — will retry next tick: %s", exc)
|
logger.warning("UniFi poll failed — will retry next tick: %s", exc)
|
||||||
_client = None # force re-auth on next tick
|
await _drop_client() # close + force re-auth on next tick
|
||||||
return
|
return
|
||||||
|
|
||||||
scraped_at = datetime.now(timezone.utc)
|
scraped_at = datetime.now(timezone.utc)
|
||||||
|
|||||||
@@ -0,0 +1,92 @@
|
|||||||
|
"""Unit tests for the UniFi poll scheduler's client lifecycle (no network/DB).
|
||||||
|
|
||||||
|
The leak guard: the module-cached UnifiClient holds a long-lived httpx pool, so
|
||||||
|
whenever the scheduler discards it (login failure, or a mid-poll error that
|
||||||
|
forces re-auth) it MUST close it first — otherwise every failure tick orphans a
|
||||||
|
connection pool and leaks file descriptors. These tests drive both discard paths
|
||||||
|
and assert the client was closed and the cache cleared. Uses asyncio.run()
|
||||||
|
directly so it doesn't depend on the pytest-asyncio mode.
|
||||||
|
"""
|
||||||
|
import asyncio
|
||||||
|
import types
|
||||||
|
|
||||||
|
import plugins.unifi.client as client_mod
|
||||||
|
import plugins.unifi.scheduler as scheduler
|
||||||
|
|
||||||
|
|
||||||
|
def _make_app():
|
||||||
|
app = types.SimpleNamespace()
|
||||||
|
app.config = {"PLUGINS": {"unifi": {"host": "h", "username": "u", "password": "p"}}}
|
||||||
|
return app
|
||||||
|
|
||||||
|
|
||||||
|
class _FakeClient:
|
||||||
|
"""Stand-in for UnifiClient; records close() and can fail on demand."""
|
||||||
|
created: list["_FakeClient"] = []
|
||||||
|
login_fails = False
|
||||||
|
health_fails = False
|
||||||
|
|
||||||
|
def __init__(self, **_kwargs):
|
||||||
|
self.closed = False
|
||||||
|
_FakeClient.created.append(self)
|
||||||
|
|
||||||
|
async def login(self):
|
||||||
|
if _FakeClient.login_fails:
|
||||||
|
raise RuntimeError("login boom")
|
||||||
|
|
||||||
|
async def get_health(self):
|
||||||
|
if _FakeClient.health_fails:
|
||||||
|
raise RuntimeError("health boom")
|
||||||
|
return []
|
||||||
|
|
||||||
|
async def close(self):
|
||||||
|
self.closed = True
|
||||||
|
|
||||||
|
|
||||||
|
def _install(monkeypatch):
|
||||||
|
monkeypatch.setattr(client_mod, "UnifiClient", _FakeClient)
|
||||||
|
monkeypatch.setattr(scheduler, "_client", None)
|
||||||
|
_FakeClient.created = []
|
||||||
|
_FakeClient.login_fails = False
|
||||||
|
_FakeClient.health_fails = False
|
||||||
|
|
||||||
|
|
||||||
|
def test_login_failure_closes_and_clears_client(monkeypatch):
|
||||||
|
_install(monkeypatch)
|
||||||
|
_FakeClient.login_fails = True
|
||||||
|
|
||||||
|
asyncio.run(scheduler._do_poll(_make_app()))
|
||||||
|
|
||||||
|
assert len(_FakeClient.created) == 1
|
||||||
|
assert _FakeClient.created[0].closed is True # closed, not just dropped
|
||||||
|
assert scheduler._client is None # cache cleared → re-auth next tick
|
||||||
|
|
||||||
|
|
||||||
|
def test_poll_failure_closes_and_clears_client(monkeypatch):
|
||||||
|
_install(monkeypatch)
|
||||||
|
_FakeClient.health_fails = True # login OK, first API call blows up
|
||||||
|
|
||||||
|
asyncio.run(scheduler._do_poll(_make_app()))
|
||||||
|
|
||||||
|
assert len(_FakeClient.created) == 1
|
||||||
|
assert _FakeClient.created[0].closed is True
|
||||||
|
assert scheduler._client is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_drop_client_is_noop_when_unset(monkeypatch):
|
||||||
|
_install(monkeypatch)
|
||||||
|
# No client cached — dropping must not raise.
|
||||||
|
asyncio.run(scheduler._drop_client())
|
||||||
|
assert scheduler._client is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_drop_client_swallows_close_errors(monkeypatch):
|
||||||
|
_install(monkeypatch)
|
||||||
|
|
||||||
|
class _Boom:
|
||||||
|
async def close(self):
|
||||||
|
raise OSError("close boom")
|
||||||
|
|
||||||
|
monkeypatch.setattr(scheduler, "_client", _Boom())
|
||||||
|
asyncio.run(scheduler._drop_client()) # must not propagate
|
||||||
|
assert scheduler._client is None
|
||||||
Reference in New Issue
Block a user