Repository navigation
perf(sandbox): push objects a store can't sign onto a VM in parallel, and over stdin on Modal VMs - #94
Merged
Conversation
… and over stdin on Modal VMs An object the store couldn't sign a URL for was read into memory whole, then appended to the VM one base64 chunk per exec, strictly in sequence. That ran at about 0.1 MiB/s, so a 161 MB snapshot restore took 19 minutes. - push_object_over_exec, the default for every VM sandbox, streams the object from the store a chunk at a time. Each chunk goes in its own exec, is written at its offset with `dd seek=… conv=notrunc`, and 32 are in flight. The chunk size is still bounded by _WFT_CHUNK_BYTES. The file is checked against the object's sha256 at the end. - push_object_over_stdin sends the object as up to 8 segments at once over exec stdin. Each segment is checked by sha256 in the exec that writes it. A reader that can't seek goes as one stream. A sandbox whose exec takes stdin implements _exec_with_stdin and uses it; ModalVmSandbox now does. 32 MiB on dev VMs, sha256-verified: | path | modal_vm | | -------------------------------------- | --------- | | before: sequential append | 0.10 MiB/s | | chunk per exec, 32 in flight | 3.0 MiB/s | | stdin, 8 segments (Modal VMs now) | 7.5 MiB/s | Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ned, and fail a segment the object can't fill - When the store opens the object as a regular file, every segment reads it by position (os.pread) through that one descriptor. All segments then see one version, even if the store replaces the object during the push. Any other reader goes as one stream. - A segment that reaches the object's end before its length fails the push. The segment's hash covers only the bytes read, so it can't catch that. - A chunk is at least one block, so a provider whose command limit is under 16 KiB fails at exec instead of pushing nothing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| data = await asyncio.to_thread(read, _STDIN_PIECE_BYTES if left is None else min(_STDIN_PIECE_BYTES, left)) | ||
| if not data: | ||
| if left is not None: | ||
| raise RuntimeError(f"The object ended {left} bytes short of the segment at offset {offset} of " |
There was a problem hiding this comment.
Failed push leaves writer waiting
When a local object is cut short during a Modal push, pieces() raises while _exec_with_stdin is feeding a running writer. The error skips write_eof() and wait(), leaving that VM process waiting for input after the push has failed. Close the input and clean up the process when feeding fails.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/providers/sandbox_providers/sandbox.py
Line: 595
Comment:
**Failed push leaves writer waiting**
When a local object is cut short during a Modal push, `pieces()` raises while `_exec_with_stdin` is feeding a running writer. The error skips `write_eof()` and `wait()`, leaving that VM process waiting for input after the push has failed. Close the input and clean up the process when feeding fails.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…readers, and let a provider set how many execs push - An object of one chunk goes over exec arguments in a single exec that writes it and prints its sha256. An object of one segment, now at least 1 MiB, goes over stdin the same way. Each used to cost a truncate exec and a check exec as well, so loads of many small files paid two to three times as many execs. - Only an io.BufferedReader or io.FileIO counts as a file that segments can read by position. A decompressing reader also names its file's descriptor, and reading that file would push the compressed bytes, with each segment's sha256 still matching. - VmSandbox._PUSHES_IN_FLIGHT, 8 by default, is how many execs carry one object at once: chunks over arguments, segments over stdin. A provider sets its own as a class attribute. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… where exec takes stdin A stdin exec takes more round trips than a plain one. On Modal VMs a 10 KiB object took 0.37 s that way, against 0.23 s in one exec whose script carries it. When the store opens the object as a file and it is smaller than a chunk, push_object_over_stdin writes it in one such exec. 30 x 10 KiB, sha256-verified, s/file: | path | modal_vm | beta_scale | | -------------------- | -------- | ---------- | | before (one heredoc) | 0.22 | 0.49 | | exec arguments | 0.23 | 0.45 | | load_s3_file | 0.23 | 0.45 | Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…r stdin Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
With a store that can't sign URLs (e.g. the local store), objects reached a VM sandbox one base64 chunk per exec, in sequence: about 0.1 MiB/s, so a 161 MB snapshot restore took 19 minutes.
push_object_over_execis now the default for every VM sandbox. It streams the object a chunk per exec, each written at its offset withdd,VmSandbox._PUSHES_IN_FLIGHT(8) at once, and checks sha256 at the end.push_object_over_stdinis for sandboxes whose exec takes stdin;ModalVmSandboxnow uses it. Segments of at least 1 MiB are read from one open file by position, and each is sha256-checked by the exec that writes it. Other readers go as one stream.Measurements
Dev VMs, local store, every run sha256-verified.
_exec_with_stdinA 110 MiB image loads and runs in 15.8 s on modal_vm, and 15.1 s on the other provider over stdin.
Chaos (live VMs)
drain()never returns once the VM dies.E2B is untested (no credentials) and gets the default path.
Testing
make unit-testpasses.vm_object_push_test.pyruns both paths through a real shell, covering:🤖 Generated with Claude Code
The PR appears safe to merge, though a failed Modal push can still leave its VM writer waiting for input.
Fix with agent prompt
Summary
Unsigned-object transfers to VM sandboxes now use parallel exec chunks by default, while Modal VMs stream the data over stdin. Both paths check the bytes written, cutting the time spent transferring large objects.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[VM needs an object] --> B{Store can sign a URL?} B -- Yes --> C[Download from signed URL] B -- No --> D{Modal VM?} D -- Yes --> E[Send segments over stdin] D -- No --> F[Send chunks through exec] E --> G[Check written bytes] F --> GReviews (4) · Last reviewed commit: "docs(sandbox): an object a store can't p..." · Reviewed by Greptile