From ed977572420e9ba642881a847d3236073e091d45 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Fri, 26 Jun 2026 10:57:29 +1000 Subject: [PATCH 1/2] fix(gitclone): skip and remove leftover clone temp dirs Interrupted clones can leave .clone-* temp directories behind whose repo subdirectory contains a HEAD file. DiscoverExisting treated these as real mirrors and warmExistingRepos ran startup fetches against the bogus upstream URL. Remove and skip them on discovery instead. --- internal/gitclone/manager.go | 16 ++++++++++++++- internal/gitclone/manager_test.go | 33 +++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/internal/gitclone/manager.go b/internal/gitclone/manager.go index 90a2bb32..b3b89793 100644 --- a/internal/gitclone/manager.go +++ b/internal/gitclone/manager.go @@ -27,6 +27,12 @@ const ( StateReady // Ready to use ) +// cloneTempPrefix is the prefix for the temporary directory created during a +// clone before it is atomically renamed into place. Interrupted clones (e.g. a +// killed process) can leave these behind, so they are skipped and removed by +// DiscoverExisting on startup. +const cloneTempPrefix = ".clone-" + func (s State) String() string { switch s { case StateEmpty: @@ -228,6 +234,14 @@ func (m *Manager) DiscoverExisting(ctx context.Context) ([]*Repository, error) { return nil } + if strings.HasPrefix(info.Name(), cloneTempPrefix) { + logging.FromContext(ctx).InfoContext(ctx, "Removing leftover clone temp dir", "path", path) + if rmErr := os.RemoveAll(path); rmErr != nil { + return errors.Wrapf(rmErr, "remove leftover clone temp dir %s", path) + } + return fs.SkipDir + } + headPath := filepath.Join(path, "HEAD") if _, statErr := os.Stat(headPath); statErr != nil { if errors.Is(statErr, os.ErrNotExist) { @@ -499,7 +513,7 @@ func (r *Repository) executeClone(ctx context.Context) error { return errors.Wrap(err, "create clone directory") } - tmpDir, err := os.MkdirTemp(parentDir, ".clone-*") + tmpDir, err := os.MkdirTemp(parentDir, cloneTempPrefix+"*") if err != nil { return errors.Wrap(err, "create temp clone directory") } diff --git a/internal/gitclone/manager_test.go b/internal/gitclone/manager_test.go index 40f8b21e..238a601f 100644 --- a/internal/gitclone/manager_test.go +++ b/internal/gitclone/manager_test.go @@ -209,6 +209,39 @@ func TestManager_DiscoverExisting(t *testing.T) { } } +func TestManager_DiscoverExisting_SkipsAndRemovesLeftoverCloneTempDirs(t *testing.T) { + _, ctx := logging.Configure(t.Context(), logging.Config{Level: slog.LevelError}) + tmpDir := t.TempDir() + config := Config{ + MirrorRoot: tmpDir, + FetchInterval: 15 * time.Minute, + RefCheckInterval: 10 * time.Second, + } + + manager, err := NewManager(ctx, config, nil) + assert.NoError(t, err) + + upstreamPath := createBareRepo(t, t.TempDir()) + + realRepo := filepath.Join(tmpDir, "github.com", "user1", "repo1") + assert.NoError(t, os.MkdirAll(filepath.Dir(realRepo), 0o755)) + assert.NoError(t, exec.Command("git", "clone", "--bare", upstreamPath, realRepo).Run()) + + // Simulate an interrupted clone: a leftover .clone-* temp dir containing a + // mirror clone at its "repo" subdirectory, alongside the real repo. + leftover := filepath.Join(tmpDir, "github.com", "user1", cloneTempPrefix+"123456") + assert.NoError(t, os.MkdirAll(leftover, 0o755)) + assert.NoError(t, exec.Command("git", "clone", "--bare", upstreamPath, filepath.Join(leftover, "repo")).Run()) + + discovered, err := manager.DiscoverExisting(context.Background()) + assert.NoError(t, err) + assert.Equal(t, 1, len(discovered)) + assert.Equal(t, "https://github.com/user1/repo1", discovered[0].UpstreamURL()) + + _, statErr := os.Stat(leftover) + assert.True(t, os.IsNotExist(statErr), "leftover clone temp dir should be removed") +} + func TestRepository_StateTransitions(t *testing.T) { repo := &Repository{ state: StateEmpty, From 7c204f8ef406cfbca4a2eac0ef840fc9bf12a191 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Fri, 26 Jun 2026 11:22:29 +1000 Subject: [PATCH 2/2] test(git): scrub GIT_* env vars in TestMain Git exports GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE when running hooks. Test git commands inherited these and operated on the ambient repository instead of their temp dirs, polluting it with junk commits during the pre-push hook. Drop them in TestMain. --- cmd/cachew/git_test.go | 12 ++++++++++++ internal/gitclone/manager_test.go | 14 +++++++++++++- internal/strategy/git/main_test.go | 18 ++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 internal/strategy/git/main_test.go diff --git a/cmd/cachew/git_test.go b/cmd/cachew/git_test.go index 4f02e804..ec8b8dbd 100644 --- a/cmd/cachew/git_test.go +++ b/cmd/cachew/git_test.go @@ -19,6 +19,18 @@ import ( "github.com/block/cachew/client" ) +// TestMain drops the GIT_* variables git exports under hooks so test git +// commands target their temp dirs, not the ambient repository. +func TestMain(m *testing.M) { + for _, v := range []string{ + "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", + "GIT_COMMON_DIR", "GIT_PREFIX", "GIT_NAMESPACE", + } { + _ = os.Unsetenv(v) + } + os.Exit(m.Run()) +} + func initGitRepo(t *testing.T, dir string, files map[string]string) { t.Helper() run := func(args ...string) { diff --git a/internal/gitclone/manager_test.go b/internal/gitclone/manager_test.go index 238a601f..c5895d37 100644 --- a/internal/gitclone/manager_test.go +++ b/internal/gitclone/manager_test.go @@ -16,6 +16,18 @@ import ( "github.com/block/cachew/internal/logging" ) +// TestMain drops the GIT_* variables git exports under hooks so test git +// commands target their temp dirs, not the ambient repository. +func TestMain(m *testing.M) { + for _, v := range []string{ + "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", + "GIT_COMMON_DIR", "GIT_PREFIX", "GIT_NAMESPACE", + } { + _ = os.Unsetenv(v) + } + os.Exit(m.Run()) +} + // testRepoConfig returns a Config with timeouts populated, suitable for // constructing Repository values directly in tests that bypass NewManager. func testRepoConfig() Config { @@ -233,7 +245,7 @@ func TestManager_DiscoverExisting_SkipsAndRemovesLeftoverCloneTempDirs(t *testin assert.NoError(t, os.MkdirAll(leftover, 0o755)) assert.NoError(t, exec.Command("git", "clone", "--bare", upstreamPath, filepath.Join(leftover, "repo")).Run()) - discovered, err := manager.DiscoverExisting(context.Background()) + discovered, err := manager.DiscoverExisting(ctx) assert.NoError(t, err) assert.Equal(t, 1, len(discovered)) assert.Equal(t, "https://github.com/user1/repo1", discovered[0].UpstreamURL()) diff --git a/internal/strategy/git/main_test.go b/internal/strategy/git/main_test.go new file mode 100644 index 00000000..b07488b8 --- /dev/null +++ b/internal/strategy/git/main_test.go @@ -0,0 +1,18 @@ +package git_test + +import ( + "os" + "testing" +) + +// TestMain drops the GIT_* variables git exports under hooks so test git +// commands target their temp dirs, not the ambient repository. +func TestMain(m *testing.M) { + for _, v := range []string{ + "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", + "GIT_COMMON_DIR", "GIT_PREFIX", "GIT_NAMESPACE", + } { + _ = os.Unsetenv(v) + } + os.Exit(m.Run()) +}