From b230111e7bcdb2687ad65188e7c62cf7941db185 Mon Sep 17 00:00:00 2001 From: bgagent Date: Fri, 10 Jul 2026 22:49:59 +0000 Subject: [PATCH] feat(agent): warm lockfile-keyed dependency cache across ECS tasks (ABCA-691) Persist node_modules + the target repo's uv .venv on a shared EFS volume mounted at /cache so consecutive tasks on the same lockfiles skip the cold yarn install + uv sync. Each entry is keyed on the LOCKFILE hash (never the commit SHA): a trunk dependency bump changes the hash -> cache miss -> full reinstall, so a build can never run against stale deps. Populate is write-to-temp-then-atomic-rename; a missing/corrupt/unmounted cache always degrades to a cold install and never fails the task. CDK: EFS filesystem + access point (POSIX-scoped to the agent uid), mounted into the BUILD def only with transit encryption + IAM auth; task-role EFS ClientMount/ClientWrite scoped to the access point; NFS egress opened on the task SG. The read-only PLANNING def does not mount it. Co-Authored-By: Claude Opus 4.8 Task-Id: 01KX715Y1NHNYVZ23GC1GXJCVF Prompt-Version: 1c9c10e027a2 --- agent/src/dep_cache.py | 300 +++++++++++++++++ agent/src/repo.py | 11 + agent/tests/test_dep_cache.py | 314 ++++++++++++++++++ agent/tests/test_repo.py | 31 ++ cdk/src/constructs/ecs-agent-cluster.ts | 113 ++++++- cdk/test/constructs/ecs-agent-cluster.test.ts | 102 ++++++ 6 files changed, 869 insertions(+), 2 deletions(-) create mode 100644 agent/src/dep_cache.py create mode 100644 agent/tests/test_dep_cache.py diff --git a/agent/src/dep_cache.py b/agent/src/dep_cache.py new file mode 100644 index 00000000..11d20197 --- /dev/null +++ b/agent/src/dep_cache.py @@ -0,0 +1,300 @@ +"""Warm dependency cache across ECS agent tasks (ABCA-691). + +Persist the two expensive *derived* dependency artifacts — Node ``node_modules`` +and the target repo's uv ``.venv`` — on a shared volume mounted at ``/cache`` so +a fresh clone can restore them instead of paying the full cold ``yarn install`` ++ ``uv sync`` (~3-5 min) on every task. + +Correctness (non-negotiable): each 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 runs. It is therefore +impossible to build against stale deps. If trunk changed only source (lockfile +unchanged) the entry HITS and reuse is safe: a lockfile fully determines the +resolved dependency tree, so a restored entry is byte-identical to what a cold +install would have produced. The git clone itself is never cached — it stays +fresh per task (the source of truth); only these derived artifacts are reused. + +Concurrency: many ECS tasks share ``/cache`` at once. A cache entry is populated +by copying into a per-task temp dir and then ``os.rename``-ing it into place — +an atomic same-filesystem move, so a half-written tree is never visible under +its final key and two tasks racing the same key can't corrupt each other (the +loser simply discards its temp; the winner's immutable entry stands). + +Best-effort by construction: a missing, empty, corrupt, or unwritable cache +NEVER fails the task — every failure path degrades to a normal cold install. +When ``/cache`` is not mounted at all (AgentCore runtime, local runs) this module +is a complete no-op and dependency handling is left exactly as it was before. +""" + +import hashlib +import os +import shutil +import uuid +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + +from shell import log +from shell import run_cmd as _default_run_cmd + +# Where the shared cache volume is mounted inside the BUILD task container. The +# CDK ``EcsAgentCluster`` construct mounts an EFS access point here (ABCA-691). +# Overridable via env for tests / alternate substrates. +DEFAULT_CACHE_DIR = "/cache" + +# Directories we never descend into when locating a lockfile — vendored / build +# trees whose own ``yarn.lock``/``uv.lock`` are not the repo's top-level lock. +_LOCK_SKIP_DIRS = frozenset({".git", "node_modules", ".venv", "cdk.out", "dist", "build"}) + +# A cold install can legitimately run several minutes; give it the same generous +# ceiling the build verify uses rather than run_cmd's 600s default. +_INSTALL_TIMEOUT_S = 1800 + + +@dataclass(frozen=True) +class Artifact: + """One cacheable dependency artifact and how to (re)build it. + + ``key`` is the ``/cache`` sub-namespace; entries live at + ``//``. ``artifact_dir`` and ``lockfile`` are + both resolved relative to the directory that contains the lockfile (so a + monorepo's ``agent/uv.lock`` → ``agent/.venv`` works, not just the root).""" + + key: str + lockfile: str + artifact_dir: str + install_argv: list[str] + install_label: str + + +# The MINIMAL scope (ABCA-691): node_modules + the target repo's uv .venv only. +# (jest/tsc build caches are an explicit out-of-scope follow-up.) +ARTIFACTS: list[Artifact] = [ + Artifact( + key="node_modules", + lockfile="yarn.lock", + artifact_dir="node_modules", + # --frozen-lockfile: install exactly what the lock pins and fail rather + # than silently mutate it, so the populated entry matches the key. + install_argv=["yarn", "install", "--frozen-lockfile"], + install_label="warm-cache-yarn-install", + ), + Artifact( + key="venv", + lockfile="uv.lock", + artifact_dir=".venv", + install_argv=["uv", "sync", "--frozen"], + install_label="warm-cache-uv-sync", + ), +] + + +def hash_lockfile(path: str) -> str: + """Return the SHA-256 hex digest of the lockfile at *path* — the cache key. + + Streams the file so a large lock is not read wholesale into memory. Raises + ``OSError`` if the file cannot be read; callers treat that as "no key". + """ + h = hashlib.sha256() + with open(path, "rb") as fh: + for chunk in iter(lambda: fh.read(65536), b""): + h.update(chunk) + return h.hexdigest() + + +def resolve_cache_dir(explicit: str | None = None) -> str | None: + """Resolve the mounted cache directory, or ``None`` when caching is off. + + Order: an explicit argument (tests), then ``$DEP_CACHE_DIR``, then + ``/cache``. Returns the path only if it exists and is a directory — when the + volume is not mounted (AgentCore, local runs) this is ``None`` and the whole + module no-ops, leaving pre-ABCA-691 dependency handling untouched. + """ + candidate = explicit or os.environ.get("DEP_CACHE_DIR") or DEFAULT_CACHE_DIR + return candidate if os.path.isdir(candidate) else None + + +def _find_lockfile(repo_dir: str, name: str) -> str | None: + """Return the shallowest ``name`` lockfile under *repo_dir*, or ``None``. + + Shallowest wins so a repo's top-level lock beats any nested one, and + vendored/build trees are pruned so we never key off a dependency's own lock. + """ + best: str | None = None + best_depth = None + for dirpath, dirnames, filenames in os.walk(repo_dir): + dirnames[:] = [d for d in dirnames if d not in _LOCK_SKIP_DIRS] + if name in filenames: + depth = os.path.relpath(dirpath, repo_dir).count(os.sep) + if best_depth is None or depth < best_depth: + best, best_depth = os.path.join(dirpath, name), depth + return best + + +def _dir_has_content(path: str) -> bool: + """True if *path* is a directory with at least one entry. + + An empty directory is treated as a MISS (a half-populated / stale marker), + so a caller falls back to a cold install rather than restoring nothing. + """ + try: + return os.path.isdir(path) and any(os.scandir(path)) + except OSError: + return False + + +def _restore(entry_dir: str, dest_dir: str) -> bool: + """Copy a cache *entry_dir* into the clone at *dest_dir*. Best-effort. + + Copies (not symlinks) so each task gets an independent, writable tree: a + build that writes into ``node_modules`` (e.g. ``.cache``) can never mutate + the shared entry and corrupt a concurrent task. ``symlinks=True`` preserves + the internal symlinks (``.bin`` shims, workspace links) so the restored tree + is byte-identical to a cold install. A failure leaves *dest_dir* removed and + returns ``False`` so the caller cold-installs. + """ + try: + if os.path.lexists(dest_dir): + shutil.rmtree(dest_dir, ignore_errors=True) + shutil.copytree(entry_dir, dest_dir, symlinks=True) + return True + except OSError as exc: + log("WARN", f"warm-cache: restore of {entry_dir} → {dest_dir} failed: {exc}") + shutil.rmtree(dest_dir, ignore_errors=True) + return False + + +def _atomic_populate(cache_subdir: str, key: str, src_dir: str) -> bool: + """Populate ``/`` from *src_dir* atomically. Best-effort. + + Copies *src_dir* into a per-task temp dir on the SAME filesystem, then + ``os.rename``-s it into place — an atomic move, so no partial tree is ever + visible under the final key. If another task won the race (the final key + already exists) or the move fails, the temp is discarded and the existing + entry is left intact — a populate never corrupts the cache. Returns whether + this task installed the entry. + """ + final = os.path.join(cache_subdir, key) + if os.path.isdir(final): + return False # already populated by a prior/concurrent task + tmp = os.path.join(cache_subdir, f".tmp-{uuid.uuid4().hex}") + try: + os.makedirs(cache_subdir, exist_ok=True) + shutil.copytree(src_dir, tmp, symlinks=True) + try: + os.rename(tmp, final) + return True + except OSError: + # Target sprang into existence (lost the race) or cross-device — the + # existing entry stands; drop our temp so no partial leaks. + shutil.rmtree(tmp, ignore_errors=True) + return False + except OSError as exc: + log("WARN", f"warm-cache: populate of {final} failed: {exc}") + shutil.rmtree(tmp, ignore_errors=True) + return False + + +def _process_artifact( + art: Artifact, + repo_dir: str, + cache_dir: str, + run_cmd: Callable[..., Any], + notes: list[str], +) -> None: + """Restore-or-install a single artifact. Never raises. + + HIT → copy the entry into the clone and skip the install. + MISS → run the install, then populate the cache for the next task. + Any read/restore failure degrades to a cold install; any install/populate + failure leaves the task to proceed with whatever the install produced. + """ + lock_path = _find_lockfile(repo_dir, art.lockfile) + if lock_path is None: + return # repo doesn't use this toolchain — nothing to cache + + proj_dir = os.path.dirname(lock_path) + dest_dir = os.path.join(proj_dir, art.artifact_dir) + cache_subdir = os.path.join(cache_dir, art.key) + + try: + key = hash_lockfile(lock_path) + except OSError as exc: + log("WARN", f"warm-cache: cannot hash {lock_path}: {exc}; cold install") + key = None + + # --- Restore path (cache HIT) --- + if key is not None: + entry_dir = os.path.join(cache_subdir, key) + if _dir_has_content(entry_dir): + if _restore(entry_dir, dest_dir): + log("SETUP", f"warm-cache HIT: {art.artifact_dir} restored from {entry_dir}") + notes.append( + f"dependency cache HIT: {art.artifact_dir} (key {key[:12]}) — skipped install" + ) + return + # Restore failed → treat as a miss and fall through to install. + log("WARN", f"warm-cache: HIT but restore failed for {art.artifact_dir}; cold install") + + # --- Miss path: install, then populate for the next task --- + log("SETUP", f"warm-cache MISS: {art.artifact_dir} — running {' '.join(art.install_argv)}") + result = run_cmd( + art.install_argv, + label=art.install_label, + cwd=proj_dir, + check=False, + timeout=_INSTALL_TIMEOUT_S, + ) + if getattr(result, "returncode", 1) != 0: + notes.append(f"dependency cache MISS: {art.artifact_dir} — install failed, not cached") + return + if not key or not os.path.isdir(dest_dir): + notes.append(f"dependency cache MISS: {art.artifact_dir} — installed (not cached)") + return + populated = _atomic_populate(cache_subdir, key, dest_dir) + if populated: + notes.append( + f"dependency cache MISS: {art.artifact_dir} — installed and cached (key {key[:12]})" + ) + else: + notes.append( + f"dependency cache MISS: {art.artifact_dir} — installed " + "(cache populated by another task)" + ) + + +def warm_dependency_cache( + repo_dir: str, + *, + run_cmd: Callable[..., Any] | None = None, + cache_dir: str | None = None, + artifacts: list[Artifact] | None = None, +) -> list[str]: + """Restore or install each dependency artifact, keyed on its lockfile hash. + + Returns human-readable notes (one per artifact acted on) for the + ``RepoSetup`` notes; never raises. When no cache volume is mounted this is a + no-op and returns a single explanatory note, leaving dependency handling + exactly as it was before ABCA-691. + """ + resolved = resolve_cache_dir(cache_dir) + if resolved is None: + log("SETUP", "warm-cache: no /cache volume mounted — skipping (cold install as before)") + return ["dependency cache not mounted; skipped warm cache"] + + run = run_cmd or _default_run_cmd + notes: list[str] = [] + for art in artifacts if artifacts is not None else ARTIFACTS: + try: + _process_artifact(art, repo_dir, resolved, run, notes) + except Exception as exc: + # A bug in the cache path must degrade to a cold install, not crash + # setup. Log loudly (this is unexpected) and move on. + log("ERROR", f"warm-cache: unexpected error for {art.key}: {type(exc).__name__}: {exc}") + notes.append(f"dependency cache error for {art.artifact_dir}; proceeded without cache") + return notes diff --git a/agent/src/repo.py b/agent/src/repo.py index 33ccf5c4..f42114e2 100644 --- a/agent/src/repo.py +++ b/agent/src/repo.py @@ -339,6 +339,17 @@ def setup_repo(config: TaskConfig, progress: Any = None) -> RepoSetup: else: notes.append("mise install: OK") + # Warm dependency cache (ABCA-691): restore node_modules / .venv from the + # shared /cache volume when the target repo's lockfile hash matches a prior + # task's, else cold-install and populate the entry for the next task. Keyed on + # the lockfile hash (never the commit SHA) so a trunk dependency bump misses + # and reinstalls — impossible to build against stale deps. Best-effort: a + # missing/corrupt cache or an unmounted /cache volume degrades to the normal + # cold install and never fails the task. + from dep_cache import warm_dependency_cache + + notes.extend(warm_dependency_cache(repo_dir)) + # Initial build (record whether the project builds before agent changes). # #1: use the repo's configured build command (default mise run build). from post_hooks import ( diff --git a/agent/tests/test_dep_cache.py b/agent/tests/test_dep_cache.py new file mode 100644 index 00000000..2dc770b3 --- /dev/null +++ b/agent/tests/test_dep_cache.py @@ -0,0 +1,314 @@ +"""Unit tests for dep_cache.py — the warm dependency cache (ABCA-691). + +Covers the four correctness pillars from the issue: + * hash-key: entries are keyed on the LOCKFILE hash (never the commit SHA), so + a changed lockfile produces a different key. + * hit: a populated entry matching the lockfile hash is restored and the + install is skipped. + * miss: a changed/absent lockfile hash MISSES → the install runs and the entry + is populated for the next task. + * atomic-populate: a concurrent populate never corrupts the cache; a + missing/corrupt/unmounted cache degrades to a cold install and never raises. + +The install seam (``run_cmd``) is faked so no real yarn/uv runs; the fake writes +a marker file into the artifact dir so we can assert restore vs. install. +""" + +import os +from types import SimpleNamespace + +import dep_cache + + +def _write(path: str, content: str = "x") -> None: + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as fh: + fh.write(content) + + +class _FakeInstall: + """Fake run_cmd that simulates an install by writing the artifact dir. + + Records each call's label + cwd. On success (default) it creates the + artifact dir with a marker file whose content proves it came from a cold + install (not a restore). Set ``rc`` to simulate an install failure. + """ + + def __init__(self, rc: int = 0, marker: str = "INSTALLED"): + self.calls: list[dict] = [] + self._rc = rc + self._marker = marker + + def __call__(self, cmd, label, cwd: str = "", check=True, timeout=None): + self.calls.append({"cmd": cmd, "label": label, "cwd": cwd}) + if self._rc == 0: + art = "node_modules" if "yarn" in cmd[0] else ".venv" + _write(os.path.join(cwd, art, "MARKER"), self._marker) + return SimpleNamespace(returncode=self._rc, stdout="", stderr="") + + def labels(self) -> list[str]: + return [c["label"] for c in self.calls] + + +def _node_repo(tmp_path, lock_content="lock-v1"): + """A minimal clone dir with a yarn.lock.""" + repo = tmp_path / "repo" + repo.mkdir() + (repo / "yarn.lock").write_text(lock_content) + return str(repo) + + +class TestHashKey: + def test_hash_is_sha256_of_lockfile_bytes(self, tmp_path): + p = tmp_path / "yarn.lock" + p.write_text("resolved-tree-A") + import hashlib + + expected = hashlib.sha256(b"resolved-tree-A").hexdigest() + assert dep_cache.hash_lockfile(str(p)) == expected + + def test_different_lockfile_content_yields_different_key(self, tmp_path): + a = tmp_path / "a.lock" + b = tmp_path / "b.lock" + a.write_text("dep@1.0.0") + b.write_text("dep@2.0.0") # trunk bumped a dependency + assert dep_cache.hash_lockfile(str(a)) != dep_cache.hash_lockfile(str(b)) + + def test_identical_content_yields_identical_key(self, tmp_path): + a = tmp_path / "a.lock" + b = tmp_path / "b.lock" + a.write_text("same") + b.write_text("same") + assert dep_cache.hash_lockfile(str(a)) == dep_cache.hash_lockfile(str(b)) + + +class TestMissThenPopulate: + def test_cold_miss_runs_install_and_populates_cache(self, tmp_path): + repo = _node_repo(tmp_path) + cache = tmp_path / "cache" + cache.mkdir() + fake = _FakeInstall() + + notes = dep_cache.warm_dependency_cache( + repo, + run_cmd=fake, + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], # node_modules only + ) + + # Install ran (cold miss). + assert "warm-cache-yarn-install" in fake.labels() + # node_modules exists in the clone. + assert os.path.isfile(os.path.join(repo, "node_modules", "MARKER")) + # The cache was populated under the lockfile hash. + key = dep_cache.hash_lockfile(os.path.join(repo, "yarn.lock")) + assert os.path.isfile(os.path.join(cache, "node_modules", key, "MARKER")) + assert any("cache MISS" in n and "cached" in n for n in notes) + + +class TestHitSkipsInstall: + def test_matching_lockfile_hash_restores_and_skips_install(self, tmp_path): + repo = _node_repo(tmp_path) + cache = tmp_path / "cache" + # Pre-populate a cache entry keyed on the CURRENT lockfile hash, with a + # marker proving it came from the cache (not a fresh install). + key = dep_cache.hash_lockfile(os.path.join(repo, "yarn.lock")) + _write(os.path.join(cache, "node_modules", key, "MARKER"), "FROM_CACHE") + fake = _FakeInstall() + + notes = dep_cache.warm_dependency_cache( + repo, + run_cmd=fake, + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], + ) + + # Install did NOT run — the entry was restored. + assert fake.labels() == [] + # The restored node_modules carries the cache marker (byte-identical). + with open(os.path.join(repo, "node_modules", "MARKER")) as fh: + assert fh.read() == "FROM_CACHE" + assert any("cache HIT" in n for n in notes) + + +class TestChangedLockfileMisses: + def test_changed_yarn_lock_misses_and_reinstalls(self, tmp_path): + """AC: a task whose clone has a CHANGED yarn.lock does a full reinstall. + + A cache entry exists for the OLD lockfile hash; the clone's lockfile + changed (trunk bumped a dep), so its hash differs → MISS → reinstall, + and the OLD entry is never restored (no stale deps).""" + repo = _node_repo(tmp_path, lock_content="dep@2.0.0") # new lock + cache = tmp_path / "cache" + # An entry exists — but keyed on the OLD lockfile's hash. + import hashlib + + old_key = hashlib.sha256(b"dep@1.0.0").hexdigest() + _write(os.path.join(cache, "node_modules", old_key, "MARKER"), "STALE") + fake = _FakeInstall() + + dep_cache.warm_dependency_cache( + repo, + run_cmd=fake, + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], + ) + + # Reinstalled (cache miss on the new hash). + assert "warm-cache-yarn-install" in fake.labels() + # The clone's node_modules is the FRESH install, never the stale entry. + with open(os.path.join(repo, "node_modules", "MARKER")) as fh: + assert fh.read() == "INSTALLED" + # And a new entry was populated under the NEW hash (old entry untouched). + new_key = dep_cache.hash_lockfile(os.path.join(repo, "yarn.lock")) + assert new_key != old_key + assert os.path.isfile(os.path.join(cache, "node_modules", new_key, "MARKER")) + assert os.path.isfile(os.path.join(cache, "node_modules", old_key, "MARKER")) + + +class TestAtomicPopulate: + def test_populate_is_atomic_no_partial_entry_visible(self, tmp_path): + # A successful populate leaves ONLY the final key dir — no leftover + # .tmp-* staging dir (which would be a partially-written tree). + repo = _node_repo(tmp_path) + cache = tmp_path / "cache" + cache.mkdir() + dep_cache.warm_dependency_cache( + repo, + run_cmd=_FakeInstall(), + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], + ) + subdir = cache / "node_modules" + leftovers = [d for d in os.listdir(subdir) if d.startswith(".tmp-")] + assert leftovers == [] + + def test_concurrent_populate_does_not_overwrite_or_corrupt(self, tmp_path): + # If the final key already exists (a concurrent task won the race), a + # second populate must NOT overwrite it and must leave no temp behind. + repo = _node_repo(tmp_path) + cache = tmp_path / "cache" + key = dep_cache.hash_lockfile(os.path.join(repo, "yarn.lock")) + # Winner's entry already in place. + _write(os.path.join(cache, "node_modules", key, "MARKER"), "WINNER") + + installed = dep_cache._atomic_populate( + str(cache / "node_modules"), + key, + str(_seed_src(tmp_path)), + ) + + assert installed is False # lost the race, did not populate + # Winner's entry is intact and unchanged. + with open(os.path.join(cache, "node_modules", key, "MARKER")) as fh: + assert fh.read() == "WINNER" + leftovers = [d for d in os.listdir(cache / "node_modules") if d.startswith(".tmp-")] + assert leftovers == [] + + def test_corrupt_empty_entry_falls_back_to_install(self, tmp_path): + # An EMPTY cache dir (a half-populated / stale marker) must be treated as + # a MISS, not restored as an empty node_modules. + repo = _node_repo(tmp_path) + cache = tmp_path / "cache" + key = dep_cache.hash_lockfile(os.path.join(repo, "yarn.lock")) + os.makedirs(os.path.join(cache, "node_modules", key)) # empty → corrupt + fake = _FakeInstall() + + dep_cache.warm_dependency_cache( + repo, + run_cmd=fake, + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], + ) + # Fell back to a cold install rather than restoring an empty tree. + assert "warm-cache-yarn-install" in fake.labels() + assert os.path.isfile(os.path.join(repo, "node_modules", "MARKER")) + + +class TestBestEffortNeverFails: + def test_unmounted_cache_is_a_noop_cold_install(self, tmp_path, monkeypatch): + # No /cache mounted and no DEP_CACHE_DIR → module no-ops, no install + # driven by the cache (the normal setup path still installs deps itself). + monkeypatch.delenv("DEP_CACHE_DIR", raising=False) + repo = _node_repo(tmp_path) + fake = _FakeInstall() + notes = dep_cache.warm_dependency_cache( + repo, + run_cmd=fake, + cache_dir=str(tmp_path / "does-not-exist"), + ) + assert fake.labels() == [] + assert any("not mounted" in n for n in notes) + + def test_install_failure_does_not_populate_and_does_not_raise(self, tmp_path): + repo = _node_repo(tmp_path) + cache = tmp_path / "cache" + cache.mkdir() + fake = _FakeInstall(rc=1) # install fails + notes = dep_cache.warm_dependency_cache( + repo, + run_cmd=fake, + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], + ) + # Install ran but failed → nothing cached, no crash. + assert "warm-cache-yarn-install" in fake.labels() + key = dep_cache.hash_lockfile(os.path.join(repo, "yarn.lock")) + assert not os.path.isdir(os.path.join(cache, "node_modules", key)) + assert any("install failed" in n for n in notes) + + def test_repo_without_lockfile_skips_artifact_entirely(self, tmp_path): + # A repo with no yarn.lock → the node_modules artifact is skipped (no + # install, no note) — nothing to cache. + repo = tmp_path / "repo" + repo.mkdir() + fake = _FakeInstall() + cache = tmp_path / "cache" + cache.mkdir() + notes = dep_cache.warm_dependency_cache( + repo_dir=str(repo), + run_cmd=fake, + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], + ) + assert fake.labels() == [] + assert notes == [] + + def test_unexpected_error_degrades_and_never_raises(self, tmp_path, monkeypatch): + # An unexpected error inside artifact processing must be swallowed + # (best-effort) and recorded, not propagated to fail the task. + repo = _node_repo(tmp_path) + cache = tmp_path / "cache" + cache.mkdir() + + def boom(*a, **k): + raise RuntimeError("disk on fire") + + monkeypatch.setattr(dep_cache, "_process_artifact", boom) + notes = dep_cache.warm_dependency_cache( + repo, + run_cmd=_FakeInstall(), + cache_dir=str(cache), + artifacts=[dep_cache.ARTIFACTS[0]], + ) + assert any("error" in n.lower() for n in notes) # did not raise + + +class TestLockfileDiscovery: + def test_shallowest_lockfile_wins_and_vendored_pruned(self, tmp_path): + repo = tmp_path / "repo" + (repo / "pkg").mkdir(parents=True) + (repo / "node_modules" / "dep").mkdir(parents=True) + # Root lock, a nested pkg lock, and a vendored lock. + (repo / "yarn.lock").write_text("root") + (repo / "pkg" / "yarn.lock").write_text("nested") + (repo / "node_modules" / "dep" / "yarn.lock").write_text("vendored") + found = dep_cache._find_lockfile(str(repo), "yarn.lock") + assert found == str(repo / "yarn.lock") # shallowest, vendored ignored + + +def _seed_src(tmp_path): + """A small source dir to populate FROM (for the atomic-populate unit test).""" + src = tmp_path / "src-artifact" + _write(str(src / "MARKER"), "LOSER") + return src diff --git a/agent/tests/test_repo.py b/agent/tests/test_repo.py index a9030401..f610f3b5 100644 --- a/agent/tests/test_repo.py +++ b/agent/tests/test_repo.py @@ -118,6 +118,37 @@ def test_pr_workflow_does_not_double_capture_head_sha(self, monkeypatch): assert "head-sha-after-setup" not in fake.labels() +class TestWarmDependencyCache: + """ABCA-691: setup_repo invokes the warm dependency cache after mise install + (and before the baseline build), and folds its notes into RepoSetup.notes. + The cache logic itself is unit-tested in test_dep_cache.py; here we assert the + wiring — setup_repo calls it exactly once with the clone dir and surfaces its + notes.""" + + def test_setup_repo_invokes_warm_cache_and_records_notes(self, monkeypatch): + fake = _fake_run_cmd() + _patch_common(monkeypatch, fake) + monkeypatch.setattr(repo, "detect_default_branch", lambda url, d: "main") + + calls = [] + + import dep_cache + + def fake_warm(repo_dir, **kwargs): + calls.append(repo_dir) + return ["dependency cache HIT: node_modules — skipped install"] + + monkeypatch.setattr(dep_cache, "warm_dependency_cache", fake_warm) + + setup = repo.setup_repo(_config()) + + # Called once, with the clone dir. + assert len(calls) == 1 + assert calls[0].endswith("/task-abc") + # Its note is folded into the setup notes the PR/telemetry read. + assert any("dependency cache HIT" in n for n in setup.notes) + + class TestReadOnlyBaselineSkip: """#299 ECS_RIGHTSIZED_PLANNING: a read_only workflow (coding/decompose-v1) never edits code, runs the post-agent gate, or opens a PR, so the pre-agent diff --git a/cdk/src/constructs/ecs-agent-cluster.ts b/cdk/src/constructs/ecs-agent-cluster.ts index 491354ad..065d6f55 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'; @@ -109,6 +110,22 @@ const PLANNING_TASK_MEMORY_MIB = 8192; // PLANNING def only clones + reads so it keeps the 20 GiB default. const BUILD_TASK_EPHEMERAL_STORAGE_GIB = 100; +// ABCA-691: where the shared warm-dependency-cache volume is mounted inside the +// BUILD container. The agent's setup_repo restores/populates node_modules + the +// target repo's .venv here (keyed on the lockfile hash) so consecutive tasks on +// the same lockfiles skip the full cold `yarn install` + `uv sync`. Kept in sync +// with agent/src/dep_cache.py::DEFAULT_CACHE_DIR. +const DEP_CACHE_MOUNT_PATH = '/cache'; +// The EFS volume name the container mounts. (An ECS task-def volume name is a +// local handle wiring the mount point to the EFS volume config.) +const DEP_CACHE_VOLUME_NAME = 'dep-cache'; +// NFS port — how the task ENIs reach the EFS mount targets. +const NFS_PORT = 2049; +// The POSIX uid/gid the agent container runs as (agent/Dockerfile: `useradd -m +// agent` → the first non-system uid, 1000). The cache access point forces this +// owner so the container can read AND write cache entries without root. +const AGENT_UID = 1000; + export class EcsAgentCluster extends Construct { public readonly cluster: ecs.Cluster; /** The 64 GB / 16 vCPU BUILD task def — for coding workflows that run a full @@ -203,6 +220,51 @@ export class EcsAgentCluster extends Construct { AGENT_SESSION_ROLE_ARN: props.agentSessionRole.role.roleArn, }), }; + // ABCA-691: warm dependency cache on EFS. Persist the two expensive derived + // dependency artifacts (Node node_modules + the target repo's uv .venv) on a + // shared filesystem so a fresh clone can restore them instead of paying the + // full cold `yarn install` + `uv sync` (~3–5 min) every task. The agent keys + // each entry on the LOCKFILE hash (never the commit SHA), so a trunk + // dependency bump misses → reinstalls; only mounted on the BUILD def (the + // read-only PLANNING def never installs deps). Encrypted at rest; an access + // point roots the container at a subtree owned by the same POSIX uid the + // agent container runs as. RETAIN so a stack teardown doesn't wipe a shared + // cache under other running tasks — it's regenerable but not disposable + // mid-flight. + const cacheFileSystem = new efs.FileSystem(this, 'DepCacheFs', { + vpc: props.vpc, + encrypted: true, + // The cache is pure derived state; transition cold entries to Infrequent + // Access to keep storage cost negligible without ever losing a warm entry. + lifecyclePolicy: efs.LifecyclePolicy.AFTER_30_DAYS, + outOfInfrequentAccessPolicy: efs.OutOfInfrequentAccessPolicy.AFTER_1_ACCESS, + // Bursting throughput is right for a cache read/write on task boot; the + // access pattern is spiky, not sustained. + throughputMode: efs.ThroughputMode.BURSTING, + removalPolicy: RemovalPolicy.RETAIN, + }); + // The agent container runs as a non-root `agent` user (uid 1000 — see + // agent/Dockerfile). Root the access point at /dep-cache and force that + // owner/perms so the container can read AND write cache entries (atomic + // write-to-temp-then-rename in dep_cache.py) without a root NFS mount. + const cacheAccessPoint = cacheFileSystem.addAccessPoint('DepCacheAp', { + path: '/dep-cache', + createAcl: { ownerUid: String(AGENT_UID), ownerGid: String(AGENT_UID), permissions: '750' }, + posixUser: { uid: String(AGENT_UID), gid: String(AGENT_UID) }, + }); + // The task ENIs mount NFS (TCP 2049) to the EFS mount targets. Allow the + // filesystem SG to accept 2049 from the task SG, and the task SG to egress + // 2049 to it (the task SG is allowAllOutbound:false — HTTPS only by default). + cacheFileSystem.connections.allowDefaultPortFrom( + this.securityGroup, + 'Allow ECS agent tasks to mount the warm-dependency-cache EFS', + ); + this.securityGroup.addEgressRule( + cacheFileSystem.connections.securityGroups[0], + ec2.Port.tcp(NFS_PORT), + 'Allow NFS egress to the warm-dependency-cache EFS mount targets', + ); + const image = ecs.ContainerImage.fromDockerImageAsset(props.agentImageAsset); const makeTaskDef = ( taskDefId: string, @@ -210,6 +272,7 @@ export class EcsAgentCluster extends Construct { memoryLimitMiB: number, extraEnv: Record, ephemeralStorageGiB?: number, + mountDepCache = false, ) => { const def = new ecs.FargateTaskDefinition(this, taskDefId, { cpu, @@ -224,11 +287,34 @@ export class EcsAgentCluster extends Construct { operatingSystemFamily: ecs.OperatingSystemFamily.LINUX, }, }); - def.addContainer(this.containerName, { + // ABCA-691: mount the warm dependency cache into the BUILD def only (the + // PLANNING def is read-only and never installs deps). Transit encryption + + // the access point (IAM-authorized) keep the NFS mount confidential. + if (mountDepCache) { + def.addVolume({ + name: DEP_CACHE_VOLUME_NAME, + 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 }, }); + if (mountDepCache) { + container.addMountPoints({ + containerPath: DEP_CACHE_MOUNT_PATH, + sourceVolume: DEP_CACHE_VOLUME_NAME, + readOnly: false, + }); + } return def; }; @@ -280,7 +366,11 @@ 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); + // ABCA-691: tell the agent where the warm dependency cache is mounted. Its + // presence also arms the cache path in dep_cache.py (absent → cold install + // as before), so the two stay in lockstep by construction. + DEP_CACHE_DIR: DEP_CACHE_MOUNT_PATH, + }, BUILD_TASK_EPHEMERAL_STORAGE_GIB, /* mountDepCache */ true); // PLANNING task def (#299 ECS_RIGHTSIZED_PLANNING) — for read-only workflows // (coding/decompose-v1) that clone + read + emit a plan artifact but NEVER @@ -326,6 +416,25 @@ export class EcsAgentCluster extends Construct { props.artifactsBucket.grantReadWrite(taskRole); } + // ABCA-691: the task role mounts the warm-dependency-cache EFS via its access + // point. EFS IAM authorization (authorizationConfig.iam=ENABLED) requires + // ClientMount + ClientWrite on the filesystem, gated by the access point ARN + // so the task can only ever touch the /dep-cache subtree — not the raw fs + // root. ClientWrite because the agent POPULATES cache entries (atomic + // write-to-temp-then-rename); ClientRootAccess is deliberately NOT granted + // (the access point already pins the POSIX uid). Only the BUILD def mounts it, + // but the role is shared with the PLANNING def — harmless: the planning def + // declares no EFS volume, so the grant is unused there. + taskRole.addToPrincipalPolicy(new iam.PolicyStatement({ + actions: ['elasticfilesystem:ClientMount', 'elasticfilesystem:ClientWrite'], + resources: [cacheFileSystem.fileSystemArn], + conditions: { + StringEquals: { + 'elasticfilesystem:AccessPointArn': cacheAccessPoint.accessPointArn, + }, + }, + })); + // ABCA-488: per-workspace Linear/Jira OAuth tokens live in Secrets Manager // under `bgagent-linear-oauth-*` (written by the CLI at setup). For a // Linear/Jira-channel task the agent resolves that token at startup diff --git a/cdk/test/constructs/ecs-agent-cluster.test.ts b/cdk/test/constructs/ecs-agent-cluster.test.ts index 850b1bad..880191be 100644 --- a/cdk/test/constructs/ecs-agent-cluster.test.ts +++ b/cdk/test/constructs/ecs-agent-cluster.test.ts @@ -459,6 +459,108 @@ describe('EcsAgentCluster construct', () => { }); }); +describe('EcsAgentCluster warm dependency cache (ABCA-691)', () => { + let cacheTemplate: Template; + + beforeAll(() => { + cacheTemplate = createStack().template; + }); + + test('creates an encrypted EFS filesystem with an access point', () => { + cacheTemplate.resourceCountIs('AWS::EFS::FileSystem', 1); + cacheTemplate.hasResourceProperties('AWS::EFS::FileSystem', { + Encrypted: true, + }); + // An access point roots the container at /dep-cache with the agent's POSIX uid. + cacheTemplate.hasResourceProperties('AWS::EFS::AccessPoint', { + RootDirectory: { + Path: '/dep-cache', + CreationInfo: { + OwnerUid: '1000', + OwnerGid: '1000', + Permissions: '750', + }, + }, + PosixUser: { Uid: '1000', Gid: '1000' }, + }); + }); + + test('mounts the cache into the BUILD def at /cache with transit encryption + IAM auth', () => { + const taskDefs = cacheTemplate.findResources('AWS::ECS::TaskDefinition'); + const buildDef = Object.values(taskDefs).find( + d => d.Properties.Cpu === '16384' && d.Properties.Memory === '122880', + ); + expect(buildDef).toBeDefined(); + // The task def declares the EFS volume with encryption + access-point IAM auth. + const volumes = buildDef!.Properties.Volumes ?? []; + const cacheVol = volumes.find((v: { Name: string }) => v.Name === 'dep-cache'); + expect(cacheVol).toBeDefined(); + expect(cacheVol.EFSVolumeConfiguration.TransitEncryption).toBe('ENABLED'); + expect(cacheVol.EFSVolumeConfiguration.AuthorizationConfig.IAM).toBe('ENABLED'); + expect(cacheVol.EFSVolumeConfiguration.AuthorizationConfig.AccessPointId).toBeDefined(); + // The container mounts it read-write at /cache. + const mounts = buildDef!.Properties.ContainerDefinitions[0].MountPoints ?? []; + const cacheMount = mounts.find((m: { ContainerPath: string }) => m.ContainerPath === '/cache'); + expect(cacheMount).toBeDefined(); + expect(cacheMount.SourceVolume).toBe('dep-cache'); + expect(cacheMount.ReadOnly).toBe(false); + // …and the agent is told where the cache is mounted. + const env = buildDef!.Properties.ContainerDefinitions[0].Environment ?? []; + expect(env.some((e: { Name: string; Value: string }) => e.Name === 'DEP_CACHE_DIR' && e.Value === '/cache')).toBe(true); + }); + + test('the read-only PLANNING def does NOT mount the cache (it never installs deps)', () => { + const taskDefs = cacheTemplate.findResources('AWS::ECS::TaskDefinition'); + const planningDef = Object.values(taskDefs).find( + d => d.Properties.Cpu === '2048' && d.Properties.Memory === '8192', + ); + expect(planningDef).toBeDefined(); + const volumes = planningDef!.Properties.Volumes ?? []; + expect(volumes.some((v: { Name: string }) => v.Name === 'dep-cache')).toBe(false); + const mounts = planningDef!.Properties.ContainerDefinitions[0].MountPoints ?? []; + expect(mounts.some((m: { ContainerPath: string }) => m.ContainerPath === '/cache')).toBe(false); + const env = planningDef!.Properties.ContainerDefinitions[0].Environment ?? []; + expect(env.some((e: { Name: string }) => e.Name === 'DEP_CACHE_DIR')).toBe(false); + }); + + test('task role can mount + write the cache EFS, scoped to the access point (never ClientRootAccess)', () => { + const policies = cacheTemplate.findResources('AWS::IAM::Policy'); + let efsStatement: { Action: string | string[]; Resource: unknown; Condition?: 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]; + if (actions.includes('elasticfilesystem:ClientMount')) efsStatement = s; + } + } + expect(efsStatement).toBeDefined(); + const actions = Array.isArray(efsStatement!.Action) ? efsStatement!.Action : [efsStatement!.Action]; + expect(actions).toContain('elasticfilesystem:ClientWrite'); + // Never root access — the access point already pins the POSIX identity. + expect(actions).not.toContain('elasticfilesystem:ClientRootAccess'); + // Gated on the access point ARN so the task can only touch /dep-cache. + const cond = JSON.stringify(efsStatement!.Condition ?? {}); + expect(cond).toContain('elasticfilesystem:AccessPointArn'); + }); + + test('the task security group can egress NFS (2049) to the cache filesystem', () => { + // The task SG is allowAllOutbound:false (HTTPS-only by default) — a 2049 + // egress rule to the EFS mount targets must be added or the mount hangs. + const sgEgress = cacheTemplate.findResources('AWS::EC2::SecurityGroupEgress'); + const has2049 = Object.values(sgEgress).some( + r => r.Properties.FromPort === 2049 && r.Properties.ToPort === 2049, + ); + // CDK may inline the egress on the SG resource OR emit a separate + // SecurityGroupEgress; accept either. + const sgs = cacheTemplate.findResources('AWS::EC2::SecurityGroup'); + const inline2049 = Object.values(sgs).some(sg => + (sg.Properties.SecurityGroupEgress ?? []).some( + (e: { FromPort?: number; ToPort?: number }) => e.FromPort === 2049 && e.ToPort === 2049, + ), + ); + expect(has2049 || inline2049).toBe(true); + }); +}); + describe('EcsAgentCluster payload bucket (#502)', () => { function createWithPayloadBucket(): Template { const app = new App();