From 9c1b3b4650cb88103f83844693d1f5feab8c8034 Mon Sep 17 00:00:00 2001 From: Thibault VINCENT Date: Mon, 17 Aug 2026 17:45:34 +0200 Subject: [PATCH] fix(update): never install a fabricated version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OTA updater could stamp a build with a version that did not come from the code being installed, which left nodes stuck on a version they could not upgrade away from. Two independent defects combined: * _fetch_latest_version() returned the newest repository tag whenever the GitHub lookup failed. On a dynamic channel such as dev, that value is from a different branch entirely, so the UI reported "up to date" while tracking a channel that was hundreds of commits ahead. The fallback also swallowed _RateLimitError, defeating the backoff path already built for it. * _do_install() then passed that predicted version to setuptools_scm as SETUPTOOLS_SCM_PRETEND_VERSION (and as the pretend-version argument of pymc-do-upgrade), so pip built the channel's code but recorded the predicted version as the installed one. When the two matched, every later upgrade — including Force Reinstall — resolved as "requirement already satisfied" and did nothing. Observed on a node tracking dev: installed 1.1.1, latest 1.1.1, no update offered, while dev was at 1.1.2.dev201. A version lookup that cannot be resolved now raises, so a failed check is reported as a failed check instead of a wrong version, and the install derives its version from the checkout pip clones. The remaining pretend-version uses in manage.sh are scoped to this distribution. The unscoped variable applies to every setuptools_scm-based project built in the same pip invocation, not only ours: dependencies normally arrive as wheels and are unaffected, but one built from an sdist inherits our version. With it set, pip wheel on the portend sdist produces portend-9.9.9; portend and jaraco.functools both build with setuptools_scm. buildroot-manage.sh already exports the scoped variant. Signed-off-by: Thibault VINCENT --- manage.sh | 18 +++-- repeater/web/update_endpoints.py | 39 ++++++++--- tests/test_update_endpoints_unit.py | 104 ++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 16 deletions(-) diff --git a/manage.sh b/manage.sh index fc11f5d..fcf0c7e 100755 --- a/manage.sh +++ b/manage.sh @@ -723,8 +723,11 @@ if ! [[ "$CHANNEL" =~ ^[a-zA-Z0-9._/-]{1,80}$ ]]; then exit 1 fi # If caller supplied a version string, tell setuptools_scm to use it (sudo -# strips env vars so it is passed as a positional argument instead). -[ -n "$PRETEND_VERSION" ] && export SETUPTOOLS_SCM_PRETEND_VERSION="$PRETEND_VERSION" +# strips env vars so it is passed as a positional argument instead). Scoped to +# this distribution: the unscoped variable applies to every setuptools_scm +# project built in the same pip run, so a dependency built from an sdist +# instead of a wheel would be stamped with our version too. +[ -n "$PRETEND_VERSION" ] && export SETUPTOOLS_SCM_PRETEND_VERSION_FOR_OPENHOP_REPEATER="$PRETEND_VERSION" # ---- Migration: ensure venv exists (handles upgrades from system-pip era) ---- if [ ! -x "$VENV_PYTHON" ]; then echo "[pymc-do-upgrade] Creating venv at $VENV_DIR ..." @@ -834,7 +837,7 @@ UPGRADEEOF # Only inspect git metadata when this directory is a checkout of the expected repo. if is_expected_repo_checkout "$SCRIPT_DIR"; then if GIT_VERSION="$(python3 -m setuptools_scm 2>/dev/null)"; then - export SETUPTOOLS_SCM_PRETEND_VERSION="$GIT_VERSION" + export SETUPTOOLS_SCM_PRETEND_VERSION_FOR_OPENHOP_REPEATER="$GIT_VERSION" echo "Installing version: $GIT_VERSION" fi fi @@ -1075,7 +1078,7 @@ upgrade_repeater() { if is_expected_repo_checkout "$SCRIPT_DIR"; then if GIT_VERSION="$(python3 -m setuptools_scm 2>/dev/null)"; then - export SETUPTOOLS_SCM_PRETEND_VERSION="$GIT_VERSION" + export SETUPTOOLS_SCM_PRETEND_VERSION_FOR_OPENHOP_REPEATER="$GIT_VERSION" echo "Upgrading to version: $GIT_VERSION" fi fi @@ -1212,8 +1215,11 @@ if ! [[ "$CHANNEL" =~ ^[a-zA-Z0-9._/-]{1,80}$ ]]; then exit 1 fi # If caller supplied a version string, tell setuptools_scm to use it (sudo -# strips env vars so it is passed as a positional argument instead). -[ -n "$PRETEND_VERSION" ] && export SETUPTOOLS_SCM_PRETEND_VERSION="$PRETEND_VERSION" +# strips env vars so it is passed as a positional argument instead). Scoped to +# this distribution: the unscoped variable applies to every setuptools_scm +# project built in the same pip run, so a dependency built from an sdist +# instead of a wheel would be stamped with our version too. +[ -n "$PRETEND_VERSION" ] && export SETUPTOOLS_SCM_PRETEND_VERSION_FOR_OPENHOP_REPEATER="$PRETEND_VERSION" # ---- Migration: ensure venv exists (handles upgrades from system-pip era) ---- if [ ! -x "$VENV_PYTHON" ]; then echo "[pymc-do-upgrade] Creating venv at $VENV_DIR ..." diff --git a/repeater/web/update_endpoints.py b/repeater/web/update_endpoints.py index 07eafcd..624165c 100644 --- a/repeater/web/update_endpoints.py +++ b/repeater/web/update_endpoints.py @@ -671,6 +671,13 @@ def _has_update(installed: str, latest: str) -> bool: def _fetch_latest_version(channel: str) -> str: + """Return the version currently published on ``channel``. + + Raises when the version cannot be determined. Returning an approximation + (such as the newest tag) would be actively harmful: the caller stores this + value as the channel's latest version, so a wrong value both misreports + the update state and gets baked into the install as the built version. + """ base_tag = _get_latest_tag() # always needed for dynamic branches @@ -680,22 +687,28 @@ def _fetch_latest_version(channel: str) -> str: body = _fetch_url(compare_url, timeout=10) data = json.loads(body) ahead_by = int(data.get("ahead_by", 0)) - return _next_dev_version(base_tag, ahead_by) + except _RateLimitError: + raise except Exception as exc: - logger.debug(f"[Update] Dynamic version compare failed for {channel!r}: {exc}") - return base_tag # fallback: show the tag + raise RuntimeError( + f"Could not compare {base_tag}...{channel} on GitHub: {exc}" + ) from exc + return _next_dev_version(base_tag, ahead_by) # Static version channel — read the pinned version from pyproject.toml on # that branch directly, so tags created on other branches don't affect it. try: toml_url = f"{GITHUB_RAW_BASE}/{channel}/pyproject.toml" toml_text = _fetch_url(toml_url, timeout=8) - m = re.search(r'^version\s*=\s*["\']([0-9][^"\']*)["\']', toml_text, re.MULTILINE) - if m: - return m.group(1) + except _RateLimitError: + raise except Exception as exc: - logger.debug(f"[Update] Static version lookup failed for {channel!r}: {exc}") - return base_tag # last-resort fallback + raise RuntimeError(f"Could not read pyproject.toml from {channel}: {exc}") from exc + + m = re.search(r'^version\s*=\s*["\']([0-9][^"\']*)["\']', toml_text, re.MULTILINE) + if not m: + raise RuntimeError(f"No pinned version found in pyproject.toml on {channel}") + return m.group(1) def _fetch_changelog(channel: str, installed: str, max_commits: int = 50) -> List[dict]: @@ -932,8 +945,13 @@ def _do_install() -> None: import os as _os + # No SETUPTOOLS_SCM_PRETEND_VERSION here: the version reported by the last + # check is a prediction, and pip derives the real one from the checkout it + # clones. Forcing the predicted value stamped the build with a version that + # did not match its code — and when that value was stale (a failed check, or + # a channel switch) pip saw the requirement as already satisfied and skipped + # the install, pinning the node to a version it could no longer leave. env = _os.environ.copy() - env["SETUPTOOLS_SCM_PRETEND_VERSION"] = _state.latest_version or "1.0.0" _VENV_DIR = "/opt/openhop_repeater/venv" _VENV_PIP = os.path.join(_VENV_DIR, "bin", "pip") @@ -991,7 +1009,8 @@ def _do_install() -> None: elif _os.path.isfile(_UPGRADE_WRAPPER): _state.append_line(f"[pyMC updater] Using sudo wrapper: {_UPGRADE_WRAPPER}") # The wrapper handles venv creation/migration internally - cmd = [_SUDO_BIN, _UPGRADE_WRAPPER, channel, _state.latest_version or ""] + # Second argument (pretend-version) deliberately omitted — see above. + cmd = [_SUDO_BIN, _UPGRADE_WRAPPER, channel] else: msg = ( f"Upgrade wrapper not found at {_UPGRADE_WRAPPER}. " diff --git a/tests/test_update_endpoints_unit.py b/tests/test_update_endpoints_unit.py index 280b581..2c7ab68 100644 --- a/tests/test_update_endpoints_unit.py +++ b/tests/test_update_endpoints_unit.py @@ -470,3 +470,107 @@ def test_do_install_wrapper_success_container_path(isolated_state, monkeypatch): assert st.state == "complete" assert st.error_message is None assert any("container will restart" in line for line in st.progress_lines) + + +def test_fetch_latest_version_refuses_to_guess_on_failure(monkeypatch): + # The value returned here is stored as the channel's latest version and is + # what the UI compares against, so a guess is worse than an error. + monkeypatch.setattr(ue, "_get_latest_tag", lambda: "1.1.1") + monkeypatch.setattr(ue, "_branch_is_dynamic", lambda _ch: True) + monkeypatch.setattr( + ue, + "_fetch_url", + lambda _url, timeout=10: (_ for _ in ()).throw(RuntimeError("compare unavailable")), + ) + + with pytest.raises(RuntimeError, match="Could not compare"): + ue._fetch_latest_version("dev") + + +def test_fetch_latest_version_propagates_rate_limit(monkeypatch): + # A rate-limited check must reach _do_check as a _RateLimitError so the + # existing backoff path runs instead of being reported as a version. + monkeypatch.setattr(ue, "_get_latest_tag", lambda: "1.1.1") + monkeypatch.setattr(ue, "_branch_is_dynamic", lambda _ch: True) + monkeypatch.setattr( + ue, + "_fetch_url", + lambda _url, timeout=10: (_ for _ in ()).throw(ue._RateLimitError("limited")), + ) + + with pytest.raises(ue._RateLimitError): + ue._fetch_latest_version("dev") + + +def test_fetch_latest_version_static_channel_without_pin_raises(monkeypatch): + monkeypatch.setattr(ue, "_get_latest_tag", lambda: "1.1.1") + monkeypatch.setattr(ue, "_branch_is_dynamic", lambda _ch: False) + monkeypatch.setattr(ue, "_fetch_url", lambda _url, timeout=8: 'name = "x"\n') + + with pytest.raises(RuntimeError, match="No pinned version"): + ue._fetch_latest_version("main") + + +def _capturing_popen(monkeypatch, calls): + class _Proc: + def __init__(self, cmd, env): + calls.append({"cmd": cmd, "env": env or {}}) + self.stdout = ["ok\n"] + self.returncode = 0 + + def wait(self): + return None + + monkeypatch.setattr(ue.subprocess, "Popen", lambda cmd, **kwargs: _Proc(cmd, kwargs.get("env"))) + + +def test_do_install_root_does_not_force_a_predicted_version(isolated_state, monkeypatch): + st = isolated_state + st.channel = "dev" + # A stale prediction, e.g. left by a check that fell back to a tag. + st.latest_version = "1.1.1" + + monkeypatch.setattr(ue.os, "geteuid", lambda: 0) + monkeypatch.setattr(ue, "is_buildroot", lambda: False) + monkeypatch.setattr(ue, "is_container", lambda: False) + monkeypatch.setattr(ue, "_migrate_service_unit", lambda: None) + monkeypatch.setattr(ue, "_disable_legacy_services", lambda: None) + monkeypatch.setattr(ue, "_cleanup_stale_dist_info", lambda *args, **kwargs: None) + monkeypatch.setattr(ue.os.path, "isfile", lambda p: True) + monkeypatch.setattr(ue.os.path, "isdir", lambda p: False) + monkeypatch.setattr(ue.time, "sleep", lambda _s: None) + monkeypatch.setattr("repeater.service_utils.restart_service", lambda: (True, "ok")) + + calls = [] + _capturing_popen(monkeypatch, calls) + + ue._do_install() + + assert calls, "expected the installer to run at least one command" + for call in calls: + assert "SETUPTOOLS_SCM_PRETEND_VERSION" not in call["env"] + install = [c for c in calls if any("git+https://github.com" in str(x) for x in c["cmd"])] + assert install, "expected a pip install from git" + assert any("@dev" in str(x) for x in install[-1]["cmd"]) + + +def test_do_install_wrapper_call_omits_pretend_version(isolated_state, monkeypatch): + st = isolated_state + st.channel = "dev" + st.latest_version = "1.1.1" + + monkeypatch.setattr(ue.os, "geteuid", lambda: 1000) + monkeypatch.setattr(ue, "is_buildroot", lambda: False) + monkeypatch.setattr(ue, "is_container", lambda: False) + monkeypatch.setattr(ue, "_cleanup_stale_dist_info", lambda *args, **kwargs: None) + monkeypatch.setattr(ue.time, "sleep", lambda _s: None) + monkeypatch.setattr(ue.os.path, "isfile", lambda p: p == "/usr/local/bin/pymc-do-upgrade") + monkeypatch.setattr("repeater.service_utils.restart_service", lambda: (True, "ok")) + + calls = [] + _capturing_popen(monkeypatch, calls) + + ue._do_install() + + assert calls[-1]["cmd"] == [ue._SUDO_BIN, "/usr/local/bin/pymc-do-upgrade", "dev"] + assert "SETUPTOOLS_SCM_PRETEND_VERSION" not in calls[-1]["env"]