feat: signal handler race condition (#170)

* refactor: Fix race condition in signal handler calling sys.exit during async execu

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

* fix: address review comments - restore sys.exit and add concurrent test

- Revert os._exit() to sys.exit() to allow proper stack unwinding and
  finally block execution (prevents leaking external resources)
- Update docstrings to clarify 'at-most-once' execution semantics
- Add concurrent cleanup test to verify thread-safety under parallel access
- The lock now correctly ensures cleanup runs only once while preserving
  proper Python cleanup semantics

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

* fix: use RLock to prevent signal handler deadlock

- Change threading.Lock to threading.RLock to prevent deadlock when signal
  arrives while atexit holds the lock (signal handlers run in same thread)
- Remove test_docker_manager_has_cleanup_lock (tests implementation details)
- Clean up dead code in concurrent test (unused tracking variables)
- Update docstring to document RLock usage

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

* fix: prevent signal handler from interrupting ongoing cleanup

- Add _cleanup_in_progress flag to detect re-entrant calls during cleanup
- Move entire cleanup operation inside the lock to prevent partial cleanup
- Return status from _cleanup_resources (True=done, False=already done, None=in progress)
- Signal handler returns without sys.exit() if cleanup is in progress,
  allowing ongoing cleanup to complete without SystemExit interruption
- Add sys.stdout.flush() before os._exit(1) to ensure message is printed
- Set _cleanup_done after cleanup completes (not before) for accurate state
- Add test for in-progress cleanup detection

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

* fix: apply race condition fixes to BinaryManager and improve tests

- Apply same thread-safe cleanup pattern to BinaryManager:
  - Add RLock, _cleanup_in_progress, _cleanup_done flags
  - Update signal handler to check cleanup result before sys.exit()
  - Add sys.stdout.flush() before os._exit(1)
  - Move cleanup operations inside lock with try/finally
- Fix _cleanup_on_exit in both managers to call cleanup unconditionally
  (it's idempotent and returns immediately if already done/in progress)
- Update concurrent test to verify return value distribution:
  exactly one True (performed cleanup), rest False (already done)

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

* refactor: use CleanupResult enum and add BinaryManager tests

- Add CleanupResult enum (PERFORMED, ALREADY_DONE, IN_PROGRESS) to constants.py
  for self-documenting cleanup return values
- Update DockerManager and BinaryManager to use CleanupResult enum
- Update DockerManager tests to use CleanupResult enum
- Add BinaryManager tests for:
  - Double cleanup prevention
  - Concurrent access handling with return value distribution
  - In-progress state detection

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

* fix: restore deleted tests and add IN_PROGRESS assertion

- Restore all graceful shutdown tests from master that were lost during rebase
- Restore all CORS validation and configuration tests
- Add IN_PROGRESS assertion to concurrent access tests to verify no re-entrancy bugs
- Rename flag state tests to clarify they test flag check logic specifically

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

* refactor: extract common cleanup logic into CleanupMixin

- Create CleanupMixin ABC in cleanup_mixin.py with:
  - _init_cleanup_state(): Initialize cleanup-related instance variables
  - _setup_signal_handlers(): Register SIGINT/SIGTERM handlers and atexit
  - _signal_handler(): Handle signals with graceful shutdown
  - _cleanup_on_exit(): Atexit handler
  - _cleanup_resources_guarded(): Thread-safe cleanup wrapper with RLock
  - _cleanup_resources(): Default implementation calling _do_cleanup()
  - remove_signal_handlers(): Restore original signal handlers
  - Abstract _do_cleanup(): Subclasses implement actual cleanup

- Update DockerManager to inherit from CleanupMixin:
  - Call _init_cleanup_state() in __init__
  - Implement _do_cleanup() with container graceful shutdown
  - Override _cleanup_resources() to add drain_timeout/stop_timeout params

- Update BinaryManager to inherit from CleanupMixin:
  - Call _init_cleanup_state() in __init__
  - Implement _do_cleanup() with process termination logic
  - Use default _cleanup_resources() from mixin

This eliminates ~90 lines of duplicated code between the managers while
maintaining identical behavior and thread-safety guarantees.

Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Sandi Fatic <chefsale@users.noreply.github.com>

Author: chefsale

merobox/commands/binary_manager.py

@@ -2,7 +2,6 @@
 Binary Manager - Manages Calimero nodes as native processes (no Docker).
 """
 
-import atexit
 import os
 import re
 import shutil
@@ -17,6 +16,7 @@
 
 from rich.console import Console
 
+from merobox.commands.cleanup_mixin import CleanupMixin
 from merobox.commands.config_utils import (
     apply_bootstrap_nodes,
     apply_e2e_defaults,
@@ -32,7 +32,7 @@
 console = Console()
 
 
-class BinaryManager:
+class BinaryManager(CleanupMixin):
     """Manages Calimero nodes as native binary processes."""
 
     def __init__(
@@ -51,6 +51,8 @@ def __init__(
                 shutdown on SIGINT/SIGTERM. Set to False in tests or when managing
                 signals externally.
         """
+        self._init_cleanup_state()
+
         if (
             binary_path
             and os.path.isfile(binary_path)
@@ -68,50 +70,12 @@ def __init__(
         self.node_rpc_ports: dict[str, int] = {}
         self.pid_file_dir = Path("./data/.pids")
         self.pid_file_dir.mkdir(parents=True, exist_ok=True)
-        self._shutting_down = False
-        self._original_sigint_handler = None
-        self._original_sigterm_handler = None
 
         if enable_signal_handlers:
             self._setup_signal_handlers()
 
-    def _setup_signal_handlers(self):
-        """Register signal handlers for graceful shutdown."""
-        # Store original handlers so we can restore them if needed
-        self._original_sigint_handler = signal.signal(
-            signal.SIGINT, self._signal_handler
-        )
-        self._original_sigterm_handler = signal.signal(
-            signal.SIGTERM, self._signal_handler
-        )
-        # Also register atexit handler for cleanup on normal exit
-        atexit.register(self._cleanup_on_exit)
-
-    def _signal_handler(self, signum, frame):
-        """Handle SIGINT/SIGTERM signals for graceful shutdown."""
-        if self._shutting_down:
-            # Already shutting down, force exit on second signal
-            console.print("\n[red]Forced exit requested, terminating...[/red]")
-            sys.exit(1)
-
-        self._shutting_down = True
-        sig_name = "SIGINT" if signum == signal.SIGINT else "SIGTERM"
-        console.print(
-            f"\n[yellow]Received {sig_name}, initiating graceful shutdown...[/yellow]"
-        )
-
-        self._cleanup_resources()
-
-        # Exit cleanly
-        sys.exit(0)
-
-    def _cleanup_on_exit(self):
-        """Cleanup handler for atexit - only runs if not already cleaned up."""
-        if not self._shutting_down:
-            self._cleanup_resources()
-
-    def _cleanup_resources(self):
-        """Stop all managed processes."""
+    def _do_cleanup(self):
+        """Perform the actual process cleanup."""
         if self.processes:
             console.print("[cyan]Stopping managed processes...[/cyan]")
             for node_name in list(self.processes.keys()):
@@ -132,15 +96,6 @@ def _cleanup_resources(self):
             self.processes.clear()
             self.node_rpc_ports.clear()
 
-    def remove_signal_handlers(self):
-        """Remove signal handlers and restore original handlers."""
-        if self._original_sigint_handler is not None:
-            signal.signal(signal.SIGINT, self._original_sigint_handler)
-            self._original_sigint_handler = None
-        if self._original_sigterm_handler is not None:
-            signal.signal(signal.SIGTERM, self._original_sigterm_handler)
-            self._original_sigterm_handler = None
-
     def _find_binary(self, require: bool = True) -> Optional[str]:
         """Find the merod binary in PATH or common locations.
 

merobox/commands/cleanup_mixin.py

@@ -0,0 +1,163 @@
+"""
+Cleanup Mixin - Thread-safe cleanup coordination for manager classes.
+
+Provides a common pattern for signal handling and resource cleanup that can be
+shared between DockerManager and BinaryManager.
+"""
+
+import atexit
+import os
+import signal
+import sys
+import threading
+from abc import ABC, abstractmethod
+
+from rich.console import Console
+
+from merobox.commands.constants import CleanupResult
+
+console = Console()
+
+
+class CleanupMixin(ABC):
+    """Mixin providing thread-safe cleanup coordination for manager classes.
+
+    This mixin handles:
+    - Signal handler registration (SIGINT/SIGTERM)
+    - Thread-safe cleanup guards (RLock, _cleanup_in_progress, _cleanup_done)
+    - Atexit handler registration
+    - Graceful shutdown coordination
+
+    Subclasses must implement:
+    - _do_cleanup(): Perform the actual resource cleanup (stop containers/processes)
+
+    Usage:
+        class MyManager(CleanupMixin):
+            def __init__(self, enable_signal_handlers=True):
+                self._init_cleanup_state()
+                # ... other initialization ...
+                if enable_signal_handlers:
+                    self._setup_signal_handlers()
+
+            def _do_cleanup(self):
+                # Stop your resources here
+                pass
+    """
+
+    def _init_cleanup_state(self):
+        """Initialize cleanup-related instance variables.
+
+        Call this in __init__ before _setup_signal_handlers().
+        """
+        self._shutting_down = False
+        self._cleanup_lock = threading.RLock()
+        self._cleanup_in_progress = False
+        self._cleanup_done = False
+        self._original_sigint_handler = None
+        self._original_sigterm_handler = None
+
+    def _setup_signal_handlers(self):
+        """Register signal handlers for graceful shutdown."""
+        self._original_sigint_handler = signal.signal(
+            signal.SIGINT, self._signal_handler
+        )
+        self._original_sigterm_handler = signal.signal(
+            signal.SIGTERM, self._signal_handler
+        )
+        atexit.register(self._cleanup_on_exit)
+
+    def _signal_handler(self, signum, frame):
+        """Handle SIGINT/SIGTERM signals for graceful shutdown.
+
+        Uses sys.exit() to allow proper stack unwinding and finally block
+        execution. If cleanup is already in progress (e.g., via atexit), we
+        return without calling sys.exit() to avoid interrupting the ongoing
+        cleanup with SystemExit.
+        """
+        if self._shutting_down:
+            console.print("\n[red]Forced exit requested, terminating...[/red]")
+            sys.stdout.flush()
+            os._exit(1)
+
+        self._shutting_down = True
+        sig_name = "SIGINT" if signum == signal.SIGINT else "SIGTERM"
+        console.print(
+            f"\n[yellow]Received {sig_name}, initiating graceful shutdown...[/yellow]"
+        )
+
+        cleanup_result = self._cleanup_resources()
+
+        if cleanup_result != CleanupResult.IN_PROGRESS:
+            sys.exit(0)
+
+    def _cleanup_on_exit(self):
+        """Cleanup handler for atexit.
+
+        Calls _cleanup_resources unconditionally since it's idempotent and
+        will return immediately if cleanup was already done or is in progress.
+        """
+        self._cleanup_resources()
+
+    def _cleanup_resources_guarded(self, cleanup_fn, *args, **kwargs) -> CleanupResult:
+        """Execute cleanup function with thread-safe guards.
+
+        Thread-safe guard ensuring at-most-once execution semantics. Uses RLock
+        to allow re-entrant calls from signal handlers in the same thread.
+
+        Args:
+            cleanup_fn: The cleanup function to execute (typically self._do_cleanup)
+            *args, **kwargs: Arguments to pass to cleanup_fn
+
+        Returns:
+            CleanupResult.PERFORMED: Cleanup was executed by this call
+            CleanupResult.ALREADY_DONE: Cleanup was already completed previously
+            CleanupResult.IN_PROGRESS: Cleanup is currently in progress (re-entrant call)
+        """
+        with self._cleanup_lock:
+            if self._cleanup_done:
+                return CleanupResult.ALREADY_DONE
+            if self._cleanup_in_progress:
+                return CleanupResult.IN_PROGRESS
+            self._cleanup_in_progress = True
+
+            try:
+                cleanup_fn(*args, **kwargs)
+            finally:
+                self._cleanup_done = True
+                self._cleanup_in_progress = False
+
+        return CleanupResult.PERFORMED
+
+    def _cleanup_resources(self) -> CleanupResult:
+        """Stop all managed resources with thread-safe guards.
+
+        Default implementation that calls _do_cleanup() with no arguments.
+        Subclasses can override this to add parameters (like drain_timeout).
+
+        Returns:
+            CleanupResult.PERFORMED: Cleanup was executed by this call
+            CleanupResult.ALREADY_DONE: Cleanup was already completed previously
+            CleanupResult.IN_PROGRESS: Cleanup is currently in progress (re-entrant call)
+        """
+        return self._cleanup_resources_guarded(self._do_cleanup)
+
+    @abstractmethod
+    def _do_cleanup(self, *args, **kwargs):
+        """Perform the actual resource cleanup.
+
+        Implement this method to stop containers, processes, or other resources.
+        This method is called inside the cleanup lock, so it's guaranteed to run
+        at most once.
+
+        Note: Do not call sys.exit() or raise SystemExit from this method.
+        """
+        pass
+
+    def remove_signal_handlers(self):
+        """Remove signal handlers and restore original handlers."""
+        if self._original_sigint_handler is not None:
+            signal.signal(signal.SIGINT, self._original_sigint_handler)
+            self._original_sigint_handler = None
+        if self._original_sigterm_handler is not None:
+            signal.signal(signal.SIGTERM, self._original_sigterm_handler)
+            self._original_sigterm_handler = None

merobox/commands/constants.py

@@ -2,6 +2,23 @@
 Constants and configuration values used across the merobox codebase.
 """
 
+from enum import Enum, auto
+
+
+class CleanupResult(Enum):
+    """Result of cleanup operation for thread-safe cleanup methods.
+
+    Used by DockerManager and BinaryManager to communicate cleanup status:
+    - PERFORMED: Cleanup was executed by this call
+    - ALREADY_DONE: Cleanup was already completed by a previous call
+    - IN_PROGRESS: Cleanup is currently in progress (re-entrant call)
+    """
+
+    PERFORMED = auto()
+    ALREADY_DONE = auto()
+    IN_PROGRESS = auto()
+
+
 # API Endpoints
 JSONRPC_ENDPOINT = "/jsonrpc"
 ADMIN_API_BASE = "/admin-api"