fix(sglang): never skip kill_process_tree when router removal fails in shutdown#32
Open
JunzheJoe wants to merge 1 commit into
Open
fix(sglang): never skip kill_process_tree when router removal fails in shutdown#32JunzheJoe wants to merge 1 commit into
JunzheJoe wants to merge 1 commit into
Conversation
…n shutdown The miles-router / old-router branch of SGLangEngine.shutdown posted /remove_worker without any exception guard. When the router is already dead — which is exactly the failure-cleanup scenario where RolloutManager.shutdown_hard drives this method — requests raises ConnectionError, the method aborts, and kill_process_tree never runs. ray.kill on the engine actor does not terminate its child processes, and the CUDA context lives in the SGLang server tree, so the orphaned servers keep their VRAM until a manual pkill; subsequent runs OOM. The >=0.3.0 router branch already had a try/except guard — this brings the other branches to parity and makes router removal best-effort: warn on failure, always kill the process tree.
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
Review finding F8-SHUTDOWN-ROUTER-LEAK (F1-F12 code review, leak-level — reported per the review scope's miles exception: crash / data corruption / resource leak).
SGLangEngine.shutdown()'s miles-router / old-router branch posts/remove_workerwith no exception guard. When the router is already dead — which is exactly the failure-cleanup scenario whereRolloutManager.shutdown_harddrives this method (router crash → training fails → driver finally → shutdown_hard) —requestsraisesConnectionError,shutdown()aborts, andkill_process_treeat the bottom never runs.The caller can't compensate:
rollout.py'sshutdown_hardcatches the exception and falls back toray.kill(handle), but killing the Ray actor does not terminate its spawned SGLang server child processes (rollout.py's own comment: "CUDA context lives in the SGLang server child processes"). The orphaned tree leaves Ray's process hierarchy, so evenray stopmay not reclaim it — the servers hold their VRAM until a manualpkill, and subsequent runs OOM.Fix
Bring the miles-router / <0.3.0 branches to parity with the
>=0.3.0branch (which already had a try/except): wrap the whole router-removal block, warn on failure, and always runkill_process_tree. Router removal becomes best-effort — shutdown's job is killing the server tree, and a dead router must not prevent that. Inner version-branch logic is unchanged (indentation-only diff inside the try).Testing
ast.parseclean; no unit test — exercising this path requires stubbingrequests+ a live process tree, and the failure mode is a straight control-flow skip that the diff makes structurally impossible.