fix(client): evict singleton instance on shutdown and prevent deadlocks on re-instantiation - #1897
himanshu-xyz-1 wants to merge 2 commits into
Conversation
…instantiation Signed-off-by: Himanshu Joshi <himanshu.zyx7@gmail.com>
| # Evict from singleton registry so subsequent client initializations | ||
| # construct a fresh, active manager instead of reusing a shut down one | ||
| if hasattr(self, "public_key") and self.public_key in self._instances: | ||
| if self._instances[self.public_key] is self: | ||
| del self._instances[self.public_key] |
There was a problem hiding this comment.
After this eviction, creating another client with the same key registers a new LangfuseSpanProcessor on the shared OpenTelemetry provider, but shutdown never removes or shuts down the old processor. Both processors accept spans for that key, so spans created after re-instantiation can be exported twice and the old exporter remains alive.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_client/resource_manager.py
Line: 660-664
Comment:
**Old processor remains active**
After this eviction, creating another client with the same key registers a new `LangfuseSpanProcessor` on the shared OpenTelemetry provider, but shutdown never removes or shuts down the old processor. Both processors accept spans for that key, so spans created after re-instantiation can be exported twice and the old exporter remains alive.
**Knowledge Base Used:**
- [SDK client lifeycle and configuration](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/sdk-client-lifecycle.md)
- [Client initialization and resource management](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/client-initialization-and-resources.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
|
||
| def add_score_task(self, event: dict, *, force_sample: bool = False) -> None: | ||
| try: | ||
| if getattr(self, "_shutdown", False): |
There was a problem hiding this comment.
The _shutdown check is separate from the queue insertion here and in add_trace_task(). A producer can pass the check, pause while shutdown flushes the empty queue and stops the consumer, and then enqueue an event that is never consumed or acknowledged. The event is lost, and a later flush() on the old client can block indefinitely on the queue's unfinished-task count.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_client/resource_manager.py
Line: 496
Comment:
**Shutdown can strand events**
The `_shutdown` check is separate from the queue insertion here and in `add_trace_task()`. A producer can pass the check, pause while shutdown flushes the empty queue and stops the consumer, and then enqueue an event that is never consumed or acknowledged. The event is lost, and a later `flush()` on the old client can block indefinitely on the queue's unfinished-task count.
**Knowledge Base Used:**
- [Background ingestion and task management](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/background-ingestion-and-task-management.md)
- [Client initialization and resource management](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/client-initialization-and-resources.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if getattr(self, "_shutdown", False): | ||
| return | ||
|
|
||
| self._shutdown = True |
There was a problem hiding this comment.
Concurrent shutdown returns early
The first caller sets _shutdown before it performs the actual flush and thread joins, so a concurrent caller sees the flag and returns while teardown is still running. This breaks the public shutdown contract that pending data has been flushed and background threads have terminated when the call returns, and can let the second caller release dependent resources too early.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_client/resource_manager.py
Line: 652-655
Comment:
**Concurrent shutdown returns early**
The first caller sets `_shutdown` before it performs the actual flush and thread joins, so a concurrent caller sees the flag and returns while teardown is still running. This breaks the public shutdown contract that pending data has been flushed and background threads have terminated when the call returns, and can let the second caller release dependent resources too early.
**Knowledge Base Used:**
- [SDK client lifeycle and configuration](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/sdk-client-lifecycle.md)
- [Client initialization and resource management](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/client-initialization-and-resources.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Signed-off-by: Himanshu Joshi <himanshu.zyx7@gmail.com>
Summary
Fixes an issue where LangfuseResourceManager retained stopped consumer threads in the _instances dictionary after shutdown(). Calling get_client() again would return the defunct instance, causing subsequent requests to block indefinitely.
Changes
Testing
uv run pytest tests/unit/test_resource_manager.py (15 passed in 2.17s).
This PR is not safe to merge until same-key re-instantiation avoids duplicate span processors and shutdown synchronizes queue admission and concurrent callers.
Summary
This PR makes resource-manager shutdown idempotent, evicts shut-down managers from the per-key singleton registry, rejects new score and trace tasks after shutdown begins, and adds lifecycle regression tests. The eviction currently leaves the old OpenTelemetry span processor registered, however, and the shutdown state transition does not fully synchronize producers or concurrent shutdown callers.
Diagram
Reviews (1) · Last reviewed commit: "fix(client): evict singleton on shutdown..."