[Core] Improve Dashborad SubprocessModuleHandle.destroy_module() Resource Cleanup and Process Termination

[daiping8]

Description

Summary

This PR improves the destroy_module() method in SubprocessModuleHandle to fix race conditions, implement graceful process termination, and ensure complete resource cleanup. The changes prevent resource leaks and improve the reliability of module restart and shutdown.

Key Changes

1. Cancel health check task first to prevent race conditions
   • Why Cancel Health Check Task First: See [Core] Improve Dashborad SubprocessModuleHandle.destroy_module() Resource Cleanup and Process Termination #60214 (comment)
   • Uses smart detection to avoid canceling the current task when called internally
   • Prevents health check task from interfering with cleanup
2. Ordered resource cleanup with clear dependencies:
   • Cancel health check task first
   • Close parent connection
   • Terminate process gracefully, then forcefully if needed
   • Close HTTP client session
3. Graceful process termination:
   • First attempts graceful termination with terminate() and 5-second timeout
   • Falls back to force kill (kill()) only if necessary
   • All join() calls have timeouts to prevent infinite blocking
4. Error handling: Try-except blocks ensure cleanup continues even if one step fails

Modified Files

1. python/ray/dashboard/subprocesses/handle.py

   • Refactored destroy_module() with ordered resource cleanup
   • Implemented graceful process termination with timeout protection
   • Added smart health check task cancellation logic
     • Smart Detection Logic:

       current_task = asyncio.current_task(loop=self.loop)
       if current_task is None or self.health_check_task is not current_task:
           self.health_check_task.cancel()  # Only cancel if not current task

       This ensures:
       • External calls: Immediately cancel health check task to prevent interference
       • Internal calls: Don't cancel current task, allowing cleanup and restart to complete
   • Added comprehensive error handling and logging

2. python/ray/dashboard/subprocesses/tests/test_e2e.py

   • Added test_destroy_module_cleans_up_resources() to verify complete resource cleanup
   • Added mock classes (_DummyConn, _DummyProcess, _DummySession) for isolated testing
   • Added cleanup logic in start_http_server_app() to prevent resource leaks between tests

Related issues

Closes #60214

[gemini-code-assist]

Code Review

This pull request refactors the dashboard subprocess handling to improve graceful shutdown and resource cleanup. The changes introduce more robust process termination, explicit resource management for HTTP sessions, and attempts to handle signals for graceful exit. While the direction is excellent, I've identified a critical issue in the new signal handling logic that would cause a RuntimeError, and a high-severity race condition in the resource cleanup order. I've provided detailed comments and suggestions to address these issues, which should make the shutdown process more reliable.

[edoakes]

@sampan-s-nayak PTAL

[sampan-s-nayak]

Thanks for working on this (and for the detailed pr description). LGTM

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.