fix(sandbox): settle PTY cleanup before cancellation - #4751
fix(sandbox): settle PTY cleanup before cancellation#4751mikemikimike wants to merge 12 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".
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".
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. |
|
@codex review Please review current head |
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Fresh re-audit of current head 10d6f2dc627a8959a602356bd67d98cf680e1083 against #4747: I do not see a new runtime blocker.
Independent control-flow reproduction of the current helper confirms the intended invariants: cleanup completes before caller cancellation is re-propagated; repeated cancellation preserves the first cancellation; registry-cleared batch cleanup attempts every entry even when the first cleanup fails; and cleanup exceptions take precedence over caller cancellation, matching the existing mount_lifecycle error-before-cancellation policy. The latest batch implementation also fixes the Sept 3 first-error gap by collecting the first error while continuing through all entries.
Scope remains correctly limited to post-registry-removal ownership in finalizer/prune/pty_terminate_all() across the seven backends. I would keep #4749/#4750 separate because that PR addresses pre-registration startup ownership instead.
External gate remains: GitHub Actions for this fork head is still action_required, so this review is not claiming upstream CI green or maintainer acceptance.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
PTY cleanup now attempts every detached entry before re-raising the first failure; the added regression covers the ownership-loss case I raised. No remaining blocker from me.
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".
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Auditoría independiente del head 54a942db5f8580417687df140247655a0868c009 contra #4747: la arquitectura base es correcta, pero no considero el PR merge-ready todavía por los 3 bloqueos P1 actualmente abiertos.
-
Liveness del owner-settled cleanup:
_settle_pty_cleanup()debe tener un límite de espera de proveedor antes de absorber cancelación indefinidamente. La cancelación del caller no puede convertir una caída de proveedor en unstop()/shutdown()que nunca termina. Recomiendo que el helper aceptetimeout, y que cada backend pase su límite de cleanup; al expirar, registrar/propagar el timeout según el contrato existente y continuar con la liberación local/batch. -
Docker PID-file race: el cambio a
wait_for(run_in_executor(...))puede cancelar el Future asyncio mientras el worker del executor sigue ejecutandoexec_run. Después,_rm_best_effort(pid_path)puede borrar el PID file antes de que el kill tardío lo lea, dejando vivo el proceso cuyo entry ya perdió ownership. Kill + PID-file removal deben ser una única operación ordenada en el mismo worker, o el PID file debe conservarse hasta que el worker confirme terminalidad. -
Regresión Docker: el readiness wait de
first_kill_startedocurre antes deltry/finally; si scheduling/executor starvation hace vencer ese wait,release_first_killnunca se establece y el worker puede quedar bloqueado durante teardown. Crear/start/wait debe quedar dentro de untry/finallyque siempre libere y drene el worker.
La parte positiva: #4751 sí cubre correctamente el ownership transferido después de pop/clear, aplica el patrón a los siete backends y ya corrige el primer-entry-error para que el batch continúe con las entradas restantes. El siguiente paso correcto es resolver estos tres bloqueos, volver a ejecutar la matriz focal y solo entonces cerrar #4747. #4749/#4750 sigue siendo un problema distinto (ownership antes de registro).
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.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3678c6364a
ℹ️ 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".
| first_error: BaseException | None = None | ||
| for entry in entries: | ||
| try: | ||
| await self._settle_pty_cleanup(cleanup_entry(entry), timeout=timeout) |
There was a problem hiding this comment.
When pty_terminate_all() has several entries and each provider close or kill stalls, cancellation caught by the first _settle_pty_cleanup is saved as first_error, but this loop then gives every later entry a fresh full timeout before re-raising it. With 64 supported PTYs, cancellation can therefore take up to 64 × cleanup_s (32 minutes with Blaxel's 30-second default), so an outer timeout still cannot bound stop() or shutdown(). Fresh evidence in this revision is the per-entry settlement call here rather than one deadline around the batch; use a shared batch deadline while still scheduling cleanup for every entry.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
| if pruned is not None: | ||
| await self._terminate_pty_entry(pruned) | ||
| await self._settle_pty_cleanup(self._terminate_pty_entry(pruned)) |
There was a problem hiding this comment.
When the PTY table is full and closing the pruned entry exceeds cleanup_s, the new command has already been sent and its entry registered, but this newly timed call raises TimeoutError before pty_exec_start() returns its process_id. The caller therefore observes a failed start while the command continues running in _pty_sessions; retrying can duplicate command side effects. Remove and terminate the new entry on this failure, or keep prune-cleanup timeout from invalidating an otherwise successful start.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
| elif timed_out: | ||
| raise asyncio.TimeoutError() |
There was a problem hiding this comment.
For a runner-owned session with a non-noop snapshot, a PTY close or kill that exceeds this new deadline raises TimeoutError from _before_stop(), so BaseSandboxSession.stop() skips _persist_snapshot(). _SandboxSessionResources.cleanup() still proceeds through shutdown() and client.delete() after that stop failure, which can delete the backend containing the only current workspace copy. Continue snapshot persistence before deleting the provider resource, or otherwise retain the backend when timed-out PTY cleanup prevents persistence.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
| await self._exec_run( | ||
| cmd=command, | ||
| workdir=None, | ||
| user=None, | ||
| timeout=_PTY_CLEANUP_TIMEOUT_S, |
There was a problem hiding this comment.
When all eight shared _DOCKER_EXECUTOR workers are occupied, this kill command remains queued until _exec_run()'s five-second wait_for expires; cancellation of a not-yet-started executor future prevents the command from ever running. _kill_pty_pid_path() suppresses the resulting timeout and termination returns after the registry entry was removed, leaving both the detached process and its PID file without an owner or retry path. Fresh evidence beyond the earlier PID-file ordering concern is this queued-executor path, where the combined kill-and-remove command never starts at all; keep the queued kill scheduled independently of the caller deadline.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5653ef8f6
ℹ️ 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".
| while not snapshot_task.done(): | ||
| try: | ||
| await asyncio.shield(snapshot_task) | ||
| except asyncio.CancelledError: | ||
| continue |
There was a problem hiding this comment.
When _before_stop() fails or is cancelled and a supported non-noop snapshot operation stalls—for example, an unbounded RemoteSnapshot.upload()—this loop absorbs every subsequent cancellation until the snapshot task finishes. Consequently, an outer asyncio.wait_for() cannot terminate runner cleanup, and shutdown(), provider deletion, and dependency closure are never reached; bound this fallback persistence or retain the backend without making cleanup permanently uncancellable.
AGENTS.md reference: AGENTS.md:L149-L150
Useful? React with 👍 / 👎.
| await asyncio.shield(snapshot_task) | ||
| except asyncio.CancelledError: | ||
| continue | ||
| snapshot_task.result() |
There was a problem hiding this comment.
When PTY cleanup has already raised or cancelled stop() and the fallback snapshot persistence also fails, snapshot_task.result() raises the snapshot exception before control can re-raise before_stop_error. This replaces the primary cleanup failure—and even converts caller cancellation into an ordinary snapshot error—so preserve the original failure while retaining the persistence failure as chained diagnostic context.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
| while pending: | ||
| remaining = deadline - loop.time() | ||
| if remaining <= 0: | ||
| break |
There was a problem hiding this comment.
When a provider kill or close exceeds the shared deadline, this branch exits without reporting that any cleanup remains pending, so pty_terminate_all() returns and stop() immediately starts snapshot persistence while the detached PTY can still modify the workspace. A stalled Daytona/E2B kill or queued Docker kill can therefore race the archive and produce a partial snapshot before runner cleanup deletes the backend; do not advance to persistence until process termination is confirmed, or fail cleanup while retaining the backend instead of treating this deadline as successful termination.
AGENTS.md reference: AGENTS.md:L149-L150
Useful? React with 👍 / 👎.
| 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)) |
There was a problem hiding this comment.
When Docker has already obtained the command's exit code but exec_inspect, PID-file removal, or reader-thread cleanup consumes the five-second cleanup budget, this default-propagating settlement raises TimeoutError instead of returning the completed PtyExecUpdate. The entry and process ID have already been removed, so the caller sees a failed command and cannot poll the result again; detach timed-out best-effort finalization as the prune path does rather than replacing the successful command outcome.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
4 participants
Summary
PTY ownership is removed from the session registry before asynchronous teardown. If the caller is cancelled during that gap, cleanup can be abandoned.
pty_terminate_all()also stopped at the first cleanup error, leaving later detached PTYs unreachable.Changes
BaseSandboxSessionhelper that owns asynchronous PTY cleanup, survives repeated caller cancellation until cleanup reaches a terminal state, preserves cleanup errors, and then re-propagates the first caller cancellation.Tests
uv run --frozen --no-sync pytest tests/sandbox/test_base_sandbox_session.py tests/extensions/sandbox/test_blaxel.py -q— 254 passed, 1 skipped.uv run --frozen --no-sync ruff format --checkon the 10 changed files — 10 files already formatted.uv run --frozen --no-sync ruff checkon the 10 changed files — passed.uv run --frozen --no-sync mypy src— success for 308 source files.uv run --frozen --no-sync pyright --project pyrightconfig.json --threads 4— 0 errors, 0 warnings, 0 informations.git diff --check— passed.Compatibility / Known limitations
action_requiredwith no jobs, so upstream approval/permissions still need to be handled by a maintainer.Fixes #4747