Repository navigation
fix(model): use os._exit for OOM to trigger pool recovery - #5005
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Out of Memory (OOM) error handling in xinference/core/model.py to use os._exit(1) instead of sys.exit(1), ensuring that the model subprocess exits immediately and allows xoscar to detect the failure and recover the pool. It also introduces a reentrancy gate, shortens the cleanup timeout to 5 seconds, and adds corresponding integration tests. The review feedback suggests two important improvements: wrapping the worker status update in a timeout to prevent indefinite blocking during OOM, and replacing time.sleep with asyncio.sleep in the asynchronous test to avoid blocking the event loop.
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.
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
One issue to address before LGTM.
sys.exit(1) only raises SystemExit, which xoscar's _ErrorProcessor swallows with an unconditional `return True`. The process then stays alive, its returncode stays None, monitor_sub_pools keeps treating it as alive, and recover_sub_pool never fires. Replace sys.exit(1) with os._exit(1) in _handle_oom_error so the subprocess actually exits and xoscar can detect the returncode change and rebuild the sub pool. Add a reentrancy gate (_oom_handling) to handle concurrent OOM on the sync batch path, and reduce the stop() timeout from 30s to 5s to bound recovery delay. Add regression tests in both sync and async client test suites. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… test Address code review feedback: 1. Wrap worker_ref.update_model_status in asyncio.wait_for(timeout=5) in _handle_oom_error. During OOM the worker may be unresponsive or under high load, and the RPC could block indefinitely, preventing os._exit(1) from being reached and delaying pool recovery. The status update is best-effort and should not block the exit path. 2. Replace time.sleep(1) with await asyncio.sleep(1) in async test test_model_oom_triggers_restart. Blocking the event loop with time.sleep in an async test can delay other async tasks and cause test flakiness or timeouts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address review feedback: _get_worker_ref() calls xo.actor_ref() which is also an xoscar RPC and can hang during OOM / worker failure. Wrap the entire status report (_get_worker_ref + update_model_status) in a single xo.wait_for(timeout=5) so the process always reaches os._exit(1) promptly, even if the worker is completely unreachable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fb4bb0d to
5646f57
Compare
3 participants
Summary
sys.exit(1)in_handle_oom_erroronly raisesSystemExit, which xoscar's_ErrorProcessor.__exit__(backends/pool.py) swallows with an unconditionalreturn True. The model subprocess stays alive, itsreturncodestaysNone,monitor_sub_poolskeeps treating it as alive, andrecover_sub_poolnever fires. The model becomes a "zombie" that keeps returning OOM errors without ever recovering.sys.exit(1)withos._exit(1)so the subprocess actually exits at the OS level and xoscar can detect the returncode change and rebuild the sub pool._oom_handling) to handle concurrent OOM on the sync batch path (where the worker thread does not await the cleanup coroutine and can trigger another OOM beforeos._exitlands).stop()timeout from 30s to 5s to bound recovery delay (OOM-site cleanup mostly cannot reclaim already-reserved GPU fragments anyway).test_client.py) and async (test_async_client.py) test suites that verify the subprocess actually exits after OOM.Test plan
test_model_oom_triggers_restartin bothtest_client.pyandtest_async_client.py— verifies the subprocess exits after OOMtest_model_error— verifies OOM error message is still propagated to clienttest_auto_recover— verifies normal auto-recovery (non-OOM path) still works🤖 Generated with Claude Code