From 0c5a1573da5daafdfad309f991394883037a1543 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 24 Jun 2026 09:38:41 -0400 Subject: [PATCH] fix(unifi): close the cached client before discarding it (fd leak) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- plugins/unifi/scheduler.py | 23 ++++++- tests/plugins/unifi/__init__.py | 0 tests/plugins/unifi/test_scheduler.py | 92 +++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 tests/plugins/unifi/__init__.py create mode 100644 tests/plugins/unifi/test_scheduler.py diff --git a/plugins/unifi/scheduler.py b/plugins/unifi/scheduler.py index e050c40..cfdeab6 100644 --- a/plugins/unifi/scheduler.py +++ b/plugins/unifi/scheduler.py @@ -15,6 +15,25 @@ logger = logging.getLogger(__name__) _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: cfg = app.config["PLUGINS"]["unifi"] interval = int(cfg.get("poll_interval_seconds", 60)) @@ -51,7 +70,7 @@ async def _do_poll(app: "Quart") -> None: await _client.login() except Exception as exc: logger.warning("UniFi initial login failed: %s", exc) - _client = None + await _drop_client() return try: @@ -60,7 +79,7 @@ async def _do_poll(app: "Quart") -> None: devices = await _client.get_devices() except Exception as 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 scraped_at = datetime.now(timezone.utc) diff --git a/tests/plugins/unifi/__init__.py b/tests/plugins/unifi/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/plugins/unifi/test_scheduler.py b/tests/plugins/unifi/test_scheduler.py new file mode 100644 index 0000000..071062f --- /dev/null +++ b/tests/plugins/unifi/test_scheduler.py @@ -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