Fix #615: OTel tracer leak from test_full_pipeline pollutes test_ping in full-suite runs - #630
Merged
Merged
Conversation
…der-Labs#615) full_stack (tests/integration/test_full_pipeline.py) reassigns the module-global tokenjam.sdk.agent._tracer to a tracer bound to its own local TracerProvider, then shuts that provider down and closes its DuckDB in teardown, without ever restoring the original _tracer. Since tests/integration collects before tests/unit, this leaks into every later test in the session. test_ping.py's record_llm_call() then routes spans through the dead TjSpanExporter/closed DB (raising "Connection already closed", logged as the "Span export failed" warning at provider.py:56) instead of the global provider that test_ping's own proof exporter is attached to — so intercepted spans are never actually captured and the 4 ping tests fail only in full-suite ordering. tests/agents/test_record_outcome.py already has the correct pattern for this hazard (save + restore the tracer around a local provider); full_stack just never applied it. This brings it in line.
Contributor
|
| Filename | Overview |
|---|---|
| tests/integration/test_full_pipeline.py | Saves the pre-test tracer and restores it during teardown, preventing fixture-local OTel state from leaking into later tests. |
Reviews (3): Last reviewed commit: "Merge branch 'main' into fix-issue-615" | Re-trigger Greptile
anilmurty
approved these changes
Jul 28, 2026
anilmurty
left a comment
Contributor
There was a problem hiding this comment.
Thanks @Axiya3749 — verified the root cause and your fix: the full_stack fixture rebinds the module-global agent_mod._tracer and never restores it, so a later test_ping routes through the dead exporter / closed DuckDB. Reproduced 4 failures on main → 0 with this patch. Clean, correct, mirrors the save/restore pattern already in test_record_outcome.py. Merging.
This was referenced Jul 28, 2026
Closed
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.
Fixes test isolation: an earlier test's leaked global state made 4 test_ping.py tests fail only when the full suite runs, training readers to ignore a red suite.
Summary
tokenjam.sdk.agent._tracerto its original value infull_stack's teardown instead of leaving it bound to a shut-down provider and closed DB.Root cause
full_stack(tests/integration/test_full_pipeline.py) rebinds the module-globalagent_mod._tracerto a tracer on its own localTracerProvider, then callsprovider.shutdown()anddb.close()in teardown — but never restores_tracer. Becausetests/integrationcollects beforetests/unitalphabetically, this leaks: test_ping.py'srecord_llm_call()calls then run through the deadTjSpanExporter/closed DuckDB (raising "Connection already closed", logged as "Span export failed" at provider.py:56) instead of the real global provider that test_ping's own proof exporter is attached to. The proof exporter never captures anything, sointerceptedis wronglyFalseand the 4 tests asserting on"intercepted a test span"/ interception-dependent output fail.Fix mirrors the existing correct pattern in
tests/agents/test_record_outcome.py: save the original tracer before rebinding, restore it in teardown, before the provider is shut down.Related issue
Closes #615
Tests / Verification
pytest tests/unit/test_ping.py -v— 10/10 passpytest tests/integration/test_full_pipeline.py -v— 11/11 passpytest tests/— full suite: the 4 previously-failing test_ping tests are fixed; confirmed via git-stash isolation that a pre-existing, unrelated failure intests/unit/test_deadweight.py(a Rich line-wrap issue in_render_deadweight, unrelated to OTel/tracer state) is present with or without this change, both standalone and in the full suiteWhat's NOT in this PR
tokenjam/source — this is purely a test-isolation fix intests/integration/test_full_pipeline.py.tests/unit/test_deadweight.py::test_render_deadweight_names_the_dead_serverremains failing — confirmed pre-existing and unrelated (fails standalone, independent of this fix and of test ordering). Separate root cause in the CLI renderer; not touched here.Checklist
pytest tests/unit/ tests/synthetic/ tests/agents/ tests/integration/)ruff check tokenjam/)mypy tokenjam/)tests/factories.py(not rawNormalizedSpan) — N/A, no spans added@anilmurtyas reviewer (or @-mentioned him above)@anilmurty