Repository navigation
fix(media): do not block flush() when no media upload consumer is left - #2031
anishgurjar wants to merge 2 commits into
Conversation
|
|
|
cc @hassiebp |
| for consumer in rm._media_upload_consumers: | ||
| consumer.pause() | ||
| for consumer in rm._media_upload_consumers: | ||
| consumer.join() | ||
| assert not consumer.is_alive() |
There was a problem hiding this comment.
The new test stops every media consumer before calling flush(), so it never reaches the timed wait in _join_media_upload_queue(). It would still pass if that wait became untimed, bringing back the hang when the last consumer exits without another queue notification.
Add a test that keeps a consumer alive until the wait starts, then stops it with an unfinished item. This is a non-blocking coverage improvement for the concurrent-shutdown case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/test_resource_manager.py
Line: 499-503
Comment:
**Shutdown race lacks a test**
The new test stops every media consumer before calling `flush()`, so it never reaches the timed wait in `_join_media_upload_queue()`. It would still pass if that wait became untimed, bringing back the hang when the last consumer exits without another queue notification.
Add a test that keeps a consumer alive until the wait starts, then stops it with an unfinished item. This is a non-blocking coverage improvement for the concurrent-shutdown case.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What does this PR do?
Relates to langfuse/langfuse#11104
Guards
flush()against blocking forever on the media upload queue when no consumer thread is left to drain it.LangfuseResourceManager.flush()ends withself._media_upload_queue.join().Queue.join()blocks until every queued item is marked done, but media upload consumers exit onpause()duringshutdown()without draining the queue. Any interleaving that leaves items pending with no live consumer - concurrentshutdown()/flush()calls, a media-bearing span ending between the join and the consumer stop, or a latershutdown()after such an interleaving - makes the nextflush()block forever.This mirrors the score-queue guard added in #2025 for the same class, one call site up: wait on the queue's
all_tasks_donecondition and return early when no consumer thread is alive to drain it. It is a minimal alternative to the stale #1473, touching onlyresource_manager.py.Type of change
Verification
List the main commands you ran:
The new regression test (consumers stopped with an item pending,
flush()on a watchdog thread) fails onmain-flush()never returns - and passes with the guard.Checklist
code_review.md..env.templateif needed.The PR appears safe to merge, with a non-blocking test coverage gap for consumers stopping during
flush().Summary
Replaces the media queue's unconditional wait with
_join_media_upload_queue().No runtime tests were run during this review.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A["flush()"] --> B{"Unfinished media work?"} B -->|No| E["Return"] B -->|Yes| C{"Any consumer alive?"} C -->|No| E C -->|Yes| D["Wait for completion or 0.1 seconds"] D --> BReviews (1) · Last reviewed commit: "test: cover flush() with stopped media c..." · Reviewed by Greptile