Uh oh!
There was an error while loading. Please reload this page.
fix(sandbox): settle PTY output before cleanup - #4738
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
Uh oh!
There was an error while loading. Please reload this page.
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
21c32b9 to
41be368Compare
fscfede-beep
left a comment
There was a problem hiding this comment.
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.
| await wait_for_output(remaining_s) | ||
| else: | ||
| try: | ||
| await asyncio.wait_for(output_notify.wait(), timeout=remaining_s) |
There was a problem hiding this comment.
Cancellation can drop a UTF-8 prefix that a previous window deliberately carried for this surviving session. Deterministic ordering on current head: first window ends with b"\xc3", so _drain_and_carry_incomplete_suffix() leaves [b"\xc3"] on output_chunks; the next collection drains that lead byte into local output, reaches this output_notify wait, and is cancelled. There is no outer cancellation handler, so the deque is now empty. If b"\xa9" arrives later, the next/final collection sees the continuation alone and returns � instead of é.
This is in-scope for this PR: before the new cross-window carry, a cancelled call could abandon its own drained window, but it could not consume persistent UTF-8 state intentionally handed forward by an earlier successful call and thereby corrupt later output.
The narrow fix is to make the drained local buffer transactional. Wrap collection/settlement in try/except asyncio.CancelledError; if output is non-empty, restore it synchronously to the front of output_chunks before re-raising (or keep ownership on the entry until commit). appendleft(bytes(output)) preserves these older bytes ahead of anything a producer queued while the call was running. Regression: cancel the second window while it is waiting here after draining the requeued b"\xc3"; assert the lead is back in the deque and a later b"\xa9" completes é.
There was a problem hiding this comment.
I turned this boundary into an executable control-flow reference against the 41be368c ownership shape. The current-shape repro drains a carried b"\xc3", cancels while the next window is waiting, and deterministically leaves the deque empty; the fixed reference restores the drained local buffer synchronously with appendleft(bytes(output)), then a later b"\xa9" reconstructs é. The focused reference suite also covers the Modal transfer below: 6/6 PASS (3 tests prove current bad behavior, 3 prove the proposed ownership repairs).
The narrow shared-collector patch contract is to put the existing collection + final settlement/drain/carry phase under try/except asyncio.CancelledError; if local output is non-empty, synchronously return it to the front of output_chunks before re-raising. The handler needs to cover cancellation from poll_output, waits, settle_output, and the final drain/carry lock acquisition. I have not run the upstream suite for this patch and am not opening a competing PR.
| stream_name="stdout", | ||
| allow_new_read=allow_new_read, | ||
| ) | ||
| stderr_chunk = await self._read_modal_stream( |
There was a problem hiding this comment.
Modal has the same survivor invariant one layer earlier: a successful stream read is consumed from the provider before poll_output() commits it to entry.output_chunks. Example on this branch: a previous window has carried b"\xc3" in entry.output_chunks; this call reads the continuation b"\xa9" from stdout, then is cancelled during this stderr await. The continuation lives only in local chunks, so it is lost while the older lead byte remains durable. A later call can then pair that lead with unrelated output or eventually replace it.
The exit drain has the same shape internally: _drain_modal_stream() accumulates consumed stream items in its own local bytearray across further awaits before returning them to poll_output(). Catching cancellation only in the outer shared collector therefore cannot recover bytes already removed from Modal's stream.
Please make each Modal ownership transfer cancellation-safe: either commit every successfully consumed stream chunk to entry-owned state before the next await, or catch CancelledError in both poll_output() and _drain_modal_stream() and synchronously restore their local consumed bytes to entry.output_chunks before propagating cancellation. Avoid an awaited lock acquisition as the only copy's next step unless cancellation around that acquisition also restores it. A regression should consume the UTF-8 continuation from stdout, block the following stderr read, cancel there, and verify the continuation remains entry-owned and the next collection reconstructs é.
There was a problem hiding this comment.
I also exercised this Modal ownership transfer independently. Current-shape repro: stdout consumes the continuation b"\xa9", stderr blocks, caller cancellation arrives, and the continuation never reaches entry.output_chunks; a separate exit-drain repro consumes b"tail", cancels on the next read, and loses that local buffer too. Both are deterministic.
The fixed reference passes by (1) moving each successful stdout/stderr read into local chunks immediately before the next await, (2) catching CancelledError in poll_output and synchronously returning any locally owned chunks to entry.output_chunks, clearing local ownership immediately after a normal queue commit to avoid replay, and (3) giving _drain_modal_stream the same cancellation-return rule for its bytearray. Together with the shared collector repair, the focused reference suite is 6/6 PASS. No provider integration execution or upstream patch application is claimed.
fscfede-beep
commented
Aug 29, 2026
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 ( |
fscfede-beep
commented
Aug 29, 2026
Selective salvage after comparing #4745 against current One test idea is still useful here: a controlled Unix lifecycle regression where I would add that regression only after/alongside the two cancellation ownership fixes already posted in |
HuzaifaChaudary
commented
Aug 29, 2026
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 entry=_UnixPtyProcessEntry(process=process, tty=True) # returncode 0# output_closed deliberately NOT set, the pump still holds the continuationentry.output_chunks.append("é".encode()[:1])
# first update must leave the session alive with the lead byte still queuedassertfirst.process_id==1assertfirst.exit_codeisNoneassertlist(entry.output_chunks) == ["é".encode()[:1]]
# then the pump delivers and closesentry.output_chunks.append("é".encode()[1:])
entry.output_closed.set()
assertfinal.output.decode("utf-8") =="é"assertfinal.process_idisNonewhat made me think it earns its place is that it fails for the right reason. swapping the predicate back to 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 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
commented
Aug 29, 2026
@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 shared collector, exceptasyncio.CancelledError:
ifoutput:
output_chunks.appendleft(bytes(output))
raisemodal, 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 that plus the blocked pump test above is everything i have. no reply needed, and i am not opening anything here. |
Co-authored-by: Henry Su <henrysu4707@gmail.com> Co-authored-by: ayaangazali <ayaangazali.work@gmail.com>
41be368 to
2b2d175CompareThere was a problem hiding this comment.
💡 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".
| stream_name="stdout", | ||
| allow_new_read=allow_new_read, | ||
| ) | ||
| stderr_chunk = await self._read_modal_stream( |
There was a problem hiding this comment.
When Modal exposes the supported fallback read interface and both stdout and stderr reads take their 0.2-second timeout, a requested 250 ms yield still takes roughly 400 ms because the stderr read starts unconditionally after stdout has already consumed most or all of the deadline. The fresh evidence in this revision is that the new outer deadline guard skips later polls but does not recheck the deadline between these two sequential reads; check the remaining time before starting stderr or bound each read by the remaining yield window.
Useful? React with 👍 / 👎.
| # the close signal that authorizes collector settlement and PTY removal. | ||
| async with entry.output_lock: | ||
| pass | ||
| entry.output_closed.set() |
There was a problem hiding this comment.
When handle.wait() fails with a transport or runtime exception that has no exit_code, the process may still be running, but this unconditional close signal makes every subsequent output collection return immediately and makes capacity pruning classify the entry as safely settled. That prevents normal long-poll behavior and can cause a still-running process to be preferentially pruned; publish output_closed only after a successful wait or a recognized terminal exception, or retain and surface the indeterminate waiter failure separately.
Useful? React with 👍 / 👎.
ErenAta16
left a comment
There was a problem hiding this comment.
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 touchsandbox/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 insandbox/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: theexcept Exception -> except BaseException change there is the only site of its
shape. I walked the AST of everything under sandbox/ andextensions/sandbox/, looking for a try whose body contains an await and
whose sole handler is except Exception while closing a descriptor, andunix_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.
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_closedfact across local, Docker, E2B, Cloudflare, Modal, Blaxel, and Daytona adapters, so exit visibility cannot drop queued bytes or carried suffixes.