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

[JianYan11]

Summary

Fixes #1144.

Provider-backed new_session() can fail after a resource has already been allocated: during provider start, readiness, or child construction. Cleanup is attempted immediately while preserving the original setup error.

If that cleanup also fails, the client now records an unresolved cleanup obligation. A later connect() or new_session() must successfully stop the old resource before starting another one, preventing the provider's single resource handle from being overwritten. Successful cleanup remains idempotent, and a failed sibling construction does not stop a provider already serving the parent or another child.

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.
• Checked open RFCs; no conflicts found.

RFC Status

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

Test Plan

• Added 60 parameterized lifecycle regression cases covering async/sync clients, container/runtime provider interfaces, partial startup, readiness and constructor failures, failed close, blocked retries, recovery ordering, shared children, and repeated failure/recovery cycles.
• Added 4 opt-in tests using real Docker containers with injected readiness/cleanup failures. They verify that a failed cleanup cannot be followed by a second allocation and that no test containers remain afterward.
• New cleanup tests plus test_generic_client.py: 190 passed, 5 skipped.
• All core tests plus relevant OpenCode, Pi, and Harbor factory/session tests: 510 passed, 16 skipped.
• Real Docker lifecycle suite: 4 passed.
• Changed files pass usort check, ruff format --check, ruff check, and git diff --check.

The repository test hook cannot execute on this macOS host because it requires GNU timeout. Running the equivalent pytest command reaches collection but the local environment lacks the optional smolagents dependency required by test_coding_codeact_env.py. The scoped core and related lifecycle suites above pass.

Claude Code Review

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

---

Note

Medium Risk
Changes container/process lifecycle and retry semantics on failure paths; behavior is well-covered by tests but affects resource ownership for all provider-backed clients.

Overview
Fixes provider-backed EnvClient leaking containers/processes when new_session() or connect() fails after a resource is allocated (start, readiness, or child constructor).

Lifecycle changes in env_client.py: The client tracks _provider_cleanup_pending and _provider_stopped, centralizes teardown in _stop_provider(), and on failed session startup only stops the provider when this call actually started it (not when a sibling session already owns a live server). If stop fails, later connect() / new_session() must successfully stop the old resource before allocating again; factory clients with a cached URL get a explicit RuntimeError to retry close() instead of reconnecting. connect() preserves the original setup error when cleanup is already pending, and avoids a redundant close() on failed cleanup retry.

Tests: Large parameterized suites (test_client_provider_cleanup.py, test_generic_client.py session cleanup) plus opt-in Docker lifecycle tests assert no double-stop, blocked retries, recovery ordering, and no orphaned containers.

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

[cursor]

Stale comment

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.

  

_{Sent by Cursor Automation: Release}

[cursor]

Stale comment

Re-reviewed exact head c3dcb8cd1b0f4741cd5979e23abdff51b7029cac; this supersedes the earlier no-defect assessment. The intended readiness-failure cleanup works, and the focused file passes locally (104 passed, 5 skipped), but two lifecycle failures remain.

1. After failed readiness, the new handler stops the provider but leaves it owned by the parent. A normal later parent.close() stops it again; with a non-idempotent provider I reproduced RuntimeError("already stopped") and two stop calls.
2. If this call successfully starts the provider and the concrete child constructor raises, construction is outside the cleanup region, so the provider remains running. I reproduced zero stop calls before explicit cleanup.

Please track whether _create_session_client() started the provider in this invocation, guarantee cleanup for every subsequent construction failure without tearing down an already-connected parent's shared provider, and make provider ownership/state prevent a successful stop from being repeated. Add regressions for parent close after readiness failure, cleanup after child-constructor failure, and the already-connected-parent constructor-failure case. The branch also remains behind main, with only Bugbot on this head; require refreshed exact-head repository CI and maintainer review afterward.

  

_{Sent by Cursor Automation: Release}

[cursor]

Stale comment

Refresh review vs main b12450f2 (Ben ask):

Verdict: still blocked on prior CHANGES_REQUESTED at c3dcb8cd (~15 behind main; only Bugbot on tip).

Readiness-fail stop is correct, but still need:

1. Clear provider ownership after successful stop so later parent.close() does not double-stop
2. Cleanup if provider start succeeds then child construction fails
3. Regressions for those cases + already-connected parent

Please address those and rebase; then I’ll re-review.

View PR

  

_{Sent by Cursor Automation: Release}

[burtenshaw]

@JianYan11 let me know if you're working on this.

[cursor]

Stale comment

Refresh review (Wed 2026-09-16)

Still BEHIND main (c3dcb8cd, 15+ commits). Tip is unchanged since the 2026-09-15 CHANGES_REQUESTED — both blockers remain:

1. After readiness failure, provider is stopped but still owned → later parent.close() can double-stop / raise.
2. Successful provider start + failing child constructor is outside the cleanup try → provider leak.

No bit-rot from #1169. Needs author fix before any on-repo replacement/refresh. Not eligible for Thursday as-is.

View PR

  

_{Sent by Cursor Automation: Release}

[JianYan11]

@burtenshaw Yes！let me check the comment of cursor bot

[cursor]

Re-review after rebase — still CHANGES_REQUESTED

Head 34965c2b (rebased onto main a4798f5d). Focused suite green (test_generic_client 114 passed / 5 skipped).

The readiness-fail cleanup path is still the right idea, but the two Tier 1 bugs from the earlier review remain on this head:

1. Double-stop on close() — _stop_provider_best_effort() does not clear self._provider, so await close() stops again and non-idempotent providers raise.
2. Child constructor leak — try/except still only covers _start_provider_if_needed(); a later ctor failure leaves the started provider running.

Both reproduced live against this SHA. Required fix shape + regressions are unchanged from the 2026-09-16 review (clear ownership fields before stop; wrap the full _create_session_client body; assert no double-stop and ctor-fail cleanup).

Fork PR: repository CI still needs maintainer Approve and run when ready.

  

_{Sent by Cursor Automation: Release}

[cursor]

Re-review — APPROVE

Head 15773f3c. Prior Tier 1 items are fixed with a retry-safe _provider_stopped latch (clearer than nuking _provider for restart):

1. Double-stop — successful stop sets the latch; await close() after a failed new_session() no longer re-stops. Reproduced: 1 stop, close OK.
2. Child ctor leak — try/except now wraps the full _create_session_client body and cleans up when this call started the provider. Reproduced: start+stop on ctor failure.

Also covered: failed cleanup can retry; restart after successful cleanup; connected/shared-provider ctor failures do not stop the running provider; sync path cleanup-once.

Focused suite: 130 passed / 5 skipped. ruff check / ruff format --check clean on the two touched files.

Gate

Fork PR — needs maintainer Approve and run before merge. Package-surface (src/openenv/core/env_client.py); fine as next-cut cargo once CI is green.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Stale Bugbot comment from a previous run.

Withdrawing APPROVE on 15773f3c — Bugbot’s failed-stop restart leak is real (reproduced). Follow-up REQUEST_CHANGES next.

[cursor]

Re-review — REQUEST_CHANGES (withdraws prior APPROVE)

Bugbot’s medium finding on 15773f3c is confirmed live.

Tier 1 — failed stop still allows a second start (leak)

_stop_provider only sets _provider_stopped after a successful stop, but always clears _base_url / _ws_url in finally. _stop_provider_best_effort swallows the stop error. The client then looks idle (_ws_url is None, flag False) while the first container/process is still running, so the next new_session() calls start_container()/start() again and orphans the first.

Reproduced on this head:

1. Readiness fail → stop raises → second new_session() starts container #2 while #1 was never successfully stopped.
2. Same shape for child-ctor fail after a successful start (Bugbot’s stated path).

test_failed_cleanup_can_be_retried covers retry-via-close(), but does not assert that a later new_session() must not start a second provider until stop has succeeded.

Required fix direction

Do not treat a failed stop as “idle / free to start again.” Concrete options:

• Keep ownership until stop succeeds (do not clear URL state on failed stop when a start actually happened), and/or
• Gate _start_provider_if_needed on “not started-or-not-stopped” (e.g. refuse or force-stop before a second start),
• Add a regression: stop fails once → new_session() must not call start* again until a successful stop (or must stop-then-start atomically).

Prior double-stop and ctor-cleanup coverage can stay; this is an additional leak class on the retry-safe latch design.

  

_{Sent by Cursor Automation: Release}

[cursor]

Re-review — APPROVE

Head ba88f81b fixes the failed-stop restart leak.

_provider_cleanup_pending is set before stop* and cleared only on success. _start_provider_if_needed retries that cleanup first and does not start if stop still raises. Live check: always-failing stop → second new_session() raises, start* stays at 1; stop-then-succeed → stop #2 then start #2 (no orphan).

New coverage: tests/test_core/test_client_provider_cleanup.py (+ opt-in Docker ledger). Combined focused suite: 190 passed / 5 skipped. ruff clean on touched files.

Gate

Fork PR — still needs maintainer Approve and run before merge.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit ba88f81. Configure here.}

Withdrawing APPROVE on ba88f81b — Bugbot’s factory-client cleanup finding is confirmed live. Follow-up REQUEST_CHANGES next.

[cursor]

Re-review — REQUEST_CHANGES (withdraws APPROVE on ba88f81b)

Bugbot high finding is confirmed live.

Tier 1 — cleanup retry stops live factory providers

_start_provider_if_needed now runs _stop_provider() whenever _provider_cleanup_pending before the _ws_url is not None early-return.

Factory clients from from_docker_image / from_env pass base_url + provider, so _start_provider_on_connect is False and a failed close() keeps the URL while setting _provider_cleanup_pending=True. A later connect() / new_session() then:

1. Retries stop and kills the still-running provider
2. Sees _ws_url still set and returns
3. Connects to a now-dead address

Reproduced on this head (failed close → pending+URL retained → connect() performs stop#2 then reuses the URL).

Required direction

Only retry cleanup when a new start is actually required (URL absent / provider-owned startup path), or invalidate factory URL state when cleanup becomes pending so reconnect cannot target a killed provider. Add a regression for base_url=…, provider=… failed-close → later connect() must not stop-then-reuse the old URL.

Prior failed-stop→second-start leak fix can stay; this is a new interaction with factory-owned URL retention.

  

_{Sent by Cursor Automation: Release}

[JianYan11]

@burtenshaw I've kept this follow-up scoped to the regression introduced by this PR. Commit 56293948 prevents a factory-created client with pending provider cleanup from implicitly stopping its resource and then reconnecting to its cached URL. connect() / new_session() now reject that state with an explicit instruction to retry close() and create a new client. Lazy provider-owned startup retains its existing cleanup-before-restart behavior. This deliberately does not redefine the pre-existing behavior of reconnecting after a successful factory-client close.

Validation after this change:

• Core suite: 491 passed, 15 skipped, including 16 new factory regression cases across sync/async, container/runtime providers, connect/new_session, and cleanup still failing/recovered. These assert no additional provider or WebSocket operation occurs during rejected retries, and explicit close can still retry cleanup exactly once successfully.
• OpenCode/Pi/Harbor factory suites: 35 passed, 1 skipped.
• Real Docker fault-injection cleanup tests: 4 passed (actual containers, not a full cloud-provider E2E).
• Both changed files pass Ruff lint/format, usort and diff checks. Repository-wide checks are not all green: the standard test hook lacks timeout on this macOS host, and global formatting has pre-existing failures. The preceding broad audit ran 2655 passed, 120 skipped, 1 failed with optional-environment/network/integration exclusions; the failure is an unchanged Harbor port-ownership test, also failing in isolation.

The wider audit suggests that stronger lifecycle guarantees need a separate, bounded follow-up—not more state flags in this fix, and not a claim of “absolute safety.” In particular, actual OpenEnv ACA/Modal/Novita/Daytona stop methods clear their sandbox handle in finally even when deletion fails. With an injected delete failure before the request is delivered, a second stop returns without deleting the still-live resource. LocalDockerProvider has analogous handle-loss/error-classification paths. Retaining the provider reference in EnvClient alone cannot make these adapters retryable. These adapter implementations are unchanged by this PR. The audit also reproduced pre-existing cancellation cleanup and failed synchronous-acquisition thread issues; those diagnostic tests are retained locally, not bundled into this narrow commit.

Useful implementation references for that follow-up:

1. E2B resource identity and deletion outcomes: SandboxApi._cls_kill deletes by stable sandbox ID, treats 404 as already absent, and raises other errors. Its instance kill implementation does not erase that ID on failure. OpenEnv adapters should retain cleanup identity after ambiguous/failed deletion instead of treating any failure as success.
2. E2B reconnect contract: connect consults the control plane, rather than inferring liveness from a cached address. Its tests distinguish paused/resumable from killed/nonexistent sandboxes. OpenEnv should explicitly define close/reconnect semantics for factory-owned versus caller-supplied endpoints.
3. Modal termination acknowledgement versus completion: terminate(wait=...) and poll() separate sending a termination request from waiting for termination. The provider contract should say which guarantee a successful stop provides; returning from a cloud API call is not universally proof that a resource has disappeared.

My suggested sequence is: merge only the narrowly validated fix when review is satisfied; separately define provider ownership and cleanup outcomes (including NotFound and unknown outcome), repair adapter handle retention, then unify failed/cancelled acquisition cleanup and sync-wrapper ownership. Use stable resource-ID/liveness assertions and fault injection for failures before and after deletion takes effect, repeated close, shared children, cancellation, and thread teardown. Whether a failed first child should immediately roll back the parent's provider is a contract decision—the parent can still own and close it—so I would not silently change that policy here. Any new public lifecycle API should go through the project's RFC process.

These are source-based references and local fault-injection results, not a claim that the external cloud SDKs or every provider have been tested end-to-end.

[cursor]

Re-review — APPROVE at 56293948

The open Tier 1 (factory reconnect while cleanup is pending) is fixed.

_start_provider_if_needed now refuses connect() / new_session() when _provider_cleanup_pending and the client already has a factory URL (not _start_provider_on_connect), instead of stopping the live provider and then early-returning onto the cached dead ws URL. Lazy provider-owned startup still retries cleanup-before-restart. New regression test_factory_pending_cleanup_requires_explicit_close covers sync/async × connect/new_session × cleanup still failing/recovered and asserts no provider/WebSocket side effects on rejected retries.

Local: tests/test_core/test_client_provider_cleanup.py 76 passed; test_generic_client.py 130 passed, 5 skipped. Bugbot green on this head.

Residual (accepted / out of scope for this PR, per author): reconnect after a successful factory close() still keeps the cached URL; cloud-adapter handle-loss on failed delete is a separate contract follow-up. Not blocking this narrow fix.

Merge gate: this is a fork PR — only Bugbot has run so far. Needs maintainer Approve and run for test.yml / package CI before merge.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Re-review — REQUEST_CHANGES (withdraws APPROVE on 56293948)

The factory reconnect fix in 56293948 is correct for that specific Bugbot finding, and the new regression test is good. Withdrawing APPROVE because the PR’s cleanup-pending model still has a Tier 1 stuck-state on this exact head.

Tier 1 — required-arg start_container() validation → permanent _provider_cleanup_pending

Reproduced on 56293948 with a provider whose start_container(image) requires image and whose stop_container() raises:

1. connect() → _start_provider_if_needed() raises the actionable ValueError (never allocated).
2. _connect_async’s except sees not _provider_cleanup_pending, so it suppress-calls close() → _stop_provider() sets _provider_cleanup_pending=True, then stop* raises → pending stays True.
3. Next connect() retries stop first and surfaces the cleanup RuntimeError instead of the original ValueError.

So a config/validation error permanently poisons the client’s reconnect path. Please either:

• not call close()/_stop_provider() on a path that never started a resource (e.g. only cleanup when a start was attempted / ownership was taken), or
• clear _provider_cleanup_pending when stop runs against a never-started provider,
  and add a regression for required-arg validation + failing stop.

Still good (not re-blocking by themselves)

• Factory pending-cleanup reject (not _start_provider_on_connect) — fixed; prior thread addressed.
• Author’s deferred cloud-adapter handle-loss / successful-factory-close reconnect — out of scope; track separately.

Local focused suites still pass (76 + 130); Bugbot green. Fork CI still needs Approve-and-run once the stuck-pending path is fixed.

  

_{Sent by Cursor Automation: Release}

[cursor]

APPROVE at 5629394845a4c57371c0c4e67aa0bdf9a046701a

This withdraws the CHANGES_REQUESTED I left on this same head on 2026-09-20. The change request was wrong, and the reason matters, so here is the evidence rather than just the verdict.

Why the last blocker does not hold

I flagged that a pre-start ValueError (provider with required start_container args) leaves _provider_cleanup_pending latched when stop_container() also raises. The behaviour is real, but I assessed it against an idealised baseline instead of against main. Measured on both trees with the same provider double:

scenario	main (7e591317)	this PR
config error, in-repo-style stop — 1st connect	ValueError (correct)	ValueError (correct)
config error, in-repo-style stop — 2nd connect	ValueError	ValueError
config error, in-repo-style stop — stop_container calls	2	1
config error, hostile stop — 1st connect	RuntimeError: stop blew up (real error masked)	ValueError (real error surfaced)
config error, hostile stop — 2nd connect	RuntimeError: stop blew up	RuntimeError: stop blew up
redundant close() ×2 after a failed connect	3 stop_container calls	1

So on the path I called a regression, main is equally wedged and it swallows the actual configuration error. This PR surfaces the real error on the first attempt and matches main afterwards. It also stops calling stop_container() on already-stopped providers, which matters for the non-idempotent providers this PR's own test double models.

The latch additionally requires a provider whose stop_container() raises when nothing was ever started. No provider in this repository can do that — LocalDockerProvider, DockerSwarmProvider, ACAProvider, ModalProvider, DaytonaProvider, NovitaSandboxProvider, HFSandboxProvider and UVProvider all short-circuit on a null handle (if self._sandbox is None: return, if self._container_id is None: return, if self._process is not None: …). It needs a third-party provider of that exact shape.

Verified at this head

• The original #1144 leak is fixed: a provider started by a failing connect() is started once and stopped once; _provider_cleanup_pending and _provider_stopped both discharge.
• Factory clients never reconnect to a dead URL: with cleanup latched, the guard raises and ws_connect call count is 0. The provider is not stopped a second time by the guard.
• Transient stop failures recover: a stop that fails once then succeeds unlatches, and the next connect allocates a new resource rather than reusing the dead handle.
• new_session() does not stop a shared provider: a child-constructor failure on a client with an existing base URL leaves stop_container uncalled, while the same failure on a client that started the provider itself cleans it up.
• 8 adversarial probes, all holding.
• tests/test_core/test_client_provider_cleanup.py + test_client_provider_cleanup_docker.py + test_generic_client.py: 206 passed, 9 skipped.
• Full CI-equivalent suite: 2858 passed, 95 skipped, 41 deselected.
• CI-equivalent lint (usort format then ruff format, then ruff check): no resulting diff, all checks pass.

One residual nit, not blocking

MCPClientBase._connect_async (production mode) still wraps its failure path in a bare await self.close() without the suppress(...) that EnvClient._connect_async gained here. When the new guard fires and the retried stop also fails, the stop error masks the guard's message. Worth a follow-up, but it is strictly better than main, which has no guard at all and silently connects to the dead URL. Safety is preserved either way — ws_connect is never reached.

Merge gate

This is not mergeable yet for reasons outside the author's control: repository CI has never run on this fork branch (only Cursor Bugbot reports), so the required checks are absent and mergeStateStatus is BLOCKED. A maintainer needs to click Approve and run on the workflows, and the PR still needs a maintainer approval. @burtenshaw — this one is ready for that from my side.

  

_{Sent by Cursor Automation: Release}

[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.