Upgrade Flow Cleanup

synodic_client/application/screen/tool_update_controller.py

@@ -29,9 +29,7 @@
     update_all_tools,
     update_tool,
 )
-from synodic_client.resolution import (
-    resolve_update_config,
-)
+from synodic_client.resolution import resolve_update_config
 
 if TYPE_CHECKING:
     from synodic_client.application.config_store import ConfigStore

synodic_client/application/screen/tray.py

@@ -29,6 +29,8 @@
 class TrayScreen:
     """Tray screen for the application."""
 
+    _MENU_POPUP_DELAY_MS = 100
+
     def __init__(
         self,
         app: QApplication,
@@ -148,6 +150,13 @@ def _on_tray_activated(self, reason: QSystemTrayIcon.ActivationReason) -> None:
         logger.debug('Tray activated: reason=%s', reason.name)
         if reason == QSystemTrayIcon.ActivationReason.DoubleClick:
             self._show_window()
+        elif reason == QSystemTrayIcon.ActivationReason.Context:
+            QTimer.singleShot(self._MENU_POPUP_DELAY_MS, self._show_tray_menu)
+
+    def _show_tray_menu(self) -> None:
+        """Show the tray context menu at the current cursor position."""
+        geo = self.tray.geometry()
+        self._menu.popup(geo.center())
 
     def _show_window(self) -> None:
         """Show, raise, and focus the main window."""

synodic_client/application/update_controller.py

@@ -31,10 +31,8 @@
 from synodic_client.application.update_model import UpdateModel
 from synodic_client.operations.schema import UpdateCheckResult
 from synodic_client.operations.update import apply_self_update, check_self_update, download_self_update
-from synodic_client.resolution import (
-    ResolvedConfig,
-    resolve_update_config,
-)
+from synodic_client.resolution import resolve_update_config
+from synodic_client.schema import ResolvedConfig, UpdateState
 from synodic_client.startup import sync_startup
 
 if TYPE_CHECKING:
@@ -93,7 +91,7 @@ def __init__(
 
         # Track update-relevant config fields to avoid reinitialising
         # on every config save (e.g. timestamp-only changes).
-        self._update_config_key = self._extract_update_key(store.config)
+        self._update_config_key = resolve_update_config(store.config)
 
         # Periodic auto-update timer
         self._auto_update_timer: QTimer | None = None
@@ -225,7 +223,7 @@ def _on_config_changed(self, config: object) -> None:
             return
         self._auto_apply = config.auto_apply
 
-        new_key = self._extract_update_key(config)
+        new_key = resolve_update_config(config)
         if new_key == self._update_config_key:
             return
         self._update_config_key = new_key
@@ -237,16 +235,6 @@ def _on_config_changed(self, config: object) -> None:
     # Updater re-initialisation
     # ------------------------------------------------------------------
 
-    @staticmethod
-    def _extract_update_key(config: ResolvedConfig) -> tuple[object, ...]:
-        """Return a hashable tuple of the fields that affect the updater."""
-        return (
-            config.update_source,
-            config.update_channel,
-            config.auto_update_interval_minutes,
-            config.auto_apply,
-        )
-
     def _reinitialize_updater(self, config: ResolvedConfig) -> None:
         """Re-derive update settings and restart the updater and timer.
 
@@ -289,6 +277,15 @@ def _do_check(self, *, silent: bool) -> None:
                 self._model.set_error('Updater is not initialized.')
             return
 
+        # A download may still be running in a thread-pool executor even
+        # after the asyncio Task wrapper has been cancelled.  Starting a
+        # new check would trigger a second concurrent download for the
+        # same package, leading to a file-rename race on Windows.
+        if self._client.updater.state == UpdateState.DOWNLOADING:
+            logger.debug('Skipping update check — download already in progress')
+            self._model.set_check_button_enabled(True)
+            return
+
         # Always disable the button; only show "Checking…" when no
         # download is already pending (to preserve the ready state).
         self._model.set_check_button_enabled(False)

synodic_client/config.py

@@ -122,11 +122,16 @@ def load_user_config() -> UserConfig:
 
     try:
         data = json.loads(path.read_text(encoding='utf-8'))
+    except json.JSONDecodeError, OSError:
+        logger.exception('Failed to read config from %s, using defaults', path)
+        return UserConfig()
+
+    try:
         config = UserConfig.model_validate(data)
         logger.debug('Loaded user config from %s', path)
         return config
     except Exception:
-        logger.exception('Failed to load user config from %s, using defaults', path)
+        logger.exception('Failed to validate user config from %s, using defaults', path)
         return UserConfig()
 
 

synodic_client/resolution.py

@@ -25,9 +25,7 @@
 from synodic_client.schema import (
     DEFAULT_AUTO_UPDATE_INTERVAL_MINUTES,
     DEFAULT_TOOL_UPDATE_INTERVAL_MINUTES,
-    GITHUB_REPO_URL,
     ResolvedConfig,
-    UpdateChannel,
     UpdateConfig,
     UserConfig,
 )
@@ -160,19 +158,16 @@ def update_user_config(**changes: object) -> ResolvedConfig:
 
 
 def resolve_update_config(config: ResolvedConfig) -> UpdateConfig:
-    """Derive an ``UpdateConfig`` from resolved configuration values.
+    """Derive an :class:`UpdateConfig` from resolved configuration values.
+
+    Delegates to :meth:`UpdateConfig.from_resolved` so the type owns
+    its own construction while this module stays the canonical entry
+    point for all resolution logic.
 
     Args:
         config: A resolved configuration snapshot.
 
     Returns:
         An ``UpdateConfig`` ready to initialise the updater.
     """
-    channel = UpdateChannel.DEVELOPMENT if config.update_channel == 'dev' else UpdateChannel.STABLE
-
-    return UpdateConfig(
-        channel=channel,
-        repo_url=config.update_source or GITHUB_REPO_URL,
-        auto_update_interval_minutes=config.auto_update_interval_minutes,
-        tool_update_interval_minutes=config.tool_update_interval_minutes,
-    )
+    return UpdateConfig.from_resolved(config)

synodic_client/schema.py

@@ -8,13 +8,16 @@
 
 from __future__ import annotations
 
+import logging
 import sys
 from dataclasses import dataclass, field
 from enum import Enum, StrEnum, auto
 from typing import Any
 
 from packaging.version import Version
-from pydantic import BaseModel
+from pydantic import BaseModel, ValidationError, model_validator
+
+logger = logging.getLogger(__name__)
 
 # ---------------------------------------------------------------------------
 # BuildConfig — read-only, lives next to the executable
@@ -109,6 +112,31 @@ class UserConfig(BaseModel):
     # tool updates have been recorded.
     last_tool_updates: dict[str, str] | None = None
 
+    @model_validator(mode='wrap')
+    @classmethod
+    def _recover_invalid_fields(cls, data: Any, handler: Any) -> UserConfig:
+        """Silently drop fields that fail validation instead of rejecting the entire config.
+
+        When an on-disk ``config.json`` contains a value whose type no longer
+        matches the schema (e.g. after a version upgrade renames or re-types a
+        field), the normal behaviour is to raise ``ValidationError`` and lose
+        *every* setting.  This wrap validator intercepts the error, removes
+        only the offending fields (so their defaults kick in), and retries.
+        """
+        try:
+            return handler(data)
+        except ValidationError as exc:
+            if not isinstance(data, dict):
+                raise
+
+            bad_fields = {str(e['loc'][0]) for e in exc.errors() if e.get('loc')}
+            cleaned = {k: v for k, v in data.items() if k not in bad_fields}
+
+            for name in bad_fields:
+                logger.warning('Discarding invalid config field %r (using default)', name)
+
+            return handler(cleaned)
+
 
 # ---------------------------------------------------------------------------
 # Update channel & state enums
@@ -201,6 +229,24 @@ class UpdateConfig:
     # Interval in minutes between tool update checks (0 = disabled)
     tool_update_interval_minutes: int = DEFAULT_TOOL_UPDATE_INTERVAL_MINUTES
 
+    @classmethod
+    def from_resolved(cls, config: ResolvedConfig) -> UpdateConfig:
+        """Derive an ``UpdateConfig`` from resolved configuration values.
+
+        Args:
+            config: A resolved configuration snapshot.
+
+        Returns:
+            An ``UpdateConfig`` ready to initialise the updater.
+        """
+        channel = UpdateChannel.DEVELOPMENT if config.update_channel == 'dev' else UpdateChannel.STABLE
+        return cls(
+            channel=channel,
+            repo_url=config.update_source or GITHUB_REPO_URL,
+            auto_update_interval_minutes=config.auto_update_interval_minutes,
+            tool_update_interval_minutes=config.tool_update_interval_minutes,
+        )
+
     @property
     def channel_name(self) -> str:
         """Get the channel name for Velopack.

synodic_client/updater.py

@@ -208,7 +208,12 @@ def check_for_update(self) -> UpdateInfo:
                 # moved past it.  A periodic re-check that discovers the
                 # same release must not regress DOWNLOADED → UPDATE_AVAILABLE,
                 # which would cause apply_update_on_exit() to reject the update.
-                if self._state not in {UpdateState.DOWNLOADED, UpdateState.APPLYING, UpdateState.APPLIED}:
+                if self._state not in {
+                    UpdateState.DOWNLOADING,
+                    UpdateState.DOWNLOADED,
+                    UpdateState.APPLYING,
+                    UpdateState.APPLIED,
+                }:
                     self._state = UpdateState.UPDATE_AVAILABLE
                 logger.info('Update available: %s -> %s', self._current_version, latest)
             else:
@@ -379,20 +384,16 @@ def _download_direct(
         # Verify checksum — prefer SHA256, fall back to SHA1.
         if asset.SHA256:
             actual = sha256_hash.hexdigest()
-            if not actual.lower() == asset.SHA256.lower():
+            if actual.lower() != asset.SHA256.lower():
                 partial_file.unlink(missing_ok=True)
-                raise RuntimeError(
-                    f'SHA256 mismatch for {asset.FileName}: expected {asset.SHA256}, got {actual}'
-                )
+                raise RuntimeError(f'SHA256 mismatch for {asset.FileName}: expected {asset.SHA256}, got {actual}')
         elif asset.SHA1:
             actual_sha1 = hashlib.sha1(partial_file.read_bytes()).hexdigest()  # noqa: S324 — verifying known digest
-            if not actual_sha1.lower() == asset.SHA1.lower():
+            if actual_sha1.lower() != asset.SHA1.lower():
                 partial_file.unlink(missing_ok=True)
-                raise RuntimeError(
-                    f'SHA1 mismatch for {asset.FileName}: expected {asset.SHA1}, got {actual_sha1}'
-                )
+                raise RuntimeError(f'SHA1 mismatch for {asset.FileName}: expected {asset.SHA1}, got {actual_sha1}')
 
-        partial_file.rename(target_file)
+        partial_file.replace(target_file)
         logger.info('Direct download complete: %s', target_file)
 
     def download_update(self, progress_callback: Callable[[int], None] | None = None) -> bool:
@@ -545,7 +546,7 @@ def _get_velopack_manager(self) -> Any:
             raise RuntimeError(f'Failed to create Velopack UpdateManager: {e}') from e
 
 
-def _on_before_uninstall(version: str) -> None:
+def on_before_uninstall(version: str) -> None:
     """Velopack hook: called before the app is uninstalled.
 
     Removes the ``synodic://`` URI protocol handler and auto-startup

tests/unit/operations/test_install.py

@@ -61,6 +61,7 @@ def _make_action(
         action.package.constraint = constraint
     else:
         action.package = None
+    action.distro = None
     return action
 
 

tests/unit/qt/conftest.py

@@ -97,6 +97,7 @@ def make_action(
     action.command = overrides.get('command')
     action.include_prereleases = overrides.get('include_prereleases', False)
     action.plugin_target = overrides.get('plugin_target')
+    action.distro = overrides.get('distro')
     return action
 
 

tests/unit/qt/test_update_controller.py

@@ -4,6 +4,8 @@
 
 from unittest.mock import MagicMock, patch
 
+import pytest
+
 from synodic_client.application.screen.update_banner import UpdateBanner
 from synodic_client.application.theme import (
     UPDATE_STATUS_AVAILABLE_STYLE,
@@ -13,6 +15,7 @@
 from synodic_client.application.update_controller import UpdateController
 from synodic_client.application.update_model import UpdateModel
 from synodic_client.operations.schema import UpdateCheckResult
+from synodic_client.schema import UpdateState
 
 from .conftest import make_config_store, make_resolved_config
 
@@ -576,3 +579,73 @@ def test_apply_without_pending_silent_logs_only() -> None:
         assert banner.state.name == 'HIDDEN'
         client.apply_update_on_exit.assert_not_called()
         app.quit.assert_not_called()
+
+
+# ---------------------------------------------------------------------------
+# DOWNLOADING guard in _do_check
+# ---------------------------------------------------------------------------
+
+
+class TestDoCheckDownloadingGuard:
+    """Verify _do_check skips checks when a download is in progress."""
+
+    @staticmethod
+    @pytest.mark.parametrize('silent', [False, True], ids=['manual', 'silent'])
+    def test_check_skipped_when_downloading(silent: bool) -> None:
+        """_do_check should return early without creating a task when DOWNLOADING."""
+        ctrl, _app, client, banner, model = _make_controller()
+        spy = ModelSpy(model)
+        client.updater.state = UpdateState.DOWNLOADING
+
+        with patch.object(ctrl, '_set_task') as mock_set_task:
+            ctrl._do_check(silent=silent)
+
+        mock_set_task.assert_not_called()
+        # Button should be re-enabled
+        assert True in spy.check_button_enabled
+
+    @staticmethod
+    def test_check_proceeds_when_not_downloading() -> None:
+        """_do_check should proceed normally when updater is not DOWNLOADING."""
+        ctrl, _app, client, banner, model = _make_controller()
+        client.updater.state = UpdateState.NO_UPDATE
+
+        with patch.object(ctrl, '_set_task') as mock_set_task:
+            ctrl._do_check(silent=False)
+
+        mock_set_task.assert_called_once()
+
+
+# ---------------------------------------------------------------------------
+# request_retry / request_apply
+# ---------------------------------------------------------------------------
+
+
+class TestRequestRetry:
+    """Verify request_retry clears the failed version and re-checks."""
+
+    @staticmethod
+    def test_clears_failed_version_and_checks() -> None:
+        """request_retry should clear _failed_version then trigger a check."""
+        ctrl, _app, _client, banner, model = _make_controller()
+        ctrl._failed_version = '2.0.0'
+
+        with patch.object(ctrl, 'check_now') as mock_check:
+            ctrl.request_retry()
+
+        assert ctrl._failed_version is None
+        mock_check.assert_called_once_with(silent=True)
+
+
+class TestRequestApply:
+    """Verify request_apply delegates to _apply_update."""
+
+    @staticmethod
+    def test_delegates_to_apply_update() -> None:
+        """request_apply should call _apply_update(silent=False)."""
+        ctrl, _app, _client, banner, model = _make_controller()
+
+        with patch.object(ctrl, '_apply_update') as mock_apply:
+            ctrl.request_apply()
+
+        mock_apply.assert_called_once_with(silent=False)

tests/unit/qt/test_update_feedback.py

@@ -512,9 +512,9 @@ def test_header_auto_update_toggled_emits_composite() -> None:
         header.set_runtime('3.11')
         spy = MagicMock()
         header.auto_update_toggled.connect(spy)
-        # Find the Auto button and click it
+        # Find the auto-update toggle button (checkable ↺ button)
         for child in header.findChildren(QPushButton):
-            if child.text() == 'Auto':
+            if child.isCheckable():
                 child.click()
                 break
         spy.assert_called_once()