feat: gc untagged manifests and signatures without references (#948)

closes #906 #907

Signed-off-by: Petu Eusebiu <peusebiu@cisco.com>

pkg/storage/common.go

@@ -250,6 +250,7 @@ func CheckIfIndexNeedsUpdate(index *ispec.Index, desc *ispec.Descriptor,
 	return updateIndex, oldDgst, nil
 }
 
+// GetIndex returns the contents of index.json.
 func GetIndex(imgStore ImageStore, repo string, log zerolog.Logger) (ispec.Index, error) {
 	var index ispec.Index
 
@@ -267,8 +268,33 @@ func GetIndex(imgStore ImageStore, repo string, log zerolog.Logger) (ispec.Index
 	return index, nil
 }
 
+// GetImageIndex returns a multiarch type image.
+func GetImageIndex(imgStore ImageStore, repo string, digest godigest.Digest, log zerolog.Logger) (ispec.Index, error) {
+	var imageIndex ispec.Index
+
+	if err := digest.Validate(); err != nil {
+		return imageIndex, err
+	}
+
+	buf, err := imgStore.GetBlobContent(repo, digest)
+	if err != nil {
+		return imageIndex, err
+	}
+
+	indexPath := path.Join(imgStore.RootDir(), repo, "blobs",
+		digest.Algorithm().String(), digest.Encoded())
+
+	if err := json.Unmarshal(buf, &imageIndex); err != nil {
+		log.Error().Err(err).Str("path", indexPath).Msg("invalid JSON")
+
+		return imageIndex, err
+	}
+
+	return imageIndex, nil
+}
+
 func RemoveManifestDescByReference(index *ispec.Index, reference string, detectCollisions bool,
-) (ispec.Descriptor, bool, error) {
+) (ispec.Descriptor, error) {
 	var removedManifest ispec.Descriptor
 
 	var found bool
@@ -297,12 +323,14 @@ func RemoveManifestDescByReference(index *ispec.Index, reference string, detectC
 	}
 
 	if foundCount > 1 && detectCollisions {
-		return ispec.Descriptor{}, false, zerr.ErrManifestConflict
+		return ispec.Descriptor{}, zerr.ErrManifestConflict
+	} else if !found {
+		return ispec.Descriptor{}, zerr.ErrManifestNotFound
 	}
 
 	index.Manifests = outIndex.Manifests
 
-	return removedManifest, found, nil
+	return removedManifest, nil
 }
 
 /*
@@ -369,21 +397,11 @@ func PruneImageManifestsFromIndex(imgStore ImageStore, repo string, digest godig
 	}
 
 	for _, otherIndex := range otherImgIndexes {
-		buf, err := imgStore.GetBlobContent(repo, otherIndex.Digest)
+		oindex, err := GetImageIndex(imgStore, repo, otherIndex.Digest, log)
 		if err != nil {
 			return nil, err
 		}
 
-		indexPath := path.Join(imgStore.RootDir(), repo, "blobs",
-			otherIndex.Digest.Algorithm().String(), otherIndex.Digest.Encoded())
-
-		var oindex ispec.Index
-		if err := json.Unmarshal(buf, &oindex); err != nil {
-			log.Error().Err(err).Str("path", indexPath).Msg("invalid JSON")
-
-			return nil, err
-		}
-
 		for _, omanifest := range oindex.Manifests {
 			_, ok := inUse[omanifest.Digest.Encoded()]
 			if ok {
@@ -479,21 +497,8 @@ func GetOrasReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest,
 			continue
 		}
 
-		buf, err := imgStore.GetBlobContent(repo, manifest.Digest)
+		artManifest, err := GetOrasManifestByDigest(imgStore, repo, manifest.Digest, log)
 		if err != nil {
-			log.Error().Err(err).Str("blob", imgStore.BlobPath(repo, manifest.Digest)).Msg("failed to read manifest")
-
-			if os.IsNotExist(err) || errors.Is(err, driver.PathNotFoundError{}) {
-				return nil, zerr.ErrManifestNotFound
-			}
-
-			return nil, err
-		}
-
-		var artManifest oras.Manifest
-		if err := json.Unmarshal(buf, &artManifest); err != nil {
-			log.Error().Err(err).Str("dir", dir).Msg("invalid JSON")
-
 			return nil, err
 		}
 
@@ -635,6 +640,32 @@ func GetReferrers(imgStore ImageStore, repo string, gdigest godigest.Digest, art
 	return index, nil
 }
 
+func GetOrasManifestByDigest(imgStore ImageStore, repo string, digest godigest.Digest, log zerolog.Logger,
+) (oras.Manifest, error) {
+	var artManifest oras.Manifest
+
+	blobPath := imgStore.BlobPath(repo, digest)
+
+	buf, err := imgStore.GetBlobContent(repo, digest)
+	if err != nil {
+		log.Error().Err(err).Str("blob", blobPath).Msg("failed to read manifest")
+
+		if os.IsNotExist(err) || errors.Is(err, driver.PathNotFoundError{}) {
+			return artManifest, zerr.ErrManifestNotFound
+		}
+
+		return artManifest, err
+	}
+
+	if err := json.Unmarshal(buf, &artManifest); err != nil {
+		log.Error().Err(err).Str("blob", blobPath).Msg("invalid JSON")
+
+		return artManifest, err
+	}
+
+	return artManifest, nil
+}
+
 func IsSupportedMediaType(mediaType string) bool {
 	return mediaType == ispec.MediaTypeImageIndex ||
 		mediaType == ispec.MediaTypeImageManifest ||

pkg/storage/common_test.go

@@ -303,3 +303,40 @@ func TestGetReferrersErrors(t *testing.T) {
 		})
 	})
 }
+
+func TestGetImageIndexErrors(t *testing.T) {
+	log := zerolog.New(os.Stdout)
+
+	Convey("Trigger invalid digest error", t, func(c C) {
+		imgStore := &mocks.MockedImageStore{}
+
+		_, err := storage.GetImageIndex(imgStore, "zot-test", "invalidDigest", log)
+		So(err, ShouldNotBeNil)
+	})
+
+	Convey("Trigger GetBlobContent error", t, func(c C) {
+		imgStore := &mocks.MockedImageStore{
+			GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) {
+				return []byte{}, errors.ErrBlobNotFound
+			},
+		}
+
+		validDigest := godigest.FromBytes([]byte("blob"))
+
+		_, err := storage.GetImageIndex(imgStore, "zot-test", validDigest, log)
+		So(err, ShouldNotBeNil)
+	})
+
+	Convey("Trigger unmarshal error", t, func(c C) {
+		imgStore := &mocks.MockedImageStore{
+			GetBlobContentFn: func(repo string, digest godigest.Digest) ([]byte, error) {
+				return []byte{}, nil
+			},
+		}
+
+		validDigest := godigest.FromBytes([]byte("blob"))
+
+		_, err := storage.GetImageIndex(imgStore, "zot-test", validDigest, log)
+		So(err, ShouldNotBeNil)
+	})
+}

pkg/storage/local/local.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -18,6 +19,7 @@ import (
 	apexlog "github.com/apex/log"
 	guuid "github.com/gofrs/uuid"
 	"github.com/minio/sha256-simd"
+	notreg "github.com/notaryproject/notation-go/registry"
 	godigest "github.com/opencontainers/go-digest"
 	imeta "github.com/opencontainers/image-spec/specs-go"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -25,8 +27,10 @@ import (
 	"github.com/opencontainers/umoci/oci/casext"
 	oras "github.com/oras-project/artifacts-spec/specs-go/v1"
 	"github.com/rs/zerolog"
+	"github.com/sigstore/cosign/pkg/oci/remote"
 
 	zerr "zotregistry.io/zot/errors"
+	"zotregistry.io/zot/pkg/common"
 	"zotregistry.io/zot/pkg/extensions/monitoring"
 	zlog "zotregistry.io/zot/pkg/log"
 	"zotregistry.io/zot/pkg/scheduler"
@@ -565,15 +569,11 @@ func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string, detectCol
 		return err
 	}
 
-	manifestDesc, found, err := storage.RemoveManifestDescByReference(&index, reference, detectCollision)
+	manifestDesc, err := storage.RemoveManifestDescByReference(&index, reference, detectCollision)
 	if err != nil {
 		return err
 	}
 
-	if !found {
-		return zerr.ErrManifestNotFound
-	}
-
 	err = storage.UpdateIndexWithPrunedImageManifests(is, &index, repo, manifestDesc, manifestDesc.Digest, is.log)
 	if err != nil {
 		return err
@@ -1267,14 +1267,18 @@ func (is *ImageStoreLocal) GetBlobContent(repo string, digest godigest.Digest) (
 	blobPath := is.BlobPath(repo, digest)
 
 	blob, err := os.ReadFile(blobPath)
-	if os.IsNotExist(err) {
-		is.log.Error().Err(err).Str("blob", blobPath).Msg("blob doesn't exist")
+	if err != nil {
+		if os.IsNotExist(err) {
+			is.log.Error().Err(err).Str("blob", blobPath).Msg("blob doesn't exist")
 
-		return []byte{}, zerr.ErrBlobNotFound
+			return []byte{}, zerr.ErrBlobNotFound
+		}
+
+		is.log.Error().Err(err).Str("blob", blobPath).Msg("failed to read blob")
+
+		return []byte{}, err
 	}
 
-	is.log.Error().Err(err).Str("blob", blobPath).Msg("failed to read blob")
-
 	return blob, nil
 }
 
@@ -1429,6 +1433,65 @@ func (is *ImageStoreLocal) garbageCollect(dir string, repo string) error {
 	}
 	defer oci.Close()
 
+	// gc untagged manifests and signatures
+	index, err := oci.GetIndex(context.Background())
+	if err != nil {
+		return err
+	}
+
+	referencedByImageIndex := []string{}
+	cosignDescriptors := []ispec.Descriptor{}
+	notationDescriptors := []ispec.Descriptor{}
+
+	/* gather manifests references by multiarch images (to skip gc)
+	gather cosign and notation signatures descriptors */
+	for _, desc := range index.Manifests {
+		switch desc.MediaType {
+		case ispec.MediaTypeImageIndex:
+			indexImage, err := storage.GetImageIndex(is, repo, desc.Digest, is.log)
+			if err != nil {
+				is.log.Error().Err(err).Str("repo", repo).Str("digest", desc.Digest.String()).
+					Msg("gc: failed to read multiarch(index) image")
+
+				return err
+			}
+
+			for _, indexDesc := range indexImage.Manifests {
+				referencedByImageIndex = append(referencedByImageIndex, indexDesc.Digest.String())
+			}
+		case ispec.MediaTypeImageManifest:
+			tag, ok := desc.Annotations[ispec.AnnotationRefName]
+			if ok {
+				// gather cosign signatures
+				if strings.HasPrefix(tag, "sha256-") && strings.HasSuffix(tag, remote.SignatureTagSuffix) {
+					cosignDescriptors = append(cosignDescriptors, desc)
+				}
+			}
+		case oras.MediaTypeArtifactManifest:
+			notationDescriptors = append(notationDescriptors, desc)
+		}
+	}
+
+	is.log.Info().Msg("gc: untagged manifests")
+
+	if err := gcUntaggedManifests(is, oci, &index, repo, referencedByImageIndex); err != nil {
+		return err
+	}
+
+	is.log.Info().Msg("gc: cosign signatures")
+
+	if err := gcCosignSignatures(is, oci, &index, repo, cosignDescriptors); err != nil {
+		return err
+	}
+
+	is.log.Info().Msg("gc: notation signatures")
+
+	if err := gcNotationSignatures(is, oci, &index, repo, notationDescriptors); err != nil {
+		return err
+	}
+
+	is.log.Info().Msg("gc: blobs")
+
 	err = oci.GC(context.Background(), ifOlderThan(is, repo, is.gcDelay))
 	if err := test.Error(err); err != nil {
 		return err
@@ -1437,25 +1500,176 @@ func (is *ImageStoreLocal) garbageCollect(dir string, repo string) error {
 	return nil
 }
 
+func gcUntaggedManifests(imgStore *ImageStoreLocal, oci casext.Engine, index *ispec.Index, repo string,
+	referencedByImageIndex []string,
+) error {
+	for _, desc := range index.Manifests {
+		// skip manifests referenced in image indexex
+		if common.Contains(referencedByImageIndex, desc.Digest.String()) {
+			continue
+		}
+
+		// remove untagged images
+		if desc.MediaType == ispec.MediaTypeImageManifest {
+			_, ok := desc.Annotations[ispec.AnnotationRefName]
+			if !ok {
+				// check if is indeed an image and not an artifact by checking it's config blob
+				buf, err := imgStore.GetBlobContent(repo, desc.Digest)
+				if err != nil {
+					imgStore.log.Error().Err(err).Str("repo", repo).Str("digest", desc.Digest.String()).
+						Msg("gc: failed to read image manifest")
+
+					return err
+				}
+
+				manifest := ispec.Manifest{}
+
+				err = json.Unmarshal(buf, &manifest)
+				if err != nil {
+					return err
+				}
+
+				// skip manifests which are not of type image
+				if manifest.Config.MediaType != ispec.MediaTypeImageConfig {
+					imgStore.log.Info().Str("config mediaType", manifest.Config.MediaType).
+						Msg("skipping gc untagged manifest, because config blob is not application/vnd.oci.image.config.v1+json")
+
+					continue
+				}
+
+				// remove manifest if it's older than gc.delay
+				canGC, err := isBlobOlderThan(imgStore, repo, desc.Digest, imgStore.gcDelay)
+				if err != nil {
+					imgStore.log.Error().Err(err).Str("repo", repo).Str("digest", desc.Digest.String()).
+						Msgf("gc: failed to check if blob is older than %s", imgStore.gcDelay.String())
+
+					return err
+				}
+
+				if canGC {
+					imgStore.log.Info().Str("repo", repo).Str("digest", desc.Digest.String()).Msg("gc: removing manifest without tag")
+
+					_, err = storage.RemoveManifestDescByReference(index, desc.Digest.String(), true)
+					if errors.Is(err, zerr.ErrManifestConflict) {
+						imgStore.log.Info().Str("repo", repo).Str("digest", desc.Digest.String()).
+							Msg("gc: skipping removing manifest due to conflict")
+
+						continue
+					}
+
+					err := oci.PutIndex(context.Background(), *index)
+					if err != nil {
+						return err
+					}
+				}
+			}
+		}
+	}
+
+	return nil
+}
+
+func gcCosignSignatures(imgStore *ImageStoreLocal, oci casext.Engine, index *ispec.Index, repo string,
+	cosignDescriptors []ispec.Descriptor,
+) error {
+	for _, cosignDesc := range cosignDescriptors {
+		foundSubject := false
+		// check if we can find the manifest which the signature points to
+		for _, desc := range index.Manifests {
+			subject := fmt.Sprintf("sha256-%s.%s", desc.Digest.Encoded(), remote.SignatureTagSuffix)
+			if subject == cosignDesc.Annotations[ispec.AnnotationRefName] {
+				foundSubject = true
+			}
+		}
+
+		if !foundSubject {
+			// remove manifest
+			imgStore.log.Info().Str("repo", repo).Str("digest", cosignDesc.Digest.String()).
+				Msg("gc: removing cosign signature without subject")
+
+			// no need to check for manifest conflict, if one doesn't have a subject, then none with same digest will have
+			_, _ = storage.RemoveManifestDescByReference(index, cosignDesc.Digest.String(), false)
+
+			err := oci.PutIndex(context.Background(), *index)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
+func gcNotationSignatures(imgStore *ImageStoreLocal, oci casext.Engine, index *ispec.Index, repo string,
+	notationDescriptors []ispec.Descriptor,
+) error {
+	for _, notationDesc := range notationDescriptors {
+		foundSubject := false
+		// check if we can find the manifest which the signature points to
+		artManifest, err := storage.GetOrasManifestByDigest(imgStore, repo, notationDesc.Digest, imgStore.log)
+		if err != nil {
+			imgStore.log.Error().Err(err).Str("repo", repo).Str("digest", notationDesc.Digest.String()).
+				Msg("gc: failed to get oras artifact manifest")
+
+			return err
+		}
+
+		// skip oras artifacts which are not signatures
+		if artManifest.ArtifactType != notreg.ArtifactTypeNotation {
+			continue
+		}
+
+		for _, desc := range index.Manifests {
+			if desc.Digest == artManifest.Subject.Digest {
+				foundSubject = true
+			}
+		}
+
+		if !foundSubject {
+			// remove manifest
+			imgStore.log.Info().Str("repo", repo).Str("digest", notationDesc.Digest.String()).
+				Msg("gc: removing notation signature without subject")
+
+			// no need to check for manifest conflict, if one doesn't have a subject, then none with same digest will have
+			_, _ = storage.RemoveManifestDescByReference(index, notationDesc.Digest.String(), false)
+
+			err := oci.PutIndex(context.Background(), *index)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
 func ifOlderThan(imgStore *ImageStoreLocal, repo string, delay time.Duration) casext.GCPolicy {
 	return func(ctx context.Context, digest godigest.Digest) (bool, error) {
-		blobPath := imgStore.BlobPath(repo, digest)
-
-		fi, err := os.Stat(blobPath)
-		if err != nil {
-			return false, err
-		}
-
-		if fi.ModTime().Add(delay).After(time.Now()) {
-			return false, nil
-		}
-
-		imgStore.log.Info().Str("digest", digest.String()).Str("blobPath", blobPath).Msg("perform GC on blob")
-
-		return true, nil
+		return isBlobOlderThan(imgStore, repo, digest, delay)
 	}
 }
 
+func isBlobOlderThan(imgStore *ImageStoreLocal, repo string, digest godigest.Digest, delay time.Duration,
+) (bool, error) {
+	blobPath := imgStore.BlobPath(repo, digest)
+
+	fileInfo, err := os.Stat(blobPath)
+	if err != nil {
+		imgStore.log.Error().Err(err).Str("digest", digest.String()).Str("blobPath", blobPath).
+			Msg("gc: failed to stat blob")
+
+		return false, err
+	}
+
+	if fileInfo.ModTime().Add(delay).After(time.Now()) {
+		return false, nil
+	}
+
+	imgStore.log.Info().Str("digest", digest.String()).Str("blobPath", blobPath).Msg("perform GC on blob")
+
+	return true, nil
+}
+
 func DirExists(d string) bool {
 	if !utf8.ValidString(d) {
 		return false

pkg/storage/local/local_test.go

@@ -1073,7 +1073,7 @@ func TestDedupeLinks(t *testing.T) {
 		Name:        "cache",
 		UseRelPaths: true,
 	}, log)
-	imgStore := local.NewImageStore(dir, true, storage.DefaultGCDelay,
+	imgStore := local.NewImageStore(dir, false, storage.DefaultGCDelay,
 		true, true, log, metrics, nil, cacheDriver)
 
 	Convey("Dedupe", t, func(c C) {
@@ -2205,6 +2205,225 @@ func TestGarbageCollectForImageStore(t *testing.T) {
 	})
 }
 
+func TestGarbageCollectErrors(t *testing.T) {
+	Convey("Make image store", t, func(c C) {
+		dir := t.TempDir()
+
+		log := log.NewLogger("debug", "")
+		metrics := monitoring.NewMetricsServer(false, log)
+		cacheDriver, _ := storage.Create("boltdb", cache.BoltDBDriverParameters{
+			RootDir:     dir,
+			Name:        "cache",
+			UseRelPaths: true,
+		}, log)
+		imgStore := local.NewImageStore(dir, true, 500*time.Millisecond, true, true, log, metrics, nil, cacheDriver)
+		repoName := "gc-index"
+
+		// create a blob/layer
+		upload, err := imgStore.NewBlobUpload(repoName)
+		So(err, ShouldBeNil)
+		So(upload, ShouldNotBeEmpty)
+
+		content := []byte("this is a blob1")
+		buf := bytes.NewBuffer(content)
+		buflen := buf.Len()
+		digest := godigest.FromBytes(content)
+		So(digest, ShouldNotBeNil)
+		blob, err := imgStore.PutBlobChunkStreamed(repoName, upload, buf)
+		So(err, ShouldBeNil)
+		So(blob, ShouldEqual, buflen)
+		bdgst1 := digest
+		bsize1 := len(content)
+
+		err = imgStore.FinishBlobUpload(repoName, upload, buf, digest)
+		So(err, ShouldBeNil)
+		So(blob, ShouldEqual, buflen)
+
+		Convey("Trigger error on GetImageIndex", func() {
+			var index ispec.Index
+			index.SchemaVersion = 2
+			index.MediaType = ispec.MediaTypeImageIndex
+
+			for i := 0; i < 4; i++ {
+				// upload image config blob
+				upload, err = imgStore.NewBlobUpload(repoName)
+				So(err, ShouldBeNil)
+				So(upload, ShouldNotBeEmpty)
+
+				cblob, cdigest := test.GetRandomImageConfig()
+				buf = bytes.NewBuffer(cblob)
+				buflen = buf.Len()
+				blob, err = imgStore.PutBlobChunkStreamed(repoName, upload, buf)
+				So(err, ShouldBeNil)
+				So(blob, ShouldEqual, buflen)
+
+				err = imgStore.FinishBlobUpload(repoName, upload, buf, cdigest)
+				So(err, ShouldBeNil)
+				So(blob, ShouldEqual, buflen)
+
+				// create a manifest
+				manifest := ispec.Manifest{
+					Config: ispec.Descriptor{
+						MediaType: ispec.MediaTypeImageConfig,
+						Digest:    cdigest,
+						Size:      int64(len(cblob)),
+					},
+					Layers: []ispec.Descriptor{
+						{
+							MediaType: ispec.MediaTypeImageLayer,
+							Digest:    bdgst1,
+							Size:      int64(bsize1),
+						},
+					},
+				}
+				manifest.SchemaVersion = 2
+				content, err = json.Marshal(manifest)
+				So(err, ShouldBeNil)
+				digest = godigest.FromBytes(content)
+				So(digest, ShouldNotBeNil)
+				_, err = imgStore.PutImageManifest(repoName, digest.String(), ispec.MediaTypeImageManifest, content)
+				So(err, ShouldBeNil)
+
+				index.Manifests = append(index.Manifests, ispec.Descriptor{
+					Digest:    digest,
+					MediaType: ispec.MediaTypeImageManifest,
+					Size:      int64(len(content)),
+				})
+			}
+
+			// upload index image
+			indexContent, err := json.Marshal(index)
+			So(err, ShouldBeNil)
+			indexDigest := godigest.FromBytes(indexContent)
+			So(indexDigest, ShouldNotBeNil)
+
+			_, err = imgStore.PutImageManifest(repoName, "1.0", ispec.MediaTypeImageIndex, indexContent)
+			So(err, ShouldBeNil)
+
+			err = os.Chmod(imgStore.BlobPath(repoName, indexDigest), 0o000)
+			So(err, ShouldBeNil)
+
+			time.Sleep(500 * time.Millisecond)
+
+			err = imgStore.RunGCRepo(repoName)
+			So(err, ShouldNotBeNil)
+		})
+
+		Convey("Trigger error on GetBlobContent and Unmarshal for untagged manifest", func() {
+			// upload image config blob
+			upload, err = imgStore.NewBlobUpload(repoName)
+			So(err, ShouldBeNil)
+			So(upload, ShouldNotBeEmpty)
+
+			cblob, cdigest := test.GetRandomImageConfig()
+			buf = bytes.NewBuffer(cblob)
+			buflen = buf.Len()
+			blob, err = imgStore.PutBlobChunkStreamed(repoName, upload, buf)
+			So(err, ShouldBeNil)
+			So(blob, ShouldEqual, buflen)
+
+			err = imgStore.FinishBlobUpload(repoName, upload, buf, cdigest)
+			So(err, ShouldBeNil)
+			So(blob, ShouldEqual, buflen)
+
+			// create a manifest
+			manifest := ispec.Manifest{
+				Config: ispec.Descriptor{
+					MediaType: ispec.MediaTypeImageConfig,
+					Digest:    cdigest,
+					Size:      int64(len(cblob)),
+				},
+				Layers: []ispec.Descriptor{
+					{
+						MediaType: ispec.MediaTypeImageLayer,
+						Digest:    bdgst1,
+						Size:      int64(bsize1),
+					},
+				},
+			}
+			manifest.SchemaVersion = 2
+			content, err = json.Marshal(manifest)
+			So(err, ShouldBeNil)
+			digest = godigest.FromBytes(content)
+			So(digest, ShouldNotBeNil)
+
+			_, err = imgStore.PutImageManifest(repoName, digest.String(), ispec.MediaTypeImageManifest, content)
+			So(err, ShouldBeNil)
+
+			// trigger GetBlobContent error
+			err = os.Remove(imgStore.BlobPath(repoName, digest))
+			So(err, ShouldBeNil)
+
+			time.Sleep(500 * time.Millisecond)
+
+			err = imgStore.RunGCRepo(repoName)
+			So(err, ShouldNotBeNil)
+
+			// trigger Unmarshal error
+			_, err = os.Create(imgStore.BlobPath(repoName, digest))
+			So(err, ShouldBeNil)
+
+			err = imgStore.RunGCRepo(repoName)
+			So(err, ShouldNotBeNil)
+		})
+
+		Convey("Trigger manifest conflict error", func() {
+			// upload image config blob
+			upload, err = imgStore.NewBlobUpload(repoName)
+			So(err, ShouldBeNil)
+			So(upload, ShouldNotBeEmpty)
+
+			cblob, cdigest := test.GetRandomImageConfig()
+			buf = bytes.NewBuffer(cblob)
+			buflen = buf.Len()
+			blob, err = imgStore.PutBlobChunkStreamed(repoName, upload, buf)
+			So(err, ShouldBeNil)
+			So(blob, ShouldEqual, buflen)
+
+			err = imgStore.FinishBlobUpload(repoName, upload, buf, cdigest)
+			So(err, ShouldBeNil)
+			So(blob, ShouldEqual, buflen)
+
+			// create a manifest
+			manifest := ispec.Manifest{
+				Config: ispec.Descriptor{
+					MediaType: ispec.MediaTypeImageConfig,
+					Digest:    cdigest,
+					Size:      int64(len(cblob)),
+				},
+				Layers: []ispec.Descriptor{
+					{
+						MediaType: ispec.MediaTypeImageLayer,
+						Digest:    bdgst1,
+						Size:      int64(bsize1),
+					},
+				},
+			}
+			manifest.SchemaVersion = 2
+			content, err = json.Marshal(manifest)
+			So(err, ShouldBeNil)
+			digest = godigest.FromBytes(content)
+			So(digest, ShouldNotBeNil)
+
+			_, err = imgStore.PutImageManifest(repoName, digest.String(), ispec.MediaTypeImageManifest, content)
+			So(err, ShouldBeNil)
+			// upload again same manifest so that we trigger manifest conflict
+			_, err = imgStore.PutImageManifest(repoName, "1.0", ispec.MediaTypeImageManifest, content)
+			So(err, ShouldBeNil)
+
+			time.Sleep(500 * time.Millisecond)
+
+			err = imgStore.RunGCRepo(repoName)
+			So(err, ShouldBeNil)
+
+			// blob shouldn't be gc'ed
+			found, _, err := imgStore.CheckBlob(repoName, digest)
+			So(err, ShouldBeNil)
+			So(found, ShouldEqual, true)
+		})
+	})
+}
+
 func randSeq(n int) string {
 	letters := []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")
 

pkg/storage/s3/s3.go

@@ -463,15 +463,11 @@ func (is *ObjectStorage) DeleteImageManifest(repo, reference string, detectColli
 		return err
 	}
 
-	manifestDesc, found, err := storage.RemoveManifestDescByReference(&index, reference, detectCollisions)
+	manifestDesc, err := storage.RemoveManifestDescByReference(&index, reference, detectCollisions)
 	if err != nil {
 		return err
 	}
 
-	if !found {
-		return zerr.ErrManifestNotFound
-	}
-
 	err = storage.UpdateIndexWithPrunedImageManifests(is, &index, repo, manifestDesc, manifestDesc.Digest, is.log)
 	if err != nil {
 		return err