fix(sandbox): clean up PTY startup cancellation - #4750
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 26ffd7de91
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except asyncio.CancelledError: | ||
| continue |
There was a problem hiding this comment.
When a Blaxel startup timeout or transport failure has entered a slow cleanup and the caller then cancels pty_exec_start(), asyncio.shield() raises CancelledError here, but the loop discards it; after cleanup, the caller raises the preexisting ExecTimeoutError or ExecTransportError, leaving a task with a nonzero cancellation count that is not cancelled. Remember cancellation observed while settling and re-raise it after cleanup so shutdown and structured-concurrency callers receive the requested cancellation.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5cd36bf and 2720f1a. _settle_pty_cleanup now records the first caller cancellation while cleanup is shielded and re-raises it after cleanup completes, including when cleanup itself fails. Added test_pty_exec_start_preserves_cancellation_during_cleanup, which verifies the caller receives CancelledError and the task is cancelled. The focused sandbox suite passes: 265 passed, 2 skipped; Ruff, mypy, and pyright also pass.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
|
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. |
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Current head fixes the reported startup ownership bug by inspection and the Codex P2 is addressed. One regression boundary is still worth pinning.
|
|
||
| class TestPtyExec: | ||
| @pytest.mark.asyncio | ||
| async def test_pty_exec_start_cancellation_closes_unregistered_http_session( |
There was a problem hiding this comment.
Coverage gap, not a runtime blocker: this cancels inside ws_connect, before the WebSocket and reader task are owned. #4749 also covers cancellation after connect while command send / registry transfer is still pending. A deterministic fake can let ws_connect return, start a blocking reader, block send_str, cancel there, then assert reader cancellation + WebSocket close + HTTP-session close + empty registry. That pins the stronger pre-registration resource state and guards against a future narrowing of the cleanup handler.
There was a problem hiding this comment.
Agreed this is a useful coverage enhancement and not a runtime blocker. I kept it out of this focused fix because the reported issue is the cleanup exception/cancellation precedence; the existing related scenarios cover the other ownership timing. I can add the broader pre-registration send-path coverage separately if maintainers want it.
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
I found one remaining cancellation-precedence defect in the current helper.
| cancellation: asyncio.CancelledError | None = None | ||
| while not cleanup_task.done(): | ||
| try: | ||
| await asyncio.shield(cleanup_task) |
There was a problem hiding this comment.
Cancellation is still lost if cleanup itself fails after the caller cancellation was caught. Deterministic ordering: cleanup sets started then waits; caller cancels pty_exec_start; this loop catches CancelledError, records it, and re-enters await shield(cleanup_task); release cleanup and have it raise RuntimeError. That second shield() propagates the cleanup RuntimeError directly, so execution never reaches the cleanup_task.result() block below that is supposed to give the recorded cancellation priority. Result: caller sees RuntimeError, not CancelledError. A separate completion waiter (the pattern used by mount_lifecycle._settle_mount_transition, and also in #4751) avoids letting the cleanup task's exception escape the shield wait; alternatively catch inner-task exceptions here and defer them to cleanup_task.result(). Please add a regression with caller cancellation followed by a failing cleanup.
There was a problem hiding this comment.
I ran the exact helper control-flow locally with caller cancellation delivered while cleanup was blocked, then released cleanup with RuntimeError. Result:
current_p4750_outcome=RuntimeError
current_p4750_cleanup_terminal=True
reference_outcome=CancelledError
reference_cleanup_terminal=True
V51_P4750_FAILURE_AFTER_CANCEL_REPRO=PASS
The reference differs only by shielding a separate asyncio.wait((cleanup_task,)) completion waiter, so the inner cleanup exception is inspected after settlement instead of escaping the second shield() await. This confirms the remaining issue is exception/cancellation precedence, not cleanup abandonment.
There was a problem hiding this comment.
Fixed in 9a94bd7. _settle_pty_cleanup now waits on a separate completion task created from asyncio.wait((cleanup_task,)), instead of shielding the cleanup task directly. This prevents a cleanup exception from escaping the post-cancellation wait; after settlement, cleanup_task.result() is inspected and the caller's cancellation takes priority.
Added test_pty_exec_start_preserves_cancellation_when_cleanup_fails, which cancels the caller while close() is blocked and then makes cleanup raise RuntimeError. The caller now receives CancelledError. The relevant Blaxel and UnixLocal test files pass with 260 passed and 8 skipped; Ruff, formatting, and Pyright also pass.
There was a problem hiding this comment.
The reported reproduction was valid and is fixed in 9a94bd7. The separate completion waiter prevents the cleanup task's RuntimeError from escaping the shield, so the original caller cancellation is preserved. The new cleanup-failure regression covers this exact ordering.
|
@codex review |
|
@codex security review |
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
The completion-waiter fix resolves the failure-after-cancel interleaving from my previous review. One smaller cancellation-propagation defect remains on the new head.
| cleanup_task.result() | ||
| except BaseException: | ||
| if caller_cancelled: | ||
| raise asyncio.CancelledError() from None |
There was a problem hiding this comment.
task.cancel("reason") loses its cancellation payload here because the caught CancelledError is reduced to a boolean and a fresh empty CancelledError() is raised after cleanup. Deterministic repro on this exact helper: cancel with "route-A" while cleanup is blocked, let cleanup complete, then inspect the awaited exception; current exc.args == (), while retaining and re-raising the first caught exception gives ("route-A",). This is the same propagation invariant Codex just flagged on #4751. Please retain the first CancelledError object (or its args) and re-raise it after settlement, including the cleanup-failure branch.
There was a problem hiding this comment.
Fixed in cbaa66e.
_settle_pty_cleanup() now retains the first CancelledError object instead of reducing cancellation to a boolean, so task.cancel("route-A") keeps its payload. The retained cancellation is re-raised after cleanup settlement, including when cleanup fails, so a secondary cleanup exception cannot replace the caller cancellation.
Added regressions for both cancellation-during-cleanup and cleanup-failure paths. The affected tests pass, along with Ruff and Pyright.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a94bd7b23
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cleanup_task.result() | ||
| except BaseException: | ||
| if caller_cancelled: | ||
| raise asyncio.CancelledError() from None | ||
| raise |
There was a problem hiding this comment.
When cancellation enters UnixLocal's new except BaseException while waiting for _pty_lock, caller_cancelled is initialized only after that cancellation has already been caught. If _terminate_pty_entry() then fails—for example, its unsuppressed os.killpg() raises PermissionError—cleanup_task.result() propagates the cleanup failure, the outer bare raise is never reached, and the task finishes non-cancelled despite retaining a cancellation count. Fresh evidence beyond the resolved thread is this initial-cancellation path: the added cleanup-failure test instead begins with TimeoutError and cancels only after cleanup starts. Preserve the original cancellation when entering the helper, including when cleanup fails.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed with an exact control-flow repro for the initial-cancellation path. Cancel the startup task with "startup-cancel", enter the current helper from except CancelledError, let cleanup block and then raise PermissionError. Current result is ('PermissionError', ('killpg denied',), cancelling=1, cancelled=False); a reference that seeds the helper with the original caught cancellation returns CancelledError('startup-cancel') and cancelled=True. So the missing state is the cancellation that existed before helper entry. A narrow repair is to pass the caught CancelledError into the settlement helper (while still retaining any later cancellation), and give that stored cancellation priority after cleanup settlement/failure.
There was a problem hiding this comment.
I reconciled this P2 with the payload-loss finding in 3885911655; they have the same missing state. The narrow fix is to let _settle_pty_cleanup() accept the already-caught CancelledError as initial_cancellation, retain the first actual cancellation object (not a bool), and re-raise that object after cleanup settlement/failure. Blaxel should pass the exception from its explicit except asyncio.CancelledError as e; UnixLocal should split CancelledError from the broader BaseException handler so it can do the same. This simultaneously preserves task.cancel("reason") args and prevents an unsuppressed cleanup failure (e.g. killpg PermissionError) from replacing the cancellation that initiated cleanup. I exercised both interleavings dependency-free and reviewed the candidate helper independently; no new issue was found. A regression for UnixLocal can hold _pty_lock, cancel with "startup-cancel", make killpg raise PermissionError, and assert the awaited task still raises CancelledError("startup-cancel") and is marked cancelled.
There was a problem hiding this comment.
Fixed in cbaa66e.
UnixLocal now handles CancelledError separately from the broader BaseException path and passes the already-caught cancellation into _settle_pty_cleanup(). Blaxel does the same in its explicit cancellation handler. This preserves an initial task.cancel("startup-cancel") even if cleanup later fails.
Added a UnixLocal caller-boundary regression that holds the PTY lock, cancels with startup-cancel, makes killpg() raise PermissionError, and verifies the awaited task still raises CancelledError("startup-cancel"). The affected tests pass: 266 passed, 8 skipped; Ruff and Pyright are clean.
|
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. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
|
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. |
2 participants
Summary
Fixes the PTY startup ownership boundary described in #4749. UnixLocal and Blaxel can create processes/provider resources before registering their entries; caller cancellation in that window previously bypassed cleanup and left the resources unreachable from the session registries.
The change cleanup-settles unregistered PTY entries before re-raising cancellation, and makes the UnixLocal TTY descriptor cleanup cover cancellation during subprocess creation.
Test plan
pytest -q tests/sandbox/test_unix_local.py tests/extensions/sandbox/test_blaxel.py— 264 passed, 2 skippedruff checkon all changed source and test files — passedruff format --checkon all changed source and test files — passedmypyon all changed source files — passedpyrighton all changed source and test files — passedgit diff --check— passedThe repository verification wrapper was also attempted. In this local environment it stops before lint/typecheck/tests because the installed uv cannot parse the repository's
exclude-newer = "7 days"setting and, in locked mode, requests a lockfile rewrite. The lockfile was not changed. Running the equivalent checks directly with the existing virtual environment produced the results above; the full repository run also exposed unrelated baseline optional-dependency/type-check failures outside this change.Issue number
Closes #4749
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR