diff --git a/tests/tools/test_local_background_child_hang.py b/tests/tools/test_local_background_child_hang.py index ee8ad95a31ef..dd9f112a5678 100644 --- a/tests/tools/test_local_background_child_hang.py +++ b/tests/tools/test_local_background_child_hang.py @@ -101,7 +101,7 @@ class TestBackgroundChildDoesNotHang: "'\\nTAIL-SENTINEL')\"" ) - result = local_env.execute(command, timeout=10) + result = local_env.execute(command, timeout=10, bounded_capture=True) assert result["returncode"] == 0 assert len(result["output"]) <= 10_000 @@ -109,6 +109,31 @@ class TestBackgroundChildDoesNotHang: assert result["output"].endswith("TAIL-SENTINEL") assert "[OUTPUT TRUNCATED" in result["output"] + def test_default_capture_is_full_fidelity_for_internal_consumers( + self, local_env + ): + """Default execute() (no bounded_capture) must return complete output. + + Internal consumers — file-operation ``cat`` reads that feed the patch + engine, code-execution RPC reads, log reads — rely on full-fidelity + capture. Bounding them at tool_output.max_bytes would CORRUPT files + on read-modify-write (#64435 review finding), so only the foreground + terminal tool opts in via bounded_capture=True. + """ + # ~200 KB — four times the default 50 KB cap. + command = ( + "python3 -c \"import sys; " + "sys.stdout.write('START-MARK\\n' + ('y' * 200000) + '\\nEND-MARK')\"" + ) + + result = local_env.execute(command, timeout=10) + + assert result["returncode"] == 0 + assert "[OUTPUT TRUNCATED" not in result["output"] + assert result["output"].startswith("START-MARK") + assert result["output"].endswith("END-MARK") + assert len(result["output"]) > 200000 + def test_continuous_output_still_honors_foreground_timeout( self, local_env, monkeypatch ): @@ -120,7 +145,7 @@ class TestBackgroundChildDoesNotHang: ) started = time.monotonic() - result = local_env.execute(command, timeout=1) + result = local_env.execute(command, timeout=1, bounded_capture=True) elapsed = time.monotonic() - started assert elapsed < 4.0 diff --git a/tools/environments/base.py b/tools/environments/base.py index b6f526902737..87049fa954dd 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -45,9 +45,14 @@ if _DEBUG_INTERRUPT: _activity_callback_local = threading.local() +# Sentinel capacity for full-fidelity capture (internal consumers). Large +# enough that the collector never evicts in practice, keeping a single code +# path for both bounded and unbounded modes. +_UNBOUNDED_CAPTURE_CHARS = 2**63 - 1 + + class _BoundedOutputCollector: """Retain a bounded 40/60 head-tail window of streamed text.""" - def __init__(self, max_chars: int): self.max_chars = max(1, int(max_chars)) self._head_limit = int(self.max_chars * 0.4) @@ -673,11 +678,21 @@ class BaseEnvironment(ABC): # Process lifecycle # ------------------------------------------------------------------ - def _wait_for_process(self, proc: ProcessHandle, timeout: int = 120) -> dict: + def _wait_for_process( + self, proc: ProcessHandle, timeout: int = 120, *, bounded_capture: bool = False + ) -> dict: """Poll-based wait with interrupt checking and stdout draining. Shared across all backends — not overridden. + ``bounded_capture=True`` (foreground terminal-tool path only) retains + at most ``tool_output.max_bytes`` of output in a head/tail window + while draining, so a verbose subprocess cannot OOM the process + (#64435). The default (False) preserves full-fidelity capture for + internal consumers — file-operation ``cat`` reads feeding the patch + engine, code-execution RPC reads, log reads — where truncation would + corrupt data. + Fires the ``activity_callback`` (if set on this instance) every 10s while the process is running so the gateway's inactivity timeout doesn't kill long-running commands. @@ -689,12 +704,18 @@ class BaseEnvironment(ABC): an orphan with ``PPID=1`` when python is shut down mid-tool — the ``sleep 300``-survives-30-min bug Physikal and I both hit. """ - try: - from tools.tool_output_limits import get_max_bytes + if bounded_capture: + try: + from tools.tool_output_limits import get_max_bytes - capture_limit = get_max_bytes() - except Exception: - capture_limit = 50_000 + capture_limit = get_max_bytes() + except Exception: + capture_limit = 50_000 + else: + # Full fidelity: effectively unbounded collector (single head + # segment, no eviction) so behavior matches the historical + # accumulate-everything semantics. + capture_limit = _UNBOUNDED_CAPTURE_CHARS output = _BoundedOutputCollector(capture_limit) # Non-blocking drain via select(). @@ -1032,8 +1053,19 @@ class BaseEnvironment(ABC): timeout: int | None = None, stdin_data: str | None = None, rewrite_compound_background: bool = True, + bounded_capture: bool = False, ) -> dict: - """Execute a command, return {"output": str, "returncode": int}.""" + """Execute a command, return {"output": str, "returncode": int}. + + ``bounded_capture=True`` caps stdout/stderr retention at + ``tool_output.max_bytes`` WHILE the stream is drained (head/tail + window) instead of holding the full output in memory (#64435). + It must only be set by callers whose output is destined for the + model/tool payload (the foreground terminal tool). Internal + full-fidelity consumers — file operations ``cat`` reads that feed + the patch engine, code-execution RPC reads, log reads — MUST leave + it False: truncating those corrupts data, not just display. + """ self._before_execute() exec_command, sudo_stdin = self._prepare_command(command) @@ -1068,7 +1100,9 @@ class BaseEnvironment(ABC): proc = self._run_bash( wrapped, login=login, timeout=effective_timeout, stdin_data=effective_stdin ) - result = self._wait_for_process(proc, timeout=effective_timeout) + result = self._wait_for_process( + proc, timeout=effective_timeout, bounded_capture=bounded_capture + ) self._update_cwd(result) return result diff --git a/tools/environments/modal_utils.py b/tools/environments/modal_utils.py index f83c0075ee3e..167ac3856cdf 100644 --- a/tools/environments/modal_utils.py +++ b/tools/environments/modal_utils.py @@ -80,11 +80,17 @@ class BaseModalExecutionEnvironment(BaseEnvironment): timeout: int | None = None, stdin_data: str | None = None, rewrite_compound_background: bool = True, + bounded_capture: bool = False, ) -> dict: # Managed/remote modal transports execute commands via explicit transport # and do not rely on shell background rewriters. Keep parameter for # compatibility with BaseEnvironment callers. _ = rewrite_compound_background + # bounded_capture: accepted for BaseEnvironment.execute() signature + # parity (the terminal tool passes it). Modal transports return the + # remote function's result in one payload, so streaming-time bounding + # does not apply; the terminal tool's final truncation still caps it. + _ = bounded_capture self._before_execute() prepared = self._prepare_modal_exec( command, diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 147492b26957..1d243f4d2fc1 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -2657,6 +2657,12 @@ def terminal_tool( execute_kwargs = { "timeout": effective_timeout, "cwd": command_cwd, + # Foreground model-facing output: cap retention while + # streaming (head/tail window) so a verbose command + # can't OOM the gateway before truncation (#64435). + # Internal env.execute() consumers (file ops cat + # reads, RPC reads) intentionally stay unbounded. + "bounded_capture": True, } result = env.execute(command, **execute_kwargs) except Exception as e: