From 0b1ef36fe2372b4fe29e3009948af139801e8e65 Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Sat, 29 Aug 2026 19:22:22 +0100 Subject: [PATCH 1/4] Cache function runtime base image layers between builds baseImageForArch builds the runtime image from remote.Image and attaches its layers with LayerByDigest. Those layers are lazy: nothing reads them until tarball.MultiWrite serialises the built images, so every build re-fetches the whole base image from the registry. For a two-architecture distroless base that is over a hundred megabytes, spread across dozens of small requests, and it is why "Writing packages to disk" dominates a build that otherwise spends most of its time idle. Wrap the base image in go-containerregistry's filesystem cache, keyed by layer digest, so a repeat build reads those layers locally. The cache directory is threaded from the command layer through the Builder and BuildContext rather than resolved inside the builders, so callers stay in control and tests can disable it by leaving it empty. It sits beside the xpkg cache rather than inside it, since the two hold different artifacts and are pruned on different terms. Measured on a two-architecture Python project, warm registry, from an empty cache: cold write phase 68.1s, total build 108.3s warm write phase 0.8s, total build 34.5s The built images are unchanged: a cached build produces byte-identical function image layers to an uncached one. Only the configuration package layer differs, and that differs between two uncached builds too, so it is pre-existing nondeterminism rather than an effect of caching. Layers are content-addressed, so a cache hit cannot be stale. Note that go-containerregistry's filesystem cache has no garbage collection, so the directory grows as base images move; pruning it is left to a follow up. Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 5 --- cmd/crossplane/project/build.go | 1 + cmd/crossplane/project/run.go | 1 + cmd/crossplane/render/op/cmd.go | 1 + cmd/crossplane/render/xr/cmd.go | 1 + internal/project/build.go | 20 +++++++++--- internal/project/functions/basecache.go | 34 +++++++++++++++++++++ internal/project/functions/build.go | 4 +++ internal/project/functions/go_templating.go | 2 +- internal/project/functions/kcl.go | 21 +++++++++++-- internal/project/functions/python.go | 2 +- 10 files changed, 78 insertions(+), 9 deletions(-) create mode 100644 internal/project/functions/basecache.go diff --git a/cmd/crossplane/project/build.go b/cmd/crossplane/project/build.go index e2c5958a..9f62d420 100644 --- a/cmd/crossplane/project/build.go +++ b/cmd/crossplane/project/build.go @@ -146,6 +146,7 @@ func (c *buildCmd) Run(logger logging.Logger, sp terminal.SpinnerPrinter, cfg *c project.BuildWithSchemaManager(schemaMgr), project.BuildWithDependencyManager(depMgr), project.BuildWithTempDir(tempDir), + project.BuildWithBaseImageCacheDir(functions.DefaultBaseImageCacheDir()), ) var imgMap project.ImageTagMap diff --git a/cmd/crossplane/project/run.go b/cmd/crossplane/project/run.go index 2b2601e0..5b829656 100644 --- a/cmd/crossplane/project/run.go +++ b/cmd/crossplane/project/run.go @@ -200,6 +200,7 @@ func (c *runCmd) Run(logger logging.Logger, sp terminal.SpinnerPrinter, cfg *con project.BuildWithSchemaManager(schemaMgr), project.BuildWithDependencyManager(depMgr), project.BuildWithTempDir(tempDir), + project.BuildWithBaseImageCacheDir(functions.DefaultBaseImageCacheDir()), ) var ( diff --git a/cmd/crossplane/render/op/cmd.go b/cmd/crossplane/render/op/cmd.go index ee158c63..70c3b4be 100644 --- a/cmd/crossplane/render/op/cmd.go +++ b/cmd/crossplane/render/op/cmd.go @@ -398,6 +398,7 @@ func (c *Cmd) loadFunctions(ctx context.Context, log logging.Logger, sp terminal project.BuildWithSchemaManager(schemaMgr), project.BuildWithDependencyManager(depMgr), project.BuildWithTempDir(tempDir), + project.BuildWithBaseImageCacheDir(functions.DefaultBaseImageCacheDir()), ) imgMap, err := b.Build(ctx, proj, projFS, diff --git a/cmd/crossplane/render/xr/cmd.go b/cmd/crossplane/render/xr/cmd.go index 7da457d5..eb08f5d2 100644 --- a/cmd/crossplane/render/xr/cmd.go +++ b/cmd/crossplane/render/xr/cmd.go @@ -474,6 +474,7 @@ func (c *Cmd) loadFunctions(ctx context.Context, log logging.Logger, sp terminal project.BuildWithSchemaManager(schemaMgr), project.BuildWithDependencyManager(depMgr), project.BuildWithTempDir(tempDir), + project.BuildWithBaseImageCacheDir(functions.DefaultBaseImageCacheDir()), ) imgMap, err := b.Build(ctx, proj, projFS, diff --git a/internal/project/build.go b/internal/project/build.go index 49fe92a2..81de2ca7 100644 --- a/internal/project/build.go +++ b/internal/project/build.go @@ -103,6 +103,14 @@ func BuildWithDependencyManager(m *dependency.Manager) BuilderOption { } } +// BuildWithBaseImageCacheDir sets where function runtime base image layers are +// cached between builds. Empty disables caching. +func BuildWithBaseImageCacheDir(dir string) BuilderOption { + return func(b *Builder) { + b.baseImageCacheDir = dir + } +} + // BuildWithTempDir sets a directory the builder can use to hold temporary // files. The images returned from Build() may depend on this directory, so // callers should not remove it until they have finished consuming the images. @@ -161,6 +169,7 @@ type Builder struct { schemaManager *manager.Manager dependencyManager *dependency.Manager tempDir string + baseImageCacheDir string } // Build builds a project into a set of packages. It returns a map containing @@ -540,11 +549,12 @@ func (b *Builder) buildDirectoryRuntime(ctx context.Context, projectFS afero.Fs, } imgs, err := fnBuilder.Build(ctx, functions.BuildContext{ - ProjectFS: projectFS, - FunctionPath: filepath.Join(project.Spec.Paths.Functions, dir.Name), - SchemasPath: project.Spec.Paths.Schemas, - Architectures: project.Spec.Architectures, - OSBasePath: fnBasePath, + ProjectFS: projectFS, + FunctionPath: filepath.Join(project.Spec.Paths.Functions, dir.Name), + SchemasPath: project.Spec.Paths.Schemas, + Architectures: project.Spec.Architectures, + OSBasePath: fnBasePath, + BaseImageCacheDir: b.baseImageCacheDir, }) if err != nil { return nil, errors.Wrap(err, "failed to build runtime images") diff --git a/internal/project/functions/basecache.go b/internal/project/functions/basecache.go new file mode 100644 index 00000000..1f309871 --- /dev/null +++ b/internal/project/functions/basecache.go @@ -0,0 +1,34 @@ +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package functions + +import ( + "os" + "path/filepath" +) + +// DefaultBaseImageCacheDir returns the default per-user cache directory for +// function runtime base image layers. It sits beside the xpkg cache rather +// than inside it, since the two hold different kinds of artifact and are +// pruned on different terms. +func DefaultBaseImageCacheDir() string { + base, err := os.UserCacheDir() + if err != nil { + base = os.TempDir() + } + return filepath.Join(base, "crossplane", "base-images") +} diff --git a/internal/project/functions/build.go b/internal/project/functions/build.go index 09c1751f..cd2795db 100644 --- a/internal/project/functions/build.go +++ b/internal/project/functions/build.go @@ -85,6 +85,10 @@ type BuildContext struct { // OSBasePath is the absolute on-disk path of the function directory. // Used by FSToTar to resolve symlinks. OSBasePath string + // BaseImageCacheDir is where runtime base image layers are cached between + // builds. Empty disables caching, so layers are fetched from the registry + // every time. + BaseImageCacheDir string } // FunctionFS returns a filesystem rooted at the function's source directory. diff --git a/internal/project/functions/go_templating.go b/internal/project/functions/go_templating.go index ce5a7710..1e7f104c 100644 --- a/internal/project/functions/go_templating.go +++ b/internal/project/functions/go_templating.go @@ -111,7 +111,7 @@ func (b *goTemplatingBuilder) Build(ctx context.Context, c BuildContext) ([]v1.I eg, _ := errgroup.WithContext(ctx) for i, arch := range c.Architectures { eg.Go(func() error { - baseImg, err := baseImageForArch(baseRef, arch, b.transport) + baseImg, err := baseImageForArch(baseRef, arch, b.transport, c.BaseImageCacheDir) if err != nil { return errors.Wrap(err, "failed to fetch go-templating base image") } diff --git a/internal/project/functions/kcl.go b/internal/project/functions/kcl.go index d92cbe8f..6b77a429 100644 --- a/internal/project/functions/kcl.go +++ b/internal/project/functions/kcl.go @@ -27,6 +27,7 @@ import ( "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/cache" "github.com/google/go-containerregistry/pkg/v1/empty" "github.com/google/go-containerregistry/pkg/v1/mutate" "github.com/google/go-containerregistry/pkg/v1/remote" @@ -85,7 +86,7 @@ func (b *kclBuilder) Build(ctx context.Context, c BuildContext) ([]v1.Image, err eg, _ := errgroup.WithContext(ctx) for i, arch := range c.Architectures { eg.Go(func() error { - baseImg, err := baseImageForArch(baseRef, arch, b.transport) + baseImg, err := baseImageForArch(baseRef, arch, b.transport, c.BaseImageCacheDir) if err != nil { return errors.Wrap(err, "failed to fetch KCL base image") } @@ -130,7 +131,16 @@ func (b *kclBuilder) Build(ctx context.Context, c BuildContext) ([]v1.Image, err // baseImageForArch pulls the image with the given ref, and returns a version of // it suitable for use as a function base image. Package and examples layers // will be removed if present. -func baseImageForArch(ref name.Reference, arch string, transport http.RoundTripper) (v1.Image, error) { +// baseImageForArch fetches the runtime base image for one architecture and +// strips the layers that belong to the package rather than the runtime. +// +// The layers it returns are lazy: their bytes are not read until the built +// image is written out. Left unwrapped that means re-fetching the whole base +// image from the registry on every build, which for a two-architecture +// distroless base is over a hundred megabytes spread across dozens of small +// requests. cacheDir, when set, backs those layers with a content-addressed +// on-disk cache so a repeat build reads them locally instead. +func baseImageForArch(ref name.Reference, arch string, transport http.RoundTripper, cacheDir string) (v1.Image, error) { img, err := remote.Image(ref, remote.WithPlatform(v1.Platform{ OS: "linux", Architecture: arch, @@ -139,6 +149,13 @@ func baseImageForArch(ref name.Reference, arch string, transport http.RoundTripp return nil, errors.Wrap(err, "failed to pull image") } + if cacheDir != "" { + // Layers are addressed by digest, so a cache hit cannot be stale. A + // failure to write the cache is not fatal: the library falls back to + // the remote layer. + img = cache.Image(img, cache.NewFilesystemCache(cacheDir)) + } + cfg, err := img.ConfigFile() if err != nil { return nil, errors.Wrap(err, "failed to get config from image") diff --git a/internal/project/functions/python.go b/internal/project/functions/python.go index c345f7c8..142efd79 100644 --- a/internal/project/functions/python.go +++ b/internal/project/functions/python.go @@ -137,7 +137,7 @@ func (b *pythonBuilder) Build(ctx context.Context, c BuildContext) ([]v1.Image, eg, _ := errgroup.WithContext(ctx) for i, arch := range c.Architectures { eg.Go(func() error { - baseImg, err := baseImageForArch(runtimeRef, arch, b.transport) + baseImg, err := baseImageForArch(runtimeRef, arch, b.transport, c.BaseImageCacheDir) if err != nil { return errors.Wrap(err, "failed to fetch python runtime base image") } From 550c8df0593341c1407e9b77ec0a3c3f2e258a8a Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Sat, 29 Aug 2026 19:43:36 +0100 Subject: [PATCH 2/4] Document the base image cache and its growth The cache is invisible to users: it changes how long a build takes and consumes disk under their user cache directory, with nothing in the CLI saying so. Describe it in `project build`'s help, including why the first build is slower than the ones after it and that deleting the directory is safe. Say plainly that nothing prunes it. Layers are keyed by content digest, so entries are never stale, but they are also never replaced: every base image version a user builds against accumulates. Record the same caveat at DefaultBaseImageCacheDir, where someone adding a retention policy will be looking. Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 5 --- cmd/crossplane/project/help/build.md | 13 +++++++++++++ internal/project/functions/basecache.go | 8 ++++++++ 2 files changed, 21 insertions(+) diff --git a/cmd/crossplane/project/help/build.md b/cmd/crossplane/project/help/build.md index cf9dae5c..25832122 100644 --- a/cmd/crossplane/project/help/build.md +++ b/cmd/crossplane/project/help/build.md @@ -17,6 +17,19 @@ The build reuses the dependency cache populated by `crossplane dependency add` and `crossplane dependency update-cache`. Override the cache location with `--cache-dir` or the `CROSSPLANE_XPKG_CACHE` environment variable. +Embedded functions are built onto a runtime base image pulled from a registry. +Those base image layers are cached on disk, under `crossplane/base-images` in +your user cache directory, so that repeat builds read them locally instead of +downloading them again. The first build of a project fills the cache and is +correspondingly slower than the ones after it. + +Cached layers are addressed by their content digest, so a cached layer is never +stale and the cache never needs invalidating. It does grow, though: nothing +prunes it today, so each new base image version adds to it rather than +replacing what came before. Expect tens of megabytes per base image, per +architecture. Delete the directory to reclaim the space; the next build +refills what it needs. + ## Examples Build the project in the current directory: diff --git a/internal/project/functions/basecache.go b/internal/project/functions/basecache.go index 1f309871..dbed4eb7 100644 --- a/internal/project/functions/basecache.go +++ b/internal/project/functions/basecache.go @@ -25,6 +25,14 @@ import ( // function runtime base image layers. It sits beside the xpkg cache rather // than inside it, since the two hold different kinds of artifact and are // pruned on different terms. +// +// Nothing prunes this directory. Layers are keyed by content digest, so +// entries are never stale, but they are also never replaced: every base image +// version a user builds against accumulates, at tens of megabytes per image +// per architecture. Users can delete the directory safely — the next build +// refetches what it needs — but the CLI should grow a retention policy or a +// prune command before this becomes the kind of thing people discover by +// running out of disk. func DefaultBaseImageCacheDir() string { base, err := os.UserCacheDir() if err != nil { From b4f0a4a2b28ba96df42009d3ad612a622e040ede Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Sat, 29 Aug 2026 19:53:13 +0100 Subject: [PATCH 3/4] Do not let a failing base image cache fail the build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment added with the cache claimed a fallback that go-containerregistry does not provide. Two paths make a bad cache fatal: cache.Image returns any Get error that is not ErrNotFound (pkg/v1/cache/cache.go), and the filesystem cache creates its backing file lazily inside Compressed, so an unwritable or full directory surfaces as an error from the layer rather than as a miss (pkg/v1/cache/fs.go). That combination is worse here than it looks. Nothing prunes this cache, so a full disk is a plausible way to reach the failure — and the result would be a build that fails even though it could have fetched the layer from the registry, as it did before the cache existed. Wrap the cache so both paths degrade instead: report a non-miss read failure as a miss, and keep the uncached layer alongside the caching one so a layer that cannot be written still reads. One case remains: a directory that fills partway through a layer surfaces the write error mid-stream, once the reader is already with the caller. Recovering there would mean re-reading consumed bytes, so it is documented rather than handled. Verified end to end against an empty read-only cache directory: the build completes and the directory stays empty, so the writes really were refused. Caching itself is unaffected — cold 65.2s write phase, warm 0.7s. Reported by CodeRabbit on #304. Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 5 --- internal/project/functions/basecache.go | 71 ++++++++++ internal/project/functions/basecache_test.go | 141 +++++++++++++++++++ internal/project/functions/kcl.go | 9 +- 3 files changed, 217 insertions(+), 4 deletions(-) create mode 100644 internal/project/functions/basecache_test.go diff --git a/internal/project/functions/basecache.go b/internal/project/functions/basecache.go index dbed4eb7..649c2332 100644 --- a/internal/project/functions/basecache.go +++ b/internal/project/functions/basecache.go @@ -17,8 +17,14 @@ limitations under the License. package functions import ( + "io" "os" "path/filepath" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/cache" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" ) // DefaultBaseImageCacheDir returns the default per-user cache directory for @@ -40,3 +46,68 @@ func DefaultBaseImageCacheDir() string { } return filepath.Join(base, "crossplane", "base-images") } + +// tolerantCache degrades to the registry instead of failing the build when the +// cache itself cannot be used. +// +// go-containerregistry's cache is not forgiving on its own. A read error that +// is not ErrNotFound propagates out of Compressed, and the filesystem cache +// creates its backing file lazily, so an unwritable or full cache directory +// surfaces as an error from the layer rather than a cache miss. Neither should +// break a build that could have fetched the layer remotely — especially since +// nothing prunes this cache, which makes a full disk a plausible way to reach +// it. See DefaultBaseImageCacheDir. +// +// One case is not recoverable here: if the directory is writable but fills up +// partway through a layer, the write error surfaces mid-stream, after the +// reader has been handed to the caller. Restarting from the remote layer at +// that point would mean re-reading bytes the caller already consumed. +type tolerantCache struct { + cache.Cache +} + +// Get reports any failure other than a genuine miss as a miss, so that an +// unreadable or corrupt entry sends the caller to the registry. +func (c tolerantCache) Get(h v1.Hash) (v1.Layer, error) { + l, err := c.Cache.Get(h) + if err != nil && !errors.Is(err, cache.ErrNotFound) { + return nil, cache.ErrNotFound + } + return l, err +} + +// Put keeps the original layer alongside the caching one so that a layer which +// cannot be written still reads. +func (c tolerantCache) Put(l v1.Layer) (v1.Layer, error) { + cached, err := c.Cache.Put(l) + if err != nil { + // Deliberate: a cache that cannot store the layer is not a reason to + // fail. Hand back the uncached layer and carry on. + return l, nil //nolint:nilerr // Cache failures degrade to no caching. + } + return tolerantLayer{Layer: cached, uncached: l}, nil +} + +// tolerantLayer reads through to an uncached layer when the caching layer +// cannot open its backing file. +type tolerantLayer struct { + v1.Layer + + uncached v1.Layer +} + +func (l tolerantLayer) Compressed() (io.ReadCloser, error) { + rc, err := l.Layer.Compressed() + if err != nil { + return l.uncached.Compressed() + } + return rc, nil +} + +func (l tolerantLayer) Uncompressed() (io.ReadCloser, error) { + rc, err := l.Layer.Uncompressed() + if err != nil { + return l.uncached.Uncompressed() + } + return rc, nil +} diff --git a/internal/project/functions/basecache_test.go b/internal/project/functions/basecache_test.go new file mode 100644 index 00000000..e6ac0ae5 --- /dev/null +++ b/internal/project/functions/basecache_test.go @@ -0,0 +1,141 @@ +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package functions + +import ( + "io" + "os" + "path/filepath" + "testing" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/cache" + "github.com/google/go-containerregistry/pkg/v1/static" + "github.com/google/go-containerregistry/pkg/v1/types" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" +) + +func testLayer() v1.Layer { + return static.NewLayer([]byte("hello from a layer"), types.DockerLayer) +} + +func readAll(t *testing.T, l v1.Layer) string { + t.Helper() + rc, err := l.Compressed() + if err != nil { + t.Fatalf("Compressed(): unexpected error: %v", err) + } + defer func() { _ = rc.Close() }() + bs, err := io.ReadAll(rc) + if err != nil { + t.Fatalf("reading layer: unexpected error: %v", err) + } + return string(bs) +} + +// erroringCache fails every operation the way a corrupt or unreadable cache +// directory would, rather than reporting a miss. +type erroringCache struct{} + +func (erroringCache) Get(v1.Hash) (v1.Layer, error) { return nil, errors.New("boom") } +func (erroringCache) Put(v1.Layer) (v1.Layer, error) { return nil, errors.New("boom") } +func (erroringCache) Delete(v1.Hash) error { return errors.New("boom") } + +func TestTolerantCacheGetReportsFailuresAsMisses(t *testing.T) { + // A read failure that is not a genuine miss must still look like a miss, + // so the caller falls through to the registry instead of erroring out. + c := tolerantCache{erroringCache{}} + + _, err := c.Get(v1.Hash{Algorithm: "sha256", Hex: "cafe"}) + if !errors.Is(err, cache.ErrNotFound) { + t.Errorf("Get(): want cache.ErrNotFound, got %v", err) + } +} + +func TestTolerantCachePutFailureReturnsUsableLayer(t *testing.T) { + // Put failing outright must hand back a layer that still reads. + c := tolerantCache{erroringCache{}} + + l, err := c.Put(testLayer()) + if err != nil { + t.Fatalf("Put(): unexpected error: %v", err) + } + if got, want := readAll(t, l), "hello from a layer"; got != want { + t.Errorf("layer contents: got %q, want %q", got, want) + } +} + +func TestTolerantCacheUnwritableDirStillReads(t *testing.T) { + // The regression this guards: an unwritable cache directory used to fail + // the build, because the filesystem cache creates its backing file lazily + // inside Compressed(). + if os.Geteuid() == 0 { + t.Skip("running as root, which can write to a read-only directory") + } + + dir := filepath.Join(t.TempDir(), "cache") + if err := os.Mkdir(dir, 0o500); err != nil { + t.Fatalf("creating read-only cache dir: %v", err) + } + + plain := cache.NewFilesystemCache(dir) + l, err := plain.Put(testLayer()) + if err != nil { + t.Fatalf("Put(): unexpected error: %v", err) + } + if _, err := l.Compressed(); err == nil { + t.Skip("cache directory turned out to be writable; nothing to assert") + } + + // Same directory, wrapped: the layer must read regardless. + tolerant := tolerantCache{cache.NewFilesystemCache(dir)} + wrapped, err := tolerant.Put(testLayer()) + if err != nil { + t.Fatalf("Put(): unexpected error: %v", err) + } + if got, want := readAll(t, wrapped), "hello from a layer"; got != want { + t.Errorf("layer contents: got %q, want %q", got, want) + } +} + +func TestTolerantCachePassesThroughOnSuccess(t *testing.T) { + // A working cache must behave exactly as it would unwrapped: a miss is + // still a miss, and a stored layer still reads back. + c := tolerantCache{cache.NewFilesystemCache(t.TempDir())} + + if _, err := c.Get(v1.Hash{Algorithm: "sha256", Hex: "cafe"}); !errors.Is(err, cache.ErrNotFound) { + t.Errorf("Get() on empty cache: want cache.ErrNotFound, got %v", err) + } + + l, err := c.Put(testLayer()) + if err != nil { + t.Fatalf("Put(): unexpected error: %v", err) + } + if got, want := readAll(t, l), "hello from a layer"; got != want { + t.Errorf("layer contents: got %q, want %q", got, want) + } + + // Reading the layer populates the cache, so the digest is now a hit. + d, err := l.Digest() + if err != nil { + t.Fatalf("Digest(): unexpected error: %v", err) + } + if _, err := c.Get(d); err != nil { + t.Errorf("Get() after populating cache: unexpected error: %v", err) + } +} diff --git a/internal/project/functions/kcl.go b/internal/project/functions/kcl.go index 6b77a429..257827ce 100644 --- a/internal/project/functions/kcl.go +++ b/internal/project/functions/kcl.go @@ -150,10 +150,11 @@ func baseImageForArch(ref name.Reference, arch string, transport http.RoundTripp } if cacheDir != "" { - // Layers are addressed by digest, so a cache hit cannot be stale. A - // failure to write the cache is not fatal: the library falls back to - // the remote layer. - img = cache.Image(img, cache.NewFilesystemCache(cacheDir)) + // Layers are addressed by digest, so a cache hit cannot be stale. + // tolerantCache keeps a cache that cannot be read or written from + // failing the build; see its doc comment for the one case it cannot + // recover from. + img = cache.Image(img, tolerantCache{cache.NewFilesystemCache(cacheDir)}) } cfg, err := img.ConfigFile() From 17f173c6dbba029fa473d04dad4ad2c65933f9d4 Mon Sep 17 00:00:00 2001 From: Steven Borrelli Date: Tue, 1 Sep 2026 08:01:10 +0100 Subject: [PATCH 4/4] Address review feedback on the base image cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite the help text in active voice. `validate-docs` generates the command reference from the built CLI and runs Vale over it with fail_on_error, and three write-good.Passive warnings in this text were failing the check: "are built", "are cached", "are addressed". Also drop the claim that "the first build of a project fills the cache". The cache is shared between projects, so a project's first build can read layers another project put there. Say what is true instead — a build takes longer the first time it needs a given base image — and say plainly that projects share it. Trimmed the megabytes-per-image estimate too, since @adamwg would rather the help carried fewer details that can go stale. Return "" rather than falling back to os.TempDir() when there is no per-user cache directory. The fallback put the cache at a predictable path another user on a shared machine could have created first with permissions of their choosing. The layers are public registry content rather than anything sensitive, but callers already treat "" as "do not cache", so declining costs nothing. Note that the pre-existing dependency.DefaultCacheDir() has the same fallback, which this does not touch. Use cmp.Diff in the tolerantCache tests for failure-output consistency, as @adamwg suggested. Left them as separate tests rather than table-driven, since each exercises a different method — @adamwg and CodeRabbit both landed there. Verified by reproducing validate-docs locally: vale 3.14.2 against the generated command reference with the docs repo's own config reports the same three warnings before the change and none after. Signed-off-by: Steven Borrelli Co-Authored-By: Claude Opus 5 --- cmd/crossplane/project/help/build.md | 22 +++++++-------- internal/project/functions/basecache.go | 29 ++++++++++++-------- internal/project/functions/basecache_test.go | 13 +++++---- 3 files changed, 35 insertions(+), 29 deletions(-) diff --git a/cmd/crossplane/project/help/build.md b/cmd/crossplane/project/help/build.md index 25832122..b1de7f2e 100644 --- a/cmd/crossplane/project/help/build.md +++ b/cmd/crossplane/project/help/build.md @@ -17,18 +17,16 @@ The build reuses the dependency cache populated by `crossplane dependency add` and `crossplane dependency update-cache`. Override the cache location with `--cache-dir` or the `CROSSPLANE_XPKG_CACHE` environment variable. -Embedded functions are built onto a runtime base image pulled from a registry. -Those base image layers are cached on disk, under `crossplane/base-images` in -your user cache directory, so that repeat builds read them locally instead of -downloading them again. The first build of a project fills the cache and is -correspondingly slower than the ones after it. - -Cached layers are addressed by their content digest, so a cached layer is never -stale and the cache never needs invalidating. It does grow, though: nothing -prunes it today, so each new base image version adds to it rather than -replacing what came before. Expect tens of megabytes per base image, per -architecture. Delete the directory to reclaim the space; the next build -refills what it needs. +The CLI builds embedded functions onto a runtime base image from a registry, and +caches those image layers on disk under `crossplane/base-images` in your user +cache directory. A build that needs a layer already in the cache reads it +locally rather than downloading it again, so a build takes longer the first time +it needs a given base image. Projects share the cache. + +Layer filenames are content digests, so a cached layer never goes stale and the +cache never needs invalidating. Nothing prunes it, though, so it grows as base +images change. Delete the directory to reclaim the space; the next build refills +what it needs. ## Examples diff --git a/internal/project/functions/basecache.go b/internal/project/functions/basecache.go index 649c2332..ca3202e4 100644 --- a/internal/project/functions/basecache.go +++ b/internal/project/functions/basecache.go @@ -28,21 +28,28 @@ import ( ) // DefaultBaseImageCacheDir returns the default per-user cache directory for -// function runtime base image layers. It sits beside the xpkg cache rather -// than inside it, since the two hold different kinds of artifact and are -// pruned on different terms. +// function runtime base image layers, or "" to disable caching when there is no +// per-user directory to put it in. It sits beside the xpkg cache rather than +// inside it, since the two hold different kinds of artifact and would be pruned +// on different terms. // -// Nothing prunes this directory. Layers are keyed by content digest, so -// entries are never stale, but they are also never replaced: every base image -// version a user builds against accumulates, at tens of megabytes per image -// per architecture. Users can delete the directory safely — the next build -// refetches what it needs — but the CLI should grow a retention policy or a -// prune command before this becomes the kind of thing people discover by -// running out of disk. +// Falling back to os.TempDir() would put the cache at a predictable path that +// another user on a shared machine could have created first, with permissions +// of their choosing. The layers are public registry content rather than +// anything sensitive, but a cache is not worth a world-readable directory +// someone else controls — and callers already treat "" as "do not cache", so +// declining is cheap. +// +// Nothing prunes this directory. Layers are keyed by content digest, so entries +// are never stale, but they are also never replaced: every base image version a +// user builds against accumulates. Users can delete the directory safely — the +// next build refetches what it needs — but the CLI should grow a retention +// policy or a prune command before this becomes the kind of thing people +// discover by running out of disk. func DefaultBaseImageCacheDir() string { base, err := os.UserCacheDir() if err != nil { - base = os.TempDir() + return "" } return filepath.Join(base, "crossplane", "base-images") } diff --git a/internal/project/functions/basecache_test.go b/internal/project/functions/basecache_test.go index e6ac0ae5..9d94961f 100644 --- a/internal/project/functions/basecache_test.go +++ b/internal/project/functions/basecache_test.go @@ -22,6 +22,7 @@ import ( "path/filepath" "testing" + "github.com/google/go-cmp/cmp" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/cache" "github.com/google/go-containerregistry/pkg/v1/static" @@ -75,8 +76,8 @@ func TestTolerantCachePutFailureReturnsUsableLayer(t *testing.T) { if err != nil { t.Fatalf("Put(): unexpected error: %v", err) } - if got, want := readAll(t, l), "hello from a layer"; got != want { - t.Errorf("layer contents: got %q, want %q", got, want) + if diff := cmp.Diff("hello from a layer", readAll(t, l)); diff != "" { + t.Errorf("layer contents (-want +got):\n%s", diff) } } @@ -108,8 +109,8 @@ func TestTolerantCacheUnwritableDirStillReads(t *testing.T) { if err != nil { t.Fatalf("Put(): unexpected error: %v", err) } - if got, want := readAll(t, wrapped), "hello from a layer"; got != want { - t.Errorf("layer contents: got %q, want %q", got, want) + if diff := cmp.Diff("hello from a layer", readAll(t, wrapped)); diff != "" { + t.Errorf("layer contents (-want +got):\n%s", diff) } } @@ -126,8 +127,8 @@ func TestTolerantCachePassesThroughOnSuccess(t *testing.T) { if err != nil { t.Fatalf("Put(): unexpected error: %v", err) } - if got, want := readAll(t, l), "hello from a layer"; got != want { - t.Errorf("layer contents: got %q, want %q", got, want) + if diff := cmp.Diff("hello from a layer", readAll(t, l)); diff != "" { + t.Errorf("layer contents (-want +got):\n%s", diff) } // Reading the layer populates the cache, so the digest is now a hit.