fix(notifications): bound broker replay store, hide broker from RPC, survive reset

bec_widgets/widgets/containers/main_window/addons/notification_center/notification_banner.py

@@ -733,16 +733,10 @@ class NotificationCentre(QScrollArea):
         )
         # tag with shared ID and hook closures to the singleton broker
         toast.notification_id = notification_id
-        broker = BECNotificationBroker()
-        toast.closed.connect(lambda nid=notification_id: broker.notification_closed.emit(nid))
-        # Once a toast auto-expires it is no longer live, so drop it from the broker's
-        # replay store. Wired here so BOTH creation paths are covered: live posts and
-        # toasts recreated by _replay_active_notifications — otherwise a replayed toast
-        # that expires would leave the entry behind and every future NotificationCentre
-        # would replay it again. MAJOR alarms use lifetime_ms=0 and never emit
-        # 'expired', so they correctly stay in history until explicitly closed.
-        toast.expired.connect(
-            lambda nid=notification_id: broker._active_notifications.pop(nid, None)
+        # resolve the broker when the toast closes, not now: reset_singleton() may have
+        # replaced it in between, and a captured instance would be a deleted QObject
+        toast.closed.connect(
+            lambda nid=notification_id: BECNotificationBroker().notification_closed.emit(nid)
         )
         toast.closed.connect(lambda: self._hide_notification(toast))
         toast.expired.connect(lambda t=toast: self._handle_expire(t))
@@ -1033,6 +1027,10 @@ class BECNotificationBroker(BECConnector, QObject):
 
     RPC = False
 
+    # Upper bound for the replay store. Expired toasts stay in every centre's history, so
+    # new centres replay them (soft-hidden) too; the oldest entries are dropped beyond this.
+    MAX_REPLAY_NOTIFICATIONS = 100
+
     _instance: BECNotificationBroker | None = None
     _initialized: bool = False
 
@@ -1059,6 +1057,9 @@ class BECNotificationBroker(BECConnector, QObject):
         # the caller-supplied parent (which may be a WA_DeleteOnClose window) and anchor it
         # to the QApplication so closing a window never destroys the broker.
         parent = QApplication.instance()
+        # Internal plumbing, never addressed over RPC: keep it out of the RPC registry so the
+        # launcher does not report it as a connection without a top-level window.
+        kwargs["rpc_exposed"] = False
         super().__init__(parent=parent, gui_id=gui_id, client=client, **kwargs)
         self._err_util = self.error_utility
         # listen to incoming alarms and scan status
@@ -1139,8 +1140,11 @@ class BECNotificationBroker(BECConnector, QObject):
             "traceback": detailed_trace,
             "lifetime_ms": lifetime,
         }
-        # close broadcasting and expiry pruning are wired inside add_notification,
-        # covering live and replayed toasts alike — no per-toast hookup needed here
+        # keep the replay store bounded: drop the oldest entries (dicts keep insertion order)
+        while len(self._active_notifications) > self.MAX_REPLAY_NOTIFICATIONS:
+            self._active_notifications.pop(next(iter(self._active_notifications)))
+        # close broadcasting is wired inside add_notification, covering live and
+        # replayed toasts alike — no per-toast hookup needed here
         for centre in centres:
             centre.add_notification(
                 title=title,

tests/unit_tests/conftest.py

@@ -28,6 +28,9 @@ from bec_widgets.utils import bec_dispatcher as bec_dispatcher_module
 from bec_widgets.utils import error_popups
 from bec_widgets.utils.bec_dispatcher import QtRedisConnector
 from bec_widgets.utils.rpc_register import RPCRegister
+from bec_widgets.widgets.containers.main_window.addons.notification_center.notification_banner import (
+    BECNotificationBroker,
+)
 
 # Patch to set default RAISE_ERROR_DEFAULT to True for tests
 # This means that by default, error popups will raise exceptions during tests
@@ -135,6 +138,13 @@ def bec_dispatcher(threads_check):  # pylint: disable=unused-argument
     bec_dispatcher_module.BECDispatcher.reset_singleton()
 
 
+@pytest.fixture(autouse=True)
+def notification_broker_reset(bec_dispatcher):  # pylint: disable=unused-argument
+    """Tear down the app-wide notification broker before the dispatcher it is bound to."""
+    yield
+    BECNotificationBroker.reset_singleton()
+
+
 @pytest.fixture(autouse=True)
 def clean_singleton():
     error_popups._popup_utility_instance = None

tests/unit_tests/test_notifications.py

@@ -366,6 +366,28 @@ def test_broker_posts_notification(qtbot, centre, mocked_client):
     assert toast._lifetime == 0
 
 
+@pytest.fixture
+def broker(mocked_client):
+    """The broker singleton, always torn down after the test (even if an assert fails)."""
+    brk = BECNotificationBroker(client=mocked_client)
+    brk._err_util = ErrorPopupUtility()
+    yield brk
+    BECNotificationBroker.reset_singleton()
+
+
+def _make_centre(qtbot) -> NotificationCentre:
+    """A NotificationCentre in its own parent widget, as a newly opened window would have."""
+    parent = QtWidgets.QWidget()
+    parent.resize(600, 400)
+    ctr = NotificationCentre(parent=parent, fixed_width=300, margin=8)
+    layout = QtWidgets.QVBoxLayout(parent)
+    layout.addWidget(ctr)
+    # qtbot only keeps a weak reference; hold the parent so GC doesn't delete the centre
+    ctr._test_parent_ref = parent  # type: ignore[attr-defined]
+    qtbot.addWidget(parent)
+    return ctr
+
+
 def test_broker_survives_parent_window_destruction(qtbot, mocked_client):
     """The broker is an app-wide singleton; closing the window that first created it must
     not destroy it, or every later window gets a dead wrapper and notifications stop."""
@@ -376,19 +398,21 @@ def test_broker_survives_parent_window_destruction(qtbot, mocked_client):
     w1 = QMainWindow()
     w1.setAttribute(Qt.WidgetAttribute.WA_DeleteOnClose, True)
     qtbot.addWidget(w1)
-    b1 = BECNotificationBroker(parent=w1, client=mocked_client)
-    assert b1.parent() is QApplication.instance()
+    try:
+        b1 = BECNotificationBroker(parent=w1, client=mocked_client)
+        assert b1.parent() is QApplication.instance()
 
-    w1.close()
-    qapp = QApplication.instance()
-    qapp.sendPostedEvents(None, QEvent.DeferredDelete)
-    qapp.processEvents(QEventLoop.AllEvents)
+        w1.close()
+        qapp = QApplication.instance()
+        qapp.sendPostedEvents(None, QEvent.DeferredDelete)
+        qapp.processEvents(QEventLoop.AllEvents)
 
-    assert shiboken6.isValid(b1)  # broker outlived the window
-    b2 = BECNotificationBroker(parent=None, client=mocked_client)
-    assert b2 is b1 and shiboken6.isValid(b2)
-    b2.notification_closed.emit("x")  # must not raise RuntimeError
-    BECNotificationBroker.reset_singleton()
+        assert shiboken6.isValid(b1)  # broker outlived the window
+        b2 = BECNotificationBroker(parent=None, client=mocked_client)
+        assert b2 is b1 and shiboken6.isValid(b2)
+        b2.notification_closed.emit("x")  # must not raise RuntimeError
+    finally:
+        BECNotificationBroker.reset_singleton()
 
 
 def test_broker_revives_after_hard_destroy(qtbot, mocked_client):
@@ -396,62 +420,90 @@ def test_broker_revives_after_hard_destroy(qtbot, mocked_client):
     fresh, re-subscribed instance instead of returning the dead wrapper."""
     import shiboken6
 
-    b1 = BECNotificationBroker(parent=None, client=mocked_client)
-    shiboken6.delete(b1)
-    assert not shiboken6.isValid(b1)
+    try:
+        b1 = BECNotificationBroker(parent=None, client=mocked_client)
+        shiboken6.delete(b1)
+        assert not shiboken6.isValid(b1)
 
-    b2 = BECNotificationBroker(parent=None, client=mocked_client)
-    assert b2 is not b1 and shiboken6.isValid(b2)
-    b2.notification_closed.emit("y")  # re-subscribed, no RuntimeError
-    BECNotificationBroker.reset_singleton()
+        b2 = BECNotificationBroker(parent=None, client=mocked_client)
+        assert b2 is not b1 and shiboken6.isValid(b2)
+        b2.notification_closed.emit("y")  # re-subscribed, no RuntimeError
+    finally:
+        BECNotificationBroker.reset_singleton()
 
 
-def test_active_notifications_dropped_on_expiry(qtbot, centre, mocked_client):
-    """Auto-expiring (non-MAJOR) notifications must not linger in the replay store; MAJOR
-    alarms (lifetime 0, never expire) stay in history until explicitly closed."""
-    qtbot.wait(20)  # let the centre's replay singleShot fire on an EMPTY store first
-    broker = BECNotificationBroker(client=mocked_client)
-    broker._err_util = ErrorPopupUtility()
+def test_broker_is_not_registered_for_rpc(broker):
+    """The broker is internal plumbing: it must stay out of the RPC registry, otherwise the
+    launcher reports it as a connection without a top-level window once all windows close."""
+    from bec_widgets.utils.rpc_register import RPCRegister
+
+    assert broker not in RPCRegister().list_all_connections().values()
+
+
+def test_expired_notification_is_replayed_into_new_centre(qtbot, broker):
+    """Expiry only soft-hides a toast (it stays in the centre's history), so a centre opened
+    later must replay it too and show the same history as the centres already open."""
+    ctr_a = _make_centre(qtbot)
+    qtbot.wait(20)  # let ctr_a's replay singleShot fire on an EMPTY store first
 
     broker.post_notification({"alarm_type": "W", "msg": "m", "severity": 0}, meta={})
-    qtbot.wait(50)
-    assert len(broker._active_notifications) == 1
-    nid = next(iter(broker._active_notifications))
-    assert len(centre.toasts) == 1  # no replay duplicate
-    centre.toasts[0].expired.emit()  # simulate auto-expiry
+    qtbot.waitUntil(lambda: len(ctr_a.toasts) == 1, timeout=2000)
+    nid = ctr_a.toasts[0].notification_id
+    ctr_a.toasts[0].expired.emit()  # simulate auto-expiry
     qtbot.wait(10)
-    assert nid not in broker._active_notifications
+    assert len(ctr_a.toasts) == 1  # still in history, only hidden
+    assert nid in broker._active_notifications
+
+    ctr_b = _make_centre(qtbot)  # e.g. a window opened after the toast expired
+    qtbot.waitUntil(lambda: len(ctr_b.toasts) == 1, timeout=2000)
+    assert ctr_b.toasts[0].notification_id == nid
+    assert not ctr_b.toasts[0].isVisible()  # replayed as history, not popped up again
+
+
+def test_closed_notification_is_not_replayed(qtbot, broker):
+    """Closing a toast removes it everywhere, including the replay store."""
+    ctr_a = _make_centre(qtbot)
+    qtbot.wait(20)
 
     broker.post_notification({"alarm_type": "E", "msg": "m", "severity": 2}, meta={})
-    qtbot.wait(50)
-    major_nid = next(iter(broker._active_notifications))
-    assert broker._active_notifications[major_nid]["lifetime_ms"] == 0
-    BECNotificationBroker.reset_singleton()
-
-
-def test_replayed_toast_expiry_prunes_replay_store(qtbot, mocked_client):
-    """A toast recreated by a NEW centre's replay must also prune the broker's replay
-    store on expiry — not only toasts created live by post_notification."""
-    broker = BECNotificationBroker(client=mocked_client)
-    broker._err_util = ErrorPopupUtility()
-
-    # post with no centre open: the entry is stored for future centres
-    broker.post_notification({"alarm_type": "W", "msg": "m", "severity": 0}, meta={})
-    assert len(broker._active_notifications) == 1
-    nid = next(iter(broker._active_notifications))
-
-    # a new centre replays the stored notification
-    parent = QtWidgets.QWidget()
-    parent.resize(600, 400)
-    ctr = NotificationCentre(parent=parent, fixed_width=300, margin=8)
-    layout = QtWidgets.QVBoxLayout(parent)
-    layout.addWidget(ctr)
-    qtbot.addWidget(parent)
-    qtbot.waitUntil(lambda: len(ctr.toasts) == 1, timeout=2000)
-
-    # expiring the REPLAYED toast must remove the entry from the broker store
-    ctr.toasts[0].expired.emit()
+    qtbot.waitUntil(lambda: len(ctr_a.toasts) == 1, timeout=2000)
+    ctr_a.toasts[0].closed.emit()
     qtbot.wait(10)
-    assert nid not in broker._active_notifications
+    assert broker._active_notifications == {}
 
-    BECNotificationBroker.reset_singleton()
+    ctr_b = _make_centre(qtbot)
+    qtbot.wait(50)
+    assert ctr_b.toasts == []
+
+
+def test_close_after_reset_reaches_all_centres(qtbot, mocked_client):
+    """A toast created before reset_singleton() must broadcast its close through the new
+    broker; a captured old broker would be deleted and raise 'Signal source has been deleted'."""
+    try:
+        BECNotificationBroker(client=mocked_client)
+        ctr_a = _make_centre(qtbot)
+        ctr_b = _make_centre(qtbot)
+        qtbot.wait(20)
+        for ctr in (ctr_a, ctr_b):
+            ctr.add_notification(
+                title="t", body="b", kind=SeverityKind.MAJOR, lifetime_ms=0, notification_id="nid"
+            )
+
+        BECNotificationBroker.reset_singleton()
+        BECNotificationBroker(client=mocked_client)
+
+        ctr_a.toasts[0].closed.emit()
+        qtbot.waitUntil(lambda: ctr_a.toasts == [] and ctr_b.toasts == [], timeout=2000)
+    finally:
+        BECNotificationBroker.reset_singleton()
+
+
+def test_replay_store_is_bounded(broker, monkeypatch):
+    """The replay store keeps only the newest MAX_REPLAY_NOTIFICATIONS entries."""
+    monkeypatch.setattr(BECNotificationBroker, "MAX_REPLAY_NOTIFICATIONS", 3)
+
+    for i in range(5):
+        broker.post_notification({"alarm_type": f"W{i}", "msg": "m", "severity": 0}, meta={})
+
+    titles = [entry["title"] for entry in broker._active_notifications.values()]
+    assert titles == ["W2", "W3", "W4"]