diff --git a/docs/src/content/docs/troubleshooting/install-failures.md b/docs/src/content/docs/troubleshooting/install-failures.md index c14f9aeaa..7ea9e2b2c 100644 --- a/docs/src/content/docs/troubleshooting/install-failures.md +++ b/docs/src/content/docs/troubleshooting/install-failures.md @@ -220,6 +220,8 @@ apm install The cache short-circuits already-downloaded packages and the integrate phase overwrites partially-deployed files. +If resolution rejects a cyclic dependency graph, fix the package manifests and run `apm install` again. APM rolls back only the package snapshots staged by the rejected resolution, so no manual `apm_modules/` deletion is required. + If files in `apm_modules/` or under target harness directories look corrupt, force a fresh deploy by combining cache bypass with overwrite: ```bash diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index b2b3393bb..4416115d3 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -24,11 +24,13 @@ from typing import TYPE_CHECKING from apm_cli.install.helpers.ref_seed import seed_ref_resolver_from_lockfile +from apm_cli.install.resolution_staging import transactional_resolution from apm_cli.models.apm_package import GitReferenceType, ResolvedReference from apm_cli.utils.short_sha import format_short_sha if TYPE_CHECKING: from apm_cli.install.context import InstallContext + from apm_cli.install.resolution_staging import ResolutionStagingSession from apm_cli.models.dependency.reference import DependencyReference _logger = logging.getLogger(__name__) @@ -374,13 +376,9 @@ def _fail_on_resolution_errors(ctx: InstallContext, dependency_graph) -> None: raise RuntimeError(f"Dependency resolution failed: {joined_errors}") -def _resolve_dependencies(ctx: InstallContext) -> None: - """Run ``APMDependencyResolver``, handle errors; populate ``ctx.deps_to_install`` and ``ctx.dependency_graph``. - - Also wires the download callback (which handles transitive package fetching), - builds ``ctx.dep_base_dirs``, writes ancillary state to ``ctx``, and cleans up - the shared clone cache. - """ +@transactional_resolution +def _resolve_dependencies(ctx: InstallContext, staging_session: ResolutionStagingSession) -> None: + """Resolve dependencies and populate the resolution fields on ``ctx``.""" import threading as _threading from apm_cli.core.scope import InstallScope @@ -499,6 +497,7 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): ) if not _force_semver_resolve: return install_path + staging_session.prepare_path(install_path) # F1 (#1116): surface a heartbeat BEFORE the network/copy work so # users see the install advancing past silent transitive lookups. # Under F7's parallel BFS this callback may run on a worker diff --git a/src/apm_cli/install/resolution_staging.py b/src/apm_cli/install/resolution_staging.py new file mode 100644 index 000000000..3c2aff031 --- /dev/null +++ b/src/apm_cli/install/resolution_staging.py @@ -0,0 +1,81 @@ +"""Rollback-scoped staging for dependency resolution writes.""" + +from __future__ import annotations + +import threading +import uuid +from collections.abc import Callable +from functools import wraps +from pathlib import Path +from typing import Any + +from apm_cli.utils.path_security import ensure_path_within, safe_rmtree + + +class ResolutionStagingSession: + """Track paths mutated during resolution and restore them on failure.""" + + def __init__(self, apm_modules_dir: Path) -> None: + """Create an empty staging session rooted below ``apm_modules``.""" + self._modules_dir = apm_modules_dir + self._staging_root = apm_modules_dir / ".apm-resolution-staging" / uuid.uuid4().hex + self._backups: dict[Path, Path | None] = {} + self._lock = threading.Lock() + + def prepare_path(self, path: Path) -> None: + """Record *path* and preserve its pre-resolution contents if present.""" + resolved = ensure_path_within(path, self._modules_dir) + with self._lock: + if resolved in self._backups: + return + backup: Path | None = None + if resolved.exists(): + resolved_base = ensure_path_within(self._modules_dir, self._modules_dir) + relative = resolved.relative_to(resolved_base) + backup = self._staging_root / relative + backup.parent.mkdir(parents=True, exist_ok=True) + resolved.replace(backup) + self._backups[resolved] = backup + + def commit(self) -> None: + """Discard preserved pre-resolution contents after successful validation.""" + self._remove_staging_root() + self._backups.clear() + + def rollback(self) -> None: + """Remove session-created paths and restore every replaced path.""" + with self._lock: + for path, backup in reversed(self._backups.items()): + if path.exists(): + safe_rmtree(path, self._modules_dir) + if backup is not None and backup.exists(): + path.parent.mkdir(parents=True, exist_ok=True) + backup.replace(path) + self._remove_staging_root() + self._backups.clear() + + def _remove_staging_root(self) -> None: + if self._staging_root.exists(): + safe_rmtree(self._staging_root, self._modules_dir) + staging_parent = self._staging_root.parent + if staging_parent.exists() and not any(staging_parent.iterdir()): + staging_parent.rmdir() + + +ResolveFunction = Callable[[Any, ResolutionStagingSession], None] + + +def transactional_resolution(resolve: ResolveFunction) -> Callable[[Any], None]: + """Wrap a resolve operation in a rollback-scoped staging session.""" + + @wraps(resolve) + def wrapped(ctx: Any) -> None: + session = ResolutionStagingSession(ctx.apm_modules_dir) + try: + resolve(ctx, session) + except BaseException: + session.rollback() + raise + session.commit() + + return wrapped diff --git a/tests/unit/install/test_resolution_transactionality.py b/tests/unit/install/test_resolution_transactionality.py new file mode 100644 index 000000000..323fa6dc6 --- /dev/null +++ b/tests/unit/install/test_resolution_transactionality.py @@ -0,0 +1,97 @@ +"""Regression coverage for transactional dependency resolution.""" + +from __future__ import annotations + +from pathlib import Path + +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.install.resolution_staging import ResolutionStagingSession +from apm_cli.models.apm_package import clear_apm_yml_cache + + +def _write_package(path: Path, name: str, dependency: str | None = None) -> None: + """Write a local package and one Copilot instruction.""" + path.mkdir(parents=True, exist_ok=True) + dependency_block = "" + if dependency is not None: + dependency_block = f"dependencies:\n apm:\n - path: {dependency}\n" + (path / "apm.yml").write_text( + f"name: {name}\nversion: 1.0.0\n{dependency_block}", + encoding="ascii", + ) + instructions = path / ".apm" / "instructions" + instructions.mkdir(parents=True, exist_ok=True) + (instructions / f"{name}.instructions.md").write_text( + f"# {name}\n", + encoding="ascii", + ) + + +def test_resolution_rollback_restores_replaced_cache_path(tmp_path: Path) -> None: + """Rollback must restore a valid cache entry replaced by this attempt.""" + modules = tmp_path / "apm_modules" + package = modules / "_local" / "package-a" + package.mkdir(parents=True) + marker = package / "marker.txt" + marker.write_text("original", encoding="ascii") + session = ResolutionStagingSession(modules) + + session.prepare_path(package) + package.mkdir(parents=True) + marker.write_text("replacement", encoding="ascii") + session.rollback() + + assert marker.read_text(encoding="ascii") == "original" + assert not (modules / ".apm-resolution-staging").exists() + + +def test_corrected_local_cycle_resumes_without_manual_cache_deletion( + tmp_path: Path, + monkeypatch, +) -> None: + """A rejected local cycle must not leave snapshots that poison the retry.""" + workspace = tmp_path / "workspace" + project = workspace / "project" + package_a = workspace / "package-a" + package_b = workspace / "package-b" + home = workspace / "home" + project.mkdir(parents=True) + home.mkdir() + (project / ".github").mkdir() + (project / "apm.yml").write_text( + "name: consumer\nversion: 1.0.0\ndependencies:\n apm:\n - path: ../package-a\n", + encoding="ascii", + ) + _write_package(package_a, "package-a", "../package-b") + _write_package(package_b, "package-b", "../package-a") + + monkeypatch.chdir(project) + monkeypatch.setenv("HOME", str(home)) + runner = CliRunner() + + rejected = runner.invoke( + cli, + ["install", "--target", "copilot", "--verbose"], + catch_exceptions=True, + ) + + assert rejected.exit_code != 0, rejected.output + assert "Circular dependencies detected" in rejected.output + modules = project / "apm_modules" + assert not (modules / "_local" / "package-a").exists() + assert not (modules / "_local" / "package-b").exists() + assert not (project / "apm.lock.yaml").exists() + + _write_package(package_b, "package-b") + clear_apm_yml_cache() + resumed = runner.invoke( + cli, + ["install", "--target", "copilot", "--verbose"], + catch_exceptions=True, + ) + + assert resumed.exit_code == 0, resumed.output + assert (project / "apm.lock.yaml").is_file() + assert (project / ".github" / "instructions" / "package-a.instructions.md").is_file()