From eb2f6fba2a5422ad390213fae380a1acac040cdf Mon Sep 17 00:00:00 2001 From: "Sean P. Kane" Date: Sun, 18 Jan 2026 23:57:14 -0800 Subject: [PATCH] fix: startup tweaks (#41) * fix: startup tweaks * fix: Add a few safety checks --- CLAUDE.md | 32 ++++--- addon/FreecadRobustMCPBridge/Init.py | 21 +++++ addon/FreecadRobustMCPBridge/InitGui.py | 60 +++++++++++-- addon/FreecadRobustMCPBridge/RELEASE_NOTES.md | 2 +- .../freecad_mcp_bridge/server.py | 2 +- .../freecad_mcp_bridge/startup_bridge.py | 12 +++ tests/unit/addon/test_addon_autostart.py | 87 +++++++++++++++++++ 7 files changed, 196 insertions(+), 20 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c67f9dc..7b50877 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1027,6 +1027,12 @@ If you put auto-start logic in `Init.py`, it will only run when the user manuall 2. Use `QTimer.singleShot()` to defer execution until GUI is fully ready 3. The `Initialize()` method is too late - it only runs when workbench is selected +**Note on GuiWaiter vs QTimer.singleShot:** + +- `GuiWaiter` works well when called from `Init.py` (workbench selection) or `startup_bridge.py` +- However, `GuiWaiter` has timing issues when used from `InitGui.py` module-level code +- For `InitGui.py`, use `QTimer.singleShot()` with a sufficient delay (e.g., 3 seconds) + **Example pattern in InitGui.py:** ```python @@ -1036,12 +1042,14 @@ try: except ImportError: from PySide6 import QtCore -def _deferred_auto_start() -> None: +def _auto_start_bridge() -> None: """Auto-start bridge after GUI is fully ready.""" # ... auto-start logic here ... # Schedule auto-start after GUI initializes (3 second delay) -QtCore.QTimer.singleShot(3000, _deferred_auto_start) +from preferences import get_auto_start +if get_auto_start(): + QtCore.QTimer.singleShot(3000, _auto_start_bridge) ``` **This is different from regular Python packages** where `__init__.py` runs on import. FreeCAD workbenches have special loading behavior. @@ -1081,28 +1089,32 @@ When FreeCAD starts in GUI mode, there's a timing window where: - Integration tests pass initial connection, then crash on first document operation - Thread check shows `is_main_thread: False` with `thread_name: 'MCP-QueueProcessor'` even when `gui_up: True` -**The Fix (in `InitGui.py` or `startup_bridge.py`):** +**The Fix:** -Use `QTimer.singleShot()` to defer bridge startup until after GUI is ready: +The approach depends on where the code runs: + +**For `InitGui.py` module-level code:** Use `QTimer.singleShot()` with a delay: ```python # WRONG - starts bridge immediately, GUI may not be ready _auto_start_bridge() -# CORRECT - wait for GUI to be ready (use sufficient delay) -QtCore.QTimer.singleShot(3000, _deferred_auto_start) +# CORRECT - defer with sufficient delay for GUI to stabilize +QtCore.QTimer.singleShot(3000, _auto_start_bridge) ``` -For `startup_bridge.py`, use `GuiWaiter` to poll for `FreeCAD.GuiUp`: +**For `startup_bridge.py` or `Init.py`:** Use `GuiWaiter` to poll for `FreeCAD.GuiUp`: ```python -# Wait for GuiUp to become True before starting -from bridge_utils import GuiWaiter +# CORRECT - poll for GuiUp to be True, then start +from freecad_mcp_bridge.bridge_utils import GuiWaiter _gui_waiter = GuiWaiter(callback=_start_bridge, log_prefix="Startup Bridge") _gui_waiter.start() ``` -**Note:** This QTimer wait pattern is **only for GUI startup scenarios**. In headless mode (`freecadcmd`), the bridge starts directly without waiting because there is no Qt event loop to wait for. The four startup paths are: +**Note:** `GuiWaiter` has timing issues when used from `InitGui.py` module-level code due to how FreeCAD loads workbenches. Use `QTimer.singleShot()` for `InitGui.py` instead. + +**Note:** These wait patterns are **only for GUI startup scenarios**. In headless mode (`freecadcmd`), the bridge starts directly without waiting because there is no Qt event loop to wait for. The four startup paths are: 1. **GUI already up** (`FreeCAD.GuiUp = True`): Start bridge immediately with Qt timer 2. **True headless** (`QCoreApplication` exists but is NOT a `QApplication`): Start directly with background thread diff --git a/addon/FreecadRobustMCPBridge/Init.py b/addon/FreecadRobustMCPBridge/Init.py index 649f79c..42bc979 100644 --- a/addon/FreecadRobustMCPBridge/Init.py +++ b/addon/FreecadRobustMCPBridge/Init.py @@ -37,6 +37,27 @@ def _auto_start_bridge() -> None: This function is called via a deferred timer (GUI mode) or directly (headless mode) after FreeCAD finishes loading. It starts the bridge without requiring the workbench to be selected. + + Args: + None. + + Returns: + None. Early returns if auto-start is disabled or bridge is already running. + + Raises: + Exception: Any exception during bridge startup is caught, logged to + FreeCAD.Console.PrintError with full traceback, and suppressed. + + Side Effects: + - Imports and checks auto-start preference from preferences module + - Creates and starts a FreecadMCPPlugin instance if not already running + - Registers the plugin with the workbench commands module + - Prints status messages to FreeCAD.Console + + Example: + This function is typically called via QTimer or GuiWaiter callback:: + + QtCore.QTimer.singleShot(1000, _auto_start_bridge) """ try: from preferences import get_auto_start diff --git a/addon/FreecadRobustMCPBridge/InitGui.py b/addon/FreecadRobustMCPBridge/InitGui.py index 097dd57..7375389 100644 --- a/addon/FreecadRobustMCPBridge/InitGui.py +++ b/addon/FreecadRobustMCPBridge/InitGui.py @@ -183,9 +183,42 @@ try: # This runs at FreeCAD GUI startup (InitGui.py module-level code) # Note: Init.py does NOT run at startup for workbench addons, so auto-start # must be triggered from here instead. - def _deferred_auto_start() -> None: - """Auto-start bridge after GUI is fully ready.""" + # + # We use GuiWaiter to poll FreeCAD.GuiUp instead of a fixed timer delay. + # This ensures we wait for the GUI to actually be ready, rather than + # hoping a fixed delay is long enough. + def _auto_start_bridge() -> None: + """Auto-start bridge after GUI is confirmed ready. + + This is the callback invoked by GuiWaiter once FreeCAD.GuiUp is True + and a defer period has elapsed. At this point, it's safe to start + the MCP bridge with Qt timer-based queue processing. + + Args: + None. + + Returns: + None. Early returns if auto-start disabled or bridge already running. + + Raises: + Exception: Any exception during bridge startup is caught, logged + to FreeCAD.Console.PrintError with full traceback, and suppressed. + + Side Effects: + - Creates and starts a FreecadMCPPlugin instance + - Registers the plugin with the workbench commands module + - Syncs the status bar widget with bridge state + """ try: + # Safety check: verify GUI is actually ready before starting + # If not ready, reschedule for another attempt + if not FreeCAD.GuiUp: + FreeCAD.Console.PrintMessage( + "Robust MCP Bridge: GUI not ready, rescheduling auto-start...\n" + ) + QtCore.QTimer.singleShot(500, _auto_start_bridge) + return + from preferences import get_auto_start if not get_auto_start(): @@ -238,12 +271,23 @@ try: FreeCAD.Console.PrintError(traceback.format_exc()) - # Schedule auto-start after a short delay to ensure GUI is fully ready - # Use a longer delay than status bar sync to avoid race conditions - QtCore.QTimer.singleShot(3000, _deferred_auto_start) - FreeCAD.Console.PrintMessage( - "Robust MCP Bridge: Auto-start scheduled from InitGui (3s)\n" - ) + # Check if auto-start is enabled before scheduling + from preferences import get_auto_start + + if get_auto_start(): + # Schedule auto-start after a delay to ensure GUI is fully ready. + # InitGui.py module-level code runs early in FreeCAD startup, so we + # need to defer the bridge start to avoid race conditions. + # Note: We use a simple QTimer.singleShot() here because GuiWaiter + # has timing issues when used from module-level code during startup. + QtCore.QTimer.singleShot(3000, _auto_start_bridge) + FreeCAD.Console.PrintMessage( + "Robust MCP Bridge: Auto-start scheduled from InitGui (3s)\n" + ) + else: + FreeCAD.Console.PrintMessage( + "Robust MCP Bridge: Auto-start disabled in preferences\n" + ) except Exception as e: FreeCAD.Console.PrintWarning( f"Robust MCP Bridge: Could not schedule status bar sync: {e}\n" diff --git a/addon/FreecadRobustMCPBridge/RELEASE_NOTES.md b/addon/FreecadRobustMCPBridge/RELEASE_NOTES.md index f8472ff..56d3a7c 100644 --- a/addon/FreecadRobustMCPBridge/RELEASE_NOTES.md +++ b/addon/FreecadRobustMCPBridge/RELEASE_NOTES.md @@ -17,7 +17,7 @@ This release fixes some auto-start issues and improves the overall startup exper - **Auto-start not working at FreeCAD startup**: Fixed bug where auto-start only worked when manually selecting the workbench. For FreeCAD workbench addons, `Init.py` does NOT run at startup - only `InitGui.py` module-level code runs. Auto-start logic has been moved to `InitGui.py`. - **Status bar not appearing after auto-start**: The status bar widget now syncs immediately after the bridge starts, instead of on a timer that ran before the bridge was ready. -- **Integration test crashes**: Fixed race condition where the bridge could start before `FreeCAD.GuiUp` was `True`, causing Qt operations to run on a background thread and crash FreeCAD. +- **Integration test crashes**: Fixed race condition where the bridge could start before `FreeCAD.GuiUp` was `True`, causing Qt operations to run on a background thread and crash FreeCAD. Auto-start is now deferred with `QTimer.singleShot()` to allow the GUI to stabilize. ### Note diff --git a/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/server.py b/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/server.py index c26cb0f..815d6f8 100644 --- a/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/server.py +++ b/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/server.py @@ -269,8 +269,8 @@ class FreecadMCPPlugin: if FREECAD_AVAILABLE: FreeCAD.Console.PrintMessage( f"MCP Bridge started (Instance ID: {self._instance_id}):\n" - f" - JSON-RPC: {self._host}:{self._port}\n" ) + FreeCAD.Console.PrintMessage(f" - JSON-RPC: {self._host}:{self._port}\n") if self._enable_xmlrpc: FreeCAD.Console.PrintMessage( f" - XML-RPC: {self._host}:{self._xmlrpc_port}\n" diff --git a/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/startup_bridge.py b/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/startup_bridge.py index c2920ab..5e1b95e 100755 --- a/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/startup_bridge.py +++ b/addon/FreecadRobustMCPBridge/freecad_mcp_bridge/startup_bridge.py @@ -59,6 +59,12 @@ def _start_bridge() -> None: environment variables and registers the plugin with the workbench commands module for visibility to other components. + Args: + None. + + Returns: + None. Early returns if bridge is already running. + Environment Variables: FREECAD_XMLRPC_PORT: XML-RPC port (default: 9875) FREECAD_SOCKET_PORT: JSON-RPC socket port (default: 9876) @@ -73,6 +79,12 @@ def _start_bridge() -> None: - Creates and starts a FreecadMCPPlugin instance - Registers the plugin with the workbench commands module - Prints status messages to FreeCAD.Console + + Example: + This function is typically called via GuiWaiter callback or directly:: + + _gui_waiter = GuiWaiter(callback=_start_bridge) + _gui_waiter.start() """ # Check if bridge is already running (from auto-start in Init.py) from bridge_utils import get_running_plugin diff --git a/tests/unit/addon/test_addon_autostart.py b/tests/unit/addon/test_addon_autostart.py index 206f71d..b2ebad6 100644 --- a/tests/unit/addon/test_addon_autostart.py +++ b/tests/unit/addon/test_addon_autostart.py @@ -278,6 +278,93 @@ class TestGuiWaiterUsage: assert "did not become ready" in bridge_utils_code +class TestInitGuiAutoStart: + """Tests for auto-start logic in InitGui.py. + + InitGui.py is the primary entry point for auto-start at FreeCAD GUI startup. + Init.py does NOT run at startup for workbench addons, so InitGui.py must + handle auto-start using QTimer.singleShot() to defer the bridge start. + """ + + @pytest.fixture + def initgui_code(self) -> str: + """Load InitGui.py content.""" + return (ADDON_DIR / "InitGui.py").read_text() + + def test_uses_single_shot_timer_for_auto_start(self, initgui_code: str) -> None: + """InitGui.py should use QTimer.singleShot for deferred auto-start.""" + # Should use singleShot with a delay + assert "QTimer.singleShot" in initgui_code + assert "_auto_start_bridge" in initgui_code + + def test_checks_auto_start_preference(self, initgui_code: str) -> None: + """InitGui.py should check auto-start preference before scheduling. + + Uses AST-based analysis to verify there's an if-condition that calls + get_auto_start(), rather than relying on brittle string matching. + """ + tree = ast.parse(initgui_code) + + def is_get_auto_start_call(node: ast.AST) -> bool: + """Check if node is a call to get_auto_start().""" + if not isinstance(node, ast.Call): + return False + func = node.func + # Handle direct name: get_auto_start() + if isinstance(func, ast.Name) and func.id == "get_auto_start": + return True + # Handle attribute access: module.get_auto_start() + return isinstance(func, ast.Attribute) and func.attr == "get_auto_start" + + def contains_get_auto_start_call(node: ast.AST) -> bool: + """Check if node or any children contain get_auto_start call.""" + return any(is_get_auto_start_call(child) for child in ast.walk(node)) + + # Find any ast.If node whose test contains a get_auto_start call + for node in ast.walk(tree): + if isinstance(node, ast.If) and contains_get_auto_start_call(node.test): + return # Found an if-condition invoking get_auto_start() + + pytest.fail( + "InitGui.py should have an if-condition that calls get_auto_start() " + "to check whether auto-start is enabled before scheduling" + ) + + def test_auto_start_bridge_function_exists(self, initgui_code: str) -> None: + """InitGui.py should have _auto_start_bridge callback function.""" + assert "def _auto_start_bridge" in initgui_code + + def test_auto_start_bridge_checks_if_running(self, initgui_code: str) -> None: + """_auto_start_bridge should check if bridge is already running.""" + # Parse the function and check for is_bridge_running check + tree = ast.parse(initgui_code) + for node in ast.walk(tree): + if isinstance(node, ast.FunctionDef) and node.name == "_auto_start_bridge": + func_code = ast.unparse(node) + assert "is_bridge_running" in func_code + return + pytest.fail("_auto_start_bridge function not found") + + def test_auto_start_syncs_status_bar(self, initgui_code: str) -> None: + """_auto_start_bridge should sync status bar after starting bridge.""" + # Parse the function and check for sync_status_with_bridge call + tree = ast.parse(initgui_code) + for node in ast.walk(tree): + if isinstance(node, ast.FunctionDef) and node.name == "_auto_start_bridge": + func_code = ast.unparse(node) + assert "sync_status_with_bridge" in func_code + return + pytest.fail("_auto_start_bridge function not found") + + def test_logs_auto_start_scheduled(self, initgui_code: str) -> None: + """InitGui.py should log when auto-start is scheduled.""" + assert "Auto-start scheduled" in initgui_code + + def test_logs_auto_start_disabled(self, initgui_code: str) -> None: + """InitGui.py should log when auto-start is disabled.""" + assert "Auto-start disabled" in initgui_code + + class TestAutoStartCodePaths: """Tests to verify all auto-start code paths are properly handled."""