From ba65b7561dd6e7aa206010e83834c3a82544a4a4 Mon Sep 17 00:00:00 2001 From: Jason Chiu Date: Mon, 27 Jul 2026 12:12:27 -0700 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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