Fix Memory Leak in RunRouter#373
Open
genematx wants to merge 3 commits into
Open
Conversation
…er.stop() _resources was keyed by resource_uid but stop() was popping with start_uid, so every Resource document from every completed run accumulated in _resources indefinitely (the pop was always a no-op). _stream_resources was never cleaned up in stop() at all. Fix both by introducing _start_to_resources and _start_to_stream_resources reverse-lookup maps (mirroring the existing _start_to_descriptors pattern). These are populated in resource() and stream_resource() and used in stop() to efficiently remove all per-run entries without an O(N) scan. Adds a regression test covering both code paths across multiple runs.
Adds missing docstring for the `hints` field in RunStart, which was present in the JSON schema but absent from the generated TypedDict. Fixes the test_generated_json_matches_typed_dict CI failure.
Member
|
Old resource documents do not have a If RunRouter is used to migrate old Resource documents, this issue is still relevant today. Perhaps we could put it behind a feature flag that does not leak by default, but can leak when we need to push very old Resource documents through a RunRouter. |
Contributor
Author
|
Hmmm, interesting. thanks Dan |
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
2 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.
Fix memory leak: clean up _resources and _stream_resources in RunRouter.stop()
Motivation and Context
_resourceswas keyed byresource_uidbutstop()was popping withstart_uid, so every Resource document from every completed run accumulated in_resourcesindefinitely (the pop was always a no-op)._stream_resourceswas never cleaned up instop()at all.Description
Fix both by introducing
_start_to_resourcesand_start_to_stream_resourcesreverse-lookup maps (mirroring the existing_start_to_descriptorspattern). These are populated inresource()andstream_resource()and used instop()to efficiently remove all per-run entries without an O(N) scan.How Has This Been Tested?
Adds a regression test covering both code paths across multiple runs.