Fix crash when quitting Nagstamon on macOS - #1241
Conversation
On macOS the application menu, the dock and Cmd-Q quit the application directly via QApplication, bypassing StatusWindow.exit(). The still running worker QThreads were then destroyed during interpreter shutdown when sip tore down the widget tree, which makes Qt call qFatal() and abort the process with SIGABRT. Connect QApplication.aboutToQuit to a new idempotent StatusWindow.shutdown_workers(), which exit() now also uses, so every way of quitting stops the threads. While at it: - Falsificate the workers' running flag before emitting finish, as delete_server_vbox() and remove_previous_server_vbox() already do. Otherwise a worker just schedules its next round via singleShot. - Bound the wait() for a thread to end and fall back to terminate(), so a worker stuck in a request cannot block the main thread until the socket timeout is reached. - Keep a registry of living TreeViews instead of relying on the layout hierarchy, because sort_server_vboxes() may drop ServerVBoxes that still own a running worker thread. - Give WorkerNotification the running attribute it was already assigned from the outside. The same abort signature shows up in the reports of HenriWahl#1055 and HenriWahl#1159, but both were reported at startup, so this does not necessarily fix them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
On macOS the keychain regularly refuses to store a password with error -25299 because the ACL of an existing entry does not accept a freshly built, unsigned binary. Nagstamon then called sys.exit(1) in the middle of saving the configuration, leaving the user with an application that dies on every attempt to save a server. Move both duplicated keyring blocks into store_password_in_keyring(), which first tries to repair the ACL case by deleting and recreating the entry. If the keyring stays unusable, macOS falls back to storing the obfuscated password in the config file and switches use_system_keyring off. Linux and Windows keep the previous loud failure. The general section of nagstamon.conf is now filled after the server configurations have been saved, so a use_system_keyring switched off during that save is actually persisted. Fixes HenriWahl#1159 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hide_macos_dock_icon() passed NSApplicationPresentation* constants to NSApp.setActivationPolicy_(), which expects NSApplicationActivationPolicy*. NSApplicationPresentationHideDock happens to be 2, which as an activation policy means Prohibited: the application cannot be activated at all and its windows never receive keyboard input. Measured with the dock icon hidden, before and after: policy 2 (Prohibited): NSApp.isActive() False, widget.hasFocus() False policy 1 (Accessory): NSApp.isActive() True, widget.hasFocus() True Accessory is what is meant here - no dock icon, but the application stays activatable. This should make the check_macos_dock_icon_fix_show/hide signal chain around every dialog unnecessary, but that is left in place for now. Also remove three pieces of dead code found next to it: - the AppKit import block in qui/__init__.py, which was never used there - the duplicated dialogs.authentication dock icon connections - the QMenuBar experiment in StatusWindow, which was never added to a layout and whose QActions were never connected Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
QThread.terminate() was the fallback when a worker did not stop within WORKER_THREAD_WAIT_TIMEOUT. That case is the normal one on quit: quit() only unwinds the event loop, so a worker sitting in server.get_status() keeps running until its request hits the socket timeout. Terminating it kills a thread which executes Python code and holds the GIL, so the following unbounded wait() - or the next bytecode of the main thread - can block forever, turning the crash on quit into a hang. Stop the workers in one shared stop_worker_thread() instead, which falsificates the running flag, quits the thread and waits for it, but abandons a still running thread rather than terminating it. An abandoned thread ends on its own when its request times out; it gets detached from its parent and kept referenced so it is not deleted while running, which is what makes Qt call qFatal(). Nagstamon does not wait for those threads any longer either: it leaves via os._exit() once app.exec() returns, so neither the Qt nor the Python teardown can destroy a thread which is still running. Also give WorkerNotification.running an effect - it was only assigned from the outside so far - by not starting a notification anymore while shutting down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I could reproduce the abort locally and verify the fix, so here is the before/after. Setup: macOS 26.6.2, Nagstamon from source with PyQt6 6.11, one server pointing at a blackhole address ( master (1ae4048): Triggered thread of that report: So it is exactly the case this PR is about: the worker This branch: The 2.1s are the bounded wait for the threads which cannot be stopped; they are left to run into their socket timeout instead of being terminated, and the process leaves via |
* fix(macos): stop worker threads on aboutToQuit On macOS the application menu, the dock and Cmd-Q quit the application directly via QApplication, bypassing StatusWindow.exit(). The still running worker QThreads were then destroyed during interpreter shutdown when sip tore down the widget tree, which makes Qt call qFatal() and abort the process with SIGABRT. Connect QApplication.aboutToQuit to a new idempotent StatusWindow.shutdown_workers(), which exit() now also uses, so every way of quitting stops the threads. While at it: - Falsificate the workers' running flag before emitting finish, as delete_server_vbox() and remove_previous_server_vbox() already do. Otherwise a worker just schedules its next round via singleShot. - Bound the wait() for a thread to end and fall back to terminate(), so a worker stuck in a request cannot block the main thread until the socket timeout is reached. - Keep a registry of living TreeViews instead of relying on the layout hierarchy, because sort_server_vboxes() may drop ServerVBoxes that still own a running worker thread. - Give WorkerNotification the running attribute it was already assigned from the outside. The same abort signature shows up in the reports of #1055 and #1159, but both were reported at startup, so this does not necessarily fix them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(macos): do not exit on keyring failure On macOS the keychain regularly refuses to store a password with error -25299 because the ACL of an existing entry does not accept a freshly built, unsigned binary. Nagstamon then called sys.exit(1) in the middle of saving the configuration, leaving the user with an application that dies on every attempt to save a server. Move both duplicated keyring blocks into store_password_in_keyring(), which first tries to repair the ACL case by deleting and recreating the entry. If the keyring stays unusable, macOS falls back to storing the obfuscated password in the config file and switches use_system_keyring off. Linux and Windows keep the previous loud failure. The general section of nagstamon.conf is now filled after the server configurations have been saved, so a use_system_keyring switched off during that save is actually persisted. Fixes #1159 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(macos): use correct NSApplicationActivationPolicy constants hide_macos_dock_icon() passed NSApplicationPresentation* constants to NSApp.setActivationPolicy_(), which expects NSApplicationActivationPolicy*. NSApplicationPresentationHideDock happens to be 2, which as an activation policy means Prohibited: the application cannot be activated at all and its windows never receive keyboard input. Measured with the dock icon hidden, before and after: policy 2 (Prohibited): NSApp.isActive() False, widget.hasFocus() False policy 1 (Accessory): NSApp.isActive() True, widget.hasFocus() True Accessory is what is meant here - no dock icon, but the application stays activatable. This should make the check_macos_dock_icon_fix_show/hide signal chain around every dialog unnecessary, but that is left in place for now. Also remove three pieces of dead code found next to it: - the AppKit import block in qui/__init__.py, which was never used there - the duplicated dialogs.authentication dock icon connections - the QMenuBar experiment in StatusWindow, which was never added to a layout and whose QActions were never connected Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(macos): do not terminate stuck worker threads QThread.terminate() was the fallback when a worker did not stop within WORKER_THREAD_WAIT_TIMEOUT. That case is the normal one on quit: quit() only unwinds the event loop, so a worker sitting in server.get_status() keeps running until its request hits the socket timeout. Terminating it kills a thread which executes Python code and holds the GIL, so the following unbounded wait() - or the next bytecode of the main thread - can block forever, turning the crash on quit into a hang. Stop the workers in one shared stop_worker_thread() instead, which falsificates the running flag, quits the thread and waits for it, but abandons a still running thread rather than terminating it. An abandoned thread ends on its own when its request times out; it gets detached from its parent and kept referenced so it is not deleted while running, which is what makes Qt call qFatal(). Nagstamon does not wait for those threads any longer either: it leaves via os._exit() once app.exec() returns, so neither the Qt nor the Python teardown can destroy a thread which is still running. Also give WorkerNotification.running an effect - it was only assigned from the outside so far - by not starting a notification anymore while shutting down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: say cleared instead of falsificated Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
On macOS Nagstamon reliably crashes with
abort()when it is quit via Cmd-Q, the application menu or the dock. The crash report showswith four worker threads still sitting in
QThread::exec().StatusWindow.exit()is the only place where the worker threads are stopped, and it is wired to Nagstamon's own menu entries only (widgets/menu.py,qui/__init__.py). Cmd-Q and the dock quit the application straight throughQApplication, soapp.exec()returns with all threads still running, sip tears down the widget tree during interpreter shutdown and Qt callsqFatal()on the firstQThreadit destroys. On Windows and Linux there is no way to quit pastexit(), which is why this only shows up on macOS.QApplication.aboutToQuitis now connected to a new idempotentStatusWindow.shutdown_workers(), whichexit()uses as well, so every way of quitting stops the threads. Three smaller things in the same code path came up while looking at it:runningflag of the workers is falsified beforefinishis emitted, asdelete_server_vbox()andremove_previous_server_vbox()already do - otherwise a worker just schedules its next round viasingleShotwait()for a thread to end is bounded and falls back toterminate(), so a worker stuck in a request cannot block the main thread until the 30 s socket timeoutsort_server_vboxes()drops ServerVBoxes that still own a running worker threadRunning from source, pressing Cmd-Q: before the change the process ends with exit code 134 and
QThread: Destroyed while thread '' is still running, afterwards with 0. Quitting through the systray menu works before and after.Also included
Two more macOS fixes that are small and sit in the same corner:
Keyring (#1159).
Config.save_multiple_config()calledsys.exit(1)wheneverkeyring.set_password()raised. On macOS the keychain regularly refuses with-25299because the ACL of an existing entry does not accept a freshly built, unsigned binary, so saving a server killed the application every single time - the traceback in #1159 is exactly this. The two duplicated blocks moved intostore_password_in_keyring(), which first tries to repair that case by deleting and recreating the entry, and otherwise switches the keyring off and stores the obfuscated password in the config file like it does without a keyring. Linux and Windows keep the previous loud failure. The general section ofnagstamon.confis written after the server configs now so ause_system_keyringturned off during saving actually ends up in the file.Dock icon.
hide_macos_dock_icon()passedNSApplicationPresentation*constants toNSApp.setActivationPolicy_(), which expectsNSApplicationActivationPolicy*.NSApplicationPresentationHideDockhappens to be 2, which as an activation policy means Prohibited: the application cannot be activated at all and its windows never get keyboard focus. Measured with the dock icon hidden:NSApp.isActive()widget.hasFocus()Accessory is what is meant here. This should make the
check_macos_dock_icon_fix_show/hidesignal chain around every dialog unnecessary, but I left that in place rather than pulling on it in the same PR. Three pieces of dead code found next to it are removed: the unused AppKit import block inqui/__init__.py, the duplicateddialogs.authenticationdock icon connections and theQMenuBarexperiment inStatusWindowthat was never added to a layout.Tested on macOS 26.6.2 on Apple Silicon with Qt 6.11 and Python 3.14.
Fixes #1159