Repository navigation
[fix] exit a storage unit whose worker dies instead of hanging callers - #201
0oshowero0 merged 4 commits into
Conversation
…ng callers The worker and proxy threads are daemons, so an unhandled exception in either only unwinds that thread. The unit process stays up with its ROUTER still bound, the kernel keeps completing handshakes and queueing requests (RCVHWM/SNDHWM are 0, so nothing rejects them), and no one is left to answer. Callers cannot tell this from a slow unit, so each one blocks until its own recv timeout and raising that timeout only makes the stall longer. Exit the process instead. A unit that cannot answer is worth less than no unit at all, because its silence is indistinguishable from work in progress; exiting turns it into a visible Ray actor failure at the moment it breaks. Exit code 70 marks the cause, and os._exit avoids the atexit and flush handlers that could block on the same ZMQ state that just failed. The proxy gets the same treatment: without it nothing reaches the worker, so the unit is equally unreachable. Aborts are suppressed once _shutdown_event is set, since an exception racing a normal teardown is expected rather than a crash. TQ_STORAGE_EXIT_ON_WORKER_DEATH=0 restores the old behavior for anyone who needs to attach a profiler to the dead process, which is what TQ_HANG_HOLD_SECONDS already does on the manager side. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
OutstanderWang
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6de1343bd3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| logger.info(f"[{self.storage_unit_id}]: worker thread started...") | ||
| perf_monitor = IntervalPerfMonitor(caller_name=f"{self.storage_unit_id}") | ||
|
|
||
| try: |
There was a problem hiding this comment.
If create_zmq_socket, connect, poller construction/registration, or IntervalPerfMonitor raises (for example because ZMQ cannot allocate another socket), this new try has not started yet. Because _worker_routine runs in a daemon thread, that exception is not propagated to __init__; _start_process_put_get still launches the proxy and publishes a storage unit with no worker, recreating the hanging-unit state this change is meant to eliminate. Include setup in the fatal guard while conditionally cleaning up resources that were initialized.
Useful? React with 👍 / 👎.
| for handler in logger.handlers: | ||
| handler.flush() |
There was a problem hiding this comment.
If a configured logging handler's flush() raises, such as for a closed or broken Ray log stream, execution never reaches os._exit, so both abort paths merely unwind their daemon thread and leave the unresponsive actor alive. This direct flush is outside any protection even though the hard exit is intended to bypass failing cleanup; ensure the exit runs even when handler flushing fails.
Useful? React with 👍 / 👎.
…s fail fast Exiting the unit does not shorten a caller's wait: a DEALER keeps queueing to a dead peer, so each request still runs to its send/recv timeout and retries. What the exit changes is that the unit shows up as a dead Ray actor with exit code 70, and the post-timeout probe reports tcp=down. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
… dead worker once get_logger adds no handler when an ancestor already has one, so flushing only the module logger's own handlers could flush nothing, and os._exit then drops the abort log that explains the exit. Flush every handler up the propagation chain, and stdout and stderr. The worker's CRITICAL line also said the process and ROUTER stay up, right before the exit; it now only reports the death with its traceback. The stay-up warning remains in the TQ_STORAGE_EXIT_ON_WORKER_DEATH=0 branch, where it holds. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
OutstanderWang
changed the title
4b8cde8 to
4824c3c
Compare
Problem
A
SimpleStorageUnitanswers requests on two daemon threads: a proxy that forwards between the client-facing ROUTER and the worker, and a worker that handles each request. An unhandled exception in either thread only ends that thread. The unit process keeps running and its ROUTER stays bound. The kernel keeps completing handshakes and queueing requests:RCVHWM/SNDHWMare 0, so nothing is rejected. No thread is left to answer them.To a caller, this looks like a slow unit. Each caller blocks until its own receive timeout, and raising that timeout only makes the stall longer. Ray still reports the actor as alive, so nothing points at the unit that broke.
Change
_worker_routinenow runs the request loop, moved unchanged into a new_worker_loop, insidetry/except BaseException. On an unhandled exception it logs the death with its traceback atCRITICAL, calls_abort_process, and re-raises. Socket cleanup moves intofinally._proxy_routineis now logged with its traceback (logger.exceptioninstead oflogger.error) and also calls_abort_process. Without the proxy nothing reaches the worker, so the unit is just as unreachable._abort_process(reason)logs the reason atCRITICAL, flushes the log output, and callsos._exit(70). Exit code 70 marks the cause in Ray's actor-failure logs.os._exitskips atexit and flush handlers, which could block on the same ZMQ state that just failed.os._exitalso skips flushing,_flush_log_outputfirst flushes every handler up the logger's propagation chain, plus stdout and stderr.get_loggeradds no handler when an ancestor already has one, so flushing only the module logger's own handlers could flush nothing and drop the line that explains the exit._shutdown_eventis set: an exception that races a normal teardown is expected, not a crash.TQ_STORAGE_EXIT_ON_WORKER_DEATH=0(default1) keeps the old behavior, for example to attach a profiler to the dead unit. ACRITICALlog then says that requests to this unit will hang.Callers do not fail any faster: a DEALER keeps queueing to a dead peer, so a request to the dead unit still runs to its send/recv timeout and retries. The unit fails visibly instead: Ray reports the actor dead with exit code 70, and the probe after the last timeout reports
tcp=downrather thantcp=up verdict=unit_not_serving.Behavior is unchanged while both threads are healthy.
Tests
New
tests/test_storage_worker_death_exits.py, which runs without Ray or real sockets:_shutdown_eventis set, do not exit;TQ_STORAGE_EXIT_ON_WORKER_DEATH=0keeps the process alive.ruff checkandruff format --checkpass for the changed files.python -m pytest -q tests/test_storage_worker_death_exits.py tests/test_simple_storage_unit.py tests/test_storage_request_retry.py tests/test_storage_key_race.py tests/test_accept_probe.py: 92 passed.