Skip to content

fix(ns_tools): release python_tool sessions in close_resources_session - #5479

Open
yaoyu-33 wants to merge 21 commits into
yuya/shared-resources-lifecyclefrom
yuya/gymint-180-ns-tools-close-sessions
Open

yaoyu-33 wants to merge 21 commits into
yuya/shared-resources-lifecyclefrom
yuya/gymint-180-ns-tools-close-sessions

Conversation

@yaoyu-33

@yaoyu-33 yaoyu-33 commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Release the lazily created Skills Python session through the shared resources lifecycle, including runs that skip verification. The server now inherits seed, close and checkpoint retirement from #5709 and performs scoring inside its public verification context.

Depends on #5709 (which depends on #5708). The child diff contains only this environment and its related documentation/configuration; shared implementation is reviewed once in the dependency. No separate issue is needed for this focused migration of an existing resources server.

  • Keep cancellation-aware publication of the Skills-generated session ID and wait for the bounded in-flight request before close can overtake allocation. Release retries transient transport/5xx failures; a failed delete keeps ownership for a later retry.
  • Retain compact timing evidence for score retries, not the live Python resource. The shared lock spans delegated scoring and cleanup so a concurrent cookie reseed cannot replace the run underneath verification. Cookie seeds retain their ID and release the preceding run first.

Retains its timing metrics (retain(metrics)) before delegating, so a retry re-judges with the original totals; the delegated verifier's judge-failure mask is carried by #5677, not this PR.

Validation on b7d76b55d6fec6f5e31cf8e7828bf08406e71624, Python 3.13.14, with this checkout first on PYTHONPATH and NEMO_GYM_EXTRA_ROOTS pointing to it:

Real-model smoke on preceding migration revision af11d42c5c6e7ff3b8aa386876ad3fa95b4f9f3a (shared core 4e92d4891) used nvidia/nemotron-3.5-lightning-30b-a3b, Python 3.13.14 and a real Skills IPython backend. Both typed normal and skip-verification cases executed the arithmetic tool and observed 385. Normal verification scored 1.0; the skip case made zero verify calls and returned its configured fixed reward 0.0. Close returned 200; the skip case reduced live backend sessions 1 → 0, and timing/session/identity maps were empty. Loaded source hashes matched that revision. The shared runner separately recorded the Litmus storage-cleanup failure; both NS case receipts passed. This synthetic task validates lifecycle behavior, not benchmark accuracy or the optional LLM judge. Retry and cleanup-failure paths are covered by unit tests only at this head.

These targeted checks do not replace the repository CI matrix or approval of the current head.

Compatibility

A tool call after /verify (within session_close_retry_window_seconds) or after close now gets 404 failure_kind: session_lost; main silently created a new Python session. Concurrent stateful_python_code_exec calls on one rollout now run one at a time (the session lock covers the whole call). A sandbox DELETE that still fails after 3 bounded attempts now fails /close_session and a cookie reseed with 500 and keeps ownership; on main the Skills cleanup logged and dropped the mapping (/verify still returns its score). A request without a session ID no longer gets a generated uuid4 session. This PR and #5677 edit the same statement in _verify; whichever lands second takes #5677's dict-merge form for the delegated verdict.

ns_tools keys a stateful NeMo Skills python session and the rollout's tool
timings by the session cookie, and released both only in /verify. Under
single_agent_turn_legacy, an episode that never reached /verify (agent
failure, skip_verification, cancellation) kept its sandbox python session
until the server shut down, because the inherited close_resources_session
kept no state.

- seed_session binds a typed seed's resources_session_id to the cookie and
  records its episode and task; the legacy /run seed body keeps no state.
- close_resources_session releases the python session and timings by
  resources_session_id, fences late seeds with ResourcesSessionFence, is
  idempotent, and raises when cleanup fails so the caller can retry.
- A tool call for a closed session gets ResourcesSessionLostError (404); a
  call that a close overtook releases the python session it recorded.
- A /verify for a closed session returns lost_session_verification.
- Drop the unreachable random-uuid fallback for a missing session ID.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Nov 3, 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.

@yaoyu-33 yaoyu-33 added area:environment Individual environments, benchmarks, verifiers, and environment-specific resources servers bug Something isn't working labels Nov 3, 2026
@yaoyu-33

yaoyu-33 commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test c131fde

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

@yaoyu-33
yaoyu-33 marked this pull request as ready for review November 3, 2026 18:33
@yaoyu-33 yaoyu-33 added the needs-review PR is ready for code review and waiting on a reviewer label Nov 3, 2026
yaoyu-33 and others added 3 commits November 3, 2026 11:05
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…the release fails

The DELETE that releases a python session passed no retry bound, and Gym's request helper retries refused connections without limit, so a restarting sandbox held close (and the session's fence) forever. It now gives up after three tries, like Skills' own delete, and close fails for the caller to retry.

Verify's finally now awaits a release that can raise, so a failed DELETE replaced a scored verify with a 500. Verify logs the failure and keeps the score; the python session stays owned, so close retries the release. The release also runs under the session's fence, so a tool call still running records its python session first. The closed check after a tool call could never fire under that fence and is gone.

Tests: an unreachable sandbox, a failed release at verify (both fail on the previous code), and the cancellation test now races a real close against the in-flight POST. The conformance import moves to nemo_gym.server_runtime.testing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@yaoyu-33

yaoyu-33 commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 0542b9e

Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@nemo-automation-bot
nemo-automation-bot Bot requested a review from a team November 3, 2026 21:43
@github-actions github-actions Bot added the sla:triage-overdue Review assignment is over the one-business-day SLA label Nov 4, 2026
yaoyu-33 and others added 5 commits November 4, 2026 15:19
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…ycle

- The repo contract test counts a server that implements the state hooks as
  closing through the inherited close_resources_session. newton_bench,
  workplace_assistant, tavily_search and indirect_prompt_injection override
  seed_session but close through the hooks, and failed it.
- Export reject_partial_typed_seed from nemo_gym.server_runtime, so servers
  that override seed_session reuse the shared guard instead of a copy.
- The MCP seed wrapper still recognises a request param annotated as
  Annotated[Request, ...]; keeping Annotated metadata had made it inject a
  second Request and fail with KeyError.
- Name the 409 for a typed seed over a cookie session "cookie-seeded", not
  "legacy", and document that a served seed_session override declares
  request before body.
- Shared tests now cover the MCP-wrapped seed route, close after verify
  (checked against the verified episode), and a cookie seed of a verified
  typed session.
- The add-benchmark skill describes the three state hooks as the default
  for a stateful server.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
The inherited retire_session_state recorded a typed session as closed. The
checkpoint participant also calls that hook to undo a restore
(delete_restored) and to drop an unclaimed session, and both promise to
refuse nothing later; with the fence, a restore that was rolled back and
installed again in the same process left its session fenced, so its verify
was masked as lost and its continuation's seed got 409. The participant
already refuses a retired attempt's requests until it forgets the rollout,
so retirement now releases the state and drops the identity without
recording a close. A close still fences.

Also write down that an exported server using the state hooks carries each
session's identity through its checkpoint itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…antics

Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@yaoyu-33
yaoyu-33 changed the base branch from main to yuya/shared-resources-lifecycle November 5, 2026 23:29
yaoyu-33 and others added 4 commits November 5, 2026 16:27
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@yaoyu-33

yaoyu-33 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test b7d76b5

The docs now describe only the typed Environment Server path. The cookie
seed still works unchanged; the PR description records that compatibility.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@yaoyu-33

yaoyu-33 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 399220b

This branch was previously deployed

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

Labels

area:environment Individual environments, benchmarks, verifiers, and environment-specific resources servers bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer sla:triage-overdue Review assignment is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant