Repository navigation
refactor(sessions): say owner for session routing and release for freeing state - #5087
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-5087-e73d7711a0c3.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 28 changed files and directly affected routing/checkpoint paths. The renamed contracts remain consistent across routing installation, server adapters, and checkpoint ownership hooks. No material defects identified in acquisition, forwarding, restore/rollback, or retire-until-forget behavior.
Static inspection only; tests were 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.
4ff73be to
2f66d27
Compare
e127a5e to
11e8d99
Compare
yaoyu-33
left a comment
There was a problem hiding this comment.
LGTM for the naming/terminology scope. Clarified current session ownership (not creator or cookie ownership) and brought in the parent fix without rewriting history. Validation: 393 targeted tests passed, 6 Linux-only skips; pre-commit --all-files and Fern check passed. Fern skipped the authenticated redirects check. Merge after #5083; CI still applies.
d2ef399 to
7fca8f0
Compare
ebd5700 to
8be19f0
Compare
8be19f0 to
9e51a52
Compare
88973d6 to
ce691a5
Compare
9e51a52 to
ac7ac7b
Compare
…eing state Session routing called the worker that holds a session a claimant and its record a claim, while the checkpoint stack uses claim for a worker taking a restored record (unclaimed, settled in #4997). Routing already said owner in x-ng-session-owner and routes_sessions_to_owner, so it now says owner everywhere: SessionClaims becomes SessionOwners, claim files become owner files in session-owners, and claimed_session_routing becomes owned_session_routing. The checkpoint stack said a retire frees a session, while the session code says release. It now says release too, and on_session_freed becomes on_session_released. A few session tests still called an episode seed typed; they now use the episode session names. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
ce691a5 to
69bf284
Compare
ac7ac7b to
e73d771
Compare
1701281
into
ananthsub/session-lifecycle-names
…eing state (#5087) * refactor(sessions): say owner for session routing and release for freeing state Session routing called the worker that holds a session a claimant and its record a claim, while the checkpoint stack uses claim for a worker taking a restored record (unclaimed, settled in #4997). Routing already said owner in x-ng-session-owner and routes_sessions_to_owner, so it now says owner everywhere: SessionClaims becomes SessionOwners, claim files become owner files in session-owners, and claimed_session_routing becomes owned_session_routing. The checkpoint stack said a retire frees a session, while the session code says release. It now says release too, and on_session_freed becomes on_session_released. A few session tests still called an episode seed typed; they now use the episode session names. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> * docs(sessions): define ownership by the current session holder Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> --------- Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> Co-authored-by: yaoyu-33 <yaoyu.094@gmail.com>
…eing state (#5087) * refactor(sessions): say owner for session routing and release for freeing state Session routing called the worker that holds a session a claimant and its record a claim, while the checkpoint stack uses claim for a worker taking a restored record (unclaimed, settled in #4997). Routing already said owner in x-ng-session-owner and routes_sessions_to_owner, so it now says owner everywhere: SessionClaims becomes SessionOwners, claim files become owner files in session-owners, and claimed_session_routing becomes owned_session_routing. The checkpoint stack said a retire frees a session, while the session code says release. It now says release too, and on_session_freed becomes on_session_released. A few session tests still called an episode seed typed; they now use the episode session names. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> * docs(sessions): define ownership by the current session holder Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> --------- Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> Co-authored-by: yaoyu-33 <yaoyu.094@gmail.com>
…ease (NVIDIA-NeMo#5083) * refactor(sessions): one name per session lifecycle concept before release Give each resources session and agent session lifecycle concept one name across code, docstrings, error messages, tests, config descriptions and docs. - Split ResourcesSessionFence into ResourcesSessionLocks, which runs a session's seed and close one at a time, and ResourcesSessionFences, which replaces ClosedResourcesSessionRecords and refuses a seed after its close until the fence expires. - Call the fence's duration the close retry window everywhere, including close_retry_window_seconds in session routing. - Add AgentSessionConflictError for agent session 409 refusals. - Use episode session, episode seed, legacy run seed, saved reply, release, lost and terminal in place of typed, untyped, close receipt, tombstone, free and final. Nothing renamed has been released, so there are no aliases. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> * fix(sessions): clarify seed names and unavailable-session errors Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> * test(sessions): match the unavailable-session message in OpenClaw and OpenCode tests The agent base now refuses a retried close or a seed after the fence expires with "Agent session is no longer available". Two sandbox tests still matched "expired". Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> * refactor(sessions): say owner for session routing and release for freeing state (NVIDIA-NeMo#5087) * refactor(sessions): say owner for session routing and release for freeing state Session routing called the worker that holds a session a claimant and its record a claim, while the checkpoint stack uses claim for a worker taking a restored record (unclaimed, settled in NVIDIA-NeMo#4997). Routing already said owner in x-ng-session-owner and routes_sessions_to_owner, so it now says owner everywhere: SessionClaims becomes SessionOwners, claim files become owner files in session-owners, and claimed_session_routing becomes owned_session_routing. The checkpoint stack said a retire frees a session, while the session code says release. It now says release too, and on_session_freed becomes on_session_released. A few session tests still called an episode seed typed; they now use the episode session names. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> * docs(sessions): define ownership by the current session holder Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> --------- Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> Co-authored-by: yaoyu-33 <yaoyu.094@gmail.com> * refactor(sessions): carry the session names to servers merged since Servers and routing code merged after this branch opened still used the earlier names: ResourcesSessionFence in browsecomp_advanced_harness and the shared SWE sandbox-session base, ClosedResourcesSessionRecords in agentic_if, officeenv, partner_if's SWE adapter and scale_swe, and claim, claimant and ROLLOUT_CLAIM_PREFIX in the rollout-stop routing. Move them onto ResourcesSessionLocks, ResourcesSessionFences and the owner names, and drop "typed", "Environment Server session", "retention window", "remembers the close" and "close receipt" from their prose, tests and docs. terminal_bench_4 keeps its own fence bookkeeping under fence names, and nemo_user_sim's closed-session map becomes fenced_resources_session_ids. Add a Sessions section to the key terminology page with each term and the names it replaces, and point AGENTS.md at it in one line. Export the new classes from nemo_gym.server_runtime and AgentSessionConflictError from nemo_gym.agents, and record the renamed names in the import-surface fixture in place of the removed ones. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> * refactor(sessions): finish the name sweep and scope the glossary to the code Review follow-ups for the session names: - The glossary's Conflict row now names the shared session code that raises ResourcesSessionConflictError and AgentSessionConflictError, instead of promising that no session code raises a plain 409. Release also covers a worker releasing ownership of a session ID, Lost says how an agent refuses a cookie for a lost session, and the note on other features' senses lists token capture and launch fences, MCP token claims, the launch claim file, the checkpoint terminal step and SandboxSession.expire(). - The harness guide says saved reply, close retry window and fence instead of "retained close", "replayed" and "final", and points 409 refusals at AgentSessionConflictError. The nemo_user_sim README names session_ttl_seconds, the window its fences really last. - partner_if's SWE adapter keeps its fences in _adapter_fences, apart from the Scale-SWE _fences it inherits, as it did before the rename. - ResourcesSessionFences.get() tells callers whose record may be None to test membership, and the TODO about sessions that are never closed moves to close_resources_session. - Test names, locals, comments and READMEs drop the remaining "typed", "legacy seed", "close receipt", "frees" and "final 422" wording. - New tests pin AgentSessionConflictError as a 409 that is still a ValueError, a fence holding a None record refusing a late seed, and the Codex refusal of a sandbox cookie that names no agent session. The agent retry tests match the exact close and seed messages. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> * refactor(sessions): carry the session names to swebench's agent-run seed NVIDIA-NeMo#4277 gave the swebench server an agent-run seed path that still used the removed ResourcesSessionFence API (_task_session_fence.closed_episode) and the old "legacy seed" and "typed session" wording. It now tests membership in the shared base's _task_session_fences and refuses with "An agent-run seed cannot replace an episode session", as r2e_gym and swe_gym do. Its test names and docstrings say episode seed and agent-run seed, and the refusal test checks the message. A pi_agent test comment merged since says "terminal 422". Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> --------- Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> Co-authored-by: yaoyu-33 <yaoyu.094@gmail.com>
What does this PR do?
Stops two words from meaning two things across session routing and partial-rollout checkpoints. Stacked on #5083, which gave each session lifecycle concept one name; review the diff against that parent branch.
"Claim" meant two things. Session routing recorded which uvicorn worker holds a session's state, so that every request for the session is forwarded to that worker, and called that record a claim (
SessionClaims, the claimant). The checkpoint stack uses claim for something else: the worker that receives a replacement attempt's/runtakes the restored record for that attempt, and a restored record no attempt has taken yet is unclaimed. #4997 settled those checkpoint names, and NeMo RL readsunclaimedas a status key. The two senses meet in the restore code, where a worker installs restored sessions and then rewrites their routing claims. The module docstring ofnemo_gym/_checkpoint/participant_workers.pyused both senses a few lines apart.Session routing already called that worker the owner, in the
x-ng-session-ownerheader and inroutes_sessions_to_owner. Routing now says owner everywhere, and claim keeps only its checkpoint meaning. The owner is the worker that currently holds the session, which is not always the worker that created it: a checkpoint restore installs a session on another worker and moves ownership there."Free" and "release" meant the same thing. #5083 made release the session code's word for freeing a session's state, but the checkpoint stack still said a retire frees a session, and its callback was
on_session_freed. The checkpoint stack now says release too.SessionOwners, "owns the session", "takes ownership", owner filesession-ownersdirectorysession-claimson_session_released,SessionReleasedon_session_freed,SessionFreed, "unfreed"flowchart TD request["Request for a session"] --> lookup["Look up the session's owner file"] lookup -->|"another worker owns it"| forward["Forward to the owner's private socket"] lookup -->|"this worker owns it, or nobody does"| serve["Serve it here"] serve -->|"a seed, or the handler used the session"| acquire["Take ownership: write the owner file"] serve -->|"its last record is gone"| release["Release ownership: remove the owner file"] restore["Checkpoint restore installs a session"] --> rewrite["Rewrite the owner file to name the installing worker"] run["Replacement attempt's /run"] --> claim["Claim the restored record from the coordinator"] retire["Checkpoint retire"] --> released["Release the attempt's state; ownership lasts until the forget"]Error messages. A request refused because another worker took ownership of the same new ID now reads "This new session's ID is already owned by another worker. Retry it." (and "This session's ID ..." from the session middleware) instead of "... is already claimed". A forget refused while a retire is unfinished, and a
/runtold to wait for a rolled-back install, now say "releasing" instead of "freeing". Status codes and error codes do not change.Episode session names in tests. A few session routing and resources server tests still called an episode seed "typed"; they now use the names #5083 settled.
Renamed names
For open PRs and downstream servers:
SessionClaims→SessionOwners; itsclaimant()→owner()andclaim()→acquire().rewrite(),rewrite_all(),disown(),mine(),serving()andrelease()keep their names.SESSION_CLAIMS_DIR("session-claims") →SESSION_OWNERS_DIR("session-owners")SESSION_CLAIM_SCOPE_KEY("nemo_gym.claim_session") →SESSION_ACQUIRE_SCOPE_KEY("nemo_gym.acquire_session")app.state.nemo_gym_session_claims→app.state.nemo_gym_session_ownersclaimed_session_routing()→owned_session_routing(), which servers override to route their sessionsSessionRoutingMiddleware(claims=...)→SessionRoutingMiddleware(owners=...)AgentSessionHolder.use_agent_session_claims()→use_agent_session_owners()SimpleResponsesAPIAgent._agent_session_claims→_agent_session_owners, and_release_agent_session_claim()→_release_agent_session_ownership()CheckpointParticipant.on_session_freed→on_session_released, typedSessionFreed→SessionReleasedUnchanged by decision: every checkpoint claim name (
unclaimed(),claim_restored,ClaimRestored,claim_key,claim_cut,ClaimSettlingError, thecheckpoint_restored_claim_settlingerror code),MCPTokenClaims, the harness supervisor'slaunch.claimfile, and the directory claimsgdpvalkeeps for its persisted deliverables.Issue
No issue. A follow-up to #5083: lining its terms up against the checkpoint stack's found these two words used in two senses.
Validation
pytest -n 16 tests/unit_tests, in a venv synced with CI's extras.NEMO_GYM_CHECKPOINT_E2E=1 pytest tests/e2e/checkpoint -k "not real_model".simple_agent,example_session_state_mgmt,example_single_tool_call,single_agent_turn, andgdpvalin its own venv built from itsrequirements.txt.pre-commit run --fileson the changed files.The only unit test that fails locally is the benchmark-listing test, which fails on this host on
maintoo because its host name resolves to127.0.1.1.Rollouts
N/A: the change renames identifiers, a runtime directory, a scope key and messages. Routing, ownership and checkpoint behavior do not change, and the checkpoint end-to-end suite, which restores sessions across several workers, covers the renamed paths.
Compatibility
claimed_session_routing()must rename it toowned_session_routing().session-claimstosession-ownersinside each server's runtime directory, which is created fresh for every server process, so nothing on disk needs migrating.Checklist
pre-commit run --fileson the changed files).git commit -s).