From 3d8917403893c96ff46c9f0e3127933b154daa06 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 24 Jul 2026 08:50:34 -0700 Subject: [PATCH] fix(repeater): report all boot-time config errors without a stack trace Startup config mistakes were handled inconsistently: an identity collision exited cleanly, but a missing or invalid config file (FileNotFoundError / RuntimeError from load_config) and a missing identity key (RuntimeError) dumped a full traceback -- and load_config ran outside main()'s try, so those errors escaped the fatal handler entirely. Add repeater/exceptions.py with ConfigurationError(RuntimeError) and re-parent IdentityConfigurationError onto it (it stays a RuntimeError, so existing except-sites are unaffected). Raise ConfigurationError for the missing/invalid config file, the config-load failure, and the missing identity key. Move load_config and daemon construction inside main()'s try and catch ConfigurationError there, logging just the message and exiting 1; unexpected failures still log with a traceback. --- repeater/config.py | 5 +-- repeater/exceptions.py | 17 ++++++++++ repeater/identity_manager.py | 4 ++- repeater/main.py | 40 +++++++++++----------- tests/test_main_py_coverage.py | 61 ++++++++++++++++++++++++++++++++-- 5 files changed, 103 insertions(+), 24 deletions(-) create mode 100644 repeater/exceptions.py diff --git a/repeater/config.py b/repeater/config.py index 7274f05..a330461 100644 --- a/repeater/config.py +++ b/repeater/config.py @@ -6,6 +6,7 @@ from typing import Any, Dict, Optional, overload import yaml +from repeater.exceptions import ConfigurationError from repeater.policy_engine import default_policy_engine_config logger = logging.getLogger("Config") @@ -202,7 +203,7 @@ def load_config(config_path: Optional[str] = None) -> Dict[str, Any]: # Check if config file exists if not Path(config_path).exists(): - raise FileNotFoundError( + raise ConfigurationError( f"Configuration file not found: {config_path}\n" f"Please create a config file. Example: \n" f" sudo cp {Path(config_path).parent}/config.yaml.example {config_path}\n" @@ -215,7 +216,7 @@ def load_config(config_path: Optional[str] = None) -> Dict[str, Any]: config = yaml.safe_load(f) or {} logger.info(f"Loaded config from {config_path}") except Exception as e: - raise RuntimeError(f"Failed to load configuration from {config_path}: {e}") from e + raise ConfigurationError(f"Failed to load configuration from {config_path}: {e}") from e storage_dir = resolve_storage_dir(config, config_path=config_path) if "storage" not in config or not isinstance(config.get("storage"), dict): diff --git a/repeater/exceptions.py b/repeater/exceptions.py new file mode 100644 index 0000000..a2fe4d0 --- /dev/null +++ b/repeater/exceptions.py @@ -0,0 +1,17 @@ +"""Shared exception types for the repeater. + +Kept dependency-free (no other ``repeater`` imports) so any module can raise +these without risking an import cycle. +""" + + +class ConfigurationError(RuntimeError): + """A user-actionable configuration problem. + + Raised for boot-time config mistakes (missing/invalid config file, missing + required keys, colliding local identities). ``main()`` reports the message + and exits non-zero *without* a stack trace, since the fix is in the config, + not the code. Subclasses ``RuntimeError`` so existing ``except + RuntimeError`` sites keep catching the errors that used to be raised as + plain ``RuntimeError``. + """ diff --git a/repeater/identity_manager.py b/repeater/identity_manager.py index 28ea4c7..c276acc 100644 --- a/repeater/identity_manager.py +++ b/repeater/identity_manager.py @@ -2,6 +2,8 @@ import logging from dataclasses import dataclass from typing import Any, Dict, Iterable, Optional, Tuple +from repeater.exceptions import ConfigurationError + logger = logging.getLogger("IdentityManager") @@ -22,7 +24,7 @@ def _namespace_for(identity_type: str) -> str: return "companion" if identity_type == "companion" else "server" -class IdentityConfigurationError(RuntimeError): +class IdentityConfigurationError(ConfigurationError): """A configured local identity cannot be represented safely.""" diff --git a/repeater/main.py b/repeater/main.py index aa3f2b0..ee663c4 100644 --- a/repeater/main.py +++ b/repeater/main.py @@ -25,6 +25,7 @@ from repeater.config_manager import ConfigManager from repeater.data_acquisition.glass_handler import GlassHandler from repeater.data_acquisition.gps_service import GPSService from repeater.engine import RepeaterHandler +from repeater.exceptions import ConfigurationError from repeater.handler_helpers import ( AdvertHelper, DiscoveryHelper, @@ -404,7 +405,7 @@ class RepeaterDaemon: identity_key = self.config.get("repeater", {}).get("identity_key") if not identity_key: logger.error("No identity key found in configuration. Cannot init repeater.") - raise RuntimeError("Identity key is required for repeater operation") + raise ConfigurationError("Identity key is required for repeater operation") local_identity = LocalIdentity(seed=identity_key) self.local_identity = local_identity @@ -1755,28 +1756,29 @@ def main(): args = parser.parse_args() - # Load configuration - config = load_config(args.config) - config_path = args.config if args.config else "/etc/openhop_repeater/config.yaml" - - if args.log_level: - if "logging" not in config: - config["logging"] = {} - config["logging"]["level"] = args.log_level - - # Don't initialize radio here - it will be done inside the async event loop - daemon = RepeaterDaemon(config, radio=None) - daemon.config_path = config_path - - # Run + # Load configuration, build the daemon, and run it. Config mistakes (a + # missing or invalid config file, a missing required key, colliding local + # identities) surface as ConfigurationError and exit cleanly with just the + # message; only unexpected failures get the full traceback. try: + config = load_config(args.config) + config_path = args.config if args.config else "/etc/openhop_repeater/config.yaml" + + if args.log_level: + if "logging" not in config: + config["logging"] = {} + config["logging"]["level"] = args.log_level + + # Don't initialize radio here - it will be done inside the async event loop + daemon = RepeaterDaemon(config, radio=None) + daemon.config_path = config_path + asyncio.run(daemon.run()) except KeyboardInterrupt: logger.info("Repeater stopped") - except IdentityConfigurationError as e: - # A misconfigured local identity is an actionable config problem, not a - # crash: report just the message so the fix is obvious, no stack trace. - logger.error("Identity configuration error: %s", e) + except ConfigurationError as e: + # An actionable config problem, not a crash: report just the message. + logger.error("Configuration error: %s", e) sys.exit(1) except Exception as e: logger.error(f"Fatal error: {e}", exc_info=True) diff --git a/tests/test_main_py_coverage.py b/tests/test_main_py_coverage.py index 85f28e5..f694891 100644 --- a/tests/test_main_py_coverage.py +++ b/tests/test_main_py_coverage.py @@ -5,6 +5,7 @@ from unittest.mock import AsyncMock, MagicMock, call, patch import pytest from repeater.companion.constants import STATS_TYPE_CORE, STATS_TYPE_PACKETS, STATS_TYPE_RADIO +from repeater.exceptions import ConfigurationError from repeater.identity_manager import IdentityConfigurationError from repeater.main import RepeaterDaemon from repeater.main import main as repeater_main @@ -538,7 +539,8 @@ def test_main_entrypoint_success_and_fatal_paths(monkeypatch): def test_main_identity_config_error_exits_cleanly_without_traceback(caplog): - """A configured-identity collision exits 1 with a clean message and no + """A configured-identity collision (an IdentityConfigurationError, which is + a ConfigurationError subclass) exits 1 with a clean message and no stack-trace dump (regression: the fatal handler used to log exc_info=True for every exception).""" @@ -563,7 +565,62 @@ def test_main_identity_config_error_exits_cleanly_without_traceback(caplog): repeater_main() exit_mock.assert_called_once_with(1) - config_errors = [r for r in caplog.records if "Identity configuration error" in r.getMessage()] + config_errors = [r for r in caplog.records if "Configuration error" in r.getMessage()] assert len(config_errors) == 1 assert config_errors[0].exc_info is None # no traceback attached assert not any("Fatal error" in r.getMessage() for r in caplog.records) + + +def test_main_config_load_error_exits_cleanly_without_traceback(caplog): + """A ConfigurationError from load_config (missing/invalid config file) is + now inside the try, so it also exits 1 cleanly rather than dumping a trace + from before the event loop starts.""" + + class _Args: + config = "/tmp/missing.yaml" + log_level = None + + with ( + patch("argparse.ArgumentParser.parse_args", return_value=_Args()), + patch( + "repeater.main.load_config", + side_effect=ConfigurationError("Configuration file not found: /tmp/missing.yaml"), + ), + patch("sys.exit", side_effect=SystemExit(1)) as exit_mock, + caplog.at_level(logging.ERROR), + ): + with pytest.raises(SystemExit): + repeater_main() + + exit_mock.assert_called_once_with(1) + config_errors = [r for r in caplog.records if "Configuration error" in r.getMessage()] + assert len(config_errors) == 1 + assert config_errors[0].exc_info is None + assert not any("Fatal error" in r.getMessage() for r in caplog.records) + + +def test_main_unexpected_error_keeps_traceback(caplog): + """A genuine, non-config failure still logs 'Fatal error' with a traceback.""" + + class _Args: + config = "/tmp/test.yaml" + log_level = None + + fake_daemon = SimpleNamespace(run=MagicMock(return_value=object())) + + with ( + patch("argparse.ArgumentParser.parse_args", return_value=_Args()), + patch("repeater.main.load_config", return_value=_base_config()), + patch("repeater.main.RepeaterDaemon", return_value=fake_daemon), + patch("asyncio.run", side_effect=RuntimeError("boom")), + patch("sys.exit", side_effect=SystemExit(1)) as exit_mock, + caplog.at_level(logging.ERROR), + ): + with pytest.raises(SystemExit): + repeater_main() + + exit_mock.assert_called_once_with(1) + fatal = [r for r in caplog.records if "Fatal error" in r.getMessage()] + assert len(fatal) == 1 + assert fatal[0].exc_info is not None # traceback preserved for real crashes + assert not any("Configuration error" in r.getMessage() for r in caplog.records)