Repository navigation
fix(media): do not block flush() when no media upload consumer is left - #2031
Closed
anishgurjar wants to merge 2 commits into
Closed
anishgurjar wants to merge 2 commits into
anishgurjar wants to merge 2 commits into
Conversation
Contributor
Author
|
cc @hassiebp |
Comment on lines
+499
to
+503
| for consumer in rm._media_upload_consumers: | ||
| consumer.pause() | ||
| for consumer in rm._media_upload_consumers: | ||
| consumer.join() | ||
| assert not consumer.is_alive() |
Contributor
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!
6 of 11 tasks
Collaborator
|
Thanks for raising this - already fixed on main |
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
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.
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