fix(sandbox): settle PTY cleanup before cancellation - #4751
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffa2214289
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| for entry in entries: | ||
| await self._terminate_pty_entry(entry) | ||
| await self._settle_pty_cleanup(self._terminate_pty_entry(entry)) |
There was a problem hiding this comment.
When multiple PTYs are registered and cancellation arrives while the first cleanup is pending, _settle_pty_cleanup finishes that entry and then re-raises CancelledError, unwinding this loop before subsequent entries are processed. Because the registry was already cleared, those remaining subprocesses or WebSockets have no owner, and retrying pty_terminate_all() cannot clean them. The same per-entry pattern occurs in all seven modified backends; settle the entire batch before propagating cancellation instead.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 8fc7aab2103ea0cb9e28a45009a9e85709b7bb5c.
pty_terminate_all() now collects the entries into a single cleanup_all() coroutine and settles that batch through _settle_pty_cleanup(). This keeps cancellation from unwinding the loop after the first entry, so all registered PTYs are cleaned up before cancellation is re-propagated. The same pattern is applied to all seven PTY backends.
Added regression coverage in tests/sandbox/test_base_sandbox_session.py for sequential batch cleanup after cancellation. Verified with:
uv run --frozen --no-sync pytest -p pytest_asyncio.plugin tests/sandbox/test_base_sandbox_session.py -q— 3 passeduv run --frozen --no-sync ruff check src/agents/sandbox/session/base_sandbox_session.py tests/sandbox/test_base_sandbox_session.py— passeduv run --frozen --no-sync pyright src/agents/sandbox/session/base_sandbox_session.py— 0 errorsgit diff --check upstream/main...HEAD— passed
|
@codex review Please review the current head |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
The batch-settlement fix now matches the #4747 ownership contract by inspection, and the fresh Codex review found no major issue. I found one test-evidence gap.
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_pty_cleanup_settles_a_sequential_batch() -> None: |
There was a problem hiding this comment.
These tests exercise _settle_pty_cleanup directly, but #4747 exists at the registry-ownership transfer. Please add one representative backend-level regression (Blaxel is the clearest): register entries, start pty_terminate_all() or a finalizer, wait until the registry has been cleared/popped and teardown is blocked, cancel the caller, release teardown, then assert cleanup completed for the resource(s) and the final registry state stays empty. That verifies the call-site wiring and shared-state invariant, not only the helper semantics.
There was a problem hiding this comment.
Concrete Blaxel regression shape against the existing test fakes, so this can stay provider-free:
@pytest.mark.asyncio
async def test_pty_terminate_all_settles_after_registry_clear(
fake_sandbox: _FakeSandboxInstance,
) -> None:
from agents.extensions.sandbox.blaxel.sandbox import _BlaxelPtySessionEntry
session = _make_session(fake_sandbox)
close_started = asyncio.Event()
release_close = asyncio.Event()
class _BlockingCloseWS(_FakeWS):
async def close(self) -> None:
close_started.set()
await release_close.wait()
self._closed = True
first_ws = _BlockingCloseWS()
second_ws = _FakeWS()
first_http = _FakeHTTPSession(first_ws)
second_http = _FakeHTTPSession(second_ws)
first = _BlaxelPtySessionEntry("first", first_ws, first_http)
second = _BlaxelPtySessionEntry("second", second_ws, second_http)
session._pty_sessions.update({1: first, 2: second})
session._reserved_pty_process_ids.update({1, 2})
task = asyncio.create_task(session.pty_terminate_all())
await close_started.wait()
# Ownership has already left the registry before cleanup finishes.
assert session._pty_sessions == {}
assert session._reserved_pty_process_ids == set()
task.cancel()
await asyncio.sleep(0)
task.cancel() # exercise repeated caller cancellation
await asyncio.sleep(0)
assert not first_ws._closed
assert not second_ws._closed
release_close.set()
with pytest.raises(asyncio.CancelledError):
await task
assert first_ws._closed and first_http._closed
assert second_ws._closed and second_http._closed
assert session._pty_sessions == {}This uses the existing _FakeWS, _FakeHTTPSession, _make_session, and Blaxel entry type, and directly proves the registry-clear → blocked real backend teardown → repeated cancellation → all resources settled invariant that the shared-helper tests cannot cover. I have not executed it inside the upstream checkout, so treat it as a precise regression proposal rather than a reported test PASS.
There was a problem hiding this comment.
Added the Blaxel-specific regression test in commit 3e9f7270. It covers registry clearing before blocked backend teardown, repeated caller cancellation, and settlement of both WebSocket and HTTP resources after release. ruff check tests/extensions/sandbox/test_blaxel.py and git diff --check pass; the targeted pytest could not start locally because the locked dependency sync hit network retries, while the host OpenAI package is incompatible with this checkout.
There was a problem hiding this comment.
The backend-level regression is now present at 3e9f7270 and matches the requested registry-clear → blocked teardown → repeated cancellation → both resources settled shape by inspection. The author could not execute the targeted pytest because of local dependency/network constraints, so I am treating the wiring coverage as implemented but not claiming an upstream test PASS.
There was a problem hiding this comment.
Retested locally after preparing the locked environment: uv run --frozen --no-sync pytest tests/sandbox/test_base_sandbox_session.py tests/extensions/sandbox/test_blaxel.py -k 'pty_cleanup or pty_terminate_all' -q passes with 6 passed and 247 deselected. The environment was prepared with uv sync --frozen --extra blaxel; ruff check and git diff --check also pass. The cancellation-reason regression is covered by the updated base-session test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e9f727088
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if caller_cancelled: | ||
| raise asyncio.CancelledError() |
There was a problem hiding this comment.
When a caller uses task.cancel("reason") while PTY cleanup is pending, the caught CancelledError is discarded and this fresh exception is raised after cleanup, so the awaiting caller receives a cancellation with empty args. This breaks normal asyncio cancellation propagation and removes information used by cancellation diagnostics or routing; retain and re-raise the original cancellation exception after the cleanup task settles.
Useful? React with 👍 / 👎.
Signed-off-by: mikemikimike <13286568797@163.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68036ee68d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| with pytest.raises(asyncio.CancelledError) as exc_info: | ||
| await task | ||
| assert completed | ||
| assert exc_info.value.args == ("cleanup requested",) |
There was a problem hiding this comment.
On CPython 3.10, a cancellation message is not propagated from a cancelled Task to its awaiter, so this assertion receives () even though _settle_pty_cleanup re-raises the caught exception. Fresh evidence is a direct reproduction with the repository-supported CPython 3.10.20, which produced empty arguments for both single and repeated cancel(message) calls; consequently, both new message assertions fail in the Python 3.10 test matrix. Conditionalize this expectation or inspect the exception inside the cancelled task instead.
AGENTS.md reference: AGENTS.md:L211-L211
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0708d65. The cancellation-message assertions are now guarded for Python 3.11+, while the cleanup behavior remains covered on Python 3.10. The focused sandbox tests pass on the current runtime.
| assert not first_ws._closed | ||
| assert not second_ws._closed | ||
|
|
||
| release_close.set() |
There was a problem hiding this comment.
If any assertion before this line fails, execution never releases _BlockingCloseWS.close(), leaving pty_terminate_all() pending. Because the new helper intentionally absorbs task cancellation until that close finishes, async teardown that cancels and drains pending tasks can then stall instead of reporting the original assertion failure; move the release and task draining into a finally block and bound the initial readiness wait.
AGENTS.md reference: AGENTS.md:L236-L236
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0708d65. The test now bounds the readiness wait and uses a finally block to release the blocking close and await the cleanup task, so assertion failures cannot strand the task or stall teardown.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0708d65538
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| completed = True | ||
|
|
||
| task = asyncio.create_task(_session()._settle_pty_cleanup(cleanup())) | ||
| await started.wait() |
There was a problem hiding this comment.
If _settle_pty_cleanup exits before invoking the supplied coroutine—the regression these tests should report—this unbounded wait never returns, while release is never set and task is never drained, so the pytest worker hangs instead of failing. The same pattern occurs at lines 70, 91, and 115; bound each readiness wait and release/cancel/drain the task in a finally block.
AGENTS.md reference: AGENTS.md:L236-L236
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 16960bd2. All four readiness waits in test_base_sandbox_session.py are now bounded with a 5-second timeout, and each test releases, cancels if needed, and drains its cleanup task in finally. Focused tests pass: 252 passed, 1 skipped; Ruff and Pyright also pass.
2 participants
Summary
PTY cleanup can be cancelled after a session removes the entry from its registry, leaving the backend resource without a registry owner. This is especially visible in finalizer cleanup and
pty_terminate_all.This change adds an internal
BaseSandboxSessioncleanup settlement helper. It runs cleanup in an owned task, tolerates repeated caller cancellation until cleanup finishes, then restores caller cancellation while preserving cleanup failures. All seven PTY backends now use it for cleanup after pruning, registry removal, or registry clearing. Startup cleanup for entries that were never registered is unchanged.Validation
uv run --frozen --no-sync ruff check— passed.sqlalchemy,httpx,numpy,litellm, anddocker).