fix(sandbox): settle PTY cleanup before cancellation - #4751
mikemikimike wants to merge 9 commits into
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".
This comment was marked as off-topic.
This comment was marked as off-topic.
|
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". |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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".
|
@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".
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".
|
当前 head 已在 0708d65 保留调用方 cancellation reason，并补充版本兼容、资源清理和聚焦测试覆盖。关闭这些已处理线程。 |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
pty_terminate_all() clears the registry before cleanup_all() runs, but cleanup_all() stops on the first _terminate_pty_entry exception. Any later PTYs are then left alive with no registry owner and cannot be retried. Could we attempt cleanup for every entry, preserve/re-raise the first error afterward, and add a first-cleanup-fails regression?
|
The new review is valid against current head A fail-closed shape that preserves the existing cancellation settlement contract is: async def cleanup_all() -> None:
first_error: BaseException | None = None
for entry in entries:
try:
await self._terminate_pty_entry(entry)
except BaseException as error:
if first_error is None:
first_error = error
if first_error is not None:
raise first_errorThen keep I have verified this is still required by inspection of the current PR diff; I have not executed the upstream test matrix or modified the author's branch. |
|
Addressed in commit 10d6f2d. PTY batch cleanup now attempts every detached entry, re-raises the first cleanup failure after the batch, and all seven providers use the shared typed helper for prune, finalizer, and registry-clear cleanup. The adjacent unregistered-startup changes were removed to keep this PR focused on #4747. Added base-session, Blaxel registry-clear cancellation, and Blaxel first-cleanup-fails regressions. Validation: focused sandbox tests 254 passed, 1 skipped; mypy 308 source files passed; Pyright passed; Ruff format/lint and git diff --check passed. The full local suite remains limited by unrelated Windows symlink privileges, MCP 502, skills workspace state, and example encoding issues. No GitHub checks are currently reported for this branch. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the update. There is still a shutdown-liveness issue in the Docker path.
_settle_pty_cleanup() suppresses caller cancellation until cleanup finishes, but Docker's _terminate_pty_entry() awaits kill/PID-file cleanup through exec_run() without a deadline. If that attached exec stream stalls, cancellation no longer releases the caller, the entry socket is never closed, and later entries in the cleared batch are never attempted. An outer asyncio.wait_for() also waits for cancellation to finish.
Please bound the Docker backend waits and ensure local resource cleanup and subsequent batch entries still run when that deadline expires. Add a regression through the Docker cleanup path with a stalled backend operation, asserting bounded completion and resource cleanup. The existing cancellation tests always release the blocked operation, so they do not cover this case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54a942db5f
ℹ️ 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".
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
🛡️ Codex Security Review · Automatically triggeredSecurity 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: 725c7e796a
ℹ️ 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 not self._cleanup_finished: | ||
| return False |
There was a problem hiding this comment.
When a run hands off across at least two SandboxAgents and an earlier resource cleanup marks its backend for preservation—for example, after a stop, snapshot, or delete failure—a later resource reaches _wait_before_backend_delete() while the manager is still iterating. Because the preserved session prevents the early success return and _cleanup_finished is only set after that loop, this callback returns False forever; the later cleanup spins, so Runner.run() never finishes cleanup. Permit this in-loop delete checkpoint to proceed, or publish the preserved state before awaiting the next resource.
AGENTS.md reference: AGENTS.md:L117-L118
Useful? React with 👍 / 👎.
| self._session_manager.register_resume_state_observer( | ||
| lambda resume_state: setattr(result, "_sandbox_resume_state", resume_state) | ||
| ) |
There was a problem hiding this comment.
When cleanup returns while a fallback snapshot is still pending, a caller can immediately use the documented result.to_state() API, which deep-copies the current _sandbox_resume_state. This observer updates only the result after the late snapshot succeeds; the already emitted RunState keeps the previous snapshot and backend reference even though deferred cleanup can then delete that backend, so resuming the checkpoint can restore stale workspace contents. Fresh evidence beyond the earlier late-resume-state comments is this independently copied public checkpoint carrier; either keep emitted checkpoints updated or prevent backend deletion until their recovery payload is definitive.
AGENTS.md reference: AGENTS.md:L117-L119
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4588b73cb1
ℹ️ 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".
| auto_previous_response_id=self._auto_previous_response_id, | ||
| ) | ||
| _copy_pending_nested_agent_tool_states(state, self) | ||
| self._sandbox_state_checkpoints.append(weakref.ref(state)) |
There was a problem hiding this comment.
When cleanup returns while a fallback snapshot is still pending, a caller can immediately persist result.to_state().to_json(). Registering a weak reference here lets the later observer update only the live RunState; it cannot update the already returned JSON or a copy written to storage. Deferred cleanup can then publish the newer snapshot and delete the backend, leaving that persisted checkpoint pointing to a deleted backend with stale workspace contents. Fresh evidence beyond the earlier checkpoint comment is that the new weak-reference mechanism explicitly covers only live objects; backend deletion or checkpoint exposure must wait until the recovery payload is definitive.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
| if self._preserved_backend_retry_required and any( | ||
| resources._has_retryable_open_dependencies() | ||
| for resources in self._resources_by_agent.values() | ||
| ): | ||
| return |
There was a problem hiding this comment.
When a runner-owned session has an open factory-backed snapshot dependency and both stop() and its immediate fallback snapshot fail, _preserved_backend_retry_required is set and this branch retains the session, dependencies, and SandboxAgent concurrency guard for a future manager.cleanup() retry. That retry is available only in helper-level tests: Runner.run() does not expose its manager and clears result._sandbox_session, so after returning the recovery state the manager is discarded while the guard remains permanently active. Reusing or resuming with the same public agent then fails the concurrent-run check, and the retained provider resources may remain live; transfer or release ownership at the public runner boundary instead of waiting for an inaccessible retry.
AGENTS.md reference: AGENTS.md:L118-L118
Useful? React with 👍 / 👎.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity 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: c70f7da8ca
ℹ️ 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".
| cleanup = self._sandbox_cleanup | ||
| if cleanup is None: | ||
| return | ||
| await cleanup() |
There was a problem hiding this comment.
When a streamed run is cancelled while the callback-started _run_sandbox_cleanup() is still pending, RunResultStreaming inherits this method and await result.aclose() invokes the cleanup closure a second time instead of awaiting _sandbox_cleanup_task. Both closures enqueue the memory result and enter the same SandboxRuntimeSessionManager.cleanup() concurrently; the later invocation can overwrite the first invocation's published resume state or race backend finalization. Join the existing streaming cleanup task before starting a retry.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
| _sandbox_resume_state_pending: bool = field(default=False, repr=False) | ||
| """Whether sandbox cleanup may still replace the resume payload.""" |
There was a problem hiding this comment.
The pending flag is enforced by to_state() and to_json(), but both Runner.run() entry paths accept an existing RunState without checking it. A caller can create a checkpoint after an incomplete cleanup, start result.aclose() (which marks that live checkpoint pending), and concurrently resume the checkpoint; the new run then reattaches from stale state while the retry may stop or delete the same backend. Reject direct resume until _sandbox_resume_state_pending clears, not only serialization.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity 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: 3373f05c7c
ℹ️ 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".
| result._update_sandbox_resume_state( | ||
| payload if self.runner_ownership_requires_transfer else None, | ||
| pending=self.cleanup_has_pending_work, |
There was a problem hiding this comment.
When initial cleanup preserves a backend without pending work, callers can validly persist result.to_state().to_json() and later invoke the new result.aclose() retry. If that retry succeeds, the manager deletes the backend and this branch clears only the live result and weakly referenced RunState objects; the already persisted JSON still points to the deleted backend and may contain the snapshot from before the successful retry. Fresh evidence beyond the earlier late-snapshot comment is this explicit retry path, which starts after serialization was allowed; keep the emitted durable checkpoint resumable rather than deleting its backend behind it.
AGENTS.md reference: AGENTS.md:L117-L119
Useful? React with 👍 / 👎.
| if threading.current_thread() is not self.thread: | ||
| self.thread.join() |
There was a problem hiding this comment.
When a prior SDK-owned run_sync() leaves deferred cleanup, the driver eventually enters shutdown_asyncgens(); if an async-generator finalizer waits indefinitely, the next run_sync() calls stop(), whose callback deliberately refuses to stop that loop once generator shutdown has begun, and this unbounded join() therefore hangs the caller forever. Bound this handoff or retain an independently driven owner without synchronously joining a potentially stuck finalizer.
AGENTS.md reference: AGENTS.md:L117-L118
Useful? React with 👍 / 👎.
| snapshot_error = await self._persist_snapshot_before_stop_error( | ||
| before_stop_error=before_stop_error | ||
| ) |
There was a problem hiding this comment.
When _before_stop() fails because PTY termination completed with an error after pty_terminate_all() removed its registry entry—for example, a Docker kill command fails—_has_pending_pty_cleanup_tasks() is false even though the process may still be alive, so this fallback starts snapshotting immediately. The surviving process can mutate files during the archive, and later cleanup retries cannot find the removed entry to terminate it, leaving the persisted workspace inconsistent; a completed cleanup task is not proof that termination succeeded, so retain the failed entry or stop the backend before snapshotting.
AGENTS.md reference: AGENTS.md:L117-L119
Useful? React with 👍 / 👎.
|
@codex review |
1 similar comment
|
@codex review |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The original detached-PTY cleanup issue is real: removing entries before asynchronous teardown can leave cleanup unowned when cancellation interrupts it, and a batch failure must not prevent attempts on the remaining entries.
Please reset this PR to that scope. The current implementation adds a public result.aclose() lifecycle, checkpoint retention/late-state machinery, generic computer-disposal deadlines, and a new run_sync loop driver with caller-surface heuristics. Those are substantive API and architecture changes beyond the PTY cleanup fix. Keep the focused PTY ownership/cancellation and attempt-all batching behavior; remove the broader runner/result/computer lifecycle redesign from this PR.
In particular, restore the released foreground run_sync loop selection. The new default selects an SDK loop despite an existing caller policy loop, bypassing its task factory/supervision and changing loop affinity. It can also reuse the process-global HTTP client's pooled transports across those two loops. More surface-detection exceptions would expand the same problem; preserving the existing foreground loop contract is the smaller correction.
Reviewed 195b4bdb93c31441738fa92a6f856fff93721131 against its merge base and the released loop behavior, including independent correctness and architecture reviews. All runtime changes and representative regression coverage were inspected; this is sufficient to request the scope reset, not a claim of exhaustive validation of the expanded architecture. GitHub reports no hosted checks, and no local runtime tests were executed.
| # Keep unbound synchronous runs on an SDK-owned loop so a caller's default loop remains | ||
| # stopped and closable. Explicit caller-owned run surfaces may already own primitives on | ||
| # that default loop, so run the foreground coroutine there when one is available. | ||
| sync_loop = _get_sync_loop() |
There was a problem hiding this comment.
[P2] Preserve the caller-selected foreground event loop
A plain run_sync now chooses a separate SDK loop even when the caller installed an open default loop. This bypasses that loop's configured task factory and supervision, unlike the deliberately tested behavior in v0.22.2. Adding context/hooks/session on a later call can switch back to the caller loop while OpenAIProvider still retains the process-global HTTP pool, creating a cross-loop transport path. Please restore the existing foreground loop selection as part of narrowing this PR to PTY cleanup, rather than adding more caller-surface heuristics.
f7f55a8 to
2e9e1e4
Compare
Signed-off-by: mikemikimike <13286568797@163.com>
e890a5b to
bb88424
Compare
|
Reworked this branch onto upstream Removed the broader runner/result/computer lifecycle redesign and restored the released
Verification on the rewritten head:
/re-review |
5 participants
This pull request fixes sandbox PTY cleanup ownership and cancellation races, and ensures PTY cleanup batches attempt every detached entry after an individual failure.
Summary
When a PTY is removed from the session registry before asynchronous teardown, caller cancellation can leave cleanup without an owner. This change keeps cleanup owned until detached teardown settles.
pty_terminate_all()also attempts every detached entry and reports the first cleanup error after the batch.Changes
Validation
30 passed, 2 deselected.19 passed, 98 deselected.9 passed, 245 deselected.tests/sandbox/test_runtime.pywas not run on Windows becauseUnixLocalSandboxis intentionally unsupported there.The changes are internal and do not add a public result lifecycle API, checkpoint state machinery, or runner/computer lifecycle changes.
Fixes #4747