From 5e4ea70793954ae317f3e38ee3672aa41e606c26 Mon Sep 17 00:00:00 2001 From: Shivam Kumar Date: Wed, 8 Jul 2026 16:31:03 +0530 Subject: [PATCH] Fix install_update leaving old release files behind The update process only copied/overwrote files from the new release into the install directory, but never removed files that existed in the old version and aren't part of the new one. This left stale files (e.g. a pre-src-restructure BackupManager.py) sitting alongside the new release. install_update now clears the install directory of everything except its own in-progress update artifacts (update_temp/, update.zip) before copying the new release in, so only the newest release's files/directories remain afterward. Added tests covering: old files being removed on update, and the empty-zip failure path leaving the install untouched. --- src/BackupManager.py | 47 +++++++++++++++---- src/test_BackupManager.py | 98 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 11 deletions(-) diff --git a/src/BackupManager.py b/src/BackupManager.py index d0dfda4..f2101a8 100644 --- a/src/BackupManager.py +++ b/src/BackupManager.py @@ -251,7 +251,13 @@ def check_for_update() -> Optional[Dict[str, Any]]: def install_update(release_info: Dict[str, Any]) -> bool: - """Installs a new release""" + """Installs a new release. + + Downloads the newest GitHub release, extracts it to a temp folder, + then replaces everything in the install directory with the newest + release's contents so that no files/directories from a previous + version are left behind. + """ try: import requests import zipfile @@ -268,6 +274,8 @@ def install_update(release_info: Dict[str, Any]) -> bool: f.write(response.content) extract_dir = os.path.join(script_dir, "update_temp") + if os.path.exists(extract_dir): + shutil.rmtree(extract_dir) os.makedirs(extract_dir, exist_ok=True) with zipfile.ZipFile(zip_path, 'r') as zip_ref: @@ -279,25 +287,45 @@ def install_update(release_info: Dict[str, Any]) -> bool: source_dir = os.path.join(extract_dir, extracted_contents[0]) else: log("Error: ZIP file is empty") + shutil.rmtree(extract_dir, ignore_errors=True) + os.remove(zip_path) return False - for item in os.listdir(source_dir): + # These are the update process's own working artifacts. They + # must survive the cleanup step below (we're still using them) + # and are removed separately once the copy is finished. + UPDATE_ARTIFACTS = {"update_temp", "update.zip"} + + # Remove everything currently in the install directory that + # isn't one of the update's own working artifacts, so that + # only the newest release's files/directories remain once we + # copy it in below. This is what fixes old-version files (e.g. + # a pre-src-restructure BackupManager.py) being left behind. + for item in os.listdir(script_dir): + if item in UPDATE_ARTIFACTS: + continue + + old_path = os.path.join(script_dir, item) + + try: + if os.path.isdir(old_path) and not os.path.islink(old_path): + shutil.rmtree(old_path) + else: + os.remove(old_path) + except OSError as e: + log(f"Error removing old file {old_path}: {e}") + # Copy the newest release's contents into the now-cleared + # install directory. + for item in os.listdir(source_dir): src = os.path.join(source_dir, item) dst = os.path.join(script_dir, item) if os.path.isdir(src): - - if os.path.exists(dst): - shutil.rmtree(dst) - shutil.copytree(src, dst) - else: - try: shutil.copy2(src, dst) - except OSError as e: log(f"Error copying {src} to {dst}: {e}") @@ -311,7 +339,6 @@ def install_update(release_info: Dict[str, Any]) -> bool: log(f"Update installation error: {e}") return False - def copy_file(src: str, dst_base: str, src_base: str, progress: Any) -> None: rel = os.path.relpath(src, src_base) diff --git a/src/test_BackupManager.py b/src/test_BackupManager.py index d9fa00b..c90a248 100644 --- a/src/test_BackupManager.py +++ b/src/test_BackupManager.py @@ -1,9 +1,13 @@ +import io +import os +import zipfile from unittest.mock import patch, MagicMock import BackupManager as bm import compression as compression_module + # ---------------------------- # Version tests # ---------------------------- @@ -175,4 +179,96 @@ def test_compress_to_zip_stores_wav_without_compression(tmp_path): assert mock_run.call_count == 2 commands = [call.args[0] for call in mock_run.call_args_list] assert commands[0][3] == "-mx=1" - assert commands[1][3] == "-mx=0" \ No newline at end of file + assert commands[1][3] == "-mx=0" + + +def _build_fake_release_zip(root_folder_name: str, files: dict) -> bytes: + """Builds an in-memory zip mimicking a GitHub zipball download: + a single top-level folder containing the release's files.""" + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + for rel_path, content in files.items(): + zf.writestr(f"{root_folder_name}/{rel_path}", content) + return buf.getvalue() + + +def test_install_update_removes_old_files_not_in_new_release(tmp_path, monkeypatch): + # Simulate the install directory as if BackupManager.py lives there + script_dir = tmp_path / "Backup-Manager" + script_dir.mkdir() + + # Old (pre-restructure) files that should NOT survive the update + (script_dir / "BackupManager.py").write_text("old root-level script", encoding="utf-8") + (script_dir / "old_helper.py").write_text("stale helper", encoding="utf-8") + old_pkg_dir = script_dir / "old_package" + old_pkg_dir.mkdir() + (old_pkg_dir / "module.py").write_text("stale package file", encoding="utf-8") + + # Point BackupManager's __file__ at our fake install directory so + # script_dir inside install_update resolves to our tmp_path fixture + monkeypatch.setattr(bm, "__file__", str(script_dir / "BackupManager.py")) + + # Build a fake GitHub zipball with the *new* release's structure + zip_bytes = _build_fake_release_zip( + "Dari0o-Backup-Manager-abcdef1", + { + "src/BackupManager.py": "new script content", + "README.md": "new readme", + }, + ) + + mock_response = MagicMock() + mock_response.content = zip_bytes + mock_response.raise_for_status = MagicMock() + + with patch("requests.get", return_value=mock_response): + result = bm.install_update( + {"version": "1.1.2", "download_url": "http://example.com/release.zip"} + ) + + assert result is True + + remaining = set(os.listdir(script_dir)) + + # Only the new release's top-level items should remain + assert remaining == {"src", "README.md"} + + # Old files/dirs must be gone + assert not (script_dir / "BackupManager.py").exists() + assert not (script_dir / "old_helper.py").exists() + assert not old_pkg_dir.exists() + + # New files must be present with correct content + assert (script_dir / "src" / "BackupManager.py").read_text(encoding="utf-8") == "new script content" + assert (script_dir / "README.md").read_text(encoding="utf-8") == "new readme" + + # Update's own working artifacts should be cleaned up afterward + assert not (script_dir / "update_temp").exists() + assert not (script_dir / "update.zip").exists() + + +def test_install_update_handles_empty_zip(tmp_path, monkeypatch): + script_dir = tmp_path / "Backup-Manager" + script_dir.mkdir() + (script_dir / "BackupManager.py").write_text("old script", encoding="utf-8") + + monkeypatch.setattr(bm, "__file__", str(script_dir / "BackupManager.py")) + + # Empty zip: no top-level entries at all + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w"): + pass + zip_bytes = buf.getvalue() + + mock_response = MagicMock() + mock_response.content = zip_bytes + mock_response.raise_for_status = MagicMock() + + with patch("requests.get", return_value=mock_response): + result = bm.install_update( + {"version": "1.1.2", "download_url": "http://example.com/release.zip"} + ) + + assert result is False + # Nothing should have been deleted since we bail out before cleanup + assert (script_dir / "BackupManager.py").read_text(encoding="utf-8") == "old script" \ No newline at end of file