fix(langchain): close finished scopes from the top of the stack down - #755
Conversation
github-actions
Bot
added
size:M
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (14)**/*📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,go,js,ts}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Files:
**/*.{rs,py,js,jsx,ts,tsx,go,c,h,cc,cpp,md,toml,yml,yaml,sh}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{rs,py,js,mjs,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
python/tests/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.py📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/{test,tests}/**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,py,go,js,jsx,ts,tsx,c,h,html,md,mdx,toml}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{toml,md,rs,py}📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
python/**/*.py📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Files:
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (19)📓 Common learnings📚 Learning: 2026-08-11T19:49:02.958ZApplied to files:
📚 Learning: 2026-08-11T17:33:57.356ZApplied to files:
📚 Learning: 2026-07-17T17:36:18.512ZApplied to files:
📚 Learning: 2026-07-17T02:50:34.317ZApplied to files:
📚 Learning: 2026-08-07T22:42:38.991ZApplied to files:
📚 Learning: 2026-08-07T22:42:38.991ZApplied to files:
📚 Learning: 2026-08-07T22:42:38.991ZApplied to files:
📚 Learning: 2026-08-07T22:42:38.991ZApplied to files:
📚 Learning: 2026-07-27T20:27:55.340ZApplied to files:
📚 Learning: 2026-07-28T21:12:11.017ZApplied to files:
📚 Learning: 2026-08-04T01:50:26.470ZApplied to files:
📚 Learning: 2026-05-21T22:49:35.949ZApplied to files:
📚 Learning: 2026-07-28T03:31:11.365ZApplied to files:
📚 Learning: 2026-07-28T20:07:33.786ZApplied to files:
📚 Learning: 2026-06-26T15:15:34.329ZApplied to files:
📚 Learning: 2026-07-28T03:31:28.749ZApplied to files:
📚 Learning: 2026-08-07T13:13:11.790ZApplied to files:
📚 Learning: 2026-07-28T21:11:43.864ZApplied to files:
🪛 Ruff (0.16.1)python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py[warning] 185-185: Use Convert to (UP017) [warning] 560-560: Dynamically typed expressions (typing.Any) are disallowed in (ANN401) [warning] 563-563: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (4)
WalkthroughThe LangChain callback handler now defers out-of-order scope closures, preserves completion data and timestamps, and drains completed scopes in stack order. Tests cover concurrent sibling runs, stack ownership, cleanup, output snapshots, and failure handling. ChangesLangChain scope closure handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LangGraph
participant NemoRelayCallbackHandler
participant ScopeStack
LangGraph->>NemoRelayCallbackHandler: start concurrent branches
NemoRelayCallbackHandler->>ScopeStack: open parent and sibling scopes
LangGraph->>NemoRelayCallbackHandler: complete fast branch
NemoRelayCallbackHandler->>ScopeStack: defer non-top sibling closure
LangGraph->>NemoRelayCallbackHandler: complete slow branch
NemoRelayCallbackHandler->>ScopeStack: close top sibling scope
NemoRelayCallbackHandler->>ScopeStack: drain deferred sibling scope
NemoRelayCallbackHandler->>ScopeStack: close parent scope
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@python/nemo_relay/integrations/langchain/callbacks.py`:
- Line 147: Replace datetime.timezone.utc with datetime.UTC at
python/nemo_relay/integrations/langchain/callbacks.py lines 147-147 in the
_PendingPop construction and
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py lines
184-184 for a_ended_by, preserving the existing UTC timestamp behavior.
- Around line 156-169: The _drain_pending_pops loop currently rescans the entire
pending queue after each removal, causing quadratic close attempts for wide
fan-out. Update _drain_pending_pops to retry only the tail pending entry whose
handle can become closable after a removal, while preserving deferred entries
and removing terminal outcomes.
- Around line 171-189: Update _close_scope to use _logger.exception for the
non-deferred scope.pop failure while leaving the debug logging unchanged.
Preserve the existing _OUT_OF_ORDER_CLOSE message classification and its
documented test coverage, and track a follow-up to replace this
integration-level message coupling with a public typed error or ordering-check
API from core.
- Around line 137-154: Synchronize shared handler state transitions in the
callback close flow around _scope_handles, _pending_pops, _close_scope, and
_drain_pending_pops so concurrent worker threads cannot race. Preserve each
run’s owning scope-stack context when deferring or replaying a close, ensuring
queued closes are executed against that stack rather than another thread’s
context and are not discarded as FAILED. Add a regression test using one shared
handler with worker-thread callbacks that verifies all scopes close correctly.
In `@python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py`:
- Around line 143-146: Replace both RuntimeError-swallowing try/except blocks
around nemo_relay.scope.pop(request) with contextlib.suppress(RuntimeError),
adding the required import. Preserve the existing explanatory comment above the
with statement near the second occurrence.
- Around line 258-260: Remove the unused handler parameter from
test_relay_still_reports_out_of_order_closes_the_way_the_handler_detects; retain
the test’s direct nemo_relay.scope behavior and autouse isolated_scope_stack
fixture unchanged.
- Around line 47-52: Rename the fixture function isolated_scope_stack to
isolated_scope_stack_fixture and configure the decorator with
name="isolated_scope_stack", preserving its autouse behavior and existing
scope-stack setup.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 581eff4c-e336-4c51-a3ef-fff08d78679c
📒 Files selected for processing (3)
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (19)
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
**/*: Use release tags in raw Rust-compatible SemVer without a leadingv; tags such asv0.1.0are prohibited.
Use branch prefixesfeat/,fix/,docs/,test/, orrefactor/according to the change purpose.
Every commit in a pull request must include a DCOSigned-off-by:sign-off.
Before submitting a pull request, ensure pre-commit hooks, relevant tests, target-specific builds, documentation updates, and a rebase on the latestmainare complete.
Use commit messages in the formtype: short description, with a valid type and a first line under 72 characters.Run the prescribed plugin validation commands, including fixture building, focused Rust and Python package tests, integration tests, documentation checks, and the broader
validate-changematrix for broad runtime or public API changes.
**/*: Keep observability changes scoped, surface assumptions, and define focused validation before editing.
Run affected Rust tests and just test-rust for event-field changes; run just test-python, just test-go, and just test-node when binding-native configuration or lifecycle changes; update docs and examples in the same branch.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: Keep FFI and Python, Go, and Node.js binding configuration objects and subscriber/exporter methods aligned with the core observability configuration and lifecycle semantics.
Preserve complete sanitized LLM request input and annotations when enable_full_payloads is enabled, while retaining credential removal and sanitizers.
Use each exporter's documented flush and deregister order before shutdown in observability examples and implementations.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,jsx,ts,tsx,go,c,h,cc,cpp,md,toml,yml,yaml,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Keep SPDX headers on source, documentation, scripts, and configuration files; the project is Apache-2.0.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Use
snake_casenaming in Rust and Python.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,mjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the existing Tokio-based asynchronous model and callback/future lifetimes; do not unexpectedly block or hide async work in bindings.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage for Python binding and wrapper changes with the Python test suite.
python/tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock, usingspecwhen necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it in aconftest.pyfile instead of repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Run focused pytest tests first when the affected area is known, and run the full suite withjust test-pythonbefore review.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Lint Python with Ruff using rule setsE,F,W, andI.
Format Python with the Ruff formatter, using a 120-character line length and double quotes.
Runtyfor Python type checking.
Use Pythonsnake_casenaming conventions.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}: Run tests for every language affected by a change; changes to the core Rust crate require tests across all bindings.
UseSONAR_IGNORE_START/SONAR_IGNORE_ENDonly for documented false positives, keep ignored blocks minimal, explain them with a comment, and obtain reviewer sign-off.
Preserve the layered architecture in which Rust provides the core runtime and C FFI, PyO3, and NAPI provide bindings that mirror the full API surface.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/{test,tests}/**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the appropriate test files for each affected language binding.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h,html,md,mdx,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include the appropriate SPDX copyright and Apache-2.0 license header in every source file.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{toml,md,rs,py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Treat plugin Relay compatibility as normal SemVer; use
>=0.5,<1.0in examples unless a plugin intentionally declares a narrower range.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/**/*.py
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Python SDK tests belong under
python/tests, not under source directories.
python/**/*.py: Format changed Python wrapper and test files withuv run ruff format python python/plugin.
Runuv run ruff format python python/pluginafter changing Python wrapper or test files.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep Python wrapper modules under
python/nemo_relay/; the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/integrations/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python integrations must use public framework or plugin APIs.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
🪛 Ruff (0.16.1)
python/nemo_relay/integrations/langchain/callbacks.py
[warning] 147-147: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 187-187: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
[warning] 143-146: Use contextlib.suppress(RuntimeError) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(RuntimeError): ...
(SIM105)
[warning] 184-184: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 246-251: Use contextlib.suppress(RuntimeError) instead of try-except-pass
(SIM105)
[warning] 259-259: Unused function argument: handler
(ARG001)
🔇 Additional comments (4)
python/nemo_relay/integrations/langchain/callbacks.py (2)
23-48: LGTM!
59-59: LGTM!python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py (1)
335-357: 📐 Maintainability & Code QualityNo fixture change is needed.
subscribed_eventsis defined once inpython/tests/conftest.pyand shared through pytest fixture discovery.> Likely an incorrect or invalid review comment.python/tests/integrations/langchain_tests/test_callbacks.py (1)
116-116: 🗄️ Data Integrity & IntegrationNo signature change is needed.
scope.pop,_native.pyi, and the PyO3 binding all accepttimestamp. The callback passes a timezone-awaredatetime.> Likely an incorrect or invalid review comment.
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
The reported single-stack failure is fixed. One additional telemetry-fidelity issue remains inline.
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
As an alternative to retrying queued handles and classifying exceptions, could we consider an owner-stack-aware, top-driven state machine?
Store:
- each run’s
handle - owning
ScopeStack - completion payload
- metadata
- timestamp
When a run ends, mark it complete and drain only its owning stack:
- inspect
scope.get_handle() - close it when the top UUID belongs to a completed run
- repeat until the top is still running or is not owned by the handler
This would preserve LIFO ordering, prevent cross-stack queue loss, and avoid depending on exception-message matching.
I would also add a regression test using one shared handler across two isolated stacks, plus Ajay’s callback-time output snapshot test.
9c01b49 to
fb379ea
Compare
SandyChapman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/nemo_relay/integrations/langchain/callbacks.py (1)
137-157: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_pop_scopecan now raise into LangChain.
on_chain_startwraps its whole body intry/exceptand logs.on_chain_endandon_chain_errorcall_pop_scopeunguarded._prepare_lc_payloads(output)on Line 153 runs user-supplied output through serialization and can raise, andgetattr/dict writes run outside any guard. Before this change, the failure surface at end time was thescope.popcall, which the old code caught. Now a serialization failure propagates out of the callback and can fail the chain run.Wrap the body so telemetry failures stay non-fatal, consistent with
on_chain_start.🛡️ Proposed fix
def _pop_scope( self, run_id: UUID, *, output: dict[str, typing.Any] | None = None, metadata: nemo_relay.Json | None = None ) -> None: - handle = self._scope_handles.pop(run_id, None) - stack = self._scope_stacks.pop(run_id, None) - if handle is None: - return - - scope_uuid = getattr(handle, "uuid", None) - if scope_uuid is None: - _logger.error("NeMo Relay: scope handle carries no uuid; cannot close it") - return - - self._completed[scope_uuid] = _CompletedScope( - handle=handle, - stack=stack, - output=_prepare_lc_payloads(output) if output is not None else None, - metadata=metadata, - ended_at=datetime.datetime.now(datetime.UTC), - ) - self._close_completed_scopes(stack) + try: + handle = self._scope_handles.pop(run_id, None) + stack = self._scope_stacks.pop(run_id, None) + if handle is None: + return + + scope_uuid = getattr(handle, "uuid", None) + if scope_uuid is None: + _logger.error("NeMo Relay: scope handle carries no uuid; cannot close it") + return + + self._completed[scope_uuid] = _CompletedScope( + handle=handle, + stack=stack, + output=_prepare_lc_payloads(output) if output is not None else None, + metadata=metadata, + ended_at=datetime.datetime.now(datetime.UTC), + ) + self._close_completed_scopes(stack) + except Exception: + _logger.exception("NeMo Relay: recording a completed scope failed")Add a test that makes
_prepare_lc_payloadsraise and asserts the callback returns normally.🤖 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/nemo_relay/integrations/langchain/callbacks.py` around lines 137 - 157, Wrap the full body of _pop_scope, including handle lookup, UUID access, completed-scope assignment, and _prepare_lc_payloads, in exception handling so telemetry failures are logged and never propagate to LangChain, matching on_chain_start behavior. Add a test that forces _prepare_lc_payloads to raise and verifies _pop_scope returns normally.
🤖 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 `@python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py`:
- Around line 285-311: Bound or explicitly account for orphaned completions
retained by the handler’s _completed collection. Update the scope-completion
logic exercised by test_a_scope_closed_out_of_band_does_not_block_other_closes
to evict entries when their owning stack is gone or enforce a finite retention
cap, while preserving correct closure behavior; alternatively, document the
expected upper bound and operational monitoring requirement in the test and
related implementation.
In `@python/tests/integrations/langchain_tests/test_callbacks.py`:
- Around line 19-52: Update the local pop function in _make_mock_nemo_relay to
assert that the supplied handle is the current top-of-stack object before
removing it, while preserving the root-scope guard. Use identity comparison with
stack[-1] so wrong-handle closes fail the mock and handle is no longer unused.
In `@python/tests/integrations/langgraph_tests/test_langgraph_integration.py`:
- Around line 9-10: Rename the unused state parameters in the graph node
functions around the reducer and two-branch topology (including the functions at
the referenced lines) to indicate they are intentionally unused, such as using
the project’s established underscore convention, while preserving the graph
behavior and signatures required for invocation.
- Around line 244-263: Strengthen
test_parallel_fan_out_leaves_the_enclosing_scope_closable by asserting that the
callback handler actually opened the expected branch scopes during
graph.ainvoke. Use the existing subscribed_events fixture and assert lifecycle
scope events for the branch nodes, rather than relying only on the returned
branches and restored handle; do not use empty dictionaries as evidence of scope
creation.
---
Outside diff comments:
In `@python/nemo_relay/integrations/langchain/callbacks.py`:
- Around line 137-157: Wrap the full body of _pop_scope, including handle
lookup, UUID access, completed-scope assignment, and _prepare_lc_payloads, in
exception handling so telemetry failures are logged and never propagate to
LangChain, matching on_chain_start behavior. Add a test that forces
_prepare_lc_payloads to raise and verifies _pop_scope returns normally.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: a143baef-a633-41aa-9f38-3c9c5aa0e5a4
📒 Files selected for processing (4)
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (19)
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
**/*: Use release tags in raw Rust-compatible SemVer without a leadingv; tags such asv0.1.0are prohibited.
Use branch prefixesfeat/,fix/,docs/,test/, orrefactor/according to the change purpose.
Every commit in a pull request must include a DCOSigned-off-by:sign-off.
Before submitting a pull request, ensure pre-commit hooks, relevant tests, target-specific builds, documentation updates, and a rebase on the latestmainare complete.
Use commit messages in the formtype: short description, with a valid type and a first line under 72 characters.Run the prescribed plugin validation commands, including fixture building, focused Rust and Python package tests, integration tests, documentation checks, and the broader
validate-changematrix for broad runtime or public API changes.
**/*: Keep observability changes scoped, surface assumptions, and define focused validation before editing.
Run affected Rust tests and just test-rust for event-field changes; run just test-python, just test-go, and just test-node when binding-native configuration or lifecycle changes; update docs and examples in the same branch.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: Keep FFI and Python, Go, and Node.js binding configuration objects and subscriber/exporter methods aligned with the core observability configuration and lifecycle semantics.
Preserve complete sanitized LLM request input and annotations when enable_full_payloads is enabled, while retaining credential removal and sanitizers.
Use each exporter's documented flush and deregister order before shutdown in observability examples and implementations.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,jsx,ts,tsx,go,c,h,cc,cpp,md,toml,yml,yaml,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Keep SPDX headers on source, documentation, scripts, and configuration files; the project is Apache-2.0.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Use
snake_casenaming in Rust and Python.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,mjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the existing Tokio-based asynchronous model and callback/future lifetimes; do not unexpectedly block or hide async work in bindings.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage for Python binding and wrapper changes with the Python test suite.
python/tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock, usingspecwhen necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it in aconftest.pyfile instead of repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Run focused pytest tests first when the affected area is known, and run the full suite withjust test-pythonbefore review.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Lint Python with Ruff using rule setsE,F,W, andI.
Format Python with the Ruff formatter, using a 120-character line length and double quotes.
Runtyfor Python type checking.
Use Pythonsnake_casenaming conventions.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}: Run tests for every language affected by a change; changes to the core Rust crate require tests across all bindings.
UseSONAR_IGNORE_START/SONAR_IGNORE_ENDonly for documented false positives, keep ignored blocks minimal, explain them with a comment, and obtain reviewer sign-off.
Preserve the layered architecture in which Rust provides the core runtime and C FFI, PyO3, and NAPI provide bindings that mirror the full API surface.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/{test,tests}/**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the appropriate test files for each affected language binding.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h,html,md,mdx,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include the appropriate SPDX copyright and Apache-2.0 license header in every source file.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{toml,md,rs,py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Treat plugin Relay compatibility as normal SemVer; use
>=0.5,<1.0in examples unless a plugin intentionally declares a narrower range.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/**/*.py
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Python SDK tests belong under
python/tests, not under source directories.
python/**/*.py: Format changed Python wrapper and test files withuv run ruff format python python/plugin.
Runuv run ruff format python python/pluginafter changing Python wrapper or test files.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep Python wrapper modules under
python/nemo_relay/; the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/integrations/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python integrations must use public framework or plugin APIs.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
🪛 Ruff (0.16.1)
python/tests/integrations/langchain_tests/test_callbacks.py
[warning] 31-31: Missing return type annotation for private function push
(ANN202)
[warning] 31-31: Missing type annotation for **kwargs
(ANN003)
[warning] 41-41: Missing return type annotation for private function pop
Add return type annotation: None
(ANN202)
[warning] 41-41: Unused function argument: handle
(ARG001)
[warning] 41-41: Missing type annotation for **kwargs
(ANN003)
[warning] 41-41: Unused function argument: kwargs
(ARG001)
python/tests/integrations/langgraph_tests/test_langgraph_integration.py
[warning] 226-226: Unused function argument: state
(ARG001)
[warning] 230-230: Unused function argument: state
(ARG001)
python/nemo_relay/integrations/langchain/callbacks.py
[warning] 46-46: Dynamically typed expressions (typing.Any) are disallowed in _current_scope_stack
(ANN401)
[warning] 155-155: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 159-159: Dynamically typed expressions (typing.Any) are disallowed in stack
(ANN401)
[warning] 189-189: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
[warning] 192-192: Dynamically typed expressions (typing.Any) are disallowed in _current_scope_handle
(ANN401)
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
[warning] 184-184: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 504-504: Unused lambda argument: args
(ARG005)
[warning] 504-504: Unused lambda argument: kwargs
(ARG005)
🔇 Additional comments (8)
python/nemo_relay/integrations/langchain/callbacks.py (3)
155-155: Ruff UP017 still applies here. Usedatetime.UTCinstead ofdatetime.timezone.utc. The same change is needed atpython/tests/integrations/langchain_tests/test_callbacks_scope_stack.pyLine 184.
159-190: Ownership fix looks correct; the G201 lint is still open.The drain now keys completions by scope UUID, checks stack identity before reading the active context, and closes only the current top. That removes the cross-stack discard reported earlier, and the loop is now O(n) rather than a rescan.
Line 189 still triggers Ruff G201: use
_logger.exception("NeMo Relay: scope.pop failed").
23-53: LGTM!python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py (4)
184-184: Ruff UP017: usedatetime.UTChere. Same change aspython/nemo_relay/integrations/langchain/callbacks.pyLine 155.
514-564: 🩺 Stability & Availability | ⚡ Quick winOwnership coverage is good; worker-thread coverage is still missing.
test_two_concurrent_invocations_share_one_handlercovers the reported asyncio failure and reproduces the interleaving deterministically. It does not cover the second half of the earlier concern:run_inline=Truedoes not bind one handler to one thread, and_scope_handles,_scope_stacks, and_completedare mutated without synchronization. Add a test that drives one shared handler from aThreadPoolExecutor, each thread on its own scope stack, and asserts every request scope closes.
1-110: LGTM!
113-283: LGTM!Also applies to: 314-511
python/tests/integrations/langchain_tests/test_callbacks.py (1)
10-10: LGTM!Also applies to: 133-133, 155-155, 214-214
fb379ea to
1dbd05d
Compare
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/nemo_relay/integrations/langchain/callbacks.py (1)
153-168: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_pop_scopecan now raise into the LangChain callback.
on_chain_endandon_chain_errorcall_pop_scopewithout a guard. Inside_pop_scope,_prepare_lc_payloads(output)runs outside anytry. The previous implementation performed serialization inside the guarded pop path, so a serialization failure was logged and swallowed. Now it propagates out of the callback and can fail the user's chain run.
on_chain_startstill swallows its exceptions, so the two paths are inconsistent.Wrap the completion-recording body in the same guard style used by
on_chain_start.🛡️ Proposed fix
handle = self._scope_handles.pop(run_id, None) stack = self._scope_stacks.pop(run_id, None) if handle is None: return - self._completed[handle.uuid] = _CompletedScope( - handle=handle, - stack=stack, - output=_prepare_lc_payloads(output) if output is not None else None, - metadata=metadata, - ended_at=datetime.datetime.now(datetime.UTC), - ) + try: + prepared_output = _prepare_lc_payloads(output) if output is not None else None + except Exception: + _logger.exception("NeMo Relay: preparing chain output failed") + prepared_output = None + + self._completed[handle.uuid] = _CompletedScope( + handle=handle, + stack=stack, + output=prepared_output, + metadata=metadata, + ended_at=datetime.datetime.now(datetime.UTC), + ) self._close_completed_scopes(stack)🤖 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/nemo_relay/integrations/langchain/callbacks.py` around lines 153 - 168, Update _pop_scope to wrap completion-recording operations, including _prepare_lc_payloads(output), _CompletedScope creation, and _close_completed_scopes(stack), in the same exception-swallowing and logging guard used by on_chain_start. Ensure serialization or close failures do not propagate through on_chain_end or on_chain_error, while preserving handle and stack removal.
🤖 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/nemo_relay/integrations/langchain/callbacks.py`:
- Around line 153-168: Update _pop_scope to wrap completion-recording
operations, including _prepare_lc_payloads(output), _CompletedScope creation,
and _close_completed_scopes(stack), in the same exception-swallowing and logging
guard used by on_chain_start. Ensure serialization or close failures do not
propagate through on_chain_end or on_chain_error, while preserving handle and
stack removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 894f28bf-8759-402f-a9fd-7d97b9ad6ca2
📒 Files selected for processing (4)
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (19)
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
**/*: Use release tags in raw Rust-compatible SemVer without a leadingv; tags such asv0.1.0are prohibited.
Use branch prefixesfeat/,fix/,docs/,test/, orrefactor/according to the change purpose.
Every commit in a pull request must include a DCOSigned-off-by:sign-off.
Before submitting a pull request, ensure pre-commit hooks, relevant tests, target-specific builds, documentation updates, and a rebase on the latestmainare complete.
Use commit messages in the formtype: short description, with a valid type and a first line under 72 characters.Run the prescribed plugin validation commands, including fixture building, focused Rust and Python package tests, integration tests, documentation checks, and the broader
validate-changematrix for broad runtime or public API changes.
**/*: Keep observability changes scoped, surface assumptions, and define focused validation before editing.
Run affected Rust tests and just test-rust for event-field changes; run just test-python, just test-go, and just test-node when binding-native configuration or lifecycle changes; update docs and examples in the same branch.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: Keep FFI and Python, Go, and Node.js binding configuration objects and subscriber/exporter methods aligned with the core observability configuration and lifecycle semantics.
Preserve complete sanitized LLM request input and annotations when enable_full_payloads is enabled, while retaining credential removal and sanitizers.
Use each exporter's documented flush and deregister order before shutdown in observability examples and implementations.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,jsx,ts,tsx,go,c,h,cc,cpp,md,toml,yml,yaml,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Keep SPDX headers on source, documentation, scripts, and configuration files; the project is Apache-2.0.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Use
snake_casenaming in Rust and Python.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,mjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the existing Tokio-based asynchronous model and callback/future lifetimes; do not unexpectedly block or hide async work in bindings.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage for Python binding and wrapper changes with the Python test suite.
python/tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock, usingspecwhen necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it in aconftest.pyfile instead of repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Run focused pytest tests first when the affected area is known, and run the full suite withjust test-pythonbefore review.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Lint Python with Ruff using rule setsE,F,W, andI.
Format Python with the Ruff formatter, using a 120-character line length and double quotes.
Runtyfor Python type checking.
Use Pythonsnake_casenaming conventions.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}: Run tests for every language affected by a change; changes to the core Rust crate require tests across all bindings.
UseSONAR_IGNORE_START/SONAR_IGNORE_ENDonly for documented false positives, keep ignored blocks minimal, explain them with a comment, and obtain reviewer sign-off.
Preserve the layered architecture in which Rust provides the core runtime and C FFI, PyO3, and NAPI provide bindings that mirror the full API surface.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/{test,tests}/**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the appropriate test files for each affected language binding.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h,html,md,mdx,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include the appropriate SPDX copyright and Apache-2.0 license header in every source file.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{toml,md,rs,py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Treat plugin Relay compatibility as normal SemVer; use
>=0.5,<1.0in examples unless a plugin intentionally declares a narrower range.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/**/*.py
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Python SDK tests belong under
python/tests, not under source directories.
python/**/*.py: Format changed Python wrapper and test files withuv run ruff format python python/plugin.
Runuv run ruff format python python/pluginafter changing Python wrapper or test files.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
python/tests/integrations/langchain_tests/test_callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep Python wrapper modules under
python/nemo_relay/; the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/integrations/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python integrations must use public framework or plugin APIs.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
🪛 Ruff (0.16.1)
python/tests/integrations/langchain_tests/test_callbacks.py
[warning] 31-31: Missing return type annotation for private function push
(ANN202)
[warning] 31-31: Missing type annotation for **kwargs
(ANN003)
[warning] 41-41: Missing return type annotation for private function pop
Add return type annotation: None
(ANN202)
[warning] 41-41: Missing type annotation for **kwargs
(ANN003)
[warning] 41-41: Unused function argument: kwargs
(ARG001)
python/tests/integrations/langgraph_tests/test_langgraph_integration.py
[warning] 226-226: Unused function argument: state
(ARG001)
[warning] 230-230: Unused function argument: state
(ARG001)
python/nemo_relay/integrations/langchain/callbacks.py
[warning] 166-166: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 200-200: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
[warning] 184-184: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 507-507: Unused lambda argument: args
(ARG005)
[warning] 507-507: Unused lambda argument: kwargs
(ARG005)
🔇 Additional comments (10)
python/nemo_relay/integrations/langchain/callbacks.py (4)
166-166: Ruff UP017: usedatetime.UTC. Still reported at Line 166 here and at Line 184 inpython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py.
170-200: Ruff G201: use_logger.exceptionat Line 200. The drain logic itself is correct: the entry is removed from_completedbefore the pop attempt, so a failure is not retried and the loop always terminates.
23-50: LGTM!
53-68: LGTM!python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py (3)
184-184: Ruff UP017: usedatetime.UTChere as well. Same rule violation aspython/nemo_relay/integrations/langchain/callbacks.pyLine 166.
285-314: The retained-completion behavior is still unbounded._completedhas no eviction path for a scope closed out of band. The docstring claims the retention is bounded, but a long-lived shared handler accumulates one_CompletedScopeper orphaned scope, each holding serialized output.
63-110: LGTM!Also applies to: 113-158, 207-253, 256-282, 317-346, 349-385, 388-422, 425-458, 461-486, 489-514, 517-567
python/tests/integrations/langgraph_tests/test_langgraph_integration.py (2)
226-232: Ruff ARG001: thestateparameters inslowandfastare still unused. Rename them to_state.
9-10: LGTM!Also applies to: 213-214, 244-272
python/tests/integrations/langchain_tests/test_callbacks.py (1)
10-10: LGTM!Also applies to: 20-53, 135-135, 157-157, 216-216
1dbd05d to
b0281d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@python/nemo_relay/integrations/langchain/callbacks.py`:
- Line 177: Update the timestamp construction near ended_at to use datetime.UTC
instead of datetime.timezone.utc, and change the error logging statements in the
relevant callback handlers around _logger at the two reported locations to use
_logger.exception(...) while preserving their existing messages and exception
context.
In `@python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py`:
- Line 185: Update the datetime construction in the test to use datetime.UTC
instead of datetime.timezone.utc, preserving the existing timezone-aware
timestamp behavior and resolving Ruff UP017.
- Around line 453-458: Both worker-thread tests ignore join timeouts. In
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py lines
453-458 and 482-486, add a worker.is_alive() liveness assertion immediately
after each worker.join(timeout=10), before the existing failures/_completed and
created assertions, using the message “the worker thread did not finish”.
In `@python/tests/integrations/langgraph_tests/test_langgraph_integration.py`:
- Around line 338-373: Strengthen test_a_failing_node_still_closes_its_siblings
by subscribing the callback handler to lifecycle events, flushing queued
subscriber events after the scope cleanup, and asserting scope.start and
scope.end were emitted for both boom and survivor. Keep the existing failure and
scope-restoration assertions, and verify the assertions use the flushed events
so both branch lifecycles are proven.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 7b1bf521-40af-4d67-bc14-12a537c55818
📒 Files selected for processing (3)
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Check / Run
- GitHub Check: Preview docs
🧰 Additional context used
📓 Path-based instructions (19)
{crates/python/src/py_api/mod.rs,python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go,crates/node/src/api/**/*.rs}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update the language-native bindings for every exposed surface in Python, Go, and Node.js.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
{python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go}
📄 CodeRabbit inference engine (.agents/skills/add-binding-feature/SKILL.md)
Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
**/*
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*: Format changed files with the language-native formatter before the final lint/test pass.
If dynamic plugin behavior changed, usemaintain-dynamic-pluginsand include the native SDK, worker protocol, Python SDK, docs, packaging, and Codecov surfaces in the validation plan.
If code changes alter APIs, bindings, commands, paths, packaging behavior, observability/adaptive semantics, or documented best practices, update any dependent maintainer or consumer skills in the same branch.
During iteration, preferuv run pre-commit run --files <changed files...>.
Before review or handoff, runuv run pre-commit run --all-files.
**/*: Use release tags in raw Rust-compatible SemVer without a leadingv; tags such asv0.1.0are prohibited.
Use branch prefixesfeat/,fix/,docs/,test/, orrefactor/according to the change purpose.
Every commit in a pull request must include a DCOSigned-off-by:sign-off.
Before submitting a pull request, ensure pre-commit hooks, relevant tests, target-specific builds, documentation updates, and a rebase on the latestmainare complete.
Use commit messages in the formtype: short description, with a valid type and a first line under 72 characters.Run the prescribed plugin validation commands, including fixture building, focused Rust and Python package tests, integration tests, documentation checks, and the broader
validate-changematrix for broad runtime or public API changes.
**/*: Keep observability changes scoped, surface assumptions, and define focused validation before editing.
Run affected Rust tests and just test-rust for event-field changes; run just test-python, just test-go, and just test-node when binding-native configuration or lifecycle changes; update docs and examples in the same branch.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,ts}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If a language surface changed, always run that language's test target even when Rust core did not change.
**/*.{rs,py,go,js,ts}: Keep FFI and Python, Go, and Node.js binding configuration objects and subscriber/exporter methods aligned with the core observability configuration and lifecycle semantics.
Preserve complete sanitized LLM request input and annotations when enable_full_payloads is enabled, while retaining credential removal and sanitizers.
Use each exporter's documented flush and deregister order before shutdown in observability examples and implementations.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,ts,tsx,go,java,kt,swift}
📄 CodeRabbit inference engine (.agents/skills/add-middleware/SKILL.md)
Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,jsx,ts,tsx,go,c,h,cc,cpp,md,toml,yml,yaml,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Keep SPDX headers on source, documentation, scripts, and configuration files; the project is Apache-2.0.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py}
📄 CodeRabbit inference engine (AGENTS.md)
Use
snake_casenaming in Rust and Python.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,js,mjs,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the existing Tokio-based asynchronous model and callback/future lifetimes; do not unexpectedly block or hide async work in bindings.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/nemo_relay/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep Python wrapper modules under
python/nemo_relay/; the native extension is built fromcrates/pythonwithmaturin.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/nemo_relay/integrations/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python integrations must use public framework or plugin APIs.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.py: Lint Python with Ruff using rule setsE,F,W, andI.
Format Python with the Ruff formatter, using a 120-character line length and double quotes.
Runtyfor Python type checking.
Use Pythonsnake_casenaming conventions.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,py,go,js,jsx,ts,tsx,c,h}: Run tests for every language affected by a change; changes to the core Rust crate require tests across all bindings.
UseSONAR_IGNORE_START/SONAR_IGNORE_ENDonly for documented false positives, keep ignored blocks minimal, explain them with a comment, and obtain reviewer sign-off.
Preserve the layered architecture in which Rust provides the core runtime and C FFI, PyO3, and NAPI provide bindings that mirror the full API surface.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{rs,py,go,js,jsx,ts,tsx,c,h,html,md,mdx,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Include the appropriate SPDX copyright and Apache-2.0 license header in every source file.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/*.{toml,md,rs,py}
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Treat plugin Relay compatibility as normal SemVer; use
>=0.5,<1.0in examples unless a plugin intentionally declares a narrower range.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/**/*.py
📄 CodeRabbit inference engine (.agents/skills/maintain-dynamic-plugins/SKILL.md)
Python SDK tests belong under
python/tests, not under source directories.
python/**/*.py: Format changed Python wrapper and test files withuv run ruff format python python/plugin.
Runuv run ruff format python python/pluginafter changing Python wrapper or test files.
Files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
python/nemo_relay/**/*
⚙️ CodeRabbit configuration file
python/nemo_relay/**/*: Review Python wrapper changes for typed API consistency, contextvars-based scope isolation, async behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/nemo_relay/integrations/langchain/callbacks.py
python/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage for Python binding and wrapper changes with the Python test suite.
python/tests/**/*.py: Use pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock, usingspecwhen necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
If a fixture is needed in multiple test files, define it in aconftest.pyfile instead of repeating it.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a<fixture_name>_fixturefunction; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Run focused pytest tests first when the affected area is known, and run the full suite withjust test-pythonbefore review.
Files:
python/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
**/{test,tests}/**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the appropriate test files for each affected language binding.
Files:
python/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}
⚙️ CodeRabbit configuration file
{crates/**/tests/**,python/tests/**,go/nemo_relay/**/*_test.go}: Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.
Prefer assertions on lifecycle events, scope stacks, middleware ordering, and binding parity over shallow smoke tests.
Files:
python/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
🧠 Learnings (22)
📓 Common learnings
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Relay PR: 755
File: python/nemo_relay/integrations/langchain/callbacks.py:137-154
Timestamp: 2026-08-11T17:33:57.356Z
Learning: In `python/nemo_relay/integrations/langchain/callbacks.py`, `NemoRelayCallbackHandler` can be shared by concurrent async LangGraph invocations that each use a different `ScopeStack`. A deferred `_PendingPop` must retain its originating scope-stack context, and `_drain_pending_pops()` must retry only entries owned by the active scope stack. A handler-wide unpartitioned pending queue can otherwise retry a close on another context's stack, receive `NotFound`, remove it as terminal, and leave the owning scope open. Synchronization alone does not prevent this ownership failure.
Learnt from: SandyChapman
Repo: NVIDIA/NeMo-Relay PR: 755
File: python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py:285-314
Timestamp: 2026-08-11T19:49:02.958Z
Learning: In `python/nemo_relay/integrations/langchain/callbacks.py`, a deferred `_CompletedScope` cannot be safely classified as orphaned because `ScopeStack` and `ScopeHandle` do not provide weak-reference support or an API to determine whether a handle remains on its owning stack. Eviction or a finite retention cap can discard a valid deferred completion and recreate the LangChain scope leak. A runtime-side stack-membership or liveness accessor is required before safe orphan-completion eviction can be implemented.
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 462
File: python/nemo_relay/_context.py:20-28
Timestamp: 2026-07-17T17:36:18.512Z
Learning: In the Python SDK, `python/nemo_relay/_context.py::ensure_scope_stack()` may use `nemo_relay._native_scope_stack_active()` when `nemo_relay._scope_stack_var` is unset: `crates/core/src/api/runtime/scope_stack.rs::sync_thread_scope_stack()` deliberately updates only the thread-local handle and does not set the explicit-active flag. Thus this branch preserves explicitly bound worker-thread stacks without allowing a stack synchronized by another asyncio task to be treated as task ownership.
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 0
File: :0-0
Timestamp: 2026-07-28T21:11:43.864Z
Learning: In `crates/core/src/api/runtime/state.rs`, async tool and LLM middleware snapshot chains must isolate callback panics with `AssertUnwindSafe(...).catch_unwind()`: sanitizer failures/panics fail open by preserving the latest valid payload, while conditional-execution guardrails and request intercepts convert panics into `FlowError::Internal`.
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/add-middleware/SKILL.md:0-0
Timestamp: 2026-07-17T02:50:34.317Z
Learning: Applies to crates/core/src/api/runtime/state.rs : Add chain-execution helpers to `NemoRelayContextState`, following existing helpers such as `tool_sanitize_request_chain` or `tool_request_intercepts_chain`.
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-04T01:50:26.470Z
Learning: Scope stacks must remain hierarchical, always include a root scope, preserve parent-child event relationships and scope-local visibility, provide cleanup boundaries, and isolate concurrent requests.
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/maintain-dynamic-plugins/SKILL.md:0-0
Timestamp: 2026-08-07T22:42:18.141Z
Learning: Applies to crates/**/*.rs : Runtime helpers must cover marks, scopes, continuations, and isolated scope stacks.
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/add-middleware/SKILL.md:0-0
Timestamp: 2026-07-17T02:50:34.317Z
Learning: Applies to **/*.{rs,py,js,ts,tsx,go,java,kt,swift} : Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
📚 Learning: 2026-08-11T19:49:02.958Z
Learnt from: SandyChapman
Repo: NVIDIA/NeMo-Relay PR: 755
File: python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py:285-314
Timestamp: 2026-08-11T19:49:02.958Z
Learning: In `python/nemo_relay/integrations/langchain/callbacks.py`, a deferred `_CompletedScope` cannot be safely classified as orphaned because `ScopeStack` and `ScopeHandle` do not provide weak-reference support or an API to determine whether a handle remains on its owning stack. Eviction or a finite retention cap can discard a valid deferred completion and recreate the LangChain scope leak. A runtime-side stack-membership or liveness accessor is required before safe orphan-completion eviction can be implemented.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-11T17:33:57.356Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Relay PR: 755
File: python/nemo_relay/integrations/langchain/callbacks.py:137-154
Timestamp: 2026-08-11T17:33:57.356Z
Learning: In `python/nemo_relay/integrations/langchain/callbacks.py`, `NemoRelayCallbackHandler` can be shared by concurrent async LangGraph invocations that each use a different `ScopeStack`. A deferred `_PendingPop` must retain its originating scope-stack context, and `_drain_pending_pops()` must retry only entries owned by the active scope stack. A handler-wide unpartitioned pending queue can otherwise retry a close on another context's stack, receive `NotFound`, remove it as terminal, and leave the owning scope open. Synchronization alone does not prevent this ownership failure.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-17T17:36:18.512Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 462
File: python/nemo_relay/_context.py:20-28
Timestamp: 2026-07-17T17:36:18.512Z
Learning: In the Python SDK, `python/nemo_relay/_context.py::ensure_scope_stack()` may use `nemo_relay._native_scope_stack_active()` when `nemo_relay._scope_stack_var` is unset: `crates/core/src/api/runtime/scope_stack.rs::sync_thread_scope_stack()` deliberately updates only the thread-local handle and does not set the explicit-active flag. Thus this branch preserves explicitly bound worker-thread stacks without allowing a stack synchronized by another asyncio task to be treated as task ownership.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-08T21:17:00.650Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/add-binding-feature/SKILL.md:0-0
Timestamp: 2026-07-08T21:17:00.650Z
Learning: Applies to {python/nemo_relay/**/*.py,python/nemo_relay/**/*.pyi,go/nemo_relay/**/*.go} : Update language wrapper helpers such as Python wrapper modules, Python type stubs, and Go shorthand packages when the new behavior belongs in those helper layers.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-17T02:50:34.317Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/add-middleware/SKILL.md:0-0
Timestamp: 2026-07-17T02:50:34.317Z
Learning: Applies to crates/core/src/api/runtime/state.rs : Add chain-execution helpers to `NemoRelayContextState`, following existing helpers such as `tool_sanitize_request_chain` or `tool_request_intercepts_chain`.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.py
📚 Learning: 2026-07-27T20:27:55.340Z
Learnt from: fede-kamel
Repo: NVIDIA/NeMo-Relay PR: 554
File: crates/core/tests/integration/pipeline_tests.rs:1442-1515
Timestamp: 2026-07-27T20:27:55.340Z
Learning: In `crates/core/tests/integration/pipeline_tests.rs`, integration tests are serialized with `TEST_MUTEX` and begin with `reset_global()`, which replaces the whole `NemoRelayContextState`. Therefore, the file’s established trailing `deregister_subscriber(...).unwrap()` cleanup pattern does not leak subscribers between tests if a prior test panics; do not request a per-test RAII cleanup guard unless proposing a deliberate file-wide cleanup-pattern refactor.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-04T01:50:26.470Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-04T01:50:26.470Z
Learning: Scope stacks must remain hierarchical, always include a root scope, preserve parent-child event relationships and scope-local visibility, provide cleanup boundaries, and isolate concurrent requests.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-28T21:11:43.864Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 0
File: :0-0
Timestamp: 2026-07-28T21:11:43.864Z
Learning: In `crates/core/src/api/runtime/state.rs`, async tool and LLM middleware snapshot chains must isolate callback panics with `AssertUnwindSafe(...).catch_unwind()`: sanitizer failures/panics fail open by preserving the latest valid payload, while conditional-execution guardrails and request intercepts convert panics into `FlowError::Internal`.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.py
📚 Learning: 2026-06-26T15:15:34.329Z
Learnt from: teerthsharma
Repo: NVIDIA/NeMo-Relay PR: 0
File: :0-0
Timestamp: 2026-06-26T15:15:34.329Z
Learning: In NeMo-Relay adaptive topology-aware ACG, the intended convergence behavior is to detect stability from a reusable agent workflow's stable prompt prefix under the same learning key, while allowing variable task-specific suffixes; learning must reopen when the prompt topology changes. This clarification is relevant to reviews of `crates/adaptive/src/acg_learner.rs` and related convergence tests.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-28T21:11:43.864Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 0
File: :0-0
Timestamp: 2026-07-28T21:11:43.864Z
Learning: Queued event-publication assertions in NeMo Relay tests must call `flush_subscribers()` after the triggering lifecycle operation, including after scope-pop cleanup checks, because sanitizer and subscriber delivery occurs asynchronously on the dispatcher.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-07T13:13:11.790Z
Learnt from: afourniernv
Repo: NVIDIA/NeMo-Relay PR: 702
File: docs/about-nemo-relay/concepts/scopes.mdx:103-108
Timestamp: 2026-08-07T13:13:11.790Z
Learning: In the Rust runtime, `crates/core/src/api/tool.rs::tool_call_execute` and `crates/core/src/api/llm.rs::llm_call_execute` do not push `Tool` or `Llm` scopes. They create parent-linked lifecycle events by using `resolve_parent_uuid` and `with_active_event_uuid`; therefore, a managed tool or LLM call under an active `Agent` scope leaves the explicit scope stack unchanged.
Applied to files:
python/nemo_relay/integrations/langchain/callbacks.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-11T19:44:42.536Z
Learnt from: SandyChapman
Repo: NVIDIA/NeMo-Relay PR: 755
File: python/tests/integrations/langgraph_tests/test_langgraph_integration.py:9-10
Timestamp: 2026-08-11T19:44:42.536Z
Learning: In `python/tests/integrations/langgraph_tests/test_langgraph_integration.py`, LangGraph node functions passed to `StateGraph.add_node` must retain their `state` parameter name because renaming it to `_state` causes `ty` to reject the node signature. The repository Ruff configuration enables only `E`, `F`, `W`, and `I`, so `ARG001` unused-argument findings do not apply.
Applied to files:
python/tests/integrations/langgraph_tests/test_langgraph_integration.py
📚 Learning: 2026-07-17T02:50:34.317Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/add-middleware/SKILL.md:0-0
Timestamp: 2026-07-17T02:50:34.317Z
Learning: Applies to **/*.{rs,py,js,ts,tsx,go,java,kt,swift} : Add tests covering registration and duplicate names, deregistration and missing names, priority ordering, callback failure policy, scope-local inheritance and cleanup, event payload semantics, immutable mark and scope fields, and parity across affected bindings.
Applied to files:
python/tests/integrations/langgraph_tests/test_langgraph_integration.pypython/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-07T22:42:38.991Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/test-python-binding/SKILL.md:0-0
Timestamp: 2026-08-07T22:42:38.991Z
Learning: Applies to python/tests/**/*.py : Define fixtures using `pytest.fixture(name="<fixture_name>"[, scope="<scope>"])` and a `<fixture_name>_fixture` function; specify `scope` only when it is not `function`.
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-07T22:42:38.991Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/test-python-binding/SKILL.md:0-0
Timestamp: 2026-08-07T22:42:38.991Z
Learning: Applies to python/tests/**/*.py : Prefer pytest fixtures over helper methods.
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-07T22:42:38.991Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/test-python-binding/SKILL.md:0-0
Timestamp: 2026-08-07T22:42:38.991Z
Learning: Applies to python/tests/**/*.py : If a fixture is needed in multiple test files, define it in a `conftest.py` file instead of repeating it.
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-08-07T22:42:38.991Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/test-python-binding/SKILL.md:0-0
Timestamp: 2026-08-07T22:42:38.991Z
Learning: Applies to python/tests/**/*.py : Prefix mocked class names with `mock`, not `fake`.
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-05-21T22:49:35.949Z
Learnt from: CR
Repo: NVIDIA/NeMo-Relay PR: 0
File: .agents/skills/prepare-pr/SKILL.md:0-0
Timestamp: 2026-05-21T22:49:35.949Z
Learning: Branch scope should be coherent and reviewable
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-28T03:31:11.365Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 564
File: crates/core/src/api/llm.rs:859-875
Timestamp: 2026-07-28T03:31:11.365Z
Learning: For NeMo Relay asynchronous middleware under RELAY-509, no implicit middleware timeout is permitted. FIFO publication barriers intentionally wait for delayed sanitizer work; pending completion-based callbacks must resolve, reject, or observe cancellation. This is documented behavior, not an availability-timeout defect.
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-28T20:07:33.786Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 571
File: crates/core/src/api/runtime/state.rs:996-1020
Timestamp: 2026-07-28T20:07:33.786Z
Learning: In NeMo Relay, RELAY-509 defines sanitizer callback failures as intentional fail-open behavior: sanitizer chains retain and publish the last valid event or payload snapshot, while logging the failure with callback context. This policy applies to event, tool request/response, and LLM request/response sanitizer chains and is documented in the public documentation and migration guide.
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
📚 Learning: 2026-07-28T03:31:28.749Z
Learnt from: willkill07
Repo: NVIDIA/NeMo-Relay PR: 564
File: crates/core/src/api/runtime/subscriber_dispatcher.rs:134-141
Timestamp: 2026-07-28T03:31:28.749Z
Learning: For RELAY-509 asynchronous middleware, `crates/core/src/api/runtime/subscriber_dispatcher.rs` intentionally uses FIFO publication barriers without an implicit timeout. `flush_subscribers` must wait for pending asynchronous publication work; pending completions are expected to resolve, reject, or observe cancellation rather than being released by a timeout.
Applied to files:
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
🪛 Ruff (0.16.1)
python/nemo_relay/integrations/langchain/callbacks.py
[warning] 177-177: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
[warning] 218-218: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
[warning] 236-236: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
python/tests/integrations/langgraph_tests/test_langgraph_integration.py
[warning] 226-226: Unused function argument: state
(ARG001)
[warning] 230-230: Unused function argument: state
(ARG001)
[warning] 284-284: Unused function argument: state
(ARG001)
[warning] 288-288: Unused function argument: state
(ARG001)
[warning] 292-292: Unused function argument: state
(ARG001)
[warning] 348-348: Unused function argument: state
(ARG001)
[warning] 350-350: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 352-352: Unused function argument: state
(ARG001)
[warning] 367-368: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py
[warning] 185-185: Use datetime.UTC alias
Convert to datetime.UTC alias
(UP017)
🔇 Additional comments (9)
python/tests/integrations/langgraph_tests/test_langgraph_integration.py (4)
9-10: LGTM!
213-241: LGTM!
244-272: LGTM!
275-336: LGTM!python/nemo_relay/integrations/langchain/callbacks.py (3)
8-10: LGTM!Also applies to: 24-71
80-87: LGTM!Also applies to: 128-129, 159-179
181-237: LGTM!python/tests/integrations/langchain_tests/test_callbacks_scope_stack.py (2)
1-46: LGTM!Also applies to: 49-112, 114-159, 162-206, 208-254, 257-283, 286-315, 318-346, 349-385, 388-422, 489-522, 525-539
133-133: 📐 Maintainability & Code QualityNo change needed.
subscribed_eventsis a function-scoped fixture inpython/tests/conftest.pyand registers before each test.> Likely an incorrect or invalid review comment.
b0281d9 to
4f8cd00
Compare
github-actions
Bot
added
size:XL
4f8cd00 to
d3dd594
Compare
Relay closes scopes strictly LIFO, but LangGraph schedules sibling chain runs as
concurrent asyncio tasks that share one scope stack, so two siblings can finish
in an order the stack rejects: A starts, B starts, A ends, B ends.
When that happened the handler lost the scope. `_pop_scope` removed the handle
from `_scope_handles` before attempting the pop, then swallowed the rejection, so
the scope stayed live on the stack with nothing left able to close it. It then
stayed current for everything that followed, and the caller's own enclosing scope
raised on exit:
RuntimeError: invalid argument: scope handle is not at the top of the stack
Downstream that reported fully successful agent work as failed (NVBug 6562846):
the enclosing scope belonged to a NeMo Fabric Deep Agents invocation, whose
adapter turned the teardown error into an invocation failure.
Track each run's handle. When a run ends, record its prepared output, metadata and
end timestamp as completed state, then close finished scopes from the top of the
active stack down: read the current handle, close it when its uuid belongs to a
completed run of ours, and stop as soon as the top is a run still going or a scope
this handler does not own.
Because only the current top is ever closed, the stack is never asked to accept a
close it would reject. There is no speculative pop, so there is no failure to
classify and no dependency on the runtime's error text. Ownership needs no separate
check either: a scope sits on exactly one stack, so its uuid can only be the current
top there. Comparing ScopeStack objects would not work in any case -- propagating a
stack to a worker thread yields a different Python wrapper for the same stack, with
no way to correlate the two -- and a handler completing a run on a worker thread has
to keep working.
Output is prepared before the run is dropped from the handler's map, and degrades to
None if it cannot be serialized. Serializing walks caller-supplied data and can fail
on its own -- a cyclic output raises RecursionError -- and a run removed with no
completion recorded is a scope nothing can ever close. Losing a payload costs one
scope's detail; letting the failure escape would cost the scope itself. End
timestamps are captured at callback time so a delayed close still reports when the
run ended.
A completion is deleted only once the stack has accepted the close. The runtime can
refuse before it mutates the stack, and the completion is the only record able to
close that scope.
Handler state is serialized: start, completion, top-check and pop are multi-step
transitions, and a top-check is only meaningful if the pop it authorizes cannot be
overtaken by another thread pushing onto the same stack. Reading the current handle
is guarded by a status check, since it would otherwise create a scope stack in a
context that had none.
Tests use a real scope stack. The existing handler tests mock nemo_relay, and the
mock accepted any pop in any order, so it could not observe the LIFO rule at all; it
now models the stack and rejects closing anything but the top. LangGraph tests drive
real graphs: a two-branch fan-out, a nested three-branch fan-out, and a failing node
completing through on_chain_error.
Reported and reviewed by @AjayThorve and @willkill07.
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
d3dd594 to
eeef8f7
Compare
A scope rebuilt from a PropagationContext carries the uuid of the scope it stands in for, so matching a queued completion on uuid alone could pop the stand-in: the pop succeeds, the completion is discarded, and the real scope is stranded on its own stack. Match on uuid and name together, which separates the two because the runtime names rebuilt scopes propagated-parent/propagated-root. Adds a regression test for the cross-stack close and one pinning those reserved names, so an upstream rename fails here instead of silently closing the wrong scope. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
|
/merge |
3 participants
Overview
What. When a LangChain chain run ends, the handler records it as completed and closes finished scopes from the top of the stack down, instead of attempting a close that the stack may reject and then abandoning the scope.
Why. Relay closes scopes strictly LIFO, but LangGraph schedules sibling chain runs as concurrent asyncio tasks that share one scope stack, so two siblings can finish in an order the stack rejects: A starts, B starts, A ends, B ends.
_pop_scoperemoved the handle from_scope_handlesbefore attempting the pop and then swallowed the rejection, so the scope stayed live on the stack with nothing left able to close it. It remained current for everything that followed, and the caller's own enclosing scope raised on exit:Downstream this reported fully successful agent work as failed (NVBug 6562846). The enclosing scope belonged to a NeMo Fabric Deep Agents invocation, whose adapter turned a teardown error into an invocation failure (since contained separately in NVIDIA/NeMo-Fabric#191) — 31 otherwise-successful agent-eval trials were scored as adapter failures in one regression run, including at parallelism 1.
Details
Each run's handle is tracked from
on_chain_start. When a run ends, its prepared output, metadata and end timestamp are recorded as completed state, and finished scopes are closed from the top of the active stack down: readscope.get_handle(), close it when its uuid belongs to a completed run of ours, stop as soon as the top is a run still going or a scope this handler does not own.Only the current top is ever closed, so the stack is never asked to accept a close it would reject. There is no speculative pop, hence no failure to classify and no dependency on the runtime's error text.
On "drain only the owning stack". Implemented, then removed, with evidence. A stack-identity check cannot be written against the public API:
set_thread_scope_stackgenuinely shares the stack (a push in the thread is visible outside it) but returns a different Python wrapper, andScopeStackexposes no id and only identity equality. A handler completing a run on a worker thread would therefore park every completion and strand every scope — the original bug, reintroduced by the guard. Dropping it also removes a strong reference to aScopeStackper retained completion. Covered bytest_a_run_completed_on_a_worker_thread_still_closes.Cross-stack safety is therefore structural, not an ownership guard: a completion is matched on the top scope's uuid and name. uuid alone is not sufficient — a stack rebuilt from a
PropagationContextre-creates a scope carrying an existing uuid, so a uuid-only match pops the stand-in, discards the completion and strands the real scope on its own stack (test_a_propagated_stack_does_not_consume_another_stacks_completion). The two are distinguishable because the runtime names rebuilt scopespropagated-parentandpropagated-root(scope_stack.rs), whichtest_propagated_stand_ins_keep_their_reserved_namespins so a rename fails loudly rather than silently closing the wrong scope. Residual: a chain run named exactly one of those two reserved names would still collide.@AjayThorve's findings. Output is prepared when the callback fires, so later mutation cannot change emitted telemetry. Cross-stack loss is structural rather than guarded, per above.
@willkill07's second review. Output is prepared before the run is dropped from the handler's map and degrades to
Noneon failure — serializing walks caller data and a cyclic output raisesRecursionError; a run removed with no completion recorded is a scope nothing can close. A completion is deleted only once the stack accepts the close, since the runtime can refuse before mutating the stack. Handler state transitions are serialized under a lock.Reading the current handle is guarded by
scope_stack_active(), which does not create a stack, so completing a run in a context without one no longer materialises an empty stack there.Where should the reviewer start?
_close_completed_scopes_lockedinpython/nemo_relay/integrations/langchain/callbacks.py— the loop that reads the top and stops as soon as it is not a completed run of ours. Note the locked/unlocked split:_pop_scopeholds the lock and calls the locked form, so the lock is never re-acquired on the normal path. That was a real fragility — an earlier revision nested the acquisition and only worked because the lock was reentrant.Then the tests, which use a real scope stack.
test_callbacks.pymocksnemo_relay, and its mock accepted any pop in any order, so it could not observe the LIFO rule this fix turns on; it now models the stack and rejects closing anything but the top.The LangGraph tests drive real compiled graphs with no stubbing: a two-branch fan-out, a nested three-branch fan-out with a subgraph, and a failing node completing through
on_chain_error.Testing
python/tests/integrations— 64 passed, 2 skipped.python/tests(excludingplugin/andtest_dynamic_plugin_host.py, which needjust build-test-plugin-fixtures) — 545 passed, 2 skipped.scope_stack_activeguard; and a handler that pushes no scopes at all.release/0.7was used as the control, and confirms the flip is caused by this change rather than by version skew.Remaining limitations
on_chain_error— still leaves its parent waiting. Unchanged by this PR, and better than an abandoned scope since the state is visible._scope_handlesentry, sync (non-async) LangGraph execution, and very wide fan-out as a performance question.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Bug Fixes
Tests