fix(sandbox): settle PTY output before cleanup

[seratch]

This pull request fixes PTY output settlement and supersedes #4572 and #4724. PTY collectors now re-drain output at timeout boundaries, carry only complete valid UTF-8 sequences across read windows, and make bounded replacement progress for invalid E0, ED, F0, and F4 prefixes.

Terminal cleanup now follows a collector-owned settled output_closed fact across local, Docker, E2B, Cloudflare, Modal, Blaxel, and Daytona adapters, so exit visibility cannot drop queued bytes or carried suffixes.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-07T15:14:54.822520Z	62c03d7	New commits
🔒 Security Review	✅ Completed 2026-09-07T15:15:11.157202Z	62c03d7	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21c32b9985

ℹ️ 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".

[chatgpt-codex-connector]

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

An unknown error occurred

ℹ️ 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".

[fscfede-beep]

I re-audited the current 41be368c head specifically around cancellation ownership. Two cancellation boundaries can still lose bytes that this PR now intends to carry across PTY windows.

[fscfede-beep]

Cross-link for collision/maintainer coordination: #4745 was opened later against the same PTY cross-window UTF-8 / settlement area and has since accumulated a different implementation plus several cancellation/finalization fixes. I’m reviewing both rather than opening a third implementation. The two current cancellation findings I left on this PR (3885258586, 3885258589) are specific to this branch’s carry/Modal ownership boundaries; #4745 currently avoids them with different transaction state, but has had its own finalization lifecycle issues. Recommend treating #4738 and #4745 as competing solutions and selecting one architecture rather than merging them independently.

[fscfede-beep]

Selective salvage after comparing #4745 against current 41be368c: I do not think its whole patch should be ported here. Two of the three potentially useful pieces are already covered or architecture-specific: restricted E0/ED/F0/F4 prefix handling is already tested here, and #4745's source_text + final tail truncation repair addresses a post-collection tail path this branch does not use.

One test idea is still useful here: a controlled Unix lifecycle regression where process.returncode is already visible but the output pump still owns the continuation of a split UTF-8 character. Hold the pump before it appends the continuation, assert the first collection/finalization keeps the session live and retains the lead byte, release the pump, then assert the next update returns é and closes. The current implementation looks correct by inspection because _watch_process_exit sets output_closed only after gather(*pump_tasks), but tests/sandbox/test_unix_local.py does not currently pin that ordering.

I would add that regression only after/alongside the two cancellation ownership fixes already posted in 3885258586 and 3885258589. No third competing PR from me.

[HuzaifaChaudary]

hi @seratch. i had a pull request in this area, #4745, which i have just closed in favour of this one. it predates mine and covers the same ground, so there is nothing to weigh up there.

@fscfede-beep reviewed both and suggested one thing from mine might still be worth having, so i am leaving it here as an idea rather than a patch. take it or ignore it.

this pr has the producer drained output_closed design already. the test below pins the ordering it protects, a process reaped while a pump still owns the continuation, which is the case where finalising on the exit code alone silently turns é into �:

entry = _UnixPtyProcessEntry(process=process, tty=True)   # returncode 0
# output_closed deliberately NOT set, the pump still holds the continuation
entry.output_chunks.append("é".encode()[:1])

# first update must leave the session alive with the lead byte still queued
assert first.process_id == 1
assert first.exit_code is None
assert list(entry.output_chunks) == ["é".encode()[:1]]

# then the pump delivers and closes
entry.output_chunks.append("é".encode()[1:])
entry.output_closed.set()

assert final.output.decode("utf-8") == "é"
assert final.process_id is None

what made me think it earns its place is that it fails for the right reason. swapping the predicate back to entry.process.returncode on its own gives:

PtyExecUpdate(process_id=None, output=b'\xef\xbf\xbd', exit_code=0, original_token_count=None)

so it catches a future tidy up that undoes the predicate, which is a one line change that reads like cleanup. the full version is in the closed branch at HuzaifaChaudary/openai-agents-python@78b5c4d, tests/sandbox/test_unix_local.py::TestUnixLocalPty::test_session_is_not_finalized_while_a_pump_still_holds_output.

happy to open it as a small test only pull request against this branch if you want it, or to leave it entirely. no need to reply if not.

[HuzaifaChaudary]

@seratch, short follow up to my note above and then i will stop.

the two cancellation findings @fscfede-beep raised on this pr are the same two i hit and fixed on the branch i closed, so there is working tested code for both if it saves you deriving them again. commits are on HuzaifaChaudary/openai-agents-python, against my own shape rather than yours, so they are a reference not a patch.

shared collector, 2070572e. exactly the contract he describes:

    except asyncio.CancelledError:
        if output:
            output_chunks.appendleft(bytes(output))
        raise

modal, bf682c80. the whole buffer goes to entry owned state, carried tail and new reads together, with the try covering both stream reads, the exit poll, the sleep, the drains and the final poll. assigning rather than appending is what stops the carried tail replaying, since chunks already starts with it.

each has a regression that fails on the commit before it. the modal one consumes the continuation off the stream, cancels on the next await, and asserts both halves are still entry owned.

one thing worth flagging, because it bit me right after i fixed these. once the tail flush moved in front of the registry pop, cancelling during the drain skipped _terminate_pty_entry entirely, and since the entry was already out of the map nothing could clean it up later. on blaxel that leaves a websocket and an aiohttp.ClientSession open. 78b5c4d0 puts the drain in a try with the terminate in the finally. worth checking whichever order this branch lands on.

that plus the blocked pump test above is everything i have. no reply needed, and i am not opening anything here.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b2d1750cf

ℹ️ 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".

[ErenAta16]

Flagging the neighbourhood rather than the diff, since three open PRs are editing
these files at once and only one of the three overlaps is a real duplicate.

@Hughhhhcoder's #4750 and @mikemikimike's #4751 both landed the day after this one.
Reading the source changes, the three are addressing different defects:

#4738  threads output_closed through _collect_pty_output      output settled before cleanup
#4750  except Exception -> BaseException in pty startup       fds leak on cancellation
#4751  wraps _terminate_pty_entry in a cancellation shield    teardown aborts on cancellation

So they are complementary, and none of them subsumes another. All three touch
sandbox/sandboxes/unix_local.py, at lines 354, 391/463 and 400/442 respectively,
which is close enough to be worth knowing about but far enough apart that the
hunks should not fight.

The real duplicate is the helper, not the call sites. #4750 adds
_settle_pty_cleanup as a module function in sandbox/session/pty_types.py, and
#4751 adds _settle_pty_cleanup as a method on BaseSandboxSession in
sandbox/session/base_sandbox_session.py. Same name, same shield-in-a-loop
algorithm, same completion.result() then task.result() sequence. Merging both
leaves the SDK with two of them.

They are not equivalent, and the difference shows up when cleanup itself fails
while the caller is being cancelled:

cleanup raises, caller cancelled
  #4750  -> CancelledError propagates    task.cancelled() = True
  #4751  -> RuntimeError propagates      task.cancelled() = False

#4750 gives the caller's cancellation priority over the cleanup error; #4751
reaches task.result() before its caller_cancellation check, so the cleanup
exception wins and the CancelledError is dropped. A task that was asked to stop
and then reports cancelled() is False is the case wait_for and TaskGroup
both rely on, so I would take #4750's ordering whichever module the helper ends
up living in.

One completeness note that may save a round trip on #4750: the
except Exception -> except BaseException change there is the only site of its
shape. I walked the AST of everything under sandbox/ and
extensions/sandbox/, looking for a try whose body contains an await and
whose sole handler is except Exception while closing a descriptor, and
unix_local.py:357 is the single match. entries/artifacts.py:714 looks similar
but its try body is fully synchronous, so Exception is sufficient there and it
is not an outlier.