From 97b1f005760986bc0d70e0599bffe6d724fd2fa2 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 11 May 2026 11:54:39 +0800 Subject: [PATCH 1/2] fix(image): force-pull when digest pinned but missing locally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EnsureImage already proves at the first Inspect call that the requested ImageDigest isn't in the local cache. The subsequent Pull, however, passed force=false and so hit the URL-level short-circuit in images/cloudimg/pull.go that skips re-downloading when "any blob is already cached under this URL." For mutable tags (e.g. epoch's /dl/ endpoint pointing at the latest push of a repo), the cached blob can be a stale digest — exactly the scenario that left clones failing downstream in vm.restore with a "Backing file I/O error" against a qcow2 backing file whose digest doesn't exist on disk. Pass force=true to Pull when ImageDigest is set. Cheap path (no digest pinned) stays cheap. Slow path triggers only when the caller wants a specific digest and we've shown it isn't local — same semantics users already get from `cocoon image pull --force`. Fixes #37. --- cmd/core/ensure_image_test.go | 135 ++++++++++++++++++++++++++++++++++ cmd/core/helpers.go | 9 ++- 2 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 cmd/core/ensure_image_test.go diff --git a/cmd/core/ensure_image_test.go b/cmd/core/ensure_image_test.go new file mode 100644 index 00000000..9e49a556 --- /dev/null +++ b/cmd/core/ensure_image_test.go @@ -0,0 +1,135 @@ +package core + +import ( + "context" + "testing" + + "github.com/cocoonstack/cocoon/gc" + imagebackend "github.com/cocoonstack/cocoon/images" + "github.com/cocoonstack/cocoon/progress" + "github.com/cocoonstack/cocoon/types" +) + +// fakeImageBackend records Pull invocations so the test can assert how +// EnsureImage decided to call it. +type fakeImageBackend struct { + typ string + + inspectByRef map[string]*types.Image // ref → image (nil if not local) + pullRefs []string // ordered Pull arg history + pullForce []bool + pullErr error + postPullInspect map[string]*types.Image // ref → image (second Inspect after Pull) +} + +func (f *fakeImageBackend) Type() string { return f.typ } + +func (f *fakeImageBackend) Pull(_ context.Context, ref string, force bool, _ progress.Tracker) error { + f.pullRefs = append(f.pullRefs, ref) + f.pullForce = append(f.pullForce, force) + return f.pullErr +} + +func (f *fakeImageBackend) Inspect(_ context.Context, ref string) (*types.Image, error) { + // First call uses inspectByRef; once Pull has been called, switch to + // postPullInspect so the test can simulate "blob now landed locally". + if len(f.pullRefs) > 0 && f.postPullInspect != nil { + return f.postPullInspect[ref], nil + } + return f.inspectByRef[ref], nil +} + +func (f *fakeImageBackend) Import(context.Context, string, progress.Tracker, ...string) error { + return nil +} + +func (f *fakeImageBackend) List(context.Context) ([]*types.Image, error) { return nil, nil } +func (f *fakeImageBackend) Delete(context.Context, []string) ([]string, error) { + return nil, nil +} +func (f *fakeImageBackend) RegisterGC(*gc.Orchestrator) {} +func (f *fakeImageBackend) Config(context.Context, []*types.VMConfig) ([][]*types.StorageConfig, []*types.BootConfig, error) { + return nil, nil, nil +} + +// TestEnsureImage_ForceWhenDigestPinned is the regression guard for issue +// 37: when vmCfg.ImageDigest is set and the digest isn't local, EnsureImage +// must pass force=true to Pull so the cloudimg URL-level short-circuit +// (which keys on URL, not digest) re-fetches the blob instead of accepting +// whatever stale content is cached under the same URL. +func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) { + const ( + url = "https://epoch.example/dl/simular/win11" + digest = "sha256:adafd938488daa114be898848eb24b9b0afffc21ac18f8b11f3f0057644b11e1" + ) + + tests := []struct { + name string + vmCfg *types.VMConfig + wantPullRef string + wantForce bool + }{ + { + name: "digest pinned → force=true", + vmCfg: &types.VMConfig{ + Config: types.Config{ + Image: url, + ImageDigest: digest, + ImageType: types.ImageTypeCloudImg, + }, + }, + wantPullRef: url, + wantForce: true, + }, + { + name: "no digest → force=false", + vmCfg: &types.VMConfig{ + Config: types.Config{ + Image: url, + ImageType: types.ImageTypeCloudImg, + }, + }, + wantPullRef: url, + wantForce: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &fakeImageBackend{typ: types.ImageTypeCloudImg} + EnsureImage(t.Context(), []imagebackend.Images{f}, tt.vmCfg) + if len(f.pullRefs) != 1 { + t.Fatalf("Pull invocations = %d, want 1", len(f.pullRefs)) + } + if f.pullRefs[0] != tt.wantPullRef { + t.Errorf("Pull ref = %q, want %q", f.pullRefs[0], tt.wantPullRef) + } + if f.pullForce[0] != tt.wantForce { + t.Errorf("Pull force = %v, want %v", f.pullForce[0], tt.wantForce) + } + }) + } +} + +// TestEnsureImage_SkipsPullWhenDigestLocal: the happy path. If Inspect by +// digest hits, EnsureImage returns without calling Pull at all — the +// expensive force-pull only fires when we've proven the digest isn't local. +func TestEnsureImage_SkipsPullWhenDigestLocal(t *testing.T) { + const digest = "sha256:adafd938488daa114be898848eb24b9b0afffc21ac18f8b11f3f0057644b11e1" + f := &fakeImageBackend{ + typ: types.ImageTypeCloudImg, + inspectByRef: map[string]*types.Image{ + digest: {ID: digest, Name: "win11", Type: types.ImageTypeCloudImg}, + }, + } + EnsureImage(t.Context(), []imagebackend.Images{f}, &types.VMConfig{ + Config: types.Config{ + Image: "https://epoch.example/dl/simular/win11", + ImageDigest: digest, + ImageType: types.ImageTypeCloudImg, + }, + }) + if len(f.pullRefs) != 0 { + t.Errorf("Pull was called %d time(s) with %v, want 0 (digest is local)", len(f.pullRefs), f.pullRefs) + } +} diff --git a/cmd/core/helpers.go b/cmd/core/helpers.go index d5b10cf5..e336cfa6 100644 --- a/cmd/core/helpers.go +++ b/cmd/core/helpers.go @@ -249,7 +249,14 @@ func EnsureImage(ctx context.Context, backends []imagebackend.Images, vmCfg *typ // version recorded at snapshot time, not whatever the tag points to now. pullRef := digestPullRef(vmCfg.Image, vmCfg.ImageDigest, vmCfg.ImageType) logger.Infof(ctx, "base image not found locally, pulling %s ...", pullRef) - if pullErr := b.Pull(ctx, pullRef, false, progress.Nop); pullErr != nil { + // Force when ImageDigest is pinned: we already proved at Inspect that + // the digest isn't local, so the URL-level short-circuit in + // images/cloudimg/pull.go would skip re-fetching even though the + // cached blob is the wrong content. That short-circuit is correct + // only for "any cached blob is fine" — pinned-digest is the opposite + // policy, matching `cocoon image pull --force`. + needForce := vmCfg.ImageDigest != "" + if pullErr := b.Pull(ctx, pullRef, needForce, progress.Nop); pullErr != nil { logger.Warnf(ctx, "auto-pull %s failed (imported image?): %v — clone may fail if base layers are missing", pullRef, pullErr) return } From 7b8fa1485054c7128a0e3cd088aad24d9f44c7a2 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 11 May 2026 12:01:56 +0800 Subject: [PATCH 2/2] chore(image): trim ensure-image comments and test scaffolding - Collapse 6-line rationale to one line in EnsureImage force-pull guard. - Drop unused pullErr/postPullInspect from the test fake; tighten test docstrings. --- cmd/core/ensure_image_test.go | 36 ++++++++++------------------------- cmd/core/helpers.go | 7 +------ 2 files changed, 11 insertions(+), 32 deletions(-) diff --git a/cmd/core/ensure_image_test.go b/cmd/core/ensure_image_test.go index 9e49a556..5eb2414e 100644 --- a/cmd/core/ensure_image_test.go +++ b/cmd/core/ensure_image_test.go @@ -10,16 +10,11 @@ import ( "github.com/cocoonstack/cocoon/types" ) -// fakeImageBackend records Pull invocations so the test can assert how -// EnsureImage decided to call it. type fakeImageBackend struct { - typ string - - inspectByRef map[string]*types.Image // ref → image (nil if not local) - pullRefs []string // ordered Pull arg history - pullForce []bool - pullErr error - postPullInspect map[string]*types.Image // ref → image (second Inspect after Pull) + typ string + inspectByRef map[string]*types.Image + pullRefs []string + pullForce []bool } func (f *fakeImageBackend) Type() string { return f.typ } @@ -27,15 +22,10 @@ func (f *fakeImageBackend) Type() string { return f.typ } func (f *fakeImageBackend) Pull(_ context.Context, ref string, force bool, _ progress.Tracker) error { f.pullRefs = append(f.pullRefs, ref) f.pullForce = append(f.pullForce, force) - return f.pullErr + return nil } func (f *fakeImageBackend) Inspect(_ context.Context, ref string) (*types.Image, error) { - // First call uses inspectByRef; once Pull has been called, switch to - // postPullInspect so the test can simulate "blob now landed locally". - if len(f.pullRefs) > 0 && f.postPullInspect != nil { - return f.postPullInspect[ref], nil - } return f.inspectByRef[ref], nil } @@ -52,11 +42,8 @@ func (f *fakeImageBackend) Config(context.Context, []*types.VMConfig) ([][]*type return nil, nil, nil } -// TestEnsureImage_ForceWhenDigestPinned is the regression guard for issue -// 37: when vmCfg.ImageDigest is set and the digest isn't local, EnsureImage -// must pass force=true to Pull so the cloudimg URL-level short-circuit -// (which keys on URL, not digest) re-fetches the blob instead of accepting -// whatever stale content is cached under the same URL. +// Regression guard for issue 37: pinned digest with no local hit must force-pull +// to bypass cloudimg's URL-keyed cache. func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) { const ( url = "https://epoch.example/dl/simular/win11" @@ -70,7 +57,7 @@ func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) { wantForce bool }{ { - name: "digest pinned → force=true", + name: "digest pinned -> force=true", vmCfg: &types.VMConfig{ Config: types.Config{ Image: url, @@ -82,7 +69,7 @@ func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) { wantForce: true, }, { - name: "no digest → force=false", + name: "no digest -> force=false", vmCfg: &types.VMConfig{ Config: types.Config{ Image: url, @@ -111,9 +98,6 @@ func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) { } } -// TestEnsureImage_SkipsPullWhenDigestLocal: the happy path. If Inspect by -// digest hits, EnsureImage returns without calling Pull at all — the -// expensive force-pull only fires when we've proven the digest isn't local. func TestEnsureImage_SkipsPullWhenDigestLocal(t *testing.T) { const digest = "sha256:adafd938488daa114be898848eb24b9b0afffc21ac18f8b11f3f0057644b11e1" f := &fakeImageBackend{ @@ -130,6 +114,6 @@ func TestEnsureImage_SkipsPullWhenDigestLocal(t *testing.T) { }, }) if len(f.pullRefs) != 0 { - t.Errorf("Pull was called %d time(s) with %v, want 0 (digest is local)", len(f.pullRefs), f.pullRefs) + t.Errorf("Pull called %d time(s), want 0", len(f.pullRefs)) } } diff --git a/cmd/core/helpers.go b/cmd/core/helpers.go index e336cfa6..f49f81f5 100644 --- a/cmd/core/helpers.go +++ b/cmd/core/helpers.go @@ -249,12 +249,7 @@ func EnsureImage(ctx context.Context, backends []imagebackend.Images, vmCfg *typ // version recorded at snapshot time, not whatever the tag points to now. pullRef := digestPullRef(vmCfg.Image, vmCfg.ImageDigest, vmCfg.ImageType) logger.Infof(ctx, "base image not found locally, pulling %s ...", pullRef) - // Force when ImageDigest is pinned: we already proved at Inspect that - // the digest isn't local, so the URL-level short-circuit in - // images/cloudimg/pull.go would skip re-fetching even though the - // cached blob is the wrong content. That short-circuit is correct - // only for "any cached blob is fine" — pinned-digest is the opposite - // policy, matching `cocoon image pull --force`. + // Pinned digest with no local hit: force past cloudimg's URL-keyed cache. needForce := vmCfg.ImageDigest != "" if pullErr := b.Pull(ctx, pullRef, needForce, progress.Nop); pullErr != nil { logger.Warnf(ctx, "auto-pull %s failed (imported image?): %v — clone may fail if base layers are missing", pullRef, pullErr)