diff --git a/csaxs_bec/bec_ipython_client/plugins/OMNY_shared/tomo_queue_mixin.py b/csaxs_bec/bec_ipython_client/plugins/OMNY_shared/tomo_queue_mixin.py index c26f7fa2..fc142da3 100644 --- a/csaxs_bec/bec_ipython_client/plugins/OMNY_shared/tomo_queue_mixin.py +++ b/csaxs_bec/bec_ipython_client/plugins/OMNY_shared/tomo_queue_mixin.py @@ -916,7 +916,14 @@ class TomoQueueMixin: # never block on input() -- see LamNI.tomo_scan()'s # fine-alignment check. self.tomo_scan(interactive=False) - except Exception as exc: + except (Exception, KeyboardInterrupt) as exc: + # KeyboardInterrupt is a BaseException, not an Exception -- a + # bare `except Exception` misses it, leaving the job stuck at + # status="running" with no explanation the next time the + # queue is inspected (confirmed live: a Ctrl+C landing during + # tomo_scan_resume()'s GUI-raise call produced exactly this -- + # a silent "running" job and a raw traceback instead of the + # informative message below). self._tomo_queue_proxy.update_by_id(job_id, status="incomplete") print(f"Tomo queue job '{label}' did not complete: {exc}") print( diff --git a/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md b/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md index 086c1ad1..efab46ed 100644 --- a/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md +++ b/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md @@ -45,6 +45,14 @@ ported to OMNY (Phase 2 of 2)" below. Mirko: "is the way to move to the widget p that. No live GUI verification yet (needs a real display, same as every other GUI change this branch) -- next step is Mirko trying it against a real OMNY session. +**Just fixed following Mirko's next round of live testing (from the test checklist)**: a +Ctrl+C during `tomo_queue_resume()`'s GUI-raise call (triggered by a frozen/unresponsive GUI +process) was escaping as a raw `KeyboardInterrupt` instead of being handled gracefully, in two +places -- see "KeyboardInterrupt during a frozen-GUI raise_window() call was not caught" below. +The underlying GUI freeze itself, and a separate "two items in the server queue... after +clearing it also did not resume" report, are not yet explained -- need more detail from Mirko +(see that section's last paragraph). + **Still open / deferred** (unchanged from before, not touched this pass): - `x_ray_eye_align.py` is still LamNI-derived, unadapted code beyond the two call-site renames that unblocked the GUI-open crash — see its own section below. The GUI itself opens fine now; @@ -543,6 +551,64 @@ equally-spaced section shows/hides correctly and the new sub-tomogram-count labe submitting/queueing a type-4 or type-5 job (confirms the `_validate()` fix specifically), and checking a queued job's tooltip/Projections column in the queue dialog. +## KeyboardInterrupt during a frozen-GUI raise_window() call was not caught (fixed) + +Mirko, running from the tomo queue on his own machine: aborted a running scan with Ctrl+C +(worked correctly -- clean `ScanInterruption: User abort`, queue paused, job marked +`"incomplete"`), then called `omny.tomo_queue_resume()` twice; the second call, while +`tomo_scan_resume()` was starting up, produced a **raw Python `KeyboardInterrupt` traceback** +instead of BEC's usual graceful abort. Separately: "the gui progress and tomo parameters was +open and both a frozen on my local machine", and "i had two items in the server queue... after +clearing it also did not resume." + +Root cause, read straight off the traceback: the stack bottomed out in +`self.gui.omny.raise_window()` -> `bec_widgets`' `RPCBase._run_rpc()`, blocked on +`self._msg_wait_event.wait(...)`. `tomo_scan()` calls `omnygui_show_progress()` +(-> `omnygui_show_gui()` -> `raise_window()`) unconditionally as its first line, on every call +including `tomo_scan_resume()`. When the GUI process itself is frozen/unresponsive (matching +Mirko's separate "both frozen on my local machine" report), that RPC call doesn't hang forever -- +`bec_widgets`' default RPC timeout is 60s (`BECGuiClient._rpc_timeout`, `client_utils.py`), after +which it raises a clean `RPCResponseTimeoutError` -- but 60s is a long, silent wait for an +operator watching a terminal, and Mirko understandably lost patience and hit Ctrl+C himself +during that wait. `KeyboardInterrupt` is a `BaseException`, not an `Exception` subclass, so it +sailed straight past two different `except Exception` handlers that were supposed to catch +exactly this kind of trouble: + +1. `omnygui_show_progress()` (`gui_tools.py`) already wrapped the whole show/raise sequence in + `try/except Exception` specifically so a GUI hiccup can never abort a scan -- but a + `KeyboardInterrupt` escaped it outright, propagating all the way up through + `tomo_scan_resume()`. **Fixed**: added a dedicated `except KeyboardInterrupt` clause that logs + a warning and continues without raising the window -- bringing the window to front is a + nicety, not something the scan itself needs. +2. `tomo_queue_execute()` (`tomo_queue_mixin.py`)'s `except Exception as exc:` around the + `tomo_scan()`/`tomo_scan_resume()` call is what marks a job `status="incomplete"` and prints + the informative "Tomo queue job '...' did not complete: ... Queue paused..." message -- the + same message Mirko saw work correctly on the *first* abort. A `KeyboardInterrupt` escaping this + handler leaves the job silently stuck at `status="running"` with no explanation printed at all, + which is exactly what "I had two items in the server queue. I do not have details" describes. + **Fixed**: widened to `except (Exception, KeyboardInterrupt) as exc:`, so any interruption -- + not just a raised `Exception` -- gets the same clean status update and message. + +Both fixes are narrowly scoped (they don't change how Ctrl+C is handled *during* an actual running +scan -- BEC's own deferred-pause/abort alarm flow for that is untouched) and only change what +happens when the interrupt lands during the pre-scan GUI-raise window. Regression tests: +`test_omnygui_show_progress_swallows_keyboardinterrupt_from_frozen_gui` (`gui_tools.py`'s +`raise_window()` mocked to raise `KeyboardInterrupt`, asserts `omnygui_show_progress()` doesn't +propagate it) and `test_tomo_queue_execute_marks_job_incomplete_on_keyboardinterrupt` +(`tomo_scan` mocked to raise `KeyboardInterrupt`, asserts the job ends up `"incomplete"` not +`"running"`, and that the interrupt still propagates to the caller afterward -- it's not +swallowed, just handled the same way any other mid-job exception already was). + +**Still unexplained, needs more detail from Mirko to pursue further**: "i had two items in the +server queue" and "after clearing it also did not resume" -- ambiguous whether "server queue" +means the OMNY-side `tomo_queue` (this session's job list, cleared via `tomo_queue_clear()`, which +is fully destructive -- if that's what was cleared, "did not resume" is expected, there was +nothing left to resume) or BEC's own scan-server execution queue (a lower-level, separate concept; +inspectable via `bec.queue.scan_queue_status()` or the BECQueue GUI widget). Also unclear whether +the underlying GUI freeze itself (client-side Qt process on Mirko's Mac) has a deeper cause worth +chasing, or was a one-off. Not reproduced live this pass -- Mirko: "i cannot move to the machine +you are running on with my tests now." + ## Environment note (not code, but will bite again if forgotten) Both `csaxs_bec` and `bec_widgets` were pip-installed editable pointing at pre-repo- diff --git a/csaxs_bec/bec_ipython_client/plugins/omny/gui_tools.py b/csaxs_bec/bec_ipython_client/plugins/omny/gui_tools.py index ffc7d1a1..e770cf2a 100644 --- a/csaxs_bec/bec_ipython_client/plugins/omny/gui_tools.py +++ b/csaxs_bec/bec_ipython_client/plugins/omny/gui_tools.py @@ -220,6 +220,17 @@ class OMNYGuiTools: self.progressbar.add_ring().set_update("manual") self.progressbar.add_ring().set_update("scan") self._omnygui_update_progress() + except KeyboardInterrupt: + # A frozen/unresponsive GUI process makes raise_window() block on + # its RPC wait (up to the client's ~60s default timeout) -- if the + # operator loses patience and hits Ctrl+C during that wait, don't + # let it escape as a raw KeyboardInterrupt and abort the whole + # tomo_scan()/tomo_scan_resume() call: bringing the window to + # front is a nicety, not something the scan itself needs. + logger.warning( + "omnygui_show_progress() interrupted (GUI unresponsive?) -- continuing " + "without raising the window." + ) except Exception as e: logger.warning(f"Error in omnygui_show_progress: {e}") diff --git a/tests/tests_bec_ipython_client/test_omny_gui_tools.py b/tests/tests_bec_ipython_client/test_omny_gui_tools.py index 55e54dd5..9e50bf59 100644 --- a/tests/tests_bec_ipython_client/test_omny_gui_tools.py +++ b/tests/tests_bec_ipython_client/test_omny_gui_tools.py @@ -183,6 +183,21 @@ def test_omnygui_show_progress_creates_three_rings_matching_flomni(): ring3.set_update.assert_called_once_with("scan") +def test_omnygui_show_progress_swallows_keyboardinterrupt_from_frozen_gui(): + """A frozen/unresponsive GUI process makes raise_window()'s RPC wait block + for up to the client's default timeout; an operator losing patience and + hitting Ctrl+C during that wait must not abort the calling tomo_scan()/ + tomo_scan_resume() with a raw KeyboardInterrupt -- confirmed live: this + is exactly what happened, since a bare `except Exception` does not catch + KeyboardInterrupt (a BaseException sibling, not a subclass).""" + tools, client = _make_gui_tools() + client.gui.windows = {"omny": mock.MagicMock()} + client.gui.omny.raise_window.side_effect = KeyboardInterrupt() + tools.progressbar = mock.MagicMock() + + tools.omnygui_show_progress() # must not raise + + def test_omnygui_update_progress_text_includes_start_eta_finish(): tools, _ = _make_gui_tools() tools.progressbar = mock.MagicMock() diff --git a/tests/tests_bec_ipython_client/test_omny_tomo_queue.py b/tests/tests_bec_ipython_client/test_omny_tomo_queue.py index 6ad9cca9..7627cd53 100644 --- a/tests/tests_bec_ipython_client/test_omny_tomo_queue.py +++ b/tests/tests_bec_ipython_client/test_omny_tomo_queue.py @@ -138,6 +138,28 @@ def test_tomo_queue_execute_resumes_incomplete_job(): assert job["status"] == "done" +def test_tomo_queue_execute_marks_job_incomplete_on_keyboardinterrupt(): + """KeyboardInterrupt is a BaseException, not an Exception -- a bare + `except Exception` around the tomo_scan()/tomo_scan_resume() call would + miss it, leaving the job silently stuck at status="running" with no + explanation. Confirmed live: a Ctrl+C landing during tomo_scan_resume()'s + GUI-raise call produced exactly that (plus a raw traceback instead of the + informative "Queue paused..." message).""" + omny = make_omny() + omny.tomo_scan = lambda *a, **k: (_ for _ in ()).throw(KeyboardInterrupt()) + + omny.tomo_queue_add(label="job1") + try: + omny.tomo_queue_execute() + raised = False + except KeyboardInterrupt: + raised = True + + assert raised + job = omny._tomo_queue_proxy.as_list()[0] + assert job["status"] == "incomplete" + + def test_tomo_queue_add_command_move_action(monkeypatch): omny = make_omny() fake_dev = {"ccm_energy": "ccm-energy-device"}