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 db5ae2c67e..9b6485e8e6 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 f5fee1bbed..5799b90361 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 d033174e4c..f7024c70f9 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 6d737639a4..af708ca514 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 e04849241a..1bc5dbe3c9 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 97fc28f851..d85f4960d9 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 208af699e9..8313643d37 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 f6cd89b992..e0251ba972 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 66e407607f..4fc560e4e5 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 4287d27168..5a5be89008 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 72b743c672..657cfd3dc8 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 f849cdb8ed..747f280717 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 690539c0b5..1de2d551de 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 b4dffb415d..54925ac464 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 16bf818cfe..e201e577e6 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 0bb926fe52..95824a5535 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 c902e22813..1806255384 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 a1178a071a..74c1602ac2 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 9b4e31cc97..4120974dd0 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 dbd12c8116..36c39f9881 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 5913288f05..027f317674 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 e78c8c5e00..8f0d822a22 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 d8066d1257..ca185f4079 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 0f4a1d0301..ff87bace8f 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 ddb8fde176..79da24ff47 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 760e625eb7..09db166c62 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 2842fb3aa8..e513d4b7e1 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 8310938d69..a3978e7aa8 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 cdf9cfce26..1f853e703f 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 29f3c3902f..aa5f80a054 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 5210f44549..bc72f0eec1 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 08bc7db1a0..081f3a71be 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 56a5d663df..7246f4e499 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 bf50ffd893..6b00e77cb2 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 7dfa10dd2d..1fc4f90829 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 57e5e93a7a..146c2b5e48 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 f70c1216ce..66ce64f010 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 bbfc191682..51b6e15bbc 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 9b6485e8e6..f893fa5c84 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 5799b90361..d1a39842e8 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 f7024c70f9..03e477abd6 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 af708ca514..23b384595d 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 1bc5dbe3c9..ffe0351590 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 d85f4960d9..99201c41be 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 8313643d37..af044c268e 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 e0251ba972..90ca984345 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 4fc560e4e5..8b28846487 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 5a5be89008..6cf6e15cda 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 657cfd3dc8..99fb5f4118 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 747f280717..8c8e718517 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 1de2d551de..a6bc287c1d 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 54925ac464..c21c9ec1f0 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 e201e577e6..e656cd6504 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 95824a5535..8c551754f9 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 1806255384..3bf4d6e3b4 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 74c1602ac2..d2c43fd8f2 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 4120974dd0..9239bec53f 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 36c39f9881..abe2083861 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 027f317674..c245cf97a3 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 8f0d822a22..bdaceb2848 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 ca185f4079..c2f6c08079 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 ff87bace8f..125dfeb152 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 79da24ff47..6dc019f57a 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 09db166c62..b6b6fd11ed 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 e513d4b7e1..90866018f4 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 a3978e7aa8..62b97ec49a 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 1f853e703f..d7cc0ca886 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 aa5f80a054..6e000eca0d 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 bc72f0eec1..b6e186e57c 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 081f3a71be..9c15ed2b36 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 7246f4e499..8ac08d190c 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 6b00e77cb2..e0799a2665 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 1fc4f90829..6ea5940352 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 146c2b5e48..899fc38f2e 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 66ce64f010..a8f4a4fed2 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 51b6e15bbc..7fa191e7da 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 af044c268e..e42855e39f 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 e656cd6504..de7fdc8f15 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 899fc38f2e..c22729d227 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 03e477abd6..12227c580a 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 8c551754f9..541e4d7f43 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 3bf4d6e3b4..2224b03886 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 9239bec53f..7b1b574b8c 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 abe2083861..f33ee1917e 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 c245cf97a3..f52f376ef5 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 bdaceb2848..ec8b9b9d24 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 c2f6c08079..c580c227a4 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 125dfeb152..08ef1e8bb9 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 6dc019f57a..e29b1bc941 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 b6b6fd11ed..2f8e9076f8 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 62b97ec49a..5f8c4e4c02 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 d7cc0ca886..dc82971399 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 6e000eca0d..730e823b44 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 b6e186e57c..a8c472099b 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 9c15ed2b36..57ba105a57 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 8c8e718517..4840577911 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 8ac08d190c..347733062c 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 a8f4a4fed2..923cdf0121 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