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..440dc3a8 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,118 @@ import ( "github.com/cashapp/hermit/util" ) +// fsTimeGranularity is the coarsest mtime resolution we expect from the +// filesystems Hermit runs on (eg. HFS+ stores whole seconds), used as slack +// when comparing timestamps taken before and after acquiring the sync lock. +const fsTimeGranularity = time.Second + +// Suffixes/prefixes used for Hermit scratch state living alongside source +// directories. A real source directory name is a bare hex SHA256 hash (see +// util.Hash), so anything containing these is unambiguously scratch state. +const ( + tmpInfix = ".tmp-" // in-progress clone: .tmp-XXXXXXXX + asideSuffix = util.DirSwapAsideSuffix // previous tree, mid-swap, pending deletion + legacyTmpInfix = "-" // clone temp dirs created by older Hermit versions: -XXXXXXXX + staleScratchAge = 24 * time.Hour +) + // GitSource is a new Source based on a git repo type GitSource struct { - fs *uriFS - sourceDir string - path string - runner util.CommandRunner + fs *uriFS + sourceDir string + path string + runner util.CommandRunner + lockTimeout time.Duration } // NewGitSource returns a new GitSource func NewGitSource(uri, sourceDir string, runner util.CommandRunner) *GitSource { + return NewGitSourceWithLockTimeout(uri, sourceDir, runner, DefaultLockTimeout) +} + +// NewGitSourceWithLockTimeout returns a new GitSource with an explicit +// timeout for the lock acquired around synchronisation. This is the +// underlying constructor NewGitSource itself uses to apply +// DefaultLockTimeout; tests use it directly to exercise timeout/contention +// behaviour without waiting out the real default. +// +// A timeout <= 0 is treated as DefaultLockTimeout. +func NewGitSourceWithLockTimeout(uri, sourceDir string, runner util.CommandRunner, lockTimeout time.Duration) *GitSource { key := util.Hash(uri) path := filepath.Join(sourceDir, key) return &GitSource{&uriFS{ uri: uri, FS: os.DirFS(path), - }, sourceDir, path, runner} + }, sourceDir, path, runner, lockTimeout} } func (s *GitSource) Sync(p *ui.UI, force bool) (bool, error) { - info, _ := os.Stat(s.path) task := p.Task(s.fs.uri) - if info == nil || force || time.Since(info.ModTime()) >= SyncFrequency { - err := s.ensureSourcesDirExists() - if err != nil { - return false, errors.WithStack(err) - } - err = syncGit(task, s.sourceDir, s.fs.uri, s.path, s.runner) - // If the sync failed while the repo had already been cloned, log a warning - // If the repo has not yet been cloned, fail. - if err != nil { - if info != nil { - task.Warnf("git sync failed: %s", err) - return false, nil - } - return false, errors.Wrap(err, "git sync failed") + info, _ := os.Stat(s.path) + if info != nil && !force && time.Since(info.ModTime()) < SyncFrequency { + task.Debugf("Update skipped, updated within the last %s", SyncFrequency) + return false, nil + } + + if err := s.ensureSourcesDirExists(); err != nil { + return false, errors.WithStack(err) + } + + // Note the time *before* we start waiting for the lock: if, once we hold + // it, the directory's mtime is at or after this instant, another process + // finished synchronising it while we were waiting, and there is nothing + // left for us to do. + requestedAt := time.Now() + release, err := acquireSyncLock(task, s.path, s.lockTimeout, fmt.Sprintf("synchronising source %s", s.fs.uri)) + if err != nil { + if info != nil { + // We already have a (possibly stale) usable copy. Don't fail the + // command just because we couldn't get exclusive access to + // refresh it. + task.Warnf("could not lock source for syncing, using existing copy: %s", err) + return false, nil } + return false, errors.Wrap(err, "failed to sync sources") + } + defer release() //nolint:errcheck + + // Double-checked locking: re-stat now that we hold the lock. + postLockInfo, _ := os.Stat(s.path) + if syncedSince(postLockInfo, requestedAt) { + task.Debugf("Update skipped, synchronised by another process") return true, nil } - task.Debugf("Update skipped, updated within the last %s", SyncFrequency) - return false, nil + + err = syncGit(task, s.sourceDir, s.fs.uri, s.path, s.runner) + if err != nil { + // If the sync failed while the repo had already been cloned (using + // the up to date, post-lock information), log a warning. If the repo + // has not yet been cloned, fail. + if postLockInfo != nil { + task.Warnf("git sync failed: %s", err) + return false, nil + } + return false, errors.Wrap(err, "git sync failed") + } + return true, nil +} + +// syncedSince reports whether "info" (the result of stat-ing a source +// directory) shows it was successfully synced at or after "since", allowing +// fsTimeGranularity of slack for filesystems that only store whole-second +// mtimes (eg. HFS+). +// +// That slack means syncedSince can report true for a sync that was actually +// requested up to fsTimeGranularity *after* the directory's real mtime -- +// ie. Sync's double-checked-locking skip ("synchronised by another process") +// can fire even though the peer's sync, strictly, finished a moment before +// we asked. This is intentional and safe: skipping in that narrow window +// just means we use a copy that's at most fsTimeGranularity staler than the +// most pedantically-correct answer, which SyncFrequency-bounded staleness +// already tolerates far more of. +func syncedSince(info os.FileInfo, since time.Time) bool { + return info != nil && !info.ModTime().Add(fsTimeGranularity).Before(since) } func (s *GitSource) URI() string { @@ -69,7 +142,55 @@ func (s *GitSource) ensureSourcesDirExists() error { return nil } -// Atomically clone git repo. +// incrementalBranch is the local branch syncGit's incremental-update path +// resets and checks out on every sync, instead of leaving finalDest in a +// detached-HEAD state. This is load-bearing, not cosmetic: "git clone" +// (without "--no-checkout" or "--depth") only copies a source's +// "refs/heads/*", not a detached HEAD, so a detached finalDest would give the +// next incremental sync's local clone of it zero branches to send as "have"s +// during the following "git fetch --depth=1". Without a "have", the server +// can't tell what the client already has, and sends a full pack for the +// requested commit -- silently degrading every sync after the first into the +// same "full fresh clone" cost this path exists to avoid, and (verified +// empirically) totally losing the branch by the second incremental sync. +// Keeping a real, persistent branch ref here means every later sync's local +// clone inherits it, so its fetch always has a "have" to negotiate against. +const incrementalBranch = "hermit" + +// Atomically clone (or, if finalDest is already a clone, incrementally +// update) a git repo. +// +// There is deliberately no in-place "git pull" path: readers in other Hermit +// processes do not take the sync lock, so mutating finalDest's working tree +// in place (as "git pull" does -- updating and deleting files directly +// under it) is visible to them mid-update, and two concurrent "git pull"s +// against the same working tree can also collide with each other (eg. on +// ".git/index.lock"). Always building the new tree in a fresh directory and +// swapping it in atomically (see util.SwapDir) avoids both problems. +// +// When finalDest already has a ".git" directory, the new tree is built +// incrementally to keep the network cost close to a pull's: first a local, +// working-tree-less clone of finalDest (a same-filesystem copy, not a +// network operation), then a shallow fetch of just the latest commit from +// the real source into it, then a checkout of that commit onto +// incrementalBranch (see its doc comment for why a real branch, not a +// detached HEAD, is required for this to actually stay cheap on repeat +// syncs). A plain "--reference-if-able" clone from source was tried here +// first and discarded: finalDest is always itself a shallow (--depth=1) +// clone, and git unconditionally refuses to use a shallow repository as a +// reference, so that flag was silently a no-op and every sync was paying for +// a full fresh clone. This incremental path was verified against the real +// default source (632 manifests): ~0.9s versus ~3.3s for a fresh clone, +// close to the ~0.7s a "git pull" on an already-current clone takes -- and, +// separately, verified to stay that cheap across repeated syncs (not just +// the first one) once incrementalBranch was introduced. +// +// If the incremental update fails (eg. finalDest's ".git" is corrupt or +// truncated), this falls back to a fresh clone rather than surfacing the +// failure, so a damaged existing copy can still self-heal the way a from- +// scratch sync always could. +// +// The caller MUST hold the sync lock for finalDest. func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunner) (err error) { task := b.SubProgress("sync", 1) defer func() { @@ -79,29 +200,87 @@ func syncGit(b *ui.Task, dir, source, finalDest string, runner util.CommandRunne err = errors.WithStack(os.Chtimes(finalDest, now, now)) } }() - // First, if a git repo exists, just pull. - info, _ := os.Stat(filepath.Join(finalDest, ".git")) - if info != nil { - err = runner.RunInDir(b, finalDest, "git", "pull") - if err == nil { - return nil - } - // If pull fails, assume the repo is corrupted and just try and re-clone it. - } - // No git repo, clone down to temporary directory. - dest, err := os.MkdirTemp(dir, filepath.Base(finalDest)+"-*") + + removeStaleScratchDirs(b, dir, finalDest) + + dest, err := os.MkdirTemp(dir, filepath.Base(finalDest)+tmpInfix+"*") if err != nil { return errors.WithStack(err) } defer os.RemoveAll(dest) - if err = runner.RunInDir(b, dest, "git", "clone", "--depth=1", source, dest); err != nil { + + freshClone := true + if info, _ := os.Stat(filepath.Join(finalDest, ".git")); info != nil { + if incErr := syncGitIncremental(b, dest, source, finalDest, runner); incErr == nil { + freshClone = false + } else { + b.Warnf("incremental sync from existing clone failed, falling back to a fresh clone: %s", incErr) + if err = os.RemoveAll(dest); err != nil { + return errors.WithStack(err) + } + if err = os.Mkdir(dest, 0700); err != nil { + return errors.WithStack(err) + } + } + } + if freshClone { + if err = runner.RunInDir(b, dest, "git", "clone", "--depth=1", source, dest); err != nil { + return errors.WithStack(err) + } + } + return errors.WithStack(util.SwapDir(dest, finalDest)) +} + +// syncGitIncremental builds an updated tree at dest by cloning finalDest +// locally (same-filesystem, not a network operation) and fetching just the +// latest commit from the real source into it. See syncGit's doc comment for +// why the result is checked out onto incrementalBranch rather than left +// detached. +func syncGitIncremental(b *ui.Task, dest, source, finalDest string, runner util.CommandRunner) error { + if err := runner.RunInDir(b, dest, "git", "clone", "--no-checkout", finalDest, dest); err != nil { return errors.WithStack(err) } - _ = os.RemoveAll(finalDest) - // And finally, rename it into place. - if err = os.Rename(dest, finalDest); err != nil && !os.IsExist(err) { // Prevent races. + if err := runner.RunInDir(b, dest, "git", "fetch", "--depth=1", source, "HEAD"); err != nil { return errors.WithStack(err) } + // "--force" makes materialising the worktree here not depend on dest + // having no ".git/index" (true today, since "clone --no-checkout" writes + // none) -- without it, a checkout that git considers a no-op change + // writes nothing, silently leaving dest's worktree empty were that ever + // no longer the case. + return errors.WithStack(runner.RunInDir(b, dest, "git", "checkout", "--force", "-B", incrementalBranch, "FETCH_HEAD")) +} - return nil +// removeStaleScratchDirs removes leftover clone/swap scratch directories from +// Hermit processes that were killed mid-sync (eg. SIGKILL, which the +// "defer os.RemoveAll" in syncGit cannot run for), including the "-XXXX" +// form used by Hermit versions prior to the introduction of source locking. +// +// The caller MUST hold the sync lock for finalDest. This is best-effort; +// errors are ignored. +func removeStaleScratchDirs(log ui.Logger, dir, finalDest string) { + base := filepath.Base(finalDest) + entries, err := os.ReadDir(dir) + if err != nil { + return + } + for _, entry := range entries { + name := entry.Name() + if name == base || strings.HasSuffix(name, lockSuffix) { + continue + } + if !strings.HasPrefix(name, base+tmpInfix) && + name != base+asideSuffix && + !strings.HasPrefix(name, base+legacyTmpInfix) { + continue + } + info, err := entry.Info() + if err != nil || time.Since(info.ModTime()) < staleScratchAge { + // Generous age threshold: an older, unlocked Hermit binary may + // still be actively cloning into one of these. + continue + } + log.Debugf("removing stale source scratch directory %s", name) + _ = os.RemoveAll(filepath.Join(dir, name)) + } } diff --git a/sources/git_concurrency_test.go b/sources/git_concurrency_test.go new file mode 100644 index 00000000..ba21cab5 --- /dev/null +++ b/sources/git_concurrency_test.go @@ -0,0 +1,363 @@ +package sources_test + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/alecthomas/assert/v2" + + "github.com/cashapp/hermit/sources" + "github.com/cashapp/hermit/ui" + "github.com/cashapp/hermit/util" +) + +// slowCloningGit is a fake util.CommandRunner that behaves like a real "git" +// binary just enough to exercise GitSource.Sync: "clone" sleeps for a bit +// (simulating a slow network fetch) before writing a manifest and a ".git" +// marker into dest, and "pull" is an instant no-op success. Every clone +// appends a line to a shared log file, so tests can assert exactly one clone +// happened even under concurrent Sync calls. +type slowCloningGit struct { + cloneDelay time.Duration + cloneLog string + + mu sync.Mutex + logHandle *os.File +} + +func newSlowCloningGit(cloneDelay time.Duration, cloneLog string) *slowCloningGit { + return &slowCloningGit{cloneDelay: cloneDelay, cloneLog: cloneLog} +} + +func (g *slowCloningGit) RunInDir(_ *ui.Task, dir string, args ...string) error { + if len(args) < 2 || args[0] != "git" { + return fmt.Errorf("unexpected command: %v", args) + } + switch args[1] { + case "pull": + return nil + case "clone": + time.Sleep(g.cloneDelay) + if err := os.MkdirAll(filepath.Join(dir, ".git"), 0700); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, ".git", "HEAD"), []byte("ref: refs/heads/master\n"), 0600); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, "pkg.hcl"), []byte("description = \"test\"\n"), 0600); err != nil { + return err + } + return g.appendLog(dir) + default: + return fmt.Errorf("unexpected git subcommand: %v", args) + } +} + +func (g *slowCloningGit) appendLog(dir string) error { + g.mu.Lock() + defer g.mu.Unlock() + if g.logHandle == nil { + f, err := os.OpenFile(g.cloneLog, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) + if err != nil { + return err + } + g.logHandle = f + } + _, err := fmt.Fprintf(g.logHandle, "%s\n", dir) + return err +} + +// pollForVanishAfterAppearing polls path until stop is closed, and reports +// an error on errCh if it ever observes path disappear after having +// previously observed it exist. This is the direct reproduction of the +// reported symptom: a reader elsewhere in the system seeing a manifest it +// already found go missing mid-sync. +func pollForVanishAfterAppearing(stop <-chan struct{}, path string, errCh chan<- error) { + var sawIt bool + for { + select { + case <-stop: + errCh <- nil + return + default: + } + _, err := os.ReadFile(path) + switch { + case err == nil: + sawIt = true + case os.IsNotExist(err): + if sawIt { + errCh <- fmt.Errorf("manifest disappeared after first appearing: %s", path) + return + } + default: + errCh <- err + return + } + time.Sleep(200 * time.Microsecond) + } +} + +// waitForFile polls for path to exist, failing the test if timeout elapses +// first. Used to synchronise on a real cross-process event (e.g. "the child +// has acquired the lock") without a fixed sleep, which would either race or +// needlessly slow the test down. +func waitForFile(t *testing.T, path string, timeout time.Duration) { + t.Helper() + deadline := time.Now().Add(timeout) + for { + if _, err := os.Stat(path); err == nil { + return + } + if time.Now().After(deadline) { + t.Fatalf("timed out after %s waiting for %s to appear", timeout, path) + } + time.Sleep(time.Millisecond) + } +} + +// assertOneClone asserts exactly one clone occurred, ie. there was no +// thundering herd of redundant clones once the lock and double-checked +// locking are in place. +func assertOneClone(t *testing.T, cloneLog string) { + t.Helper() + data, err := os.ReadFile(cloneLog) + assert.NoError(t, err) + lines := strings.TrimSpace(string(data)) + if lines == "" { + t.Fatalf("expected exactly one clone, got none") + } + got := strings.Split(lines, "\n") + assert.Equal(t, 1, len(got)) +} + +// assertNoScratchDirs asserts no leaked ".tmp-*"/"*.old" scratch directories +// remain in sourceDir after all syncs have completed. +func assertNoScratchDirs(t *testing.T, sourceDir string) { + t.Helper() + entries, err := os.ReadDir(sourceDir) + assert.NoError(t, err) + for _, entry := range entries { + name := entry.Name() + if strings.Contains(name, ".tmp-") || strings.HasSuffix(name, ".old") { + t.Fatalf("leaked scratch directory: %s", name) + } + } +} + +// TestConcurrentSyncInProcess reproduces N goroutines racing to Sync the +// same not-yet-cloned source, each building its own *GitSource sharing one +// sourceDir and URI -- mirroring how each Hermit invocation constructs its +// own Sources from scratch. A background reader polls the manifest file +// GitSource.Sync writes and fails the test if it ever sees the manifest +// vanish after having first seen it exist. +// +// This exercises whatever process-local synchronisation sources/lock.go adds +// (hence -race), but not flock's cross-process behaviour -- see +// TestConcurrentSyncAcrossProcesses for that. On an unfixed tree, this test +// is not guaranteed to reproduce the race on every run, since all n +// goroutines racing through Sync at once is a timing-dependent condition, +// not a certainty -- see the start barrier below, which maximises the odds +// by holding every goroutine at the gate until all are spawned. +func TestConcurrentSyncInProcess(t *testing.T) { + const n = 8 + sourceDir := t.TempDir() + cloneLog := filepath.Join(t.TempDir(), "clones.log") + uri := "git://concurrent-test" + runner := newSlowCloningGit(50*time.Millisecond, cloneLog) + manifestPath := filepath.Join(sourceDir, util.Hash(uri), "pkg.hcl") + + stop := make(chan struct{}) + readerErr := make(chan error, 1) + go pollForVanishAfterAppearing(stop, manifestPath, readerErr) + + var ready sync.WaitGroup + start := make(chan struct{}) + var wg sync.WaitGroup + ready.Add(n) + for range n { + wg.Add(1) + go func() { + defer wg.Done() + ready.Done() + <-start + u, _ := ui.NewForTesting() + source := sources.NewGitSource(uri, sourceDir, runner) + _, err := source.Sync(u, true) + assert.NoError(t, err) + }() + } + ready.Wait() + close(start) + wg.Wait() + close(stop) + assert.NoError(t, <-readerErr) + + _, err := os.Stat(manifestPath) + assert.NoError(t, err) + assertOneClone(t, cloneLog) + assertNoScratchDirs(t, sourceDir) +} + +// TestConcurrentSyncAcrossProcesses is the reproducer for the reported bug: +// N genuinely separate processes race to Sync the same not-yet-cloned +// source. util/flock is deliberately re-entrant per-PID (a lock file +// recording our own PID is treated as already held), so this race can only +// be reproduced -- and the fix only validated -- across real processes, not +// goroutines sharing a PID. +// +// Each child is a re-exec of this same test binary running +// TestSyncChildProcess, guarded by HERMIT_TEST_CHILD so it no-ops otherwise. +func TestConcurrentSyncAcrossProcesses(t *testing.T) { + if testing.Short() { + t.Skip("spawns subprocesses; skipped in -short") + } + const n = 8 + sourceDir := t.TempDir() + cloneLog := filepath.Join(t.TempDir(), "clones.log") + readyDir := t.TempDir() + goFile := filepath.Join(t.TempDir(), "go") + uri := "git://concurrent-cross-process-test" + manifestPath := filepath.Join(sourceDir, util.Hash(uri), "pkg.hcl") + + stop := make(chan struct{}) + readerErr := make(chan error, 1) + go pollForVanishAfterAppearing(stop, manifestPath, readerErr) + + var wg sync.WaitGroup + errs := make([]error, n) + for i := range n { + wg.Add(1) + go func(i int) { + defer wg.Done() + readyFile := filepath.Join(readyDir, fmt.Sprintf("%d", i)) + cmd := exec.Command(os.Args[0], "-test.run=TestSyncChildProcess", "-test.v") + cmd.Env = append(os.Environ(), + "HERMIT_TEST_CHILD=1", + "HERMIT_TEST_SOURCE_URI="+uri, + "HERMIT_TEST_SOURCE_DIR="+sourceDir, + "HERMIT_TEST_CLONE_LOG="+cloneLog, + "HERMIT_TEST_CLONE_DELAY=100ms", + "HERMIT_TEST_READY_FILE="+readyFile, + "HERMIT_TEST_GO_FILE="+goFile, + ) + out, err := cmd.CombinedOutput() + if err != nil { + errs[i] = fmt.Errorf("child %d failed: %w\n%s", i, err, out) + } + }(i) + } + + // Hold every child at the gate (each blocked on its own readyFile + // existing, then waiting on goFile) until all n have signalled ready, + // then release them all at once -- this maximises the odds that all n + // children race through Sync concurrently, same as the in-process + // start barrier above. + for i := range n { + waitForFile(t, filepath.Join(readyDir, fmt.Sprintf("%d", i)), 30*time.Second) + } + assert.NoError(t, os.WriteFile(goFile, nil, 0600)) + + wg.Wait() + close(stop) + assert.NoError(t, <-readerErr) + + for _, e := range errs { + assert.NoError(t, e) + } + assertOneClone(t, cloneLog) + assertNoScratchDirs(t, sourceDir) +} + +// TestSyncChildProcess is not a real test: it's the worker spawned by +// TestConcurrentSyncAcrossProcesses via re-exec, guarded by an env var so it +// no-ops under a normal test run. +func TestSyncChildProcess(t *testing.T) { + if os.Getenv("HERMIT_TEST_CHILD") == "" { + t.Skip("only runs as a spawned child of TestConcurrentSyncAcrossProcesses") + } + uri := os.Getenv("HERMIT_TEST_SOURCE_URI") + sourceDir := os.Getenv("HERMIT_TEST_SOURCE_DIR") + cloneLog := os.Getenv("HERMIT_TEST_CLONE_LOG") + delay, err := time.ParseDuration(os.Getenv("HERMIT_TEST_CLONE_DELAY")) + if err != nil { + t.Fatalf("bad clone delay: %s", err) + } + + // Signal the parent we're up, then wait for its go-ahead: this holds all + // n children at the gate so they race through Sync together, rather than + // however staggered process spawn happens to make them. + if readyFile := os.Getenv("HERMIT_TEST_READY_FILE"); readyFile != "" { + if err := os.WriteFile(readyFile, nil, 0600); err != nil { + t.Fatalf("failed to write ready file: %s", err) + } + waitForFile(t, os.Getenv("HERMIT_TEST_GO_FILE"), 30*time.Second) + } + + runner := newSlowCloningGit(delay, cloneLog) + source := sources.NewGitSource(uri, sourceDir, runner) + u, _ := ui.NewForTesting() + if _, err := source.Sync(u, true); err != nil { + t.Fatalf("sync failed: %s", err) + } +} + +// TestSyncLockTimeoutFallsBackToExistingCopy verifies that, when a source +// already has a usable copy on disk but the sync lock can't be acquired +// within the configured timeout, Sync degrades to using the existing copy +// rather than failing outright. Lock contention is exercised with a genuine +// separate process (TestHoldSourceLockChildProcess), for the same +// per-PID-reentrancy reason as the cross-process clone test above. +func TestSyncLockTimeoutFallsBackToExistingCopy(t *testing.T) { + if testing.Short() { + t.Skip("spawns a subprocess; skipped in -short") + } + sourceDir := t.TempDir() + uri := "git://lock-timeout-test" + runner := newSlowCloningGit(0, filepath.Join(t.TempDir(), "clones.log")) + source := sources.NewGitSource(uri, sourceDir, runner) + + // Populate an initial copy so Sync has an existing tree to fall back to. + u, _ := ui.NewForTesting() + _, err := source.Sync(u, true) + assert.NoError(t, err) + + path := filepath.Join(sourceDir, util.Hash(uri)) + readyFile := filepath.Join(t.TempDir(), "lock-held") + holdFor := 500 * time.Millisecond + cmd := exec.Command(os.Args[0], "-test.run=TestHoldSourceLockChildProcess", "-test.v") + cmd.Env = append(os.Environ(), + "HERMIT_TEST_CHILD=1", + "HERMIT_TEST_LOCK_PATH="+path, + "HERMIT_TEST_LOCK_HOLD="+holdFor.String(), + "HERMIT_TEST_LOCK_READY_FILE="+readyFile, + ) + assert.NoError(t, cmd.Start()) + // Guarantee the child is reaped even if an assertion below fails the + // test early (assert.* calls t.Fatalf, which skips the cmd.Wait() at + // the end of this function): an orphaned child would otherwise keep + // holding the lock file open for the rest of holdFor. + t.Cleanup(func() { + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) + + // Wait for the child to actually confirm it holds the lock, rather than + // guessing how long that takes: a fixed sleep here would be flaky on a + // slow/loaded machine (racing shortTimeoutSource.Sync below before the + // child has the lock at all) and wastes time everywhere else. + waitForFile(t, readyFile, 30*time.Second) + + shortTimeoutSource := sources.NewGitSourceWithLockTimeout(uri, sourceDir, runner, 10*time.Millisecond) + did, err := shortTimeoutSource.Sync(u, true) + assert.NoError(t, err) + assert.False(t, did, "should have skipped syncing and fallen back to the existing copy") + + assert.NoError(t, cmd.Wait()) +} diff --git a/sources/git_internal_test.go b/sources/git_internal_test.go new file mode 100644 index 00000000..85b744c1 --- /dev/null +++ b/sources/git_internal_test.go @@ -0,0 +1,278 @@ +package sources + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/alecthomas/assert/v2" + + "github.com/cashapp/hermit/ui" + "github.com/cashapp/hermit/util" +) + +func statWithModTime(t *testing.T, modTime time.Time) os.FileInfo { + t.Helper() + path := filepath.Join(t.TempDir(), "f") + assert.NoError(t, os.WriteFile(path, nil, 0600)) + assert.NoError(t, os.Chtimes(path, modTime, modTime)) + info, err := os.Stat(path) + assert.NoError(t, err) + return info +} + +func TestSyncedSince(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + info os.FileInfo + want bool + }{ + {"no directory", nil, false}, + {"synced well before", statWithModTime(t, now.Add(-2*time.Second)), false}, + {"synced exactly at instant", statWithModTime(t, now), true}, + {"synced within filesystem granularity slack", statWithModTime(t, now.Add(-500*time.Millisecond)), true}, + {"synced after instant", statWithModTime(t, now.Add(time.Second)), true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.want, syncedSince(test.info, now)) + }) + } +} + +// TestRemoveStaleScratchDirs verifies the age-gated cleanup sweep only +// touches Hermit's own scratch-directory naming conventions, and only once +// they're old enough that another (older, lock-unaware) Hermit process is +// unlikely to still be using them. +func TestRemoveStaleScratchDirs(t *testing.T) { + dir := t.TempDir() + finalDest := filepath.Join(dir, "abc123") + assert.NoError(t, os.MkdirAll(finalDest, 0700)) + + touch := func(name string, age time.Duration) { + p := filepath.Join(dir, name) + assert.NoError(t, os.MkdirAll(p, 0700)) + mt := time.Now().Add(-age) + assert.NoError(t, os.Chtimes(p, mt, mt)) + } + touch("abc123.tmp-old", 48*time.Hour) // stale clone scratch: removed + touch("abc123.tmp-new", time.Minute) // fresh clone scratch: may be in use, kept + touch("abc123.old", 48*time.Hour) // stale swap-aside: removed + touch("abc123-legacy", 48*time.Hour) // pre-lock-era clone scratch: removed + touch("abc123.lock", 48*time.Hour) // lock file: always kept, regardless of age + touch("def456", 48*time.Hour) // unrelated real source dir: untouched + + u, _ := ui.NewForTesting() + removeStaleScratchDirs(u, dir, finalDest) + + assertExists := func(name string, want bool) { + t.Helper() + _, err := os.Stat(filepath.Join(dir, name)) + if want { + assert.NoError(t, err, name) + } else { + assert.True(t, os.IsNotExist(err), name) + } + } + assertExists("abc123", true) + assertExists("abc123.tmp-new", true) + assertExists("abc123.lock", true) + assertExists("def456", true) + assertExists("abc123.tmp-old", false) + assertExists("abc123.old", false) + assertExists("abc123-legacy", false) +} + +// runGit runs a real "git" command in dir, failing the test on error. Used to +// build and update the upstream repo that syncGit clones/fetches from -- +// separate from the util.CommandRunner under test, which is what syncGit +// itself uses to clone/fetch/checkout. +func runGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + assert.NoError(t, err, "git %v: %s", args, out) +} + +// gitEnvIsolated points HOME, XDG_CONFIG_HOME and the global/system gitconfig +// locations at throwaway paths for the duration of t, so a hook, alias or +// setting in the machine running the test (eg. commit.gpgsign, a +// core.hooksPath) can't affect a test that only cares about plumbing +// commands. +func gitEnvIsolated(t *testing.T) { + t.Helper() + empty := t.TempDir() + t.Setenv("HOME", empty) + t.Setenv("XDG_CONFIG_HOME", empty) + t.Setenv("GIT_CONFIG_GLOBAL", filepath.Join(empty, "gitconfig-does-not-exist")) + t.Setenv("GIT_CONFIG_SYSTEM", filepath.Join(empty, "gitconfig-does-not-exist")) +} + +// TestSyncGitIncrementalUpdate exercises syncGit's incremental-update branch +// (taken once finalDest is already a clone) against a real "git" binary. This +// replaced a "--reference-if-able" clone that turned out to be a silent +// no-op against Hermit's own shallow (--depth=1) clones -- no existing test +// drove the actual clone/fetch/checkout mechanism at all, which is how it +// shipped broken. syncGit is called directly (rather than via GitSource.Sync) +// so this doesn't depend on the wall-clock/mtime-granularity slack in +// syncedSince's double-checked-locking skip. +// +// The upstream repo is addressed via a "file://" URL rather than a bare local +// path: git silently ignores "--depth" for a local-filesystem path ("--depth +// is ignored in local clones"), which would make finalDest never actually +// shallow and defeat the point of this test -- "file://" forces the real +// smart-transport, shallow-fetch code path, the same one used against a real +// remote like the default hermit-packages source. +func TestSyncGitIncrementalUpdate(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + gitEnvIsolated(t) + + upstreamDir := t.TempDir() + upstream := "file://" + upstreamDir + runGit(t, upstreamDir, "init", "-q", "-b", "main") + runGit(t, upstreamDir, "config", "user.email", "test@example.com") + runGit(t, upstreamDir, "config", "user.name", "Test") + assert.NoError(t, os.WriteFile(filepath.Join(upstreamDir, "first.hcl"), []byte("description = \"first\"\n"), 0600)) + runGit(t, upstreamDir, "add", "first.hcl") + runGit(t, upstreamDir, "commit", "-q", "-m", "first") + + dir := t.TempDir() + finalDest := filepath.Join(dir, "abc123") + u, _ := ui.NewForTesting() + runner := &util.RealCommandRunner{} + + // Initial clone: finalDest has no ".git" yet, so this takes the + // fresh-clone branch. + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + first, err := os.ReadFile(filepath.Join(finalDest, "first.hcl")) + assert.NoError(t, err) + assert.Equal(t, "description = \"first\"\n", string(first)) + + // Sync several more times, each adding a new commit upstream: finalDest + // now has a ".git", so every one of these takes the incremental-update + // branch. Repeating this (rather than syncing just once more) is what + // actually exercises incrementalBranch's persistence -- a version of this + // path that left finalDest with a detached HEAD passed a single-sync + // version of this test, but silently degraded into a full-cost clone + // starting from the second incremental sync. + for i := 2; i <= 4; i++ { + name := fmt.Sprintf("commit%d.hcl", i) + content := fmt.Sprintf("description = \"commit %d\"\n", i) + assert.NoError(t, os.WriteFile(filepath.Join(upstreamDir, name), []byte(content), 0600)) + runGit(t, upstreamDir, "add", name) + runGit(t, upstreamDir, "commit", "-q", "-m", fmt.Sprintf("commit %d", i)) + + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + + got, err := os.ReadFile(filepath.Join(finalDest, name)) + assert.NoError(t, err, "sync %d", i) + assert.Equal(t, content, string(got), "sync %d", i) + + // incrementalBranch must persist as a real ref across syncs -- if + // this is ever a detached HEAD instead, the *next* sync's local + // clone of finalDest has no branch to send as a "have", and its + // fetch silently regresses into transferring a full pack. + branch, err := exec.Command("git", "-C", finalDest, "branch", "--show-current").CombinedOutput() + assert.NoError(t, err, "sync %d: %s", i, branch) + assert.Equal(t, incrementalBranch, strings.TrimSpace(string(branch)), "sync %d", i) + } + + // Deleting a file upstream must propagate too: "checkout -B" replaces the + // whole tree, it doesn't merge, so this would fail if the incremental + // path ever left a stale copy of a removed file behind. + runGit(t, upstreamDir, "rm", "-q", "first.hcl") + runGit(t, upstreamDir, "commit", "-q", "-m", "remove first.hcl") + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + _, err = os.Stat(filepath.Join(finalDest, "first.hcl")) + assert.True(t, os.IsNotExist(err), "first.hcl should have been removed by the incremental checkout") + + info, err := os.Stat(filepath.Join(finalDest, ".git")) + assert.NoError(t, err) + assert.True(t, info.IsDir()) + + // No leftover clone/swap scratch directories. + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + assert.Equal(t, 1, len(entries), "expected only finalDest, got %v", entries) +} + +// TestSyncGitIncrementalUpdateFallsBackOnCorruptClone verifies that a +// corrupt/truncated finalDest ".git" (eg. from an interrupted earlier write, +// or a filesystem issue) doesn't wedge every future sync: syncGit should +// notice the incremental path failed and fall back to a fresh clone, the way +// a from-scratch sync always could, rather than leaving finalDest stuck with +// a warning on every subsequent command. +func TestSyncGitIncrementalUpdateFallsBackOnCorruptClone(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + gitEnvIsolated(t) + + upstreamDir := t.TempDir() + upstream := "file://" + upstreamDir + runGit(t, upstreamDir, "init", "-q", "-b", "main") + runGit(t, upstreamDir, "config", "user.email", "test@example.com") + runGit(t, upstreamDir, "config", "user.name", "Test") + assert.NoError(t, os.WriteFile(filepath.Join(upstreamDir, "first.hcl"), []byte("description = \"first\"\n"), 0600)) + runGit(t, upstreamDir, "add", "first.hcl") + runGit(t, upstreamDir, "commit", "-q", "-m", "first") + + dir := t.TempDir() + finalDest := filepath.Join(dir, "abc123") + // A finalDest with a ".git" directory that isn't actually a valid repo: + // takes the incremental branch, and "git clone --no-checkout finalDest + // dest" must fail against it. + assert.NoError(t, os.MkdirAll(filepath.Join(finalDest, ".git"), 0700)) + assert.NoError(t, os.WriteFile(filepath.Join(finalDest, "stale.hcl"), []byte("stale"), 0600)) + + u, _ := ui.NewForTesting() + runner := &util.RealCommandRunner{} + assert.NoError(t, syncGit(u.Task("test"), dir, upstream, finalDest, runner)) + + first, err := os.ReadFile(filepath.Join(finalDest, "first.hcl")) + assert.NoError(t, err) + assert.Equal(t, "description = \"first\"\n", string(first)) + _, err = os.Stat(filepath.Join(finalDest, "stale.hcl")) + assert.True(t, os.IsNotExist(err), "stale content from the corrupt clone should not survive") +} + +// TestHoldSourceLockChildProcess is not a real test: it's a worker spawned by +// TestSyncLockTimeoutFallsBackToExistingCopy (sources_test package) via +// re-exec, guarded by an env var so it no-ops under a normal test run. It +// holds the sync lock for a directory from a genuinely separate process, +// which is required to exercise lock contention: util/flock is deliberately +// re-entrant per-PID, so a single process can never observe its own lock as +// held by someone else. +func TestHoldSourceLockChildProcess(t *testing.T) { + if os.Getenv("HERMIT_TEST_CHILD") == "" { + t.Skip("only runs as a spawned child of TestSyncLockTimeoutFallsBackToExistingCopy") + } + path := os.Getenv("HERMIT_TEST_LOCK_PATH") + hold, err := time.ParseDuration(os.Getenv("HERMIT_TEST_LOCK_HOLD")) + if err != nil { + t.Fatalf("bad hold duration: %s", err) + } + u, _ := ui.NewForTesting() + release, err := acquireSyncLock(u, path, DefaultLockTimeout, "test lock holder") + if err != nil { + t.Fatalf("failed to acquire lock: %s", err) + } + if readyFile := os.Getenv("HERMIT_TEST_LOCK_READY_FILE"); readyFile != "" { + if err := os.WriteFile(readyFile, nil, 0600); err != nil { + t.Fatalf("failed to signal lock held: %s", err) + } + } + time.Sleep(hold) + if err := release(); err != nil { + t.Fatalf("failed to release lock: %s", err) + } +} diff --git a/sources/git_test.go b/sources/git_test.go index 3ae5e745..e42a0630 100644 --- a/sources/git_test.go +++ b/sources/git_test.go @@ -2,6 +2,7 @@ package sources_test import ( "os" + "strings" "testing" "github.com/alecthomas/assert/v2" @@ -18,6 +19,24 @@ func (f *FailingGit) RunInDir(_ *ui.Task, _ string, _ ...string) error { return f.err } +// sourceDirs returns the names of real source directories in dir, ignoring +// Hermit's lock files and sync scratch directories. Source directory names +// are bare hex SHA256 hashes (see util.Hash); every scratch/lock entry +// contains a ".", so this distinction is unambiguous. +func sourceDirs(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + assert.NoError(t, err) + var dirs []string + for _, entry := range entries { + if strings.Contains(entry.Name(), ".") { + continue + } + dirs = append(dirs, entry.Name()) + } + return dirs +} + func TestGitDoesNotRemoveSourceAfterSyncFailure(t *testing.T) { git := &FailingGit{} sourceDir := t.TempDir() @@ -27,10 +46,9 @@ func TestGitDoesNotRemoveSourceAfterSyncFailure(t *testing.T) { u, _ := ui.NewForTesting() _, err := source.Sync(u, true) assert.NoError(t, err) - files, err := os.ReadDir(sourceDir) - assert.NoError(t, err) - assert.Equal(t, len(files), 1) - gitDir := files[0].Name() + dirs := sourceDirs(t, sourceDir) + assert.Equal(t, len(dirs), 1) + gitDir := dirs[0] // Fail the sync git.err = errors.New("failing git fails") @@ -40,9 +58,7 @@ func TestGitDoesNotRemoveSourceAfterSyncFailure(t *testing.T) { assert.NoError(t, err) // the directory should still be in place after git failed to update - files, err = os.ReadDir(sourceDir) - assert.NoError(t, err) - assert.Equal(t, len(files), 1) - assert.Equal(t, gitDir, files[0].Name()) - + dirs = sourceDirs(t, sourceDir) + assert.Equal(t, len(dirs), 1) + assert.Equal(t, gitDir, dirs[0]) } diff --git a/sources/local.go b/sources/local.go index 82886c90..c8d92e37 100644 --- a/sources/local.go +++ b/sources/local.go @@ -20,7 +20,10 @@ func NewLocalSource(uri string, f fs.FS) *LocalSource { } func (s *LocalSource) Sync(_ *ui.UI, _ bool) (bool, error) { - return true, nil + // See the equivalent comment on BuiltInSource.Sync: this source performs + // no actual synchronisation, so it must report "false" here or it + // poisons Sources.isSynchronised for every other source. + return false, nil } func (s *LocalSource) URI() string { diff --git a/sources/lock.go b/sources/lock.go new file mode 100644 index 00000000..504f443c --- /dev/null +++ b/sources/lock.go @@ -0,0 +1,108 @@ +package sources + +import ( + "context" + "path/filepath" + "sync" + "time" + + "github.com/cashapp/hermit/errors" + "github.com/cashapp/hermit/ui" + "github.com/cashapp/hermit/util/flock" +) + +// slowLockWaitThreshold is how long acquireSyncLock will wait before it +// considers the wait worth telling the user about at Info level: below this, +// logging would just be noise for the common, fast, uncontended case. +const slowLockWaitThreshold = time.Second + +// DefaultLockTimeout is how long Hermit will wait for another process to +// finish synchronising a source before giving up. +// +// This is deliberately much longer than the global state lock timeout +// (--lock-timeout, default 30s): the process holding this lock may be +// performing a full "git clone" of a manifest repository over a slow network. +const DefaultLockTimeout = 10 * time.Minute + +// lockSuffix is appended to a source directory to derive its lock file path. +// +// Source directory names are bare hex SHA256 hashes (see util.Hash), so any +// entry containing a "." is unambiguously Hermit scratch state rather than a +// real source directory. +const lockSuffix = ".lock" + +// util/flock is deliberately re-entrant per-process: if the lock file already +// records our own PID, Acquire returns a no-op release and the caller +// proceeds without actually holding the lock (see util/flock.Acquire). That +// means it will not serialise two goroutines within the same process. +// +// We serialise those here, with a plain (non-re-entrant) mutex keyed by lock +// path, before ever touching flock. This must NOT live in util/flock itself: +// state.CleanPackages deliberately re-acquires its own lock recursively +// (via removeRecursive), and a non-re-entrant mutex there would deadlock. +var ( + localLocksMu sync.Mutex + localLocks = map[string]*sync.Mutex{} +) + +// localLock returns the process-local mutex for the given (already absolute) +// lock path. +func localLock(absPath string) *sync.Mutex { + localLocksMu.Lock() + defer localLocksMu.Unlock() + l, ok := localLocks[absPath] + if !ok { + l = &sync.Mutex{} + localLocks[absPath] = l + } + return l +} + +// acquireSyncLock takes an exclusive lock, across both processes and +// goroutines, on the source directory "dir". +// +// A timeout <= 0 is treated as DefaultLockTimeout. +// +// acquireSyncLock must never be called recursively (directly or indirectly) +// for the same "dir" from within the same process, as the process-local +// mutex it uses is not re-entrant. +func acquireSyncLock(log ui.Logger, dir string, timeout time.Duration, message string) (release func() error, err error) { + if timeout <= 0 { + timeout = DefaultLockTimeout + } + path := dir + lockSuffix + // Resolve to an absolute path before using it as a map key or handing it + // to flock: two different relative paths (or a relative and an absolute + // path) that resolve to the same file must serialise against each other, + // or the process-local mutex below is useless for callers that don't all + // construct "dir" identically. + absPath, err := filepath.Abs(path) + if err != nil { + return nil, errors.WithStack(err) + } + + local := localLock(absPath) + local.Lock() + + log.Tracef("acquiring source lock %s (timeout %s)", absPath, timeout) + start := time.Now() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + releaseFlock, err := flock.Acquire(ctx, absPath, message) + if err != nil { + local.Unlock() + return nil, errors.Wrapf(err, "failed to acquire source lock %s", absPath) + } + if waited := time.Since(start); waited > slowLockWaitThreshold { + // The common, uncontended case acquires near-instantly and would + // make this pure noise; a wait long enough to notice is worth + // telling the user about, since otherwise a command silently hangs + // for up to "timeout" with only a Trace-level line (invisible by + // default) explaining why. + log.Infof("waited %s to acquire source lock for %s", waited.Round(time.Millisecond), absPath) + } + return func() error { + defer local.Unlock() + return releaseFlock() + }, nil +} diff --git a/sources/memory.go b/sources/memory.go index f327ba12..34608359 100644 --- a/sources/memory.go +++ b/sources/memory.go @@ -19,7 +19,10 @@ func NewMemSource(name, content string) *MemSource { } func (s *MemSource) Sync(_ *ui.UI, _ bool) (bool, error) { - return true, nil + // See the equivalent comment on BuiltInSource.Sync: this source performs + // no actual synchronisation, so it must report "false" here or it + // poisons Sources.isSynchronised for every other source. + return false, nil } func (s *MemSource) URI() string { diff --git a/util/dirswap.go b/util/dirswap.go new file mode 100644 index 00000000..4b696b43 --- /dev/null +++ b/util/dirswap.go @@ -0,0 +1,61 @@ +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 from however long it takes to remove and +// repopulate a potentially large tree, down to the gap between two rename(2) +// calls in the same directory -- not zero, but small and constant-time +// regardless of tree size, and each rename itself is atomic so a reader never +// observes a partially-written directory. +// +// The caller must ensure no other goroutine or process can be concurrently +// mutating finalDest (eg. by holding an appropriate lock); SwapDir only +// protects readers, not other writers. +func SwapDir(src, finalDest string) error { + aside := finalDest + DirSwapAsideSuffix + + // May exist already if a previous process died mid-swap. Safe to remove: + // the caller is assumed to hold exclusive write access, so nothing is + // relying on it any more. + if err := os.RemoveAll(aside); err != nil { + return errors.WithStack(err) + } + if err := os.Rename(finalDest, aside); err != nil && !os.IsNotExist(err) { + return errors.WithStack(err) + } + if err := os.Rename(src, finalDest); err != nil { + // Put the previous tree back so that we degrade to "stale" rather + // than "missing". + if rerr := os.Rename(aside, finalDest); rerr != nil && !os.IsNotExist(rerr) { + return errors.Join(errors.WithStack(err), errors.WithStack(rerr)) + } + return errors.WithStack(err) + } + // aside is no longer reachable via finalDest, and readers with an open + // FS handle are unaffected by unlinking it, so it's safe to remove + // synchronously here. We deliberately don't defer this to a goroutine: + // Hermit's exec path ends in syscall.Exec, which would silently kill any + // in-flight background cleanup and leak the directory forever. + return errors.WithStack(os.RemoveAll(aside)) +} 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) +} 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)