Skip to content

fix(sandbox): settle PTY cleanup before cancellation #4751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mikemikimike wants to merge 15 commits into
base: main
Choose a base branch
from
Open

fix(sandbox): settle PTY cleanup before cancellation #4751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ffa2214
fix(sandbox): settle PTY cleanup before cancellation
mikemikimike Aug 29, 2026
8fc7aab
fix(sandbox): settle PTY cleanup batches
mikemikimike Aug 29, 2026
3e9f727
test(sandbox): cover Blaxel PTY batch cleanup cancellation
mikemikimike Aug 29, 2026
9ce15f7
fix(sandbox): preserve PTY cleanup cancellation reasons
mikemikimike Aug 29, 2026
68036ee
fix(sandbox): preserve PTY cleanup cancellation reason
mikemikimike Aug 29, 2026
0708d65
test(sandbox): address PTY cleanup review feedback
mikemikimike Aug 29, 2026
16960bd
test(sandbox): bound cleanup readiness waits
mikemikimike Aug 29, 2026
3fb9e81
fix(sandbox): settle every PTY cleanup after failure
mikemikimike Sep 3, 2026
10d6f2d
test(sandbox): keep PTY cleanup regression isolated
mikemikimike Sep 3, 2026
54a942d
fix(sandbox): bound Docker PTY cleanup waits
mikemikimike Sep 7, 2026
3678c63
fix(sandbox): bound PTY cleanup settlement
mikemikimike Sep 8, 2026
f5653ef
fix(sandbox): preserve PTY cleanup ownership
mikemikimike Sep 8, 2026
b684ea7
:bug: make PTY cleanup and snapshot fallback bounded
mikemikimike Sep 9, 2026
9d74e54
fix sandbox cleanup rollback and preservation
mikemikimike Sep 9, 2026
475d351
fix(sandbox): drain detached cleanup before closing dependencies
mikemikimike Sep 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/agents/extensions/sandbox/blaxel/sandbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,18 @@ async def pty_exec_start(
raise _blaxel_exec_transport_error(command=command, cause=e) from e

if pruned is not None:
await self._terminate_pty_entry(pruned)
try:
await self._settle_pty_cleanup(
self._terminate_pty_entry(pruned), propagate_timeout=True
)
except BaseException:
await self._rollback_pty_start(
process_id,
entry,
self._pty_sessions,
lambda: self._terminate_pty_entry(entry),
)
raise

if process_count >= PTY_PROCESSES_WARNING:
logger.warning(
Expand Down Expand Up @@ -910,8 +921,8 @@ async def pty_terminate_all(self) -> None:
entries = list(self._pty_sessions.values())
self._pty_sessions.clear()
self._reserved_pty_process_ids.clear()
for entry in entries:
await self._terminate_pty_entry(entry)

await self._cleanup_pty_entries(entries, self._terminate_pty_entry)

# -- PTY internals -------------------------------------------------------

Expand Down Expand Up @@ -990,7 +1001,9 @@ async def _finalize_pty_update(
removed = self._pty_sessions.pop(process_id, None)
self._reserved_pty_process_ids.discard(process_id)
if removed is not None:
await self._terminate_pty_entry(removed)
await self._settle_pty_cleanup(
self._terminate_pty_entry(removed), propagate_timeout=False
)
live_process_id = None

return PtyExecUpdate(
Expand Down
20 changes: 16 additions & 4 deletions src/agents/extensions/sandbox/cloudflare/sandbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,9 @@ async def _finalize_pty_update(
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)
await self._settle_pty_cleanup(
self._terminate_pty_entry(removed), propagate_timeout=False
)
live_process_id = None

return PtyExecUpdate(
Expand Down Expand Up @@ -1211,7 +1213,18 @@ async def pty_exec_start(
raise ExecTransportError(command=tuple(str(part) for part in command), cause=e) from e

if pruned_entry is not None:
await self._terminate_pty_entry(pruned_entry)
try:
await self._settle_pty_cleanup(
self._terminate_pty_entry(pruned_entry), propagate_timeout=True
)
except BaseException:
await self._rollback_pty_start(
process_id,
entry,
self._pty_processes,
lambda: self._terminate_pty_entry(entry),
)
raise

if process_count >= PTY_PROCESSES_WARNING:
logger.warning(
Expand Down Expand Up @@ -1275,8 +1288,7 @@ async def pty_terminate_all(self) -> None:
self._pty_processes.clear()
self._reserved_pty_process_ids.clear()

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

async def read(self, path: Path | str, *, user: str | User | None = None) -> io.IOBase:
if user is not None:
Expand Down
21 changes: 17 additions & 4 deletions src/agents/extensions/sandbox/daytona/sandbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,18 @@ async def _on_data(chunk: bytes | str) -> None:
raise

if pruned is not None:
await self._terminate_pty_entry(pruned)
try:
await self._settle_pty_cleanup(
self._terminate_pty_entry(pruned), propagate_timeout=True
)
except BaseException:
await self._rollback_pty_start(
process_id,
entry,
self._pty_sessions,
lambda: self._terminate_pty_entry(entry),
)
raise

if process_count >= PTY_PROCESSES_WARNING:
logger.warning(
Expand Down Expand Up @@ -863,7 +874,9 @@ async def _finalize_pty_update(
removed = self._pty_sessions.pop(process_id, None)
self._reserved_pty_process_ids.discard(process_id)
if removed is not None:
await self._terminate_pty_entry(removed)
await self._settle_pty_cleanup(
self._terminate_pty_entry(removed), propagate_timeout=False
)
live_process_id = None

return PtyExecUpdate(
Expand All @@ -878,8 +891,8 @@ async def pty_terminate_all(self) -> None:
entries = list(self._pty_sessions.values())
self._pty_sessions.clear()
self._reserved_pty_process_ids.clear()
for entry in entries:
await self._terminate_pty_entry(entry)

await self._cleanup_pty_entries(entries, self._terminate_pty_entry)

async def _collect_pty_output(
self,
Expand Down
20 changes: 16 additions & 4 deletions src/agents/extensions/sandbox/e2b/sandbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1041,7 +1041,18 @@ async def _append_output(payload: bytes | bytearray | str | object) -> None:
)

if pruned_entry is not None:
await self._terminate_pty_entry(pruned_entry)
try:
await self._settle_pty_cleanup(
self._terminate_pty_entry(pruned_entry), propagate_timeout=True
)
except BaseException:
await self._rollback_pty_start(
process_id,
entry,
self._pty_processes,
lambda: self._terminate_pty_entry(entry),
)
raise

if process_count >= PTY_PROCESSES_WARNING:
logger.warning(
Expand Down Expand Up @@ -1108,8 +1119,7 @@ async def pty_terminate_all(self) -> None:
self._pty_processes.clear()
self._reserved_pty_process_ids.clear()

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

async def read(self, path: Path, *, user: str | User | None = None) -> io.IOBase:
if user is not None:
Expand Down Expand Up @@ -1277,7 +1287,9 @@ async def _finalize_pty_update(
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)
await self._settle_pty_cleanup(
self._terminate_pty_entry(removed), propagate_timeout=False
)
live_process_id = None

return PtyExecUpdate(
Expand Down
20 changes: 16 additions & 4 deletions src/agents/extensions/sandbox/modal/sandbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,18 @@ async def pty_exec_start(
raise _modal_exec_transport_error(command=command, cause=e) from e

if pruned_entry is not None:
await self._terminate_pty_entry(pruned_entry)
try:
await self._settle_pty_cleanup(
self._terminate_pty_entry(pruned_entry), propagate_timeout=True
)
except BaseException:
await self._rollback_pty_start(
process_id,
entry,
self._pty_processes,
lambda: self._terminate_pty_entry(entry),
)
raise

if process_count >= PTY_PROCESSES_WARNING:
logger.warning(
Expand Down Expand Up @@ -961,8 +972,7 @@ async def pty_terminate_all(self) -> None:
self._pty_processes.clear()
self._reserved_pty_process_ids.clear()

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

async def _write_pty_stdin(self, process: ContainerProcess[bytes], payload: bytes) -> None:
stdin = process.stdin
Expand Down Expand Up @@ -1118,7 +1128,9 @@ async def _finalize_pty_update(
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)
await self._settle_pty_cleanup(
self._terminate_pty_entry(removed), propagate_timeout=False
)
live_process_id = None

return PtyExecUpdate(
Expand Down
26 changes: 17 additions & 9 deletions src/agents/sandbox/runtime_session_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,24 +95,32 @@ async def cleanup(self) -> None:
except BaseException as exc: # pragma: no cover
if cleanup_error is None:
cleanup_error = exc
preserve_backend = (
isinstance(self._session, SandboxSession)
and self._session._should_preserve_backend_on_cleanup()
)
if not preserve_backend:
Comment on lines +98 to +102

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recheck preservation after detached snapshots finish

When fallback snapshot persistence exceeds cleanup_s but later succeeds, stop() leaves _backend_preservation_required set and this early decision skips shutdown permanently; _aclose_dependencies() then waits for that same upload to finish, but cleanup never reevaluates the now-durable snapshot before clearing its resources. This leaks a runner-owned remote backend—and can keep it billable—even though the policy already permits deletion when fallback persistence succeeds within the deadline; defer or revisit this decision after the tracked snapshot settles.

AGENTS.md reference: AGENTS.md:L149-L150

Useful? React with 👍 / 👎.

try:
await self._session.shutdown()
except BaseException as exc: # pragma: no cover
if cleanup_error is None:
cleanup_error = exc
try:
await self._session.shutdown()
if (
self._client is not None
and isinstance(self._session, SandboxSession)
and not self._session._should_preserve_backend_on_cleanup()
):
await self._client.delete(self._session)
except BaseException as exc: # pragma: no cover
if cleanup_error is None:
cleanup_error = exc
finally:
try:
if self._client is not None and isinstance(self._session, SandboxSession):
await self._client.delete(self._session)
await self._session._aclose_dependencies()
except BaseException as exc: # pragma: no cover
if cleanup_error is None:
cleanup_error = exc
finally:
try:
await self._session._aclose_dependencies()
except BaseException as exc: # pragma: no cover
if cleanup_error is None:
cleanup_error = exc
if cleanup_error is not None:
raise cleanup_error

Expand Down
Loading