Merge pull request #45005 from corhere/fix-loopclosure-test-bugs

Fix loop-closure bugs in tests and shadowed bugs in distribution/xfer

distribution/xfer/download.go

@@ -267,7 +267,7 @@ func (ldm *LayerDownloadManager) makeDownloadFunc(descriptor DownloadDescriptor,
 				downloadReader io.ReadCloser
 				size           int64
 				err            error
-				retries        int
+				attempt        int = 1
 			)
 
 			defer descriptor.Close()
@@ -287,16 +287,16 @@ func (ldm *LayerDownloadManager) makeDownloadFunc(descriptor DownloadDescriptor,
 				default:
 				}
 
-				retries++
-				if _, isDNR := err.(DoNotRetry); isDNR || retries > ldm.maxDownloadAttempts {
-					logrus.Errorf("Download failed after %d attempts: %v", retries, err)
+				if _, isDNR := err.(DoNotRetry); isDNR || attempt >= ldm.maxDownloadAttempts {
+					logrus.Errorf("Download failed after %d attempts: %v", attempt, err)
 					d.err = err
 					return
 				}
 
-				logrus.Infof("Download failed, retrying (%d/%d): %v", retries, ldm.maxDownloadAttempts, err)
-				delay := retries * 5
+				logrus.Infof("Download failed, retrying (%d/%d): %v", attempt, ldm.maxDownloadAttempts, err)
+				delay := attempt * 5
 				ticker := time.NewTicker(ldm.waitDuration)
+				attempt++
 
 			selectLoop:
 				for {

distribution/xfer/download_test.go

@@ -216,7 +216,7 @@ func (d *mockDownloadDescriptor) Download(ctx context.Context, progressOutput pr
 
 	if d.retries < d.simulateRetries {
 		d.retries++
-		return nil, 0, fmt.Errorf("simulating download attempt %d/%d", d.retries, d.simulateRetries)
+		return nil, 0, fmt.Errorf("simulating download attempt failure %d/%d", d.retries, d.simulateRetries)
 	}
 
 	return d.mockTarStream(), 0, nil
@@ -367,28 +367,29 @@ func TestMaxDownloadAttempts(t *testing.T) {
 	}{
 		{
 			name:                "max-attempts=5, succeed at 2nd attempt",
-			simulateRetries:     2,
+			simulateRetries:     1,
 			maxDownloadAttempts: 5,
 		},
 		{
 			name:                "max-attempts=5, succeed at 5th attempt",
+			simulateRetries:     4,
+			maxDownloadAttempts: 5,
+		},
+		{
+			name:                "max-attempts=5, fail at 5th attempt",
 			simulateRetries:     5,
 			maxDownloadAttempts: 5,
+			expectedErr:         "simulating download attempt failure 5/5",
 		},
 		{
-			name:                "max-attempts=5, fail at 6th attempt",
-			simulateRetries:     6,
-			maxDownloadAttempts: 5,
-			expectedErr:         "simulating download attempt 5/6",
-		},
-		{
-			name:                "max-attempts=0, fail after 1 attempt",
+			name:                "max-attempts=1, fail after 1 attempt",
 			simulateRetries:     1,
-			maxDownloadAttempts: 0,
-			expectedErr:         "simulating download attempt 1/1",
+			maxDownloadAttempts: 1,
+			expectedErr:         "simulating download attempt failure 1/1",
 		},
 	}
 	for _, tc := range tests {
+		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 			layerStore := &mockLayerStore{make(map[layer.ChainID]*mockLayer)}

integration/build/build_test.go

@@ -90,6 +90,7 @@ func TestBuildWithRemoveAndForceRemove(t *testing.T) {
 	client := testEnv.APIClient()
 	ctx := context.Background()
 	for _, c := range cases {
+		c := c
 		t.Run(c.name, func(t *testing.T) {
 			t.Parallel()
 			dockerfile := []byte(c.dockerfile)