fix(nemo-agents): bound live Fabric sessions with an LRU cap - #1240
Draft
marcusds wants to merge 2 commits into
Draft
fix(nemo-agents): bound live Fabric sessions with an LRU cap#1240marcusds wants to merge 2 commits into
marcusds wants to merge 2 commits into
Conversation
A chat-completions call that carries no X-Nemo-Session-Id opens a session, and
opening a session starts a Fabric runtime. The caller never learns to close it —
the evaluator's generic agent target sends only a URL, a body and a JSONPath, so
it cannot send the session header or issue DELETE /v1/sessions/{id} — and until
now nothing reclaimed those sessions but the 30-minute idle sweep.
An evaluation job is exactly that shape: every task is a fresh request with no
session header. A 50-task run left 50 runtimes alive for half an hour after the
job finished.
Sessions are now capped. Opening one evicts the least recently used idle
sessions above max_live_sessions, defaulting to 8 to match the invocation cap,
so a saturated server keeps as many runtimes as it can concurrently use. Zero
disables the cap.
Eviction deliberately does not touch a session that is mid-invocation, so the
registry can sit above the cap while every session is busy rather than kill a
turn in flight, and it never evicts the session the caller just opened. A
failure to stop an evicted runtime is logged rather than failing the request
that triggered the eviction.
Capping rather than closing each stateless session on completion keeps the
multi-turn affordance intact. The only way to obtain a session ID is the header
returned on a headerless response, so closing those would leave both the
X-Nemo-Session-Id request path and DELETE /v1/sessions/{id} unreachable. An
evicted session behaves like an expired one: the next request with that ID gets
a 404 and can open a fresh session.
Signed-off-by: mschwab <mschwab@nvidia.com>
Contributor
|
The rationale for the cap belongs in the pull request, not above the constant. What is left is the one fact the code cannot show: a caller that sends no session header has no way to close what it opens. DEFAULT_MAX_LIVE_SESSIONS now derives from DEFAULT_MAX_CONCURRENT_INVOCATIONS rather than restating the tie in prose, so raising the invocation cap carries the session cap with it. The value is unchanged. The evict_over_capacity docstring drops to one line, matching every other method in the registry. That it skips a session mid-invocation is already legible from the invocation_lock check. Signed-off-by: mschwab <mschwab@nvidia.com>
marcusds
marked this pull request as ready for review
August 11, 2026 20:34
Contributor
📝 WalkthroughWalkthroughFabric now supports a configurable maximum number of live sessions. The CLI and serving settings validate and forward the limit. The session manager evicts least-recently-used idle sessions while preserving active sessions and the newly opened session. ChangesFabric live-session capacity
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant FabricSessionManager
participant FabricSessionRegistry
participant FabricRuntimeSession
CLI->>FabricSessionManager: Open a new session
FabricSessionManager->>FabricSessionRegistry: Register the session
FabricSessionManager->>FabricSessionRegistry: Evict idle sessions over capacity
FabricSessionRegistry-->>FabricSessionManager: Return evicted sessions
FabricSessionManager->>FabricRuntimeSession: Stop each evicted runtime
``
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 5</summary>
<details>
<summary>✅ Passed checks (5 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :--------------------------------------------------------------------------------------------------------- |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: adding an LRU cap for live Fabric sessions. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>📝 Generate docstrings</summary>
- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> Create stacked PR
- [ ] <!-- {"checkboxId": "3e1879ae-f29b-4d0d-8e06-d12b7ba33d98"} --> Commit on current branch
</details>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `fabric-live-session-cap/mschwab`
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-agents/src/nemo_agents_plugin/fabric/session_manager.py`:
- Around line 105-120: The session manager must re-run _evict_over_capacity
after invoke_session and stream_session release invocation_lock, so sessions
that become idle are evicted when the live-session cap was previously exceeded.
Add the enforcement to both release paths while preserving existing cleanup
behavior, and add a manager test covering busy sessions exceeding capacity
followed by lock release and eviction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c3ee158d-0c68-47ad-b2a8-38a80dffc830
📒 Files selected for processing (4)
plugins/nemo-agents/src/nemo_agents_plugin/fabric/server.pyplugins/nemo-agents/src/nemo_agents_plugin/fabric/session_manager.pyplugins/nemo-agents/src/nemo_agents_plugin/fabric/session_registry.pyplugins/nemo-agents/tests/unit/test_fabric_session_registry.py
Comment on lines
+105
to
+120
| await self._evict_over_capacity(keep=session.session_id) | ||
| return session | ||
|
|
||
| async def _evict_over_capacity(self, *, keep: str) -> None: | ||
| """Stop the least recently used idle sessions above the live-session cap.""" | ||
| evicted = await self._session_registry.evict_over_capacity( | ||
| max_sessions=self._max_live_sessions, | ||
| keep=keep, | ||
| ) | ||
| for session in evicted: | ||
| logger.info("Evicting idle Fabric session %s to stay within the live-session cap.", session.session_id) | ||
| try: | ||
| await self._stop_session(session) | ||
| except FabricSessionStopError: | ||
| logger.exception("Failed to stop evicted Fabric session %s.", session.session_id) | ||
|
|
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Re-enforce capacity after a session becomes idle.
_evict_over_capacity runs only after open_session. If all sessions are busy then, no victim is removed. After those invocations or streams release their locks, no code retries eviction. The registry can stay above max_live_sessions until another session opens or the idle timeout expires.
Run capacity enforcement after invoke_session and stream_session release invocation_lock. Add a manager test for this sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/nemo-agents/src/nemo_agents_plugin/fabric/session_manager.py` around
lines 105 - 120, The session manager must re-run _evict_over_capacity after
invoke_session and stream_session release invocation_lock, so sessions that
become idle are evicted when the live-session cap was previously exceeded. Add
the enforcement to both release paths while preserving existing cleanup
behavior, and add a manager test covering busy sessions exceeding capacity
followed by lock release and eviction.
marcusds
marked this pull request as draft
August 12, 2026 18:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A chat-completions call that carries no
X-Nemo-Session-Idopens a session, and opening a session starts a Fabric runtime. The caller never learns to close it, and until now nothing reclaimed those sessions but the 30-minute idle sweep.An evaluation job is exactly that shape. The evaluator's generic agent target sends only a URL, a Jinja body and a JSONPath — it has no way to set a request header or to call
DELETE /v1/sessions/{id}— so every task is a fresh headerless request. A 50-task run left 50 runtimes alive for half an hour after the job finished.Sessions are now capped: opening one evicts the least recently used idle sessions above
max_live_sessions.Related Issue
Follow-up to #1219, which moved Studio's eval target onto
/-/v1/chat/completionsand made this the shape every Fabric agent evaluation takes. Surfaced while reviewing that change; recorded there as a known limitation.Changes
FabricSessionRegistry.evict_over_capacity(max_sessions, keep)— removes least-recently-used first.FabricSessionManagertakesmax_live_sessionsand evicts after a successfulregisterinopen_session. The default derives fromDEFAULT_MAX_CONCURRENT_INVOCATIONSrather than restating it, so a saturated server keeps as many runtimes as it can concurrently use and raising the invocation cap carries the session cap with it.0disables the cap.FabricServingSettings.max_live_sessionswith the same non-negative validation as the other settings, plus a--max-live-sessionsflag on the packaged server.Three deliberate choices:
remove_expiredalready uses.Scope: the cap is per deployment, not per host
Worth knowing before judging the default. Each deployment runs its own Fabric server process, bound to one agent config —
_fabric_server_cli_argsin the container backend,_spawn_fabricin the subprocess one.create_fabric_serving_appbuilds exactly one registry and one manager per process, and the registry is, per its own docstring, process-local.So the hierarchy is agent → deployment → process → registry → sessions, and a session never spans agents. The cap bounds the pile-up per agent, which is the right unit because the leak is per agent, but it is not a machine-wide budget: N deployed Fabric agents can hold up to N × the cap, and two deployments of the same agent get one cap each. That is what the flag is for on a host running many agents.
A related consequence, independent of this change: session IDs are
uuid4and process-local, so an ID minted by one deployment 404s at another. The gateway proxies by agent name and takesrunning[0]with, per its own comment, "first-match, no load-balancing across running deployments" — so a client holding a session ID can be routed to a process that has never heard of it. Session reuse through the proxy is fragile on its own terms.Why a cap rather than closing each stateless session
Closing a headerless call's session once the response completes is the tighter fix for the leak, but it amputates multi-turn. The only way to obtain a session ID is the header returned on a headerless response, so closing those would leave both the
X-Nemo-Session-Idrequest path andDELETE /v1/sessions/{id}with no reachable way to get an ID. Nothing in the tree sends that header today — only the server's own tests — but the affordance is built, and removing it is a larger call than fixing the leak.A cap bounds the harm without touching the HTTP contract. An evicted session behaves exactly like an expired one: the next request carrying that ID gets a 404 and can open a fresh session.
Note that eviction is only triggered by
open_session. Traffic that only reuses existing sessions never runs a sweep, but it also never grows the registry, so the count cannot climb; decay for that case remains the idle sweep. This bounds growth, it is not a background reaper.This does not address the per-request runtime cold start, which is inherent to giving each eval task an isolated runtime — sharing one would let tasks contaminate each other.
Type of Change
Quality Gates
--help.Verification
Pull request title follows the repository's Conventional Commit format
Every commit includes an appropriate
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted tests pass, or tests are marked not applicable above
No secrets, API keys, or credentials are included
uv run pytestovertest_fabric_session_registry.py,test_fabric_session_manager.pyandtest_fabric_server.py— 60 passed, including four new registry tests: LRU order, keep-the-just-opened-session, never-evict-a-busy-session, and no-op both under the cap and when unlimited.uv run ruff check/ruff format --checkon the changed paths — clean.uv run --frozen ty check plugins/nemo-agents/src/nemo_agents_plugin/fabric— 6 diagnostics, all pre-existing and none on a line this branch touches (git blameputs them on imports authored 2026-07-28/29 plustranslator.py:125).Two caveats, stated rather than hidden:
--no-verify. Thetypre-commit hook reportsunused-ignore-commentfor the# ty: ignore[unresolved-import]on thenemo_fabricimports atsession_manager.py:31andsession_registry.py:15. Both lines predate this branch and are load-bearing in CI, which type-checks this plugin viaextra-pathswithout installing its deps; they only read as unused locally becausenemo_fabricis installed. Removing them would break CI. The hook surfaces them here only because it checks staged files and this branch stages those files.plugins/nemo-agentsunit suite has not been run end to end, only the three Fabric session and server test modules.Summary by CodeRabbit
New Features
--max-live-sessionsoption; setting it to zero allows unlimited sessions.Bug Fixes