From 5ca92cc58ed847a6c0a67c6bc463a65b0064f94c Mon Sep 17 00:00:00 2001 From: Upper9527 Date: Wed, 21 Oct 2026 11:07:42 +0800 Subject: [PATCH] fix: prune skill backups when staging creation fails --- skillclaw/skill_hub.py | 28 ++++--- tests/test_skill_hub_backups.py | 129 ++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 12 deletions(-) create mode 100644 tests/test_skill_hub_backups.py diff --git a/skillclaw/skill_hub.py b/skillclaw/skill_hub.py index 5e63534..dbe37e3 100644 --- a/skillclaw/skill_hub.py +++ b/skillclaw/skill_hub.py @@ -721,10 +721,12 @@ def _result( backup_dir="", ) - os.makedirs(staging_dir, exist_ok=True) resolved_targets: dict[str, str] = {} + staging_ready = False try: + os.makedirs(staging_dir, exist_ok=True) + staging_ready = True for name, rec in manifest.items(): category = str(rec.get("category", "general") or "general") target_dir = self._resolve_pull_target_dir( @@ -781,16 +783,18 @@ def _result( self._remove_duplicate_local_skill_dirs(name, dst_dir, local_dirs_by_name) except Exception as e: - logger.warning("[SkillHub] mirror pull failed, restoring backup: %s", e) - try: - if os.path.isdir(skills_dir): - shutil.rmtree(skills_dir) - shutil.copytree(backup_dir, skills_dir, symlinks=True) - restored_from_backup = True - logger.info("[SkillHub] local skills restored from backup: %s", backup_dir) - except Exception as restore_err: - logger.error("[SkillHub] backup restore failed: %s", restore_err) - self._prune_backups(backup_root, backup_prefix, keep=3) + logger.warning("[SkillHub] mirror pull failed: %s", e) + # A failed staging mkdir has not touched the local library. Do not + # delete it just to restore the identical backup on a failing disk. + if staging_ready: + try: + if os.path.isdir(skills_dir): + shutil.rmtree(skills_dir) + shutil.copytree(backup_dir, skills_dir, symlinks=True) + restored_from_backup = True + logger.info("[SkillHub] local skills restored from backup: %s", backup_dir) + except Exception as restore_err: + logger.error("[SkillHub] backup restore failed: %s", restore_err) return _result( downloaded=0, skipped=0, @@ -802,6 +806,7 @@ def _result( finally: if os.path.isdir(staging_dir): shutil.rmtree(staging_dir, ignore_errors=True) + self._prune_backups(backup_root, backup_prefix, keep=3) logger.info( "[SkillHub] pull complete: %d downloaded, %d skipped, %d deleted, %d total remote", @@ -810,7 +815,6 @@ def _result( deleted, len(manifest), ) - self._prune_backups(backup_root, backup_prefix, keep=3) return _result( downloaded=downloaded, skipped=skipped, diff --git a/tests/test_skill_hub_backups.py b/tests/test_skill_hub_backups.py new file mode 100644 index 0000000..34a43ba --- /dev/null +++ b/tests/test_skill_hub_backups.py @@ -0,0 +1,129 @@ +import os +import shutil +from pathlib import Path + +import pytest + +from skillclaw.skill_hub import SkillHub + + +@pytest.fixture +def mirror(tmp_path): + hub = SkillHub( + backend="local", + endpoint="", + bucket="", + access_key_id="", + secret_access_key="", + local_root=str(tmp_path / "shared"), + group_id="test", + user_alias="tester", + ) + source = tmp_path / "source" / "demo" + source.mkdir(parents=True) + text = "---\nname: demo\ndescription: Example\n---\nRemote content\n" + (source / "SKILL.md").write_text(text) + hub.push_skills(str(source.parent)) + skills = tmp_path / "skills" + skills.mkdir() + (skills / "local.txt").write_text("preserve me") + backups = tmp_path / ".skillclaw_backups" + backups.mkdir() + for i in range(5): + (backups / f"skills_20000101T00000{i}Z").mkdir() + (backups / "other_20000101T000000Z").mkdir() + return hub, skills, backups, text + + +def _dangling_link(skills): + link = skills / "_staging" / "broken" + link.parent.mkdir() + try: + link.symlink_to("../../missing-target") + except OSError as exc: + pytest.skip(f"Symlink creation unavailable: {exc}") + return link + + +def test_mirror_backup_preserves_dangling_symlink(mirror): + hub, skills, backups, text = mirror + link = _dangling_link(skills) + result = hub.pull_skills(str(skills)) + assert result["downloaded"] == 1 + assert (skills / "demo" / "SKILL.md").read_text() == text + saved_link = Path(result["backup_dir"]) / link.relative_to(skills) + assert saved_link.is_symlink() + assert os.readlink(saved_link) == "../../missing-target" + assert len(list(backups.glob("skills_*"))) == 3 + assert (backups / "other_20000101T000000Z").is_dir() + + +def test_repeated_backup_failure_removes_partial_copies(mirror, monkeypatch): + hub, skills, backups, _ = mirror + + def fail_copy(src, dst, **kwargs): + Path(dst).mkdir() + (Path(dst) / "partial").write_text("incomplete") + raise OSError("simulated disk full") + + monkeypatch.setattr(shutil, "copytree", fail_copy) + for _ in range(5): + result = hub.pull_skills(str(skills)) + assert result["downloaded"] == 0 + assert result["backup_dir"] == "" + assert (skills / "local.txt").read_text() == "preserve me" + assert not (skills / "demo").exists() + assert len(list(backups.glob("skills_*"))) == 3 + assert not list(backups.rglob("partial")) + + +@pytest.mark.parametrize("restore_fails", [False, True]) +def test_mirror_failure_prunes_and_preserves_recovery_backup(mirror, monkeypatch, restore_fails): + hub, skills, backups, _ = mirror + link = _dangling_link(skills) + + def fail_download(*args, **kwargs): + raise OSError("simulated upstream failure") + + monkeypatch.setattr(hub, "_download_skill_bundle", fail_download) + copytree = shutil.copytree + + def copy_or_fail_restore(src, dst, *args, **kwargs): + if restore_fails and Path(dst) == skills: + raise OSError("simulated restore failure") + return copytree(src, dst, *args, **kwargs) + + monkeypatch.setattr(shutil, "copytree", copy_or_fail_restore) + result = hub.pull_skills(str(skills)) + assert result["restored_from_backup"] is (not restore_fails) + assert result["downloaded"] == 0 + saved = Path(result["backup_dir"]) + assert (saved / "local.txt").read_text() == "preserve me" + assert (saved / link.relative_to(skills)).is_symlink() + assert len(list(backups.glob("skills_*"))) == 3 + assert not list(skills.parent.glob(".skillclaw_pull_stage_*")) + if not restore_fails: + assert (skills / "local.txt").read_text() == "preserve me" + assert link.is_symlink() + assert os.readlink(link) == "../../missing-target" + + +def test_staging_creation_failure_still_prunes_backups(mirror, monkeypatch): + hub, skills, backups, _ = mirror + makedirs = os.makedirs + original_inode = (skills / "local.txt").stat().st_ino + + def fail_staging(path, *args, **kwargs): + if Path(path).name.startswith(".skillclaw_pull_stage_"): + raise OSError("simulated staging directory failure") + return makedirs(path, *args, **kwargs) + + monkeypatch.setattr(os, "makedirs", fail_staging) + for _ in range(5): + result = hub.pull_skills(str(skills)) + assert result["downloaded"] == 0 + assert result["restored_from_backup"] is False + assert (skills / "local.txt").read_text() == "preserve me" + assert (skills / "local.txt").stat().st_ino == original_inode + assert len(list(backups.glob("skills_*"))) == 3 + assert Path(result["backup_dir"]).is_dir()