PTY teardown can be abandoned by cancellation after registry removal

[fscfede-beep]

Please read this first

• Have you searched for related issues? Yes. I did not find an issue covering PTY entries that are removed from the registry before cancellation-settled backend teardown.
• Scope: this reproduces on current main at 89c02c828ee8510fe9a84ee6675608193aa13b02. It is pre-existing and is not being reported as a regression from PR fix: keep a utf-8 character that straddles two pty collection windows #4745.

Describe the bug

The PTY finalizers remove a finished entry from the session registry and then await backend teardown:

async with self._pty_lock:
    removed = self._pty_processes.pop(process_id, None)
    self._reserved_pty_process_ids.discard(process_id)

if removed is not None:
    await self._terminate_pty_entry(removed)

Once the pop commits, pty_terminate_all() can no longer reach that entry. If the caller is cancelled while _terminate_pty_entry() is suspended, teardown is cancelled too and there is no registry owner left to retry it.

Blaxel is the clearest concrete resource leak: its terminate path awaits cleanup that closes the PTY/WebSocket-side resources and the aiohttp.ClientSession. Other backends also have awaitful teardown paths, although I would not claim identical runtime exposure for every provider without provider-specific integration tests.

Why this matters

Registry removal transfers ownership of cleanup away from the session map. After that point, caller cancellation should be allowed to cancel delivery of the result, but should not be able to abandon resource teardown.

The repository already has the stronger ownership pattern in sandbox/session/mount_lifecycle.py::_settle_mount_transition: create and own the work task, await a separate completion task under asyncio.shield, keep waiting across repeated caller cancellations, consume the work result, and only then propagate the caller's cancellation state.

Deterministic regression shape

This does not require a real provider. A unit test can monkeypatch termination so it blocks after starting:

terminated = []
terminate_started = asyncio.Event()
release_terminate = asyncio.Event()

async def blocked_terminate(target):
    terminate_started.set()
    await release_terminate.wait()
    terminated.append(target)

session._terminate_pty_entry = blocked_terminate

# register a finished entry, then:
task = asyncio.create_task(
    session._finalize_pty_update(
        process_id=1,
        entry=entry,
        output=b"",
        original_token_count=None,
    )
)

await terminate_started.wait()

# The registry removal has already committed here.
assert 1 not in session._pty_processes

task.cancel()
with contextlib.suppress(asyncio.CancelledError):
    await task

release_terminate.set()
await asyncio.sleep(0)

assert terminated == [entry]

On the current ownership shape, the final assertion fails: cancellation propagates into the awaited terminate coroutine after the entry has already become unreachable from the registry.

A dependency-free control-flow reproduction of the same transaction gives:

• registry removed: yes
• cancellation during teardown: yes
• cleanup completed: no
• registry can recover entry: no

A settled-owner version modeled after _settle_mount_transition survives repeated cancellation and propagates cancellation only after cleanup completes.

Expected behavior

Once PTY registry removal commits, backend teardown should have an owner that survives caller cancellation until cleanup reaches a terminal state.

A narrow implementation could factor a PTY cleanup helper analogous to _settle_mount_transition rather than sprinkling bare asyncio.shield() calls that leave background task ownership ambiguous.

Relationship to PR #4745

During review of #4745 I initially treated a repeated-cancellation version of this as part of that PR's new final-tail path. Comparing against the exact PR base showed that this post-pop terminate-await ownership boundary already exists on main, so I corrected the review and withdrew it as a blocker on that PR.

PR #4745 has a separate, PR-specific single-cancellation regression around its newly-added tail-drain await; that is distinct from this pre-existing lifecycle-hardening issue.

Debug information

• Repository: openai/openai-agents-python
• Main SHA: 89c02c828ee8510fe9a84ee6675608193aa13b02
• Provider/model: n/a for the deterministic ownership repro
• Network/provider integration execution for this report: not required / not claimed

Second manifestation: pty_terminate_all() clears ownership before batch cleanup

The same ownership problem exists in every current pty_terminate_all() implementation. All seven backends copy the entries, clear the registry, and then await teardown sequentially:

async with self._pty_lock:
    entries = list(registry.values())
    registry.clear()
    reserved_ids.clear()

for entry in entries:
    await self._terminate_pty_entry(entry)

If caller cancellation lands while cleaning the first entry, the current cleanup is cancelled and every later entry is already unreachable from the registry, so their cleanup never starts.

A deterministic two-entry control-flow reproduction gives:

• registry cleared: yes
• cancellation during cleanup of A: yes
• cleanup completed: none
• cleanup of B started: no

A settled-owner batch preserving the current sequential order cleans A and B first, then re-propagates the caller's cancellation.

This means the shared helper should own an arbitrary cleanup operation, so finalizers can settle one terminate call and pty_terminate_all() can settle a sequential cleanup_all() coroutine.

Backend scope verified on main

At 89c02c828ee8510fe9a84ee6675608193aa13b02:

• Unix local: non-TTY finished finalization has awaitful teardown; TTY schedules fd close and returns without the same post-pop await.
• Docker: awaitful teardown.
• Blaxel: awaitful reader/WebSocket/aiohttp.ClientSession teardown; clearest concrete resource leak.
• Cloudflare: awaitful WebSocket/pump teardown.
• Daytona: awaitful provider session + worker teardown.
• E2B: a finished entry with an exit code returns immediately from _terminate_pty_entry; do not claim that finished-finalizer path is exposed by this exact mechanism.
• Modal: awaitful process/read-task teardown.

pty_terminate_all() is broader: because it can include running entries, all seven batch implementations need cancellation-settled ownership after the registry is cleared.

Candidate implementation contract

A narrow helper can mirror the repository's existing mount_lifecycle._settle_mount_transition pattern:

async def settle_pty_cleanup(operation):
    task = asyncio.create_task(operation, name="agents.pty_cleanup")
    completion = asyncio.create_task(asyncio.wait((task,)))
    caller_cancelled = False

    while not completion.done():
        try:
            await asyncio.shield(completion)
        except asyncio.CancelledError:
            caller_cancelled = True

    completion.result()
    task.result()

    if caller_cancelled:
        raise asyncio.CancelledError()

Local reference checks cover normal completion, single cancellation, repeated cancellation, cleanup-exception preservation, and sequential batch cleanup under cancellation: 5/5 PASS.

This remains an implementation proposal; no upstream patch or provider integration execution is claimed.

[fscfede-beep]

I built and exercised a dependency-free reference for the ownership fix described above. The narrow helper mirrors mount_lifecycle._settle_mount_transition: create/own the cleanup task, create/own a completion waiter, loop on await asyncio.shield(completion) across repeated caller cancellation, consume task.result(), and only then re-propagate CancelledError if cancellation arrived while waiting.

Reference semantics exercised locally:

async def settle_cleanup(operation):
    task = asyncio.create_task(operation, name="agents.pty_cleanup")
    completion = asyncio.create_task(asyncio.wait((task,)))
    caller_cancelled = False

    while not completion.done():
        try:
            await asyncio.shield(completion)
        except asyncio.CancelledError:
            caller_cancelled = True

    completion.result()
    task.result()  # preserve cleanup failures

    if caller_cancelled:
        raise asyncio.CancelledError

Focused reference tests: 4/4 PASS:

1. repeated cancellation cannot abandon cleanup;
2. first cancellation during cleanup is re-propagated only after cleanup finishes;
3. cleanup exceptions are not lost even if the caller was cancelled;
4. no-cancellation path returns normally.

For finalizers the integration remains narrow: after registry removal, replace the bare await self._terminate_pty_entry(removed) with an owned/settled cleanup await. In #4745's newer tail path, keep the current try/finally and settle only the terminate call inside finally.

I have not run provider integration suites for this issue and am not claiming an upstream patch is applied. This is a control-flow reference to make the intended cancellation semantics concrete.

[fscfede-beep]

Post-filing duplicate check: the closest related issue I found is #4744, which covers UTF-8 corruption across PTY collection windows and the tail/finalization machinery added in #4745. It does not cover this issue's pre-existing ownership boundary on main: registry removal followed by an awaitable backend teardown that caller cancellation can abandon. Searches for PTY teardown/cancellation, terminate/cleanup, registry-removal cancellation, and Blaxel WebSocket/aiohttp cleanup did not surface another issue for that lifecycle ownership case. Keeping #4747 separate therefore appears appropriate.

[fscfede-beep]

I audited the exact main teardown paths at 89c02c828ee8510fe9a84ee6675608193aa13b02 to bound this issue instead of assuming every backend is equally exposed.

backend	finished-entry teardown after registry removal	cancellation exposure on current main
Unix local	TTY path returns after scheduling fd close; non-TTY awaits pump/wait-task gathers	non-TTY exposed; TTY finalizer path has no comparable await after pop
Docker	awaits exit refresh, kill/rm, thread join, wait-task gather	exposed
Blaxel	awaits reader cancellation, WebSocket close, aiohttp.ClientSession.close()	concretely exposed
Cloudflare	awaits WebSocket close and pump-task completion	exposed
Daytona	awaits kill/delete session and worker-task settlement	exposed
E2B	_terminate_pty_entry() returns immediately when the finished entry already has an exit code	finished-finalizer path not exposed by this mechanism; running-entry cleanup is separate
Modal	awaits terminate/write-EOF calls and read-task gather	exposed

That makes the implementation scope narrower than "change all seven blindly": a shared settled-cleanup helper is still useful, but tests should target at least one concrete resource backend (Blaxel) plus one generic/fake finalizer, and E2B should not be claimed as a reproduced finished-entry leak.

I also collision-audited the repository with the issue terms (PTY teardown cancellation registry removal, terminate_pty_entry cancellation, pty_terminate_all cancellation cleanup). I found no separate issue or PR implementing this ownership fix; #4745 only references it from the review history. So #4747 remains a clean follow-up target at this check.

[fscfede-beep]

There is a second direct manifestation of the same ownership bug on current main: every pty_terminate_all() implementation clears its registry first and then awaits entry teardown sequentially. If the caller is cancelled during the first await, the current entry and every later entry are already unreachable from the registry and the remaining cleanups never start.

All seven backends have the same batch shape:

async with self._pty_lock:
    entries = list(registry.values())
    registry.clear()
    reserved_ids.clear()

for entry in entries:
    await self._terminate_pty_entry(entry)

I exercised a dependency-free two-entry reproduction. Current semantics:

registry cleared: yes
cancel during cleanup of A
after cancellation: cleaned=[]
B cleanup started: no

A settled-owner batch preserving the existing sequential order gives:

registry cleared: yes
cancel during cleanup of A
cleanup continues: A, then B
caller cancellation re-propagated only after both finish

Focused repro result: V44_PTY_TERMINATE_ALL_CANCELLATION_REPRO=PASS (PASS means the test deterministically demonstrates the current failure and the reference semantics).

This suggests the helper should support an owned cleanup operation, not just a single entry. The narrow integration can keep backend ordering unchanged:

async def cleanup_all() -> None:
    for entry in entries:
        await self._terminate_pty_entry(entry)

await settle_pty_cleanup(cleanup_all())

and finalizers can call the same helper with one terminate coroutine after pop. That fixes both ownership boundaries with one cancellation contract and avoids spawning unowned per-entry shield tasks.

[fscfede-beep]

A third direct manifestation of the same ownership bug exists on current main: PTY pruning removes the old entry from the registry before awaiting its teardown. I checked all seven backends at 89c02c828ee8510fe9a84ee6675608193aa13b02; each has the same shape (_prune_pty_*() pops under _pty_lock, then the caller later does await self._terminate_pty_entry(pruned...)). Cancellation during that await leaves the pruned entry unreachable just like the finalizer and pty_terminate_all() cases.

I exercised a deterministic reference:

current_registry_removed=True
current_pruned_entry_cleaned=False
reference_registry_removed=True
reference_pruned_entry_cleaned=True
V45_PTY_PRUNE_CANCELLATION_REPRO=PASS

So one shared settled-cleanup contract can cover all three ownership transfers:

# finalizer after pop
await settle_pty_cleanup(self._terminate_pty_entry(removed))

# pruning after pop
await settle_pty_cleanup(self._terminate_pty_entry(pruned_entry))

# terminate-all after registry clear
async def cleanup_all() -> None:
    for entry in entries:
        await self._terminate_pty_entry(entry)
await settle_pty_cleanup(cleanup_all())

The helper can keep the existing _settle_mount_transition semantics: own the cleanup task plus a completion waiter, survive repeated caller cancellation with asyncio.shield(completion), consume the cleanup result, then re-propagate caller cancellation. This avoids unowned shield tasks and preserves cleanup exceptions.

I also checked the active #4738 branch: it retains this pre-existing prune/finalizer/batch ownership shape, so #4747 is orthogonal and should not block that UTF-8 PR; merging #4738 would simply leave this lifecycle follow-up open. I am intentionally not expanding this comment to never-registered startup cleanup yet—that is adjacent but deserves its own source-by-source audit before widening the patch.

[fscfede-beep]

Follow-up on the startup boundary I deliberately excluded above: the source-by-source audit is complete and the direct single-cancellation gaps are distinct enough to track separately as #4749. #4749 is limited to UnixLocal + Blaxel pre-registration ownership; Docker/Cloudflare/Daytona/E2B/Modal already have explicit/BaseException startup cleanup paths and are not included in that direct claim. Keeping #4747 focused on ownership after registry removal/clear/prune avoids turning one fix into an unbounded PTY cancellation umbrella.

[fscfede-beep]

I folded the prune path into the same ownership model and exercised the combined contract locally. Exact-main audit confirms the same post-removal pattern across all seven prune paths: the pruned entry leaves the registry first and its teardown is awaited afterward. A V4 reference now covers three ownership transfers with one cancellation-settled helper: (1) awaitful finished-entry finalizers after pop, (2) prune-after-pop across all seven backends, and (3) sequential pty_terminate_all() after registry clear. Focused semantic suite: 6/6 PASS (normal, single cancel, repeated cancel, cleanup exception, two-entry batch cancellation, pruned-entry cancellation). Python compile PASS. I am not claiming an applied upstream patch, provider integration execution, full suite, or maintainer acceptance. The important design constraint is to preserve existing provider teardown ordering and settle the owned cleanup operation rather than spawning unowned per-entry shields.

[mikemikimike]

Implemented and opened PR #4751.

The fix adds an internal cleanup settlement path that keeps PTY teardown running after registry ownership is removed, tolerates repeated cancellation from the caller, then restores cancellation after cleanup completes. It is applied to prune, finalizer, and pty_terminate_all paths across the seven PTY backends. Startup cleanup for entries that were never registered remains unchanged.

Validation: the new deterministic regression tests pass (2 passed), Ruff passes, and targeted Pyright reports no errors. Full local collection is limited by this Windows environment's missing optional dependencies.

[fscfede-beep]

Independent source audit of implementation PR #4751 at current head 8fc7aab2103ea0cb9e28a45009a9e85709b7bb5c:

• _settle_pty_cleanup() matches the cancellation-settled ownership pattern discussed here: owned cleanup task + completion waiter, repeated caller cancellation tolerated, cleanup result/exception consumed, then caller cancellation re-propagated.
• All 7 PTY backends now route prune-after-pop cleanup through the helper.
• All 7 route finished-entry finalizer cleanup through the helper.
• All 7 now settle one sequential cleanup_all() batch after pty_terminate_all() clears its registry. This addresses Codex's first-head P1 that per-entry settlement would still unwind before later entries; the batch correction is commit 8fc7aab2103.
• The base-session regression suite now includes cancellation settlement, cleanup-exception preservation, and sequential batch settlement. The author reports 3 targeted tests PASS, Ruff PASS, targeted Pyright 0 errors, and git diff --check PASS.

I have not independently executed the upstream/provider suite for this head, and the repository Actions run currently exposes action_required with 0 jobs, so I am not treating CI as PASS or the issue as upstream-accepted yet. Source-wise, though, the three ownership transfers reported in #4747 (finalizer, prune, terminate-all) are represented in the current PR candidate.

Scope note: #4751 applies the finalizer helper somewhat more broadly than the minimal exposure matrix in the issue (including E2B / Unix TTY paths). I did not find a source-level correctness regression from that broader use, but provider validation remains an upstream gate.

[fscfede-beep]

Current implementation follow-up on PR #4751, now at 9ce15f766d710695b1e93b48a7e255b3551662ca:

• The requested representative Blaxel registry/resource regression is now present in commit 3e9f7270887. By source inspection it uses the existing _FakeWS, _FakeHTTPSession, _make_session, and _BlaxelPtySessionEntry, blocks the first real backend ws.close(), confirms the registry/reserved IDs are already cleared, injects repeated caller cancellation, then requires both WebSocket and HTTP resources to settle before CancelledError escapes.
• The author reports Ruff + git diff --check PASS for that change, but targeted pytest could not start locally because dependency sync hit network retries. I therefore classify the backend wiring regression as implemented at source / not runtime-proven upstream.
• Commit 9ce15f766d additionally preserves the original cancellation reason across settlement. I exercised the exact helper semantics independently: repeated cancellation preserves the first caller reason, cleanup exceptions still take precedence, and a two-entry settled batch completes both entries before cancellation propagates.
• A fresh Codex review on 9ce15f766d reports no major issues (top-level PR comment 5461021149).

Current remaining gate is execution/maintainer evidence, not a known source-contract mismatch: PR #4751 is still open/not merged; Actions for the head remain action_required with 0 jobs/check-runs exposed through the API, so I am not treating CI or upstream acceptance as PASS.