From ea4fa40becdd19aac99ce9e65ff87701de69e7f8 Mon Sep 17 00:00:00 2001 From: Hemil Desai Date: Tue, 4 Aug 2026 21:09:46 -0700 Subject: [PATCH 1/3] fix(sandbox): health-check on connect in the OpenSandbox provider `OpenSandboxProvider.connect()` hardcoded `skip_health_check=True`, so a handle rebuilt from a sandbox id was handed back without checking that the sandbox was reachable. A sandbox id only proves the workload exists. The server reports a sandbox ready once its pod is Running with an IP, which happens before execd binds its port, so an unchecked handle defers that startup gap to the first real call -- where it surfaces as `502 Could not connect to the backend sandbox endpoint=...:44772` instead of a short wait. Measured on a production cell during a 1,452-sandbox burst: 31% of sandboxes saw at least one 502 on the execd port, clearing after p50 11s / p90 28s / p99 55s. Clients whose readiness gate ran absorbed this invisibly on the ping endpoint; clients reaching a sandbox through `connect()` had no gate at all and took the failure on real commands. `connect()` now honours the existing `skip_health_check` setting, which defaults to False, so the SDK polls until the sandbox answers. Callers that deliberately want an unchecked handle can still opt out, and the setting now behaves consistently between `create()` and `connect()`. Note that `connect()` bounds the wait with `connect_attempt_timeout_s` (default 30s); deployments seeing p99-scale startup gaps may want to raise it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Hemil Desai --- .../sandbox/providers/opensandbox/provider.py | 13 +++++++-- tests/unit_tests/test_opensandbox_provider.py | 27 +++++++++++++++++++ tests/unit_tests/test_sandbox.py | 4 ++- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/nemo_gym/sandbox/providers/opensandbox/provider.py b/nemo_gym/sandbox/providers/opensandbox/provider.py index 9cefd4d5dd..8e72dd5ee3 100644 --- a/nemo_gym/sandbox/providers/opensandbox/provider.py +++ b/nemo_gym/sandbox/providers/opensandbox/provider.py @@ -660,7 +660,16 @@ async def serialize_handle(self, handle: SandboxHandle, *, scope: str | None = N return {"sandbox_id": handle.sandbox_id} async def connect(self, descriptor: Mapping[str, Any]) -> SandboxHandle: - """Rebuild a live handle from an OpenSandbox sandbox id via the SDK.""" + """Rebuild a live handle from an OpenSandbox sandbox id via the SDK. + + The health check is on by default here. A sandbox id only tells us the + workload exists, not that execd is listening yet: the server reports a + sandbox ready once its pod is Running with an IP, which happens before + execd binds its port. Returning an unchecked handle pushes that gap onto + the first real call, which then fails with a 502 instead of waiting. + Honour ``skip_health_check`` so callers that deliberately want an + unchecked handle can still opt out. + """ Sandbox, _, _, _, _ = _require_opensandbox_sdk() sandbox_id = str(descriptor["sandbox_id"]) timeout_s = self._create.connect_attempt_timeout_s @@ -669,7 +678,7 @@ async def connect(self, descriptor: Mapping[str, Any]) -> SandboxHandle: sandbox_id, connection_config=self._connection_config(request_timeout_s=timeout_s), connect_timeout=timedelta(seconds=timeout_s), - skip_health_check=True, + skip_health_check=self._create.skip_health_check, ), timeout=timeout_s, ) diff --git a/tests/unit_tests/test_opensandbox_provider.py b/tests/unit_tests/test_opensandbox_provider.py index 2f977f1c19..a95916f25b 100644 --- a/tests/unit_tests/test_opensandbox_provider.py +++ b/tests/unit_tests/test_opensandbox_provider.py @@ -1000,3 +1000,30 @@ async def test_create_attribution_run_id_generated( def test_attribution_invalid_key_prefix_raises(key_prefix: str) -> None: with pytest.raises(ValueError, match="key_prefix"): opensandbox_provider.OpenSandboxAttributionConfig(key_prefix=key_prefix) + + +async def test_connect_health_checks_by_default(fake_opensandbox_sdk: None) -> None: + """connect() must hand back a handle that is actually usable. + + A sandbox id only proves the workload exists; the server reports a sandbox + ready once its pod is Running with an IP, which is before execd binds its + port. Skipping the check here defers that gap to the first real call, which + surfaces as a 502 rather than a wait. + """ + provider = opensandbox_provider.OpenSandboxProvider(probe={"command": None}) + + await provider.connect({"sandbox_id": "sandbox-9"}) + + assert FakeSandbox.connected_kwargs["skip_health_check"] is False + + +async def test_connect_honours_skip_health_check_opt_out(fake_opensandbox_sdk: None) -> None: + """Callers that explicitly opt out still get an unchecked handle.""" + provider = opensandbox_provider.OpenSandboxProvider( + create={"skip_health_check": True}, + probe={"command": None}, + ) + + await provider.connect({"sandbox_id": "sandbox-9"}) + + assert FakeSandbox.connected_kwargs["skip_health_check"] is True diff --git a/tests/unit_tests/test_sandbox.py b/tests/unit_tests/test_sandbox.py index 26ab22382c..c108cf6c6d 100644 --- a/tests/unit_tests/test_sandbox.py +++ b/tests/unit_tests/test_sandbox.py @@ -1329,5 +1329,7 @@ async def connect(cls, sandbox_id: str, **kwargs: Any) -> "FakeSDKSandbox": assert isinstance(handle.raw, FakeSDKSandbox) connect_call = FakeSDKSandbox.connect_calls[0] assert connect_call["sandbox_id"] == "sdk-sandbox-9" - assert connect_call["skip_health_check"] is True + # connect() health-checks by default so the handle it returns is usable; + # otherwise the first call pays the execd startup gap as a 502. + assert connect_call["skip_health_check"] is False assert connect_call["connection_config"].kwargs["domain"] == "sandbox.example" From 3e6730ea629f5548ee6f6fd0feda7312113e6949 Mon Sep 17 00:00:00 2001 From: Hemil Desai Date: Tue, 4 Aug 2026 21:15:40 -0700 Subject: [PATCH 2/3] fix(sandbox): default skip_health_check to False everywhere The reconnect inside `_connect_after_create` still hardcoded `skip_health_check=True`, and two shipped configs turned the check off, so the health check was effectively opt-in rather than opt-out. All call sites now derive the flag from configuration, whose default is False, and the configs that disabled it are flipped back on. Skipping the check lets the first command race a pod whose exec daemon is not listening yet, which returns a 502. Both configs also raise `create.timeout_s` above their `ready_timeout_s`, matching the guidance already documented in the provider's own reference config: that timeout bounds the whole create call, which now includes the readiness wait, so leaving the two equal would turn the wait into a timeout. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Hemil Desai --- nemo_gym/sandbox/providers/opensandbox/provider.py | 2 +- resources_servers/litmus_agent/configs/litmus_agent.yaml | 8 ++++++-- responses_api_agents/mini_swe_agent_2/README.md | 8 ++++++-- tests/unit_tests/test_sandbox.py | 3 ++- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/nemo_gym/sandbox/providers/opensandbox/provider.py b/nemo_gym/sandbox/providers/opensandbox/provider.py index 8e72dd5ee3..24deac960c 100644 --- a/nemo_gym/sandbox/providers/opensandbox/provider.py +++ b/nemo_gym/sandbox/providers/opensandbox/provider.py @@ -847,7 +847,7 @@ async def _connect_after_create(self, handle: SandboxHandle, spec: SandboxSpec) handle.sandbox_id, connection_config=self._connection_config(), connect_timeout=timedelta(seconds=attempt_timeout_s), - skip_health_check=True, + skip_health_check=self._create.skip_health_check, ), timeout=attempt_timeout_s, ) diff --git a/resources_servers/litmus_agent/configs/litmus_agent.yaml b/resources_servers/litmus_agent/configs/litmus_agent.yaml index a525558e9e..acdd127785 100644 --- a/resources_servers/litmus_agent/configs/litmus_agent.yaml +++ b/resources_servers/litmus_agent/configs/litmus_agent.yaml @@ -30,8 +30,12 @@ litmus_agent: use_server_proxy: true create: request_timeout_s: 1200 - timeout_s: 1200 - skip_health_check: true + # Must exceed the spec's ready_timeout_s below: this bounds the + # whole create call, which includes the readiness wait. + timeout_s: 1500 + # Skipping the check lets the first command race a pod whose exec + # daemon is not listening yet, which returns a 502. + skip_health_check: false retries: 10 retry_delay_s: 5.0 retry_max_delay_s: 90.0 diff --git a/responses_api_agents/mini_swe_agent_2/README.md b/responses_api_agents/mini_swe_agent_2/README.md index c4d57a401f..1ff167c76c 100644 --- a/responses_api_agents/mini_swe_agent_2/README.md +++ b/responses_api_agents/mini_swe_agent_2/README.md @@ -218,8 +218,12 @@ sandbox: # name referenced by the agent's sandbox_provider use_server_proxy: true create: request_timeout_s: 1200 - timeout_s: 1200 - skip_health_check: true + # Must exceed the spec's ready_timeout_s above: this bounds the whole + # create call, which includes the readiness wait. + timeout_s: 1500 + # Skipping the check lets the first command race a pod whose exec + # daemon is not listening yet, which returns a 502. + skip_health_check: false retries: 10 retry_delay_s: 5.0 retry_max_delay_s: 90.0 diff --git a/tests/unit_tests/test_sandbox.py b/tests/unit_tests/test_sandbox.py index c108cf6c6d..3ff962d8c3 100644 --- a/tests/unit_tests/test_sandbox.py +++ b/tests/unit_tests/test_sandbox.py @@ -836,7 +836,8 @@ async def connect(cls, sandbox_id: str, **kwargs: Any) -> "FakeSDKSandbox": assert handle.sandbox_id == "sdk-sandbox-1" assert isinstance(handle.raw, FakeSDKSandbox) connect_call = FakeSDKSandbox.connect_calls[0] - assert connect_call["skip_health_check"] is True + # This provider does not opt out, so the reconnect health-checks too. + assert connect_call["skip_health_check"] is False connection_kwargs = dict(connect_call["connection_config"].kwargs) # Transport identity is asserted in test_opensandbox_provider.py. connection_kwargs.pop("transport", None) From 6e6a63f121e52ba54525b5573defb631a794acee Mon Sep 17 00:00:00 2001 From: Hemil Desai Date: Tue, 4 Aug 2026 21:17:21 -0700 Subject: [PATCH 3/3] refactor(sandbox): trim comments on the health-check change Comments state the why only; the rationale is not repeated at each site. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Hemil Desai --- nemo_gym/sandbox/providers/opensandbox/provider.py | 10 +++------- .../litmus_agent/configs/litmus_agent.yaml | 6 ++---- responses_api_agents/mini_swe_agent_2/README.md | 6 ++---- tests/unit_tests/test_opensandbox_provider.py | 8 +------- tests/unit_tests/test_sandbox.py | 3 +-- 5 files changed, 9 insertions(+), 24 deletions(-) diff --git a/nemo_gym/sandbox/providers/opensandbox/provider.py b/nemo_gym/sandbox/providers/opensandbox/provider.py index 24deac960c..a0f89c9c6f 100644 --- a/nemo_gym/sandbox/providers/opensandbox/provider.py +++ b/nemo_gym/sandbox/providers/opensandbox/provider.py @@ -662,13 +662,9 @@ async def serialize_handle(self, handle: SandboxHandle, *, scope: str | None = N async def connect(self, descriptor: Mapping[str, Any]) -> SandboxHandle: """Rebuild a live handle from an OpenSandbox sandbox id via the SDK. - The health check is on by default here. A sandbox id only tells us the - workload exists, not that execd is listening yet: the server reports a - sandbox ready once its pod is Running with an IP, which happens before - execd binds its port. Returning an unchecked handle pushes that gap onto - the first real call, which then fails with a 502 instead of waiting. - Honour ``skip_health_check`` so callers that deliberately want an - unchecked handle can still opt out. + Health-checks unless the caller opts out: a sandbox id only proves the + workload exists, not that its exec daemon is listening yet, so an + unchecked handle turns that gap into a 502 on the first call. """ Sandbox, _, _, _, _ = _require_opensandbox_sdk() sandbox_id = str(descriptor["sandbox_id"]) diff --git a/resources_servers/litmus_agent/configs/litmus_agent.yaml b/resources_servers/litmus_agent/configs/litmus_agent.yaml index acdd127785..36e503df2e 100644 --- a/resources_servers/litmus_agent/configs/litmus_agent.yaml +++ b/resources_servers/litmus_agent/configs/litmus_agent.yaml @@ -30,11 +30,9 @@ litmus_agent: use_server_proxy: true create: request_timeout_s: 1200 - # Must exceed the spec's ready_timeout_s below: this bounds the - # whole create call, which includes the readiness wait. + # Must exceed ready_timeout_s below: this bounds the whole create + # call, which includes the readiness wait. timeout_s: 1500 - # Skipping the check lets the first command race a pod whose exec - # daemon is not listening yet, which returns a 502. skip_health_check: false retries: 10 retry_delay_s: 5.0 diff --git a/responses_api_agents/mini_swe_agent_2/README.md b/responses_api_agents/mini_swe_agent_2/README.md index 1ff167c76c..59826d9b6c 100644 --- a/responses_api_agents/mini_swe_agent_2/README.md +++ b/responses_api_agents/mini_swe_agent_2/README.md @@ -218,11 +218,9 @@ sandbox: # name referenced by the agent's sandbox_provider use_server_proxy: true create: request_timeout_s: 1200 - # Must exceed the spec's ready_timeout_s above: this bounds the whole - # create call, which includes the readiness wait. + # Must exceed ready_timeout_s above: this bounds the whole create call, + # which includes the readiness wait. timeout_s: 1500 - # Skipping the check lets the first command race a pod whose exec - # daemon is not listening yet, which returns a 502. skip_health_check: false retries: 10 retry_delay_s: 5.0 diff --git a/tests/unit_tests/test_opensandbox_provider.py b/tests/unit_tests/test_opensandbox_provider.py index a95916f25b..0bc778c302 100644 --- a/tests/unit_tests/test_opensandbox_provider.py +++ b/tests/unit_tests/test_opensandbox_provider.py @@ -1003,13 +1003,7 @@ def test_attribution_invalid_key_prefix_raises(key_prefix: str) -> None: async def test_connect_health_checks_by_default(fake_opensandbox_sdk: None) -> None: - """connect() must hand back a handle that is actually usable. - - A sandbox id only proves the workload exists; the server reports a sandbox - ready once its pod is Running with an IP, which is before execd binds its - port. Skipping the check here defers that gap to the first real call, which - surfaces as a 502 rather than a wait. - """ + """An unchecked handle would defer the exec-daemon startup gap to the first call.""" provider = opensandbox_provider.OpenSandboxProvider(probe={"command": None}) await provider.connect({"sandbox_id": "sandbox-9"}) diff --git a/tests/unit_tests/test_sandbox.py b/tests/unit_tests/test_sandbox.py index 3ff962d8c3..02a63ba607 100644 --- a/tests/unit_tests/test_sandbox.py +++ b/tests/unit_tests/test_sandbox.py @@ -1330,7 +1330,6 @@ async def connect(cls, sandbox_id: str, **kwargs: Any) -> "FakeSDKSandbox": assert isinstance(handle.raw, FakeSDKSandbox) connect_call = FakeSDKSandbox.connect_calls[0] assert connect_call["sandbox_id"] == "sdk-sandbox-9" - # connect() health-checks by default so the handle it returns is usable; - # otherwise the first call pays the execd startup gap as a 502. + # connect() health-checks by default so the handle it returns is usable. assert connect_call["skip_health_check"] is False assert connect_call["connection_config"].kwargs["domain"] == "sandbox.example"