Tweak solver's same-origin heuristic (#2208)

Previously, we'd always prefer providers of constraints that come from
the same origin as a package we'd already selected. This made it very
difficult to move a package to a different origin, as the heuristic
would select the older version of an APK that matched the same origin.

This change tracks not just origins we've already selected, but the
version of that origin, so we don't end up stuck on an older version.

Signed-off-by: Jon Johnson <jon.johnson@chainguard.dev>

Author: jonjohnsonjr

pkg/apk/apk/repo.go

@@ -569,11 +569,11 @@ func (p *PkgResolver) GetPackagesWithDependencies(ctx context.Context, packages
 func (p *PkgResolver) GetPackageWithDependencies(ctx context.Context, pkgName string, existing map[string]*RepositoryPackage, dq map[*RepositoryPackage]string) (*RepositoryPackage, []*RepositoryPackage, []string, error) {
 	parents := make(map[string]bool)
 	localExisting := make(map[string]*RepositoryPackage, len(existing))
-	existingOrigins := map[string]bool{}
+	existingOrigins := map[string]string{}
 	for k, v := range existing {
 		localExisting[k] = v
 		if v != nil && v.Origin != "" {
-			existingOrigins[v.Origin] = true
+			existingOrigins[v.Origin] = v.Version
 		}
 	}
 
@@ -712,7 +712,7 @@ func (p *PkgResolver) resolvePackage(pkgName string, dq map[*RepositoryPackage]s
 // It might change the order of install.
 // In other words, this _should_ be a DAG (acyclical), but because the packages
 // are just listing dependencies in text, it might be cyclical. We need to be careful of that.
-func (p *PkgResolver) getPackageDependencies(ctx context.Context, pkg *RepositoryPackage, allowPin string, parents map[string]bool, existing map[string]*RepositoryPackage, existingOrigins map[string]bool, dq map[*RepositoryPackage]string) (dependencies []*RepositoryPackage, conflicts []string, err error) {
+func (p *PkgResolver) getPackageDependencies(ctx context.Context, pkg *RepositoryPackage, allowPin string, parents map[string]bool, existing map[string]*RepositoryPackage, existingOrigins map[string]string, dq map[*RepositoryPackage]string) (dependencies []*RepositoryPackage, conflicts []string, err error) {
 	if err := ctx.Err(); err != nil {
 		return nil, nil, context.Cause(ctx)
 	}
@@ -902,7 +902,9 @@ func (p *PkgResolver) getPackageDependencies(ctx context.Context, pkg *Repositor
 		conflicts = append(conflicts, confs...)
 		for _, dep := range subDeps {
 			existing[dep.Name] = dep
-			existingOrigins[dep.Origin] = true
+			if dep.Origin != "" {
+				existingOrigins[dep.Origin] = dep.Version
+			}
 		}
 	}
 	return dependencies, conflicts, nil
@@ -944,11 +946,11 @@ func cachedResolvePackageNameVersionPin(pkgName string) ParsedConstraint {
 // For example, if the original search was for package "a", then pkgs may contain some that
 // are named "a", but others that provided "a". In that case, we should look not at the
 // version of the package, but the version of "a" that the package provides.
-func (p *PkgResolver) sortPackages(pkgs []*repositoryPackage, compare *RepositoryPackage, name string, existing map[string]*RepositoryPackage, existingOrigins map[string]bool, pin string) {
+func (p *PkgResolver) sortPackages(pkgs []*repositoryPackage, compare *RepositoryPackage, name string, existing map[string]*RepositoryPackage, existingOrigins map[string]string, pin string) {
 	slices.SortFunc(pkgs, p.comparePackages(compare, name, existing, existingOrigins, pin))
 }
 
-func (p *PkgResolver) comparePackages(compare *RepositoryPackage, name string, existing map[string]*RepositoryPackage, existingOrigins map[string]bool, pin string) func(a, b *repositoryPackage) int { //nolint:gocyclo
+func (p *PkgResolver) comparePackages(compare *RepositoryPackage, name string, existing map[string]*RepositoryPackage, existingOrigins map[string]string, pin string) func(a, b *repositoryPackage) int { //nolint:gocyclo
 	return func(a, b *repositoryPackage) int {
 		// determine versions
 		iVersionStr := p.getDepVersionForName(a, name)
@@ -990,9 +992,12 @@ func (p *PkgResolver) comparePackages(compare *RepositoryPackage, name string, e
 		}
 		// both matched, so keep looking
 
-		// see if an origin already is installed
-		iOriginMatched := existingOrigins[a.Origin]
-		jOriginMatched := existingOrigins[b.Origin]
+		// Prefer a candidate whose origin we've already pulled in, but only at
+		// the version we pulled in. Otherwise this heuristic would keep us on
+		// an older version of an origin (e.g. when a package moves origins at
+		// a newer version, or an old binary lingers in the index after rebuild).
+		iOriginMatched := a.Origin != "" && existingOrigins[a.Origin] == a.Version
+		jOriginMatched := b.Origin != "" && existingOrigins[b.Origin] == b.Version
 		if iOriginMatched && !jOriginMatched {
 			return -1
 		}
@@ -1052,7 +1057,7 @@ func (p *PkgResolver) comparePackages(compare *RepositoryPackage, name string, e
 	}
 }
 
-func (p *PkgResolver) bestPackage(pkgs []*repositoryPackage, compare *RepositoryPackage, name string, existing map[string]*RepositoryPackage, existingOrigins map[string]bool, pin string) *repositoryPackage {
+func (p *PkgResolver) bestPackage(pkgs []*repositoryPackage, compare *RepositoryPackage, name string, existing map[string]*RepositoryPackage, existingOrigins map[string]string, pin string) *repositoryPackage {
 	if len(pkgs) == 0 {
 		return nil
 	}

pkg/apk/apk/repo_test.go

@@ -764,7 +764,7 @@ func TestSortPackages(t *testing.T) {
 				pkgs            []*RepositoryPackage
 				pkg             *RepositoryPackage
 				existing        = map[string]*RepositoryPackage{}
-				existingOrigins = map[string]bool{}
+				existingOrigins = map[string]string{}
 			)
 			for _, pkg := range tt.pkgs {
 				// we cheat and use the InstalledSize for the preferred order, so that it gets carried around.
@@ -778,7 +778,7 @@ func TestSortPackages(t *testing.T) {
 			}
 			for _, pkg := range tt.existing {
 				existing[pkg.pkg.Name] = NewRepositoryPackage(pkg.pkg, &RepositoryWithIndex{Repository: &Repository{URI: pkg.repo}})
-				existingOrigins[pkg.pkg.Origin] = true
+				existingOrigins[pkg.pkg.Origin] = pkg.pkg.Version
 			}
 			namedPkgs := testNamedPackageFromPackages(pkgs)
 			pr := NewPkgResolver(context.Background(), []NamedIndex{})
@@ -900,6 +900,43 @@ func TestHigherProvidedVersion(t *testing.T) {
 	}
 }
 
+// When an origin is bumped to a new version that no longer ships one of its
+// old subpackages, but the old subpackage still lingers in the index providing
+// some so:, the resolver must not prefer that stale package over a fresh
+// provider in a different origin just because we already pulled the origin at
+// a different version.
+//
+// As a contrived example, if we want to drop libcrypt1 from glibc's origin,
+// it was difficult because we would prefer the libcrypt.so.1 provider due to
+// that same-origin heuristic. This tests that the heuristic does not activate
+// if the origins match but the versions don't.
+func TestProviderAcrossOriginVersionBump(t *testing.T) {
+	repo := Repository{}
+	index := repo.WithIndex(&APKIndex{
+		Packages: []*Package{
+			{Name: "glibc", Version: "1", Origin: "glibc"},
+			{Name: "libcrypt1", Version: "1", Origin: "glibc",
+				Provides: []string{"so:libcrypt.so.1=1"}},
+			{Name: "glibc", Version: "2", Origin: "glibc"},
+			{Name: "libxcrypt", Version: "2", Origin: "libxcrypt",
+				Provides: []string{"so:libcrypt.so.1=1"}},
+			{Name: "consumer", Version: "1",
+				Dependencies: []string{"so:libcrypt.so.1"}},
+		},
+	})
+	resolver := NewPkgResolver(context.Background(), testNamedRepositoryFromIndexes([]*RepositoryWithIndex{index}))
+	pkgs, _, err := resolver.GetPackagesWithDependencies(context.Background(), []string{"consumer", "glibc=2"}, nil)
+	require.NoError(t, err)
+
+	got := make([]string, 0, len(pkgs))
+	for _, p := range pkgs {
+		got = append(got, p.Filename())
+	}
+	require.NotContains(t, got, "libcrypt1-1.apk", "should not pull stale libcrypt1 from old glibc origin")
+	require.Contains(t, got, "libxcrypt-2.apk", "should select libxcrypt for so:libcrypt.so.1")
+	require.Contains(t, got, "glibc-2.apk")
+}
+
 func TestConstrains(t *testing.T) {
 	providers := map[string][]string{
 		"ld-linux=2.38-r10": {"so:ld-linux-aarch64.so.1=1.0"},

pkg/apk/apk/version_test.go

@@ -910,10 +910,10 @@ func TestResolveVersion(t *testing.T) {
 			found := filterPackages(pkgs, map[*RepositoryPackage]string{}, withVersion(tt.version, tt.compare), withPreferPin(tt.pin), withInstalledPackage(tt.installed))
 			// add the existing in, if any
 			existing := make(map[string]*RepositoryPackage)
-			existingOrigins := make(map[string]bool)
+			existingOrigins := make(map[string]string)
 			if tt.installed != nil {
 				existing[tt.installed.Name] = tt.installed
-				existingOrigins[tt.installed.Origin] = true
+				existingOrigins[tt.installed.Origin] = tt.installed.Version
 			}
 			pkg := pr.bestPackage(found, nil, "", existing, existingOrigins, tt.pin)
 			if tt.want == "" {