feat: Add pty support to Vercel sandbox#2909
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6983993e94
ℹ️ 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".
| await session.ready() | ||
| entry.session = session |
There was a problem hiding this comment.
Close PTY session when readiness handshake fails
If AsyncPTYSession.open(...) succeeds but session.ready() times out or raises, cleanup calls _terminate_pty_entry(entry) while entry.session is still None, so the opened PTY connection is never closed. In flaky network conditions this leaves orphaned PTY processes/connections on the backend and can exhaust session limits over repeated retries. Assigning entry.session before awaiting readiness (or explicitly closing the local session in the exception path) avoids the leak.
Useful? React with 👍 / 👎.
| except Exception as e: | ||
| if not registered: | ||
| await self._terminate_pty_entry(entry) |
There was a problem hiding this comment.
Clean up unregistered PTY session on task cancellation
This startup path creates the reader task before registration, but the exception handling only catches Exception; asyncio.CancelledError (a BaseException) bypasses cleanup. If a caller cancels during pty_exec_start after the task/session are created, the PTY remains running but is never added to _pty_sessions, so it cannot be terminated via normal session bookkeeping.
Useful? React with 👍 / 👎.
|
Two more sandbox PTY test scenarios: Starting PTY with a one-shot command:Example: p PTY after stop/resume:Flow:
|
Uses the recently published
AsyncPTYSessionclass fromvercelto implement pty support.