diff --git a/internal/git/catfile/cache_test.go b/internal/git/catfile/cache_test.go index db5ae2c67e4aa3bfa7dc864e7b5b83339588899c..f893fa5c84e7c72f4d680234d0ceeef1d49e830d 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(cleanup) quarantineRepo := quarantineDir.QuarantinedRepo() diff --git a/internal/git/catfile/object_reader_test.go b/internal/git/catfile/object_reader_test.go index f5fee1bbed0dec50540f24cec4c43791596b0a5a..d1a39842e8e2c90ae14663929ea8f2938f266b9d 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(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 d033174e4c531457a4dcb5edc2087cc6096310ba..12227c580a895942d7fc386dd85351adba08b5ad 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -77,10 +77,11 @@ 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 cleanup() repoPath, err := repo.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) if err != nil { @@ -156,10 +157,11 @@ 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 cleanup() fetchConfig := []gitcmd.ConfigPair{ {Key: "remote.inmemory.url", Value: bundlePath}, @@ -192,17 +194,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(), 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 +215,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 6d737639a4d6549e709af2cdce549b38f3f6bf71..23b384595d86c0eb130532a149349a7eb401ffd3 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(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 e04849241a43f5c6afc5460b525288d33e7990f4..ffe035159019585f9a47094dc3615b2462e6a2e9 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) { 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 97fc28f851fa388c015595ed1e16ddbe4e0ca04a..99201c41be09c1ead11d517efc1c844401b12e28 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(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 208af699e9bef02ff10fc501ab4f12ee00cfe01f..e42855e39f9f7a87d5ad250b44542cbb71e0dea3 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(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() + 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(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(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(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) + 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/hook/postreceive_test.go b/internal/gitaly/hook/postreceive_test.go index f6cd89b992b32d0f9f4df5b90ffb0e62e83b9e56..90ca984345ea4fe92c73fb870c3e3ceefaff8d58 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(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 66e407607f4a82376743c7e1ea9d4b38370d7286..8b2884648724ea919be8dcdff9ac79744165338e 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(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 4287d271683e19a16a236ac2939fae2e4dd0a0d7..6cf6e15cdac2b0d518058e8287eaedd7f546f26e 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(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 72b743c672b42ce34208a486c90e32938decd2a1..99fb5f41181c8b3187b122920501ef043b8d88c3 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(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 f849cdb8ed05c60351c18b546e2e03cde4a9eaa6..4840577911b83139cf24703bd0b26c35448b252d 100644 --- a/internal/gitaly/repoutil/create.go +++ b/internal/gitaly/repoutil/create.go @@ -104,16 +104,11 @@ 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()) - }() + 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/blob/blobs_test.go b/internal/gitaly/service/blob/blobs_test.go index 690539c0b5bb31fb99c5b07b2997cc8d797cd7f6..a6bc287c1d9bdfca2c0006a521d40bb8f8ee4822 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(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 b4dffb415d53b806ac0c34bc654b9de790043a92..c21c9ec1f0de0b6eef6610fa512b616104111437 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(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(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 16bf818cfef52b33f10eba18fcffabceb381ca39..de7fdc8f15721c6290078c59fbb138bcc90bb1eb 100644 --- a/internal/gitaly/service/cleanup/rewrite_history.go +++ b/internal/gitaly/service/cleanup/rewrite_history.go @@ -110,10 +110,11 @@ 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 cleanup() // Check state of source repository prior to running filter-repo. initialChecksum, err := checksumRepo(ctx, repo) @@ -185,11 +186,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) { + 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 +203,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 +217,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 +231,11 @@ 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 cleanup() 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 0bb926fe52dc3364c4318afc72f8efbe42db969c..541e4d7f43099a82d0e60d5c53ab5ac9d87d668e 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -23,10 +23,11 @@ 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 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 c902e22813604c270f1329c5ba1a6be09f5e4d49..2224b03886b8cec40a3d52f3b2c989d9ecaf65ad 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -143,10 +143,11 @@ 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 cleanup() if err := s.repoWithBranchCommit(ctx, quarantineRepo, diff --git a/internal/gitaly/service/conflicts/server.go b/internal/gitaly/service/conflicts/server.go index a1178a071a175c2516798320cefb8ecc5157af52..d2c43fd8f24268df41eaf17d484f5701e0585ffb 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) { + 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 9b4e31cc97cabd35171e1f9dd583c646717d3d40..7b1b574b8c0b304cb4c12fea2fe476dca32fff49 100644 --- a/internal/gitaly/service/diff/diff_blobs.go +++ b/internal/gitaly/service/diff/diff_blobs.go @@ -148,10 +148,11 @@ 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 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 dbd12c8116b1b4b05fb7e525a82686e40f8f722e..f33ee1917e2b8e72c9d046f19187f4077a0ab95d 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -21,10 +21,11 @@ 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 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 5913288f05d20bab5012df8a3f03061424c95537..f52f376ef5b09a708b8053e3a9a68959d25f739c 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -521,10 +521,11 @@ 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 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 e78c8c5e00a27ebf296d445d95a1b54741b5b372..ec8b9b9d24a0167dd55bced051b0f343f831cf1f 100644 --- a/internal/gitaly/service/operations/ff_branch.go +++ b/internal/gitaly/service/operations/ff_branch.go @@ -23,10 +23,11 @@ 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 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 d8066d1257d753923f147a463bcf5af7fb547ef4..c580c227a4acc904c1b04494ee4e74e68afbd8f3 100644 --- a/internal/gitaly/service/operations/merge_branch.go +++ b/internal/gitaly/service/operations/merge_branch.go @@ -29,10 +29,11 @@ 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 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 0f4a1d0301051457ccb39b3630e4cc31a0a58cb9..08ef1e8bb98cdc2704101c24a7ba34293242ccb9 100644 --- a/internal/gitaly/service/operations/rebase_confirmable.go +++ b/internal/gitaly/service/operations/rebase_confirmable.go @@ -31,10 +31,11 @@ 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 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 ddb8fde1761b64369c6a73ac6350315b3f88390e..e29b1bc941e8cbb7108258cc1da8b04ff51fc811 100644 --- a/internal/gitaly/service/operations/rebase_to_ref.go +++ b/internal/gitaly/service/operations/rebase_to_ref.go @@ -18,10 +18,11 @@ 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 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 760e625eb704d80417ae622c8b898fc4afbece6d..2f8e9076f834418447b85f5fa36438c61fd2bf6d 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -20,10 +20,11 @@ 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 cleanup() 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 2842fb3aa8f9ea502a58412df58752823f99368c..90866018f4d380ddbc1adee4dbbbc5461a2415e5 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) { + 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 8310938d694c1a2d61ef9ec87477f2ee791dfa79..5f8c4e4c02a75fd9c7c12db05dfe85e2384474b4 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -77,10 +77,11 @@ 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 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 cdf9cfce2646f39ec3ed1178a9b4cd94a05c2141..dc82971399b8a30254fe91617aaa1cdd895e8d01 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -25,10 +25,11 @@ 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 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 29f3c3902fea46de722e403ebbbeabbe39ae7c91..730e823b4449c6a6d25975c2bad992ee75ae9e66 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -129,10 +129,11 @@ 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 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 5210f445498b01cf7437731bf1f4eb03c9ee10f2..a8c472099b1d13939d72e808713b66107febe882 100644 --- a/internal/gitaly/service/operations/user_create_branch.go +++ b/internal/gitaly/service/operations/user_create_branch.go @@ -34,10 +34,11 @@ 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 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 08bc7db1a041b3af9f6d34493144cd08d541305c..57ba105a572629d0f14039c6a9cd965ba7a5337b 100644 --- a/internal/gitaly/service/operations/user_update_branch.go +++ b/internal/gitaly/service/operations/user_update_branch.go @@ -63,10 +63,11 @@ 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 cleanup() 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 56a5d663dfd50178438ef24db92890a7460afd41..347733062c9414012081f5e8ee5abee2a4115610 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -30,10 +30,11 @@ 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 cleanup() 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 bf50ffd893d45794e4f15e0a284fc027c6604c4e..e0799a266521ddd319c3bf3920061363edd3efb7 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 7dfa10dd2d9fbb2bc545789b38fb54cb1e41d793..6ea59403520df152808968651e7e0044244919d9 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) { + 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 57e5e93a7aa11f0018d9f4aa5756e33c74e44012..c22729d227b62ff4e13518e3f96d3ca7015c4400 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(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) + t.Cleanup(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) + 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 diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go index f70c1216ce0aa83c362b12318ebb1aa166530d8c..923cdf0121210f56c400c2fcbe528a082fbbf65d 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,59 @@ 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) { 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) { dir, err := newDirectory(ctx, storageName, prefix, logger, locator) if err != nil { - return Dir{}, err + return Dir{}, nil, err } - go dir.cleanupOnDone(ctx) + cleanup := func() { + if err := os.RemoveAll(dir.path); err != nil { + logger.WithError(err).WithField("temporary_directory", dir.path).ErrorContext(ctx, "failed to cleanup temp dir") + } + } - 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) { 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 +97,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 bbfc1916826828cd5bdedc32f629ee63cb521562..7fa191e7da12c9ef57c2e34f4c09d7756355c5f2 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 + 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(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) }