fix(python): Shut workers down without raising from signal handlers - #764
Merged
Conversation
Both worker signal handlers raised KeyboardInterrupt. That unwinds at an arbitrary bytecode, and the worker's shutdown event is a multiprocessing.Event that both workers also use as their backoff sleep. Interrupting multiprocessing.Event.wait() can leave its condition variable's lock unheld while the enclosing `with` block still releases it, or leave the sleeping/woken counters skewed so the next notify() trips one of its asserts. Signal handlers now only assign a bool, which takes no lock, and the loops act on it. Calling Event.set() from a handler is not an option either: it takes a non-reentrant lock, so a signal arriving while the same thread is inside wait() deadlocks the process outright. Consequences: - The worker shutdown event is now a ShutdownSignal wrapping a threading.Event. It was never shared with child processes, unlike the pool's own _shutdown_event, so it does not need to be a multiprocessing.Event. - Sleeps poll the flag every SHUTDOWN_POLL_INTERVAL_SEC, and the push worker polls wait_for_termination() rather than being unblocked by a server.stop() call from inside the handler. - The pull worker returns 0 instead of re-raising KeyboardInterrupt, so the CLI gets an exit code. - The push worker skips server.start() if shutdown was already requested, and only stops a server it started. - A pull worker that is shutting down no longer asks for another task, in fetch_task() or via update_task's fetch_next. A SIGTERM arriving during a blocking get_task() or the client's all-hosts-unavailable sleep is still not noticed until that call returns. Putting a deadline on those calls is a separate change. ref SENTRY-5MB5
evanh
approved these changes
Jul 27, 2026
|
|
||
| def test_start_does_not_serve_when_shutdown_during_warmup() -> None: | ||
| from grpc_health.v1 import health_pb2 | ||
| @contextlib.contextmanager |
Member
There was a problem hiding this comment.
This is some complicated testing. I'm OK with it but we are getting to edge of "human-readable".
3 tasks
|
PR reverted: e9d1494 |
sentry-taskbroker-fast-revert-bot Bot
pushed a commit
that referenced
this pull request
Jul 29, 2026
untitaker
added a commit
that referenced
this pull request
Aug 25, 2026
…784) * fix(python): Shut workers down without raising from signal handlers Reland of #764 (reverted in e9d1494) with the bug that forced the revert fixed. Signal handlers now only flip a bool instead of raising KeyboardInterrupt. Raising unwinds at an arbitrary bytecode and can leave locks held by the interrupted code in a broken state, which is where the `ValueError: semaphore or lock released too many times` crashes came from. Anything that takes a lock, including `Event.set()` and `server.stop()`, is also unsafe to call from a handler, so `ShutdownSignal.request()` does nothing but assign. The previous attempt exited on its own shortly after startup because the serve loop read the return value of `grpc.Server.wait_for_termination(timeout=...)` as "the server terminated". It actually returns True when the timeout elapsed, i.e. while the server is healthy, and False once it has terminated - the inverse of `Event.wait()`. A healthy worker therefore broke out of the loop after one poll interval and shut down cleanly with exit code 0. The serve loop no longer looks at that return value at all. It sleeps on `ShutdownSignal.wait()`, so our own flag is the only thing that can end it. ref STREAM-1649 * fix(python): Address review on shutdown handling - Notice a gRPC server that terminated on its own again. Polling only the shutdown flag meant an internally failed server left the parent running with live children and a green health check. Read wait_for_termination(timeout=...) with its real semantics: True means the timeout elapsed and the server is still up, False means it terminated. - Drop a pull-mode task claimed while shutting down. get_task() blocks with no deadline, so SIGTERM can land mid-RPC; handing the activation to a child claims work we won't run, which then has to expire on the broker before anyone else picks it up. * test(python): Make the shutdown tests actually observe the exit condition Verified by reintroducing the inverted boolean and re-running: previously one test failed, one hung until timeout, and the SIGTERM test passed regardless. Now all three fail fast. - test_push_start_exits_cleanly_on_sigterm fired SIGTERM from the first poll, so the loop exited after one iteration whichever way the exit condition was read. That is how the inverted boolean got through review. Deliver the signal on the third poll instead, so surviving to it proves the condition is being exercised. - test_push_start_exits_when_server_terminates_unexpectedly spun forever when the loop ignored a terminated server. Bail out after a few polls so it fails with a message instead of hanging CI. * test(python): Pin the property ShutdownSignal actually exists for Mutation check: making request() also call _event.set() -- which destroys the whole point of the class, since Event.set() takes the lock a signal handler must not touch -- passed all 18 shutdown tests. The two wakeup tests look like they cover this but cannot: one allows <5s, loose enough to pass whether request() polls or wakes instantly, and the other goes through set(), which the mutation makes identical to request(). Assert the contract directly instead: request() flips the bool and leaves the event alone, set() sets it. The mutation now fails.
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
2 participants
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.
Replaces #762.
The bug
Both worker signal handlers raised
KeyboardInterrupt. That unwinds at an arbitrary bytecode, and the worker's shutdown event is amultiprocessing.Eventthat both workers also use as their backoff sleep (fetch_task,_send_update_task,_send_results).Interrupting
multiprocessing.Event.wait()lands insideCondition.wait(), which releases the lock N times, sleeps on a semaphore, then reacquires in afinally. An interrupt during thatfinallyleaves the lock unheld whileEvent.wait's enclosingwithblock still releases it, or leaves_sleeping_count/_woken_countskewed so the nextnotify()trips one of its three asserts.Why not just set the event from the handler
That is what #762 does, and it swaps the corruption for a hang.
Event.set()— both the threading and multiprocessing flavours — takes a non-reentrant lock. A signal arriving while the same thread is insidewait()holding that lock makes the handler block on a lock only that thread can release. The process is then unkillable by SIGTERM, so k8s waits out the grace period and SIGKILLs, losing exactly the in-flight tasks the change was meant to protect. The multiprocessing flavour is worse still: another process can be holding it.The fix
Python runs signal handlers on the main thread between bytecodes, so a plain attribute assignment is safe and anything that takes a lock is not. Handlers now only do
self._shutdown_signal.request(), which assigns a bool. The loops act on it.ShutdownSignal, wrapping athreading.Event. It was never shared with child processes — unlike the pool's own_shutdown_event— so it never needed to be amultiprocessing.Event.SHUTDOWN_POLL_INTERVAL_SEC(0.5s). The event is still there so a shutdown noticed on the main thread wakes the result thread immediately, and is only ever set from normal code.server.wait_for_termination(timeout=...)instead of relying on aserver.stop()call from inside the handler. grpc'sstop()takes its own locks and is not safe there either.KeyboardInterrupt, socli.pygets an exit code.server.start()if shutdown was already requested, and only stops a server it started.fetch_task()or viaupdate_task'sfetch_next.The last three are carried over from #762.
Known gap
A SIGTERM arriving during a blocking
get_task()or the client's all-hosts-unavailabletime.sleep()(worker/client.py:371, up to 20s) is not noticed until that call returns. The oldKeyboardInterruptdid cut those short. Fixing it properly means putting a deadline on those calls, which is a separate change.Tests
ShutdownSignalunit tests cover the timeout, both wakeup paths, and a real SIGALRM handler firing whilewait()is sleeping. Worker tests cover clean SIGTERM exit in pull and push mode, shutdown beforeserver.start(), shutdown during warmup, and the two fetch-suppression paths.ref SENTRY-5MB5
🤖 Generated with Claude Code