From db5cf0aa7ffd40be6cbb4175d5e7c09c76074661 Mon Sep 17 00:00:00 2001
From: Daniel J Walsh <dwalsh@redhat.com>
Date: Fri, 21 Nov 2025 10:50:03 -0500
Subject: [PATCH] Fix race condition in concurrent artifact additions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes a race condition where concurrent 'podman artifact add'
commands for different artifacts would result in only one artifact
being created, without any error messages.

The root cause was in the artifact store's Add() method, which
would:
1. Acquire lock
2. Read OCI layout index
3. Create ImageDestination (which snapshots the index)
4. RELEASE lock (optimization for blob copying)
5. Copy blobs (while unlocked)
6. Reacquire lock
7. Commit changes (write new index)

When two concurrent additions happened:
- Process A: Lock → Read index → Create dest A → Unlock → Copy blobs
- Process B: Lock → Read index (no artifact A!) → Create dest B → Unlock
- Process A: Lock → Commit (write index with A)
- Process B: Lock → Commit (write index with B, OVERWRITING A)

The fix keeps the lock held for the entire operation. While this
reduces concurrency for blob copying, it prevents the index file
corruption that caused artifacts to be lost.

Changes:
- Remove lock release/reacquire around blob copying in store.Add()
- Simplify lock management (no more conditional unlock)

Fixes: https://github.com/containers/podman/issues/27569

Generated-with: Cursor AI
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
---
 common/pkg/libartifact/store/store.go | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/common/pkg/libartifact/store/store.go b/common/pkg/libartifact/store/store.go
index 0665cbe73c..7730222fc6 100644
--- a/common/pkg/libartifact/store/store.go
+++ b/common/pkg/libartifact/store/store.go
@@ -218,13 +218,8 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
 		return nil, errors.New("append option is not compatible with type option")
 	}
 
-	locked := true
 	as.lock.Lock()
-	defer func() {
-		if locked {
-			as.lock.Unlock()
-		}
-	}()
+	defer as.lock.Unlock()
 
 	// Check if artifact already exists
 	artifacts, err := as.getArtifacts(ctx, nil)
@@ -297,10 +292,11 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
 	}
 	defer imageDest.Close()
 
-	// Unlock around the actual pull of the blobs.
-	// This is ugly as hell, but should be safe.
-	locked = false
-	as.lock.Unlock()
+	// Keep the lock held during blob copying to prevent concurrent artifact adds
+	// from racing on the OCI layout index file. The lock was previously released
+	// here as an optimization, but this created a race condition where concurrent
+	// artifact additions could overwrite each other's index entries.
+	// See: https://github.com/containers/podman/issues/27569
 
 	// ImageDestination, in general, requires the caller to write a full image; here we may write only the added layers.
 	// This works for the oci/layout transport we hard-code.
@@ -356,8 +352,7 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
 		artifactManifest.Layers = append(artifactManifest.Layers, newLayer)
 	}
 
-	as.lock.Lock()
-	locked = true
+	// Lock is still held from the beginning of the function
 
 	rawData, err := json.Marshal(artifactManifest)
 	if err != nil {