Skip to content

Close LLM client httpx pool before asyncio.run exits#70

Merged
daniel-thom merged 2 commits into
mainfrom
fix/llm-client-aclose
May 10, 2026
Merged

Close LLM client httpx pool before asyncio.run exits#70
daniel-thom merged 2 commits into
mainfrom
fix/llm-client-aclose

Conversation

@daniel-thom
Copy link
Copy Markdown
Contributor

Summary

  • Add aclose() to the LLMClient protocol and implement it on AnthropicLLMClient and _OpenAICompatibleClient (delegates to the SDK's async close(), which drains the underlying httpx.AsyncClient connection pool).
  • Wrap the body of cli.run_ask_pipeline, generate._run, and tidy._call in try/finally: await llm_client.aclose() so the pool is closed inside the same event loop that uses it — before asyncio.run shuts the loop down.

Without this, the SDK's httpx client gets garbage-collected after the loop is already closed, and its scheduled aclose() trips RuntimeError: Event loop is closed. asyncio prints that as a "Task exception was never retrieved" traceback. Particularly noisy with datasight ask --files, which spawns one event loop per question — every row in a benchmark batch produced the traceback.

The fix also covers datasight generate and datasight tidy review, which had the same teardown pattern.

Test plan

  • pytest -m "not integration" — 1492 passed.
  • prek run --all-files — ruff / ruff-format / ty all clean.
  • New tests/test_llm.py cases assert that AnthropicLLMClient.aclose() and OllamaLLMClient.aclose() actually invoke the SDK's close().
  • Manual: run datasight ask --files=questions.txt and confirm no "Event loop is closed" traceback at the end of the run.

🤖 Generated with Claude Code

Adds aclose() to the LLMClient protocol and implements it on the
Anthropic and OpenAI-compatible backends (delegating to the SDK's
async close()). Each CLI flow that runs the client under asyncio.run
(`ask`, `generate`, `tidy review`) now awaits aclose() in a finally
block. Without this, the SDK's underlying httpx.AsyncClient is
finalized after the loop has already shut down and trips a noisy
"RuntimeError: Event loop is closed" traceback — particularly visible
in `datasight ask --files`, which spawns one event loop per question.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds an explicit async teardown hook to the LLM client abstraction and ensures CLI code paths close the underlying SDK/httpx connection pool before asyncio.run() shuts down the event loop, preventing noisy RuntimeError: Event loop is closed “Task exception was never retrieved” tracebacks during garbage collection.

Changes:

  • Extend the LLMClient protocol with aclose() and implement it for Anthropic and OpenAI-compatible backends.
  • Wrap key CLI async entrypoints (ask, generate, tidy review) in try/finally to await llm_client.aclose() within the same event loop.
  • Update/add tests and test stubs to satisfy the new protocol and assert the SDK close() method is awaited.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/datasight/llm.py Add LLMClient.aclose() and implement pool shutdown for Anthropic and OpenAI-compatible clients.
src/datasight/cli.py Ensure run_ask_pipeline closes the LLM client before loop teardown via try/finally.
src/datasight/cli_commands/generate.py Ensure the generate command closes the LLM client within the _run() event loop.
src/datasight/cli_commands/tidy.py Ensure the tidy review LLM call closes the LLM client within the _call() event loop.
tests/test_llm.py Add async tests asserting aclose() awaits the underlying SDK close().
tests/test_cli_tools.py Add/update LLM stubs to provide aclose() for new teardown logic in pipelines.
tests/test_cli_commands.py Update CLI test LLM stub to implement aclose().
tests/test_verify.py Update fake LLM clients to implement aclose() for protocol compatibility.
tests/test_tidy_review.py Update fake LLM client to implement aclose().
tests/test_generate_persist_db.py Update stub LLM client to implement aclose().
tests/test_agent_extra.py Update fake LLM client to implement aclose().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/datasight/cli.py
Comment on lines +135 to +140
# Close the SDK's httpx pool before the event loop shuts down.
# Without this, asyncio.run closes the loop, the GC later finalizes
# the httpx client, and its scheduled aclose() trips
# "RuntimeError: Event loop is closed" — noisy, especially in
# `datasight ask --files` where this runs once per question.
try:
@codecov
Copy link
Copy Markdown

codecov Bot commented May 10, 2026

Codecov Report

❌ Patch coverage is 76.52174% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.86%. Comparing base (b635119) to head (7e432ae).

Files with missing lines Patch % Lines
src/datasight/cli_commands/generate.py 78.18% 12 Missing ⚠️
src/datasight/cli_commands/verify.py 0.00% 12 Missing ⚠️
src/datasight/cli.py 92.50% 3 Missing ⚠️

❌ Your patch check has failed because the patch coverage (76.52%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
- Coverage   86.87%   86.86%   -0.01%     
==========================================
  Files          61       61              
  Lines       11539    11552      +13     
==========================================
+ Hits        10024    10035      +11     
- Misses       1515     1517       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot caught a missed call site: `datasight verify` also creates an
LLM client inside `asyncio.run` and never closed it, so it could still
trip the same "Event loop is closed" traceback on GC. Wrap the client
in the same try/finally pattern as the other CLI flows.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@daniel-thom daniel-thom merged commit 44bc403 into main May 10, 2026
7 of 8 checks passed
@daniel-thom daniel-thom deleted the fix/llm-client-aclose branch May 10, 2026 19:37
github-actions Bot pushed a commit that referenced this pull request May 10, 2026
* Close LLM client httpx pool before asyncio.run exits

Adds aclose() to the LLMClient protocol and implements it on the
Anthropic and OpenAI-compatible backends (delegating to the SDK's
async close()). Each CLI flow that runs the client under asyncio.run
(`ask`, `generate`, `tidy review`) now awaits aclose() in a finally
block. Without this, the SDK's underlying httpx.AsyncClient is
finalized after the loop has already shut down and trips a noisy
"RuntimeError: Event loop is closed" traceback — particularly visible
in `datasight ask --files`, which spawns one event loop per question.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants