Skip to content

fix(sandbox): fail fast when attaching to a missing OpenSandbox PTY session - #3641

Merged
hemildesai merged 1 commit into
mainfrom
hemild/fix-pty-attach-missing-session
Sep 23, 2026
Merged

hemildesai merged 1 commit into
mainfrom
hemild/fix-pty-attach-missing-session

Conversation

@hemildesai

Copy link
Copy Markdown
Contributor

What changed and why

Follow-up to #2636 (sandbox pause/resume). On cell-3, re-attaching to a PTY session opened before a pause took about 95s to fail, and the error was misleading:

SandboxPtyError: PTY session already has an attached client

On the Kubernetes backend, pause commits the rootfs and replaces the runtime, so the old session no longer exists. But execd answers a WebSocket attach to a missing session with the same 1008 close it uses for a session held by another client. I checked this live: a random session id, a random id with takeover=1, and a genuinely held session all close 1008 with an empty reason within 0.3s. So Gym treated it as a stale holder and waited out every retry: the session's own takeover retries (2+5+10s), the provider's retries, and a final sandbox-death check.

execd's HTTP API does tell the two apart, which the same live probe showed:

Request Answer
GET /pty/<missing> 404 {"code":"CONTEXT_NOT_FOUND","message":"pty session … not found"}
GET /pty/<live> 200 {"session_id":…,"running":true,…}

OpenSandboxProvider.attach_pty() now makes that GET before dialing. Only execd's own 404, with the CONTEXT_NOT_FOUND code, counts as missing. A proxy 404 while a route is still registering, or a failed check, lets the attach proceed exactly as before. A missing session now fails immediately with PTY session <id> not found, with or without takeover. Before, without takeover, a missing session was also misreported as "already has an attached client".

Cost: one extra small GET per attach. Attaches are reconnect operations, not per-command.

No issue filed. This was found while validating #2636 end to end.

Validation

Unit tests (test_opensandbox_pty.py, test_opensandbox_provider.py, test_sandbox.py): 246 passed. New cases cover:

  • A missing session fails with "not found", makes no WebSocket dial, and never consults sandbox status. Covered for both takeover values.
  • A proxy 404 without execd's code, and a failed check, both keep the existing takeover retries.

Two existing tests now count the existence-check client. pre-commit run on the changed files passes.

Live on cell-3 (tenant namespace, Kubernetes backend), running the pause/resume e2e across two OS processes:

Step Result
pause, then reconnect from a new process paused
resume running
file written before pause preserved
non-PTY exec, foreground path rc 3; stdout and stderr correct
non-PTY exec, background path (what opensandbox.yaml enables) rc 3; combined stream in stdout, as documented
attach to the pre-pause PTY session fails in 0.3s with not found (was about 95s with "already has an attached client")
fresh PTY after resume works

A separate probe confirmed the normal case is unaffected. Detaching from a live session and re-attaching by id succeeds with and without takeover, in about 0.6s, with shell state intact. All sandboxes were deleted afterwards.

Rollout evidence: N/A. This is a sandbox provider change with no environment, agent, or benchmark behavior change.

Noticed, not changed here

The foreground exec path appends the SDK's error to stderr, so a command that exits non-zero gets a synthetic CommandExecError: <code> line after its own stderr. This behavior dates from the original sandbox API, and it's out of scope here.

🤖 Generated with Claude Code

@copy-pr-bot

copy-pr-bot Bot commented Sep 23, 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
hemildesai marked this pull request as ready for review September 23, 2026 00:41
@yaoyu-33 yaoyu-33 added area:sandbox Sandbox providers, isolation, and sandbox runtime contracts bug Something isn't working complexity:low Localized change in one scope with a small, straightforward review surface ready-to-merge PR is approved, current, and waiting for final CI or merge labels Sep 23, 2026
@hemildesai

Copy link
Copy Markdown
Contributor Author

/ok to test d2cb1a7

@hemildesai

Copy link
Copy Markdown
Contributor Author

The three red checks are unrelated to this change. Server suite (shard 3) fails only on responses_api_agents/osworld_agent, whose environment cannot resolve:

osworld==0.1.0 depends on numpy<2 … and you require osworld and numpy>=2.1,<2.5 … requirements are unsatisfiable

That conflict came in with #3592, whose server-suite shards were skipped. Nemo_CICD_Test and Server suite are the aggregates of that shard. The fix is #3649 by the OSWorld owner. Every other check passed, including Unit tests / Test, which runs the sandbox and PTY tests this PR touches. I'll re-run CI here once #3649 lands.

🤖 Generated with Claude Code

…ession

execd refuses a WebSocket attach to a session that does not exist with
the same 1008 close as one held by another client. After a pause on the
Kubernetes backend replaces the runtime, re-attaching to a pre-pause
session was therefore treated as a stale holder: the session's own
takeover retries (2+5+10s), the provider's retries, and a closing
sandbox-death check took about 95s on cell-3 before reporting a
misleading "already has an attached client".

attach_pty() now asks execd first (GET /pty/{id}). Only execd's own 404
(CONTEXT_NOT_FOUND) counts as missing: a proxy 404 while a route is still
registering, or a failed check, leaves the attach to proceed as before.
A missing session now fails in about 0.3s with "PTY session <id> not
found", with or without takeover.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Hemil Desai <hemild@nvidia.com>
@hemildesai
hemildesai force-pushed the hemild/fix-pty-attach-missing-session branch from d2cb1a7 to 5af14a4 Compare September 23, 2026 15:07
@hemildesai

Copy link
Copy Markdown
Contributor Author

/ok to test 5af14a4

@github-actions

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/35881522695

Merged via the queue into main with commit d6404d8 Sep 23, 2026
41 checks passed
@hemildesai
hemildesai deleted the hemild/fix-pty-attach-missing-session branch September 23, 2026 15:37

This branch was previously deployed

1 inactive deployment
public — 5af14a4a Deployed Sep 23, 2026 by copy-pr-bot[bot] via release / finalize / notify #3571
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 complexity:low Localized change in one scope with a small, straightforward review surface ready-to-merge PR is approved, current, and waiting for final CI or merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants