Repository navigation
Conversation
…ess close Add ResourcesSessionFence, which serializes seed and close per resources_session_id and remembers closed sessions for CLOSED_SESSION_RETENTION_S instead of until shutdown. A seed for a closed session gets 409, which single_agent_turn does not retry. State that close must release state by resources_session_id when the request carries no cookie, as after a lost seed reply, and check it in check_resources_session_contract with an optional holds_state probe. Move example_single_tool_call onto the fence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…s at verify Seed and close refusals that no retry can fix were plain ValueErrors, so they became a 500 and single_agent_turn dispatched the whole episode again. Add ResourcesSeedRejectedError (422) for input that fails validation and ResourcesSessionConflictError (409) for an ID bound to another episode, a close for another episode, or a closed session. A verify for a session the server no longer holds returned reward 0 (a policy failure) or an error. Add lost_session_verification, which builds a masked sample with failure_kind session_lost or session_expired, and ResourcesSessionLostError (404) for tool calls on such a session. check_resources_session_contract now requires 409 for a conflicting or late seed and, with verify_body, a masked verify after close. Move gdpval and nemo_user_sim, which already run the check, onto these rules; nemo_user_sim also maps resources_session_id to its cookie session so a cookie-less close releases it and a conflicting seed is caught. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…completion A verify for a session gdpval no longer holds now returns a masked session_lost sample instead of a 409. The multistage orchestrator decides success by the failure class alone, so it counted that sample as a judged success: a final stage with half its tasks judged reported complete with a headline ELO, and a partial earlier stage was coverage-blocked and never retried. tag_results now turns a session_lost or session_expired result into the terminal verification failure the 409 used to produce, so routing, success, resume gating and partial-stage acceptance behave as before. aggregate_metrics also leaves such a row out of its stage's observed rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…view - gdpval: bind the session cookie only after a seed's checks pass, so a refused 409 or 422 reply no longer hands the caller another episode's session. - example_single_tool_call and nemo_user_sim: a close of a closed session that names another episode gets 409, and check_resources_session_contract now checks it (keeps_state only). nemo_user_sim records the closing episode. - nemo_user_sim: a seed dropped after session_ttl_seconds verifies as a masked session_expired sample instead of session_lost. - lost_session_verification logs a warning, since a masked sample is otherwise visible only in coverage metrics, and documents the dict merge that avoids a duplicate-keyword TypeError with extra="allow" requests. - ResourcesSessionLostError says what each harness does with the 404 and validates its failure_kind. - Tests: fence locks are per session, clear() forgets records, the contract check rejects a 500 late seed, refusals reach the wire as 422/404, a masked sample is kept for coverage and dropped from quality, gdpval TaskData 422 and judged-not-masked gone session, nemo_user_sim close by ID first. - Docs: the resources server tutorial and the nemo_user_sim README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
main added ClosedResourcesSessionRecords and session routing (#3905): closed resources sessions are remembered for the configurable session_close_retry_window_seconds, a retried close keeps the first record, and with several workers a record must not outlive the session's routing claim. ResourcesSessionFence now keeps its per-session lock and delegates closed records to ClosedResourcesSessionRecords[EpisodeId], constructed with the server's session_close_retry_window_seconds. CLOSED_SESSION_RETENTION_S (a fixed 3600 s that renewed on every close) is gone. example_single_tool_call and gdpval build the fence from their config. The close-contract docstring now covers both a routed server, where a typed close is given resources_session_id even without a cookie, and an app without routing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Resolve the test import conflict: main's #3591 imports BaseRunRequest next to this branch's BaseVerifyRequest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…on aggregation - A failed request that minted a new session no longer sets the session cookie. With session routing, which run_webserver installs even for one worker, a typed seed's session ID is the body's resources_session_id before the handler runs, so a seed refused for another episode used to get that live session's cookie and could act on it (for example gdpval's /abandon_task_finish). The gdpval regression test now also runs on the routed stack. - gdpval aggregation keeps lost-session rows in their stage, so coverage metrics and stage diagnostics survive, including when every row is lost. They are left out only when counting observed final-stage rows. - gdpval refuses a rollout_id that is not <task_index>-<rollout_index> with ResourcesSeedRejectedError (422) instead of a retried 500. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Uvicorn can remain in its 30-second worker health check after SIGKILL. Since pytest is not the worker parent, psutil waits for the supervisor to reap the worker; the old 10-second wait could fail before that happened. Allow 60 seconds in both worker-loss tests while retaining replacement readiness and session assertions. Validation: session routing E2E, 5 passed and 8 opt-in stress tests skipped; targeted worker-loss stress with 200 sessions, 1 passed; pre-commit run --all-files passed. A real Uvicorn 0.37.0 process probe reproduced the old timeout at 10 seconds and succeeded with the new wait at 29.8 seconds. Independently reviewed. Model rollouts N/A for this test-only follow-up. Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — one RISK: TTL eviction frees finance data but leaves the evicted sessions open in lifecycle tracking.
Reviewed both changed files and traced the affected seed, close, verify, routing, and checkpoint contracts. Static inspection only; tests and model rollouts were not executed in this read-only review.
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.
Stacked on #4679 and uses its session helpers. Accept ResourcesSeedSessionRequest on /seed_session alongside the legacy body. A typed seed is keyed by resources_session_id under the ResourcesSessionFence lock; a retry returns the open session without clearing its documents, and a seed binding the ID to another episode or task gets ResourcesSessionConflictError (409). The cookie is bound to resources_session_id only after those checks pass, so a refused seed's reply does not hand out the live session. close_resources_session releases the session's data storage, time budget and typed seed by ID, with or without the cookie, and fences it so a late typed or legacy seed gets 409. Verify still releases the session, since finance_agent's legacy /run never closes it; its score does not depend on session state, so it is not masked. parse_html_page and retrieve_information raise ResourcesSessionLostError (404) for a session that is not open instead of creating storage, and a seed discards sessions older than session_ttl_seconds that nothing verified or closed. Closes GYMINT-171. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
ac5aaec to
33939a3
Compare
|
/ok to test 33939a3 |
|
🌿 Preview your docs: https://nvidia-preview-pr-4948-65276d07ab37.docs.buildwithfern.com/nemo/gym |
…ted sessions A successful seed opens the session's routing claim until a close or verify ends it, and binds it in checkpoint tracking. A session evicted for exceeding session_ttl_seconds has no such request, so with several workers its claim stayed open for the server's lifetime. Eviction now reports the evicted session as ended to the checkpoint participant and the resources session lifetimes, the same notifications the end-session hook sends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test 65276d0 |
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> # Conflicts: # resources_servers/finance_sec_search/tests/test_app.py
|
/ok to test bee3a52 |
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A legacy `{}` seed, or a model's tool call named seed_session, carrying a
typed session's cookie took the legacy branch and wiped that session's
documents and restarted its time budget. It now gets 409, as a cookie seed
of a closed session already does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
- The eviction test installs the session router with the public install_session_routing instead of setting the router's app.state attribute by hand, so a rename in core fails it. This also drops the test's import from a submodule outside the public API. - The route-level contract check now sends a verify after close. - New: a typed seed's cookie reaches the tools through the routes without the session router; a seed keeps other open sessions within their TTL; a verify re-sent before close gets the same score. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test 031b002 |
…ce-sec-search-sessions Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
…nces Main's #5083 split ResourcesSessionFence into ResourcesSessionLocks and ResourcesSessionFences and renamed the routing helpers, so after merging main the server no longer imported. - Use ResourcesSessionLocks.locked and ResourcesSessionFences (fence, get, raise_if_fenced); the tests use owned_session_routing. - Verify releases only an agent-run session. An episode session keeps its seed until its close, so a close that names another episode is refused after verify too. - A session that outlived session_ttl_seconds is masked at verify as session_expired instead of scored; an evicted episode session is fenced as after a close. - Name sessions as main does: episode seed/session and agent-run seed. 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>
|
/ok to test 2fa1099 |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — no eligible findings remain in this follow-up.
I verified that the prior eviction finding is fixed: stale-session eviction now ends checkpoint tracking and routing ownership. The regression test checks that the abandoned session’s lifetime expires while the fresh session remains held. I also checked the follow-up lock/fence integration, seed guards, verification and expiry behavior, retirement cleanup, and session documentation against their directly affected contracts.
The previously reviewed tree was unavailable, so I couldn’t compare the complete inter-review source delta. This was a targeted follow-up, not a fresh whole-PR review. I found no proven introduced regression above the prior moderate impact ceiling. Static inspection only; I didn’t execute tests or model rollouts.
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 findings-only review. Request a full review with /review reset=true.
What and why
finance_sec_searchkeeps per-rollout state:_data_storage, which holds parsed documents, and_session_start_times, which tracks each rollout's time budget. But it only served the agent-run seed, so an Environment Server's episode seed got{}back. It also had noclose_resources_session, so a rollout that never reached/verifyleft its state in memory for as long as the server ran.This PR implements the resources sessions contract as tightened in #4679:
/seed_sessionacceptsResourcesSeedSessionRequest | BaseSeedSessionRequest. An episode seed is keyed byresources_session_idand serialized per ID withResourcesSessionLocks.locked.ResourcesSessionConflictError(409).SESSION_ID_KEYis bound toresources_session_idonly after these checks pass, so a refused seed's reply doesn't hand out the live session. The same fix was made in GDPVal in fix(resources): tighten the session contract for close, refusals and lost sessions #4679.{}.close_resources_sessionreleases the session's storage, time budget and episode seed by ID. That works with or without the session cookie, which covers the case where the seed's reply was lost.ResourcesSessionFences.fence. Until the fence expires, a later episode or agent-run seed gets 409.finance_agentruns behindlegacy_agent, which never calls/close_session, so its verify releases the session. An episode session keeps its seed and documents until its close, so that close still checks the seed's episode. A/verifyfor a session that was already closed returns a maskedlost_session_verificationsample even though the score comes only fromsubmit_final_resultand the judge; this is kept on purpose so every server on the contract treats an out-of-order verify alike (single_agent_turnalways verifies before it closes).parse_html_pageandretrieve_informationraiseResourcesSessionLostError(404,failure_kind=session_lost) when the session isn't open. Before, they created storage that nothing would ever release.finance_agentpasses a non-200 tool body to the model unchanged.retire_session_statenow releases the session, asworkplace_assistantandexample_session_state_mgmtdo.session_ttl_seconds.session_ttl_seconds(default 6 h). That limits the leak on the agent-run path when a rollout fails before/verify. A verify for an evicted session is masked withfailure_kind=session_expired, and an evicted episode session is fenced as after a close. The TODO inSimpleResourcesServer.close_resources_sessionplans a general version of this.Why the cookie is bound explicitly
An earlier review said binding the cookie is unnecessary because the session router already names the cookie by
resources_session_id. That only holds when the router middleware is installed, whichrun_webserverdoes. An app built withsetup_webserver()alone gets a random cookie ID. #4679'sclose_resources_sessiondocstring now says the same thing. The first version of this PR failed the route-level conformance test until the cookie was bound.Not in this PR:
finance_agent_v2(same pattern): feat(finance_agent_v2): serve the resources sessions API #5491, stacked on this PR./discard_sessionin the same file: if it lands first, its discard should route toclose_resources_session.finance_agentagent sessions and the configs that run it behindsingle_agent_turn_legacy: feat(finance_agent): enable session execution in default configs #5492.No separate public issue: this is one server's step in the resources-sessions migration that #4679 defines; the stack's other PRs are listed above.
Compatibility
session_ttl_seconds(default 21600). It must be longer than the longest rollout.parse_html_pageorretrieve_informationon a session that is not open now returns 404 withfailure_kind=session_lost. Before, it returned an observation.finance_agentalways seeds before calling tools.session_ttl_secondsis masked (failure_kind=session_expired) instead of scored, because its tool calls after the eviction got 404.Current stack validation (November 6)
Real-model validation is now complete at stack head
f0b200ce6568ff02cbc2d51a6dd5d46ccb3d6b5bin #5492. Theresources_servers/finance_sec_search/implementation and tests are identical to this PR's current head; only README/config changes differ in the top PR. The resource suite passed 116 tests there, and full pre-commit checks passed (the unavailable private guardwords catalog was skipped). The entire selected stack validation passed 822 tests.The canonical resources config passed a bounded real-NIM HTTP smoke with reward 1.0, actual policy/retrieval/judge calls, successful close responses, zero active agent state and empty captured document/time maps. The judge recovered from two upstream 429s. The renamed resources seed map is covered by source/tests, not directly captured by that driver. See #5492's validation for the complete scope and compatibility notes.
This supersedes the earlier no-real-model statements below for the integrated stack. It does not establish standalone lower-PR episode activation, CLI collection, live SEC/Tavily/Tiingo coverage, full benchmark results or relay score parity. Historical evidence retains its original revision.
Historical validation
Update on
788316605(mergedmainatc6cad9e52, addedretire_session_stateand the README section):resources_servers/finance_sec_search/.venv/bin/python -m pytest resources_servers/finance_sec_search/tests: 106 passed, including the newtest_retired_attempt_releases_its_session.pre-commit run --fileson the three touched files: clean.Earlier validation, on the pre-merge head:
All on the final commit, on top of #4679
a49da9c9b:gym env test +entrypoint=resources_servers/finance_sec_search +should_validate_data=true: 104 passed.check_resources_session_contract(..., keeps_state=True, holds_state=...)from fix(resources): tighten the session contract for close, refusals and lost sessions #4679, run through the real routes. It covers the conflicting seed, the conflicting close, a cookie-less close after a lost seed reply (state released), and late seeds.test_typed_seed_for_another_episode_is_refused_without_its_cookiefail.setup_webserver()plusinstall_session_routing(socket_dir=None, ...):{}.pre-commit run --all-files: clean.Review fixes on
031b0022d7c9(team review)Merged
main(clean) and fixed what a review team found:{}seed, or a model calling a tool namedseed_sessionthrough the grant, wiped the episode session's documents and restarted its time budget.install_session_routinginstead of setting the router'sapp.stateattribute by hand, so a rename in core fails it (and the test no longer imports a submodule). New tests cover the episode seed's cookie reaching the tools through the routes (removing the explicit bind now fails it), the route-level contract check with a verify body, a seed that keeps other sessions within their TTL, and a re-sent verify before close getting the same score.Validation at
031b0022d7c9:pytest resources_servers/finance_sec_search/tests: 111 passed (worktree modules confirmed via__file__);pre-commit run --fileson the changed files: passed. Not run: a real-model smoke at this head (the paired smoke below is from33939a380).Review fixes on
2fa1099b4(second team review)Merged
main(now379ae226d). Main's #5083 splitResourcesSessionFenceintoResourcesSessionLocksandResourcesSessionFenceswith no alias, so after the merge the server no longer imported. Fixed, plus what the review found:ResourcesSessionLocks.locked, andResourcesSessionFences(fence,get,raise_if_fenced); the tests useowned_session_routing. Names follow refactor(sessions): one name per session lifecycle concept before release #5083's glossary: episode seed/session, agent-run seed.example_single_tool_calldoes).session_ttl_secondsis masked at verify assession_expiredinstead of scored; an evicted episode session is fenced; a new agent-run seed for the ID clears the mark.Validation at
2fa1099b4:pytest resources_servers/finance_sec_search/testswith the server venv: 116 passed (worktree modules confirmed);tests/unit_tests/test_session_contract_repo.py+tests/unit_tests/config/test_global_config.py: 233 passed;pre-commit run --fileson the PR's files: clean. CI at031b0022dfailed onlyopenclaw_agent/tests/test_exec_transport.py::test_exec_only_supervision_leaves_task_children_for_sandbox_owner[cancel](a known four-workers-per-shard flake, issues #5638 and #5672) and main'stest_each_cleanup_has_its_own_budgettiming test; the session-routing e2e failure is fixed by main's d45f9cf, now merged in. Not run: a real-model smoke at this head.Rollout evidence
Environment Server plumbing smoke on this stack (no real model). No model endpoint or API key is configured on the machine that ran it, so this used a scripted OpenAI-compatible stub for the policy, retrieval and judge, behind real
openai_modelservers.gym env start --config resources_servers/finance_sec_search/configs/finance_sec_search_single_agent_turn.yaml(from #5492) at #5492's head, thengym eval run --no-serve --agent finance_agenton the first row ofdata/example.jsonl:single_agent_turn_legacy, reward 1.0 (judge[[2]]), no failure rows.parse_html_page→retrieve_information→submit_final_result.retrieve_informationfound the documentparse_html_pagestored, so the episode seed's session reached both tools through the agent's direct HTTP grant.finance_sec_search.yaml, result typelegacy_agent) gave the same tool sequence and reward.This proves the wiring, not model quality. The paired real-model smoke below is from the earlier head.
Paired real-model smoke on an internal cluster. Setup: Qwen3-8B on vLLM (TP=1, temperature 0). The run drives the production
legacy_agent→finance_agent→finance_sec_searchpath over real local HTTP, behind the session router:parse_html_page(a synthetic financial report)retrieve_informationsubmit_final_resultRetrieval and the judge also run against the real model. Both arms use identical task settings.
Current head, rerun after the rebase. One job held the model and controller; it completed
0:0in 2m21s, and model shutdown returned 0.33939a380(this PR)[[2]]/ 1.0a49da9c9b(#4679, parent)[[2]]/ 1.0mask_sample=falseand no failure kind.33939a380, with no skips.An independent reviewer had already run the same paired smoke on the pre-rebase head (
ac5aaec46vs2b5faf08d), with the same outcome.This is plumbing and compatibility evidence, not benchmark accuracy. Not covered: live SEC search, multi-worker routing, and cancellation. The episode-seed path has no real caller until #5492, so the route-level conformance test covers it.
🤖 Generated with Claude Code