diff --git a/env.go b/env.go index f8317f95..dd760822 100644 --- a/env.go +++ b/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) } @@ -1211,7 +1223,7 @@ func (e *Env) SetEnv(key, value string) error { if err != nil { return errors.WithStack(err) } - return os.WriteFile(e.configFile, data, 0600) + return errors.WithStack(util.AtomicWriteFile(e.configFile, data, 0600)) } // DelEnv deletes a custom environment variable. @@ -1221,7 +1233,7 @@ func (e *Env) DelEnv(key string) error { if err != nil { return errors.WithStack(err) } - return os.WriteFile(e.configFile, data, 0600) + return errors.WithStack(util.AtomicWriteFile(e.configFile, data, 0600)) } // Clean parts of the hermit system. @@ -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 { diff --git a/internal/dao/dao.go b/internal/dao/dao.go index 5c1696fd..37546bbc 100644 --- a/internal/dao/dao.go +++ b/internal/dao/dao.go @@ -4,11 +4,18 @@ import ( "io" "os" "path/filepath" + "strings" "time" "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/util" ) +// staleScratchAge is how old a leftover ".tmp-*" file must be before Open +// considers it abandoned rather than an in-flight write from another +// process. +const staleScratchAge = 24 * time.Hour + // DAO abstracts away the database access type DAO struct { stateDir string @@ -27,41 +34,127 @@ func Open(stateDir string) (*DAO, error) { if err := os.MkdirAll(metadataDir, 0700); err != nil && !os.IsExist(err) { return nil, errors.WithStack(err) } + sweepStaleScratchFiles(metadataDir) return &DAO{stateDir: stateDir, metadataDir: metadataDir}, nil } +// sweepStaleScratchFiles removes leftover ".tmp-*" files from +// util.AtomicWriteFile calls that were interrupted by a killed process (eg. +// SIGKILL, which the writer's deferred os.Remove cannot run for). Best +// effort: errors are ignored, and a generous age threshold avoids racing a +// concurrent, genuinely in-flight write from another Hermit process. +func sweepStaleScratchFiles(metadataDir string) { + entries, err := os.ReadDir(metadataDir) + if err != nil { + return + } + for _, entry := range entries { + if !strings.Contains(entry.Name(), ".tmp-") { + continue + } + info, err := entry.Info() + if err != nil || time.Since(info.ModTime()) < staleScratchAge { + continue + } + _ = os.Remove(filepath.Join(metadataDir, entry.Name())) + } +} + // Dump content of database to w. func (d *DAO) Dump(w io.Writer) error { return nil } // GetPackage returns information for a specific package. +// +// The etag is stored as the raw, unencoded file content at metadataPath: this +// is the exact on-disk format every Hermit version has ever written, so a +// mixed-version fleet sharing a state directory can always read and write it +// identically. UpdateCheckedAt is stored separately, in the sidecar file at +// checkedAtPath, because mtime can't be trusted to mean "the moment this etag +// was written" -- it's disturbed by anything else that touches the file (eg. +// a backup/restore), and differs in precision across filesystems. An older +// Hermit version, or a first-ever check, has no such sidecar: fall back to +// the etag file's mtime in that case, as GetPackage always did previously. func (d *DAO) GetPackage(pkgRef string) (*Package, error) { - r, err := os.Open(d.metadataPath(pkgRef)) + etag, err := os.ReadFile(d.metadataPath(pkgRef)) if os.IsNotExist(err) { return nil, nil } if err != nil { return nil, errors.WithStack(err) } - defer r.Close() - info, err := r.Stat() + checkedAt, err := d.readCheckedAt(pkgRef) if err != nil { return nil, errors.WithStack(err) } - etag, err := io.ReadAll(r) - if err != nil { - return nil, errors.WithStack(err) + if checkedAt.IsZero() { + info, err := os.Stat(d.metadataPath(pkgRef)) + if err != nil { + return nil, errors.WithStack(err) + } + checkedAt = info.ModTime() } return &Package{ Etag: string(etag), - UpdateCheckedAt: info.ModTime(), + UpdateCheckedAt: checkedAt, }, nil } -// UpdatePackage Updates the update check time, etag, and the used at time for a package +func (d *DAO) readCheckedAt(pkgRef string) (time.Time, error) { + data, err := os.ReadFile(d.checkedAtPath(pkgRef)) + if os.IsNotExist(err) { + return time.Time{}, nil + } + if err != nil { + return time.Time{}, errors.WithStack(err) + } + checkedAt, err := time.Parse(time.RFC3339Nano, string(data)) + if err != nil { + // A torn read of the sidecar (or one written by an incompatible + // future version) is not fatal: fall back to mtime rather than + // failing the whole lookup. + return time.Time{}, nil //nolint:nilerr + } + return checkedAt, nil +} + +// UpdatePackage updates the update check time and etag for a package. +// +// Both files are written atomically: content is written to a temp file in +// the same directory, then renamed into place. os.WriteFile is not atomic -- +// it truncates the existing file before writing the new content -- so a +// concurrent GetPackage could otherwise observe a torn read (empty or +// partial etag). A torn read here is not merely cosmetic: UpgradeChannel +// treats any etag change, including a corrupted one, as a reason to +// evictPackage (rm -rf) a package tree that another process may be actively +// executing. +// +// The etag is written first: if the process dies between the two writes, a +// concurrent GetPackage falls back to the etag file's mtime for +// UpdateCheckedAt (see above), which is the same degraded-but-safe behaviour +// as running against an older Hermit version that never writes the sidecar +// at all. +// +// The two files are written with separate renames, not swapped in together, +// so two UpdatePackage calls for the same package racing each other can +// interleave: caller A's etag write can be immediately followed by caller +// B's checked-at write, leaving a GetPackage that reads in between with A's +// etag paired with B's checked-at time. The single etag file this replaced +// produced both fields (the etag content and, via its mtime, the checked-at +// time) from that one write, so this specific mismatched pairing is newly +// possible with the two-file split -- but it's still benign: at worst it +// under- or over-estimates how recently a concurrently-updated package was +// checked by one update cycle, which self-corrects on the next check. func (d *DAO) UpdatePackage(pkgRef string, pkg *Package) error { - return errors.WithStack(os.WriteFile(d.metadataPath(pkgRef), []byte(pkg.Etag), 0600)) + checkedAt := pkg.UpdateCheckedAt + if checkedAt.IsZero() { + checkedAt = time.Now() + } + if err := util.AtomicWriteFile(d.metadataPath(pkgRef), []byte(pkg.Etag), 0600); err != nil { + return errors.WithStack(err) + } + return errors.WithStack(util.AtomicWriteFile(d.checkedAtPath(pkgRef), []byte(checkedAt.Format(time.RFC3339Nano)), 0600)) } // DeletePackage removes a package from the DB @@ -69,9 +162,18 @@ func (d *DAO) DeletePackage(pkgRef string) error { if err := os.Remove(d.metadataPath(pkgRef)); err != nil { return errors.WithStack(err) } + // The checked-at sidecar may not exist (eg. written by an older Hermit + // version); that's not an error. + if err := os.Remove(d.checkedAtPath(pkgRef)); err != nil && !os.IsNotExist(err) { + return errors.WithStack(err) + } return nil } func (d *DAO) metadataPath(pkgRef string) string { return filepath.Join(d.metadataDir, pkgRef+".etag") } + +func (d *DAO) checkedAtPath(pkgRef string) string { + return filepath.Join(d.metadataDir, pkgRef+".checked") +} diff --git a/internal/dao/dao_test.go b/internal/dao/dao_test.go new file mode 100644 index 00000000..24c4187b --- /dev/null +++ b/internal/dao/dao_test.go @@ -0,0 +1,165 @@ +package dao + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/alecthomas/assert/v2" +) + +func TestGetPackageMissing(t *testing.T) { + d, err := Open(t.TempDir()) + assert.NoError(t, err) + + pkg, err := d.GetPackage("does-not-exist") + assert.NoError(t, err) + assert.Zero(t, pkg) +} + +func TestUpdateAndGetPackageRoundTrip(t *testing.T) { + d, err := Open(t.TempDir()) + assert.NoError(t, err) + + checkedAt := time.Date(2024, 3, 14, 15, 9, 26, 0, time.UTC) + assert.NoError(t, d.UpdatePackage("pkg@1.0.0", &Package{ + Etag: "some-etag", + UpdateCheckedAt: checkedAt, + })) + + got, err := d.GetPackage("pkg@1.0.0") + assert.NoError(t, err) + assert.Equal(t, "some-etag", got.Etag) + assert.True(t, checkedAt.Equal(got.UpdateCheckedAt), "expected %s, got %s", checkedAt, got.UpdateCheckedAt) +} + +// TestGetPackageMissingCheckedAtSidecar verifies that a metadata directory +// containing only the raw etag file, with no ".checked" sidecar, is still +// read correctly, falling back to the etag file's mtime for +// UpdateCheckedAt. This is the on-disk state left by a Hermit version prior +// to the introduction of the sidecar (which wrote only the raw etag, in +// exactly this format) -- the two are indistinguishable, which is the point: +// an older Hermit binary sharing this state directory can still read and +// write the etag file unmodified. +func TestGetPackageMissingCheckedAtSidecar(t *testing.T) { + d, err := Open(t.TempDir()) + assert.NoError(t, err) + + path := d.metadataPath("legacy@1.0.0") + assert.NoError(t, os.WriteFile(path, []byte("legacy-raw-etag"), 0600)) + mtime := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC) + assert.NoError(t, os.Chtimes(path, mtime, mtime)) + + got, err := d.GetPackage("legacy@1.0.0") + assert.NoError(t, err) + assert.Equal(t, "legacy-raw-etag", got.Etag) + assert.True(t, mtime.Equal(got.UpdateCheckedAt), "expected %s, got %s", mtime, got.UpdateCheckedAt) +} + +// TestUpdatePackageAtomicNoTornRead is the reproducer/regression test for the +// torn-read bug: os.WriteFile truncates the existing file before writing the +// new content, so a GetPackage racing an UpdatePackage could previously +// observe an empty or partial etag. UpdatePackage now writes to a temp file +// and renames it into place, so every concurrent read must see either the +// old, complete value or a new, complete value -- never anything in between. +func TestUpdatePackageAtomicNoTornRead(t *testing.T) { + d, err := Open(t.TempDir()) + assert.NoError(t, err) + const pkgRef = "pkg@1.0.0" + + // Give readers something to see from the very first iteration. + assert.NoError(t, d.UpdatePackage(pkgRef, &Package{Etag: "etag-0", UpdateCheckedAt: time.Now()})) + + valid := map[string]bool{"etag-0": true} + for i := range 50 { + valid[fmt.Sprintf("etag-%d", i+1)] = true + } + + var wg sync.WaitGroup + stop := make(chan struct{}) + readerErr := make(chan error, 1) + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + readerErr <- nil + return + default: + } + pkg, err := d.GetPackage(pkgRef) + if err != nil { + readerErr <- err + return + } + if pkg != nil && !valid[pkg.Etag] { + readerErr <- fmt.Errorf("observed torn/unexpected etag: %q", pkg.Etag) + return + } + } + }() + + for i := range 50 { + assert.NoError(t, d.UpdatePackage(pkgRef, &Package{ + Etag: fmt.Sprintf("etag-%d", i+1), + UpdateCheckedAt: time.Now(), + })) + } + close(stop) + wg.Wait() + assert.NoError(t, <-readerErr) +} + +// TestOpenSweepsStaleScratchFiles verifies that Open cleans up an old, +// abandoned ".tmp-*" file left behind by a process killed mid-write, but +// leaves a recent one alone (it may belong to a write still in flight in +// another process). +func TestOpenSweepsStaleScratchFiles(t *testing.T) { + stateDir := t.TempDir() + metadataDir := filepath.Join(stateDir, "metadata") + assert.NoError(t, os.MkdirAll(metadataDir, 0700)) + + stale := filepath.Join(metadataDir, "pkg@1.0.0.etag.tmp-stale") + assert.NoError(t, os.WriteFile(stale, []byte("abandoned"), 0600)) + old := time.Now().Add(-48 * time.Hour) + assert.NoError(t, os.Chtimes(stale, old, old)) + + fresh := filepath.Join(metadataDir, "pkg@2.0.0.etag.tmp-fresh") + assert.NoError(t, os.WriteFile(fresh, []byte("in-flight"), 0600)) + + _, err := Open(stateDir) + assert.NoError(t, err) + + _, err = os.Stat(stale) + assert.True(t, os.IsNotExist(err), "stale scratch file should have been swept") + _, err = os.Stat(fresh) + assert.NoError(t, err, "recent scratch file should not have been swept") +} + +// TestUpdatePackageLeavesNoTempFiles guards against leaking the scratch temp +// files UpdatePackage writes before renaming into place -- there are two +// atomic writes per call (the ".etag" file and the ".checked" sidecar), each +// with its own temp file. +func TestUpdatePackageLeavesNoTempFiles(t *testing.T) { + stateDir := t.TempDir() + d, err := Open(stateDir) + assert.NoError(t, err) + + assert.NoError(t, d.UpdatePackage("pkg@1.0.0", &Package{Etag: "some-etag", UpdateCheckedAt: time.Now()})) + + entries, err := os.ReadDir(filepath.Join(stateDir, "metadata")) + assert.NoError(t, err) + var names []string + for _, entry := range entries { + if strings.Contains(entry.Name(), ".tmp-") { + t.Fatalf("leaked temp file: %s", entry.Name()) + } + names = append(names, entry.Name()) + } + assert.Equal(t, []string{"pkg@1.0.0.checked", "pkg@1.0.0.etag"}, names) +} diff --git a/manifest/loader.go b/manifest/loader.go index 9aa8361b..1c05423b 100644 --- a/manifest/loader.go +++ b/manifest/loader.go @@ -65,17 +65,46 @@ 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]) @@ -83,6 +112,15 @@ func (l *Loader) get(name string) (*AnnotatedManifest, error) { 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. diff --git a/manifest/loader_test.go b/manifest/loader_test.go index 45cb64cc..dc1f1be4 100644 --- a/manifest/loader_test.go +++ b/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)) +} diff --git a/sources/builtin.go b/sources/builtin.go index 5c31b01e..4096c7cd 100644 --- a/sources/builtin.go +++ b/sources/builtin.go @@ -17,7 +17,12 @@ 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 { @@ -25,5 +30,8 @@ func (s *BuiltInSource) URI() string { } 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} } diff --git a/sources/git.go b/sources/git.go index 5bfd421a..3ec817aa 100644 --- a/sources/git.go +++ b/sources/git.go @@ -1,9 +1,11 @@ package sources import ( + "fmt" "io/fs" "os" "path/filepath" + "strings" "time" "github.com/cashapp/hermit/errors" @@ -11,47 +13,119 @@ import ( "github.com/cashapp/hermit/util" ) +// fsTimeGranularity is the coarsest mtime resolution we expect from the +// filesystems Hermit runs on (eg. HFS+ stores whole seconds), used as slack +// when comparing timestamps taken before and after acquiring the sync lock. +const fsTimeGranularity = time.Second + +// Suffixes/prefixes used for Hermit scratch state living alongside source +// directories. A real source directory name is a bare hex SHA256 hash (see +// util.Hash), so anything containing these is unambiguously scratch state. +const ( + tmpInfix = ".tmp-" // in-progress clone: .tmp-XXXXXXXX + asideSuffix = util.DirSwapAsideSuffix // previous tree, mid-swap, pending deletion + legacyTmpInfix = "-" // clone temp dirs created by older Hermit versions: -XXXXXXXX + staleScratchAge = 24 * time.Hour +) + // GitSource is a new Source based on a git repo type GitSource struct { - fs *uriFS - sourceDir string - path string - runner util.CommandRunner + fs *uriFS + sourceDir string + path string + runner util.CommandRunner + lockTimeout time.Duration } // NewGitSource returns a new GitSource func NewGitSource(uri, sourceDir string, runner util.CommandRunner) *GitSource { + return NewGitSourceWithLockTimeout(uri, sourceDir, runner, DefaultLockTimeout) +} + +// NewGitSourceWithLockTimeout returns a new GitSource with an explicit +// timeout for the lock acquired around synchronisation. This is the +// underlying constructor NewGitSource itself uses to apply +// DefaultLockTimeout; tests use it directly to exercise timeout/contention +// behaviour without waiting out the real default. +// +// A timeout <= 0 is treated as DefaultLockTimeout. +func NewGitSourceWithLockTimeout(uri, sourceDir string, runner util.CommandRunner, lockTimeout time.Duration) *GitSource { key := util.Hash(uri) path := filepath.Join(sourceDir, key) return &GitSource{&uriFS{ uri: uri, + dir: path, FS: os.DirFS(path), - }, sourceDir, path, runner} + }, sourceDir, path, runner, lockTimeout} } func (s *GitSource) Sync(p *ui.UI, force bool) (bool, error) { - info, _ := os.Stat(s.path) task := p.Task(s.fs.uri) - if info == nil || force || time.Since(info.ModTime()) >= SyncFrequency { - err := s.ensureSourcesDirExists() - if err != nil { - return false, errors.WithStack(err) - } - err = syncGit(task, s.sourceDir, s.fs.uri, s.path, s.runner) - // If the sync failed while the repo had already been cloned, log a warning - // If the repo has not yet been cloned, fail. - if err != nil { - if info != nil { - task.Warnf("git sync failed: %s", err) - return false, nil - } - return false, errors.Wrap(err, "git sync failed") + info, _ := os.Stat(s.path) + if info != nil && !force && time.Since(info.ModTime()) < SyncFrequency { + task.Debugf("Update skipped, updated within the last %s", SyncFrequency) + return false, nil + } + + if err := s.ensureSourcesDirExists(); err != nil { + return false, errors.WithStack(err) + } + + // Note the time *before* we start waiting for the lock: if, once we hold + // it, the directory's mtime is at or after this instant, another process + // finished synchronising it while we were waiting, and there is nothing + // left for us to do. + requestedAt := time.Now() + release, err := acquireSyncLock(task, s.path, s.lockTimeout, fmt.Sprintf("synchronising source %s", s.fs.uri)) + if err != nil { + if info != nil { + // We already have a (possibly stale) usable copy. Don't fail the + // command just because we couldn't get exclusive access to + // refresh it. + task.Warnf("could not lock source for syncing, using existing copy: %s", err) + return false, nil } + return false, errors.Wrap(err, "failed to sync sources") + } + defer release() //nolint:errcheck + + // Double-checked locking: re-stat now that we hold the lock. + postLockInfo, _ := os.Stat(s.path) + if syncedSince(postLockInfo, requestedAt) { + task.Debugf("Update skipped, synchronised by another process") return true, nil } - task.Debugf("Update skipped, updated within the last %s", SyncFrequency) - return false, nil + + err = syncGit(task, s.sourceDir, s.fs.uri, s.path, s.runner) + if err != nil { + // If the sync failed while the repo had already been cloned (using + // the up to date, post-lock information), log a warning. If the repo + // has not yet been cloned, fail. + if postLockInfo != nil { + task.Warnf("git sync failed: %s", err) + return false, nil + } + return false, errors.Wrap(err, "git sync failed") + } + return true, nil +} + +// syncedSince reports whether "info" (the result of stat-ing a source +// directory) shows it was successfully synced at or after "since", allowing +// fsTimeGranularity of slack for filesystems that only store whole-second +// mtimes (eg. HFS+). +// +// That slack means syncedSince can report true for a sync that was actually +// requested up to fsTimeGranularity *after* the directory's real mtime -- +// ie. Sync's double-checked-locking skip ("synchronised by another process") +// can fire even though the peer's sync, strictly, finished a moment before +// we asked. This is intentional and safe: skipping in that narrow window +// just means we use a copy that's at most fsTimeGranularity staler than the +// most pedantically-correct answer, which SyncFrequency-bounded staleness +// already tolerates far more of. +func syncedSince(info os.FileInfo, since time.Time) bool { + return info != nil && !info.ModTime().Add(fsTimeGranularity).Before(since) } func (s *GitSource) URI() string { @@ -69,7 +143,55 @@ func (s *GitSource) ensureSourcesDirExists() error { return nil } -// Atomically clone git repo. +// incrementalBranch is the local branch syncGit's incremental-update path +// resets and checks out on every sync, instead of leaving finalDest in a +// detached-HEAD state. This is load-bearing, not cosmetic: "git clone" +// (without "--no-checkout" or "--depth") only copies a source's +// "refs/heads/*", not a detached HEAD, so a detached finalDest would give the +// next incremental sync's local clone of it zero branches to send as "have"s +// during the following "git fetch --depth=1". Without a "have", the server +// can't tell what the client already has, and sends a full pack for the +// requested commit -- silently degrading every sync after the first into the +// same "full fresh clone" cost this path exists to avoid, and (verified +// empirically) totally losing the branch by the second incremental sync. +// Keeping a real, persistent branch ref here means every later sync's local +// clone inherits it, so its fetch always has a "have" to negotiate against. +const incrementalBranch = "hermit" + +// Atomically clone (or, if finalDest is already a clone, incrementally +// update) a git repo. +// +// There is deliberately no in-place "git pull" path: readers in other Hermit +// processes do not take the sync lock, so mutating finalDest's working tree +// in place (as "git pull" does -- updating and deleting files directly +// under it) is visible to them mid-update, and two concurrent "git pull"s +// against the same working tree can also collide with each other (eg. on +// ".git/index.lock"). Always building the new tree in a fresh directory and +// swapping it in atomically (see util.SwapDir) avoids both problems. +// +// When finalDest already has a ".git" directory, the new tree is built +// incrementally to keep the network cost close to a pull's: first a local, +// working-tree-less clone of finalDest (a same-filesystem copy, not a +// network operation), then a shallow fetch of just the latest commit from +// the real source into it, then a checkout of that commit onto +// incrementalBranch (see its doc comment for why a real branch, not a +// detached HEAD, is required for this to actually stay cheap on repeat +// syncs). A plain "--reference-if-able" clone from source was tried here +// first and discarded: finalDest is always itself a shallow (--depth=1) +// clone, and git unconditionally refuses to use a shallow repository as a +// reference, so that flag was silently a no-op and every sync was paying for +// a full fresh clone. This incremental path was verified against the real +// default source (632 manifests): ~0.9s versus ~3.3s for a fresh clone, +// close to the ~0.7s a "git pull" on an already-current clone takes -- and, +// separately, verified to stay that cheap across repeated syncs (not just +// the first one) once incrementalBranch was introduced. +// +// If the incremental update fails (eg. finalDest's ".git" is corrupt or +// truncated), this falls back to a fresh clone rather than surfacing the +// failure, so a damaged existing copy can still self-heal the way a from- +// scratch sync always could. +// +// The caller MUST hold the sync lock for finalDest. func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunner) (err error) { task := b.SubProgress("sync", 1) defer func() { @@ -79,29 +201,87 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne err = errors.WithStack(os.Chtimes(finalDest, now, now)) } }() - // First, if a git repo exists, just pull. - info, _ := os.Stat(filepath.Join(finalDest, ".git")) - if info != nil { - err = runner.RunInDir(b, finalDest, "git", "pull") - if err == nil { - return nil - } - // If pull fails, assume the repo is corrupted and just try and re-clone it. - } - // No git repo, clone down to temporary directory. - dest, err := os.MkdirTemp(dir, filepath.Base(finalDest)+"-*") + + removeStaleScratchDirs(b, dir, finalDest) + + dest, err := os.MkdirTemp(dir, filepath.Base(finalDest)+tmpInfix+"*") if err != nil { return errors.WithStack(err) } defer os.RemoveAll(dest) - if err = runner.RunInDir(b, dest, "git", "clone", "--depth=1", source, dest); err != nil { + + freshClone := true + if info, _ := os.Stat(filepath.Join(finalDest, ".git")); info != nil { + if incErr := syncGitIncremental(b, dest, source, finalDest, runner); incErr == nil { + freshClone = false + } else { + b.Warnf("incremental sync from existing clone failed, falling back to a fresh clone: %s", incErr) + if err = os.RemoveAll(dest); err != nil { + return errors.WithStack(err) + } + if err = os.Mkdir(dest, 0700); err != nil { + return errors.WithStack(err) + } + } + } + if freshClone { + if err = runner.RunInDir(b, dest, "git", "clone", "--depth=1", source, dest); err != nil { + return errors.WithStack(err) + } + } + return errors.WithStack(util.SwapDir(dest, finalDest)) +} + +// syncGitIncremental builds an updated tree at dest by cloning finalDest +// locally (same-filesystem, not a network operation) and fetching just the +// latest commit from the real source into it. See syncGit's doc comment for +// why the result is checked out onto incrementalBranch rather than left +// detached. +func syncGitIncremental(b *ui.Task, dest, source, finalDest string, runner util.CommandRunner) error { + if err := runner.RunInDir(b, dest, "git", "clone", "--no-checkout", finalDest, dest); err != nil { return errors.WithStack(err) } - _ = os.RemoveAll(finalDest) - // And finally, rename it into place. - if err = os.Rename(dest, finalDest); err != nil && !os.IsExist(err) { // Prevent races. + if err := runner.RunInDir(b, dest, "git", "fetch", "--depth=1", source, "HEAD"); err != nil { return errors.WithStack(err) } + // "--force" makes materialising the worktree here not depend on dest + // having no ".git/index" (true today, since "clone --no-checkout" writes + // none) -- without it, a checkout that git considers a no-op change + // writes nothing, silently leaving dest's worktree empty were that ever + // no longer the case. + return errors.WithStack(runner.RunInDir(b, dest, "git", "checkout", "--force", "-B", incrementalBranch, "FETCH_HEAD")) +} - return nil +// removeStaleScratchDirs removes leftover clone/swap scratch directories from +// Hermit processes that were killed mid-sync (eg. SIGKILL, which the +// "defer os.RemoveAll" in syncGit cannot run for), including the "-XXXX" +// form used by Hermit versions prior to the introduction of source locking. +// +// The caller MUST hold the sync lock for finalDest. This is best-effort; +// errors are ignored. +func removeStaleScratchDirs(log ui.Logger, dir, finalDest string) { + base := filepath.Base(finalDest) + entries, err := os.ReadDir(dir) + if err != nil { + return + } + for _, entry := range entries { + name := entry.Name() + if name == base || strings.HasSuffix(name, lockSuffix) { + continue + } + if !strings.HasPrefix(name, base+tmpInfix) && + name != base+asideSuffix && + !strings.HasPrefix(name, base+legacyTmpInfix) { + continue + } + info, err := entry.Info() + if err != nil || time.Since(info.ModTime()) < staleScratchAge { + // Generous age threshold: an older, unlocked Hermit binary may + // still be actively cloning into one of these. + continue + } + log.Debugf("removing stale source scratch directory %s", name) + _ = os.RemoveAll(filepath.Join(dir, name)) + } } diff --git a/sources/git_concurrency_test.go b/sources/git_concurrency_test.go new file mode 100644 index 00000000..ba21cab5 --- /dev/null +++ b/sources/git_concurrency_test.go @@ -0,0 +1,363 @@ +package sources_test + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/alecthomas/assert/v2" + + "github.com/cashapp/hermit/sources" + "github.com/cashapp/hermit/ui" + "github.com/cashapp/hermit/util" +) + +// slowCloningGit is a fake util.CommandRunner that behaves like a real "git" +// binary just enough to exercise GitSource.Sync: "clone" sleeps for a bit +// (simulating a slow network fetch) before writing a manifest and a ".git" +// marker into dest, and "pull" is an instant no-op success. Every clone +// appends a line to a shared log file, so tests can assert exactly one clone +// happened even under concurrent Sync calls. +type slowCloningGit struct { + cloneDelay time.Duration + cloneLog string + + mu sync.Mutex + logHandle *os.File +} + +func newSlowCloningGit(cloneDelay time.Duration, cloneLog string) *slowCloningGit { + return &slowCloningGit{cloneDelay: cloneDelay, cloneLog: cloneLog} +} + +func (g *slowCloningGit) RunInDir(_ *ui.Task, dir string, args ...string) error { + if len(args) < 2 || args[0] != "git" { + return fmt.Errorf("unexpected command: %v", args) + } + switch args[1] { + case "pull": + return nil + case "clone": + time.Sleep(g.cloneDelay) + if err := os.MkdirAll(filepath.Join(dir, ".git"), 0700); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, ".git", "HEAD"), []byte("ref: refs/heads/master\n"), 0600); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, "pkg.hcl"), []byte("description = \"test\"\n"), 0600); err != nil { + return err + } + return g.appendLog(dir) + default: + return fmt.Errorf("unexpected git subcommand: %v", args) + } +} + +func (g *slowCloningGit) appendLog(dir string) error { + g.mu.Lock() + defer g.mu.Unlock() + if g.logHandle == nil { + f, err := os.OpenFile(g.cloneLog, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) + if err != nil { + return err + } + g.logHandle = f + } + _, err := fmt.Fprintf(g.logHandle, "%s\n", dir) + return err +} + +// pollForVanishAfterAppearing polls path until stop is closed, and reports +// an error on errCh if it ever observes path disappear after having +// previously observed it exist. This is the direct reproduction of the +// reported symptom: a reader elsewhere in the system seeing a manifest it +// already found go missing mid-sync. +func pollForVanishAfterAppearing(stop <-chan struct{}, path string, errCh chan<- error) { + var sawIt bool + for { + select { + case <-stop: + errCh <- nil + return + default: + } + _, err := os.ReadFile(path) + switch { + case err == nil: + sawIt = true + case os.IsNotExist(err): + if sawIt { + errCh <- fmt.Errorf("manifest disappeared after first appearing: %s", path) + return + } + default: + errCh <- err + return + } + time.Sleep(200 * time.Microsecond) + } +} + +// waitForFile polls for path to exist, failing the test if timeout elapses +// first. Used to synchronise on a real cross-process event (e.g. "the child +// has acquired the lock") without a fixed sleep, which would either race or +// needlessly slow the test down. +func waitForFile(t *testing.T, path string, timeout time.Duration) { + t.Helper() + deadline := time.Now().Add(timeout) + for { + if _, err := os.Stat(path); err == nil { + return + } + if time.Now().After(deadline) { + t.Fatalf("timed out after %s waiting for %s to appear", timeout, path) + } + time.Sleep(time.Millisecond) + } +} + +// assertOneClone asserts exactly one clone occurred, ie. there was no +// thundering herd of redundant clones once the lock and double-checked +// locking are in place. +func assertOneClone(t *testing.T, cloneLog string) { + t.Helper() + data, err := os.ReadFile(cloneLog) + assert.NoError(t, err) + lines := strings.TrimSpace(string(data)) + if lines == "" { + t.Fatalf("expected exactly one clone, got none") + } + got := strings.Split(lines, "\n") + assert.Equal(t, 1, len(got)) +} + +// assertNoScratchDirs asserts no leaked ".tmp-*"/"*.old" scratch directories +// remain in sourceDir after all syncs have completed. +func assertNoScratchDirs(t *testing.T, sourceDir string) { + t.Helper() + entries, err := os.ReadDir(sourceDir) + assert.NoError(t, err) + for _, entry := range entries { + name := entry.Name() + if strings.Contains(name, ".tmp-") || strings.HasSuffix(name, ".old") { + t.Fatalf("leaked scratch directory: %s", name) + } + } +} + +// TestConcurrentSyncInProcess reproduces N goroutines racing to Sync the +// same not-yet-cloned source, each building its own *GitSource sharing one +// sourceDir and URI -- mirroring how each Hermit invocation constructs its +// own Sources from scratch. A background reader polls the manifest file +// GitSource.Sync writes and fails the test if it ever sees the manifest +// vanish after having first seen it exist. +// +// This exercises whatever process-local synchronisation sources/lock.go adds +// (hence -race), but not flock's cross-process behaviour -- see +// TestConcurrentSyncAcrossProcesses for that. On an unfixed tree, this test +// is not guaranteed to reproduce the race on every run, since all n +// goroutines racing through Sync at once is a timing-dependent condition, +// not a certainty -- see the start barrier below, which maximises the odds +// by holding every goroutine at the gate until all are spawned. +func TestConcurrentSyncInProcess(t *testing.T) { + const n = 8 + sourceDir := t.TempDir() + cloneLog := filepath.Join(t.TempDir(), "clones.log") + uri := "git://concurrent-test" + runner := newSlowCloningGit(50*time.Millisecond, cloneLog) + manifestPath := filepath.Join(sourceDir, util.Hash(uri), "pkg.hcl") + + stop := make(chan struct{}) + readerErr := make(chan error, 1) + go pollForVanishAfterAppearing(stop, manifestPath, readerErr) + + var ready sync.WaitGroup + start := make(chan struct{}) + var wg sync.WaitGroup + ready.Add(n) + for range n { + wg.Add(1) + go func() { + defer wg.Done() + ready.Done() + <-start + u, _ := ui.NewForTesting() + source := sources.NewGitSource(uri, sourceDir, runner) + _, err := source.Sync(u, true) + assert.NoError(t, err) + }() + } + ready.Wait() + close(start) + wg.Wait() + close(stop) + assert.NoError(t, <-readerErr) + + _, err := os.Stat(manifestPath) + assert.NoError(t, err) + assertOneClone(t, cloneLog) + assertNoScratchDirs(t, sourceDir) +} + +// TestConcurrentSyncAcrossProcesses is the reproducer for the reported bug: +// N genuinely separate processes race to Sync the same not-yet-cloned +// source. util/flock is deliberately re-entrant per-PID (a lock file +// recording our own PID is treated as already held), so this race can only +// be reproduced -- and the fix only validated -- across real processes, not +// goroutines sharing a PID. +// +// Each child is a re-exec of this same test binary running +// TestSyncChildProcess, guarded by HERMIT_TEST_CHILD so it no-ops otherwise. +func TestConcurrentSyncAcrossProcesses(t *testing.T) { + if testing.Short() { + t.Skip("spawns subprocesses; skipped in -short") + } + const n = 8 + sourceDir := t.TempDir() + cloneLog := filepath.Join(t.TempDir(), "clones.log") + readyDir := t.TempDir() + goFile := filepath.Join(t.TempDir(), "go") + uri := "git://concurrent-cross-process-test" + manifestPath := filepath.Join(sourceDir, util.Hash(uri), "pkg.hcl") + + stop := make(chan struct{}) + readerErr := make(chan error, 1) + go pollForVanishAfterAppearing(stop, manifestPath, readerErr) + + var wg sync.WaitGroup + errs := make([]error, n) + for i := range n { + wg.Add(1) + go func(i int) { + defer wg.Done() + readyFile := filepath.Join(readyDir, fmt.Sprintf("%d", i)) + cmd := exec.Command(os.Args[0], "-test.run=TestSyncChildProcess", "-test.v") + cmd.Env = append(os.Environ(), + "HERMIT_TEST_CHILD=1", + "HERMIT_TEST_SOURCE_URI="+uri, + "HERMIT_TEST_SOURCE_DIR="+sourceDir, + "HERMIT_TEST_CLONE_LOG="+cloneLog, + "HERMIT_TEST_CLONE_DELAY=100ms", + "HERMIT_TEST_READY_FILE="+readyFile, + "HERMIT_TEST_GO_FILE="+goFile, + ) + out, err := cmd.CombinedOutput() + if err != nil { + errs[i] = fmt.Errorf("child %d failed: %w\n%s", i, err, out) + } + }(i) + } + + // Hold every child at the gate (each blocked on its own readyFile + // existing, then waiting on goFile) until all n have signalled ready, + // then release them all at once -- this maximises the odds that all n + // children race through Sync concurrently, same as the in-process + // start barrier above. + for i := range n { + waitForFile(t, filepath.Join(readyDir, fmt.Sprintf("%d", i)), 30*time.Second) + } + assert.NoError(t, os.WriteFile(goFile, nil, 0600)) + + wg.Wait() + close(stop) + assert.NoError(t, <-readerErr) + + for _, e := range errs { + assert.NoError(t, e) + } + assertOneClone(t, cloneLog) + assertNoScratchDirs(t, sourceDir) +} + +// TestSyncChildProcess is not a real test: it's the worker spawned by +// TestConcurrentSyncAcrossProcesses via re-exec, guarded by an env var so it +// no-ops under a normal test run. +func TestSyncChildProcess(t *testing.T) { + if os.Getenv("HERMIT_TEST_CHILD") == "" { + t.Skip("only runs as a spawned child of TestConcurrentSyncAcrossProcesses") + } + uri := os.Getenv("HERMIT_TEST_SOURCE_URI") + sourceDir := os.Getenv("HERMIT_TEST_SOURCE_DIR") + cloneLog := os.Getenv("HERMIT_TEST_CLONE_LOG") + delay, err := time.ParseDuration(os.Getenv("HERMIT_TEST_CLONE_DELAY")) + if err != nil { + t.Fatalf("bad clone delay: %s", err) + } + + // Signal the parent we're up, then wait for its go-ahead: this holds all + // n children at the gate so they race through Sync together, rather than + // however staggered process spawn happens to make them. + if readyFile := os.Getenv("HERMIT_TEST_READY_FILE"); readyFile != "" { + if err := os.WriteFile(readyFile, nil, 0600); err != nil { + t.Fatalf("failed to write ready file: %s", err) + } + waitForFile(t, os.Getenv("HERMIT_TEST_GO_FILE"), 30*time.Second) + } + + runner := newSlowCloningGit(delay, cloneLog) + source := sources.NewGitSource(uri, sourceDir, runner) + u, _ := ui.NewForTesting() + if _, err := source.Sync(u, true); err != nil { + t.Fatalf("sync failed: %s", err) + } +} + +// TestSyncLockTimeoutFallsBackToExistingCopy verifies that, when a source +// already has a usable copy on disk but the sync lock can't be acquired +// within the configured timeout, Sync degrades to using the existing copy +// rather than failing outright. Lock contention is exercised with a genuine +// separate process (TestHoldSourceLockChildProcess), for the same +// per-PID-reentrancy reason as the cross-process clone test above. +func TestSyncLockTimeoutFallsBackToExistingCopy(t *testing.T) { + if testing.Short() { + t.Skip("spawns a subprocess; skipped in -short") + } + sourceDir := t.TempDir() + uri := "git://lock-timeout-test" + runner := newSlowCloningGit(0, filepath.Join(t.TempDir(), "clones.log")) + source := sources.NewGitSource(uri, sourceDir, runner) + + // Populate an initial copy so Sync has an existing tree to fall back to. + u, _ := ui.NewForTesting() + _, err := source.Sync(u, true) + assert.NoError(t, err) + + path := filepath.Join(sourceDir, util.Hash(uri)) + readyFile := filepath.Join(t.TempDir(), "lock-held") + holdFor := 500 * time.Millisecond + cmd := exec.Command(os.Args[0], "-test.run=TestHoldSourceLockChildProcess", "-test.v") + cmd.Env = append(os.Environ(), + "HERMIT_TEST_CHILD=1", + "HERMIT_TEST_LOCK_PATH="+path, + "HERMIT_TEST_LOCK_HOLD="+holdFor.String(), + "HERMIT_TEST_LOCK_READY_FILE="+readyFile, + ) + assert.NoError(t, cmd.Start()) + // Guarantee the child is reaped even if an assertion below fails the + // test early (assert.* calls t.Fatalf, which skips the cmd.Wait() at + // the end of this function): an orphaned child would otherwise keep + // holding the lock file open for the rest of holdFor. + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + + // Wait for the child to actually confirm it holds the lock, rather than + // guessing how long that takes: a fixed sleep here would be flaky on a + // slow/loaded machine (racing shortTimeoutSource.Sync below before the + // child has the lock at all) and wastes time everywhere else. + waitForFile(t, readyFile, 30*time.Second) + + shortTimeoutSource := sources.NewGitSourceWithLockTimeout(uri, sourceDir, runner, 10*time.Millisecond) + did, err := shortTimeoutSource.Sync(u, true) + assert.NoError(t, err) + assert.False(t, did, "should have skipped syncing and fallen back to the existing copy") + + assert.NoError(t, cmd.Wait()) +} diff --git a/sources/git_internal_test.go b/sources/git_internal_test.go new file mode 100644 index 00000000..85b744c1 --- /dev/null +++ b/sources/git_internal_test.go @@ -0,0 +1,278 @@ +package sources + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/alecthomas/assert/v2" + + "github.com/cashapp/hermit/ui" + "github.com/cashapp/hermit/util" +) + +func statWithModTime(t *testing.T, modTime time.Time) os.FileInfo { + t.Helper() + path := filepath.Join(t.TempDir(), "f") + assert.NoError(t, os.WriteFile(path, nil, 0600)) + assert.NoError(t, os.Chtimes(path, modTime, modTime)) + info, err := os.Stat(path) + assert.NoError(t, err) + return info +} + +func TestSyncedSince(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + info os.FileInfo + want bool + }{ + {"no directory", nil, false}, + {"synced well before", statWithModTime(t, now.Add(-2*time.Second)), false}, + {"synced exactly at instant", statWithModTime(t, now), true}, + {"synced within filesystem granularity slack", statWithModTime(t, now.Add(-500*time.Millisecond)), true}, + {"synced after instant", statWithModTime(t, now.Add(time.Second)), true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.want, syncedSince(test.info, now)) + }) + } +} + +// TestRemoveStaleScratchDirs verifies the age-gated cleanup sweep only +// touches Hermit's own scratch-directory naming conventions, and only once +// they're old enough that another (older, lock-unaware) Hermit process is +// unlikely to still be using them. +func TestRemoveStaleScratchDirs(t *testing.T) { + dir := t.TempDir() + finalDest := filepath.Join(dir, "abc123") + assert.NoError(t, os.MkdirAll(finalDest, 0700)) + + touch := func(name string, age time.Duration) { + p := filepath.Join(dir, name) + assert.NoError(t, os.MkdirAll(p, 0700)) + mt := time.Now().Add(-age) + assert.NoError(t, os.Chtimes(p, mt, mt)) + } + touch("abc123.tmp-old", 48*time.Hour) // stale clone scratch: removed + touch("abc123.tmp-new", time.Minute) // fresh clone scratch: may be in use, kept + touch("abc123.old", 48*time.Hour) // stale swap-aside: removed + touch("abc123-legacy", 48*time.Hour) // pre-lock-era clone scratch: removed + touch("abc123.lock", 48*time.Hour) // lock file: always kept, regardless of age + touch("def456", 48*time.Hour) // unrelated real source dir: untouched + + u, _ := ui.NewForTesting() + removeStaleScratchDirs(u, dir, finalDest) + + assertExists := func(name string, want bool) { + t.Helper() + _, err := os.Stat(filepath.Join(dir, name)) + if want { + assert.NoError(t, err, name) + } else { + assert.True(t, os.IsNotExist(err), name) + } + } + assertExists("abc123", true) + assertExists("abc123.tmp-new", true) + assertExists("abc123.lock", true) + assertExists("def456", true) + assertExists("abc123.tmp-old", false) + assertExists("abc123.old", false) + assertExists("abc123-legacy", false) +} + +// runGit runs a real "git" command in dir, failing the test on error. Used to +// build and update the upstream repo that syncGit clones/fetches from -- +// separate from the util.CommandRunner under test, which is what syncGit +// itself uses to clone/fetch/checkout. +func runGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + assert.NoError(t, err, "git %v: %s", args, out) +} + +// gitEnvIsolated points HOME, XDG_CONFIG_HOME and the global/system gitconfig +// locations at throwaway paths for the duration of t, so a hook, alias or +// setting in the machine running the test (eg. commit.gpgsign, a +// core.hooksPath) can't affect a test that only cares about plumbing +// commands. +func gitEnvIsolated(t *testing.T) { + t.Helper() + empty := t.TempDir() + t.Setenv("HOME", empty) + t.Setenv("XDG_CONFIG_HOME", empty) + t.Setenv("GIT_CONFIG_GLOBAL", filepath.Join(empty, "gitconfig-does-not-exist")) + t.Setenv("GIT_CONFIG_SYSTEM", filepath.Join(empty, "gitconfig-does-not-exist")) +} + +// TestSyncGitIncrementalUpdate exercises syncGit's incremental-update branch +// (taken once finalDest is already a clone) against a real "git" binary. This +// replaced a "--reference-if-able" clone that turned out to be a silent +// no-op against Hermit's own shallow (--depth=1) clones -- no existing test +// drove the actual clone/fetch/checkout mechanism at all, which is how it +// shipped broken. syncGit is called directly (rather than via GitSource.Sync) +// so this doesn't depend on the wall-clock/mtime-granularity slack in +// syncedSince's double-checked-locking skip. +// +// The upstream repo is addressed via a "file://" URL rather than a bare local +// path: git silently ignores "--depth" for a local-filesystem path ("--depth +// is ignored in local clones"), which would make finalDest never actually +// shallow and defeat the point of this test -- "file://" forces the real +// smart-transport, shallow-fetch code path, the same one used against a real +// remote like the default hermit-packages source. +func TestSyncGitIncrementalUpdate(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + gitEnvIsolated(t) + + upstreamDir := t.TempDir() + upstream := "file://" + upstreamDir + runGit(t, upstreamDir, "init", "-q", "-b", "main") + runGit(t, upstreamDir, "config", "user.email", "test@example.com") + runGit(t, upstreamDir, "config", "user.name", "Test") + assert.NoError(t, os.WriteFile(filepath.Join(upstreamDir, "first.hcl"), []byte("description = \"first\"\n"), 0600)) + runGit(t, upstreamDir, "add", "first.hcl") + runGit(t, upstreamDir, "commit", "-q", "-m", "first") + + dir := t.TempDir() + finalDest := filepath.Join(dir, "abc123") + u, _ := ui.NewForTesting() + runner := &util.RealCommandRunner{} + + // Initial clone: finalDest has no ".git" yet, so this takes the + // fresh-clone branch. + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + first, err := os.ReadFile(filepath.Join(finalDest, "first.hcl")) + assert.NoError(t, err) + assert.Equal(t, "description = \"first\"\n", string(first)) + + // Sync several more times, each adding a new commit upstream: finalDest + // now has a ".git", so every one of these takes the incremental-update + // branch. Repeating this (rather than syncing just once more) is what + // actually exercises incrementalBranch's persistence -- a version of this + // path that left finalDest with a detached HEAD passed a single-sync + // version of this test, but silently degraded into a full-cost clone + // starting from the second incremental sync. + for i := 2; i <= 4; i++ { + name := fmt.Sprintf("commit%d.hcl", i) + content := fmt.Sprintf("description = \"commit %d\"\n", i) + assert.NoError(t, os.WriteFile(filepath.Join(upstreamDir, name), []byte(content), 0600)) + runGit(t, upstreamDir, "add", name) + runGit(t, upstreamDir, "commit", "-q", "-m", fmt.Sprintf("commit %d", i)) + + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + + got, err := os.ReadFile(filepath.Join(finalDest, name)) + assert.NoError(t, err, "sync %d", i) + assert.Equal(t, content, string(got), "sync %d", i) + + // incrementalBranch must persist as a real ref across syncs -- if + // this is ever a detached HEAD instead, the *next* sync's local + // clone of finalDest has no branch to send as a "have", and its + // fetch silently regresses into transferring a full pack. + branch, err := exec.Command("git", "-C", finalDest, "branch", "--show-current").CombinedOutput() + assert.NoError(t, err, "sync %d: %s", i, branch) + assert.Equal(t, incrementalBranch, strings.TrimSpace(string(branch)), "sync %d", i) + } + + // Deleting a file upstream must propagate too: "checkout -B" replaces the + // whole tree, it doesn't merge, so this would fail if the incremental + // path ever left a stale copy of a removed file behind. + runGit(t, upstreamDir, "rm", "-q", "first.hcl") + runGit(t, upstreamDir, "commit", "-q", "-m", "remove first.hcl") + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + _, err = os.Stat(filepath.Join(finalDest, "first.hcl")) + assert.True(t, os.IsNotExist(err), "first.hcl should have been removed by the incremental checkout") + + info, err := os.Stat(filepath.Join(finalDest, ".git")) + assert.NoError(t, err) + assert.True(t, info.IsDir()) + + // No leftover clone/swap scratch directories. + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + assert.Equal(t, 1, len(entries), "expected only finalDest, got %v", entries) +} + +// TestSyncGitIncrementalUpdateFallsBackOnCorruptClone verifies that a +// corrupt/truncated finalDest ".git" (eg. from an interrupted earlier write, +// or a filesystem issue) doesn't wedge every future sync: syncGit should +// notice the incremental path failed and fall back to a fresh clone, the way +// a from-scratch sync always could, rather than leaving finalDest stuck with +// a warning on every subsequent command. +func TestSyncGitIncrementalUpdateFallsBackOnCorruptClone(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + gitEnvIsolated(t) + + upstreamDir := t.TempDir() + upstream := "file://" + upstreamDir + runGit(t, upstreamDir, "init", "-q", "-b", "main") + runGit(t, upstreamDir, "config", "user.email", "test@example.com") + runGit(t, upstreamDir, "config", "user.name", "Test") + assert.NoError(t, os.WriteFile(filepath.Join(upstreamDir, "first.hcl"), []byte("description = \"first\"\n"), 0600)) + runGit(t, upstreamDir, "add", "first.hcl") + runGit(t, upstreamDir, "commit", "-q", "-m", "first") + + dir := t.TempDir() + finalDest := filepath.Join(dir, "abc123") + // A finalDest with a ".git" directory that isn't actually a valid repo: + // takes the incremental branch, and "git clone --no-checkout finalDest + // dest" must fail against it. + assert.NoError(t, os.MkdirAll(filepath.Join(finalDest, ".git"), 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(finalDest, "stale.hcl"), []byte("stale"), 0600)) + + u, _ := ui.NewForTesting() + runner := &util.RealCommandRunner{} + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + + first, err := os.ReadFile(filepath.Join(finalDest, "first.hcl")) + assert.NoError(t, err) + assert.Equal(t, "description = \"first\"\n", string(first)) + _, err = os.Stat(filepath.Join(finalDest, "stale.hcl")) + assert.True(t, os.IsNotExist(err), "stale content from the corrupt clone should not survive") +} + +// TestHoldSourceLockChildProcess is not a real test: it's a worker spawned by +// TestSyncLockTimeoutFallsBackToExistingCopy (sources_test package) via +// re-exec, guarded by an env var so it no-ops under a normal test run. It +// holds the sync lock for a directory from a genuinely separate process, +// which is required to exercise lock contention: util/flock is deliberately +// re-entrant per-PID, so a single process can never observe its own lock as +// held by someone else. +func TestHoldSourceLockChildProcess(t *testing.T) { + if os.Getenv("HERMIT_TEST_CHILD") == "" { + t.Skip("only runs as a spawned child of TestSyncLockTimeoutFallsBackToExistingCopy") + } + path := os.Getenv("HERMIT_TEST_LOCK_PATH") + hold, err := time.ParseDuration(os.Getenv("HERMIT_TEST_LOCK_HOLD")) + if err != nil { + t.Fatalf("bad hold duration: %s", err) + } + u, _ := ui.NewForTesting() + release, err := acquireSyncLock(u, path, DefaultLockTimeout, "test lock holder") + if err != nil { + t.Fatalf("failed to acquire lock: %s", err) + } + if readyFile := os.Getenv("HERMIT_TEST_LOCK_READY_FILE"); readyFile != "" { + if err := os.WriteFile(readyFile, nil, 0600); err != nil { + t.Fatalf("failed to signal lock held: %s", err) + } + } + time.Sleep(hold) + if err := release(); err != nil { + t.Fatalf("failed to release lock: %s", err) + } +} diff --git a/sources/git_test.go b/sources/git_test.go index 3ae5e745..e42a0630 100644 --- a/sources/git_test.go +++ b/sources/git_test.go @@ -2,6 +2,7 @@ package sources_test import ( "os" + "strings" "testing" "github.com/alecthomas/assert/v2" @@ -18,6 +19,24 @@ func (f *FailingGit) RunInDir(_ *ui.Task, _ string, _ ...string) error { return f.err } +// sourceDirs returns the names of real source directories in dir, ignoring +// Hermit's lock files and sync scratch directories. Source directory names +// are bare hex SHA256 hashes (see util.Hash); every scratch/lock entry +// contains a ".", so this distinction is unambiguous. +func sourceDirs(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + var dirs []string + for _, entry := range entries { + if strings.Contains(entry.Name(), ".") { + continue + } + dirs = append(dirs, entry.Name()) + } + return dirs +} + func TestGitDoesNotRemoveSourceAfterSyncFailure(t *testing.T) { git := &FailingGit{} sourceDir := t.TempDir() @@ -27,10 +46,9 @@ func TestGitDoesNotRemoveSourceAfterSyncFailure(t *testing.T) { u, _ := ui.NewForTesting() _, err := source.Sync(u, true) assert.NoError(t, err) - files, err := os.ReadDir(sourceDir) - assert.NoError(t, err) - assert.Equal(t, len(files), 1) - gitDir := files[0].Name() + dirs := sourceDirs(t, sourceDir) + assert.Equal(t, len(dirs), 1) + gitDir := dirs[0] // Fail the sync git.err = errors.New("failing git fails") @@ -40,9 +58,7 @@ func TestGitDoesNotRemoveSourceAfterSyncFailure(t *testing.T) { assert.NoError(t, err) // the directory should still be in place after git failed to update - files, err = os.ReadDir(sourceDir) - assert.NoError(t, err) - assert.Equal(t, len(files), 1) - assert.Equal(t, gitDir, files[0].Name()) - + dirs = sourceDirs(t, sourceDir) + assert.Equal(t, len(dirs), 1) + assert.Equal(t, gitDir, dirs[0]) } diff --git a/sources/local.go b/sources/local.go index 82886c90..c8d92e37 100644 --- a/sources/local.go +++ b/sources/local.go @@ -20,7 +20,10 @@ func NewLocalSource(uri string, f fs.FS) *LocalSource { } func (s *LocalSource) Sync(_ *ui.UI, _ bool) (bool, error) { - return true, nil + // See the equivalent comment on BuiltInSource.Sync: this source performs + // no actual synchronisation, so it must report "false" here or it + // poisons Sources.isSynchronised for every other source. + return false, nil } func (s *LocalSource) URI() string { diff --git a/sources/lock.go b/sources/lock.go new file mode 100644 index 00000000..504f443c --- /dev/null +++ b/sources/lock.go @@ -0,0 +1,108 @@ +package sources + +import ( + "context" + "path/filepath" + "sync" + "time" + + "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/ui" + "github.com/cashapp/hermit/util/flock" +) + +// slowLockWaitThreshold is how long acquireSyncLock will wait before it +// considers the wait worth telling the user about at Info level: below this, +// logging would just be noise for the common, fast, uncontended case. +const slowLockWaitThreshold = time.Second + +// DefaultLockTimeout is how long Hermit will wait for another process to +// finish synchronising a source before giving up. +// +// This is deliberately much longer than the global state lock timeout +// (--lock-timeout, default 30s): the process holding this lock may be +// performing a full "git clone" of a manifest repository over a slow network. +const DefaultLockTimeout = 10 * time.Minute + +// lockSuffix is appended to a source directory to derive its lock file path. +// +// Source directory names are bare hex SHA256 hashes (see util.Hash), so any +// entry containing a "." is unambiguously Hermit scratch state rather than a +// real source directory. +const lockSuffix = ".lock" + +// util/flock is deliberately re-entrant per-process: if the lock file already +// records our own PID, Acquire returns a no-op release and the caller +// proceeds without actually holding the lock (see util/flock.Acquire). That +// means it will not serialise two goroutines within the same process. +// +// We serialise those here, with a plain (non-re-entrant) mutex keyed by lock +// path, before ever touching flock. This must NOT live in util/flock itself: +// state.CleanPackages deliberately re-acquires its own lock recursively +// (via removeRecursive), and a non-re-entrant mutex there would deadlock. +var ( + localLocksMu sync.Mutex + localLocks = map[string]*sync.Mutex{} +) + +// localLock returns the process-local mutex for the given (already absolute) +// lock path. +func localLock(absPath string) *sync.Mutex { + localLocksMu.Lock() + defer localLocksMu.Unlock() + l, ok := localLocks[absPath] + if !ok { + l = &sync.Mutex{} + localLocks[absPath] = l + } + return l +} + +// acquireSyncLock takes an exclusive lock, across both processes and +// goroutines, on the source directory "dir". +// +// A timeout <= 0 is treated as DefaultLockTimeout. +// +// acquireSyncLock must never be called recursively (directly or indirectly) +// for the same "dir" from within the same process, as the process-local +// mutex it uses is not re-entrant. +func acquireSyncLock(log ui.Logger, dir string, timeout time.Duration, message string) (release func() error, err error) { + if timeout <= 0 { + timeout = DefaultLockTimeout + } + path := dir + lockSuffix + // Resolve to an absolute path before using it as a map key or handing it + // to flock: two different relative paths (or a relative and an absolute + // path) that resolve to the same file must serialise against each other, + // or the process-local mutex below is useless for callers that don't all + // construct "dir" identically. + absPath, err := filepath.Abs(path) + if err != nil { + return nil, errors.WithStack(err) + } + + local := localLock(absPath) + local.Lock() + + log.Tracef("acquiring source lock %s (timeout %s)", absPath, timeout) + start := time.Now() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + releaseFlock, err := flock.Acquire(ctx, absPath, message) + if err != nil { + local.Unlock() + return nil, errors.Wrapf(err, "failed to acquire source lock %s", absPath) + } + if waited := time.Since(start); waited > slowLockWaitThreshold { + // The common, uncontended case acquires near-instantly and would + // make this pure noise; a wait long enough to notice is worth + // telling the user about, since otherwise a command silently hangs + // for up to "timeout" with only a Trace-level line (invisible by + // default) explaining why. + log.Infof("waited %s to acquire source lock for %s", waited.Round(time.Millisecond), absPath) + } + return func() error { + defer local.Unlock() + return releaseFlock() + }, nil +} diff --git a/sources/memory.go b/sources/memory.go index f327ba12..34608359 100644 --- a/sources/memory.go +++ b/sources/memory.go @@ -19,7 +19,10 @@ func NewMemSource(name, content string) *MemSource { } func (s *MemSource) Sync(_ *ui.UI, _ bool) (bool, error) { - return true, nil + // See the equivalent comment on BuiltInSource.Sync: this source performs + // no actual synchronisation, so it must report "false" here or it + // poisons Sources.isSynchronised for every other source. + return false, nil } func (s *MemSource) URI() string { diff --git a/sources/sources.go b/sources/sources.go index 501def7c..30d0ffed 100644 --- a/sources/sources.go +++ b/sources/sources.go @@ -17,6 +17,15 @@ import ( // SyncFrequency determines how frequently sources will be synced. const SyncFrequency = time.Hour * 24 +// ErrSourceUnavailable indicates that a source's backing directory could not +// be found at all -- as opposed to the directory existing but simply not +// containing the requested manifest. Distinguishing the two matters because +// a git source's directory can be transiently absent while another Hermit +// process is mid-sync (see GitSource.Sync), which is not the same thing as +// "genuinely unknown package": callers should retry rather than treat it as +// authoritative. +var ErrSourceUnavailable = errors.New("source unavailable") + // Source is a single source for manifest files type Source interface { // Sync synchronises these sources from the possibly remote origin. @@ -184,6 +193,15 @@ func (s *Sources) Bundles() []fs.FS { // This exists to provide useful debugging information back to the user. type uriFS struct { uri string + // dir, if set, is the backing directory on disk for this source. It is + // used to distinguish "this manifest doesn't exist in this bundle" from + // "this bundle's backing directory itself is currently missing" (eg. + // because another process is mid-sync, or the source configuration or + // permissions are wrong). Only set for sources actually backed by a + // directory that can meaningfully vanish (GitSource): leaving it empty + // for in-memory sources avoids misreporting them as unavailable, since + // some (eg. vfs.InMemoryFS) return fs.ErrNotExist unconditionally. + dir string fs.FS } @@ -191,3 +209,25 @@ func (u *uriFS) Stat(name string) (fs.FileInfo, error) { return fs.Stat(u.F func (u *uriFS) ReadDir(name string) ([]fs.DirEntry, error) { return fs.ReadDir(u.FS, name) } func (u *uriFS) Glob(pattern string) ([]string, error) { return fs.Glob(u.FS, pattern) } func (u *uriFS) String() string { return u.uri } + +// Open wraps the underlying FS's Open, reporting ErrSourceUnavailable +// instead of the usual fs.ErrNotExist when the failure is because this +// source's entire backing directory is missing, rather than just the +// requested file within it. +// +// The os.Stat below is necessarily retrospective and best-effort: it checks +// whether the directory is missing *now*, not whether it was missing at the +// moment FS.Open failed above. A directory that vanishes and reappears +// between those two calls (eg. a fast concurrent resync) can still be +// misreported either way. That's fine for our purposes -- callers only use +// ErrSourceUnavailable as a signal to retry, never as an authoritative +// answer -- but it means this is a heuristic, not a guarantee. +func (u *uriFS) Open(name string) (fs.File, error) { + f, err := u.FS.Open(name) + if err != nil && u.dir != "" && errors.Is(err, fs.ErrNotExist) { + if _, statErr := os.Stat(u.dir); os.IsNotExist(statErr) { + return nil, errors.Wrap(ErrSourceUnavailable, u.uri) + } + } + return f, err +} diff --git a/state/state.go b/state/state.go index de91e15d..2a614642 100644 --- a/state/state.go +++ b/state/state.go @@ -262,6 +262,13 @@ func (s *State) ReadPackageState(pkg *manifest.Package) { } // WritePackageState updates the fields and usage time stamp of the given package +// +// A zero UpdateCheckedAt (when p.UpdateInterval <= 0, ie. this package never +// checks for updates) is stored as "now" by dao.UpdatePackage rather than as +// a literal zero time -- see its docs. That's harmless here specifically: +// EnsureChannelIsUpToDate short-circuits on UpdateInterval == 0 before ever +// consulting UpdatedAt, so the substituted value is never read back for a +// package in this state. func (s *State) WritePackageState(p *manifest.Package) error { updatedAt := time.Time{} if p.UpdateInterval > 0 { @@ -304,7 +311,7 @@ func (s *State) removeRecursive(b *ui.Task, dest string) error { return errors.WithStack(err) }) task.Debugf("rm -rf %s", dest) - return errors.WithStack(os.RemoveAll(dest)) + return errors.WithStack(util.RemoveAllAtomic(dest)) } // CacheAndUnpack downloads a package and extracts it if it is not present. @@ -383,34 +390,43 @@ func (s *State) CacheAndDigest(b *ui.Task, p *manifest.Package) (string, error) return actualDigest, nil } +// linkBinaries creates symlinks in s.binaryDir/ for each of the +// package's binaries, replacing any existing set. +// +// The new set of links is built in a temporary sibling directory and swapped +// into place with util.SwapDir, rather than removing the existing directory +// and recreating it in place. This method runs under s.acquireLock, but its +// readers don't: CacheAndUnpack's pre-lock fast path (areBinariesLinked) +// checks this directory without taking any lock, and by the time it returns +// "linked", the caller may go on to actually exec a binary through it. A +// destructive remove-then-recreate would leave a window during which the +// directory is missing or only partially populated, visible to either of +// those. func (s *State) linkBinaries(p *manifest.Package) error { dir := filepath.Join(s.binaryDir, p.Reference.String()) - // clean up the binaryDir before - if err := os.RemoveAll(dir); err != nil { + + bins, err := p.ResolveBinaries() + if err != nil { return errors.WithStack(err) } - if err := os.MkdirAll(dir, 0o700); err != nil { + if err := os.MkdirAll(s.binaryDir, 0o700); err != nil { return errors.WithStack(err) } - - bins, err := p.ResolveBinaries() + tmp, err := os.MkdirTemp(s.binaryDir, filepath.Base(dir)+".tmp-*") if err != nil { return errors.WithStack(err) } + defer os.RemoveAll(tmp) // harmless once swapped into place for _, bin := range bins { - to := filepath.Join(dir, filepath.Base(bin)) - - if dest, err := os.Readlink(to); err == nil && dest == bin { - continue - } - + to := filepath.Join(tmp, filepath.Base(bin)) if err := os.Symlink(bin, to); err != nil { return errors.WithStack(err) } } - return nil + + return errors.WithStack(util.SwapDir(tmp, dir)) } func (s *State) extract(b *ui.Task, p *manifest.Package) error { @@ -438,15 +454,25 @@ func (s *State) extract(b *ui.Task, p *manifest.Package) error { if err != nil { return errors.WithStack(err) } - // Copy manifest referred files + // From here on, p.Dest is already published: archive.Extract renames it + // into place before returning, not after finalise() runs. That means an + // unlocked reader could be looking at it, so any failure below must clean + // it up the same reader-safe way as everywhere else in this package. For + // the common case where the manifest doesn't override "root" (so it + // defaults to p.Dest, see manifest.Package), leaving p.Dest behind on + // failure is worse than "wedged": CacheAndUnpack's isExtracted check + // would see p.Root already present and skip re-extraction on retry + // entirely, silently leaving the package installed without these files + // forever. Copy manifest referred files. for _, file := range p.Files { err = vfs.CopyFile(file.FS, file.FromPath, file.ToPath) if err != nil { + _ = util.RemoveAllAtomic(p.Dest) return errors.WithStack(err) } } if _, err = p.Trigger(b, manifest.EventUnpack); err != nil { - _ = os.RemoveAll(p.Dest) + _ = util.RemoveAllAtomic(p.Dest) return errors.WithStack(err) } return errors.WithStack(finalise()) diff --git a/util/atomicfile.go b/util/atomicfile.go new file mode 100644 index 00000000..b785b079 --- /dev/null +++ b/util/atomicfile.go @@ -0,0 +1,50 @@ +package util + +import ( + "os" + "path/filepath" + + "github.com/cashapp/hermit/errors" +) + +// AtomicWriteFile writes data to path atomically: it is written to a temp +// file in the same directory, then renamed into place. Unlike os.WriteFile, +// which truncates the existing file before writing, a concurrent reader can +// never observe an empty or partially-written file. +// +// This deliberately does not fsync the temp file before renaming it: doing so +// closes a narrow crash-durability gap (a crash between a successful-looking +// write and the underlying data actually reaching disk could otherwise leave +// path pointing at a zero-length or truncated file), but on macOS, Go's +// File.Sync issues fcntl(F_FULLFSYNC), which is roughly two orders of +// magnitude slower than a plain write. That cost is judged not worth paying +// here for either of this helper's callers: internal/dao.UpdatePackage's +// writes (a cached etag and check timestamp) already sit behind a network +// round trip and aren't authoritative -- losing one to a crash just costs one +// extra upstream check next run -- and while Env.SetEnv/DelEnv's writes to +// the user's bin/hermit.hcl are more consequential (a crash could lose a +// just-persisted "hermit env" change), that's accepted as the cost of a +// single shared helper rather than special-casing fsync per caller. +func AtomicWriteFile(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*") + if err != nil { + return errors.WithStack(err) + } + tmpPath := tmp.Name() + // Harmless once the rename below succeeds: nothing left to remove. + defer os.Remove(tmpPath) + + _, writeErr := tmp.Write(data) + closeErr := tmp.Close() + if writeErr != nil { + return errors.WithStack(writeErr) + } + if closeErr != nil { + return errors.WithStack(closeErr) + } + if err := os.Chmod(tmpPath, perm); err != nil { + return errors.WithStack(err) + } + return errors.WithStack(os.Rename(tmpPath, path)) +} diff --git a/util/dirswap.go b/util/dirswap.go new file mode 100644 index 00000000..e411f0dc --- /dev/null +++ b/util/dirswap.go @@ -0,0 +1,95 @@ +package util + +import ( + "os" + "path/filepath" + + "github.com/cashapp/hermit/errors" +) + +// DirSwapAsideSuffix is appended to finalDest to name the location its +// previous contents are moved aside to during SwapDir. Exported so callers +// that sweep up leftover scratch state (eg. after a crash mid-swap) can +// recognise these directories without duplicating the literal. +const DirSwapAsideSuffix = ".old" + +// SwapDir atomically (from the point of view of an unlocked reader) replaces +// finalDest with src. +// +// Some callers have readers that check a directory's contents (or existence) +// without taking any lock -- eg. another Hermit process reading a manifest +// source, or CacheAndUnpack's pre-lock fast path checking a package's linked +// binaries. For a large directory, removing finalDest outright and +// recreating it can take long enough (or leave it transiently +// missing/partial for long enough) that a concurrent unlocked read observes +// ENOENT or an incomplete directory. Instead, the existing tree is renamed +// aside and the new one is renamed into its place. This shrinks the window +// in which finalDest does not exist from however long it takes to remove and +// repopulate a potentially large tree, down to the gap between two rename(2) +// calls in the same directory -- not zero, but small and constant-time +// regardless of tree size, and each rename itself is atomic so a reader never +// observes a partially-written directory. +// +// The caller must ensure no other goroutine or process can be concurrently +// mutating finalDest (eg. by holding an appropriate lock); SwapDir only +// protects readers, not other writers. +func SwapDir(src, finalDest string) error { + aside := finalDest + DirSwapAsideSuffix + + // May exist already if a previous process died mid-swap. Safe to remove: + // the caller is assumed to hold exclusive write access, so nothing is + // relying on it any more. + if err := os.RemoveAll(aside); err != nil { + return errors.WithStack(err) + } + if err := os.Rename(finalDest, aside); err != nil && !os.IsNotExist(err) { + return errors.WithStack(err) + } + if err := os.Rename(src, finalDest); err != nil { + // Put the previous tree back so that we degrade to "stale" rather + // than "missing". + if rerr := os.Rename(aside, finalDest); rerr != nil && !os.IsNotExist(rerr) { + return errors.Join(errors.WithStack(err), errors.WithStack(rerr)) + } + return errors.WithStack(err) + } + // aside is no longer reachable via finalDest, and readers with an open + // FS handle are unaffected by unlinking it, so it's safe to remove + // synchronously here. We deliberately don't defer this to a goroutine: + // Hermit's exec path ends in syscall.Exec, which would silently kill any + // in-flight background cleanup and leak the directory forever. + return errors.WithStack(os.RemoveAll(aside)) +} + +// RemoveAllAtomic removes dir in a way that's safe for an unlocked reader: +// dir is first renamed to a uniquely-named sibling, then removed. This closes +// the same reader-visible window SwapDir does for replacement -- a reader +// that stats or opens dir sees either the whole original tree or ENOENT, +// never a tree with some entries already unlinked out from under it. +// +// The caller must ensure no other goroutine or process can be concurrently +// mutating dir. +// +// Unlike os.RemoveAll, RemoveAllAtomic is not nil-safe for a wholly-missing +// path: it requires dir's parent directory to exist (MkdirTemp needs +// somewhere to create the sibling), and returns an error if the parent is +// itself missing. A missing dir with an existing parent is still handled -- +// that case returns nil, same as os.RemoveAll. +func RemoveAllAtomic(dir string) error { + aside, err := os.MkdirTemp(filepath.Dir(dir), filepath.Base(dir)+DirSwapAsideSuffix+"-*") + if err != nil { + return errors.WithStack(err) + } + // MkdirTemp creates aside itself; remove the placeholder so the rename + // below can take its place. + if err := os.Remove(aside); err != nil { + return errors.WithStack(err) + } + if err := os.Rename(dir, aside); err != nil { + if os.IsNotExist(err) { + return nil + } + return errors.WithStack(err) + } + return errors.WithStack(os.RemoveAll(aside)) +} diff --git a/util/dirswap_test.go b/util/dirswap_test.go new file mode 100644 index 00000000..ab8f4267 --- /dev/null +++ b/util/dirswap_test.go @@ -0,0 +1,97 @@ +package util + +import ( + "os" + "path/filepath" + "testing" + + "github.com/alecthomas/assert/v2" +) + +func TestSwapDir(t *testing.T) { + dir := t.TempDir() + finalDest := filepath.Join(dir, "final") + assert.NoError(t, os.MkdirAll(finalDest, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(finalDest, "old.txt"), []byte("old"), 0600)) + + src := filepath.Join(dir, "new") + assert.NoError(t, os.MkdirAll(src, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(src, "new.txt"), []byte("new"), 0600)) + + assert.NoError(t, SwapDir(src, finalDest)) + + _, err := os.Stat(filepath.Join(finalDest, "new.txt")) + assert.NoError(t, err) + _, err = os.Stat(filepath.Join(finalDest, "old.txt")) + assert.True(t, os.IsNotExist(err)) + _, err = os.Stat(src) + assert.True(t, os.IsNotExist(err)) + _, err = os.Stat(finalDest + DirSwapAsideSuffix) + assert.True(t, os.IsNotExist(err)) +} + +// TestSwapDirRecoversFromMissingSource verifies SwapDir cleans up a stale +// ".old" left behind by a process that crashed mid-swap, and that finalDest +// never has an intervening moment where it doesn't exist for a caller that +// only checks before and after (a genuine no-gap guarantee needs a +// concurrent reader, which is exercised at the sources.GitSource level). +func TestSwapDirRecoversFromMissingSource(t *testing.T) { + dir := t.TempDir() + finalDest := filepath.Join(dir, "final") + assert.NoError(t, os.MkdirAll(finalDest, 0700)) + + // Simulate a previous crash mid-swap: a stale ".old" already exists. + aside := finalDest + DirSwapAsideSuffix + assert.NoError(t, os.MkdirAll(aside, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(aside, "junk"), []byte("junk"), 0600)) + + src := filepath.Join(dir, "new") + assert.NoError(t, os.MkdirAll(src, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(src, "new.txt"), []byte("new"), 0600)) + + assert.NoError(t, SwapDir(src, finalDest)) + + _, err := os.Stat(filepath.Join(finalDest, "new.txt")) + assert.NoError(t, err) + _, err = os.Stat(aside) + assert.True(t, os.IsNotExist(err)) +} + +// TestSwapDirNoPreviousDest verifies SwapDir works when finalDest doesn't +// exist yet at all (the common case: first-ever creation of a directory). +func TestSwapDirNoPreviousDest(t *testing.T) { + dir := t.TempDir() + finalDest := filepath.Join(dir, "final") + + src := filepath.Join(dir, "new") + assert.NoError(t, os.MkdirAll(src, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(src, "new.txt"), []byte("new"), 0600)) + + assert.NoError(t, SwapDir(src, finalDest)) + + _, err := os.Stat(filepath.Join(finalDest, "new.txt")) + assert.NoError(t, err) +} + +func TestRemoveAllAtomic(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "victim") + assert.NoError(t, os.MkdirAll(target, 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(target, "f"), []byte("data"), 0600)) + + assert.NoError(t, RemoveAllAtomic(target)) + + _, err := os.Stat(target) + assert.True(t, os.IsNotExist(err)) + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + assert.Equal(t, 0, len(entries), "no scratch entries should be left behind") +} + +// TestRemoveAllAtomicMissingTarget verifies RemoveAllAtomic is a no-op (not +// an error) when the target doesn't exist, matching os.RemoveAll's +// semantics. +func TestRemoveAllAtomicMissingTarget(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, RemoveAllAtomic(filepath.Join(dir, "does-not-exist"))) +} diff --git a/util/flock/flock.go b/util/flock/flock.go index cbd67ffc..f779825b 100644 --- a/util/flock/flock.go +++ b/util/flock/flock.go @@ -72,6 +72,19 @@ func Acquire(ctx context.Context, path, message string) (release func() error, e } } +// acquire takes the flock itself, then records our PID in the lock file's +// contents for the benefit of Acquire's own-PID re-entrancy check above. +// +// There is a small window between the LOCK_EX succeeding and the PID payload +// being written below: a concurrent Acquire call in another process that +// reads the file's contents during that window sees either an empty file +// (first-ever acquisition) or a stale PID from whoever held the lock +// previously -- never our own PID, so its own-PID check simply falls +// through to the normal wait/retry path rather than misbehaving. This is +// pre-existing and has always been benign, but it is now load-bearing: +// sources/lock.go's cross-process source lock depends on Acquire's +// re-entrancy check to avoid a second sync within the same process +// deadlocking against itself. func acquire(path, message string) (release func() error, err error) { pid := getPID() fd, err := unix.Open(path, unix.O_CREAT|unix.O_RDWR|unix.O_CLOEXEC|unix.O_SYNC, 0600)