diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index c885d62443..5eba7b6672 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/src/specify_cli/bundles/primitives.py @@ -465,55 +465,113 @@ def install(self, component: ComponentRef) -> None: ) def refresh(self, component: ComponentRef) -> None: - # Preserve an existing step until we've validated we can perform refresh. - # For already-installed steps, keep a backup and restore it if the - # remove+reinstall path fails. + # Offline and not-yet-installed steps have nothing to roll back. + # Delegate to install and skip the backup path entirely. if not (self._allow_network and self.is_installed(component)): self.install(component) return + import copy + import json import shutil import tempfile - step_dir = self._registry.steps_dir / component.id - metadata = self._registry.get(component.id) - backup_dir = Path(tempfile.mkdtemp(prefix="speckit-step-refresh-")) / component.id + from ..workflows.catalog import StepRegistry + from ..workflows.step import command_remove + from ..workflows.step import installer as step_installer + + # Snapshot and removal share the lock ``step add`` / ``step remove`` + # already use. ``self.remove()`` and ``workflow_step_remove`` acquire + # that same lock, and ``_exclusive_project_lock`` blocks in + # ``fcntl.flock(LOCK_EX)`` on a new fd, so a nested acquire in this + # process deadlocks. Remove through ``_remove_step_locked`` instead. + # The catalog reinstall stays outside the lock. + backup_root: Path | None = None + keep_backup = False + metadata = None try: - if step_dir.exists(): - shutil.copytree(step_dir, backup_dir) - self.remove(component) + try: + with step_installer._step_install_transaction(self._root): + registry = StepRegistry(self._root) + entry = registry.get(component.id) + metadata = copy.deepcopy(entry) if entry is not None else None + backup_root = Path( + tempfile.mkdtemp(prefix="speckit-step-refresh-") + ) + backup_dir = backup_root / component.id + step_dir = registry.steps_dir / component.id + if step_dir.exists(): + shutil.copytree(step_dir, backup_dir) + with _chdir(self._root): + _delegate_command( + "remove", + f"step '{component.id}'", + lambda: command_remove._remove_step_locked( + self._root, component.id + ), + ) + except step_installer.StepInstallError as exc: + # Lock acquisition failed before any package or registry snapshot. + raise BundlerError( + f"Failed to refresh step '{component.id}': {exc}" + ) from exc + try: self.install(component) - except BundlerError: - if backup_dir.exists(): - shutil.copytree(backup_dir, step_dir, dirs_exist_ok=True) - # Re-read the registry: ``StepRegistry`` snapshots the file once - # in ``__init__`` (``self.data = self._load()``) and - # ``is_installed`` only consults that snapshot. ``self.remove()`` - # above has already deleted the entry from disk, but - # ``self._registry``'s snapshot still contains it -- so the - # guard was always False here and the restore never ran, in - # exactly the failure case it was written for. The step package - # came back but stayed unregistered: ``workflow step list`` - # stopped showing it and ``workflow step add`` then refused with - # "Step directory already exists". - from ..workflows.catalog import StepRegistry - - current = StepRegistry(self._root) - if metadata is not None and not current.is_installed(component.id): - # Restore the saved entry verbatim rather than via ``add()``, - # which would rewrite the metadata it is meant to roll back: - # this registry is freshly constructed *after* - # ``self.remove()`` deleted the entry, so ``add()`` sees no - # existing record and stamps ``installed_at`` with - # ``datetime.now()`` (it also overwrites ``updated_at`` - # unconditionally). ``workflow_step_remove`` bypasses - # ``add()`` for exactly this reason. - current.data["steps"][component.id] = metadata - current.save() + except BundlerError as original: + try: + with step_installer._step_install_transaction(self._root): + assert backup_root is not None + backup_dir = backup_root / component.id + current = StepRegistry(self._root) + # ``save()`` replaces the whole file. The document it + # writes is the on-disk ``steps`` object read after this + # load, plus this step's snapshotted entry when that + # object does not already contain the id. A later step + # operation's package and registry keys are left alone. + registry_path = current.registry_path + if registry_path.is_file(): + loaded = json.loads( + registry_path.read_text(encoding="utf-8") + ) + else: + loaded = None + if not isinstance(loaded, dict): + document = { + "schema_version": StepRegistry.SCHEMA_VERSION, + "steps": {}, + } + else: + document = loaded + if not isinstance(document.get("steps"), dict): + document["steps"] = {} + steps = document["steps"] + if component.id not in steps: + step_dir = current.steps_dir / component.id + if step_dir.exists(): + shutil.rmtree(step_dir) + if backup_dir.exists(): + shutil.copytree(backup_dir, step_dir) + if metadata is not None: + # Insert the snapshot verbatim. ``StepRegistry.add`` + # would rewrite ``installed_at`` and ``updated_at``. + steps[component.id] = metadata + current.data = document + current.save() + except Exception as restore_exc: # noqa: BLE001 + # The install error is what the caller handles. A failed + # copy-back or registry write is recorded on it, and the + # temp backup stays on disk for recovery. + keep_backup = True + original.add_note( + f"Could not restore step '{component.id}' from backup " + f"'{backup_root}': {restore_exc}" + ) + raise original from None raise finally: - shutil.rmtree(backup_dir.parent, ignore_errors=True) + if backup_root is not None and not keep_backup: + shutil.rmtree(backup_root, ignore_errors=True) def remove(self, component: ComponentRef) -> None: from .. import workflow_step_remove diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index 31c729ab65..928c9d3203 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -632,6 +632,7 @@ def test_step_refresh_restores_registry_entry_when_reinstall_fails( seeded = StepRegistry(tmp_path).get("my-step") assert StepRegistry(tmp_path).is_installed("my-step") + package_text = (steps_dir / "my-step" / "step.yml").read_text(encoding="utf-8") # Removal succeeds (real code path); only the re-install fails, which is # what a catalog 404 / size-limit / type_key mismatch produces. @@ -640,10 +641,55 @@ def _boom(step_id, *args, **kwargs): monkeypatch.setattr(specify_cli, "workflow_step_add", _boom) + # The backup copy and the locked remove must both run while the step lock + # is held. A nested ``workflow_step_remove`` would take the same flock and + # hang, so the stand-in refuses a second hold before calling the real lock. + import contextlib + import shutil + + import specify_cli.workflows.step.command_remove as command_remove + import specify_cli.workflows.step.installer as step_installer + + hold = {"depth": 0} + backup_copy_depths: list[int] = [] + remove_depths: list[int] = [] + real_txn = step_installer._step_install_transaction + real_copytree = shutil.copytree + real_remove = command_remove._remove_step_locked + + @contextlib.contextmanager + def _tracking_transaction(project_root): + if hold["depth"] >= 1: + raise AssertionError("nested _step_install_transaction") + hold["depth"] += 1 + try: + with real_txn(project_root): + yield + finally: + hold["depth"] -= 1 + + def _tracking_copytree(src, dst, *args, **kwargs): + if "speckit-step-refresh-" in str(dst): + backup_copy_depths.append(hold["depth"]) + return real_copytree(src, dst, *args, **kwargs) + + def _tracking_remove(project_root, step_id): + remove_depths.append(hold["depth"]) + return real_remove(project_root, step_id) + + monkeypatch.setattr( + step_installer, "_step_install_transaction", _tracking_transaction + ) + monkeypatch.setattr(shutil, "copytree", _tracking_copytree) + monkeypatch.setattr(command_remove, "_remove_step_locked", _tracking_remove) + manager = primitive_manager("steps", tmp_path, allow_network=True) with pytest.raises(BundlerError): manager.refresh(_component("steps", "my-step")) + assert backup_copy_depths and min(backup_copy_depths) >= 1 + assert remove_depths and min(remove_depths) >= 1 + # Read the registry fresh from disk — the point of the fix. restored = StepRegistry(tmp_path) assert restored.is_installed("my-step"), ( @@ -652,3 +698,251 @@ def _boom(step_id, *args, **kwargs): # A rollback must be a rollback: the entry comes back byte-for-byte, not # re-registered with fresh ``installed_at`` / ``updated_at`` stamps. assert restored.get("my-step") == seeded + assert (steps_dir / "my-step" / "step.yml").read_text(encoding="utf-8") == ( + package_text + ) + assert (steps_dir / "my-step" / "__init__.py").read_text(encoding="utf-8") == "" + + +def _seed_refresh_step(root: Path) -> tuple[Path, dict]: + """Install ``my-step`` on disk the way a previous ``step add`` would have.""" + import json + + from specify_cli.workflows.catalog import StepRegistry + + steps_dir = root / ".specify" / "workflows" / "steps" + package = steps_dir / "my-step" + package.mkdir(parents=True) + (package / "step.yml").write_text( + "step:\n type_key: my-step\n", encoding="utf-8" + ) + (package / "__init__.py").write_text("", encoding="utf-8") + entry = { + "name": "My Step", + "version": "1.0.0", + "type_key": "my-step", + "installed_at": "2020-01-01T00:00:00+00:00", + "updated_at": "2020-02-02T00:00:00+00:00", + } + (steps_dir / StepRegistry.REGISTRY_FILE).write_text( + json.dumps( + {"schema_version": "1.0", "steps": {"my-step": entry}} + ), + encoding="utf-8", + ) + return steps_dir, entry + + +def test_step_refresh_keeps_a_later_commit_when_reinstall_fails( + tmp_path: Path, monkeypatch +): + """A step operation that lands before rollback must survive the failure. + + The pre-fix restore copied the backup with ``dirs_exist_ok=True``, which + overwrote a package a later ``step add`` had already committed. + """ + import json + + import specify_cli + from specify_cli.workflows.catalog import StepRegistry + + steps_dir, _entry = _seed_refresh_step(tmp_path) + newer_step_yml = "step:\n type_key: my-step\n version: 2.0.0\n" + committed = { + "my-step": { + "name": "My Step", + "version": "2.0.0", + "type_key": "my-step", + "installed_at": "2024-03-03T00:00:00+00:00", + "updated_at": "2024-04-04T00:00:00+00:00", + }, + "other-step": { + "name": "Other Step", + "version": "1.0.0", + "installed_at": "2024-05-05T00:00:00+00:00", + "updated_at": "2024-05-05T00:00:00+00:00", + }, + } + + def _boom(step_id, *args, **kwargs): + package = steps_dir / step_id + package.mkdir(parents=True, exist_ok=True) + (package / "step.yml").write_text(newer_step_yml, encoding="utf-8") + (steps_dir / StepRegistry.REGISTRY_FILE).write_text( + json.dumps({"schema_version": "1.0", "steps": committed}), + encoding="utf-8", + ) + raise BundlerError(f"Failed to install step '{step_id}'.") + + monkeypatch.setattr(specify_cli, "workflow_step_add", _boom) + + manager = primitive_manager("steps", tmp_path, allow_network=True) + with pytest.raises(BundlerError, match="Failed to install step 'my-step'"): + manager.refresh(_component("steps", "my-step")) + + assert (steps_dir / "my-step" / "step.yml").read_text(encoding="utf-8") == ( + newer_step_yml + ) + assert StepRegistry(tmp_path).data["steps"] == committed + + +def test_step_refresh_rollback_keeps_registry_keys_written_after_load( + tmp_path: Path, monkeypatch +): + """Rollback must save the on-disk steps object, not the loaded snapshot. + + After ``StepRegistry`` loads, a key written to ``step-registry.json`` + without being inserted into that instance has to survive ``save()``. + """ + import json + + import specify_cli + from specify_cli.workflows.catalog import StepRegistry + + steps_dir, entry = _seed_refresh_step(tmp_path) + registry_path = steps_dir / StepRegistry.REGISTRY_FILE + other_step = { + "name": "Other Step", + "version": "3.0.0", + "installed_at": "2024-06-06T00:00:00+00:00", + "updated_at": "2024-06-06T00:00:00+00:00", + } + real_load = StepRegistry._load + injected = {"done": False} + + def _load(self): + data = real_load(self) + steps = data.get("steps") + if ( + not injected["done"] + and isinstance(steps, dict) + and "my-step" not in steps + and self.registry_path.is_file() + ): + injected["done"] = True + disk = json.loads(self.registry_path.read_text(encoding="utf-8")) + disk["steps"]["other-step"] = other_step + self.registry_path.write_text(json.dumps(disk), encoding="utf-8") + return data + + def _boom(step_id, *args, **kwargs): + raise BundlerError(f"Failed to install step '{step_id}'.") + + monkeypatch.setattr(StepRegistry, "_load", _load) + monkeypatch.setattr(specify_cli, "workflow_step_add", _boom) + + manager = primitive_manager("steps", tmp_path, allow_network=True) + with pytest.raises(BundlerError, match="Failed to install step 'my-step'"): + manager.refresh(_component("steps", "my-step")) + + assert injected["done"] + saved = json.loads(registry_path.read_text(encoding="utf-8")) + assert saved["steps"]["my-step"] == entry + assert saved["steps"]["other-step"] == other_step + + +@pytest.mark.parametrize("failure", ["copy", "save"]) +def test_step_refresh_notes_restoration_failure_and_keeps_backup( + tmp_path: Path, monkeypatch, failure: str +): + """A failed copy-back or registry write stays on the original install error.""" + import shutil + + import specify_cli + from specify_cli.workflows.catalog import StepRegistry, StepValidationError + + steps_dir, _entry = _seed_refresh_step(tmp_path) + original_step_yml = (steps_dir / "my-step" / "step.yml").read_text( + encoding="utf-8" + ) + + def _boom(step_id, *args, **kwargs): + raise BundlerError(f"Failed to install step '{step_id}'.") + + monkeypatch.setattr(specify_cli, "workflow_step_add", _boom) + + if failure == "copy": + real_copytree = shutil.copytree + + def _copytree(src, dst, *args, **kwargs): + if "speckit-step-refresh-" in str(src): + raise OSError("restore copy failed") + return real_copytree(src, dst, *args, **kwargs) + + monkeypatch.setattr(shutil, "copytree", _copytree) + failure_text = "restore copy failed" + else: + real_save = StepRegistry.save + armed = {"on": False} + + def _boom_then_arm(step_id, *args, **kwargs): + armed["on"] = True + raise BundlerError(f"Failed to install step '{step_id}'.") + + def _save(self): + if armed["on"]: + raise StepValidationError("registry write failed") + return real_save(self) + + monkeypatch.setattr(specify_cli, "workflow_step_add", _boom_then_arm) + monkeypatch.setattr(StepRegistry, "save", _save) + failure_text = "registry write failed" + + manager = primitive_manager("steps", tmp_path, allow_network=True) + with pytest.raises(BundlerError) as caught: + manager.refresh(_component("steps", "my-step")) + + assert str(caught.value) == "Failed to install step 'my-step'." + note = caught.value.__notes__[0] + assert failure_text in note + assert "speckit-step-refresh-" in note + backup_root = Path(note.split("from backup '", 1)[1].split("'", 1)[0]) + try: + assert backup_root.is_dir() + assert (backup_root / "my-step" / "step.yml").read_text( + encoding="utf-8" + ) == original_step_yml + finally: + shutil.rmtree(backup_root, ignore_errors=True) + + +def test_step_refresh_skips_backup_when_offline_or_not_installed( + tmp_path: Path, monkeypatch +): + """Offline and not-installed refresh delegate to install with no backup.""" + import tempfile + + import specify_cli + import specify_cli.workflows.step.installer as step_installer + + calls: list[tuple[str, Path]] = [] + + def _add(step_id: str) -> None: + calls.append((step_id, Path.cwd())) + + def _forbid_step_lock(*_args, **_kwargs): + raise AssertionError("step refresh took the step lock") + + real_mkdtemp = tempfile.mkdtemp + + def _mkdtemp(*args, **kwargs): + prefix = kwargs.get("prefix", args[0] if args else "") + if str(prefix).startswith("speckit-step-refresh-"): + raise AssertionError("step refresh took the backup path") + return real_mkdtemp(*args, **kwargs) + + monkeypatch.setattr(specify_cli, "workflow_step_add", _add) + monkeypatch.setattr(step_installer, "_step_install_transaction", _forbid_step_lock) + monkeypatch.setattr(tempfile, "mkdtemp", _mkdtemp) + + missing = primitive_manager("steps", tmp_path, allow_network=True) + missing.refresh(_component("steps", "missing-step")) + assert calls == [("missing-step", tmp_path)] + + _seed_refresh_step(tmp_path) + offline = primitive_manager("steps", tmp_path, allow_network=False) + with pytest.raises( + BundlerError, match="refreshing this component requires network access" + ): + offline.refresh(_component("steps", "my-step")) + assert calls == [("missing-step", tmp_path)]