From 879a86bd80d4f9fd1f4476bd985e83b5b36925be Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 17 Apr 2026 14:37:21 -0700 Subject: [PATCH] fix(message_handler): keep #80 correlation strictly observation-level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplifies find_recent_rf_data so the sample it returns is always a single wire observation — snr, rssi, raw_hex, and routing_info all belong to the same decoded copy. The #80 fix stays intact via the two earlier invariants: unmatched correlation_key returns None (no fallback to an unrelated recent packet), and the no-key DM path is constrained to a narrow rf_fallback_window. An earlier iteration tried to enrich routing_info with the longest observed same-packet_hash forward, but that would have left message_stats rows with hops sourced from the matched observation while path was a different forward's — an internal skew visible in the web viewer and !path output. The enrichment is documented in TODO.md as a v0.9.1+ option-C follow-up that must also sync message.hops for consistency. Regression tests in tests/test_message_handler.py::TestFindRecentRfData cover: return-None on unmatched key, narrow-fallback-window rejection of stale samples, and the no-cross-wiring assertion for same-packet_hash forwards. Made-with: Cursor --- BUGS.md | 2 +- CHANGELOG.md | 11 ++++--- TODO.md | 1 + modules/message_handler.py | 54 +++++++++++------------------------ tests/test_message_handler.py | 44 +++++++++++++++++++--------- 5 files changed, 55 insertions(+), 57 deletions(-) diff --git a/BUGS.md b/BUGS.md index 9c9daa0..3d2577c 100644 --- a/BUGS.md +++ b/BUGS.md @@ -12,7 +12,7 @@ Issues closed in this release. See `CHANGELOG.md` for the full feature/infra lis | Reference | Summary | |-----------|---------| -| Issue #80 | `find_recent_rf_data` no longer mis-correlates `message_stats.path` — when a `correlation_key` is supplied it returns `None` instead of falling back to an arbitrary recent sample, and among duplicates sharing a `packet_hash` it prefers the longest observed path. The no-key fallback is constrained to a narrow `rf_fallback_window` (2s default). Regression tests in `tests/test_message_handler.py::TestFindRecentRfData`. | +| Issue #80 | `find_recent_rf_data` no longer mis-correlates `message_stats.path`. When a `correlation_key` is supplied it returns `None` instead of falling back to an arbitrary recent sample. Correlation stays strictly observation-level: the returned sample's `snr`, `rssi`, `raw_hex`, and `routing_info` all belong to the same decoded wire copy (no cross-forward merging). The no-key fallback is constrained to a narrow `rf_fallback_window` (2s default). Regression tests in `tests/test_message_handler.py::TestFindRecentRfData`. | | Issue #83 | `KeyError('msg_hash')` asyncio parser spam fixed upstream by `meshcore >= 2.3.6` (new `meshcore_parser.py` guards with `'msg_hash' in l`). | | Issue #91 | Bot `!version` command added and version string shown in the web-viewer footer (`883b67d`, `fbf3995`). | | Issue #126 | `can't convert negative int to unsigned` on flood contacts fixed upstream by `meshcore >= 2.3.6` (out_path_len `-1` → `255` before packing). Defensive wire-field rebuild in `_ensure_contact_meshcore_path_encoding` (`ba52c3b`). | diff --git a/CHANGELOG.md b/CHANGELOG.md index c5045f5..f32ec3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,10 +105,13 @@ hardening fixes. ### Fixed - **#80**: `find_recent_rf_data` no longer falls back to an arbitrary recent RF - sample when a `correlation_key` is provided. When multiple RF samples share a - `packet_hash`, the one with the longest observed `path_length` is preferred. The - no-key fallback is constrained to a narrow `rf_fallback_window` (2 s default) to - prevent DMs from picking up unrelated flood-traffic routing info. + sample when a `correlation_key` is provided — it now returns `None` instead + of attaching `message_stats.path` to an unrelated flood packet. Correlation + stays strictly observation-level: the returned sample's `snr`, `rssi`, + `raw_hex`, and `routing_info` all belong to the same decoded wire copy. + The no-key fallback is also constrained to a narrow `rf_fallback_window` + (2 s default) to prevent DMs from picking up unrelated flood-traffic + routing info. - **#126** (negative `out_path_len`): fixed via `meshcore >= 2.3.6` dep bump plus defensive handling in `_ensure_contact_meshcore_path_encoding`. - **#83** (`KeyError('msg_hash')` asyncio spam): fixed via `meshcore >= 2.3.6` dep diff --git a/TODO.md b/TODO.md index 380384f..2c6dfe0 100644 --- a/TODO.md +++ b/TODO.md @@ -210,6 +210,7 @@ Dispositions recorded at v0.9.0 tagging. Revisit for v0.9.1. - Issue #160 — web viewer feed channel edit not persisting - Issue #137 — minimal-config install "not responding" (needs reporter repro with logs first) - TASK-00 — meshcore `IndexError` asyncio handler polish (BUG-022 base fix already shipped; revisit the paused refactor) +- **Path enrichment from same-`packet_hash` flood forwards (option C)** — follow-up to #80. When `find_recent_rf_data` returns the exact `packet_prefix` match, enrich its `routing_info` with the longest observed path from any cached sample sharing the same `packet_hash`, and simultaneously sync `message.hops` to the enriched `path_length` so `message_stats` rows stay internally consistent. Gives the `!path` command and mesh-graph updates the most complete route evidence without the hops/path skew that the naive upgrade would introduce. Touch points: `MessageHandler.find_recent_rf_data`, plus the two sites that copy `recent_rf_data['routing_info']` onto `MeshMessage` (lines ~426 and ~2081 in `modules/message_handler.py`). Regression tests should cover: (a) SNR/RSSI stay pinned to the matched observation, (b) `message.hops` == upgraded `path_length`, (c) no enrichment when `packet_hash` is missing. ### Stale PR triage diff --git a/modules/message_handler.py b/modules/message_handler.py index 6bd6ccf..ced386b 100644 --- a/modules/message_handler.py +++ b/modules/message_handler.py @@ -1178,7 +1178,7 @@ class MessageHandler: def find_recent_rf_data( self, correlation_key: str | None = None, max_age_seconds: float | None = None ) -> dict[str, Any] | None: - """Find recent RF data for SNR/RSSI and packet decoding with improved correlation + """Find recent RF data for SNR/RSSI and packet decoding. Args: correlation_key: Can be either: @@ -1193,10 +1193,11 @@ class MessageHandler: the most recent sample within a narrow ``rf_fallback_window`` so unrelated flood-forwarded packets' routing info can't bleed into DMs. - When multiple samples share the same ``packet_hash`` as a matched sample, the - one with the largest ``routing_info.path_length`` is preferred — this handles - flood-forwarded duplicates where the final hop may arrive after the short - earlier snapshot (issue #80). + The returned sample is always a single wire observation — its ``snr``, + ``rssi``, ``raw_hex`` and ``routing_info`` all belong to the same decoded + copy. No cross-forward merging is performed (see TODO for option C, + which would enrich ``routing_info`` from longer-path forwards of the same + ``packet_hash`` while also syncing ``message.hops`` for consistency). """ import time current_time = time.time() @@ -1213,48 +1214,25 @@ class MessageHandler: self.logger.debug(f"No recent RF data found within {max_age_seconds}s window") return None - def _prefer_longest_path(match: dict[str, Any]) -> dict[str, Any]: - """Among samples sharing ``packet_hash`` with ``match``, pick the longest - observed path. Flood forwards of the same logical packet share a hash but - different path snapshots; we want the most complete route (issue #80).""" - target_hash = match.get('packet_hash') - if not target_hash: - return match - same_hash = [d for d in recent_data if d.get('packet_hash') == target_hash] - if len(same_hash) <= 1: - return match - - def _path_length(d: dict[str, Any]) -> int: - routing = d.get('routing_info') or {} - try: - return int(routing.get('path_length') or 0) - except (TypeError, ValueError): - return 0 - - best = max(same_hash, key=_path_length) - if best is not match: - self.logger.debug( - f"Promoting longer-path duplicate for packet_hash={target_hash}: " - f"path_length {_path_length(match)} -> {_path_length(best)}" - ) - return best - if correlation_key: - # Strategy 1: exact packet prefix match + # Strategy 1: exact packet prefix match — observation-level identity. + # Both the message event and the RF-log event carry the same raw_hex for + # the specific wire copy that was decoded, so this is the authoritative + # match for SNR/RSSI/path attribution. for data in recent_data: rf_packet_prefix = data.get('packet_prefix', '') or '' if rf_packet_prefix == correlation_key: self.logger.debug(f"Found exact packet prefix match: {rf_packet_prefix}") - return _prefer_longest_path(data) + return data - # Strategy 2: exact pubkey prefix match (for message correlation) + # Strategy 2: exact pubkey prefix match (for message correlation). for data in recent_data: rf_pubkey_prefix = data.get('pubkey_prefix', '') or '' if rf_pubkey_prefix == correlation_key: self.logger.debug(f"Found exact pubkey prefix match: {rf_pubkey_prefix}") - return _prefer_longest_path(data) + return data - # Strategy 3: partial packet prefix match (≥ 16 chars) + # Strategy 3: partial packet prefix match (≥ 16 chars). for data in recent_data: rf_packet_prefix = data.get('packet_prefix', '') or '' min_length = min(len(rf_packet_prefix), len(correlation_key), 16) @@ -1263,7 +1241,7 @@ class MessageHandler: f"Found partial packet prefix match: {rf_packet_prefix[:16]}... " f"matches {correlation_key[:16]}..." ) - return _prefer_longest_path(data) + return data # Issue #80: when a correlation_key was provided but nothing matched, do NOT # fall back to an arbitrary recent sample — that's how message_stats.path ends @@ -1288,7 +1266,7 @@ class MessageHandler: f"Using most recent RF data (narrow fallback): {packet_prefix} " f"at {most_recent['timestamp']}" ) - return _prefer_longest_path(most_recent) + return most_recent def store_message_for_correlation(self, message_id: str, message_data: dict[str, Any]) -> None: """Store a message temporarily to wait for RF data correlation""" diff --git a/tests/test_message_handler.py b/tests/test_message_handler.py index 6caadfa..1d3632e 100644 --- a/tests/test_message_handler.py +++ b/tests/test_message_handler.py @@ -342,18 +342,22 @@ class TestFindRecentRfData: """Tests for MessageHandler.find_recent_rf_data().""" def _rf_entry(self, age=0, packet_prefix="aabbccdd", pubkey_prefix="1122", - packet_hash=None, path_length=None): + packet_hash=None, path_length=None, snr=5, rssi=-80, + path_hex=None): entry = { "timestamp": time.time() - age, - "snr": 5, - "rssi": -80, + "snr": snr, + "rssi": rssi, "packet_prefix": packet_prefix, "pubkey_prefix": pubkey_prefix, } if packet_hash is not None: entry["packet_hash"] = packet_hash if path_length is not None: - entry["routing_info"] = {"path_length": path_length} + routing: dict = {"path_length": path_length} + if path_hex is not None: + routing["path_hex"] = path_hex + entry["routing_info"] = routing return entry def test_returns_none_when_empty(self, handler): @@ -430,21 +434,33 @@ class TestFindRecentRfData: result = handler.find_recent_rf_data("deadbeefdeadbeef1234567890abcdef") assert result is None - def test_flood_duplicate_prefers_longest_path(self, handler): - """Issue #80: when multiple RF samples share a packet_hash, prefer the - one with the longest observed path.""" + def test_flood_duplicate_returns_exact_match_not_longer(self, handler): + """Issue #80 (option B): even when multiple RF samples share a + ``packet_hash`` via flood forwarding, ``find_recent_rf_data`` returns + the exact ``packet_prefix`` match verbatim — no cross-forward merging. + The matched sample's ``snr``/``rssi``/``routing_info`` all belong to the + same wire observation that was decoded.""" handler.rf_data_timeout = 30 - short = self._rf_entry( + matched = self._rf_entry( age=2, packet_prefix="aabb" * 8, - packet_hash="9D272188AB364743", path_length=4, + packet_hash="9D272188AB364743", path_length=4, path_hex="f230df22", + snr=3, rssi=-90, ) - long = self._rf_entry( + longer = self._rf_entry( age=1, packet_prefix="ccdd" * 8, - packet_hash="9D272188AB364743", path_length=5, + packet_hash="9D272188AB364743", path_length=5, path_hex="f230df2279", + snr=12, rssi=-60, ) - handler.recent_rf_data = [short, long] - result = handler.find_recent_rf_data(short["packet_prefix"]) - assert result is long + handler.recent_rf_data = [matched, longer] + result = handler.find_recent_rf_data(matched["packet_prefix"]) + + assert result is matched + # Path and signal fields stay anchored to the matched observation — no + # enrichment from the longer forward. + assert result["routing_info"]["path_length"] == 4 + assert result["routing_info"]["path_hex"] == "f230df22" + assert result["snr"] == 3 + assert result["rssi"] == -90 def test_narrow_fallback_window_rejects_stale(self, handler): """Issue #80: with no correlation_key, only very recent samples qualify