Skip to content

Commit a702db9

Browse files
committed
fix: tolerate post-commit preset cleanup failures
Keep successful preset updates successful when backup or staging cleanup cannot remove a recovery artifact, while warning with the retained path. Assisted-by: GitHub Copilot (model: gpt-5.6-luna, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1782a274-fe10-44b1-b5b7-65a5f13cdc27
1 parent 4770e49 commit a702db9

2 files changed

Lines changed: 100 additions & 4 deletions

File tree

‎src/specify_cli/presets/__init__.py‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4383,12 +4383,27 @@ def _update_from_directory_locked(
43834383
os.rmdir(swap_token)
43844384
staging_dir = Path(f"{swap_token}.staging")
43854385
backup_dir = Path(f"{swap_token}.bak")
4386+
43864387
def remove_swap_path(path: Path) -> None:
43874388
if path.is_symlink():
43884389
path.unlink()
43894390
elif path.exists():
43904391
shutil.rmtree(path)
43914392

4393+
def remove_committed_swap_path(path: Path, artifact_kind: str) -> None:
4394+
try:
4395+
remove_swap_path(path)
4396+
except OSError as exc:
4397+
import warnings
4398+
4399+
warnings.warn(
4400+
f"Preset '{target_id}' was updated successfully, but its "
4401+
f"temporary {artifact_kind} path could not be removed: "
4402+
f"{path} ({exc}). Inspect and remove this recovery "
4403+
"artifact manually once it is safe to do so.",
4404+
stacklevel=2,
4405+
)
4406+
43924407
def ignore_staging_state(directory: str, names: List[str]) -> Set[str]:
43934408
directory_path = Path(directory)
43944409
return {
@@ -4470,8 +4485,8 @@ def ignore_staging_state(directory: str, names: List[str]) -> Set[str]:
44704485
# provenance until removal. Updating only replaces the source and
44714486
# metadata while retaining enough ownership data for a later
44724487
# removal to clean up artifacts left on disk.
4473-
remove_swap_path(backup_dir)
4474-
remove_swap_path(staging_dir)
4488+
remove_committed_swap_path(backup_dir, "backup")
4489+
remove_committed_swap_path(staging_dir, "staging")
44754490
return new_manifest, diff
44764491

44774492
primary_command_names = {
@@ -4810,8 +4825,8 @@ def _preset_has_constitution_layer(
48104825
stacklevel=2,
48114826
)
48124827
finally:
4813-
remove_swap_path(backup_dir)
4814-
remove_swap_path(staging_dir)
4828+
remove_committed_swap_path(backup_dir, "backup")
4829+
remove_committed_swap_path(staging_dir, "staging")
48154830
return new_manifest, diff
48164831

48174832
def update_from_archive(

‎tests/test_presets.py‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15617,6 +15617,51 @@ def test_update_preserves_priority_and_enabled_state(
1561715617
assert metadata["enabled"] is False
1561815618
assert (project_dir / ".specify/presets/test-pack/preset.yml").exists()
1561915619

15620+
@pytest.mark.parametrize("enabled", [False, True])
15621+
def test_post_commit_backup_cleanup_failure_warns_without_failing_update(
15622+
self, project_dir, pack_dir, monkeypatch, enabled
15623+
):
15624+
manager = PresetManager(project_dir)
15625+
manager.install_from_directory(pack_dir, "0.1.0")
15626+
if not enabled:
15627+
manager.registry.update("test-pack", {"enabled": False})
15628+
source = self._updated_source(pack_dir)
15629+
original_rmtree = shutil.rmtree
15630+
15631+
def fail_committed_backup(path, *args, **kwargs):
15632+
if Path(path).name.startswith(".test-pack.update-") and Path(
15633+
path
15634+
).name.endswith(".bak"):
15635+
raise PermissionError("simulated committed backup cleanup failure")
15636+
return original_rmtree(path, *args, **kwargs)
15637+
15638+
monkeypatch.setattr(shutil, "rmtree", fail_committed_backup)
15639+
15640+
with pytest.warns(UserWarning, match="updated successfully") as caught:
15641+
manifest, diff = manager.update_from_directory(
15642+
source, "0.1.0", pack_id="test-pack"
15643+
)
15644+
15645+
assert manifest.version == "2.0.0"
15646+
assert diff["added"]
15647+
metadata = manager.registry.get("test-pack")
15648+
assert metadata["version"] == "2.0.0"
15649+
assert metadata["enabled"] is enabled
15650+
assert "2.0.0" in (
15651+
project_dir / ".specify/presets/test-pack/preset.yml"
15652+
).read_text(encoding="utf-8")
15653+
backups = list(
15654+
(project_dir / ".specify/presets").glob(
15655+
".test-pack.update-*.bak"
15656+
)
15657+
)
15658+
assert len(backups) == 1
15659+
warning = str(caught[0].message)
15660+
assert "temporary backup path" in warning
15661+
assert str(backups[0]) in warning
15662+
assert "Inspect and remove this recovery artifact" in warning
15663+
original_rmtree(backups[0])
15664+
1562015665
def test_dry_run_does_not_modify_installation(self, project_dir, pack_dir):
1562115666
manager = PresetManager(project_dir)
1562215667
manager.install_from_directory(pack_dir, "0.1.0", priority=4)
@@ -16962,6 +17007,42 @@ def test_cli_bulk_confirms_once_for_multiple_presets(
1696217007
assert registry.get("test-pack")["version"] == "2.0.0"
1696317008
assert registry.get("second-pack")["version"] == "2.0.0"
1696417009

17010+
def test_cli_bulk_cleanup_warning_does_not_report_successful_update_as_failed(
17011+
self, project_dir, pack_dir, monkeypatch
17012+
):
17013+
from typer.testing import CliRunner
17014+
from specify_cli import app
17015+
17016+
self._two_actionable_packs(project_dir, pack_dir, monkeypatch)
17017+
original_rmtree = shutil.rmtree
17018+
17019+
def fail_first_committed_backup(path, *args, **kwargs):
17020+
name = Path(path).name
17021+
if name.startswith(".test-pack.update-") and name.endswith(".bak"):
17022+
raise PermissionError("simulated committed backup cleanup failure")
17023+
return original_rmtree(path, *args, **kwargs)
17024+
17025+
monkeypatch.setattr(shutil, "rmtree", fail_first_committed_backup)
17026+
17027+
with pytest.warns(UserWarning, match="updated successfully") as caught:
17028+
result = CliRunner().invoke(app, ["preset", "update"], input="y\n")
17029+
17030+
assert result.exit_code == 0, result.output
17031+
assert "test-pack: updated" in result.output
17032+
assert "second-pack: updated" in result.output
17033+
assert "failed" not in result.output
17034+
registry = PresetManager(project_dir).registry
17035+
assert registry.get("test-pack")["version"] == "2.0.0"
17036+
assert registry.get("second-pack")["version"] == "2.0.0"
17037+
backups = list(
17038+
(project_dir / ".specify/presets").glob(
17039+
".test-pack.update-*.bak"
17040+
)
17041+
)
17042+
assert len(backups) == 1
17043+
assert str(backups[0]) in str(caught[0].message)
17044+
original_rmtree(backups[0])
17045+
1696517046
def test_cli_all_flag_matches_bare_bulk_invocation(
1696617047
self, project_dir, pack_dir, monkeypatch
1696717048
):

0 commit comments

Comments
 (0)