fix(sandbox): stamp transient Daytona transport failures at the SDK boundary - #970
Merged
Conversation
… retryable
Under concurrent Daytona load the exec that launches the sandbox LiteLLM
proxy could fail with a message carrying no information at all:
LiteLLM proxy failed to start for model 'deepseek/deepseek-v4-flash':
Failed to execute session command: .
Two separate defects produced that string and its consequence.
Diagnosability. The Daytona SDK wraps every toolbox call as
"<prefix>: " + str(underlying), and httpx raises its timeout and
connection errors with an *empty* message. A read timeout on
execute_session_command therefore stringifies to the bare prefix, and the
exception class -- DaytonaTimeoutError vs DaytonaConnectionError, the only
surviving evidence of what actually went wrong -- was discarded by
interpolating the exception alone. describe_exception() now leads with the
class name, names an empty detail explicitly instead of trailing off, and
appends the status_code/error_code that DaytonaError carries.
Classification. _sandbox_exec calls create_session, then
execute_session_command, then polls with get_session_command. A transport
blip can hit any of the three, but only the "get" prefix was listed as an
infra marker, so the same blip on the other two classified as "other" --
the one category the retry policy never retries. The rollout died on a
transient that had not yet run any agent work, while the identical failure
one call later would have been retried. List all three.
Note this deliberately does not add an SDK-level retry around
execute_session_command: with run_async=True that call is not safely
idempotent (a request that lands but whose response times out would run
the command twice, starting two proxies). Rollout-level retry, which
starts from a fresh sandbox, is the safe bound and already exists.
…boundary
Follow-up to the previous commit, which fixed the reported signature but
closed only two of the roughly six doors on the same corridor and wired
the diagnosability half at a single call site.
Classify at the raise site, not by vendor prefix. The Daytona SDK stamps a
different prefix per method -- "Failed to execute session command: ",
"Failed to upload files: ", "Failed to get file info: " -- while the
failure is identical in every case: an httpx timeout or disconnect against
the toolbox API, raised with an empty message. Enumerating those prefixes
in the classifier is one entry per vendor method, silently incomplete the
moment the SDK adds or renames one (the proxy start's own upload step, one
call earlier than the reported failure, was already missed), and it
over-matches, since a permanent 401/400/409 shares the prefix. The
exception type is only alive at the SDK boundary; by classification time it
is a string that may have crossed a worker boundary. So stamp_transient_
transport wraps every SDK-touching method, asks the existing
_is_daytona_transient_retry_error whether the real class is transient, and
re-raises TransientSandboxTransportError carrying one stable marker.
_looks_like_infra_error matches that marker instead of a prefix census.
Verified against the installed SDK: all eight wrappers on the corridor
(exec, create session, upload, download, search, stat, mkdir, chmod) now
reach infra_failure/retryable, while a permanent failure does not.
Diagnosability is wired into the generic funnel rather than one call site:
Rollout's `except Exception` now persists describe_exception(e), so a
detail-free SDK error raised during the agent run or verifier can no longer
be recorded bare. The two hand-rolled f"{type(exc).__name__}: {exc}" copies
in the agent-plugin loader are subsumed, and describe_exception moves to
_utils/text.py, whose job is plain-text rendering -- diagnostics.py exists
to argue against reverse-engineering error strings.
Still no SDK-level retry on the two session-start calls, now recorded with
the real reason. create_session fixes its uuid before the call, so a
landed-but-timed-out create returns DaytonaConflictError on replay -- a
permanent error outside the transient set, turning a blip into a hard
failure. execute_session_command is worse than non-idempotent: the LiteLLM
launcher binds an ephemeral port, so a replayed exec does not fail loudly
on EADDRINUSE -- it binds a different port and its rm -f overwrites the
first launcher's state/pid files, orphaning a proxy that holds the provider
key and LITELLM_MASTER_KEY, unkillable by _terminate_sandbox_litellm, for
the life of the sandbox. Rollout-level retry against a fresh sandbox is the
safe bound.
Adds a contract test driven through the installed daytona SDK's real
intercept_errors. The stub-based tests would all stay green through a
vendor prefix rename or a remapped exception class while rollouts silently
stopped retrying -- the failure this machinery exists to prevent.
xdotli
temporarily deployed
to
pypi-internal-preview
August 9, 2026 23:37 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed286452af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| TRANSIENT_SANDBOX_TRANSPORT_MARKER = "transient sandbox transport failure" | ||
|
|
||
|
|
||
| class TransientSandboxTransportError(ConnectionError): |
There was a problem hiding this comment.
When a stamped Daytona exec, upload, download, or stat error escapes Rollout.run, inheriting from ConnectionError routes it through the dedicated connection-error handler, whose capture_transport() records a TransportClosedDiagnostic. _build_rollout_result() gives that diagnostic precedence over string classification and therefore persists pipe_closed, not the intended infra_failure; configurations with retry_on_pipe=False and retry_on_infra=True will not retry these transient provider failures, and experiment metrics will misclassify them. Preserve the infra category rather than routing this exception through the generic transport-closed diagnostic.
Useful? React with 👍 / 👎.
|
|
||
| @pytest.mark.asyncio | ||
| async def test_proxy_start_failure_keeps_the_exception_type_and_retries(monkeypatch): | ||
| """A detail-free transport failure must stay diagnosable and retryable. |
There was a problem hiding this comment.
This test explicitly guards the regression where detail-free Daytona timeouts became non-retryable, but its docstring does not identify the PR or commit being protected; add that identifier here and to the other newly introduced regression tests so future maintainers can trace the expected behavior.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
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
1 participant
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.
A rollout could die before running any agent work, with an error that said nothing:
What that empty detail actually was
Reproduced live against the installed Daytona SDK:
"Failed to execute session command: "is the SDK'sintercept_errorsprefix wrapping an httpx transport error whosestr()is empty (ReadTimeout/ConnectTimeout→DaytonaTimeoutError,ConnectError→DaytonaConnectionError). The exec never ran — this is neither "exec failed" nor "exec returned nothing". The SDK classified it correctly; benchflow discarded that by formattingf"...: {exc}".The consequence was the real bug:
_looks_like_infra_errormatched only"failed to get session command", so the identical blip on a sibling call classified as"other"— the one category the retry policy never retries — and killed the rollout. One call later in the same function it would have been absorbed.The fix: decide where the type is still alive
By classification time the exception is a string that may have crossed a worker boundary, so type-matching there is impossible. So the verdict is stamped at the raise site:
stamp_transient_transportwraps every SDK-touching method, consults the existing_is_daytona_transient_retry_errorwhile the vendor class is live, and re-raises a benchflow-ownedTransientSandboxTransportErrorcarrying one stable marker — instead of the classifier tracking one message prefix per vendor method. Applied outside_SDK_RETRY, so the retry budget is spent before stamping.Verified independently across the whole proxy-start corridor — all eight reachable wrappers (
execute session command,create session,upload files,download file,search files,get file info,create folder,set file permissions) now classifyinfra_failureand retry; a permanent 401 sharing the same vendor prefix still classifiesotherand does not retry.Also:
describe_exception(in_utils/text.py) leads with the exception class and names an empty detail explicitly — wired into the genericexcept Exceptionfunnel inrollout/__init__.py(not just one call site) and subsuming two hand-rolled copies inagents/registry.py. The signature now readsDaytonaTimeoutError: Failed to execute session command: (no detail).Deliberately not done: SDK-level retry around
execute_session_command. The launcher binds an ephemeral port, so a replayed exec would not fail loudly onEADDRINUSE— it would bind elsewhere, overwrite the first launcher's state/pid files, and orphan a proxy holding the provider key andLITELLM_MASTER_KEYbeyond_terminate_sandbox_litellm's reach.create_sessionis equally unsafe (itsuuid4is fixed before the call, so a replay returnsDaytonaConflictError, outside the transient set). Rollout-level retry from a fresh sandbox is the safe bound.A contract test drives the real
intercept_errorsfrom the installed SDK through the stamp, so a vendor prefix rename fails a test instead of silently un-retrying rollouts.Honest scope
This does not make Daytona more reliable. If these blips are provider-side capacity, they will keep happening; what changes is that they are legible in the artifact and absorbed by existing bounded retry instead of silently poisoning a result. The motivating run artifacts were lost to a tmp cleanup before analysis, so the mechanism is established from code plus live SDK reproduction, but the frequency and load-dependence are untested — no failure-rate claim is made here.
Review
Structural review (REQUEST-CHANGES → fixed) proved the first cut closed only 2 of ~6 doors on the same path —
_upload_runtime_files_to_sandboxruns one call earlier and was still unretried — and independently validated the non-retry decision with the ephemeral-port reasoning above.Gates: ruff format/check, ty clean; targeted 93 passed; daytona/sandbox/litellm/acp/scoring/rollout sweep 1295 passed; full suite 5397 passed (only the three known host-specific failures).