test(remote-worker): fix a data race in the egress tests, and honour TMPDIR for socket dirs

remote-worker/cmd/moca-egress/main_test.go

@@ -17,6 +17,7 @@ import (
 	"time"
 
 	"github.com/rossoctl/moca/remote-worker/internal/egress/control"
+	"github.com/rossoctl/moca/remote-worker/internal/shortdir"
 )
 
 func TestBinaryServesAndShutsDown(t *testing.T) {
@@ -46,11 +47,7 @@ func TestBinaryServesAndShutsDown(t *testing.T) {
 		c.Write(buf)
 	}()
 
-	dir, err := os.MkdirTemp("/tmp", "meg")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(dir)
+	dir := shortdir.New(t, "meg", len("/c.sock"))
 	sock := filepath.Join(dir, "c.sock")
 
 	cmd := exec.Command(bin)

remote-worker/cmd/moca-egress/sdnotify_test.go

@@ -4,10 +4,11 @@ package main
 
 import (
 	"net"
-	"os"
 	"path/filepath"
 	"testing"
 	"time"
+
+	"github.com/rossoctl/moca/remote-worker/internal/shortdir"
 )
 
 func TestSdNotifyIsANoOpWithoutNotifySocket(t *testing.T) {
@@ -18,11 +19,7 @@ func TestSdNotifyIsANoOpWithoutNotifySocket(t *testing.T) {
 }
 
 func TestSdNotifySendsTheStateDatagram(t *testing.T) {
-	dir, err := os.MkdirTemp("/tmp", "sdn")
-	if err != nil {
-		t.Fatal(err)
-	}
-	t.Cleanup(func() { _ = os.RemoveAll(dir) })
+	dir := shortdir.New(t, "sdn", len("/notify"))
 	path := filepath.Join(dir, "notify")
 	ln, err := net.ListenUnixgram("unixgram", &net.UnixAddr{Name: path, Net: "unixgram"})
 	if err != nil {

remote-worker/egress/conformance/conformance.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/rossoctl/moca/remote-worker/internal/egress/control"
+	"github.com/rossoctl/moca/remote-worker/internal/shortdir"
 )
 
 // The environment variables of the contract.
@@ -192,7 +193,10 @@ type harness struct {
 // resolver and control-socket variables. The caller adds the rest of env
 // and calls start.
 func newHarness(ctx context.Context) (*harness, error) {
-	dir, err := os.MkdirTemp("/tmp", "mconf") // short: darwin's sun_path is 104 bytes
+	// Short AND writable: darwin's sun_path is 104 bytes, and /tmp is not writable everywhere
+	// (a sandbox, a hardened image), so this honours TMPDIR when a directory under it still has
+	// room for the "/c.sock" below, and falls back when it does not. See internal/shortdir.
+	dir, err := shortdir.Make("mconf", len("/c.sock"))
 	if err != nil {
 		return nil, err
 	}

remote-worker/internal/egress/control/control_test.go

@@ -19,6 +19,7 @@ import (
 	"golang.org/x/sys/unix"
 
 	egressv1 "github.com/rossoctl/moca/gen/go/egress/v1"
+	"github.com/rossoctl/moca/remote-worker/internal/shortdir"
 	"google.golang.org/protobuf/proto"
 )
 
@@ -64,11 +65,7 @@ func (r *recorder) Unregister(vmID string) error {
 
 func shortDir(t *testing.T) string {
 	t.Helper()
-	d, err := os.MkdirTemp("/tmp", "ectl")
-	if err != nil {
-		t.Fatal(err)
-	}
-	t.Cleanup(func() { _ = os.RemoveAll(d) })
+	d := shortdir.New(t, "ectl", len("/vm1.sock"))
 	return d
 }
 

remote-worker/internal/egress/proxy_test.go

@@ -24,6 +24,7 @@ import (
 	"time"
 
 	"github.com/rossoctl/moca/remote-worker/internal/egress/control"
+	"github.com/rossoctl/moca/remote-worker/internal/shortdir"
 )
 
 // recSink records events and their JSON form; safe for concurrent use.
@@ -218,11 +219,7 @@ func allow(t *testing.T, s string) []AllowEntry {
 
 func newHarness(t *testing.T, allowList string, mod func(*Config)) *harness {
 	t.Helper()
-	dir, err := os.MkdirTemp("/tmp", "eg")
-	if err != nil {
-		t.Fatal(err)
-	}
-	t.Cleanup(func() { os.RemoveAll(dir) })
+	dir := shortdir.New(t, "eg", len("/vm-1.sock"))
 	h := &harness{t: t, sink: &recSink{}, res: newResolver(), dir: dir}
 	cfg := Config{
 		Allow:                 allow(t, allowList),

remote-worker/internal/guestagent/egress_test.go

@@ -4,12 +4,13 @@ import (
 	"errors"
 	"io"
 	"net"
-	"os"
 	"path/filepath"
 	"strings"
 	"sync"
 	"testing"
 	"time"
+
+	"github.com/rossoctl/moca/remote-worker/internal/shortdir"
 )
 
 // startForwarder serves a forwarder on tcp 127.0.0.1:0 using the given Dial and
@@ -20,20 +21,38 @@ func startForwarder(t *testing.T, dial func() (net.Conn, error)) (addr string, d
 	if err != nil {
 		t.Fatal(err)
 	}
-	f := &EgressForwarder{Dial: dial, Logf: t.Logf}
+	// `t.Logf` must not be called once the test has returned: the race detector flags it as a
+	// write/read on testing.common, and the testing package itself panics on it. Closing the
+	// listener stops Serve, but an in-flight `handle` goroutine can still be mid-request and log
+	// after that. So the logger is gated -- Cleanup flips it off under the mutex before the test
+	// ends, and a later line is dropped rather than written to a finished *testing.T.
+	var mu sync.Mutex
+	live := true
+	t.Cleanup(func() {
+		mu.Lock()
+		live = false
+		mu.Unlock()
+	})
+	logf := func(format string, a ...any) {
+		mu.Lock()
+		defer mu.Unlock()
+		if !live {
+			return
+		}
+		t.Logf(format, a...)
+	}
+	f := &EgressForwarder{Dial: dial, Logf: logf}
 	done = make(chan error, 1)
 	go func() { done <- f.Serve(ln) }()
+	// Registered after the gate above, so it runs BEFORE it (Cleanup is LIFO): the listener closes
+	// and Serve returns first, and only then does logging stop.
 	t.Cleanup(func() { ln.Close() })
 	return ln.Addr().String(), done
 }
 
 func unixUpstream(t *testing.T) (net.Listener, func() (net.Conn, error)) {
 	t.Helper()
-	dir, err := os.MkdirTemp("/tmp", "gfwd")
-	if err != nil {
-		t.Fatal(err)
-	}
-	t.Cleanup(func() { os.RemoveAll(dir) })
+	dir := shortdir.New(t, "gfwd", len("/u.sock"))
 	path := filepath.Join(dir, "u.sock")
 	ul, err := net.Listen("unix", path)
 	if err != nil {

remote-worker/internal/guestagent/listen_test.go

@@ -3,10 +3,11 @@ package guestagent
 import (
 	"io"
 	"net"
-	"os"
 	"path/filepath"
 	"testing"
 	"time"
+
+	"github.com/rossoctl/moca/remote-worker/internal/shortdir"
 )
 
 // These cases exercise the address-parsing and unix: paths that are shared, byte for
@@ -31,14 +32,11 @@ func TestListenUnsupportedSchemeIsError(t *testing.T) {
 // without error.
 func TestListenUnixRoundTrip(t *testing.T) {
 	// t.TempDir()'s path is well past the ~104-byte sun_path limit AF_UNIX enforces
-	// on darwin (and on some Linux libc/kernel combos), so this uses a short-named
-	// dir directly under /tmp instead — long enough to be unique, short enough that
-	// the bind itself is what the test exercises, not the platform's path limit.
-	dir, err := os.MkdirTemp("/tmp", "sh-ga-")
-	if err != nil {
-		t.Fatalf("MkdirTemp: %v", err)
-	}
-	t.Cleanup(func() { _ = os.RemoveAll(dir) })
+	// on darwin (and on some Linux libc/kernel combos), so this asks shortdir for a
+	// dir with room for "/s.sock" under it — TMPDIR when that fits, /tmp otherwise.
+	// Long enough to be unique, short enough that the bind itself is what the test
+	// exercises, not the platform's path limit.
+	dir := shortdir.New(t, "sh-ga-", len("/s.sock"))
 	sockPath := filepath.Join(dir, "s.sock")
 	ln, err := Listen("unix:" + sockPath)
 	if err != nil {

remote-worker/internal/shortdir/shortdir.go

@@ -0,0 +1,110 @@
+// Package shortdir makes the temporary directories that hold Unix sockets in tests.
+//
+// It exists because of a conflict between two constraints. A Unix socket path is bounded by
+// sockaddr_un.sun_path -- 104 bytes on macOS, 108 on Linux -- and `bind` fails with EINVAL (or
+// silently truncates) past it, so these directories have to be SHORT. But hardcoding "/tmp", as
+// these tests used to, fails outright wherever /tmp is not writable: a sandboxed or hardened
+// environment, a CI image that points TMPDIR elsewhere, or a user without write access to /tmp.
+// `go test` itself honours TMPDIR, so hardcoding /tmp also meant the Go tests ignored the one knob
+// everything else in the repo respects.
+//
+// So: prefer TMPDIR when a directory under it still leaves room for the caller's own socket path,
+// and fall back to /tmp or the operating system's default otherwise. Either way the caller gets a
+// directory it can put a socket in, or an error that says why not.
+//
+// The room a caller needs is the caller's to state, because it varies by two orders of magnitude
+// across this suite: "/c.sock" is 7 bytes, while the Firecracker collision test binds at
+// "/firecracker/vm-6/root/run/firecracker.socket", 45. A single built-in allowance cannot be right
+// for both -- too small and a long TMPDIR is accepted that cannot hold the socket, too large and a
+// perfectly usable TMPDIR is rejected -- so New and Make take `need` and measure the directory they
+// actually created against it.
+package shortdir
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// MaxSocketPath is the smaller of the two platform limits for sockaddr_un.sun_path, so a path that
+// fits here fits everywhere this code runs. The real limit is 104 on macOS and 108 on Linux.
+const MaxSocketPath = 104
+
+// New returns a directory for a test's Unix sockets, removed when the test ends.
+//
+// need is how many bytes the caller will append to the returned directory for its longest socket
+// path, separator included -- normally written as a len() of the literal suffix, so it cannot drift
+// from the path the test actually binds:
+//
+//	dir := shortdir.New(t, "fcguard", len("/firecracker/vm-6/root/run/firecracker.socket"))
+//
+// It tries each candidate in turn and takes the first where the directory it creates is short
+// enough AND writable, so a read-only /tmp or an over-long TMPDIR costs a fallback rather than a
+// failure. If none works the test fails naming every candidate and why, which is the information
+// needed to fix the environment -- far better than `bind` failing later with a bare EINVAL.
+func New(t *testing.T, prefix string, need int) string {
+	t.Helper()
+	dir, err := Make(prefix, need)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.RemoveAll(dir) })
+	return dir
+}
+
+// Make is New without the testing dependency, for the conformance harness and anything else that
+// is not a test. The caller owns the cleanup.
+func Make(prefix string, need int) (string, error) {
+	var tried []string
+	for _, base := range candidates() {
+		// The length that matters is the one of the directory that actually exists: MkdirTemp's
+		// random suffix is up to 10 digits and the prefix is the caller's, so neither the base
+		// alone nor any fixed allowance for them predicts it. Create first, measure the real
+		// thing, and give the directory back if it does not fit.
+		dir, err := os.MkdirTemp(base, prefix)
+		if err != nil {
+			tried = append(tried, fmt.Sprintf("%s (%v)", base, err))
+			continue
+		}
+		if len(dir)+need > MaxSocketPath {
+			_ = os.RemoveAll(dir)
+			tried = append(tried, fmt.Sprintf(
+				"%s (%s is %d bytes, %d with the caller's %d-byte socket name, over the %d limit)",
+				base, dir, len(dir), len(dir)+need, need, MaxSocketPath,
+			))
+			continue
+		}
+		return dir, nil
+	}
+	return "", fmt.Errorf(
+		"shortdir: no directory both writable and short enough to hold a %d-byte socket name under "+
+			"it (sun_path limit %d); tried: %s",
+		need, MaxSocketPath, strings.Join(tried, "; "),
+	)
+}
+
+// candidates lists the parent directories to try, in order of preference: TMPDIR first, because it
+// is the knob the rest of the repo honours and the one a sandbox sets; then /tmp, which is short
+// and usually writable; then the Go default, which is TMPDIR-or-/tmp but may differ on platforms
+// where neither applies. Duplicates and empties are dropped so the error message lists each place
+// once.
+func candidates() []string {
+	var out []string
+	seen := map[string]bool{}
+	for _, d := range []string{os.Getenv("TMPDIR"), "/tmp", os.TempDir()} {
+		if d == "" {
+			continue
+		}
+		// MkdirTemp joins without adding a separator, so a trailing slash would double it in the
+		// path and in the length check.
+		d = filepath.Clean(d)
+		if seen[d] {
+			continue
+		}
+		seen[d] = true
+		out = append(out, d)
+	}
+	return out
+}