Skip to content

fix: replace O(n²) string concat with array buffering for stdout/stderr #1271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
beran-t wants to merge 3 commits into from
+42 −24
Closed

fix: replace O(n²) string concat with array buffering for stdout/stderr #1271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8cefe9
fix: replace O(n²) string concatenation with array buffering for stdo…
beran-t Apr 14, 2026
0b4ee1f
chore: remove memory reproduction script
beran-t Apr 14, 2026
438a9b6
fix: add cached lazy join for Python async SDK stdout/stderr properties
beran-t Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions packages/js-sdk/src/sandbox/commands/commandHandle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,10 @@ export class CommandHandle
Omit<CommandResult, 'exitCode' | 'error'>,
Partial<Pick<CommandResult, 'exitCode' | 'error'>>
{
private _stdout = ''
private _stderr = ''
private _stdoutChunks: string[] = []
private _stderrChunks: string[] = []
private _stdoutCached?: string
private _stderrCached?: string

private result?: CommandResult
private iterationError?: Error
Expand Down Expand Up @@ -130,14 +132,20 @@ export class CommandHandle
* Command execution stderr output.
*/
get stderr() {
return this._stderr
if (this._stderrCached === undefined) {
this._stderrCached = this._stderrChunks.join('')
}
return this._stderrCached
}

/**
* Command execution stdout output.
*/
get stdout() {
return this._stdout
if (this._stdoutCached === undefined) {
this._stdoutCached = this._stdoutChunks.join('')
}
return this._stdoutCached
}

/**
Expand Down Expand Up @@ -196,12 +204,14 @@ export class CommandHandle
switch (e.value.output.case) {
case 'stdout':
out = new TextDecoder().decode(e.value.output.value)
this._stdout += out
this._stdoutChunks.push(out)
this._stdoutCached = undefined
yield [out as Stdout, null, null]
break
case 'stderr':
out = new TextDecoder().decode(e.value.output.value)
this._stderr += out
this._stderrChunks.push(out)
this._stderrCached = undefined
yield [null, out as Stderr, null]
break
case 'pty':
Expand Down
28 changes: 18 additions & 10 deletions packages/python-sdk/e2b/sandbox_async/commands/command_handle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,18 @@ def stdout(self):
"""
Command stdout output.
"""
return self._stdout
if self._stdout_cached is None:
self._stdout_cached = "".join(self._stdout_chunks)
return self._stdout_cached

@property
def stderr(self):
"""
Command stderr output.
"""
return self._stderr
if self._stderr_cached is None:
self._stderr_cached = "".join(self._stderr_chunks)
return self._stderr_cached

Comment on lines 41 to 56
Copy link
Copy Markdown

@claude claude Bot Apr 14, 2026

Choose a reason for hiding this comment

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

🔴 The Python async SDK's stdout and stderr properties call ''.join(self._stdout_chunks) on every access with no caching, reintroducing the same O(n²) pattern the PR set out to fix. If a user accesses handle.stdout inside an on_stdout callback (as code-interpreter's readLines does), each callback invocation triggers a full re-join of all accumulated chunks — O(total_size) per callback, O(n²) total. Fix by adding _stdout_cached: Optional[str] = None, returning the cached value in the getter, and setting it to None on each append, mirroring the JS implementation.

Extended reasoning...

What the bug is: The Python async SDK's stdout and stderr properties in command_handle.py (lines 41–52) always call ''.join(self._stdout_chunks) on every property access with no caching. The JS SDK correctly maintains _stdoutCached/_stderrCached fields that are set to undefined on each chunk push and lazily computed in the getter. The Python async SDK received the array-buffering change (replacing +=) but did not receive the corresponding caching layer.

The specific code path: When a user passes on_stdout to sandbox.commands.start(), the async SDK calls that callback for each chunk in _handle_events(). If the callback (or any downstream consumer) accesses handle.stdout to get the accumulated output — exactly the pattern code-interpreter's readLines uses to scan for newlines via str.find() — each callback invocation hits the property getter, which calls ''.join(self._stdout_chunks) from scratch.

Why existing code doesn't prevent it: The PR replaced self._stdout += out (O(n) per append) with self._stdout_chunks.append(out) (O(1) per append), which is correct. But it did not add a cache guard in the getter. Before the PR, reading self._stdout in a callback was O(1) (a simple attribute read). After the PR, reading handle.stdout in a callback is O(accumulated_size) because join() must traverse all chunks every time.

The impact: With N chunks totaling T bytes, accessing handle.stdout once per callback produces O(T) work per call and O(N × T) total — quadratic in T when N grows proportionally to T (fixed chunk sizes). The PR's own benchmark documents this scenario ('string flattening') measuring 669 MB peak heap / 3.5x amplification for 200 MB of output in the JS SDK after the fix. The Python async SDK has no protection at all, so this scenario will be at least as bad as the pre-fix JS numbers.

How to fix: Mirror the JS implementation exactly. Add self._stdout_cached: Optional[str] = None and self._stderr_cached: Optional[str] = None as instance variables in __init__. In the stdout property getter, return self._stdout_cached if it is not None, otherwise compute ''.join(self._stdout_chunks), store it in self._stdout_cached, and return it. In _iterate_events, after self._stdout_chunks.append(out), set self._stdout_cached = None. Apply the same pattern for stderr.

Step-by-step proof: Suppose a user runs sandbox.commands.start(cmd, on_stdout=lambda _: print(len(handle.stdout))) against a command producing 200,000 chunks of ~1,000 bytes each (200 MB total, matching the PR's benchmark). Chunk 1 arrives → _stdout_chunks has 1 element → handle.stdout joins 1 string, ~1,000 bytes of work. Chunk 2 → joins 2 strings, ~2,000 bytes. Chunk k → joins k strings, ~k×1,000 bytes. Total work = 1,000 × (1 + 2 + … + 200,000) ≈ 2×10¹³ bytes of string copying — clearly O(n²). This is the exact scenario the PR benchmarks, and the Python async SDK has no defense against it.

@property
def error(self):
Expand Down Expand Up @@ -87,8 +91,10 @@ def __init__(
self._handle_kill = handle_kill
self._events = events

self._stdout: str = ""
self._stderr: str = ""
self._stdout_chunks: list[str] = []
self._stderr_chunks: list[str] = []
self._stdout_cached: Optional[str] = None
self._stderr_cached: Optional[str] = None

self._on_stdout = on_stdout
self._on_stderr = on_stderr
Expand All @@ -113,18 +119,20 @@ async def _iterate_events(
if event.event.HasField("data"):
if event.event.data.stdout:
out = event.event.data.stdout.decode("utf-8", "replace")
self._stdout += out
self._stdout_chunks.append(out)
self._stdout_cached = None
yield out, None, None
if event.event.data.stderr:
out = event.event.data.stderr.decode("utf-8", "replace")
self._stderr += out
self._stderr_chunks.append(out)
self._stderr_cached = None
yield None, out, None
if event.event.data.pty:
yield None, None, event.event.data.pty
if event.event.HasField("end"):
self._result = CommandResult(
stdout=self._stdout,
stderr=self._stderr,
stdout="".join(self._stdout_chunks),
stderr="".join(self._stderr_chunks),
exit_code=event.event.end.exit_code,
error=event.event.end.error,
)
Expand Down Expand Up @@ -176,8 +184,8 @@ async def wait(self) -> CommandResult:

if self._result.exit_code != 0:
raise CommandExitException(
stdout=self._stdout,
stderr=self._stderr,
stdout=self._result.stdout,
stderr=self._result.stderr,
exit_code=self._result.exit_code,
error=self._result.error,
)
Expand Down
16 changes: 8 additions & 8 deletions packages/python-sdk/e2b/sandbox_sync/commands/command_handle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ def __init__(
self._handle_kill = handle_kill
self._events = events

self._stdout: str = ""
self._stderr: str = ""
self._stdout_chunks: list[str] = []
self._stderr_chunks: list[str] = []

self._result: Optional[CommandResult] = None
self._iteration_exception: Optional[Exception] = None
Comment on lines 37 to 44
Copy link
Copy Markdown

@claude claude Bot Apr 14, 2026

Choose a reason for hiding this comment

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

🟣 The sync CommandHandle has no public stdout or stderr properties, while the async AsyncCommandHandle exposes them via @property. This is a pre-existing API inconsistency: before this PR the sync SDK stored private _stdout: str, and after this PR it stores private _stdout_chunks: list[str] — neither exposes a public accessor. Users holding a background sync handle cannot inspect accumulated output via handle.stdout and would receive AttributeError.

Extended reasoning...

What the bug is

The async AsyncCommandHandle has always exposed stdout and stderr as @property accessors returning ''.join(self._stdout_chunks). The sync CommandHandle has never had equivalent public properties — before this PR it had self._stdout: str = '' (private), and after this PR it has self._stdout_chunks: list[str] = [] (still private). There is no public stdout or stderr attribute on the sync handle.

The specific code path

In packages/python-sdk/e2b/sandbox_sync/commands/command_handle.py, the class defines only a pid property. The _stdout_chunks and _stderr_chunks fields added by this PR at lines 42-43 are purely internal. The async counterpart in packages/python-sdk/e2b/sandbox_async/commands/command_handle.py defines @property stdout and @property stderr that are part of the documented public API.

Why existing code does not prevent it

The PR only changes internal storage from str to list[str] and correctly wires up the CommandResult returned by wait(). It does not add or remove any public properties. The public API surface of the sync handle is unchanged before and after this PR.

Addressing the refutation

The refutation argues this asymmetry is intentional: the sync SDK processes events lazily (only when iterated), so accumulated output mid-run is less meaningful, and users can use wait(on_stdout=...) callbacks or iterate via for stdout, stderr, pty in handle:. This is architecturally accurate. However, a user who uses both SDKs or who reads the async SDK docs would reasonably expect handle.stdout to work on either handle type. The AsyncCommandHandle treats it as standard public API, creating a discoverable inconsistency. The refutation is correct that this is intentional design, but it does not eliminate the confusion users encounter when the APIs diverge.

Impact

Any code that does handle = sandbox.commands.run(cmd, background=True) followed by handle.stdout will raise AttributeError: 'CommandHandle' object has no attribute 'stdout'. The fix would be trivially additive: @property stdout: return ''.join(self._stdout_chunks).

Step-by-step proof

  1. User calls handle = sandbox.commands.run('sleep 5 && echo done', background=True)
  2. User tries print(handle.stdout) to check intermediate output (as they would with AsyncCommandHandle)
  3. Python raises AttributeError: 'CommandHandle' object has no attribute 'stdout'
  4. Same code with the async SDK works fine because @property stdout is defined on AsyncCommandHandle
  5. This is pre-existing: even before this PR, step 3 would raise the same error — the private _stdout field was never a public property

Pre-existing nature

Before this PR: self._stdout: str = '' — private, no public property. After this PR: self._stdout_chunks: list[str] = [] — still private, still no public property. The inconsistency predates this PR.

Expand Down Expand Up @@ -67,18 +67,18 @@ def _handle_events(
if event.event.HasField("data"):
if event.event.data.stdout:
out = event.event.data.stdout.decode("utf-8", "replace")
self._stdout += out
self._stdout_chunks.append(out)
yield out, None, None
if event.event.data.stderr:
out = event.event.data.stderr.decode("utf-8", "replace")
self._stderr += out
self._stderr_chunks.append(out)
yield None, out, None
if event.event.data.pty:
yield None, None, event.event.data.pty
if event.event.HasField("end"):
self._result = CommandResult(
stdout=self._stdout,
stderr=self._stderr,
stdout="".join(self._stdout_chunks),
stderr="".join(self._stderr_chunks),
exit_code=event.event.end.exit_code,
error=event.event.end.error,
)
Expand Down Expand Up @@ -131,8 +131,8 @@ def wait(

if self._result.exit_code != 0:
raise CommandExitException(
stdout=self._stdout,
stderr=self._stderr,
stdout=self._result.stdout,
stderr=self._result.stderr,
exit_code=self._result.exit_code,
error=self._result.error,
)
Expand Down
Loading