diff --git a/src/vocalinux/main.py b/src/vocalinux/main.py index b8c093f..f842d04 100644 --- a/src/vocalinux/main.py +++ b/src/vocalinux/main.py @@ -29,9 +29,10 @@ def _should_append_trailing_space() -> bool: """Return whether completed transcriptions should get a trailing space. Reads config.json from disk on each call so Settings toggles take effect - immediately. TrayIndicator and main() each construct their own - ConfigManager, so an in-memory read from main's instance would miss - Settings writes (same pattern as TextInjector._should_copy_to_clipboard). + immediately. Historically TrayIndicator and main() each constructed their + own ConfigManager, so an in-memory read would miss Settings writes; the + instance is shared now, but the disk read stays as the conservative path + (same pattern as TextInjector._should_copy_to_clipboard). """ try: import json @@ -325,7 +326,7 @@ def main(): from .text_injection import text_injector from .ui import tray_indicator from .ui.action_handler import ActionHandler - from .ui.config_manager import ConfigManager + from .ui.config_manager import get_shared_config_manager from .ui.logging_manager import initialize_logging # Initialize logging manager early @@ -342,7 +343,7 @@ def main(): except Exception as e: logger.debug(f"Could not start IBus daemon: {e}") - config_manager = ConfigManager() + config_manager = get_shared_config_manager() saved_settings = config_manager.get_settings().get("speech_recognition", {}) audio_settings = config_manager.get_settings().get("audio", {}) @@ -501,8 +502,7 @@ def main(): text_to_inject = capitalize_sentences(text_to_inject) - # Read from disk so the Settings toggle applies without restart - # (main and tray each own a ConfigManager instance). + # Read from disk so the Settings toggle applies without restart. append_trailing_space = _should_append_trailing_space() if append_trailing_space: diff --git a/src/vocalinux/ui/audio_feedback.py b/src/vocalinux/ui/audio_feedback.py index d78d5d0..230957b 100644 --- a/src/vocalinux/ui/audio_feedback.py +++ b/src/vocalinux/ui/audio_feedback.py @@ -59,18 +59,18 @@ ERROR_SOUND = _resource_manager.get_sound_path("error") def _is_sound_effects_enabled() -> bool: try: - from .config_manager import ConfigManager + from .config_manager import get_shared_config_manager - return ConfigManager().is_sound_effects_enabled() + return get_shared_config_manager().is_sound_effects_enabled() except Exception: return True def _resolved_tone() -> str: try: - from .config_manager import ConfigManager + from .config_manager import get_shared_config_manager - return ConfigManager().get_sound_effects_tone() + return get_shared_config_manager().get_sound_effects_tone() except Exception: return DEFAULT_SOUND_EFFECT_TONE diff --git a/src/vocalinux/ui/config_manager.py b/src/vocalinux/ui/config_manager.py index 333524c..dff3672 100644 --- a/src/vocalinux/ui/config_manager.py +++ b/src/vocalinux/ui/config_manager.py @@ -8,6 +8,7 @@ import copy import json import logging import os +import threading from typing import Any, Optional from ..utils.paths import config_dir @@ -487,3 +488,27 @@ class ConfigManager: self._update_dict_recursive(target[key], value) else: target[key] = value + + +# The one instance the application should share. Every ConfigManager caches the +# whole config in memory and save_config() writes that whole cache back, so two +# live instances silently revert each other's saves — the second save wins with +# whatever stale values it still holds. Tests may still construct ConfigManager +# directly; application code goes through this accessor. +_shared_instance: Optional[ConfigManager] = None +_shared_instance_lock = threading.Lock() + + +def get_shared_config_manager() -> ConfigManager: + """Return the process-wide ConfigManager, creating it on first use. + + Creation is locked: __init__ does makedirs() plus a full load_config(), + so two threads racing the first call would otherwise each build their own + instance and reintroduce the very overwrite this accessor prevents. + """ + global _shared_instance + if _shared_instance is None: + with _shared_instance_lock: + if _shared_instance is None: + _shared_instance = ConfigManager() + return _shared_instance diff --git a/src/vocalinux/ui/tray_indicator.py b/src/vocalinux/ui/tray_indicator.py index adbf52a..41c40b1 100644 --- a/src/vocalinux/ui/tray_indicator.py +++ b/src/vocalinux/ui/tray_indicator.py @@ -40,7 +40,7 @@ from ..suspend_handler import SuspendHandler from ..utils.resource_manager import ResourceManager from ..utils.update_checker import ReleaseInfo from ..utils.update_monitor import UpdateMonitor -from .config_manager import ConfigManager +from .config_manager import get_shared_config_manager from .keyboard_backends import DEFAULT_SHORTCUT, DEFAULT_SHORTCUT_MODE from .keyboard_shortcuts import KeyboardShortcutManager from .settings_dialog import SettingsDialog @@ -104,7 +104,9 @@ class TrayIndicator: """ self.speech_engine = speech_engine self.text_injector = text_injector - self.config_manager = ConfigManager() # Added: Initialize ConfigManager + # Shared with main() and the settings dialog: separate instances would + # overwrite each other's saves with stale in-memory copies. + self.config_manager = get_shared_config_manager() self._syncing_autostart_menu = False # Get configured shortcut and mode from config diff --git a/tests/conftest.py b/tests/conftest.py index 21283a6..0e6963f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -140,6 +140,30 @@ def _suppress_desktop_notifications(request): yield +@pytest.fixture(autouse=True) +def _reset_shared_config_manager(): + """Drop the process-wide ConfigManager between tests. + + In production one shared instance is the point (separate instances + overwrite each other's saves with stale caches). In tests it would be + created under whichever mocks the first caller had active and then leak + that mock into every later test, so each test starts without one. + """ + try: + from vocalinux.ui import config_manager as _cm + + _cm._shared_instance = None + except Exception: + pass + yield + try: + from vocalinux.ui import config_manager as _cm + + _cm._shared_instance = None + except Exception: + pass + + @pytest.fixture(autouse=True) def _clear_hardware_detection_cache(): """Clear lru_cache on hardware-detection helpers between tests. diff --git a/tests/test_audio_feedback.py b/tests/test_audio_feedback.py index 43417c3..92691e7 100644 --- a/tests/test_audio_feedback.py +++ b/tests/test_audio_feedback.py @@ -419,7 +419,7 @@ class TestAudioFeedback(unittest.TestCase): import vocalinux.ui.audio_feedback as audio_feedback with patch( - "vocalinux.ui.config_manager.ConfigManager", + "vocalinux.ui.config_manager.get_shared_config_manager", side_effect=Exception("No config"), ): result = audio_feedback._is_sound_effects_enabled() diff --git a/tests/test_main.py b/tests/test_main.py index 9cf9d0e..cb2e211 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -125,7 +125,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.main.check_dependencies") @patch("vocalinux.main.parse_arguments") @patch("vocalinux.main.sys.exit") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_main_exits_on_init_error( self, mock_init_logging, mock_config, mock_exit, mock_parse, mock_check_deps @@ -162,7 +162,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") @patch("vocalinux.main.logging") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_main_initializes_components( self, @@ -259,7 +259,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.speech_recognition.recognition_manager.SpeechRecognitionManager") @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_registered_callbacks_keep_spacing_across_processing_to_listening( self, @@ -373,7 +373,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.speech_recognition.recognition_manager.SpeechRecognitionManager") @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_vosk_auto_capitalize_applies_to_injected_text( self, @@ -402,7 +402,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.speech_recognition.recognition_manager.SpeechRecognitionManager") @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_vosk_auto_capitalize_can_be_disabled( self, @@ -431,7 +431,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.speech_recognition.recognition_manager.SpeechRecognitionManager") @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_whisper_skips_auto_capitalize( self, @@ -462,7 +462,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") @patch("vocalinux.main.logging") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_main_with_debug_enabled( self, @@ -525,7 +525,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") @patch("vocalinux.ui.first_run_dialog.show_first_run_dialog") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_main_first_run_later_keeps_prompt_enabled( self, @@ -581,7 +581,7 @@ class TestMainModule(unittest.TestCase): @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") @patch("vocalinux.ui.first_run_dialog.show_first_run_dialog") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_main_start_minimized_skips_first_run_prompt( self, @@ -824,7 +824,7 @@ class TestMainConfigPrecedence(unittest.TestCase): @patch("vocalinux.speech_recognition.recognition_manager.SpeechRecognitionManager") @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_cli_args_override_config( self, @@ -889,7 +889,7 @@ class TestMainConfigPrecedence(unittest.TestCase): @patch("vocalinux.speech_recognition.recognition_manager.SpeechRecognitionManager") @patch("vocalinux.text_injection.text_injector.TextInjector") @patch("vocalinux.ui.tray_indicator.TrayIndicator") - @patch("vocalinux.ui.config_manager.ConfigManager") + @patch("vocalinux.ui.config_manager.get_shared_config_manager") @patch("vocalinux.ui.logging_manager.initialize_logging") def test_config_used_when_no_cli_args( self, @@ -1151,7 +1151,9 @@ class TestMainCallbackTrailingSpaceEdges(unittest.TestCase): stack = ExitStack() stack.enter_context(patch("vocalinux.main.check_dependencies", return_value=True)) - mock_config_cls = stack.enter_context(patch("vocalinux.ui.config_manager.ConfigManager")) + mock_config_cls = stack.enter_context( + patch("vocalinux.ui.config_manager.get_shared_config_manager") + ) mock_config = MagicMock() mock_config.get_settings.return_value = { "speech_recognition": {}, @@ -1195,9 +1197,15 @@ class TestMainCallbackTrailingSpaceEdges(unittest.TestCase): mock_args.wayland = False mock_args.start_minimized = False mock_parse.return_value = mock_args - main() + try: + main() + text_cb = mock_speech.register_text_callback.call_args.args[0] + except BaseException: + # The caller only closes the stack once it gets one back, so a + # failure here would leak these patches into every later test. + stack.close() + raise - text_cb = mock_speech.register_text_callback.call_args.args[0] return stack, text_cb, mock_text def test_whitespace_only_is_skipped_through_main(self): diff --git a/tests/test_main_args_deps.py b/tests/test_main_args_deps.py index 99ff9f2..95012b8 100644 --- a/tests/test_main_args_deps.py +++ b/tests/test_main_args_deps.py @@ -298,10 +298,10 @@ class TestMainFunction(unittest.TestCase): """Test main() when another instance is already running.""" from vocalinux.main import main - mock_single_instance = MagicMock() - mock_single_instance.acquire_lock.return_value = False - - with patch.dict(sys.modules, {"vocalinux.single_instance": mock_single_instance}): + # Patch the function itself: main() binds the module via the package + # attribute, so a patch.dict(sys.modules, ...) entry is ignored whenever + # an earlier test already imported the real vocalinux.single_instance. + with patch("vocalinux.single_instance.acquire_lock", return_value=False): with pytest.raises(SystemExit) as exc_info: main() assert exc_info.value.code == 1 diff --git a/tests/test_main_edge_cases.py b/tests/test_main_edge_cases.py index 26402b7..70f56ee 100644 --- a/tests/test_main_edge_cases.py +++ b/tests/test_main_edge_cases.py @@ -84,10 +84,10 @@ class TestMainFunctionExtra: """Test that notification is attempted when another instance is running.""" from vocalinux.main import main - mock_single_instance = MagicMock() - mock_single_instance.acquire_lock.return_value = False - - with patch.dict(sys.modules, {"vocalinux.single_instance": mock_single_instance}): + # Patch the function itself: main() binds the module via the package + # attribute, so a patch.dict(sys.modules, ...) entry is ignored whenever + # an earlier test already imported the real vocalinux.single_instance. + with patch("vocalinux.single_instance.acquire_lock", return_value=False): with pytest.raises(SystemExit) as exc_info: main() assert exc_info.value.code == 1 diff --git a/tests/test_shared_config_manager.py b/tests/test_shared_config_manager.py new file mode 100644 index 0000000..e854bfb --- /dev/null +++ b/tests/test_shared_config_manager.py @@ -0,0 +1,98 @@ +"""Tests for the process-wide shared ConfigManager.""" + +import json +import os + +import pytest + +from vocalinux.ui import config_manager as cm + + +@pytest.fixture +def isolated_config(tmp_path, monkeypatch): + """Point the config file at a temp dir and reset the shared instance.""" + monkeypatch.setattr(cm, "CONFIG_DIR", str(tmp_path)) + monkeypatch.setattr(cm, "CONFIG_FILE", str(tmp_path / "config.json")) + monkeypatch.setattr(cm, "_shared_instance", None) + yield tmp_path + cm._shared_instance = None + + +def _seed_config(engine, model_size): + """Write a config file that both readers will load before any save.""" + with open(cm.CONFIG_FILE, "w") as handle: + json.dump({"speech_recognition": {"engine": engine, "model_size": model_size}}, handle) + + +def _read_config(): + with open(cm.CONFIG_FILE) as handle: + return json.load(handle) + + +def test_accessor_returns_one_instance(isolated_config): + assert cm.get_shared_config_manager() is cm.get_shared_config_manager() + + +def test_saves_through_the_accessor_do_not_revert_each_other(isolated_config): + """The failure mode of separate instances, run through the accessor. + + Both handles are taken before either save, the way two collaborators + (tray and audio feedback) grab one at startup. With two ConfigManagers + the second save would win with its stale cache and revert the first + one's engine; one shared instance keeps every earlier save in the file. + """ + _seed_config(engine="vosk", model_size="medium") + + first = cm.get_shared_config_manager() + second = cm.get_shared_config_manager() + assert first is second + + first.update_speech_recognition_settings({"engine": "whisper_cpp", "model_size": "small"}) + first.save_config() + + second.set("general", "autostart", True) + second.save_config() + + on_disk = _read_config() + assert on_disk["speech_recognition"]["engine"] == "whisper_cpp" + assert on_disk["speech_recognition"]["model_size"] == "small" + assert on_disk["general"]["autostart"] is True + + +def test_two_instances_still_revert_each_other(isolated_config): + """Characterization of the bug the accessor exists to prevent (#689). + + Constructing ConfigManager directly is still possible, and it still + loses writes. This is what the production call sites used to do, and + what a future one would reintroduce. + """ + _seed_config(engine="vosk", model_size="medium") + + first = cm.ConfigManager() + second = cm.ConfigManager() + + first.update_speech_recognition_settings({"engine": "whisper_cpp", "model_size": "small"}) + first.save_config() + + second.set("general", "autostart", True) + second.save_config() + + on_disk = _read_config() + assert on_disk["speech_recognition"]["engine"] == "vosk" + assert on_disk["speech_recognition"]["model_size"] == "medium" + + +def test_application_code_does_not_construct_its_own_instance(): + """Source guard: a direct ConfigManager() anywhere in src reintroduces the + stale-cache overwrite, so only the defining module may construct one.""" + src_root = os.path.join(os.path.dirname(__file__), "..", "src", "vocalinux") + offenders = [] + for root, _dirs, files in os.walk(src_root): + for name in files: + if not name.endswith(".py") or name == "config_manager.py": + continue + path = os.path.join(root, name) + with open(path, "r") as handle: + if "ConfigManager()" in handle.read(): + offenders.append(os.path.relpath(path, src_root)) + assert offenders == [] diff --git a/tests/test_tray_indicator.py b/tests/test_tray_indicator.py index 0196389..52ebde2 100644 --- a/tests/test_tray_indicator.py +++ b/tests/test_tray_indicator.py @@ -130,7 +130,7 @@ class TestTrayIndicator(unittest.TestCase): self.mock_makedirs = self.patcher_makedirs.start() # Patch ConfigManager constructor - self.patcher_config_manager = patch("vocalinux.ui.tray_indicator.ConfigManager") + self.patcher_config_manager = patch("vocalinux.ui.tray_indicator.get_shared_config_manager") self.mock_config_manager_class = self.patcher_config_manager.start() self.mock_config_manager_class.return_value = self.mock_config_manager diff --git a/tests/test_tray_indicator_ext.py b/tests/test_tray_indicator_ext.py index 8f37c27..cacde70 100644 --- a/tests/test_tray_indicator_ext.py +++ b/tests/test_tray_indicator_ext.py @@ -72,7 +72,7 @@ class TestTrayIndicatorInitialization(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -126,7 +126,7 @@ class TestTrayIndicatorInitialization(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -193,7 +193,7 @@ class TestTrayIndicatorMenuOperations(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -232,7 +232,7 @@ class TestTrayIndicatorMenuOperations(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -293,7 +293,7 @@ class TestTrayIndicatorStateHandling(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -332,7 +332,7 @@ class TestTrayIndicatorStateHandling(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -371,7 +371,7 @@ class TestTrayIndicatorStateHandling(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -432,7 +432,7 @@ class TestTrayIndicatorSignalHandling(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ): @@ -475,7 +475,7 @@ class TestTrayIndicatorSignalHandling(unittest.TestCase): with ( patch("vocalinux.ui.tray_indicator.logging"), patch("vocalinux.ui.tray_indicator._resource_manager") as mock_resource_manager, - patch("vocalinux.ui.tray_indicator.ConfigManager") as mock_config_manager, + patch("vocalinux.ui.tray_indicator.get_shared_config_manager") as mock_config_manager, patch("vocalinux.ui.tray_indicator.KeyboardShortcutManager") as mock_keyboard_manager, ):