feat(core): give stateful resources servers a teardown hook and an optional idle sweeper - #2612
feat(core): give stateful resources servers a teardown hook and an optional idle sweeper#2612waple0820 wants to merge 5 commits into
Conversation
70f0b3e to
eec331c
Compare
eec331c to
52adc3b
Compare
|
Rebased onto main and out of draft. Worth flagging what the rebase surfaced: six tests in One of the failures doubles as evidence for the design. When the release call itself fails, the episode logs and returns its result normally. A reclaim that can break a rollout is worse than the leak it fixes, so the failure path is a warning by construction. Also see #2750 (comment) on why a caller-driven release is necessary but not sufficient — the sweeper here is the part that survives a killed collector. |
52adc3b to
753c2fb
Compare
|
#2886 landed yesterday — That is the same shape as That is what the hook here is for — the same guarantee in one place, plus the idle sweeper for the cases no call can reach. |
d0ed159 to
99fc941
Compare
|
The lifecycle hook, agent finally block, TTL sweeper, and tests remain useful, but #2609 now owns a stricter wire contract: caller-created session identity, one-session close capability, seed deduplication and conflict detection, dedicated close outcomes, and idempotent recovery after lost responses. #3037 owns generic TTL and server adoption. This PR should be refactored or replaced against #2609 rather than merged as the final API. |
…tional idle sweeper Gym's episode lifecycle is seed_session -> responses -> verify, with no teardown step, so environments release external resources inside verify() - the one call that does not happen when the training side aborts, cancels or times out. simple_agent shows the gap: /seed_session, then /v1/responses guarded by raise_for_status, then /verify, with no try/finally between them. Of 102 resources servers, 11 override seed_session and they use five mutually incompatible cleanup conventions; five have none at all. newton_bench independently wrote a TTL sweeper and NVIDIA-NeMo#946 ships a second one. * close_session on SimpleResourcesServer, default no-op, served at POST /close_session. Environments that hold something override it, and the contract requires idempotency - the same as SandboxProvider.close(). * session_scope() on the agent base pairs /seed_session with /close_session in a finally; simple_agent adopts it, so the guarantee is structural rather than per-environment discipline. * Optional idle sweeper behind session_ttl_s (default None = today's behavior), as a backstop for what no call can reach: a killed trainer, a dropped connection. Modelled on newton_bench's working implementation. This is the resource-reclaim half of NVIDIA-NeMo#23: cancelling the request is necessary but not sufficient, because a cancelled handler still leaves the session behind. Closes NVIDIA-NeMo#2609 Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
MCP auto-exposure harvests typed POST routes, so the new `/close_session` endpoint was being published as a callable tool: a policy could have ended its own episode's resources mid-rollout. It joins `verify`, `seed_session` and `aggregate_metrics` in `RESERVED_MCP_TOOL_NAMES`, which is what that set is for. Found by the existing `test_mcp_auto_exposure` suite after rebasing. Added an assertion next to the lifecycle tests so the constraint is stated where the hook is, not only where exposure is computed. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
The sweeper called `close_session(BaseCloseSessionRequest())` with an empty request. Served over HTTP the session comes from the cookie, but the sweeper has no request to read one from, so an override was never told which session to release: the loop logged a reclaim, dropped the id from its own tracking, and released nothing. The backstop for a killed trainer did not work. `BaseCloseSessionRequest` now carries an optional `session_id`. The HTTP route resolves it from the cookie before delegating, so `close_session` keeps one signature and both callers reach the same code. The existing test asserted only that the hook fired, recording a literal string rather than the session, which is why this passed. It now records `body.session_id` and asserts the sweeper names the session it reclaims; without the fix it fails with `[None] == ['idle']`. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
…one-shot release Reworks the lifecycle against the contract NVIDIA-NeMo#2609 now owns. The previous shape released a session but could not address one whose seed response was lost, and put HTTP identity inside the environment's cleanup method. The caller creates `_ng_session_id` before the first seed transmission, so the id survives a response that never arrives; the provider's own handle cannot serve here because it is learned from that response. A separate close capability is stored only as a digest and authorizes releasing exactly that one session when the cookie is gone. Seeding deduplicates on the caller's id. The same payload replays the stored response instead of allocating again, a different payload is a 409, and a released id keeps a bounded tombstone so a seed delayed in the network cannot recreate what was already torn down. That last rule is not spelled out in the design: replaying the stored response would hand the caller a session that no longer exists, so the id is treated as spent. `_close_session_endpoint` owns identity, conflict checks, idempotency and the response; `release_session(session_id, reason)` owns only release. The hook is deliberately not named `close_session` — `GymnasiumServer`, TALES and OpenAir already define that name with a different signature, and a base method sharing it would silently override theirs. A migration bridge is one line and is tested. Release runs at most once per session: the first closer creates the task and concurrent closers await it, so provider cleanup does not run per caller. A failed release reports `release_failed` and leaves the session retryable. An unknown id and an already-released one answer identically, so the endpoint cannot be used to probe whether another rollout's session exists. There is no `not_found` status for the same reason, which differs from the five listed in the issue body and follows the design document. Expiry no longer trusts idle time alone. A session with handlers in flight is skipped, since a rollout can sit in a model call for minutes without touching the resources server, and an absolute lifetime that activity does not suppress covers a handler that has hung. Agent-side orchestration, the bounded cancellation shield, the cleanup status header and the collector metadata are a follow-up, per "Agent and resources-server adoption are separate" in the design. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
99fc941 to
a0f7e02
Compare
Closes #2609
/seed_sessionallocates a browser, container, sandbox or provider lease; nothing releases it. A rollout that fails, times out, is cancelled or loses its seed response leaves that resource alive, and a retry allocates another while the first stays unreachable.Identity comes from the caller
The caller creates
_ng_session_idbefore the first seed transmission, so the id survives a response that never arrives. The provider's own handle cannot serve here — it is learned from that response, which is exactly what goes missing. A separate_ng_session_close_tokenis stored only as a digest and authorizes releasing exactly that one session when the cookie is gone.Seeding deduplicates
The transport may repeat a seed it is not sure was delivered.
The last rule is not spelled out in the design. Replaying the stored response would hand the caller a session that no longer exists, so a released id is treated as spent.
The adapter owns everything that is not release
The hook is deliberately not named
close_session.GymnasiumServer, TALES and OpenAir already define that name with a different signature, and a base method sharing it would silently override theirs. Migration is one line, and is tested:We hit a version of that collision already:
close_sessionas a typed POST route meant MCP auto-exposure published it as a model-callable tool, so a policy could have ended its own episode's resources.Release runs once
The first closer creates the release task and concurrent closers await it, so provider cleanup does not run per caller. A failed release reports
release_failedwith 503 and leaves the session retryable rather than marking it closed.An unknown id and an already-released one answer identically. There is no
not_foundstatus — that would let the endpoint be used to probe whether another rollout's session exists. This follows the close-session design rather than the five outcomes listed in #2609's body.Expiry does not trust idle time alone
A rollout can sit in a model call for minutes without touching the resources server, so a session with handlers in flight is skipped. An absolute lifetime that activity does not suppress covers the case the first rule cannot: a handler that has hung.
active_session()marks a handler in flight and refuses to start one once release has begun, so close cannot tear a resource down while verification is still reading it. The base uses it on its own routes; broad adoption across environment tool routes is #3037.Scope
Agent-side orchestration, the bounded cancellation shield,
X-NeMo-Gym-Cleanup-Statusand the collector's_ng_run_info.cleanupare a follow-up, per "Agent and resources-server adoption are separate" in the design. The TTL mechanism stays here as the reference implementation; #3037 owns adoption across stateful servers.Tests
27 cases over seven groups: seed replay does not allocate twice, payload conflict is 409, a delayed seed cannot resurrect a tombstone; the four terminal outcomes, a cookie/id conflict releasing neither, unknown and already-closed responses being byte-identical, ten concurrent closes releasing once, a failed release staying retryable; capability match, mismatch, id-alone rejection, and the token never reaching the record; idle expiry, busy suppression, absolute lifetime overriding busy, tombstone cleanup; handler counting and refusal after close; the base not defining
close_sessionand the bridge working; route registration andrelease_sessionnever reaching the model.uv run --extra dev pytest tests/unit_tests -q→ 4367 passed.test_e2b_providerandtest_enroot_providerfail on a cleanupstream/maintoo.