fix(serverless): force-kill worker on fitness check failure#536
Open
deanq wants to merge 3 commits into
Open
Conversation
A failed fitness check calls sys.exit(1), which only raises SystemExit and triggers cooperative interpreter shutdown. That shutdown blocks in threading._shutdown() joining every non-daemon thread. Workers routinely have such threads alive by the time checks run (notably vLLM's AsyncLLMEngine, constructed at module import before the checks), so the worker logged "Worker is unhealthy, exiting." but never terminated and kept serving jobs. Route the unhealthy exit through a _terminate_unhealthy helper that calls os._exit, which bypasses thread joins, atexit handlers, and asyncgen cleanup and terminates unconditionally. SLS-379
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
There was a problem hiding this comment.
Pull request overview
This PR changes serverless fitness-check failure handling so an unhealthy worker is force-terminated immediately (via os._exit(1)) instead of attempting cooperative shutdown via sys.exit(1), which can hang when non-daemon background threads are still running (e.g., vLLM engine threads).
Changes:
- Add
_terminate_unhealthy()helper inrp_fitnessthat flushes stdio then callsos._exit(1)on fitness-check failure. - Update
run_fitness_checks()to use_terminate_unhealthy(1)when a check fails. - Add regression tests that reproduce the hang scenario in a subprocess and verify
os._exitis used; adjust test fixtures to preventos._exitfrom killing the pytest process.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
runpod/serverless/modules/rp_fitness.py |
Introduces hard-exit helper and routes fitness failures through it. |
tests/test_serverless/test_modules/test_fitness/conftest.py |
Autouse fixture neutralizes os._exit during in-process tests by raising SystemExit. |
tests/test_serverless/test_modules/test_fitness/test_force_kill.py |
Adds subprocess regression test for non-daemon thread hang and asserts helper uses os._exit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Guard the stdio flush in _terminate_unhealthy so a closed or None stream cannot raise and block the os._exit hard-kill. - Update module and run_fitness_checks docstrings to describe os._exit semantics (hard exit, no cooperative SystemExit). - Remove unused pytest import from the regression test; add a test that a failing flush still reaches os._exit. SLS-379
Replace the empty try/except-pass around the stdio flush with contextlib.suppress(Exception), which clears the CodeQL py/empty-except finding and reads more clearly. Behavior is unchanged: a flush failure is still swallowed so os._exit always runs. SLS-379
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
A failed worker fitness check is supposed to force-kill the worker so the orchestrator restarts it. Instead, on workers with live non-daemon background threads, the worker logs
Worker is unhealthy, exiting.and keeps serving jobs. Observed on a live vLLM endpoint.Root cause
rp_fitness.run_fitness_checkscallssys.exit(1)on failure.sys.exit()only raisesSystemExit, which unwinds to the top ofasyncio.run(run_fitness_checks())(worker.py:40) and begins cooperative interpreter shutdown. That shutdown blocks inthreading._shutdown()joining every non-daemon thread.worker-vllmconstructsAsyncLLMEngine.from_engine_args(...)at module import — beforerunpod.serverless.start(...)runs, and therefore before fitness checks run. Its non-daemon background threads/loops never return, so the interpreter blocks forever on the join and the process never dies.Reproduction (against the real
run_fitness_checks)A non-daemon thread loops forever, a failing check is registered, then
asyncio.run(run_fitness_checks())runs exactly asworker.pydoes:sys.exit(1)(before)os._exit(1)(after)Fix
Route the unhealthy exit through a
_terminate_unhealthyhelper that flushes stdio then callsos._exit(1). A fitness failure means the environment is broken and the worker must die now;os._exitbypasses thread joins, atexit handlers, and asyncgen cleanup — the required semantics.Test plan
test_force_kill.py:test_unhealthy_exit_terminates_with_live_non_daemon_thread— subprocess with a live non-daemon thread; fails (hangs to timeout) before the fix, exits 1 after.test_terminate_unhealthy_uses_os_exit— asserts the helper callsos._exit.SystemExit-based fitness tests kept green via an autouse fixture that redirectsos._exitto raiseSystemExit(1)(so it can't kill the pytest process).make quality-check: 525 passed, coverage 94.20% (threshold 90%).Closes SLS-379