Skip to content

fix(langfuse): close root context during shutdown - #43677

Open
bkutasi wants to merge 1 commit into
NousResearch:mainfrom
bkutasi:fix/langfuse-shutdown-cleanup
Open

fix(langfuse): close root context during shutdown#43677
bkutasi wants to merge 1 commit into
NousResearch:mainfrom
bkutasi:fix/langfuse-shutdown-cleanup

Conversation

@bkutasi

@bkutasi bkutasi commented Jun 10, 2026

Copy link
Copy Markdown

What does this PR do?

Closes the Langfuse root trace context during plugin shutdown and adds an atexit handler to clean up traces still open when the Python interpreter exits. Previously the plugin manually entered the root context via root_ctx.__enter__() in _start_root_trace but never called root_ctx.__exit__(), leaving OpenTelemetry's active span context attached until interpreter teardown — at which point module globals may already be torn down. This caused noisy Exception ignored in: <generator object Langfuse._create_span_with_parent_context ...> and TypeError: isinstance() arg 2 must be a type... errors on exit.

Related Issue

No related issue; this is a bug fix identified from noisy shutdown logs.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/observability/langfuse/__init__.py
    • Added import atexit at module level.
    • Added _close_root_context(state) helper — reads state.root_ctx, sets it to None before calling __exit__ to guarantee single-close semantics, then calls root_ctx.__exit__(None, None, None) to detach the OpenTelemetry active span context.
    • Added _shutdown_active_traces() — iterates all open TraceState entries under the lock, ends any active observations and root spans, closes root contexts, and flushes the Langfuse client.
    • _finish_trace now calls _close_root_context(state) in its finally block so the context is always cleaned up on normal trace completion.
    • atexit.register(_shutdown_active_traces) is at module level (not inside register()) so the handler is registered as soon as the module is imported.
  • tests/plugins/test_langfuse_plugin.py
    • Added from types import SimpleNamespace.
    • Added TestRootContextCleanup test class with two test methods.

How to Test

  1. Run the focused test suite: pytest tests/plugins/test_langfuse_plugin.py::TestRootContextCleanup -v
  2. Run the plugin tests: pytest tests/plugins/test_langfuse_plugin.py -q — all 43 tests should pass.
  3. Run linters: ruff check plugins/observability/langfuse/__init__.py tests/plugins/test_langfuse_plugin.py and python -m py_compile plugins/observability/langfuse/__init__.py — both should pass.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've ran the full pytest tests/ -q suite (not run for this PR; see verification above)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux (Ubuntu 24.04, Python 3.x)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Before fix (noisy exceptions on interpreter shutdown):

Exception ignored in: <generator object Langfuse._create_span_with_parent_context at 0x...>
Traceback (most recent call last):
  File ".../opentelemetry/trace/__init__.py", line ...
TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union

After fix: clean shutdown, no exceptions.

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification comment from automated review

Reviewed the diff — this correctly addresses a real resource leak. The plugin calls root_ctx.__enter__() directly in _start_root_trace (bypassing with), so __exit__ is never invoked during normal operation. On interpreter shutdown, OpenTelemetry module globals may already be torn down, producing noisy exceptions from opentelemetry.trace.use_span.

The fix is well-structured:

  • _close_root_context is idempotent (sets state.root_ctx = None before exiting)
  • Called from both _finish_trace (normal path) and _shutdown_active_traces (atexit path)
  • atexit.register ensures orphaned traces are cleaned up even on abnormal exits
  • Error handling is fail-open (except Exception + _debug)
  • Tests cover both normal finish and atexit cleanup paths

No issues found. LGTM.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins labels Jun 10, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting a real lifecycle leak: current main still manually enters the Langfuse root context in plugins/observability/langfuse/__init__.py:639, :649, and :659 without an exit on normal completion.

Problems

  • Current main now also bounds orphaned trace state in _evict_stale_locked() (plugins/observability/langfuse/__init__.py:713-734). That path removes a state and calls only state.root_span.end() at :732; it does not exit the manually entered state.root_ctx. The proposed helper is not wired there, so long-running processes can retain the same context leak when eviction occurs.

Suggested changes

  • Invoke _close_root_context(state) from _evict_stale_locked() and add a forced-eviction test asserting a single context exit.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 14, 2026
@bkutasi
bkutasi force-pushed the fix/langfuse-shutdown-cleanup branch from c7728c9 to ef0213e Compare July 15, 2026 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants