fix(tests): close leaked Microscope/MPC instances to stop flaky CI segfaults

.github/workflows/main.yml

@@ -52,6 +52,25 @@ jobs:
           sudo apt install libxkbcommon-x11-0 xvfb libxcb-icccm4 libxcb-image0 libxcb-keysyms1 libxcb-randr0 libxcb-render-util0 libxcb-xinerama0 libxcb-xfixes0 x11-utils
           /sbin/start-stop-daemon --start --quiet --pidfile /tmp/custom_xvfb_99.pid --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -screen 0 1920x1200x24 -ac +extension GLX
       - name: Run the tests
-        # test_HighContentScreeningGui.py: PySide6 QObject double-init bug (separate issue)
+        # SQUID_PYTEST_HARD_EXIT: belt-and-braces against segfaults during
+        # interpreter shutdown AFTER all tests pass (Qt destructor order vs
+        # Python GC — the same crash main_hcs.py avoids with os._exit()). The
+        # conftest hook hard-exits with pytest's real status once the session
+        # is complete, so real test failures still fail the step.
         run: python3 -m pytest --ignore=tests/control/test_HighContentScreeningGui.py
         working-directory: ./software
+        env:
+          SQUID_PYTEST_HARD_EXIT: "1"
+      - name: Run the GUI test in its own process
+        # Own invocation: the full-application GUI test leaves Qt/thread state
+        # behind that deterministically segfaults later Qt tests (e.g.
+        # test_channel_sequence) when run in the same pytest process.
+        # Step-scoped pyqt5 pin: without it, pytest-qt picks PySide6 for qtbot
+        # while the app code (qtpy) uses PyQt5, and the mixed bindings fail
+        # with a QObject double-init error.
+        run: python3 -m pytest tests/control/test_HighContentScreeningGui.py
+        working-directory: ./software
+        env:
+          QT_API: pyqt5
+          PYTEST_QT_API: pyqt5
+          SQUID_PYTEST_HARD_EXIT: "1"

software/control/microscope.py

@@ -443,6 +443,7 @@ def __init__(
         skip_init: bool = False,
     ):
         self._log = squid.logging.get_logger(self.__class__.__name__)
+        self._closed = False
 
         self.stage: AbstractStage = stage
         self.camera: AbstractCamera = camera
@@ -1046,7 +1047,12 @@ def close(self) -> None:
 
         Attempts to cleanly shut down all hardware components. Errors during
         shutdown are logged but do not prevent other components from being closed.
+        Calling close() more than once is a no-op.
         """
+        if self._closed:
+            return
+        self._closed = True
+
         try:
             self.stop_live()
         except Exception as e:

software/tests/conftest.py

@@ -0,0 +1,122 @@
+"""
+Suite-wide pytest fixtures.
+
+Ensures hardware-simulation objects created during a test (Microscope,
+MultiPointController, Microcontroller) are closed at test teardown. Leaked
+instances keep daemon threads (camera streaming, laser-engine tick, slack
+notifier) and JobRunner child processes alive; those have caused CI segfaults
+both mid-suite (a leftover thread touching a destroyed Qt object) and at
+interpreter shutdown (daemon threads frozen inside C code during
+finalization).
+"""
+
+import logging
+import os
+import sys
+import tempfile
+from unittest.mock import patch
+
+import pytest
+
+import control.microcontroller
+import control.microscope
+from control.core.multi_point_controller import MultiPointController
+
+logger = logging.getLogger(__name__)
+
+# Junk dir for watchdog breadcrumbs written by leaked acquisitions while
+# cleanup_leaked_hardware closes them; nothing reads it. A plain string, not
+# pytest's tmp_path — tmp_path would cost a mkdir per test suite-wide, and the
+# breadcrumb writer creates parent dirs itself.
+_CLEANUP_STATE_DIR = os.path.join(tempfile.gettempdir(), f"squid-test-watchdog-cleanup-{os.getpid()}")
+
+
+def pytest_sessionfinish(session, exitstatus):
+    session.config._squid_exitstatus = int(exitstatus)
+
+
+def pytest_unconfigure(config):
+    """Optionally skip interpreter teardown after the test session.
+
+    A pytest process that constructed the full HCS GUI segfaults during
+    interpreter shutdown (Qt C++ destructor order conflicts with Python GC)
+    even though every test passed. main_hcs.py sidesteps the same crash with
+    os._exit(); SQUID_PYTEST_HARD_EXIT=1 lets CI do likewise, preserving
+    pytest's exit status so real test failures still fail the step.
+    """
+    if os.environ.get("SQUID_PYTEST_HARD_EXIT") == "1":
+        sys.stdout.flush()
+        sys.stderr.flush()
+        # Default 1, not 0: if pytest_sessionfinish never ran (e.g. a
+        # sessionstart failure), an unrecorded status must fail the step.
+        os._exit(getattr(config, "_squid_exitstatus", 1))
+
+
+def _make_tracking_init(original_init, instances_list):
+    """Create an __init__ wrapper that records constructed instances."""
+
+    def _tracking_init(self, *args, **kwargs):
+        original_init(self, *args, **kwargs)
+        instances_list.append(self)
+
+    return _tracking_init
+
+
+def _close_quietly(obj, label):
+    try:
+        obj.close()
+    except Exception:
+        logger.exception(f"Failed to close {label} in test cleanup")
+
+
+@pytest.fixture(autouse=True)
+def cleanup_leaked_hardware(monkeypatch):
+    """
+    Automatically close hardware-simulation objects created during each test.
+
+    Teardown order matters:
+    1. MultiPointControllers first — joins the acquisition thread and shuts
+       down JobRunner child processes while the microcontroller is still
+       alive, so the worker's stage-return move can complete instead of
+       timing out.
+    2. Microscopes next — stops camera streaming threads and closes the
+       microcontroller and addons.
+    3. Any Microcontrollers created standalone (skipped if a Microscope
+       already closed them).
+    """
+    microscopes = []
+    controllers = []
+    microcontrollers = []
+
+    with patch.object(
+        control.microscope.Microscope,
+        "__init__",
+        _make_tracking_init(control.microscope.Microscope.__init__, microscopes),
+    ), patch.object(
+        MultiPointController,
+        "__init__",
+        _make_tracking_init(MultiPointController.__init__, controllers),
+    ), patch.object(
+        control.microcontroller.Microcontroller,
+        "__init__",
+        _make_tracking_init(control.microcontroller.Microcontroller.__init__, microcontrollers),
+    ):
+        yield
+
+    # A breadcrumb written by a leaked acquisition while it is closed below
+    # must not land in the real user state dir. Setting the env var here, at
+    # teardown start, is deterministic regardless of what the test body or
+    # other fixtures did with it: their monkeypatches are already undone (they
+    # were set up after this fixture), raw os.environ writes are overwritten,
+    # and our own monkeypatch reverts this value after the fixture finishes.
+    monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", _CLEANUP_STATE_DIR)
+
+    for controller in reversed(controllers):
+        _close_quietly(controller, "MultiPointController")
+
+    for microscope in reversed(microscopes):
+        _close_quietly(microscope, "Microscope")
+
+    for micro in reversed(microcontrollers):
+        if not micro.terminate_reading_received_packet_thread:
+            _close_quietly(micro, "Microcontroller")

software/tests/control/conftest.py

@@ -1,60 +1,14 @@
 """
 Pytest fixtures for control module tests.
 
-This module provides fixtures to ensure proper cleanup of Microcontroller instances,
-preventing background threads from causing segfaults in subsequent tests.
+Microcontroller/Microscope/MultiPointController cleanup is handled suite-wide
+by the autouse fixture in tests/conftest.py.
 """
 
-import logging
-from unittest.mock import patch
-
 import pytest
 
-import control.microcontroller
 from control.firmware_sim_serial import FirmwareSimSerial
 
-logger = logging.getLogger(__name__)
-
-
-def _make_tracking_init(original_init, instances_list):
-    """Create a wrapper that tracks Microcontroller instances."""
-
-    def _tracking_init(self, *args, **kwargs):
-        original_init(self, *args, **kwargs)
-        instances_list.append(self)
-
-    return _tracking_init
-
-
-@pytest.fixture(autouse=True)
-def cleanup_microcontrollers():
-    """
-    Fixture that automatically cleans up all Microcontroller instances after each test.
-
-    This prevents background threads from causing segfaults when subsequent tests run,
-    especially those involving Qt event loops. The Microcontroller.read_received_packet
-    method runs in a background thread that must be stopped via close().
-    """
-    # Track instances created during this test (scoped to this fixture invocation)
-    active_microcontrollers = []
-
-    # Capture original __init__ at fixture runtime, not module load time
-    original_init = control.microcontroller.Microcontroller.__init__
-
-    with patch.object(
-        control.microcontroller.Microcontroller, "__init__", _make_tracking_init(original_init, active_microcontrollers)
-    ):
-        yield
-
-    # Clean up all tracked instances
-    for micro in active_microcontrollers:
-        try:
-            if hasattr(micro, "terminate_reading_received_packet_thread"):
-                if not micro.terminate_reading_received_packet_thread:
-                    micro.close()
-        except Exception as e:
-            logger.warning(f"Failed to close Microcontroller in test cleanup: {e}")
-
 
 @pytest.fixture
 def firmware_sim():

software/tests/control/test_watchdog_breadcrumbs.py

@@ -15,5 +15,8 @@ def test_run_acquisition_writes_running_breadcrumb(qtbot):
     assert rec["status"] == "running"
     assert rec["pid"] == os.getpid()
     assert rec["expected"]["timepoints"] >= 1
-    mpc.request_abort_aquisition()
+    # close() aborts the acquisition and joins its thread; that must happen
+    # before closing the scope, otherwise the thread keeps running against a
+    # closed microcontroller and dies with a TimeoutError.
+    mpc.close()
     scope.close()

software/tests/control/test_watchdog_integration.py

@@ -33,4 +33,8 @@ def test_simulated_acquisition_writes_ended_breadcrumb(qtbot):
     assert rec["status"] == "ended"
     assert rec["reason"] in {"completed", "completed_with_errors", "user_abort", "error"}
     assert rec["ended_at"] is not None
+    # The "ended" breadcrumb is written before the worker's final stage-return
+    # move; join the acquisition thread before closing the scope so it doesn't
+    # run against a closed microcontroller.
+    mpc.close()
     scope.close()