[rodrigosetti]

Summary

Fixes #1308. Cancelling from_docker_image(...) or from_env(...) while the first WebSocket connection is still pending left the started provider running. _connect_async only cleans up on Exception, and asyncio.CancelledError is a BaseException. The factory raised before returning a client, so the caller had nothing to close.

_BootstrapResult._resolve_async now catches CancelledError around client.connect(). It calls client.close(), which stops the provider once, and then re-raises the original cancellation, message included. Suppression is limited to that cleanup call, so neither of these replaces the original cancellation:

• a second cancellation that arrives during teardown;
• a provider stop failure.

Ordinary connection failures behave as before: one stop, then ConnectionError.

Why this boundary: the fix sits where the factory hands its client over, not in _connect_async. When no client reaches the caller, the factory still owns cleanup. Putting it in _connect_async would also stop the container when a caller's deadline (for example asyncio.wait_for(env.step(...))) cancels a lazy reconnect, and that would be a wider behaviour change.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation
• New environment
• Refactoring

Alignment Checklist

Before submitting, verify:

• I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
• I have checked .claude/docs/INVARIANTS.md and no invariants are violated (internal client lifecycle only; no API, interface or client/server boundary changes)
• I have run /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues

RFC Status

• Not required (bug fix, docs, minor refactoring)
• RFC exists: #___
• RFC needed (will create before merge)

Test Plan

New regressions in tests/test_core/test_client_provider_cleanup.py. Each one waits for ws_connect to start and then calls a real Task.cancel(). They run with container-style (start_container/stop_container) and runtime-style (start/stop) providers, and with from_docker_image and from_env. The existing ResourceLedger treats a duplicate stop as an error.

Test	Asserts
test_factory_cancelled_during_connect_stops_provider_once	Exactly one stop; the original message "factory cancelled" is kept; the task ends cancelled
test_factory_cancelled_again_during_cleanup_keeps_original_cancellation	A second cancel() lands inside a teardown that suspends; the provider is still stopped once and the original cancellation propagates
test_factory_cancellation_survives_provider_cleanup_failure	If stop raises, the result is still CancelledError, not the cleanup error
test_factory_connect_failure_stops_provider_once	An ordinary connect failure still raises ConnectionError with the original cause and stops once

Results:

• On unmodified main (49aa302): the 9 new cancellation tests fail. The 4 ordinary connect-failure tests pass.
• Mutation check: without CancelledError in the cleanup suppression, the 3 second-cancellation tests fail.
• Focused suites (test_client_provider_cleanup.py and test_generic_client.py): 218 passed, 9 skipped on Python 3.10, 3.11, 3.12 and 3.14. The tests capture the cancellation inside the task, because on 3.10 await task drops the message.
• Stability: 15 repeated runs of the new tests, all passing.
• Reproduction script from EnvClient: cancelling the async factory during WebSocket connection leaks the started provider #1308: before the fix provider still started: True, stop calls: 0. After: False, 1.
• Full suite, using the CI command and extras (Python 3.14): 3335 passed, 1 failed. The failure is tests/envs/test_harbor_capture_server.py::test_start_refuses_a_port_held_by_a_non_capture_listener, which fails the same way on unmodified main and looks like a local issue.
• Lint: ruff check passes, and the changed files pass ruff format. Repo-wide usort and ruff format checks still flag files this PR doesn't touch (tests/envs/test_grid_world.py, tests/envs/test_julia_env.py, src/openenv/core/harness/README.md); these appear on main too.

Known limitations (out of scope)

• If stopping the provider fails during cancellation, the error is suppressed so that cancellation still propagates. Existing cleanup in this file already suppresses errors this way (_connect_async startup failure, .sync() bootstrap failure). The resource stays up, and the caller still holds the provider it passed in, so it can stop the provider itself.
• Related gaps remain where this fix doesn't reach:
  • async with Client(provider=p): cancellation during __aenter__.
  • The sync .sync() path when interrupted by KeyboardInterrupt.

Claude Code Review

N/A. /alignment-review was not run. The change was written with Claude Code and checked with /simplify, the mutation check and the tests above.

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches async factory bootstrap and provider lifecycle on cancellation; behavior change is scoped to factory resolution before a client is returned, with regression tests but subtle asyncio edge cases.

Overview
Fixes a leak when await from_env(...) or await from_docker_image(...) is cancelled while the first WebSocket connect is still in flight: the provider had already started, but CancelledError skipped _connect_async’s except Exception cleanup and no client was returned for the caller to close().

_BootstrapResult._resolve_async now catches cancellation around connect(), best-effort close() (one provider stop), then re-raises the original cancellation—including its message—without letting a second cancel or stop failure replace it.

Tests cover cancel during connect (both factories and container/runtime providers), double-cancel during teardown, stop failure during cancel cleanup, and unchanged ConnectionError on ordinary connect failure.

^{Reviewed by Cursor Bugbot for commit 30ef3c1. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Reviewed at head ee2817c0. The change is narrowly scoped: on CancelledError during connect(), the not-yet-returned client is closed so the provider is released, then the original cancellation is re-raised, and cleanup failures or a second cancel cannot replace it. This matches the existing ownership rule in _connect_async. Merged onto current main (053aa3e6) in a scrubbed local environment: tests/test_core client/provider subset 504 passed, 14 skipped, ruff clean.

Blocked only on fork CI (Approve and run needed). Touches src/openenv/core/env_client.py, so it is a 0.7.1 candidate once exact-head CI is green and the branch is current.

  

_{Sent by Cursor Automation: Release}

[rodrigosetti]

Thanks! looks like the PR workflows needs maintenance approval 🙏

[burtenshaw]

Reviewed cancellation ownership and repeated cancellation behavior. The factory closes its unreturned client and preserves the original cancellation; cleanup tests pass (88 passed, 4 skipped), and touched lint/format/import checks pass. No correctness or complexity findings.

[bot-ci-comment]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.