Skip to content

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

Merged
Alpaca233 merged 3 commits into
masterfrom
fix/test-hardware-leaks
Jul 29, 2026
Merged

fix(tests): close leaked Microscope/MPC instances to stop flaky CI segfaults#604
Alpaca233 merged 3 commits into
masterfrom
fix/test-hardware-leaks

Conversation

@Alpaca233

Copy link
Copy Markdown
Collaborator

Problem

CI has been failing ~30% of runs since 2026-06-19 with what look like random test failures. They are almost never test failures: in 11 of 12 failed runs examined, every test passed and the pytest process then segfaulted (exit 139) — usually during interpreter shutdown (e.g. run 30389568104: 1612 passed → segfault 26s later), occasionally mid-suite inside the Qt event loop (run 27848898456 has the faulthandler dump).

Root cause

~40 test call sites build a full simulated Microscope/MultiPointController; only ~15 ever close one. Each leak leaves behind:

  • daemon threads: camera stream_fn (1ms busy-loop generating frames), slack-notifier workers, laser-engine _tick_loop
  • a prewarmed JobRunner child process per MultiPointController (failed runs show ~17 orphaned python3 processes at job cleanup; green runs show none)

A leftover thread touching a destroyed Qt object mid-suite, or frozen inside Qt/numpy C code during interpreter finalization, segfaults the process. Green vs red is a race on thread timing — hence "random".

A visible marker in every run (green and red): Exception in thread Acquisition thread ... TimeoutError: Current mcu operation timed out after 3 [s] — the watchdog tests abort an acquisition and close the scope without joining the acquisition thread, which then times out returning the stage home against a closed simulated microcontroller.

Fix

  • tests/conftest.py (new): suite-wide autouse fixture tracking every Microscope, MultiPointController (incl. QtMultiPointController) and Microcontroller built during a test, closing them at teardown in dependency order — MPC first (joins the acquisition thread and shuts down JobRunner children while the microcontroller is still alive), then Microscope (stops camera streaming), then stray microcontrollers. Replaces the Microcontroller-only fixture in tests/control/conftest.py and extends coverage to tests/squid/ and root-level tests. Watchdog breadcrumb writes during cleanup are pinned to a tmp dir.
  • Microscope.close() is now idempotent, so tests that already close explicitly don't double-close every component when the fixture runs.
  • Watchdog tests call mpc.close() before scope.close(), fixing the standing TimeoutError thread leak.

Incorporates #595 (by @hongquanli) as belt-and-braces

  • SQUID_PYTEST_HARD_EXIT=1: a pytest_unconfigure hook os._exit()s with pytest's real exit status after the session completes, skipping the crash-prone interpreter teardown entirely (same trick main_hcs.py uses). Real test failures still fail the step.
  • test_HighContentScreeningGui.py runs in its own pytest process pinned to PyQt5 instead of being skipped (its full-GUI Qt state crashes later tests in a shared process; mixed pytest-qt/qtpy bindings cause the QObject double-init error).

The hooks moved to the new top-level tests/conftest.py, so if this merges, #595 is superseded.

Verification

  • Watchdog tests pass locally with no PytestUnhandledThreadExceptionWarning (previously present in every CI run).
  • test_MultiPointController.py passes under the new fixture.
  • Hard-exit hook preserves exit status in both directions (0 on pass, 1 on fail, verified locally).
  • One local-only failure (test_microscope_wraps_pi_focus_when_enabled) reproduces identically on unmodified master — pre-existing local machine-config difference, unrelated.
  • CI on this PR is the authoritative check for the flake itself; the leaked-thread warning should be absent from the log.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to eliminate flaky CI segfaults by ensuring simulated hardware objects created in tests are reliably shut down, and by optionally hard-exiting pytest after completion to avoid crash-prone interpreter teardown (notably with Qt).

Changes:

  • Add a suite-wide autouse pytest fixture that tracks and closes leaked MultiPointController, Microscope, and Microcontroller instances after each test.
  • Make Microscope.close() idempotent and adjust watchdog tests to close mpc before scope to avoid thread leaks.
  • Update CI to enable an optional hard-exit hook and run the full-GUI test in a dedicated pytest process pinned to PyQt5.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
software/tests/control/test_watchdog_integration.py Ensures the acquisition thread is joined via mpc.close() before closing the microscope to prevent post-close MCU access.
software/tests/control/test_watchdog_breadcrumbs.py Joins acquisition thread during teardown to prevent leaked worker thread errors.
software/tests/control/conftest.py Removes microcontroller-only cleanup in favor of the new suite-wide cleanup; keeps watchdog state dir redirected to tmp for these tests.
software/tests/conftest.py Adds suite-wide instance tracking/teardown and optional SQUID_PYTEST_HARD_EXIT pytest hooks.
software/control/microscope.py Makes Microscope.close() a no-op on repeated calls to avoid double-close during test cleanup.
.github/workflows/main.yml Enables hard-exit in CI and runs the GUI test in its own pytest invocation pinned to PyQt5.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread software/tests/conftest.py Outdated
Comment on lines +73 to +75
# 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Fixed in 4392fe1 - reworded to distinguish the two orderings: monkeypatches set up after this fixture (child-conftest autouse, test-scoped) are undone before this teardown runs, while this fixture's own monkeypatch reverts only after it finishes — which is why the value set here is in effect during cleanup.

Comment on lines +95 to +97
yield

for controller in reversed(controllers):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Fixed in 4392fe1 - the tmp state dir is now re-applied via monkeypatch.setenv right after yield, so a raw os.environ write in a test body can no longer redirect teardown-time breadcrumb writes. (Today only monkeypatch-based fixtures set this var, so this is hardening against future tests.)

Alpaca233 and others added 2 commits July 29, 2026 09:57
…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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
@Alpaca233
Alpaca233 merged commit 30a193e into master Jul 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants