Skip to content

Run shutdown cleanup on the menu and dialog quit paths - #425

Merged
gbeane merged 12 commits into
mainfrom
fix/quit-runs-shutdown-cleanup
Aug 7, 2026
Merged

Run shutdown cleanup on the menu and dialog quit paths#425
gbeane merged 12 commits into
mainfrom
fix/quit-runs-shutdown-cleanup

Conversation

@gbeane

@gbeane gbeane commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two of the three ways to quit JABS never delivered a close event, so MainWindow.closeEvent — and the cleanup it performs — was skipped. With a background thread still running, the process aborts with SIGABRT at interpreter shutdown; the process pool shutdown was also being skipped.

Stacked on #424 (both branches touch main_window.py). Base retargets to main once that merges.

The problem

Quit path Delivered a close event?
Window close button yes
Quit menu item / Ctrl+Q (menu_builder.py) no — wired straight to QCoreApplication.quit, which leaves the event loop without closing anything
"Quit JABS" on the first-behavior prompt (main_control_widget.py) no — bare sys.exit(0)

Measured with a background scan in flight (20 videos), the two bypassing paths exited with code 134 (SIGABRT) after QThread: Destroyed while thread is still running, and _process_pool.shutdown() was never called.

The gap predates the feature cache scan (ProjectLoaderThread is exposed the same way, and it runs longer), but the scan makes it easy to hit because it starts automatically when a project opens — the seconds right after opening are exactly when someone might hit Ctrl+Q, and the first-behavior prompt appears in that same window.

The fix

  • MainWindow.quit_application(): close() to deliver the close event synchronously, then QtWidgets.QApplication.quit(). The Quit action now points at this.
  • The explicit quit is load-bearing. Connecting the action to close() alone leaves the application running when another top-level window is open, and JABS creates some without a parent (TrainingReportDialog(..., parent=None)), so Ctrl+Q with a training report open would have closed the main window and left the app alive.
  • The first-behavior prompt's quit calls QApplication.closeAllWindows() before sys.exit(0), which delivers the close event synchronously so cleanup finishes first.

Verification

Measured with a scan in flight (20 videos, 0.4 s each):

Path Before After
Quit menu / Ctrl+Q SIGABRT (134), pool never shut down exit 0, scan stopped in 0.11 s, pool shut down
Quit menu, unparented window open n/a exit 0, cleanup ran, app still quits
First-behavior "Quit JABS" bypassed cleanup exit 0, scan stopped, pool shut down
Window close button already clean unchanged, exit 0

962 tests pass (7 added covering the quit call order and the first-behavior prompt path). Ruff clean.

Known remaining case

If a filesystem call is genuinely wedged, the bounded wait in closeEvent times out and the abort still happens on every path: no cooperative cancel can interrupt a blocked h5py.File() open. Left alone deliberately — a hung mount makes JABS unusable regardless, and QThread::terminate() is a worse trade.

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 updates JABS’ quit/exit pathways so MainWindow.closeEvent reliably runs, ensuring shutdown cleanup (notably stopping background threads and shutting down the process pool) occurs even when quitting via the menu/Ctrl+Q or via the first-behavior prompt.

Changes:

  • Route the Quit menu action to MainWindow.quit_application() to trigger closeEvent cleanup before exiting the event loop.
  • Ensure the first-behavior “Quit JABS” path closes windows before exiting, so cleanup runs first.
  • Add/adjust UI tests to verify quit call order and the first-behavior quit path behavior.

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
tests/ui/test_main_window.py Adds a test asserting quit_application() closes the window before quitting.
tests/ui/test_main_control_widget.py Adds a test asserting the first-label “Quit JABS” path closes windows before exiting.
src/jabs/ui/main_window/menu_builder.py Rewires Ctrl+Q/Quit action to call MainWindow.quit_application() instead of QCoreApplication.quit().
src/jabs/ui/main_window/main_window.py Introduces quit_application() to close the main window then quit the app.
src/jabs/ui/main_window/central_widget.py Extends the “features not cached” dialog text to point to jabs-init --help.
src/jabs/ui/main_control_widget/main_control_widget.py Updates first-label dialog cancel path to close all windows before exiting.

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

Comment thread src/jabs/ui/main_window/main_window.py Outdated
Comment on lines +577 to +578
self.close()
QtWidgets.QApplication.quit()
Comment on lines 677 to 678
QtWidgets.QApplication.closeAllWindows()
sys.exit(0)
Base automatically changed from feature/feature-cache-status to main August 7, 2026 02:21
@gbeane

gbeane commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Both addressed, though the second one differently than suggested.

quit_application() quitting after a rejected close. Valid, and fixed: the quit is now conditional on close(), with a debug log when it is declined. Nothing declines the close today, so this is defensive, but it encodes the invariant the method exists for.

That prompted a related fix in the same spirit. closeEvent was running the cleanup before delegating to super(), so a declined close would have torn down the process pool and the scan thread while the window stayed open — the mirror image of the bug you flagged. It now delegates first and only cleans up when event.isAccepted().

Verified with a real window and an event filter that rejects the close: close() returns False, the scan thread keeps running, the pool is not shut down, the window stays visible, and quit_application() does not quit. Removing the filter and closing again stops the scan and shuts the pool down as before.

closeAllWindows() return value. The premise does not hold: QApplication::closeAllWindows() is declared void in Qt 6, and PySide6 returns None — I checked in this venv (closeAllWindows() returned: None NoneType). So there is no success value to gate on, and "only exit after closeAllWindows() succeeds" cannot be written as described.

The underlying concern is real, so I fixed it a better way: the prompt now closes the window that holds the widget (self.window(), which resolves to the MainWindow) rather than blindly closing every window, and close() does return a bool. A declined close returns without exiting. That also targets the window whose closeEvent actually holds the cleanup instead of relying on "close everything and hope the right one was included".

window = self.window()
if window is not None and not window.close():
    return
sys.exit(0)

Tests cover both directions on both paths (accepted → quits/exits, declined → neither), and the existing quit-path smoke checks still pass unchanged: the real Quit menu action with a responsive scan exits 0 with the pool shut down, and the wedged-storage case still exits 0 after the forced stop.

1013 tests, ruff clean.

@gbeane
gbeane merged commit a0f8963 into main Aug 7, 2026
5 checks passed
@gbeane
gbeane deleted the fix/quit-runs-shutdown-cleanup branch August 7, 2026 02:25
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