Fixup: Bundle clean-up

Personally, I think it would be simpler to use `GetVersions` from
the registry.

Bundle registry is a dependency for both runtime registry and
manager (which will be later made a worker?). I find this
unnecessary abstraction (store interface), but maybe there is
a good reason for it?

go/runtime/bundle/manager.go

@@ -48,11 +48,13 @@ type ManifestStore interface {
 	// to the specified directory, to the store.
 	AddManifest(manifest *Manifest, dir string) error
 
-	// RemoveManifests removes all regular manifests for the specified runtimeID
-	// whose versions are lower than the provided version.
+	// RemoveManifest removes a manifest with provided hash.
 	//
-	// Returns a list of directories for the removed exploded bundles.
-	RemoveManifests(runtimeID common.Namespace, version version.Version) ([]string, error)
+	// Returns a path to exploded bundle dir of the removed manifest.
+	RemoveManifest(hash hash.Hash) (string, error)
+
+	// Manifests returns all known manifests (regular and detached).
+	Manifests() []*Manifest
 }
 
 // Manager is responsible for managing bundles.
@@ -276,7 +278,7 @@ func (m *Manager) download() {
 	}
 }
 
-// Clean removes outdated manifest hashes and deletes corresponding
+// Clean removes outdated manifest and deletes corresponding
 // exploded bundles for runtimes in the clean-up queue.
 func (m *Manager) Clean() {
 	m.logger.Info("removing regular bundles")
@@ -481,21 +483,53 @@ func (m *Manager) cleanStaleBundles(runtimeID common.Namespace) {
 		return
 	}
 
-	// TODO should you also remove dirs for manifests successfully removed,
-	// even if error?
-	dirs, err := m.store.RemoveManifests(runtimeID, active)
+	m.logger.Info("starting clean-up of stale bundles",
+		"id", runtimeID,
+		"active", active)
+
+	for _, manifest := range m.store.Manifests() {
+		if manifest.ID != runtimeID {
+			continue
+		}
+
+		if manifest.IsDetached() {
+			continue
+		}
+
+		if !manifest.GetVersion().Less(active) {
+			continue
+		}
+
+		err := m.cleanStaleBundle(manifest.Hash())
+		if err != nil {
+			m.logger.Error("failed to clean stale bundle",
+				"err", err)
+			m.logger.Info("aborting clean-up",
+				"id", runtimeID,
+				"active", active)
+			return
+		}
+	}
+
+	// Remove runtime ID from the clean-up queue if active version is still the same.
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	if m.cleanupQueue[runtimeID].Cmp(active) == 0 {
+		delete(m.cleanupQueue, runtimeID)
+	}
+}
+
+func (m *Manager) cleanStaleBundle(hash hash.Hash) error {
+	dir, err := m.store.RemoveManifest(hash)
 	if err != nil {
-		m.logger.Error("failed to remove regular manifests from the registry",
-			"runtimeID", runtimeID,
-			"version", active,
-			"err", err,
-		)
-		return
+		return fmt.Errorf("failed to remove manifest with hash %s from the registry: %w", hash.Hex(), err)
 	}
 
-	for _, dir := range dirs {
-		_ = m.removeBundle(dir)
+	// Remove exploded bundle dir.
+	if err := m.removeBundle(dir); err != nil {
+		return err
 	}
+	return nil
 }
 
 func (m *Manager) loadManifests() (map[string]*Manifest, error) {

go/runtime/bundle/manager_test.go

@@ -5,9 +5,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	"github.com/oasisprotocol/oasis-core/go/common"
 	"github.com/oasisprotocol/oasis-core/go/common/crypto/hash"
-	"github.com/oasisprotocol/oasis-core/go/common/version"
 )
 
 type mockStore struct {
@@ -30,7 +28,11 @@ func (r *mockStore) AddManifest(manifest *Manifest, _ string) error {
 	return nil
 }
 
-func (r *mockStore) RemoveManifests(_ common.Namespace, _ version.Version) ([]string, error) {
+func (r *mockStore) Manifests() []*Manifest {
+	panic("RemoveManifests not implemented")
+}
+
+func (r *mockStore) RemoveManifest(_ hash.Hash) (string, error) {
 	panic("RemoveManifests not implemented")
 }
 

go/runtime/bundle/manifest.go

@@ -138,6 +138,27 @@ func (m *Manifest) GetComponentByID(id component.ID) *Component {
 	return nil
 }
 
+// GetVersion returns the runtime version.
+//
+// TODO: discuss with reviewers I would argue we should reintroduce this function?
+// Agree with the bug fix of constant manifest hash...
+func (m *Manifest) GetVersion() version.Version {
+	// We also support legacy manifests which define version at the top-level.
+	for _, comp := range m.Components {
+		if !comp.ID().IsRONL() {
+			continue
+		}
+
+		if comp.Version.ToU64() > m.Version.ToU64() {
+			return comp.Version
+		}
+
+		break
+	}
+
+	return m.Version
+}
+
 // UnmarshalJSON customizes the unmarshalling of the manifest.
 func (m *Manifest) UnmarshalJSON(b []byte) (err error) {
 	// Unmarshal into the auxiliary struct to avoid recursion.

go/runtime/bundle/registry.go

@@ -1,7 +1,6 @@
 package bundle
 
 import (
-	"errors"
 	"fmt"
 	"maps"
 	"slices"
@@ -20,14 +19,20 @@ import (
 	rtConfig "github.com/oasisprotocol/oasis-core/go/runtime/config"
 )
 
+// explodedManifest is manifest with corresponding exploded bundle dir.
+type explodedManifest struct {
+	manifest    *Manifest
+	explodedDir string
+}
+
 // Registry is a registry of manifests and components.
 type Registry struct {
 	mu sync.RWMutex
 
-	manifestHashes   map[hash.Hash]struct{}
-	regularManifests map[common.Namespace]map[version.Version]*Manifest
-	components       map[common.Namespace]map[component.ID]map[version.Version]*ExplodedComponent
-	notifiers        map[common.Namespace]*pubsub.Broker
+	explodedManifests map[hash.Hash]explodedManifest
+	regularManifests  map[common.Namespace]map[version.Version]*Manifest
+	components        map[common.Namespace]map[component.ID]map[version.Version]*ExplodedComponent
+	notifiers         map[common.Namespace]*pubsub.Broker
 
 	logger *logging.Logger
 }
@@ -37,11 +42,11 @@ func NewRegistry() *Registry {
 	logger := logging.GetLogger("runtime/bundle/registry")
 
 	return &Registry{
-		manifestHashes:   make(map[hash.Hash]struct{}),
-		regularManifests: make(map[common.Namespace]map[version.Version]*Manifest),
-		components:       make(map[common.Namespace]map[component.ID]map[version.Version]*ExplodedComponent),
-		notifiers:        make(map[common.Namespace]*pubsub.Broker),
-		logger:           logger,
+		explodedManifests: make(map[hash.Hash]explodedManifest),
+		regularManifests:  make(map[common.Namespace]map[version.Version]*Manifest),
+		components:        make(map[common.Namespace]map[component.ID]map[version.Version]*ExplodedComponent),
+		notifiers:         make(map[common.Namespace]*pubsub.Broker),
+		logger:            logger,
 	}
 }
 
@@ -51,7 +56,7 @@ func (r *Registry) HasManifest(hash hash.Hash) bool {
 	r.mu.RLock()
 	defer r.mu.RUnlock()
 
-	_, ok := r.manifestHashes[hash]
+	_, ok := r.explodedManifests[hash]
 	return ok
 }
 
@@ -69,7 +74,7 @@ func (r *Registry) AddManifest(manifest *Manifest, dir string) error {
 	)
 
 	// Skip already processed manifests.
-	if _, ok := r.manifestHashes[manifestHash]; ok {
+	if _, ok := r.explodedManifests[manifestHash]; ok {
 		return nil
 	}
 
@@ -143,7 +148,7 @@ func (r *Registry) AddManifest(manifest *Manifest, dir string) error {
 	}
 
 	// Remember which manifests were added.
-	r.manifestHashes[manifestHash] = struct{}{}
+	r.explodedManifests[manifestHash] = explodedManifest{manifest: manifest, explodedDir: dir}
 
 	r.logger.Info("manifest added",
 		"name", manifest.Name,
@@ -153,58 +158,41 @@ func (r *Registry) AddManifest(manifest *Manifest, dir string) error {
 	return nil
 }
 
-// RemoveManifests removes all regular manifests for the specified runtime ID
-// whose versions are lower than the provided version.
-//
-// Returns a list of directories for the removed exploded bundles.
-func (r *Registry) RemoveManifests(runtimeID common.Namespace, version version.Version) ([]string, error) {
-	r.logger.Info("removing manifests with versions lower then provided",
-		"id", runtimeID,
-		"version", version,
-	)
-
-	var stale []string
-	for _, v := range r.GetVersions(runtimeID) {
-		if !v.Less(version) {
-			continue
-		}
-
-		dir, err := r.removeManifest(runtimeID, v)
-		if err != nil {
-			return nil, err
-		}
-		stale = append(stale, dir)
+// Manifests returns all known manifests (regular and detached).
+func (r *Registry) Manifests() []*Manifest {
+	r.mu.RLock()
+	defer r.mu.RUnlock()
+	var manifests []*Manifest
+	for _, m := range r.explodedManifests {
+		manifests = append(manifests, m.manifest)
 	}
-	return stale, nil
+	return manifests
 }
 
-func (r *Registry) removeManifest(runtimeID common.Namespace, version version.Version) (string, error) {
+// RemoveManifest removes a manifest with provided hash.
+//
+// Returns a path to exploded bundle dir of the removed manifest.
+func (r *Registry) RemoveManifest(hash hash.Hash) (string, error) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
-	var explDir string
-	manifest, ok := r.regularManifests[runtimeID][version]
+	explManifest, ok := r.explodedManifests[hash]
 	if !ok {
-		return explDir, fmt.Errorf("missing regular manifest for runtime ID %v with %s", runtimeID, version.String())
-	}
-	explComp := r.components[runtimeID][component.ID_RONL][version]
-	if explComp == nil {
-		return explDir, errors.New("components missing RONL component")
+		return "", fmt.Errorf("missing manifest with hash %s", hash.Hex())
 	}
-	explDir = explComp.ExplodedDataDir
-	manifestHash := manifest.Hash()
 
-	r.logger.Info("Removing (regular) manifest from registry",
+	runtimeID := explManifest.manifest.ID
+	version := explManifest.manifest.GetVersion()
+	r.logger.Info("Removing manifest from registry",
 		"id", runtimeID,
 		"version", version,
-		"manifest_hash", manifestHash,
+		"manifest_hash", hash,
 	)
-
 	delete(r.regularManifests[runtimeID], version)
-	delete(r.manifestHashes, manifestHash)
-	for _, c := range manifest.Components {
+	delete(r.explodedManifests, hash)
+	for _, c := range explManifest.manifest.Components {
 		delete(r.components[runtimeID][c.ID()], c.Version)
 	}
-	return explDir, nil
+	return explManifest.explodedDir, nil
 }
 
 // GetVersions returns versions for the given runtime, sorted in ascending