fix(observability): restore Relay metrics without Anthropic context reentry - #73120
Conversation
… metrics" Signed-off-by: Alex Fournier <afournier@nvidia.com>
Signed-off-by: Alex Fournier <afournier@nvidia.com>
|
@jquesnelle The replacement is ready for review. I reproduced the exact error from |
|
@jquesnelle Follow-up: the remaining live Anthropic validation gap is now closed. A real native Anthropic streamed Hermes turn through Relay against |
|
@mnajafian-nv Tagging for visibility: #67607 was merged, then reverted in #73053 after Anthropic streaming calls began failing with a This PR reapplies the original Relay/shared-metrics change on current |
Signed-off-by: Alex Fournier <afournier@nvidia.com>
|
Caught this branch up with upstream The new upstream abort-race tests also exposed a semantic overlap beyond the conflict: Relay was masking provider stream-close failures, which could allow a client with a checked-out/broken connection to be cached. Explicit Relay stream close now surfaces that provider failure once, allowing both chat and Codex paths to poison the reuse slot correctly; normal exhaustion and destructor cleanup remain fail-safe. Validation on head
|
Signed-off-by: Alex Fournier <afournier@nvidia.com>
Incident triageFor visibility, here is the blameless readout for the Anthropic regression from #67607 and the follow-up work in this PR. Timeline
What failedThe streaming adapter captured one
The fix keeps the caller's values but enters a fresh ImpactThe observed production impact was native Anthropic streaming while a Relay managed-execution consumer was active; those calls failed before delivering a response. Because the unsafe code was in the generic managed stream adapter, other managed streaming providers had the same latent re-entry risk, although Anthropic is the provider we reproduced and live-verified. Non-streaming calls used independent context copies and were not affected. Calls on the no-consumer bypass, unsupported/no-wheel platforms, and tool execution were also outside this failure path. This was a runtime availability regression, not a telemetry privacy or data-integrity incident. Why green validation missed itAll required checks on #67607 were green, including all eight Python slices and E2E. The issue was missing coverage, not a skipped or broken runner. The context regression added with The PR's size and repeated main merges made final-head review harder, but a merge conflict did not introduce this bug; the unsafe context reuse was already present in a normal feature commit and survived the merges unchanged. Recovery and safeguardsThe revert was the right containment action and landed quickly. The replacement now has:
Focused validation is For future core transport changes, the practical rule should be: test each native provider with its real SDK and mocked transport through the managed runtime, rerun that contract gate after the final main merge, and record any manual live canary against the exact head being proposed for merge. Live API calls should remain confirmation rather than the required CI gate. |
Signed-off-by: Alex Fournier <afournier@nvidia.com>
020a115 to
956fc87
Compare
|
hey @afourniernv we merged this but are not getting the following errors in the logs: |
|
Addressed in #73493, which has been merged. The fix was validated against the released NeMo Relay native binding and live Anthropic Messages, OpenAI Chat Completions, and OpenAI Responses endpoints, with 142 focused regression tests passing. |
…ay-anthropic-context fix(observability): restore Relay metrics without Anthropic context reentry
What does this PR do?
Reapplies the Relay runtime and shared-metrics work from #67607 after its revert in #73053, with the Anthropic streaming regression fixed.
The regression came from reusing one captured
contextvars.Contextacross provider iteration, Relay collection, stream cleanup, and finalization. Those callbacks can overlap or nest, and Python rejects entering the sameContexttwice. Streaming callbacks now enter a fresh copy of the captured context while preserving the intended context values.This PR adds a deterministic test that reproduced the exact production error on the reverted merge commit. It also adds an end-to-end test using the real Anthropic SDK parser against deterministic mock SSE, through Hermes' worker thread and Relay managed execution.
Related Issue
Restores #67607 after the regression-driven revert in #73053.
Type of Change
Changes Made
main.Context.tests/agent/test_relay_llm.py.tests/run_agent/test_streaming.py.How to Test
.venv/bin/python -m pytest tests/agent/test_relay_llm.py tests/run_agent/test_streaming.py -q..venv/bin/python scripts/smoke_nemo_relay_shared_metrics.py --hermes-repo .and verify the SQLite counters and schema-validated export.HERMES_HOME.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A.
Screenshots / Logs
Before the fix, the new regression reproduced the production failure on merge commit
1470022:Validation on the replacement branch with released
nemo-relay==0.6.0:git diff --checkare clean.Live-provider validation used credentials only from the process environment and isolated temporary
HERMES_HOMEdirectories:nvidia/nemotron-3-nano-30b-a3breturnedRELAY_METRICS_LIVE_OKin one API call.claude-haiku-4-5-20251001returnedANTHROPIC_RELAY_LIVE_OKin one API call.provider_family=direct,model_family=claude,locality=remote, andoutcome=success, plus successful task start/finish counters.The native Anthropic production path that originally regressed has therefore been exercised against Anthropic's live endpoint on this PR head.