Actually make reusing layers found by TOC work

Look up the layer by TOC; and don't abort when diffID is not around.

We could, instead, look up the layers only in tryReusingBlobAsPending,
and record the layer metadata at that time.  That would be simpler,
but it would also widen a race with concurrent image pulls/deletions:
the current code can find one layer (ChainID) to reuse, and when that layer
is deleted, it can find some other layer (ChainID) to actually consume.

The time between tryReusingBlobAsPending and createNewLayer can be fairly
significant, so opening a ~determinitic race singificantly more might
lead to reproducible issues.

Even if anyone encountering such issues has fundamental workflow problems
that should be fixed.

Signed-off-by: Miloslav Trmač <[email protected]>

storage/storage_dest.go

@@ -806,45 +806,48 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D
 		return layer, nil
 	}
 
-	s.lock.Lock()
-	diffID, ok := s.lockProtected.blobDiffIDs[layerDigest]
-	s.lock.Unlock()
-	if !ok {
-		return nil, fmt.Errorf("failed to find diffID for layer: %q", layerDigest)
-	}
-	optionalDiffID := diffID // FIXME
-
 	// Check if we previously cached a file with that blob's contents.  If we didn't,
 	// then we need to read the desired contents from a layer.
 	var trustedUncompressedDigest, trustedOriginalDigest digest.Digest // For storage.LayerOptions
 	s.lock.Lock()
+	tocDigest := s.lockProtected.indexToTOCDigest[index]       // "" if not set
+	optionalDiffID := s.lockProtected.blobDiffIDs[layerDigest] // "" if not set
 	filename, gotFilename := s.lockProtected.filenames[layerDigest]
-	_, layerIdentifiedByTOC := s.lockProtected.indexToTOCDigest[index]
 	s.lock.Unlock()
-	if gotFilename && !layerIdentifiedByTOC {
-		// If layerIdentifiedByTOC, if we now happen to find a layerDigest match, the newLayerID has already been computed as TOC-based,
+	if gotFilename && tocDigest == "" {
+		// If tocDigest != "", if we now happen to find a layerDigest match, the newLayerID has already been computed as TOC-based,
 		// and we don't know the relationship of the layerDigest and TOC digest.
 		// We could recompute newLayerID to be DiffID-based and use the file, but such a within-image layer
 		// reuse is expected to be pretty rare; instead, ignore the unexpected file match and proceed to the
 		// originally-planned TOC match.
 
-		// Because !layerIdentifiedByTOC, optionaldiffID must have been set; and even if it weren’t, PutLayer will recompute the digest from the stream.
+		// Because tocDigest == "", optionaldiffID must have been set; and even if it weren’t, PutLayer will recompute the digest from the stream.
 		trustedUncompressedDigest = optionalDiffID
 		trustedOriginalDigest = layerDigest // The code setting .filenames[layerDigest] is responsible for the contents matching.
 	} else {
-		// Try to find the layer with contents matching that blobsum.
+		// Try to find the layer with contents matching the data we use.
 		var layer *storage.Layer // = nil
-		layers, err2 := s.imageRef.transport.store.LayersByUncompressedDigest(diffID)
-		if err2 == nil && len(layers) > 0 {
-			layer = &layers[0]
+		if tocDigest != "" {
+			layers, err2 := s.imageRef.transport.store.LayersByTOCDigest(tocDigest)
+			if err2 == nil && len(layers) > 0 {
+				layer = &layers[0]
+			} else {
+				return nil, fmt.Errorf("locating layer for TOC digest %q: %w", tocDigest, err2)
+			}
 		} else {
-			layers, err2 = s.imageRef.transport.store.LayersByCompressedDigest(layerDigest)
+			// Because tocDigest == "", optionaldiffID must have been set
+			layers, err2 := s.imageRef.transport.store.LayersByUncompressedDigest(optionalDiffID)
 			if err2 == nil && len(layers) > 0 {
 				layer = &layers[0]
+			} else {
+				layers, err2 = s.imageRef.transport.store.LayersByCompressedDigest(layerDigest)
+				if err2 == nil && len(layers) > 0 {
+					layer = &layers[0]
+				}
+			}
+			if layer == nil {
+				return nil, fmt.Errorf("locating layer for blob %q: %w", layerDigest, err2)
 			}
-		}
-		if layer == nil {
-			return nil, fmt.Errorf("locating layer for blob %q: %w", layerDigest, err2)
 		}
 		// Read the layer's contents.
 		noCompression := archive.Uncompressed
@@ -876,7 +879,7 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D
 		}
 
 		// FIXME CLEAN THIS UP
-		if !layerIdentifiedByTOC {
+		if tocDigest == "" {
 			// If we found the layer using LayersByUncompressedDigest(diffID):
 			// - trustedUncompressedDigest = diffID by definition, and diffID is set
 			// - trustedOriginalDigest = layerDigest is trusted by construction of blobDiffIDs (probably from BlobInfoCache, which is trusted)