From 76fe2ebd1d3f4c7e14d4257f8348671a6571cc23 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 30 Sep 2026 20:23:07 -0400 Subject: [PATCH 1/6] Let the connector run on macOS The connector was Linux-only because the CLI only sealed inherited descriptors there, and the task token arrives on one. macOS can seal them too: list /dev/fd and mark each close-on-exec, as Linux's fallback already does with /proc/self/fd. The token reader is built for both, and the platform check allows macOS. The background service stays Linux-only, since it writes a systemd unit. Also, found running the connector's tests on a Mac: - macOS refuses any signal to a process group whose only members are zombies, where Linux delivers it. A finished worker's group read as "probably still there" and couldn't be confirmed gone. Such a group is now the absent group it is everywhere else, when the kernel's own listing shows nothing running. - The claude driver's "deaf" test worker blocked on an empty select, which the runtime ends as a deadlock on macOS before the pipe fills. It sleeps. --- internal/cli/inherited_fds_darwin.go | 55 ++++++++++++++ internal/cli/inherited_fds_linux.go | 59 --------------- internal/cli/inherited_fds_other.go | 6 +- internal/cli/inherited_fds_seal.go | 72 +++++++++++++++++++ internal/cli/token_handover_platforms_test.go | 2 +- internal/commands/connect.go | 2 +- internal/commands/connect_operator_test.go | 10 ++- internal/commands/connect_run.go | 6 +- internal/commands/connect_run_test.go | 7 +- internal/commands/connect_service.go | 4 ++ internal/commands/connect_service_test.go | 38 ++++++---- .../commands/connect_setup_guided_test.go | 4 +- internal/commands/mcp_token_other.go | 10 +-- ...mcp_token_linux.go => mcp_token_sealed.go} | 10 +-- internal/connector/admission/policy.go | 2 +- .../connector/driver/claude/claude_test.go | 9 ++- internal/connector/driver/worker_unix.go | 14 +++- 17 files changed, 202 insertions(+), 108 deletions(-) create mode 100644 internal/cli/inherited_fds_darwin.go create mode 100644 internal/cli/inherited_fds_seal.go rename internal/commands/{mcp_token_linux.go => mcp_token_sealed.go} (92%) diff --git a/internal/cli/inherited_fds_darwin.go b/internal/cli/inherited_fds_darwin.go new file mode 100644 index 000000000..0fdcbf727 --- /dev/null +++ b/internal/cli/inherited_fds_darwin.go @@ -0,0 +1,55 @@ +//go:build darwin + +package cli + +import ( + "fmt" + + "golang.org/x/sys/unix" + + "github.com/basecamp/basecamp-cli/internal/sysfd" +) + +// sealInheritedDescriptors keeps every descriptor this process inherited out +// of the processes it starts, by marking each one close-on-exec. It is the +// Linux one's (inherited_fds_linux.go) for macOS, where there is no +// close_range: the process's own list of descriptors is /dev/fd, and each is +// marked as the Linux fallback marks /proc/self/fd's. It fails closed in the +// same way, for the same reason. +func sealInheritedDescriptors() error { + if err := sealListedDescriptors(devFD); err == nil { + return nil + } + // /dev/fd is a filesystem macOS mounts by default, and one that is not + // there must not leave a descriptor unsealed: try every number the + // process may hold instead. + return sealEveryDescriptorUpTo(descriptorLimit()) +} + +// devFD is where macOS lists the descriptors a process holds. +const devFD = "/dev/fd" + +// descriptorLimit is the highest number a descriptor of this process can have, +// plus one. +func descriptorLimit() uint64 { + var lim unix.Rlimit + if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &lim); err != nil || lim.Cur == 0 || lim.Cur > 1<<20 { + return 1 << 16 + } + return lim.Cur +} + +// sealEveryDescriptorUpTo marks close-on-exec every open descriptor above +// standard error and below limit, asking each number in turn. +func sealEveryDescriptorUpTo(limit uint64) error { + for fd := uint64(sysfd.FirstNonStandard); fd < limit; fd++ { + d, err := sysfd.Of(uintptr(fd)) + if err != nil { + return fmt.Errorf("could not seal the descriptors this process inherited: %w", err) + } + if err := sealDescriptor(d); err != nil { + return err + } + } + return nil +} diff --git a/internal/cli/inherited_fds_linux.go b/internal/cli/inherited_fds_linux.go index 4f0011c55..b4901485d 100644 --- a/internal/cli/inherited_fds_linux.go +++ b/internal/cli/inherited_fds_linux.go @@ -3,10 +3,7 @@ package cli import ( - "errors" - "fmt" "math" - "os" "golang.org/x/sys/unix" @@ -46,59 +43,3 @@ func sealInheritedDescriptors() error { // parameter of sealListedDescriptors only so a test can watch the walk refuse // a listing it cannot trust. const procSelfFD = "/proc/self/fd" - -func sealListedDescriptors(listingDir string) error { - dir, err := os.Open(listingDir) - if err != nil { - return fmt.Errorf("could not open %s to seal the descriptors this process inherited: %w", listingDir, err) - } - defer func() { _ = dir.Close() }() - names, err := dir.Readdirnames(-1) - if err != nil { - return fmt.Errorf("could not read %s to seal the descriptors this process inherited: %w", listingDir, err) - } - listing, err := sysfd.Of(dir.Fd()) - if err != nil { - return fmt.Errorf("could not seal the descriptors this process inherited: %w", err) - } - for _, name := range names { - fd, err := sysfd.Parse(name) - if err != nil { - return fmt.Errorf("could not seal the descriptors this process inherited: %s holds %q, which is not a descriptor", listingDir, name) - } - if fd < sysfd.FirstNonStandard || fd == listing { - continue - } - if err := sealDescriptor(fd); err != nil { - return err - } - } - return nil -} - -// sealDescriptor marks one descriptor close-on-exec. -// -// A descriptor that is no longer open is the one failure that is not a -// failure: the listing is a snapshot, and a number that has been closed since -// it was taken is already out of reach of every child. Anything else means -// the descriptor is open and still inheritable, which is the thing this -// refuses to let happen quietly. -func sealDescriptor(fd sysfd.Descriptor) error { - flags, err := unix.FcntlInt(fd.Uintptr(), unix.F_GETFD, 0) - if err != nil { - if errors.Is(err, unix.EBADF) { - return nil - } - return fmt.Errorf("could not read the flags of inherited descriptor %s: %w", fd, err) - } - if flags&unix.FD_CLOEXEC != 0 { - return nil - } - if _, err := unix.FcntlInt(fd.Uintptr(), unix.F_SETFD, flags|unix.FD_CLOEXEC); err != nil { - if errors.Is(err, unix.EBADF) { - return nil - } - return fmt.Errorf("could not keep inherited descriptor %s from the processes this one starts: %w", fd, err) - } - return nil -} diff --git a/internal/cli/inherited_fds_other.go b/internal/cli/inherited_fds_other.go index b488ba929..71d19b7c2 100644 --- a/internal/cli/inherited_fds_other.go +++ b/internal/cli/inherited_fds_other.go @@ -1,10 +1,10 @@ -//go:build !linux +//go:build !linux && !darwin package cli // sealInheritedDescriptors has nothing to seal where the handover it protects // cannot happen: a task token arrives on an inherited descriptor only on -// Linux, which is what internal/commands/mcp_token_linux.go accepts and -// internal/commands/mcp_token_other.go refuses. Elsewhere this keeps the +// Linux and macOS, which is what internal/commands/mcp_token_sealed.go accepts +// and internal/commands/mcp_token_other.go refuses. Elsewhere this keeps the // program building and starting the same way. func sealInheritedDescriptors() error { return nil } diff --git a/internal/cli/inherited_fds_seal.go b/internal/cli/inherited_fds_seal.go new file mode 100644 index 000000000..7ae75e1a0 --- /dev/null +++ b/internal/cli/inherited_fds_seal.go @@ -0,0 +1,72 @@ +//go:build linux || darwin + +package cli + +import ( + "errors" + "fmt" + "os" + + "golang.org/x/sys/unix" + + "github.com/basecamp/basecamp-cli/internal/sysfd" +) + +// sealListedDescriptors marks close-on-exec every descriptor listingDir names: +// /proc/self/fd on Linux, /dev/fd on macOS, each the process's own list of +// what it holds. +func sealListedDescriptors(listingDir string) error { + dir, err := os.Open(listingDir) + if err != nil { + return fmt.Errorf("could not open %s to seal the descriptors this process inherited: %w", listingDir, err) + } + defer func() { _ = dir.Close() }() + names, err := dir.Readdirnames(-1) + if err != nil { + return fmt.Errorf("could not read %s to seal the descriptors this process inherited: %w", listingDir, err) + } + listing, err := sysfd.Of(dir.Fd()) + if err != nil { + return fmt.Errorf("could not seal the descriptors this process inherited: %w", err) + } + for _, name := range names { + fd, err := sysfd.Parse(name) + if err != nil { + return fmt.Errorf("could not seal the descriptors this process inherited: %s holds %q, which is not a descriptor", listingDir, name) + } + if fd < sysfd.FirstNonStandard || fd == listing { + continue + } + if err := sealDescriptor(fd); err != nil { + return err + } + } + return nil +} + +// sealDescriptor marks one descriptor close-on-exec. +// +// A descriptor that is no longer open is the one failure that is not a +// failure: the listing is a snapshot, and a number that has been closed since +// it was taken is already out of reach of every child. Anything else means +// the descriptor is open and still inheritable, which is the thing this +// refuses to let happen quietly. +func sealDescriptor(fd sysfd.Descriptor) error { + flags, err := unix.FcntlInt(fd.Uintptr(), unix.F_GETFD, 0) + if err != nil { + if errors.Is(err, unix.EBADF) { + return nil + } + return fmt.Errorf("could not read the flags of inherited descriptor %s: %w", fd, err) + } + if flags&unix.FD_CLOEXEC != 0 { + return nil + } + if _, err := unix.FcntlInt(fd.Uintptr(), unix.F_SETFD, flags|unix.FD_CLOEXEC); err != nil { + if errors.Is(err, unix.EBADF) { + return nil + } + return fmt.Errorf("could not keep inherited descriptor %s from the processes this one starts: %w", fd, err) + } + return nil +} diff --git a/internal/cli/token_handover_platforms_test.go b/internal/cli/token_handover_platforms_test.go index 554d8cb22..d74afb793 100644 --- a/internal/cli/token_handover_platforms_test.go +++ b/internal/cli/token_handover_platforms_test.go @@ -15,7 +15,7 @@ import ( // asks about the code rather than about a build tag it repeats. const ( sealingDir = "." - sealingFile = "inherited_fds_linux.go" + sealingFile = "inherited_fds_seal.go" handoverDir = "../commands" refusalFile = "mcp_token_other.go" ) diff --git a/internal/commands/connect.go b/internal/commands/connect.go index c966f099d..8c147a019 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -54,7 +54,7 @@ go to stderr. SIGINT and SIGTERM cancel live workers with stop reason shutdown, settle them, and exit 130 and 143. --shadow admits and logs in an isolated state directory and dispatches nothing. --hold sets a durable hold: intake and admission run, nothing dispatches or posts, and earlier records -wait for review, until basecamp connect release. Linux only. +wait for review, until basecamp connect release. Linux and macOS only. basecamp connect status what it heard, holds and ran basecamp connect doctor what it needs to run diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 563c5d9a8..3a52436b9 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -267,18 +267,16 @@ func TestConnectDoctorRefusesOnlyWhatTheRunCommandRefuses(t *testing.T) { // The check and the run command's refusal say the one reason, so a person // cannot be told two different things about the same platform. func TestConnectDoctorPlatformCheckNamesTheConstraintThatApplies(t *testing.T) { - c := connectUnsupportedOSCheck("darwin") + c := connectUnsupportedOSCheck("windows") assert.Equal(t, "Platform", c.Name) assert.Equal(t, setup.StatusFail, c.Status) - assert.Contains(t, c.Message, "darwin", "it names the platform it refuses") + assert.Contains(t, c.Message, "windows", "it names the platform it refuses") assert.Contains(t, c.Message, connectLinuxOnlyReason, "it gives the reason the run command gives") - assert.NotContains(t, c.Message, "macOS", - "a refusal must not name a capability the platform it refuses actually has") - err := connectUnsupportedOSError("darwin") + err := connectUnsupportedOSError("windows") require.Error(t, err) assert.Contains(t, err.Error(), connectLinuxOnlyReason, "doctor and the run command give the one reason") - assert.NotContains(t, err.Error(), "macOS") + assert.Contains(t, err.Error(), "Linux and macOS only, not windows") } // The acp driver runs a pinned adapter out of the connector's own npm diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 63887a2b6..78c348338 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -629,13 +629,13 @@ func connectConsumerNamespace(agentID int64, shadow bool) string { // at the handshake, so the connector says so here rather than at the far // end of each task. func connectSupportedOS(goos string) bool { - return goos == "linux" + return goos == "linux" || goos == "darwin" } // connectLinuxOnlyReason is why, in one place: the run command's refusal and // doctor's Platform check say the same thing, so a person who meets one and // then the other is not told two different stories about their machine. -const connectLinuxOnlyReason = "the task token reaches a worker's MCP server over an inherited descriptor, and Linux is the only platform that seals the descriptors a process inherits" +const connectLinuxOnlyReason = "the task token reaches a worker's MCP server over an inherited descriptor, and the CLI keeps inherited descriptors from the programs it starts only on Linux and macOS" // connectUnsupportedOSError is the refusal on a platform the connector does // not run on, given by the run command and by `service install`, which would @@ -645,7 +645,7 @@ const connectLinuxOnlyReason = "the task token reaches a worker's MCP server ove // refusal is prose, not a command to run, and a hint that begins like a // command is read as one — by a person, and by TestHintCommandsResolve. func connectUnsupportedOSError(goos string) error { - return output.ErrUsage(fmt.Sprintf("The connector runs on Linux only, not %s: %s", goos, connectLinuxOnlyReason)) + return output.ErrUsage(fmt.Sprintf("The connector runs on Linux and macOS only, not %s: %s", goos, connectLinuxOnlyReason)) } // connectServed is connect.json's served projects as they are now, not as diff --git a/internal/commands/connect_run_test.go b/internal/commands/connect_run_test.go index 37a058e9d..f7cf68ee0 100644 --- a/internal/commands/connect_run_test.go +++ b/internal/commands/connect_run_test.go @@ -50,10 +50,11 @@ func TestConnectStateLivesUnderXDGStateHome(t *testing.T) { // Copilot on #738: macOS passed this check and then failed every non-shadow // dispatch at the worker's MCP handshake, because the token hand-over onto // an inherited descriptor is accepted only where those descriptors are -// sealed — Linux (#736). -func TestConnectRunsOnLinuxOnly(t *testing.T) { +// sealed — Linux (#736), and macOS, which seals them through /dev/fd. +func TestConnectRunsOnLinuxAndMacOS(t *testing.T) { assert.True(t, connectSupportedOS("linux")) - for _, goos := range []string{"darwin", "freebsd", "openbsd", "windows"} { + assert.True(t, connectSupportedOS("darwin")) + for _, goos := range []string{"freebsd", "openbsd", "windows"} { assert.False(t, connectSupportedOS(goos), goos) } } diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go index 824940ff1..d8319c208 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -189,6 +189,10 @@ func connectServiceProfile(app *appctx.App) (string, error) { if !connectSupportedOS(connectServiceGOOS) { return "", connectUnsupportedOSError(connectServiceGOOS) } + // The connector runs on macOS too; the unit this writes is systemd's. + if connectServiceGOOS != "linux" { + return "", output.ErrUsage(fmt.Sprintf("The connector's background service is a systemd unit, so it runs on Linux only, not %s. Start the connector yourself in the folder it should work in.", connectServiceGOOS)) + } name := app.Config.ActiveProfile if name == "" { return "", output.ErrUsageHint("The connector's service needs the agent's profile", "Pass -P/--profile , a profile set up with `basecamp connect setup`.") diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go index 0101189e6..6f2406428 100644 --- a/internal/commands/connect_service_test.go +++ b/internal/commands/connect_service_test.go @@ -334,23 +334,31 @@ func TestConnectServiceInstallRefusesWhenSystemdReadsAnotherFile(t *testing.T) { assert.Contains(t, err.Error(), "/etc/systemd/user/basecamp-connect-agent.service") } -// The platform gate is the run command's, so a Mac is refused before a unit -// is written for a connector that would not start on it. +// The platform gate is the run command's, so a platform the connector does +// not run on is refused before a unit is written for it; and the unit is +// systemd's, so a Mac, where the connector runs, is refused for that. func TestConnectServiceRefusesAPlatformTheConnectorDoesNotRunOn(t *testing.T) { - connectServiceHome(t) - writeConnectSetup(t, "agent") - prev := connectServiceGOOS - connectServiceGOOS = "darwin" - t.Cleanup(func() { connectServiceGOOS = prev }) - app, _ := connectServiceApp(t, "agent") - - _, err := runConnectServiceCmd(t, app, "install") - require.Error(t, err) - assert.Contains(t, err.Error(), connectLinuxOnlyReason) + for goos, want := range map[string]string{ + "windows": connectLinuxOnlyReason, + "darwin": "background service is a systemd unit, so it runs on Linux only, not darwin", + } { + t.Run(goos, func(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + prev := connectServiceGOOS + connectServiceGOOS = goos + t.Cleanup(func() { connectServiceGOOS = prev }) + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install") + require.Error(t, err) + assert.Contains(t, err.Error(), want) - path, err := connectServiceUnitPath("agent") - require.NoError(t, err) - assert.NoFileExists(t, path) + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + assert.NoFileExists(t, path) + }) + } } // When systemctl is missing, or fails in the running rather than in what it diff --git a/internal/commands/connect_setup_guided_test.go b/internal/commands/connect_setup_guided_test.go index 078fbd090..d2f54bbc6 100644 --- a/internal/commands/connect_setup_guided_test.go +++ b/internal/commands/connect_setup_guided_test.go @@ -271,11 +271,11 @@ func TestGuidedConnectSetupRefusesAnUnsupportedPlatformBeforeConnecting(t *testi ownedByTheOperator(s) s.agentProjects = projectsNamed(setupProject) guided(t, &scriptedPrompter{}) - connectServiceGOOS = "darwin" + connectServiceGOOS = "windows" out, err := runConnectSetupCmd(t, bareSetupApp(t, s, "agent")) require.Error(t, err, out) - assert.Contains(t, err.Error(), "Linux only, not darwin") + assert.Contains(t, err.Error(), "Linux and macOS only, not windows") assert.NotContains(t, out, "connect this computer") assertNotWritten(t, "agent") } diff --git a/internal/commands/mcp_token_other.go b/internal/commands/mcp_token_other.go index 6a11677df..a545017ef 100644 --- a/internal/commands/mcp_token_other.go +++ b/internal/commands/mcp_token_other.go @@ -1,4 +1,4 @@ -//go:build !linux +//go:build !linux && !darwin package commands @@ -8,14 +8,14 @@ import ( ) // readTaskToken is refused wherever the descriptor carrying the token cannot -// be kept from the processes this one starts. Only Linux seals the +// be kept from the processes this one starts. Only Linux and macOS seal the // descriptors this process inherited, in internal/cli's -// inherited_fds_linux.go, so only Linux may be handed a credential on one: +// inherited_fds_seal.go, so only they may be handed a credential on one: // accepting a token where nothing seals it would leave it inheritable through // every hook that runs before this command, which is the whole point of // handing it over on a descriptor instead of in a file or an environment. // -// The connector runs on Linux, so this refuses nothing that works today. +// The connector runs on Linux and macOS, so this refuses nothing that works today. func readTaskToken(sysfd.Descriptor) (string, error) { - return "", output.ErrUsage("--connect-state is only available on Linux, where an inherited descriptor can be kept from the processes this one starts") + return "", output.ErrUsage("--connect-state is only available on Linux and macOS, where an inherited descriptor can be kept from the processes this one starts") } diff --git a/internal/commands/mcp_token_linux.go b/internal/commands/mcp_token_sealed.go similarity index 92% rename from internal/commands/mcp_token_linux.go rename to internal/commands/mcp_token_sealed.go index dbbbe30a3..fe5de3151 100644 --- a/internal/commands/mcp_token_linux.go +++ b/internal/commands/mcp_token_sealed.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || darwin package commands @@ -22,10 +22,10 @@ import ( // exists at a path, in argv or in the environment; once read, the descriptor // is gone too, and nothing this process starts can inherit it. // -// This file is built for Linux alone, and shares that constraint with -// internal/cli's inherited_fds_linux.go on purpose: a credential may arrive -// on an inherited descriptor only where startup has already sealed every -// inherited descriptor against the children the pre-command hooks start. +// This file is built for Linux and macOS alone, and shares that constraint +// with internal/cli's inherited_fds_seal.go on purpose: a credential may +// arrive on an inherited descriptor only where startup has already sealed +// every inherited descriptor against the children the pre-command hooks start. // Everywhere else mcp_token_other.go refuses the handover, and // TestTheTokenIsOnlyReadWhereItIsSealed holds the two constraints together. // diff --git a/internal/connector/admission/policy.go b/internal/connector/admission/policy.go index 270f756c6..e63b0f975 100644 --- a/internal/connector/admission/policy.go +++ b/internal/connector/admission/policy.go @@ -176,7 +176,7 @@ func checkLegacyPathValue(raw json.RawMessage) error { if lossyJSONString(bytes.TrimSpace(raw)) { return errors.New(`the "path" of a connector that routed projects is not encoded the way it was written: it holds malformed UTF-8 or an unpaired surrogate escape, which encoding/json silently replaces with U+FFFD, so the value read back is not the value in the file`) } - // POSIX, not this host's rules. The connector runs on Linux only, so the + // POSIX, not this host's rules. The connector runs on Linux and macOS, so the // writer of this key wrote a POSIX path — and path.IsAbs answers for // that format on every platform, where filepath.IsAbs answers for // whatever the reader happens to be compiled for. This package builds diff --git a/internal/connector/driver/claude/claude_test.go b/internal/connector/driver/claude/claude_test.go index 1aa4e1e7a..995a92d90 100644 --- a/internal/connector/driver/claude/claude_test.go +++ b/internal/connector/driver/claude/claude_test.go @@ -112,13 +112,16 @@ func fakeClaude(scenario string) { } if scenario == "deaf" || scenario == "deaf-secret" { - // Reads nothing, ever: the pipe fills and a write blocks. - select {} + // Reads nothing, ever: the pipe fills and a write blocks. A sleep, + // not an empty select: with nothing else running, the runtime calls + // an empty select a deadlock and ends the process, which on macOS + // it does before the pipe fills. + time.Sleep(time.Hour) } if scenario == "badmode-eager" { // An init before any prompt, in a mode the policy did not ask for. emit(map[string]any{"type": "system", "subtype": "init", "session_id": sessionID, "permissionMode": "bypassPermissions", "mcp_servers": []any{}}) - select {} + time.Sleep(time.Hour) } in := bufio.NewScanner(os.Stdin) diff --git a/internal/connector/driver/worker_unix.go b/internal/connector/driver/worker_unix.go index 12bde4d01..3da0c6508 100644 --- a/internal/connector/driver/worker_unix.go +++ b/internal/connector/driver/worker_unix.go @@ -3,6 +3,7 @@ package driver import ( + "errors" "os/exec" "sync" "syscall" @@ -28,7 +29,18 @@ func signalGroup(pgid int, sig syscall.Signal) error { if pgid <= 1 || pgid == syscall.Getpgrp() { return syscall.EINVAL } - return syscall.Kill(-pgid, sig) + err := syscall.Kill(-pgid, sig) + if errors.Is(err, syscall.EPERM) { + // macOS refuses any signal to a group whose only members are + // zombies, where Linux delivers it; nothing in such a group runs. + // When the kernel's own listing says so, it is the absent group it + // is everywhere else. A group that runs but may not be signaled + // (another user's) still lists its members, and keeps the refusal. + if running, listErr := groupRunning(pgid); listErr == nil && !running { + return syscall.ESRCH + } + } + return err } // probeGroup is a probe run as the leader of a group of its own. From 6897300ae1c4a26f16eafcfbcb4cb90b63edbc24 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 30 Sep 2026 20:30:55 -0400 Subject: [PATCH 2/6] On macOS, a peer that left before it was named is undelivered, not refused macOS answers LOCAL_PEERPID only while the peer is connected (it still names the user). A worker's MCP server that closed before its token was read was recorded as an untrusted connection and spent the socket; it is now undelivered, as on Linux, and its next start gets the token. --- internal/connector/tokensocket.go | 30 +++++++++++++++++------- internal/connector/tokensocket_darwin.go | 6 +++++ internal/connector/tokensocket_test.go | 25 ++++++++++++++++++++ 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/internal/connector/tokensocket.go b/internal/connector/tokensocket.go index dccbc38fd..8c1441de1 100644 --- a/internal/connector/tokensocket.go +++ b/internal/connector/tokensocket.go @@ -645,7 +645,12 @@ func (s *TokenSocket) handOne(window time.Duration) (Handoff, driver.Process) { } defer func() { _ = conn.Close() }() _ = conn.SetDeadline(deadline) - if !s.trusted(conn, deadline) { + if ok, gone := s.trusted(conn, deadline); !ok { + if gone { + // This user's process, closed before it could be named: nothing + // untrusted asked, and nothing was handed over. + return HandoffUndelivered, driver.Process{} + } return HandoffRefused, driver.Process{} } if _, err := conn.Write([]byte(s.token + "\n")); err != nil { @@ -667,12 +672,21 @@ func (s *TokenSocket) allowedGroup() int { } } +// errPeerGone is a peer that closed before the kernel would name its +// process. macOS answers LOCAL_PEERPID only while the peer is connected, and +// still names its user. +var errPeerGone = errors.New("connector: the peer closed before it could be identified") + // trusted reports whether the peer is this user's process in the worker's -// own process group. -func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) bool { +// own process group. gone is a peer of this user's that closed before its +// process could be named: not trusted, and not a refusal either. +func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) (ok, gone bool) { cred, err := s.peer(conn) + if errors.Is(err, errPeerGone) { + return false, cred.UID == os.Getuid() + } if err != nil || cred.UID != os.Getuid() || cred.PID <= 0 { - return false + return false, false } ctx, cancel := context.WithDeadline(context.Background(), deadline) defer cancel() @@ -681,15 +695,15 @@ func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) bool { case want = <-s.group: s.group <- want case <-ctx.Done(): - return false + return false, false } if want <= 1 { - return false + return false, false } if got, err := s.groupOf(cred.PID); err == nil && got == want { - return true + return true, false } - return s.descendsFrom(cred.PID, want) + return s.descendsFrom(cred.PID, want), false } // maxAncestry bounds the walk up a peer's parents. diff --git a/internal/connector/tokensocket_darwin.go b/internal/connector/tokensocket_darwin.go index c2e09369c..71d7cd38b 100644 --- a/internal/connector/tokensocket_darwin.go +++ b/internal/connector/tokensocket_darwin.go @@ -1,6 +1,7 @@ package connector import ( + "errors" "net" "golang.org/x/sys/unix" @@ -33,6 +34,11 @@ func peerCredentials(conn *net.UnixConn) (PeerCredentials, error) { if credOK != nil { return PeerCredentials{}, credOK } + if errors.Is(pidOK, unix.ENOTCONN) { + // The user is still answered; the process is not, once the peer has + // closed. + return PeerCredentials{UID: int(cred.Uid)}, errPeerGone + } if pidOK != nil { return PeerCredentials{}, pidOK } diff --git a/internal/connector/tokensocket_test.go b/internal/connector/tokensocket_test.go index 531d29461..1caf5dc31 100644 --- a/internal/connector/tokensocket_test.go +++ b/internal/connector/tokensocket_test.go @@ -526,3 +526,28 @@ func TestAHandoffStillInFlightAtTheReleasePointHoldsTheAttempt(t *testing.T) { holder := settledTaker(s, slog.New(slog.DiscardHandler), "att", 50*time.Millisecond) assert.True(t, holder.Held(), "an attempt is not released around a handoff that is still deciding") } + +// macOS names a peer's process only while it is connected: a peer of this +// user's that closed first is not a refusal (nothing untrusted asked) and is +// handed nothing; the socket waits for the next start. Another user's is +// still refused. +func TestAPeerGoneBeforeItIsNamedIsUndeliveredNotRefused(t *testing.T) { + for uid, want := range map[int]Handoff{os.Getuid(): HandoffUndelivered, os.Getuid() + 1: HandoffRefused} { + gone := func(*net.UnixConn) (PeerCredentials, error) { return PeerCredentials{UID: uid}, errPeerGone } + s, err := serveTaskTokenWith(tokenDir(t), socketTestToken, 5*time.Second, gone, + processGroupOf, parentProcessOf, driver.LookupProcess) + require.NoError(t, err) + handoffs := make(chan Handoff, 4) + s.OnHandoff(func(h Handoff, _ driver.Process, _ bool) { handoffs <- h }) + s.AllowGroup(syscall.Getpgrp()) + + dialer := net.Dialer{Timeout: 2 * time.Second} + conn, err := dialer.DialContext(context.Background(), "unix", s.Path()) + require.NoError(t, err) + got, _ := io.ReadAll(conn) + _ = conn.Close() + assert.Empty(t, got, "no token for a peer that could not be named") + assert.Equal(t, want, <-handoffs, "uid %d", uid) + s.Close() + } +} From 653ee247f3cc07165821fedefd6761b296dd3016 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 30 Sep 2026 21:02:12 -0400 Subject: [PATCH 3/6] Address review on macOS support - The /dev/fd fallback fails closed: it refuses when the descriptor limit can't be read or is too large to scan, rather than checking a smaller range. - The zombie-only group refusal applies on macOS alone: on Linux, EPERM is a group this process may not signal, and /proc mounted hidepid can't list its members to say otherwise. - The token's taker is the peer the trust check named before the write, not a second reading: a server that reads its token and closes at once would come back unnamed on macOS and hold the attempt. - The other platforms' token refusal names --connect-token-fd. - Both skills say the connector runs on Linux and macOS. - Comments and names that still said Linux only. - Tests: macOS sealing (/dev/fd walk and fallback), the token reader and the MCP connect flow now build and run on macOS too. --- internal/cli/inherited_fds_darwin.go | 26 ++++-- internal/cli/inherited_fds_darwin_test.go | 90 +++++++++++++++++++ internal/commands/connect_doctor.go | 2 +- internal/commands/connect_operator_test.go | 4 +- internal/commands/connect_run.go | 30 +++---- internal/commands/connect_service.go | 6 +- internal/commands/connect_service_test.go | 4 +- internal/commands/connect_worker_mcp_other.go | 2 +- ...ect_test.go => mcp_connect_sealed_test.go} | 2 +- ...st.go => mcp_connect_token_sealed_test.go} | 2 +- internal/commands/mcp_token_other.go | 2 +- internal/connector/driver/worker_unix.go | 6 +- internal/connector/tokensocket.go | 31 ++++--- internal/connector/tokensocket_test.go | 37 +++++++- skills/basecamp-connect/SKILL.md | 5 +- skills/basecamp/SKILL.md | 6 +- 16 files changed, 199 insertions(+), 56 deletions(-) create mode 100644 internal/cli/inherited_fds_darwin_test.go rename internal/commands/{mcp_connect_test.go => mcp_connect_sealed_test.go} (99%) rename internal/commands/{mcp_connect_token_linux_test.go => mcp_connect_token_sealed_test.go} (99%) diff --git a/internal/cli/inherited_fds_darwin.go b/internal/cli/inherited_fds_darwin.go index 0fdcbf727..82e1f3dac 100644 --- a/internal/cli/inherited_fds_darwin.go +++ b/internal/cli/inherited_fds_darwin.go @@ -22,21 +22,33 @@ func sealInheritedDescriptors() error { } // /dev/fd is a filesystem macOS mounts by default, and one that is not // there must not leave a descriptor unsealed: try every number the - // process may hold instead. - return sealEveryDescriptorUpTo(descriptorLimit()) + // process may hold instead, or fail closed when that number is not known. + limit, err := descriptorLimit() + if err != nil { + return err + } + return sealEveryDescriptorUpTo(limit) } // devFD is where macOS lists the descriptors a process holds. const devFD = "/dev/fd" +// maxDescriptorScan is the most descriptor numbers the fallback will try one +// by one. A limit above it is not scanned partly: that would report every +// descriptor sealed while one above the scan could still be inherited. +const maxDescriptorScan = 1 << 20 + // descriptorLimit is the highest number a descriptor of this process can have, -// plus one. -func descriptorLimit() uint64 { +// plus one, or an error when it cannot be known or is too large to scan. +func descriptorLimit() (uint64, error) { var lim unix.Rlimit - if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &lim); err != nil || lim.Cur == 0 || lim.Cur > 1<<20 { - return 1 << 16 + if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &lim); err != nil { + return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be read, and neither could the descriptor limit: %w", err) + } + if lim.Cur == 0 || lim.Cur > maxDescriptorScan { + return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be read, and the descriptor limit (%d) is too large to check one by one", lim.Cur) } - return lim.Cur + return lim.Cur, nil } // sealEveryDescriptorUpTo marks close-on-exec every open descriptor above diff --git a/internal/cli/inherited_fds_darwin_test.go b/internal/cli/inherited_fds_darwin_test.go new file mode 100644 index 000000000..98a7feb19 --- /dev/null +++ b/internal/cli/inherited_fds_darwin_test.go @@ -0,0 +1,90 @@ +//go:build darwin + +package cli + +import ( + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +// The descriptor number is high on purpose: a child picks the lowest free one +// for its own files. +const inheritedFDDarwin = 200 + +// inheritDarwin puts a readable pipe at fd, the way an exec'd child receives +// one: open, with no close-on-exec flag of its own. +func inheritDarwin(t *testing.T, fd int) { + t.Helper() + reader, writer, err := os.Pipe() + require.NoError(t, err) + _, err = writer.WriteString("a-task-token\n") + require.NoError(t, err) + require.NoError(t, writer.Close()) + t.Cleanup(func() { _ = reader.Close() }) + + require.NoError(t, unix.Dup2(int(reader.Fd()), fd)) + t.Cleanup(func() { _ = unix.Close(fd) }) + flags, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + require.NoError(t, err) + require.Zero(t, flags&unix.FD_CLOEXEC, "an inherited descriptor arrives without it") +} + +func cloexec(t *testing.T, fd int) bool { + t.Helper() + flags, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + require.NoError(t, err) + return flags&unix.FD_CLOEXEC != 0 +} + +// On macOS an inherited descriptor does not reach the processes this one +// starts, and stays readable here. +func TestSealedDescriptorsDoNotReachChildrenOnMacOS(t *testing.T) { + inheritDarwin(t, inheritedFDDarwin) + + require.NoError(t, sealInheritedDescriptors()) + + assert.True(t, cloexec(t, inheritedFDDarwin)) + out, err := exec.CommandContext(t.Context(), "/bin/sh", "-c", + "if (: <&200) 2>/dev/null; then echo inherited; else echo sealed; fi").Output() + require.NoError(t, err) + assert.Equal(t, "sealed\n", string(out), "the child inherited the descriptor") + + buffer := make([]byte, len("a-task-token\n")) + read, err := unix.Read(inheritedFDDarwin, buffer) + require.NoError(t, err) + assert.Equal(t, "a-task-token\n", string(buffer[:read]), "marked, not closed or consumed") +} + +// /dev/fd is how macOS lists a process's descriptors, and the walk seals what +// it names. +func TestTheDevFDWalkSealsWhatItNames(t *testing.T) { + inheritDarwin(t, inheritedFDDarwin) + require.NoError(t, sealListedDescriptors(devFD)) + assert.True(t, cloexec(t, inheritedFDDarwin)) +} + +// Without /dev/fd the fallback tries every number up to the descriptor limit, +// and seals the same descriptor. +func TestTheFallbackSealsEveryDescriptorUpToTheLimit(t *testing.T) { + inheritDarwin(t, inheritedFDDarwin) + limit, err := descriptorLimit() + require.NoError(t, err) + require.Greater(t, limit, uint64(inheritedFDDarwin)) + + require.NoError(t, sealEveryDescriptorUpTo(limit)) + assert.True(t, cloexec(t, inheritedFDDarwin)) + for _, fd := range []int{0, 1, 2} { + _, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + assert.NoError(t, err, "standard descriptor %d is left open", fd) + } +} + +// A listing that cannot be read is an error, never a quiet success. +func TestTheDevFDWalkRefusesAListingItCannotRead(t *testing.T) { + require.Error(t, sealListedDescriptors(t.TempDir()+"/missing")) +} diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 3778cbb83..1a32cef8b 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -371,7 +371,7 @@ func preflightReason(err error) string { // then refused it anyway. func connectUnsupportedOSCheck(goos string) setup.Check { return setup.Check{Name: "Platform", Status: setup.StatusFail, - Message: fmt.Sprintf("The connector does not run on %s: %s", goos, connectLinuxOnlyReason), + Message: fmt.Sprintf("The connector does not run on %s: %s", goos, connectSupportedOSReason), Hint: "Run the connector on Linux; the rest of the CLI runs here."} } diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 3a52436b9..7d53bbaac 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -271,11 +271,11 @@ func TestConnectDoctorPlatformCheckNamesTheConstraintThatApplies(t *testing.T) { assert.Equal(t, "Platform", c.Name) assert.Equal(t, setup.StatusFail, c.Status) assert.Contains(t, c.Message, "windows", "it names the platform it refuses") - assert.Contains(t, c.Message, connectLinuxOnlyReason, "it gives the reason the run command gives") + assert.Contains(t, c.Message, connectSupportedOSReason, "it gives the reason the run command gives") err := connectUnsupportedOSError("windows") require.Error(t, err) - assert.Contains(t, err.Error(), connectLinuxOnlyReason, "doctor and the run command give the one reason") + assert.Contains(t, err.Error(), connectSupportedOSReason, "doctor and the run command give the one reason") assert.Contains(t, err.Error(), "Linux and macOS only, not windows") } diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 78c348338..3755693fc 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -614,28 +614,26 @@ func connectConsumerNamespace(agentID int64, shadow bool) string { return name } -// connectSupportedOS is where the connector runs: Linux, and for now only -// Linux. +// connectSupportedOS is where the connector runs: Linux and macOS. // -// Two things have to hold, and macOS has only one of them. The driver must -// be able to read process start times, so a recorded worker group is never -// signaled after its pid was reused — macOS can. And the task token has to -// reach the worker's MCP server, which it does on an inherited descriptor: -// `connect worker-mcp` execs `basecamp mcp --connect-token-fd`, and that -// hand-over is accepted only where the descriptors this process inherited -// are sealed against everything it starts, which is Linux alone -// (mcp_token_linux.go, and #736, which gated it deliberately). On macOS -// every non-shadow dispatch would start a worker whose Basecamp tools fail -// at the handshake, so the connector says so here rather than at the far -// end of each task. +// Two things have to hold. The driver must be able to read process start +// times, so a recorded worker group is never signaled after its pid was +// reused. And the task token has to reach the worker's MCP server, which it +// does on an inherited descriptor: `connect worker-mcp` execs `basecamp mcp +// --connect-token-fd`, and that hand-over is accepted only where the +// descriptors this process inherited are sealed against everything it +// starts (mcp_token_sealed.go, internal/cli's inherited_fds_seal.go). Linux +// and macOS do both; anywhere else every non-shadow dispatch would start a +// worker whose Basecamp tools fail at the handshake, so the connector says so +// here rather than at the far end of each task. func connectSupportedOS(goos string) bool { return goos == "linux" || goos == "darwin" } -// connectLinuxOnlyReason is why, in one place: the run command's refusal and +// connectSupportedOSReason is why, in one place: the run command's refusal and // doctor's Platform check say the same thing, so a person who meets one and // then the other is not told two different stories about their machine. -const connectLinuxOnlyReason = "the task token reaches a worker's MCP server over an inherited descriptor, and the CLI keeps inherited descriptors from the programs it starts only on Linux and macOS" +const connectSupportedOSReason = "the task token reaches a worker's MCP server over an inherited descriptor, and the CLI keeps inherited descriptors from the programs it starts only on Linux and macOS" // connectUnsupportedOSError is the refusal on a platform the connector does // not run on, given by the run command and by `service install`, which would @@ -645,7 +643,7 @@ const connectLinuxOnlyReason = "the task token reaches a worker's MCP server ove // refusal is prose, not a command to run, and a hint that begins like a // command is read as one — by a person, and by TestHintCommandsResolve. func connectUnsupportedOSError(goos string) error { - return output.ErrUsage(fmt.Sprintf("The connector runs on Linux and macOS only, not %s: %s", goos, connectLinuxOnlyReason)) + return output.ErrUsage(fmt.Sprintf("The connector runs on Linux and macOS only, not %s: %s", goos, connectSupportedOSReason)) } // connectServed is connect.json's served projects as they are now, not as diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go index d8319c208..eca0e1bc8 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -32,9 +32,9 @@ import ( // OS. These commands write the unit that does the restarting, and take it // away again. // -// systemd only. The connector runs on Linux alone, for the reason the run -// command gives, so there is no second supervisor to write for: a launchd -// agent would supervise a process that refuses to start. +// systemd only, so Linux only. The connector also runs on macOS, where a +// launchd agent would be the supervisor; none is written yet, and the service +// stays hidden until one is. // connectServiceUnitPrefix begins every unit this writes. The profile name // completes it, so one machine can supervise several agents. diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go index 6f2406428..d06e74333 100644 --- a/internal/commands/connect_service_test.go +++ b/internal/commands/connect_service_test.go @@ -337,9 +337,9 @@ func TestConnectServiceInstallRefusesWhenSystemdReadsAnotherFile(t *testing.T) { // The platform gate is the run command's, so a platform the connector does // not run on is refused before a unit is written for it; and the unit is // systemd's, so a Mac, where the connector runs, is refused for that. -func TestConnectServiceRefusesAPlatformTheConnectorDoesNotRunOn(t *testing.T) { +func TestConnectServiceRefusesAnInstallOffLinux(t *testing.T) { for goos, want := range map[string]string{ - "windows": connectLinuxOnlyReason, + "windows": connectSupportedOSReason, "darwin": "background service is a systemd unit, so it runs on Linux only, not darwin", } { t.Run(goos, func(t *testing.T) { diff --git a/internal/commands/connect_worker_mcp_other.go b/internal/commands/connect_worker_mcp_other.go index 0ca35b9e8..3ce0c0d73 100644 --- a/internal/commands/connect_worker_mcp_other.go +++ b/internal/commands/connect_worker_mcp_other.go @@ -5,5 +5,5 @@ package commands import "errors" func execWorkerMCP(string, string, string, string) error { - return errors.New("worker-mcp runs on Linux only") + return errors.New("worker-mcp runs on Linux and macOS only") } diff --git a/internal/commands/mcp_connect_test.go b/internal/commands/mcp_connect_sealed_test.go similarity index 99% rename from internal/commands/mcp_connect_test.go rename to internal/commands/mcp_connect_sealed_test.go index e8e328a84..141dbc6cd 100644 --- a/internal/commands/mcp_connect_test.go +++ b/internal/commands/mcp_connect_sealed_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || darwin package commands diff --git a/internal/commands/mcp_connect_token_linux_test.go b/internal/commands/mcp_connect_token_sealed_test.go similarity index 99% rename from internal/commands/mcp_connect_token_linux_test.go rename to internal/commands/mcp_connect_token_sealed_test.go index bee398b8d..f6e5b4caf 100644 --- a/internal/commands/mcp_connect_token_linux_test.go +++ b/internal/commands/mcp_connect_token_sealed_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || darwin package commands diff --git a/internal/commands/mcp_token_other.go b/internal/commands/mcp_token_other.go index a545017ef..f14cfd2b1 100644 --- a/internal/commands/mcp_token_other.go +++ b/internal/commands/mcp_token_other.go @@ -17,5 +17,5 @@ import ( // // The connector runs on Linux and macOS, so this refuses nothing that works today. func readTaskToken(sysfd.Descriptor) (string, error) { - return "", output.ErrUsage("--connect-state is only available on Linux and macOS, where an inherited descriptor can be kept from the processes this one starts") + return "", output.ErrUsage("--connect-token-fd is only available on Linux and macOS, where an inherited descriptor can be kept from the processes this one starts") } diff --git a/internal/connector/driver/worker_unix.go b/internal/connector/driver/worker_unix.go index 3da0c6508..0912ee7c4 100644 --- a/internal/connector/driver/worker_unix.go +++ b/internal/connector/driver/worker_unix.go @@ -5,6 +5,7 @@ package driver import ( "errors" "os/exec" + "runtime" "sync" "syscall" "time" @@ -30,7 +31,10 @@ func signalGroup(pgid int, sig syscall.Signal) error { return syscall.EINVAL } err := syscall.Kill(-pgid, sig) - if errors.Is(err, syscall.EPERM) { + // macOS only: Linux delivers a signal to a zombie-only group, so EPERM + // there is a group this process may not signal — and with /proc mounted + // hidepid, its listing cannot see that group's members to say otherwise. + if runtime.GOOS == "darwin" && errors.Is(err, syscall.EPERM) { // macOS refuses any signal to a group whose only members are // zombies, where Linux delivers it; nothing in such a group runs. // When the kernel's own listing says so, it is the absent group it diff --git a/internal/connector/tokensocket.go b/internal/connector/tokensocket.go index 8c1441de1..be6c16a69 100644 --- a/internal/connector/tokensocket.go +++ b/internal/connector/tokensocket.go @@ -645,7 +645,8 @@ func (s *TokenSocket) handOne(window time.Duration) (Handoff, driver.Process) { } defer func() { _ = conn.Close() }() _ = conn.SetDeadline(deadline) - if ok, gone := s.trusted(conn, deadline); !ok { + peer, ok, gone := s.trusted(conn, deadline) + if !ok { if gone { // This user's process, closed before it could be named: nothing // untrusted asked, and nothing was handed over. @@ -658,7 +659,10 @@ func (s *TokenSocket) handOne(window time.Duration) (Handoff, driver.Process) { // socket a peer that has gone makes this EPIPE at once. return HandoffUndelivered, driver.Process{} } - return HandoffDelivered, s.takerOfConn(conn) + // The peer named before the write, not asked again: a server that reads + // its token and closes at once is the ordinary case, and macOS no longer + // names a peer that has closed. + return HandoffDelivered, s.takerOf(peer.PID) } // allowedGroup is the worker's process group, or 0 before it is named. @@ -680,13 +684,13 @@ var errPeerGone = errors.New("connector: the peer closed before it could be iden // trusted reports whether the peer is this user's process in the worker's // own process group. gone is a peer of this user's that closed before its // process could be named: not trusted, and not a refusal either. -func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) (ok, gone bool) { +func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) (peer PeerCredentials, ok, gone bool) { cred, err := s.peer(conn) if errors.Is(err, errPeerGone) { - return false, cred.UID == os.Getuid() + return cred, false, cred.UID == os.Getuid() } if err != nil || cred.UID != os.Getuid() || cred.PID <= 0 { - return false, false + return cred, false, false } ctx, cancel := context.WithDeadline(context.Background(), deadline) defer cancel() @@ -695,15 +699,15 @@ func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) (ok, gone case want = <-s.group: s.group <- want case <-ctx.Done(): - return false, false + return cred, false, false } if want <= 1 { - return false, false + return cred, false, false } if got, err := s.groupOf(cred.PID); err == nil && got == want { - return true, false + return cred, true, false } - return s.descendsFrom(cred.PID, want), false + return cred, s.descendsFrom(cred.PID, want), false } // maxAncestry bounds the walk up a peer's parents. @@ -724,16 +728,15 @@ func (s *TokenSocket) descendsFrom(pid, ancestor int) bool { return false } -// takerOfConn is the identity of the process the token just went to, so the +// takerOf is the identity of the process the token just went to, so the // release point can end it: it is outside the worker's process group whenever // the agent started it in one of its own. A restarted MCP server is a new // process, and the newest is the one holding the token. -func (s *TokenSocket) takerOfConn(conn *net.UnixConn) driver.Process { - cred, err := s.peer(conn) - if err != nil || cred.PID <= 0 { +func (s *TokenSocket) takerOf(pid int) driver.Process { + if pid <= 0 { return driver.Process{} } - taker, err := s.lookup(cred.PID) + taker, err := s.lookup(pid) if err != nil { return driver.Process{} } diff --git a/internal/connector/tokensocket_test.go b/internal/connector/tokensocket_test.go index 1caf5dc31..fcfb42692 100644 --- a/internal/connector/tokensocket_test.go +++ b/internal/connector/tokensocket_test.go @@ -544,10 +544,45 @@ func TestAPeerGoneBeforeItIsNamedIsUndeliveredNotRefused(t *testing.T) { dialer := net.Dialer{Timeout: 2 * time.Second} conn, err := dialer.DialContext(context.Background(), "unix", s.Path()) require.NoError(t, err) - got, _ := io.ReadAll(conn) + got, err := io.ReadAll(conn) + require.NoError(t, err) _ = conn.Close() assert.Empty(t, got, "no token for a peer that could not be named") assert.Equal(t, want, <-handoffs, "uid %d", uid) s.Close() } } + +// The process that took the token is the one the trust check named before +// the write. A server that reads its token and closes at once is the +// ordinary case, and macOS will not name a peer that has closed: asking a +// second time would lose the taker, and hold the attempt over a delivery that +// worked. +func TestTheTakerIsThePeerNamedBeforeTheWrite(t *testing.T) { + var calls atomic.Int32 + namedOnce := func(*net.UnixConn) (PeerCredentials, error) { + if calls.Add(1) > 1 { + return PeerCredentials{UID: os.Getuid()}, errPeerGone + } + return PeerCredentials{PID: os.Getpid(), UID: os.Getuid()}, nil + } + s, err := serveTaskTokenWith(tokenDir(t), socketTestToken, 5*time.Second, namedOnce, + processGroupOf, parentProcessOf, driver.LookupProcess) + require.NoError(t, err) + defer s.Close() + type handed struct { + h Handoff + taker driver.Process + } + handoffs := make(chan handed, 2) + s.OnHandoff(func(h Handoff, taker driver.Process, _ bool) { handoffs <- handed{h, taker} }) + s.AllowGroup(syscall.Getpgrp()) + + got, err := fetch(t, s.Path()) + require.NoError(t, err) + assert.Equal(t, socketTestToken, strings.TrimSpace(got)) + first := <-handoffs + assert.Equal(t, HandoffDelivered, first.h) + assert.Equal(t, os.Getpid(), first.taker.PID, "the taker is the peer the trust check named") + assert.Equal(t, int32(1), calls.Load(), "the peer is asked once") +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index c26edacb5..d25a205ef 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -502,8 +502,9 @@ Pass the run's shape to the connector itself: `--project ` (repeatable) to hear only some projects, `--shadow` to admit and log without dispatching or posting anything, `--hold` to run with the durable hold set. -Linux only for now: on any other system the connector refuses to start, and -says so. If a person on a Mac asks, say that. +Linux and macOS only: on any other system the connector refuses to start, +and says so. The background service (`connect service`) is Linux only, since +it writes a systemd unit; on a Mac, start the connector yourself. Quote the profile, as everywhere else in this skill. diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 2b821002d..469c3c46a 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1477,9 +1477,9 @@ basecamp connect setup -P agent --worker codex # Run workers with Codex inst answer. Stdout is a wire of one JSON object per line (events seen, verdicts, dispatches — ids and states, never content) and the logs are on stderr, so read the lines rather than the log. SIGINT and SIGTERM cancel whatever workers are -running, settle them, and exit 130 and 143. It runs on Linux only: the task -token's hand-over onto a descriptor the next program inherits is sealed only -there. It refuses a second connector for the same agent, and takes `--project` +running, settle them, and exit 130 and 143. It runs on Linux and macOS: the +task token's hand-over onto a descriptor the next program inherits is sealed +only there. It refuses a second connector for the same agent, and takes `--project` (repeatable) to hear and dispatch only those projects. Run it under a supervisor rather than from a session you will close. From 2c13d3f5509efa5157bfc148a1747ebc2363b100 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 30 Sep 2026 21:33:17 -0400 Subject: [PATCH 4/6] Address the second review on macOS support - connect doctor's Platform hint and comment name Linux and macOS. - The /dev/fd fallback runs only when /dev/fd can't be opened; a descriptor the walk fails to seal is that error. Its messages tell a zero limit from one too large to scan. - macOS tests for a zombie-only group (no such group) beside one with a live member (still signaled), and for a real socket peer that closed (gone, with its user). - The connect skill no longer mentions the hidden service. --- internal/cli/inherited_fds_darwin.go | 25 ++++++---- internal/cli/inherited_fds_darwin_test.go | 4 +- internal/commands/connect_doctor.go | 8 ++-- .../connector/driver/zombie_darwin_test.go | 46 +++++++++++++++++++ internal/connector/tokensocket_darwin_test.go | 46 +++++++++++++++++++ skills/basecamp-connect/SKILL.md | 3 +- 6 files changed, 116 insertions(+), 16 deletions(-) create mode 100644 internal/connector/driver/zombie_darwin_test.go create mode 100644 internal/connector/tokensocket_darwin_test.go diff --git a/internal/cli/inherited_fds_darwin.go b/internal/cli/inherited_fds_darwin.go index 82e1f3dac..29012e8e8 100644 --- a/internal/cli/inherited_fds_darwin.go +++ b/internal/cli/inherited_fds_darwin.go @@ -3,7 +3,9 @@ package cli import ( + "errors" "fmt" + "os" "golang.org/x/sys/unix" @@ -17,12 +19,16 @@ import ( // marked as the Linux fallback marks /proc/self/fd's. It fails closed in the // same way, for the same reason. func sealInheritedDescriptors() error { - if err := sealListedDescriptors(devFD); err == nil { - return nil + // /dev/fd is a filesystem macOS mounts by default. Where it lists, its + // walk decides, and a descriptor it fails to seal is that error, not a + // reason to try another way. + if dir, err := os.Open(devFD); err == nil { + _ = dir.Close() + return sealListedDescriptors(devFD) } - // /dev/fd is a filesystem macOS mounts by default, and one that is not - // there must not leave a descriptor unsealed: try every number the - // process may hold instead, or fail closed when that number is not known. + // Where it is not there, it must not leave a descriptor unsealed: try + // every number the process may hold instead, or fail closed when that + // number is not known. limit, err := descriptorLimit() if err != nil { return err @@ -43,10 +49,13 @@ const maxDescriptorScan = 1 << 20 func descriptorLimit() (uint64, error) { var lim unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &lim); err != nil { - return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be read, and neither could the descriptor limit: %w", err) + return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be opened, and the descriptor limit could not be read: %w", err) } - if lim.Cur == 0 || lim.Cur > maxDescriptorScan { - return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be read, and the descriptor limit (%d) is too large to check one by one", lim.Cur) + switch { + case lim.Cur == 0: + return 0, errors.New("could not seal the descriptors this process inherited: /dev/fd could not be opened, and the descriptor limit reads as zero") + case lim.Cur > maxDescriptorScan: + return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be opened, and the descriptor limit (%d) is too large to check one by one", lim.Cur) } return lim.Cur, nil } diff --git a/internal/cli/inherited_fds_darwin_test.go b/internal/cli/inherited_fds_darwin_test.go index 98a7feb19..e6ffadf54 100644 --- a/internal/cli/inherited_fds_darwin_test.go +++ b/internal/cli/inherited_fds_darwin_test.go @@ -5,6 +5,7 @@ package cli import ( "os" "os/exec" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -49,8 +50,9 @@ func TestSealedDescriptorsDoNotReachChildrenOnMacOS(t *testing.T) { require.NoError(t, sealInheritedDescriptors()) assert.True(t, cloexec(t, inheritedFDDarwin)) + fd := strconv.Itoa(inheritedFDDarwin) out, err := exec.CommandContext(t.Context(), "/bin/sh", "-c", - "if (: <&200) 2>/dev/null; then echo inherited; else echo sealed; fi").Output() + "if (: <&"+fd+") 2>/dev/null; then echo inherited; else echo sealed; fi").Output() //nolint:gosec // G204: the descriptor number is the test's own constant require.NoError(t, err) assert.Equal(t, "sealed\n", string(out), "the child inherited the descriptor") diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 1a32cef8b..b8d4746ca 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -365,14 +365,12 @@ func preflightReason(err error) string { // connectUnsupportedOSCheck is the Platform check on a GOOS the connector // does not run on. It gives the constraint that actually applies // (connectSupportedOS, and #736): the task token reaches a worker's MCP -// server over an inherited descriptor, and Linux alone seals the descriptors -// a process passes on. Reading a process's start time is the half macOS has, -// so naming that here told a Mac reader the connector could run there and -// then refused it anyway. +// server over an inherited descriptor, and only Linux and macOS seal the +// descriptors a process passes on. func connectUnsupportedOSCheck(goos string) setup.Check { return setup.Check{Name: "Platform", Status: setup.StatusFail, Message: fmt.Sprintf("The connector does not run on %s: %s", goos, connectSupportedOSReason), - Hint: "Run the connector on Linux; the rest of the CLI runs here."} + Hint: "Run the connector on Linux or macOS; the rest of the CLI runs here."} } // driverChecks refuses what the run command refuses: doctor never calls a diff --git a/internal/connector/driver/zombie_darwin_test.go b/internal/connector/driver/zombie_darwin_test.go new file mode 100644 index 000000000..c31d6c5c5 --- /dev/null +++ b/internal/connector/driver/zombie_darwin_test.go @@ -0,0 +1,46 @@ +//go:build darwin + +package driver + +import ( + "os/exec" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// macOS refuses any signal to a process group whose only members are +// zombies. That refusal is the absent group it is on Linux: signalGroup +// answers ESRCH, so the group can be confirmed gone. +func TestAZombieOnlyGroupIsNoSuchGroupOnMacOS(t *testing.T) { + leader := exec.CommandContext(t.Context(), "/bin/sleep", "300") + leader.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + require.NoError(t, leader.Start()) + pgid := leader.Process.Pid + t.Cleanup(func() { _ = leader.Wait() }) + require.NoError(t, syscall.Kill(pgid, syscall.SIGKILL)) + // Dead, and not yet waited for: a zombie, alone in its group. + require.Eventually(t, func() bool { _, err := processStartTime(pgid); return err != nil }, 5*time.Second, 10*time.Millisecond) + require.ErrorIs(t, syscall.Kill(-pgid, 0), syscall.EPERM, "macOS refuses a zombie-only group") + + assert.ErrorIs(t, signalGroup(pgid, 0), syscall.ESRCH) + assert.NoError(t, groupGone(pgid)) +} + +// A group whose leader is a zombie but whose child still runs is not gone: +// the signal reaches the child. +func TestAGroupWithALiveMemberIsStillSignaledOnMacOS(t *testing.T) { + leader := exec.CommandContext(t.Context(), "/bin/sh", "-c", "/bin/sleep 300 & exit 0") + leader.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + require.NoError(t, leader.Start()) + pgid := leader.Process.Pid + t.Cleanup(func() { _ = signalGroup(pgid, syscall.SIGKILL); _ = leader.Wait() }) + require.Eventually(t, func() bool { _, err := processStartTime(pgid); return err != nil }, 5*time.Second, 10*time.Millisecond, "the leader has exited") + + err := signalGroup(pgid, 0) + assert.NoError(t, err, "the live child receives it") + assert.Error(t, groupGone(pgid), "a group with a live member is not gone") +} diff --git a/internal/connector/tokensocket_darwin_test.go b/internal/connector/tokensocket_darwin_test.go new file mode 100644 index 000000000..68354cc88 --- /dev/null +++ b/internal/connector/tokensocket_darwin_test.go @@ -0,0 +1,46 @@ +//go:build darwin + +package connector + +import ( + "context" + "net" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// macOS names a peer's process only while it is connected, and still names +// its user. peerCredentials maps that to errPeerGone with the user, which is +// what keeps an early close undelivered rather than refused. +func TestMacOSNamesAClosedPeerAsGoneWithItsUser(t *testing.T) { + path := filepath.Join(tokenDir(t), "s") + ln, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + require.NoError(t, err) + defer ln.Close() + + dialer := net.Dialer{Timeout: 2 * time.Second} + client, err := dialer.DialContext(context.Background(), "unix", path) + require.NoError(t, err) + conn, err := ln.AcceptUnix() + require.NoError(t, err) + defer conn.Close() + + open, err := peerCredentials(conn) + require.NoError(t, err) + assert.Equal(t, os.Getpid(), open.PID) + assert.Equal(t, os.Getuid(), open.UID) + + require.NoError(t, client.Close()) + require.Eventually(t, func() bool { + _, err := peerCredentials(conn) + return err != nil + }, 5*time.Second, 10*time.Millisecond) + gone, err := peerCredentials(conn) + require.ErrorIs(t, err, errPeerGone) + assert.Equal(t, os.Getuid(), gone.UID) +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index d25a205ef..8e8338640 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -503,8 +503,7 @@ hear only some projects, `--shadow` to admit and log without dispatching or posting anything, `--hold` to run with the durable hold set. Linux and macOS only: on any other system the connector refuses to start, -and says so. The background service (`connect service`) is Linux only, since -it writes a systemd unit; on a Mac, start the connector yourself. +and says so. Quote the profile, as everywhere else in this skill. From fe73f8f9f8b7ece8ec29eede68e0613ed65ab1e2 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 30 Sep 2026 21:52:33 -0400 Subject: [PATCH 5/6] Fail closed on macOS when /dev/fd can't be read The descriptor limit doesn't bound which descriptors are open: a parent can open one high and then lower the limit. /dev/fd is always mounted on macOS, so there's no fallback; if it can't be read, sealing fails. --- internal/cli/inherited_fds_darwin.go | 70 ++----------------- internal/cli/inherited_fds_darwin_test.go | 16 ----- internal/connector/tokensocket_darwin_test.go | 3 +- 3 files changed, 9 insertions(+), 80 deletions(-) diff --git a/internal/cli/inherited_fds_darwin.go b/internal/cli/inherited_fds_darwin.go index 29012e8e8..195c53ce0 100644 --- a/internal/cli/inherited_fds_darwin.go +++ b/internal/cli/inherited_fds_darwin.go @@ -2,75 +2,19 @@ package cli -import ( - "errors" - "fmt" - "os" - - "golang.org/x/sys/unix" - - "github.com/basecamp/basecamp-cli/internal/sysfd" -) - // sealInheritedDescriptors keeps every descriptor this process inherited out // of the processes it starts, by marking each one close-on-exec. It is the // Linux one's (inherited_fds_linux.go) for macOS, where there is no // close_range: the process's own list of descriptors is /dev/fd, and each is -// marked as the Linux fallback marks /proc/self/fd's. It fails closed in the -// same way, for the same reason. +// marked as the Linux fallback marks /proc/self/fd's. +// +// /dev/fd is a filesystem macOS always mounts. Where it cannot be read this +// fails closed: no other source says which descriptors are open, and the +// descriptor limit is no bound, since a parent can lower it after opening one +// above it (Copilot on #812). func sealInheritedDescriptors() error { - // /dev/fd is a filesystem macOS mounts by default. Where it lists, its - // walk decides, and a descriptor it fails to seal is that error, not a - // reason to try another way. - if dir, err := os.Open(devFD); err == nil { - _ = dir.Close() - return sealListedDescriptors(devFD) - } - // Where it is not there, it must not leave a descriptor unsealed: try - // every number the process may hold instead, or fail closed when that - // number is not known. - limit, err := descriptorLimit() - if err != nil { - return err - } - return sealEveryDescriptorUpTo(limit) + return sealListedDescriptors(devFD) } // devFD is where macOS lists the descriptors a process holds. const devFD = "/dev/fd" - -// maxDescriptorScan is the most descriptor numbers the fallback will try one -// by one. A limit above it is not scanned partly: that would report every -// descriptor sealed while one above the scan could still be inherited. -const maxDescriptorScan = 1 << 20 - -// descriptorLimit is the highest number a descriptor of this process can have, -// plus one, or an error when it cannot be known or is too large to scan. -func descriptorLimit() (uint64, error) { - var lim unix.Rlimit - if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &lim); err != nil { - return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be opened, and the descriptor limit could not be read: %w", err) - } - switch { - case lim.Cur == 0: - return 0, errors.New("could not seal the descriptors this process inherited: /dev/fd could not be opened, and the descriptor limit reads as zero") - case lim.Cur > maxDescriptorScan: - return 0, fmt.Errorf("could not seal the descriptors this process inherited: /dev/fd could not be opened, and the descriptor limit (%d) is too large to check one by one", lim.Cur) - } - return lim.Cur, nil -} - -// sealEveryDescriptorUpTo marks close-on-exec every open descriptor above -// standard error and below limit, asking each number in turn. -func sealEveryDescriptorUpTo(limit uint64) error { - for fd := uint64(sysfd.FirstNonStandard); fd < limit; fd++ { - d, err := sysfd.Of(uintptr(fd)) - if err != nil { - return fmt.Errorf("could not seal the descriptors this process inherited: %w", err) - } - if err := sealDescriptor(d); err != nil { - return err - } - } - return nil -} diff --git a/internal/cli/inherited_fds_darwin_test.go b/internal/cli/inherited_fds_darwin_test.go index e6ffadf54..5eb728937 100644 --- a/internal/cli/inherited_fds_darwin_test.go +++ b/internal/cli/inherited_fds_darwin_test.go @@ -70,22 +70,6 @@ func TestTheDevFDWalkSealsWhatItNames(t *testing.T) { assert.True(t, cloexec(t, inheritedFDDarwin)) } -// Without /dev/fd the fallback tries every number up to the descriptor limit, -// and seals the same descriptor. -func TestTheFallbackSealsEveryDescriptorUpToTheLimit(t *testing.T) { - inheritDarwin(t, inheritedFDDarwin) - limit, err := descriptorLimit() - require.NoError(t, err) - require.Greater(t, limit, uint64(inheritedFDDarwin)) - - require.NoError(t, sealEveryDescriptorUpTo(limit)) - assert.True(t, cloexec(t, inheritedFDDarwin)) - for _, fd := range []int{0, 1, 2} { - _, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) - assert.NoError(t, err, "standard descriptor %d is left open", fd) - } -} - // A listing that cannot be read is an error, never a quiet success. func TestTheDevFDWalkRefusesAListingItCannotRead(t *testing.T) { require.Error(t, sealListedDescriptors(t.TempDir()+"/missing")) diff --git a/internal/connector/tokensocket_darwin_test.go b/internal/connector/tokensocket_darwin_test.go index 68354cc88..973606ff3 100644 --- a/internal/connector/tokensocket_darwin_test.go +++ b/internal/connector/tokensocket_darwin_test.go @@ -4,6 +4,7 @@ package connector import ( "context" + "errors" "net" "os" "path/filepath" @@ -38,7 +39,7 @@ func TestMacOSNamesAClosedPeerAsGoneWithItsUser(t *testing.T) { require.NoError(t, client.Close()) require.Eventually(t, func() bool { _, err := peerCredentials(conn) - return err != nil + return errors.Is(err, errPeerGone) }, 5*time.Second, 10*time.Millisecond) gone, err := peerCredentials(conn) require.ErrorIs(t, err, errPeerGone) From 352932e09b757212018db9abc393153a3ae0549d Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 30 Sep 2026 22:11:31 -0400 Subject: [PATCH 6/6] Name the token's taker before writing the token Once the worker has read its token and gone, its pid could belong to another process. Looked up before the write, the taker is the process that was checked. --- internal/connector/tokensocket.go | 10 ++++++---- internal/connector/tokensocket_test.go | 8 ++++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/connector/tokensocket.go b/internal/connector/tokensocket.go index be6c16a69..05b2998c9 100644 --- a/internal/connector/tokensocket.go +++ b/internal/connector/tokensocket.go @@ -654,15 +654,17 @@ func (s *TokenSocket) handOne(window time.Duration) (Handoff, driver.Process) { } return HandoffRefused, driver.Process{} } + // The peer named before the write, not asked again, and looked up before + // the write too: a server that reads its token and closes at once is the + // ordinary case, macOS no longer names a peer that has closed, and once + // it has gone its pid could belong to another process (Copilot on #812). + taker := s.takerOf(peer.PID) if _, err := conn.Write([]byte(s.token + "\n")); err != nil { // The peer was the worker's; the write is what failed. On a unix // socket a peer that has gone makes this EPIPE at once. return HandoffUndelivered, driver.Process{} } - // The peer named before the write, not asked again: a server that reads - // its token and closes at once is the ordinary case, and macOS no longer - // names a peer that has closed. - return HandoffDelivered, s.takerOf(peer.PID) + return HandoffDelivered, taker } // allowedGroup is the worker's process group, or 0 before it is named. diff --git a/internal/connector/tokensocket_test.go b/internal/connector/tokensocket_test.go index fcfb42692..34ad4e60b 100644 --- a/internal/connector/tokensocket_test.go +++ b/internal/connector/tokensocket_test.go @@ -302,10 +302,13 @@ func TestTheShortSocketBaseIsChosenSoTheSocketFits(t *testing.T) { // an empty taker for a token that was in fact handed over. func TestAHandoffInFlightIsFinishedBeforeTheTakerIsRead(t *testing.T) { // The identity lookup is where the handoff is slowest; hold it there. - slow := make(chan struct{}) + // It runs before the write (Copilot on #812), so the peer is connected + // and waiting for its token. + reached, slow := make(chan struct{}), make(chan struct{}) s, err := serveTaskTokenWith(tokenDir(t), socketTestToken, 2*time.Second, peerCredentials, processGroupOf, parentProcessOf, func(pid int) (driver.Process, error) { + close(reached) <-slow return driver.LookupProcess(pid) }) @@ -318,7 +321,7 @@ func TestAHandoffInFlightIsFinishedBeforeTheTakerIsRead(t *testing.T) { token, _ := fetch(t, s.Path()) got <- token }() - require.Equal(t, socketTestToken, strings.TrimSpace(<-got), "the token is out before the taker is known") + <-reached _, ok := s.Taker() require.False(t, ok, "the fixture must have the handoff still deciding") @@ -330,6 +333,7 @@ func TestAHandoffInFlightIsFinishedBeforeTheTakerIsRead(t *testing.T) { require.True(t, ok, "and the process that took the token is known by then") assert.Equal(t, os.Getpid(), taker.PID) assert.Equal(t, HandoffDelivered, s.Result()) + assert.Equal(t, socketTestToken, strings.TrimSpace(<-got), "the handoff in flight still delivered") } // Opus r8: after a delivery the socket does not arm again while the process