testhelper: Add simple race-free way to retrieve umask

Several of our tests implicitly depend on the host umask. But because
the umask is process-wide, figuring out the umask at test execution time
is non-trivial when tests run in parallel. We currently work around this
issue by simply not running any such tests in parallel, but that is not
exactly great.

Let's fix this issue by introducing a new `testhelper.Umask()` helper.
At startup, we pre-determine the umask and cache it so that it becomes
safe to ask for the umask even in parallel tests. As our tests are
expected to not modify global state this should be a safe thing to do.

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 b38a492af..25022fb9b 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