Skip to content

fix(client): clean up provider when session startup fails - #1145

Open
JianYan11 wants to merge 1 commit into
huggingface:mainfrom
JianYan11:fix/session-provider-cleanup
Open

fix(client): clean up provider when session startup fails#1145
JianYan11 wants to merge 1 commit into
huggingface:mainfrom
JianYan11:fix/session-provider-cleanup

Conversation

@JianYan11

@JianYan11 JianYan11 commented Sep 10, 2026

Copy link
Copy Markdown

Summary

Fixes #1144.

When a provider-backed client is used directly as a session factory, a readiness failure escapes before a child client exists, leaving the provider running. Catch startup failures in _create_session_client() and reuse _stop_provider_best_effort() before re-raising the original exception.

This keeps the change local to session creation, without adding a second cleanup to the existing connect() failure path or changing any public API. Both container and runtime providers are covered, including a failing cleanup operation. The existing success test now also exercises new_session() without first connecting the parent.

Type of Change

  • Bug fix

Alignment Checklist

  • Read .claude/docs/PRINCIPLES.md; this preserves provider lifecycle ownership.
  • Checked .claude/docs/INVARIANTS.md; no API, protocol, or agent/orchestrator boundary changes.
  • Full pre-submit hook suite (see scoped validation below).

RFC Status

  • Not required: bug fix with no public API or architectural change.

Test Plan

  • The four new parametrized readiness-failure cases fail on unchanged main because the provider stop method is never called, and pass with this fix.
  • tests/test_core/test_generic_client.py: 104 passed, 5 skipped using a lightweight Python 3.13 environment. No Docker, model calls, or environment-specific dependencies required.
  • Changed files pass ruff check, ruff format --check, usort check, and git diff --check.
  • Full repository tests were not run. The lint hook was replaced with direct checks because its current implementation can discard working-tree changes (tracked separately in fix(hooks): stop lint.sh from modifying or discarding working tree changes #1070).

Reproduce the focused test run:

PYTHONPATH=src:envs uv run --no-project --python 3.13 \
  --with 'fastmcp>=3,<4' --with pytest --with pytest-asyncio \
  --with websockets --with pyyaml --with requests --with fastapi \
  --with tomli-w -- python -m pytest tests/test_core/test_generic_client.py -q -rs

Claude Code Review

N/A (implemented and checked with OpenAI Codex). AI-assisted contribution; human maintainer review is still needed.


Note

Low Risk
Localized lifecycle fix in private session creation with no API changes; risk is mainly regressions in provider start/stop timing around new_session().

Overview
Fixes a provider lifecycle leak when new_session() starts a container/runtime via _create_session_client() but wait_for_ready (or other startup) fails before any child client exists—unlike connect(), that path previously had no cleanup.

_create_session_client() now wraps _start_provider_if_needed() in try/except, calls _stop_provider_best_effort() on any exception, then re-raises the original error. No public API change; connect() failure handling is unchanged.

Tests add parametrized coverage for readiness failures on both start_container/stop_container and start/stop, including when stop itself fails, and extend the “reuse provider server” case to new_session() without a prior parent connect().

Reviewed by Cursor Bugbot for commit c3dcb8c. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor
cursor Bot requested a review from burtenshaw September 10, 2026 09:06

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head c3dcb8cd1b0f4741cd5979e23abdff51b7029cac against current main 42cde4c2e5efaef81be92f310fc954fd5bbce3b5. The change correctly closes the new_session() provider leak identified in #1144: readiness failure now invokes the matching container/runtime stop method, cleanup errors cannot mask the original startup exception, and no child or WebSocket is created. The parametrized tests cover both supported provider shapes, failing cleanup, and first-use new_session() behavior. No correctness, public-API, or alignment defect found.

This is not merge-ready yet: the external branch is behind main, and only Bugbot ran at this head. Refresh it, approve the external-contributor workflows, and require exact-head Tests, Package CI, and docs checks before merge. Normal maintainer review is also still required.

Open in Web View Automation 

Sent by Cursor Automation: Release

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.

EnvClient.new_session() leaks the provider-started container when wait_for_ready() fails

1 participant