Skip to content

Commit db5cf0a

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 2371269 commit db5cf0a

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
@@ -218,13 +218,8 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
218218
return nil, errors.New("append option is not compatible with type option")
219219
}
220220

221-
locked := true
222221
as.lock.Lock()
223-
defer func() {
224-
if locked {
225-
as.lock.Unlock()
226-
}
227-
}()
222+
defer as.lock.Unlock()
228223

229224
// Check if artifact already exists
230225
artifacts, err := as.getArtifacts(ctx, nil)
@@ -297,10 +292,11 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, artifactBlobs []li
297292
}
298293
defer imageDest.Close()
299294

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

305301
// ImageDestination, in general, requires the caller to write a full image; here we may write only the added layers.
306302
// 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
356352
artifactManifest.Layers = append(artifactManifest.Layers, newLayer)
357353
}
358354

359-
as.lock.Lock()
360-
locked = true
355+
// Lock is still held from the beginning of the function
361356

362357
rawData, err := json.Marshal(artifactManifest)
363358
if err != nil {

0 commit comments

Comments
 (0)