Repository navigation
refactor(sessions): one name per session lifecycle concept before release - #5083
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-5083-f7d5ab2c7a08.docs.buildwithfern.com/nemo/gym |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP
LGTM — no reliability concerns.
Reviewed all 50 changed files and traced affected seed, close, retire, routing, and exception-handling paths. The resources helper split preserves locking and non-renewing fence expiry. The agent migrations retain failed-cleanup retries and stale-cookie rejection, and the renamed routing keyword propagates consistently through server startup. No material defects found.
Static review only; tests were inspected, not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
e127a5e to
11e8d99
Compare
yaoyu-33
left a comment
There was a problem hiding this comment.
LGTM for the naming/terminology scope. Fixed agent-run seed wording and unavailable-session messages without changing lifecycle behavior. Validation: 311 targeted tests passed, 6 Linux-only skips; pre-commit --all-files passed. Rollout N/A (wording-only fixes).
|
/ok to test d2ef399 |
|
💬 The author’s new head, |
|
✅ CI is green on |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/42107460119 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/42108052103 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/42108265571 |
|
💬 All reported checks on |
|
The PR is no longer an open ready-to-merge PR targeting main. |
Main split ResourcesSessionFence into ResourcesSessionLocks and ResourcesSessionFences (#5083). A server holds the session's ResourcesSessionLocks lock around lend() and release(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
What does this PR do?
Gives every resources session and agent session lifecycle concept one name, used the same way in code, docstrings, error messages, tests, config descriptions and the docs, as #4997 did for partial-rollout checkpoints.
The session contract grew across several PRs, and the same concept ended up with several names:
ResourcesSessionFencedid two jobs. It held the per-session lock that keeps a seed and a close from interleaving, and it kept the record that refuses a seed after the close. Servers therefore wroteself._fence.locked(...), which reads as a lock and a fence at once.ClosedResourcesSessionRecords.remember), a closed mark (mark_closed,closed_episode,raise_if_closed), a close receipt, and a tombstone, depending on the file.window_secondsandclose_window_seconds, although the released config field already calls itsession_close_retry_window_seconds./runwas an untyped seed.HTTPException(409), while resources servers hadResourcesSessionConflictError.Nothing renamed here has shipped as a public API, so every rename is a plain rename with no aliases.
One term per concept.
ResourcesSessionLocks.locked()ResourcesSessionFence.locked,_FenceLockResourcesSessionFences[T]withfence(),get(),raise_if_fenced(),clear()ClosedResourcesSessionRecords.remember,ResourcesSessionFence.mark_closed,closed_episode,raise_if_closed, "remembers the close", "close receipt", "tombstone"close_retry_window_secondswindow_seconds,close_window_seconds, "close window", "retry window", "retention window"expire_agent_session_records()_release_retired_session(),on_session_released,SessionReleased_free_session,on_session_freed,SessionFreed, "free", "unfreed"SessionOwners, owner file insession-ownersSessionClaims, claim file insession-claims, routing claim/runAgentSessionConflictErrorHTTPException(409, ...)"Expire" keeps one meaning in session code: a timed lifetime ran out. A fence expires, a checkpoint lease expires, and an open session whose lifetime runs out verifies as
session_expired. The subject of the sentence tells them apart.SandboxSession.expire(), which ends a harness as if its time limit had passed, is listed in the glossary with the other words that other features use in their own senses.Lifecycle.
flowchart TD seed["Episode seed, holding the session's lock"] --> open["Open session"] open --> work["Tool calls and verify"] work --> close["Close, holding the session's lock"] close --> release["Release the session's state"] release --> fence["Fence the session for the close retry window"] fence -->|"a seed arrives late"| refused["409 conflict, terminal"] fence -->|"the close is retried"| saved["Same answer; an agent returns the saved reply"] fence -->|"the close retry window passes"| expired_fence["The fence expires and the ID is new again"] work -->|"the server no longer holds the session"| lost["Lost: 404 on a tool call, masked verify with session_lost"] open -->|"its lifetime runs out (planned)"| expired_session["Expired: masked verify with session_expired"]Locks and fences are now separate classes.
ResourcesSessionFenceis split in two.ResourcesSessionLocksholds the per-session lock, andResourcesSessionFences[T]replaces both the fence half ofResourcesSessionFenceandClosedResourcesSessionRecords[T], which were the same structure with different method names. A fence holds a record of the server's choice:example_single_tool_call,enterpriseops_gymandgdpvalstore the closing episode,partner_ifstores an optional episode, and the NOOA counter server stores the close request. A server that usedResourcesSessionFencenow holds both:raise_if_fenced()refuses any fenced ID, including one whose record isNone, aspartner_ifstores for a retired session. The oldraise_if_closed()refused only when the stored episode was notNone. No server's behavior changes: every server that calls it stores an episode, andpartner_iftests membership itself.get()returnsNoneboth for an ID that is not fenced and for aNonerecord, so its docstring tells such callers to testin.partner_if's SWE adapter keeps its fences in_adapter_fences, apart from the Scale-SWE_fencesit inherits, as the two stores were apart before the rename.Agent sessions.
SimpleResponsesAPIAgentraisesAgentSessionConflictErrorfor every 409 it returned before, with the same status and, apart from the two messages below, the same detail. LikeResourcesSessionConflictError, it is also aValueErrorfor direct callers. A seed or close whose cookie names a session the server no longer holds is refused with "Agent session cookie names a session that is no longer available" and "Agent session is no longer available" instead of "Agent session cookie has expired" and "Agent close receipt has expired". Missing state does not by itself establish that a fence expired. The Codex agent's stale-cookie refusal follows the same wording. Agent subclasses and resources servers that raise a plain 409HTTPExceptionkeep doing so; moving them onto the conflict errors is left for a follow-up.Routing says owner. Session routing records which uvicorn worker holds each session and forwards the session's requests there. It already called that worker the owner in the
x-ng-session-ownerheader and inroutes_sessions_to_owner, and now says owner everywhere:SessionOwnerswithowner()andacquire(), owner files in asession-ownersdirectory, andowned_session_routing(). Claim keeps only its checkpoint meaning. The rollout-stop routing #5145 added names its key/rollouts/<rollout id>withROLLOUT_KEY_PREFIXandSessionKind.path_key. The checkpoint stack says release for freeing state, as the session code does.Docs. The glossary page (About → Key Terminology) gains a Sessions section: one row per term with the names it replaces, and a note on the words other features use in their own senses (receipts, tombstones, token capture and launch fences, MCP token claims and the launch claim file, the checkpoint terminal step, and
SandboxSession.expire()). Its Conflict row names the shared session code that raises the conflict errors and asks new session code to do the same, since many existing servers still raise a plain 409. Release also covers a worker releasing its ownership of a session ID, the opposite of acquiring it.AGENTS.mdpoints assistants at that section in one line, so the table is not loaded into every session. The resources server API guide describes episode sessions,ResourcesSessionLocksandResourcesSessionFences. The harness guide, the troubleshooting guide, the NeMo user simulator rollout pages and the Codex, Hermes, OpenClaw, OpenCode, NeMo user simulator and SWE-Together READMEs use fence, saved reply, close retry window and resources session.Unchanged by decision:
session_close_retry_window_secondsalready names the close retry window.session_lostandsession_expiredalready name lost and expired sessions._locked_agent_sessionandterminal_bench_2_1's_locked_resources_sessionalready say lock.mark_retired_sessions(a retired session is marked, not fenced, and its mark ends at forget),unclaimed, andResourcesSessionLifetimes.retainandforget.deletestays;ResourcesSessionFences.clear()is documented as deleting every fence, in the same sense.judge_receipt_*gap codes,gdpval's journal tombstones andgenrm_compare's cohort tombstones./runstays, except where a sentence paired it with "typed": there it becomes agent-run seed.Servers merged with the earlier names move onto the settled ones here too:
swebench_pro,interactive_browserandrna_design(fix(resources): move swebench_pro and interactive_browser onto the session contract #4692, feat(rna_design): RNA design environment #4931) move ontoResourcesSessionLocksandResourcesSessionFences.agentic_if,officeenv,partner_if's SWE adapter andscale_swemove fromClosedResourcesSessionRecordstoResourcesSessionFences.browsecomp_advanced_harnessand the shared SWE sandbox-session base inswebench, whichr2e_gymandswe_gymuse, move fromResourcesSessionFenceto both classes. Their prose, test names and attributes drop "typed", "Environment Server session" and "remembers the close" (for example_typed_sessions→_episode_sessions).terminal_bench_4keeps its own fence bookkeeping, now named for fences:_fenced_resources,_fence_expiry,_start_fence_expiry()and_expire_fences()instead of closed records and a retention window.nemo_user_sim'sclosed_resources_session_ids→fenced_resources_session_ids.swebench's ownseed_sessionan agent-run seed path that still calledResourcesSessionFence.closed_episode(), which this PR removes. It now tests membership in the shared base's_task_session_fencesand refuses with "An agent-run seed cannot replace an episode session", asr2e_gymandswe_gymdo. Its docstrings and test names say episode seed and agent-run seed, and the refusal test checks the message.Renamed public names
For open PRs and downstream servers:
ResourcesSessionFence(window_seconds)→ResourcesSessionLocks()plusResourcesSessionFences[EpisodeId](close_retry_window_seconds)ResourcesSessionFence.locked()→ResourcesSessionLocks.locked()ResourcesSessionFence.mark_closed()→ResourcesSessionFences.fence()ResourcesSessionFence.closed_episode()→ResourcesSessionFences.get()ResourcesSessionFence.raise_if_closed()→ResourcesSessionFences.raise_if_fenced()ClosedResourcesSessionRecords[T](window_seconds)→ResourcesSessionFences[T](close_retry_window_seconds), andremember()→fence()install_session_routing(close_window_seconds=...)and theclaimed_session_routing()keyclose_window_seconds→close_retry_window_secondsResourcesSessionLifetimes(window_seconds)and itswindow_secondsattribute →close_retry_window_secondsSimpleResponsesAPIAgent._free_session()→_release_retired_session(), which subclasses call fromretire_agent_sessionSimpleResponsesAPIAgent._closed_session_records→_fenced_session_records, which agent tests readAgentSessionConflictErrorinnemo_gym.agents.base_responses_api_agent, exported fromnemo_gym.agentsSessionClaims→SessionOwners; itsclaimant()→owner()andclaim()→acquire()SESSION_CLAIMS_DIR("session-claims") →SESSION_OWNERS_DIR("session-owners"), andSESSION_CLAIM_SCOPE_KEY→SESSION_ACQUIRE_SCOPE_KEYapp.state.nemo_gym_session_claims→app.state.nemo_gym_session_ownersclaimed_session_routing()→owned_session_routing(), andSessionRoutingMiddleware(claims=...)→SessionRoutingMiddleware(owners=...)AgentSessionHolder.use_agent_session_claims()→use_agent_session_owners();SimpleResponsesAPIAgent._agent_session_claims→_agent_session_ownersand_release_agent_session_claim()→_release_agent_session_ownership()ROLLOUT_CLAIM_PREFIX→ROLLOUT_KEY_PREFIX,SessionKind.path_claim(groupclaim) →path_key(groupkey),_claim_rollout()→_acquire_rollout_ownership()and_release_rollout_claim()→_release_rollout_ownership()CheckpointParticipant.on_session_freed→on_session_released, andSessionFreed→SessionReleasedThe import-surface fixture that guards old module paths records the new names in place of the removed ones for the six old paths that served them, as #5248 did for its rename.
Issue
No issue. The names came up while reviewing #4692, which moves
swebench_proandinteractive_browseronto the resources session contract; most of the confusing terms there come from names already onmain. Settling them before release keeps the open session PRs from spreading them further.Open PRs that rebase onto this
These open PRs add servers or routing code with the earlier names; each needs only the renames above:
feat(example-session-state-mgmt): serve Environment Server sessions #4058 (
example_session_state_mgmt), feat(finance_sec_search): serve the resources sessions API #4948 (finance_sec_search), feat(example_mcp_weather): serve the typed resources sessions API #5480 (example_mcp_weather) and feat(finance_agent_v2): serve the resources sessions API #5491 (finance_agent_v2) serve "typed" resources sessions.feat(openenv): serve the typed resources sessions API #5483 (
openenv), feat(newton_bench): serve the typed resources sessions API #5485 (newton_bench), feat(workplace_assistant): serve the typed resources sessions API #5486 (workplace_assistant), feat(tavily_search): serve the typed resources sessions API #5487 (tavily_search) and feat(indirect_prompt_injection): serve the typed resources sessions API #5488 (indirect_prompt_injection) serve resources sessions withResourcesSessionFence, and feat(newton_bench): serve the typed resources sessions API #5485 also usesClosedResourcesSessionRecords.fix(ns_tools): release python_tool sessions in close_resources_session #5479 (
ns_tools), fix(math_with_code): release the Python worker in close_resources_session #5481 (math_with_code) and fix(litmus_agent): release the sandbox through the shared session hooks #5482 (litmus_agent) release state inclose_resources_sessionwithResourcesSessionFence.feat(swebench): run multimodal through environment server sessions #5659 (
swebenchmultimodal) usesclosed_episode()and says "typed session"; feat(gdp-pdf): add OpenCode environment and benchmark #5635 (gdp_pdf) and feat(partner_if): add advertised-tool checker and update dataset pin #5314 (partner_if) useResourcesSessionFenceorClosedResourcesSessionRecords.Rshahbazyan/hle web tools #5176 and feat(mcp_atlas): add MCP-Atlas with per-rollout agent-environment sandboxes #5287 (
mcp_atlas) add servers on the session contract.feat(server): run every server that can with several workers #4790 says "tombstone" once for a close record and uses the routing claim names; feat(environment): route BaseEnvironmentServerWithTools tool calls to the worker that runs the episode #5350 and Sniebler/history delta #5503 use
SessionClaims, and feat(environment): route BaseEnvironmentServerWithTools tool calls to the worker that runs the episode #5350 also usesclaimed_session_routing()andclose_window_seconds.feat(checkpoint): checkpoint evaluation runs from rollout collection #3893 (checkpointed rollout collection) says "freed" and "claimed" in comments for releasing and owning sessions.
The sandbox checkpointing stack has the largest overlap, and the merge order matters. In merge order the stack is #4220 → #4221 → #4222 → #4576 → #4274, with drafts #4451 → #4553 → #4778 → #4779 → #4936 above it. Its branches still carry the multi-worker session routing commits from before #3909 was squashed, so #4220's diff against
mainadds about 130 lines withSessionClaims,claim/claimed,close_window_secondsandClosedResourcesSessionRecords, and #4221 and #4222 add one more each. The stack needs a rebase ontomainregardless. If this PR merges first, that rebase picks up the owner, release and fence names in the same pass. If the stack merges first, this PR has to rename the sandbox checkpointing code as well.#4893 also edits the glossary page; whichever of the two merges second needs a small rebase.
Validation
After rebasing onto
mainat b5f8da9:pytest -n 16 tests/unit_testsin a venv synced with CI's extras. The failures are those listed below, which fail onmaintoo.swebench's server tests in its own venv, for the agent-run seed path this rebase moved onto the settled names.mainhad rewritten; the resolution keepsmain's version withowner()in place ofclaimant(). That test needs the end-to-end environment, so CI runs it.pre-commit run --fileson the changed files.Before that, after rebasing onto
mainat af77ecc:pytest -n 16 tests/unit_testsin a venv synced with CI's extras. The import-surface checks pass with the updated fixture.agentic_if,officeenv,partner_if,scale_swe,r2e_gym,swe_gym,gymnasium,example_session_state_mgmt,simple_agent,single_agent_turnandswe_together.pyproject.toml, for the servers whose packages the root extras lack:browsecomp_advanced_harness,swebench,terminal_bench_4,interactive_browser,nemo_user_sim(both the resources server and the environment server) andnooa_agent. The resources servers among them runcheck_resources_session_contract.example_single_tool_call,enterpriseops_gym,gdpval,terminal_bench_2_1,codex_agent,harbor_oracle_agent,opencode_agent,hermes_agent,openclaw_agent,miniswe_sandboxed_agent,claude_code_agent,swebench_proandrna_design.pre-commit run --fileson every file the branch changes.Unit tests that fail locally fail on
mainat the same commit too: the benchmark-listing test, because this host's name resolves to127.0.1.1, and the Harbor run-summary tests. Two Docker PTY tests failed once under the full parallel run and pass on their own; this branch does not touch the sandbox code.haystack_agentandlanggraph_agentchange only a comment each, so they were not run.Checkpoint stack, at
f7d5ab2c7a, in a venv synced with CI's extras so that no checkpoint test skips for a missing package:pytest -n 16 tests/unit_tests -k checkpoint), across server runtime, models and coordination. Only two unrelated tests skip, for the optionalcometandnemo.lenspackages.NEMO_GYM_CHECKPOINT_E2E=1 pytest tests/e2e/checkpoint -k "not real_model".NEMO_GYM_CHECKPOINT_MULTIWORKER_STRESS=64), which restores sessions across workers through the renamed owner, release and fence code.The split of
ResourcesSessionFenceandClosedResourcesSessionRecordsintoResourcesSessionLocksandResourcesSessionFencesis the one refactor here that could change checkpoint behavior; these runs show none.After the review follow-up commit:
pytest -n 16 tests/unit_tests/agents tests/unit_tests/server_runtime.codex_agent,openclaw_agent,opencode_agent,simple_agent,partner_if,r2e_gym,swe_gym,terminal_bench_2_1,agentic_if,swebench_pro,enterpriseops_gymandharborin the root venv, andswebench,terminal_bench_4,nemo_user_sim(both servers),interactive_browserandnooa_agentin their own venvs.pi_agent's tests pass except its pinned-runtime tests, which fail on this host because its Node runtime lackswebidl.util.markAsUncloneable; that commit only renames onepi_agenttest.hermes_agent's tests need packages neither venv has; that commit only renames one test name there.officeenv's tests skip without the OfficeEnv package, so its renamed session code has not run here.pre-commit run --fileson the changed files.Rollouts
N/A: the change renames classes, methods, parameters and messages and splits one class into two with the same behavior. No rollout, verifier or session routing behavior changes, and the session conformance checks cover every server whose session code changed.
Compatibility
ResourcesSessionFence,ClosedResourcesSessionRecordsand the routing parameters have not been released.r2e_gym,swe_gymandswebenchrefuse an agent-run seed over an episode session with "An agent-run seed cannot replace an episode session" instead of "A legacy seed cannot replace a typed resources session"; the status does not change.session-claimstosession-ownersinside each server's runtime directory, which every server process creates fresh, so nothing on disk needs migrating.claimed_session_routing()must rename it toowned_session_routing().main(at 9477426609) imports none of the renamed names. Its code imports the checkpoint stack'scoordinationmodule andAdmissionClosedError, and its tests also useResourcesParticipantandRestartOnlyAgentParticipant, whose constructors and hooks keep their names, and reads theunclaimedstatus, which is unchanged. It handles a refused forget by its failures, not by the message text that now says "releasing".harbor_oracle_agent404 for a request without a seeded session now reads "No seeded oracle agent session for this request".check_agent_seed_rejectedreports an unsupported seed that did not fail with "a terminal 422" instead of "a final 422"; a test that matches that assertion text needs the new word.Checklist
pre-commit run --fileson the changed files).git commit -s).