From 2aafedae029a1a749e6c75c627088e1c6098238e Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 26 Jul 2026 16:06:18 -0700 Subject: [PATCH] fix(discovery): persist discover.neighbors results with the storage actually wired in MeshCLI._auto_add_discovery_result reached for storage_handler.record_advert and returned early when it was absent. record_advert lives on StorageCollector, but MeshCLI is constructed with the SQLiteHandler (main.py -> TextHelper -> MeshCLI), which exposes only store_advert. The guard therefore always failed in production: `discover.neighbors` discovered nodes and recorded none of them, so `neighbors` listed whatever adverts happened to arrive rather than what discovery found. Tests missed it because they injected a hand-rolled double that did have record_advert. Extract the shared persistence into discovery.persist_discovery_result, which accepts either storage object its callers actually hold: record_advert when present (store plus the MQTT/Glass advert publish), falling back to store_advert. neighbors_publisher._enrich_discovery_result, which had its own working copy against the StorageCollector, now calls it too, so the two paths build an identical advert row from one implementation. The regression test specs its mock off the real SQLiteHandler class rather than hand-rolling the surface, so a double can no longer claim a method the wired-in object does not have. It fails against the previous code with KeyError 'auto_added'. --- repeater/handler_helpers/discovery.py | 65 ++++++++++++++++++++++++++ repeater/handler_helpers/mesh_cli.py | 34 +------------- repeater/neighbors_publisher.py | 32 +------------ tests/test_handler_helpers_mesh_cli.py | 54 +++++++++++++++++++++ 4 files changed, 123 insertions(+), 62 deletions(-) diff --git a/repeater/handler_helpers/discovery.py b/repeater/handler_helpers/discovery.py index 42825c6..7ede2e8 100644 --- a/repeater/handler_helpers/discovery.py +++ b/repeater/handler_helpers/discovery.py @@ -36,6 +36,71 @@ NODE_TYPE_NAMES = { } +def build_discovery_advert_record(result: dict) -> Optional[dict]: + """Turn a discovery response into a zero-hop advert row, or None if unusable. + + A node-discover response is only ever answered by a node that heard our + zero-hop broadcast directly, so the row is recorded with ``zero_hop=True`` + and ``route_type=2`` — the same thing firmware ``putNeighbour()`` records + when a discovery response arrives. + """ + pubkey = str(result.get("pub_key") or "").strip().lower() + if pubkey.startswith("0x"): + pubkey = pubkey[2:] + if not pubkey: + return None + + node_type = int(result.get("node_type", 0) or 0) + rssi = result.get("rssi") + # response_snr is our RX of their reply; plain snr is the fallback shape. + snr = result.get("response_snr", result.get("snr")) + + return { + "timestamp": time.time(), + "pubkey": pubkey, + "node_name": result.get("node_name"), + "is_repeater": node_type == 2, + "route_type": 2, + "contact_type": NODE_TYPE_NAMES.get(node_type, "Unknown"), + "latitude": None, + "longitude": None, + "rssi": int(rssi) if rssi is not None else None, + "snr": float(snr) if snr is not None else None, + "is_new_neighbor": True, + "zero_hop": True, + } + + +def persist_discovery_result(storage, result: dict) -> bool: + """Store one discovery response, returning whether it was written. + + Accepts either storage object the callers actually hold: ``StorageCollector`` + exposes ``record_advert`` (store plus the MQTT/Glass advert publish), while + ``SQLiteHandler`` exposes only ``store_advert``. Preferring the first and + falling back to the second is what makes this work from the mesh CLI, which + is constructed with the SQLiteHandler — checking for ``record_advert`` alone + made ``discover.neighbors`` silently persist nothing. + """ + if storage is None: + return False + + record = build_discovery_advert_record(result) + if record is None: + return False + + writer = getattr(storage, "record_advert", None) or getattr(storage, "store_advert", None) + if not callable(writer): + logger.debug("Storage backend cannot persist discovery results") + return False + + try: + writer(record) + return True + except Exception as e: + logger.debug("Failed to persist discovery result for %s: %s", record["pubkey"][:8], e) + return False + + class DiscoveryHelper: """Helper class for processing discovery requests in the repeater.""" diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index 2d783d4..b14941c 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -129,41 +129,11 @@ class MeshCLI: if not self.storage_handler: return enriched - record_advert = getattr(self.storage_handler, "record_advert", None) - if not callable(record_advert): - return enriched + from repeater.handler_helpers.discovery import persist_discovery_result - try: - import time - - node_type = int(enriched.get("node_type", 0) or 0) - contact_type = { - 1: "Chat Node", - 2: "Repeater", - 3: "Room Server", - }.get(node_type, "Unknown") - - rssi = enriched.get("rssi") - snr = enriched.get("response_snr", enriched.get("snr")) - advert_record = { - "timestamp": time.time(), - "pubkey": pub_key, - "node_name": enriched.get("node_name"), - "is_repeater": node_type == 2, - "route_type": 2, - "contact_type": contact_type, - "latitude": None, - "longitude": None, - "rssi": int(rssi) if rssi is not None else None, - "snr": float(snr) if snr is not None else None, - "is_new_neighbor": True, - "zero_hop": True, - } - record_advert(advert_record) + if persist_discovery_result(self.storage_handler, enriched): enriched["known_neighbor"] = True enriched["auto_added"] = True - except Exception as exc: - logger.debug("Auto-add discovery result failed for %s: %s", pub_key, exc) return enriched diff --git a/repeater/neighbors_publisher.py b/repeater/neighbors_publisher.py index 4c6438b..77c1014 100644 --- a/repeater/neighbors_publisher.py +++ b/repeater/neighbors_publisher.py @@ -25,6 +25,7 @@ import time from datetime import datetime, timezone from typing import Any, Callable, Dict, List, Optional +from repeater.handler_helpers.discovery import persist_discovery_result from repeater.handler_helpers.neighbor_scopes import ( STATUS_RESPONDED, STATUS_TIMEOUT, @@ -431,36 +432,7 @@ class NeighborsPublisher: "snr": float(snr_raw) if snr_raw is not None else 0.0, } - storage = self._storage() - record_advert = getattr(storage, "record_advert", None) if storage else None - if not callable(record_advert): - return result - - node_type = int(result.get("node_type", 0) or 0) - rssi = result.get("rssi") - snr = result.get("response_snr", result.get("snr")) - try: - record_advert( - { - "timestamp": time.time(), - "pubkey": pubkey, - "node_name": result.get("node_name"), - "is_repeater": node_type == 2, - "route_type": 2, - "contact_type": {1: "Chat Node", 2: "Repeater", 3: "Room Server"}.get( - node_type, "Unknown" - ), - "latitude": None, - "longitude": None, - "rssi": int(rssi) if rssi is not None else None, - "snr": float(snr) if snr is not None else None, - "is_new_neighbor": True, - "zero_hop": True, - } - ) - except Exception as e: - logger.debug(f"Could not persist discovery result for {pubkey[:8]}: {e}") - + persist_discovery_result(self._storage(), result) return result def _snapshot_neighbors(self) -> List[NeighborSnapshot]: diff --git a/tests/test_handler_helpers_mesh_cli.py b/tests/test_handler_helpers_mesh_cli.py index 6504176..854be6e 100644 --- a/tests/test_handler_helpers_mesh_cli.py +++ b/tests/test_handler_helpers_mesh_cli.py @@ -475,6 +475,60 @@ def test_discovery_auto_add_skips_local_node_and_persists_remote(): storage.record_advert.assert_called_once() +def test_discovery_auto_add_persists_through_the_storage_actually_wired_in(): + """MeshCLI is constructed with the SQLiteHandler, not the StorageCollector. + + See repeater/main.py:516 -> TextHelper(sqlite_handler=...storage.sqlite_handler) + -> MeshCLI(storage_handler=self.sqlite_handler). SQLiteHandler exposes + store_advert but no record_advert, so a persistence path that only looked for + record_advert made `discover.neighbors` silently record nothing in production + while passing tests that injected a hand-rolled double. Spec'ing the mock off + the real class is what keeps this honest. + """ + from repeater.data_acquisition.sqlite_handler import SQLiteHandler + + identity = SimpleNamespace(get_public_key=lambda: bytes.fromhex("11" * 32)) + storage = MagicMock(spec=SQLiteHandler) + cli = MeshCLI( + "/tmp/cfg.yaml", _base_config(), _cfg_mgr(), identity=identity, storage_handler=storage + ) + + result = cli._auto_add_discovery_result( + { + "pub_key": "22" * 32, + "node_name": "Remote Repeater", + "node_type": 2, + "rssi": -70, + "response_snr": 4.25, + } + ) + + assert result["auto_added"] is True + storage.store_advert.assert_called_once() + record = storage.store_advert.call_args.args[0] + assert record["pubkey"] == "22" * 32 + assert record["is_repeater"] is True + assert record["zero_hop"] is True + assert record["snr"] == 4.25 + assert record["rssi"] == -70 + + +def test_discovery_auto_add_prefers_record_advert_when_the_collector_is_wired_in(): + """StorageCollector.record_advert also publishes the advert; prefer it.""" + from repeater.data_acquisition.storage_collector import StorageCollector + + identity = SimpleNamespace(get_public_key=lambda: bytes.fromhex("11" * 32)) + storage = MagicMock(spec=StorageCollector) + cli = MeshCLI( + "/tmp/cfg.yaml", _base_config(), _cfg_mgr(), identity=identity, storage_handler=storage + ) + + result = cli._auto_add_discovery_result({"pub_key": "22" * 32, "node_type": 2}) + + assert result["auto_added"] is True + storage.record_advert.assert_called_once() + + def test_cmd_set_save_failure_reports_error_and_skips_live_update(): mgr = _cfg_mgr(save_ok=False) cli = MeshCLI("/tmp/cfg.yaml", _base_config(), mgr)