diff --git a/CHANGELOG.md b/CHANGELOG.md index afb384a..9535e6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,40 @@ project uses [semantic versioning][semver]. ### Added +- **The desktop app checks for updates, and declines while a job is running.** + One check a couple of seconds after the window opens, saying nothing unless + there is something to say, with the release named in the header and + **Help, Check for updates now** for an answer either way. **Options, Check + for updates on launch** turns the automatic check off. The check and the + signature probe run on a pool thread, so an unreachable network delays + nothing. + + **Three outcomes, not two.** A check that could not be made is reported as a + failure rather than as "you are the newest release": the never-raising form + of the check returns nothing for a failed fetch, a TLS error and an + unparseable feed alike, and saying you are up to date on the strength of a + failed lookup is a claim. Whose result it is belongs to the check that is + running, so a manual check started during the first couple of seconds + survives the launch timer firing behind it instead of being answered + silently. + + Installing follows the installer's refusal rather than working around it: a + running *or paused* job declines the update with the reason, since a paused + job is a partially written destination waiting to continue. Otherwise the + installer is downloaded and verified, the queue is checked a second time + because a job can start while the bytes arrive, and the app then closes + itself so maintenance can proceed. That close is what makes the update + possible, so it is announced rather than surprising. Progress is throttled + to roughly one signal per 256 KB but always reports the final block, so the + bar finishes instead of stopping just short. + + **Cancel cancels.** The download is a blocking read loop on a pool thread and + cannot be interrupted, so cancellation is cooperative through the progress + callback — but it is terminal: the incomplete download is removed, nothing is + verified, and the app cannot go on to offer what the user just declined. Only + the file the release names is removed, since the download directory can be + one the caller owns. + - **A tag-triggered release candidate workflow.** Pushing `v*` gates the tag against `src/offloader/_version.py` before spending a packaging run on it, builds the unsigned bundle and installer on `windows-latest`, confirms every diff --git a/docs/release-plan.md b/docs/release-plan.md index d48859f..7a1b0e3 100644 --- a/docs/release-plan.md +++ b/docs/release-plan.md @@ -57,7 +57,7 @@ tests or builds were run for this documentation task. | Dependencies | Minimum versions and optional extras | Recorded build environment and pinned release dependency sets | | Media tools | ffmpeg/ffprobe discovered externally; copying works without them | Explicit installer dependency policy and useful missing-tool messaging | | Integrity | Detailed guarantees and remaining limits in `data-safety.md` | Release-specific regression evidence and operational validation | -| Updates | `offloader update` checks GitHub Releases, verifies the signed installer and hands it over; no in-app check or notification | Wire the check into the desktop interface; qualify an end-to-end update against a published release | +| Updates | `offloader update` and the desktop app both check GitHub Releases, verify the signed installer and hand it over, declining while a job is active | Qualify an end-to-end update against a published signed release on a clean machine | Sources: [`pyproject.toml`](../pyproject.toml), [`CI`](../.github/workflows/ci.yml), [`README`](../README.md), @@ -207,15 +207,19 @@ an arbitrary install directory that might contain user material. user-context relaunch, and safe settings preservation. A generic unattended deployment must not launch an app in a missing or unrelated user's session. -The command-line half of the update client is now implemented in -`src/offloader/update.py` and documented in [updates.md](updates.md). It meets -the conditions this section set: the signature, publisher and embedded version -are all verified before elevation, and the `/D=` target is computed from the -running executable rather than from the uninstall registry key. It does not -close a running instance, so an update requires the operator to finish first -and the installer's refusal remains the guarantee. What is still deferred is -the in-app part: an automatic check, a notification, and a progress surface in -the desktop interface. +The update client is now implemented in `src/offloader/update.py`, wrapped for +the desktop app in `src/offloader/gui/updates.py`, and documented in +[updates.md](updates.md). It meets the conditions this section set: the +signature, publisher and embedded version are all verified before elevation, +and the `/D=` target is computed from the running executable rather than from +the uninstall registry key. + +The app never replaces itself under an active transfer. A running or paused job +declines the update with a reason, the queue is rechecked immediately before +the installer is launched, and the app then closes itself deliberately so +maintenance can proceed. What remains is qualification rather than +implementation: an end-to-end update from one signed published release to the +next, on a clean machine, which needs a signed release to exist first. **Acceptance gates:** exercise first install, custom path, same-version reinstall, upgrade, failed upgrade recovery, silent install, and uninstall on diff --git a/docs/updates.md b/docs/updates.md index ac0aeff..d5edb9a 100644 --- a/docs/updates.md +++ b/docs/updates.md @@ -93,9 +93,52 @@ place where the installer's own transactional recovery manages it (see `installation.py`); the updater does not attempt a second recovery mechanism on top. Nothing is deleted by the updater itself. -**There is no automatic check yet.** `offloader update` is explicit. An in-app -check, a notification and a timer belong with the desktop interface and are -not implemented here; see [release-plan.md](release-plan.md). +## In the desktop app + +The app checks once, a couple of seconds after the window opens, and says +nothing unless there is something to say: an unrequested check that reports +"you are up to date" is noise. When a release is found, the header carries a +line naming it. **Help, Check for updates now** runs the same check and does +report either outcome, and **Options, Check for updates on launch** turns the +automatic one off. The check runs on a pool thread, so a slow or unreachable +network delays nothing and a failure appears in the status bar rather than in +a dialog. + +Three outcomes, not two. `update.check()` never raises and returns `None` for +a failed fetch, a TLS error and an unparseable feed alike, so a caller that +reports its result to somebody cannot use it: "you are the newest release" is +a claim, and it would be made on the strength of a failed DNS lookup. The app +uses `check_feed()`, which returns `None` only when the feed answered and had +nothing newer, and raises `FeedError` otherwise. A check that could not be +made is reported as a failure. + +Whose result it is belongs to the check that is running, not to whoever asked +last. A manual check started inside the first couple of seconds is still +running when the launch timer fires; the timer's check is refused as a +duplicate, and the announcement the manual request asked for survives it. The +intent is only ever raised while work is in flight, so a manual request behind +an automatic check is answered too. + +**Cancel cancels.** The download runs on a pool thread and cannot be +interrupted, so cancellation is cooperative: the progress callback is the one +place the loop hands control back often enough, and it raises there. The +request is then terminal — the incomplete download is removed, nothing is +verified, and `ready` is not emitted, so the app cannot go on to offer what +the user just declined. Only the file named by the release is removed, because +the download directory can be one the caller owns. + +Installing from the app follows the refusal above rather than working around +it. If any job is running or paused, the update is declined with the reason; +a paused job counts, because it is a partially written destination waiting to +continue. Otherwise the installer is downloaded, verified, and the app asks +once more before closing itself so the installer can proceed. The app closing +is what makes the update possible, so it is announced rather than surprising, +and the queue is checked a second time immediately before it happens: a job +can be started while the bytes are arriving. + +`src/offloader/gui/updates.py` holds that sequence, with every step +injectable, so `tests/test_gui_updates.py` drives all of it, including each +refusal, without a network, a certificate or an installer. ## Reference diff --git a/src/offloader/gui/main_window.py b/src/offloader/gui/main_window.py index de8ad90..021a667 100644 --- a/src/offloader/gui/main_window.py +++ b/src/offloader/gui/main_window.py @@ -4,13 +4,14 @@ from pathlib import Path -from PySide6.QtCore import Qt +from PySide6.QtCore import Qt, QTimer from PySide6.QtGui import QAction, QKeySequence from PySide6.QtWidgets import ( QApplication, QButtonGroup, QMainWindow, QMessageBox, + QProgressDialog, QSplitter, QStackedWidget, QVBoxLayout, @@ -26,6 +27,7 @@ from .preset_mode import PresetModePanel from .queue_view import QueuePanel, reveal from .simple_mode import SimpleModePanel +from .updates import UpdateController, refusal from .widgets import button, label, row from .worker import JobState, QueueController @@ -34,6 +36,10 @@ "sound_on_completion": True, "warn_on_duplicate": True, "mode": "preset", + # On by default, and one network request: a packaged copy that never + # learns a fix exists is the worse default for a tool people trust with + # original media. + "check_for_updates": True, } @@ -69,6 +75,18 @@ def __init__(self) -> None: self.queue = QueuePanel(self.controller) + self._update_progress: QProgressDialog | None = None + self._update_banner = label("", "muted") + self._update_banner.setStyleSheet(f"color: {theme.ACCENT};") + self._update_banner.setVisible(False) + self.updates = UpdateController(self) + self.updates.found.connect(self._on_update_available) + self.updates.upToDate.connect(self._on_update_absent) + self.updates.progress.connect(self._on_update_progress) + self.updates.ready.connect(self._on_update_ready) + self.updates.failed.connect(self._on_update_failed) + self.updates.cancelled.connect(self._on_update_cancelled) + # ----------------------------------------------------------- chrome self._preset_button = button("Presets") self._simple_button = button("Simple") @@ -87,6 +105,9 @@ def __init__(self) -> None: self._preset_button, self._simple_button, None, + # Right of the stretch, so an available update is visible without + # taking a dialog's worth of attention from whatever is running. + self._update_banner, ) left = QWidget() @@ -123,6 +144,13 @@ def __init__(self) -> None: self._set_mode(1 if self.settings.get("mode") == "simple" else 0) self.drives.start() + if self.settings.get("check_for_updates", True): + # Deferred rather than run here: the first paint should not wait on + # a network round trip, and a failure must not stop the window + # opening. + QTimer.singleShot(2500, + lambda: self._check_for_updates(announce=False)) + # ---------------------------------------------------------------- chrome def _build_menu(self) -> None: job_menu = self.menuBar().addMenu("&Job") @@ -166,11 +194,144 @@ def _build_menu(self) -> None: open_config.triggered.connect(lambda: reveal(config_file(SETTINGS_FILE))) options_menu.addAction(open_config) + self._update_action = QAction("Check for updates on launch", self, + checkable=True) + self._update_action.setChecked(bool(self.settings["check_for_updates"])) + self._update_action.toggled.connect( + lambda on: self._save_setting("check_for_updates", on)) + options_menu.addAction(self._update_action) + help_menu = self.menuBar().addMenu("&Help") + self._check_updates = QAction("Check for updates now", self) + self._check_updates.triggered.connect( + lambda: self._check_for_updates(announce=True)) + help_menu.addAction(self._check_updates) + help_menu.addSeparator() about = QAction("About", self) about.triggered.connect(self._show_about) help_menu.addAction(about) + # ---------------------------------------------------------------- updates + def _check_for_updates(self, *, announce: bool) -> None: + """`announce` reports "you are up to date"; the automatic check does + not, because an unasked-for check should only ever speak up when there + is something to say. + + The intent belongs to the controller, with the check it started. This + used to be stamped here before `check_now` reported whether a check + actually began, so a manual check requested inside the first 2.5 + seconds was downgraded by the launch timer firing behind it: the + duplicate was refused, the manual intent was already overwritten, and + the result the user asked for was handled silently. + """ + if not self.updates.check_now(announce=announce) and announce: + self.statusBar().showMessage( + "An update check is already running; its result will be " + "shown when it finishes.", 5000) + + def _on_update_available(self, release) -> None: + self._update_banner.setText( + f"Offloader {release.version} is available. " + f"Help → Check for updates now to install it.") + self._update_banner.setVisible(True) + if not self.updates.announce: + return + refused = refusal( + [item.state for item in self.controller.items]) + if refused is not None: + QMessageBox.information(self, "Update available", refused) + return + notes = (release.notes or "").strip() + detail = f"\n\n{notes[:800]}" if notes else "" + answer = QMessageBox.question( + self, "Update available", + f"Offloader {release.version} is available; this copy is " + f"{__version__}.\n\nIt will be downloaded and its signature " + f"checked before anything runs. Offloader then has to close for " + f"the installer to replace it.{detail}\n\nDownload it now?", + QMessageBox.Yes | QMessageBox.No, QMessageBox.Yes) + if answer != QMessageBox.Yes: + return + self._update_progress = QProgressDialog( + f"Downloading Offloader {release.version}…", "Cancel", 0, 100, + self) + self._update_progress.setWindowTitle("Update") + self._update_progress.setWindowModality(Qt.WindowModal) + self._update_progress.setAutoClose(False) + # The button offers to stop the download, so it has to stop it. Hiding + # the dialog while the bytes kept arriving, and then offering to + # install what the user had just declined, is worse than no button. + self._update_progress.canceled.connect(self.updates.cancel) + self._update_progress.show() + self.updates.prepare() + + def _on_update_progress(self, done: int, total: int) -> None: + if self._update_progress is None: + return + if total: + self._update_progress.setMaximum(100) + self._update_progress.setValue(min(100, done * 100 // total)) + else: + self._update_progress.setMaximum(0) + + def _on_update_ready(self, release, digest: str) -> None: + self._close_update_progress() + # Checked again here, not only before the download: a job can be + # started while the bytes are arriving, and this is the last moment + # before the installer is asked to replace a running application. + refused = refusal( + [item.state for item in self.controller.items]) + if refused is not None: + QMessageBox.information(self, "Update ready", refused) + return + answer = QMessageBox.question( + self, "Install update", + f"Offloader {release.version} is verified and ready.\n\n" + f"SHA-256 {digest[:16]}…\n\nOffloader will close so the " + f"installer can replace it. Install now?", + QMessageBox.Yes | QMessageBox.No, QMessageBox.Yes) + if answer != QMessageBox.Yes: + return + try: + self.updates.install() + except Exception as exc: + QMessageBox.warning(self, "Update", f"Could not start the " + f"installer: {exc}") + return + # Closing is what lets the installer proceed, so it is done only after + # the elevated process has actually started. + self.close() + + def _on_update_failed(self, message: str) -> None: + self._close_update_progress() + if self.updates.announce: + QMessageBox.warning(self, "Update", message) + else: + self.statusBar().showMessage(f"Update check: {message}", 8000) + + def _on_update_cancelled(self) -> None: + self._close_update_progress() + self.statusBar().showMessage("Update download cancelled.", 5000) + + def _close_update_progress(self) -> None: + if self._update_progress is None: + return + # Disconnected first: closing a QProgressDialog emits `canceled`, and + # a cancel raised while tearing down the dialog for a result that has + # already arrived would be reported as the user asking for one. + try: + self._update_progress.canceled.disconnect(self.updates.cancel) + except (RuntimeError, TypeError): + pass + self._update_progress.close() + self._update_progress = None + + def _on_update_absent(self) -> None: + if self.updates.announce: + QMessageBox.information( + self, "Up to date", + f"Offloader {__version__} is the newest release available.") + def _save_setting(self, key: str, value) -> None: self.settings[key] = value write_json(config_file(SETTINGS_FILE), self.settings) diff --git a/src/offloader/gui/updates.py b/src/offloader/gui/updates.py new file mode 100644 index 0000000..6a2c49e --- /dev/null +++ b/src/offloader/gui/updates.py @@ -0,0 +1,325 @@ +"""The desktop app's half of updating: check, download, verify, hand over. + +`offloader.update` does the work and knows nothing about Qt. This wraps it so +the network and the signature probe happen off the GUI thread, and so the +sequence has somewhere to live that is not the window. + +The sequence is not the obvious one, because Offloader's installer refuses +maintenance while a transfer is in flight and never force-kills a running copy. +That rules out the usual desktop pattern of replacing the app underneath +itself. So: + +* a job in the queue means the update is declined, with a reason, rather than + queued behind it or forced through; +* when nothing is running, the installer is launched and the app closes itself + so the installer can proceed. The app closing is the thing that makes the + update possible, which is why it is deliberate and announced rather than a + side effect. + +Every step is injectable. A test should be able to drive the whole sequence, +including the refusals, without a network, a certificate, or an installer. +""" + +from __future__ import annotations + +import tempfile +from collections.abc import Callable +from pathlib import Path + +from PySide6.QtCore import QObject, QRunnable, Qt, QThreadPool, Signal + +from .._version import __version__ +from ..update import UpdateError +from ..update import apply as apply_release +from ..update import check_feed as check_release +from ..update import download as download_release +from ..update import verify as verify_release + +#: How often an automatic check may run, in milliseconds. Long, because a +#: release appears a few times a year and the check is a network round trip +#: nobody asked for. +CHECK_INTERVAL_MS = 6 * 60 * 60 * 1000 + +#: Bytes between progress signals. A 200 MB download at chunk granularity +#: would emit thousands of times and the only visible effect is a busier event +#: loop than the copy engine's. +PROGRESS_STEP = 1 << 18 + + +class _Cancelled(Exception): + """Raised out of the progress callback to stop a download in flight. + + Cooperative because there is nothing to interrupt: the download is a + blocking read loop on a pool thread, and the only place it hands control + back often enough to be stopped is the progress callback. + """ + + +class _Signals(QObject): + """Owned by the controller, not the runnable. + + A QRunnable's Python wrapper can be collected as soon as `start()` returns, + so signals belonging to it would be destroyed under the running thread. + The same reason `drives._ScanTask` keeps them separate. + """ + + found = Signal(object) + upToDate = Signal() + progress = Signal(int, int) + ready = Signal(object, str) + failed = Signal(str) + cancelled = Signal() + + +class _Task(QRunnable): + def __init__(self, work: Callable[[], None]) -> None: + super().__init__() + self._work = work + + def run(self) -> None: # noqa: D102 - Qt entry point + self._work() + + +class UpdateController(QObject): + """Checks for a release, and prepares one for installation. + + Signals carry versions and byte counts, never the download URL: the window + has no use for it, and a URL that never reaches the interface cannot be + rendered into one. + """ + + found = Signal(object) + upToDate = Signal() + progress = Signal(int, int) + ready = Signal(object, str) + failed = Signal(str) + cancelled = Signal() + + def __init__(self, parent: QObject | None = None, *, + check: Callable[..., object] | None = None, + download: Callable[..., tuple[Path, str]] | None = None, + verify: Callable[..., object] | None = None, + apply: Callable[..., None] | None = None, + installed: str = __version__) -> None: + super().__init__(parent) + self._check = check or check_release + self._download = download or download_release + self._verify = verify or verify_release + self._apply = apply or apply_release + self._installed = installed + self._busy = False + self._announce = False + self._cancelled = False + self._release: object | None = None + self._installer: Path | None = None + + self._signals = _Signals(self) + self._signals.found.connect(self.found, Qt.QueuedConnection) + self._signals.upToDate.connect(self.upToDate, Qt.QueuedConnection) + self._signals.progress.connect(self.progress, Qt.QueuedConnection) + self._signals.ready.connect(self.ready, Qt.QueuedConnection) + self._signals.failed.connect(self.failed, Qt.QueuedConnection) + self._signals.cancelled.connect(self.cancelled, Qt.QueuedConnection) + + # ------------------------------------------------------------- inspection + @property + def busy(self) -> bool: + """Whether a check or a download is already running.""" + return self._busy + + @property + def release(self) -> object | None: + """The release last found, if any.""" + return self._release + + @property + def installer(self) -> Path | None: + """The verified installer, once `prepare` has succeeded.""" + return self._installer + + @property + def announce(self) -> bool: + """Whether the work in flight was explicitly asked for. + + Held here rather than by the window because it belongs to the check + that is running, not to the last call that wanted one. A window that + stamped its own flag before calling `check_now` lost a manual request + to the launch timer firing behind it: the timer's silent intent + overwrote it, the duplicate check was refused, and the result the user + had asked for was then handled as an automatic one and never shown. + """ + return self._announce + + # ------------------------------------------------------------------ steps + def check_now(self, *, announce: bool = False) -> bool: + """Ask for a newer release. Returns whether a check was started. + + A refused duplicate still records an announcement: the result in + flight is owed to whoever asked for it, whichever call started it. + The intent is only ever raised, never lowered, while work is running. + """ + if self._busy: + self._announce = self._announce or announce + return False + self._announce = announce + self._cancelled = False + self._busy = True + QThreadPool.globalInstance().start(_Task(self._run_check)) + return True + + def _run_check(self) -> None: + release = None + failure: str | None = None + try: + release = self._check(self._installed) + except UpdateError as exc: + failure = str(exc) + except Exception as exc: + # Nothing escapes a pool thread. Without this an injected or + # future check that raised produced a bare QRunnable traceback and + # no signal at all, so the window waited on a result that was + # never coming. + failure = f"the update check failed: {exc}" + finally: + # Cleared before the result is published, so that anything which + # can see a release can also start the download. The other order + # leaves a window where `release` is set and `prepare` still + # refuses, which reads as an update that quietly does nothing. + self._busy = False + if failure is not None: + # Not `upToDate`. The feed was never read, so there is nothing to + # be up to date with, and saying so would be a claim made on the + # strength of a failed lookup. + self._release = None + self._signals.failed.emit(failure) + return + self._release = release + if release is None: + self._signals.upToDate.emit() + else: + self._signals.found.emit(release) + + def prepare(self, directory: Path | None = None) -> bool: + """Download and verify the release found earlier. + + Nothing is executed here. `ready` carries the verified installer and + its digest; deciding to run it is the window's call, because it is the + step that closes the app. + """ + if self._busy or self._release is None: + return False + self._busy = True + self._cancelled = False + target = directory + QThreadPool.globalInstance().start( + _Task(lambda: self._run_prepare(target))) + return True + + def cancel(self) -> bool: + """Ask the work in flight to stop. Returns whether there was any. + + Cooperative: the download is a blocking read loop on a pool thread and + cannot be interrupted, so this sets a flag the loop's progress + callback reads. What it guarantees is the part that was missing -- a + cancelled request does not go on to report itself ready, and the + incomplete download is removed rather than left on disk. + """ + if not self._busy: + return False + self._cancelled = True + return True + + @property + def cancelling(self) -> bool: + """Whether a cancel has been asked for and not yet taken effect.""" + return self._cancelled + + def _run_prepare(self, directory: Path | None) -> None: + release = self._release + where: Path | None = None + installer: Path | None = None + try: + where = Path(directory) if directory is not None else Path( + tempfile.mkdtemp(prefix="offloader-update-")) + where.mkdir(parents=True, exist_ok=True) + + last = 0 + + def report(done: int, total: int) -> None: + nonlocal last + if self._cancelled: + raise _Cancelled + # Throttled, and always emitted for the final block so the bar + # finishes rather than stopping just short. + if done - last >= PROGRESS_STEP or (total and done >= total): + last = done + self._signals.progress.emit(done, total) + + installer, digest = self._download(release, where, progress=report) + # Checked again: a download small enough to finish between two + # callbacks would otherwise sail past the cancel and go on to + # verify and offer itself for installation. + if self._cancelled: + raise _Cancelled + self._verify(installer, release, expected_digest=digest, + actual_digest=digest) + if self._cancelled: + raise _Cancelled + self._installer = installer + self._signals.ready.emit(release, digest) + except _Cancelled: + self._installer = None + self._discard(where, release, installer) + self._signals.cancelled.emit() + except UpdateError as exc: + self._signals.failed.emit(str(exc)) + except Exception as exc: # pragma: no cover - defensive + # A check that took the app down would be a worse bug than a + # missed update, so nothing escapes this thread. + self._signals.failed.emit(f"the update could not be prepared: {exc}") + finally: + self._busy = False + + def _discard(self, directory: Path | None, release: object, + installer: Path | None) -> None: + """Remove the incomplete download, and only that. + + Named from the release rather than swept from the directory, because a + caller may pass one it owns and a cancelled update has no business + deleting whatever else is in it. + """ + candidates = [path for path in (installer,) if path is not None] + name = getattr(release, "asset_name", None) + if directory is not None and isinstance(name, str) and name: + candidates.append(Path(directory) / name) + for path in candidates: + try: + Path(path).unlink(missing_ok=True) + except OSError: + # A file we could not remove is untidy; failing the cancel + # over it would be worse. + pass + + def install(self) -> None: + """Run the verified installer. Raises `UpdateError` if it cannot.""" + if self._installer is None: + raise UpdateError("no verified installer is ready") + self._apply(self._installer) + + +def refusal(states: list[object]) -> str | None: + """Why an update cannot be installed right now, or None if it can. + + Takes the queue's states rather than the controller, so the rule is a + function of what is running and can be tested without a window. A paused + job counts: it is a partially written destination waiting to continue, and + replacing the application under it is not a way to find out what happens. + """ + from .worker import JobState + + active = [state for state in states + if state in (JobState.RUNNING, JobState.PAUSED)] + if active: + return ("Offloader is copying. The installer will not replace a " + "running transfer, so finish or cancel the job first.") + return None diff --git a/src/offloader/update.py b/src/offloader/update.py index 74f1659..50034bc 100644 --- a/src/offloader/update.py +++ b/src/offloader/update.py @@ -87,6 +87,16 @@ class UpdateError(RuntimeError): """An update was found but could not be trusted or applied.""" +class FeedError(UpdateError): + """The feed could not be read, so nothing is known about updates. + + Distinct from "no newer release", and the distinction is the point: a + check that never got an answer must not be reported as a confirmation + that this is the newest release. That is a claim, and it would be made on + the strength of a failed DNS lookup. + """ + + @dataclass(frozen=True) class Release: """A candidate release, as far as the feed describes it.""" @@ -178,15 +188,37 @@ def release_from_feed(payload: Any, *, installed: str = __version__) -> Release return None +def check_feed(installed: str = __version__, *, url: str = FEED_URL, + fetch: Callable[[str], Any] = _fetch_json) -> Release | None: + """Ask the feed for a newer release, raising if it could not be asked. + + None here means the feed answered and has nothing newer. A feed that could + not be fetched, or that answered with something that is not a release + description, raises `FeedError` instead, so a caller with somewhere to put + the difference can tell "you are up to date" from "I could not find out". + """ + try: + payload = fetch(url) + except UpdateError: + raise + except Exception as exc: + raise FeedError(f"could not reach the release feed: {exc}") from exc + if not isinstance(payload, dict): + raise FeedError("the release feed did not describe a release") + return release_from_feed(payload, installed=installed) + + def check(installed: str = __version__, *, url: str = FEED_URL, fetch: Callable[[str], Any] = _fetch_json) -> Release | None: """Ask the feed for a newer release. Never raises. - Called on a timer and from a menu item, where the cost of an exception is - an interrupted app and the cost of returning None is one missed check. + Called where the cost of an exception is an interrupted caller and the + cost of returning None is one missed check. A caller that reports the + outcome to somebody wants `check_feed`: this one cannot tell a failure + from a confirmation, and neither can anyone reading its result. """ try: - return release_from_feed(fetch(url), installed=installed) + return check_feed(installed, url=url, fetch=fetch) except Exception: return None diff --git a/tests/test_gui_updates.py b/tests/test_gui_updates.py new file mode 100644 index 0000000..5137929 --- /dev/null +++ b/tests/test_gui_updates.py @@ -0,0 +1,497 @@ +"""The desktop app's update sequence. + +The behaviour that needs protecting is the refusal. Offloader's installer will +not replace a running transfer and never force-kills one, so the app must +decline an update rather than queue it, force it, or close itself while a copy +is in flight. Getting that wrong would not look like a bug in testing: it would +look like an update that worked, on a machine that happened to be idle. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") +pytest.importorskip("PySide6", reason="GUI extra not installed") + +from PySide6.QtCore import QDeadlineTimer, QEventLoop # noqa: E402 +from PySide6.QtWidgets import QApplication # noqa: E402 + +from offloader.gui.updates import PROGRESS_STEP, UpdateController, refusal # noqa: E402 +from offloader.gui.worker import JobState # noqa: E402 +from offloader.update import FeedError, Release, UpdateError # noqa: E402 + +RELEASE = Release(version="0.9.0", asset_name="Offloader-Setup-0.9.0.exe", + download_url="https://github.com/owenpkent/offloader/x.exe", + notes="Fixes a verification bug.") + + +@pytest.fixture(scope="session") +def qapp(): + return QApplication.instance() or QApplication([]) + + +def _pump(app, predicate, timeout_ms: int = 10_000) -> bool: + deadline = QDeadlineTimer(timeout_ms) + while not predicate(): + if deadline.hasExpired(): + return False + app.processEvents(QEventLoop.AllEvents, 20) + return True + + +# --------------------------------------------------------------- the refusal + + +@pytest.mark.parametrize("state", [JobState.RUNNING, JobState.PAUSED]) +def test_an_active_job_refuses_the_update(state): + """A paused job counts. It is a partially written destination waiting to + continue, and replacing the application under it is not a way to find out + what happens.""" + reason = refusal([state]) + assert reason is not None + assert "finish or cancel" in reason.lower() + + +@pytest.mark.parametrize("states", [ + [], + [JobState.QUEUED], + [JobState.DONE, JobState.FAILED, JobState.CANCELLED], + [JobState.QUEUED, JobState.DONE], +]) +def test_an_idle_queue_allows_the_update(states): + """A queued job has not started writing anything, so it is not a reason to + decline: the installer replaces the app and the job runs afterwards.""" + assert refusal(states) is None + + +def test_one_active_job_among_many_still_refuses(): + assert refusal([JobState.DONE, JobState.RUNNING, JobState.QUEUED]) is not None + + +# ------------------------------------------------------------- checking + + +def test_a_newer_release_is_reported(qapp): + seen: list[object] = [] + controller = UpdateController(check=lambda _installed: RELEASE) + controller.found.connect(seen.append) + + assert controller.check_now() + assert _pump(qapp, lambda: bool(seen)) + assert seen[0].version == "0.9.0" + assert controller.release is RELEASE + + +def test_being_up_to_date_is_reported_separately(qapp): + """The window says nothing on an automatic check that finds nothing, so + the two outcomes cannot share a signal.""" + calls: list[int] = [] + controller = UpdateController(check=lambda _installed: None) + controller.upToDate.connect(lambda: calls.append(1)) + + controller.check_now() + assert _pump(qapp, lambda: bool(calls)) + assert controller.release is None + + +def test_a_second_check_is_not_started_while_one_runs(qapp): + import threading + + release = threading.Event() + + def slow(_installed): + release.wait(5) + return None + + controller = UpdateController(check=slow) + assert controller.check_now() + assert _pump(qapp, lambda: controller.busy) + assert controller.check_now() is False + release.set() + assert _pump(qapp, lambda: not controller.busy) + + +def test_a_visible_release_can_always_be_prepared(qapp, tmp_path): + """The ordering the controller has to keep. The check publishes its result + and clears its own busy flag, and if it did those in the other order there + would be a window where a release is visible and `prepare` still refuses: + an update that reports itself and then quietly does nothing.""" + controller = UpdateController(check=lambda _installed: RELEASE, + download=lambda *_a, **_k: (tmp_path / "x", ""), + verify=lambda *_a, **_k: {}) + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + + assert not controller.busy, "a release was visible while still busy" + assert controller.prepare(tmp_path) is not False + + +def test_a_check_that_raises_does_not_escape_the_thread(qapp): + """`update.check` already swallows failures, but an injected or future + implementation must not be able to take the app down from a pool thread.""" + def explode(_installed): + raise OSError("no network") + + controller = UpdateController(check=explode) + controller.check_now() + assert _pump(qapp, lambda: not controller.busy) + + +# ---------------------------------------------- failure is not up to date + + +@pytest.mark.parametrize("failure", [ + OSError("no network"), + FeedError("could not reach the release feed"), + UpdateError("the release feed did not describe a release"), +]) +def test_a_check_that_could_not_be_made_reports_failure(qapp, failure): + """REGRESSION. The controller read `None` as "you are the newest release", + and `update.check` returns `None` for a failed fetch, a TLS error and an + unparseable feed alike. So a manual check told the user this was the newest + release on the strength of a failed DNS lookup, and an automatic failure + never reached the documented failure path at all.""" + def explode(_installed): + raise failure + + failures: list[str] = [] + up_to_date: list[int] = [] + controller = UpdateController(check=explode) + controller.failed.connect(failures.append) + controller.upToDate.connect(lambda: up_to_date.append(1)) + + controller.check_now() + assert _pump(qapp, lambda: bool(failures)) + assert up_to_date == [], "a failed check claimed the app was up to date" + assert controller.release is None + + +def test_a_genuine_up_to_date_answer_is_still_up_to_date(qapp): + """The other half. Distinguishing failure must not turn every quiet check + into a warning.""" + failures: list[str] = [] + up_to_date: list[int] = [] + controller = UpdateController(check=lambda _i: None) + controller.failed.connect(failures.append) + controller.upToDate.connect(lambda: up_to_date.append(1)) + + controller.check_now() + assert _pump(qapp, lambda: bool(up_to_date)) + assert failures == [] + + +@pytest.mark.parametrize("payload,outcome", [ + ({"tag_name": "v0.9.0", "assets": [ + {"name": "Offloader-Setup-0.9.0.exe", + "browser_download_url": "https://github.com/a/b/x.exe"}]}, "found"), + ({"tag_name": "v0.1.0", "assets": []}, "up to date"), + ("a proxy login page", "failed"), + (None, "failed"), +]) +def test_the_feed_check_separates_its_three_outcomes(payload, outcome): + """At the source, where the distinction is made. `check_feed` raises for a + feed it could not read and returns None only when the feed answered and had + nothing newer.""" + from offloader import update + + def fetch(_url): + return payload + + if outcome == "failed": + with pytest.raises(FeedError): + update.check_feed("0.5.0", fetch=fetch) + return + result = update.check_feed("0.5.0", fetch=fetch) + assert (result is not None) is (outcome == "found") + + +def test_a_fetch_that_raises_becomes_a_feed_error(): + from offloader import update + + def fetch(_url): + raise OSError("name or service not known") + + with pytest.raises(FeedError, match="could not reach"): + update.check_feed("0.5.0", fetch=fetch) + # The never-raising form is unchanged, for callers with nowhere to put it. + assert update.check("0.5.0", fetch=fetch) is None + + +# ------------------------------------------------------------- cancelling + + +def test_cancel_stops_a_download_in_flight(qapp, tmp_path): + """REGRESSION. The dialog's Cancel was never connected to anything. It hid + the dialog, the download and verification carried on, and the app then + offered to install what the user had just declined.""" + import threading + import time + + blocked = threading.Event() + verified: list[int] = [] + + def download(release, directory, progress=None): + path = Path(directory) / release.asset_name + path.write_bytes(b"MZ partial") + progress(1, 100) + blocked.set() + # Keeps calling back until the cancel is noticed, the way a real + # download does: the callback is the only place it hands control over + # often enough to be stopped. + deadline = time.monotonic() + 5 + while time.monotonic() < deadline: + progress(2, 100) + time.sleep(0.01) + raise AssertionError("the download was never cancelled") + + controller = UpdateController(check=lambda _i: RELEASE, download=download, + verify=lambda *_a, **_k: verified.append(1)) + outcomes: list[str] = [] + controller.cancelled.connect(lambda: outcomes.append("cancelled")) + controller.ready.connect(lambda *_a: outcomes.append("ready")) + controller.failed.connect(lambda *_a: outcomes.append("failed")) + + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + assert controller.prepare(tmp_path) + assert _pump(qapp, lambda: blocked.is_set()) + + assert controller.cancel() is True + assert _pump(qapp, lambda: bool(outcomes)) + + assert outcomes == ["cancelled"], "a cancelled download still reported in" + assert verified == [], "verification continued after the cancel" + assert controller.installer is None + assert not (tmp_path / RELEASE.asset_name).exists(), \ + "the incomplete download was left behind" + + +def test_cancel_after_the_bytes_have_arrived_still_suppresses_ready(qapp, tmp_path): + """A download small enough to finish between two callbacks would otherwise + sail past the cancel and offer itself for installation.""" + def download(release, directory, progress=None): + path = Path(directory) / release.asset_name + path.write_bytes(b"MZ") + controller.cancel() + return path, "d" * 64 + + outcomes: list[str] = [] + controller = UpdateController(check=lambda _i: RELEASE, download=download, + verify=lambda *_a, **_k: {}) + controller.cancelled.connect(lambda: outcomes.append("cancelled")) + controller.ready.connect(lambda *_a: outcomes.append("ready")) + + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + controller.prepare(tmp_path) + assert _pump(qapp, lambda: bool(outcomes)) + + assert outcomes == ["cancelled"] + assert controller.installer is None + assert not (tmp_path / RELEASE.asset_name).exists() + + +def test_cancelling_removes_only_the_download(qapp, tmp_path): + """The directory can be one the caller owns.""" + keep = tmp_path / "somebody-elses.txt" + keep.write_text("keep me", encoding="utf-8") + + def download(release, directory, progress=None): + (Path(directory) / release.asset_name).write_bytes(b"MZ") + controller.cancel() + return Path(directory) / release.asset_name, "d" * 64 + + controller = UpdateController(check=lambda _i: RELEASE, download=download, + verify=lambda *_a, **_k: {}) + done: list[int] = [] + controller.cancelled.connect(lambda: done.append(1)) + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + controller.prepare(tmp_path) + assert _pump(qapp, lambda: bool(done)) + + assert keep.read_text(encoding="utf-8") == "keep me" + + +def test_cancelling_nothing_says_so(qapp): + assert UpdateController().cancel() is False + + +# ------------------------------------------------- whose result it is + + +def test_a_refused_duplicate_check_keeps_an_announcement(qapp): + """REGRESSION. A manual check requested during the first 2.5 seconds was + still running when the launch timer fired. The timer's silent intent + overwrote it, the controller then refused the duplicate, and the manual + result was handled as an automatic one -- so the dialog the user asked for + never appeared.""" + import threading + + release = threading.Event() + + def slow(_installed): + release.wait(5) + return None + + controller = UpdateController(check=slow) + try: + assert controller.check_now(announce=True) + assert _pump(qapp, lambda: controller.busy) + + assert controller.check_now(announce=False) is False + assert controller.announce is True, "the manual request was downgraded" + finally: + release.set() + assert _pump(qapp, lambda: not controller.busy) + + +def test_a_manual_request_behind_an_automatic_one_is_still_announced(qapp): + """The other direction: the automatic check is already running when the + user asks. They asked, so they get an answer.""" + import threading + + release = threading.Event() + controller = UpdateController(check=lambda _i: release.wait(5) or None) + try: + assert controller.check_now(announce=False) + assert _pump(qapp, lambda: controller.busy) + assert controller.check_now(announce=True) is False + assert controller.announce is True + finally: + release.set() + assert _pump(qapp, lambda: not controller.busy) + + +def test_a_check_that_starts_sets_its_own_intent(qapp): + """Raised while work is in flight, but not sticky: the next automatic + check is silent again.""" + controller = UpdateController(check=lambda _i: None) + controller.check_now(announce=True) + assert _pump(qapp, lambda: not controller.busy) + assert controller.announce is True + + controller.check_now(announce=False) + assert _pump(qapp, lambda: not controller.busy) + assert controller.announce is False + + +# -------------------------------------------------------------- preparing + + +def test_preparing_downloads_verifies_and_reports_ready(qapp, tmp_path): + verified: list[tuple] = [] + ready: list[tuple] = [] + + def download(release, directory, progress=None): + path = Path(directory) / release.asset_name + path.write_bytes(b"MZ") + if progress is not None: + progress(2, 2) + return path, "d" * 64 + + def verify(path, release, **kwargs): + verified.append((path, release, kwargs)) + return {} + + controller = UpdateController(check=lambda _i: RELEASE, + download=download, verify=verify) + controller.ready.connect(lambda rel, digest: ready.append((rel, digest))) + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + + assert controller.prepare(tmp_path) + assert _pump(qapp, lambda: bool(ready)) + assert ready[0][1] == "d" * 64 + # The digest compared is the one taken while streaming, on both sides. + assert verified[0][2]["expected_digest"] == "d" * 64 + assert verified[0][2]["actual_digest"] == "d" * 64 + assert controller.installer is not None + + +def test_nothing_is_prepared_before_a_release_is_found(qapp, tmp_path): + controller = UpdateController(check=lambda _i: None) + assert controller.prepare(tmp_path) is False + + +def test_a_failed_verification_is_reported_and_nothing_is_ready(qapp, tmp_path): + """The case that matters most: a download that cannot be trusted must not + leave an installer the window could go on to run.""" + def download(release, directory, progress=None): + path = Path(directory) / release.asset_name + path.write_bytes(b"MZ") + return path, "d" * 64 + + def verify(_path, _release, **_kwargs): + raise UpdateError("the installer was signed by a different certificate") + + failures: list[str] = [] + controller = UpdateController(check=lambda _i: RELEASE, + download=download, verify=verify) + controller.failed.connect(failures.append) + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + controller.prepare(tmp_path) + + assert _pump(qapp, lambda: bool(failures)) + assert "different certificate" in failures[0] + assert controller.installer is None + + +def test_progress_is_throttled_but_finishes(qapp, tmp_path): + """One signal per block would emit thousands of times for a 200 MB + download; stopping short of the end would leave the bar at 99%.""" + total = PROGRESS_STEP * 4 + + def download(release, directory, progress=None): + path = Path(directory) / release.asset_name + path.write_bytes(b"MZ") + step = PROGRESS_STEP // 8 + for done in range(step, total + 1, step): + progress(done, total) + return path, "d" * 64 + + updates: list[tuple[int, int]] = [] + controller = UpdateController(check=lambda _i: RELEASE, download=download, + verify=lambda *_a, **_k: {}) + controller.progress.connect(lambda done, tot: updates.append((done, tot))) + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + controller.prepare(tmp_path) + assert _pump(qapp, lambda: updates and updates[-1][0] == total) + + assert len(updates) < 32, f"too many progress signals: {len(updates)}" + assert updates[-1] == (total, total) + + +# ---------------------------------------------------------------- installing + + +def test_installing_without_a_verified_installer_refuses(qapp): + controller = UpdateController() + with pytest.raises(UpdateError, match="no verified installer"): + controller.install() + + +def test_installing_runs_the_verified_file(qapp, tmp_path): + started: list[Path] = [] + + def download(release, directory, progress=None): + path = Path(directory) / release.asset_name + path.write_bytes(b"MZ") + return path, "d" * 64 + + controller = UpdateController(check=lambda _i: RELEASE, download=download, + verify=lambda *_a, **_k: {}, + apply=started.append) + controller.check_now() + assert _pump(qapp, lambda: controller.release is not None) + controller.prepare(tmp_path) + assert _pump(qapp, lambda: controller.installer is not None) + + controller.install() + assert started == [controller.installer] diff --git a/tests/test_gui_window.py b/tests/test_gui_window.py index bdf1fc1..18a0324 100644 --- a/tests/test_gui_window.py +++ b/tests/test_gui_window.py @@ -14,6 +14,7 @@ os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") pytest.importorskip("PySide6", reason="GUI extra not installed") +from PySide6.QtCore import QDeadlineTimer, QEventLoop # noqa: E402 from PySide6.QtWidgets import QApplication, QMessageBox # noqa: E402 from offloader.gui import main_window as mw # noqa: E402 @@ -238,3 +239,85 @@ def test_environment_summary_reports_missing_tools(window, monkeypatch): monkeypatch.setattr(mw.thumbs, "ffmpeg_path", lambda: None) summary = window._environment_summary() assert "ffprobe" in summary and "ffmpeg" in summary + + +# ----------------------------------------------------------------- updates + + +def test_a_manual_check_survives_the_launch_timer(window, qapp): + """REGRESSION. The window stamped its announcement intent before asking + whether a check had actually started. A manual check requested inside the + first 2.5 seconds was still running when the launch timer fired, the + timer's silent intent overwrote it, the controller refused the duplicate, + and the result the user had asked for was then handled silently.""" + import threading + + release = threading.Event() + window.updates._check = lambda _installed: release.wait(5) or None + try: + window._check_for_updates(announce=True) + deadline = QDeadlineTimer(5000) + while not window.updates.busy and not deadline.hasExpired(): + qapp.processEvents(QEventLoop.AllEvents, 20) + + # The launch timer, firing behind it. + window._check_for_updates(announce=False) + assert window.updates.announce is True + finally: + release.set() + deadline = QDeadlineTimer(5000) + while window.updates.busy and not deadline.hasExpired(): + qapp.processEvents(QEventLoop.AllEvents, 20) + + +def test_the_download_dialog_cancel_reaches_the_controller(window, qapp, + monkeypatch, tmp_path): + """REGRESSION. The dialog offered Cancel and nothing was connected to it, + so clicking it hid the dialog while the download carried on, and the app + then offered to install what the user had just declined.""" + from offloader.update import Release + + release = Release(version="9.9.9", asset_name="Offloader-Setup-9.9.9.exe", + download_url="https://github.com/a/b/x.exe") + cancelled: list[int] = [] + monkeypatch.setattr(window.updates, "cancel", + lambda: bool(cancelled.append(1)) or True) + monkeypatch.setattr(window.updates, "prepare", lambda *a, **k: True) + monkeypatch.setattr(mw.QMessageBox, "question", + staticmethod(lambda *a, **k: mw.QMessageBox.Yes)) + monkeypatch.setattr(window.updates, "_announce", True) + + window._on_update_available(release) + assert window._update_progress is not None + + # Emitted rather than calling `cancel()`: the button's `clicked` is wired + # to this signal, and the slot of that name does not raise it. + window._update_progress.canceled.emit() + qapp.processEvents(QEventLoop.AllEvents, 20) + assert cancelled, "Cancel was not connected to anything" + + +def test_closing_the_dialog_for_a_result_is_not_a_cancel(window, qapp, + monkeypatch, tmp_path): + """`QProgressDialog::closeEvent` emits `canceled` itself. Tearing the + dialog down because the download finished must not be reported back as the + user asking to stop it.""" + from offloader.update import Release + + release = Release(version="9.9.9", asset_name="Offloader-Setup-9.9.9.exe", + download_url="https://github.com/a/b/x.exe") + cancelled: list[int] = [] + monkeypatch.setattr(window.updates, "cancel", + lambda: bool(cancelled.append(1)) or True) + monkeypatch.setattr(window.updates, "prepare", lambda *a, **k: True) + monkeypatch.setattr(mw.QMessageBox, "question", + staticmethod(lambda *a, **k: mw.QMessageBox.Yes)) + monkeypatch.setattr(window.updates, "_announce", True) + + window._on_update_available(release) + assert window._update_progress is not None + + window._close_update_progress() + qapp.processEvents(QEventLoop.AllEvents, 20) + assert cancelled == [], "closing the dialog was reported as a cancel" + assert window._update_progress is None