fix: keep a utf-8 character that straddles two pty collection windows - #4745
fix: keep a utf-8 character that straddles two pty collection windows#4745HuzaifaChaudary wants to merge 15 commits into
Conversation
pty output is collected in repeated windows over one persistent deque, and each window decoded with errors=replace. a character whose bytes land either side of a window boundary was therefore replaced twice, once per half, and the bytes were gone before any later window could put it back together. no error, just wrong text the incomplete tail is now handed back to the deque for the next window. once the provider is done there is no next window so those bytes really are invalid and still get replaced
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea133bf8d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| expected = 3 | ||
| else: | ||
| expected = 4 | ||
| return back if back < expected else 0 |
There was a problem hiding this comment.
When a running PTY ends a collection window with an invalid UTF-8 prefix such as b"\xff", this helper treats the byte as the start of a four-byte character and requeues it. Every subsequent idle poll repeats that behavior, so the replacement character previously produced by errors="replace" remains invisible until the process exits or emits more output. PTY subprocesses can produce arbitrary bytes, so only syntactically valid incomplete prefixes should be held back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
good catch, this was real. \xff is not a legal lead byte but my width table read it as the start of a four byte character and held it back, and since the next window did the same thing the replacement character stayed hidden until the process exited. same for \xc0 \xc1 and \xf5.
pushed f077787. i dropped the hand written table and used an incremental decoder instead:
decoder = codecs.getincrementaldecoder("utf-8")("replace")
text = decoder.decode(output, final=is_done())
pending = decoder.getstate()[0]
if pending:
async with output_lock:
output_chunks.appendleft(bytes(pending))it buffers only a genuine partial sequence and replaces anything that cannot begin a character right away, so nothing is held that will never be completed. final=is_done() covers the other end, a tail with no later window to finish it is replaced rather than kept. it also handles overlong forms and surrogates, which my table did not.
added the invalid lead bytes as test cases. four of the six fail on the version you reviewed:
FAILED ...does_not_hold_back_bytes_that_start_no_character[\xff]
FAILED ...does_not_hold_back_bytes_that_start_no_character[\xc0]
FAILED ...does_not_hold_back_bytes_that_start_no_character[\xc1]
FAILED ...does_not_hold_back_bytes_that_start_no_character[\xf5]
4 failed, 2 passed
all 20 pass now, and the sandbox suite is 1298 passed against 1280 on main, with the same 2 test_mount_security docker mount failures that fail on main too.
my first version read the lead byte and worked out the width by hand, which accepted lead bytes that are not legal. ff c0 c1 and f5 all looked like the start of something longer so they were held back and requeued every window, and the replacement character that used to appear right away stayed hidden until the process exited using an incremental decoder instead. it buffers only a real partial sequence and replaces anything that cannot begin a character straight away, and passing final when the provider is done closes it so a truncated tail is replaced too. it also knows about overlong forms and surrogates which my table did not added the invalid lead bytes as cases. four of the six fail on the old version
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f077787878
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # character are not held, they are replaced straight away as before. Completing the | ||
| # decoder once the provider is done replaces a tail that no later window will finish. | ||
| decoder = codecs.getincrementaldecoder("utf-8")("replace") | ||
| text = decoder.decode(output, final=is_done()) |
There was a problem hiding this comment.
When a live PTY ends a collection window with b"\xed\xa0", Python's incremental UTF-8 decoder buffers it even though no continuation byte can make it valid—ED A0..BF encodes the forbidden surrogate range. The helper therefore returns no replacement and requeues these bytes on every idle poll, hiding the output until the process exits or emits more data. Fresh evidence beyond the earlier b"\xff" report is that the added test covers only the completed b"\xed\xa0\x80" sequence, while this two-byte impossible prefix still enters the pending branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
right again, thanks. ED A0..BF leads a surrogate so no third byte can finish it, and the decoder still buffers it. held back it goes round the deque again every window, which is the same hidden output the last commit was supposed to stop.
i wanted to know whether it was only surrogates, so i walked every one, two and three byte prefix the decoder buffers and asked which of them can never become a character:
prefixes CPython buffers that can never complete: 32
all length 2, first byte 237, second byte 0xA0..0xBF
so it is exactly the range you named and nothing else. overlong and out of range prefixes like E0 80, F0 80 and F4 90 are already rejected on the spot, which is why they never reach the deque.
fixed in the pushed commit:
if len(pending) == 2 and pending[0] == 0xED and pending[1] >= 0xA0:
text += pending.decode("utf-8", errors="replace")
pending = b""it replaces them the same way the decoder would once closed, so b"\xed\xa0" gives two replacement characters and the deque is left empty.
ED 80..9F is U+D000..U+D7FF, a real character, so that one still waits for its third byte. there is a test for each direction. the three surrogate cases fail on the commit you reviewed and pass now.
suite is 1303 passed against 1298, with the same 2 test_mount_security docker mount failures that fail on main.
the decoder buffers ED A0..BF even though that leads a surrogate and no third byte can finish it. held back it would go round the deque again every window and the output would stay hidden until the process exits, which is the same thing the last commit was meant to stop i checked every one and two and three byte prefix the decoder buffers. those 32 are the only ones that can never become a character, everything else it holds is still waiting on a real continuation. so the check is only for them ED 80..9F is U+D000 to U+D7FF and is a real character, so that still waits
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 144efe1997
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # and it is handed back for the next window to finish. Bytes that cannot begin a | ||
| # character are not held, they are replaced straight away as before. Completing the | ||
| # decoder once the provider is done replaces a tail that no later window will finish. | ||
| decoder = codecs.getincrementaldecoder("utf-8")("replace") |
There was a problem hiding this comment.
This decoder fixes only callers of the shared helper; the supported Modal, E2B, and Cloudflare PTY paths still independently decode each collection window with errors="replace" in their _collect_pty_output implementations. A multibyte character split across windows therefore remains corrupted on those backends—particularly Cloudflare binary frames and Modal byte streams—even though the same pty_exec_start/pty_write_stdin contract now works on UnixLocal, Docker, Blaxel, and Daytona. Route the remaining collectors through equivalent persistent boundary handling rather than leaving provider-dependent output semantics.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
you are right, i had only covered the backends that call the shared helper. fixed in 91f5fb9.
cloudflare and e2b turned out to be near copies of the shared helper, down to the same output_chunks deque, output_lock and output_notify event. so they now call it, the way daytona and blaxel already did:
return await collect_pty_output(
output_chunks=entry.output_chunks,
output_lock=entry.output_lock,
output_notify=entry.output_notify,
is_done=entry.output_closed.is_set,
yield_time_ms=yield_time_ms,
max_output_tokens=max_output_tokens,
)that deletes the two copies instead of fixing the same bug in three places. is_done is self._entry_exit_code(entry) is not None for e2b and entry.output_closed.is_set for cloudflare, which is what each was already checking.
modal reads from a stream rather than a deque, so it cannot call the helper. i pulled the window decode out into decode_pty_window(data, *, is_final) and modal keeps its own tail on the entry:
chunks = bytearray(entry.pending_output)
entry.pending_output = b""
...
exited = await self._peek_exit_code(entry.process) is not None
text, entry.pending_output = decode_pty_window(chunks, is_final=exited)so the utf-8 rules, including the ED A0..BF case from your last comment, live in one place rather than being written out four times.
added a split character test for modal, since that one is not covered by the shared helpers tests. on the previous commit it gives:
E + h??llo
FAILED tests/extensions/sandbox/test_modal.py::test_modal_pty_output_keeps_a_character_split_across_windows
and passes now.
| check | result |
|---|---|
tests/sandbox/test_pty_output.py |
25 passed |
tests/extensions/sandbox before |
8 failed, 690 passed |
tests/extensions/sandbox after |
8 failed, 691 passed |
| the 8 | pre existing, test_runloop_mounts and friends, identical with the branch stashed |
tests/sandbox |
2 failed, 1303 passed, the same 2 docker mount ones that fail on main |
| ruff, ruff format, mypy | clean |
net effect is 73 lines of duplicated collector deleted.
runloop and vercel do not have a pty collector, so there is nothing to route there.
fixing the shared helper only covered unix_local docker blaxel and daytona. cloudflare e2b and modal each carry their own copy of the collector and each one decoded its window with errors=replace, so the character was still lost there cloudflare and e2b were near copies of the shared helper down to the deque and the lock and the notify event, so they just call it now the way daytona and blaxel already did. that deletes the copies rather than fixing them twice modal reads a stream instead of a deque so it cannot call the helper. pulled the window decode out into decode_pty_window and modal keeps its own tail on the entry, so there is still one place that knows the utf-8 rules added a split character test for modal. it gives h??llo before this
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91f5fb97d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| text = chunks.decode("utf-8", errors="replace") | ||
| exited = await self._peek_exit_code(entry.process) is not None | ||
| text, entry.pending_output = decode_pty_window(chunks, is_final=exited) |
There was a problem hiding this comment.
When a Modal process exits between the awaited poll on line 1017 and the second awaited poll in _finalize_pty_update, this stores an incomplete UTF-8 suffix because the first poll reported the process as running, but finalization then observes the exit and removes the entry without decoding pending_output as final. A process whose last output ends mid-character can therefore return neither the previous replacement character nor the pending bytes. Revalidate and flush the pending suffix before removing an entry that the finalizer finds exited.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
confirmed and fixed in fa487b9. this one was a regression i introduced, not a pre existing gap, so thanks for catching it.
the two polls are the problem. _collect_pty_output asks for the exit code at the end of the window, and _finalize_pty_update asks again straight after. a process that ends between them means the collector saw it running, kept the partial character back for a window that never comes, and then the finalizer pops the entry and the bytes leave with it.
that is worse than what was there before this pr. the old code decoded that window with errors="replace" so you at least saw the replacement character. my version dropped the bytes silently, which is the exact failure this pr is supposed to remove.
the finalizer now closes the tail before it removes the entry:
if exit_code is not None:
if entry.pending_output:
tail, _ = decode_pty_window(entry.pending_output, is_final=True)
entry.pending_output = b""
output += tail.encode("utf-8", errors="replace")
async with self._pty_lock:
removed = self._pty_processes.pop(process_id, None)the test sets a pending first byte on the entry and calls the finalizer with an exited process, so it pins the race down without depending on timing. on the previous commit:
E + hi
FAILED tests/extensions/sandbox/test_modal.py::test_modal_pty_finalize_flushes_a_partial_character_when_the_process_has_exited
the byte is simply gone. now it is hi �.
i checked the other two removal paths as well. pty_terminate_all and _prune_pty_processes_if_needed also drop entries, but neither returns output to the caller, so a tail stranded there was never going to be seen either way. the finalizer is the only one that answers with output.
cloudflare and e2b do not have this shape. their tail goes back on entry.output_chunks, which the next window drains, and collect_pty_output closes the decoder itself when is_done() is true.
one more thing in that commit. i had knocked the @pytest.mark.asyncio off the split test in 91f5fb9. asyncio_mode = "auto" meant it still ran, but every other test in the file carries it, so it is back.
| check | result |
|---|---|
tests/sandbox/test_pty_output.py |
25 passed |
tests/extensions/sandbox |
8 failed, 692 passed, the same 8 that fail with the branch stashed |
tests/sandbox |
2 failed, 1303 passed, the same 2 docker mount ones that fail on main |
| ruff, ruff format | clean |
| mypy on modal | 1 error, exception_module no-redef, present on main too |
the collector polls for the exit code and the finalizer polls again after it. a process that ends between those two polls means the collector kept a partial character back for a next window, and then the finalizer drops the entry and the bytes go with it that is worse than what was there before my change. the old code replaced those bytes in the same window so you saw a question mark. this way the output just loses them quietly so the finalizer closes the tail before it removes the entry. added a test for it, which gives hi instead of hi ? on the last commit also put back the pytest.mark.asyncio i dropped off the split test last time. asyncio_mode is auto so it still ran, but every other test here carries it
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa487b9275
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| chunks = bytearray() | ||
| # a character split across two windows starts in the tail the last one held back | ||
| chunks = bytearray(entry.pending_output) | ||
| entry.pending_output = b"" |
There was a problem hiding this comment.
When a Modal window has stored an incomplete UTF-8 prefix and the next pty_write_stdin call is cancelled at any await in this collection loop, this clears the only retained copy before the call completes. The process remains registered, so a later poll sees the continuation without its lead byte and returns a replacement character, silently corrupting the surviving session's output; keep the prefix on the entry until decoding succeeds or restore it during cancellation.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
| if entry.pending_output: | ||
| tail, _ = decode_pty_window(entry.pending_output, is_final=True) | ||
| entry.pending_output = b"" | ||
| output += tail.encode("utf-8", errors="replace") |
There was a problem hiding this comment.
When max_output_tokens is tight—especially zero—and the process exits between the collector's poll and the finalizer's poll, _collect_pty_output has already truncated output, but this appends the replacement for the pending suffix afterward without updating original_token_count. The returned update can therefore exceed the requested output cap and report a count that excludes the flushed tail; incorporate the tail before truncation or recompute both values here.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
correcting myself on the last comment. i wrote that cloudflare and e2b do not have this shape. that was wrong, and i only checked it properly after posting.
their tail goes back on entry.output_chunks rather than a field, so it looked different, but the race is the same one. collect_pty_output evaluates is_done() once more when it decodes, the finalizer evaluates it again, and if it flips in between the entry is popped with those bytes still in the deque. e2b reads entry.exit_code which the wait task sets, cloudflare reads entry.output_closed, and either can be set by the time the finalizer looks.
fixed both the same way in the pushed commit, draining what is left and closing it before the entry goes:
leftover = bytearray()
async with entry.output_lock:
while entry.output_chunks:
leftover.extend(entry.output_chunks.popleft())
if leftover:
tail, _ = decode_pty_window(leftover, is_final=True)
output += tail.encode("utf-8", errors="replace")that also picks up anything that landed after the collector's last drain, which was being dropped on removal before this pr as well.
added a cloudflare test for it. on the previous commit:
FAILED tests/extensions/sandbox/test_cloudflare.py::test_cloudflare_pty_finalize_flushes_a_partial_character_left_in_the_deque
the output is hi with the byte gone, and hi � now.
so all three backends that keep a tail now close it at the point the entry is removed, and the one place that knows the utf-8 rules is still decode_pty_window.
| check | result |
|---|---|
tests/sandbox/test_pty_output.py |
25 passed |
tests/extensions/sandbox |
8 failed, 693 passed, the same 8 that fail with the branch stashed |
tests/sandbox |
2 failed, 1303 passed, the same 2 docker mount ones that fail on main |
| ruff, ruff format, mypy | clean on the three backends |
i said on the pr that these two did not have the race modal had. that was wrong and i checked it properly after saying it their tail goes back on entry.output_chunks rather than a field, but the shape is the same. collect_pty_output asks is_done one last time, the finalizer asks again, and if it flipped in between the entry is popped with the bytes still sitting in the deque so both finalizers drain what is left and close it before removing the entry. the cloudflare test gives hi instead of hi ? on the last commit
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e983ef2689
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| text, pending = decode_pty_window(output, is_final=is_done()) | ||
| if pending: | ||
| async with output_lock: | ||
| output_chunks.appendleft(pending) |
There was a problem hiding this comment.
When UnixLocal or Docker reports process exit before its output pump closes, collect_pty_output still sees output_closed == False and requeues an incomplete suffix here, but the backend finalizer sees process.returncode/exit_code and immediately removes the entry without draining that suffix. The caller therefore permanently loses the final bytes instead of receiving the replacement character. Fresh evidence beyond the earlier Modal finding is that these two unchanged finalizers use a different completion predicate from the shared collector and received no equivalent tail flush; preserve or flush the prefix before removing the entry.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23b2189665
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| text = output.decode("utf-8", errors="replace") + tail | ||
| truncated, recounted = truncate_text_by_tokens(text, max_output_tokens) |
There was a problem hiding this comment.
When the collection window already exceeded max_output_tokens, output is the formatted, truncated representation rather than the original text, but this recomputes original_token_count from that representation plus the tail and ignores the accurate count passed into the function. For example, a 100-byte window capped at 10 tokens reports about 25 tokens initially, then a pending final byte causes this path to recount only the short displayed truncation marker and tail. Preserve or adjust the existing source count instead of treating truncated output as the source. This is fresh evidence beyond the earlier tail-limit comment: the new recount uses line 58's already-truncated output, so the attempted fix still returns an incorrect count.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I independently rechecked this on the current 5e124bce head and the finding still applies. A concrete case: 100 ASCII bytes truncated at max_output_tokens=10 produce original_token_count=25. Passing that displayed/truncated output into close_pty_tail(...) with a final incomplete é lead byte makes the current helper recount the display representation and return 11; the finalized untruncated source is 100 bytes plus � (3 bytes), whose current approximation is 26 tokens.
There is also an information-loss boundary here: once all we retain is original_token_count=25, the exact final count cannot always be reconstructed after adding a tail. 25 could represent 97–100 source bytes; adding a 3-byte replacement yields an approximate count of either 25 or 26. So recounting from the displayed text is definitely wrong, but exact adjustment from the rounded token count alone is also impossible.
For an exact contract, the narrow fix needs to carry the pre-truncation byte count (internal-only is enough) through collection/finalization, or defer the final truncation/count until the tail is known. A regression should at minimum pin that the 25-token source never collapses to 11; if exact source-count semantics are preserved, the 100-byte example should finalize at 26.
There was a problem hiding this comment.
confirmed, and your numbers are exact. i ran your case:
window count : 25
after close_tail : 11
and it is worse than the count alone. the display is truncated a second time as well, so …22 tokens truncated… becomes …7 tokens truncated… and more of the real output goes.
you are also right about the information loss, and that is what decided the fix. once all that survives is the truncated display plus original_token_count, the exact final count cannot be reconstructed, so any recount there is guesswork. rather than invent one i left that case alone, in 2070572:
if original_token_count is not None:
return output, original_token_countthe reasoning is that a window which already hit the cap has output at the cap, the tail sits past it exactly like the rest of what was dropped, and original_token_count has already told the caller the output is short. one more replacement character beyond the cap adds nothing the caller can use, and dropping it is consistent with everything else past the cap.
the untruncated case is unchanged and still folds the tail in, because there the output really is the whole thing and the count still fits it:
window count : 25 -> after close_tail: 25, display unchanged
untruncated case : 'hi �' count=None
two tests, one per branch. the capped one fails on 5e124bc.
| check | result |
|---|---|
make lint |
passes |
tests/sandbox/test_pty_output.py |
32 passed |
tests/extensions/sandbox |
8 failed, 694 passed, the same 8 with the branch stashed |
tests/sandbox |
2 failed, 1310 passed, the same 2 docker mount ones that fail on main |
| if exit_code is not None: | ||
| output, original_token_count = await flush_pty_tail( | ||
| output_chunks=entry.output_chunks, | ||
| output_lock=entry.output_lock, | ||
| output=output, | ||
| original_token_count=original_token_count, | ||
| max_output_tokens=max_output_tokens, | ||
| ) |
There was a problem hiding this comment.
In UnixLocal, if the subprocess return code becomes visible at the collection deadline before _watch_process_exit has finished the output pumps, this drains only the chunks currently queued and then removes the entry; _terminate_pty_entry subsequently cancels those pumps, so a continuation or other final bytes still buffered in the pipe are lost. Docker has the analogous reader-thread interval. Fresh evidence beyond the earlier Unix/Docker comment is that the added flush still runs before the producer is joined or output_closed is established, whereas _watch_process_exit explicitly waits for every pump before setting that event.
AGENTS.md reference: AGENTS.md:L149-L150
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
all three are right, and rather than patch a fourth finalizer by hand i took this as the complexity reset checkpoint AGENTS.md asks for. pushed f479af8.
why a reset. i had now written the same flush by hand into three finalizers, and each round of review found the next backend i had missed. that is the signal to stop extending the design rather than add another case.
grouping the findings, they are one root cause. holding a partial character back puts it in state that outlives the call, so every path that ends a session has to know to close it. there are seven _finalize_pty_update implementations and they each decide the process is gone with their own predicate. your unix/docker finding is the sharpest example of that:
# unix_local collector
is_done=entry.output_closed.is_set
# unix_local finalizer
exit_code: int | None = entry.process.returncodetwo different sources, so they can genuinely disagree, exactly as you described.
the contract now. one rule, two entry points for the two storage shapes:
def close_pty_tail(*, leftover, output, original_token_count, max_output_tokens): ...
async def flush_pty_tail(*, output_chunks, output_lock, output, original_token_count, max_output_tokens): ...all seven finalizers call one of them, so unix_local, docker, blaxel and daytona are covered too, not just the three i had reached by hand.
the token cap. close_pty_tail truncates the combined text and returns the recount, because the tail is added after the window already applied the cap. that is your second finding, and it now cannot come back per backend since there is one place that folds a tail in.
cancellation. one line. modal no longer clears pending_output before the decode commits:
chunks = bytearray(entry.pending_output)
# the field is left alone until the decode below commitsthe field is only overwritten by decode_pty_window at the end, so a cancelled call leaves the lead byte where the surviving session will find it. the test cancels a collection mid await and asserts the byte is still there. it fails if the clearing line goes back in.
max_output_tokens is threaded into the finalizers with a None default, so the existing test_pty_finalize_done_session in test_blaxel.py keeps working unchanged.
| check | result |
|---|---|
make lint |
passes |
make typecheck |
46 errors, and 46 on main with the branch stashed, so none of them are mine |
tests/sandbox/test_pty_output.py |
28 passed |
tests/extensions/sandbox |
8 failed, 694 passed, the same 8 with the branch stashed |
tests/sandbox |
2 failed, 1306 passed, the same 2 docker mount ones that fail on main |
one thing i want to put to you rather than decide myself. the issue is about collect_pty_output, and this now touches all seven backends. i think it belongs together because the collector is what strands the tail, so leaving six finalizers unaware of it would ship a known hole. but if you would rather this pr stayed at the shared helper plus routing e2b and cloudflare through it, say so and i will move the finalizer contract into a follow up.
There was a problem hiding this comment.
I rechecked this thread against the current 2070572e head. The complexity reset centralized tail decoding, but the Unix/Docker producer-completion race is still present because finalization still uses a different terminal predicate from collection.
Unix currently has:
# collector finality
is_done=entry.output_closed.is_set
# output_closed is set only after process.wait() + all pump tasks
await entry.process.wait()
await asyncio.gather(*entry.pump_tasks, return_exceptions=True)
entry.output_closed.set()
# finalizer finality
exit_code = entry.process.returncode
if exit_code is not None:
await flush_pty_tail(...)
...
await self._terminate_pty_entry(removed) # cancels pump_tasksDeterministic ordering: the process has returncode=0, b"\xc3" is requeued, but b"\xa9" is still buffered behind a pump task. The finalizer observes the return code first, drains/closes b"\xc3", removes the entry, and _terminate_pty_entry cancels the pump before it can enqueue b"\xa9". The continuation is lost and the output closes as � rather than é.
Docker has the same split source of truth: _watch_pty_exit can set entry.exit_code before _pump_pty_socket reaches its finally and calls _mark_pty_output_closed, while _finalize_pty_update currently finalizes on entry.exit_code alone.
The smallest correction is to make removal/finalization use the same producer-drained predicate as the collector:
# unix_local
exit_code = entry.process.returncode if entry.output_closed.is_set() else None
# docker
if entry.output_closed.is_set() and entry.exit_code is None:
await self._refresh_pty_exit_code(entry)
exit_code = entry.exit_code if entry.output_closed.is_set() else NoneCloudflare already follows this shape. A controlled regression should expose process/exit-code first while blocking the final pump, assert that the first finalizer leaves the session live and does not close the pending prefix, then release the pump and verify the next/final update returns é. That pins the lifecycle invariant rather than just the shared helper.
There was a problem hiding this comment.
confirmed, and this was the more important of the two. fixed in ff58b75 with your correction.
unix_local finalised on entry.process.returncode while collection waited for entry.output_closed, which is only set after the process is reaped and every pump has drained. so the finaliser could decide the session was over, remove it, and _terminate_pty_entry would cancel a pump that still held the rest of a character. docker had the same split, with _watch_pty_exit able to set exit_code before _pump_pty_socket reached its finally.
both now use the drained predicate collection already uses:
# unix_local
exit_code: int | None = (
entry.process.returncode if entry.output_closed.is_set() else None
)
# docker
exit_code = entry.exit_code if entry.output_closed.is_set() else Noneas you said, cloudflare already had this shape, which is why it was the one backend not affected.
this is the better fix than the tail flush, because it removes the divergence rather than compensating for it. the flush stays as the narrow safety net for the remaining case where the flag flips between collection and finalisation, but it is no longer papering over two different sources of truth.
i have not added the controlled pump-blocking lifecycle test yet. it needs a fake that holds a pump task open while the exit code is already visible, and i would rather put that in properly than bolt it on at the end of this thread. tell me if you want it here and i will add it before this merges.
| check | result |
|---|---|
make lint |
passes |
mypy src/agents/sandbox |
clean |
tests/sandbox/test_pty_output.py |
33 passed |
tests/extensions/sandbox |
8 failed, 694 passed, identical with the branch stashed |
tests/sandbox |
2 failed, 1311 passed, the same 2 docker mount ones that fail on main |
There was a problem hiding this comment.
Verified on current head bf682c80cdd387a6522c6e8ab68bd666b92de13e: the ff58b757 Unix/Docker predicate correction is still present and matches the collector's producer-drained completion condition. Yes — I would add the controlled pump-blocking regression before merge. This is a concurrency/lifecycle invariant, and without a test a future cleanup could easily regress back to returncode/exit_code as the removal predicate.
The test can stay deterministic: expose the process exit code first while keeping the final pump/reader blocked; call _finalize_pty_update and assert it leaves process_id live and does not close/remove the queued UTF-8 lead byte. Then release the pump with the continuation, mark output_closed, call finalization again, and assert the final update returns é and removes the session. The Docker analogue should pin the same ordering (exit_code visible before _mark_pty_output_closed). Even one backend-specific lifecycle regression plus the shared invariant assertion would be materially better than relying on timing.
There was a problem hiding this comment.
added, in the pushed commit. you were right to want it, and it turned out to be the test i would have missed most, since the predicate fix is a one line change that reads like a tidy-up and would be easy to undo.
it does the ordering you described. the process is already reaped, output_closed is not set because the pump still holds the continuation, and a previous window has handed the lead byte back:
entry.output_chunks.append(raw[:1])
...
assert first.process_id == 1 # session stays live
assert first.exit_code is None
assert list(entry.output_chunks) == [raw[:1]] # prefix not closed
assert 1 in session._pty_processesthen the pump delivers the rest and closes, and the next update finishes it:
assert final.output.decode("utf-8") == "é"
assert final.exit_code == 0
assert final.process_id is Nonei checked it fails for the right reason rather than just passing. putting entry.process.returncode back on its own gives:
E + where None = PtyExecUpdate(process_id=None, output=b'\xef\xbf\xbd', exit_code=0, original_token_count=None).process_id
which is exactly the sequence you predicted, the session closed early with � as its whole output.
docker is not covered by a test of its own. its split is the same shape and the same one line correction, but reaching it needs a fake socket pump and an exit watcher, and i did not want to write a second harness that proves the same invariant. say if you would rather it had one.
thanks also for going back to the base commit on the drain finding and pulling it. that saved me chasing something that was never mine.
| check | result |
|---|---|
make lint |
passes |
tests/sandbox/test_unix_local.py |
16 passed |
tests/sandbox/test_pty_output.py |
33 passed |
tests/sandbox |
2 failed, 1312 passed, the same 2 docker mount ones that fail on main |
tests/extensions/sandbox |
8 failed, 695 passed, identical with the branch stashed |
There was a problem hiding this comment.
Verified on new head 590a28b782235c04710ce44055af2f20a32db679: the added test_session_is_not_finalized_while_a_pump_still_holds_output pins the lifecycle ordering I requested. It presents returncode=0 while output_closed is false, proves the first finalizer leaves the session live and the lead byte queued, then supplies the continuation + output_closed and proves the final update returns é and removes the session. That is deterministic and directly fails the old returncode-only predicate. I consider the missing Unix lifecycle-regression request addressed.
There was a problem hiding this comment.
I would stop at the Unix controlled regression for this PR. It directly pins the new lifecycle invariant, and Docker already has test_docker_pty_exec_waits_for_socket_drain_after_process_exit exercising its socket-drain path. A second bespoke race harness would add maintenance cost without much new contract coverage here. The separate Docker predicate line is simple enough to keep under code review; I would only add backend-parity coverage if maintainers specifically want it.
this is the complexity reset AGENTS.md asks for. i had written the same flush by hand into three finalizers and was about to write a fourth, and every round of review found the next backend i had missed the root cause of all of it is one thing. holding a partial character back puts it in state that outlives the call, and then every path that ends a session has to know to close it. there are seven of those and they each decide the process is gone with their own predicate. unix_local reads process.returncode while its collector reads output_closed, so they genuinely disagree so there is now close_pty_tail for the rule and flush_pty_tail for draining a deque, and all seven finalizers call one of them. that also fixes the token cap, since the tail used to be appended after the window had already truncated modal no longer clears its tail before the decode commits, so a cancelled call keeps it rather than losing the lead byte for the session that survives
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
I audited the latest f479af8d state specifically for cancellation/commit boundaries. There is one remaining shared-helper race below.
| # decoder once the provider is done replaces a tail that no later window will finish. | ||
| text, pending = decode_pty_window(output, is_final=is_done()) | ||
| if pending: | ||
| async with output_lock: |
There was a problem hiding this comment.
Cancellation can still drop the pending UTF-8 prefix in the shared deque path. By this point the source bytes have already been drained from output_chunks, and pending exists only in this local variable; acquiring output_lock is cancellable. If the caller is cancelled while another producer holds the lock, appendleft() never runs, the PTY entry can remain alive, and the later continuation is decoded without its lead byte.
I reproduced this deterministically against the helper logic at f479af8d: start with output_chunks=[b"\xc3"], let the collector drain it, hold output_lock until the collection deadline so the task blocks here, cancel the task, then release the lock. The deque is empty. Appending b"\xa9" and doing the next/final collection returns b"\xef\xbf\xbd" (�) instead of completing é.
This is the same persistence invariant the Modal change now protects by keeping entry.pending_output until decode commits. The shared deque path needs an equivalent cancellation-safe/transactional carry (ideally entry-owned state, or otherwise restoration that completes before cancellation propagates), plus a regression test that cancels while this requeue lock is held.
There was a problem hiding this comment.
thanks, this is a good catch and your repro is exact. i reproduced it before changing anything. my first attempt held the lock from the start, which just blocks the loops first drain and leaves the byte in the deque, so nothing is lost. following your ordering, drain first and then hold the lock across the deadline, gives it:
drained, deque now : []
deque after cancel : []
next window returns : b'\xef\xbf\xbd'
decoded : '�'
fixed in 5e124bc, though not the way you suggested, so let me say why.
you offered entry owned state or restoration that completes before cancellation propagates. i went for a third option, which is to remove the cancellation point instead of recovering from it:
text, pending = decode_pty_window(output, is_final=is_done())
if pending:
# Deliberately not under ``output_lock``. Those bytes have already left the deque, so
# this is the only copy, and awaiting the lock is a cancellation point ...
output_chunks.appendleft(pending)the reasoning is that this lock is not protecting a single append. i went through every holder of it:
| holder | critical section |
|---|---|
| cloudflare, e2b, blaxel, daytona, unix_local pumps | one output_chunks.append(...) |
| docker pump | one output_chunks.extend(...) |
collect_pty_output |
while output_chunks: popleft() |
flush_pty_tail |
while output_chunks: popleft() |
nothing does a read modify write across an await, and nothing clears and rebuilds. the lock exists to make the multi step drain atomic. appendleft is a single synchronous call, tasks only switch at an await so it cannot land inside one of those drain loops, and deque is documented safe for append/appendleft even from another thread. so taking the lock buys nothing here and costs the cancellation window you found.
restoration would also have worked, but it leaves a window to get wrong later. this way there is no await between the decode and the byte being back in the deque, so there is nothing to interrupt.
regression test follows your repro directly. it drains the lead byte, has a producer hold the lock across the deadline, cancels the task, then releases and checks both that the byte is still queued and that the next window still gives é. on f479af8:
FAILED tests/sandbox/test_pty_output.py::test_collect_pty_output_keeps_the_tail_when_a_window_is_cancelled
worth noting the modal path you compared this to is protected differently and still correctly. it keeps entry.pending_output until decode_pty_window overwrites it, so there is no await between reading and committing there either. same invariant, reached from the other side.
| check | result |
|---|---|
make lint |
passes |
tests/sandbox/test_pty_output.py |
29 passed |
tests/extensions/sandbox |
8 failed, 694 passed, the same 8 with the branch stashed |
tests/sandbox |
2 failed, 1307 passed, the same 2 docker mount ones that fail on main |
There was a problem hiding this comment.
Verified against 5e124bce410cc9ce716d21eaf5e2d95624ccbe7f: this fixes the exact cancellation point I reported. There is now no await between decode_pty_window(...) producing pending and the synchronous appendleft(pending), so my original drain → hold lock across deadline → cancel ordering no longer loses the lead byte; the next window can complete é. I’m resolving this thread. The newer cancellation-during-collection finding in the separate Codex thread is a different boundary and remains unresolved.
the bytes have already left the deque when the window decodes, so the local pending is the only copy of them. awaiting output_lock to put it back is a cancellation point, and a caller cancelled while a producer holds the lock loses the character while the session carries on to read its continuation appendleft is one synchronous call and deque is documented safe for it. every holder of that lock is either a single append or a drain loop with no await inside, so there is nothing for this to interleave with. the lock is there for the multi step drain, not for one append doing it without the lock removes the cancellation point rather than recovering from it, so there is no window left to get wrong test follows the reported repro. it drains the lead byte, holds the lock across the deadline, cancels, and checks the byte is still there and the next window still gives e acute
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e124bce41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # and it is handed back for the next window to finish. Bytes that cannot begin a | ||
| # character are not held, they are replaced straight away as before. Completing the | ||
| # decoder once the provider is done replaces a tail that no later window will finish. | ||
| text, pending = decode_pty_window(output, is_final=is_done()) |
There was a problem hiding this comment.
When a first window requeues an incomplete lead byte and the next collection is cancelled while awaiting output, that call has already drained the lead byte into its local output but never reaches this decode/requeue step. The surviving session then receives only the continuation byte and returns �, silently losing the character. Fresh evidence beyond the earlier Modal-specific finding is that this affects every backend using collect_pty_output; retain ownership on the entry until the window commits or restore drained bytes on cancellation.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I reproduced this separately on 5e124bce; it is distinct from the cancellation point that commit fixed. Here the second collection drains the previously requeued b"\xc3" into its local output, then cancellation arrives while it is awaiting output_notify, so the task exits before decode_pty_window() and the deque is empty. Appending only b"\xa9" afterward yields �.
A narrow repair fits the same invariant already used by 5e124bce: restore the entire drained local window synchronously before propagating cancellation, e.g. wrap the collection loop and on asyncio.CancelledError do output_chunks.appendleft(bytes(output)) when output is non-empty, then raise. That single appendleft has no await, preserves the older drained bytes ahead of anything producers appended meanwhile, and also prevents ordinary (non-UTF-8-specific) output drained by a cancelled poll from disappearing.
The regression should cancel while the collector is still inside the wait_for(output_notify.wait(), ...) phase—not after the deadline—because the existing test_collect_pty_output_keeps_the_tail_when_a_window_is_cancelled now exercises the later decode/requeue boundary that 5e124bce removed.
There was a problem hiding this comment.
you are right, and it is distinct from the one 5e124bc fixed. reproduced it exactly as you set it out, cancelling while the loop waits on output_notify after it has drained the lead byte:
drained, deque now : []
deque after cancel : []
next window : b'\xef\xbf\xbd' -> '�'
fixed in 2070572, and i took your suggested shape since the whole drained window has the same problem, not only the requeued byte:
except asyncio.CancelledError:
# Everything drained so far lives only in this buffer, and the session outlives a
# cancelled call, so put it back before the cancellation goes on.
if output:
output_chunks.appendleft(bytes(output))
raiseappending the whole buffer as one chunk at the front keeps it in order ahead of anything the producer added while this call was running.
worth being clear about what is new here and what is not. a cancelled collection always lost what it had drained, and before this pr that was only ever the window going with the call that asked for it. what this pr changed is that the buffer can now hold a lead byte an earlier window requeued, so the loss became visible to the session that carries on rather than dying with the caller. so the fix is mine to make.
same repro after:
deque after cancel : [b'\xc3']
next window : b'\xc3\xa9' -> 'é'
regression test cancels while the window waits and asserts both that the byte is back in the deque and that the next window completes é. it fails on 5e124bc.
There was a problem hiding this comment.
Verified on current head 2070572ee89dc983ebf9de67dfba2bfdfaabefdb: the cancellation-during-collection case in this thread is fixed. The CancelledError handler now restores the complete drained local buffer with a synchronous appendleft(bytes(output)) before cancellation propagates, preserving ordering ahead of later producer output. The new regression also cancels during the long output_notify wait (the boundary this finding required), then verifies both the restored b"\xc3" and the subsequent completion of é. I consider this specific finding addressed; the remaining Modal cancellation transaction and finalization/lifecycle findings are separate threads.
…d one two things from review a cancelled collection loses everything it had drained, because that only lives in the local buffer. before this pr that was just the window going with the call that asked for it, but now a lead byte a previous window requeued can be in there, so the session that carries on reads the continuation alone and reports a replacement character for output that did arrive. the loop now puts the buffer back before the cancellation goes on close_pty_tail was folding the tail into output the window had already truncated. that truncates it a second time and recounts the shortened display instead of the source. for 100 bytes at a cap of 10 the window reports 25 tokens and this turned it into 11, with the visible text cut down again as well. a window that already hit the cap is now left alone, since the tail is past the cap like everything else that got dropped and the count already says the output is short both tests fail on 5e124bc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2070572ee8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if original_token_count is not None: | ||
| return output, original_token_count |
There was a problem hiding this comment.
When the collected window already exceeded max_output_tokens, this early return preserves the count measured before finalization even though flush_pty_tail may have drained additional complete chunks or an incomplete suffix that becomes �. The returned PtyExecUpdate.original_token_count therefore understates the actual source output; fresh evidence beyond the earlier recount issue is that the current finalizer drains arbitrary queued bytes into leftover and then this branch excludes all of them from accounting.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I re-audited 2070572e and there is a second user-visible consequence of this same information-loss boundary: the early return also preserves a stale suffix, even though the repository's token truncation policy intentionally keeps both the beginning and the end of the source.
Concrete current-head repro using the existing truncator:
source before final flush : "A" * 100
max_output_tokens : 10
window display : AAAAAAA…22 tokens truncated…AAAAAAAA
window count : 25
leftover flushed at exit : <<<FINAL-ERROR>>>
close_pty_tail() at 2070572e returns that same display/count. But truncating the actual finalized source once ("A" * 100 + "<<<FINAL-ERROR>>>") produces:
AAAAAAA…26 tokens truncated…ERROR>>>
original_token_count = 30
So the final bytes are not merely "past the cap": they are supposed to participate in the retained right-hand suffix. The current branch can hide exactly the final error/status text that tail-preserving truncation is meant to keep.
The exact fix cannot be reconstructed from the formatted display + rounded count, as discussed above. The narrow lossless boundary is to carry the untruncated decoded window text (internal-only) from collection to finalization. If finalization drains a tail, decode it as final and run truncate_text_by_tokens(source_text + tail, max_output_tokens) once; if there is no tail, keep the already-rendered result. This needs no public PtyExecUpdate field and fixes both the stale suffix and the source count exactly. A structured head/tail + exact byte-count state would also work, but is more machinery for the same contract.
I would add the regression above in addition to the count-only case: assert that ERROR>>> survives in the finalized display and that the count becomes 30.
There was a problem hiding this comment.
you are right and i had the truncation policy wrong. i had assumed it kept the head, so "past the cap" seemed like a safe thing to say about a tail. it keeps the head and the tail, which makes a tail arriving at session end exactly the part that is meant to survive. that branch could hide the last thing a process said, which is the worst possible thing to drop.
implemented your design in ff58b75. collect_pty_output now also returns the decoded window before truncation, the backends carry it to finalisation, and a tail is folded into that and truncated once:
truncated, counted = truncate_text_by_tokens(source_text + tail, max_output_tokens)your repro, on the new head:
window display : AAAAAAA…22 tokens truncated…AAAAAAAA
window count : 25
after flush : AAAAAAA…26 tokens truncated…ERROR>>>
count : 30
expected : AAAAAAA…26 tokens truncated…ERROR>>>
expected count : 30
MATCH : True
it is a third return value on the helper, internal only, and no public field carries it. that was the cheaper of the two shapes you offered, and the structured head/tail state does not buy anything the source text does not.
added the regression you asked for, asserting ERROR>>> survives and the count becomes 30, plus a second one pinning that the count never comes back lower than the window measured. the codex thread about flushed bytes being missing from the count is covered by the same change, since the count now comes from truncating the real source rather than being carried over.
There was a problem hiding this comment.
Verified against current head bf682c80cdd387a6522c6e8ab68bd666b92de13e: ff58b757 implements the lossless source-text design correctly. collect_pty_output now returns the decoded pre-truncation window text internally, every backend threads it into finalization, and close_pty_tail computes truncate_text_by_tokens(source_text + tail, max_output_tokens) exactly once. The previous stale-suffix/count case is therefore fixed: the final retained suffix can contain ERROR>>> and the count is recomputed from the completed source rather than the rendered window. I consider this specific finding addressed.
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
I re-audited the current 2070572e head after the shared cancellation repair. Modal still has a separate transaction boundary for bytes already consumed from its streams.
|
|
||
| text = chunks.decode("utf-8", errors="replace") | ||
| exited = await self._peek_exit_code(entry.process) is not None | ||
| text, entry.pending_output = decode_pty_window(chunks, is_final=exited) |
There was a problem hiding this comment.
Cancellation can still silently drop newly read Modal output, even though pending_output itself is no longer cleared early. Once stdout_chunk/stderr_chunk has returned, the underlying stream item is consumed and those bytes live only in local chunks until this line commits. Any later await in the loop (the other stream read, _peek_exit_code, sleep/drain, or this final poll) can be cancelled while the registered session survives; the next call then starts only from the old entry.pending_output, so the consumed bytes never reappear.
I reproduced this deterministically against the current shape with entry.pending_output=b"OLD": make the stdout read return b"NEW", block the following stderr read, then cancel the collector. After cancellation the stdout stream is empty and entry.pending_output is still only b"OLD"; b"NEW" is lost. The existing cancellation test uses never-ending streams, so it proves the old prefix survives but never exercises a chunk consumed before cancellation.
The narrow transactional fix is to persist the local buffer only on cancellation, avoiding per-chunk copying:
chunks = bytearray(entry.pending_output)
try:
# existing collection loop + final exit poll
...
except asyncio.CancelledError:
entry.pending_output = bytes(chunks)
raise
text, entry.pending_output = decode_pty_window(chunks, is_final=exited)A regression can make stdout return one chunk, block inside the following stderr read, cancel there, and assert the consumed stdout bytes are now in entry.pending_output and are returned by the next collection. This is the Modal equivalent of restoring the shared deque's drained window before cancellation propagates.
There was a problem hiding this comment.
you are both right, and this one is worse than losing the bytes. fixed in the pushed commit.
not clearing pending_output only protected the carried lead byte. a stream item is gone from the stream once it has been read, so a continuation this call had already swallowed was still lost, and the next window would then pair the surviving lead byte with whatever arrived after it. that is a wrong character rather than a missing one, which is the worse failure.
there is no deque to hand it back to here, so the whole buffer goes on the entry, carried tail and new reads together:
except asyncio.CancelledError:
# A stream item is gone from the stream once it has been read, so anything already
# taken lives only in this buffer. There is no deque to hand it back to, and the
# session outlives a cancelled call, so it goes on the entry for the next window.
if chunks:
entry.pending_output = bytes(chunks)
raisethe try covers every await in the collection, including the drains and the final _peek_exit_code, since any of them can be the one that gets cancelled. assigning rather than appending means the carried tail is not duplicated, because chunks already starts with it.
the test gives the entry a lead byte, lets the collection read the continuation off the stream, then cancels at a later await and asserts both halves are on the entry. on ff58b75:
FAILED tests/extensions/sandbox/test_modal.py::test_modal_pty_collection_keeps_bytes_it_already_read_when_cancelled
| check | result |
|---|---|
make lint |
passes |
tests/extensions/sandbox |
8 failed, 695 passed, the same 8 with the branch stashed |
tests/sandbox |
2 failed, 1311 passed, the same 2 docker mount ones that fail on main |
@fscfede-beep thanks for confirming the shared deque path separately, that thread is settled from my side too.
There was a problem hiding this comment.
Verified against current head bf682c80cdd387a6522c6e8ab68bd666b92de13e: this exact Modal cancellation boundary is fixed. The try/except asyncio.CancelledError now covers the full collection loop, drains, and final exit poll, and it commits bytes(chunks) back to entry.pending_output before cancellation propagates. The new regression consumes the UTF-8 continuation from the stream, cancels at a later await, and proves both halves survive on the entry. I consider this specific finding addressed.
two more from review, both correct truncation keeps the start and the end of the output, which i had wrong. i was treating a tail found at session end as past the cap and leaving it out, but it belongs in the end that truncation is meant to keep, so that branch could hide the last thing a process said. the window now carries its decoded text before truncation, and a tail is folded into that and truncated once. for the reported case the display goes from ending AAAAAAAA to ending ERROR>>> and the count from 25 to 30, which is what truncating the real source gives unix_local finalised on process.returncode while collection waited for output_closed, which is only set after the process is reaped and the pumps have drained. so the finaliser could remove the session and cancel a pump that still held the rest of a character. docker had the same split through exit_code. both now use the same drained predicate collection uses, which is the shape cloudflare already had the carried text is internal, no public field holds it
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff58b7572c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| chunks = bytearray() | ||
| # a character split across two windows starts in the tail the last one held back. the | ||
| # field is left alone until the decode below commits, so a cancelled call keeps it | ||
| chunks = bytearray(entry.pending_output) |
There was a problem hiding this comment.
When a Modal session enters this window with a saved UTF-8 lead byte, then _read_modal_stream returns its continuation and the task is cancelled at a later await (such as the stderr read, exit poll, or sleep), only the old entry.pending_output survives; the continuation already appended to local chunks is dropped, so a later poll corrupts the process output. Fresh evidence beyond the earlier pending-prefix comment is that this line preserves only the preexisting bytes, while the changed collector has no cancellation handler to commit or restore bytes read afterward; restore the complete local buffer without duplicating the original prefix before propagating cancellation.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
a stream item is gone from the stream once it has been read, so anything the window has taken lives only in the local buffer until the decode commits. any await after that read can be cancelled, the stderr read, the exit poll, the sleep, the drains or the final poll not clearing pending_output only saved the carried lead byte. the continuation this call had already swallowed was still lost, and the next window would then pair that lead byte with whatever arrived after it, which is worse than dropping it so the whole buffer goes on the entry before the cancellation goes on, carried tail and new reads together, and the next window picks up where this one stopped test fails on ff58b75
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
Re-auditing bf682c80 found one remaining Modal lifecycle hole: the exit-path drain still conflates a read that is merely not ready after 200 ms with actual stream EOF.
| if remaining_s <= 0: | ||
| exit_code = await self._peek_exit_code(entry.process) | ||
| if exit_code is not None: | ||
| stdout_chunks = await self._drain_modal_stream( |
There was a problem hiding this comment.
Self-correction after checking the PR base (89c02c828ee8510fe9a84ee6675608193aa13b02): this is a real Modal lifecycle weakness, but it predates this PR and I should not have presented it as a blocking finding on #4745.
The base already had the same two-state drain:
wait_timeout = 0.2 if await_pending else 0
done, _ = await asyncio.wait({task}, timeout=wait_timeout)
if not done:
return b""and _drain_modal_stream() already treated that b"" exactly like EOF. The deterministic delayed-final-chunk repro still demonstrates the pre-existing behavior, but #4745 did not introduce it. Under this repository's review rules, that belongs in a separate follow-up rather than as a defect charged to this PR.
So: please do not block or expand #4745 for this comment. I withdraw it as a PR finding. The producer-drained idea may still be useful for a dedicated Modal follow-up, but the current PR should be judged on the regressions it actually changes. My earlier request for a controlled Unix/Docker lifecycle regression remains in-scope because that predicate was changed by this PR.
fscfede-beep
left a comment
fscfede-beep
left a comment
There was a problem hiding this comment.
One in-scope cancellation boundary remains in the new final-tail path at bf682c80: finalizers destructively consume entry-owned tail state before they acquire the session-map lock that commits removal.
| live_process_id: int | None = process_id | ||
|
|
||
| if exit_code is not None: | ||
| output, original_token_count = await flush_pty_tail( |
There was a problem hiding this comment.
The new tail flush is destructive before finalization owns the removal transaction. flush_pty_tail() drains entry.output_chunks; only afterwards does this method await self._pty_lock. If the caller is cancelled while that lock is contended, the PTY entry remains registered but its final bytes are already gone from entry-owned state. A later poll cannot recover them.
This is an in-scope regression from the new final-tail machinery (and the same ordering is repeated in the other deque-backed finalizers; Modal has the analogous close_pty_tail(...) + entry.pending_output = b"" before awaiting _pty_lock). The collection path now correctly restores drained bytes on cancellation, but finalization has the same survivor invariant one await later.
Deterministic regression:
- finished entry is still in
_pty_processes, with a final queued tail; - hold
self._pty_lockfrom another task; - start
_finalize_pty_updateand wait untilflush_pty_tailhas emptiedentry.output_chunks; - cancel the finalizer while it is blocked acquiring
_pty_lock; - assert the entry is still registered and the tail is still recoverable by the next poll.
Current head fails step 5: the map still owns the entry, but the queue is empty. For Modal the same test can assert pending_output survives cancellation before map removal.
The invariant is: either removal commits with the tail delivered, or cancellation leaves the entry's tail intact. The narrowest implementation is to make destructive tail transfer and map removal one transaction—e.g. acquire _pty_lock, revalidate that process_id still maps to this entry, then flush/close the tail and synchronously pop before releasing the lock, if that lock ordering is acceptable. Otherwise flush_pty_tail needs to expose/restorable raw leftover so CancelledError before removal can put it back. A regression with a held _pty_lock pins the ownership boundary without timing.
There was a problem hiding this comment.
I also checked the lock graph on current head 590a28b7 before recommending the transaction-first shape. In all seven changed backends, producer paths take only entry.output_lock for a short append/extend and do not acquire _pty_lock while holding it; _pty_lock map operations likewise do not currently run under output_lock. So _pty_lock -> output_lock in finalization does not introduce an obvious ABBA cycle in the changed PTY paths.
That makes the simplest repair viable: acquire _pty_lock, revalidate registry.get(process_id) is entry, then await flush_pty_tail (or close/clear Modal's pending_output), synchronously pop/discard, release _pty_lock, and only then await backend termination. Cancellation while waiting for output_lock is safe because no tail has been drained yet; once output_lock is acquired, the drain + pop path has no further suspension before the removal commit.
I would still pin this with the held-_pty_lock cancellation regression from the first comment, because that is the exact ownership boundary this change is meant to protect.
There was a problem hiding this comment.
right, and it is mine, the flush is what put a destructive read in front of that lock. fixed in the pushed commit with the transaction first shape you describe.
the removal commits first and the drain follows it:
if exit_code is not None:
async with self._pty_lock:
removed = self._pty_processes.pop(process_id, None)
self._reserved_pty_process_ids.discard(process_id)
# Draining is destructive and the tail lives on the entry, so the removal has to
# commit first. Cancelled the other way round, the session stays registered with
# its last bytes already gone and a later call cannot get them back.
output, original_token_count = await flush_pty_tail(...)
if removed is not None:
await self._terminate_pty_entry(removed)the asymmetry is the point. cancelled after the removal, the bytes go with a session nobody can read from any more. cancelled before it, under the old order, they went while the session was still listed and still pollable, which is the case that actually loses output. all seven finalisers now do it this way, modal included, where reading the tail is what clears the field.
thanks for checking the lock graph first. i verified the same thing before pushing, producers only take entry.output_lock for a single append or extend and never reach for _pty_lock underneath it, and no _pty_lock map operation runs under output_lock, so _pty_lock then output_lock here has no cycle to make. draining still happens before _terminate_pty_entry so the pump is not cancelled out from under it.
test holds the map lock, cancels a finalise while it is blocked, then checks nothing was consumed and the session is still there to be finalised properly afterwards. on 590a28b it fails.
on docker, taking your answer, i will leave it at the unix regression. i had not spotted test_docker_pty_exec_waits_for_socket_drain_after_process_exit, and it does cover that path. worth saying plainly that i cannot run it here, tests/sandbox/test_docker.py fails to import for me with ModuleNotFoundError: No module named 'docker', which is true on main as well. so the docker predicate line is covered by that test in ci and by review, not by anything i have executed.
| check | result |
|---|---|
make lint |
passes |
mypy src/agents/sandbox |
clean |
tests/sandbox/test_unix_local.py |
17 passed |
tests/sandbox/test_pty_output.py |
33 passed |
tests/sandbox |
2 failed, 1313 passed, the same 2 docker mount ones that fail on main |
tests/extensions/sandbox |
8 failed, 695 passed, identical with the branch stashed |
There was a problem hiding this comment.
Verified current head bdef1730a6 independently at the source/diff level. The transaction-first ordering is present in all seven finalizers changed by this commit (Unix, Docker, Blaxel, Cloudflare, Daytona, E2B, Modal): the registry removal commits under _pty_lock before flush_pty_tail / close_pty_tail can destructively consume entry-owned bytes. I also verified test_finalize_does_not_consume_the_tail_before_removal_commits: it holds _pty_lock, starts finalization, cancels while blocked, then asserts both the queued lead byte and registered session survive. That pins the exact cancellation boundary from this thread. I did not execute the upstream suite in my environment, so this is source/regression verification rather than an independent runtime test result. This addresses my transaction-ordering finding; no further blocker from this thread.
the predicate fix had no test, so a later cleanup could put returncode back and nothing would say anything. this holds a process that is already reaped while its pump still has the rest of a character, and checks the first finalise leaves the session alive with the lead byte still queued, then releases the pump and checks the next one gives e acute and closes against the old returncode only predicate it fails the way it should, closing the session early with the replacement character as its output
draining empties entry owned state, and it was happening before the finaliser took the session map lock. cancelled while that lock is contended, the session is still registered but its last bytes are already gone, and a later call has nowhere to get them from so the removal goes first now and the drain follows it. cancelled after that point the session is gone anyway, so the bytes go with a session nobody can read from rather than with one that is still listed. same for modal, where reading the tail clears the field test holds the map lock, cancels a finalise, and checks nothing was consumed and the session is still there to finalise properly. it fails on 590a28b
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdef1730a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Draining is destructive and the tail lives on the entry, so the removal has to | ||
| # commit first. Cancelled the other way round, the session stays registered with | ||
| # its last bytes already gone and a later call cannot get them back. | ||
| output, original_token_count = await flush_pty_tail( |
There was a problem hiding this comment.
When a final Blaxel PTY call is cancelled after the entry is removed from _pty_sessions but while flush_pty_tail() awaits output_lock, cancellation exits before _terminate_pty_entry(removed) runs. The removed entry is then unreachable by pty_terminate_all(), leaving its WebSocket and aiohttp.ClientSession unclosed; run termination in a cancellation-safe finally or otherwise guarantee cleanup after the removal commits.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed on bdef1730: this is the necessary counterpart to the removal-first transaction. Once the map pop commits, cleanup has to become cancellation-settled, because no later pty_terminate_all() can reach that entry. Blaxel is the clearest leak (WS + aiohttp.ClientSession), but the same ordering now exists in the other finalizers that pop before flush_pty_tail() / close_pty_tail() and then await backend termination.
There is already a repository-local pattern for exactly this ownership boundary in Daytona startup cleanup: create a cleanup task, shield it, and if the caller is cancelled while waiting, keep waiting for the same cleanup task before propagating the primary cancellation. The stronger mount-lifecycle helper uses the same principle in a loop so repeated cancellation cannot abandon cleanup.
I would keep the new remove first ordering (it fixes the survivor/tail-loss race) and make everything after successful removal a settled cleanup region. Conceptually:
removed = ...pop(...)
try:
output, count = await flush_pty_tail(...)
finally:
if removed is not None:
cleanup = asyncio.create_task(self._terminate_pty_entry(removed))
completion = asyncio.create_task(asyncio.wait((cleanup,)))
while not completion.done():
try:
await asyncio.shield(completion)
except asyncio.CancelledError:
pass
completion.result()
# _terminate_pty_entry already treats provider cleanup failures as non-fatal where neededThe exact helper can be factored once rather than copied seven times. Important invariant: after registry removal, caller cancellation may abort output delivery, but it must not abort resource teardown. Regression: hold entry.output_lock, let finalization pop the entry, cancel while flush_pty_tail() is blocked, release the lock, and assert the backend cleanup completes (for Blaxel: WS and HTTP session closed) before the cancellation escapes.
There was a problem hiding this comment.
both right, and this one is on me twice over, since it is the gap my own reordering opened. fixed in the pushed commit.
the point i missed is the one you put plainly: once the pop commits, cleanup has to be settled, because nothing can reach that entry again. pty_terminate_all walks _pty_sessions, and the entry is not in it any more. blaxel is the clearest case, _terminate_pty_entry there closes the websocket and the aiohttp.ClientSession, so both were being left open.
the drain is in a try now and the terminate is in the finally:
try:
output, original_token_count = await flush_pty_tail(...)
finally:
if removed is not None:
await self._terminate_pty_entry(removed)all seven, and you were right that the ordering is now the same everywhere so the exposure was too, not only blaxel. modal is the one that could not actually lose it today, since close_pty_tail is synchronous and there is no await between its pop and its terminate, but it is written the same way so that an await added in front of it later does not quietly start leaking.
test holds entry.output_lock so the drain blocks, checks the removal has already committed, then cancels and asserts the entry was still terminated. on bdef173:
FAILED tests/sandbox/test_unix_local.py::TestUnixLocalPty::test_finalize_still_cleans_up_when_the_drain_is_cancelled
one thing i have not done, and will not pretend otherwise. a second cancellation landing inside the finally itself would still skip the close. shielding it would fix that, but it turns cleanup into fire and forget with nothing owning the result, and this repo already has a pattern for owned background cleanup in unix_local._fd_close_tasks. that is a bigger call than this thread, so i have left it and am flagging it rather than reaching for shield on my own.
| check | result |
|---|---|
make lint |
passes |
tests/sandbox/test_unix_local.py |
18 passed |
tests/sandbox |
2 failed, 1314 passed, the same 2 docker mount ones that fail on main |
tests/extensions/sandbox |
8 failed, 695 passed, identical with the branch stashed |
There was a problem hiding this comment.
Self-correction after comparing the exact PR base 89c02c828ee8510fe9a84ee6675608193aa13b02: the repeated-cancellation cleanup weakness is real, but it is not a regression introduced by #4745 and I should not have left it as a blocker on this PR.
The base finalizers already did pop(...) and then await self._terminate_pty_entry(removed). A cancellation landing inside that terminate await could already abandon cleanup after the entry became unreachable from pty_terminate_all(). The two-cancel sequence I reproduced on 78b5c4d019 reaches the same pre-existing ownership weakness after the new finally, but the underlying second-cancellation exposure was already present before this PR.
What is new in #4745 was the additional await inserted between removal and termination by flush_pty_tail(): on bdef1730, a single cancellation during that new drain skipped cleanup. Commit 78b5c4d019 fixes that PR-introduced regression by putting termination in finally, and the added controlled test pins exactly that boundary.
So for #4745: please do not block or expand this PR for my repeated-cancellation comment. I withdraw it as a PR finding. A cancellation-settled teardown helper may still be worthwhile as a separate lifecycle hardening follow-up, using the repository's _settle_mount_transition pattern, but it should not be charged to this patch.
I rechecked the current test/source shape: test_finalize_still_cleans_up_when_the_drain_is_cancelled holds the new drain await open, cancels once after removal, and verifies termination still runs. That is the correct regression for the code this PR added.
There was a problem hiding this comment.
Verified current head 78b5c4d019ad7e1abb2863fb31542d5540790547 against the exact regression introduced by the removal-first tail path. The new try/finally guarantees that a single cancellation during the newly-added flush_pty_tail() await still reaches _terminate_pty_entry(removed), and test_finalize_still_cleans_up_when_the_drain_is_cancelled pins that ordering by holding output_lock, confirming removal committed, cancelling the finalizer, and asserting termination ran.
After comparing the PR base, I also corrected my later repeated-cancellation comment: cancellation inside _terminate_pty_entry() after the pop already existed before #4745, so that is follow-up hardening rather than a blocker here. For the regression this thread identified, 78b5c4d addresses it.
There was a problem hiding this comment.
Follow-up hardening is now tracked separately as #4747 (PTY teardown can be abandoned by cancellation after registry removal) against current main/the exact PR base. That issue carries the deterministic cancellation-ownership repro and settled-cleanup direction. I am not treating it as a blocker on #4745.
putting the removal first left a gap i did not think about. once the entry is out of the map nothing can reach it again, pty_terminate_all included, so a cancellation while the drain waits on the output lock skipped _terminate_pty_entry and the entry leaked. on blaxel that is a websocket and an aiohttp session left open the drain is in a try now with the terminate in the finally, so the cleanup happens on both paths. modal has no await between the pop and the terminate so it could not lose it today, but it is written the same way so an await added in front of it later does not start leaking test holds the output lock, cancels the finalise once the removal has gone through, and checks the entry was still terminated. it fails on bdef173
|
@codex review Please review the current head |
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Cross-link for collision/maintainer coordination: #4738 predates this PR and targets the same PTY output-settlement / cross-window UTF-8 area with a collector-owned |
|
thanks for the cross link. i had not seen #4738 and i should have, so let me put the facts up plainly.
and it is the same fourteen files, the same seven backends, the same so #4738 is not a competing approach, it is the same work done first and done wider, by a maintainer. my view is that this pr should give way to it. i am happy to close it, and i would rather do that than have you and @seratch carry two reviews of one fix. what i missed is worth saying out loud because it is a process failure rather than bad luck. i check whether an issue is claimed and whether anything is linked to it. #4738 predates #4744 and does not reference it, so nothing linked and it looked open. the check i did not do is the obvious one, look for open pull requests already touching the same files. if any of it is useful to fold into #4738 rather than lost, i think it is these, and only these:
say the word and i will close this and open nothing further, or port those three into #4738 as a patch for @seratch to take or drop. either is fine, it is his call and yours, not mine. noted on #4747 as well, and thank you for going back to the base commit twice and pulling findings that were not mine. that is more care than i had any right to expect. |
|
Given your offer, my recommendation as a reviewer is: close #4745 in favor of #4738 and do not open another porting PR unless @seratch asks for one. I compared the three pieces you called out against
I have already left two branch-specific cancellation findings on #4738 ( |
|
closing, per your recommendation and my offer above. #4738 predates this by eleven hours, covers the same fourteen files, and is the maintainers own work, so there is no reason for it to carry a second review. thank you for checking the three pieces against
that leaves the blocked pump regression as the one thing worth offering, and i will raise it on #4738 as an idea rather than a patch, as you suggested. nothing else follows unless @seratch asks. leaving the branch and the history in place so the thread stays readable. for anyone reading this later, the useful part of it is not the fix. it is that every check i had asked whether the issue was taken, and none asked whether the code was already being fixed. #4744 was clean on all of them, no claim, no assignee, nothing linked, because #4738 predates the issue and never mentions it. i have added a check for open pull requests in the same area, tuned so that a maintainers pull request stops the work and a weak title match only warns, because the loose version would have blocked a change of mine that has since merged elsewhere. thanks for the review. eleven rounds on something that is going to be closed is a lot of your time, and the findings were right every time, including the two you went back and withdrew after checking them against the base. |
Closes #4744
problem
collect_pty_outputdecodes each collection window witherrors="replace". pty output is not collected once, it is collected in repeated windows over one persistentoutput_chunksdeque, so a multi byte character whose bytes land either side of a window boundary is decoded as two separate partial sequences and both halves becomeU+FFFD. the bytes are destroyed at that first decode, so no later window can put the character back together. nothing errors, the text is just wrong.it is the shared helper behind the
unix_local,docker,blaxel,daytonaandmodalpty paths, so it is not backend specific.reproducing with the reporter's script, splitting
"héllo wörld"midé:fix
hold the incomplete trailing sequence back and hand it to the deque for the next window:
the
is_done()guard matters. once the provider is finished there is no next window, so a trailing partial sequence really is invalid and should still be replaced rather than held forever._incomplete_utf8_suffix_lenwalks back at most three bytes to the lead byte and compares the bytes present against the width that lead byte announces. it returns 0 for ascii, for a complete sequence, and for anything that is not a truncated tail, so genuinely invalid bytes keep going througherrors="replace"exactly as before.tests
SPLIT_TEXTis"aé☃𝄞b", which is a one, two, three and four byte character, and the test splits it at every byte offset:main, which is every offset that lands inside a sequenceplus two cases for the boundaries of the guard itself: a stream that ends mid character still gets
U+FFFDand leaves the deque empty, and complete multi byte output is untouched.verification
mainh��llo wörldhéllo wörldtests/sandbox/test_pty_output.pymaintests/sandboxbeforetests/sandboxaftertest_mount_securitydocker mount examples and fail onmaintooruff check,ruff format --check,mypythe docker test modules are skipped locally because the optional
dockerpackage is not installed, onmainas well.disclaimer: this contribution was prepared with the assistance of an ai agent. i ran the reporter's reproduction against
mainfirst, checked theis_doneboundary myself, and ran the sandbox suite and linters locally before opening this.