fix: close backend clients and event-loop threads to stop resource leaks

[psschwei]

Pull Request

Issue

Fixes #349

Description

Fixes the resource leaks behind #349: 50+ ResourceWarnings (unclosed sockets, transports, event loops) across the test suite, all pointing at real cleanup gaps rather than just log noise.

Correction to the issue: the warnings aren't actually visible today — pytest 9 no longer forces always for all warning categories, so ResourceWarning falls back to Python's default-ignore list. Nothing shows up unless you pass -W always::ResourceWarning. The leaks are still there; only the "noise" symptom is gone.

Escalation found while investigating: a separate, worse leak in the same area — _EventLoopHandler spawned a fresh event loop + daemon thread on every nested _run_async_in_thread call and never shut it down. This is reachable from ordinary user code (call_tools() re-enters the handler loop from an async @tool wrapper), so it's fixed here alongside the resource warnings rather than split out.

Root causes fixed

1. _EventLoopHandler._close_event_loop called loop.stop() from a foreign thread (not thread-safe, often didn't wake the selector), never joined the thread, never called loop.close(). The nested branch in __call__ also built a fresh _EventLoopHandler() per call and dropped it, leaking a thread + loop + fds every time. Rewrote shutdown to cancel tasks on the loop, gather, then call_soon_threadsafe(loop.stop) → thread join → loop.close(), made idempotent, and made teardown deterministic in __call__ instead of relying on __del__.
2. ClientCache evicted entries without closing them and had no clear(). It now keys on the event loop object itself (not id(loop), which can be reused by a new loop after the old one is GC'd) and takes an optional aclose closer, invoked on eviction and by new clear()/aclear() methods.
3. Backend had no way to release the clients it holds open. Added default no-op close()/aclose() to the ABC, overridden in OllamaModelBackend, OpenAIBackend, and WatsonxAIBackend to close their sync client and clear their cached async client(s).

Testing

• Tests added to the respective file if code was changed
• New code has 100% coverage if code was added
• Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Added:

• test/helpers/test_event_loop_helper.py: nested-call thread-leak regression test, _close_event_loop idempotency test.
• test/helpers/test_async_helpers.py: ClientCache closer-on-eviction and clear()/aclear() tests.
• test/backends/test_client_close_unit.py (new): close()/aclose() unit tests for Ollama, OpenAI, and Watsonx backends.
• test/backends/test_ollama_socket_leak.py (new): real-socket e2e regression guard — creates a backend, drives it, closes it, asserts no ResourceWarning survives gc.collect(). This is a targeted stand-in for globally re-enabling -W always::ResourceWarning, scoped to one backend so it doesn't drown in third-party warning noise.
• Closed backends in the module/function-scoped fixtures the leak census implicated: test/backends/test_ollama.py, test/backends/test_openai_ollama.py, test/backends/test_watsonx.py, test/stdlib/test_session.py.

Verified against a live Ollama server: full suite (pytest test/ -m "not qualitative" -W "always::ResourceWarning") drops from a 54-socket/26-transport baseline to 44/22. ruff format/ruff check/mypy clean; docstring quality gate (Backend gained public methods) at 100% coverage, 0 issues.

Known, accepted residual: the remaining warnings all trace to async def tests whose per-test event loop (pytest-asyncio's default function scope) closes before any fixture teardown runs — structurally the same as an async client stranded on an already-closed asyncio.run() loop, just triggered by pytest-asyncio rather than by test code directly. Fixture-level cleanup can't reach these; closing them would need either inline await backend.aclose() in every affected async test or a broader pytest-asyncio loop-scope change. Neither is in scope here — this PR fixes what fixture-level cleanup can reach and adds a regression test for it, matching the "targeted test over global suppression" approach.

Attribution

• AI coding assistants used

---

Adding a new component, requirement, sampling strategy, or tool?

If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.

• Component
• Requirement
• Sampling Strategy
• Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

[planetf1]

Left inline comments — a few gaps in the new close/eviction machinery, mainly around the eviction/close path still being able to block the caller's event loop in some cases.

[ajbozarth]

Some feedback from Claude (reviewed on my behalf):

Solid work — the event-loop shutdown reordering, the nested-handler teardown, and the loop-object rekeying all check out, and the test coverage is thorough. I verified the rekeying is applied consistently across all three backends with no leftover id(...) call site, and that the HF (aLoRA) and LiteLLM backends are correctly out of scope. Not requesting changes; three low-priority notes inline, none blocking.

[ajbozarth]

Behavior change worth a sanity check: eviction used to just drop the reference and let GC close the client lazily; now it actively schedules aclose() on the client's loop. If a backend is ever driven by more than 2 concurrent live event loops, an evicted client could have its transport closed mid-request. Very unlikely given the single-background-loop design — just confirming it was a considered tradeoff.

[psschwei]

mellea/helpers/async_helpers.py:344 delegated the stopped-but-not-closed loop case to _close_client_for_loop, which calls loop.run_until_complete() — that raises inside the caller's already-running loop and got swallowed, so the client fell to GC. Now it goes through asyncio.to_thread(...), where no loop is running, so run_until_complete can actually drive the stopped loop; the coroutine awaits the result. Verified the old path silently no-oped (it also leaked a "coroutine was never awaited" warning); added test_aclear_drives_a_stopped_loop_to_close_its_entry, which fails on the old code.

[ajbozarth]

This branch silently no-ops in one case: for a client bound to a loop that's neither running nor closed, reached from async code, delegating to _close_client_for_loop runs loop.run_until_complete(...), which raises RuntimeError inside the already-running caller loop and gets swallowed by the except — so the client falls to GC rather than closing here. Rare degenerate case and consistent with the documented "GC reclaims it" philosophy, but the delegation doesn't actually achieve what the branch comment intends.

[psschwei]

Left the behavior as is and wrote the reasoning into ClientCache.put: closing on eviction assumes at most capacity loops drive a backend concurrently (true for the single-background-loop design); past that an evicted transport could close under an in-flight request, and the alternative (drop the reference, wait for GC) is exactly what leaked sockets. Worth noting the hazard is narrower than it looks: an evicted entry whose loop has already closed can't be closed at all, so the risky window needs 3+ live loops on one backend.

[ajbozarth]

Optional/nit: close() closes the sync self._client permanently with no reuse guard, while clearing the async cache lets _async_client rebuild on next access — so after close() the backend is in an asymmetric state (sync calls fail, async calls silently work). Fine for a teardown-only API; flagging in case reuse-after-close should instead raise cleanly. Same asymmetry applies to the Ollama sync client.

[psschwei]

Added a "teardown only, not a reset" paragraph to Backend.close (published API reference) and to the Ollama/OpenAI close() overrides, stating plainly that the sync client can't be reopened while a later async call rebuilds one into the emptied cache, so callers should build a new backend. I did not add a _closed guard that raises: that's a user-facing behavior change to a teardown path, beyond the note's scope. Say the word if you'd rather it raise cleanly, it's a small follow-up.

[planetf1]

Following up on my earlier eviction-blocking comment — looks like put() got fixed (now wait=False on eviction), but clear() still calls into this same branch with wait=True:

if loop is None:
    if wait:
        _run_async_in_thread(aclose(client))

_run_async_in_thread has no timeout, unlike the "close on another thread's loop" branch below (future.result(CLIENT_CLOSE_TIMEOUT)) and unlike what this function's own docstring promises ("waiting up to CLIENT_CLOSE_TIMEOUT only if wait").

This isn't a corner case — every sync-constructed backend (Ollama/OpenAI/Watsonx) creates its None-keyed async client in __init__, so this is the path close() actually takes for all of them via ClientCache.clear(). If the background loop is ever busy when this runs, close() — meant to be a quick, safe teardown call — would hang instead of giving up after 5s like everywhere else.

Could this use the same bounded-wait pattern as the other-thread branch: schedule the close and call .result(CLIENT_CLOSE_TIMEOUT) when wait is true, done-callback otherwise?

[psschwei]

added an update for this

[psschwei]

Fixed: that branch now uses the same bounded-wait pattern as the other-thread branch (_schedule_async_in_thread(...) then .result(CLIENT_CLOSE_TIMEOUT) when wait, done-callback otherwise), and the docstring bullet says so.

Two notes beyond the timeout:

• A timed-out .result() doesn't cancel the coroutine, so the close stays scheduled and still completes on the loop that owns the client.
• This is also more correct in the pathological case. When clear() ran on Mellea's background loop thread, _run_async_in_thread spun up a nested loop and closed the client on the wrong loop; now it's scheduled on the owning loop and the caller just gives up waiting after 5s.

Test: test_clear_bounds_its_wait_on_an_unbound_client puts a None-keyed client whose close blocks 10s and asserts clear() returns promptly. Verified it blocks the full 10s against the old code.

[planetf1]

This fixture now has to close the backend manually to avoid the resource warnings this PR fixes — which made me check MelleaSession.__exit__/cleanup(), and neither calls backend.close()/aclose() either. So for the normal usage pattern, with start_session() as m: ..., the leak this PR targets still happens: the fix only takes effect if the caller remembers to close the backend themselves, same as this fixture had to.

I get why session teardown might not want to auto-close — a backend can be shared across multiple sessions, so closing it on one session's __exit__ could pull the rug out from under another session still using it. If that's the reasoning, could we at least document it? Right now there's nothing telling a caller they need to close the backend once they're done with a session.

[psschwei]

Your reading of the reasoning is right: a backend can be shared by several sessions, so closing it in __exit__ could pull its clients out from under a session that is still generating. Documented rather than changed:

• start_session: paragraph before Args: stating that neither leaving a with block nor cleanup() closes the backend, and to call session.backend.close() / await session.backend.aclose() once nothing else needs it. The direct-usage example now shows the close call.
• cleanup(): carries the reasoning (shared backend, caller owns the connections).
• __exit__: one-line pointer to cleanup().

Auto-closing backends that start_session built is doable (it constructs them, so ownership is unambiguous), but it's a behavior change beyond this PR's scope: code that keeps using m.backend after the with block would break. Happy to open a follow-up issue if you think the default should change.

[planetf1]

put()'s docstring says it "clos[es] the evicted entry if one is displaced," but that only holds for the LRU-eviction branch below. When put() is called with a key already in the cache, the old value here is popped and discarded — never passed to _close_entry. So overwriting an existing key leaks that client's connections, the same class of leak this PR fixes elsewhere. Either close the displaced value here too, or narrow the docstring to say "the least-recently-used entry."

[psschwei]

Narrowed the docstring, and wrote in why closing there isn't safe.

A same-key put can only happen when another caller built a client for that key between this caller's get miss and this put (the three backends only put right after a miss). That means the value displaced here is the one that caller is holding and about to issue a request on, so closing it would break their in-flight request. Dropping the reference costs sockets until GC; closing costs a live request.

Summary line is now "closing the least-recently-used entry if one is evicted," with a paragraph on the same-key case. Added test_put_over_an_existing_key_does_not_close_the_displaced_client so the behavior is pinned rather than incidental.

Worth noting the underlying race is what makes this unavoidable: making the _async_client property atomic across get/create/put would remove the same-key path entirely, which feels like its own change.

[planetf1]

This filters ResourceWarnings by checking for the literal string "11434" in the message — but OllamaModelBackend resolves its host through OLLAMA_HOST, not a fixed port (see the docstring in ollama.py: defaults to env(OLLAMA_HOST) or localhost:11434). On any environment where that env var points elsewhere, this filter matches nothing and the test passes even if the leak comes back. Worth deriving the expected port from the resolved client's base URL instead, or asserting on any ResourceWarning in the del backend; gc.collect() window rather than one matching a specific string.

[psschwei]

Good catch, fixed. The port is now read off the constructed client (backend._client._client.base_url) via a _resolved_port() helper that asserts it resolved, so a filter that could match nothing fails loudly instead of passing silently.

One detail worth recording: backend._base_url is None unless base_url is passed explicitly (the ollama SDK resolves OLLAMA_HOST internally), so the constructed client's base URL is the only place the effective port exists.

Verified passing with the default and with OLLAMA_HOST=127.0.0.1:11434.

[planetf1]

__del__ calls _close_event_loop(join_timeout=1.0), which waits up to 1s for task cleanup plus up to 1s for the thread join. During interpreter finalization, sys.is_finalizing() is already True, and a daemon thread often can't be rescheduled to respond to call_soon_threadsafe at that point — if that's the case here, these two waits would just be dead time on every process exit rather than doing anything, which would be worth guarding with an is_finalizing() check. Haven't measured this myself though, so flagging it as worth a look rather than a confirmed timing issue.

[psschwei]

Measured this, and the mechanism is right but it isn't firing at exit today. Tracing a real shutdown: __del__ does run with sys.is_finalizing() true, but by then the loop thread is already dead and the loop is no longer running, so both waits are skipped and process teardown costs ~40-70ms (Python 3.12, macOS). No dead time to reclaim in that path.

The 2s is real whenever the thread is alive but can't be rescheduled, which is exactly what a frozen daemon thread looks like, so I added the guard as insurance: __del__ passes join_timeout=0.0 when finalizing, and _close_event_loop skips scheduling finalize_tasks() at a zero timeout so it doesn't trade the blocking wait for a "coroutine was never awaited" warning.

New test stands in for a frozen loop thread by parking a callback on the loop: 2.01s before the guard, under 0.5s after.

[planetf1]

The Raises: section says this raises RuntimeError once the loop's been closed, but since close() now sets self._event_loop = None, calling submit() afterward actually raises AttributeError (checked — asyncio.run_coroutine_threadsafe(co, None) raises AttributeError: 'NoneType' object has no attribute 'call_soon_threadsafe'). Worth either raising RuntimeError explicitly when self._event_loop is None, or updating the docstring — and maybe worth making __call__ behave the same way in that state, since right now it silently no-ops instead of erroring at all.

[psschwei]

Fixed by raising explicitly: a new _require_event_loop() raises RuntimeError when the loop reference has been cleared, and both submit() and __call__ go through it. Confirmed your repro, AttributeError: 'NoneType' object has no attribute 'call_soon_threadsafe', and the Raises: text now matches.

One correction on __call__: it wasn't silently no-opping. With _event_loop cleared, self._event_loop == get_current_event_loop() is None == None for any sync caller, so it took the same-loop branch and ran the coroutine to completion on a throwaway nested handler. A test against the old code shows the coroutine body running with nothing raised, which is arguably worse than the AttributeError. Both entry points now raise, and the reason for the explicit check is in __call__'s docstring.