fix: bound bucket metadata migration locking

Use a short dedicated migration lock timeout and fall back to the already loaded legacy or target configuration in memory when persistence is contended. Never fall back to an unlocked metadata save, and keep peer fan-out contexts free of stale lock markers.\n\nRefs: #102

Signed-off-by: Feng Ruohang <rh@vonng.com>

cmd/bucket-metadata-sys.go

@@ -123,6 +123,7 @@ func (sys *BucketMetadataSys) updateAndParse(ctx context.Context, bucket string,
 	if isMinioMetaBucketName(bucket) {
 		return updatedAt, errInvalidArgument
 	}
+	notifyCtx := ctx
 	ctx, unlock, err := lockBucketMetadata(ctx, objAPI, bucket)
 	if err != nil {
 		return updatedAt, err
@@ -195,7 +196,7 @@ func (sys *BucketMetadataSys) updateAndParse(ctx context.Context, bucket string,
 	if err != nil {
 		return updatedAt, err
 	}
-	globalNotificationSys.LoadBucketMetadata(bgContext(ctx), bucket) // Do not use caller context here
+	globalNotificationSys.LoadBucketMetadata(bgContext(notifyCtx), bucket) // Do not use caller context here
 	return updatedAt, nil
 }
 
@@ -228,8 +229,12 @@ func (sys *BucketMetadataSys) saveMetadata(ctx context.Context, objAPI ObjectLay
 }
 
 func lockBucketMetadata(ctx context.Context, objectAPI ObjectLayer, bucket string) (context.Context, func(), error) {
+	return lockBucketMetadataWithTimeout(ctx, objectAPI, bucket, globalOperationTimeout)
+}
+
+func lockBucketMetadataWithTimeout(ctx context.Context, objectAPI ObjectLayer, bucket string, timeout *dynamicTimeout) (context.Context, func(), error) {
 	lock := objectAPI.NewNSLock(minioMetaBucket, pathJoin(bucketMetaPrefix, bucket, "metadata.lock"))
-	lkctx, err := lock.GetLock(ctx, globalOperationTimeout)
+	lkctx, err := lock.GetLock(ctx, timeout)
 	if err != nil {
 		return nil, nil, err
 	}

cmd/bucket-metadata.go

@@ -250,10 +250,13 @@ func loadBucketMetadataParse(ctx context.Context, objectAPI ObjectLayer, bucket
 
 		if len(configs) > 0 {
 			if !bucketMetadataLockHeld(ctx, bucket) {
-				return loadBucketMetadataParseUnderLock(ctx, objectAPI, bucket, parse)
+				migrated, lockErr := loadBucketMetadataParseUnderLock(ctx, objectAPI, bucket, parse)
-			}
+				if lockErr == nil {
-			// Old bucket without bucket metadata. Hence we migrate existing settings.
+					return migrated, nil
-			if err = b.convertLegacyConfigs(ctx, objectAPI, configs); err != nil {
+				}
+				internalLogOnceIf(ctx, fmt.Errorf("unable to persist bucket metadata migration for %s, using the legacy configuration in memory: %w", bucket, lockErr), "bucket-metadata-migration-lock-"+bucket)
+				b.applyLegacyConfigs(configs)
+			} else if err = b.convertLegacyConfigs(ctx, objectAPI, configs); err != nil {
 				return b, err
 			}
 		}
@@ -274,7 +277,12 @@ func loadBucketMetadataParse(ctx context.Context, objectAPI ObjectLayer, bucket
 
 	// migrate unencrypted remote targets
 	if len(b.BucketTargetsConfigJSON) != 0 && GlobalKMS != nil && len(b.BucketTargetsConfigMetaJSON) == 0 && !bucketMetadataLockHeld(ctx, bucket) {
-		return loadBucketMetadataParseUnderLock(ctx, objectAPI, bucket, parse)
+		migrated, lockErr := loadBucketMetadataParseUnderLock(ctx, objectAPI, bucket, parse)
+		if lockErr == nil {
+			return migrated, nil
+		}
+		internalLogOnceIf(ctx, fmt.Errorf("unable to persist encrypted bucket target metadata for %s, using the existing configuration in memory: %w", bucket, lockErr), "bucket-metadata-migration-lock-"+bucket)
+		return b, nil
 	}
 	if err = b.migrateTargetConfig(ctx, objectAPI); err != nil {
 		return b, err
@@ -284,7 +292,7 @@ func loadBucketMetadataParse(ctx context.Context, objectAPI ObjectLayer, bucket
 }
 
 func loadBucketMetadataParseUnderLock(ctx context.Context, objectAPI ObjectLayer, bucket string, parse bool) (BucketMetadata, error) {
-	ctx, unlock, err := lockBucketMetadata(ctx, objectAPI, bucket)
+	ctx, unlock, err := lockBucketMetadataWithTimeout(ctx, objectAPI, bucket, bucketMetadataMigrationTimeout)
 	if err != nil {
 		return newBucketMetadata(bucket), err
 	}
@@ -292,6 +300,8 @@ func loadBucketMetadataParseUnderLock(ctx context.Context, objectAPI ObjectLayer
 	return loadBucketMetadataParse(ctx, objectAPI, bucket, parse)
 }
 
+var bucketMetadataMigrationTimeout = newDynamicTimeout(5*time.Second, time.Second)
+
 // loadBucketMetadata loads and migrates to bucket metadata.
 func loadBucketMetadata(ctx context.Context, objectAPI ObjectLayer, bucket string) (BucketMetadata, error) {
 	return loadBucketMetadataParse(ctx, objectAPI, bucket, true)
@@ -455,7 +465,7 @@ func (b *BucketMetadata) getAllLegacyConfigs(ctx context.Context, objectAPI Obje
 	return configs, nil
 }
 
-func (b *BucketMetadata) convertLegacyConfigs(ctx context.Context, objectAPI ObjectLayer, configs map[string][]byte) error {
+func (b *BucketMetadata) applyLegacyConfigs(configs map[string][]byte) {
 	for legacyFile, configData := range configs {
 		switch legacyFile {
 		case legacyBucketObjectLockEnabledConfigFile:
@@ -487,6 +497,10 @@ func (b *BucketMetadata) convertLegacyConfigs(ctx context.Context, objectAPI Obj
 		}
 	}
 	b.defaultTimestamps()
+}
+
+func (b *BucketMetadata) convertLegacyConfigs(ctx context.Context, objectAPI ObjectLayer, configs map[string][]byte) error {
+	b.applyLegacyConfigs(configs)
 
 	if err := b.Save(ctx, objectAPI); err != nil {
 		return err