sources, manifest: don't misreport a transiently-unavailable source as "unknown package" (3/4)

env.go

@@ -688,6 +688,18 @@ func (e *Env) Install(l *ui.UI, pkg *manifest.Package) (*shell.Changes, error) {
 	return allChanges.Merge(changes), nil
 }
 
+// isUnresolved reports whether err means a package reference couldn't be
+// resolved, whether because it's genuinely unknown (manifest.ErrUnknownPackage)
+// or because a source needed to confirm that was transiently unreachable
+// (sources.ErrSourceUnavailable). Callers that fall back to an alternate
+// resolution strategy (a virtual package, a different selector, a resync)
+// should attempt that fallback in both cases: a transiently-unavailable
+// source is not evidence that the alternate strategy would fail too, and may
+// well succeed via a different, healthy source.
+func isUnresolved(err error) bool {
+	return errors.Is(err, manifest.ErrUnknownPackage) || errors.Is(err, sources.ErrSourceUnavailable)
+}
+
 // resolveRuntimeDependencies checks all runtime dependencies for a package are available.
 //
 // Aggregate and collect the package names and binaries of all runtime dependencies to avoid collisions.
@@ -702,7 +714,7 @@ func (e *Env) resolveRuntimeDependencies(l *ui.UI, p *manifest.Package, aggregat
 
 		depPkg, err := e.Resolve(l, manifest.ExactSelector(ref), true)
 		// If the package doesn't exist, try resolving as a virtual package
-		if err != nil && errors.Is(err, manifest.ErrUnknownPackage) {
+		if err != nil && isUnresolved(err) {
 			virtualRef, verr := e.resolveVirtual(l, ref.Name)
 			if verr != nil {
 				return errors.WithStack(err) // Return original error
@@ -945,7 +957,7 @@ func (e *Env) Resolve(l *ui.UI, selector manifest.Selector, syncOnMissing bool)
 	}
 	resolved, err := resolver.Resolve(l, selector)
 	// If the package is missing sync sources and try again, once.
-	if syncOnMissing && errors.Is(err, manifest.ErrUnknownPackage) {
+	if syncOnMissing && isUnresolved(err) {
 		if err = resolver.Sync(l, true); err != nil {
 			return nil, errors.WithStack(err)
 		}
@@ -1575,7 +1587,7 @@ func (e *Env) ResolveWithDeps(l *ui.UI, installed []manifest.Reference, selector
 
 		// First search from virtual providers
 		ref, err = e.resolveVirtual(l, req)
-		if err != nil && errors.Is(err, manifest.ErrUnknownPackage) {
+		if err != nil && isUnresolved(err) {
 			// Secondly search by the package name
 			sel, err := manifest.ParseGlobSelector(req)
 			if err != nil {

manifest/loader.go

@@ -65,24 +65,62 @@ func (l *Loader) get(name string) (*AnnotatedManifest, error) {
 	file, ok := l.files[name]
 	if !ok {
 		path := name + ".hcl"
+		// unavailable records the first sources.ErrSourceUnavailable seen
+		// while searching, but we keep searching the remaining bundles: one
+		// transiently-unavailable source must never mask a package provided
+		// by another, healthy source.
+		var unavailable error
 		for _, bundle := range l.sources.Bundles() {
-			file = load(bundle, name, path)
-			if file == nil {
+			f, err := load(bundle, name, path)
+			if err != nil {
+				if unavailable == nil {
+					unavailable = err
+				}
+				continue
+			}
+			if f == nil {
 				continue
 			}
-			l.files[name] = file
+			file = f
+			if unavailable == nil {
+				// Only cache the result once every bundle consulted ahead of
+				// it, in preference order, was actually reachable. If a
+				// higher-preference bundle was unavailable, this answer may
+				// be shadowed by that bundle's own manifest once it
+				// recovers -- caching it here would let a transient outage
+				// permanently invert source precedence for the rest of this
+				// process's lifetime. Leaving it uncached means the next
+				// lookup re-tries the unavailable bundle from scratch, so it
+				// self-heals as soon as that bundle recovers.
+				l.files[name] = file
+			}
 			break
 		}
+		// Only report unavailability if the manifest was found nowhere else.
+		// Callers (Load) use this to distinguish "retry, this was
+		// inconclusive" from a genuine ErrUnknownPackage.
+		if file == nil && unavailable != nil {
+			return nil, unavailable
+		}
 	}
 	if file == nil {
-		return nil, errors.Wrap(ErrUnknownPackage, name)
+		return nil, errors.Wrap(ErrUnknownPackage, l.unknownPackageDetail(name))
 	}
 	if len(file.Errors) > 0 {
 		return nil, errors.WithStack(file.Errors[0])
 	}
 	return file, nil
 }
 
+// unknownPackageDetail enumerates the sources consulted when a package could
+// not be found in any of them. Without this, a permanently misconfigured or
+// inaccessible source (a bad "sources = [...]" entry, the wrong
+// HERMIT_STATE_DIR, or a permissions problem) masquerades as "unknown
+// package" for every package name, with no indication of why.
+func (l *Loader) unknownPackageDetail(name string) string {
+	return fmt.Sprintf("%s (searched %s)", name, strings.Join(l.sources.Sources(), ", "))
+}
+
 // Load a manifest for the given package.
 // Syncs the sources if the manifest is not initially found.
 // Will return a wrapped ErrUnknownPackage if the package could not be found.
@@ -91,15 +129,36 @@ func (l *Loader) get(name string) (*AnnotatedManifest, error) {
 func (l *Loader) Load(u *ui.UI, name string) (*AnnotatedManifest, error) {
 	mnf, err := l.get(name)
 	if err != nil {
-		err := l.sources.Sync(u, true)
-		if err != nil {
-			return nil, errors.Wrap(err, err.Error())
+		// Actively sync before falling back to sleeping through the bounded
+		// backoff below: a source that has never been cloned needs a real
+		// sync to ever become available, and sleeping first would add up to
+		// ~620ms of pure latency to every such cold start for no benefit --
+		// nothing changes the source's state on its own. This also covers
+		// the genuinely-unknown-package case, in case the source's cache is
+		// simply stale.
+		if syncErr := l.sources.Sync(u, true); syncErr != nil {
+			return nil, errors.WithStack(syncErr)
 		}
-		// Try again.
 		mnf, err = l.get(name)
-		if err != nil {
-			return nil, errors.WithStack(err)
+	}
+	// sourceUnavailableRetryBackoff bounds how long we'll additionally wait
+	// for a transiently-unavailable source (see sources.ErrSourceUnavailable,
+	// eg. another Hermit process mid-sync) to become available by itself --
+	// useful when our own Sync call above was a no-op (Sources.Sync skips
+	// sources once any one of them reports success) but a sibling process's
+	// concurrent sync of this specific source finishes in the meantime.
+	// Total worst case is ~620ms, deliberately short so a genuinely unknown
+	// package is never delayed by it.
+	sourceUnavailableRetryBackoff := []time.Duration{20 * time.Millisecond, 100 * time.Millisecond, 500 * time.Millisecond}
+	for _, backoff := range sourceUnavailableRetryBackoff {
+		if !errors.Is(err, sources.ErrSourceUnavailable) {
+			break
 		}
+		time.Sleep(backoff)
+		mnf, err = l.get(name)
+	}
+	if err != nil {
+		return nil, errors.WithStack(err)
 	}
 	return mnf, nil
 }
@@ -171,7 +230,14 @@ func (l *Loader) Glob(glob string) ([]*AnnotatedManifest, error) {
 			mu.Unlock()
 
 			wg.Go(func() error {
-				manifest := load(bundle, name, file)
+				manifest, err := load(bundle, name, file)
+				if err != nil {
+					// A transiently-unavailable source isn't fatal here:
+					// unlike Load, Glob/All are best-effort enumerations
+					// across every bundle, so just skip what this one
+					// bundle couldn't provide right now.
+					return nil //nolint:nilerr
+				}
 				if manifest != nil {
 					mftC <- result{manifest, name}
 				}
@@ -202,30 +268,37 @@ func (l *Loader) Errors() ManifestErrors {
 
 // Load manifest from bundle.
 //
-// Will return nil if it does not exist.
-func load(bundle fs.FS, name, filename string) *AnnotatedManifest {
+// Returns (nil, nil) if the manifest genuinely does not exist in this
+// bundle. Returns a non-nil error wrapping sources.ErrSourceUnavailable if
+// this bundle's backing source could not be read at all (eg. because
+// another process is mid-sync) -- callers should treat that as
+// "inconclusive", not "not found here".
+func load(bundle fs.FS, name, filename string) (*AnnotatedManifest, error) {
 	annotated := &AnnotatedManifest{
 		FS:   bundle,
 		Name: name,
 		Path: fmt.Sprintf("%s/%s", bundle, filename),
 	}
 	data, err := fs.ReadFile(bundle, filename)
-	if errors.Is(err, os.ErrNotExist) {
-		return nil
-	} else if err != nil {
+	switch {
+	case errors.Is(err, sources.ErrSourceUnavailable):
+		return nil, errors.WithStack(err)
+	case errors.Is(err, os.ErrNotExist):
+		return nil, nil
+	case err != nil:
 		annotated.Errors = append(annotated.Errors, errors.WithStack(err))
-		return annotated
+		return annotated, nil
 	}
 	manifest := &Manifest{}
 	err = hcl.Unmarshal(data, manifest)
 	if err != nil {
 		annotated.Errors = append(annotated.Errors, errors.WithStack(err))
-		return annotated
+		return annotated, nil
 	}
 	annotated.Manifest = manifest
 	annotated.Errors = append(annotated.Errors, annotated.validate()...)
 	synthesise(annotated)
-	return annotated
+	return annotated, nil
 }
 
 // LoadManifestFile Utility function to just load a manifest file.

manifest/loader_test.go

@@ -2,11 +2,15 @@ package manifest
 
 import (
 	"os"
+	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/alecthomas/assert/v2"
+	"github.com/cashapp/hermit/errors"
 	"github.com/cashapp/hermit/sources"
 	"github.com/cashapp/hermit/ui"
+	"github.com/cashapp/hermit/vfs"
 )
 
 func TestLoader(t *testing.T) {
@@ -28,3 +32,105 @@ func TestLoader(t *testing.T) {
 	assert.NotZero(t, loader.Errors()["test:///corrupt.hcl"])
 	assert.Equal(t, len(manifests), 2)
 }
+
+// noopRunner is a util.CommandRunner that never actually runs anything. It's
+// only used below to construct a GitSource whose backing directory is
+// deliberately never populated, so Sync is never expected to be called.
+type noopRunner struct{}
+
+func (noopRunner) RunInDir(_ *ui.Task, _ string, _ ...string) error { return nil }
+
+func TestLoaderMissingManifestIsUnknownPackage(t *testing.T) {
+	l, _ := ui.NewForTesting()
+	stateDir := t.TempDir()
+	srcs := sources.New(stateDir, []sources.Source{
+		sources.NewLocalSource("test://", os.DirFS("./testdata")),
+	})
+	loader := NewLoader(srcs)
+	_, err := loader.Load(l, "does-not-exist")
+	assert.Error(t, err)
+	assert.True(t, errors.Is(err, ErrUnknownPackage))
+	assert.False(t, errors.Is(err, sources.ErrSourceUnavailable))
+}
+
+// TestLoaderMissingSourceDirIsSourceUnavailable verifies that a GitSource
+// whose backing directory has never been created (eg. because another
+// Hermit process hasn't finished its initial sync yet) is reported as
+// ErrSourceUnavailable, not folded into "unknown package" -- the whole point
+// of the distinction is that Loader.Load treats the two differently.
+func TestLoaderMissingSourceDirIsSourceUnavailable(t *testing.T) {
+	stateDir := t.TempDir()
+	git := sources.NewGitSource("git://missing", filepath.Join(stateDir, "src"), noopRunner{})
+	srcs := sources.New(stateDir, []sources.Source{git})
+	loader := NewLoader(srcs)
+
+	_, err := loader.get("anything")
+	assert.Error(t, err)
+	assert.True(t, errors.Is(err, sources.ErrSourceUnavailable))
+}
+
+// TestLoaderFallsBackToHealthySourceWhenAnotherIsUnavailable verifies that
+// one unavailable source doesn't mask a package provided by another, healthy
+// source: get() must keep searching remaining bundles rather than bailing
+// out on the first unavailable one.
+func TestLoaderFallsBackToHealthySourceWhenAnotherIsUnavailable(t *testing.T) {
+	stateDir := t.TempDir()
+	missing := sources.NewGitSource("git://missing", filepath.Join(stateDir, "missing-src"), noopRunner{})
+	local := sources.NewLocalSource("test://", os.DirFS("./testdata"))
+	srcs := sources.New(stateDir, []sources.Source{missing, local})
+	loader := NewLoader(srcs)
+
+	manifest, err := loader.get("protoc")
+	assert.NoError(t, err)
+	assert.Equal(t, "protoc is a compiler for protocol buffers definitions files.", manifest.Description)
+}
+
+// fakeCloneRunner is a util.CommandRunner whose "clone" writes a manifest
+// into dest instead of shelling out to git, modelling a source that has
+// simply never been cloned yet (rather than one that's genuinely broken).
+type fakeCloneRunner struct{}
+
+func (fakeCloneRunner) RunInDir(_ *ui.Task, dir string, args ...string) error {
+	if len(args) >= 2 && args[0] == "git" && args[1] == "clone" {
+		return os.WriteFile(filepath.Join(dir, "foo.hcl"), []byte(`description = "hi"`), 0600)
+	}
+	return errors.Errorf("unexpected command: %v", args)
+}
+
+// TestLoaderSyncsBeforeSleepingThroughBackoff verifies that Load, on hitting
+// ErrSourceUnavailable, actively syncs the source before falling back to the
+// bounded sleep-based backoff -- sleeping first would never make a source
+// that has never been cloned appear, and would add its full ~620ms worst
+// case to every such cold start for nothing. This is a regression test for
+// that ordering: with the old (sleep-first) order, this would still
+// eventually succeed, just roughly 620ms slower.
+func TestLoaderSyncsBeforeSleepingThroughBackoff(t *testing.T) {
+	l, _ := ui.NewForTesting()
+	stateDir := t.TempDir()
+	git := sources.NewGitSource("git://not-cloned-yet", filepath.Join(stateDir, "src"), fakeCloneRunner{})
+	srcs := sources.New(stateDir, []sources.Source{git})
+	loader := NewLoader(srcs)
+
+	start := time.Now()
+	manifest, err := loader.Load(l, "foo")
+	elapsed := time.Since(start)
+
+	assert.NoError(t, err)
+	assert.Equal(t, "hi", manifest.Description)
+	assert.True(t, elapsed < 200*time.Millisecond, "Load took %s, expected a sync-first fast path, not the ~620ms backoff", elapsed)
+}
+
+// TestLoaderBuiltInSourceMissingManifestIsUnknownPackage guards against the
+// pitfall where an in-memory source (vfs.InMemoryFS, used by BuiltInSource
+// and MemSource) unconditionally returns fs.ErrNotExist with no backing
+// directory to probe: it must never be misreported as ErrSourceUnavailable.
+func TestLoaderBuiltInSourceMissingManifestIsUnknownPackage(t *testing.T) {
+	builtin := sources.NewBuiltInSource(vfs.InMemoryFS(map[string]string{}))
+	srcs := sources.New(t.TempDir(), []sources.Source{builtin})
+	loader := NewLoader(srcs)
+
+	_, err := loader.get("anything")
+	assert.Error(t, err)
+	assert.True(t, errors.Is(err, ErrUnknownPackage))
+	assert.False(t, errors.Is(err, sources.ErrSourceUnavailable))
+}

sources/builtin.go

@@ -17,13 +17,21 @@ func NewBuiltInSource(dir fs.FS) *BuiltInSource {
 }
 
 func (s *BuiltInSource) Sync(_ *ui.UI, _ bool) (bool, error) {
-	return true, nil
+	// This source performs no actual synchronisation, so "false" is the
+	// correct answer to "did I actually update anything?" -- returning
+	// "true" here poisons Sources.isSynchronised (sources.go), which is set
+	// if *any* source reports it synced, and since BuiltInSource is always
+	// prepended, that made every other source's "sync and retry" a no-op.
+	return false, nil
 }
 
 func (s *BuiltInSource) URI() string {
 	return "builtin:///"
 }
 
 func (s *BuiltInSource) Bundle() fs.FS {
-	return &uriFS{s.URI(), s.fs}
+	// dir is deliberately left empty: this source is backed by an in-memory
+	// FS with no directory on disk that could vanish out from under it (see
+	// the comment on uriFS.dir).
+	return &uriFS{uri: s.URI(), FS: s.fs}
 }