fix(vision): retry container exec-read for Docker cold-start, surface stderr (#76566)
Under the Docker terminal backend, vision_analyze's first exec-read sometimes returned empty / non-zero against a freshly started container, producing 'could not read <path> inside the sandbox' on a file the agent could cat seconds later. Cold pipe setup on the first exec against a new container, not a permissions or mount problem. Retry once after a short delay (150 ms covers Docker exec warm-up without making a real failure feel sluggish). When every attempt still fails, fold the container's first stderr line into the raised error so the user can tell 'no such file' from 'permission denied' instead of staring at one opaque message. Tests cover the retry-then-succeed path, the diagnostic-on-exhausted path, and confirm the existing single-attempt raise is preserved.
This commit is contained in:
@@ -244,6 +244,57 @@ class TestExecReadSafety:
|
||||
await isrc.resolve_image_source(
|
||||
"/workspace/nope.png", isrc.ResolveContext(task_id="t1"))
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_exec_read_retries_cold_start_then_succeeds(self, tmp_path, monkeypatch):
|
||||
"""#76566: under Docker, vision's first exec-read can fail (cold
|
||||
container / pipe setup) and an identical retry succeeds. The
|
||||
resolver must transparently retry before raising, so users don't
|
||||
see 'could not read inside the sandbox' on a file that is fully
|
||||
readable on the second attempt."""
|
||||
home = tmp_path / "hermes"
|
||||
isrc = _reload(monkeypatch, home)
|
||||
monkeypatch.setenv("TERMINAL_ENV", "docker")
|
||||
|
||||
calls = {"n": 0}
|
||||
b64 = base64.b64encode(PNG).decode()
|
||||
|
||||
def fake_execute(cmd, **kw):
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 1:
|
||||
# First call: cold start — empty pipe, exit non-zero.
|
||||
return {"returncode": 1, "output": ""}
|
||||
return {"returncode": 0, "output": b64}
|
||||
|
||||
with patch("tools.image_source._get_active_env",
|
||||
return_value=SimpleNamespace(execute=fake_execute)):
|
||||
res = await isrc.resolve_image_source(
|
||||
"/workspace/cold.png", isrc.ResolveContext(task_id="t1"))
|
||||
assert res.origin == "container"
|
||||
assert res.data == PNG
|
||||
assert calls["n"] == 2
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_exec_read_retries_exhausted_includes_diagnostic(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
"""#76566: when every retry still fails, the error must carry the
|
||||
container's stderr/stdout so the user can tell 'no such file'
|
||||
from 'permission denied' from 'cold start never came up'."""
|
||||
home = tmp_path / "hermes"
|
||||
isrc = _reload(monkeypatch, home)
|
||||
monkeypatch.setenv("TERMINAL_ENV", "docker")
|
||||
|
||||
def fake_execute(cmd, **kw):
|
||||
return {"returncode": 1, "output": "head: can't open '/x': No such file or directory"}
|
||||
|
||||
with patch("tools.image_source._get_active_env",
|
||||
return_value=SimpleNamespace(execute=fake_execute)):
|
||||
with pytest.raises(isrc.SourceNotFound) as excinfo:
|
||||
await isrc.resolve_image_source(
|
||||
"/workspace/missing.png", isrc.ResolveContext(task_id="t1"))
|
||||
# Diagnostic surfaced — the user can act on it.
|
||||
assert "No such file or directory" in str(excinfo.value)
|
||||
|
||||
|
||||
class TestSvgNormalization:
|
||||
"""SVG resolves end-to-end: the resolver passes it through as
|
||||
|
||||
@@ -315,6 +315,17 @@ async def _resolve_container_fallback(
|
||||
|
||||
Fail-closed: if there is no active sandbox env we refuse rather than falling
|
||||
back to a host read, so a non-cache host path under a sandbox never leaks.
|
||||
|
||||
Cold-start retry: under Docker the very first exec against a freshly
|
||||
started container can fail (empty pipe / partial setup) while an identical
|
||||
second call succeeds. We retry once with a short delay before giving up,
|
||||
so callers don't see "could not read inside the sandbox" on a file that is
|
||||
verifiably readable on the immediate retry. See #76566.
|
||||
|
||||
Diagnostic: when every attempt fails, the container's own output (stderr
|
||||
+ stdout) is folded into the raised error so the user can distinguish
|
||||
"no such file" from "permission denied" from "container never came up"
|
||||
instead of staring at one opaque message.
|
||||
"""
|
||||
import asyncio
|
||||
import shlex
|
||||
@@ -340,13 +351,29 @@ async def _resolve_container_fallback(
|
||||
# env.execute is a blocking backend exec; keep it off the event loop so a
|
||||
# multi-MB base64 read doesn't stall every other coroutine.
|
||||
qp = shlex.quote(str(p))
|
||||
res = await asyncio.to_thread(
|
||||
env.execute,
|
||||
f"head -c {_MAX_INGEST_BYTES + 1} < {qp} | base64 | tr -d '\\n'")
|
||||
if res.get("returncode", 1) != 0:
|
||||
raise SourceNotFound(f"could not read '{p}' inside the sandbox", src=src, origin="container")
|
||||
cmd = f"head -c {_MAX_INGEST_BYTES + 1} < {qp} | base64 | tr -d '\\n'"
|
||||
|
||||
last_res: dict = {"returncode": 1, "output": ""}
|
||||
for attempt in range(2):
|
||||
last_res = await asyncio.to_thread(env.execute, cmd)
|
||||
if last_res.get("returncode", 1) == 0:
|
||||
break
|
||||
if attempt == 0:
|
||||
# Cold-start: give the container a moment to settle its pipes
|
||||
# before retrying. 150ms covers Docker exec warm-up in practice
|
||||
# without making a real failure feel sluggish.
|
||||
await asyncio.sleep(0.15)
|
||||
if last_res.get("returncode", 1) != 0:
|
||||
diag = (last_res.get("output") or "").strip().splitlines()
|
||||
# Keep the diagnostic small and noise-free: first non-empty line,
|
||||
# trimmed to a sane length so it slots into the agent's error UI.
|
||||
first = next((ln.strip() for ln in diag if ln.strip()), "")
|
||||
suffix = f" ({first[:200]})" if first else ""
|
||||
raise SourceNotFound(
|
||||
f"could not read '{p}' inside the sandbox{suffix}",
|
||||
src=src, origin="container")
|
||||
try:
|
||||
data = base64.b64decode(res.get("output", ""), validate=True)
|
||||
data = base64.b64decode(last_res.get("output", ""), validate=True)
|
||||
except Exception as exc:
|
||||
raise NotAnImage(f"sandbox returned non-image data for '{p}': {exc}", src=src)
|
||||
if len(data) > _MAX_INGEST_BYTES:
|
||||
|
||||
Reference in New Issue
Block a user