fix(observability): NeMo Relay plugin-config teardown failure-safety + post-finalize guard (follow-up to #41551) - #41961
Closed
kshitijk4poor wants to merge 3 commits into
Conversation
Clear NeMo Relay plugin-config observability only after the last active Hermes session finalizes. Use the plugin's async-safe awaitable helper for both initialize and clear so session rotation remains safe under active event loops. Disable the direct ATIF fallback when plugins.toml already owns the ATIF exporter lifecycle to avoid duplicate trajectory export on finalization.
… succeeds Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
…fe and guard against post-finalize resurrection Two follow-up hardening fixes on top of the bundled NeMo Relay observability plugin lifecycle work: 1. clear()-failure no longer strands the runtime. _clear_plugins_toml flipped _plugin_config_initialized/_plugin_config_needs_reinit only AFTER _resolve_awaitable(clear()) returned. If the relay's clear() raised (caught+logged in close_session), the flags stayed initialized=True / needs_reinit=False, so no later session ever reinitialized plugins.toml or activated the direct fallbacks. Flip the flags in a finally so a failed clear still re-arms reinit on the next session start. 2. ensure_session no longer resurrects a finalized session. Any hook that funnels through ensure_session after the terminal close_session for an id (on_session_end is the named example) re-initialized plugins.toml and pushed an unpopped scope, silently re-arming an exporter for an ended session. Track a bounded set of recently-finalized ids; a stray hook for a finalized id returns a detached, unstored state (no reinit, no scope push). An explicit on_session_start (create=True) clears the marker so a genuine restart of the same id still works. Adds three regression tests; the first two fail against the pre-fix code. Behavior-preserving for all existing paths (full plugins suite green).
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Two follow-up hardening fixes on top of the bundled NeMo Relay observability plugin lifecycle work in #41551, found while reviewing that PR. Stacks on
#41551(branched from its head).1.
clear()-failure no longer strands the runtime_clear_plugins_tomlflipped_plugin_config_initialized/_plugin_config_needs_reinitonly after_resolve_awaitable(clear())returned.close_sessioncatches and logs a raisingclear(), so on failure the flags stayedinitialized=True / needs_reinit=False— and no later session would ever reinitialize plugins.toml or activate the direct fallbacks. The exporter was silently dead for the rest of the process.Fix: flip the flags in a
finally, so a partial/failed clear still re-arms reinit (or the direct fallback) on the next session start.2.
ensure_sessionno longer resurrects a finalized sessionAny hook that funnels through
ensure_sessionafter the terminalclose_sessionfor an id re-initialized plugins.toml and pushed a scope that was never popped — silently re-arming an exporter for an ended session (and leaking the scope).on_session_endis the named example.Fix: track a bounded set of recently-finalized session ids. A stray hook for a finalized id returns a detached, unstored
_SessionState(no reinit, no scope push). An expliciton_session_start(create=True) clears the marker, so a genuine restart of the same id still works.Verification
_resolve_awaitablerepro: drove both failure modes — clear-failure recovers (reinit fires on next start), stray post-finalize hook produces no resurrection and balancedscope.push/scope.pop.tests/plugins/full suite green (1089 passed), ruff clean. Behavior-preserving for all existing paths (24 prior nemo_relay tests still pass).Type of Change
Related
Follow-up to #41551 (and its parent #38232). Branched from #41551's head — merge that first, or treat these two commits as fixups to fold in.