From 2eaa609d296b516077d5a58c937c7d091fa50e13 Mon Sep 17 00:00:00 2001 From: x01dc Date: Wed, 2 Sep 2026 17:46:14 +0200 Subject: [PATCH] fix(omny): migrate scilog writes to bec.messaging.scilog, checking _enabled write_to_scilog()/write_pdf_report() used the old bec.logbook/client.logbook. send_logbook_message() API, which has no "is scilog even configured" check. Confirmed live: on a session without scilog credentials (this dev deployment has no /etc/bec/secrets/.scilog.env at all), send() blocks indefinitely instead of failing fast -- a real tomo_scan() hung inside _write_subtomo_to_scilog() for 29+ minutes during Phase 1 live testing. flomni already migrated off this API to bec.messaging.scilog, which gates every send behind scilog._enabled (published from the deployment's actual scilog config) -- per Mirko's direction, ported the same mechanism to both OMNY call sites, matching flomni's write_pdf_report()/_scilog_write() exactly rather than merely adding an active_account check. Added 3 unit tests locking in the _enabled gate (skip when disabled, send when enabled, tolerate a send() exception). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01QLrD7sVYGLAzsQjLVJpCgt --- .../bec_ipython_client/plugins/omny/omny.py | 71 ++++++++++++------ .../test_omny_tomo_scan.py | 74 +++++++++++++++++++ 2 files changed, 123 insertions(+), 22 deletions(-) diff --git a/csaxs_bec/bec_ipython_client/plugins/omny/omny.py b/csaxs_bec/bec_ipython_client/plugins/omny/omny.py index c40e74b1..231194a4 100644 --- a/csaxs_bec/bec_ipython_client/plugins/omny/omny.py +++ b/csaxs_bec/bec_ipython_client/plugins/omny/omny.py @@ -1066,17 +1066,36 @@ class OMNY( def sample_name(self): return dev.omny_samples.get_sample_name_in_samplestage() - def write_to_scilog(self, content, tags: list = None): + def write_to_scilog(self, content, tags: list = None) -> bool: + """Write a text entry to scilog, tolerating scilog being disabled or + unreachable. Mirrors flomni's _scilog_write(): checks + scilog._enabled before attempting a send, rather than trying and + catching whatever exception (or hang) an unconfigured/unreachable + scilog produces -- the old bec.logbook/client.logbook. + send_logbook_message() API used here previously had no such check, + so a session without scilog credentials configured would block + indefinitely on send() instead of failing fast (confirmed live: a + real tomo_scan() hung inside _write_subtomo_to_scilog() on a + session with no /etc/bec/secrets/.scilog.env at all). + + Returns True if the entry was sent, False if it was skipped/failed + (already logged as a warning either way -- callers that don't care + about the outcome can ignore the return value). + """ + scilog = getattr(bec.messaging, "scilog", None) + if scilog is None or not getattr(scilog, "_enabled", False): + logger.warning("scilog is not enabled; skipping scilog entry.") + return False + if tags is not None: + tags.append("BEC") + else: + tags = ["BEC"] try: - if tags is not None: - tags.append("BEC") - else: - tags = ["BEC"] - msg = bec.logbook.LogbookMessage() - msg.add_text(content).add_tag(tags) - self.client.logbook.send_logbook_message(msg) - except Exception: - logger.warning("Failed to write to scilog.") + scilog.new().add_text(content.replace("\n", "
")).add_tags(tags).send() + return True + except Exception as exc: + logger.warning(f"Failed to write to scilog: {exc}") + return False def tomo_alignment_scan(self): """ @@ -1885,19 +1904,27 @@ class OMNY( "xterm /work/sls/spec/local/XOMNY/bin/upload/upload_last_pon.sh &", shell=True ) # status = subprocess.run(f"cp /tmp/spec-e20131-specES1.pdf {user_target}", shell=True) - # Same tolerance as write_to_scilog(): a session without scilog/ - # logbook configured (e.g. a dev/sim session) must not crash report - # generation over the logbook upload -- the PDF itself is already - # written above. - try: - msg = bec.logbook.LogbookMessage() + # bec.logbook/client.logbook.send_logbook_message() was the old, + # pre-migration scilog API -- it has no "is scilog even configured" + # check, so on a session without scilog credentials (e.g. this dev + # sandbox has no /etc/bec/secrets/.scilog.env at all) the send() + # call blocks indefinitely instead of failing fast, hanging the + # whole tomo scan. flomni's write_pdf_report() uses + # bec.messaging.scilog instead, which is enabled/disabled based on + # the deployment's actual published scilog config -- mirrored here. + scilog = getattr(bec.messaging, "scilog", None) + if scilog is None or not getattr(scilog, "_enabled", False): + logger.warning("scilog is not enabled; skipping PDF report entry.") + else: logo_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "OMNY.png") - msg.add_file(logo_path).add_text("".join(content).replace("\n", "

")).add_tag( - ["BEC", "tomo_parameters", f"dataset_id_{dataset_id}", "OMNY", self.sample_name] - ) - self.client.logbook.send_logbook_message(msg) - except Exception: - logger.warning("Failed to write PDF report to scilog.") + try: + scilog.new().add_attachment(logo_path, width=200).add_text( + content.replace("\n", "
") + ).add_tags( + ["BEC", "tomo_parameters", f"dataset_id_{dataset_id}", "OMNY", self.sample_name] + ).send() + except Exception as exc: + logger.warning(f"Failed to write PDF report to scilog: {exc}") def vcs_show_all(): dev.omny_vcs.show_all() diff --git a/tests/tests_bec_ipython_client/test_omny_tomo_scan.py b/tests/tests_bec_ipython_client/test_omny_tomo_scan.py index ade871ba..202d3a20 100644 --- a/tests/tests_bec_ipython_client/test_omny_tomo_scan.py +++ b/tests/tests_bec_ipython_client/test_omny_tomo_scan.py @@ -181,3 +181,77 @@ def test_tomo_scan_sets_timing_fields_on_new_scan(monkeypatch): assert omny.progress["estimated_remaining_time"] is None assert omny.progress["estimated_finish_time"] is None assert omny.progress["accumulated_idle_time"] == 0.0 + + +# --- write_to_scilog(): checks scilog._enabled before attempting a send ----- +# +# The old bec.logbook/client.logbook.send_logbook_message() API had no such +# check -- on a session without scilog credentials configured (e.g. no +# /etc/bec/secrets/.scilog.env at all) its send() call blocked indefinitely +# instead of failing fast, hanging a real tomo_scan() inside +# _write_subtomo_to_scilog() (confirmed live). Mirrors flomni's +# _scilog_write(), which gates on bec.messaging.scilog._enabled instead. + + +class _FakeSciLogMessage: + def __init__(self, calls): + self._calls = calls + + def add_text(self, text): + self._calls.append(("add_text", text)) + return self + + def add_tags(self, tags): + self._calls.append(("add_tags", tags)) + return self + + def send(self): + self._calls.append(("send",)) + + +def _make_omny_with_scilog(monkeypatch, enabled: bool, raise_on_send: bool = False): + omny = object.__new__(OMNY) + calls = [] + + def _new(): + if raise_on_send: + + class _Boom(_FakeSciLogMessage): + def send(self): + raise RuntimeError("scilog unreachable") + + return _Boom(calls) + return _FakeSciLogMessage(calls) + + fake_scilog = types.SimpleNamespace(_enabled=enabled, new=_new) + fake_bec = types.SimpleNamespace(messaging=types.SimpleNamespace(scilog=fake_scilog)) + monkeypatch.setattr(omny_module, "bec", fake_bec, raising=False) + return omny, calls + + +def test_write_to_scilog_skips_when_scilog_disabled(monkeypatch): + omny, calls = _make_omny_with_scilog(monkeypatch, enabled=False) + + result = omny.write_to_scilog("hello") + + assert result is False + assert calls == [] + + +def test_write_to_scilog_sends_when_scilog_enabled(monkeypatch): + omny, calls = _make_omny_with_scilog(monkeypatch, enabled=True) + + result = omny.write_to_scilog("hello", tags=["mytag"]) + + assert result is True + assert ("add_text", "hello") in calls + assert ("add_tags", ["mytag", "BEC"]) in calls + assert ("send",) in calls + + +def test_write_to_scilog_tolerates_send_exception(monkeypatch): + omny, calls = _make_omny_with_scilog(monkeypatch, enabled=True, raise_on_send=True) + + result = omny.write_to_scilog("hello") + + assert result is False