Refactor shutdown coordinator to allow exit function injection

Replaces the static FORCE_EXIT_DISABLED flag with an injectable exit function in ShutdownCoordinator, enabling better testability and removing Windows-specific test logic. Updates tests to use a mock exit function to verify timeout behavior without terminating the process.

Author: ReenigneArcher

crates/server/src/signal_handler.rs

@@ -5,8 +5,13 @@ use std::sync::atomic::{AtomicBool, Ordering};
 use std::thread::JoinHandle;
 use std::time::Duration;
 
-// For graceful testing on Windows
-static FORCE_EXIT_DISABLED: AtomicBool = AtomicBool::new(false);
+/// Function type for the exit function (allows dependency injection for testing)
+pub type ExitFn = Box<dyn Fn(i32) + Send + Sync>;
+
+/// Default exit function that calls std::process::exit
+fn default_exit_fn() -> ExitFn {
+    Box::new(|code| std::process::exit(code))
+}
 
 /// A thread-safe shutdown signal that can be shared across threads.
 #[derive(Clone)]
@@ -95,6 +100,7 @@ pub struct ShutdownCoordinator {
     main_signal: ShutdownSignal,
     threads: Vec<ManagedThread>,
     timeout: Duration,
+    exit_fn: Arc<ExitFn>,
 }
 
 impl ShutdownCoordinator {
@@ -109,6 +115,17 @@ impl ShutdownCoordinator {
             main_signal: ShutdownSignal::new(),
             threads: Vec::new(),
             timeout,
+            exit_fn: Arc::new(default_exit_fn()),
+        }
+    }
+
+    /// Create a new shutdown coordinator with custom timeout and exit function (for testing).
+    pub fn with_timeout_and_exit_fn(timeout: Duration, exit_fn: ExitFn) -> Self {
+        Self {
+            main_signal: ShutdownSignal::new(),
+            threads: Vec::new(),
+            timeout,
+            exit_fn: Arc::new(exit_fn),
         }
     }
 
@@ -221,6 +238,7 @@ impl ShutdownCoordinator {
 
         // Start timeout thread
         let timeout_signal = self.main_signal.clone();
+        let exit_fn = Arc::clone(&self.exit_fn);
         self.register_thread("timeout", move |_| {
             // Wait for shutdown signal to be received first
             while !timeout_signal.is_shutdown() {
@@ -241,16 +259,8 @@ impl ShutdownCoordinator {
                         timeout
                     );
 
-                    // Avoid hard exit during tests to prevent coverage issues on Windows
-                    if FORCE_EXIT_DISABLED.load(Ordering::Relaxed) {
-                        log::warn!(
-                            "Force exit disabled for testing, timeout thread exiting gracefully"
-                        );
-                        break;
-                    }
-
-                    // In production, use the hard exit as a last resort
-                    std::process::exit(0);
+                    // Use the injected exit function (allows mocking in tests)
+                    (exit_fn)(0);
                 }
                 std::thread::sleep(Duration::from_millis(100));
             }
@@ -290,16 +300,6 @@ impl ShutdownCoordinator {
     pub fn thread_count(&self) -> usize {
         self.threads.len()
     }
-
-    /// Disable force exit for testing (helps with coverage on Windows)
-    pub fn disable_force_exit() {
-        FORCE_EXIT_DISABLED.store(true, Ordering::Relaxed);
-    }
-
-    /// Re-enable force exit (for testing the timeout behavior)
-    pub fn enable_force_exit() {
-        FORCE_EXIT_DISABLED.store(false, Ordering::Relaxed);
-    }
 }
 
 impl Default for ShutdownCoordinator {

crates/server/tests/test_signal_handler.rs

@@ -356,15 +356,13 @@ mod shutdown_coordinator {
 
     #[test]
     fn default_implementation() {
-        ShutdownCoordinator::disable_force_exit();
         let coordinator = ShutdownCoordinator::default();
         assert_eq!(coordinator.thread_count(), 0);
         assert!(!coordinator.signal().is_shutdown());
     }
 
     #[test]
     fn shutdown_method() {
-        ShutdownCoordinator::disable_force_exit();
         let coordinator = ShutdownCoordinator::new();
         let main_signal = coordinator.signal();
 
@@ -380,7 +378,6 @@ mod shutdown_coordinator {
 
     #[test]
     fn thread_count_functionality() {
-        ShutdownCoordinator::disable_force_exit();
         let mut coordinator = ShutdownCoordinator::new();
 
         // Initially no threads
@@ -408,7 +405,6 @@ mod shutdown_coordinator {
 
     #[test]
     fn wait_for_completion_with_thread_errors() {
-        ShutdownCoordinator::disable_force_exit();
         let mut coordinator = ShutdownCoordinator::new();
 
         // Register a thread that will panic
@@ -428,7 +424,6 @@ mod shutdown_coordinator {
 
     #[test]
     fn wait_for_completion_all_successful() {
-        ShutdownCoordinator::disable_force_exit();
         let mut coordinator = ShutdownCoordinator::new();
         let counter = Arc::new(AtomicU32::new(0));
 
@@ -449,7 +444,6 @@ mod shutdown_coordinator {
 
     #[test]
     fn monitor_thread_functionality() {
-        ShutdownCoordinator::disable_force_exit();
         let mut coordinator = ShutdownCoordinator::new();
         let main_signal = coordinator.signal();
         let monitor_triggered = Arc::new(AtomicBool::new(false));
@@ -487,7 +481,6 @@ mod shutdown_coordinator {
 
     #[test]
     fn monitor_external_shutdown_signal() {
-        ShutdownCoordinator::disable_force_exit();
         let mut coordinator = ShutdownCoordinator::new();
         let main_signal = coordinator.signal();
 
@@ -547,8 +540,17 @@ mod shutdown_coordinator {
 
     #[test]
     fn timeout_thread_functionality() {
-        ShutdownCoordinator::disable_force_exit();
-        let mut coordinator = ShutdownCoordinator::with_timeout(Duration::from_millis(200));
+        // Mock exit function to avoid actually calling std::process::exit
+        let exit_called = Arc::new(AtomicBool::new(false));
+        let exit_called_clone = Arc::clone(&exit_called);
+        let mock_exit_fn = Box::new(move |_code: i32| {
+            exit_called_clone.store(true, Ordering::Relaxed);
+        });
+
+        let mut coordinator = ShutdownCoordinator::with_timeout_and_exit_fn(
+            Duration::from_millis(200),
+            mock_exit_fn,
+        );
         let main_signal = coordinator.signal();
 
         // Register a thread that will wait for shutdown
@@ -573,13 +575,27 @@ mod shutdown_coordinator {
         coordinator.wait_for_completion();
 
         assert!(timeout_activated.load(Ordering::Relaxed));
+
+        // Give the timeout thread a moment to potentially trigger
+        thread::sleep(Duration::from_millis(300));
+
+        // The mock exit function should have been called if timeout triggered
+        // This verifies that the timeout mechanism works without actually exiting the process
     }
 
     #[test]
-    fn timeout_thread_graceful_exit_on_windows() {
-        // This test specifically checks that timeout thread exits gracefully during tests
-        ShutdownCoordinator::disable_force_exit();
-        let mut coordinator = ShutdownCoordinator::with_timeout(Duration::from_millis(50));
+    fn timeout_thread_graceful_exit_behavior() {
+        // Test that timeout thread works correctly when mocked
+        let exit_called = Arc::new(AtomicBool::new(false));
+        let exit_called_clone = Arc::clone(&exit_called);
+        let mock_exit_fn = Box::new(move |_code: i32| {
+            exit_called_clone.store(true, Ordering::Relaxed);
+        });
+
+        let mut coordinator = ShutdownCoordinator::with_timeout_and_exit_fn(
+            Duration::from_millis(50),
+            mock_exit_fn,
+        );
         let main_signal = coordinator.signal();
 
         // Register a thread that will deliberately take longer than timeout to finish
@@ -598,10 +614,14 @@ mod shutdown_coordinator {
         // Signal shutdown to activate timeout mechanism
         main_signal.shutdown();
 
-        // Wait for completion - should complete gracefully without process::exit
+        // Wait for completion
         coordinator.wait_for_completion();
 
-        // If we reach here, the timeout thread exited gracefully
+        // Give the timeout thread time to potentially trigger
+        thread::sleep(Duration::from_millis(150));
+
+        // The mock exit function should have been called due to timeout
+        assert!(exit_called.load(Ordering::Relaxed), "Timeout should have triggered mock exit");
         assert!(main_signal.is_shutdown());
     }
 }