Skip to content

feat: Add pty support to Vercel sandbox#2909

Open
scotttrinh wants to merge 1 commit intoopenai:mainfrom
scotttrinh:scotttrinh/vercel-pty
Open

feat: Add pty support to Vercel sandbox#2909
scotttrinh wants to merge 1 commit intoopenai:mainfrom
scotttrinh:scotttrinh/vercel-pty

Conversation

@scotttrinh
Copy link
Copy Markdown
Contributor

Uses the recently published AsyncPTYSession class from vercel to implement pty support.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 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".

Comment on lines +520 to +521
await session.ready()
entry.session = session
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge 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 👍 / 👎.

Comment on lines +537 to +539
except Exception as e:
if not registered:
await self._terminate_pty_entry(entry)
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 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 👍 / 👎.

@alfozan alfozan self-requested a review April 16, 2026 22:57
@seratch seratch added this to the 0.14.x milestone Apr 17, 2026
@alfozan
Copy link
Copy Markdown
Collaborator

alfozan commented Apr 17, 2026

Two more sandbox PTY test scenarios:

Starting PTY with a one-shot command:

Example: pty_exec_start("echo PTY_ONE_SHOT")
Expected: command runs and returns output

PTY after stop/resume:

Flow:

  • create sandbox
  • stop()
  • resume()
  • start a new PTY shell
    Expected: PTY opens normally after resume

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants