From a4e31930c3c763922d4c2770a9ddfbd4c0738805 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Wed, 30 Jul 2025 05:34:45 +0530 Subject: [PATCH 1/2] Tempdir: return cleanup functions instead of launching goroutines The tempdir package launches a goroutine every time a temporary directory is created to clean up on context cancellation. This has downsides: - In our tests, we look for goroutines that are still running after tests. This deletion goroutine is not synchronized and can cause flakes. - If context is canceled, the directory is deleted while code may still be operating on it, causing unexpected errors. Instead of goroutines, make tempdir.New* functions return a cleanup function as a third return value. This approach: - Makes cleanup explicit and harder to miss (compiler enforces it) - Ensures cleanup happens on all code paths, including errors - Eliminates goroutine-related test flakes This implements the approach suggested by maintainer feedback. Signed-off-by: Siddharth Asthana --- internal/git/catfile/cache_test.go | 3 +- internal/git/catfile/object_reader_test.go | 3 +- internal/git/localrepo/bundle.go | 29 +++++++--- internal/git/localrepo/paths_test.go | 3 +- internal/git/quarantine/quarantine.go | 17 +++--- .../git/quarantine/quarantine_ext_test.go | 3 +- internal/git/quarantine/quarantine_test.go | 25 ++++---- internal/gitaly/hook/postreceive_test.go | 3 +- internal/gitaly/hook/prereceive_test.go | 3 +- internal/gitaly/hook/update_test.go | 3 +- .../hook/updateref/update_with_hooks_test.go | 3 +- internal/gitaly/repoutil/create.go | 8 +-- internal/gitaly/service/blob/blobs_test.go | 3 +- .../gitaly/service/blob/lfs_pointers_test.go | 6 +- .../gitaly/service/cleanup/rewrite_history.go | 33 +++++++---- .../service/conflicts/list_conflict_files.go | 8 ++- .../service/conflicts/resolve_conflicts.go | 8 ++- internal/gitaly/service/conflicts/server.go | 8 +-- internal/gitaly/service/diff/diff_blobs.go | 8 ++- .../gitaly/service/operations/cherry_pick.go | 8 ++- .../gitaly/service/operations/commit_files.go | 8 ++- .../gitaly/service/operations/ff_branch.go | 8 ++- .../gitaly/service/operations/merge_branch.go | 8 ++- .../service/operations/rebase_confirmable.go | 8 ++- .../service/operations/rebase_to_ref.go | 8 ++- internal/gitaly/service/operations/revert.go | 8 ++- internal/gitaly/service/operations/server.go | 8 +-- internal/gitaly/service/operations/squash.go | 8 ++- .../gitaly/service/operations/submodules.go | 8 ++- internal/gitaly/service/operations/tags.go | 8 ++- .../service/operations/user_create_branch.go | 8 ++- .../service/operations/user_update_branch.go | 8 ++- internal/gitaly/service/repository/fetch.go | 8 ++- .../gitaly/service/repository/fetch_remote.go | 12 +++- internal/gitaly/service/repository/server.go | 8 +-- .../gitaly/service/repository/size_test.go | 9 ++- internal/tempdir/tempdir.go | 58 ++++++++----------- internal/tempdir/tempdir_test.go | 16 ++--- 38 files changed, 261 insertions(+), 131 deletions(-) diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go index db5ae2c67e4..797b145cbb1 100644 --- a/internal/git/catfile/cache_test.go +++ b/internal/git/catfile/cache_test.go @@ -433,8 +433,9 @@ func TestCache_ObjectReader_quarantine(t *testing.T) { }) gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main")) - quarantineDir, err := quarantine.New(testhelper.Context(t), repo, logger, locator) + quarantineDir, cleanup, err := quarantine.New(testhelper.Context(t), repo, logger, locator) require.NoError(t, err) + defer func() { _ = cleanup() }() quarantineRepo := quarantineDir.QuarantinedRepo() diff --git a/internal/git/catfile/object_reader_test.go b/internal/git/catfile/object_reader_test.go index f5fee1bbed0..1df7dbdba16 100644 --- a/internal/git/catfile/object_reader_test.go +++ b/internal/git/catfile/object_reader_test.go @@ -720,8 +720,9 @@ func TestObjectReader_logging(t *testing.T) { SkipCreationViaService: true, }) - quarantineDir, err := quarantine.New(ctx, repoProto, logger, locator) + quarantineDir, cleanup, err := quarantine.New(ctx, repoProto, logger, locator) require.NoError(t, err) + defer func() { _ = cleanup() }() reader, err := newObjectReader(ctx, newRepoExecutor(t, cfg, quarantineDir.QuarantinedRepo()), nil) require.NoError(t, err) diff --git a/internal/git/localrepo/bundle.go b/internal/git/localrepo/bundle.go index d033174e4c5..664d33e11f7 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -77,10 +77,15 @@ func (repo *Repo) CreateBundle(ctx context.Context, out io.Writer, opts *CreateB func (repo *Repo) CloneBundle(ctx context.Context, reader io.Reader) error { // When cloning from a file, `git-clone(1)` requires the path to the file. Create a temporary // file with the Git bundle contents that is used for cloning. - bundlePath, err := repo.createTempBundle(ctx, reader) + bundlePath, cleanup, err := repo.createTempBundle(ctx, reader) if err != nil { return err } + defer func() { + if err := cleanup(); err != nil { + repo.logger.WithError(err).Error("failed to cleanup temp bundle directory") + } + }() repoPath, err := repo.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) if err != nil { @@ -156,10 +161,15 @@ func (repo *Repo) FetchBundle(ctx context.Context, txManager transaction.Manager opts = &FetchBundleOpts{} } - bundlePath, err := repo.createTempBundle(ctx, reader) + bundlePath, cleanup, err := repo.createTempBundle(ctx, reader) if err != nil { return fmt.Errorf("fetch bundle: %w", err) } + defer func() { + if err := cleanup(); err != nil { + repo.logger.WithError(err).Error("failed to cleanup temp bundle directory") + } + }() fetchConfig := []gitcmd.ConfigPair{ {Key: "remote.inmemory.url", Value: bundlePath}, @@ -192,17 +202,19 @@ func (repo *Repo) FetchBundle(ctx context.Context, txManager transaction.Manager // createTempBundle copies reader onto the filesystem so that a path can be // passed to git. git-fetch does not support streaming a bundle over a pipe. -func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundlPath string, returnErr error) { - tmpDir, err := tempdir.New(ctx, repo.GetStorageName(), repo.logger, repo.locator) +// The caller is responsible for calling the returned cleanup function. +func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundlPath string, cleanup func() error, returnErr error) { + tmpDir, cleanup, err := tempdir.New(ctx, repo.GetStorageName(), repo.logger, repo.locator) if err != nil { - return "", fmt.Errorf("create temp bundle: %w", err) + return "", nil, fmt.Errorf("create temp bundle: %w", err) } bundlePath := filepath.Join(tmpDir.Path(), "repo.bundle") file, err := os.Create(bundlePath) if err != nil { - return "", fmt.Errorf("create temp bundle: %w", err) + cleanup() // Clean up if we fail after creating the temp directory + return "", nil, fmt.Errorf("create temp bundle: %w", err) } defer func() { if err := file.Close(); err != nil && returnErr == nil { @@ -211,10 +223,11 @@ func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundl }() if _, err = io.Copy(file, reader); err != nil { - return "", fmt.Errorf("create temp bundle: %w", err) + cleanup() // Clean up if we fail after creating the temp directory + return "", nil, fmt.Errorf("create temp bundle: %w", err) } - return bundlePath, nil + return bundlePath, cleanup, nil } // updateHeadFromBundle updates HEAD from a bundle file diff --git a/internal/git/localrepo/paths_test.go b/internal/git/localrepo/paths_test.go index 6d737639a4d..71cf74de66d 100644 --- a/internal/git/localrepo/paths_test.go +++ b/internal/git/localrepo/paths_test.go @@ -73,8 +73,9 @@ func TestRepo_ObjectDirectoryPath(t *testing.T) { }) locator := config.NewLocator(cfg) - quarantine, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) + quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) require.NoError(t, err) + defer func() { _ = cleanup() }() quarantinedRepo := quarantine.QuarantinedRepo() // Transactions store their set a quarantine directory in the transaction's temporary diff --git a/internal/git/quarantine/quarantine.go b/internal/git/quarantine/quarantine.go index e04849241a4..cee1ce1ffe1 100644 --- a/internal/git/quarantine/quarantine.go +++ b/internal/git/quarantine/quarantine.go @@ -32,23 +32,24 @@ type Dir struct { locator storage.Locator } -// New creates a new quarantine directory and returns the directory. The repository is cleaned -// up when the user invokes the Migrate() functionality on the Dir. -func New(ctx context.Context, repo *gitalypb.Repository, logger log.Logger, locator storage.Locator) (*Dir, error) { +// New creates a new quarantine directory and returns the directory and a cleanup function. +// The cleanup function must be called to remove the quarantine directory. +func New(ctx context.Context, repo *gitalypb.Repository, logger log.Logger, locator storage.Locator) (*Dir, func() error, error) { repoPath, err := locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) if err != nil { - return nil, structerr.NewInternal("getting repo path: %w", err) + return nil, nil, structerr.NewInternal("getting repo path: %w", err) } - quarantineDir, err := tempdir.NewWithPrefix(ctx, repo.GetStorageName(), + quarantineDir, cleanup, err := tempdir.NewWithPrefix(ctx, repo.GetStorageName(), storage.QuarantineDirectoryPrefix(repo), logger, locator) if err != nil { - return nil, fmt.Errorf("creating quarantine: %w", err) + return nil, nil, fmt.Errorf("creating quarantine: %w", err) } quarantinedRepo, err := Apply(repoPath, repo, quarantineDir.Path()) if err != nil { - return nil, err + cleanup() // Clean up if we fail after creating the temp directory + return nil, nil, err } return &Dir{ @@ -56,7 +57,7 @@ func New(ctx context.Context, repo *gitalypb.Repository, logger log.Logger, loca quarantinedRepo: quarantinedRepo, locator: locator, dir: quarantineDir, - }, nil + }, cleanup, nil } // Apply applies the quarantine on the repository. This is done by setting the quarantineDirectory diff --git a/internal/git/quarantine/quarantine_ext_test.go b/internal/git/quarantine/quarantine_ext_test.go index 97fc28f851f..89d5ece8025 100644 --- a/internal/git/quarantine/quarantine_ext_test.go +++ b/internal/git/quarantine/quarantine_ext_test.go @@ -28,8 +28,9 @@ func TestQuarantine_localrepo(t *testing.T) { locator := config.NewLocator(cfg) - quarantine, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) + quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) require.NoError(t, err) + defer func() { _ = cleanup() }() quarantined := localrepo.NewTestRepo(t, cfg, quarantine.QuarantinedRepo()) diff --git a/internal/git/quarantine/quarantine_test.go b/internal/git/quarantine/quarantine_test.go index 208af699e9b..d69a42af53f 100644 --- a/internal/git/quarantine/quarantine_test.go +++ b/internal/git/quarantine/quarantine_test.go @@ -1,7 +1,6 @@ package quarantine import ( - "context" "os" "path/filepath" "testing" @@ -50,8 +49,9 @@ func TestQuarantine_lifecycle(t *testing.T) { logger := testhelper.NewLogger(t) t.Run("quarantine directory gets created", func(t *testing.T) { - quarantine, err := New(ctx, repo, logger, locator) + quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) + defer func() { _ = cleanup() }() relativeQuarantinePath, err := filepath.Rel(repoPath, quarantine.dir.Path()) require.NoError(t, err) @@ -72,15 +72,12 @@ func TestQuarantine_lifecycle(t *testing.T) { require.DirExists(t, quarantine.dir.Path()) }) - t.Run("context cancellation cleans up quarantine directory", func(t *testing.T) { - ctx, cancel := context.WithCancel(ctx) - - quarantine, err := New(ctx, repo, logger, locator) + t.Run("explicit cleanup removes quarantine directory", func(t *testing.T) { + quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) require.DirExists(t, quarantine.dir.Path()) - cancel() - quarantine.dir.WaitForCleanup() + require.NoError(t, cleanup()) require.NoDirExists(t, quarantine.dir.Path()) }) } @@ -102,8 +99,9 @@ func TestQuarantine_Migrate(t *testing.T) { oldContents := listEntries(t, repoPath) - quarantine, err := New(ctx, repo, logger, locator) + quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) + defer func() { _ = cleanup() }() require.NoError(t, quarantine.Migrate(ctx)) @@ -120,8 +118,9 @@ func TestQuarantine_Migrate(t *testing.T) { oldContents := listEntries(t, repoPath) require.NotContains(t, oldContents, "objects/file") - quarantine, err := New(ctx, repo, logger, locator) + quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) + defer func() { _ = cleanup() }() require.NoError(t, os.WriteFile(filepath.Join(quarantine.dir.Path(), "file"), []byte("foobar"), mode.File)) require.NoError(t, quarantine.Migrate(ctx)) @@ -143,16 +142,18 @@ func TestQuarantine_Migrate(t *testing.T) { repoContents := listEntries(t, repoPath) require.NotContains(t, repoContents, "objects/file") - quarantine, err := New(ctx, repo, logger, locator) + quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) + defer func() { _ = cleanup() }() require.Empty(t, listEntries(t, quarantine.dir.Path())) // Quarantine the already quarantined repository and write the object there. We expect the // object to be migrated from the second level quarantine to the first level quarantine. The // main repository should stay untouched. - recursiveQuarantine, err := New(ctx, quarantine.QuarantinedRepo(), logger, locator) + recursiveQuarantine, recursiveCleanup, err := New(ctx, quarantine.QuarantinedRepo(), logger, locator) require.NoError(t, err) + defer func() { _ = recursiveCleanup() }() require.NoError(t, os.WriteFile(filepath.Join(recursiveQuarantine.dir.Path(), "file"), []byte("foobar"), mode.File)) require.NoError(t, recursiveQuarantine.Migrate(ctx)) diff --git a/internal/gitaly/hook/postreceive_test.go b/internal/gitaly/hook/postreceive_test.go index f6cd89b992b..09e01b26e48 100644 --- a/internal/gitaly/hook/postreceive_test.go +++ b/internal/gitaly/hook/postreceive_test.go @@ -427,8 +427,9 @@ func TestPostReceive_quarantine(t *testing.T) { SkipCreationViaService: true, }) - quarantine, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) + quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) require.NoError(t, err) + defer func() { _ = cleanup() }() quarantinedRepo := localrepo.NewTestRepo(t, cfg, quarantine.QuarantinedRepo()) blobID, err := quarantinedRepo.WriteBlob(ctx, strings.NewReader("allyourbasearebelongtous"), localrepo.WriteBlobConfig{}) diff --git a/internal/gitaly/hook/prereceive_test.go b/internal/gitaly/hook/prereceive_test.go index 66e407607f4..64c4b2769d2 100644 --- a/internal/gitaly/hook/prereceive_test.go +++ b/internal/gitaly/hook/prereceive_test.go @@ -223,8 +223,9 @@ func TestPrereceive_quarantine(t *testing.T) { SkipCreationViaService: true, }) - quarantine, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) + quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) require.NoError(t, err) + defer func() { _ = cleanup() }() quarantinedRepo := localrepo.NewTestRepo(t, cfg, quarantine.QuarantinedRepo()) blobID, err := quarantinedRepo.WriteBlob(ctx, strings.NewReader("allyourbasearebelongtous"), localrepo.WriteBlobConfig{}) diff --git a/internal/gitaly/hook/update_test.go b/internal/gitaly/hook/update_test.go index 4287d271683..a39f78f1725 100644 --- a/internal/gitaly/hook/update_test.go +++ b/internal/gitaly/hook/update_test.go @@ -253,8 +253,9 @@ func TestUpdate_quarantine(t *testing.T) { SkipCreationViaService: true, }) - quarantine, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) + quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) require.NoError(t, err) + defer func() { _ = cleanup() }() quarantinedRepo := localrepo.NewTestRepo(t, cfg, quarantine.QuarantinedRepo()) blobID, err := quarantinedRepo.WriteBlob(ctx, strings.NewReader("allyourbasearebelongtous"), localrepo.WriteBlobConfig{}) diff --git a/internal/gitaly/hook/updateref/update_with_hooks_test.go b/internal/gitaly/hook/updateref/update_with_hooks_test.go index 72b743c672b..edd6d561984 100644 --- a/internal/gitaly/hook/updateref/update_with_hooks_test.go +++ b/internal/gitaly/hook/updateref/update_with_hooks_test.go @@ -318,8 +318,9 @@ func TestUpdaterWithHooks_quarantine(t *testing.T) { unquarantinedRepo := localrepo.NewTestRepo(t, cfg, repoProto) - quarantine, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) + quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) require.NoError(t, err) + defer func() { _ = cleanup() }() quarantinedRepo := localrepo.NewTestRepo(t, cfg, quarantine.QuarantinedRepo()) blobID, err := quarantinedRepo.WriteBlob(ctx, strings.NewReader("1834298812398123"), localrepo.WriteBlobConfig{}) require.NoError(t, err) diff --git a/internal/gitaly/repoutil/create.go b/internal/gitaly/repoutil/create.go index f849cdb8ed0..d85c97583fc 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -104,15 +104,13 @@ func Create( return fmt.Errorf("pre-lock stat: %w", err) } - newRepoProto, newRepoDir, err := tempdir.NewRepository(ctx, repository.GetStorageName(), logger, locator) + newRepoProto, newRepoDir, cleanup, err := tempdir.NewRepository(ctx, repository.GetStorageName(), logger, locator) if err != nil { return fmt.Errorf("creating temporary repository: %w", err) } defer func() { - // We don't really care about whether this succeeds or not. It will either get - // cleaned up after the context is done, or eventually by the tempdir walker when - // it's old enough. - _ = os.RemoveAll(newRepoDir.Path()) + // We don't really care about whether this succeeds or not. + _ = cleanup() }() // Note that we do not create the repository directly in its target location, but diff --git a/internal/gitaly/service/blob/blobs_test.go b/internal/gitaly/service/blob/blobs_test.go index 690539c0b5b..6cab6d8c7d8 100644 --- a/internal/gitaly/service/blob/blobs_test.go +++ b/internal/gitaly/service/blob/blobs_test.go @@ -331,8 +331,9 @@ func TestListAllBlobs(t *testing.T) { repo, _, _ := setupRepoWithLFS(t, ctx, cfg) - quarantine, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) + quarantine, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) require.NoError(t, err) + defer func() { _ = cleanup() }() // quarantine.New in Gitaly would receive an already rewritten repository. Gitaly would then calculate // the quarantine directories based on the rewritten relative path. That quarantine would then be looped diff --git a/internal/gitaly/service/blob/lfs_pointers_test.go b/internal/gitaly/service/blob/lfs_pointers_test.go index b4dffb415d5..1f165c2d904 100644 --- a/internal/gitaly/service/blob/lfs_pointers_test.go +++ b/internal/gitaly/service/blob/lfs_pointers_test.go @@ -218,8 +218,9 @@ size 12345` setup: func(t *testing.T) setupData { repo, _, _ := setupRepoWithLFS(t, ctx, cfg) - quarantineDir, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) + quarantineDir, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) require.NoError(t, err) + defer func() { _ = cleanup() }() repo.GitObjectDirectory = quarantineDir.QuarantinedRepo().GetGitObjectDirectory() @@ -241,8 +242,9 @@ size 12345` // this case, LFS pointer checks may want to inspect all newly // pushed objects, denoted by a repository proto message which only // has its object directory set to the quarantine directory. - quarantineDir, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) + quarantineDir, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) require.NoError(t, err) + defer func() { _ = cleanup() }() // Note that we need to continue using the non-rewritten repository // here as `localrepo.NewTestRepo()` already will try to rewrite it diff --git a/internal/gitaly/service/cleanup/rewrite_history.go b/internal/gitaly/service/cleanup/rewrite_history.go index 16bf818cfef..a11b795b2f5 100644 --- a/internal/gitaly/service/cleanup/rewrite_history.go +++ b/internal/gitaly/service/cleanup/rewrite_history.go @@ -110,10 +110,15 @@ func (s *server) rewriteHistory( return fmt.Errorf("finding HEAD reference: %w", err) } - stagingRepo, stagingRepoPath, err := s.initStagingRepo(ctx, repoProto, defaultBranch) + stagingRepo, stagingRepoPath, cleanup, err := s.initStagingRepo(ctx, repoProto, defaultBranch) if err != nil { return fmt.Errorf("setting up staging repo: %w", err) } + defer func() { + if err := cleanup(); err != nil { + s.logger.WithError(err).Error("failed to cleanup staging repo directory") + } + }() // Check state of source repository prior to running filter-repo. initialChecksum, err := checksumRepo(ctx, repo) @@ -185,11 +190,11 @@ func (s *server) rewriteHistory( } // initStagingRepo creates a new bare repository to write the rewritten history into -// with default branch is set to match the source repo. -func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, defaultBranch git.ReferenceName) (*localrepo.Repo, string, error) { - stagingRepoProto, stagingRepoDir, err := tempdir.NewRepository(ctx, repo.GetStorageName(), s.logger, s.locator) +// with default branch is set to match the source repo. Returns the repo, path, and cleanup function. +func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, defaultBranch git.ReferenceName) (*localrepo.Repo, string, func() error, error) { + stagingRepoProto, stagingRepoDir, cleanup, err := tempdir.NewRepository(ctx, repo.GetStorageName(), s.logger, s.locator) if err != nil { - return nil, "", err + return nil, "", nil, err } var stderr strings.Builder @@ -202,11 +207,13 @@ func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, Args: []string{stagingRepoDir.Path()}, }, gitcmd.WithStderr(&stderr)) if err != nil { - return nil, "", fmt.Errorf("spawning git-init: %w", err) + cleanup() // Clean up on error + return nil, "", nil, fmt.Errorf("spawning git-init: %w", err) } if err := cmd.Wait(); err != nil { - return nil, "", structerr.New("creating repository: %w", err).WithMetadata("stderr", &stderr) + cleanup() // Clean up on error + return nil, "", nil, structerr.New("creating repository: %w", err).WithMetadata("stderr", &stderr) } stagingRepo := s.localRepoFactory.Build(stagingRepoProto) @@ -214,10 +221,11 @@ func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, // Ensure HEAD matches the source repository. In practice a mismatch doesn't cause problems, // but out of an abundance of caution let's keep the two repos as similar as possible. if err := stagingRepo.SetDefaultBranch(ctx, s.txManager, defaultBranch); err != nil { - return nil, "", fmt.Errorf("setting default branch: %w", err) + cleanup() // Clean up on error + return nil, "", nil, fmt.Errorf("setting default branch: %w", err) } - return stagingRepo, stagingRepoDir.Path(), nil + return stagingRepo, stagingRepoDir.Path(), cleanup, nil } func (s *server) runFilterRepo( @@ -227,10 +235,15 @@ func (s *server) runFilterRepo( redactions [][]byte, ) error { // Place argument files in a tempdir so that cleanup is handled automatically. - tmpDir, err := tempdir.New(ctx, srcRepo.GetStorageName(), s.logger, s.locator) + tmpDir, cleanup, err := tempdir.New(ctx, srcRepo.GetStorageName(), s.logger, s.locator) if err != nil { return fmt.Errorf("create tempdir: %w", err) } + defer func() { + if err := cleanup(); err != nil { + s.logger.WithError(err).Error("failed to cleanup temp directory") + } + }() flags := make([]gitcmd.Option, 0, 2) diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go index 0bb926fe52d..61177f7dedb 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -8,6 +8,7 @@ import ( "io" "unicode/utf8" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -23,10 +24,15 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s return structerr.NewInvalidArgument("%w", err) } - _, quarantineRepo, err := s.quarantinedRepo(ctx, request.GetRepository()) + _, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, request.GetRepository()) if err != nil { return err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() ours, err := quarantineRepo.ResolveRevision(ctx, git.Revision(request.GetOurCommitOid()+"^{commit}")) if err != nil { diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index c902e228136..858f2e8ab8a 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -11,6 +11,7 @@ import ( "sort" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/conflict" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -143,10 +144,15 @@ func (s *server) resolveConflicts(header *gitalypb.ResolveConflictsRequestHeader return err } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, header.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, header.GetRepository()) if err != nil { return err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() if err := s.repoWithBranchCommit(ctx, quarantineRepo, diff --git a/internal/gitaly/service/conflicts/server.go b/internal/gitaly/service/conflicts/server.go index a1178a071a1..74c1602ac2b 100644 --- a/internal/gitaly/service/conflicts/server.go +++ b/internal/gitaly/service/conflicts/server.go @@ -40,12 +40,12 @@ func NewServer(deps *service.Dependencies) gitalypb.ConflictsServiceServer { } } -func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, error) { - quarantineDir, err := quarantine.New(ctx, repo, s.logger, s.locator) +func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func() error, error) { + quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.logger, s.locator) if err != nil { - return nil, nil, structerr.NewInternal("creating object quarantine: %w", err) + return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) } quarantineRepo := s.localRepoFactory.Build(quarantineDir.QuarantinedRepo()) - return quarantineDir, quarantineRepo, nil + return quarantineDir, quarantineRepo, cleanup, nil } diff --git a/internal/gitaly/service/diff/diff_blobs.go b/internal/gitaly/service/diff/diff_blobs.go index 9b4e31cc97c..1897b7def3e 100644 --- a/internal/gitaly/service/diff/diff_blobs.go +++ b/internal/gitaly/service/diff/diff_blobs.go @@ -7,6 +7,7 @@ import ( "fmt" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" @@ -148,10 +149,15 @@ func (s *server) diffBlobs(ctx context.Context, // of right blob pair. Unlike an empty tree object, an empty blob object is not special cased // and must exist in the repository to be used. Since the DiffBlobs RPC is read-only, we create // a quarantine directory to stage an empty blob object for use with diff generation only. - quarantineDir, err := quarantine.New(ctx, request.GetRepository(), s.logger, s.locator) + quarantineDir, cleanup, err := quarantine.New(ctx, request.GetRepository(), s.logger, s.locator) if err != nil { return structerr.NewInternal("creating quarantine directory: %w", err) } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() repo := s.localRepoFactory.Build(quarantineDir.QuarantinedRepo()) diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go index dbd12c8116b..46ec13c3afb 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" @@ -21,10 +22,15 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() startRevision, err := s.fetchStartRevision(ctx, quarantineRepo, req) if err != nil { diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 5913288f05d..a73293ef839 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -521,10 +522,15 @@ func (s *Server) userCommitFiles( stream gitalypb.OperationService_UserCommitFilesServer, objectHash git.ObjectHash, ) error { - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, header.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, header.GetRepository()) if err != nil { return err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() repoPath, err := quarantineRepo.Path(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/ff_branch.go b/internal/gitaly/service/operations/ff_branch.go index e78c8c5e00a..d4aee6e90e2 100644 --- a/internal/gitaly/service/operations/ff_branch.go +++ b/internal/gitaly/service/operations/ff_branch.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -23,10 +24,15 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ // While we're creating a quarantine directory, we know that it won't ever have any new // objects given that we're doing a fast-forward merge. We still want to create one such // that Rails can efficiently compute new objects. - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, in.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, in.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/merge_branch.go b/internal/gitaly/service/operations/merge_branch.go index d8066d1257d..725b209f3d5 100644 --- a/internal/gitaly/service/operations/merge_branch.go +++ b/internal/gitaly/service/operations/merge_branch.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" @@ -29,10 +30,15 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc return structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, firstRequest.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, firstRequest.GetRepository()) if err != nil { return err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/rebase_confirmable.go b/internal/gitaly/service/operations/rebase_confirmable.go index 0f4a1d03010..5cb3ee716a8 100644 --- a/internal/gitaly/service/operations/rebase_confirmable.go +++ b/internal/gitaly/service/operations/rebase_confirmable.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" @@ -31,10 +32,15 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba return structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, header.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, header.GetRepository()) if err != nil { return structerr.NewInternal("creating repo quarantine: %w", err) } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/rebase_to_ref.go b/internal/gitaly/service/operations/rebase_to_ref.go index ddb8fde1761..56cddcb85b5 100644 --- a/internal/gitaly/service/operations/rebase_to_ref.go +++ b/internal/gitaly/service/operations/rebase_to_ref.go @@ -4,6 +4,7 @@ import ( "context" "errors" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" @@ -18,10 +19,15 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, request.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, request.GetRepository()) if err != nil { return nil, structerr.NewInternal("creating repo quarantine: %w", err) } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() oid, err := quarantineRepo.ResolveRevision(ctx, git.Revision(request.GetFirstParentRef())) if err != nil { diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index 760e625eb70..ab92306d5f6 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -20,10 +21,15 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() startRevision, err := s.fetchStartRevision(ctx, quarantineRepo, req) if err != nil { diff --git a/internal/gitaly/service/operations/server.go b/internal/gitaly/service/operations/server.go index 2842fb3aa8f..e513d4b7e10 100644 --- a/internal/gitaly/service/operations/server.go +++ b/internal/gitaly/service/operations/server.go @@ -47,12 +47,12 @@ func NewServer(deps *service.Dependencies) *Server { } } -func (s *Server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, error) { - quarantineDir, err := quarantine.New(ctx, repo, s.logger, s.locator) +func (s *Server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func() error, error) { + quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.logger, s.locator) if err != nil { - return nil, nil, structerr.NewInternal("creating object quarantine: %w", err) + return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) } quarantineRepo := s.localRepoFactory.Build(quarantineDir.QuarantinedRepo()) - return quarantineDir, quarantineRepo, nil + return quarantineDir, quarantineRepo, cleanup, nil } diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go index 8310938d694..8f084570e00 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -77,10 +78,15 @@ func validateUserSquashRequest(ctx context.Context, locator storage.Locator, req func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest) (string, error) { // All new objects are staged into a quarantine directory first so that we can do // transactional voting before we commit data to disk. - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return "", structerr.NewInternal("creating quarantine: %w", err) } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() // We need to retrieve the start commit such that we can create the new commit with // all parents of the start commit. diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go index cdf9cfce264..7eee56e83f3 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" @@ -25,10 +26,15 @@ func (s *Server) UserUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 29f3c3902fe..59099e48263 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -7,6 +7,7 @@ import ( "fmt" "regexp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -129,10 +130,15 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() objectHash, err := quarantineRepo.ObjectHash(ctx) if err != nil { diff --git a/internal/gitaly/service/operations/user_create_branch.go b/internal/gitaly/service/operations/user_create_branch.go index 5210f445498..6998279a50e 100644 --- a/internal/gitaly/service/operations/user_create_branch.go +++ b/internal/gitaly/service/operations/user_create_branch.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -34,10 +35,15 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB if err := validateUserCreateBranchRequest(ctx, s.locator, req); err != nil { return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, quarantineRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, quarantineRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() // BEGIN TODO: Uncomment if StartPoint started behaving sensibly // like BranchName. See diff --git a/internal/gitaly/service/operations/user_update_branch.go b/internal/gitaly/service/operations/user_update_branch.go index 08bc7db1a04..791f9222978 100644 --- a/internal/gitaly/service/operations/user_update_branch.go +++ b/internal/gitaly/service/operations/user_update_branch.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" @@ -63,10 +64,15 @@ func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateB referenceName := git.NewReferenceNameFromBranchName(string(req.GetBranchName())) - quarantineDir, _, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, _, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.GetUser(), quarantineDir, referenceName, newOID, oldOID); err != nil { var customHookErr updateref.CustomHookError diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 56a5d663dfd..ccc127e3c80 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -4,6 +4,7 @@ import ( "context" "errors" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/remoterepo" @@ -30,10 +31,15 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc return nil, structerr.NewInvalidArgument("%w", err) } - quarantineDir, targetRepo, err := s.quarantinedRepo(ctx, req.GetRepository()) + quarantineDir, targetRepo, cleanup, err := s.quarantinedRepo(ctx, req.GetRepository()) if err != nil { return nil, err } + defer func() { + if err := cleanup(); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to cleanup quarantine directory") + } + }() sourceRepo, err := remoterepo.New(ctx, req.GetSourceRepository(), s.conns) if err != nil { diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go index bf50ffd893d..655a3053098 100644 --- a/internal/gitaly/service/repository/fetch_remote.go +++ b/internal/gitaly/service/repository/fetch_remote.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" @@ -76,11 +77,11 @@ func (s *server) fetchRemoteAtomic(ctx context.Context, req *gitalypb.FetchRemot return false, false, err } - sshCommand, cleanup, err := gitcmd.BuildSSHInvocation(ctx, s.logger, req.GetSshKey(), req.GetKnownHosts()) + sshCommand, sshCleanup, err := gitcmd.BuildSSHInvocation(ctx, s.logger, req.GetSshKey(), req.GetKnownHosts()) if err != nil { return false, false, err } - defer cleanup() + defer sshCleanup() opts.Env = append(opts.Env, "GIT_SSH_COMMAND="+sshCommand) @@ -88,10 +89,15 @@ func (s *server) fetchRemoteAtomic(ctx context.Context, req *gitalypb.FetchRemot // to be updated, unreachable objects could be left in the repository that would need to be // garbage collected. To be more atomic, a quarantine directory is set up where objects will be // fetched prior to being migrated to the main repository when reference updates are committed. - quarantineDir, err := quarantine.New(ctx, req.GetRepository(), s.logger, s.locator) + quarantineDir, quarantineCleanup, err := quarantine.New(ctx, req.GetRepository(), s.logger, s.locator) if err != nil { return false, false, fmt.Errorf("creating quarantine directory: %w", err) } + defer func() { + if cleanupErr := quarantineCleanup(); cleanupErr != nil { + ctxlogrus.Extract(ctx).WithError(cleanupErr).Error("failed to cleanup quarantine directory") + } + }() quarantineRepo := s.localRepoFactory.Build(quarantineDir.QuarantinedRepo()) if err := quarantineRepo.FetchRemote(ctx, "inmemory", opts); err != nil { diff --git a/internal/gitaly/service/repository/server.go b/internal/gitaly/service/repository/server.go index 7dfa10dd2d9..1fc4f90829b 100644 --- a/internal/gitaly/service/repository/server.go +++ b/internal/gitaly/service/repository/server.go @@ -71,12 +71,12 @@ func NewServer(deps *service.Dependencies) gitalypb.RepositoryServiceServer { } } -func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, error) { - quarantineDir, err := quarantine.New(ctx, repo, s.logger, s.locator) +func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func() error, error) { + quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.logger, s.locator) if err != nil { - return nil, nil, structerr.NewInternal("creating object quarantine: %w", err) + return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) } quarantineRepo := s.localRepoFactory.Build(quarantineDir.QuarantinedRepo()) - return quarantineDir, quarantineRepo, nil + return quarantineDir, quarantineRepo, cleanup, nil } diff --git a/internal/gitaly/service/repository/size_test.go b/internal/gitaly/service/repository/size_test.go index 57e5e93a7aa..24240e7a612 100644 --- a/internal/gitaly/service/repository/size_test.go +++ b/internal/gitaly/service/repository/size_test.go @@ -228,8 +228,9 @@ func TestGetObjectDirectorySize_quarantine(t *testing.T) { requireObjectDirectorySize(t, ctx, client, repo, 16) - quarantine, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), logger, locator) + quarantine, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), logger, locator) require.NoError(t, err) + defer func() { _ = cleanup() }() // quarantine.New in Gitaly would receive an already rewritten repository. Gitaly would then calculate // the quarantine directories based on the rewritten relative path. That quarantine would then be looped @@ -276,12 +277,14 @@ func TestGetObjectDirectorySize_quarantine(t *testing.T) { t.Run("quarantined repo with different relative path", func(t *testing.T) { repo1, _ := gittest.CreateRepository(t, ctx, cfg) - quarantine1, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo1), logger, locator) + quarantine1, cleanup1, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo1), logger, locator) require.NoError(t, err) + defer func() { _ = cleanup1() }() repo2, _ := gittest.CreateRepository(t, ctx, cfg) - quarantine2, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo2), logger, locator) + quarantine2, cleanup2, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo2), logger, locator) require.NoError(t, err) + defer func() { _ = cleanup2() }() // We swap out the the object directories of both quarantines. So while both are // valid, we still expect that this RPC call fails because we detect that the diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go index f70c1216ce0..e52ca5c43eb 100644 --- a/internal/tempdir/tempdir.go +++ b/internal/tempdir/tempdir.go @@ -17,7 +17,6 @@ import ( type Dir struct { logger log.Logger path string - doneCh chan struct{} } // Path returns the absolute path of the temporary directory. @@ -25,54 +24,58 @@ func (d Dir) Path() string { return d.path } -// New returns the path of a new temporary directory for the given storage. The directory is removed -// asynchronously with os.RemoveAll when the context expires. -func New(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (Dir, error) { +// New returns the path of a new temporary directory for the given storage and a cleanup function +// that must be called to remove the directory. +func New(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (Dir, func() error, error) { return NewWithPrefix(ctx, storageName, "repo", logger, locator) } // NewWithPrefix returns the path of a new temporary directory for the given storage with a specific -// prefix used to create the temporary directory's name. The directory is removed asynchronously -// with os.RemoveAll when the context expires. -func NewWithPrefix(ctx context.Context, storageName, prefix string, logger log.Logger, locator storage.Locator) (Dir, error) { +// prefix used to create the temporary directory's name, and a cleanup function that must be called +// to remove the directory. +func NewWithPrefix(ctx context.Context, storageName, prefix string, logger log.Logger, locator storage.Locator) (Dir, func() error, error) { dir, err := newDirectory(ctx, storageName, prefix, logger, locator) if err != nil { - return Dir{}, err + return Dir{}, nil, err } - go dir.cleanupOnDone(ctx) + cleanup := func() error { + return os.RemoveAll(dir.path) + } - return dir, nil + return dir, cleanup, nil } // NewWithoutContext returns a temporary directory for the given storage suitable which is not -// storage scoped. The temporary directory will thus not get cleaned up when the context expires, -// but instead when the temporary directory is older than MaxAge. +// storage scoped. The temporary directory will thus not get cleaned up automatically. func NewWithoutContext(storageName string, logger log.Logger, locator storage.Locator) (Dir, error) { prefix := fmt.Sprintf("%s-repositories.old.%d.", storageName, time.Now().Unix()) - return newDirectory(context.Background(), storageName, prefix, logger, locator) + dir, err := newDirectory(context.Background(), storageName, prefix, logger, locator) + return dir, err } // NewRepository is the same as New, but it returns a *gitalypb.Repository for the created directory -// as well as the bare path as a string. -func NewRepository(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (*gitalypb.Repository, Dir, error) { +// as well as the bare path as a string, and a cleanup function that must be called to remove the directory. +func NewRepository(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (*gitalypb.Repository, Dir, func() error, error) { storagePath, err := locator.GetStorageByName(ctx, storageName) if err != nil { - return nil, Dir{}, err + return nil, Dir{}, nil, err } - dir, err := New(ctx, storageName, logger, locator) + dir, cleanup, err := New(ctx, storageName, logger, locator) if err != nil { - return nil, Dir{}, err + return nil, Dir{}, nil, err } newRepo := &gitalypb.Repository{StorageName: storageName} newRepo.RelativePath, err = filepath.Rel(storagePath, dir.Path()) if err != nil { - return nil, Dir{}, err + // Clean up the directory if we fail after creating it + cleanup() + return nil, Dir{}, nil, err } - return newRepo, dir, nil + return newRepo, dir, cleanup, nil } func newDirectory(ctx context.Context, storageName string, prefix string, logger log.Logger, loc storage.Locator) (Dir, error) { @@ -93,20 +96,5 @@ func newDirectory(ctx context.Context, storageName string, prefix string, logger return Dir{ logger: logger, path: tempDir, - doneCh: make(chan struct{}), }, err } - -func (d Dir) cleanupOnDone(ctx context.Context) { - <-ctx.Done() - if err := os.RemoveAll(d.Path()); err != nil { - d.logger.WithError(err).WithField("temporary_directory", d.Path).ErrorContext(ctx, "failed to cleanup temp dir") - } - close(d.doneCh) -} - -// WaitForCleanup waits until the temporary directory got removed via the asynchronous cleanupOnDone -// call. This is mainly intended for use in tests. -func (d Dir) WaitForCleanup() { - <-d.doneCh -} diff --git a/internal/tempdir/tempdir_test.go b/internal/tempdir/tempdir_test.go index bbfc1916826..559fbc4e1fa 100644 --- a/internal/tempdir/tempdir_test.go +++ b/internal/tempdir/tempdir_test.go @@ -1,7 +1,6 @@ package tempdir import ( - "context" "os" "path/filepath" "testing" @@ -15,13 +14,14 @@ import ( ) func TestNewRepositorySuccess(t *testing.T) { - ctx, cancel := context.WithCancel(testhelper.Context(t)) + ctx := testhelper.Context(t) cfg := testcfg.Build(t) locator := config.NewLocator(cfg) - repo, tempDir, err := NewRepository(ctx, cfg.Storages[0].Name, testhelper.NewLogger(t), locator) + repo, tempDir, cleanup, err := NewRepository(ctx, cfg.Storages[0].Name, testhelper.NewLogger(t), locator) require.NoError(t, err) + require.Equal(t, cfg.Storages[0].Name, repo.GetStorageName()) require.Contains(t, repo.GetRelativePath(), tmpRootPrefix) @@ -33,9 +33,8 @@ func TestNewRepositorySuccess(t *testing.T) { require.DirExists(t, tempDir.Path()) - cancel() // This should trigger async removal of the temporary directory - tempDir.WaitForCleanup() - + // Directory should be removed after cleanup + require.NoError(t, cleanup()) require.NoDirExists(t, tempDir.Path()) } @@ -44,14 +43,15 @@ func TestNewWithPrefix(t *testing.T) { locator := config.NewLocator(cfg) ctx := testhelper.Context(t) - dir, err := NewWithPrefix(ctx, cfg.Storages[0].Name, "foobar-", testhelper.NewLogger(t), locator) + dir, cleanup, err := NewWithPrefix(ctx, cfg.Storages[0].Name, "foobar-", testhelper.NewLogger(t), locator) require.NoError(t, err) + defer func() { _ = cleanup() }() require.Contains(t, dir.Path(), "/foobar-") } func TestNewAsRepositoryFailStorageUnknown(t *testing.T) { ctx := testhelper.Context(t) - _, err := New(ctx, "does-not-exist", testhelper.NewLogger(t), config.NewLocator(config.Cfg{})) + _, _, err := New(ctx, "does-not-exist", testhelper.NewLogger(t), config.NewLocator(config.Cfg{})) require.Error(t, err) } -- GitLab From fdc32511eede90a74eba8428abf25a118967ee89 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Wed, 30 Jul 2025 10:48:44 +0530 Subject: [PATCH 2/2] Fix errcheck linting and test race conditions in tempdir cleanup Address two issues identified after the tempdir cleanup refactoring: 1. Fix errcheck linter warnings by explicitly ignoring cleanup function errors in error paths using `_ = cleanup()` instead of bare `cleanup()`. In error scenarios, we want to return the primary error rather than a potential cleanup error. 2. Fix race conditions in blob service tests by using `t.Cleanup()` instead of `defer` for quarantine cleanup. This ensures the temporary directories are not cleaned up until after the test and all Git commands have completed, preventing "fatal: not a git repository" errors. Changelog: fixed Signed-off-by: Siddharth Asthana --- internal/git/localrepo/bundle.go | 4 ++-- internal/git/quarantine/quarantine.go | 2 +- internal/gitaly/service/blob/blobs_test.go | 2 +- internal/gitaly/service/blob/lfs_pointers_test.go | 4 ++-- internal/gitaly/service/cleanup/rewrite_history.go | 6 +++--- internal/tempdir/tempdir.go | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/git/localrepo/bundle.go b/internal/git/localrepo/bundle.go index 664d33e11f7..0e091fa9d03 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -213,7 +213,7 @@ func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundl file, err := os.Create(bundlePath) if err != nil { - cleanup() // Clean up if we fail after creating the temp directory + _ = cleanup() // Clean up if we fail after creating the temp directory return "", nil, fmt.Errorf("create temp bundle: %w", err) } defer func() { @@ -223,7 +223,7 @@ func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundl }() if _, err = io.Copy(file, reader); err != nil { - cleanup() // Clean up if we fail after creating the temp directory + _ = cleanup() // Clean up if we fail after creating the temp directory return "", nil, fmt.Errorf("create temp bundle: %w", err) } diff --git a/internal/git/quarantine/quarantine.go b/internal/git/quarantine/quarantine.go index cee1ce1ffe1..1bc5dbe3c97 100644 --- a/internal/git/quarantine/quarantine.go +++ b/internal/git/quarantine/quarantine.go @@ -48,7 +48,7 @@ func New(ctx context.Context, repo *gitalypb.Repository, logger log.Logger, loca quarantinedRepo, err := Apply(repoPath, repo, quarantineDir.Path()) if err != nil { - cleanup() // Clean up if we fail after creating the temp directory + _ = cleanup() // Clean up if we fail after creating the temp directory return nil, nil, err } diff --git a/internal/gitaly/service/blob/blobs_test.go b/internal/gitaly/service/blob/blobs_test.go index 6cab6d8c7d8..1de2d551de8 100644 --- a/internal/gitaly/service/blob/blobs_test.go +++ b/internal/gitaly/service/blob/blobs_test.go @@ -333,7 +333,7 @@ func TestListAllBlobs(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) require.NoError(t, err) - defer func() { _ = cleanup() }() + t.Cleanup(func() { _ = cleanup() }) // quarantine.New in Gitaly would receive an already rewritten repository. Gitaly would then calculate // the quarantine directories based on the rewritten relative path. That quarantine would then be looped diff --git a/internal/gitaly/service/blob/lfs_pointers_test.go b/internal/gitaly/service/blob/lfs_pointers_test.go index 1f165c2d904..54925ac464c 100644 --- a/internal/gitaly/service/blob/lfs_pointers_test.go +++ b/internal/gitaly/service/blob/lfs_pointers_test.go @@ -220,7 +220,7 @@ size 12345` quarantineDir, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) require.NoError(t, err) - defer func() { _ = cleanup() }() + t.Cleanup(func() { _ = cleanup() }) repo.GitObjectDirectory = quarantineDir.QuarantinedRepo().GetGitObjectDirectory() @@ -244,7 +244,7 @@ size 12345` // has its object directory set to the quarantine directory. quarantineDir, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), testhelper.NewLogger(t), config.NewLocator(cfg)) require.NoError(t, err) - defer func() { _ = cleanup() }() + t.Cleanup(func() { _ = cleanup() }) // Note that we need to continue using the non-rewritten repository // here as `localrepo.NewTestRepo()` already will try to rewrite it diff --git a/internal/gitaly/service/cleanup/rewrite_history.go b/internal/gitaly/service/cleanup/rewrite_history.go index a11b795b2f5..8866b49a9d0 100644 --- a/internal/gitaly/service/cleanup/rewrite_history.go +++ b/internal/gitaly/service/cleanup/rewrite_history.go @@ -207,12 +207,12 @@ func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, Args: []string{stagingRepoDir.Path()}, }, gitcmd.WithStderr(&stderr)) if err != nil { - cleanup() // Clean up on error + _ = cleanup() // Clean up on error return nil, "", nil, fmt.Errorf("spawning git-init: %w", err) } if err := cmd.Wait(); err != nil { - cleanup() // Clean up on error + _ = cleanup() // Clean up on error return nil, "", nil, structerr.New("creating repository: %w", err).WithMetadata("stderr", &stderr) } @@ -221,7 +221,7 @@ func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, // Ensure HEAD matches the source repository. In practice a mismatch doesn't cause problems, // but out of an abundance of caution let's keep the two repos as similar as possible. if err := stagingRepo.SetDefaultBranch(ctx, s.txManager, defaultBranch); err != nil { - cleanup() // Clean up on error + _ = cleanup() // Clean up on error return nil, "", nil, fmt.Errorf("setting default branch: %w", err) } diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go index e52ca5c43eb..08d55f8d0f1 100644 --- a/internal/tempdir/tempdir.go +++ b/internal/tempdir/tempdir.go @@ -71,7 +71,7 @@ func NewRepository(ctx context.Context, storageName string, logger log.Logger, l newRepo.RelativePath, err = filepath.Rel(storagePath, dir.Path()) if err != nil { // Clean up the directory if we fail after creating it - cleanup() + _ = cleanup() return nil, Dir{}, nil, err } -- GitLab