fix(sdk-python): detach Run finalizer on finish/exit (fixes #97) - #102
Open
Srinivasan8888 wants to merge 1 commit into
Open
Srinivasan8888 wants to merge 1 commit into
Srinivasan8888 wants to merge 1 commit into
Conversation
…ouble-Run guard Each Run arms weakref.finalize(self, _release_active_run, experiment_id) as a safety net for leaked (never-finished) runs. finish()/__exit__ released the registry slot but never detached that finalizer, so when a *finished* Run was later garbage-collected its finalizer fired and discarded the experiment_id from _ACTIVE_RUNS -- even when that id now belonged to a different, still-active Run. That silently freed the live Run's slot and let a third Run for the same id start, defeating the guard the registry exists to enforce. Detach the finalizer right after releasing the slot in both finish() and the __exit__ exception path. The leaked-run safety net is unchanged. Fixes evo-hq#97.
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.
Problem
The Python SDK keeps a process-wide registry (
_ACTIVE_RUNS) so twoRuns with the sameexperiment_idcan't be live at once. EachRunalso armsweakref.finalize(self, _release_active_run, self._experiment_id)as a safety net for a leaked run.finish()/__exit__release the slot but neverdetach()the finalizer. Since the registry is keyed only byexperiment_id, GC of an already-finishedRunfires its stale finalizer and frees the slot of a different, still-activeRunthat reused the id:r2andr3then both run and collide writingEVO_RESULT_PATH— the exact late-failure the registry was added to prevent.Fix
detach()the finalizer immediately after the slot is released, in bothfinish()'sfinallyand__exit__'s exception path. The leaked-run safety net (finalizer fires for a never-finishedRun) is unchanged.Tests
tests/unit/test_sdk_run_finalizer.py(TDD, failing first):Rundoes NOT free a live Run's slot (viafinish()and viawith)Runstill releases its slot on GCExisting
sdk/python/testpasses.Fixes #97.