Skip to content

fix(opensandbox): delete the sandbox of a create the caller gave up on - #4799

Open
gwarmstrong wants to merge 14 commits into
NVIDIA-NeMo:mainfrom
gwarmstrong:georgea/opensandbox-failure-handling
Open

gwarmstrong wants to merge 14 commits into
NVIDIA-NeMo:mainfrom
gwarmstrong:georgea/opensandbox-failure-handling

Conversation

@gwarmstrong

@gwarmstrong gwarmstrong commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

A create request that times out on the client, or that the caller cancels, can still make its sandbox on the server, and the provider never learns its id. #3807 (now on main) handles this with an attempt-key sweep: it lists the sandboxes carrying the attempt's metadata key and deletes them. The sweep misses a sandbox the server records after the lookup, and finds nothing when the lookup itself fails.

This PR adds @hemildesai's approach under the sweep, with his commit cherry-picked. The create request is not cancelled when the caller gives up. The provider waits on it through asyncio.shield, so the caller still gets its timeout or cancellation on time. Then:

  • If the sweep found and deleted the attempt's sandbox, the request is cancelled. The server never answers a create whose Pending sandbox was deleted, so waiting on it would hold the request open until the SDK's request timeout.
  • If the sweep found nothing, a tracked task waits for the server's answer and deletes the sandbox it names.

aclose() waits for those tasks up to operations.close_timeout_s, then cancels the rest. The OpenSandbox docs describe both steps.

No issue: found in Terminal-Bench-Science runs, where a 70-task oracle run on OpenSandbox left two sandboxes running until their TTL.

Validation

  • tests/unit_tests/test_opensandbox_provider.py: 160 passed after merging main (2026-10-29, 7269de9); 159 before. That includes Hemil's tests and a new one:

    • Hemil's: a create that finishes after the caller gave up (now with a sweep that finds nothing), and aclose() cancelling creates the server never answers.
    • New: a request cancelled once the sweep deleted its sandbox. It fails without the change.

    The other OpenSandbox unit test files pass too, except two --help script tests that need aiohttp on the host Python.

  • Live on OpenSandbox at this head: a create timing out after 1 s, a caller cancelling after 1 s, and three attempts that all time out. Each time the sweep found the Pending sandbox and deleted it, aclose() returned at once, and no sandbox was left. When the request was left open instead, the same run held aclose() for the full 120 s request timeout. The found-nothing path doesn't occur on our cell, so only the unit test covers it.

  • Re-checked live after merging main (7269de9, 2026-10-29), with the same three cases: a create timing out after 1 s, a caller cancelling after 1 s, and three attempts that all time out. Each Pending sandbox was swept and deleted (five in all), aclose() returned in under 0.03 s, and no sandbox was left.

Rollouts

N/A (provider error handling).

Compatibility and benchmark impact

No settings change. A timed-out or cancelled create whose sandbox the sweep did not find keeps its request open in the background until the server answers, then deletes the sandbox. aclose() waits up to operations.close_timeout_s for those requests.

…ed its own output

execd keeps background-command output under /tmp/opensandbox-execd. A command
that empties /tmp (terminal-bench-science's rdkit-ic-constraints verifier resets
/tmp, /var/tmp and /dev/shm between worker calls) deletes that file, so the
post-completion log read failed with "error open combined output file ... no
such file or directory" and the trial was reported as an infrastructure error.

The status read has already returned the exit code, so keep it and report no
output for that one error; any other log-read failure still raises.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
A create request that times out (or is cancelled) client-side can still make
its sandbox on the server. The SDK can only clean up a sandbox whose id it
received, so the sandbox ran untracked until its TTL while the retry made
another one. A 70-task Terminal-Bench-Science oracle run left two such
verifier sandboxes behind.

Every attempt of one create() call now carries the same create-id metadata
label (under the attribution key prefix). After a failed attempt, and again
after a retry succeeds (an earlier request can land late), the provider lists
that label and deletes every sandbox it did not return. The cleanup is best
effort and only logs its own failures.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
A saturated sandbox can answer some background-command status reads and time
out on others. Each poll then recovers within its retries, so the command
looks alive until its own timeout, which is eight hours for a
Terminal-Bench-Science agent.

operations.status_failure_window_s (off by default) ends such a command: once
status_failure_limit (5) reads fail within the window, the poll raises
SandboxUnresponsiveError, a SandboxBackendUnreachableError, so callers treat
the run as a retryable infrastructure failure and an OOM-killed sandbox is
still named as such. A non-retryable read error such as SANDBOX::ENDED still
ends the poll at once.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 20, 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.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added area:sandbox Sandbox providers, isolation, and sandbox runtime contracts bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer labels Oct 21, 2026
@hemildesai

Copy link
Copy Markdown
Contributor

Thanks for tracking this leak down. The diagnosis is right, but I'd like to propose a different fix that needs no create-id label, no list-and-kill sweep, and also covers the case where every attempt fails.

Where the leak actually is

In the SDK (0.1.15), Sandbox.create sets sandbox_id = response.id as soon as the create request returns. From then on, its own except BaseException handler kills the sandbox on any failure, including cancellation. So a sandbox can only leak while the create request is still waiting for its response: our asyncio.wait_for (or a caller's cancel) interrupts it, and the reply carrying the id is thrown away.

Proposal: never cancel the create request

Run Sandbox.create as its own task and wait on it through asyncio.shield. If our timeout fires or the caller cancels, the caller still gets its error on time, but the request keeps running. A tracked background task waits for the response and deletes the sandbox if one comes back:

create = asyncio.ensure_future(Sandbox.create(**kwargs))
try:
    sandbox = await asyncio.wait_for(asyncio.shield(create), timeout=timeout_s)
except BaseException:
    self._reap_abandoned_create(create)  # awaits `create`; on success -> self._cleanup_failed_create_handle(handle)
    raise

_reap_abandoned_create keeps its tasks in a set, and aclose() waits for them before closing the transport. The whole change is about 25 lines in provider.py, and the existing provider unit tests pass unchanged.

Compared with the label approach:

  • Exact: it deletes the one sandbox that was abandoned, as soon as the server answers. There are no list calls and no metadata filter.
  • Covers every failed attempt: in this PR, the sweep after the retry loop only runs when a later attempt succeeds. If every attempt times out, each attempt's sandbox can appear after that attempt's own sweep listed nothing, and all of them leak.
  • Adds no load during overload: the sweep here runs after every failed attempt, including 429s and other clear rejections. Each sweep adds a list request and a new SDK client, which is the most traffic at the worst time.
  • No new label on every sandbox, and no extra cleanup holding up a cancelled task.

One requirement: the create request timeout (create.request_timeout_s) must be longer than the server's own create timeout, so the response always arrives. If the connection drops before then, the id is lost, as it is today. Process crashes stay covered by the run attribution label and cleanup_sandboxes.py.

E2E evidence

I tested this against a live OpenSandbox cell (python:3.12-slim, use_server_proxy: true). Create took about 4–8s there, so I set the create timeout to 1s to force timeouts. Each scenario used its own attribution run label; after the create failed, the script waited 90s and listed sandboxes with that label. I ran the same script against unmodified main and against the shield change:

Scenario main shield
Normal create, then close no leak no leak
Create times out at 1s, no retry 1 still running no leak
Caller cancels the create after 1s 1 still running no leak
All 3 retry attempts time out 3 still running no leak

With the shield, the caller still got OpenSandboxCreateTimeoutError at 1.0s (3.3s across three attempts), or the cancellation at 1.0s. The provider logged Deleting OpenSandbox sandbox … made by an abandoned create for all 5 abandoned creates, about 2–4s after the caller gave up.

Not covered: many creates at once, and connections that drop before the response arrives.

Suggested path

  • Replace the create-id label and _delete_create_leftovers with the shield approach. Add a unit test where the create finishes after the caller has given up, and one where the caller cancels it.
  • Consider splitting status_failure_window_s (a new opt-in feature) and the deleted-output fix into their own PRs, and link the tracking issue (or say why there isn't one).

Happy to push the prototype branch if it helps.

@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Oct 21, 2026
gwarmstrong and others added 6 commits October 21, 2026 15:25
…ve sandbox"

This reverts commit 12f14bd.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
…ttempts"

This reverts commit c61327c.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
…it deleted its own output"

This reverts commit 2b8b38b.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
…-failure-handling

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
A create request that times out client-side or is cancelled can still make
its sandbox on the server. Once the create request has returned the id, the SDK
kills the sandbox on any failure, so the only leak is a request
interrupted before the server answers: its reply, carrying the id, is
dropped and the sandbox runs untracked until its TTL.

Never cancel the request instead. _create_once waits on it through
asyncio.shield; when the timeout fires or the caller cancels, the caller
gets its error on time and a tracked task waits for the server's answer
and deletes the sandbox. aclose() waits for those tasks for
operations.close_timeout_s, then cancels the rest.

Alternative to the create-id label and list-and-kill sweep in NVIDIA-NeMo#4799.
Validated against a live OpenSandbox cell: main leaked a sandbox for every
timed-out, cancelled, or all-retries-failed create; with this change none
leaked. 500 concurrent creates add about 0.07s per create on the client.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
(cherry picked from commit e68e1dd)
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
An abandoned create request is left to finish and its sandbox deleted, which
only works if the request outlives the server's create, so say to keep
create.request_timeout_s above it.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
@gwarmstrong gwarmstrong changed the title fix(opensandbox): delete sandboxes left by timed-out create attempts fix(opensandbox): delete the sandbox of a create the caller gave up on Oct 21, 2026
@gwarmstrong

Copy link
Copy Markdown
Contributor Author

Thanks @hemildesai, this is a much better fix. I've switched the PR to your approach:

  • Your commit from shield-create-prototype is cherry-picked unchanged (fcf69f3), on top of reverts of the create-id sweep and of the two unrelated changes. The deleted-output fix is now fix(opensandbox): keep a finished command's exit status when it deleted its own output #4871, and the status-failure window is out of this PR. I didn't force-push, so the history shows the reverts; the net diff is your change plus one docs paragraph.
  • The docs paragraph (13b2b36) says to keep create.request_timeout_s above the server's create time, the requirement you pointed out.
  • I ran your three scenarios live on our cell with this head: the caller got its error after 1.1 s, 1.0 s and 3.0 s, all five abandoned sandboxes were deleted 2 to 4 s later, and none was left.

The description is updated to match.

@gwarmstrong

Copy link
Copy Markdown
Contributor Author

/ok to test 13b2b36

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

NVIDIA-NeMo#3807 added an attempt-key sweep to _create_once: a create that times out, is cancelled or hits the
SDK's request timeout lists the sandboxes carrying its key and deletes them. The shielded request
from this branch sits under it, so both run: the sweep deletes what the server has recorded, and
the abandoned request deletes the sandbox the server answers with later. Both sides added methods
after _create_once; both are kept.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
…equest together

The paragraph asked to keep create.request_timeout_s above the server's create time, which main now
caps at ready_timeout_s on purpose; a request the SDK cuts off is found by its attempt key instead.
The test of an abandoned create now has the sweep find nothing, the case the request is kept for.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
…ts sandbox

The server never answers a create whose Pending sandbox the attempt-key sweep deleted, so the
request it was left to finish stayed open until the SDK's request timeout, and aclose() waited up to
operations.close_timeout_s for it. Live, every abandoned create held aclose() for the full 120 s
request timeout. The request is now cancelled when the sweep found its sandbox, and kept only when the
sweep found none: the server had not recorded the sandbox yet, or the lookup failed.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
@gwarmstrong

Copy link
Copy Markdown
Contributor Author

@hemildesai since your review, main picked up #3807, which added an attempt-key sweep to _create_once. It lists the sandboxes carrying the attempt's key, deletes them, and reports one still Pending. I merged main and kept both: the sweep runs first, and your shielded request covers what it misses, which is a sandbox the server records after the lookup, or a failed lookup.

One interaction showed up live. The server never answers a create whose Pending sandbox the sweep deleted, so the open request held aclose() for the full 120 s request timeout. 7ba7057 cancels the request when the sweep found its sandbox and keeps it only when the sweep found none. With that, your three scenarios return at once and leave nothing behind.

The docs paragraph no longer asks for create.request_timeout_s above the create time, because main now caps it at ready_timeout_s on purpose.

@gwarmstrong

Copy link
Copy Markdown
Contributor Author

/ok to test 7ba7057

…-failure-handling

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
@gwarmstrong

Copy link
Copy Markdown
Contributor Author

/ok to test 7269de9

Main now raises typed failures (provider_create_timeout,
provider_create_pending) in place of OpenSandboxCreateTimeoutError and
OpenSandboxCreatePendingError, and moved the provider tests to
tests/unit_tests/sandbox/. The abandoned-create tests now assert the
failure kinds, and _raise_timed_out_create keeps main's docstring plus
the note on its request argument.

Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
@gwarmstrong

Copy link
Copy Markdown
Contributor Author

/ok to test 62d8e44

This branch was previously deployed

1 inactive deployment
public — 62d8e44d Deployed Nov 7, 2026 by copy-pr-bot[bot] via release / finalize / notify #10830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:sandbox Sandbox providers, isolation, and sandbox runtime contracts bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer sla:review-overdue Review response is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants