fix(cli): don't raise from the worker signal handler so SIGTERM always drains - #6738
Open
RGB-loop wants to merge 1 commit into
Open
fix(cli): don't raise from the worker signal handler so SIGTERM always drains#6738RGB-loop wants to merge 1 commit into
RGB-loop wants to merge 1 commit into
Conversation
…s drains The start-mode signal handler raised _ExitCli directly. CPython delivers signal handlers on the main thread at an arbitrary bytecode boundary, so when the event loop was executing a task — e.g. a user request_fnc doing blocking I/O inside a job-request task — the exception surfaced inside that task instead of at the run_until_complete boundary. The task died with an unretrieved _ExitCli, the worker never entered the drain/aclose path, and it kept accepting new dispatches until SIGKILL. Because the handler latches, the first (and in container environments, only) SIGTERM was wasted. Schedule the exit on the loop via call_soon_threadsafe instead, the same signal-safe pattern the TCP/console path in this module already uses. run_until_complete now waits on either the main task or the exit future, leaving server.run alive so the existing drain/aclose sequence still has a live worker. Second signal still force-exits. Added a subprocess regression test that SIGTERMs a worker while its event loop is blocked by a synchronous call inside a task; on the previous code the worker never shuts down. Fixes livekit#6724 Generated with AI Co-Authored-By: AI <ai@example.com>
|
jingyan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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.
Fixes #6724
The start-mode signal handler raised
_ExitClidirectly. CPython delivers signal handlers on the main thread at an arbitrary bytecode boundary, so when the event loop was executing a task — e.g. a userrequest_fncdoing blocking I/O inside a job-request task — the exception surfaced inside that task instead of at therun_until_completeboundary. The task died with an unretrieved_ExitCli, the worker never entered the drain/aclosepath, and it kept accepting new dispatches until SIGKILL. Because the handler latches, the first (and in container environments, only) SIGTERM was wasted.Changes:
call_soon_threadsafe— the same signal-safe pattern the TCP/console path in this module already uses — instead of raising.run_until_completewaits on either the main task or the exit future, leavingserver.runalive so the existing drain/aclosesequence still has a live worker. A second signal still force-exits.Added a subprocess regression test (
tests/test_cli_sigterm.py) that SIGTERMs a worker while its event loop is blocked by a synchronous call inside a task. On the previous code the worker never shuts down (test times out after 30s); with this change it drains and exits cleanly