Skip to content

Commit 2247980

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) - Add e2e test for concurrent artifact additions - Add standalone test script to verify the fix Fixes: #27569 Generated-with: Cursor AI Signed-off-by: Daniel J Walsh <[email protected]>
1 parent 7cd9b81 commit 2247980

File tree

3 files changed

+65
-12
lines changed

3 files changed

+65
-12
lines changed

go.mod

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,3 +195,5 @@ require (
195195
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
196196
tags.cncf.io/container-device-interface/specs-go v1.0.0 // indirect
197197
)
198+
199+
replace go.podman.io/common => github.com/rhatdan/container-libs b801c9d727f587ca2c9d50ca3a3278fd6c4c979d

test/e2e/artifact_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -741,6 +741,62 @@ var _ = Describe("Podman artifact", func() {
741741
session = podmanTest.PodmanExitCleanly("artifact", "inspect", artifactDigest[:12], "-f", "{{.Name}}")
742742
Expect(session.OutputToString()).To(Equal(artifact1Name))
743743
})
744+
745+
// Regression test for https://github.com/containers/podman/issues/27569
746+
It("podman artifact add concurrent - no race condition", func() {
747+
const numArtifacts = 5
748+
749+
// Create temporary files for artifacts
750+
artifactFiles := make([]string, numArtifacts)
751+
artifactNames := make([]string, numArtifacts)
752+
for i := 0; i < numArtifacts; i++ {
753+
file, err := createArtifactFile(int64(1024 * (i + 1))) // Different sizes
754+
Expect(err).ToNot(HaveOccurred())
755+
artifactFiles[i] = file
756+
artifactNames[i] = fmt.Sprintf("localhost/concurrent/artifact%d", i)
757+
}
758+
759+
// Run all artifact add commands concurrently
760+
sessions := make([]*PodmanSessionIntegration, numArtifacts)
761+
for i := 0; i < numArtifacts; i++ {
762+
// Start all commands without waiting
763+
sessions[i] = podmanTest.Podman([]string{"artifact", "add", artifactNames[i], artifactFiles[i]})
764+
}
765+
766+
// Wait for all to complete and collect results
767+
digests := make([]string, numArtifacts)
768+
for i := 0; i < numArtifacts; i++ {
769+
sessions[i].WaitWithDefaultTimeout()
770+
Expect(sessions[i]).Should(ExitCleanly())
771+
digests[i] = sessions[i].OutputToString()
772+
Expect(digests[i]).To(HaveLen(64)) // SHA256 digest length
773+
}
774+
775+
// Verify all artifacts were created successfully
776+
listSession := podmanTest.PodmanExitCleanly("artifact", "ls", "--format", "{{.Repository}}")
777+
output := listSession.OutputToStringArray()
778+
779+
// Check that all artifact names are in the list
780+
for i := 0; i < numArtifacts; i++ {
781+
Expect(output).To(ContainElement(artifactNames[i]),
782+
fmt.Sprintf("Artifact %s should be in the list", artifactNames[i]))
783+
}
784+
785+
// Verify each artifact can be inspected
786+
for i := 0; i < numArtifacts; i++ {
787+
inspectSession := podmanTest.PodmanExitCleanly("artifact", "inspect", artifactNames[i])
788+
Expect(inspectSession.OutputToString()).To(ContainSubstring(artifactNames[i]))
789+
}
790+
791+
// Verify each artifact has the correct size
792+
for i := 0; i < numArtifacts; i++ {
793+
expectedSize := 1024 * (i + 1)
794+
sizeSession := podmanTest.PodmanExitCleanly("artifact", "ls", "--format", "{{.VirtualSize}}", "--noheading")
795+
sizes := sizeSession.OutputToStringArray()
796+
Expect(sizes).To(ContainElement(fmt.Sprintf("%d", expectedSize)),
797+
fmt.Sprintf("Artifact %d should have size %d", i, expectedSize))
798+
}
799+
})
744800
})
745801

746802
func digestToFilename(digest string) string {

vendor/go.podman.io/common/pkg/libartifact/store/store.go

Lines changed: 7 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)