Merge branch 'pks-testhelper-simplify-umask' into 'master'

testhelper: Add simple race-free way to retrieve umask

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6264

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: James Fargher <jfargher@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

8 files changed, 21 insertions, 37 deletions

diff --git a/internal/cli/gitaly/hooks_test.go b/internal/cli/gitaly/hooks_test.go
index e88091ccf..a3287c830 100644
--- a/internal/cli/gitaly/hooks_test.go
+++ b/internal/cli/gitaly/hooks_test.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/setup"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
 	internalclient "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/client"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver"
@@ -25,11 +24,11 @@ import (
 	"google.golang.org/grpc"
 )
 
-// This test cannot be made to run in parallel because it relies on setting the
-// umask which is unfortunately not thread safe.
 func TestSetHooksSubcommand(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
-	umask := perm.GetUmask()
+	umask := testhelper.Umask()
 
 	cfg := testcfg.Build(t, testcfg.WithStorages("default", "another-storage"))
 	testcfg.BuildGitaly(t, cfg)
diff --git a/internal/gitaly/repoutil/custom_hooks_test.go b/internal/gitaly/repoutil/custom_hooks_test.go
index 8b3db9962..cc97954ee 100644
--- a/internal/gitaly/repoutil/custom_hooks_test.go
+++ b/internal/gitaly/repoutil/custom_hooks_test.go
@@ -115,7 +115,9 @@ func TestGetCustomHooks_nonexistentHooks(t *testing.T) {
 }
 
 func TestExtractHooks(t *testing.T) {
-	umask := perm.GetUmask()
+	t.Parallel()
+
+	umask := testhelper.Umask()
 
 	writeFile := func(writer *tar.Writer, path string, mode fs.FileMode, content string) {
 		require.NoError(t, writer.WriteHeader(&tar.Header{
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index b5b542da5..6bca019f6 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -751,7 +751,7 @@ func testReplicateRepositoryTransactional(t *testing.T, ctx context.Context) {
 	// we use a temporary file here to figure out the expected permissions as they would in fact be subject
 	// to change depending on the current umask.
 	noHooksVoteData := [5]byte{'.', 0, 0, 0, 0}
-	binary.BigEndian.PutUint32(noHooksVoteData[1:], uint32(fs.ModeDir|testhelper.MaskedPerms(t, perm.PublicDir)))
+	binary.BigEndian.PutUint32(noHooksVoteData[1:], uint32(testhelper.Umask().Mask(perm.PublicDir|fs.ModeDir)))
 	noHooksVote := voting.VoteFromData(noHooksVoteData[:])
 
 	expectedVotes := []voting.Vote{
diff --git a/internal/gitaly/storage/storagemgr/partition_manager_test.go b/internal/gitaly/storage/storagemgr/partition_manager_test.go
index 17213eb46..ddb960fa5 100644
--- a/internal/gitaly/storage/storagemgr/partition_manager_test.go
+++ b/internal/gitaly/storage/storagemgr/partition_manager_test.go
@@ -24,11 +24,10 @@ import (
 )
 
 func TestPartitionManager(t *testing.T) {
-	umask := perm.GetUmask()
-
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
+	umask := testhelper.Umask()
 
 	// steps defines execution steps in a test. Each test case can define multiple steps to exercise
 	// more complex behavior.
diff --git a/internal/gitaly/storage/storagemgr/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
index 37e0910c4..ca4d5cf7a 100644
--- a/internal/gitaly/storage/storagemgr/transaction_manager_test.go
+++ b/internal/gitaly/storage/storagemgr/transaction_manager_test.go
@@ -150,10 +150,9 @@ func reverseIndexFileDirectoryEntry(cfg config.Cfg) testhelper.DirectoryEntry {
 }
 
 func TestTransactionManager(t *testing.T) {
-	umask := perm.GetUmask()
-
 	t.Parallel()
 
+	umask := testhelper.Umask()
 	ctx := testhelper.Context(t)
 
 	type testCommit struct {
diff --git a/internal/helper/perm/perm.go b/internal/helper/perm/perm.go
index 418c9b971..74deab662 100644
--- a/internal/helper/perm/perm.go
+++ b/internal/helper/perm/perm.go
@@ -6,7 +6,6 @@ package perm
 
 import (
 	"io/fs"
-	"syscall"
 )
 
 const (
@@ -63,11 +62,3 @@ type Umask int
 func (mask Umask) Mask(mode fs.FileMode) fs.FileMode {
 	return mode & ^fs.FileMode(mask)
 }
-
-// GetUmask gets the currently set umask. Not safe to call concurrently with other
-// file operations as it has to set the Umask to get the old value.
-func GetUmask() Umask {
-	umask := syscall.Umask(0)
-	syscall.Umask(umask)
-	return Umask(fs.FileMode(umask))
-}
diff --git a/internal/testhelper/directory_test.go b/internal/testhelper/directory_test.go
index 6b4eb2889..a5c95af3a 100644
--- a/internal/testhelper/directory_test.go
+++ b/internal/testhelper/directory_test.go
@@ -39,13 +39,11 @@ func (r *tbRecorder) FailNow() {
 }
 
 func TestRequireDirectoryState(t *testing.T) {
-	umask := perm.GetUmask()
-
 	t.Parallel()
 
 	rootDir := t.TempDir()
-
 	relativePath := "assertion-root"
+	umask := Umask()
 
 	require.NoError(t,
 		os.MkdirAll(
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 897afe429..64e2269df 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"math/rand"
 	"net"
 	"net/http"
@@ -326,20 +325,17 @@ func WriteExecutable(tb testing.TB, path string, content []byte) string {
 	return path
 }
 
-// MaskedPerms determines permission bits with the current process umask applied. This is preferable over calls to
-// `perm.GetUmask()` because it can be executed in parallel. Note that this function does not include non-permission
-// mode bits like `fs.ModeDir` or `fs.ModeSocket`. You can OR these manually as required.
-func MaskedPerms(tb testing.TB, mode fs.FileMode) fs.FileMode {
-	tb.Helper()
-
-	f, err := os.OpenFile(filepath.Join(TempDir(tb), "file"), os.O_CREATE|os.O_EXCL, mode)
-	require.NoError(tb, err)
-	defer MustClose(tb, f)
-
-	stat, err := f.Stat()
-	require.NoError(tb, err)
-
-	return stat.Mode().Perm()
+var umask perm.Umask = func() perm.Umask {
+	oldMask := syscall.Umask(0)
+	syscall.Umask(oldMask)
+	return perm.Umask(oldMask)
+}()
+
+// Umask return the umask of the current procses. Note that this value is computed once at initialization time because
+// it is shared global state that is unsafe to access when there are multiple threads running at the same time. It
+// follows that tests should never update the umask.
+func Umask() perm.Umask {
+	return umask
 }
 
 // Unsetenv unsets an environment variable. The variable will be restored after the test has