|
| 1 | +# Plan: pre-create pubsub event/state nodes at module startup |
| 2 | + |
| 3 | +Status: **proposed** |
| 4 | + |
| 5 | +Tracks #824. Repos: pyobs-core. |
| 6 | + |
| 7 | +Background: `2026-08-16-explicit-pubsub-event-subscriptions.md` moved event delivery onto |
| 8 | +explicit XEP-0060 subscriptions against the shared `pubsub.<domain>` service; nodes are |
| 9 | +named `pyobs:event:{module}:{Event}:{version}` (`_event_node`, `xmppcomm.py:929`) and |
| 10 | +`pyobs:state:{module}:{Interface}:{version}` (`_state_node`, `xmppcomm.py:1102`). |
| 11 | + |
| 12 | +## Problem |
| 13 | + |
| 14 | +Pubsub nodes are only created by the server when the publisher publishes to them for the |
| 15 | +first time (lazy auto-create). Before that, a subscribe attempt fails with |
| 16 | +`item-not-found`, which is why both subscribe paths retry indefinitely in background |
| 17 | +tasks (`_subscribe_event_with_retry`, `xmppcomm.py:942-968`; `_subscribe_with_retry`, |
| 18 | +`xmppcomm.py:1293-1338`). Consequences: |
| 19 | + |
| 20 | +- A consumer cannot complete a subscription — and therefore cannot receive anything — |
| 21 | + until the producer has sent that event/state once (e.g. a client that connects before |
| 22 | + the camera's first exposure can never subscribe to `camera:NewImageEvent` until that |
| 23 | + first image exists). |
| 24 | +- The retry machinery is the only thing bridging that gap, and #824 shows it is not |
| 25 | + robust: `_retry_delay` (`xmppcomm.py:52-60`) overflows at attempt 1024, the retry task |
| 26 | + dies, and the `(peer, event)` key stays in `_event_subscriptions`, permanently marking |
| 27 | + the pair subscribed while nothing is subscribed and nothing is retrying. The state path |
| 28 | + has the same stuck-state class of bug (`_state_node_handlers`, `xmppcomm.py:1348`). |
| 29 | + |
| 30 | +## Design |
| 31 | + |
| 32 | +### Phase 1 — pre-create own nodes at startup (the core change) |
| 33 | + |
| 34 | +A module creates its own event and state nodes during startup, so they exist *before* the |
| 35 | +first publish — and, importantly, *before* the module announces presence, so peers that |
| 36 | +react to it in `_got_online` (`xmppcomm.py:669-746`) land their subscriptions on the first |
| 37 | +attempt. |
| 38 | + |
| 39 | +- **New helper `_create_node(node)`**: `await self._safe_send(self.client.plugin["xep_0060"].create_node, self._pubsub_service, node)`. |
| 40 | + `create_node` is standard XEP-0060 (no item, no event). Wrap in |
| 41 | + `except slixmpp.exceptions.IqError` → `log.debug(...)` and continue: the realistic |
| 42 | + error is `<conflict/>` on restart (nodes persist server-side with `persist_items`), and |
| 43 | + a permission denial should degrade gracefully to today's lazy auto-create, never block |
| 44 | + startup. `_safe_send` already bounds the call (`xmppcomm.py:1055-1096`); IqError |
| 45 | + propagates to our handler, IqTimeout is retried there, so a dead server cannot hang |
| 46 | + `open()` beyond its existing budget. |
| 47 | +- **Event nodes** — in `XmppComm._register_events` (`xmppcomm.py:889`), in the |
| 48 | + `handler is None` branch (send-only declaration): for each `ev` in `events`, if |
| 49 | + `self._module is not None`, create `self._event_node(self._module.name, ev)`. This is |
| 50 | + the moment a module declares what it publishes (`Comm.register_event`, `comm.py:452` |
| 51 | + puts the class into `_events_sent`); module `_open()`s call it after `comm.open()` |
| 52 | + (e.g. `basecamera.py:126`, `weather.py:92`) and before `startup()` announces presence. |
| 53 | + Local events never reach `_register_events` (`comm.py:455`), so nothing changes there. |
| 54 | +- **State nodes** — in `XmppComm.open()` (`xmppcomm.py:276`), next to the disco-feature |
| 55 | + loop that already knows the exact set (`xmppcomm.py:352-356`): for each `i` in |
| 56 | + `self._module.interfaces` with `i.has_own_state()`, create |
| 57 | + `self._state_node(self._module.name, i)`. |
| 58 | +- **No config form**: pre-created nodes inherit the server's `default_node_config`, the |
| 59 | + same settings auto-created nodes get today (test ejabberd.yml: `max_items: 1`, |
| 60 | + `persist_items: true`, `deliver_payloads: true`), so effective behavior is identical — |
| 61 | + an empty node delivers nothing until the first publish, and a late subscriber still |
| 62 | + gets only the latest item. |
| 63 | +- **Out of scope**: module-less comms (GUI, admin tools) create no nodes — they only |
| 64 | + subscribe; their own publish set is not part of this plan. |
| 65 | +- **Bonus side-effect**: `_get_derived_events` expansion (`comm.py:437`) means peers |
| 66 | + subscribe to every `role="send"`-advertised node (`xmppcomm.py:740-746`), including |
| 67 | + derived classes that may never be published; pre-creating the full declared set removes |
| 68 | + the "retry forever on a node that will never be created" case for the module's own |
| 69 | + nodes too. |
| 70 | + |
| 71 | +The retry loops stay as a backstop: a subscriber that starts *before* the publisher's |
| 72 | +startup finishes creating nodes, or while the publisher is offline, still needs them. |
| 73 | + |
| 74 | +### Phase 2 — harden the retry machinery (issue #824) |
| 75 | + |
| 76 | +Small, separable from Phase 1, but they are what make the backstop trustworthy: |
| 77 | + |
| 78 | +- **Clamp the exponent in `_retry_delay`** (`xmppcomm.py:60`): |
| 79 | + `return random.uniform(0, min(cap, base * (2 ** min(attempt, 60))))`. |
| 80 | + `2**60 ≈ 1.15e18` is far above any real cap and well inside float range, so |
| 81 | + `min()` now protects the computation as intended. Fixes the `OverflowError` at every |
| 82 | + call site (`xmppcomm.py:968, 1089, 1093, 1239, 1318`). |
| 83 | +- **Discard the key on abnormal exit in `_subscribe_event_with_retry`** (`xmppcomm.py:942-968`): |
| 84 | + wrap the retry `while` loop in `try/except Exception`: on an unexpected exception, |
| 85 | + `self._event_subscriptions.discard(key)` (key added at `:953`, guard read at `:951`), |
| 86 | + then re-raise so the failure surfaces once via the task-exception logger |
| 87 | + (`_log_task_exception`, `xmppcomm.py:63`). A later `register_event`/`_got_online` |
| 88 | + re-subscribes from scratch instead of short-circuiting on the stale key — the reporter's |
| 89 | + own suggested behavior in #824. |
| 90 | +- **Same treatment for state**: `_subscribe_with_retry` (`xmppcomm.py:1293-1338`) has the |
| 91 | + identical stuck-state class of bug — if the task dies, the `_state_node_handlers` entry |
| 92 | + (created at `:1348`) is never cleaned and `_subscribe_state` short-circuits at |
| 93 | + `:1342-1346`, so live state updates are lost forever. On unexpected exception: |
| 94 | + `log.exception(...)`, remove `node` from `_state_node_handlers`, re-raise. (The exponent |
| 95 | + clamp is the primary protection; this is belt-and-braces. Note the callback-append path |
| 96 | + at `:1344`: a later re-subscribe registers only the latest callback — acceptable for a |
| 97 | + catastrophic, unexpected failure, and strictly better than permanent silent loss.) |
| 98 | + |
| 99 | +## Checklist |
| 100 | + |
| 101 | +### Phase 1: node pre-creation |
| 102 | + |
| 103 | +- [ ] `_create_node(node)` helper on `XmppComm`: `_safe_send(xep_0060.create_node, ...)`, |
| 104 | + `IqError` → debug log, never raise. |
| 105 | +- [ ] Event nodes: `XmppComm._register_events` handler-`None` branch, guarded on |
| 106 | + `self._module is not None`. |
| 107 | +- [ ] State nodes: `XmppComm.open()` next to the disco-feature loop (`xmppcomm.py:352-356`), |
| 108 | + for `has_own_state()` interfaces. |
| 109 | +- [ ] Confirm local events still bypass pubsub entirely (they never reach `_register_events`). |
| 110 | +- [ ] Unit test: `_event_node`/`_state_node` naming unchanged. |
| 111 | +- [ ] Integration test: publisher starts and never publishes → node exists server-side |
| 112 | + (subscribe succeeds / `get_nodes` shows it). |
| 113 | + |
| 114 | +### Phase 2: retry hardening (#824) |
| 115 | + |
| 116 | +- [ ] `_retry_delay` exponent clamp (`xmppcomm.py:60`). |
| 117 | +- [ ] `_subscribe_event_with_retry`: discard key on abnormal exit, re-raise. |
| 118 | +- [ ] `_subscribe_with_retry`: drop `_state_node_handlers` entry on abnormal exit, re-raise. |
| 119 | +- [ ] Unit tests: `_retry_delay(1024)` / `_retry_delay(10**6)` return a float in `[0, cap]` |
| 120 | + (would overflow before the fix). |
| 121 | +- [ ] Integration regression: simulated retry-task failure leaves the pair re-subscribable. |
| 122 | + |
| 123 | +## Testing / validation (`tests/xmpp/docker-compose.yml` harness) |
| 124 | + |
| 125 | +Extend the existing `tests/integration/test_xmpp_event_subscriptions.py` pattern: |
| 126 | + |
| 127 | +1. **Subscribe before first publish**: `observer` subscribes to `camera`'s |
| 128 | + `NewImageEvent` node after `camera` started but *before* `camera` ever publishes; |
| 129 | + assert the subscribe succeeds (previously it retried until the first publish). |
| 130 | +2. **First event still delivered** after the pre-created-node subscription lands |
| 131 | + (delivery semantics unchanged). |
| 132 | +3. **Restart**: `camera` restarts (same bare JID) → `<conflict/>` tolerated, no error |
| 133 | + spam, existing and new subscriptions keep working. |
| 134 | +4. **Subscriber before publisher**: `observer` connects first, `camera` starts later → |
| 135 | + `observer`'s retry lands as soon as `camera`'s startup pre-creates the node, without |
| 136 | + any publish having happened. |
| 137 | +5. **#824 regression (event)**: after the retry task fails abnormally, a fresh |
| 138 | + registration re-subscribes successfully (old code: short-circuits on the stale key). |
| 139 | +6. Full existing unit + XMPP integration suite stays green (1600+ unit tests currently |
| 140 | + passing). |
| 141 | + |
| 142 | +## Rollout |
| 143 | + |
| 144 | +- **No protocol change, no node-naming change, rolling-upgrade compatible.** Old modules |
| 145 | + still lazy-create on first publish; new modules pre-create. Mixed fleets work: the |
| 146 | + retry loops bridge any ordering. |
| 147 | +- **Server ACL**: creating nodes on the shared pubsub service requires |
| 148 | + `access_createnode` for the module's JID. The test ejabberd allows all local users |
| 149 | + (`tests/xmpp/ejabberd.yml:23`); for production, verify the pubsub ACL permits module |
| 150 | + JIDs. If it doesn't, Phase 1 degrades gracefully (creation logs, lazy creation |
| 151 | + continues) — Phase 2 still ships independently. |
| 152 | +- **Changelog**: add entries under the next `CHANGELOG.rst` dev heading when this lands |
| 153 | + (dev releases are exempt from `scripts/check_changelog.sh`). |
0 commit comments