Skip to content

libimage: fix manifest race for AnnotateInstance() and RemoveInstance() #2504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 3 commits into from
Aug 14, 2025
Merged

libimage: fix manifest race for AnnotateInstance() and RemoveInstance() #2504

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4fffcf3
libimage: add missing manifest lock
Luap99 Aug 7, 2025
7da5c7a
libimage: dedup manifest reload() and saveAndReload()
Luap99 Aug 7, 2025
18a0916
libimage: remove extra image reload in manifest reload
Luap99 Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 29 additions & 16 deletions libimage/manifest_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,27 +319,16 @@ func (m *ManifestList) saveAndReload() error {
if err != nil {
return err
}

// Make sure to reload the image from the containers storage to fetch
// the latest data (e.g., new or delete digests).
if err := m.image.reload(); err != nil {
return err
}
image, list, err := m.image.runtime.lookupManifestList(newID)
if err != nil {
return err
}
m.image = image
m.list = list
return nil
return m.reloadID(newID)
}

// Reload the image and list instances from storage
func (m *ManifestList) reload() error {
listID := m.ID()
if err := m.image.reload(); err != nil {
return err
}
return m.reloadID(listID)
}

func (m *ManifestList) reloadID(listID string) error {
image, list, err := m.image.runtime.lookupManifestList(listID)
if err != nil {
return err
Expand Down Expand Up @@ -669,6 +658,18 @@ func (m *ManifestList) AnnotateInstance(d digest.Digest, options *ManifestListAn
return nil
}

locker, err := manifests.LockerForImage(m.image.runtime.store, m.ID())
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider refactoring the repeated lock, reload, and unlock logic into a helper method to reduce boilerplate and improve maintainability.

Here’s one way to collapse all of the repeated “lock, reload, defer‐unlock” boilerplate into a small helper. You still get the same behavior (locking + initial reload) and leave both saveAndReload and reload untouched.

// add this near the top of manifest_list.go

// withLockAndReload grabs the image‐specific mutex, reloads the
// in‐memory state, then runs `action`.  Unlock happens via defer.
func (m *ManifestList) withLockAndReload(action func() error) error {
    locker, err := manifests.LockerForImage(m.image.runtime.store, m.ID())
    if err != nil {
        return err
    }
    locker.Lock()
    defer locker.Unlock()

    if err := m.reload(); err != nil {
        return err
    }
    return action()
}

Then reduce your two methods to:

func (m *ManifestList) AnnotateInstance(d digest.Digest, options *ManifestListAnnotateOptions) error {
    if options == nil {
        return nil
    }
    return m.withLockAndReload(func() error {
        if len(options.OS) > 0 {
            if err := m.list.SetOS(d, options.OS); err != nil {
                return err
            }
        }
        if len(options.OSVersion) > 0 {
            if err := m.list.SetOSVersion(d, options.OSVersion); err != nil {
                return err
            }
        }
        return m.saveAndReload()
    })
}

func (m *ManifestList) RemoveInstance(d digest.Digest) error {
    return m.withLockAndReload(func() error {
        if err := m.list.Remove(d); err != nil {
            return err
        }
        return m.saveAndReload()
    })
}

This removes the duplicated lock/unlock+reload in each method while preserving all original functionality.

if err != nil {
return err
}
locker.Lock()
defer locker.Unlock()
// Make sure to reload the image from the containers storage to fetch
// the latest data (e.g., new or delete digests).
if err := m.reload(); err != nil {
return err
}

if len(options.OS) > 0 {
if err := m.list.SetOS(d, options.OS); err != nil {
return err
Expand Down Expand Up @@ -752,6 +753,18 @@ func (m *ManifestList) AnnotateInstance(d digest.Digest, options *ManifestListAn
// RemoveInstance removes the instance specified by `d` from the manifest list.
// Returns the new ID of the image.
func (m *ManifestList) RemoveInstance(d digest.Digest) error {
locker, err := manifests.LockerForImage(m.image.runtime.store, m.ID())
if err != nil {
return err
}
locker.Lock()
defer locker.Unlock()
// Make sure to reload the image from the containers storage to fetch
// the latest data (e.g., new or delete digests).
if err := m.reload(); err != nil {
return err
}

if err := m.list.Remove(d); err != nil {
return err
}
Expand Down