From b1839b8728df09678c46a4d862dd586b240df0ee Mon Sep 17 00:00:00 2001 From: huangruiteng Date: Sun, 6 Sep 2026 14:24:23 +0800 Subject: [PATCH] fix(update): retry bootstrap downloads with safe diagnostics Signed-off-by: huangruiteng --- docs/guides/installing-loopx.md | 17 +++ examples/loopx-update-smoke.py | 61 +++------- loopx/self_update.py | 37 +++--- loopx/self_update_download.py | 108 ++++++++++++++++++ tests/test_self_update_download.py | 113 +++++++++++++++++++ tests/test_self_update_runtime_activation.py | 5 +- 6 files changed, 274 insertions(+), 67 deletions(-) create mode 100644 loopx/self_update_download.py create mode 100644 tests/test_self_update_download.py diff --git a/docs/guides/installing-loopx.md b/docs/guides/installing-loopx.md index 88c559890d..c25ee829d8 100644 --- a/docs/guides/installing-loopx.md +++ b/docs/guides/installing-loopx.md @@ -140,6 +140,23 @@ loopx update plan # read-only command, validation, and rollback plan loopx update apply # explicit local-environment mutation ``` +For archive installs, `update apply` downloads the bootstrap installer to a +private temporary file before executing it. Downloading is limited to three +attempts, a 60-second total download budget (or the smaller command timeout), +and 20 seconds per transfer, with 1- and 2-second retry delays. Transient +403/408/429/500/502/503/504 responses and selected connection/transfer failures +are retried; 401/404 and certificate-verification failures stop immediately. +Installer execution is never retried, and partial downloads are never executed. +The command timeout covers downloading and installer execution together. + +JSON `execution.installer_download` and the text execution report show the +stage and each attempt's HTTP status and curl exit code. HTTP `0` means no +usable HTTP status was received. Download diagnostics exclude URLs, response +bodies, headers, and raw curl errors so proxy credentials and signed parameters +cannot leak. These retries cover the bootstrap download, not subsequent +archive downloads or a failed installation. Pip/pipx and read-only plans keep +their existing behavior. + Bare `loopx update` remains a read-only plan. The older `--check`, `--dry-run`, and `--execute` spellings remain compatibility aliases, but new instructions should use the named actions. diff --git a/examples/loopx-update-smoke.py b/examples/loopx-update-smoke.py index 4744b5f97c..f4ef26894d 100644 --- a/examples/loopx-update-smoke.py +++ b/examples/loopx-update-smoke.py @@ -183,7 +183,7 @@ def test_module_plan() -> None: assert payload["plan"]["backup"]["rollback_release_id"] == "20260621T170342Z", payload assert "loopx update --rollback 20260621T170342Z" in payload["plan"]["backup"]["rollback_command"], payload assert "ln -sfn" not in payload["plan"]["backup"]["rollback_command"], payload - assert "LOOPX_ARCHIVE_URL=https://example.invalid/loopx.tar.gz" in payload["plan"]["install_command"], payload + assert "--archive-url https://example.invalid/loopx.tar.gz" in payload["plan"]["install_command"], payload rendered = render_update_plan_markdown(payload) assert "**No update was applied.**" in rendered, rendered assert "## Next Action" in rendered, rendered @@ -199,8 +199,8 @@ def test_default_source_uses_stable_ref() -> None: assert payload["source"]["channel"] == "github_archive_stable", payload assert payload["source"]["ref_source"] == "default_stable", payload assert "/tar.gz/stable" in payload["source"]["archive_url"], payload - assert "LOOPX_REF=stable" in payload["plan"]["install_command"], payload - assert "LOOPX_ARCHIVE_URL=" not in payload["plan"]["install_command"], payload + assert payload["plan"]["install_command"] == "loopx update apply", payload + assert "--archive-url" not in payload["plan"]["install_command"], payload default_env = _installer_env_for_source( payload["source"], base_env={"LOOPX_ARCHIVE_URL": "https://stale.invalid/archive.tar.gz"}, @@ -216,47 +216,19 @@ def test_explicit_archive_url_reaches_installer() -> None: doctor_payload=fake_doctor_payload(), ) assert payload["source"]["channel"] == "github_archive_url_override", payload - assert "LOOPX_ARCHIVE_URL=https://example.invalid/loopx.tar.gz" in payload["plan"]["install_command"], payload - assert "set -o errexit -o pipefail" in payload["plan"]["install_command"], payload - assert "export LOOPX_REPO=example/loopx" in payload["plan"]["install_command"], payload - assert "export LOOPX_REF=fixture" in payload["plan"]["install_command"], payload + assert "--archive-url https://example.invalid/loopx.tar.gz" in payload["plan"]["install_command"], payload + assert "loopx update apply" in payload["plan"]["install_command"], payload + assert "--repo example/loopx" in payload["plan"]["install_command"], payload + assert "--ref fixture" in payload["plan"]["install_command"], payload installer_env = _installer_env_for_source(payload["source"], base_env={}) assert installer_env["LOOPX_ARCHIVE_URL"] == "https://example.invalid/loopx.tar.gz", installer_env -def test_update_preview_stops_before_doctor_when_download_fails() -> None: - payload = build_update_plan( - repo="example/loopx", - ref="fixture", - archive_url="https://example.invalid/loopx.tar.gz", - execute=False, - doctor_payload=fake_doctor_payload(), - ) - with TemporaryDirectory() as tmpdir: - bin_dir = Path(tmpdir) - marker_path = bin_dir / "doctor-ran" - curl_bin = bin_dir / "curl" - curl_bin.write_text("#!/usr/bin/env bash\nexit 22\n", encoding="utf-8") - curl_bin.chmod(0o755) - loopx_bin = bin_dir / "loopx" - loopx_bin.write_text( - "#!/usr/bin/env bash\n" - f"touch {json.dumps(str(marker_path))}\n" - "exit 0\n", - encoding="utf-8", - ) - loopx_bin.chmod(0o755) - env = dict(os.environ) - env["PATH"] = f"{bin_dir}{os.pathsep}{env.get('PATH', '')}" - result = subprocess.run( - ["bash", "-c", payload["plan"]["install_command"]], - text=True, - capture_output=True, - env=env, - ) - - assert result.returncode == 22, result - assert not marker_path.exists(), result +def test_update_preview_uses_managed_download_path() -> None: + payload = build_update_plan(doctor_payload=fake_doctor_payload()) + command = payload["plan"]["install_command"] + assert command.startswith("loopx update apply"), command + assert "curl" not in command, command def test_execute_update_propagates_installer_download_failure() -> None: @@ -290,10 +262,13 @@ def test_execute_update_propagates_installer_download_failure() -> None: assert result["ok"] is False, result install_command = run.call_args_list[0].args[0] - assert install_command[:2] == ["bash", "-lc"], install_command - assert "set -o pipefail" in install_command[2], install_command + assert install_command[0] == "curl", install_command + assert "--output" in install_command, install_command assert result["execution"]["install_returncode"] == 22, result assert result["execution"]["doctor_returncode"] == 0, result + assert result["execution"]["installer_download"]["stage"] == "installer_download", result + assert result["changes_applied"] is False, result + assert "Download attempt 1: HTTP `0`, curl `22`" in render_update_plan_markdown(result) def test_active_release_python_reaches_installer() -> None: @@ -562,7 +537,7 @@ def main() -> int: test_module_plan() test_default_source_uses_stable_ref() test_explicit_archive_url_reaches_installer() - test_update_preview_stops_before_doctor_when_download_fails() + test_update_preview_uses_managed_download_path() test_execute_update_propagates_installer_download_failure() test_active_release_python_reaches_installer() test_active_release_python_marker_fails_closed() diff --git a/loopx/self_update.py b/loopx/self_update.py index 827b9bfba6..8f8628a038 100644 --- a/loopx/self_update.py +++ b/loopx/self_update.py @@ -13,6 +13,7 @@ from .doctor import collect_doctor from .install_contract import NO_CLONE_INSTALL_URL +from .self_update_download import run_archive_installer UPDATE_PLAN_SCHEMA_VERSION = "loopx_update_plan_v0" @@ -105,20 +106,7 @@ def _command_for_source(source: dict[str, Any]) -> str: "Update a trusted LoopX checkout, then run its " "`scripts/install-windows.ps1` with PowerShell 7." ) - exports = [ - f"LOOPX_REPO={shlex.quote(str(source['repo']))}", - f"LOOPX_REF={shlex.quote(str(source['ref']))}", - ] - archive_url = source.get("archive_url") - if source.get("channel") == "github_archive_url_override" and archive_url: - exports.append(f"LOOPX_ARCHIVE_URL={shlex.quote(str(archive_url))}") - return ( - "set -o errexit -o pipefail\n" - + "\n".join(f"export {value}" for value in exports) - + f"\ncurl -fsSL {shlex.quote(str(source['installer_url']))} | bash\n" - 'export PATH="$HOME/.local/bin:$PATH"\n' - "loopx doctor" - ) + return _update_action_command(UpdateAction.APPLY, source) def _installer_env_for_source( @@ -1104,16 +1092,10 @@ def execute_update_plan( current_release_root if isinstance(current_release_root, str) else None ), ) - install_result = subprocess.run( - [ - "bash", - "-lc", - f"set -o pipefail; curl -fsSL {shlex.quote(installer_url)} | bash", - ], - text=True, - capture_output=True, + install_result, download_observation = run_archive_installer( + installer_url, env=env, - timeout=timeout_seconds, + timeout_seconds=timeout_seconds, ) loopx_bin = Path.home() / ".local" / "bin" / "loopx" doctor_result = subprocess.run( @@ -1124,6 +1106,7 @@ def execute_update_plan( timeout=timeout_seconds, ) execution = { + "installer_download": download_observation, "install_returncode": install_result.returncode, "doctor_returncode": doctor_result.returncode, "install_stdout_tail": install_result.stdout[-2000:], @@ -1472,4 +1455,12 @@ def render_update_plan_markdown(payload: dict[str, Any]) -> str: f"- Doctor return code: `{execution.get('doctor_returncode')}`", ] ) + download = execution.get("installer_download") + if isinstance(download, dict): + lines.append(f"- Installer stage: `{download.get('stage')}`") + for attempt in download.get("attempts", []): + lines.append( + f"- Download attempt {attempt['attempt']}: " + f"HTTP `{attempt['http_status']}`, curl `{attempt['curl_returncode']}`" + ) return "\n".join(lines) + "\n" diff --git a/loopx/self_update_download.py b/loopx/self_update_download.py new file mode 100644 index 0000000000..e145701ef8 --- /dev/null +++ b/loopx/self_update_download.py @@ -0,0 +1,108 @@ +"""Bounded archive-installer download; never execute a partial response.""" + +import subprocess +import time +from pathlib import Path +from tempfile import TemporaryDirectory +from typing import Any + + +def run_archive_installer( + url: str, *, env: dict[str, str], timeout_seconds: int +) -> tuple[subprocess.CompletedProcess[str], dict[str, Any]]: + deadline = time.monotonic() + timeout_seconds + download_deadline = min(deadline, time.monotonic() + 60) + observation: dict[str, Any] = {"stage": "installer_download", "attempts": []} + # Deliberately omit URLs, headers, bodies and raw curl errors: any of these + # can carry proxy credentials or signed URL parameters. + with TemporaryDirectory(prefix="loopx-update-") as raw: + script = Path(raw) / "install.sh" + script.touch(mode=0o600) + code = 28 + for attempt in range(1, 4): + remaining = download_deadline - time.monotonic() + if remaining <= 0: + break + # Remove bytes from a failed transfer before the next attempt. + script.write_bytes(b"") + args = [ + "curl", + "--silent", + "--show-error", + "--fail", + "--location", + "--connect-timeout", + "10", + "--max-time", + str(min(20, remaining)), + "--output", + str(script), + "--write-out", + "%{http_code}", + url, + ] + try: + result = subprocess.run( + args, + text=True, + capture_output=True, + env=env, + timeout=remaining, + check=False, + ) + code = result.returncode + status = ( + int(result.stdout.strip()) if result.stdout.strip().isdigit() else 0 + ) + except subprocess.TimeoutExpired: + code, status = 28, 0 + except OSError: + code, status = 127, 0 + observation["attempts"].append( + {"attempt": attempt, "curl_returncode": code, "http_status": status} + ) + if code == 0 and 200 <= status < 300 and script.stat().st_size: + remaining = deadline - time.monotonic() + if remaining <= 0: + code = 28 + break + observation["stage"] = "installer_execution" + try: + return subprocess.run( + ["bash", str(script)], + check=False, + text=True, + capture_output=True, + env=env, + timeout=remaining, + ), observation + except subprocess.TimeoutExpired: + return subprocess.CompletedProcess( + [], + 124, + "", + "Installer execution timed out; it was not retried.", + ), observation + # 403 can be transient at an edge/proxy; retry only within this + # budget. Never change credentials, endpoint, or TLS verification. + retryable = status in {403, 408, 429, 500, 502, 503, 504} or code in { + 5, + 6, + 7, + 18, + 28, + 35, + 52, + 55, + 56, + } + code = code or 22 + if not retryable or attempt == 3: + break + remaining = download_deadline - time.monotonic() + if remaining <= 0: + break + time.sleep(min(attempt, remaining)) + return subprocess.CompletedProcess( + [], code, "", "Installer download failed; see installer_download diagnostics." + ), observation diff --git a/tests/test_self_update_download.py b/tests/test_self_update_download.py new file mode 100644 index 0000000000..4b105f46bf --- /dev/null +++ b/tests/test_self_update_download.py @@ -0,0 +1,113 @@ +import subprocess +from pathlib import Path + +import pytest + +from loopx.self_update_download import run_archive_installer + + +@pytest.mark.parametrize( + "failure", [(22, "403"), (56, "403"), (18, "200"), (28, "000")] +) +def test_transient_download_discards_partial_bytes_before_execution( + monkeypatch, failure +): + calls = [] + + def run(args, **kwargs): + calls.append(args) + if args[0] == "curl": + script = Path(args[args.index("--output") + 1]) + assert script.read_bytes() == b"" + assert script.stat().st_mode & 0o777 == 0o600 + if len(calls) == 1: + script.write_text("echo PARTIAL_SECRET") + return subprocess.CompletedProcess( + args, failure[0], failure[1], "SECRET" + ) + script.write_text("printf complete") + return subprocess.CompletedProcess(args, 0, "200", "") + assert Path(args[1]).read_text() == "printf complete" + return subprocess.CompletedProcess(args, 0, "complete", "") + + monkeypatch.setattr("loopx.self_update_download.subprocess.run", run) + monkeypatch.setattr("loopx.self_update_download.time.sleep", lambda _: None) + result, diagnostic = run_archive_installer( + "https://example.invalid/install?secret=SECRET", env={}, timeout_seconds=90 + ) + assert result.stdout == "complete" + assert [call[0] for call in calls] == ["curl", "curl", "bash"] + assert diagnostic["stage"] == "installer_execution" + assert len(diagnostic["attempts"]) == 2 + assert "SECRET" not in str(diagnostic) + assert not Path(calls[-1][1]).exists() + + +@pytest.mark.parametrize( + "status,attempts", [("403", 3), ("503", 3), ("401", 1), ("404", 1)] +) +def test_failed_download_never_executes_and_reports_only_safe_fields( + monkeypatch, status, attempts +): + calls = [] + + def run(args, **kwargs): + calls.append(args) + assert args[0] == "curl" + Path(args[args.index("--output") + 1]).write_text("PRIVATE_RESPONSE_BODY") + return subprocess.CompletedProcess( + args, 22, status, "PRIVATE_PROXY_CREDENTIALS" + ) + + monkeypatch.setattr("loopx.self_update_download.subprocess.run", run) + monkeypatch.setattr("loopx.self_update_download.time.sleep", lambda _: None) + result, diagnostic = run_archive_installer( + "https://example.invalid/", env={}, timeout_seconds=90 + ) + assert result.returncode == 22 + assert len(calls) == attempts + assert diagnostic["stage"] == "installer_download" + assert diagnostic["attempts"][-1]["http_status"] == int(status) + assert "PRIVATE" not in str(diagnostic) + result.stdout + result.stderr + + +def test_download_timeout_honors_total_budget(monkeypatch): + now = [0.0] + calls = [] + + def run(args, **kwargs): + calls.append(args) + now[0] += kwargs["timeout"] + raise subprocess.TimeoutExpired(args, kwargs["timeout"], stderr="SECRET") + + monkeypatch.setattr("loopx.self_update_download.time.monotonic", lambda: now[0]) + monkeypatch.setattr("loopx.self_update_download.subprocess.run", run) + result, diagnostic = run_archive_installer( + "https://example.invalid/", env={}, timeout_seconds=2 + ) + assert result.returncode == 28 + assert len(calls) == 1 + assert now[0] == 2 + assert diagnostic["attempts"][0]["http_status"] == 0 + + +@pytest.mark.parametrize("timeout", [False, True]) +def test_installer_failure_is_not_retried(monkeypatch, timeout): + calls = [] + + def run(args, **kwargs): + calls.append(args) + if args[0] == "curl": + Path(args[args.index("--output") + 1]).write_text("exit 9") + return subprocess.CompletedProcess(args, 0, "200", "") + if timeout: + raise subprocess.TimeoutExpired(args, kwargs["timeout"]) + return subprocess.CompletedProcess(args, 9, "", "installer failed") + + monkeypatch.setattr("loopx.self_update_download.subprocess.run", run) + result, diagnostic = run_archive_installer( + "https://example.invalid/", env={}, timeout_seconds=90 + ) + assert result.returncode == (124 if timeout else 9) + assert len(calls) == 2 + assert diagnostic["stage"] == "installer_execution" diff --git a/tests/test_self_update_runtime_activation.py b/tests/test_self_update_runtime_activation.py index 46bf10c3f0..aab60bea28 100644 --- a/tests/test_self_update_runtime_activation.py +++ b/tests/test_self_update_runtime_activation.py @@ -444,6 +444,7 @@ def test_successful_update_revalidates_enabled_extensions( "plan": {"backup": {}}, } completed = [ + subprocess.CompletedProcess([], 0, "200", ""), subprocess.CompletedProcess([], 0, "installed", ""), subprocess.CompletedProcess([], 0, '{"ok": true}', ""), subprocess.CompletedProcess([], 0, '{"ok": true}', ""), @@ -454,6 +455,8 @@ def fake_run( args: list[str], **_kwargs: object ) -> subprocess.CompletedProcess[str]: calls.append(list(args)) + if args[0] == "curl": + Path(args[args.index("--output") + 1]).write_text("echo installed") return completed[len(calls) - 1] monkeypatch.setattr(Path, "home", staticmethod(lambda: tmp_path)) @@ -467,7 +470,7 @@ def fake_run( assert updated["ok"] is True assert updated["execution"]["extension_doctor_returncode"] == 0 - assert calls[2] == [ + assert calls[3] == [ str(tmp_path / ".local" / "bin" / "loopx"), "--format", "json",