Skip to content

Improve Langfuse tracing reliability - #45048

Open
zhuxungit wants to merge 1 commit into
NousResearch:mainfrom
zhuxungit:codex-langfuse-tracing-reliability
Open

zhuxungit wants to merge 1 commit into
NousResearch:mainfrom
zhuxungit:codex-langfuse-tracing-reliability

Conversation

@zhuxungit

Copy link
Copy Markdown

Summary

  • serialize Langfuse client initialization so concurrent first calls construct only one client
  • close root trace contexts and flush active traces on normal finish and process exit
  • support fixed trace IDs only in explicit EvalOps context, preventing accidental trace-id reuse if the env var leaks
  • add tests for client init concurrency, fixed trace-id guard, root context close, and active-trace shutdown

Why

The Langfuse plugin can lose child observations when a process exits before active traces are finalized. Concurrent first-use initialization can also construct multiple clients and leak the losing client's background resources. These changes keep tracing fail-open while making trace finalization and client lifecycle more deterministic.

Tests

  • /Users/zhuxun/.hermes/hermes-agent/venv/bin/python -m pytest tests/plugins/test_langfuse_plugin.py -q

@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Code Review — Looks Good

Verified the full diff (2 files, ~180 lines):

  • TOCTOU fix — the double-checked locking pattern (_LANGFUSE_CLIENT_LOCK separate from _STATE_LOCK) correctly prevents two concurrent first-callers from both constructing Langfuse(**kwargs) and leaking the loser's HTTP connection + flush thread. The fast path (_LANGFUSE_CLIENT is not None) avoids lock contention on the hot path.

  • Root context lifecycle_close_root_context() extracts and nulls state.root_ctx, calls __exit__, and is idempotent. Called from both _finish_trace (normal path) and _shutdown_active_traces (atexit path). Previously the root context was never explicitly closed — the propagate_attributes context manager's __exit__ was relied upon implicitly.

  • atexit handler_shutdown_active_traces() iterates all active trace states, ends pending observations, closes root contexts, and flushes. The _TRACE_STATE.clear() inside _STATE_LOCK ensures no double-processing if called concurrently with _finish_trace.

  • Fixed trace ID — correctly gated behind EvalOps context (HERMES_EVALOPS_RUN_ID or HERMES_EVALOPS_CASE_ID); standalone HERMES_LANGFUSE_TRACE_ID without EvalOps metadata triggers a warning and is ignored.

  • Test coverage — concurrency test uses threading.Barrier(8) to force race conditions, verifying exactly 1 Langfuse instance is created. Trace lifecycle tests cover _close_root_context idempotency and _shutdown_active_traces double-call safety.

No issues found. The _close_root_context fix addresses a real resource leak where propagate_attributes.__exit__ was never called on abnormal trace termination.

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

Copy link
Copy Markdown
Collaborator

Thanks for addressing a real lifecycle gap. On current main, _start_root_trace manually enters and retains root_ctx (plugins/observability/langfuse/__init__.py:624-667), while _finish_trace only ends root_span (:737-766), so explicit context closure is useful.

Problems

  • Current main also evicts non-finalizing traces in _evict_stale_locked (plugins/observability/langfuse/__init__.py:713-735). That path ends only root_span; the proposed helper is not applied there. Salvage should close the evicted root_ctx too and add a regression test.
  • The diff adds HERMES_LANGFUSE_TRACE_ID and HERMES_EVALOPS_* as behavioral environment settings. AGENTS.md:102-107 requires new non-secret behavior configuration to use config.yaml, not .env.

Suggested changes

  • Apply root-context closure to the current eviction path and test it.
  • Split out or rework the fixed-trace-id/EvalOps configuration surface.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 14, 2026
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 sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants