From 2bbb5e813673ad260d3c0e2f728dcfab3dca669d Mon Sep 17 00:00:00 2001 From: You Yan Date: Tue, 28 Jul 2026 23:12:02 -0700 Subject: [PATCH 1/3] fix(tests): close leaked Microscope/MPC instances to stop CI segfaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI has been failing ~30% of runs since 2026-06-19 with segfaults (exit 139) — usually during interpreter shutdown after every test passed, occasionally mid-suite inside the Qt event loop. Root cause: ~40 test call sites build a full simulated Microscope/MultiPointController and most never close them, leaking camera-streaming/laser-engine/slack daemon threads and prewarmed JobRunner child processes. A leftover thread touching Qt at teardown (or frozen inside C code during finalization) segfaults the process. - tests/conftest.py (new): suite-wide autouse fixture that tracks every Microscope, MultiPointController and Microcontroller built during a test and closes them at teardown in dependency order (MPC first so the acquisition thread joins while the microcontroller is still alive). Replaces the Microcontroller-only fixture in tests/control/conftest.py and now also covers tests/squid and root-level tests. - Microscope.close() is now idempotent so explicit closes in tests don't double-close every component when the fixture runs. - Watchdog tests join the acquisition thread (mpc.close()) before closing the scope; previously the leaked thread died ~3s later with "TimeoutError: Current mcu operation timed out" in every CI run. Also incorporates #595 (by @hongquanli) as belt-and-braces: - SQUID_PYTEST_HARD_EXIT=1 + conftest hook os._exit()s with pytest's real status after the session, skipping the crash-prone interpreter teardown (same trick main_hcs.py uses). - test_HighContentScreeningGui.py runs in its own pytest process pinned to PyQt5 instead of being skipped. Co-Authored-By: Claude Fable 5 --- .github/workflows/main.yml | 21 +++- software/control/microscope.py | 5 + software/tests/conftest.py | 114 ++++++++++++++++++ software/tests/control/conftest.py | 50 +------- .../control/test_watchdog_breadcrumbs.py | 3 + .../control/test_watchdog_integration.py | 4 + 6 files changed, 148 insertions(+), 49 deletions(-) create mode 100644 software/tests/conftest.py diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2a4fa735f..0aa741ebb 100644 --- a/.github/workflows/main.yml +++ b/.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" diff --git a/software/control/microscope.py b/software/control/microscope.py index 5ee8b3e8e..ad856d10f 100644 --- a/software/control/microscope.py +++ b/software/control/microscope.py @@ -1046,7 +1046,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 getattr(self, "_closed", False): + return + self._closed = True + try: self.stop_live() except Exception as e: diff --git a/software/tests/conftest.py b/software/tests/conftest.py new file mode 100644 index 000000000..469dd0a7f --- /dev/null +++ b/software/tests/conftest.py @@ -0,0 +1,114 @@ +""" +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 +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__) + + +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 + + +@pytest.fixture(autouse=True) +def cleanup_leaked_hardware(tmp_path, 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). + """ + # Safety net: this fixture tears down after test-scoped monkeypatches are + # undone, so a leaked acquisition finishing during cleanup would otherwise + # write its watchdog breadcrumb to the real user state dir. + monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", str(tmp_path / "watchdog-cleanup")) + + 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 + + for controller in reversed(controllers): + try: + controller.close() + except Exception: + logger.exception("Failed to close MultiPointController in test cleanup") + + for microscope in reversed(microscopes): + try: + microscope.close() + except Exception: + logger.exception("Failed to close Microscope in test cleanup") + + for micro in reversed(microcontrollers): + try: + if not micro.terminate_reading_received_packet_thread: + micro.close() + except Exception: + logger.exception("Failed to close Microcontroller in test cleanup") diff --git a/software/tests/control/conftest.py b/software/tests/control/conftest.py index 485098188..ff41628ef 100644 --- a/software/tests/control/conftest.py +++ b/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(): diff --git a/software/tests/control/test_watchdog_breadcrumbs.py b/software/tests/control/test_watchdog_breadcrumbs.py index fc9b27daa..40ff84c6d 100644 --- a/software/tests/control/test_watchdog_breadcrumbs.py +++ b/software/tests/control/test_watchdog_breadcrumbs.py @@ -16,4 +16,7 @@ def test_run_acquisition_writes_running_breadcrumb(qtbot): assert rec["pid"] == os.getpid() assert rec["expected"]["timepoints"] >= 1 mpc.request_abort_aquisition() + # Join the acquisition thread before closing the scope, otherwise it keeps + # running against a closed microcontroller and dies with a TimeoutError. + mpc.close() scope.close() diff --git a/software/tests/control/test_watchdog_integration.py b/software/tests/control/test_watchdog_integration.py index 1cf35bd78..54d2be8df 100644 --- a/software/tests/control/test_watchdog_integration.py +++ b/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() From 4392fe145b1b23c918d07ca9e1e63ad362c9b39a Mon Sep 17 00:00:00 2001 From: You Yan Date: Wed, 29 Jul 2026 09:57:10 -0700 Subject: [PATCH 2/3] fix(tests): clarify fixture teardown-order comment, pin state dir in teardown Address Copilot review on #604: the old comment conflated other fixtures' monkeypatches (undone before this teardown) with our own (undone after), and a raw os.environ write in a test body could have redirected teardown-time breadcrumb writes; re-apply the tmp state dir after yield. Co-Authored-By: Claude Fable 5 --- software/tests/conftest.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/software/tests/conftest.py b/software/tests/conftest.py index 469dd0a7f..56927e0b9 100644 --- a/software/tests/conftest.py +++ b/software/tests/conftest.py @@ -70,10 +70,14 @@ def cleanup_leaked_hardware(tmp_path, monkeypatch): 3. Any Microcontrollers created standalone (skipped if a Microscope already closed them). """ - # Safety net: this fixture tears down after test-scoped monkeypatches are - # undone, so a leaked acquisition finishing during cleanup would otherwise - # write its watchdog breadcrumb to the real user state dir. - monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", str(tmp_path / "watchdog-cleanup")) + # Safety net for teardown-time breadcrumb writes: a leaked acquisition + # finishing during the cleanup below must not write to the real user state + # dir. Ordering: monkeypatches set up after this fixture (e.g. the autouse + # _watchdog_state_to_tmp in tests/control/conftest.py, or a test's own) + # are already undone when this teardown runs, restoring the value set + # here; our own monkeypatch reverts only after this fixture finishes. + cleanup_state_dir = str(tmp_path / "watchdog-cleanup") + monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", cleanup_state_dir) microscopes = [] controllers = [] @@ -94,6 +98,10 @@ def cleanup_leaked_hardware(tmp_path, monkeypatch): ): yield + # Re-apply in case the test body changed the env var with a raw + # os.environ write, which nothing has undone at this point. + monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", cleanup_state_dir) + for controller in reversed(controllers): try: controller.close() From 4ea14570737305d9a7a30b4fd7a361b8855daba6 Mon Sep 17 00:00:00 2001 From: You Yan Date: Wed, 29 Jul 2026 10:23:16 -0700 Subject: [PATCH 3/3] refactor(tests): simplify cleanup fixture per review - Factor the copy-paste close loops into _close_quietly() - Drop tmp_path from the autouse fixture: it cost a real mkdir+dir-scan for every test in the suite; use a module-level tempfile.gettempdir() path and set the env var once, at teardown start, which is deterministic regardless of fixture/env ordering (replaces the pre-yield set + re-apply pair) - Initialize Microscope._closed in __init__ instead of getattr-with-default - Drop redundant request_abort_aquisition() before mpc.close() (close aborts) Co-Authored-By: Claude Fable 5 --- software/control/microscope.py | 3 +- software/tests/conftest.py | 52 +++++++++---------- .../control/test_watchdog_breadcrumbs.py | 6 +-- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/software/control/microscope.py b/software/control/microscope.py index ad856d10f..94e4ef186 100644 --- a/software/control/microscope.py +++ b/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 @@ -1048,7 +1049,7 @@ def close(self) -> None: shutdown are logged but do not prevent other components from being closed. Calling close() more than once is a no-op. """ - if getattr(self, "_closed", False): + if self._closed: return self._closed = True diff --git a/software/tests/conftest.py b/software/tests/conftest.py index 56927e0b9..253574ea5 100644 --- a/software/tests/conftest.py +++ b/software/tests/conftest.py @@ -13,6 +13,7 @@ import logging import os import sys +import tempfile from unittest.mock import patch import pytest @@ -23,6 +24,12 @@ 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) @@ -55,8 +62,15 @@ def _tracking_init(self, *args, **kwargs): 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(tmp_path, monkeypatch): +def cleanup_leaked_hardware(monkeypatch): """ Automatically close hardware-simulation objects created during each test. @@ -70,15 +84,6 @@ def cleanup_leaked_hardware(tmp_path, monkeypatch): 3. Any Microcontrollers created standalone (skipped if a Microscope already closed them). """ - # Safety net for teardown-time breadcrumb writes: a leaked acquisition - # finishing during the cleanup below must not write to the real user state - # dir. Ordering: monkeypatches set up after this fixture (e.g. the autouse - # _watchdog_state_to_tmp in tests/control/conftest.py, or a test's own) - # are already undone when this teardown runs, restoring the value set - # here; our own monkeypatch reverts only after this fixture finishes. - cleanup_state_dir = str(tmp_path / "watchdog-cleanup") - monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", cleanup_state_dir) - microscopes = [] controllers = [] microcontrollers = [] @@ -98,25 +103,20 @@ def cleanup_leaked_hardware(tmp_path, monkeypatch): ): yield - # Re-apply in case the test body changed the env var with a raw - # os.environ write, which nothing has undone at this point. - monkeypatch.setenv("SQUID_WATCHDOG_STATE_DIR", cleanup_state_dir) + # 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): - try: - controller.close() - except Exception: - logger.exception("Failed to close MultiPointController in test cleanup") + _close_quietly(controller, "MultiPointController") for microscope in reversed(microscopes): - try: - microscope.close() - except Exception: - logger.exception("Failed to close Microscope in test cleanup") + _close_quietly(microscope, "Microscope") for micro in reversed(microcontrollers): - try: - if not micro.terminate_reading_received_packet_thread: - micro.close() - except Exception: - logger.exception("Failed to close Microcontroller in test cleanup") + if not micro.terminate_reading_received_packet_thread: + _close_quietly(micro, "Microcontroller") diff --git a/software/tests/control/test_watchdog_breadcrumbs.py b/software/tests/control/test_watchdog_breadcrumbs.py index 40ff84c6d..e539a7092 100644 --- a/software/tests/control/test_watchdog_breadcrumbs.py +++ b/software/tests/control/test_watchdog_breadcrumbs.py @@ -15,8 +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() - # Join the acquisition thread before closing the scope, otherwise it keeps - # running against a closed microcontroller and dies with a TimeoutError. + # 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()