diff --git a/agent/README.md b/agent/README.md index 5feb0f5e..1753271e 100644 --- a/agent/README.md +++ b/agent/README.md @@ -125,6 +125,7 @@ The `run.sh` script overrides the container's default CMD to run `python /app/sr | `DRY_RUN` | No | | Set to `1` to validate config and print the prompt without running the agent | | `ANTHROPIC_DEFAULT_HAIKU_MODEL` | No | `anthropic.claude-haiku-4-5-20251001-v1:0` | Bedrock model ID for the pre-flight safety check (see below) | | `NUDGES_TABLE_NAME` | No | | **Phase 2.** DynamoDB table for mid-task user nudges (`` XML blocks injected between turns). If unset, the agent runs without nudge support — `nudge_reader.read_pending()` returns `[]` and logs a WARN once. Set automatically by the CDK stack on both AgentCore runtimes. | +| `DEPENDENCY_CACHE_DIR` | No | `/cache` | **Warm dependency cache (ABCA-691).** Directory (an EFS mount on the ECS build task def) where `dependency_cache.py` persists derived dependency artifacts — `node_modules` and the target-repo `.venv` — keyed by lockfile hash, so a task on unchanged lockfiles skips the cold `yarn install` / `uv sync`. When the path is absent or not writable (AgentCore, local dev, tests) the cache is disabled and every task installs cold. Set automatically on the ECS build task def by the CDK stack. | **Bedrock model access (main model):** Configuring `ANTHROPIC_MODEL` and IAM credentials is not enough. Your AWS account must be able to **invoke** that model in Amazon Bedrock: follow [Request access to models](https://docs.aws.amazon.com/bedrock/latest/userguide/model-access.html) (Marketplace permissions on first use, Anthropic first-time use where required, valid payment method for Marketplace-backed models). Use an inference profile ID such as `us.anthropic.claude-sonnet-4-6` when Bedrock requires it. If the CLI stops with a message that the model is not available on your Bedrock deployment, fix model access in the console or switch `ANTHROPIC_MODEL` to an entitled profile, then retry. diff --git a/agent/src/dependency_cache.py b/agent/src/dependency_cache.py new file mode 100644 index 00000000..b27e267a --- /dev/null +++ b/agent/src/dependency_cache.py @@ -0,0 +1,282 @@ +"""Warm dependency cache across ECS tasks (ABCA-691). + +Each ECS agent task otherwise pays a full COLD dependency install (``yarn +install`` + ``uv sync``, ~3-5 min) even when the target repo's dependencies +have not changed since the last task. This module persists the *derived* +dependency artifacts — ``node_modules`` and the target-repo ``.venv`` — on a +shared filesystem (an EFS volume mounted at ``/cache`` on the build task def) +so a subsequent task on the SAME lockfiles restores them from local disk +instead of re-downloading from the network. + +Correctness invariant (non-negotiable) +--------------------------------------- +Every cache entry is keyed on the **lockfile hash**, NEVER the commit SHA: + +* ``node_modules`` → ``sha256(yarn.lock)`` +* ``.venv`` → ``sha256(uv.lock)`` + +If the target repo's trunk bumped a dependency, the lockfile bytes differ, the +hash differs, and the lookup MISSES → a full reinstall. It is therefore +impossible to build against stale deps: a hit can only occur when the exact +lockfile that produced the cached artifact is present again. If trunk changed +only source (lockfile unchanged), reuse is safe. The git clone itself is never +cached — it stays fresh per task (source of truth); only these two derived +artifacts are reused. + +Concurrency + resilience +------------------------ +Many tasks run at once against the same shared volume. Cache entries are +published with a write-to-temp-then-atomic-rename so a reader never observes a +half-written entry, and two tasks publishing the same key race harmlessly (the +first ``rename`` wins; the loser discards its temp). Every operation here is +**best-effort**: a missing, corrupt, or unreadable entry — or an absent/unwritable +cache root (e.g. the AgentCore substrate, local dev, or tests) — falls back to a +cold install and NEVER fails the task. A restored artifact is always followed by +the repo's normal validating install, so a cache hit produces the identical +``node_modules`` a cold install would. +""" + +import hashlib +import os +import shutil +import uuid + +from shell import log + +# Where the shared cache volume is mounted in the build task def (CDK EFS access +# point → ``/cache``). Overridable via env for local dev / tests. +DEFAULT_CACHE_ROOT = "/cache" + +# Cache namespaces (subdirectories under the cache root). Each artifact kind +# gets its own tree so ``node_modules`` and ``.venv`` digests can never collide. +CACHE_KIND_NODE_MODULES = "node_modules" +CACHE_KIND_VENV = "venv" + +# Read the lockfile in bounded chunks so a large lockfile never balloons memory. +_HASH_CHUNK_BYTES = 1 << 20 # 1 MiB + +# Directories never worth scanning for nested ``uv.lock`` files — vendored trees +# and build outputs are huge and any lockfile inside them is not a project root. +_UV_LOCK_SKIP_DIRS = frozenset( + {".git", "node_modules", ".venv", "cdk.out", "dist", "build", ".tox", "__pycache__"} +) + + +def cache_root() -> str | None: + """Return the writable cache root, or ``None`` when caching is unavailable. + + The cache is available only when the mount point exists AND is writable — + i.e. the EFS access point is mounted (build task def). On any substrate + without it (AgentCore runtime, local dev, unit tests) this returns ``None`` + and every caller degrades to a plain cold install. + """ + root = os.environ.get("DEPENDENCY_CACHE_DIR", DEFAULT_CACHE_ROOT) + if not root: + return None + if not os.path.isdir(root) or not os.access(root, os.W_OK): + return None + return root + + +def hash_lockfile(path: str) -> str | None: + """Return the hex ``sha256`` of the lockfile at *path*, or ``None``. + + ``None`` means the lockfile is absent or unreadable — there is nothing to + key a cache entry on, so the caller skips caching for that artifact (a repo + with no ``yarn.lock`` simply has no node_modules cache). Never raises. + """ + try: + digest = hashlib.sha256() + with open(path, "rb") as handle: + while True: + chunk = handle.read(_HASH_CHUNK_BYTES) + if not chunk: + break + digest.update(chunk) + return digest.hexdigest() + except OSError: + return None + + +def _entry_path(root: str, kind: str, digest: str) -> str: + """Absolute path of the cache entry directory for ``(kind, digest)``.""" + return os.path.join(root, kind, digest) + + +def _safe_rmtree(path: str | None) -> None: + """Best-effort recursive delete used to clean up temp dirs. Never raises.""" + if not path: + return + shutil.rmtree(path, ignore_errors=True) + + +def restore_artifact(root: str, kind: str, digest: str, dest: str) -> bool: + """Restore a cached artifact directory into *dest*. Return ``True`` on HIT. + + Copies the cache entry ``//`` into *dest* so the task + gets its OWN copy — the shared entry is treated read-only, so a subsequent + install that mutates *dest* can never corrupt the cache for concurrent + tasks. The copy lands via a per-task temp dir then an atomic rename so a + crash mid-copy leaves no partial *dest*. + + Returns ``False`` (cold install) when the entry is absent, *dest* already + exists (nothing to restore over), or anything goes wrong (corrupt/partial + entry, permission error) — best-effort read, never fatal. + """ + entry = _entry_path(root, kind, digest) + tmp: str | None = None + try: + if not os.path.isdir(entry): + return False + # Never clobber an existing dest: a present artifact means the caller + # already has it (e.g. a venv baked into the image). Cold-path it. + if os.path.exists(dest): + return False + parent = os.path.dirname(os.path.abspath(dest)) or "." + os.makedirs(parent, exist_ok=True) + # Temp sits next to dest (same filesystem as the clone) so the final + # rename into place is atomic. + tmp = os.path.join(parent, f".{os.path.basename(dest)}.cache-restore-{uuid.uuid4().hex}") + shutil.copytree(entry, tmp, symlinks=True) + os.rename(tmp, dest) + tmp = None + return True + except OSError as exc: + # Corrupt/partial entry or a race that created dest first. Clean up and + # fall back to a cold install — a bad cache entry must never fail a task. + log("WARN", f"dependency-cache restore miss ({kind}): {type(exc).__name__}: {exc}") + _safe_rmtree(tmp) + return False + + +def populate_artifact(root: str, kind: str, digest: str, source: str) -> bool: + """Publish *source* into the cache under ``(kind, digest)``. Atomic. + + Copies *source* into a unique temp directory ON THE CACHE FILESYSTEM, then + ``rename``s it to the final entry path. The rename is atomic, so a reader + observes either no entry or a fully-formed one — never a half-written tree. + When two tasks publish the same key concurrently the first rename wins and + the loser (rename onto a non-empty dir fails) discards its temp; the winning + entry is equally valid because an identical lockfile hash means identical + dependencies. + + Returns ``True`` when this call published a new entry; ``False`` when an + entry already existed (hit or concurrent winner), *source* is absent, or + anything went wrong. Best-effort — never raises. + """ + entry = _entry_path(root, kind, digest) + tmp: str | None = None + try: + # Already cached (a hit, or a concurrent task already won). Nothing to do. + if os.path.isdir(entry): + return False + if not os.path.isdir(source): + return False + kind_dir = os.path.join(root, kind) + os.makedirs(kind_dir, exist_ok=True) + # Temp MUST live on the same filesystem as the final entry (the cache + # volume) for the rename to be atomic — so build it under kind_dir. + tmp = os.path.join(kind_dir, f".tmp-populate-{uuid.uuid4().hex}") + shutil.copytree(source, tmp, symlinks=True) + try: + os.rename(tmp, entry) + tmp = None + return True + except OSError: + # Lost the publish race: another task's rename created the (non-empty) + # entry first, so ours fails. Its entry is valid — discard our temp. + _safe_rmtree(tmp) + tmp = None + return False + except OSError as exc: + log("WARN", f"dependency-cache populate skipped ({kind}): {type(exc).__name__}: {exc}") + _safe_rmtree(tmp) + return False + + +def _find_uv_locks(repo_dir: str) -> list[str]: + """Return every ``uv.lock`` under *repo_dir*, skipping vendored/build trees. + + A monorepo may have several Python project roots (ABCA keeps one at + ``agent/uv.lock``); each drives its own sibling ``.venv``. + """ + locks: list[str] = [] + for dirpath, dirnames, filenames in os.walk(repo_dir): + dirnames[:] = [d for d in dirnames if d not in _UV_LOCK_SKIP_DIRS] + if "uv.lock" in filenames: + locks.append(os.path.join(dirpath, "uv.lock")) + return locks + + +def discover_artifacts(repo_dir: str) -> list[tuple[str, str, str]]: + """Discover cacheable derived artifacts in the clone. + + Returns a list of ``(kind, lockfile_path, artifact_path)`` tuples: + + * the root ``yarn.lock`` → ``node_modules`` (Yarn workspaces install every + workspace's deps into the single root ``node_modules``), and + * each ``uv.lock`` → its sibling ``.venv``. + + Only pairs whose lockfile exists are returned; the artifact dir need not + exist yet (it won't on the restore pass, before install runs). + """ + plan: list[tuple[str, str, str]] = [] + yarn_lock = os.path.join(repo_dir, "yarn.lock") + if os.path.isfile(yarn_lock): + plan.append((CACHE_KIND_NODE_MODULES, yarn_lock, os.path.join(repo_dir, "node_modules"))) + for uv_lock in _find_uv_locks(repo_dir): + venv = os.path.join(os.path.dirname(uv_lock), ".venv") + plan.append((CACHE_KIND_VENV, uv_lock, venv)) + return plan + + +def restore_dependency_cache(repo_dir: str, notes: list[str]) -> None: + """Restore warm dependency artifacts into the clone before install runs. + + Best-effort and idempotent: for each cacheable ``(lockfile, artifact)`` pair + whose lockfile hashes to a present cache entry, copy the artifact into the + clone (a HIT) so the repo's subsequent ``yarn install`` / ``uv sync`` finds + the packages already present and validates instead of cold-downloading. A + miss (or no cache volume) leaves the clone untouched → a normal cold install. + Records a human-readable note per artifact for the PR / setup log. + """ + root = cache_root() + if root is None: + log("SETUP", "Dependency cache unavailable (no writable /cache) — cold install") + notes.append("Dependency cache: unavailable (cold install)") + return + for kind, lockfile, artifact in discover_artifacts(repo_dir): + digest = hash_lockfile(lockfile) + if digest is None: + continue + if restore_artifact(root, kind, digest, artifact): + msg = f"cache HIT for {kind} (key {digest[:12]}) — restored, install will validate" + log("SETUP", f"Dependency {msg}") + notes.append(f"Dependency {msg}") + else: + msg = f"cache MISS for {kind} (key {digest[:12]}) — cold install" + log("SETUP", f"Dependency {msg}") + notes.append(f"Dependency {msg}") + + +def populate_dependency_cache(repo_dir: str, notes: list[str]) -> None: + """Publish freshly-installed dependency artifacts into the shared cache. + + Best-effort: called AFTER the repo's install/build has produced the + artifacts. For each cacheable pair whose lockfile hash has no entry yet, the + artifact is atomically published so the NEXT task on the same lockfile hits. + An already-present entry (this task hit, or a concurrent task won the race) + is left untouched. Never fails the task. + """ + root = cache_root() + if root is None: + return + for kind, lockfile, artifact in discover_artifacts(repo_dir): + if not os.path.isdir(artifact): + continue + digest = hash_lockfile(lockfile) + if digest is None: + continue + if populate_artifact(root, kind, digest, artifact): + log("SETUP", f"Dependency cache populated for {kind} (key {digest[:12]})") + notes.append(f"Dependency cache: populated {kind} (key {digest[:12]})") diff --git a/agent/src/repo.py b/agent/src/repo.py index 33ccf5c4..7aecaef1 100644 --- a/agent/src/repo.py +++ b/agent/src/repo.py @@ -325,6 +325,19 @@ def setup_repo(config: TaskConfig, progress: Any = None) -> RepoSetup: check=False, ) + # Warm dependency cache (ABCA-691): restore node_modules / .venv from the + # shared /cache volume BEFORE any install/build runs, keyed on the lockfile + # hash so a hit can only reuse artifacts built from the exact same lockfiles. + # The repo's own install (driven by the build command / mise install) still + # runs and validates the restored tree, so a hit produces the identical + # result a cold install would — it just skips the multi-minute network + # download. Best-effort: a miss / no cache volume leaves the clone untouched + # and the install runs cold. Populated at the END of setup (below), once the + # baseline build has produced the artifacts. + from dependency_cache import populate_dependency_cache, restore_dependency_cache + + restore_dependency_cache(repo_dir, notes) + # mise install (deterministic — not left to the LLM) log("SETUP", "Running mise install...") result = run_cmd( @@ -537,6 +550,13 @@ def setup_repo(config: TaskConfig, progress: Any = None) -> RepoSetup: if head_res.returncode == 0: head_sha_before = head_res.stdout.strip() + # Publish the freshly-installed dependency artifacts into the shared warm + # cache (ABCA-691) so the NEXT task on the same lockfiles hits. Runs after + # the install + baseline build have populated node_modules / .venv. Atomic + # write-to-temp-then-rename; a no-op when an entry already exists (this task + # hit, or a concurrent task won the publish race). Best-effort — never fails. + populate_dependency_cache(repo_dir, notes) + return RepoSetup( repo_dir=repo_dir, branch=branch, diff --git a/agent/tests/test_repo.py b/agent/tests/test_repo.py index a9030401..274511f3 100644 --- a/agent/tests/test_repo.py +++ b/agent/tests/test_repo.py @@ -5,9 +5,11 @@ (detect_default_branch) — recording argv and returning scripted results. """ +import os import subprocess from types import SimpleNamespace +import dependency_cache import repo from tests.conftest import FakeRunCmd, make_task_config @@ -648,3 +650,285 @@ def test_remediation_does_not_widen_creds_or_egress(self, monkeypatch): assert "set-remote-url" not in labels assert "configure-git-credential-helper" not in labels assert "safe-directory" in labels # only the pre-clone step ran + + +# --------------------------------------------------------------------------- +# ABCA-691: warm dependency cache (lockfile-keyed node_modules / .venv reuse) +# --------------------------------------------------------------------------- + + +def _write(path: str, content: str = "x") -> None: + """Create a file (and its parent dirs) with *content*.""" + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as handle: + handle.write(content) + + +def _make_dir_with_file(path: str, filename: str = "marker", content: str = "data") -> None: + """Create directory *path* containing one file (an artifact stand-in).""" + os.makedirs(path, exist_ok=True) + _write(os.path.join(path, filename), content) + + +class TestHashLockfile: + """The cache key is the lockfile hash, NEVER the commit SHA — the crux of the + correctness invariant (a changed lockfile must miss).""" + + def test_hash_is_stable_for_identical_bytes(self, tmp_path): + a = str(tmp_path / "yarn.a.lock") + b = str(tmp_path / "yarn.b.lock") + _write(a, "same bytes") + _write(b, "same bytes") + assert dependency_cache.hash_lockfile(a) == dependency_cache.hash_lockfile(b) + + def test_changed_lockfile_changes_the_hash(self, tmp_path): + lock = str(tmp_path / "yarn.lock") + _write(lock, "dep@1.0.0") + before = dependency_cache.hash_lockfile(lock) + _write(lock, "dep@2.0.0") # trunk bumped a dependency + after = dependency_cache.hash_lockfile(lock) + assert before != after + + def test_missing_lockfile_returns_none(self, tmp_path): + assert dependency_cache.hash_lockfile(str(tmp_path / "nope.lock")) is None + + +class TestCacheRoot: + def test_none_when_mount_absent(self, tmp_path, monkeypatch): + monkeypatch.setenv("DEPENDENCY_CACHE_DIR", str(tmp_path / "does-not-exist")) + assert dependency_cache.cache_root() is None + + def test_returns_writable_root(self, tmp_path, monkeypatch): + monkeypatch.setenv("DEPENDENCY_CACHE_DIR", str(tmp_path)) + assert dependency_cache.cache_root() == str(tmp_path) + + +class TestRestoreAndPopulate: + """Hit/miss + atomic populate on the low-level artifact primitives.""" + + def test_populate_then_restore_roundtrips_identical_bytes(self, tmp_path): + root = str(tmp_path / "cache") + os.makedirs(root) + source = str(tmp_path / "node_modules") + _make_dir_with_file(source, "pkg.js", "installed-payload") + + assert dependency_cache.populate_artifact(root, "node_modules", "abc123", source) is True + + dest = str(tmp_path / "clone" / "node_modules") + assert dependency_cache.restore_artifact(root, "node_modules", "abc123", dest) is True + # A hit produces the identical tree a cold install would have. + with open(os.path.join(dest, "pkg.js")) as handle: + assert handle.read() == "installed-payload" + + def test_restore_miss_when_key_absent(self, tmp_path): + root = str(tmp_path / "cache") + os.makedirs(root) + dest = str(tmp_path / "clone" / "node_modules") + assert dependency_cache.restore_artifact(root, "node_modules", "missing", dest) is False + assert not os.path.exists(dest) # nothing restored → cold install + + def test_populate_is_atomic_no_partial_entry_on_the_final_path(self, tmp_path, monkeypatch): + # The published entry only ever appears via an atomic rename: there is no + # window where // exists but is half-copied. + root = str(tmp_path / "cache") + os.makedirs(root) + source = str(tmp_path / "venv") + _make_dir_with_file(source, "python", "bin") + + renames: list[tuple[str, str]] = [] + real_rename = os.rename + + def spy_rename(src, dst): + renames.append((src, dst)) + return real_rename(src, dst) + + monkeypatch.setattr(os, "rename", spy_rename) + assert dependency_cache.populate_artifact(root, "venv", "deadbeef", source) is True + + entry = os.path.join(root, "venv", "deadbeef") + # Exactly one rename, and its destination is the final entry path — the + # temp was built elsewhere and moved into place in one atomic step. + assert any(dst == entry for _src, dst in renames) + assert os.path.isdir(entry) + + def test_second_populate_same_key_is_noop(self, tmp_path): + root = str(tmp_path / "cache") + os.makedirs(root) + source = str(tmp_path / "node_modules") + _make_dir_with_file(source) + assert dependency_cache.populate_artifact(root, "node_modules", "k", source) is True + # A concurrent/second publish of the same key finds the entry present. + assert dependency_cache.populate_artifact(root, "node_modules", "k", source) is False + + def test_concurrent_populate_race_leaves_one_valid_entry(self, tmp_path, monkeypatch): + # Simulate two tasks publishing the SAME key: the second's rename lands on + # a now-non-empty dir and fails; it must discard its temp and report False, + # leaving exactly one valid entry (no corruption, no leftover temp). + root = str(tmp_path / "cache") + os.makedirs(root) + source = str(tmp_path / "node_modules") + _make_dir_with_file(source, "pkg.js", "payload") + + real_rename = os.rename + + def racing_rename(src, dst): + # Before our rename runs, a concurrent task has already published the + # entry, so ``dst`` now exists and is non-empty → rename raises. + os.makedirs(dst, exist_ok=True) + _write(os.path.join(dst, "pkg.js"), "payload") + return real_rename(src, dst) # onto non-empty → OSError + + monkeypatch.setattr(os, "rename", racing_rename) + result = dependency_cache.populate_artifact(root, "node_modules", "shared", source) + + assert result is False # lost the race + entry = os.path.join(root, "node_modules", "shared") + assert os.path.isdir(entry) # the winner's entry is intact + # No leftover temp dirs polluting the kind dir. + kind_dir = os.path.join(root, "node_modules") + leftovers = [d for d in os.listdir(kind_dir) if d.startswith(".tmp")] + assert leftovers == [] + + def test_restore_does_not_clobber_existing_dest(self, tmp_path): + root = str(tmp_path / "cache") + os.makedirs(root) + source = str(tmp_path / "src_nm") + _make_dir_with_file(source, "pkg.js", "cached") + dependency_cache.populate_artifact(root, "node_modules", "k", source) + + dest = str(tmp_path / "clone" / "node_modules") + _make_dir_with_file(dest, "pkg.js", "already-present") + # dest exists → not a restore target; leave it untouched. + assert dependency_cache.restore_artifact(root, "node_modules", "k", dest) is False + with open(os.path.join(dest, "pkg.js")) as handle: + assert handle.read() == "already-present" + + +class TestDiscoverArtifacts: + def test_discovers_root_yarn_lock_and_nested_uv_locks(self, tmp_path): + repo_dir = str(tmp_path / "clone") + _write(os.path.join(repo_dir, "yarn.lock")) + _write(os.path.join(repo_dir, "agent", "uv.lock")) + plan = dependency_cache.discover_artifacts(repo_dir) + kinds = {k for k, _lock, _art in plan} + assert dependency_cache.CACHE_KIND_NODE_MODULES in kinds + assert dependency_cache.CACHE_KIND_VENV in kinds + # node_modules maps to the ROOT (yarn workspaces install centrally). + nm = next(a for k, _l, a in plan if k == dependency_cache.CACHE_KIND_NODE_MODULES) + assert nm == os.path.join(repo_dir, "node_modules") + # .venv is the SIBLING of the uv.lock that keys it. + venv = next(a for k, _l, a in plan if k == dependency_cache.CACHE_KIND_VENV) + assert venv == os.path.join(repo_dir, "agent", ".venv") + + def test_skips_vendored_and_build_dirs_for_uv_locks(self, tmp_path): + repo_dir = str(tmp_path / "clone") + _write(os.path.join(repo_dir, "agent", "uv.lock")) + _write(os.path.join(repo_dir, "node_modules", "pkg", "uv.lock")) + _write(os.path.join(repo_dir, "cdk.out", "asset", "uv.lock")) + plan = dependency_cache.discover_artifacts(repo_dir) + venvs = [a for k, _l, a in plan if k == dependency_cache.CACHE_KIND_VENV] + assert venvs == [os.path.join(repo_dir, "agent", ".venv")] + + def test_no_lockfiles_returns_empty(self, tmp_path): + assert dependency_cache.discover_artifacts(str(tmp_path)) == [] + + +class TestRestoreAndPopulateHighLevel: + """The setup-facing helpers: end-to-end hit/miss keyed on lockfile hash.""" + + def test_changed_yarn_lock_is_a_miss_then_repopulates(self, tmp_path, monkeypatch): + # Two consecutive "tasks" with a CHANGED yarn.lock: the second must MISS + # (different lockfile hash) and cold-install — never reuse stale deps. + root = str(tmp_path / "cache") + os.makedirs(root) + monkeypatch.setenv("DEPENDENCY_CACHE_DIR", root) + + # Task 1: lockfile v1, installs node_modules, populates cache. + repo1 = str(tmp_path / "task1") + _write(os.path.join(repo1, "yarn.lock"), "dep@1.0.0") + notes1: list[str] = [] + dependency_cache.restore_dependency_cache(repo1, notes1) + assert any("cache MISS for node_modules" in n for n in notes1) + _make_dir_with_file(os.path.join(repo1, "node_modules"), "pkg.js", "v1") + dependency_cache.populate_dependency_cache(repo1, notes1) + + # Task 2: SAME lockfile → HIT (restores identical tree). + repo2 = str(tmp_path / "task2") + _write(os.path.join(repo2, "yarn.lock"), "dep@1.0.0") + notes2: list[str] = [] + dependency_cache.restore_dependency_cache(repo2, notes2) + assert any("cache HIT for node_modules" in n for n in notes2) + with open(os.path.join(repo2, "node_modules", "pkg.js")) as handle: + assert handle.read() == "v1" + + # Task 3: trunk BUMPED the dependency → different hash → MISS (no stale reuse). + repo3 = str(tmp_path / "task3") + _write(os.path.join(repo3, "yarn.lock"), "dep@2.0.0") + notes3: list[str] = [] + dependency_cache.restore_dependency_cache(repo3, notes3) + assert any("cache MISS for node_modules" in n for n in notes3) + assert not os.path.exists(os.path.join(repo3, "node_modules")) + + def test_no_cache_volume_is_a_clean_cold_install(self, tmp_path, monkeypatch): + monkeypatch.setenv("DEPENDENCY_CACHE_DIR", str(tmp_path / "absent")) + repo_dir = str(tmp_path / "clone") + _write(os.path.join(repo_dir, "yarn.lock"), "dep@1.0.0") + notes: list[str] = [] + dependency_cache.restore_dependency_cache(repo_dir, notes) # must not raise + assert any("unavailable" in n for n in notes) + # populate is likewise a safe no-op with no volume. + _make_dir_with_file(os.path.join(repo_dir, "node_modules")) + dependency_cache.populate_dependency_cache(repo_dir, notes) # must not raise + + +class TestSetupRepoWiresDependencyCache: + """setup_repo must restore before install and populate after — best-effort.""" + + def test_setup_calls_restore_then_populate(self, monkeypatch): + fake = _fake_run_cmd() + _patch_common(monkeypatch, fake) + monkeypatch.setattr(repo, "detect_default_branch", lambda url, d: "main") + + calls: list[str] = [] + monkeypatch.setattr( + "dependency_cache.restore_dependency_cache", + lambda repo_dir, notes: calls.append("restore"), + ) + monkeypatch.setattr( + "dependency_cache.populate_dependency_cache", + lambda repo_dir, notes: calls.append("populate"), + ) + + repo.setup_repo(_config()) + + assert calls == ["restore", "populate"] + + def test_setup_end_to_end_hit_across_two_runs(self, tmp_path, monkeypatch): + # End-to-end through the REAL cache helpers (only run_cmd + workspace + # faked): a first setup_repo populates the cache from an installed + # node_modules, and a second run on the same lockfile restores it (a HIT). + root = str(tmp_path / "cache") + os.makedirs(root) + monkeypatch.setenv("DEPENDENCY_CACHE_DIR", root) + + fake = _fake_run_cmd() + _patch_common(monkeypatch, fake) + monkeypatch.setattr(repo, "detect_default_branch", lambda url, d: "main") + # Point the clone dir into tmp_path (not the real /workspace). + workspace = str(tmp_path / "ws") + monkeypatch.setattr(repo, "AGENT_WORKSPACE", workspace) + + # Task 1: seed a yarn.lock + an "installed" node_modules so the post-setup + # populate publishes it. + repo_dir1 = os.path.join(workspace, "task-one") + _write(os.path.join(repo_dir1, "yarn.lock"), "dep@1.0.0") + _make_dir_with_file(os.path.join(repo_dir1, "node_modules"), "pkg.js", "payload") + setup1 = repo.setup_repo(_config(task_id="task-one")) + assert any("populated node_modules" in n for n in setup1.notes) + + # Task 2: same lockfile, fresh clone dir → restore HIT. + repo_dir2 = os.path.join(workspace, "task-two") + _write(os.path.join(repo_dir2, "yarn.lock"), "dep@1.0.0") + setup2 = repo.setup_repo(_config(task_id="task-two")) + assert any("cache HIT for node_modules" in n for n in setup2.notes) + assert os.path.exists(os.path.join(repo_dir2, "node_modules", "pkg.js")) diff --git a/cdk/src/constructs/ecs-agent-cluster.ts b/cdk/src/constructs/ecs-agent-cluster.ts index e5dfbaf6..a3fff2a7 100644 --- a/cdk/src/constructs/ecs-agent-cluster.ts +++ b/cdk/src/constructs/ecs-agent-cluster.ts @@ -22,6 +22,7 @@ import * as dynamodb from 'aws-cdk-lib/aws-dynamodb'; import * as ec2 from 'aws-cdk-lib/aws-ec2'; import * as ecr_assets from 'aws-cdk-lib/aws-ecr-assets'; import * as ecs from 'aws-cdk-lib/aws-ecs'; +import * as efs from 'aws-cdk-lib/aws-efs'; import * as iam from 'aws-cdk-lib/aws-iam'; import * as logs from 'aws-cdk-lib/aws-logs'; import * as s3 from 'aws-cdk-lib/aws-s3'; @@ -94,6 +95,22 @@ export interface EcsAgentClusterProps { /** HTTPS port — the only egress allowed from the agent task ENIs. */ const HTTPS_PORT = 443; +/** NFS port for EFS mounts — the warm dependency cache (ABCA-691). */ +const NFS_PORT = 2049; + +/** + * POSIX uid/gid the EFS Access Point enforces for the warm dependency cache + * (ABCA-691). The container runs as the non-root ``agent`` user (uid/gid 1000, + * created in the Dockerfile), so the access point owns its root dir and squashes + * all file ops to this identity — the task never touches raw EFS uids and reads/ + * writes ``/cache`` as itself. + */ +const CACHE_POSIX_UID = 1000; +const CACHE_POSIX_GID = 1000; + +/** Where the warm dependency cache EFS access point mounts in the BUILD task. */ +const CACHE_MOUNT_PATH = '/cache'; + /** * Fargate task sizes (vCPU units / MiB). The empirical sizing history that * justifies these lives on the two ``makeTaskDef`` call sites below. @@ -168,6 +185,57 @@ export class EcsAgentCluster extends Construct { 'Allow HTTPS egress (GitHub API, AWS services)', ); + // Warm dependency cache (ABCA-691): an EFS filesystem shared across build + // tasks that persists the derived dependency artifacts (node_modules and the + // target-repo .venv) so a task on the same lockfiles skips the cold + // yarn install + uv sync (~3–5 min). Keyed by lockfile hash in the agent + // (dependency_cache.py); EFS just provides the durable shared bytes. Only + // the BUILD def mounts it — the read-only PLANNING def never installs deps. + // + // ENCRYPTED at rest; lifecycle policy reaps cold entries so a stale lockfile's + // artifacts don't accumulate cost forever. Mount targets land in the VPC's + // private subnets (one per AZ) so every Fargate task ENI can reach NFS. + const cacheFileSystem = new efs.FileSystem(this, 'DependencyCacheFs', { + vpc: props.vpc, + encrypted: true, + // Reap cache entries not accessed for 30 days (a lockfile no longer in use + // stops being restored, so its bytes are pure cost) and pull them back to + // primary storage on first access after a move to IA. + lifecyclePolicy: efs.LifecyclePolicy.AFTER_30_DAYS, + outOfInfrequentAccessPolicy: efs.OutOfInfrequentAccessPolicy.AFTER_1_ACCESS, + // Bursting throughput scales with stored size — right for an intermittent + // read-heavy cache without provisioning a fixed (billed) throughput floor. + throughputMode: efs.ThroughputMode.BURSTING, + // The cache is derived, reproducible artifacts — a cold install rebuilds it. + // DESTROY keeps teardown clean rather than orphaning a filesystem. + removalPolicy: RemovalPolicy.DESTROY, + }); + + // Access point: the container mounts THIS (not the raw filesystem root), so + // EFS owns/creates the cache dir with the container's non-root POSIX identity + // (uid/gid 1000 = the Dockerfile's ``agent`` user) and squashes all ops to it. + // The task therefore reads/writes /cache as itself without any chown dance. + const cacheAccessPoint = cacheFileSystem.addAccessPoint('DependencyCacheAp', { + path: '/dependency-cache', + createAcl: { ownerUid: String(CACHE_POSIX_UID), ownerGid: String(CACHE_POSIX_GID), permissions: '0755' }, + posixUser: { uid: String(CACHE_POSIX_UID), gid: String(CACHE_POSIX_GID) }, + }); + + // Allow the task ENIs to reach the EFS mount targets over NFS (2049). The + // filesystem's own SG is created by the FileSystem construct; open an ingress + // rule from the task SG so only agent tasks (443-egress-locked) can mount. + cacheFileSystem.connections.allowFrom( + this.securityGroup, + ec2.Port.tcp(NFS_PORT), + 'Allow agent build tasks to mount the warm dependency cache over NFS', + ); + // The task SG blocks all egress except 443; NFS to the cache needs 2049 out. + this.securityGroup.addEgressRule( + cacheFileSystem.connections.securityGroups[0], + ec2.Port.tcp(NFS_PORT), + 'Allow NFS egress to the warm dependency cache EFS mount targets', + ); + // CloudWatch log group for agent task output const logGroup = new logs.LogGroup(this, 'TaskLogGroup', { retention: logs.RetentionDays.THREE_MONTHS, @@ -219,12 +287,20 @@ export class EcsAgentCluster extends Construct { }), }; const image = ecs.ContainerImage.fromDockerImageAsset(props.agentImageAsset); + // Logical name of the EFS-backed volume on the task def(s) that mount the + // warm dependency cache (ABCA-691). + const cacheVolumeName = 'DependencyCache'; const makeTaskDef = ( taskDefId: string, cpu: number, memoryLimitMiB: number, extraEnv: Record, ephemeralStorageGiB?: number, + // Mount the warm dependency cache EFS volume at /cache (ABCA-691). Only the + // BUILD def sets this — the read-only PLANNING def never installs deps, so + // it neither needs nor gets the mount (keeps its ENI free of NFS + its task + // role identical by construction, just without the volume). + mountDependencyCache = false, ) => { const def = new ecs.FargateTaskDefinition(this, taskDefId, { cpu, @@ -239,11 +315,40 @@ export class EcsAgentCluster extends Construct { operatingSystemFamily: ecs.OperatingSystemFamily.LINUX, }, }); - def.addContainer(this.containerName, { + if (mountDependencyCache) { + // EFS-backed volume via the access point (POSIX identity + squash). TLS + // encrypts the NFS traffic in transit; IAM authorization ties the mount + // to the task role's elasticfilesystem:ClientMount/Write grant below. + def.addVolume({ + name: cacheVolumeName, + efsVolumeConfiguration: { + fileSystemId: cacheFileSystem.fileSystemId, + transitEncryption: 'ENABLED', + authorizationConfig: { + accessPointId: cacheAccessPoint.accessPointId, + iam: 'ENABLED', + }, + }, + }); + } + const container = def.addContainer(this.containerName, { image, logging: ecs.LogDrivers.awsLogs({ logGroup, streamPrefix: 'agent' }), - environment: { ...baseEnvironment, ...extraEnv }, + // Point the agent's dependency_cache at the mount only when it's present; + // absent → cache_root() returns None and every task installs cold. + environment: { + ...baseEnvironment, + ...(mountDependencyCache && { DEPENDENCY_CACHE_DIR: CACHE_MOUNT_PATH }), + ...extraEnv, + }, }); + if (mountDependencyCache) { + container.addMountPoints({ + containerPath: CACHE_MOUNT_PATH, + sourceVolume: cacheVolumeName, + readOnly: false, + }); + } return def; }; @@ -295,7 +400,10 @@ export class EcsAgentCluster extends Construct { // Propagates to both the platform push (post_hooks.py) and the agent's own // git-tool pushes via shell.py::_clean_env (blacklist — passes SKIP through). SKIP: 'monorepo-tests-pre-push', - }, BUILD_TASK_EPHEMERAL_STORAGE_GIB); + // Only the build def mounts the warm dependency cache (ABCA-691) — it is + // the def that runs installs. The final `true` turns on the /cache EFS + // mount + DEPENDENCY_CACHE_DIR env. + }, BUILD_TASK_EPHEMERAL_STORAGE_GIB, true); // PLANNING task def (#299 ECS_RIGHTSIZED_PLANNING) — for read-only workflows // (coding/decompose-v1) that clone + read + emit a plan artifact but NEVER @@ -320,6 +428,21 @@ export class EcsAgentCluster extends Construct { // touched by the reconciler/orchestrator path; keep it on the task role. props.userConcurrencyTable.grantReadWriteData(taskRole); + // Warm dependency cache (ABCA-691): the build task mounts the EFS access + // point with IAM authorization enabled, so the task role needs EFS client + // mount + read/write. ``grant`` scopes it to THIS filesystem's ARN + // (Condition on the access point) — no wildcard. Both task defs share the + // role, but only the build def carries the volume/mount, so the planning + // def's ENI never opens an NFS mount despite holding the (unused) grant. + // ClientMount + ClientWrite only — the access point squashes all ops to the + // container's non-root POSIX user (uid/gid 1000), so ClientRootAccess is + // neither needed nor granted (least privilege). + cacheFileSystem.grant( + taskRole, + 'elasticfilesystem:ClientMount', + 'elasticfilesystem:ClientWrite', + ); + // Secrets Manager read for GitHub token (read once at startup, before the // agent assumes the SessionRole — stays on the task role). props.githubTokenSecret.grantRead(taskRole); diff --git a/cdk/test/constructs/ecs-agent-cluster.test.ts b/cdk/test/constructs/ecs-agent-cluster.test.ts index cb256704..d33f963f 100644 --- a/cdk/test/constructs/ecs-agent-cluster.test.ts +++ b/cdk/test/constructs/ecs-agent-cluster.test.ts @@ -381,6 +381,122 @@ describe('EcsAgentCluster construct', () => { }); }); + // ABCA-691: warm dependency cache. An EFS filesystem + access point is mounted + // into the BUILD task def at /cache so consecutive tasks on the same lockfiles + // reuse node_modules / .venv instead of paying the cold install every time. + describe('warm dependency cache (ABCA-691)', () => { + test('creates one encrypted EFS filesystem', () => { + baseTemplate.resourceCountIs('AWS::EFS::FileSystem', 1); + baseTemplate.hasResourceProperties('AWS::EFS::FileSystem', { + Encrypted: true, + }); + }); + + test('creates an EFS access point that squashes to the container POSIX user (uid/gid 1000)', () => { + baseTemplate.resourceCountIs('AWS::EFS::AccessPoint', 1); + baseTemplate.hasResourceProperties('AWS::EFS::AccessPoint', { + PosixUser: { Uid: '1000', Gid: '1000' }, + RootDirectory: { + Path: '/dependency-cache', + CreationInfo: { OwnerUid: '1000', OwnerGid: '1000', Permissions: '0755' }, + }, + }); + }); + + test('mounts the cache EFS volume into the BUILD def at /cache (read-write) but NOT the planning def', () => { + const taskDefs = baseTemplate.findResources('AWS::ECS::TaskDefinition'); + const build = Object.values(taskDefs).find( + d => d.Properties.Cpu === '16384' && d.Properties.Memory === '122880', + ); + const planning = Object.values(taskDefs).find( + d => d.Properties.Cpu === '2048' && d.Properties.Memory === '8192', + ); + expect(build).toBeDefined(); + expect(planning).toBeDefined(); + + // BUILD def carries the EFS volume + a /cache mount point. + const buildVolumes = build!.Properties.Volumes ?? []; + expect(buildVolumes.some((v: { EFSVolumeConfiguration?: unknown }) => v.EFSVolumeConfiguration)).toBe(true); + const buildMounts = build!.Properties.ContainerDefinitions[0].MountPoints ?? []; + expect(buildMounts.some((m: { ContainerPath: string; ReadOnly?: boolean }) => + m.ContainerPath === '/cache' && !m.ReadOnly, + )).toBe(true); + + // PLANNING def has NO EFS volume/mount — a read-only planner installs nothing. + const planningVolumes = planning!.Properties.Volumes ?? []; + expect(planningVolumes.some((v: { EFSVolumeConfiguration?: unknown }) => v.EFSVolumeConfiguration)).toBe(false); + const planningMounts = planning!.Properties.ContainerDefinitions[0].MountPoints ?? []; + expect(planningMounts.some((m: { ContainerPath: string }) => m.ContainerPath === '/cache')).toBe(false); + }); + + test('the EFS volume uses transit encryption + IAM authorization via the access point', () => { + const taskDefs = baseTemplate.findResources('AWS::ECS::TaskDefinition'); + const build = Object.values(taskDefs).find( + d => d.Properties.Cpu === '16384' && d.Properties.Memory === '122880', + ); + const efsVolume = (build!.Properties.Volumes ?? []).find( + (v: { EFSVolumeConfiguration?: unknown }) => v.EFSVolumeConfiguration, + ); + expect(efsVolume.EFSVolumeConfiguration.TransitEncryption).toBe('ENABLED'); + expect(efsVolume.EFSVolumeConfiguration.AuthorizationConfig.IAM).toBe('ENABLED'); + expect(efsVolume.EFSVolumeConfiguration.AuthorizationConfig.AccessPointId).toBeDefined(); + }); + + test('sets DEPENDENCY_CACHE_DIR=/cache on the BUILD def only (planning installs cold)', () => { + const taskDefs = baseTemplate.findResources('AWS::ECS::TaskDefinition'); + const build = Object.values(taskDefs).find( + d => d.Properties.Cpu === '16384' && d.Properties.Memory === '122880', + ); + const planning = Object.values(taskDefs).find( + d => d.Properties.Cpu === '2048' && d.Properties.Memory === '8192', + ); + const buildEnv = build!.Properties.ContainerDefinitions[0].Environment ?? []; + expect(buildEnv.some((e: { Name: string; Value: string }) => + e.Name === 'DEPENDENCY_CACHE_DIR' && e.Value === '/cache', + )).toBe(true); + const planningEnv = planning!.Properties.ContainerDefinitions[0].Environment ?? []; + expect(planningEnv.some((e: { Name: string }) => e.Name === 'DEPENDENCY_CACHE_DIR')).toBe(false); + }); + + test('grants the task role EFS client mount + write scoped to the cache filesystem (no ClientRootAccess, no wildcard)', () => { + const policies = baseTemplate.findResources('AWS::IAM::Policy'); + const efsActions = new Set(); + let efsStatement: { Resource?: unknown } | undefined; + for (const p of Object.values(policies)) { + for (const s of p.Properties.PolicyDocument.Statement) { + const actions = Array.isArray(s.Action) ? s.Action : [s.Action]; + for (const a of actions) { + if (typeof a === 'string' && a.startsWith('elasticfilesystem:')) { + efsActions.add(a); + efsStatement = s; + } + } + } + } + expect(efsActions.has('elasticfilesystem:ClientMount')).toBe(true); + expect(efsActions.has('elasticfilesystem:ClientWrite')).toBe(true); + // Least privilege: root access is squashed by the access point, never granted. + expect(efsActions.has('elasticfilesystem:ClientRootAccess')).toBe(false); + // Scoped to a concrete filesystem ARN, not a bare wildcard. + expect(efsStatement).toBeDefined(); + expect(efsStatement!.Resource).not.toEqual('*'); + }); + + test('opens NFS (2049) between the task SG and the EFS mount targets', () => { + // Ingress on the EFS SG from the task SG, and egress from the task SG. + baseTemplate.hasResourceProperties('AWS::EC2::SecurityGroupIngress', { + IpProtocol: 'tcp', + FromPort: 2049, + ToPort: 2049, + }); + baseTemplate.hasResourceProperties('AWS::EC2::SecurityGroupEgress', { + IpProtocol: 'tcp', + FromPort: 2049, + ToPort: 2049, + }); + }); + }); + describe('with a SessionRole wired (#209)', () => { function createWithSessionRole(): Template { const app = new App();