Merge branch 'jv-cleanup-tempdir' into 'master'

Hooks: eagerly delete sidechannel socket dir

See merge request gitlab-org/gitaly!4369

2 files changed, 33 insertions, 1 deletions

diff --git a/internal/gitaly/hook/sidechannel.go b/internal/gitaly/hook/sidechannel.go
index 3311a56c5..ca83a11f0 100644
--- a/internal/gitaly/hook/sidechannel.go
+++ b/internal/gitaly/hook/sidechannel.go
@@ -39,11 +39,16 @@ func GetSidechannel(ctx context.Context) (net.Conn, error) {
 // receives a connection. The address of the listener is stored in the
 // returned context, so that the caller can propagate it to a server. The
 // caller must Close the SidechannelWaiter to prevent resource leaks.
-func SetupSidechannel(ctx context.Context, callback func(*net.UnixConn) error) (context.Context, *SidechannelWaiter, error) {
+func SetupSidechannel(ctx context.Context, callback func(*net.UnixConn) error) (_ context.Context, _ *SidechannelWaiter, err error) {
 	socketDir, err := os.MkdirTemp("", "gitaly")
 	if err != nil {
 		return nil, nil, err
 	}
+	defer func() {
+		if err != nil {
+			_ = os.RemoveAll(socketDir)
+		}
+	}()
 
 	address := path.Join(socketDir, sidechannelSocket)
 	l, err := net.ListenUnix("unix", &net.UnixAddr{Net: "unix", Name: address})
@@ -80,6 +85,12 @@ func (wt *SidechannelWaiter) run(callback func(*net.UnixConn) error) {
 		}
 		defer c.Close()
 
+		// Eagerly remove the socket directory, in case the process exits before
+		// wt.Close() can run.
+		if err := os.RemoveAll(wt.socketDir); err != nil {
+			return err
+		}
+
 		return callback(c)
 	}()
 }
@@ -90,8 +101,12 @@ func (wt *SidechannelWaiter) Close() error {
 	// Run all cleanup actions _before_ checking errors, so that we cannot
 	// forget one.
 	cleanupErrors := []error{
+		// If wt.run() is blocked on AcceptUnix(), this will unblock it.
 		wt.listener.Close(),
+		// Remove the socket directory to prevent garbage in case wt.run() did
+		// not run.
 		os.RemoveAll(wt.socketDir),
+		// Block until wt.run() is done.
 		wt.Wait(),
 	}
 
diff --git a/internal/gitaly/hook/sidechannel_test.go b/internal/gitaly/hook/sidechannel_test.go
index 7cd36a588..cffe5305a 100644
--- a/internal/gitaly/hook/sidechannel_test.go
+++ b/internal/gitaly/hook/sidechannel_test.go
@@ -25,6 +25,8 @@ func TestSidechannel(t *testing.T) {
 	require.NoError(t, err)
 	defer wt.Close()
 
+	require.DirExists(t, wt.socketDir)
+
 	// Server side
 	ctxIn := metadata.OutgoingToIncoming(ctxOut)
 	c, err := GetSidechannel(ctxIn)
@@ -35,10 +37,25 @@ func TestSidechannel(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "ping", string(buf))
 
+	require.NoDirExists(t, wt.socketDir)
+
 	// Client side
 	require.NoError(t, wt.Wait())
 }
 
+func TestSidechannel_cleanup(t *testing.T) {
+	_, wt, err := SetupSidechannel(
+		testhelper.Context(t),
+		func(c *net.UnixConn) error { return nil },
+	)
+	require.NoError(t, err)
+	defer wt.Close()
+
+	require.DirExists(t, wt.socketDir)
+	_ = wt.Close()
+	require.NoDirExists(t, wt.socketDir)
+}
+
 func TestGetSidechannel(t *testing.T) {
 	ctx := testhelper.Context(t)