fix: skip OCI conversion when image is already synced #3823

* Fix #3823: skip OCI conversion when image is already synced

When syncRef determines an image is already synced, it now returns a
bool to signal the skip. syncImage checks this and returns early before
attempting OCI conversion, preventing misleading 'failed to convert
docker image to oci' errors caused by a non-existent temp directory.

* Keep syncReferrers and CommitAll running for already-synced images

Address review feedback: new referrers can be added upstream after
initial sync, so we must not skip syncReferrers. Only the OCI
conversion is guarded by the skipped flag, since converting an
already-stored image is both unnecessary and incorrect.

Signed-off-by: Ugur Tafrali <ugur.tafrali@gmail.com>

---------

Signed-off-by: Ugur Tafrali <ugur.tafrali@gmail.com>

pkg/extensions/sync/service.go

@@ -461,7 +461,7 @@ func (service *BaseService) SyncRepo(ctx context.Context, repo string) error {
 
 func (service *BaseService) syncRef(ctx context.Context, localRepo string, remoteImageRef, localImageRef ref.Ref,
 	remoteDigest godigest.Digest, recursive bool,
-) error {
+) (bool, error) {
 	var reference string
 
 	var skipImage bool
@@ -498,13 +498,13 @@ func (service *BaseService) syncRef(ctx context.Context, localRepo string, remot
 				Str("local image", fmt.Sprintf("%s:%s", localRepo, remoteImageRef.Tag)).Msg("failed to sync image")
 		}
 
-		return err
+		return false, err
 	}
 
 	service.log.Info().Str("image", remoteImageRef.CommonName()).
 		Msg("skipping image because it's already synced")
 
-	return nil
+	return true, nil
 }
 
 // get "would be" digest of image after synced.
@@ -617,7 +617,7 @@ func (service *BaseService) syncImage(ctx context.Context, localRepo, remoteRepo
 	defer service.destination.CleanupImage(localImageRef, localRepo) //nolint: errcheck
 
 	// first sync image
-	err = service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, localDigest, false)
+	skipped, err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, localDigest, false)
 	if err != nil {
 		return err
 	}
@@ -627,7 +627,7 @@ func (service *BaseService) syncImage(ctx context.Context, localRepo, remoteRepo
 	}
 
 	// convert image to oci if needed
-	if isConverted && !service.config.PreserveDigest {
+	if !skipped && isConverted && !service.config.PreserveDigest {
 		localImageRef, err = mod.Apply(ctx, service.rc, localImageRef,
 			mod.WithRefTgt(localImageRef),
 			mod.WithManifestToOCI(),
@@ -737,7 +737,7 @@ func (service *BaseService) syncReferrers(ctx context.Context, tags []string, lo
 
 			localImageRef = localImageRef.SetDigest(desc.Digest.String())
 
-			err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, desc.Digest, false)
+			_, err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, desc.Digest, false)
 			if err != nil {
 				service.log.Error().Err(err).Str("errortype", common.TypeOf(err)).
 					Str("repo", localRepo).Str("local reference", localImageRef.Tag).
@@ -755,7 +755,7 @@ func (service *BaseService) syncReferrers(ctx context.Context, tags []string, lo
 
 				localImageRef = localImageRef.SetTag(tag)
 
-				err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, remoteDigest, true)
+				_, err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, remoteDigest, true)
 				if err != nil {
 					service.log.Error().Err(err).Str("errortype", common.TypeOf(err)).
 						Str("repo", localRepo).Str("local reference", localImageRef.Tag).

pkg/extensions/sync/sync_internal_test.go

@@ -127,6 +127,57 @@ func TestService(t *testing.T) {
 		So(panicOccurred, ShouldBeTrue)
 	})
 
+	Convey("test syncImage skips OCI conversion but still commits when image already synced", t, func() {
+		conf := syncconf.RegistryConfig{
+			URLs: []string{"http://localhost"},
+		}
+
+		service, err := New(conf, "", nil, t.TempDir(), storage.StoreController{}, mocks.MetaDBMock{}, log.NewTestLogger())
+		So(err, ShouldBeNil)
+
+		// Mock remote returns isConverted=true so OCI conversion would be attempted if not skipped
+		mockRemote := &mocks.SyncRemoteMock{
+			GetImageReferenceFn: func(repo string, tag string) (ref.Ref, error) {
+				return ref.New("mock-registry/" + repo + ":" + tag)
+			},
+			GetOCIDigestFn: func(ctx context.Context, repo, tag string) (godigest.Digest, godigest.Digest, bool, error) {
+				// isConverted=true means OCI conversion would be attempted
+				return godigest.Digest("sha256:abc123"), godigest.Digest("sha256:def456"), true, nil
+			},
+		}
+		service.remote = mockRemote
+
+		commitAllCalled := false
+
+		// Mock destination returns CanSkipImage=true (already synced)
+		mockDest := &mocks.SyncDestinationMock{
+			GetImageReferenceFn: func(repo string, tag string) (ref.Ref, error) {
+				return ref.New("local/" + repo + ":" + tag)
+			},
+			CanSkipImageFn: func(repo string, tag string, digest godigest.Digest) (bool, error) {
+				return true, nil
+			},
+			CommitAllFn: func(repo string, imageReference ref.Ref) error {
+				commitAllCalled = true
+				return nil
+			},
+			CleanupImageFn: func(imageReference ref.Ref, repo string) error {
+				return nil
+			},
+		}
+		service.destination = mockDest
+
+		ctx := context.Background()
+		err = service.syncImage(ctx, "localrepo", "remoterepo", "tag1", []string{}, false)
+
+		// Should succeed without error
+		So(err, ShouldBeNil)
+		// CommitAll should still be called to persist any new referrers even when image was skipped
+		So(commitAllCalled, ShouldBeTrue)
+		// OCI conversion is NOT called because skipped=true guards it.
+		// If mod.Apply were called, it would fail since service.rc is nil - proving the guard works.
+	})
+
 	Convey("test syncImage ReferrerList error with OnlySigned", t, func() {
 		onlySigned := true
 		conf := syncconf.RegistryConfig{