Repository navigation
fix(worker): strip test envs before recover and clean up GPU orphans - #5031
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces GPU orphan process cleanup and VRAM reclaim waiting during model recovery and termination, along with associated unit tests. The review feedback highlights several critical issues: the process-killing logic is too broad and risks terminating unrelated processes, the unit tests reference non-existent helper functions, the VRAM polling loops do not handle missing pynvml gracefully (leading to unnecessary 30-second delays), and pynvml resources are not properly released via nvmlShutdown.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Three layers of defense against GPU resource leaks and recover loops: 1. Strip XINFERENCE_TEST_* envs from cached launch_args before recover and startup recovery. Test-injected env vars (e.g. OOM simulation) were being cached and replayed, causing infinite recover loops. Includes per-uid persist dirty tracking with retry limit. 2. After terminate_model in recover_sub_pool, poll VRAM free ratio via pynvml (with psutil fallback) and SIGKILL orphan GPU processes that survived subpool removal. Replaces implicit reliance on fixed 3s sleep with NVML-based readiness check. 3. In terminate_model normal path (is_model_die=False), scan for spawn-created EngineCore orphans that survive stop() + remove_sub_pool. Uses cmdline verification (vllm/enginecore/model_uid) to identify orphans, SIGKILL them, and wait for VRAM reclaim. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Require model_uid in process cmdline before SIGKILL to prevent killing unrelated processes on shared GPUs (recover + terminate paths) - Extract _kill_orphan_gpu_pids and _wait_pids_dead as standalone functions so test imports succeed (tests referenced non-existent funcs) - Break VRAM polling early when _snapshot_gpu_free_ratio returns -1 (pynvml unavailable) to avoid pointless 30-second stalls - Add pynvml.nvmlShutdown() in try/finally to prevent resource leaks Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Run black on worker.py and test_launch_orphan_cleanup.py - Run isort on test_launch_orphan_cleanup.py - Remove unused pytest import from test_strip_test_envs.py (F401) - Replace "falsy" with "empty/evaluates to False" in comments (codespell) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
183b8b0 to
758ae9a
Compare
- test_strip_test_envs_non_dict_envs_logs_error: assert on the type name "str" the impl actually logs via type(envs).__name__, not the literal input value "not_a_dict" which never appears in the message. - test_kill_orphan_gpu_pids_diff_only: mock psutil (locally imported inside _kill_orphan_gpu_pids) so psutil.Process(300).cmdline() does not raise NoSuchProcess before os.kill is reached. Without this the diff path was short-circuited and killed_pids was always empty. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
os.kill(pid, signal.SIGKILL) does not exist on Windows (signal.SIGKILL is POSIX-only), causing test_kill_orphan_gpu_pids_diff_only to fail on the Windows CI matrices with AttributeError. psutil.Process.kill() is cross-platform and already used elsewhere in the codebase. Test updated to attribute kill() calls per-PID via separate Process mocks, since p.kill() takes no PID argument. Also asserts pre_pids (100, 200) are not killed, strengthening the test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The model_uid filter in _kill_orphan_gpu_pids was too strict: vLLM worker processes had only 'Xinf vLLM worker: <rank>' in their title (no model_uid), and EngineCore processes (forked by vLLM) had no xinference provenance at all. Both were skipped by the filter, making orphan cleanup a no-op. Fix: - Set XINFERENCE_MODEL_UID env var in VLLMModel.load() before engine creation, so all child processes inherit it. - Include model_uid in vLLM worker process title (distributed_executor and distributed_executor_v1). - Walk up the parent chain in _kill_orphan_gpu_pids to match EngineCore processes whose parent has model_uid in cmdline. - Add tests covering the positive model_uid path and cross-model skip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
m199369309
left a comment
There was a problem hiding this comment.
Addressed review finding:
model_uid provenance for vLLM child processes (qinxuye): Fixed with a two-pronged approach:
-
Process title marker:
VLLMModel.load()now setsXINFERENCE_MODEL_UIDenv var before engine creation. Bothdistributed_executor.pyanddistributed_executor_v1.pyWorkerActors now include[model_uid]in their process title:Xinf vLLM worker: <rank> [<uid>]. -
Ancestry walk for EngineCore: EngineCore is forked by vLLM itself — we can't control its process title, but it inherits the environment from its parent. Added
_process_or_ancestor_has_uid()helper that walks up the parent chain checking cmdline. This catches EngineCore whose parent (the vLLM worker) now has model_uid in its title. -
Test coverage: Added
test_kill_orphan_gpu_pids_model_uid_filter(covers: worker with uid in cmdline → killed, EngineCore with uid only in parent → killed, unrelated process → skipped) andtest_kill_orphan_gpu_pids_model_uid_skips_other_model(covers: other model's worker → skipped).
Other findings (already fixed in prior commits, no changes needed):
- Gemini Critical ×2 (recover/terminate path missing model_uid check): both call sites already pass model_uid
- Gemini High ×2 (missing helper functions):
_kill_orphan_gpu_pidsand_wait_pids_deadare defined in worker.py - Gemini High ×2 (VRAM reclaim loop not breaking on -1): both loops already have
if _free_ratio < 0: break - Gemini Medium ×3 (pynvml not shut down): all three call sites already have
try...finallywithnvmlShutdown()
…gpu_idx=None When gpu_idx=None is passed to launch, _create_subpool() auto-selects GPUs but doesn't write them back to launch_args. The terminate path only checked launch_args["gpu_idx"], so _gpu_indices_for_terminate stayed empty and orphan cleanup was skipped for the default launch path. Fix: fall back to model_spec["accelerators"] (the allocated devices saved by _create_subpool()) when launch_args["gpu_idx"] is empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
m199369309
left a comment
There was a problem hiding this comment.
Terminate path missing GPU indices when gpu_idx=None (qinxuye): Fixed.
When gpu_idx=None is passed to launch (the default), _create_subpool() auto-selects GPUs via allocate_devices() but doesn't write them back to launch_args. The terminate path only checked launch_args["gpu_idx"], so _gpu_indices_for_terminate stayed empty and orphan cleanup was skipped entirely for the default launch path.
Fix: fall back to model_spec["accelerators"] (the allocated devices saved at model.model_family.accelerators = devices after _create_subpool() returns) when launch_args["gpu_idx"] is empty. This covers both user-specified and auto-allocated GPU paths.
Other findings (already fixed in prior commits, no changes needed):
- Gemini Critical ×2 (recover/terminate missing model_uid check): both call sites pass model_uid
- Gemini High ×2 (missing helper functions): both functions exist in worker.py
- Gemini High ×2 (VRAM reclaim loop not breaking on -1): both loops have
if _free_ratio < 0: break - Gemini Medium ×3 (pynvml not shut down): all three call sites have
try...finallywithnvmlShutdown() - qinxuye (model_uid provenance): fixed with
XINFERENCE_MODEL_UIDenv var + ancestry walk
- worker.py: change type annotation from "psutil.Process" to Any (psutil is imported locally, not at module level) - test_launch_orphan_cleanup.py: reformat with black Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
XINFERENCE_TEST_*envs from cachedlaunch_argsbefore recover and startup recovery. Test-injected env vars (e.g. OOM simulation flags) were being cached alongside model parameters and replayed on every recover, causing infinite recover loops where the model repeatedly crashes with the same test env.recover_sub_pool. Replaces implicit reliance on process exit timing with explicit VRAM free-ratio check (90% threshold, 30s timeout). Gracefully degrades to psutil if pynvml is unavailable.terminate_modelin both recover and normal terminate paths. Spawn-created EngineCore subprocesses can survivestop()+remove_sub_poolas orphans, holding ~21.5G VRAM and blocking subsequent launches. Uses cmdline verification (vllm/enginecore/model_uid) to safely identify orphans._PERSIST_RETRY_MAX=3) prevents infinite retry storms when disk writes fail._try_recover_models) also strips test envs from persisted launch_args.Test plan
_strip_test_envs(13 test cases covering prefix matching, reference isolation, non-dict envs, exception safety)_parse_gpu_indices,_snapshot_gpu_occupying_pids,_snapshot_gpu_free_ratio,_kill_orphan_gpu_pids)pytest xinference/core/tests/test_strip_test_envs.py -vpassespytest xinference/core/tests/test_launch_orphan_cleanup.py -vpassesRoot cause
When models are recovered (OOM self-heal or worker restart), cached
launch_argscontaining test-injected env vars are replayed verbatim, causing the model to crash again with the same test flag. Additionally,spawn-created EngineCore subprocesses (introduced for fork deadlock avoidance) survivestop()as orphans, holding GPU VRAM and causingValueError: Free memory on device ... less than desiredon subsequent launches.Design notes
pynvmlis an optional dependency — all GPU utility functions gracefully degrade when unavailableXINFERENCE_TEST_prefix🤖 Generated with Claude Code