From b16d9be0645b6d0157ed9baff6b819800d87d7f6 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Wed, 27 Aug 2025 23:08:32 +0530 Subject: [PATCH 1/5] 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 The cleanup function logs any errors but does not return them, aligning with the existing behavior where errors were logged but not propagated. This reduces complexity as callers don't need to handle cleanup errors. Use t.Cleanup() in tests instead of defer to ensure proper test synchronization and prevent race conditions where directories are deleted before Git commands complete. 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 | 25 +++++--- 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 | 9 ++- internal/gitaly/service/blob/blobs_test.go | 3 +- .../gitaly/service/blob/lfs_pointers_test.go | 6 +- .../gitaly/service/cleanup/rewrite_history.go | 29 ++++++---- .../service/conflicts/list_conflict_files.go | 5 +- .../service/conflicts/resolve_conflicts.go | 5 +- internal/gitaly/service/conflicts/server.go | 8 +-- internal/gitaly/service/diff/diff_blobs.go | 5 +- .../gitaly/service/operations/cherry_pick.go | 5 +- .../gitaly/service/operations/commit_files.go | 5 +- .../gitaly/service/operations/ff_branch.go | 5 +- .../gitaly/service/operations/merge_branch.go | 5 +- .../service/operations/rebase_confirmable.go | 5 +- .../service/operations/rebase_to_ref.go | 5 +- internal/gitaly/service/operations/revert.go | 5 +- internal/gitaly/service/operations/server.go | 8 +-- internal/gitaly/service/operations/squash.go | 5 +- .../gitaly/service/operations/submodules.go | 5 +- internal/gitaly/service/operations/tags.go | 5 +- .../service/operations/user_create_branch.go | 5 +- .../service/operations/user_update_branch.go | 5 +- internal/gitaly/service/repository/fetch.go | 5 +- .../gitaly/service/repository/fetch_remote.go | 9 ++- 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, 204 insertions(+), 130 deletions(-) diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go index db5ae2c67e4..9b6485e8e6d 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) + t.Cleanup(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..5799b903614 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) + t.Cleanup(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..f7024c70f99 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -77,10 +77,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() repoPath, err := repo.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) if err != nil { @@ -156,10 +159,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() fetchConfig := []gitcmd.ConfigPair{ {Key: "remote.inmemory.url", Value: bundlePath}, @@ -192,17 +198,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 +219,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..af708ca5141 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) + t.Cleanup(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..1bc5dbe3c97 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..d85f4960d94 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) + t.Cleanup(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..8313643d37c 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) + t.Cleanup(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) + t.Cleanup(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) + t.Cleanup(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) + t.Cleanup(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..e0251ba9727 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) + t.Cleanup(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..4fc560e4e58 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) + t.Cleanup(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..5a5be890080 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) + t.Cleanup(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..657cfd3dc8f 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) + t.Cleanup(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..747f2807173 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -104,15 +104,14 @@ 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. It will eventually get + // cleaned by the tempdir walker of the OS when it's old enough. + _ = 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..1de2d551de8 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) + 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 b4dffb415d5..54925ac464c 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) + t.Cleanup(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) + 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 16bf818cfef..e201e577e68 100644 --- a/internal/gitaly/service/cleanup/rewrite_history.go +++ b/internal/gitaly/service/cleanup/rewrite_history.go @@ -110,10 +110,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() // Check state of source repository prior to running filter-repo. initialChecksum, err := checksumRepo(ctx, repo) @@ -185,11 +188,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 +205,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() + 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() + return nil, "", nil, structerr.New("creating repository: %w", err).WithMetadata("stderr", &stderr) } stagingRepo := s.localRepoFactory.Build(stagingRepoProto) @@ -214,10 +219,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() + 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 +233,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..95824a5535b 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -23,10 +23,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..18062553841 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -143,10 +143,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..4120974dd02 100644 --- a/internal/gitaly/service/diff/diff_blobs.go +++ b/internal/gitaly/service/diff/diff_blobs.go @@ -148,10 +148,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..36c39f98818 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -21,10 +21,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..027f317674d 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -521,10 +521,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..8f0d822a22b 100644 --- a/internal/gitaly/service/operations/ff_branch.go +++ b/internal/gitaly/service/operations/ff_branch.go @@ -23,10 +23,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..ca185f4079f 100644 --- a/internal/gitaly/service/operations/merge_branch.go +++ b/internal/gitaly/service/operations/merge_branch.go @@ -29,10 +29,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..ff87bace8f9 100644 --- a/internal/gitaly/service/operations/rebase_confirmable.go +++ b/internal/gitaly/service/operations/rebase_confirmable.go @@ -31,10 +31,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..79da24ff475 100644 --- a/internal/gitaly/service/operations/rebase_to_ref.go +++ b/internal/gitaly/service/operations/rebase_to_ref.go @@ -18,10 +18,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..09db166c62e 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -20,10 +20,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..a3978e7aa80 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -77,10 +77,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() // 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..1f853e703f9 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -25,10 +25,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..aa5f80a054e 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -129,10 +129,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..bc72f0eec12 100644 --- a/internal/gitaly/service/operations/user_create_branch.go +++ b/internal/gitaly/service/operations/user_create_branch.go @@ -34,10 +34,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() // 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..081f3a71bed 100644 --- a/internal/gitaly/service/operations/user_update_branch.go +++ b/internal/gitaly/service/operations/user_update_branch.go @@ -63,10 +63,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..7246f4e499d 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -30,10 +30,13 @@ 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() { + _ = cleanup() // Errors are logged by the tempdir package + }() 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..6b00e77cb29 100644 --- a/internal/gitaly/service/repository/fetch_remote.go +++ b/internal/gitaly/service/repository/fetch_remote.go @@ -76,11 +76,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 +88,13 @@ 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() { + _ = quarantineCleanup() // Errors are logged by the tempdir package + }() 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..146c2b5e48a 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) + 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 @@ -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..66ce64f010e 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,60 @@ 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 { + if err := os.RemoveAll(dir.path); err != nil { + logger.WithError(err).WithField("temporary_directory", dir.path).Error("failed to cleanup temp dir") + } + return nil + } - 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) } // 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 +98,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..51b6e15bbcd 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) + t.Cleanup(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 13e3523827de44082b5062ec5848117e47eaf10b Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Fri, 29 Aug 2025 02:34:39 +0530 Subject: [PATCH 2/5] Tempdir: remove error return from cleanup functions Cleanup functions no longer need to return errors since they only log failures internally and always returned nil. This simplifies the API by removing unused error handling: - Update all tempdir.New* functions to return func() instead of func() error - Update quarantine.New to return func() instead of func() error - Update all quarantinedRepo helpers to return func() instead of func() error - Simplify all cleanup call sites to use cleanup() instead of _ = cleanup() - Update tests to use t.Cleanup(cleanup) instead of t.Cleanup(func() { _ = cleanup() }) The cleanup functions continue to log errors as before, but callers no longer need to handle meaningless error returns. This addresses the feedback that since we're logging errors internally, returning them serves no purpose. Signed-off-by: Siddharth Asthana --- internal/git/catfile/cache_test.go | 2 +- internal/git/catfile/object_reader_test.go | 2 +- internal/git/localrepo/bundle.go | 10 +++++----- internal/git/localrepo/paths_test.go | 2 +- internal/git/quarantine/quarantine.go | 4 ++-- internal/git/quarantine/quarantine_ext_test.go | 2 +- internal/git/quarantine/quarantine_test.go | 12 ++++++------ internal/gitaly/hook/postreceive_test.go | 2 +- internal/gitaly/hook/prereceive_test.go | 2 +- internal/gitaly/hook/update_test.go | 2 +- .../gitaly/hook/updateref/update_with_hooks_test.go | 2 +- internal/gitaly/repoutil/create.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 | 12 ++++++------ .../gitaly/service/conflicts/list_conflict_files.go | 2 +- .../gitaly/service/conflicts/resolve_conflicts.go | 2 +- internal/gitaly/service/conflicts/server.go | 2 +- internal/gitaly/service/diff/diff_blobs.go | 2 +- internal/gitaly/service/operations/cherry_pick.go | 2 +- internal/gitaly/service/operations/commit_files.go | 2 +- internal/gitaly/service/operations/ff_branch.go | 2 +- internal/gitaly/service/operations/merge_branch.go | 2 +- .../gitaly/service/operations/rebase_confirmable.go | 2 +- internal/gitaly/service/operations/rebase_to_ref.go | 2 +- internal/gitaly/service/operations/revert.go | 2 +- internal/gitaly/service/operations/server.go | 2 +- internal/gitaly/service/operations/squash.go | 2 +- internal/gitaly/service/operations/submodules.go | 2 +- internal/gitaly/service/operations/tags.go | 2 +- .../gitaly/service/operations/user_create_branch.go | 2 +- .../gitaly/service/operations/user_update_branch.go | 2 +- internal/gitaly/service/repository/fetch.go | 2 +- internal/gitaly/service/repository/fetch_remote.go | 2 +- internal/gitaly/service/repository/server.go | 2 +- internal/gitaly/service/repository/size_test.go | 6 +++--- internal/tempdir/tempdir.go | 11 +++++------ internal/tempdir/tempdir_test.go | 4 ++-- 38 files changed, 61 insertions(+), 62 deletions(-) diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go index 9b6485e8e6d..f893fa5c84e 100644 --- a/internal/git/catfile/cache_test.go +++ b/internal/git/catfile/cache_test.go @@ -435,7 +435,7 @@ func TestCache_ObjectReader_quarantine(t *testing.T) { quarantineDir, cleanup, err := quarantine.New(testhelper.Context(t), repo, logger, locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(cleanup) quarantineRepo := quarantineDir.QuarantinedRepo() diff --git a/internal/git/catfile/object_reader_test.go b/internal/git/catfile/object_reader_test.go index 5799b903614..d1a39842e8e 100644 --- a/internal/git/catfile/object_reader_test.go +++ b/internal/git/catfile/object_reader_test.go @@ -722,7 +722,7 @@ func TestObjectReader_logging(t *testing.T) { quarantineDir, cleanup, err := quarantine.New(ctx, repoProto, logger, locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 f7024c70f99..03e477abd66 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -82,7 +82,7 @@ func (repo *Repo) CloneBundle(ctx context.Context, reader io.Reader) error { return err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() repoPath, err := repo.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) @@ -164,7 +164,7 @@ func (repo *Repo) FetchBundle(ctx context.Context, txManager transaction.Manager return fmt.Errorf("fetch bundle: %w", err) } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() fetchConfig := []gitcmd.ConfigPair{ @@ -199,7 +199,7 @@ 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. // 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) { +func (repo *Repo) createTempBundle(ctx context.Context, reader io.Reader) (bundlPath string, cleanup func(), returnErr error) { tmpDir, cleanup, err := tempdir.New(ctx, repo.GetStorageName(), repo.logger, repo.locator) if err != nil { return "", nil, fmt.Errorf("create temp bundle: %w", err) @@ -209,7 +209,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() { @@ -219,7 +219,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/localrepo/paths_test.go b/internal/git/localrepo/paths_test.go index af708ca5141..23b384595d8 100644 --- a/internal/git/localrepo/paths_test.go +++ b/internal/git/localrepo/paths_test.go @@ -75,7 +75,7 @@ func TestRepo_ObjectDirectoryPath(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 1bc5dbe3c97..ffe03515901 100644 --- a/internal/git/quarantine/quarantine.go +++ b/internal/git/quarantine/quarantine.go @@ -34,7 +34,7 @@ type Dir struct { // 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) { +func New(ctx context.Context, repo *gitalypb.Repository, logger log.Logger, locator storage.Locator) (*Dir, func(), error) { repoPath, err := locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) if err != nil { return nil, nil, structerr.NewInternal("getting repo path: %w", err) @@ -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/git/quarantine/quarantine_ext_test.go b/internal/git/quarantine/quarantine_ext_test.go index d85f4960d94..99201c41be0 100644 --- a/internal/git/quarantine/quarantine_ext_test.go +++ b/internal/git/quarantine/quarantine_ext_test.go @@ -30,7 +30,7 @@ func TestQuarantine_localrepo(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 8313643d37c..af044c268e3 100644 --- a/internal/git/quarantine/quarantine_test.go +++ b/internal/git/quarantine/quarantine_test.go @@ -51,7 +51,7 @@ func TestQuarantine_lifecycle(t *testing.T) { t.Run("quarantine directory gets created", func(t *testing.T) { quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(cleanup) relativeQuarantinePath, err := filepath.Rel(repoPath, quarantine.dir.Path()) require.NoError(t, err) @@ -77,7 +77,7 @@ func TestQuarantine_lifecycle(t *testing.T) { require.NoError(t, err) require.DirExists(t, quarantine.dir.Path()) - require.NoError(t, cleanup()) + cleanup() require.NoDirExists(t, quarantine.dir.Path()) }) } @@ -101,7 +101,7 @@ func TestQuarantine_Migrate(t *testing.T) { quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(cleanup) require.NoError(t, quarantine.Migrate(ctx)) @@ -120,7 +120,7 @@ func TestQuarantine_Migrate(t *testing.T) { quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(cleanup) require.NoError(t, os.WriteFile(filepath.Join(quarantine.dir.Path(), "file"), []byte("foobar"), mode.File)) require.NoError(t, quarantine.Migrate(ctx)) @@ -144,7 +144,7 @@ func TestQuarantine_Migrate(t *testing.T) { quarantine, cleanup, err := New(ctx, repo, logger, locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(cleanup) require.Empty(t, listEntries(t, quarantine.dir.Path())) @@ -153,7 +153,7 @@ func TestQuarantine_Migrate(t *testing.T) { // main repository should stay untouched. recursiveQuarantine, recursiveCleanup, err := New(ctx, quarantine.QuarantinedRepo(), logger, locator) require.NoError(t, err) - defer func() { _ = recursiveCleanup() }() + 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 e0251ba9727..90ca984345e 100644 --- a/internal/gitaly/hook/postreceive_test.go +++ b/internal/gitaly/hook/postreceive_test.go @@ -429,7 +429,7 @@ func TestPostReceive_quarantine(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 4fc560e4e58..8b288464872 100644 --- a/internal/gitaly/hook/prereceive_test.go +++ b/internal/gitaly/hook/prereceive_test.go @@ -225,7 +225,7 @@ func TestPrereceive_quarantine(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 5a5be890080..6cf6e15cdac 100644 --- a/internal/gitaly/hook/update_test.go +++ b/internal/gitaly/hook/update_test.go @@ -255,7 +255,7 @@ func TestUpdate_quarantine(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.SharedLogger(t), config.NewLocator(cfg)) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 657cfd3dc8f..99fb5f41181 100644 --- a/internal/gitaly/hook/updateref/update_with_hooks_test.go +++ b/internal/gitaly/hook/updateref/update_with_hooks_test.go @@ -320,7 +320,7 @@ func TestUpdaterWithHooks_quarantine(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, repoProto, testhelper.NewLogger(t), locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 747f2807173..8c8e7185176 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -111,7 +111,7 @@ func Create( defer func() { // We don't really care about whether this succeeds or not. It will eventually get // cleaned by the tempdir walker of the OS when it's old enough. - _ = cleanup() + 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 1de2d551de8..a6bc287c1d9 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) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 54925ac464c..c21c9ec1f0d 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) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 e201e577e68..e656cd65047 100644 --- a/internal/gitaly/service/cleanup/rewrite_history.go +++ b/internal/gitaly/service/cleanup/rewrite_history.go @@ -115,7 +115,7 @@ func (s *server) rewriteHistory( return fmt.Errorf("setting up staging repo: %w", err) } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() // Check state of source repository prior to running filter-repo. @@ -189,7 +189,7 @@ 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. 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) { +func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, defaultBranch git.ReferenceName) (*localrepo.Repo, string, func(), error) { stagingRepoProto, stagingRepoDir, cleanup, err := tempdir.NewRepository(ctx, repo.GetStorageName(), s.logger, s.locator) if err != nil { return nil, "", nil, err @@ -205,12 +205,12 @@ func (s *server) initStagingRepo(ctx context.Context, repo *gitalypb.Repository, Args: []string{stagingRepoDir.Path()}, }, gitcmd.WithStderr(&stderr)) if err != nil { - _ = cleanup() + cleanup() return nil, "", nil, fmt.Errorf("spawning git-init: %w", err) } if err := cmd.Wait(); err != nil { - _ = cleanup() + cleanup() return nil, "", nil, structerr.New("creating repository: %w", err).WithMetadata("stderr", &stderr) } @@ -219,7 +219,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() + cleanup() return nil, "", nil, fmt.Errorf("setting default branch: %w", err) } @@ -238,7 +238,7 @@ func (s *server) runFilterRepo( return fmt.Errorf("create tempdir: %w", err) } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() 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 95824a5535b..8c551754f96 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -28,7 +28,7 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s return err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() ours, err := quarantineRepo.ResolveRevision(ctx, git.Revision(request.GetOurCommitOid()+"^{commit}")) diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index 18062553841..3bf4d6e3b47 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -148,7 +148,7 @@ func (s *server) resolveConflicts(header *gitalypb.ResolveConflictsRequestHeader return err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() if err := s.repoWithBranchCommit(ctx, diff --git a/internal/gitaly/service/conflicts/server.go b/internal/gitaly/service/conflicts/server.go index 74c1602ac2b..d2c43fd8f24 100644 --- a/internal/gitaly/service/conflicts/server.go +++ b/internal/gitaly/service/conflicts/server.go @@ -40,7 +40,7 @@ func NewServer(deps *service.Dependencies) gitalypb.ConflictsServiceServer { } } -func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func() error, error) { +func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func(), error) { quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.logger, s.locator) if err != nil { return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) diff --git a/internal/gitaly/service/diff/diff_blobs.go b/internal/gitaly/service/diff/diff_blobs.go index 4120974dd02..9239bec53ff 100644 --- a/internal/gitaly/service/diff/diff_blobs.go +++ b/internal/gitaly/service/diff/diff_blobs.go @@ -153,7 +153,7 @@ func (s *server) diffBlobs(ctx context.Context, return structerr.NewInternal("creating quarantine directory: %w", err) } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() 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 36c39f98818..abe20838614 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -26,7 +26,7 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() startRevision, err := s.fetchStartRevision(ctx, quarantineRepo, req) diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 027f317674d..c245cf97a3e 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -526,7 +526,7 @@ func (s *Server) userCommitFiles( return err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() repoPath, err := quarantineRepo.Path(ctx) diff --git a/internal/gitaly/service/operations/ff_branch.go b/internal/gitaly/service/operations/ff_branch.go index 8f0d822a22b..bdaceb2848f 100644 --- a/internal/gitaly/service/operations/ff_branch.go +++ b/internal/gitaly/service/operations/ff_branch.go @@ -28,7 +28,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() objectHash, err := quarantineRepo.ObjectHash(ctx) diff --git a/internal/gitaly/service/operations/merge_branch.go b/internal/gitaly/service/operations/merge_branch.go index ca185f4079f..c2f6c080791 100644 --- a/internal/gitaly/service/operations/merge_branch.go +++ b/internal/gitaly/service/operations/merge_branch.go @@ -34,7 +34,7 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc return err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() objectHash, err := quarantineRepo.ObjectHash(ctx) diff --git a/internal/gitaly/service/operations/rebase_confirmable.go b/internal/gitaly/service/operations/rebase_confirmable.go index ff87bace8f9..125dfeb1527 100644 --- a/internal/gitaly/service/operations/rebase_confirmable.go +++ b/internal/gitaly/service/operations/rebase_confirmable.go @@ -36,7 +36,7 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba return structerr.NewInternal("creating repo quarantine: %w", err) } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() objectHash, err := quarantineRepo.ObjectHash(ctx) diff --git a/internal/gitaly/service/operations/rebase_to_ref.go b/internal/gitaly/service/operations/rebase_to_ref.go index 79da24ff475..6dc019f57a5 100644 --- a/internal/gitaly/service/operations/rebase_to_ref.go +++ b/internal/gitaly/service/operations/rebase_to_ref.go @@ -23,7 +23,7 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba return nil, structerr.NewInternal("creating repo quarantine: %w", err) } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() oid, err := quarantineRepo.ResolveRevision(ctx, git.Revision(request.GetFirstParentRef())) diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index 09db166c62e..b6b6fd11ede 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -25,7 +25,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() startRevision, err := s.fetchStartRevision(ctx, quarantineRepo, req) diff --git a/internal/gitaly/service/operations/server.go b/internal/gitaly/service/operations/server.go index e513d4b7e10..90866018f4d 100644 --- a/internal/gitaly/service/operations/server.go +++ b/internal/gitaly/service/operations/server.go @@ -47,7 +47,7 @@ func NewServer(deps *service.Dependencies) *Server { } } -func (s *Server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func() error, error) { +func (s *Server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func(), error) { quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.logger, s.locator) if err != nil { return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go index a3978e7aa80..62b97ec49ac 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -82,7 +82,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest return "", structerr.NewInternal("creating quarantine: %w", err) } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() // We need to retrieve the start commit such that we can create the new commit with diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go index 1f853e703f9..d7cc0ca8865 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -30,7 +30,7 @@ func (s *Server) UserUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() objectHash, err := quarantineRepo.ObjectHash(ctx) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index aa5f80a054e..6e000eca0df 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -134,7 +134,7 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() objectHash, err := quarantineRepo.ObjectHash(ctx) diff --git a/internal/gitaly/service/operations/user_create_branch.go b/internal/gitaly/service/operations/user_create_branch.go index bc72f0eec12..b6e186e57c1 100644 --- a/internal/gitaly/service/operations/user_create_branch.go +++ b/internal/gitaly/service/operations/user_create_branch.go @@ -39,7 +39,7 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() // BEGIN TODO: Uncomment if StartPoint started behaving sensibly diff --git a/internal/gitaly/service/operations/user_update_branch.go b/internal/gitaly/service/operations/user_update_branch.go index 081f3a71bed..9c15ed2b36e 100644 --- a/internal/gitaly/service/operations/user_update_branch.go +++ b/internal/gitaly/service/operations/user_update_branch.go @@ -68,7 +68,7 @@ func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateB return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.GetUser(), quarantineDir, referenceName, newOID, oldOID); err != nil { diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 7246f4e499d..8ac08d190c9 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -35,7 +35,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc return nil, err } defer func() { - _ = cleanup() // Errors are logged by the tempdir package + cleanup() // Errors are logged by the tempdir package }() sourceRepo, err := remoterepo.New(ctx, req.GetSourceRepository(), s.conns) diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go index 6b00e77cb29..e0799a26652 100644 --- a/internal/gitaly/service/repository/fetch_remote.go +++ b/internal/gitaly/service/repository/fetch_remote.go @@ -93,7 +93,7 @@ func (s *server) fetchRemoteAtomic(ctx context.Context, req *gitalypb.FetchRemot return false, false, fmt.Errorf("creating quarantine directory: %w", err) } defer func() { - _ = quarantineCleanup() // Errors are logged by the tempdir package + quarantineCleanup() // Errors are logged by the tempdir package }() quarantineRepo := s.localRepoFactory.Build(quarantineDir.QuarantinedRepo()) diff --git a/internal/gitaly/service/repository/server.go b/internal/gitaly/service/repository/server.go index 1fc4f90829b..6ea59403520 100644 --- a/internal/gitaly/service/repository/server.go +++ b/internal/gitaly/service/repository/server.go @@ -71,7 +71,7 @@ func NewServer(deps *service.Dependencies) gitalypb.RepositoryServiceServer { } } -func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func() error, error) { +func (s *server) quarantinedRepo(ctx context.Context, repo *gitalypb.Repository) (*quarantine.Dir, *localrepo.Repo, func(), error) { quarantineDir, cleanup, err := quarantine.New(ctx, repo, s.logger, s.locator) if err != nil { return nil, nil, nil, structerr.NewInternal("creating object quarantine: %w", err) diff --git a/internal/gitaly/service/repository/size_test.go b/internal/gitaly/service/repository/size_test.go index 146c2b5e48a..899fc38f2e7 100644 --- a/internal/gitaly/service/repository/size_test.go +++ b/internal/gitaly/service/repository/size_test.go @@ -230,7 +230,7 @@ func TestGetObjectDirectorySize_quarantine(t *testing.T) { quarantine, cleanup, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo), logger, locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(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 @@ -279,12 +279,12 @@ func TestGetObjectDirectorySize_quarantine(t *testing.T) { repo1, _ := gittest.CreateRepository(t, ctx, cfg) quarantine1, cleanup1, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo1), logger, locator) require.NoError(t, err) - defer func() { _ = cleanup1() }() + defer func() { cleanup1() }() repo2, _ := gittest.CreateRepository(t, ctx, cfg) quarantine2, cleanup2, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo2), logger, locator) require.NoError(t, err) - defer func() { _ = cleanup2() }() + 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 66ce64f010e..a8f4a4fed23 100644 --- a/internal/tempdir/tempdir.go +++ b/internal/tempdir/tempdir.go @@ -26,24 +26,23 @@ func (d Dir) Path() string { // 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) { +func New(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (Dir, func(), 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, 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) { +func NewWithPrefix(ctx context.Context, storageName, prefix string, logger log.Logger, locator storage.Locator) (Dir, func(), error) { dir, err := newDirectory(ctx, storageName, prefix, logger, locator) if err != nil { return Dir{}, nil, err } - cleanup := func() error { + cleanup := func() { if err := os.RemoveAll(dir.path); err != nil { logger.WithError(err).WithField("temporary_directory", dir.path).Error("failed to cleanup temp dir") } - return nil } return dir, cleanup, nil @@ -58,7 +57,7 @@ func NewWithoutContext(storageName string, logger log.Logger, locator storage.Lo // 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, 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) { +func NewRepository(ctx context.Context, storageName string, logger log.Logger, locator storage.Locator) (*gitalypb.Repository, Dir, func(), error) { storagePath, err := locator.GetStorageByName(ctx, storageName) if err != nil { return nil, Dir{}, nil, err @@ -73,7 +72,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 } diff --git a/internal/tempdir/tempdir_test.go b/internal/tempdir/tempdir_test.go index 51b6e15bbcd..7fa191e7da1 100644 --- a/internal/tempdir/tempdir_test.go +++ b/internal/tempdir/tempdir_test.go @@ -34,7 +34,7 @@ func TestNewRepositorySuccess(t *testing.T) { require.DirExists(t, tempDir.Path()) // Directory should be removed after cleanup - require.NoError(t, cleanup()) + cleanup() require.NoDirExists(t, tempDir.Path()) } @@ -45,7 +45,7 @@ func TestNewWithPrefix(t *testing.T) { dir, cleanup, err := NewWithPrefix(ctx, cfg.Storages[0].Name, "foobar-", testhelper.NewLogger(t), locator) require.NoError(t, err) - t.Cleanup(func() { _ = cleanup() }) + t.Cleanup(cleanup) require.Contains(t, dir.Path(), "/foobar-") } -- GitLab From 29d603b61fead56c299da4264d0bc1690aa9d8e6 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Mon, 1 Sep 2025 08:01:08 +0530 Subject: [PATCH 3/5] Use t.Cleanup() consistently and simplify defer cleanup patterns Replace anonymous function defer patterns with direct cleanup calls in non-test files and use t.Cleanup() in test files for better consistency. --- internal/git/quarantine/quarantine_test.go | 2 +- internal/gitaly/service/cleanup/rewrite_history.go | 8 ++------ internal/gitaly/service/repository/size_test.go | 4 ++-- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/internal/git/quarantine/quarantine_test.go b/internal/git/quarantine/quarantine_test.go index af044c268e3..e42855e39f9 100644 --- a/internal/git/quarantine/quarantine_test.go +++ b/internal/git/quarantine/quarantine_test.go @@ -153,7 +153,7 @@ func TestQuarantine_Migrate(t *testing.T) { // main repository should stay untouched. recursiveQuarantine, recursiveCleanup, err := New(ctx, quarantine.QuarantinedRepo(), logger, locator) require.NoError(t, err) - defer func() { recursiveCleanup() }() + t.Cleanup(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/service/cleanup/rewrite_history.go b/internal/gitaly/service/cleanup/rewrite_history.go index e656cd65047..de7fdc8f157 100644 --- a/internal/gitaly/service/cleanup/rewrite_history.go +++ b/internal/gitaly/service/cleanup/rewrite_history.go @@ -114,9 +114,7 @@ func (s *server) rewriteHistory( if err != nil { return fmt.Errorf("setting up staging repo: %w", err) } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() // Check state of source repository prior to running filter-repo. initialChecksum, err := checksumRepo(ctx, repo) @@ -237,9 +235,7 @@ func (s *server) runFilterRepo( if err != nil { return fmt.Errorf("create tempdir: %w", err) } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() flags := make([]gitcmd.Option, 0, 2) diff --git a/internal/gitaly/service/repository/size_test.go b/internal/gitaly/service/repository/size_test.go index 899fc38f2e7..c22729d227b 100644 --- a/internal/gitaly/service/repository/size_test.go +++ b/internal/gitaly/service/repository/size_test.go @@ -279,12 +279,12 @@ func TestGetObjectDirectorySize_quarantine(t *testing.T) { repo1, _ := gittest.CreateRepository(t, ctx, cfg) quarantine1, cleanup1, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo1), logger, locator) require.NoError(t, err) - defer func() { cleanup1() }() + t.Cleanup(cleanup1) repo2, _ := gittest.CreateRepository(t, ctx, cfg) quarantine2, cleanup2, err := quarantine.New(ctx, gittest.RewrittenRepository(t, ctx, cfg, repo2), logger, locator) require.NoError(t, err) - defer func() { cleanup2() }() + t.Cleanup(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 -- GitLab From e54fe2b2473ac99a66b02e65b6637d006745cddd Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Mon, 1 Sep 2025 09:16:10 +0530 Subject: [PATCH 4/5] Simplify defer cleanup patterns across service files Replace defer function wrappers with direct cleanup calls for better code clarity and consistency. Signed-off-by: Siddharth Asthana --- internal/git/localrepo/bundle.go | 8 ++------ internal/gitaly/service/conflicts/list_conflict_files.go | 4 +--- internal/gitaly/service/conflicts/resolve_conflicts.go | 4 +--- internal/gitaly/service/diff/diff_blobs.go | 4 +--- internal/gitaly/service/operations/cherry_pick.go | 4 +--- internal/gitaly/service/operations/commit_files.go | 4 +--- internal/gitaly/service/operations/ff_branch.go | 4 +--- internal/gitaly/service/operations/merge_branch.go | 4 +--- internal/gitaly/service/operations/rebase_confirmable.go | 4 +--- internal/gitaly/service/operations/rebase_to_ref.go | 4 +--- internal/gitaly/service/operations/revert.go | 4 +--- internal/gitaly/service/operations/squash.go | 4 +--- internal/gitaly/service/operations/submodules.go | 4 +--- internal/gitaly/service/operations/tags.go | 4 +--- internal/gitaly/service/operations/user_create_branch.go | 4 +--- internal/gitaly/service/operations/user_update_branch.go | 4 +--- 16 files changed, 17 insertions(+), 51 deletions(-) diff --git a/internal/git/localrepo/bundle.go b/internal/git/localrepo/bundle.go index 03e477abd66..12227c580a8 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -81,9 +81,7 @@ func (repo *Repo) CloneBundle(ctx context.Context, reader io.Reader) error { if err != nil { return err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() repoPath, err := repo.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) if err != nil { @@ -163,9 +161,7 @@ func (repo *Repo) FetchBundle(ctx context.Context, txManager transaction.Manager if err != nil { return fmt.Errorf("fetch bundle: %w", err) } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() fetchConfig := []gitcmd.ConfigPair{ {Key: "remote.inmemory.url", Value: bundlePath}, diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go index 8c551754f96..541e4d7f430 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -27,9 +27,7 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s if err != nil { return err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 3bf4d6e3b47..2224b03886b 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -147,9 +147,7 @@ func (s *server) resolveConflicts(header *gitalypb.ResolveConflictsRequestHeader if err != nil { return err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() if err := s.repoWithBranchCommit(ctx, quarantineRepo, diff --git a/internal/gitaly/service/diff/diff_blobs.go b/internal/gitaly/service/diff/diff_blobs.go index 9239bec53ff..7b1b574b8c0 100644 --- a/internal/gitaly/service/diff/diff_blobs.go +++ b/internal/gitaly/service/diff/diff_blobs.go @@ -152,9 +152,7 @@ func (s *server) diffBlobs(ctx context.Context, if err != nil { return structerr.NewInternal("creating quarantine directory: %w", err) } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 abe20838614..f33ee1917e2 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -25,9 +25,7 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 c245cf97a3e..f52f376ef5b 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -525,9 +525,7 @@ func (s *Server) userCommitFiles( if err != nil { return err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 bdaceb2848f..ec8b9b9d24a 100644 --- a/internal/gitaly/service/operations/ff_branch.go +++ b/internal/gitaly/service/operations/ff_branch.go @@ -27,9 +27,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 c2f6c080791..c580c227a4a 100644 --- a/internal/gitaly/service/operations/merge_branch.go +++ b/internal/gitaly/service/operations/merge_branch.go @@ -33,9 +33,7 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc if err != nil { return err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 125dfeb1527..08ef1e8bb98 100644 --- a/internal/gitaly/service/operations/rebase_confirmable.go +++ b/internal/gitaly/service/operations/rebase_confirmable.go @@ -35,9 +35,7 @@ func (s *Server) UserRebaseConfirmable(stream gitalypb.OperationService_UserReba if err != nil { return structerr.NewInternal("creating repo quarantine: %w", err) } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 6dc019f57a5..e29b1bc941e 100644 --- a/internal/gitaly/service/operations/rebase_to_ref.go +++ b/internal/gitaly/service/operations/rebase_to_ref.go @@ -22,9 +22,7 @@ func (s *Server) UserRebaseToRef(ctx context.Context, request *gitalypb.UserReba if err != nil { return nil, structerr.NewInternal("creating repo quarantine: %w", err) } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 b6b6fd11ede..2f8e9076f83 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -24,9 +24,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() startRevision, err := s.fetchStartRevision(ctx, quarantineRepo, req) if err != nil { diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go index 62b97ec49ac..5f8c4e4c02a 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -81,9 +81,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest if err != nil { return "", structerr.NewInternal("creating quarantine: %w", err) } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() // 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 d7cc0ca8865..dc82971399b 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -29,9 +29,7 @@ func (s *Server) UserUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 6e000eca0df..730e823b444 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -133,9 +133,7 @@ func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() 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 b6e186e57c1..a8c472099b1 100644 --- a/internal/gitaly/service/operations/user_create_branch.go +++ b/internal/gitaly/service/operations/user_create_branch.go @@ -38,9 +38,7 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() // 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 9c15ed2b36e..57ba105a572 100644 --- a/internal/gitaly/service/operations/user_update_branch.go +++ b/internal/gitaly/service/operations/user_update_branch.go @@ -67,9 +67,7 @@ func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateB if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() if err := s.updateReferenceWithHooks(ctx, req.GetRepository(), req.GetUser(), quarantineDir, referenceName, newOID, oldOID); err != nil { var customHookErr updateref.CustomHookError -- GitLab From 098f0eec047a77d88e81eac0b0cfaac688174b69 Mon Sep 17 00:00:00 2001 From: Siddharth Asthana Date: Mon, 1 Sep 2025 12:46:01 +0530 Subject: [PATCH 5/5] Add context to tempdir cleanup error logging Include correlation ID and RPC details in cleanup error logs. --- internal/gitaly/repoutil/create.go | 6 +----- internal/gitaly/service/repository/fetch.go | 4 +--- internal/tempdir/tempdir.go | 2 +- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/internal/gitaly/repoutil/create.go b/internal/gitaly/repoutil/create.go index 8c8e7185176..4840577911b 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -108,11 +108,7 @@ func Create( 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 eventually get - // cleaned by the tempdir walker of the OS when it's old enough. - cleanup() - }() + defer cleanup() // Note that we do not create the repository directly in its target location, but // instead create it in a temporary directory, first. This is done such that we can diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 8ac08d190c9..347733062c9 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -34,9 +34,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc if err != nil { return nil, err } - defer func() { - cleanup() // Errors are logged by the tempdir package - }() + defer cleanup() sourceRepo, err := remoterepo.New(ctx, req.GetSourceRepository(), s.conns) if err != nil { diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go index a8f4a4fed23..923cdf01212 100644 --- a/internal/tempdir/tempdir.go +++ b/internal/tempdir/tempdir.go @@ -41,7 +41,7 @@ func NewWithPrefix(ctx context.Context, storageName, prefix string, logger log.L cleanup := func() { if err := os.RemoveAll(dir.path); err != nil { - logger.WithError(err).WithField("temporary_directory", dir.path).Error("failed to cleanup temp dir") + logger.WithError(err).WithField("temporary_directory", dir.path).ErrorContext(ctx, "failed to cleanup temp dir") } } -- GitLab