Skip to content

Commit f6b706a

Browse files
committed
Fix race condition in concurrent artifact additions
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: containers/podman#27569 Generated-with: Cursor AI Signed-off-by: Daniel J Walsh <[email protected]>
1 parent aa970d2 commit f6b706a

File tree

1 file changed

+7
-12
lines changed

1 file changed

+7
-12
lines changed

common/pkg/libartifact/store/store.go

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -225,13 +225,8 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
225225
return nil, fmt.Errorf("cannot override filename with %s annotation", specV1.AnnotationTitle)
226226
}
227227

228-
locked := true
229228
as.lock.Lock()
230-
defer func() {
231-
if locked {
232-
as.lock.Unlock()
233-
}
234-
}()
229+
defer as.lock.Unlock()
235230

236231
// Check if artifact already exists
237232
artifacts, err := as.getArtifacts(ctx, nil)
@@ -304,10 +299,11 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
304299
}
305300
defer imageDest.Close()
306301

307-
// Unlock around the actual pull of the blobs.
308-
// This is ugly as hell, but should be safe.
309-
locked = false
310-
as.lock.Unlock()
302+
// Keep the lock held during blob copying to prevent concurrent artifact adds
303+
// from racing on the OCI layout index file. The lock was previously released
304+
// here as an optimization, but this created a race condition where concurrent
305+
// artifact additions could overwrite each other's index entries.
306+
// See: https://github.com/containers/podman/issues/27569
311307

312308
// ImageDestination, in general, requires the caller to write a full image; here we may write only the added layers.
313309
// This works for the oci/layout transport we hard-code.
@@ -353,8 +349,7 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
353349
artifactManifest.Layers = append(artifactManifest.Layers, newLayer)
354350
}
355351

356-
as.lock.Lock()
357-
locked = true
352+
// Lock is still held from the beginning of the function
358353

359354
rawData, err := json.Marshal(artifactManifest)
360355
if err != nil {

0 commit comments

Comments
 (0)