-
Notifications
You must be signed in to change notification settings - Fork 318
fix(resource-manager): prevent shutdown queue deadlock #1800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,10 +134,14 @@ | |
| id_generator: Optional[IdGenerator] = None, | ||
| span_exporter: Optional[SpanExporter] = None, | ||
| ) -> "LangfuseResourceManager": | ||
| if public_key in cls._instances: | ||
| return cls._instances[public_key] | ||
|
|
||
| with cls._lock: | ||
| cached_instance = cls._instances.get(public_key) | ||
| if cached_instance is not None and not cached_instance._shutdown: | ||
| return cached_instance | ||
|
|
||
| if cached_instance is not None: | ||
| cls._instances.pop(public_key, None) | ||
|
|
||
| if public_key not in cls._instances: | ||
| instance = super(LangfuseResourceManager, cls).__new__(cls) | ||
|
|
||
|
|
@@ -226,6 +230,7 @@ | |
|
|
||
| self._custom_httpx_client = httpx_client | ||
| self._init_api_clients() | ||
| self._span_processor: Optional[LangfuseSpanProcessor] = None | ||
|
|
||
| # Media | ||
| self._media_upload_enabled = os.environ.get( | ||
|
|
@@ -260,12 +265,13 @@ | |
| additional_headers=additional_headers, | ||
| span_exporter=span_exporter, | ||
| media_manager=self._media_manager, | ||
| mask_otel_spans=mask_otel_spans, | ||
| ) | ||
| tracer_provider.add_span_processor(langfuse_processor) | ||
| self._span_processor = langfuse_processor | ||
|
|
||
| self._otel_tracer = tracer_provider.get_tracer( | ||
| LANGFUSE_TRACER_NAME, | ||
|
Check failure on line 274 in langfuse/_client/resource_manager.py
|
||
| langfuse_version, | ||
| attributes={"public_key": self.public_key}, | ||
| ) | ||
|
|
@@ -476,11 +482,12 @@ | |
| @classmethod | ||
| def reset(cls) -> None: | ||
| with cls._lock: | ||
| for key in cls._instances: | ||
| cls._instances[key].shutdown() | ||
|
|
||
| instances = list(cls._instances.values()) | ||
| cls._instances.clear() | ||
|
|
||
| for instance in instances: | ||
| instance.shutdown() | ||
|
|
||
| def add_score_task(self, event: dict, *, force_sample: bool = False) -> None: | ||
| try: | ||
| # Sample scores with the same sampler that is used for tracing | ||
|
|
@@ -505,16 +512,23 @@ | |
| is not None # do not sample out session / dataset run scores | ||
| else True | ||
| ) | ||
| ) | ||
|
|
||
| if should_sample: | ||
| langfuse_logger.debug( | ||
| f"Score: Enqueuing event type={event['type']} for trace_id={event['body'].trace_id} name={event['body'].name} value={event['body'].value}" | ||
| ) | ||
| self._score_ingestion_queue.put(event, block=False) | ||
| with self._lock: | ||
| if self._shutdown: | ||
| langfuse_logger.warning( | ||
| "Score: Dropping event because the Langfuse client has already been shut down." | ||
| ) | ||
| return | ||
|
|
||
| langfuse_logger.debug( | ||
| f"Score: Enqueuing event type={event['type']} for trace_id={event['body'].trace_id} name={event['body'].name} value={event['body'].value}" | ||
| ) | ||
| self._score_ingestion_queue.put(event, block=False) | ||
|
|
||
| except Full: | ||
| langfuse_logger.warning( | ||
|
Check warning on line 531 in langfuse/_client/resource_manager.py
|
||
|
Comment on lines
515
to
531
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 add_score_task/add_trace_task (lines 518, 548) now do Extended reasoning...The bug: |
||
| "System overload: Score ingestion queue has reached capacity (100,000 items). Score will be dropped. Consider increasing flush frequency or decreasing event volume." | ||
| ) | ||
|
|
||
|
|
@@ -531,10 +545,17 @@ | |
| event: dict, | ||
| ) -> None: | ||
| try: | ||
| langfuse_logger.debug( | ||
| f"Trace: Enqueuing event type={event['type']} for trace_id={event['body'].id}" | ||
| ) | ||
| self._score_ingestion_queue.put(event, block=False) | ||
| with self._lock: | ||
| if self._shutdown: | ||
| langfuse_logger.warning( | ||
| "Trace: Dropping event because the Langfuse client has already been shut down." | ||
| ) | ||
| return | ||
|
|
||
| langfuse_logger.debug( | ||
| f"Trace: Enqueuing event type={event['type']} for trace_id={event['body'].id}" | ||
| ) | ||
| self._score_ingestion_queue.put(event, block=False) | ||
|
|
||
| except Full: | ||
| langfuse_logger.warning( | ||
|
|
@@ -612,13 +633,24 @@ | |
| langfuse_logger.debug("Successfully flushed media upload queue") | ||
|
|
||
| def shutdown(self) -> None: | ||
| self._shutdown = True | ||
| with self._lock: | ||
| if self._shutdown: | ||
| return | ||
|
Comment on lines
+637
to
+638
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When two wrappers sharing this manager call AGENTS.md reference: AGENTS.md:L134-L134 Useful? React with 👍 / 👎. |
||
|
|
||
| self._shutdown = True | ||
| if self._instances.get(self.public_key) is self: | ||
| self._instances.pop(self.public_key) | ||
| self._media_manager.begin_shutdown() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When shutdown begins with pending OpenTelemetry spans containing base64 media attributes, AGENTS.md reference: AGENTS.md:L134-L134 Useful? React with 👍 / 👎. |
||
|
|
||
| # Unregister the atexit handler first | ||
| atexit.unregister(self.shutdown) | ||
|
|
||
| self.flush() | ||
| self._stop_and_join_consumer_threads() | ||
| try: | ||
| self.flush() | ||
| finally: | ||
| self._stop_and_join_consumer_threads() | ||
|
Check failure on line 651 in langfuse/_client/resource_manager.py
|
||
|
Comment on lines
+636
to
+651
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 shutdown() now calls self._media_manager.begin_shutdown() before self.flush(), so any span still buffered in the BatchSpanProcessor at shutdown time has its media silently dropped instead of uploaded during the force-flush that is supposed to export it. This mainly affects media embedded via third-party OTEL instrumentation (raw base64 attributes only processed at export time), which is exactly the case flush()/shutdown() exists to protect. Move begin_shutdown() to run after flush() completes. Extended reasoning...The bug: In The code path that triggers it: Export goes through Why nothing else prevents it: The media upload consumer threads are still alive at this point — they are only paused/joined later, in Scope: This does not affect the common case of media created via the Langfuse SDK's own span/generation API, since Step-by-step proof:
The fix: Move the |
||
| if self._span_processor is not None: | ||
| self._span_processor.shutdown() | ||
|
|
||
|
|
||
| def _init_tracer_provider( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import os | ||
| import threading | ||
| import time | ||
| from queue import Empty, Full, Queue | ||
| from typing import Any, Callable, Optional, TypeVar, cast | ||
|
|
@@ -42,6 +43,8 @@ def __init__( | |
| self._httpx_client = httpx_client | ||
| self._queue = media_upload_queue | ||
| self._max_retries = max_retries | ||
| self._state_lock = threading.Lock() | ||
| self._shutdown = False | ||
| self._enabled = os.environ.get( | ||
| LANGFUSE_MEDIA_UPLOAD_ENABLED, "True" | ||
| ).lower() not in ("false", "0") | ||
|
|
@@ -53,9 +56,15 @@ def reinitialize( | |
| httpx_client: httpx.Client, | ||
| media_upload_queue: Queue, | ||
| ) -> None: | ||
| self._api_client = api_client | ||
| self._httpx_client = httpx_client | ||
| self._queue = media_upload_queue | ||
| with self._state_lock: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If another thread holds AGENTS.md reference: AGENTS.md:L134-L134 Useful? React with 👍 / 👎. |
||
| self._api_client = api_client | ||
| self._httpx_client = httpx_client | ||
| self._queue = media_upload_queue | ||
| self._shutdown = False | ||
|
|
||
| def begin_shutdown(self) -> None: | ||
| with self._state_lock: | ||
| self._shutdown = True | ||
|
|
||
| def process_next_media_upload(self) -> None: | ||
| try: | ||
|
|
@@ -99,6 +108,13 @@ def _find_and_process_media( | |
| if not self._enabled: | ||
| return data | ||
|
|
||
| with self._state_lock: | ||
| if self._shutdown: | ||
| logger.warning( | ||
| "Media: Skipping upload because the Langfuse client has already been shut down." | ||
| ) | ||
| return data | ||
|
|
||
| seen = set() | ||
| max_levels = 10 | ||
|
|
||
|
|
@@ -279,10 +295,17 @@ def _process_media( | |
| field=field, | ||
| ) | ||
|
|
||
| self._queue.put( | ||
| item=upload_media_job, | ||
| block=False, | ||
| ) | ||
| with self._state_lock: | ||
| if self._shutdown: | ||
| logger.warning( | ||
| f"Media: Skipping upload for media_id={media._media_id} because the Langfuse client has already been shut down." | ||
| ) | ||
| return | ||
|
|
||
| self._queue.put( | ||
| item=upload_media_job, | ||
| block=False, | ||
| ) | ||
| logger.debug( | ||
| f"Queue: Enqueued media ID {media._media_id} for upload processing | trace_id={trace_id} | field={field}" | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 This PR's new eviction logic in
__new__lets a shut-downLangfuseResourceManagerfor a givenpublic_keybe replaced by a fresh one, butshutdown()only stops the oldLangfuseSpanProcessor— it is never detached from the shared globalTracerProvider(OTel has noremove_span_processorAPI), so each shutdown+recreate cycle for the same key permanently appends another dead processor to the provider's processor list. This is an unbounded leak in exactly the flow this PR targets (repeated same-key client recreation in tests/app restarts), and it also means every subsequent span export/force_flush call additionally iterates over the accumulated dead processors.Extended reasoning...
The bug:
_init_tracer_provider()(bottom ofresource_manager.py) only constructs a newTracerProviderwhen the OTel global default is still aProxyTracerProvider. The very firstLangfuseclient created in a process callsotel_trace_api.set_tracer_provider(provider), which is a one-time, non-overridable action in the OTel SDK. Every subsequent call to_init_tracer_provider()— for the samepublic_keyor a different one — hits theelsebranch and simply returns that one shared globalprovider.In
_initialize_instance()(lines 262-274), each time aLangfuseResourceManageris constructed for a key withtracing_enabled=True, a brand-newLangfuseSpanProcessoris created and appended to that shared provider viatracer_provider.add_span_processor(langfuse_processor).add_span_processoron OTel's SDKTracerProvideronly ever appends to an internal list; there is no public API to remove an entry once added.shutdown()(lines ~648-660) setsself._shutdown = True, evicts the instance from_instances, flushes, joins the consumer threads, and finally callsself._span_processor.shutdown(). That stops theBatchSpanProcessorbackground thread and shuts down its exporter, but it does not call anything onself.tracer_providerto detach/remove the processor — and there is no such API to call. The dead processor object stays registered on the shared provider forever.Why this PR changes the picture: Before this PR,
__new__returned the cached instance for apublic_keyunconditionally (if public_key in cls._instances: return cls._instances[public_key]), even if it had already been shut down. So callingLangfuse(public_key=pk)again aftershutdown()never constructed a new manager and never added a second processor — the leak path was unreachable for the same key. This PR's whole point is to change that:__new__now pops a shut-down instance out of_instancesand builds a genuinely freshLangfuseResourceManager, which runs_initialize_instance()again and appends a brand-newLangfuseSpanProcessorto the shared provider. This is precisely the scenario exercised by the PR's own new test,test_shutdown_evicts_manager_and_rejects_stale_client_tasks(shutdown → construct a fresh client for the same key), and it's the pytest/app-restart use case the PR description says it is fixing.Step-by-step proof:
Langfuse(public_key="pk")is created anywhere._init_tracer_provider()sees aProxyTracerProviderdefault, so it createsprovider_Aand callsset_tracer_provider(provider_A)._initialize_instance()buildsprocessor_1and callsprovider_A.add_span_processor(processor_1).client.shutdown()is called._span_processor.shutdown()stopsprocessor_1's thread/exporter, butprovider_A._active_span_processor(its internal composite) still holdsprocessor_1.Langfuse(public_key="pk")is constructed again (e.g. a new pytest test, or an app restarting the client).__new__sees the cached instance's_shutdown == True, pops it from_instances, and builds a fresh manager._init_tracer_provider()now sees the global default isprovider_A(not aProxyTracerProvideranymore), so it returnsprovider_Aunchanged._initialize_instance()buildsprocessor_2and callsprovider_A.add_span_processor(processor_2).provider_Anow holds both the deadprocessor_1and the liveprocessor_2. Repeat steps 2-3 N times (e.g. N tests in a suite that each create/shutdown a same-key client) andprovider_Aaccumulates N dead processors that are never freed — each one still holding its exporter, HTTP client references, and internal buffers.force_flush()/shutdown ofprovider_A, now iterates over all N+1 processors, so the overhead (not just memory) grows with the number of create/shutdown cycles.Impact: In a long-running process (e.g. a web app that reconstructs its Langfuse client on config reload) or in a test suite that repeatedly builds/tears-down same-key clients — exactly the pattern this PR's new test and its stated goal cover — this leaks a
LangfuseSpanProcessor(plus its exporter and any queued references) per cycle, unboundedly. It's not a crash or incorrect trace data (the dead processors are harmless no-ops aftershutdown()), but it is a genuine, PR-introduced resource leak in the exact code path this PR adds.Suggested fix: Track the processor per-manager and either (a) reuse/replace it in place via a wrapper that supports swapping its inner processor without needing a new
add_span_processorcall, or (b) give eachLangfuseResourceManagerits own isolatedTracerProviderinstead of relying on the ambient global one when one isn't explicitly passed in, so a shut-down manager's provider (and all its processors) can simply be dropped and garbage collected.