Fix SDK session runtime lifecycle - #16
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPython session handling now uses native runtime handles for start/invoke/stop flows, and the Hermes SDK adapter now loads prior conversation state from SessionDB using runtime ids. Documentation and tests were updated to match the new session lifecycle. ChangesRuntime session lifecycle
Sequence Diagram(s)Native session lifecyclesequenceDiagram
participant FabricClient
participant _native
participant Session
FabricClient->>_native: start_runtime(plan_json)
_native-->>FabricClient: runtime
FabricClient->>Session: construct(plan, runtime)
Session->>_native: invoke_runtime(plan_json, runtime_json, request_json)
_native-->>Session: result
Session->>_native: stop_runtime(plan_json, runtime_json)
_native-->>Session: stop result
Hermes runtime historysequenceDiagram
participant run_hermes_sdk
participant SessionDB
participant AIAgent
run_hermes_sdk->>SessionDB: load_runtime_history(session_id)
SessionDB-->>run_hermes_sdk: conversation_history
run_hermes_sdk->>AIAgent: initialize(session_id, session_db, platform="fabric")
run_hermes_sdk->>AIAgent: run_conversation(conversation_history)
AIAgent-->>run_hermes_sdk: output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
AjayThorve
force-pushed
the
ajay/fix-session-runtime-lifecycle
branch
from
June 24, 2026 19:58
0cdae6d to
eaf558a
Compare
AjayThorve
marked this pull request as ready for review
June 24, 2026 19:59
Collaborator
Author
|
@coderabbitai can you review |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/nemo_fabric/client.py (1)
560-562: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccept an explicit empty transcript update.
The doc says
messagesupdates when the adapter returns a messages list, butand messageskeeps the previous transcript when the runtime intentionally returns[]. That leavessession.messagesstale after a transcript reset.Proposed fix
messages = output.get("messages") - if isinstance(messages, list) and messages: + if isinstance(messages, list): self._messages = deepcopy(messages)🤖 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 `@python/src/nemo_fabric/client.py` around lines 560 - 562, The transcript update in client.py is skipping explicit empty lists because the current guard in the session update logic rejects falsy values, which leaves stale messages behind after a reset. Update the messages handling in the relevant session method that reads output["messages"] so it accepts any list, including [], and assigns deepcopy(messages) to self._messages whenever the adapter returns a list. Keep the existing type check around messages, but remove the truthiness condition so empty transcripts replace the previous state.
🧹 Nitpick comments (3)
python/tests/smoke_sdk_sessions.py (1)
125-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the cancel smoke checking native cleanup.
This test no longer retains the
FakeNative, so it misses whethercancel()actually callsstop_runtimeonce. That is the key lifecycle contract for SDK-side cleanup.Proposed test strengthening
async def cancel_when_idle_marks_cancelled() -> None: - session = _session(FakeNative()) + native = FakeNative() + session = _session(native) await session.cancel() + await session.cancel() assert session.status is SessionStatus.CANCELLED + assert native.stopped == 1🤖 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 `@python/tests/smoke_sdk_sessions.py` around lines 125 - 128, The cancel smoke test is no longer verifying SDK cleanup through the native layer, so strengthen cancel_when_idle_marks_cancelled by keeping a reference to FakeNative and asserting that Session.cancel() triggers stop_runtime exactly once. Use the existing _session helper and the Session.cancel method to locate the flow, and add the native-side assertion alongside the status check so the lifecycle contract is covered.tests/test_session.py (2)
348-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
start_config(...)session-path test.This file now covers
start(...),run(...), andrun_config(...), but the typed-config session entrypoint added alongside them is still untested. A regression inFabricClient.start_config()would miss CI even though the README now documents it as the session equivalent. Mirroring this case withawait client.start_config(config)and onesession.invoke(...)would close that gap.🤖 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 `@tests/test_session.py` around lines 348 - 357, Add a session-path test for the typed-config entrypoint by mirroring the existing NativeClient.run_config coverage in this test module. Create a new async test around FabricClient.start_config() using the same FakeNative/config setup, call await client.start_config(config), then exercise one session.invoke(...) and assert the expected runtime/session behavior so start_config() is covered alongside start(), run(), and run_config(). Reference the FabricClient.start_config method and the session.invoke path to keep the test easy to locate.
126-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the test double observe
start()'s plan inputs.
NativeClient.plan()returns_plan()unconditionally, so thestart(...)tests never verify that the production code forwarded the requestedpathandprofileinto planning. Delegating toFakeNative.plan(...)or asserting those arguments here would keep this suite from passing on a brokenFabricClient.start()call path.🤖 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 `@tests/test_session.py` around lines 126 - 135, The NativeClient test double currently ignores the inputs passed into plan(), so the start() tests do not verify that FabricClient.start forwards the requested path and profile into planning. Update NativeClient.plan to delegate to FakeNative.plan with the received path and profile, or add assertions on those arguments there, so the suite exercises the real call path through FabricClient.start and FakeNative.
🤖 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 `@adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py`:
- Around line 65-71: Move the resume-session alias resolution in
load_runtime_history() ahead of the session_db.get_session(session_id) early
return so the resolved session ID is checked first. Use
resolve_resume_session_id() to map the raw runtime_id to the actual session ID,
then perform the get_session() guard against that resolved ID before loading
history. Update the existing load_runtime_history() flow so resumed Fabric
sessions can hydrate even when the original session row is missing.
In `@crates/fabric-python/src/lib.rs`:
- Around line 161-190: Release the GIL around the blocking runtime lifecycle
calls in the PyO3 wrappers: `start_runtime`, `invoke_runtime`, and
`stop_runtime` currently call `fabric_core::start_runtime`,
`fabric_core::invoke_runtime`, and `fabric_core::stop_runtime` directly, which
can block Python threads during subprocess work. Update these `#[pyfunction]`
bodies to execute the `fabric_core` calls inside `py.allow_threads(...)` (or the
equivalent current PyO3 API) while keeping the existing parsing and JSON
conversion logic outside that block.
In `@python/src/nemo_fabric/client.py`:
- Around line 520-528: The session cleanup path in the cancel/stop flow makes
failed cancellation unrecoverable because the finally block in the method
handling _stop_runtime() always sets SessionStatus.CANCELLED even when cleanup
fails. Update the logic around _stop_runtime() so the status is only set to
CANCELLED after a successful stop, or move to a retryable cleanup state on
failure, and preserve the ability for stop()/cancel() to retry rather than
returning early.
- Around line 530-535: `Client.stop()` currently allows `_stop_runtime()` to run
while `invoke_runtime()` may still be active, which can let a late result be
absorbed after the session is stopped. Add a guard in `stop()` to detect an
in-flight turn and either reject the stop or delegate to `cancel()` instead of
proceeding. Use the existing `stop()`, `cancel()`, `invoke_runtime()`,
`_stop_runtime()`, and `SessionStatus` symbols to keep the behavior consistent
with the session state machine.
- Around line 664-665: The cleanup call in the `invoke_runtime` flow is
overwriting the original failure because `stop_runtime` is executed in the
`finally` block without protecting an in-flight exception. Update the
`invoke_runtime`/`finally` logic in `client.py` so that `stop_runtime` errors
are suppressed or handled separately when `invoke_runtime` has already raised,
and only surface the cleanup failure when there was no prior invoke error.
---
Outside diff comments:
In `@python/src/nemo_fabric/client.py`:
- Around line 560-562: The transcript update in client.py is skipping explicit
empty lists because the current guard in the session update logic rejects falsy
values, which leaves stale messages behind after a reset. Update the messages
handling in the relevant session method that reads output["messages"] so it
accepts any list, including [], and assigns deepcopy(messages) to self._messages
whenever the adapter returns a list. Keep the existing type check around
messages, but remove the truthiness condition so empty transcripts replace the
previous state.
---
Nitpick comments:
In `@python/tests/smoke_sdk_sessions.py`:
- Around line 125-128: The cancel smoke test is no longer verifying SDK cleanup
through the native layer, so strengthen cancel_when_idle_marks_cancelled by
keeping a reference to FakeNative and asserting that Session.cancel() triggers
stop_runtime exactly once. Use the existing _session helper and the
Session.cancel method to locate the flow, and add the native-side assertion
alongside the status check so the lifecycle contract is covered.
In `@tests/test_session.py`:
- Around line 348-357: Add a session-path test for the typed-config entrypoint
by mirroring the existing NativeClient.run_config coverage in this test module.
Create a new async test around FabricClient.start_config() using the same
FakeNative/config setup, call await client.start_config(config), then exercise
one session.invoke(...) and assert the expected runtime/session behavior so
start_config() is covered alongside start(), run(), and run_config(). Reference
the FabricClient.start_config method and the session.invoke path to keep the
test easy to locate.
- Around line 126-135: The NativeClient test double currently ignores the inputs
passed into plan(), so the start() tests do not verify that FabricClient.start
forwards the requested path and profile into planning. Update NativeClient.plan
to delegate to FakeNative.plan with the received path and profile, or add
assertions on those arguments there, so the suite exercises the real call path
through FabricClient.start and FakeNative.
🪄 Autofix (Beta)
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: 6ee54338-5cc7-486b-924e-339cd2e6cadb
📒 Files selected for processing (16)
README.mdadapters/hermes-sdk/README.mdadapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pycrates/fabric-python/src/lib.rsexamples/session_quickstart.pypython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/_native.pyipython/src/nemo_fabric/client.pypython/tests/smoke_environment_handle.pypython/tests/smoke_native_sdk.pypython/tests/smoke_sdk_sessions.pypython/tests/smoke_typed_config.pytests/fixtures/hermes-shim-agent/adapters/hermes-shim/src/nemo_fabric_test_adapters/hermes_shim/adapter.pytests/smoke_hermes_session.pytests/test_hermes_sdk_adapter.pytests/test_session.py
💤 Files with no reviewable changes (2)
- python/src/nemo_fabric/init.py
- examples/session_quickstart.py
dagardner-nv
approved these changes
Jun 24, 2026
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/nemo_fabric/client.py (1)
520-536: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBlock new turns while cleanup is in progress.
cancel()andstop()await_stop_runtime()while the session still reportsACTIVE. Once a cancelledinvoke()clears_current_task—or whenstop()starts from idle—anotherinvoke()can enter and callinvoke_runtimeagainst a runtime that is already being stopped. Add a closing state/flag checked byinvoke(), and clear it if_stop_runtime()fails so cleanup remains retryable.Suggested direction
class Session: def __init__( @@ self._status = SessionStatus.ACTIVE self._current_task: asyncio.Task[Any] | None = None + self._closing = False @@ if self._status is not SessionStatus.ACTIVE: raise RuntimeError(f"cannot invoke a {self._status.value} session") + if self._closing: + raise RuntimeError("cannot invoke while session is closing") if self._current_task is not None: raise RuntimeError( "session is already running a turn; turns are ordered (one at a time)" @@ if self._status is not SessionStatus.ACTIVE: return + if self._closing: + return + self._closing = True task = self._current_task - if task is not None and not task.done() and task is not asyncio.current_task(): - task.cancel() - await self._stop_runtime() - self._status = SessionStatus.CANCELLED + try: + if task is not None and not task.done() and task is not asyncio.current_task(): + task.cancel() + await self._stop_runtime() + self._status = SessionStatus.CANCELLED + finally: + self._closing = False @@ if self._status is SessionStatus.ACTIVE: + if self._closing: + return task = self._current_task if task is not None and not task.done() and task is not asyncio.current_task(): raise RuntimeError("cannot stop while a turn is in flight; use cancel()") - await self._stop_runtime() - self._status = SessionStatus.STOPPED + self._closing = True + try: + await self._stop_runtime() + self._status = SessionStatus.STOPPED + finally: + self._closing = False🤖 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 `@python/src/nemo_fabric/client.py` around lines 520 - 536, Add a closing/in-progress state guard in the session lifecycle so new turns cannot start while `cancel()` or `stop()` is awaiting `_stop_runtime()`. Update `invoke()` to check this flag before calling `invoke_runtime`, and set it at the start of `cancel()` and `stop()` in `python/src/nemo_fabric/client.py` alongside `SessionStatus` handling. If `_stop_runtime()` fails, make sure the flag is cleared so the cleanup path remains retryable, and keep the existing `SessionStatus.CANCELLED` / `SessionStatus.STOPPED` transitions only after successful shutdown.
🤖 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.
Outside diff comments:
In `@python/src/nemo_fabric/client.py`:
- Around line 520-536: Add a closing/in-progress state guard in the session
lifecycle so new turns cannot start while `cancel()` or `stop()` is awaiting
`_stop_runtime()`. Update `invoke()` to check this flag before calling
`invoke_runtime`, and set it at the start of `cancel()` and `stop()` in
`python/src/nemo_fabric/client.py` alongside `SessionStatus` handling. If
`_stop_runtime()` fails, make sure the flag is cleared so the cleanup path
remains retryable, and keep the existing `SessionStatus.CANCELLED` /
`SessionStatus.STOPPED` transitions only after successful shutdown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 86f1669e-9c9a-40e8-b5ba-992dcef6a828
📒 Files selected for processing (6)
adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pycrates/fabric-python/src/lib.rspython/src/nemo_fabric/client.pypython/tests/smoke_sdk_sessions.pytests/test_hermes_sdk_adapter.pytests/test_session.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_hermes_sdk_adapter.py
- adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py
- crates/fabric-python/src/lib.rs
- python/tests/smoke_sdk_sessions.py
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
This was referenced Jun 25, 2026
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
2 participants
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
start/invoke/stop.Streamis a buffered wrapper over invoke;cancelis SDK-side cooperative cleanup over stop.runtime_idand resume via HermesSessionDB.Validation
cargo testcargo check -p fabric-pythoncargo fmt --all --checkpython3 -m pytest tests/test_hermes_sdk_adapter.py tests/test_session.py -qpython3 python/tests/smoke_sdk_sessions.pyRUN_FABRIC_HERMES_INTEGRATION=1 "$HERMES_PYTHON" tests/smoke_hermes_session.pySummary by CodeRabbit
FabricSessionUnsupportedErrorfrom the package’s public re-exports.