Implement handling for STDOUT StreamType in sandbox exec #3717

saltzm · 2025-11-03T20:24:01Z

Describe your changes

Provide Linear issue reference (e.g. CLI-1234) if available.

Checklists

Compatibility checklist

Check these boxes or delete any item (or this section) if not relevant for this PR.

Client+Server: this change is compatible with old servers
Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.

Release checklist

If you intend for this commit to trigger a full release to PyPI, please ensure that the following steps have been taken:

Version file (modal_version/__init__.py) has been updated with the next logical version
Changelog has been cleaned up and given an appropriate subhead

Changelog

Note

Enable StreamType.STDOUT for sandbox exec via the command router by streaming to the client and printing immediately to local stdout (text-only), with tests covering behavior.

Streams (modal/io_streams.py):
- New STDOUT reader: Add _StdoutPrintingStreamReaderThroughCommandRouter that consumes text output and prints to local stdout immediately; disallows read()/iteration; cleans up background task on aclose().
- Router path support: StreamReader now instantiates the new reader when stream_type == StreamType.STDOUT (requires text=True); otherwise uses existing text/bytes readers.
Sandbox exec (modal/sandbox.py):
- Router-backed exec: Map stdout=STDOUT to router PIPE streaming so client-side reader can print locally; preserves existing configs for PIPE/DEVNULL/STDERR->STDOUT.
Tests (test/sandbox_test.py):
- Parametrize server/router backends and add cases verifying that stdout=STDOUT prints immediately (including bufsize=1), reading from stdout raises InvalidError, and router backend requires text=True.

^{Written by Cursor Bugbot for commit 2f66dec. This will update automatically on new commits. Configure here.}

saltzm · 2025-11-03T20:24:11Z

@cursor review

saltzm · 2025-11-03T21:12:08Z

@cursor review

freider · 2025-11-05T16:17:13Z

modal/io_streams.py

+        self._task: Optional[asyncio.Task[None]] = None
+        self._file_descriptor = params.file_descriptor
+        # Kick off a background task that reads from the underlying text stream and prints to stdout.
+        self._start_printing_task(params, by_line)


It would be great if we can avoid kicking off this task in the constructor and instead have some kind of delegation method we use from whatever constructs this in an async context to make the "asyncness" of this function explicit, e.g. have an a async def _init_stream() method on all StreamReader/StreamWriter objects that's called by their constructor to kick off any potential background tasks.

This helps with both testing and making sure refactors don't accidentally push this construction calling into some non-async scope where create_task wouldn't work

Thanks, yeah I remember you commenting on this when you were working on the other issue. I'll give this a try

Hmm, it looks like this would unfortunately be pretty invasive. We instantiate StreamReaders pretty much only from sync functions, e.g. Sandbox._hydrate_metadata (here) and in ContainerProcess*.__init__, e.g. here. So we'd have to do a lot of rearranging to make this workable.

Let me know if you think it's still worth doing. But I think we really need to redesign a lot of the Sandbox API and do proper resource management via context managers and the like at some point anyway...

I see what you mean... Yeah the similar thing I ran across in Italy was ContainerProcess also doing something similar - starting asyncio Tasks from the constructor. And _hydrate_metadata() should also not be doing anything with event loops, that's risky 😬

I think we should clean this up sooner rather than later, without necessarily adding the contextmanager support (that should probably be optional anyway to not break backwards compatibility or similarities with Popen), but I'm ok with it being a followup on this since it's not directly related.

freider · 2025-11-05T16:19:10Z

modal/io_streams.py

+    ) -> None:
+        self._reader: Optional[_TextStreamReaderThroughCommandRouter[str]] = None
+        self._task: Optional[asyncio.Task[None]] = None
+        self._file_descriptor = params.file_descriptor


should we maybe just add self._params = params and then reference that from the rest of the implementation code?

I had that at first but changed it because I generally like to only keep data that's actually needed

I think the full params are "needed" if you count them being used to pass into _TextStreamReaderThroughCommandRouter in the _start_printing_task - esp. if we would defer that task creation to some followup step that's not part of the contstrcutor (then it wouldn't take this as an argument but have to find it as some instance var)

freider · 2025-11-05T16:19:20Z

modal/io_streams.py

+        self._start_printing_task(params, by_line)
+
+    @property
+    def file_descriptor(self) -> int:


This would be a "api_pb2.FileDescriptor.ValueType" right?

True, the _StreamReader class has always had int as the return type here though

I see... so many mini cleanups. Maybe it makes sense to add it as a quick driveby improvement to all the _StreamReaders?

freider · 2025-11-05T16:22:00Z

modal/io_streams.py

+
+    def _start_printing_task(self, params: _StreamReaderThroughCommandRouterParams, by_line: bool) -> None:
+        async def _run():
+            self._reader = _TextStreamReaderThroughCommandRouter[str](params, by_line)


Is there a case for printing non-str (binary) data to stdout sometimes?

Yeah good question, I wasn't sure, what do you think? I sort of assumed people wouldn't want to do that

I could see some potential use cases for it - e.g if you want to run commands that output terminal control sequences which aren't necessarily text-encodable. I guess we could have different underlying _reader implementations here depending on text=True | False ?

Btw, why is _TextStreamReaderThroughCommandRouter and _BytesStreamReader... - wouldn't they always use str as the generic type var for the first and bytes for the second? 🤔

I think maybe a word is missing there, are you asking why they're generic? If that's the question, then there's no good reason, I should make them concrete

Ah yeah, missed a word 🤦 Generic yes, they shouldn't need to be generic

freider · 2025-11-05T20:08:17Z

modal/io_streams.py

+        self._task = asyncio.create_task(_run())
+
+    async def read(self) -> T:
+        raise InvalidError("Logs can only be retrieved using the PIPE stream type.")


Nit, maybe use "Output can only be read..." rather than the "Logs can only be retrieved" - command output isn't necessarily seen as "logs"? Maybe that's just me though

No I agree with you, this was copied from an existing error message but I prefer output so I'll change it

freider · 2025-11-05T20:11:07Z

modal/io_streams.py

+    async def aclose(self):
+        if self._task is not None:
+            self._task.cancel()
+            with contextlib.suppress(Exception):


If this is to suppress the expected asyncio.CancelledError specifically, can we use that type instead?

This is me not examining LLM generated code closely enough, I'll see if I can figure out the intent and scope it more specifically

freider · 2025-11-05T20:15:36Z

modal/io_streams.py

+                if stream_type == StreamType.STDOUT:
+                    if not text:
+                        raise ValueError("StreamType.STDOUT is only supported when text=True")
+                    self._impl = _StdoutPrintingStreamReaderThroughCommandRouter(params, by_line)


Now that I think about it (considering the bytes vs str reading etc.) - could we implement the _StdoutPrintingStreamReaderThroughCommandRouter as accepting another streamreader as the underlying buffer - passing the _TextStreamReaderThroughCommandRouter or _BytesStreamReaderThroughCommandRouter created below into the constructor of _StdoutPrintingStreamReaderThroughCommandRouter ? Then we don't have to duplicate the logic of determining the underlying stream reader by looking at params etc.?

yeah that's better

Implement handling for STDOUT StreamType in sandbox exec

28bce8a

This comment was marked as outdated.

Sign in to view

saltzm self-assigned this Nov 3, 2025

Cleanup/tests

3bc5dff

This comment was marked as outdated.

Sign in to view

saltzm marked this pull request as ready for review November 3, 2025 21:23

saltzm requested a review from freider November 3, 2025 21:23

saltzm added 4 commits November 4, 2025 16:39

Include servicer in test

aeb113a

Make test async

7967cd0

Sleep infinity.. hopefully fix windows test

a1ba48b

skip_non_subprocess

2f66dec

freider reviewed Nov 5, 2025

View reviewed changes

Implement handling for STDOUT StreamType in sandbox exec #3717

Are you sure you want to change the base?

Implement handling for STDOUT StreamType in sandbox exec #3717

Conversation

saltzm commented Nov 3, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Compatibility checklist

Release checklist

Changelog

Uh oh!

saltzm commented Nov 3, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

saltzm commented Nov 3, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

saltzm commented Nov 3, 2025 •

edited by cursor bot

Loading