fix(omny): catch KeyboardInterrupt around frozen-GUI raise_window() calls
CI for csaxs_bec / test (push) Successful in 5m0s
CI for csaxs_bec / test (push) Successful in 5m0s
A Ctrl+C landing during raise_window()'s RPC wait (e.g. when the GUI process is frozen/unresponsive) was escaping two `except Exception` handlers that don't catch KeyboardInterrupt (a BaseException sibling, not an Exception subclass): omnygui_show_progress() aborted the whole tomo_scan()/tomo_scan_resume() call instead of just skipping the window-raise, and tomo_queue_execute() left the job silently stuck at status="running" instead of the usual clean "incomplete" + explanatory message. Reproduced live by Mirko via the tomo queue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLrD7sVYGLAzsQjLVJpCgt
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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-
|
||||
|
||||
@@ -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}")
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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"}
|
||||
|
||||
Reference in New Issue
Block a user