From f7a8f8bac0316b83d1fb869d93afc00333aa8097 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sat, 24 Oct 2026 20:08:50 -0400 Subject: [PATCH] fix(build): an unlabelled release image is adopted only from a pinned release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A released tag whose image predates per-class build labels records only the aggregate input key, so its code cannot be compared with this build's. #1148 adopted it anyway, from any source: an edited checkout whose version did not move would be handed the earlier release's image, with a warning and nothing compared. It is now adopted only when the service builds from a pinned release the CLI materialized — under the git-clone or verified-package cache roots, which hold nothing but CLI output — because that code is the release's by construction. From a checkout or an override it refuses, naming the two ways out. A root configured through CODEFLY_MODULE_CACHE is never proof: a developer may keep their own checkouts there. The cache roots move to a leaf package, pkg/modulecache, so the builder can ask without an import cycle; pkg/composition delegates to it unchanged. Co-Authored-By: Claude Opus 5.5 --- docs/commands.md | 2 +- pkg/composition/materialize.go | 89 +++-------------- pkg/modulecache/modulecache.go | 118 +++++++++++++++++++++++ pkg/modulecache/modulecache_test.go | 32 ++++++ pkg/orchestration/build_plan.go | 24 +++-- pkg/orchestration/image_cache_test.go | 25 ++++- pkg/orchestration/override_build_test.go | 19 ++-- 7 files changed, 220 insertions(+), 89 deletions(-) create mode 100644 pkg/modulecache/modulecache.go create mode 100644 pkg/modulecache/modulecache_test.go diff --git a/docs/commands.md b/docs/commands.md index afa50d2e..837df00e 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4095,7 +4095,7 @@ move with its code: | records this build's code; its recipe or bases differ (another checkout, another builder, a re-pushed base) | adopts it — it is the release's build — and warns naming the class that differs | | records other code | refuses: the code changed without its version moving, and the fix is a new release, never a rebuild under the old version | | records another agent than the release pins | refuses: the tag holds a build the release does not name (made under an [agent override](#moving-the-agent-version-of-composed-services) by a CLI that pushed it under the release tag) | -| records only the aggregate (labelled before the classes existed) | adopts it with a warning that its code was not compared; `--rebuild` builds it again instead | +| records only the aggregate (labelled before the classes existed) | adopts it, with a warning, only when the service builds from a pinned release the CLI materialized (its code is the release's by construction); from a checkout or an override it refuses, since nothing can compare the code — build from the pinned release, or release a new version | Adopting is the default for an image that cannot be compared because the two alternatives are worse: refusing blocks every render of every module on a cell diff --git a/pkg/composition/materialize.go b/pkg/composition/materialize.go index fdd195af..e11a58ac 100644 --- a/pkg/composition/materialize.go +++ b/pkg/composition/materialize.go @@ -14,6 +14,8 @@ import ( "sync" "time" + "github.com/codefly-dev/cli/pkg/modulecache" + "github.com/Masterminds/semver" "github.com/codefly-dev/cli/pkg/cli" "github.com/codefly-dev/core/resources" @@ -691,22 +693,10 @@ func (roots moduleCacheRoots) holds(path string) bool { return underAnyDir([]string{roots.clone, roots.verified}, path) } -// verifiedPinnedModuleCacheRoot is the content-addressed module cache the core -// Materializer writes into. By default it is workspace-scoped — the digest tree -// NewMaterializer(workspaceDir) picks — while the git-clone cache root is -// process-global (pinnedModuleCacheRoot); both are recognized when deciding -// whether an overlay path is CLI-managed. A configured ModuleCacheEnv moves it -// beside the clones, under the one reserved subdirectory, so a workspace -// repository holds no module bytes at all. +// verifiedPinnedModuleCacheRoot is the verified-package cache root +// (modulecache.VerifiedRoot). func verifiedPinnedModuleCacheRoot(workspaceDir string) (string, error) { - configured, err := configuredModuleCacheRoot() - if err != nil { - return "", err - } - if configured != "" { - return filepath.Join(configured, verifiedPackageCacheDirName), nil - } - return filepath.Join(workspaceDir, ".codefly", "cache", "modules"), nil + return modulecache.VerifiedRoot(workspaceDir) } // pruneStalePinnedEntries drops auto-managed cache entries whose module is no @@ -1130,20 +1120,12 @@ func gitCommand(ctx context.Context, args ...string) *exec.Cmd { return cmd } -// ModuleCacheEnv relocates the module cache — both the git-clone cache and the -// verified-package cache — to a directory the developer chooses, so the modules -// a workspace composes can be browsed (and deleted) where they expect them -// rather than inside CODEFLY_HOME and inside the workspace respectively. It -// must name an absolute path; a leading "~" is expanded. -const ModuleCacheEnv = "CODEFLY_MODULE_CACHE" - -// verifiedPackageCacheDirName is the subdirectory of a configured cache root -// that holds verified module packages. Those are content-addressed by digest -// (that addressing is what makes a cache hit checkable against what was -// verified), so they cannot share the browsable // tree -// the git clones occupy; keeping them under one reserved name is what lets both -// live under the single root the developer configured. -const verifiedPackageCacheDirName = ".packages" +// ModuleCacheEnv relocates the module cache (modulecache.Env). +const ModuleCacheEnv = modulecache.Env + +// verifiedPackageCacheDirName is the verified packages' subdirectory of a +// configured cache root (modulecache.VerifiedDirName). +const verifiedPackageCacheDirName = modulecache.VerifiedDirName // stagingCacheDirName is the subdirectory of a cache root that holds in-flight // clones. They live under one reserved name rather than as `.pull-*` siblings of @@ -1153,53 +1135,14 @@ const verifiedPackageCacheDirName = ".packages" // the checkouts it is promoted into, which being under the same root guarantees. const stagingCacheDirName = ".staging" -// configuredModuleCacheRoot returns the root named by ModuleCacheEnv, or "" when -// it is unset. A value that is not usable as a root is an error rather than an -// ignored setting: silently falling back would put the modules somewhere other -// than where the developer said, and they would look for them where they asked. -func configuredModuleCacheRoot() (string, error) { - raw := strings.TrimSpace(os.Getenv(ModuleCacheEnv)) - if raw == "" { - return "", nil - } - expanded := raw - if raw == "~" || strings.HasPrefix(raw, "~/") { - home, err := os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("%s is %q but the home directory cannot be resolved: %w", ModuleCacheEnv, raw, err) - } - expanded = filepath.Join(home, strings.TrimPrefix(strings.TrimPrefix(raw, "~"), "/")) - } - if !filepath.IsAbs(expanded) { - return "", fmt.Errorf("%s must be an absolute path (or start with ~), got %q", ModuleCacheEnv, raw) - } - return filepath.Clean(expanded), nil -} +// configuredModuleCacheRoot is modulecache.Configured. +func configuredModuleCacheRoot() (string, error) { return modulecache.Configured() } -// pinnedModuleCacheRoot is the root of the git-clone cache: the configured root -// when there is one, otherwise the process-global default. Under either, a -// module lands at //// plus its optional module -// subpath, which is the layout a developer pointing ModuleCacheEnv at their own -// vendor directory browses. -func pinnedModuleCacheRoot() (string, error) { - configured, err := configuredModuleCacheRoot() - if err != nil { - return "", err - } - if configured != "" { - return configured, nil - } - return filepath.Join(resources.CodeflyHomeDir(), "modules"), nil -} +// pinnedModuleCacheRoot is the git-clone cache root (modulecache.CloneRoot). +func pinnedModuleCacheRoot() (string, error) { return modulecache.CloneRoot() } // underDir reports whether path is dir itself or nested inside it. -func underDir(dir, path string) bool { - rel, err := filepath.Rel(dir, path) - if err != nil { - return false - } - return rel == "." || (!strings.HasPrefix(rel, ".."+string(filepath.Separator)) && rel != "..") -} +func underDir(dir, path string) bool { return modulecache.Under(dir, path) } // underAnyDir reports whether path is under any of dirs (empty entries are // skipped). Used to recognize both the git-clone fallback's cache root and the diff --git a/pkg/modulecache/modulecache.go b/pkg/modulecache/modulecache.go new file mode 100644 index 00000000..dc5669b8 --- /dev/null +++ b/pkg/modulecache/modulecache.go @@ -0,0 +1,118 @@ +// Package modulecache names where the CLI materializes the modules a +// workspace pins, and answers whether a path is one it wrote. It is a leaf so +// that both the composition that materializes modules and the builder that +// must know whether a service's code is a pinned release can read one answer. +package modulecache + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/codefly-dev/core/resources" +) + +// Env relocates the module cache — both the git-clone cache and the +// verified-package cache — to a directory the developer chooses, so the modules +// a workspace composes can be browsed (and deleted) where they expect them +// rather than inside CODEFLY_HOME and inside the workspace respectively. It +// must name an absolute path; a leading "~" is expanded. +const Env = "CODEFLY_MODULE_CACHE" + +// VerifiedDirName is the subdirectory of a configured cache root that holds +// verified module packages. Those are content-addressed by digest (that +// addressing is what makes a cache hit checkable against what was verified), +// so they cannot share the browsable // tree the git +// clones occupy; keeping them under one reserved name is what lets both live +// under the single root the developer configured. +const VerifiedDirName = ".packages" + +// Configured returns the root named by Env, or "" when it is unset. A value +// that is not usable as a root is an error rather than an ignored setting: +// silently falling back would put the modules somewhere other than where the +// developer said, and they would look for them where they asked. +func Configured() (string, error) { + raw := strings.TrimSpace(os.Getenv(Env)) + if raw == "" { + return "", nil + } + expanded := raw + if raw == "~" || strings.HasPrefix(raw, "~/") { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("%s is %q but the home directory cannot be resolved: %w", Env, raw, err) + } + expanded = filepath.Join(home, strings.TrimPrefix(strings.TrimPrefix(raw, "~"), "/")) + } + if !filepath.IsAbs(expanded) { + return "", fmt.Errorf("%s must be an absolute path (or start with ~), got %q", Env, raw) + } + return filepath.Clean(expanded), nil +} + +// CloneRoot is the root of the git-clone cache: the configured root when there +// is one, otherwise the process-global default. Under either, a module lands +// at //// plus its optional module subpath, which is +// the layout a developer pointing Env at their own vendor directory browses. +func CloneRoot() (string, error) { + configured, err := Configured() + if err != nil { + return "", err + } + if configured != "" { + return configured, nil + } + return filepath.Join(resources.CodeflyHomeDir(), "modules"), nil +} + +// VerifiedRoot is the content-addressed module cache the core Materializer +// writes into. By default it is workspace-scoped — the digest tree +// NewMaterializer(workspaceDir) picks — while the git-clone cache root is +// process-global (CloneRoot). A configured Env moves it beside the clones, +// under the one reserved subdirectory, so a workspace repository holds no +// module bytes at all. +func VerifiedRoot(workspaceDir string) (string, error) { + configured, err := Configured() + if err != nil { + return "", err + } + if configured != "" { + return filepath.Join(configured, VerifiedDirName), nil + } + return filepath.Join(workspaceDir, ".codefly", "cache", "modules"), nil +} + +// Owned reports whether path lies under a cache root that holds nothing but +// what the CLI materialized for workspaceDir: a pinned module's code exactly as +// its release carries it. A root the developer configured through Env is never +// proof — it is a directory they chose and may keep their own checkouts in. +func Owned(workspaceDir, path string) (bool, error) { + configured, err := Configured() + if err != nil || configured != "" { + return false, err + } + clone, err := CloneRoot() + if err != nil { + return false, err + } + verified, err := VerifiedRoot(workspaceDir) + if err != nil { + return false, err + } + for _, root := range []string{clone, verified} { + if Under(root, path) { + return true, nil + } + } + return false, nil +} + +// Under reports whether path is dir itself or nested inside it. +func Under(dir, path string) bool { + rel, err := filepath.Rel(dir, path) + if err != nil { + return false + } + return rel == "." || (!strings.HasPrefix(rel, ".."+string(filepath.Separator)) && rel != "..") +} diff --git a/pkg/modulecache/modulecache_test.go b/pkg/modulecache/modulecache_test.go new file mode 100644 index 00000000..01942fda --- /dev/null +++ b/pkg/modulecache/modulecache_test.go @@ -0,0 +1,32 @@ +package modulecache + +import ( + "path/filepath" + "testing" +) + +// Only a root that holds nothing but CLI output proves a path is a pinned +// release; a checkout beside it, or any path under a configured root, does not. +func TestOwnedIsOnlyTheCLIsOwnRoots(t *testing.T) { + workspace := t.TempDir() + t.Setenv(Env, "") + clone, err := CloneRoot() + if err != nil { + t.Fatal(err) + } + for path, want := range map[string]bool{ + filepath.Join(clone, "acme", "module-x", "v1.2.3", "services", "api"): true, + filepath.Join(workspace, ".codefly", "cache", "modules", "sha256", "ab"): true, + filepath.Join(workspace, "modules", "x", "services", "api"): false, + filepath.Join(filepath.Dir(clone), "modules-checkout", "x"): false, + } { + if got, err := Owned(workspace, path); err != nil || got != want { + t.Errorf("Owned(%s) = %v, %v, want %v", path, got, err, want) + } + } + t.Setenv(Env, t.TempDir()) + configured, _ := Configured() + if got, err := Owned(workspace, filepath.Join(configured, "acme", "module-x", "v1.2.3")); err != nil || got { + t.Errorf("a configured root proved ownership: %v, %v", got, err) + } +} diff --git a/pkg/orchestration/build_plan.go b/pkg/orchestration/build_plan.go index 832f0a7b..b2290353 100644 --- a/pkg/orchestration/build_plan.go +++ b/pkg/orchestration/build_plan.go @@ -14,6 +14,8 @@ import ( "strings" "time" + "github.com/codefly-dev/cli/pkg/modulecache" + "google.golang.org/protobuf/proto" "github.com/codefly-dev/cli/pkg/environments" @@ -244,7 +246,11 @@ func (b *Builder) judgePublishedRelease(ctx context.Context, w *wool.Wool, recip // other inputs. return nil, w.Wrap(publishedFromOtherInputs(recipe.GetImage(), b.instance.Service.Agent, labels[buildAgentLabel])) case identity.Key != "" && published != "": - warning, refusal := publishedReleaseVerdict(recipe.GetImage(), identity, b.instance.Service.Agent, labels) + pinned, err := modulecache.Owned(b.world.Workspace.Dir(), b.instance.Service.Dir()) + if err != nil { + return nil, w.Wrapf(err, "cannot tell whether %s builds a pinned release", b.instance.Unique()) + } + warning, refusal := publishedReleaseVerdict(recipe.GetImage(), identity, b.instance.Service.Agent, labels, pinned) if refusal != nil { return nil, w.Wrap(refusal) } @@ -1127,9 +1133,12 @@ func withBuildInputsLabel(args []string, identity imageIdentity) []string { // override (override_build.go), which the release does not name. // // An image labelled before the classes were recorded carries only the -// aggregate, so its code cannot be compared: it is adopted, saying so, and -// --rebuild is the operator's way to build it again instead. -func publishedReleaseVerdict(image string, identity imageIdentity, agent *resources.Agent, labels map[string]string) (string, error) { +// aggregate, so its code cannot be compared with this build's. It is adopted +// only when pinned says this build's code is a pinned release the CLI +// materialized (modulecache.Owned) — the release's own code, by construction — +// and refused otherwise: from a checkout, an edited service whose version did +// not move would be handed the earlier release's image with nothing compared. +func publishedReleaseVerdict(image string, identity imageIdentity, agent *resources.Agent, labels map[string]string, pinned bool) (string, error) { const release = "release a new version (codefly publish) instead of rebuilding a published one" publishedAgent := labels[buildAgentLabel] if agent != nil && publishedAgent != "" && publishedAgent != agent.Identifier() { @@ -1141,9 +1150,12 @@ func publishedReleaseVerdict(image string, identity imageIdentity, agent *resour } code := labels[buildCodeLabel] switch { + case code == "" && pinned: + return "the published image predates per-class build labels; this build's code is the pinned release the CLI materialized, " + + "so adopting it as the release's build (--rebuild builds it again)", nil case code == "": - return "the published image predates per-class build labels, so its code was not compared with this build's; " + - "adopting it as the release's build (--rebuild builds it again)", nil + return "", fmt.Errorf("%s is already published, and it predates per-class build labels, so its code cannot be compared with this build's, "+ + "which is not a pinned release the CLI materialized (a checkout or an override): build from the pinned release, or %s", image, release) case code != classes.Code: return "", fmt.Errorf("%s is already published from other code: the service's build context or Go module manifests differ from the published build's, "+ "so its code changed without its version moving; %s", image, release) diff --git a/pkg/orchestration/image_cache_test.go b/pkg/orchestration/image_cache_test.go index 393dbc50..33bae6e1 100644 --- a/pkg/orchestration/image_cache_test.go +++ b/pkg/orchestration/image_cache_test.go @@ -9,6 +9,8 @@ import ( "testing" "time" + "github.com/codefly-dev/cli/pkg/modulecache" + "github.com/codefly-dev/cli/pkg/environments" coreservices "github.com/codefly-dev/core/agents/services" builderv0 "github.com/codefly-dev/core/generated/go/codefly/services/builder/v0" @@ -856,10 +858,14 @@ func TestAPublishedVersionFromTheSameInputsIsAdoptedNotRebuilt(t *testing.T) { } // An image labelled before builds recorded their input classes carries only the -// aggregate, which cannot say whether its code is this build's. It is the -// release's build, so it is adopted, not rebuilt over and not refused. -func TestAPublishedVersionLabelledBeforeClassesIsAdopted(t *testing.T) { +// aggregate, which cannot say whether its code is this build's. Built from the +// pinned release the CLI materialized, the code is the release's by +// construction, so the image is adopted, not rebuilt over and not refused. +func TestAPublishedVersionLabelledBeforeClassesIsAdoptedFromAPinnedRelease(t *testing.T) { harness := newImageCacheHarness(t, true, true) + // The clone root is /modules, which holds the service. + t.Setenv(resources.CodeflyHomeEnv, harness.workspaceDir) + t.Setenv(modulecache.Env, "") harness.peer.image = "example.test/app:1.2.3" published := "sha256:" + strings.Repeat("d", 64) stubPublishedImage(t, published, "sha256:"+strings.Repeat("e", 64)) @@ -867,6 +873,19 @@ func TestAPublishedVersionLabelledBeforeClassesIsAdopted(t *testing.T) { require.Equal(t, 0, harness.docker.calls(t, "build")) } +// From a checkout, the same image is refused: an edited service whose version +// did not move would otherwise be handed the earlier release's image with +// nothing compared. +func TestAPublishedVersionLabelledBeforeClassesIsRefusedFromACheckout(t *testing.T) { + harness := newImageCacheHarness(t, true, true) + harness.peer.image = "example.test/app:1.2.3" + stubPublishedImage(t, "sha256:"+strings.Repeat("d", 64), "sha256:"+strings.Repeat("e", 64)) + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + require.ErrorContains(t, harness.tryBuild(ctx, t), "cannot be compared with this build's") + require.Equal(t, 0, harness.docker.calls(t, "build"), "a published release is never rebuilt") +} + // An image published before builds recorded their inputs proves nothing either // way, so it is built again, as it always was. func TestAPublishedVersionWithNoInputRecordIsBuiltAgain(t *testing.T) { diff --git a/pkg/orchestration/override_build_test.go b/pkg/orchestration/override_build_test.go index b7ec1061..76c24506 100644 --- a/pkg/orchestration/override_build_test.go +++ b/pkg/orchestration/override_build_test.go @@ -140,9 +140,12 @@ func TestAPublishedReleaseIsJudgedByItsCode(t *testing.T) { refuse string }{ "same code, other recipe and bases": {labels: with(map[string]string{buildInputsLabel: other, buildRecipeLabel: other, buildBasesLabel: other})}, - "labelled before the classes": {labels: with(map[string]string{ + // The harness builds from a checkout, not a pinned release the CLI + // materialized, so code nothing can compare is refused + // (TestPublishedReleaseVerdictNamesTheClass adopts it from one). + "labelled before the classes, from a checkout": {labels: with(map[string]string{ buildInputsLabel: other, buildCodeLabel: "", buildRecipeLabel: "", buildBasesLabel: "", - })}, + }), refuse: "cannot be compared with this build's"}, "other code": {labels: with(map[string]string{buildInputsLabel: other, buildCodeLabel: other}), refuse: "is already published from other code"}, "other agent": {labels: with(map[string]string{buildInputsLabel: other, buildAgentLabel: "codefly.dev/go:0.0.9"}), refuse: "from a build by agent codefly.dev/go:0.0.9"}, } { @@ -168,18 +171,22 @@ func TestPublishedReleaseVerdictNamesTheClass(t *testing.T) { labels := func(code, recipe, bases string) map[string]string { return map[string]string{buildInputsLabel: "sha256:other", buildCodeLabel: code, buildRecipeLabel: recipe, buildBasesLabel: bases} } - warning, err := publishedReleaseVerdict("app:1.2.3", ours, agent, labels("sha256:c", "sha256:x", "sha256:b")) + warning, err := publishedReleaseVerdict("app:1.2.3", ours, agent, labels("sha256:c", "sha256:x", "sha256:b"), false) require.NoError(t, err) require.Contains(t, warning, "the build recipe its agent emitted") require.NotContains(t, warning, "base images") - warning, err = publishedReleaseVerdict("app:1.2.3", ours, agent, labels("sha256:c", "sha256:r", "sha256:x")) + warning, err = publishedReleaseVerdict("app:1.2.3", ours, agent, labels("sha256:c", "sha256:r", "sha256:x"), false) require.NoError(t, err) require.Contains(t, warning, "the resolved base images") require.NotContains(t, warning, "recipe") - warning, err = publishedReleaseVerdict("app:1.2.3", ours, agent, map[string]string{buildInputsLabel: "sha256:other"}) + unlabelled := map[string]string{buildInputsLabel: "sha256:other"} + warning, err = publishedReleaseVerdict("app:1.2.3", ours, agent, unlabelled, true) require.NoError(t, err) require.Contains(t, warning, "predates per-class build labels") - _, err = publishedReleaseVerdict("app:1.2.3", ours, agent, labels("sha256:x", "sha256:r", "sha256:b")) + _, err = publishedReleaseVerdict("app:1.2.3", ours, agent, unlabelled, false) + require.ErrorContains(t, err, "cannot be compared") + require.ErrorContains(t, err, "release a new version") + _, err = publishedReleaseVerdict("app:1.2.3", ours, agent, labels("sha256:x", "sha256:r", "sha256:b"), true) require.ErrorContains(t, err, "app:1.2.3 is already published from other code") require.ErrorContains(t, err, "release a new version") }