Skip to content

feat(opensandbox): keepalive-bounded transport + create hardening + resource requests/limits - #2212

Merged
bxyu-nvidia merged 31 commits into
mainfrom
hemil/opensandbox-transport-hardening
Jul 31, 2026
Merged

feat(opensandbox): keepalive-bounded transport + create hardening + resource requests/limits#2212
bxyu-nvidia merged 31 commits into
mainfrom
hemil/opensandbox-transport-hardening

Conversation

@hemildesai

@hemildesai hemildesai commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Hardens the OpenSandbox provider and the mini_swe_agent_2 eval path so that large sandboxed agent evals (SWE-bench Verified scale: hundreds of concurrent rollouts against a Kubernetes-backed OpenSandbox deployment) run end-to-end reliably: every finished rollout is delivered and recorded, and sandbox-infrastructure noise is bounded instead of silently zeroing rewards. Diagnosed on multi-node runs at concurrency 300–1500; the same failure signatures appear at concurrency 8, just less often.

1. Server disconnected without sending a response

The SDK's default httpx pool keeps idle connections for 30s (opensandbox.config.connection.with_transport_if_missing), but the OpenSandbox server's uvicorn keep-alive reaper closes idle sockets after ~5s. Agent workloads idle between sandbox commands (model think time), so commands routinely reuse a socket the server already closed — surfacing as httpx.RemoteProtocolErrorSandboxInternalException, which permanently kills the rollout.

Fix: the provider injects a transport whose keepalive_expiry sits below the server's keep-alive timeout (default 3s, connection.keepalive_expiry_s):

  • connection.transport_backend: httpx (default) — stock httpx.AsyncHTTPTransport(limits=..., retries=connect_retries). No new required dependencies.
  • connection.transport_backend: aiohttp (opt-in) — aiohttp pool under the SDK's httpx surface via httpx-aiohttp; falls back to the httpx transport with a warning when absent.
  • keepalive_expiry_s: null disables injection entirely (SDK default transport).

2. 502 could not connect to the backend sandbox endpoint='<podIP>:44772' on first command

With create.skip_health_check: true, Sandbox.create returns before the pod's exec daemon is listening; the first command races pod startup and the server proxy 502s. Under a create burst this killed 7–12 rollouts per run.

Fix: create.skip_health_check: false (create waits for readiness, bounded by the spec's ready_timeout_s; create.timeout_s kept above the ready timeout).

command_retries stays at 0. Retrying a command the server may have already started would execute it twice, and agent commands are frequently mutating. The keepalive bound removes the stale-connection failures retries were compensating for. Raise it only for idempotent workloads.

3. Separate resource requests and limits

Sandbox.create accepts distinct resource (limits) and resource_requests maps; a lone resource map is applied by the server as requests=limits. This exposes the requests side via sandbox_spec.provider_options.resource_requests (same keys as SandboxSpec.resources). Motivation: SWE-bench test suites OOM-killed sandbox pods at 2Gi, but raising a single combined map to 8Gi would 4× the cluster reservation; with the split, limits rise while requests stay small and pods pack densely. Requires opensandbox>=0.1.15 (lower bound raised here).

4. Sandbox teardown off the rollout's critical path

env.cleanup() ran in the rollout Ray task's finally block, so a finished result only became fetchable after the sandbox DELETE returned — and a teardown failure re-raised over the successful eval, degrading it to a reward-0 error row. Teardown is also effectively one-shot (Sandbox.stop latches _closed before dispatch), so a single failed DELETE leaked the sandbox while still costing the rollout.

Fix: cleanup runs on a best-effort daemon thread; results return immediately, teardown errors are logged rather than raised, and orphans remain covered by the provider's sandbox TTL.

5. High-concurrency rollout delivery (mini_swe_agent_2)

Two coupled fixes so every finished rollout is delivered at high concurrency (note: at very high concurrency the single-process policy-model proxy remains a separate bottleneck — connection-refused storms confirmed by a controlled A/B — with its scaling fix tracked separately):

  • await the Ray ObjectRef instead of asyncio.to_thread(ray.get, ...): the default executor caps at min(32, cpu+4) threads, each pinned for a full rollout, so delivery stalls at ~32 concurrent rollouts and finished tasks queue behind blocked ray.get calls.
  • bounded litellm retries (num_retries=5, config-overridable) — no retry means one transient LLM-call failure kills a whole rollout; unbounded retries make failures look like hangs — and num_cpus=0.25 on the rollout Ray task so concurrency is not capped at cluster core count (rollouts are I/O-bound).

6. datasets declared in the base agent config

The struct-mode config merge rejects keys absent from the base config, so a benchmark config using _inherit_from could not add its dataset list (ConfigKeyError: Key 'datasets' is not in struct). Declared empty in the base server config, matching how other agents (e.g. swe_agents) expose it.

Validation

  • Requests/limits split (paired multi-node runs, identical except the variable under test): infra-failed rollouts 41/316 (13.0%) at 2Gi requests=limits → 19/288 (6.6%) at 8Gi limits / 2Gi requests; pass@1 58.0% → 67.1%.
  • Keepalive bound + health-checked create: zero Server disconnected events and zero create-race 502s across all subsequent runs (previously 2–12 per run).
  • Full stack, end-to-end from this branch: single-pass SWE-bench Verified (500 instances, concurrency 500+) delivered >98% of rollouts with ~1.6% residual sandbox-infra failures (server-side proxy 502s on established sandboxes, tracked separately) and zero client-transport failures. Before the delivery fixes (Alias as Penguin #4/Comp-Coding Verifier #5), an identically shaped run lost the majority of completed evaluations at the wall — hundreds of finished evals, only tens recorded.

Relationship to #2020

Complementary, no overlap; #2020 (job attribution metadata) has since merged and this branch is updated on top of it. Field-validated together — the attribution labels are what make post-cancellation sandbox garbage collection safely scoped to a single job.

Testing

  • tests/unit_tests/test_opensandbox_provider.py: transport-backend coverage (httpx default with keepalive expiry, custom pool settings, fallback when httpx_aiohttp is unavailable, null disables injection), aiohttp opt-in (importorskip-guarded), and requests/limits plumbing tests. 16 passed with and without httpx-aiohttp.
  • responses_api_agents/mini_swe_agent_2/tests/test_app.py: updated for the awaited ObjectRef (awaitable FakeObjectRef); assertions strengthened to check the Ray call's params.
  • CI green except a pre-existing tau2 failure on main (reproduces on unrelated PRs).

🤖 Generated with Claude Code

hemildesai and others added 7 commits July 30, 2026 09:43
…nd hardening

Sandboxed agent evals at high concurrency hit two infra failure classes that
silently zero out rollout rewards:

1. "Server disconnected without sending a response": the SDK's default httpx
   pool keeps idle connections 30s, but the OpenSandbox server's uvicorn
   keep-alive reaper closes them after ~5s. Agent workloads idle between
   commands (model think time), so commands routinely reuse a socket the
   server already closed. The provider now injects a transport with
   keepalive_expiry below the server timeout (default 3s) — aiohttp-backed
   via httpx-aiohttp (connection.transport_backend: aiohttp, new sandbox
   extra dependency), falling back to httpx.AsyncHTTPTransport when the
   bridge is unavailable.

2. 502 "could not connect to the backend sandbox endpoint": with
   create.skip_health_check: true the first command races a pod whose exec
   daemon is not listening yet. The shipped config now health-checks on
   create (bounded by the spec's ready_timeout_s; create.timeout_s lowered
   to 900 to stay coherent) and allows 2 command retries — retries only
   fire for retryable-classified errors, never for command timeouts, so
   long-running commands are not double-executed.

Validated on a 4-node SWE-bench Verified run (1500 rollouts, concurrency
500) against an EKS OpenSandbox deployment: disconnect and 502 rollout
kills went from ~5 per 6 rollouts collected / ~7-12 per 64-rollout run to
zero observed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
…tp opt-in

httpx-aiohttp is pre-1.0 with limited adoption, so it should not be a hard
dependency or the default path. The keepalive-expiry fix is transport-
agnostic and fully delivered by the stock httpx transport; the aiohttp
backend remains available behind connection.transport_backend: aiohttp for
users who install httpx-aiohttp explicitly (graceful fallback + warning
when missing).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
…tions

The OpenSandbox SDK's create() accepts distinct `resource` (limits) and
`resource_requests` maps; the server applies a lone `resource` map as
requests=limits. Expose the requests side through
sandbox_spec.provider_options.resource_requests (same keys as
SandboxSpec.resources), so memory-spiky workloads (e.g. SWE-bench test
suites) can run with high limits while keeping small scheduling requests
for dense packing — without inflating cluster reservations.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Retrying a command the server may have already started would execute it
twice, and agent commands are frequently mutating (file writes, patch
application, test-state changes). The keepalive bound added in this branch
removes the stale-connection failures that retries were compensating for,
so the retry default is not needed and is reverted to 0.

Also raises the opensandbox lower bound to 0.1.15, the version exposing
separate `resource`/`resource_requests` on Sandbox.create that the new
provider_options.resource_requests support depends on.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
…limits

Applies the new provider_options.resource_requests support to the SWE-bench
harness config: limits 1 vCPU / 8Gi (the burst ceiling that keeps
memory-spiky test suites from being OOM-killed mid-rollout) with scheduling
requests kept at 0.5 vCPU / 2Gi so the cluster still packs sandboxes densely.

Measured on 4-node GB200 SWE-bench Verified runs: raising the memory ceiling
this way cut infra-caused rollout losses from 13.0% to 6.6% and lifted pass@1
from 58.0% to 67.1%, without increasing the per-sandbox cluster reservation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
Cut the explanatory comment blocks down to the constraint each setting
actually needs, matching surrounding comment density.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
…tion

The provider now injects a keepalive-bounded transport into ConnectionConfig
whenever connection.keepalive_expiry_s is set (default 3.0), so the exact-dict
assertion in test_opensandbox_connect_after_create_preserves_request_timeout
gained an unexpected 'transport' key and failed. Drop that key before the
comparison; the transport's identity and pool settings are already asserted in
test_opensandbox_provider.py.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

hemildesai and others added 6 commits July 30, 2026 09:48
The requests/limits split is the point of this change; lowering the CPU
ceiling was an unrelated and unjustified edit. Limits stay at the existing
2 vCPU / 8Gi, with scheduling requests added at 0.5 vCPU / 2Gi.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
With skip_health_check now false, Sandbox.create waits for readiness
internally (ready_timeout=1200s for mini_swe_agent_2), but the provider
wraps that call in asyncio.wait_for(create.timeout_s). At 900s the outer
guard fired first and aborted creates the SDK was still legitimately
waiting on, and with create.retries=10 each attempt burned the full 900s.

Raises timeout_s to 1500 so the outer bound sits above the readiness wait
with headroom for the create round-trip itself.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
…ults

env.cleanup() ran in the rollout task's finally block, so the computed
result only became fetchable after the sandbox DELETE returned - and a
teardown failure re-raised over the successful eval, degrading it to a
reward-0 error row. Teardown is also effectively one-shot (Sandbox.stop
latches _closed before dispatch), so a single failed DELETE leaks the
sandbox permanently while still costing the rollout.

Run cleanup on a best-effort daemon thread instead: results return
immediately, teardown errors are logged rather than raised, and orphans
remain covered by the provider's sandbox TTL.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
Five coupled fixes that let a full-benchmark eval run deliver every
finished rollout instead of stalling behind infrastructure limits:

- await the Ray ObjectRef instead of asyncio.to_thread(ray.get, ...):
  the default executor caps at min(32, cpu+4) threads, each pinned for
  a full rollout, so finished tasks queue behind blocked ray.get calls
  and results stop flowing at ~32 concurrent rollouts.
- move the per-rollout sandbox-config and result-JSON writes off the
  event loop (asyncio.to_thread); under high concurrency synchronous
  shared-filesystem writes in the handler block every other rollout.
- NEMO_GYM_LOCAL_RESULTS_DIR can point the per-instance result JSONs
  (and the skip_if_exists resume check, which now reads the same root)
  at node-local disk; default behavior is unchanged.
- NEMO_GYM_BYPASS_POLICY_PROXY=1 points litellm straight at the policy
  model's base URL, skipping the single-process policy-model proxy that
  saturates ahead of the model under high rollout concurrency. Off by
  default; loses rollout capture, which pure eval runs do not use.
- default litellm num_retries to 5 (config-overridable) so a transient
  LLM-call failure does not kill a whole rollout, and reserve 0.25 CPUs
  per rollout Ray task so concurrency is not capped at core count.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
The global ClientSession had no timeouts at all, so a request whose
connection is never established blocks its caller forever. sock_connect
bounds setup; deliberately no read/total timeout, because some requests
on this session (e.g. an agent /run) legitimately stay silent on the
wire until a whole rollout finishes, and aborting them triggers the
retry loop to re-POST duplicate rollouts.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
run() now awaits the Ray ObjectRef directly instead of going through
asyncio.to_thread(ray.get, ...), and offloads the config and result
writes to asyncio.to_thread instead of writing them inline.

The tests still patched asyncio.to_thread as a stand-in for ray.get, so
after the change the mocked ObjectRef stayed a bare MagicMock that
cannot be awaited, and the patch silently swallowed both file dumps.
That failed five tests: the successful-run reward dropped to 0, the two
config-reading tests hit FileNotFoundError, and the two error tests had
their injected exception raised from the config dump, before the
try/except that is supposed to catch it.

Replace the MagicMock ObjectRef with an awaitable stand-in that resolves
to the result or raises, and give the to_thread mock a side effect that
runs the callable in place, restoring the synchronous write semantics
the assertions were written against. assert_run_mini_swe_called now
checks the Ray remote call's params, which is what it was really after.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
@hemildesai

Copy link
Copy Markdown
Contributor Author

/claude review

@hemildesai

Copy link
Copy Markdown
Contributor Author

/ok to test 3eb2171

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

SHIP WITH CARE — high-concurrency perf/reliability tuning for the mini_swe_agent_2 harness and OpenSandbox provider. No silent-scoring or async-hang defects found; the core changes match the repo's own prescribed patterns. One operability tradeoff worth sizing before merge (inline).

Verified good:

  • result = await future on the Ray ObjectRef (app.py:868) is exactly the pattern CLAUDE.md prescribes and removes the asyncio.to_thread(ray.get, ...) executor cap (~32 in-flight) that serialized finished rollouts. Correct.
  • File dumps (_dump_config, _dump_result) moved off the event loop via asyncio.to_thread — right call for a handler serving all concurrent rollouts; shared-FS mkdir+write was blocking the loop.
  • ClientTimeout(sock_connect=30) on the shared global aiohttp client bounds connect only, deliberately leaving no read/total timeout so long-silent /run rollouts aren't aborted and re-POSTed as duplicates. Reasoning is sound and documented.
  • num_retries=5 via setdefault (config wins) is a reasonable transient-LLM-failure guard.
  • resource_requests plumbing: validated as a mapping in from_mapping, converted through SandboxResources.from_mapping, and covered by a new test asserting both requests and limits reach the SDK, plus the TypeError and unknown-key paths. Good coverage.
  • Transport injection into the OpenSandbox SDK provides the aiohttp-adapter escape hatch CLAUDE.md recommends (transport_backend: aiohttp), guarded by ImportError→httpx fallback. Defaulting to httpx here is defensible: sandbox create/exec/delete is low-volume per provider, not the 16k-request path the O(n²) warning targets. Tests cover default, custom-pool, and fallback paths.

One item to weigh (inline, NOTE): sandbox teardown is now a fire-and-forget daemon thread. The finally-block motivation is correct, but the "orphans covered by TTL" backstop weakens on Ray worker churn — a daemon thread dying mid-cleanup() leaks the sandbox until ttl_s (18000s / 5h in the exemplar). Under sustained high concurrency that's a large orphan pool if the provider bills per live sandbox or enforces a pod quota. Size the TTL against expected worker churn, or add a bounded reaper.

opensandbox>=0.1.15 bump is correctly declared in both pyproject.toml and uv.lock, justified by the new resource_requests create arg.

threading.Thread(target=_cleanup_env_best_effort, args=(env,), daemon=True).start()


def _cleanup_env_best_effort(env: Any) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NOTE — sandbox teardown moved to a fire-and-forget daemon thread. Motivation is sound (a slow/hanging stop() in finally previously delayed every finished result and re-raised over it). But the leak backstop is weaker than the comment implies: on Ray worker churn (autoscale-down, task eviction, worker crash), the daemon thread dies mid-cleanup() and the sandbox leaks until TTL — ttl_s: 18000 (5h) in the exemplar config. Under high rollout concurrency that's a large steady-state pool of orphaned sandboxes. If the provider bills per live sandbox or has a pod quota, this can starve new rollouts. Consider a bounded reaper or a shorter TTL for eval runs. Deliberate tradeoff — author's call, but size the TTL against expected worker churn.

commit_id: 3eb2171

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deliberate tradeoff, keeping it as-is for this PR — but the analysis is fair and worth recording.

Why the daemon thread: the in-band stop() in finally ran before the task result became fetchable, so a slow or hanging teardown gated every finished rollout, and a raising teardown destroyed an otherwise successful result. Under high rollout concurrency that was the dominant source of both stalled delivery and lost work, which is what this PR is about.

On the leak window, you are right that it is not zero and not identical to the old behavior. With in-band cleanup, teardown finished before the task returned, so worker teardown could not interrupt it. Fire-and-forget genuinely widens the window: on autoscale-down, eviction, or worker crash the thread can die mid-cleanup(). Accepted, because the backstop is not the thread — orphans are reclaimed by the provider TTL plus an out-of-band sweep on the operator side, and a crashed worker was never going to complete an in-band cleanup either.

On TTL sizing: agreed that 5h is provisioned for long rollouts rather than for churn, and that it is the right knob if orphan pressure shows up. Leaving it unchanged here so this PR stays scoped to the transport and delivery path; sizing it against observed churn is a config-level follow-up for the author to make with real numbers rather than a guess.

The struct-mode config merge rejects keys absent from the base config, so
a benchmark config using _inherit_from could not add its dataset list and
'gym eval prepare' failed with ConfigKeyError: Key 'datasets' is not in
struct. Declare it empty in the base server config, matching how other
agents (e.g. swe_agents) expose it.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
@hemildesai

Copy link
Copy Markdown
Contributor Author

/ok to test 3e8e5e6

@hemildesai

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

SHIP WITH CARE — solid throughput/reliability work, well-commented and test-backed. No blockers; two things worth an explicit eyes-open before merge.

Reviewed against CLAUDE.md conventions:

  • await future (app.py:868) replacing await asyncio.to_thread(ray.get, future) matches the documented Ray-in-async pattern exactly, and lifts the ~32-thread executor ceiling that was serializing finished rollouts. Good fix.
  • The httpx.AsyncHTTPTransport in provider.py:_build_transport is not a regression against the "no httpx" rule: the OpenSandbox SDK is already httpx-based, so this only bounds the pool (max_connections=100, keepalive_expiry=3.0) to fix stale-socket reuse against uvicorn's ~5s idle close. The aiohttp bridge is offered as opt-in. Correct call.
  • ClientTimeout(sock_connect=30) (server_utils.py) deliberately keeps no read/total timeout on the shared session — right, since a long-silent /run must not be aborted and re-POSTed as a duplicate rollout.

RISK — fire-and-forget sandbox cleanup can leak up to the TTL (app.py:~832, _run_mini_swe_v2 finally → threading.Thread(..., daemon=True)):

  • WHAT BREAKS: cleanup now runs in a daemon thread that outlives the Ray task return. If the Ray worker process is recycled/torn down (idle autoscale-down, worker reuse churn) before the thread finishes env.cleanup(), the sandbox delete never lands.
  • BLAST RADIUS: orphaned sandboxes persist until ttl_s (18000s = 5h in the exemplar). Under high-churn training this can accumulate many live sandboxes → provider resource/cost pressure, not a correctness bug. The comment acknowledges TTL coverage — flagging so it's a conscious tradeoff, not a surprise on the first big run.
  • FIX (optional): if leak accumulation bites, lower ttl_s or move cleanup to a bounded provider-side reaper rather than per-task daemon threads.

NOTE — undeclared direct httpx import (provider.py:_build_transport, import httpx): httpx is only a transitive dep (via opensandbox/openai), not a declared direct dependency in pyproject.toml. It's guaranteed present wherever this sandbox-extra code runs, so this won't break today, but per the dependency-hygiene convention a direct import ideally gets a direct declaration.

Config changes (skip_health_check: false, create timeout_s: 1500 > ready_timeout_s: 1200, resource_requests, num_retries: 5, num_cpus=0.25) are internally consistent and the new resource_requests path is covered by test_direct_create_passes_resource_requests_to_sdk_create. Test refactor to FakeObjectRef correctly models the awaitable ObjectRef.

hemildesai and others added 2 commits July 31, 2026 14:36
connect_retries was passed only to the httpx transport, so opting into
transport_backend: aiohttp silently dropped it. httpx_aiohttp's
AiohttpTransport takes the same retries argument, so pass it on both
paths.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
connect_retries was configured in the transport test but never asserted,
on either backend — which is why it went unnoticed that the aiohttp path
dropped it.

Assert it on both: the httpx pool carries the configured value, and the
aiohttp bridge does too. The bridge defaults retries to 0, so that
assertion fails against the pre-fix build rather than passing either way.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
@hemildesai

Copy link
Copy Markdown
Contributor Author

/ok to test 3698bcf

@hemildesai

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

SHIP — reliability-hardening across the OpenSandbox provider, mini_swe_agent_2 harness, and the global aiohttp session. No verifier/scoring logic touched; the risky bits are async lifecycle and pooling, and they check out.

Verified:

  • Ray await pattern (app.py:835): await future replacing asyncio.to_thread(ray.get, ...) matches CLAUDE.md's endorsed pattern and removes the ~32-worker default-executor ceiling on concurrent rollouts. Tests updated correctly via FakeObjectRef.__await__, covering both success and error paths.
  • Shared transport lifecycle (provider.py:617-648): lazy _get_transport() build is check-then-assign with no await in between, so no event-loop race; the provider owns the transport and aclose() is reached from sandbox/api.py:84,132, so no pool leak. Injection is skipped cleanly when keepalive_expiry_s is null.
  • httpx import (provider.py:625): guarded behind the sandbox extra and keepalive_expiry_s is not None; httpx is a transitive dep of opensandbox, so no undeclared import. httpx_aiohttp is optional with an ImportError→warning fallback and skip-guarded tests.
  • Global aiohttp timeout (server_utils.py:177): ClientTimeout(sock_connect=30) strictly adds a connect bound to what was previously unbounded (ClientTimeout()); the no-read/no-total choice is correct for long-silent /run requests and the rationale is documented.
  • resource_requests, transport backends, and shared-transport close are all covered by new unit tests; the opensandbox>=0.1.15 bump is reflected in uv.lock.

Notes (author's call, non-blocking):

  • Fire-and-forget cleanup thread (app.py:588-247): daemon-thread env.cleanup() is unbounded per rollout and dies abruptly on process exit. The comment correctly notes orphans are covered by the provider sandbox TTL, so this is an acceptable tradeoff — just flagging that teardown is now best-effort and failures only surface via a print, not logs/metrics.
  • command_retries: 0 and the keepalive/timeout tuning are well-commented; no action needed.

hemildesai and others added 3 commits July 31, 2026 15:02
The shared transport's pool also bounds how many sandbox operations one
process can have in flight, which is a hard ceiling for callers driving
hundreds of sandboxes from a single process. httpx treats
max_connections=None as unbounded, so accept null. Pairs with
max_keepalive_connections: 0 to disable reuse entirely.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
max_connections: null and max_keepalive_connections: 0 are supported
values; assert they produce a usable transport with an unbounded pool
and no reuse.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
The shipped config now documents max_connections: null as "no cap" and
max_keepalive_connections: 0 as "no reuse". Neither promise is enforced
by our code — both rest on httpx mapping None to an unbounded pool — so
assert them against the built pool.

To be clear about what this does and does not catch: it is a contract
test, not a regression test for the annotation change. The config
dataclass does no runtime type checking, so null was already accepted
before max_connections was widened to int | None. This guards the
documented behavior against a future validator or an httpx change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
@hemildesai

Copy link
Copy Markdown
Contributor Author

/ok to test 717de2a

e69b029 already covered max_connections: null and
max_keepalive_connections: 0; my 717de2a landed the same assertions a
few lines earlier in the same test, having been written against the
pre-e69b029c tree. Keep the original and remove the duplicate.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
@hemildesai

Copy link
Copy Markdown
Contributor Author

/ok to test f3cdeb4

@hemildesai
hemildesai marked this pull request as ready for review July 31, 2026 22:06
@hemildesai
hemildesai requested a review from a team as a code owner July 31, 2026 22:06
Keep this PR scoped to the sandbox provider and agent delivery path;
the shared-session timeout policy is a separate discussion.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
@bxyu-nvidia
bxyu-nvidia merged commit 3cf8756 into main Jul 31, 2026
14 of 16 checks passed
@bxyu-nvidia
bxyu-nvidia deleted the hemil/opensandbox-transport-hardening branch July 31, 2026 23:06
hemildesai added a commit that referenced this pull request Aug 1, 2026
Exposes SandboxSpec.provider_options as a harbor_environment_kwargs entry so
benchmarks can set per-sandbox scheduling requests (resource_requests) below
the resource limits, using the requests/limits split from #2212.

Signed-off-by: Hemil Desai <hemild@nvidia.com>
hemildesai added a commit that referenced this pull request Aug 2, 2026
…config

The dataclass default (false) now applies, making the keepalive-bounded
pooling from #2212 the effective default. The provider option itself is
unchanged. Also retitles the background_exec note, which had lost its
opening line and described the old fresh-connection default.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
OlegSudakov pushed a commit to OlegSudakov/Gym that referenced this pull request Aug 7, 2026
…esource requests/limits (NVIDIA-NeMo#2212)

## Summary

Hardens the OpenSandbox provider and the `mini_swe_agent_2` eval path so
that large sandboxed agent evals (SWE-bench Verified scale: hundreds of
concurrent rollouts against a Kubernetes-backed OpenSandbox deployment)
run end-to-end reliably: every finished rollout is delivered and
recorded, and sandbox-infrastructure noise is bounded instead of
silently zeroing rewards. Diagnosed on multi-node runs at concurrency
300–1500; the same failure signatures appear at concurrency 8, just less
often.

### 1. `Server disconnected without sending a response`

The SDK's default httpx pool keeps idle connections for 30s
(`opensandbox.config.connection.with_transport_if_missing`), but the
OpenSandbox server's uvicorn keep-alive reaper closes idle sockets after
~5s. Agent workloads idle between sandbox commands (model think time),
so commands routinely reuse a socket the server already closed —
surfacing as `httpx.RemoteProtocolError` → `SandboxInternalException`,
which permanently kills the rollout.

**Fix:** the provider injects a transport whose `keepalive_expiry` sits
*below* the server's keep-alive timeout (default 3s,
`connection.keepalive_expiry_s`):

- `connection.transport_backend: httpx` (default) — stock
`httpx.AsyncHTTPTransport(limits=..., retries=connect_retries)`. No new
required dependencies.
- `connection.transport_backend: aiohttp` (opt-in) — aiohttp pool under
the SDK's httpx surface via
[`httpx-aiohttp`](https://github.com/karpetrosyan/httpx-aiohttp); falls
back to the httpx transport with a warning when absent.
- `keepalive_expiry_s: null` disables injection entirely (SDK default
transport).

### 2. 502 `could not connect to the backend sandbox
endpoint='<podIP>:44772'` on first command

With `create.skip_health_check: true`, `Sandbox.create` returns before
the pod's exec daemon is listening; the first command races pod startup
and the server proxy 502s. Under a create burst this killed 7–12
rollouts per run.

**Fix:** `create.skip_health_check: false` (create waits for readiness,
bounded by the spec's `ready_timeout_s`; `create.timeout_s` kept above
the ready timeout).

**`command_retries` stays at 0.** Retrying a command the server may have
already started would execute it twice, and agent commands are
frequently mutating. The keepalive bound removes the stale-connection
failures retries were compensating for. Raise it only for idempotent
workloads.

### 3. Separate resource requests and limits

`Sandbox.create` accepts distinct `resource` (limits) and
`resource_requests` maps; a lone `resource` map is applied by the server
as requests=limits. This exposes the requests side via
`sandbox_spec.provider_options.resource_requests` (same keys as
`SandboxSpec.resources`). Motivation: SWE-bench test suites OOM-killed
sandbox pods at 2Gi, but raising a single combined map to 8Gi would 4×
the cluster reservation; with the split, limits rise while requests stay
small and pods pack densely. Requires `opensandbox>=0.1.15` (lower bound
raised here).

### 4. Sandbox teardown off the rollout's critical path

`env.cleanup()` ran in the rollout Ray task's `finally` block, so a
finished result only became fetchable after the sandbox DELETE returned
— and a teardown failure re-raised *over* the successful eval, degrading
it to a reward-0 error row. Teardown is also effectively one-shot
(`Sandbox.stop` latches `_closed` before dispatch), so a single failed
DELETE leaked the sandbox while still costing the rollout.

**Fix:** cleanup runs on a best-effort daemon thread; results return
immediately, teardown errors are logged rather than raised, and orphans
remain covered by the provider's sandbox TTL.

### 5. High-concurrency rollout delivery (`mini_swe_agent_2`)

Two coupled fixes so every finished rollout is delivered at high
concurrency (note: at very high concurrency the single-process
policy-model proxy remains a separate bottleneck — connection-refused
storms confirmed by a controlled A/B — with its scaling fix tracked
separately):

- **await the Ray ObjectRef** instead of `asyncio.to_thread(ray.get,
...)`: the default executor caps at `min(32, cpu+4)` threads, each
pinned for a full rollout, so delivery stalls at ~32 concurrent rollouts
and finished tasks queue behind blocked `ray.get` calls.
- **bounded litellm retries (`num_retries=5`, config-overridable)** — no
retry means one transient LLM-call failure kills a whole rollout;
unbounded retries make failures look like hangs — and
**`num_cpus=0.25`** on the rollout Ray task so concurrency is not capped
at cluster core count (rollouts are I/O-bound).

### 6. `datasets` declared in the base agent config

The struct-mode config merge rejects keys absent from the base config,
so a benchmark config using `_inherit_from` could not add its dataset
list (`ConfigKeyError: Key 'datasets' is not in struct`). Declared empty
in the base server config, matching how other agents (e.g. `swe_agents`)
expose it.

## Validation

- Requests/limits split (paired multi-node runs, identical except the
variable under test): infra-failed rollouts 41/316 (**13.0%**) at 2Gi
requests=limits → 19/288 (**6.6%**) at 8Gi limits / 2Gi requests; pass@1
58.0% → **67.1%**.
- Keepalive bound + health-checked create: zero `Server disconnected`
events and zero create-race 502s across all subsequent runs (previously
2–12 per run).
- Full stack, end-to-end from this branch: single-pass SWE-bench
Verified (500 instances, concurrency 500+) delivered **>98% of
rollouts** with ~1.6% residual sandbox-infra failures (server-side proxy
502s on established sandboxes, tracked separately) and zero
client-transport failures. Before the delivery fixes (NVIDIA-NeMo#4/NVIDIA-NeMo#5), an
identically shaped run lost the majority of *completed* evaluations at
the wall — hundreds of finished evals, only tens recorded.

## Relationship to NVIDIA-NeMo#2020

Complementary, no overlap; NVIDIA-NeMo#2020 (job attribution metadata) has since
merged and this branch is updated on top of it. Field-validated together
— the attribution labels are what make post-cancellation sandbox garbage
collection safely scoped to a single job.

## Testing

- `tests/unit_tests/test_opensandbox_provider.py`: transport-backend
coverage (httpx default with keepalive expiry, custom pool settings,
fallback when `httpx_aiohttp` is unavailable, `null` disables
injection), aiohttp opt-in (importorskip-guarded), and requests/limits
plumbing tests. 16 passed with and without `httpx-aiohttp`.
- `responses_api_agents/mini_swe_agent_2/tests/test_app.py`: updated for
the awaited ObjectRef (awaitable `FakeObjectRef`); assertions
strengthened to check the Ray call's params.
- CI green except a pre-existing `tau2` failure on `main` (reproduces on
unrelated PRs).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Signed-off-by: Hemil Desai <hemild@nvidia.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants