carlos / platform Public

cgroup: merge SysProcAttr instead of clobbering it, gate the fallback latch on real refusal errnos

Start's clone3 path now adds UseCgroupFD/CgroupFD onto whatever
SysProcAttr build() already configured (setInto) instead of overwriting
it, so a builder-set Setpgid/Pdeathsig survives the clone3 attempt.

The fallback only latches when the clone3 error actually proves the
mechanism is gone (EPERM/ENOSYS/EINVAL, via shouldLatch) — a transient,
spawn-specific errno like ENODEV no longer permanently downgrades the
process off a fluke.

Also: drop the ghost per-host cgroup dir when a spawn ultimately fails
(startAndMove), so a failed wake doesn't leave the sampler reporting
permanent zeros; join both errors when clone3 and its retry both fail;
guard the PID move against a broken startCmd seam that reports success
with no cmd.Process; document that the PID move migrates only the
child, not grandchildren it has already forked; and match the comment
on cgroup_other.go's intoCgroupSupported to its linux twin.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 764ed2b1a30e46f821701758df8178d5d395e278 parent 8cbfd0e

5 files changed, +214 −13

internal/cgroup/cgroup.go +33 −8
internal/cgroup/cgroup_linux.go +27 −3
internal/cgroup/cgroup_linux_test.go +104 −0
internal/cgroup/cgroup_other.go +7 −2
internal/cgroup/cgroup_test.go +43 −0

diff --git a/internal/cgroup/cgroup.go b/internal/cgroup/cgroup.go

index e0b5fccb5..dfa02cce1 100644

--- a/internal/cgroup/cgroup.go

+++ b/internal/cgroup/cgroup.go

@@ -13,6 +13,7 @@

package cgroup

import (

+ "errors"

"fmt"

"os"

"os/exec"

@@ -183,10 +184,15 @@ func (m *Manager) hostDir(host string) string { return filepath.Join(m.root, Exe

// — the child never spends a cycle in the wrong cgroup. If that start

// fails, the cgroup fd may or may not be why (a seccomp filter older than

// clone3 answers EPERM; so does a missing binary): retry WITHOUT the fd,

-// and only if the retry succeeds — proving the cgroup was the problem —

-// latch the fallback and move the child by PID. The window between start

-// and move charges the child's first moments to the agent: mis-accounting

-// in the safe direction.

+// and only if the retry succeeds — proving the cgroup was the problem — and

+// only if the failure actually means the mechanism is gone (shouldLatch),

+// latch the fallback and move the child by PID. A transient, spawn-specific

+// error (e.g. the cgroup dir vanishing between os.Open and start) falls

+// back for this spawn only; the next Start still tries clone3. The window

+// between start and move charges the child's first moments to the agent:

+// mis-accounting in the safe direction. Note the PID write migrates only

+// the child process itself — any grandchildren it has already forked by

+// then stay charged to the agent's cgroup, not the child's.

func (m *Manager) Start(host string, build func() *exec.Cmd) (*exec.Cmd, error) {

if m == nil {

cmd := build()

@@ -205,7 +211,7 @@ func (m *Manager) Start(host string, build func() *exec.Cmd) (*exec.Cmd, error)

return m.startAndMove(host, build)

}

cmd := build()

- cmd.SysProcAttr = sysProcAttrInto(int(f.Fd()))

+ setInto(cmd, int(f.Fd()))

err = startCmd(cmd)

f.Close()

if err == nil {

@@ -214,10 +220,17 @@ func (m *Manager) Start(host string, build func() *exec.Cmd) (*exec.Cmd, error)

firstErr := err

retried, retryErr := m.startAndMove(host, build)

if retryErr != nil {

- return nil, retryErr // failed both ways: not the cgroup's fault

+ // startAndMove already dropped the ghost host dir on its own

+ // failure; join both errors so the operator can see a clone3

+ // attempt happened at all, not just that the retry failed.

+ return nil, errors.Join(firstErr, retryErr)

+ }

+ if shouldLatch(firstErr) {

+ m.fallback.Store(true)

+ m.logf("cgroup: clone3-into-cgroup refused (%v) — latching PID-move fallback for the life of this process", firstErr)

+ } else {

+ m.logf("cgroup: clone3-into-cgroup failed for this spawn (%v) — not latching", firstErr)

}

- m.fallback.Store(true)

- m.logf("cgroup: clone3-into-cgroup refused (%v) — latching PID-move fallback for the life of this process", firstErr)

return retried, nil

}

return m.startAndMove(host, build)

@@ -229,8 +242,20 @@ func (m *Manager) Start(host string, build func() *exec.Cmd) (*exec.Cmd, error)

func (m *Manager) startAndMove(host string, build func() *exec.Cmd) (*exec.Cmd, error) {

cmd := build()

if err := startCmd(cmd); err != nil {

+ // The spawn never happened: drop the now-ownerless host dir so a

+ // failed wake doesn't leave the sampler reporting permanent zeros

+ // for a host with nothing running. rmdir-safe — os.Remove refuses a

+ // non-empty dir, so this can never take a live sibling's dir.

+ _ = os.Remove(m.hostDir(host))

return nil, err

}

+ if cmd.Process == nil {

+ // A broken startCmd seam reporting success with no process: this

+ // package must never panic the agent over it. Log and skip the

+ // move — the child, if any, simply stays unaccounted.

+ m.logf("cgroup: %s: start reported success with no process — skipping pid move", host)

+ return cmd, nil

+ }

procs := filepath.Join(m.hostDir(host), "cgroup.procs")

if err := os.WriteFile(procs, []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil {

m.logf("cgroup: %s: move pid %d: %v — child stays unaccounted", host, cmd.Process.Pid, err)

diff --git a/internal/cgroup/cgroup_linux.go b/internal/cgroup/cgroup_linux.go

index 80615ed85..5b362aab9 100644

--- a/internal/cgroup/cgroup_linux.go

+++ b/internal/cgroup/cgroup_linux.go

@@ -2,13 +2,37 @@

package cgroup

-import "syscall"

+import (

+ "errors"

+ "os/exec"

+ "syscall"

// intoCgroupSupported gates the clone3 CLONE_INTO_CGROUP attempt to

// platforms whose SysProcAttr has the fields — everywhere else Start goes

// straight to the PID-move fallback.

const intoCgroupSupported = true

-func sysProcAttrInto(fd int) *syscall.SysProcAttr {

- return &syscall.SysProcAttr{UseCgroupFD: true, CgroupFD: fd}

+// setInto arms cmd for clone3-into-cgroup without clobbering any

+// SysProcAttr build() already configured (Setpgid, Pdeathsig, ...) — build()

+// owns all Cmd configuration; Start only adds to it, never replaces it.

+func setInto(cmd *exec.Cmd, fd int) {

+ if cmd.SysProcAttr == nil {

+ cmd.SysProcAttr = &syscall.SysProcAttr{}

+ }

+ cmd.SysProcAttr.UseCgroupFD = true

+ cmd.SysProcAttr.CgroupFD = fd

+// shouldLatch reports whether err proves clone3-into-cgroup is unavailable

+// for the life of this process — EPERM (a seccomp filter predating clone3,

+// or one that forbids it outright) and ENOSYS (a kernel predating clone3)

+// mean the mechanism itself is gone; EINVAL means the kernel understood

+// clone3 but rejected CLONE_INTO_CGROUP. Anything else (e.g. ENODEV from the

+// cgroup dir disappearing between os.Open and start, raced by a concurrent

+// CleanupHost) is spawn-specific and says nothing about the mechanism —

+// latching on it would permanently downgrade the process over a fluke and

+// log a lie ("refused").

+func shouldLatch(err error) bool {

+ return errors.Is(err, syscall.EPERM) || errors.Is(err, syscall.ENOSYS) || errors.Is(err, syscall.EINVAL)

}

diff --git a/internal/cgroup/cgroup_linux_test.go b/internal/cgroup/cgroup_linux_test.go

index 69dd6091d..05d902b58 100644

--- a/internal/cgroup/cgroup_linux_test.go

+++ b/internal/cgroup/cgroup_linux_test.go

@@ -3,6 +3,7 @@

package cgroup

import (

+ "errors"

"os"

"os/exec"

"path/filepath"

@@ -79,3 +80,106 @@ func TestStartFallsBackAndLatches(t *testing.T) {

t.Fatal("latched spawn still set SysProcAttr")

}

+// TestStartMergesIntoBuilderSysProcAttr: build() owns all Cmd configuration

+// — Start must ADD UseCgroupFD/CgroupFD to whatever SysProcAttr build()

+// already set, never replace it. A builder setting Setpgid or Pdeathsig

+// must keep that field on the clone3 attempt.

+func TestStartMergesIntoBuilderSysProcAttr(t *testing.T) {

+ root := newDelegatedRoot(t)

+ m, err := SetupAt(root, t.Logf)

+ if err != nil {

+ t.Fatal(err)

+ }

+ calls := fakeStart(t, func(*exec.Cmd) error { return nil })

+ build := func() *exec.Cmd {

+ cmd := exec.Command("/bin/true")

+ cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}

+ return cmd

+ }

+ if _, err := m.Start("h.example.com", build); err != nil {

+ t.Fatal(err)

+ }

+ attr := (*calls)[0].SysProcAttr

+ if attr == nil || !attr.Setpgid {

+ t.Fatalf("SysProcAttr = %+v, want Setpgid preserved from build()", attr)

+ }

+ if !attr.UseCgroupFD {

+ t.Fatalf("SysProcAttr = %+v, want UseCgroupFD added by Start", attr)

+ }

+// TestStartTransientClone3FailureDoesNotLatch: ENODEV (e.g. the cgroup dir

+// disappearing between os.Open and start, raced by a concurrent

+// CleanupHost) says nothing about whether clone3-into-cgroup works at all —

+// the spawn falls back for THIS call only, and the NEXT Start still

+// attempts clone3.

+func TestStartTransientClone3FailureDoesNotLatch(t *testing.T) {

+ root := newDelegatedRoot(t)

+ m, err := SetupAt(root, t.Logf)

+ if err != nil {

+ t.Fatal(err)

+ }

+ calls := fakeStart(t, func(cmd *exec.Cmd) error {

+ if cmd.SysProcAttr != nil {

+ return syscall.ENODEV

+ }

+ return nil

+ })

+ if _, err := m.Start("h.example.com", buildTrue); err != nil {

+ t.Fatal(err)

+ }

+ if len(*calls) != 2 {

+ t.Fatalf("first spawn: %d start calls, want 2 (clone3 attempt + plain retry)", len(*calls))

+ }

+ if m.fallback.Load() {

+ t.Fatal("a transient clone3 failure latched the fallback")

+ }

+ if _, err := m.Start("h2.example.com", buildTrue); err != nil {

+ t.Fatal(err)

+ }

+ // The fake keeps refusing clone3 with ENODEV for h2 too, so the second

+ // spawn again pays for a clone3 attempt (fails) plus a plain retry

+ // (succeeds): two more calls. What matters is call index 2 — the second

+ // spawn's FIRST attempt — still carries SysProcAttr: proof clone3 was

+ // retried rather than skipped by a latched fallback.

+ if len(*calls) != 4 {

+ t.Fatalf("second spawn: %d total start calls, want 4 (clone3 attempted again + its own retry)", len(*calls))

+ }

+ if (*calls)[2].SysProcAttr == nil || !(*calls)[2].SysProcAttr.UseCgroupFD {

+ t.Fatal("second spawn did not attempt clone3 again — fallback must not have latched")

+ }

+// TestStartJoinsBothErrorsOnTotalFailure: when the clone3 attempt AND the

+// PID-move retry both fail, the operator needs to see that a clone3 attempt

+// happened at all — the returned error must reference both, and the ghost

+// host dir must not survive.

+func TestStartJoinsBothErrorsOnTotalFailure(t *testing.T) {

+ root := newDelegatedRoot(t)

+ m, err := SetupAt(root, t.Logf)

+ if err != nil {

+ t.Fatal(err)

+ }

+ errClone3 := errors.New("clone3 boom")

+ errRetry := errors.New("retry boom")

+ first := true

+ fakeStart(t, func(*exec.Cmd) error {

+ if first {

+ first = false

+ return errClone3

+ }

+ return errRetry

+ })

+ _, err = m.Start("h.example.com", buildTrue)

+ if err == nil {

+ t.Fatal("Start with two failing spawns succeeded, want error")

+ }

+ if !errors.Is(err, errClone3) || !errors.Is(err, errRetry) {

+ t.Fatalf("Start error = %v, want it to wrap both %v and %v", err, errClone3, errRetry)

+ }

+ if _, statErr := os.Stat(filepath.Join(root, ExecName("h.example.com"))); !os.IsNotExist(statErr) {

+ t.Errorf("ghost host dir survived total spawn failure: %v", statErr)

+ }

diff --git a/internal/cgroup/cgroup_other.go b/internal/cgroup/cgroup_other.go

index c865ee827..04e8dbcbb 100644

--- a/internal/cgroup/cgroup_other.go

+++ b/internal/cgroup/cgroup_other.go

@@ -2,8 +2,13 @@

package cgroup

-import "syscall"

+import "os/exec"

+// intoCgroupSupported gates the clone3 CLONE_INTO_CGROUP attempt to

+// platforms whose SysProcAttr has the fields — everywhere else Start goes

+// straight to the PID-move fallback.

const intoCgroupSupported = false

-func sysProcAttrInto(fd int) *syscall.SysProcAttr { return nil }

+func setInto(cmd *exec.Cmd, fd int) {}

+func shouldLatch(error) bool { return false }

diff --git a/internal/cgroup/cgroup_test.go b/internal/cgroup/cgroup_test.go

index 10f447212..ee4ffba26 100644

--- a/internal/cgroup/cgroup_test.go

+++ b/internal/cgroup/cgroup_test.go

@@ -224,3 +224,46 @@ func TestCleanupHost(t *testing.T) {

var nilM *Manager

nilM.CleanupHost("h.example.com") // must not panic

}

+// TestStartAndMoveRemovesGhostDirOnSpawnFailure: when the spawn itself

+// never happens, the host cgroup dir must not survive it — a ghost dir

+// would make the sampler report permanent zeros for a host with nothing

+// running.

+func TestStartAndMoveRemovesGhostDirOnSpawnFailure(t *testing.T) {

+ root := newDelegatedRoot(t)

+ m, err := SetupAt(root, t.Logf)

+ if err != nil {

+ t.Fatal(err)

+ }

+ dir := m.hostDir("h.example.com")

+ if err := os.MkdirAll(dir, 0o755); err != nil {

+ t.Fatal(err)

+ }

+ fakeStart(t, func(*exec.Cmd) error { return errors.New("no such binary") })

+ if _, err := m.startAndMove("h.example.com", buildTrue); err == nil {

+ t.Fatal("startAndMove with a failing spawn succeeded, want error")

+ }

+ if _, err := os.Stat(dir); !os.IsNotExist(err) {

+ t.Errorf("ghost host dir survived a failed spawn: %v", err)

+ }

+// TestStartAndMoveToleratesBrokenSeam: a startCmd seam that reports success

+// without ever setting cmd.Process must not panic while writing the PID —

+// this package must never panic the agent over a broken seam.

+func TestStartAndMoveToleratesBrokenSeam(t *testing.T) {

+ root := newDelegatedRoot(t)

+ m, err := SetupAt(root, t.Logf)

+ if err != nil {

+ t.Fatal(err)

+ }

+ if err := os.MkdirAll(m.hostDir("h.example.com"), 0o755); err != nil {

+ t.Fatal(err)

+ }

+ orig := startCmd

+ startCmd = func(cmd *exec.Cmd) error { return nil } // never sets cmd.Process

+ t.Cleanup(func() { startCmd = orig })

+ if _, err := m.startAndMove("h.example.com", buildTrue); err != nil {

+ t.Fatal(err)

+ }