Skip to content

Conversation

aadit-juneja
Copy link

@aadit-juneja aadit-juneja commented Sep 29, 2025

Client Update for Fixing Multiple Sandbox StreamWriter Issue

This issue fixes an issue in which multiple stdin StreamWriters cannot be opened to a Sandbox. Previously, only one StreamWriter could be opened to write messages to stdin. Our new fix, at a high level, more explicitly enforces only one open StreamWriter, assigning a uuid to each writer and adds an active_writer_id attribute for sandboxes. Protobuf changes include adding a writer_id to RuntimeInputMessage and toSandboxStdinWriteRequest.

The Linear issue contains a Slack Thread for a relevant client problem.

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

@aadit-juneja aadit-juneja changed the title Client Update for Multiple open StreamWriter's fix [ART-8] Client Update for Fixing Multiple Sandbox StreamWriter Issue [ART-8] Sep 30, 2025
bytes message = 1;
uint64 message_index = 2;
bool eof = 3;
string writer_id = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are two spots where we create RuntimeInputMessages in the client. Do those need to be updated to take this new writer_id value, too?

Copy link
Author

Choose a reason for hiding this comment

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

I included the field because RuntimeInputMessage was used in server code for the code flow of a SandboxStdinWriteRequest and it needs to be sent over to the worker for deduplication (monorepo PR). The us of RuntimeInputMessage PR in the client looks like it's once for non-sandbox tasks and another for modal shell.

I think that if the field is not included in when it is sent it gets set to a default string value ("") and it'll be ignored on the code paths for those two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants