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

[caiotheodoro]

EnvClient.new_session() can leak the container/process it starts if the environment never becomes healthy.

new_session() → _create_session_client() → _start_provider_if_needed() (src/openenv/core/env_client.py:362):

def _start_provider_if_needed(self) -> None:
    if self._ws_url is not None:
        return
    ...
    base_url = self._provider.start_container()
    self._provider.wait_for_ready(base_url)
    ...
    self._set_base_url(base_url)

No try/except here. If start_container() succeeds and wait_for_ready() then raises (container never passes its health check), the exception propagates straight out of new_session() with the container still running — nothing calls provider.stop_container(). Compare with the two other places that do the same start+wait sequence: _connect_async wraps this exact call in try: ... except Exception: await self.close(); raise (which does stop the provider), and _bootstrap_container (used by from_docker_image) has its own explicit except Exception: provider.stop_container(); raise. _create_session_client's caller, new_session(), has neither.

This is reachable whenever a provider-backed client's first use is new_session() rather than connect() — i.e. using the client purely as a session factory, which is exactly what new_session() is documented for ("Create and connect a new session against the same environment server").

Repro (fake provider standing in for LocalDockerProvider, so it's provider-shape only, no real docker needed):

import asyncio
from openenv.core.env_client import EnvClient
from openenv.core.client_types import StepResult

class FakeProvider:
    def __init__(self):
        self.started = False
        self.stopped = False
    def start_container(self, **kwargs):
        self.started = True
        return "http://127.0.0.1:9"
    def wait_for_ready(self, base_url, timeout_s=30.0):
        raise TimeoutError(f"container at {base_url} never became ready")
    def stop_container(self):
        self.stopped = True

class EchoClient(EnvClient):
    def _step_payload(self, action): return action
    def _parse_result(self, payload): return StepResult(observation=payload, reward=0.0, done=False)
    def _parse_state(self, payload): return payload

async def main():
    provider = FakeProvider()
    client = EchoClient(provider=provider)  # connect() never called
    try:
        await client.new_session()
    except TimeoutError as e:
        print(f"new_session() raised: {e}")
    print(f"started={provider.started} stopped={provider.stopped}")

asyncio.run(main())

Output:

new_session() raised: container at http://127.0.0.1:9 never became ready
started=True stopped=False

The existing test for this path (tests/test_core/test_generic_client.py::test_new_session_reuses_provider_server) always calls client.connect() before new_session(), so _start_provider_if_needed() short-circuits on the first line (self._ws_url is not None) and start_container/wait_for_ready never actually run inside new_session() in that test — the gap here isn't covered either way.

Fix: wrap _start_provider_if_needed()'s body the same way _bootstrap_container does — on any exception after start_container() succeeds, call self._provider.stop_container() (or .stop()) before re-raising.

Repro'd against a0ae130fa2af0d6c318d650e08cdd0ea20c1baf2 (current main), Python 3.13.12, macOS.

[JianYan11]

Quick update: the fix for this issue is ready in #1145 🚀

It now covers the original provider cleanup leak as well as the edge case uncovered during review. I also added regression coverage across async/sync clients and validated the fix against the core, factory, and real Docker cleanup tests.

I found a few broader, pre-existing lifecycle edge cases during the audit. I documented those separately so we can follow up without expanding the scope of this fix.

@burtenshaw, would appreciate a final look — if everything looks good, this should be ready to merge. Thanks!