Conversation
_replace_process_state! removes the old process id from c.test_processes, but it does not call on_process_terminated. The TestProcessTerminatedMsg that the IO task of the old process posts later finds no entry, so its handler does not call the callback either. A consumer that keeps its own list of test processes, for example TestItemRuns, then keeps the id of a process that does not exist. A test run replaces a pooled process when the env content hash changed, or when the process has no endpoint because a termination started before the run. The Revise restart and the IO-error restart use the same function. _replace_process_state! now calls on_process_terminated(old_id) after it removes the old id. The callers report the new id with on_process_created after this.
This branch has not been deployed
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.
I found this problem while I was trying out a customized version of the JuliaMCP.jl server. I used AI help to debug the problem to its root cause and to prepare the fix.
Problem
The docstring of
terminate_test_processsays thaton_process_terminatedfires when the process ends. This does not happen when a test run replaces a pooled process with a new one. A consumer that keeps its own list of test processes then keeps the old id. TestItemRuns is one such consumer: it adds an entry tosession.processesonon_process_createdand removes it only onon_process_terminated. Its process list then shows a process that does not exist, andterminate_test_processon that id only logsIgnoring terminate request for unknown process.Reproduction:
env_content_hash = "hash-A".env_content_hash = "hash-B".on_process_createdreports two ids.on_process_terminatedreports only the second id.A run that starts after
terminate_test_processon an idle process, but before the process exits, gives the same result. A restart that Revise asks for gives it too.Cause
handle!(::GetProcsForTestRunMsg)restarts a reused process when the process has no endpoint or when the env content hash changed (src/testitemcontroller.jl:789-808). The restart calls_replace_process_state!(src/testitemcontroller.jl:2499-2575). That function makes a new process, kills the old one and removes the old id fromc.test_processes(:2572). It does not callon_process_terminated. The IO task of the old process then postsTestProcessTerminatedMsg, but its handler finds no entry for the id (:652) and calls no callback. The Revise restart (:2054) and the IO-error restart (:2239) use the same function.Fix
_replace_process_state!callson_process_terminated(old_id)after it removes the old id. The callers report the new id withon_process_createdafter this, so a consumer sees the old id go before the new id comes.Test
The PR adds the test item
Process restart on env hash change reports the old process as terminatedtotest/test_process_reuse.jl. It runs one item twice with two env hashes and then shuts down the controller. It checks thaton_process_terminatedreports each id fromon_process_createdone time. Without the fix it fails, because the first id has no report. With the fix it passes. The other test items intest/test_process_reuse.jlalso pass.