fix(mqtt): drop dead paho fallback, fit MQTT 3.1 client_id limit

ljmerza · ljmerza · commit c98187faa14e · 2026-05-01T21:20:02.000-04:00
- Drop the `try mqtt.Client(client_id=...) except TypeError: mqtt.Client(client_id)` fallback in `_build_client`. paho-mqtt is pinned &gt;=2.0; the kwarg call always succeeds. If the fallback ever did fire on paho 2.x, the positional arg would be fed to `callback_api_version` (expects an enum) and crash with a worse error than the kwarg call itself. Removed the matching test that locked in the broken behavior.
- Shrink the per-process client_id suffix from 8 hex chars (4 bytes) to 6 hex chars (3 bytes). Default composed `latchpoint-alarm-XXXXXX` now lands at exactly 23 bytes — the MQTT 3.1 spec maximum. paho 2.0+ defaults to 3.1.1 which raises the cap, but staying inside 3.1 protects against stricter brokers. 6 hex still gives 16M distinct values, far above what's needed for 1-2 process collision avoidance.
- Add `test_default_composed_client_id_fits_mqtt_3_1_length_limit` regression guard so a future suffix/prefix bump can't silently push us over.
- Update user-facing strings: settings registry title `"Client ID"` → `"Client ID prefix"` with description explaining the suffix and the 16-char prefix budget; rc=2 connect-error messages now reference "client_id prefix" so users know which knob they're turning.
diff --git a/backend/alarm/settings_registry.py b/backend/alarm/settings_registry.py
@@ -360,9 +360,13 @@ class SettingDefinition:
                 },
                 "client_id": {
                     "type": "string",
-                    "title": "Client ID",
+                    "title": "Client ID prefix",
                     "default": "latchpoint-alarm",
-                    "description": "MQTT client identifier (must be unique per broker)",
+                    "description": (
+                        "Prefix for the MQTT client identifier; a per-process random suffix is "
+                        "appended automatically so multiple Latchpoint processes never collide on "
+                        "the broker. Keep ≤16 chars to stay within MQTT 3.1's 23-byte limit."
+                    ),
                 },
                 "keepalive_seconds": {
                     "type": "integer",
diff --git a/backend/transports_mqtt/manager.py b/backend/transports_mqtt/manager.py
@@ -40,8 +40,8 @@ def _format_connect_error(*, rc_int: int, settings: "MqttConnectionSettings | No
     if rc_int == 2:
         client_id = (settings.get("client_id") or "").strip()
         if client_id:
-            return f"{message} Try a different client_id."
-        return f"{message} Set a unique client_id."
+            return f"{message} Try a different client_id prefix."
+        return f"{message} Set a client_id prefix."
     if rc_int == 3:
         return f"{message} Verify host/port and broker availability."
 
@@ -167,7 +167,13 @@ def __init__(self) -> None:
         # as the same identity and the broker would knock them off each other
         # in a tight loop. Stable within a process, so reconnects/settings
         # changes don't churn the broker's view of the live client.
-        self._client_id_suffix = secrets.token_hex(4)
+        #
+        # Sized at 3 bytes (6 hex chars) so the default composed client_id
+        # `latchpoint-alarm-XXXXXX` lands at exactly 23 characters — the
+        # MQTT 3.1 spec maximum. paho-mqtt 2.0+ defaults to MQTT 3.1.1
+        # which raises this to 65535, but staying inside the 3.1 limit
+        # keeps us safe against any stricter brokers a user might point at.
+        self._client_id_suffix = secrets.token_hex(3)
 
     @staticmethod
     def _topic_matches(*, topic_filter: str, topic: str) -> bool:
@@ -244,8 +250,9 @@ def test_connection(self, *, settings: MqttConnectionSettings, timeout_seconds:
 
         # A fresh suffix per test guarantees the throwaway client never knocks
         # the live persistent client off the broker, even when called from the
-        # same process.
-        client = self._build_client(mqtt=mqtt, settings=settings, client_id_suffix=secrets.token_hex(4))
+        # same process. Sized to match `_client_id_suffix` so the composed ID
+        # stays within MQTT 3.1's 23-byte limit.
+        client = self._build_client(mqtt=mqtt, settings=settings, client_id_suffix=secrets.token_hex(3))
 
         def on_connect(_client, _userdata, _flags, rc, _properties=None):
             """Capture connect return code and release the waiter."""
@@ -388,14 +395,15 @@ def _build_client(self, *, mqtt, settings: MqttConnectionSettings, client_id_suf
         on the same MQTT identity. Callers (e.g. `test_connection`) may pass
         an override suffix to ensure their throwaway client never collides
         with the live one.
+
+        paho-mqtt >=2.0 is pinned (see pyproject.toml); the `client_id=`
+        kwarg call is supported across that range, so no compat fallback
+        is needed.
         """
         base_client_id = str(settings.get("client_id") or "latchpoint-alarm")
         suffix = client_id_suffix or self._client_id_suffix
         client_id = f"{base_client_id}-{suffix}"
-        try:
-            client = mqtt.Client(client_id=client_id)
-        except TypeError:
-            client = mqtt.Client(client_id)
+        client = mqtt.Client(client_id=client_id)
 
         username = settings.get("username") or ""
         password = settings.get("password") or ""
diff --git a/backend/transports_mqtt/tests/test_manager.py b/backend/transports_mqtt/tests/test_manager.py
@@ -139,6 +139,24 @@ def test_empty_configured_client_id_falls_back_to_latchpoint_alarm_prefix(self):
             f"expected default prefix preserved, got {client.client_id!r}",
         )
 
+    def test_default_composed_client_id_fits_mqtt_3_1_length_limit(self):
+        """
+        MQTT 3.1 caps `client_id` at 23 UTF-8 bytes. paho-mqtt 2.0+ defaults
+        to 3.1.1 (which raises the cap to 65535), but keeping the default
+        composed ID inside the 3.1 limit guards against any stricter broker
+        a user might point Latchpoint at. If this test fails, either the
+        suffix grew or the default prefix did — both would silently break
+        connection on a 3.1-only broker.
+        """
+        mgr = MqttConnectionManager()
+        client = mgr._build_client(mqtt=_FakePahoModule, settings={})
+        composed = client.client_id
+        self.assertLessEqual(
+            len(composed),
+            23,
+            f"default composed client_id exceeds MQTT 3.1's 23-byte limit: {composed!r} ({len(composed)} bytes)",
+        )
+
     def test_explicit_suffix_override_does_not_collide_with_live_suffix(self):
         """`test_connection` opts into a fresh suffix so it never knocks the live client off."""
         mgr = MqttConnectionManager()
@@ -151,46 +169,6 @@ def test_explicit_suffix_override_does_not_collide_with_live_suffix(self):
         self.assertNotEqual(live.client_id, test.client_id)
         self.assertTrue(test.client_id.endswith("-abcd1234"))
 
-    def test_typeerror_on_kwarg_falls_back_to_positional_with_suffix_intact(self):
-        """
-        Compat fallback: if a paho version ever rejects `client_id=` as a
-        kwarg, the manager retries positionally. The composed
-        `<prefix>-<suffix>` MUST still reach the broker — otherwise the
-        per-process collision fix is silently lost.
-        """
-        captured: dict[str, object] = {}
-
-        class _KwargRejectingPahoModule:
-            class Client:
-                def __init__(self, *args, **kwargs):
-                    if "client_id" in kwargs:
-                        raise TypeError("client_id keyword unsupported in this paho version")
-                    captured["positional_args"] = args
-                    self.client_id = args[0] if args else None
-
-                def username_pw_set(self, *_a, **_k):
-                    pass
-
-                def tls_set(self, *_a, **_k):
-                    pass
-
-                def tls_insecure_set(self, *_a, **_k):
-                    pass
-
-                def reconnect_delay_set(self, *_a, **_k):
-                    pass
-
-        mgr = MqttConnectionManager()
-        client = mgr._build_client(
-            mqtt=_KwargRejectingPahoModule,
-            settings={"client_id": "latchpoint-alarm"},
-        )
-        self.assertTrue(
-            client.client_id.startswith("latchpoint-alarm-"),
-            f"fallback dropped the prefix: {client.client_id!r}",
-        )
-        self.assertEqual(client.client_id, captured["positional_args"][0])
-
     def test_test_connection_passes_a_fresh_non_live_suffix_to_build_client(self):
         """
         Wiring guard: `test_connection()` MUST forward a non-None override
