feat: request migration for SGLang - #5659
Conversation
WalkthroughThis PR introduces a centralized graceful shutdown mechanism using asyncio.Event across sglang components. A shutdown_event parameter is threaded through initialization functions and worker handlers to enable coordinated termination. Signal handlers chain to previous handlers and trigger both the shutdown_event and runtime shutdown. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/src/dynamo/sglang/request_handlers/handler_base.py (1)
309-333: Shutdown can be missed before the request ID is available.
_handle_cancellationawaitsrequest_id_futurebefore watchingshutdown_event, so a shutdown that occurs before the first response won't be detected. The task remains blocked at theawait request_id_futureline and never reaches the code that checks for shutdown. Consider racingrequest_id_futureagainstshutdown_eventconcurrently, exiting early withGeneratorExitif shutdown fires first.🐛 Suggested fix
- # Always wait for the request ID to ensure we can abort the request - sglang_request_id = await request_id_future + shutdown_task = None + if self.shutdown_event: + shutdown_task = asyncio.create_task(self.shutdown_event.wait()) + + # If shutdown happens before we have a request ID, bail out early + if shutdown_task: + done, _ = await asyncio.wait( + [request_id_future, shutdown_task], + return_when=asyncio.FIRST_COMPLETED, + ) + if shutdown_task in done: + raise GeneratorExit( + "Engine was shut down before request id became available" + ) + + # Always wait for the request ID to ensure we can abort the request + sglang_request_id = await request_id_future @@ - shutdown_task = None - - if self.shutdown_event: - # Create task for shutdown monitoring and add to wait list - shutdown_task = asyncio.create_task(self.shutdown_event.wait()) - wait_for.append(shutdown_task) + if shutdown_task: + wait_for.append(shutdown_task)components/src/dynamo/sglang/main.py (1)
39-67: Non‑leader nodes can hang on shutdown.
_handle_non_leader_node()waits on a newasyncio.Event()instead of the sharedshutdown_event, so SIGTERM/SIGINT won’t unblock it and the process can hang indefinitely. Please accept the shared event and await it.🔧 Proposed fix
-async def _handle_non_leader_node( - engine: sgl.Engine, - generate_endpoint, -) -> None: +async def _handle_non_leader_node( + engine: sgl.Engine, + generate_endpoint, + shutdown_event: asyncio.Event, +) -> None: @@ - await asyncio.Event().wait() + await shutdown_event.wait()- await _handle_non_leader_node(engine, generate_endpoint) + await _handle_non_leader_node(engine, generate_endpoint, shutdown_event)
🤖 Fix all issues with AI agents
In `@components/src/dynamo/sglang/main.py`:
- Around line 83-99: signal_handler currently calls any previous handler
(old_handlers[signum]) which for SIGINT may be signal.default_int_handler and
raises KeyboardInterrupt before runtime.shutdown() runs; change signal_handler
(the function referencing shutdown_event, loop.call_soon_threadsafe,
old_handlers, and runtime.shutdown) so that runtime.shutdown() is guaranteed to
run first (e.g., schedule/await shutdown_event and call runtime.shutdown in a
try/finally) and only invoke the old handler afterwards, or explicitly skip
calling old_handler when it is signal.default_int_handler; if you must call
old_handler preserve it inside a try/except so a raised KeyboardInterrupt does
not prevent runtime.shutdown().
kthui
left a comment
There was a problem hiding this comment.
I see the strategy here - suppressing the overwrite does ensure Dynamo keeps control of the signal, which is great.
However, I have a concern about discarding the handler SGLang tries to register. By returning None in watching_add_signal_handler, we are effectively "silencing" SGLang's internal shutdown logic. If the sgl.Engine relies on that handler to flush buffers, detach from a Ray cluster, or clean up shared memory, those operations will never happen.
Instead of just suppressing it, we should capture the callback and invoke it manually within our own signal_handler.
Updated:
All migration test pass |
kthui
left a comment
There was a problem hiding this comment.
Thanks for preserving the SGLang handler.
The logic looks generally correct to me, but I have one shutdown flow item and one cleanup suggestion:
- Ordering Confirmation: Could we hit a race condition if SGLang shutdown signal handler is called before
runtime.shutdown()? - Code Cleanup: The
install_graceful_shutdownimplementation can be better refined - e.g.runtime.shutdown()returnsNone, so theasyncio.iscoroutinecheck that followed is unnecessary. The cleanup can be done in a follow-up PR.
|
@ishandhanani could you help double-check the monkey patch logic on Python shutdown signal handler? High-level flow summary for verification:
|
kthui
left a comment
There was a problem hiding this comment.
Thanks for the updates, and cleaning up corner cases!
For the record, here is the summary of the SGLang backend shutdown flow:
- Register SGLang backend shutdown signal handler.
- Monkey-patch
loop.add_signal_handlerto intercept registration. - Capture and store SGLang's handler when it tries to register.
- On shutdown signal:
a. Stop accepting new requests -runtime.shutdown().
b. Cancel and return incomplete for all ongoing requests -shutdown_event.set().
c. Clean up the engine -handler.cleanup().
d. Call the captured SGLang shutdown signal handler.
Overview:
Implement request migration when worker shuts down for sglang
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.