From ba65b7561dd6e7aa206010e83834c3a82544a4a4 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 12:12:27 -0700 Subject: [PATCH 01/15] sources: add reproducer for concurrent source sync race Executing a Hermit-managed binary that hasn't been installed yet, several times within a few milliseconds of each other, can make some invocations fail with "unknown package" even though the package is perfectly valid. GitSource.Sync has no cross-process or cross-goroutine locking, so concurrent syncs of the same not-yet-cloned source race: each clones independently and then wipes and replaces the shared manifest tree, leaving a window where a concurrent reader sees ENOENT partway through. TestConcurrentSyncInProcess and TestConcurrentSyncAcrossProcesses reproduce this directly (the latter across genuine child processes, since util/flock is deliberately re-entrant per-PID and so cannot exercise cross-process contention from goroutines alone). Both fail against the current implementation; the fix follows in a subsequent change. --- sources/git_concurrency_test.go | 253 ++++++++++++++++++++++++++++++++ 1 file changed, 253 insertions(+) create mode 100644 sources/git_concurrency_test.go diff --git a/sources/git_concurrency_test.go b/sources/git_concurrency_test.go new file mode 100644 index 00000000..35fc7dc0 --- /dev/null +++ b/sources/git_concurrency_test.go @@ -0,0 +1,253 @@ +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) + } +} + +// 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 the process-local mutex in sources/lock.go (hence -race), +// but not flock's cross-process behaviour -- see +// TestConcurrentSyncAcrossProcesses for that. +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 wg sync.WaitGroup + for range n { + wg.Add(1) + go func() { + defer wg.Done() + u, _ := ui.NewForTesting() + source := sources.NewGitSource(uri, sourceDir, runner) + _, err := source.Sync(u, true) + assert.NoError(t, err) + }() + } + 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") + 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() + 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", + ) + out, err := cmd.CombinedOutput() + if err != nil { + errs[i] = fmt.Errorf("child %d failed: %w\n%s", i, err, out) + } + }(i) + } + 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) + } + 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) + } +} From 8f2d0e48e4d5e7125c2a8684942d8857ab774a4a Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 13:32:01 -0700 Subject: [PATCH 02/15] sources: add start barrier to maximise reproducer race odds TestConcurrentSyncInProcess and TestConcurrentSyncAcrossProcesses previously let goroutines/child processes begin racing as soon as each was spawned, so on a fast machine some finished before the last one even started, understating how often the race actually reproduces. Hold every goroutine/child at a barrier until all have signalled ready, then release them together, so all n consistently race through Sync concurrently. Also corrects TestConcurrentSyncInProcess's doc comment, which overclaimed that -race specifically exercises "the process-local mutex in sources/lock.go" -- that file doesn't exist yet at this point in the stack. --- sources/git_concurrency_test.go | 62 +++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/sources/git_concurrency_test.go b/sources/git_concurrency_test.go index 35fc7dc0..1bcb2c5f 100644 --- a/sources/git_concurrency_test.go +++ b/sources/git_concurrency_test.go @@ -104,6 +104,24 @@ func pollForVanishAfterAppearing(stop <-chan struct{}, path string, errCh chan<- } } +// 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. @@ -140,9 +158,13 @@ func assertNoScratchDirs(t *testing.T, sourceDir string) { // GitSource.Sync writes and fails the test if it ever sees the manifest // vanish after having first seen it exist. // -// This exercises the process-local mutex in sources/lock.go (hence -race), -// but not flock's cross-process behaviour -- see -// TestConcurrentSyncAcrossProcesses for that. +// 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() @@ -155,17 +177,24 @@ func TestConcurrentSyncInProcess(t *testing.T) { 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) @@ -192,6 +221,8 @@ func TestConcurrentSyncAcrossProcesses(t *testing.T) { 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") @@ -205,6 +236,7 @@ func TestConcurrentSyncAcrossProcesses(t *testing.T) { 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", @@ -212,6 +244,8 @@ func TestConcurrentSyncAcrossProcesses(t *testing.T) { "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 { @@ -219,6 +253,17 @@ func TestConcurrentSyncAcrossProcesses(t *testing.T) { } }(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) @@ -244,6 +289,17 @@ func TestSyncChildProcess(t *testing.T) { 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() From 457c7e573634009815ffb4fec4462c61de25ad0b Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 12:14:43 -0700 Subject: [PATCH 03/15] sources: serialise and de-destruct git source syncing Fixes the race reproduced in the previous commit. GitSource.Sync had no cross-process or cross-goroutine locking, so concurrent syncs of the same not-yet-cloned source raced: every caller passed the same pre-lock check, cloned independently, and each then did RemoveAll(dest) + Rename(tmp, dest) to install its result -- an unlink storm over the whole manifest tree that any concurrent reader could observe mid-way through as ENOENT, which is exactly the "unknown package" failure this was reported as. - sources/lock.go adds acquireSyncLock: a cross-process flock plus a process-local sync.Mutex (needed because util/flock is deliberately re-entrant per-PID, so it's a no-op between goroutines of the same process). - GitSource.Sync now takes this lock around the whole sync, with double-checked locking against the pre/post-lock mtime so a waiter that loses the race skips redundant work, and degrades to the existing copy (rather than failing) if the lock can't be acquired in time and a usable tree already exists. - The install step no longer destroys the target before the new tree is ready: util.SwapDir (new, util/dirswap.go) replaces RemoveAll+Rename with rename-aside + rename-into-place + cleanup, so a concurrent unlocked reader sees either the old or the new tree, but never neither. A crashed swap is recoverable from the "aside" copy on the next sync. - Stale scratch directories left by a killed-mid-sync process (clone temp dirs, interrupted swap asides, and the legacy pre-lock naming scheme) are swept on a generous age threshold under the lock. - BuiltInSource/LocalSource/MemSource.Sync now correctly report "false" (no synchronisation performed) instead of "true": they were unconditionally poisoning Sources.isSynchronised, which made every later "sync and retry" elsewhere in the codebase a silent no-op. TestConcurrentSyncInProcess and TestConcurrentSyncAcrossProcesses from the previous commit now pass, along with new coverage for the swap recovery, stale-scratch sweep, and lock-timeout fallback paths. --- sources/builtin.go | 12 ++- sources/git.go | 166 ++++++++++++++++++++++++++------ sources/git_concurrency_test.go | 41 ++++++++ sources/git_internal_test.go | 141 +++++++++++++++++++++++++++ sources/git_test.go | 34 +++++-- sources/local.go | 5 +- sources/lock.go | 82 ++++++++++++++++ sources/memory.go | 5 +- util/dirswap.go | 58 +++++++++++ util/dirswap_test.go | 74 ++++++++++++++ 10 files changed, 576 insertions(+), 42 deletions(-) create mode 100644 sources/git_internal_test.go create mode 100644 sources/lock.go create mode 100644 util/dirswap.go create mode 100644 util/dirswap_test.go 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..6c825d65 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,107 @@ 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. +// +// A timeout <= 0 is treated as DefaultLockTimeout. This is primarily useful +// for tests. +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, 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+). +func syncedSince(info os.FileInfo, since time.Time) bool { + return info != nil && !info.ModTime().Add(fsTimeGranularity).Before(since) } func (s *GitSource) URI() string { @@ -70,6 +132,8 @@ func (s *GitSource) ensureSourcesDirExists() error { } // Atomically clone git repo. +// +// 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,6 +143,9 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne err = errors.WithStack(os.Chtimes(finalDest, now, now)) } }() + + removeStaleScratchDirs(b, dir, finalDest) + // First, if a git repo exists, just pull. info, _ := os.Stat(filepath.Join(finalDest, ".git")) if info != nil { @@ -89,7 +156,7 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne // 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)+"-*") + dest, err := os.MkdirTemp(dir, filepath.Base(finalDest)+tmpInfix+"*") if err != nil { return errors.WithStack(err) } @@ -97,11 +164,52 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne if err = runner.RunInDir(b, dest, "git", "clone", "--depth=1", source, 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. - return errors.WithStack(err) - } + return errors.WithStack(swapDir(dest, finalDest)) +} - return nil +// swapDir atomically (from the point of view of an unlocked reader) replaces +// finalDest with src. See util.SwapDir for how. +// +// Readers in other Hermit processes do not take the sync lock, so this +// matters here specifically to avoid the ENOENT-during-clone window that +// makes every package look unknown while a large source tree is being +// replaced. +// +// The caller MUST hold the sync lock for finalDest. +func swapDir(src, finalDest string) error { + return util.SwapDir(src, finalDest) +} + +// 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 index 1bcb2c5f..777e53a6 100644 --- a/sources/git_concurrency_test.go +++ b/sources/git_concurrency_test.go @@ -307,3 +307,44 @@ func TestSyncChildProcess(t *testing.T) { 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)) + 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(), + ) + assert.NoError(t, cmd.Start()) + + // Give the child a moment to actually acquire the lock before we race it. + time.Sleep(50 * time.Millisecond) + + 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..6e900c79 --- /dev/null +++ b/sources/git_internal_test.go @@ -0,0 +1,141 @@ +package sources + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/alecthomas/assert/v2" + + "github.com/cashapp/hermit/ui" +) + +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)) + }) + } +} + +// TestSwapDirRecoversFromMissingSource verifies that swapDir cleans up a +// stale ".old" left behind by a process that crashed mid-swap, and leaves no +// scratch directories behind on success. +func TestSwapDirRecoversFromMissingSource(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.hcl"), []byte("old"), 0600)) + + // Simulate a previous crash mid-swap: a stale ".old" already exists. + aside := finalDest + asideSuffix + 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.hcl"), []byte("new"), 0600)) + + assert.NoError(t, swapDir(src, finalDest)) + + _, err := os.Stat(filepath.Join(finalDest, "new.hcl")) + assert.NoError(t, err) + _, err = os.Stat(aside) + assert.True(t, os.IsNotExist(err)) + _, err = os.Stat(src) + assert.True(t, os.IsNotExist(err)) +} + +// 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) +} + +// 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) + } + 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..54c1f77e --- /dev/null +++ b/sources/lock.go @@ -0,0 +1,82 @@ +package sources + +import ( + "context" + "sync" + "time" + + "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/ui" + "github.com/cashapp/hermit/util/flock" +) + +// 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{} +) + +func localLock(path string) *sync.Mutex { + localLocksMu.Lock() + defer localLocksMu.Unlock() + l, ok := localLocks[path] + if !ok { + l = &sync.Mutex{} + localLocks[path] = 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 + + local := localLock(path) + local.Lock() + + log.Tracef("acquiring source lock %s (timeout %s)", path, timeout) + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + releaseFlock, err := flock.Acquire(ctx, path, message) + if err != nil { + local.Unlock() + return nil, errors.Wrapf(err, "failed to acquire source lock %s", path) + } + 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/util/dirswap.go b/util/dirswap.go new file mode 100644 index 00000000..8987c372 --- /dev/null +++ b/util/dirswap.go @@ -0,0 +1,58 @@ +package util + +import ( + "os" + + "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 to the gap between two rename(2) calls +// in the same directory, which is not observable by another process. +// +// 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)) +} diff --git a/util/dirswap_test.go b/util/dirswap_test.go new file mode 100644 index 00000000..025fd4d2 --- /dev/null +++ b/util/dirswap_test.go @@ -0,0 +1,74 @@ +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) +} From b4fb272f927e55c5190effa4aa60470289d81642 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 13:35:03 -0700 Subject: [PATCH 04/15] sources: remove pull-path race, harden lock, address review nits High: syncGit's "git pull" fast path mutated finalDest's working tree in place with no lock held at the time it was added, and two concurrent pulls could also collide on .git/index.lock, escalating into a destructive re-clone via the "assume corrupted" fallback. Drop the pull path entirely; always clone to a fresh temp dir and swap it in, using "--reference-if-able --dissociate" against the existing clone so the network cost stays close to a pull's. Medium: log at Info level when acquireSyncLock waits more than a second, so lock contention is visible without needing -v/Trace. Low: resolve the lock path to absolute before using it as the process-local mutex key, so two callers that reach the same lock file via different relative paths still serialise against each other; remove the now-redundant swapDir wrapper and its duplicate test; document the acquire()/PID-write race window in util/flock now that it's load-bearing for lock re-entrancy; document syncedSince's fsTimeGranularity slack; correct doc comments that overclaimed either NewGitSourceWithLockTimeout's test-only-ness or SwapDir's rename gap being unobservable. Also replaces TestSyncLockTimeoutFallsBackToExistingCopy's fixed sleep with a ready-file handshake from the lock-holding child process (fixed sleeps are flaky under load) and guarantees that child is reaped via t.Cleanup even if an earlier assertion fails the test first. --- sources/git.go | 66 ++++++++++++++++++--------------- sources/git_concurrency_test.go | 17 ++++++++- sources/git_internal_test.go | 33 +++-------------- sources/lock.go | 40 ++++++++++++++++---- util/dirswap.go | 7 +++- util/flock/flock.go | 13 +++++++ 6 files changed, 108 insertions(+), 68 deletions(-) diff --git a/sources/git.go b/sources/git.go index 6c825d65..baf880d7 100644 --- a/sources/git.go +++ b/sources/git.go @@ -43,10 +43,12 @@ func NewGitSource(uri, sourceDir string, runner util.CommandRunner) *GitSource { } // NewGitSourceWithLockTimeout returns a new GitSource with an explicit -// timeout for the lock acquired around synchronisation. +// 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. This is primarily useful -// for tests. +// 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) @@ -112,6 +114,15 @@ func (s *GitSource) Sync(p *ui.UI, force bool) (bool, error) { // 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) } @@ -131,7 +142,21 @@ func (s *GitSource) ensureSourcesDirExists() error { return nil } -// Atomically clone git repo. +// Atomically clone (or, if finalDest is already a clone, re-clone) 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 cloning to a fresh directory and swapping it in +// atomically (see util.SwapDir) avoids both problems, at the cost of always +// paying for a fresh clone rather than an incremental fetch. That's mitigated +// with "--reference-if-able": when finalDest already has a ".git" directory, +// it's used as a local object-store cache for the clone below, so the +// network cost is close to that of a pull, without mutating finalDest itself +// until the clone has fully succeeded. // // The caller MUST hold the sync lock for finalDest. func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunner) (err error) { @@ -146,38 +171,21 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne removeStaleScratchDirs(b, dir, finalDest) - // 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)+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 { + + args := []string{"git", "clone", "--depth=1"} + if info, _ := os.Stat(filepath.Join(finalDest, ".git")); info != nil { + args = append(args, "--reference-if-able", finalDest, "--dissociate") + } + args = append(args, source, dest) + if err = runner.RunInDir(b, dest, args...); err != nil { return errors.WithStack(err) } - return errors.WithStack(swapDir(dest, finalDest)) -} - -// swapDir atomically (from the point of view of an unlocked reader) replaces -// finalDest with src. See util.SwapDir for how. -// -// Readers in other Hermit processes do not take the sync lock, so this -// matters here specifically to avoid the ENOENT-during-clone window that -// makes every package look unknown while a large source tree is being -// replaced. -// -// The caller MUST hold the sync lock for finalDest. -func swapDir(src, finalDest string) error { - return util.SwapDir(src, finalDest) + return errors.WithStack(util.SwapDir(dest, finalDest)) } // removeStaleScratchDirs removes leftover clone/swap scratch directories from diff --git a/sources/git_concurrency_test.go b/sources/git_concurrency_test.go index 777e53a6..ba21cab5 100644 --- a/sources/git_concurrency_test.go +++ b/sources/git_concurrency_test.go @@ -329,17 +329,30 @@ func TestSyncLockTimeoutFallsBackToExistingCopy(t *testing.T) { 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() + }) - // Give the child a moment to actually acquire the lock before we race it. - time.Sleep(50 * time.Millisecond) + // 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) diff --git a/sources/git_internal_test.go b/sources/git_internal_test.go index 6e900c79..70b0b4c4 100644 --- a/sources/git_internal_test.go +++ b/sources/git_internal_test.go @@ -42,34 +42,6 @@ func TestSyncedSince(t *testing.T) { } } -// TestSwapDirRecoversFromMissingSource verifies that swapDir cleans up a -// stale ".old" left behind by a process that crashed mid-swap, and leaves no -// scratch directories behind on success. -func TestSwapDirRecoversFromMissingSource(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.hcl"), []byte("old"), 0600)) - - // Simulate a previous crash mid-swap: a stale ".old" already exists. - aside := finalDest + asideSuffix - 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.hcl"), []byte("new"), 0600)) - - assert.NoError(t, swapDir(src, finalDest)) - - _, err := os.Stat(filepath.Join(finalDest, "new.hcl")) - assert.NoError(t, err) - _, err = os.Stat(aside) - assert.True(t, os.IsNotExist(err)) - _, err = os.Stat(src) - assert.True(t, os.IsNotExist(err)) -} - // 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 @@ -134,6 +106,11 @@ func TestHoldSourceLockChildProcess(t *testing.T) { 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/lock.go b/sources/lock.go index 54c1f77e..504f443c 100644 --- a/sources/lock.go +++ b/sources/lock.go @@ -2,6 +2,7 @@ package sources import ( "context" + "path/filepath" "sync" "time" @@ -10,6 +11,11 @@ import ( "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. // @@ -39,13 +45,15 @@ var ( localLocks = map[string]*sync.Mutex{} ) -func localLock(path 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[path] + l, ok := localLocks[absPath] if !ok { l = &sync.Mutex{} - localLocks[path] = l + localLocks[absPath] = l } return l } @@ -63,17 +71,35 @@ func acquireSyncLock(log ui.Logger, dir string, timeout time.Duration, message s 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(path) + local := localLock(absPath) local.Lock() - log.Tracef("acquiring source lock %s (timeout %s)", path, timeout) + 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, path, message) + releaseFlock, err := flock.Acquire(ctx, absPath, message) if err != nil { local.Unlock() - return nil, errors.Wrapf(err, "failed to acquire source lock %s", path) + 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() diff --git a/util/dirswap.go b/util/dirswap.go index 8987c372..4b696b43 100644 --- a/util/dirswap.go +++ b/util/dirswap.go @@ -23,8 +23,11 @@ const DirSwapAsideSuffix = ".old" // 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 to the gap between two rename(2) calls -// in the same directory, which is not observable by another process. +// 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 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) From 9859d53d9202a7816420c76da70c83daaab93860 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 14:30:55 -0700 Subject: [PATCH 05/15] sources: replace inert --reference-if-able clone with a real incremental fetch --reference-if-able (plus --dissociate) was meant to keep an already-synced source's re-sync cost close to a "git pull", by letting the new clone borrow objects from the existing one instead of re-fetching them. It never worked: finalDest is always itself a shallow (--depth=1) clone, and git unconditionally refuses to use a shallow repository as a reference/alternate, so the flag was silently a no-op and every sync paid for a full fresh clone anyway -- with no test covering the actual clone mechanism to catch it. Replace it with a local, working-tree-less clone of finalDest (same-filesystem, not a network operation) followed by a shallow fetch of just the latest commit from the real source and a checkout of that commit. 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. Add a test exercising this incremental path against a real git binary, since none of the existing fakes simulate a second sync over an already-cloned finalDest. --- sources/git.go | 41 +++++++++++++------- sources/git_internal_test.go | 74 ++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 14 deletions(-) diff --git a/sources/git.go b/sources/git.go index baf880d7..e9b0490f 100644 --- a/sources/git.go +++ b/sources/git.go @@ -142,21 +142,29 @@ func (s *GitSource) ensureSourcesDirExists() error { return nil } -// Atomically clone (or, if finalDest is already a clone, re-clone) a git -// repo. +// 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 cloning to a fresh directory and swapping it in -// atomically (see util.SwapDir) avoids both problems, at the cost of always -// paying for a fresh clone rather than an incremental fetch. That's mitigated -// with "--reference-if-able": when finalDest already has a ".git" directory, -// it's used as a local object-store cache for the clone below, so the -// network cost is close to that of a pull, without mutating finalDest itself -// until the clone has fully succeeded. +// ".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. 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. // // The caller MUST hold the sync lock for finalDest. func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunner) (err error) { @@ -177,12 +185,17 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne } defer os.RemoveAll(dest) - args := []string{"git", "clone", "--depth=1"} if info, _ := os.Stat(filepath.Join(finalDest, ".git")); info != nil { - args = append(args, "--reference-if-able", finalDest, "--dissociate") - } - args = append(args, source, dest) - if err = runner.RunInDir(b, dest, args...); err != nil { + if err = runner.RunInDir(b, dest, "git", "clone", "--no-checkout", finalDest, dest); err != nil { + return errors.WithStack(err) + } + if err = runner.RunInDir(b, dest, "git", "fetch", "--depth=1", source, "HEAD"); err != nil { + return errors.WithStack(err) + } + if err = runner.RunInDir(b, dest, "git", "checkout", "--detach", "FETCH_HEAD"); err != nil { + return errors.WithStack(err) + } + } else 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)) diff --git a/sources/git_internal_test.go b/sources/git_internal_test.go index 70b0b4c4..5b1ff5ce 100644 --- a/sources/git_internal_test.go +++ b/sources/git_internal_test.go @@ -2,6 +2,7 @@ package sources import ( "os" + "os/exec" "path/filepath" "testing" "time" @@ -9,6 +10,7 @@ import ( "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 { @@ -85,6 +87,78 @@ func TestRemoveStaleScratchDirs(t *testing.T) { 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) +} + +// 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. +func TestSyncGitIncrementalUpdate(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + upstream := t.TempDir() + runGit(t, upstream, "init", "-q", "-b", "main") + runGit(t, upstream, "config", "user.email", "test@example.com") + runGit(t, upstream, "config", "user.name", "Test") + assert.NoError(t, os.WriteFile(filepath.Join(upstream, "first.hcl"), []byte("description = \"first\"\n"), 0600)) + runGit(t, upstream, "add", "first.hcl") + runGit(t, upstream, "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)) + + // Add a second commit upstream, then sync again: finalDest now has a + // ".git", so this must take the incremental-update branch. + assert.NoError(t, os.WriteFile(filepath.Join(upstream, "second.hcl"), []byte("description = \"second\"\n"), 0600)) + runGit(t, upstream, "add", "second.hcl") + runGit(t, upstream, "commit", "-q", "-m", "second") + + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + + // The new commit's content must be present... + second, err := os.ReadFile(filepath.Join(finalDest, "second.hcl")) + assert.NoError(t, err) + assert.Equal(t, "description = \"second\"\n", string(second)) + // ...and finalDest must still be a valid, checked-out git repo, not left + // mid-checkout by "git clone --no-checkout". + first, err = os.ReadFile(filepath.Join(finalDest, "first.hcl")) + assert.NoError(t, err) + assert.Equal(t, "description = \"first\"\n", string(first)) + 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) +} + // 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 From 3978175bf384ff4ebe937f6b80d3b2c3b1fd4e38 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 15:05:50 -0700 Subject: [PATCH 06/15] sources: keep the incremental clone's local branch alive across repeat syncs Independent review caught that the previous commit's incremental path left finalDest in a detached-HEAD state after "git checkout --detach FETCH_HEAD". "git clone" only copies a source's "refs/heads/*", not a detached HEAD, so the next incremental sync's local clone of finalDest had zero branches to offer as "have"s during its own "git fetch --depth=1" -- silently degrading every sync after the second into the same full-clone cost this path exists to avoid. Verified empirically: with "checkout --detach", finalDest loses its last real ref by the second incremental sync and its fetch negotiation falls back to a full pack transfer; checking out onto a persistent local branch instead ("checkout -B") keeps every subsequent fetch negotiating a clean incremental ACK, indefinitely. Also make syncGit self-healing again for this path: if the incremental update fails (eg. finalDest's ".git" is corrupt or truncated), fall back to a fresh clone instead of surfacing the failure, restoring the same recovery behaviour a from-scratch sync always had. Rewrite the incremental-path test to use a "file://" source (a bare local path silently ignores "--depth", which would hide exactly this class of bug), repeat the sync several times to actually exercise the persistence issue above, verify the persistent branch ref and an upstream deletion both propagate correctly, and isolate it from the running machine's git config/hooks. Add a second test covering the new corrupt-clone fallback. --- sources/git.go | 77 ++++++++++++++++----- sources/git_internal_test.go | 128 +++++++++++++++++++++++++++++------ 2 files changed, 168 insertions(+), 37 deletions(-) diff --git a/sources/git.go b/sources/git.go index e9b0490f..b0b45dc4 100644 --- a/sources/git.go +++ b/sources/git.go @@ -142,6 +142,21 @@ func (s *GitSource) ensureSourcesDirExists() error { return nil } +// 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. // @@ -157,14 +172,23 @@ func (s *GitSource) ensureSourcesDirExists() error { // 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. 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. +// 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) { @@ -185,22 +209,43 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne } defer os.RemoveAll(dest) + freshClone := true if info, _ := os.Stat(filepath.Join(finalDest, ".git")); info != nil { - if err = runner.RunInDir(b, dest, "git", "clone", "--no-checkout", finalDest, dest); err != nil { - return errors.WithStack(err) + 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 err = runner.RunInDir(b, dest, "git", "fetch", "--depth=1", source, "HEAD"); err != nil { - return errors.WithStack(err) - } - if err = runner.RunInDir(b, dest, "git", "checkout", "--detach", "FETCH_HEAD"); err != nil { + } + if freshClone { + if err = runner.RunInDir(b, dest, "git", "clone", "--depth=1", source, dest); err != nil { return errors.WithStack(err) } - } else 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) + } + if err := runner.RunInDir(b, dest, "git", "fetch", "--depth=1", source, "HEAD"); err != nil { + return errors.WithStack(err) + } + return errors.WithStack(runner.RunInDir(b, dest, "git", "checkout", "-B", incrementalBranch, "FETCH_HEAD")) +} + // 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" diff --git a/sources/git_internal_test.go b/sources/git_internal_test.go index 5b1ff5ce..85b744c1 100644 --- a/sources/git_internal_test.go +++ b/sources/git_internal_test.go @@ -1,9 +1,11 @@ package sources import ( + "fmt" "os" "os/exec" "path/filepath" + "strings" "testing" "time" @@ -99,6 +101,20 @@ func runGit(t *testing.T, dir string, args ...string) { 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 @@ -107,18 +123,27 @@ func runGit(t *testing.T, dir string, args ...string) { // 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) - upstream := t.TempDir() - runGit(t, upstream, "init", "-q", "-b", "main") - runGit(t, upstream, "config", "user.email", "test@example.com") - runGit(t, upstream, "config", "user.name", "Test") - assert.NoError(t, os.WriteFile(filepath.Join(upstream, "first.hcl"), []byte("description = \"first\"\n"), 0600)) - runGit(t, upstream, "add", "first.hcl") - runGit(t, upstream, "commit", "-q", "-m", "first") + 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") @@ -132,23 +157,44 @@ func TestSyncGitIncrementalUpdate(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "description = \"first\"\n", string(first)) - // Add a second commit upstream, then sync again: finalDest now has a - // ".git", so this must take the incremental-update branch. - assert.NoError(t, os.WriteFile(filepath.Join(upstream, "second.hcl"), []byte("description = \"second\"\n"), 0600)) - runGit(t, upstream, "add", "second.hcl") - runGit(t, upstream, "commit", "-q", "-m", "second") + // 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") - // The new commit's content must be present... - second, err := os.ReadFile(filepath.Join(finalDest, "second.hcl")) - assert.NoError(t, err) - assert.Equal(t, "description = \"second\"\n", string(second)) - // ...and finalDest must still be a valid, checked-out git repo, not left - // mid-checkout by "git clone --no-checkout". - first, err = os.ReadFile(filepath.Join(finalDest, "first.hcl")) - assert.NoError(t, err) - assert.Equal(t, "description = \"first\"\n", string(first)) info, err := os.Stat(filepath.Join(finalDest, ".git")) assert.NoError(t, err) assert.True(t, info.IsDir()) @@ -159,6 +205,46 @@ func TestSyncGitIncrementalUpdate(t *testing.T) { 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 From 630da06427258a7b76d3bccfaf32569fc5e176f2 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 15:35:41 -0700 Subject: [PATCH 07/15] sources: make incremental checkout robust to a non-empty index The doc comment explaining why detached HEAD was replaced with a named branch relied on "git clone --no-checkout" never writing a ".git/index", which is what actually makes "checkout -B" materialise the worktree. Add "--force" so this doesn't depend on that subtlety: without it, a checkout git considers a no-op would silently leave dest's worktree empty, discarding the manifest tree. --- sources/git.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sources/git.go b/sources/git.go index b0b45dc4..440dc3a8 100644 --- a/sources/git.go +++ b/sources/git.go @@ -243,7 +243,12 @@ func syncGitIncremental(b *ui.Task, dest, source, finalDest string, runner util. if err := runner.RunInDir(b, dest, "git", "fetch", "--depth=1", source, "HEAD"); err != nil { return errors.WithStack(err) } - return errors.WithStack(runner.RunInDir(b, dest, "git", "checkout", "-B", incrementalBranch, "FETCH_HEAD")) + // "--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")) } // removeStaleScratchDirs removes leftover clone/swap scratch directories from From d5c44a2e0a369dc151df5018f21a72ed88a9ec30 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 12:15:44 -0700 Subject: [PATCH 08/15] sources, manifest: distinguish a transiently-missing source from an unknown package Belt to the previous commit's braces, and worth it independently: machines will run mixed Hermit versions for a while, and an older binary sharing a state dir still syncs destructively without taking the new lock. sources.ErrSourceUnavailable is now reported (via a uriFS.dir field and Open override) when a source's entire backing directory is missing, as opposed to the directory existing but simply not containing the requested manifest. The distinction matters: a git source's directory can be transiently absent while another Hermit process is mid-sync, which is not evidence the package doesn't exist. uriFS.dir is left unset for in-memory sources (BuiltInSource/MemSource), since vfs.InMemoryFS unconditionally returns fs.ErrNotExist and would otherwise be misreported as unavailable on every lookup. manifest.Loader.get now keeps searching remaining bundles when one is unavailable rather than bailing out immediately, so one transiently- missing source never masks a package provided by another, healthy source, and only reports ErrSourceUnavailable if the package was found nowhere. Load retries on that specific error with a short bounded backoff (~620ms worst case) before falling back to its existing sync-and-retry, so a genuinely unknown package is never delayed by it. The ErrUnknownPackage message now also enumerates the sources that were searched, which previously gave no indication that a misconfigured or inaccessible source was the real cause. Also fixes errors.Wrap(err, err.Error()) in Load, which duplicated the wrapped error's message. --- manifest/loader.go | 85 +++++++++++++++++++++++++++++++++-------- manifest/loader_test.go | 70 +++++++++++++++++++++++++++++++++ sources/git.go | 1 + sources/sources.go | 32 ++++++++++++++++ 4 files changed, 173 insertions(+), 15 deletions(-) diff --git a/manifest/loader.go b/manifest/loader.go index 9aa8361b..1f15588e 100644 --- a/manifest/loader.go +++ b/manifest/loader.go @@ -65,17 +65,35 @@ 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 } + file = f 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 +101,23 @@ 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(), ", ")) +} + +// sourceUnavailableRetryBackoff bounds how long Load will wait for a +// transiently-unavailable source (see sources.ErrSourceUnavailable, eg. +// another Hermit process mid-sync) to become available again, before +// falling back to the existing sync-and-retry below. Total worst case is +// ~620ms, deliberately short so a genuinely unknown package is never +// delayed by it. +var sourceUnavailableRetryBackoff = []time.Duration{20 * time.Millisecond, 100 * time.Millisecond, 500 * time.Millisecond} + // 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. @@ -90,10 +125,16 @@ func (l *Loader) get(name string) (*AnnotatedManifest, error) { // If any errors occur during the load, the first error will be returned. func (l *Loader) Load(u *ui.UI, name string) (*AnnotatedManifest, error) { mnf, err := l.get(name) + for _, backoff := range sourceUnavailableRetryBackoff { + if !errors.Is(err, sources.ErrSourceUnavailable) { + break + } + time.Sleep(backoff) + mnf, err = l.get(name) + } if err != nil { - err := l.sources.Sync(u, true) - if err != nil { - return nil, errors.Wrap(err, err.Error()) + if err := l.sources.Sync(u, true); err != nil { + return nil, errors.WithStack(err) } // Try again. mnf, err = l.get(name) @@ -171,7 +212,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 +250,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..5d56a139 100644 --- a/manifest/loader_test.go +++ b/manifest/loader_test.go @@ -2,11 +2,14 @@ package manifest import ( "os" + "path/filepath" "testing" "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 +31,70 @@ 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) +} + +// 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/git.go b/sources/git.go index 440dc3a8..3ec817aa 100644 --- a/sources/git.go +++ b/sources/git.go @@ -54,6 +54,7 @@ func NewGitSourceWithLockTimeout(uri, sourceDir string, runner util.CommandRunne path := filepath.Join(sourceDir, key) return &GitSource{&uriFS{ uri: uri, + dir: path, FS: os.DirFS(path), }, sourceDir, path, runner, lockTimeout} } diff --git a/sources/sources.go b/sources/sources.go index 501def7c..c1aa2c9d 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,17 @@ 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. +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 +} From 0946721202434ef0f578aafc224b837df8d1db60 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 13:36:20 -0700 Subject: [PATCH 09/15] manifest, sources: fix retry ordering, avoid caching a shadowed manifest High: Load slept through the full sourceUnavailableRetryBackoff before ever calling Sync, so a source that has simply never been cloned paid ~620ms of pure latency every time before the sync that could actually fix it ran. Sync first, then only fall back to the bounded backoff for the remaining case: a sibling process's concurrent sync of this specific source completing while our own Sync call was a no-op. Medium: get() no longer caches a manifest found in a lower-preference bundle when a higher-preference bundle was unavailable at lookup time. Caching it would let a transient outage permanently invert source precedence for the rest of the process's lifetime; leaving it uncached lets the next lookup retry the unavailable bundle and self-heal once it recovers. TestLoaderFallsBackToHealthySourceWhenAnotherIsUnavailable still passes -- availability-over-precedence fallback still happens per-lookup, it just isn't permanently pinned. Low: document that uriFS.Open's directory-missing check is retrospective and best-effort, not an authoritative point-in-time answer -- it's only ever used as a retry signal. --- manifest/loader.go | 52 +++++++++++++++++++++++++++-------------- manifest/loader_test.go | 36 ++++++++++++++++++++++++++++ sources/sources.go | 8 +++++++ 3 files changed, 79 insertions(+), 17 deletions(-) diff --git a/manifest/loader.go b/manifest/loader.go index 1f15588e..1c05423b 100644 --- a/manifest/loader.go +++ b/manifest/loader.go @@ -82,7 +82,18 @@ func (l *Loader) get(name string) (*AnnotatedManifest, error) { continue } file = f - l.files[name] = file + 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. @@ -110,14 +121,6 @@ func (l *Loader) unknownPackageDetail(name string) string { return fmt.Sprintf("%s (searched %s)", name, strings.Join(l.sources.Sources(), ", ")) } -// sourceUnavailableRetryBackoff bounds how long Load will wait for a -// transiently-unavailable source (see sources.ErrSourceUnavailable, eg. -// another Hermit process mid-sync) to become available again, before -// falling back to the existing sync-and-retry below. Total worst case is -// ~620ms, deliberately short so a genuinely unknown package is never -// delayed by it. -var sourceUnavailableRetryBackoff = []time.Duration{20 * time.Millisecond, 100 * time.Millisecond, 500 * time.Millisecond} - // 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. @@ -125,6 +128,28 @@ var sourceUnavailableRetryBackoff = []time.Duration{20 * time.Millisecond, 100 * // If any errors occur during the load, the first error will be returned. func (l *Loader) Load(u *ui.UI, name string) (*AnnotatedManifest, error) { mnf, err := l.get(name) + if err != nil { + // 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) + } + mnf, err = l.get(name) + } + // 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 @@ -133,14 +158,7 @@ func (l *Loader) Load(u *ui.UI, name string) (*AnnotatedManifest, error) { mnf, err = l.get(name) } if err != nil { - if err := l.sources.Sync(u, true); err != nil { - return nil, errors.WithStack(err) - } - // Try again. - mnf, err = l.get(name) - if err != nil { - return nil, errors.WithStack(err) - } + return nil, errors.WithStack(err) } return mnf, nil } diff --git a/manifest/loader_test.go b/manifest/loader_test.go index 5d56a139..dc1f1be4 100644 --- a/manifest/loader_test.go +++ b/manifest/loader_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/alecthomas/assert/v2" "github.com/cashapp/hermit/errors" @@ -84,6 +85,41 @@ func TestLoaderFallsBackToHealthySourceWhenAnotherIsUnavailable(t *testing.T) { 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 diff --git a/sources/sources.go b/sources/sources.go index c1aa2c9d..30d0ffed 100644 --- a/sources/sources.go +++ b/sources/sources.go @@ -214,6 +214,14 @@ func (u *uriFS) String() string { return u.uri } // 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) { From 692928285fdcc6ee5eac8722356764389362b415 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 14:31:26 -0700 Subject: [PATCH 10/15] env: treat a transiently-unavailable source like an unknown package for fallback The three call sites that fall back to an alternate resolution strategy (a virtual package, a resync-then-retry, a glob-selector search) on manifest.ErrUnknownPackage predate sources.ErrSourceUnavailable, and didn't know about it: a source that's merely unreachable right now silently skipped the same fallback a genuinely-missing package would trigger, even though the alternate strategy may well succeed via a different, healthy source. Broaden all three checks to also match ErrSourceUnavailable. --- env.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/env.go b/env.go index f8317f95..e68c7415 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) } @@ -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 { From 3b319e078f25ba27db4ebc9d49b11a89216d29c6 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 12:16:20 -0700 Subject: [PATCH 11/15] internal/dao, state: atomic writes for two more exec-path races Two more races on the same "hermit exec" hot path, independent of the git source sync issue fixed earlier in this stack. internal/dao.UpdatePackage wrote a package's cached etag with a plain os.WriteFile, which truncates the existing file before writing the new content. A concurrent GetPackage could observe a torn (empty or partial) etag -- not merely cosmetic, since 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 exec'ing. UpdatePackage now writes to a temp file in the same directory and renames it into place, so a reader always sees either the old, complete value or the new one. UpdateCheckedAt is now also stored explicitly instead of inferred from the file's mtime, which a torn write could also disturb; a legacy (pre-JSON-envelope) metadata file written by an older Hermit version still falls back to mtime. state.linkBinaries built its symlink directory with RemoveAll + recreate, which CacheAndUnpack's unlocked pre-lock fast path (areBinariesLinked) can observe mid-rebuild: a caller that sees "already linked" may go on to exec a binary through a directory that gets removed out from under it moments later. It now builds the new set of symlinks in a temporary sibling directory and swaps it into place with util.SwapDir (introduced earlier in this stack for the git source fix), so readers only ever see the complete old or new set. --- internal/dao/dao.go | 69 +++++++++++++++++++-- internal/dao/dao_test.go | 131 +++++++++++++++++++++++++++++++++++++++ state/state.go | 33 ++++++---- 3 files changed, 216 insertions(+), 17 deletions(-) create mode 100644 internal/dao/dao_test.go diff --git a/internal/dao/dao.go b/internal/dao/dao.go index 5c1696fd..c1dc8d74 100644 --- a/internal/dao/dao.go +++ b/internal/dao/dao.go @@ -1,6 +1,7 @@ package dao import ( + "encoding/json" "io" "os" "path/filepath" @@ -35,6 +36,18 @@ func (d *DAO) Dump(w io.Writer) error { return nil } +// metadataFile is the on-disk encoding of Package written by UpdatePackage. +// +// UpdateCheckedAt is stored explicitly, rather than inferred from the file's +// mtime (as earlier versions of Hermit did): 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. +type metadataFile struct { + Etag string `json:"etag"` + UpdateCheckedAt time.Time `json:"update_checked_at"` +} + // GetPackage returns information for a specific package. func (d *DAO) GetPackage(pkgRef string) (*Package, error) { r, err := os.Open(d.metadataPath(pkgRef)) @@ -49,19 +62,65 @@ func (d *DAO) GetPackage(pkgRef string) (*Package, error) { if err != nil { return nil, errors.WithStack(err) } - etag, err := io.ReadAll(r) + data, err := io.ReadAll(r) if err != nil { return nil, errors.WithStack(err) } + var mf metadataFile + if err := json.Unmarshal(data, &mf); err != nil { + // Metadata file written by a Hermit version prior to the + // introduction of this format: it contains only the raw etag, with + // no recorded check time. Fall back to the file's mtime, as + // GetPackage always did previously. + return &Package{ + Etag: string(data), + UpdateCheckedAt: info.ModTime(), + }, nil + } return &Package{ - Etag: string(etag), - UpdateCheckedAt: info.ModTime(), + Etag: mf.Etag, + UpdateCheckedAt: mf.UpdateCheckedAt, }, nil } -// UpdatePackage Updates the update check time, etag, and the used at time for a package +// UpdatePackage updates the update check time, etag, and the used at time for a package. +// +// The write is atomic: 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. func (d *DAO) UpdatePackage(pkgRef string, pkg *Package) error { - return errors.WithStack(os.WriteFile(d.metadataPath(pkgRef), []byte(pkg.Etag), 0600)) + path := d.metadataPath(pkgRef) + checkedAt := pkg.UpdateCheckedAt + if checkedAt.IsZero() { + checkedAt = time.Now() + } + data, err := json.Marshal(metadataFile{Etag: pkg.Etag, UpdateCheckedAt: checkedAt}) + if err != nil { + return errors.WithStack(err) + } + + tmp, err := os.CreateTemp(d.metadataDir, 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) + } + return errors.WithStack(os.Rename(tmpPath, path)) } // DeletePackage removes a package from the DB diff --git a/internal/dao/dao_test.go b/internal/dao/dao_test.go new file mode 100644 index 00000000..894d755f --- /dev/null +++ b/internal/dao/dao_test.go @@ -0,0 +1,131 @@ +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) +} + +// TestGetPackageLegacyFormat verifies that a metadata file written by a +// Hermit version prior to the introduction of the JSON envelope (ie. one +// containing only the raw etag, with no recorded check time) is still read +// correctly, falling back to the file's mtime for UpdateCheckedAt exactly as +// GetPackage always did previously. +func TestGetPackageLegacyFormat(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) +} + +// TestUpdatePackageLeavesNoTempFiles guards against leaking the scratch temp +// file UpdatePackage writes before renaming into place. +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) + for _, entry := range entries { + if strings.Contains(entry.Name(), ".tmp-") { + t.Fatalf("leaked temp file: %s", entry.Name()) + } + } +} diff --git a/state/state.go b/state/state.go index de91e15d..3cd2335a 100644 --- a/state/state.go +++ b/state/state.go @@ -383,34 +383,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 { From ac24e0fd3b45d2b0a86fb38a2e07de43fab5bb74 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 13:38:43 -0700 Subject: [PATCH 12/15] internal/dao, state, env: drop JSON envelope, add shared atomic-write helper High: the etag metadata file's JSON envelope required parsing on every GetPackage, and a torn or short read (concurrent UpdatePackage without this envelope's atomicity) parsed as garbage that UpgradeChannel would treat as a genuine etag change, triggering evictPackage's rm -rf of a package tree that may be actively executing. Drop the envelope entirely: the etag is now the exact raw bytes every Hermit version has always written, stored via the new shared util.AtomicWriteFile; UpdateCheckedAt moves to a separate ".checked" sidecar file so mixed Hermit versions sharing a state directory keep reading and writing the etag identically, with graceful fallback to the etag file's mtime when the sidecar is missing or unparseable. Medium: sweep stale ".tmp-*" scratch files left behind by a killed process (whose deferred cleanup never got to run) out of the metadata directory on DAO Open, bounded by a generous age threshold so a genuinely in-flight write from another process is never touched. Extend the same atomic-write treatment to env.go's SetEnv/DelEnv, and give state.removeRecursive an atomic (rename-aside) removal via the new util.RemoveAllAtomic, matching the reader-visible-window fix util.SwapDir already applies to replacement. Also documents that WritePackageState storing a zero UpdateCheckedAt as "now" (via dao.UpdatePackage) is harmless when UpdateInterval == 0, since EnsureChannelIsUpToDate short-circuits before ever consulting it. --- env.go | 4 +- internal/dao/dao.go | 146 ++++++++++++++++++++++++--------------- internal/dao/dao_test.go | 48 +++++++++++-- state/state.go | 9 ++- util/atomicfile.go | 36 ++++++++++ util/dirswap.go | 28 ++++++++ util/dirswap_test.go | 23 ++++++ 7 files changed, 227 insertions(+), 67 deletions(-) create mode 100644 util/atomicfile.go diff --git a/env.go b/env.go index e68c7415..dd760822 100644 --- a/env.go +++ b/env.go @@ -1223,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. @@ -1233,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. diff --git a/internal/dao/dao.go b/internal/dao/dao.go index c1dc8d74..e4e79b20 100644 --- a/internal/dao/dao.go +++ b/internal/dao/dao.go @@ -1,15 +1,21 @@ package dao import ( - "encoding/json" "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 @@ -28,99 +34,116 @@ 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 } -// metadataFile is the on-disk encoding of Package written by UpdatePackage. -// -// UpdateCheckedAt is stored explicitly, rather than inferred from the file's -// mtime (as earlier versions of Hermit did): 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. -type metadataFile struct { - Etag string `json:"etag"` - UpdateCheckedAt time.Time `json:"update_checked_at"` -} - // 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) } - data, err := io.ReadAll(r) - if err != nil { - return nil, errors.WithStack(err) - } - var mf metadataFile - if err := json.Unmarshal(data, &mf); err != nil { - // Metadata file written by a Hermit version prior to the - // introduction of this format: it contains only the raw etag, with - // no recorded check time. Fall back to the file's mtime, as - // GetPackage always did previously. - return &Package{ - Etag: string(data), - UpdateCheckedAt: info.ModTime(), - }, nil + if checkedAt.IsZero() { + info, err := os.Stat(d.metadataPath(pkgRef)) + if err != nil { + return nil, errors.WithStack(err) + } + checkedAt = info.ModTime() } return &Package{ - Etag: mf.Etag, - UpdateCheckedAt: mf.UpdateCheckedAt, + Etag: string(etag), + 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. // -// The write is atomic: 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 +// 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. func (d *DAO) UpdatePackage(pkgRef string, pkg *Package) error { - path := d.metadataPath(pkgRef) checkedAt := pkg.UpdateCheckedAt if checkedAt.IsZero() { checkedAt = time.Now() } - data, err := json.Marshal(metadataFile{Etag: pkg.Etag, UpdateCheckedAt: checkedAt}) - if err != nil { + if err := util.AtomicWriteFile(d.metadataPath(pkgRef), []byte(pkg.Etag), 0600); err != nil { return errors.WithStack(err) } - - tmp, err := os.CreateTemp(d.metadataDir, 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) - } - return errors.WithStack(os.Rename(tmpPath, path)) + return errors.WithStack(util.AtomicWriteFile(d.checkedAtPath(pkgRef), []byte(checkedAt.Format(time.RFC3339Nano)), 0600)) } // DeletePackage removes a package from the DB @@ -128,9 +151,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 index 894d755f..24c4187b 100644 --- a/internal/dao/dao_test.go +++ b/internal/dao/dao_test.go @@ -37,12 +37,15 @@ func TestUpdateAndGetPackageRoundTrip(t *testing.T) { assert.True(t, checkedAt.Equal(got.UpdateCheckedAt), "expected %s, got %s", checkedAt, got.UpdateCheckedAt) } -// TestGetPackageLegacyFormat verifies that a metadata file written by a -// Hermit version prior to the introduction of the JSON envelope (ie. one -// containing only the raw etag, with no recorded check time) is still read -// correctly, falling back to the file's mtime for UpdateCheckedAt exactly as -// GetPackage always did previously. -func TestGetPackageLegacyFormat(t *testing.T) { +// 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) @@ -112,8 +115,36 @@ func TestUpdatePackageAtomicNoTornRead(t *testing.T) { 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 -// file UpdatePackage writes before renaming into place. +// 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) @@ -123,9 +154,12 @@ func TestUpdatePackageLeavesNoTempFiles(t *testing.T) { 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/state/state.go b/state/state.go index 3cd2335a..8bc958f3 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. diff --git a/util/atomicfile.go b/util/atomicfile.go new file mode 100644 index 00000000..b6f9a040 --- /dev/null +++ b/util/atomicfile.go @@ -0,0 +1,36 @@ +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. +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 index 4b696b43..462d2fbd 100644 --- a/util/dirswap.go +++ b/util/dirswap.go @@ -2,6 +2,7 @@ package util import ( "os" + "path/filepath" "github.com/cashapp/hermit/errors" ) @@ -59,3 +60,30 @@ func SwapDir(src, finalDest string) error { // 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. +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 index 025fd4d2..ab8f4267 100644 --- a/util/dirswap_test.go +++ b/util/dirswap_test.go @@ -72,3 +72,26 @@ func TestSwapDirNoPreviousDest(t *testing.T) { _, 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"))) +} From 8f8c24597cf94921e2ae80f53e3d18fe080c1ef1 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 14:31:58 -0700 Subject: [PATCH 13/15] state, internal/dao, util: durability fixes and doc caveats from review - state.extract: archive.Extract's own deferred rename already publishes p.Dest before EventUnpack's trigger runs, so cleaning it up on trigger failure with plain os.RemoveAll is the same reader-unsafe unlink-storm this stack replaced everywhere else. Use util.RemoveAllAtomic instead, consistent with removeRecursive's existing use of it. - util.AtomicWriteFile: fsync the temp file before renaming it into place. Without this, a crash shortly after a successful-looking write can still leave the renamed-to path pointing at a zero-length or truncated file, since the rename being durable doesn't make the data behind it durable. - util.RemoveAllAtomic: document that, unlike os.RemoveAll, it requires dir's parent to exist (it needs somewhere to create the sibling via MkdirTemp). - internal/dao.UpdatePackage: document that the etag and checked-at sidecar are written via separate renames, so two concurrent UpdatePackage calls for the same package can interleave and pair a stale etag with a fresh checked-at time. Carried over from the single-JSON-file format this replaced, not introduced by the two-file split. --- internal/dao/dao.go | 9 +++++++++ state/state.go | 6 +++++- util/atomicfile.go | 10 ++++++++++ util/dirswap.go | 6 ++++++ 4 files changed, 30 insertions(+), 1 deletion(-) diff --git a/internal/dao/dao.go b/internal/dao/dao.go index e4e79b20..3479b2d9 100644 --- a/internal/dao/dao.go +++ b/internal/dao/dao.go @@ -135,6 +135,15 @@ func (d *DAO) readCheckedAt(pkgRef string) (time.Time, error) { // 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. This is a pre-existing risk carried +// over from the single-JSON-file format this replaced (which had the same +// last-writer-wins exposure across the two logical fields, just within one +// file); it is not introduced by the two-file split. func (d *DAO) UpdatePackage(pkgRef string, pkg *Package) error { checkedAt := pkg.UpdateCheckedAt if checkedAt.IsZero() { diff --git a/state/state.go b/state/state.go index 8bc958f3..5fce28ac 100644 --- a/state/state.go +++ b/state/state.go @@ -462,7 +462,11 @@ func (s *State) extract(b *ui.Task, p *manifest.Package) error { } } if _, err = p.Trigger(b, manifest.EventUnpack); err != nil { - _ = os.RemoveAll(p.Dest) + // p.Dest is already published (archive.Extract renames it into place + // before returning), so an unlocked reader could be looking at it -- + // remove it the same reader-safe way as everywhere else in this + // package rather than deleting it out from under them entry by entry. + _ = util.RemoveAllAtomic(p.Dest) return errors.WithStack(err) } return errors.WithStack(finalise()) diff --git a/util/atomicfile.go b/util/atomicfile.go index b6f9a040..9b3b3104 100644 --- a/util/atomicfile.go +++ b/util/atomicfile.go @@ -22,10 +22,20 @@ func AtomicWriteFile(path string, data []byte, perm os.FileMode) error { defer os.Remove(tmpPath) _, writeErr := tmp.Write(data) + var syncErr error + if writeErr == nil { + // Without this, a crash shortly after Rename can leave path pointing + // at a temp file the filesystem never flushed, ie. a zero-length or + // truncated file, despite the rename itself being durable. + syncErr = tmp.Sync() + } closeErr := tmp.Close() if writeErr != nil { return errors.WithStack(writeErr) } + if syncErr != nil { + return errors.WithStack(syncErr) + } if closeErr != nil { return errors.WithStack(closeErr) } diff --git a/util/dirswap.go b/util/dirswap.go index 462d2fbd..e411f0dc 100644 --- a/util/dirswap.go +++ b/util/dirswap.go @@ -69,6 +69,12 @@ func SwapDir(src, finalDest string) error { // // 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 { From 90158b686016fc3ad79206b64806fea5574607db Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 15:06:45 -0700 Subject: [PATCH 14/15] state, internal/dao, util: revert hot-path fsync, fix leak and doc inaccuracy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - util.AtomicWriteFile: drop the fsync added last round. On macOS, Go's File.Sync issues fcntl(F_FULLFSYNC), which measured ~140x slower than the plain write here (confirmed on this machine: ~4.3ms vs ~30µs/op) -- and this helper runs on Hermit's "exec" hot path via dao.UpdatePackage. The data it protects is a regenerable cache (etag + check timestamp), so losing it to a crash just costs one extra upstream check; that's not worth paying this cost on every invocation. Documented as a deliberate omission. - state.extract: the "copy manifest referred files" loop can also fail after archive.Extract has already published p.Dest, the same condition the previous commit fixed for the EventUnpack trigger a few lines below it -- missed because it wasn't the line called out by review. Now cleaned up with util.RemoveAllAtomic here too, otherwise a retry of the same package is permanently wedged behind archive.Extract's "destination already exists". - internal/dao.UpdatePackage: correct a doc comment claiming the etag/ checked-at interleave risk was "not introduced by the two-file split" -- the single-JSON-file format it replaced wrote both fields in one atomic rename, so this specific mismatched pairing is in fact newly possible. Still benign: worst case is a one-cycle-stale check time that self-corrects. --- internal/dao/dao.go | 10 ++++++---- state/state.go | 12 ++++++++---- util/atomicfile.go | 21 +++++++++++---------- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/internal/dao/dao.go b/internal/dao/dao.go index 3479b2d9..4f9484c2 100644 --- a/internal/dao/dao.go +++ b/internal/dao/dao.go @@ -140,10 +140,12 @@ func (d *DAO) readCheckedAt(pkgRef string) (time.Time, error) { // 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. This is a pre-existing risk carried -// over from the single-JSON-file format this replaced (which had the same -// last-writer-wins exposure across the two logical fields, just within one -// file); it is not introduced by the two-file split. +// etag paired with B's checked-at time. The single-JSON-file format this +// replaced wrote both fields in one atomic rename, 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 { checkedAt := pkg.UpdateCheckedAt if checkedAt.IsZero() { diff --git a/state/state.go b/state/state.go index 5fce28ac..d2ac30b8 100644 --- a/state/state.go +++ b/state/state.go @@ -454,18 +454,22 @@ func (s *State) extract(b *ui.Task, p *manifest.Package) error { if err != nil { return errors.WithStack(err) } + // 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, and archive.Extract itself + // refuses to extract into a p.Dest that already exists -- so any failure + // below must clean it up the same reader-safe way as everywhere else in + // this package, or a retry of this package is permanently wedged behind + // "destination already exists". // 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 { - // p.Dest is already published (archive.Extract renames it into place - // before returning), so an unlocked reader could be looking at it -- - // remove it the same reader-safe way as everywhere else in this - // package rather than deleting it out from under them entry by entry. _ = util.RemoveAllAtomic(p.Dest) return errors.WithStack(err) } diff --git a/util/atomicfile.go b/util/atomicfile.go index 9b3b3104..11fae109 100644 --- a/util/atomicfile.go +++ b/util/atomicfile.go @@ -11,6 +11,17 @@ import ( // 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 -- and this is called from +// internal/dao.UpdatePackage on Hermit's "exec" hot path. The data this +// protects (a cached etag and check timestamp) is not authoritative state: +// losing it to a crash just costs one extra upstream check on the next run, +// which doesn't justify that cost on every invocation. func AtomicWriteFile(path string, data []byte, perm os.FileMode) error { dir := filepath.Dir(path) tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*") @@ -22,20 +33,10 @@ func AtomicWriteFile(path string, data []byte, perm os.FileMode) error { defer os.Remove(tmpPath) _, writeErr := tmp.Write(data) - var syncErr error - if writeErr == nil { - // Without this, a crash shortly after Rename can leave path pointing - // at a temp file the filesystem never flushed, ie. a zero-length or - // truncated file, despite the rename itself being durable. - syncErr = tmp.Sync() - } closeErr := tmp.Close() if writeErr != nil { return errors.WithStack(writeErr) } - if syncErr != nil { - return errors.WithStack(syncErr) - } if closeErr != nil { return errors.WithStack(closeErr) } From de3bb66a4ce5920ceed4b93dc6dc63e823b643ae Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 15:36:34 -0700 Subject: [PATCH 15/15] internal/dao, state, util: correct two doc-comment inaccuracies from review util.AtomicWriteFile's fsync-omission rationale claimed internal/dao.UpdatePackage runs on Hermit's "exec" hot path -- it doesn't; it's gated behind an update-interval check and always follows a network round trip, so the fsync cost was never actually avoided on every invocation. The comment also ignored the helper's other caller, Env.SetEnv/DelEnv, which rewrites the user's bin/hermit.hcl and is the caller for which losing data to a crash is actually consequential. dao.UpdatePackage's comment on its etag/checked-at interleave risk described it as carried over from "the single-JSON-file format this replaced" -- that JSON format never existed on master; it was introduced and removed again within this same PR stack. The real predecessor (a single etag file whose mtime doubled as the checked-at time) still supports the same conclusion -- the mismatched pairing is newly possible with the two-file split -- just not for the reason originally given. Also tightens the state.go comment on the p.Files copy-loop cleanup: the previous wording described a "wedged" retry, but for the common case where a manifest doesn't override "root", a leftover p.Dest instead makes isExtracted see it as already-installed and skip re-extraction on retry entirely, silently omitting the copied files forever -- worse than "wedged", and worth saying so. --- internal/dao/dao.go | 12 ++++++------ state/state.go | 14 ++++++++------ util/atomicfile.go | 13 ++++++++----- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/internal/dao/dao.go b/internal/dao/dao.go index 4f9484c2..37546bbc 100644 --- a/internal/dao/dao.go +++ b/internal/dao/dao.go @@ -140,12 +140,12 @@ func (d *DAO) readCheckedAt(pkgRef string) (time.Time, error) { // 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-JSON-file format this -// replaced wrote both fields in one atomic rename, 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. +// 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 { checkedAt := pkg.UpdateCheckedAt if checkedAt.IsZero() { diff --git a/state/state.go b/state/state.go index d2ac30b8..2a614642 100644 --- a/state/state.go +++ b/state/state.go @@ -456,12 +456,14 @@ func (s *State) extract(b *ui.Task, p *manifest.Package) error { } // 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, and archive.Extract itself - // refuses to extract into a p.Dest that already exists -- so any failure - // below must clean it up the same reader-safe way as everywhere else in - // this package, or a retry of this package is permanently wedged behind - // "destination already exists". - // Copy manifest referred files + // 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 { diff --git a/util/atomicfile.go b/util/atomicfile.go index 11fae109..b785b079 100644 --- a/util/atomicfile.go +++ b/util/atomicfile.go @@ -17,11 +17,14 @@ import ( // 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 -- and this is called from -// internal/dao.UpdatePackage on Hermit's "exec" hot path. The data this -// protects (a cached etag and check timestamp) is not authoritative state: -// losing it to a crash just costs one extra upstream check on the next run, -// which doesn't justify that cost on every invocation. +// 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-*")