[sandbox audit] Bound what a transfer or a command's output costs the client - #4839
[sandbox audit] Bound what a transfer or a command's output costs the client#4839Wauplin wants to merge 1 commit into
Conversation
…lient Four unbounded allocations in the caller's process, each proportional to something the sandbox controls. **Output was accumulated even when a callback was consuming it.** `run()` appended every chunk to `stdout_parts`/`stderr_parts` regardless, so a streaming consumer still paid full memory for the command's entire output, and a runaway command (a loop printing to stdout) was an unbounded allocation. New `capture_output=False` hands output to the callbacks and drops it; the default now raises above a ceiling instead of being OOM-killed with no explanation, and the error names the flag to use. An explicit flag rather than "stop capturing when a callback is present": silently changing what `result.stdout` contains based on another argument is a footgun, and code that passes a callback *and* reads `result.stdout` today keeps working. **A parallel read doubled peak memory.** `_read_ranges` collected every chunk into a list and the caller then joined it, so a 2 GB file peaked at about 4 GB. It is a generator now, yielding `(offset, bytes)` as ranges land: `read()` places each chunk into one preallocated buffer, `download()` seeks and writes it out, and both drop it immediately. `read()` also refuses above a ceiling and points at `download()`, which streams to disk. **A parallel upload read the whole file into memory.** `upload()` did `f.read()` above the threshold -- 10 GB resident for a 10 GB file. Each worker now `pread`s its own range off the shared descriptor (`pread`, not `seek`+`read`, because the workers share the descriptor and a seeking read would race with its siblings). **And it left a stale tail.** A ranged write does not truncate, so overwriting a larger file with a smaller one left the old bytes past the new end. Each upload's final chunk now sends the intended total size, which the server truncates to. Also follows the server's new listing pagination, so `files.list()` still returns one complete list however the directory is chunked on the wire -- and the fake server now serves `/files/list` at all, which it never did, so that path had no coverage. Validation: 88 tests pass, up from 83. New coverage for opting out of capture, the runaway-output ceiling and its escape hatch, the read ceiling, and cursor-following in `list()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 53871e5. Configure here.
|
|
||
| def push(rng: tuple[int, int]) -> None: | ||
| offset, length = rng | ||
| self._put_range(path, offset, os.pread(fileno, length, offset), mode, total=size) |
There was a problem hiding this comment.
Large uploads crash on Windows
High Severity
_write_ranges_from_file calls os.pread, which exists only on Unix. On Windows, files.upload of any file above PARALLEL_THRESHOLD raises AttributeError instead of sending the file. The same module already special-cases Windows for download flags, and the CLI accepts Windows paths for this copy path.
Reviewed by Cursor Bugbot for commit 53871e5. Configure here.
| "or redirect it to a file in the sandbox and download that." | ||
| ) | ||
| captured += len(data) | ||
| parts.append(data) |
There was a problem hiding this comment.
Streaming CLI hits capture ceiling
Medium Severity
run() now raises once captured output exceeds 64 MB, and capture_output still defaults to True. hf sandbox exec already streams via on_stdout/on_stderr and only reads exit status from the result, so any command that prints more than 64 MB now fails even though the output is already being consumed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 53871e5. Configure here.


Why
Four unbounded allocations in the caller's process, each proportional to something the
sandbox controls.
1. Output was accumulated even when a callback was consuming it.
run()appended everychunk to
stdout_parts/stderr_partsregardless ofon_stdout/on_stderr, so a streamingconsumer still paid full memory for the command's entire output — and a runaway command (a
loop printing to stdout) was an unbounded allocation with no ceiling.
2. A parallel read doubled peak memory.
_read_rangescollected every chunk into a listand the caller then
b"".joined it, so a 2 GB file peaked at roughly 4 GB.3. A parallel upload read the whole file into memory —
f.read()above the threshold, so10 GB resident for a 10 GB file.
4. And it left a stale tail. A ranged write doesn't truncate, so overwriting a larger file
with a smaller one left the old bytes past the new end.
Approach
capture_output=Falsehands output to the callbacks and drops it. The default nowraises above a ceiling rather than being OOM-killed with no explanation, and the error names
the flag.
An explicit flag rather than "stop capturing when a callback is present": silently changing
what
result.stdoutcontains based on another argument is a footgun, and code that passes acallback and reads
result.stdouttoday keeps working._read_rangesis a generator, yielding(offset, bytes)as ranges land.read()placeseach chunk into one preallocated buffer;
download()seeks and writes it out. Both drop itimmediately.
read()also refuses above a ceiling and points atdownload().upload()preads each range off the shared descriptor.preadrather thanseek+readbecause the workers share one descriptor and a seeking read would race withits siblings — worth stating, since the obvious refactor is wrong here.
Each upload's final chunk sends the intended total size, which the server truncates to.
Plus:
files.list()follows the server's new pagination cursor, so a caller still sees onecomplete list however the directory is chunked on the wire.
The fake never served
/files/listSo that path had no coverage at all —
files.list()was never exercised against anything.It does now, with pagination, which is how the cursor-following is tested.
Validation
88 tests pass, up from 83. New coverage: opting out of capture (callback still sees
everything, result deliberately empty); the runaway-output ceiling and the escape hatch its
error names; the read ceiling; cursor-following in
list().Behaviour changes
run()raisesSandboxErrorabove 64 MB of captured output instead of growing withoutbound. Anything that relied on unbounded capture was one runaway command from an OOM.
files.read()raises above 512 MB and points atdownload().run()gains acapture_outputkeyword (defaultTrue, so existing calls are unaffected).Note
Medium Risk
Behavior changes for very large command output or in-memory reads (now explicit errors), and ranged uploads depend on server
truncate_to; defaults preserve existingrun()capture semantics.Overview
Adds client-side limits and streaming so sandbox workloads cannot grow the caller's process without bound.
Sandbox.run()gainscapture_output(defaultTrue). WhenFalse, stdout/stderr go only to callbacks and stay empty on the result. When capturing, output above 64 MB raisesSandboxErrorwith guidance to stream or redirect.SandboxFilesrefusesread()/read_text()above 512 MB and points callers atdownload(). Parallel read, download, and upload no longer buffer whole files: ranged reads yield(offset, bytes)into a single buffer or seek-writes; large uploads usepreadper range. Ranged writes sendtruncate_toon the last chunk so shrinking a file does not leave a stale tail (needs the paired server change).list()follows the server's paginatednextcursor and returns one full directory listing.Docs update pool resource limits wording (per-process rlimits and server-clamped
max_procs/max_mem_mb). Tests cover capture opt-out, output ceiling, read ceiling, and list pagination.Reviewed by Cursor Bugbot for commit 53871e5. Bugbot is set up for automated code reviews on this repo. Configure here.