Add Daytona sandbox provider - #1513
Conversation
| if allow_partial: | ||
| prefix: list[SandboxHandle] = [] | ||
| for result in results: | ||
| if isinstance(result, Exception): | ||
| break | ||
| prefix.append(result) | ||
| return prefix | ||
| for handle in handles: | ||
| await self.close(handle, delete=True) | ||
| raise DaytonaCreateError( | ||
| f"One or more Daytona sandboxes failed during batch create; failed={len(errors)}, requested={count}" | ||
| ) from errors[0] |
There was a problem hiding this comment.
when allow_partial is True this function returns only the successful sandboxes up to the first exception, ignoring any later successful handles, which leaks/hides successful sandboxes. Return a structured success/error result, or delete every success not returned; cleanup should attempt all deletes and aggregate cleanup errors.
There was a problem hiding this comment.
create_batch now waits for every create attempt, returns all successful handles when allow_partial=True, and cleans up every successful handle while aggregating cleanup errors when partial results are not allowed. Commit: hemildesai@57d9689
| async def create(self, spec: SandboxSpec) -> SandboxHandle: | ||
| spec = _normalize_spec(spec) | ||
| max_attempts = self._create.retries + 1 | ||
| for attempt_number in range(1, max_attempts + 1): | ||
| try: | ||
| return await self._create_once(spec) | ||
| except Exception as e: | ||
| if attempt_number >= max_attempts or not _is_retryable_create_error(e): | ||
| raise | ||
| await asyncio.sleep( | ||
| self._retry_sleep_s(attempt_number, self._create.retry_delay_s, self._create.retry_max_delay_s) | ||
| ) | ||
| raise RuntimeError("Daytona create retry loop did not run") |
There was a problem hiding this comment.
Make create retries idempotent. If Daytona creates a sandbox and the client times out before receiving the ID, this retry path can create a duplicate that cleanup can't identify.
There was a problem hiding this comment.
create retries now use a retry-stable generated Daytona sandbox name when the caller has not provided daytona.name; explicit names are preserved. Ambiguous create timeouts remain non-retried. Commit: hemildesai@e703f21
| LOGGER.debug( | ||
| "Ignoring unsupported Daytona %s settings: %s", config_cls.__name__, ", ".join(unsupported_keys) | ||
| ) | ||
| return config_cls(**{key: val for key, val in value.items() if key in field_names}) |
There was a problem hiding this comment.
Fail fast on unsupported Daytona provider config keys instead of silently dropping them. A typo in lifecycle/network settings (for example auto_delete_interval or network_block_all) can make the caller think a safety control is enabled when it is not.
Suggested direction:
if unsupported_keys:
raise ValueError(
f"Unsupported Daytona {config_cls.__name__} settings: {', '.join(unsupported_keys)}"
)There was a problem hiding this comment.
unsupported Daytona provider config keys now fail fast with a ValueError, including a test for unknown create settings. Commit: hemildesai@57d9689
| return ImageParams(**kwargs) | ||
| if snapshot_id is not None: | ||
| kwargs["snapshot"] = snapshot_id | ||
| return SnapshotParams(**kwargs) |
There was a problem hiding this comment.
Reject resources when creating from a Daytona snapshot, or split the API shape so image-backed and snapshot-backed creation cannot be confused. Right now resources are applied only to CreateSandboxFromImageParams; with snapshot_id, the same SandboxSpec.resources value is silently ignored.
Suggested direction:
if snapshot_id is not None:
if spec.resources:
raise ValueError("Daytona snapshot creation does not support resource overrides")
kwargs["snapshot"] = snapshot_id
return SnapshotParams(**kwargs)There was a problem hiding this comment.
snapshot-backed Daytona creation now rejects resource overrides instead of silently ignoring them, with a unit test for that path. Commit: hemildesai@57d9689
| timeout_s=self._operations.close_timeout_s, | ||
| ) | ||
| info_status = getattr(info, "status", None) | ||
| return _to_sandbox_status(getattr(info_status, "state", info_status)) |
There was a problem hiding this comment.
Use Daytona's native sandbox state refresh path here. The async SDK exposes sandbox state as sandbox.state after await sandbox.refresh_data(), so checking raw.status / get_info().status can classify a valid sandbox as UNKNOWN if those attributes are not populated on the SDK object.
Suggested direction:
await self._await_operation(
lambda: handle.raw.refresh_data(),
operation="refresh_data",
sandbox_id=handle.sandbox_id,
timeout_s=self._operations.close_timeout_s,
)
return _to_sandbox_status(getattr(handle.raw, "state", None))There was a problem hiding this comment.
status() now prefers Daytona native refresh_data() and reads sandbox.state, with fallbacks only for SDK objects that do not expose that method. Commit: hemildesai@57d9689
| operation="process.exec", | ||
| sandbox_id=handle.sandbox_id, | ||
| timeout_s=float(timeout_s) + self._operations.command_timeout_margin_s if timeout_s is not None else None, | ||
| retries=self._command_retry_count() if retries is None else retries, |
There was a problem hiding this comment.
Default command retries to zero. Retrying create/status/file/delete operations is fine, but retrying arbitrary guest commands can run user work twice if the command reached Daytona and only the response was lost. That can duplicate writes, re-apply patches, or rerun non-idempotent setup/tests.
Suggested direction:
command_retries: int = 0and keep explicit opt-in for callers that know their command is idempotent.
There was a problem hiding this comment.
command_retries now defaults to zero, so arbitrary guest commands are not retried unless a caller explicitly opts in. Commit: hemildesai@57d9689
| return command | ||
| if isinstance(user, int): | ||
| return f"setpriv --reuid={user} --regid={user} --clear-groups /bin/sh -c {shlex.quote(command)}" | ||
| return f"su -s /bin/sh -c {shlex.quote(command)} {shlex.quote(user)}" |
There was a problem hiding this comment.
Document or enforce that per-command user= support is Linux/POSIX-only for this provider. This wrapper depends on /bin/sh, su, and setpriv, so it will break on Windows snapshots and can also fail on minimal Linux images that do not include those tools.
Suggested direction: either reject non-root user unless the provider is known to be running a POSIX image with the required tools, or leave user unsupported until Daytona exposes provider-native per-command user switching.
There was a problem hiding this comment.
per-command non-root user= switching is now unsupported for Daytona and raises clearly; callers can use create.os_user for sandbox-level user selection until Daytona has native per-command support. Commit: hemildesai@57d9689
| if stderr is None and artifacts is not None: | ||
| stderr = getattr(artifacts, "stderr", None) | ||
| return_code = getattr(response, "exit_code", None) | ||
| return SandboxExecResult(stdout=stdout, stderr=stderr, return_code=0 if return_code is None else return_code) |
There was a problem hiding this comment.
Add a provider test for stderr-only output. Daytona's plain process.exec() docs clearly expose result / artifacts.stdout; stderr is clearer on the session-command API than on plain exec. This mapping may be correct, but it should be proven because compiler/test diagnostics often appear only on stderr.
Suggested test shape:
result = await provider.exec(handle, "python -c 'import sys; print("err", file=sys.stderr)'")
assert result.stderr and "err" in result.stderrThere was a problem hiding this comment.
Added a provider unit test covering stderr-only output from Daytona process.exec() artifacts and preserving the non-zero exit code. Commit: hemildesai@57d9689
|
|
||
| async def close(self, handle: SandboxHandle, *, delete: bool) -> None: | ||
| if not delete: | ||
| return |
There was a problem hiding this comment.
Clarify the lifecycle semantics here. With delete=False, Daytona close() does not stop, archive, or delete the remote sandbox; it only detaches the client. That is useful for debugging, but dangerous as a default in eval/RL runs because quota-consuming sandboxes can be left alive.
Suggested direction: make the normal eval path delete/ephemeral by default, and expose retention as an explicit debug/retain mode rather than making close(delete=False) look like remote cleanup.
There was a problem hiding this comment.
close(handle) now deletes by default for the normal eval path, while explicit close(delete=False) remains available for debug retention and logs that the remote sandbox is being left alive. Commit: hemildesai@e703f21
13dd946 to
4ba2396
Compare
fcc8418 to
e703f21
Compare
| batch: | ||
| concurrency: 3 | ||
| sandbox_spec: | ||
| timeout_s: 18000 |
There was a problem hiding this comment.
This config value gets ignored: MiniSWESandboxEnvironment reads ttl_s not timeout_s
| timeout_s: 18000 | |
| ttl_s: 18000 |
There was a problem hiding this comment.
Changed in ca552e8: the Daytona smoke config now uses ttl_s: 18000 instead of timeout_s.
| provider_options: | ||
| platform: | ||
| os: linux | ||
| arch: amd64 |
There was a problem hiding this comment.
The Daytona provider does not read these values, and Daytona has no equivalent for {os, arch}. Ideally unknown spec/provider fields should be rejected instead of silently ignored.
| provider_options: | |
| platform: | |
| os: linux | |
| arch: amd64 | |
| provider_options: |
There was a problem hiding this comment.
Changed in ca552e8: removed the ignored platform os/arch block from the smoke config and added Daytona provider_options validation so unsupported provider options now fail fast.
| for key in ( | ||
| "api_key", | ||
| "jwt_token", | ||
| "organization_id", | ||
| "api_url", | ||
| "server_url", | ||
| "target", | ||
| "connection_pool_maxsize", | ||
| "otel_enabled", | ||
| ): | ||
| value = getattr(self._connection, key) | ||
| if value is not None: | ||
| kwargs[key] = value | ||
| self._daytona = AsyncDaytona() if not kwargs else AsyncDaytona(DaytonaConfig(**kwargs)) | ||
| return self._daytona |
There was a problem hiding this comment.
The Daytona SDK supports connection_pool_maxsize=None to remove the HTTP connection limit, but that gets filtered out by this provider before constructing DaytonaConfig preventing users from intentionally selecting the unlimited-pool mode.
Suggested fix: preserve which config keys were supplied, or use an explicit unset sentinel, so connection_pool_maxsize: null can be passed through intentionally.
There was a problem hiding this comment.
Changed in ca552e8: tracked explicitly supplied connection config keys so connection_pool_maxsize: null is passed through to DaytonaConfig, with unit coverage.
| handle = SandboxHandle(sandbox_id=str(sandbox.id), provider_name=self.name, raw=sandbox) | ||
| try: | ||
| await self._verify_created_handle(handle) | ||
| except Exception: | ||
| await self.close(handle, delete=True) | ||
| raise | ||
| return handle |
There was a problem hiding this comment.
When Daytona creates a sandbox but the readiness probe fails, the provider tries to delete the sandbox. If that delete also fails, the cleanup exception replaces the original probe failure.
Suggested fix: catch cleanup exceptions, log them with sandbox_id, and re-raise the original probe/create exception.
There was a problem hiding this comment.
Changed in ca552e8: create-probe cleanup now catches/logs delete failures with sandbox_id and re-raises the original probe/create exception, with unit coverage.
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
9e0bd89 to
ca552e8
Compare
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
5 participants
Stacked on #1377.
Closes #1687
Adds a Daytona-backed sandbox provider to the sandbox API provider registry, including SDK dependency wiring and provider lifecycle/exec/file helpers.
Smoke notes:
Total disk limit exceeded. Maximum allowed: 30GiB), so the observed 1/16 reward is not a meaningful model-quality pass rate.sandbox-api: daytona-sdk.Daytona full SWE-bench Verified validation
hemild-daytona-full-500-c64-014139completed successfully (1/1, pod0restarts)./mnt/rl-workspace/hemild/gym_eval/refactor/runs/mini_swe_agent_2_ng_collect_rollouts/20260624-014139-daytona-full-verified-c64-step1800/results/mini_swe_agent_2_sglang_qwen35_dflash_daytona_full500.jsonl500/500rollout rows,324/500solved, pass@1 / avg rollout reward0.648(64.8%).64,enable_thinking=true,temperature=0.6,top_p=0.95,max_output_tokens=32768, step timeout1800s, per-command timeout1500s, Daytona command retries0.31/500runtime error rows (6.2%eval error rate):10Daytona blank/process/executeHTTP/request timeouts,9Daytona command-timeout408s,10Daytona container-IP resolution failures,1Daytona502 Bad Gateway, and1modellitellm/API timeout. No separate Gym schema/config failure class was observed; remaining failures were normal model/test failures.