From f6df85a8cd3cb8fd170e6e56df0442525b24d20c Mon Sep 17 00:00:00 2001 From: didericis Date: Mon, 27 Jul 2026 13:18:15 -0400 Subject: [PATCH] fix(fc): ship agent-image build context files to the infra VM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent images build with buildah inside the infra VM, and `_build_in_infra` shipped only the Dockerfile into an empty `{ctx}/ctx` context — on the documented assumption that agent Dockerfiles COPY nothing from the context. The "pin & verify image inputs" work broke that: the codex Dockerfile COPYs `codex-package_SHA256SUMS` and the claude/pi Dockerfiles COPY their npm `package.json`/`package-lock.json`, so any fresh agent image build fails with `COPY ...: no such file or directory` (existing agents only survive on a rootfs cached from before the change). Parse each Dockerfile's context COPY sources, tar those committed files from the build root into the VM-side `{ctx}/ctx`, and fold their content into the rootfs cache digest so a repinned input rebuilds instead of reusing stale bytes. `COPY --from=` and absolute/traversing sources are excluded. Closes #538. Co-Authored-By: Claude Opus 4.8 --- .../backend/firecracker/image_builder.py | 106 ++++++++++++++++-- tests/unit/test_firecracker_image_builder.py | 86 ++++++++++++++ 2 files changed, 184 insertions(+), 8 deletions(-) diff --git a/bot_bottle/backend/firecracker/image_builder.py b/bot_bottle/backend/firecracker/image_builder.py index 9930415d..cf9585e2 100644 --- a/bot_bottle/backend/firecracker/image_builder.py +++ b/bot_bottle/backend/firecracker/image_builder.py @@ -19,6 +19,7 @@ from __future__ import annotations import fcntl import hashlib import os +import re import shlex import shutil import subprocess @@ -41,19 +42,69 @@ _BUILD_TIMEOUT_SECONDS = 900.0 def _dockerfile_hash(dockerfile: Path) -> str: - """The Dockerfile's content hash. The shipped agent Dockerfiles COPY - nothing from the build context (see .dockerignore), so their content fully - determines the built image; a Dockerfile that adds COPY will want the - context folded in here too.""" + """The Dockerfile's content hash. Agent Dockerfiles mostly COPY nothing + from the build context, so their text nearly determines the built image; + any files they *do* COPY are folded into `_rootfs_digest` (so a changed + input busts the cache) and shipped to the VM-side context by + `_send_build_context`.""" return hashlib.sha256(dockerfile.read_bytes()).hexdigest()[:16] +def _context_copy_sources(dockerfile: Path) -> list[str]: + """The build-context-relative paths a Dockerfile ``COPY``s in. + + Agent Dockerfiles are meant to COPY nothing from the context (the VM-side + build ships only the Dockerfile), but one may pin an input by COPYing a + committed file — a checksum list, an npm lockfile. Return those source + paths so the builder can both ship them to the VM context and fold them + into the cache key. ``COPY --from=`` reads a build stage, not the + context, so it is excluded; the JSON/exec COPY form is unused by the + shipped images and is skipped rather than mis-parsed.""" + joined = re.sub(r"\\\n", " ", dockerfile.read_text(encoding="utf-8")) + sources: list[str] = [] + for line in joined.splitlines(): + stripped = line.strip() + if not re.match(r"(?i)^COPY\s", stripped): + continue + tokens = stripped.split()[1:] + if any(t.startswith("--from=") for t in tokens): + continue + args = [t for t in tokens if not t.startswith("--")] + if len(args) < 2 or args[0].startswith("["): + continue + sources.extend(args[:-1]) + return sources + + +def _context_files(dockerfile: Path) -> list[tuple[str, Path]]: + """``(context-relative path, host path)`` for every existing file a + Dockerfile COPYs from the build root — globs expanded, sorted, de-duped. + Absolute or traversing (`..`) sources are dropped: the shipped context + only ever mirrors files under the build root.""" + root = resources.build_root() + resolved: dict[str, Path] = {} + for src in _context_copy_sources(dockerfile): + if src.startswith("/") or ".." in Path(src).parts: + continue + if any(ch in src for ch in "*?["): + matches = [p for p in root.glob(src) if p.is_file()] + else: + candidate = root / src + matches = [candidate] if candidate.is_file() else [] + for path in matches: + resolved[str(path.relative_to(root))] = path + return sorted(resolved.items()) + + def _rootfs_digest(dockerfile: Path) -> str: - """Cache key for the built AND boot-injected agent rootfs. Two inputs - determine the on-disk rootfs: the Dockerfile (the image) and the guest init - injected into it (`util._GUEST_INIT`). Folding the init in means a fix to + """Cache key for the built AND boot-injected agent rootfs. Its inputs are + the Dockerfile (the image), the centralized build args, the guest init + injected into it (`util._GUEST_INIT`), and the content of any files the + Dockerfile COPYs from the build context. Folding the init in means a fix to it — e.g. making /tmp world-writable — busts the cache instead of silently - reusing a stale rootfs built with the old init.""" + reusing a stale rootfs built with the old init; folding the COPYed context + files in means a repinned input (e.g. a changed checksum list) rebuilds + rather than reusing a rootfs baked from the old bytes.""" h = hashlib.sha256() h.update(_dockerfile_hash(dockerfile).encode()) h.update(b"\0") @@ -63,6 +114,11 @@ def _rootfs_digest(dockerfile: Path) -> str: h.update(value.encode()) h.update(b"\0") h.update(util._GUEST_INIT.encode()) + for rel, path in _context_files(dockerfile): + h.update(b"\0") + h.update(rel.encode()) + h.update(b"\0") + h.update(path.read_bytes()) return h.hexdigest()[:16] @@ -154,6 +210,7 @@ def _build_in_infra( if prep.returncode != 0: die(f"preparing build dir in the infra VM failed: {prep.stderr.strip()}") _send_dockerfile(key, ip, dockerfile, ctx) + _send_build_context(key, ip, dockerfile, ctx) _buildah_build( key, ip, @@ -197,6 +254,39 @@ def _send_dockerfile(private_key: Path, guest_ip: str, dockerfile: Path, ctx: st f"{proc.stderr.decode(errors='replace').strip()}") +def _send_build_context(private_key: Path, guest_ip: str, dockerfile: Path, ctx: str) -> None: + """Ship the files ``dockerfile`` COPYs from the build root into the infra + VM's ``{ctx}/ctx``, preserving their build-root-relative paths. + + Usually a no-op — agent Dockerfiles COPY nothing — so `{ctx}/ctx` stays the + empty context the build otherwise runs against. It exists so a Dockerfile + that pins an input by COPYing a committed file (a checksum list, an npm + lockfile) still finds that file in the VM-side context. Streamed as a tar + so directories and multiple files land in one round trip.""" + files = _context_files(dockerfile) + if not files: + return + root = resources.build_root() + rels = [rel for rel, _ in files] + tar = subprocess.Popen( + ["tar", "-C", str(root), "-cf", "-", *rels], stdout=subprocess.PIPE, + ) + try: + proc = subprocess.run( + util.ssh_base_argv(private_key, guest_ip) + [f"tar -C {ctx}/ctx -xf -"], + stdin=tar.stdout, capture_output=True, timeout=120, check=False, + ) + finally: + if tar.stdout is not None: + tar.stdout.close() + tar.wait() + if tar.returncode != 0: + die(f"packing the agent build context failed (tar exit {tar.returncode})") + if proc.returncode != 0: + die("sending the agent build context to the infra VM failed: " + f"{proc.stderr.decode(errors='replace').strip() or ''}") + + def _buildah_build( private_key: Path, guest_ip: str, diff --git a/tests/unit/test_firecracker_image_builder.py b/tests/unit/test_firecracker_image_builder.py index d8c1c1d1..4bcb5c88 100644 --- a/tests/unit/test_firecracker_image_builder.py +++ b/tests/unit/test_firecracker_image_builder.py @@ -7,6 +7,7 @@ the smoke-test no-op — the logic that must hold without a VM. from __future__ import annotations +import subprocess import tempfile import unittest from pathlib import Path @@ -91,6 +92,91 @@ class TestBuildAgentRootfsDir(unittest.TestCase): self.assertNotEqual(first, second) +class TestBuildContext(unittest.TestCase): + """The COPY-source parsing + context shipping that lets a Dockerfile pin an + input by COPYing a committed file (codex checksum list, claude/pi npm + lockfiles) through the otherwise-empty VM-side build context.""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = Path(self._tmp.name) + self.addCleanup(self._tmp.cleanup) + codex = self.root / "bot_bottle" / "contrib" / "codex" + codex.mkdir(parents=True) + self.sums = codex / "codex-package_SHA256SUMS" + self.sums.write_text("aaaa codex-package-x86_64-unknown-linux-musl.tar.gz\n") + self.dockerfile = self.root / "Dockerfile" + self.dockerfile.write_text( + "FROM node:22-slim\n" + "COPY --chown=node:node " + "bot_bottle/contrib/codex/codex-package_SHA256SUMS /tmp/x\n" + ) + + def _patch_root(self): + return patch.object( + image_builder.resources, "build_root", return_value=self.root) + + def test_copy_sources_parses_flags_and_continuations(self): + df = self.root / "Multi" + df.write_text( + "FROM x\n" + "COPY a/one.json \\\n a/two.json /dest/\n" + "COPY --from=builder /built /built\n" # excluded: build stage + "COPY --chown=n:n b/three /dest\n" + ) + self.assertEqual( + image_builder._context_copy_sources(df), + ["a/one.json", "a/two.json", "b/three"], + ) + + def test_context_files_resolves_existing_under_root(self): + with self._patch_root(): + files = image_builder._context_files(self.dockerfile) + self.assertEqual( + [rel for rel, _ in files], + ["bot_bottle/contrib/codex/codex-package_SHA256SUMS"], + ) + + def test_context_files_drops_missing_and_traversal(self): + df = self.root / "Bad" + df.write_text("FROM x\nCOPY ../escape /d\nCOPY does/not/exist /d\n") + with self._patch_root(): + self.assertEqual(image_builder._context_files(df), []) + + def test_rootfs_digest_tracks_context_file_content(self): + with self._patch_root(), \ + patch.object(image_builder.util, "cache_dir", return_value=self.root): + first = image_builder._rootfs_digest(self.dockerfile) + self.sums.write_text("bbbb codex-package-x86_64-unknown-linux-musl.tar.gz\n") + second = image_builder._rootfs_digest(self.dockerfile) + self.assertNotEqual(first, second) + + def test_send_build_context_noop_without_copy(self): + df = self.root / "None" + df.write_text("FROM x\n") + with self._patch_root(), \ + patch.object(image_builder.subprocess, "Popen") as popen: + image_builder._send_build_context(Path("/k"), "10.0.0.1", df, "/tmp/c") + popen.assert_not_called() + + def test_send_build_context_streams_copied_files_to_vm(self): + completed = subprocess.CompletedProcess([], 0, stdout=b"", stderr=b"") + with self._patch_root(), \ + patch.object(image_builder.subprocess, "Popen") as popen, \ + patch.object(image_builder.subprocess, "run", + return_value=completed) as run: + popen.return_value.stdout = None + popen.return_value.returncode = 0 + popen.return_value.wait.return_value = 0 + image_builder._send_build_context( + Path("/k"), "10.0.0.1", self.dockerfile, "/tmp/c") + tar_argv = popen.call_args.args[0] + self.assertEqual(tar_argv[:3], ["tar", "-C", str(self.root)]) + self.assertIn( + "bot_bottle/contrib/codex/codex-package_SHA256SUMS", tar_argv) + self.assertIn("tar -C /tmp/c/ctx -xf -", run.call_args.args[0][-1]) + + class TestSmokeTest(unittest.TestCase): def test_buildah_receives_centralized_image_build_args(self): with patch.object(