fix(langfuse): guard _get_langfuse() against concurrent-init TOCTOU - #42326
Closed
nftpoetrist wants to merge 1 commit into
Closed
fix(langfuse): guard _get_langfuse() against concurrent-init TOCTOU#42326nftpoetrist wants to merge 1 commit into
nftpoetrist wants to merge 1 commit into
Conversation
Two concurrent first callers both pass the ``_LANGFUSE_CLIENT is not None`` guard, both construct a ``Langfuse(**kwargs)`` client, and the loser's client leaks an open HTTPS connection and a background flush thread. This is the same TOCTOU class fixed for the Honcho and FAL clients via ``plugins/plugin_utils.py`` (PR NousResearch#24759). Add ``_LANGFUSE_CLIENT_LOCK = threading.Lock()`` and wrap the entire init path in a double-checked lock. The two outer fast-path checks (``_INIT_FAILED`` / ``is not None``) stay outside the lock so the hot path — every hook call after the first — takes zero contention. ``_STATE_LOCK`` is deliberately not reused: it guards the hot-path ``_TRACE_STATE`` dict and nesting it here would risk deadlock.
alt-glitch
added
type/bug
Contributor
|
Positive verification comment — reviewed the full diff, no issues found. The double-checked locking pattern for
No resource leak, no missed edge case. The comment at line 55-60 explaining why a separate lock is used is particularly good. |
Open
1 task
Contributor
|
Thanks for the focused concurrency fix. The premise remains valid on current main: Problems
Suggested changes
Automated hermes-sweeper review. |
teknium1
added
the
sweeper:blast-contained
erosika
added a commit
to erosika/hermes-agent
that referenced
this pull request
Aug 10, 2026
_get_langfuse() double-checked a global with no lock, so two concurrent first callers (e.g. two gateway sessions firing hooks at once) could both pass the None guard, both construct a client, and leak the loser's HTTP connection and background flush thread. First build is now serialized by a module lock with a re-check inside; the settled fast path stays lock-free. The atexit finalizer registration moves inside the locked section so it registers exactly once, after the winning client. Adopted from NousResearch#42326 — thanks @nftpoetrist for the report and fix. Co-authored-by: nftpoetrist <264138787+nftpoetrist@users.noreply.github.com>
Contributor
|
adopted into #83437 with co-author credit — thanks for catching the concurrent-init leak. one integration change: the atexit finalizer registration (added in that PR) moved inside the locked section so it registers exactly once. |
kshitijk4poor
added a commit
that referenced
this pull request
Aug 13, 2026
… fan-out Salvaged from PR #83437 by @erosika, with adopted fixes from @bgodlin (#81054), @aldoeliacim (#82332), @nftpoetrist (#42326), @rodboev (#39653), @FnExpress (#64292, supersedes #32175 by @db-aeon), @Per0-1 (#61166), @NaMinhyeok (#64797), and @liuhao1024 (#43130). Widens the bundled Langfuse plugin from 6 to 11 hooks and fixes two attribution bugs. Also adopts shutdown/atexit lifecycle fixes and composes 8 prior community PRs with interaction-fix follow-ups. Model attribution: on_pre_llm_request and on_post_llm_call now prefer the wire value (request body model, response model) over the agent attribute, which goes stale after /model switch or provider fallback. Cost total: both cost paths now send a summed total alongside the per-type breakdown, since Langfuse does not derive calculatedTotalCost from cost_details keys. Subscription-included routes send no cost keys at all. New coverage: api_request_error closes failed generations with ERROR level; on_session_finalize/on_session_end close dangling traces for tool-only and interrupted turns; subagent_start/subagent_stop trace delegated children as spans; MoA advisor fan-out emits one generation per advisor priced at the advisor's own model. Capture modes: HERMES_LANGFUSE_CAPTURE=metadata|sanitized|full (default sanitized). Sanitized mode redacts secret patterns before truncation. Adopted lifecycle fixes: shutdown client at session finalize when reason=shutdown (not on session rotation); atexit finalizer ends open root spans for short-lived processes; root context manager exited to prevent interpreter-teardown TypeError; TOCTOU on _get_langfuse() fixed with lock; reasoning_content surfaced in traces; system prompt included in generation input for Anthropic/Codex/Bedrock; SDK v3 update_trace replaces set_trace_io. Closes #29482, #43129, #72661. Supersedes #81054, #82332, #42326, #39653, #64292, #32175, #61166, #64797, #43130. Partially addresses #67544 (capture modes + secret redaction; user_id remains open).
Collaborator
|
Merged via #85439 — your fix was adopted and composed into the wider Langfuse tracing PR by @erosika. Your contribution is credited in the commit body. Thanks @nftpoetrist! |
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
6 participants
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.
Summary
_get_langfuse()holds a global_LANGFUSE_CLIENTvariable with no lockprotecting the double-check initialization. Two concurrent first callers (e.g.
two gateway sessions both triggering a Langfuse hook at the same time) both
pass the
_LANGFUSE_CLIENT is not Noneguard, both construct aLangfuse(**kwargs)client, and the loser's client leaks an open HTTPSconnection and a background flush thread. Under sustained gateway load this
accumulates silently.
This is the same TOCTOU class fixed for the Honcho and FAL clients in
plugins/plugin_utils.py(PR #24759 / commit47d5177a7).Changes:
_LANGFUSE_CLIENT_LOCK = threading.Lock()alongside the existing_STATE_LOCK(deliberately separate —_STATE_LOCKis already on the hotpath for every hook call and nesting would risk deadlock).
_get_langfuse()in a double-checked lock._INIT_FAILED/is not None) remainoutside the lock so every hook call after the first stays zero-contention.
_INIT_FAILEDsentinel semantics (missing SDK, missing creds,placeholder keys, construction exception) are preserved unchanged.
Test plan
tests/plugins/test_langfuse_plugin.py— 37 existing tests all passunchanged,
_INIT_FAILEDcached on any init failure, same log messages)