fix(cli): fail fast when the broker credential fd is not inherited

In broker mode fduty reaches Flashduty through a control socket the
runner passes as an inherited fd (FLASHDUTY_CRED_FD). Programs that spawn
fduty without passing inherited fds - Python's subprocess (close_fds=True
by default), Node's child_process, sudo - leave that fd closed or reused
by an unrelated file. The failure then only surfaced at the first request
as "broker handshake send: bad file descriptor" (or "socket operation on
non-socket"), after the SDK's request URL prefix, with no hint of the fix.

newBrokerHTTPClient now checks that the fd is an open socket before
building the client and returns an error that names the fd and how to
keep it open (run from the shell, Python pass_fds, Node stdio entry). It
returns (*http.Client, error), so the non-unix stub returns
errBrokerUnsupported directly and root.go drops its nil check.

Author: ysyneu

internal/cli/broker_dial_other.go

@@ -7,7 +7,7 @@ import (
 	"net/http"
 )
 
-func newBrokerHTTPClient(int) *http.Client { return nil }
+func newBrokerHTTPClient(int) (*http.Client, error) { return nil, errBrokerUnsupported }
 
 var errBrokerUnsupported = errors.New("flashduty: broker mode is not supported on this platform")
 

internal/cli/broker_dial_unix.go

@@ -14,11 +14,6 @@ import (
 	"time"
 )
 
-// errBrokerUnsupported is returned when broker mode is requested on a build that
-// cannot provide it. On unix this is effectively unreachable (newBrokerHTTPClient
-// never returns nil), but defaultNewClient references it on every platform.
-var errBrokerUnsupported = errors.New("flashduty: broker mode is not supported on this platform")
-
 // errBrokerClosed is returned (wrapped) when the runner-side broker control
 // channel is gone: the runner exited, or reclaimed the channel once the
 // command that started this process finished, so fduty calls from a
@@ -110,7 +105,21 @@ func (d *brokerDialer) dial(_ context.Context, _, _ string) (net.Conn, error) {
 // every connection over the inherited control fd. Timeout matches the SDK's
 // historical default (30s) so behavior is unchanged for non-streaming calls;
 // streaming export relies on request context like before.
-func newBrokerHTTPClient(credFD int) *http.Client {
+//
+// It first checks that credFD is an open socket in this process. The runner
+// hands the control end to bash, and only processes that inherit fd credFD
+// reach fduty with it intact: Python's subprocess (close_fds=True by default),
+// Node's child_process and sudo all close it. Without the check that surfaces
+// as a handshake EBADF/ENOTSOCK at the first request, after the SDK's URL
+// prefix, with nothing saying how to fix it.
+func newBrokerHTTPClient(credFD int) (*http.Client, error) {
+	if _, err := syscall.GetsockoptInt(credFD, syscall.SOL_SOCKET, syscall.SO_TYPE); err != nil {
+		return nil, fmt.Errorf("FLASHDUTY_CRED_FD=%d is not an open socket in this process (%v): "+
+			"the program that started fduty did not pass the credential channel down. "+
+			"Run fduty from the shell, or keep fd %d open when spawning it: "+
+			"Python subprocess.run(cmd, pass_fds=(%d,)); Node: set entry %d of spawn's stdio array to %d",
+			credFD, err, credFD, credFD, credFD, credFD)
+	}
 	d := &brokerDialer{credFD: credFD}
 	return &http.Client{
 		Timeout: 30 * time.Second,
@@ -125,5 +134,5 @@ func newBrokerHTTPClient(credFD int) *http.Client {
 			IdleConnTimeout:       90 * time.Second,
 			ResponseHeaderTimeout: 0,
 		},
-	}
+	}, nil
 }

internal/cli/broker_dial_unix_test.go

@@ -106,9 +106,9 @@ func TestBrokerHTTPClient_DialAndRewrite(t *testing.T) {
 	defer func() { _ = syscall.Close(childFD) }()
 	defer stop()
 
-	client := newBrokerHTTPClient(childFD)
-	if client == nil {
-		t.Fatal("newBrokerHTTPClient returned nil")
+	client, err := newBrokerHTTPClient(childFD)
+	if err != nil {
+		t.Fatalf("newBrokerHTTPClient: %v", err)
 	}
 	defer client.CloseIdleConnections() // release dispatched keep-alive conns
 	// The CLI's base URL is an http placeholder; broker rewrites host.
@@ -208,6 +208,41 @@ func TestDefaultNewClient_RejectsStdioFD(t *testing.T) {
 	}
 }
 
+// TestDefaultNewClient_CredFDNotInherited covers a caller that drops the
+// inherited control fd before exec (Python's subprocess closes fds >= 3 by
+// default): the fd number is then closed, or reused by an unrelated file.
+// Either way defaultNewClient must fail up front with an error that names the
+// fd and the fix, instead of a handshake errno at the first request.
+func TestDefaultNewClient_CredFDNotInherited(t *testing.T) {
+	t.Setenv("HOME", t.TempDir())
+	t.Setenv("FLASHDUTY_APP_KEY", "")
+
+	f, err := os.CreateTemp(t.TempDir(), "not-a-socket")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = f.Close() }()
+	pair, err := syscall.Socketpair(syscall.AF_UNIX, controlSockType, 0)
+	if err != nil {
+		t.Fatalf("socketpair: %v", err)
+	}
+	closedFD := pair[0]
+	_ = syscall.Close(pair[0])
+	_ = syscall.Close(pair[1])
+
+	for name, fd := range map[string]int{"closed fd": closedFD, "regular file": int(f.Fd())} {
+		t.Setenv("FLASHDUTY_CRED_FD", strconv.Itoa(fd))
+		_, err := defaultNewClient()
+		if err == nil {
+			t.Fatalf("%s: defaultNewClient must fail", name)
+		}
+		want := "FLASHDUTY_CRED_FD=" + strconv.Itoa(fd) + " is not an open socket"
+		if msg := err.Error(); !strings.HasPrefix(msg, want) || !strings.Contains(msg, "pass_fds=("+strconv.Itoa(fd)+",)") {
+			t.Fatalf("%s: error must start with %q and name pass_fds, got: %v", name, want, msg)
+		}
+	}
+}
+
 // TestBrokerHTTPClient_RefusedReturnsError verifies the dialer surfaces the
 // broker's 0xFF refusal (e.g. the runner failed to mint a connection) as a real
 // error instead of hanging or wrapping a nil conn.
@@ -232,7 +267,10 @@ func TestBrokerHTTPClient_RefusedReturnsError(t *testing.T) {
 		}
 	}()
 
-	client := newBrokerHTTPClient(childFD)
+	client, err := newBrokerHTTPClient(childFD)
+	if err != nil {
+		t.Fatalf("newBrokerHTTPClient: %v", err)
+	}
 	req, _ := http.NewRequestWithContext(context.Background(), "GET",
 		"http://flashduty.broker.local/x?app_key=SENTINEL", nil)
 	if _, err := client.Do(req); err == nil {

internal/cli/root.go

@@ -234,9 +234,9 @@ func defaultNewClient() (*flashduty.Client, error) {
 		if perr != nil || fd < 3 {
 			return nil, fmt.Errorf("invalid FLASHDUTY_CRED_FD=%q", fdStr)
 		}
-		hc := newBrokerHTTPClient(fd)
-		if hc == nil {
-			return nil, errBrokerUnsupported
+		hc, err := newBrokerHTTPClient(fd)
+		if err != nil {
+			return nil, err
 		}
 		opts = append(opts, flashduty.WithHTTPClient(hc))
 		appKey = "broker-sentinel" // non-empty: go-flashduty rejects ""; broker overwrites it