From c566fa97c8ea0a3b44082a6da0fc9a8f32954e4b Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 16 Sep 2026 17:32:54 -0700 Subject: [PATCH] fix(channel): improve channel fetching with retries and handle empty results cleanly (#266) --- CHANGELOG.md | 5 +++ modules/channel_manager.py | 54 +++++++++++++++++++++-------- modules/core.py | 7 ++-- tests/test_channel_manager_logic.py | 32 +++++++++++++++++ tests/test_core.py | 22 ++++++++++++ 5 files changed, 103 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1611b51..1fa35ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ semantic versioning. ### Fixed +- A radio connection is no longer accepted when every channel read times out or + returns no usable channel data (#266). Startup and reconnect now retry the + channel scan three times, keep an empty result out of the valid cache and + database, then fail the connection cleanly so the normal restart/reconnect + path can try again instead of running a bot that cannot route replies. - Daily Weather Service forecasts now retry transient Open-Meteo failures at 5, 15, and 30 minutes after the original run (#264). HTTP 429, 500, 502, 503, and 504 responses plus transport failures use one replaceable retry job, diff --git a/modules/channel_manager.py b/modules/channel_manager.py index b065783..6d72363 100644 --- a/modules/channel_manager.py +++ b/modules/channel_manager.py @@ -49,15 +49,26 @@ class ChannelManager: """Lowercase channel name without a leading # for cache lookups.""" return name.removeprefix("#").lower() - async def fetch_channels(self): - """Fetch channels from the MeshCore node using enhanced concurrent fetching""" + async def fetch_channels(self, max_attempts: int = 3, retry_delay: float = 2.0) -> bool: + """Fetch configured channels, retrying when the node returns no usable data.""" self.logger.info("Fetching channels from MeshCore node using enhanced concurrent method...") - try: - # Wait a moment for the device to be ready - await asyncio.sleep(2) + if max_attempts < 1: + raise ValueError("max_attempts must be at least 1") - # Fetch all channels concurrently - channels = await self.fetch_all_channels(force_refresh=True) + # Give the companion protocol a moment to settle after transport connect. + await asyncio.sleep(2) + + for attempt in range(1, max_attempts + 1): + try: + channels = await self.fetch_all_channels(force_refresh=True) + except Exception as e: + channels = [] + self.logger.warning( + "Channel fetch attempt %d/%d failed: %s", + attempt, + max_attempts, + e, + ) if channels: self.logger.info(f"Successfully fetched {len(channels)} channels from MeshCore node") @@ -68,13 +79,25 @@ class ChannelManager: self.logger.info(f" Channel {channel_idx}: {channel_name}") else: self.logger.debug(f" Channel {channel_idx}: (empty)") - else: - self.logger.warning("No channels found on MeshCore node") - self.bot.meshcore.channels = {} + return True - except Exception as e: - self.logger.error(f"Failed to fetch channels: {e}") + self._cache_valid = False self.bot.meshcore.channels = {} + if attempt < max_attempts: + self.logger.warning( + "No channels received from MeshCore node on attempt %d/%d; " + "retrying in %.1fs", + attempt, + max_attempts, + retry_delay, + ) + await asyncio.sleep(retry_delay) + + self.logger.error( + "No channels received from MeshCore node after %d attempt(s)", + max_attempts, + ) + return False async def fetch_all_channels(self, force_refresh: bool = False) -> list[dict[str, Any]]: """ @@ -136,14 +159,15 @@ class ChannelManager: break continue - self._cache_valid = True + self._cache_valid = bool(valid_channels) self.logger.info(f"Successfully fetched {len(valid_channels)} channels") # Update the bot's meshcore channels for compatibility self.bot.meshcore.channels = self._channels_cache - # Store channels in database for web viewer access - self._store_channels_in_db(valid_channels) + # Do not erase the last known channel table on a transient all-empty scan. + if valid_channels: + self._store_channels_in_db(valid_channels) return valid_channels diff --git a/modules/core.py b/modules/core.py index c1ea460..3597d63 100644 --- a/modules/core.py +++ b/modules/core.py @@ -1839,8 +1839,11 @@ long_jokes = false # Wait for contacts to load await self.wait_for_contacts() - # Fetch channels - await self.channel_manager.fetch_channels() + # A connected transport without channel data cannot route replies. + if not await self.channel_manager.fetch_channels(): + raise ConnectionError( + "MeshCore node returned no channels after retries" + ) # Setup message event handlers await self.setup_message_handlers() diff --git a/tests/test_channel_manager_logic.py b/tests/test_channel_manager_logic.py index b0a08c7..bbbcbfd 100644 --- a/tests/test_channel_manager_logic.py +++ b/tests/test_channel_manager_logic.py @@ -187,6 +187,38 @@ class TestFetchAllChannelsCacheLifecycle: # Should have aborted before fetching all 8 channels assert call_count < cm.max_channels assert channels == [] + assert cm._cache_valid is False + cm._store_channels_in_db.assert_not_called() + + +class TestFetchChannelsRetry: + """A connected node with no returned channels must not look ready (#266).""" + + @pytest.mark.asyncio + async def test_retries_empty_scan_then_succeeds(self, cm): + cm.bot.connected = True + channel = {"channel_name": "general", "channel_idx": 0} + cm.fetch_all_channels = AsyncMock(side_effect=[[], [channel]]) + + with patch("asyncio.sleep", new_callable=AsyncMock): + result = await cm.fetch_channels(max_attempts=3, retry_delay=0) + + assert result is True + assert cm.fetch_all_channels.await_count == 2 + + @pytest.mark.asyncio + async def test_exhausted_empty_scans_fail_and_invalidate_cache(self, cm): + cm.bot.connected = True + cm._cache_valid = True + cm.fetch_all_channels = AsyncMock(return_value=[]) + + with patch("asyncio.sleep", new_callable=AsyncMock): + result = await cm.fetch_channels(max_attempts=3, retry_delay=0) + + assert result is False + assert cm.fetch_all_channels.await_count == 3 + assert cm._cache_valid is False + assert cm.bot.meshcore.channels == {} # --------------------------------------------------------------------------- diff --git a/tests/test_core.py b/tests/test_core.py index fabbc9c..a60bef8 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -868,6 +868,28 @@ class TestTransportReconnect: assert bot._radio_relinks_in_progress == 0 assert bot.keep_running is False + def test_connect_fails_when_channel_fetch_never_returns_channels(self, tmp_path): + bot = self._make_bot(tmp_path, connection_type="serial") + new_meshcore = MagicMock() + new_meshcore.is_connected = True + new_meshcore.self_info = {} + new_meshcore.disconnect = AsyncMock() + + bot.wait_for_contacts = AsyncMock() + bot.channel_manager.fetch_channels = AsyncMock(return_value=False) + bot.setup_message_handlers = AsyncMock() + + with patch( + "modules.core.meshcore.MeshCore.create_serial", + AsyncMock(return_value=new_meshcore), + ): + assert asyncio.run(bot.connect()) is False + + new_meshcore.disconnect.assert_awaited_once() + bot.setup_message_handlers.assert_not_awaited() + assert bot.meshcore is None + assert bot.connected is False + def test_keep_running_through_web_viewer_reboot(self, tmp_path): bot = self._make_bot(tmp_path) bot.meshcore = MagicMock()