Add cached image quickstart #336
@@ -36,10 +36,10 @@ import os
|
||||
import shlex
|
||||
import sys
|
||||
from abc import ABC, abstractmethod
|
||||
from contextlib import AbstractContextManager
|
||||
from contextlib import AbstractContextManager, contextmanager
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import Any, Generic, Sequence, TypeVar
|
||||
from typing import Any, Generator, Generic, Sequence, TypeVar
|
||||
|
||||
from ..agent_provider import AgentProvisionPlan, get_provider, build_agent_provision_plan
|
||||
from ..egress import EgressPlan
|
||||
@@ -435,8 +435,25 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]):
|
||||
prompt file, Dockerfile path, and guest home all live on
|
||||
`agent_provision_plan` — the source of truth."""
|
||||
|
||||
@contextmanager
|
||||
def launch(
|
||||
self, plan: PlanT, *, skip_stale: bool = False
|
||||
) -> Generator[Bottle, None, None]:
|
||||
"""Template: optionally check for stale cached images, then delegate
|
||||
to `_launch_impl`. Pass `skip_stale=True` to bypass the stale check
|
||||
(used by the interactive CLI after the operator confirms)."""
|
||||
|
|
||||
if not skip_stale:
|
||||
self._image_stale_checks(plan)
|
||||
with self._launch_impl(plan) as bottle:
|
||||
yield bottle
|
||||
|
didericis
commented
Implementation should I think have canonical logic for deciding whether to build the image based on the plan. Something like this (forget what build policy flag was called/is probably wrong, use the correct one) Implementation should I think have canonical logic for deciding whether to build the image based on the plan. Something like this (forget what build policy flag was called/is probably wrong, use the correct one)
```py
image = None
if plan.use_cached_images:
images = self._load_cached_images(plan)
else:
images = self._build_images(plan)
with self._launch_impl(plan, images) as bottle:
yield bottle
```
|
||||
|
||||
def _image_stale_checks(self, plan: PlanT) -> None:
|
||||
"""Raise StaleImageError if any cached image used by this plan is stale.
|
||||
No-op default; backends override to call the shared `check_stale*`
|
||||
helpers on their image/artifact timestamps."""
|
||||
|
||||
@abstractmethod
|
||||
def launch(self, plan: PlanT) -> AbstractContextManager[Bottle]:
|
||||
def _launch_impl(self, plan: PlanT) -> AbstractContextManager[Bottle]:
|
||||
"""Build/run the bottle and yield a handle; tear down on exit."""
|
||||
|
||||
def provision(self, plan: PlanT, bottle: "Bottle") -> str | None:
|
||||
|
||||
@@ -85,8 +85,11 @@ class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanup
|
||||
stage_dir=stage_dir,
|
||||
)
|
||||
|
||||
def _image_stale_checks(self, plan: DockerBottlePlan) -> None:
|
||||
_launch.stale_checks(plan)
|
||||
|
||||
@contextmanager
|
||||
def launch(self, plan: DockerBottlePlan) -> Generator[DockerBottle, None, None]:
|
||||
def _launch_impl(self, plan: DockerBottlePlan) -> Generator[DockerBottle, None, None]:
|
||||
with _launch.launch(plan, provision=self.provision) as bottle:
|
||||
yield bottle
|
||||
|
||||
|
||||
@@ -41,7 +41,7 @@ from ...git_gate import (
|
||||
provision_git_gate_dynamic_keys,
|
||||
revoke_git_gate_provisioned_keys,
|
||||
)
|
||||
from ...image_cache import warn_if_stale
|
||||
from ...image_cache import check_stale
|
||||
from ...log import die, info, warn
|
||||
from . import network as network_mod
|
||||
from . import util as docker_mod
|
||||
@@ -105,11 +105,6 @@ def launch(
|
||||
cached_policy = plan.spec.image_policy == "cached"
|
||||
if committed and docker_mod.image_exists(committed):
|
||||
info(f"using committed image {committed!r}")
|
||||
if cached_policy:
|
||||
warn_if_stale(
|
||||
f"agent image {committed!r}",
|
||||
docker_mod.image_created_at(committed),
|
||||
)
|
||||
plan = dataclasses.replace(
|
||||
plan,
|
||||
agent_provision=dataclasses.replace(plan.agent_provision, image=committed),
|
||||
@@ -127,14 +122,6 @@ def launch(
|
||||
)
|
||||
info(f"using cached agent image {plan.image!r}")
|
||||
info(f"using cached sidecar image {SIDECAR_BUNDLE_IMAGE!r}")
|
||||
warn_if_stale(
|
||||
f"agent image {plan.image!r}",
|
||||
docker_mod.image_created_at(plan.image),
|
||||
)
|
||||
warn_if_stale(
|
||||
f"sidecar image {SIDECAR_BUNDLE_IMAGE!r}",
|
||||
docker_mod.image_created_at(SIDECAR_BUNDLE_IMAGE),
|
||||
)
|
||||
else:
|
||||
docker_mod.build_image(
|
||||
plan.image, _REPO_DIR,
|
||||
@@ -236,3 +223,21 @@ def launch(
|
||||
yield bottle
|
||||
finally:
|
||||
teardown()
|
||||
|
||||
|
||||
def stale_checks(plan: DockerBottlePlan) -> None:
|
||||
"""Raise StaleImageError if a cached image is older than the configured
|
||||
threshold. Only runs when image_policy is 'cached'. Called by the backend
|
||||
class's _image_stale_checks before _launch_impl starts any resources."""
|
||||
if plan.spec.image_policy != "cached":
|
||||
return
|
||||
committed = read_committed_image(plan.slug)
|
||||
if committed and docker_mod.image_exists(committed):
|
||||
check_stale(f"agent image {committed!r}", docker_mod.image_created_at(committed))
|
||||
return
|
||||
for label, ref in [
|
||||
("agent image", plan.image),
|
||||
("sidecar image", SIDECAR_BUNDLE_IMAGE),
|
||||
]:
|
||||
if docker_mod.image_exists(ref):
|
||||
check_stale(f"{label} {ref!r}", docker_mod.image_created_at(ref))
|
||||
|
||||
@@ -67,8 +67,11 @@ class MacosContainerBottleBackend(
|
||||
stage_dir=stage_dir,
|
||||
)
|
||||
|
||||
def _image_stale_checks(self, plan: MacosContainerBottlePlan) -> None:
|
||||
_launch.stale_checks(plan)
|
||||
|
||||
@contextmanager
|
||||
def launch(
|
||||
def _launch_impl(
|
||||
self, plan: MacosContainerBottlePlan
|
||||
) -> Generator[MacosContainerBottle, None, None]:
|
||||
with _launch.launch(plan, provision=self.provision) as bottle:
|
||||
|
||||
@@ -32,6 +32,7 @@ from ...git_gate import (
|
||||
provision_git_gate_dynamic_keys,
|
||||
revoke_git_gate_provisioned_keys,
|
||||
)
|
||||
from ...image_cache import check_stale
|
||||
from ...log import die, info, warn
|
||||
from ...supervise import DB_PATH_IN_CONTAINER, SUPERVISE_PORT
|
||||
from ...util import expand_tilde
|
||||
@@ -149,29 +150,53 @@ def _mint_certs(plan: MacosContainerBottlePlan) -> MacosContainerBottlePlan:
|
||||
|
||||
|
||||
def _build_images(plan: MacosContainerBottlePlan) -> MacosContainerBottlePlan:
|
||||
container_mod.build_image(
|
||||
SIDECAR_BUNDLE_IMAGE,
|
||||
_REPO_DIR,
|
||||
dockerfile=SIDECAR_BUNDLE_DOCKERFILE,
|
||||
)
|
||||
cached = plan.spec.image_policy == "cached"
|
||||
|
didericis
commented
This is a smell: you shouldn't call This is a smell: you shouldn't call `_build_images` when the policy is `cached`, you should skip the `_build_images` function (you're not actually building the image). Keep this in mind when refactoring for this: https://gitea.dideric.is/didericis/bot-bottle/pulls/336/files#issuecomment-3100
|
||||
committed = read_committed_image(plan.slug)
|
||||
if committed and container_mod.image_exists(committed):
|
||||
info(f"using committed image {committed!r}")
|
||||
if not cached:
|
||||
container_mod.build_image(SIDECAR_BUNDLE_IMAGE, _REPO_DIR, dockerfile=SIDECAR_BUNDLE_DOCKERFILE)
|
||||
return dataclasses.replace(
|
||||
plan,
|
||||
agent_provision=dataclasses.replace(
|
||||
plan.agent_provision,
|
||||
image=committed,
|
||||
),
|
||||
agent_provision=dataclasses.replace(plan.agent_provision, image=committed),
|
||||
)
|
||||
container_mod.build_image(
|
||||
plan.image,
|
||||
_REPO_DIR,
|
||||
dockerfile=plan.dockerfile_path,
|
||||
)
|
||||
if cached:
|
||||
if not container_mod.image_exists(plan.image):
|
||||
die(
|
||||
f"cached agent image {plan.image!r} not found; "
|
||||
"run without --cached-images to build it"
|
||||
)
|
||||
if not container_mod.image_exists(SIDECAR_BUNDLE_IMAGE):
|
||||
die(
|
||||
f"cached sidecar image {SIDECAR_BUNDLE_IMAGE!r} not found; "
|
||||
"run without --cached-images to build it"
|
||||
)
|
||||
info(f"using cached agent image {plan.image!r}")
|
||||
info(f"using cached sidecar image {SIDECAR_BUNDLE_IMAGE!r}")
|
||||
return plan
|
||||
container_mod.build_image(SIDECAR_BUNDLE_IMAGE, _REPO_DIR, dockerfile=SIDECAR_BUNDLE_DOCKERFILE)
|
||||
container_mod.build_image(plan.image, _REPO_DIR, dockerfile=plan.dockerfile_path)
|
||||
return plan
|
||||
|
||||
|
||||
def stale_checks(plan: MacosContainerBottlePlan) -> None:
|
||||
"""Raise StaleImageError if a cached image is older than the configured
|
||||
threshold. Only runs when image_policy is 'cached'. Called by the backend
|
||||
class's _image_stale_checks before _launch_impl starts any resources."""
|
||||
if plan.spec.image_policy != "cached":
|
||||
return
|
||||
committed = read_committed_image(plan.slug)
|
||||
if committed and container_mod.image_exists(committed):
|
||||
check_stale(f"agent image {committed!r}", container_mod.image_created_at(committed))
|
||||
return
|
||||
for label, ref in [
|
||||
("agent image", plan.image),
|
||||
("sidecar image", SIDECAR_BUNDLE_IMAGE),
|
||||
]:
|
||||
if container_mod.image_exists(ref):
|
||||
check_stale(f"{label} {ref!r}", container_mod.image_created_at(ref))
|
||||
|
||||
|
||||
def _create_networks(
|
||||
internal_network: str,
|
||||
egress_network: str,
|
||||
|
||||
@@ -10,6 +10,7 @@ import shutil
|
||||
import subprocess
|
||||
import tempfile
|
||||
import time
|
||||
from datetime import datetime, timezone
|
||||
from typing import Iterable
|
||||
|
||||
from ...log import die, info
|
||||
@@ -458,6 +459,39 @@ def image_id(ref: str) -> str:
|
||||
raise AssertionError("unreachable")
|
||||
|
||||
|
||||
def image_created_at(ref: str) -> datetime:
|
||||
"""Return the image creation timestamp as an aware UTC datetime.
|
||||
|
||||
Parses the `created` field from `container image inspect` JSON output."""
|
||||
result = subprocess.run(
|
||||
[_CONTAINER, "image", "inspect", ref],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=False,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
die(
|
||||
f"container image inspect for {ref!r} failed: "
|
||||
f"{(result.stderr or '').strip() or '<no stderr>'}"
|
||||
)
|
||||
try:
|
||||
data = json.loads(result.stdout or "{}")
|
||||
except json.JSONDecodeError as exc:
|
||||
die(f"container image inspect for {ref!r} returned malformed JSON: {exc}")
|
||||
if isinstance(data, list) and data:
|
||||
data = data[0]
|
||||
if isinstance(data, dict):
|
||||
value = data.get("created") or data.get("Created")
|
||||
if isinstance(value, str) and value:
|
||||
try:
|
||||
ts = value.rstrip("Z")
|
||||
return datetime.fromisoformat(ts).replace(tzinfo=timezone.utc)
|
||||
except ValueError:
|
||||
pass
|
||||
die(f"container image inspect for {ref!r} did not include a creation timestamp")
|
||||
raise AssertionError("unreachable")
|
||||
|
||||
|
||||
def save(ref: str, output: str) -> None:
|
||||
subprocess.run([_CONTAINER, "image", "save", ref, "-o", output], check=True)
|
||||
|
||||
|
||||
+32
-23
@@ -36,6 +36,7 @@ from ..bottle_state import (
|
||||
is_preserved,
|
||||
mark_preserved,
|
||||
)
|
||||
from ..image_cache import StaleImageError
|
||||
from ..log import info, die
|
||||
from ..manifest import Manifest, ManifestIndex
|
||||
from ._common import PROG, USER_CWD, read_tty_line
|
||||
@@ -555,30 +556,38 @@ def _launch_bottle(
|
||||
return 0
|
||||
|
||||
backend = get_bottle_backend(backend_name)
|
||||
with backend.launch(plan) as bottle:
|
||||
agent_provider_template = getattr(plan, "agent_provider_template", "claude")
|
||||
extra_args: tuple[str, ...] = ()
|
||||
if headless_prompt_text:
|
||||
extra_args = tuple(
|
||||
get_provider(agent_provider_template).headless_prompt(
|
||||
headless_prompt_text
|
||||
skip_stale = False
|
||||
while True:
|
||||
try:
|
||||
with backend.launch(plan, skip_stale=skip_stale) as bottle:
|
||||
agent_provider_template = getattr(plan, "agent_provider_template", "claude")
|
||||
extra_args: tuple[str, ...] = ()
|
||||
if headless_prompt_text:
|
||||
extra_args = tuple(
|
||||
get_provider(agent_provider_template).headless_prompt(
|
||||
headless_prompt_text
|
||||
)
|
||||
)
|
||||
exit_code = attach_agent(
|
||||
bottle,
|
||||
agent_provider_template=agent_provider_template,
|
||||
startup_args=plan.agent_provision.startup_args + extra_args,
|
||||
)
|
||||
)
|
||||
exit_code = attach_agent(
|
||||
bottle,
|
||||
agent_provider_template=agent_provider_template,
|
||||
startup_args=plan.agent_provision.startup_args + extra_args,
|
||||
)
|
||||
info(
|
||||
f"session ended (exit {exit_code}); "
|
||||
f"container {bottle.name} will be removed"
|
||||
)
|
||||
# While the container is still alive: always snapshot the
|
||||
# transcript and — if the agent exited non-zero — mark
|
||||
# the state for preservation. This picks up crashes /
|
||||
# Ctrl-Cs / OOM kills before cleanup removes the state dir.
|
||||
if agent_provider_template == "claude":
|
||||
capture_claude_session_state(identity, exit_code)
|
||||
info(
|
||||
f"session ended (exit {exit_code}); "
|
||||
f"container {bottle.name} will be removed"
|
||||
)
|
||||
if agent_provider_template == "claude":
|
||||
capture_claude_session_state(identity, exit_code)
|
||||
break
|
||||
except StaleImageError as exc:
|
||||
if assume_yes:
|
||||
die(str(exc))
|
||||
sys.stderr.write(f"bot-bottle: {exc}\nLaunch anyway? [y/N] ")
|
||||
sys.stderr.flush()
|
||||
if read_tty_line() not in ("y", "Y", "yes", "YES"):
|
||||
break
|
||||
skip_stale = True
|
||||
return 0
|
||||
finally:
|
||||
# PRD 0018 chunk 2: prepare now writes the bottle's bind-mount
|
||||
|
||||
+15
-11
@@ -1,4 +1,4 @@
|
||||
"""Shared helpers for cached-image quickstart warnings."""
|
||||
"""Shared helpers for cached-image quickstart stale checks."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -7,13 +7,19 @@ from pathlib import Path
|
||||
|
||||
try:
|
||||
from .config_store import ConfigStore
|
||||
from .log import warn
|
||||
except ImportError:
|
||||
from config_store import ConfigStore # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
|
||||
from log import warn # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
|
||||
|
||||
|
||||
def warn_if_stale(label: str, created_at: datetime) -> None:
|
||||
class StaleImageError(Exception):
|
||||
"""Raised when a cached image or artifact exceeds the configured staleness
|
||||
threshold. Callers can catch this to prompt interactively; headless paths
|
||||
let it propagate as a fatal error."""
|
||||
|
||||
|
||||
def check_stale(label: str, created_at: datetime) -> None:
|
||||
"""Raise StaleImageError if `created_at` is older than the configured
|
||||
stale-warning threshold. Negative threshold disables the check."""
|
||||
threshold_days = ConfigStore().cached_image_stale_warning_days()
|
||||
if threshold_days < 0:
|
||||
return
|
||||
@@ -22,17 +28,15 @@ def warn_if_stale(label: str, created_at: datetime) -> None:
|
||||
age = now - created
|
||||
if age.total_seconds() <= threshold_days * 86400:
|
||||
return
|
||||
warn(
|
||||
raise StaleImageError(
|
||||
f"cached {label} is {age.days} day(s) old; "
|
||||
"quickstart does not verify it matches the current Dockerfile/context"
|
||||
)
|
||||
|
||||
|
||||
def warn_if_stale_path(label: str, path: Path) -> None:
|
||||
warn_if_stale(
|
||||
label,
|
||||
datetime.fromtimestamp(path.stat().st_mtime, tz=timezone.utc),
|
||||
)
|
||||
def check_stale_path(label: str, path: Path) -> None:
|
||||
"""Raise StaleImageError if `path`'s mtime exceeds the staleness threshold."""
|
||||
check_stale(label, datetime.fromtimestamp(path.stat().st_mtime, tz=timezone.utc))
|
||||
|
||||
|
||||
__all__ = ["warn_if_stale", "warn_if_stale_path"]
|
||||
__all__ = ["StaleImageError", "check_stale", "check_stale_path"]
|
||||
|
||||
@@ -105,10 +105,6 @@ class TestLaunchCommittedImage(unittest.TestCase):
|
||||
launch_mod.docker_mod, "image_exists", return_value=image_present,
|
||||
), mock.patch.object(
|
||||
launch_mod.docker_mod, "build_image", side_effect=fake_build,
|
||||
), mock.patch.object(
|
||||
launch_mod.docker_mod, "image_created_at",
|
||||
), mock.patch.object(
|
||||
launch_mod, "warn_if_stale",
|
||||
), mock.patch.object(
|
||||
launch_mod, "egress_tls_init",
|
||||
return_value=(Path("/egress_ca"), Path("/egress_cert")),
|
||||
|
||||
@@ -81,7 +81,7 @@ def _plan(
|
||||
provisioned_env={"CODEX_HOME": "/run/codex-home"},
|
||||
)
|
||||
return cast(MacosContainerBottlePlan, SimpleNamespace(
|
||||
spec=SimpleNamespace(),
|
||||
spec=SimpleNamespace(image_policy="fresh"),
|
||||
manifest=_MANIFEST,
|
||||
stage_dir=stage_dir,
|
||||
slug="dev-abc",
|
||||
@@ -292,7 +292,7 @@ class TestMacosContainerLaunchArgv(unittest.TestCase):
|
||||
|
||||
def _build_plan(stage_dir: Path) -> MacosContainerBottlePlan:
|
||||
return MacosContainerBottlePlan(
|
||||
spec=cast(BottleSpec, SimpleNamespace()),
|
||||
spec=cast(BottleSpec, SimpleNamespace(image_policy="fresh")),
|
||||
manifest=_MANIFEST,
|
||||
stage_dir=stage_dir,
|
||||
git_gate_plan=cast(GitGatePlan, SimpleNamespace(upstreams=())),
|
||||
|
||||
@@ -187,56 +187,6 @@ class TestAgentFromPath(unittest.TestCase):
|
||||
dockerfile="/repo/Dockerfile",
|
||||
)
|
||||
|
||||
def test_cached_policy_uses_existing_artifact_without_build(self):
|
||||
with tempfile.TemporaryDirectory(prefix="cached-smolmachine.") as tmp:
|
||||
digest = "abcdef0123456789"
|
||||
artifact = Path(tmp) / f"{digest}.smolmachine.smolmachine"
|
||||
artifact.write_text("")
|
||||
plan = SimpleNamespace(
|
||||
slug="dev-abc12",
|
||||
agent_image="bot-bottle-claude:latest",
|
||||
spec=SimpleNamespace(image_policy="cached"),
|
||||
)
|
||||
with patch.object(
|
||||
_launch_mod, "_SMOLMACHINE_CACHE_DIR", Path(tmp),
|
||||
), patch.object(
|
||||
_launch_mod, "read_committed_image", return_value="",
|
||||
), patch.object(
|
||||
_launch_mod.docker_mod, "image_exists", return_value=True,
|
||||
), patch.object(
|
||||
_launch_mod.docker_mod, "image_id",
|
||||
return_value=f"sha256:{digest}fffffffffffffffff",
|
||||
), patch.object(
|
||||
_launch_mod, "warn_if_stale_path",
|
||||
), patch.object(
|
||||
_launch_mod, "_ensure_smolmachine",
|
||||
) as ensure:
|
||||
result = _launch_mod._agent_from_path(cast(Any, plan))
|
||||
|
||||
self.assertEqual(artifact, result)
|
||||
ensure.assert_not_called()
|
||||
|
||||
def test_cached_policy_dies_when_artifact_missing(self):
|
||||
with tempfile.TemporaryDirectory(prefix="cached-smolmachine.") as tmp:
|
||||
plan = SimpleNamespace(
|
||||
slug="dev-abc12",
|
||||
agent_image="bot-bottle-claude:latest",
|
||||
spec=SimpleNamespace(image_policy="cached"),
|
||||
)
|
||||
with patch.object(
|
||||
_launch_mod, "_SMOLMACHINE_CACHE_DIR", Path(tmp),
|
||||
), patch.object(
|
||||
_launch_mod, "read_committed_image", return_value="",
|
||||
), patch.object(
|
||||
_launch_mod.docker_mod, "image_exists", return_value=True,
|
||||
), patch.object(
|
||||
_launch_mod.docker_mod, "image_id",
|
||||
return_value="sha256:abcdef0123456789fffffffffffffffff",
|
||||
):
|
||||
from bot_bottle.log import Die
|
||||
with self.assertRaises(Die):
|
||||
_launch_mod._agent_from_path(cast(Any, plan))
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user
Actually this was a bad idea, the check for a stale image should be in prelaunch, not here. remove the
skip_staleoption and move the logic where we check for a stale image (we also probably shouldn't throw an error: instead, in the plan, we should include a field that says the image is stale, and have the cli check the plan during the preflight confirmation for confirming whether to continue using the cached image or to rebuild it)