Skip to content

fix(sandbox): retain PTY output queued at timeout - #4572

Open
hsusul wants to merge 1 commit into
openai:mainfrom
hsusul:fix/pty-collector-final-drain
Open

fix(sandbox): retain PTY output queued at timeout#4572
hsusul wants to merge 1 commit into
openai:mainfrom
hsusul:fix/pty-collector-final-drain

Conversation

@hsusul

@hsusul hsusul commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • drain shared, E2B, Cloudflare, and Modal PTY collectors after deadline/timeout
  • preserve output queued between the final poll and timeout
  • add a deterministic regression test for timeout-tail output

Testing

  • python -m compileall -q on changed source and test files
  • focused pytest was attempted but this fresh worktree could not import the repository because optional dependency httpx2 was unavailable; dependency sync was blocked by package-index DNS failures

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

Copy link
Copy Markdown

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: e3359bb71c

ℹ️ 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 +1016 to +1017
stdout_chunk = await self._read_modal_stream(entry=entry, stream_name="stdout")
stderr_chunk = await self._read_modal_stream(entry=entry, stream_name="stderr")

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 Keep the final Modal reads non-blocking

When a Modal stream lacks __aiter__ and uses the supported read() fallback, each of these calls can wait for the helper's 0.2-second timeout, despite the deadline already having expired. The added stdout/stderr reads can therefore make pty_exec_start or pty_write_stdin return up to roughly 400 ms later than before, even when no output is available; use a deadline-safe/non-blocking fallback for this final probe.

Useful? React with 👍 / 👎.

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you fix all the review comments?

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.

2 participants