amadan

carlos / platform Public

zdd plan 2 final fix: kernel tests always end their children

Every spawn in the two kernel tests now registers its cleanup straight
after the start and before anything that can fail. The cleanup tries
the cgroup's own kill on a fresh 3s budget, then kills the process
directly, closes the pipes the test holds and reaps on a bounded 5s
wait, so a failed OpenCleanupChild or KillAndEmpty can no longer leave
the test blocked on an echo child waiting on stdin. The strict test
gets the same cleanup (it had none after starting children), and its
two KillAndEmpty calls each get their own budget. The hog's StdinPipe
error is checked, and cgroupOf returns its error so the caller fails
with the hog's cleanup still to run.

Final review finding D2 (triage T4 cleanup items).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 7f889d9ab7840aded237751c42afa747ea702ebf parent 792eb01
1 file changed, +151 −69
  • internal/cgroup/gen_kernel_linux_test.go +151 −69
diff --git a/internal/cgroup/gen_kernel_linux_test.go b/internal/cgroup/gen_kernel_linux_test.go
index 217a07d52..d6fc09c80 100644
--- a/internal/cgroup/gen_kernel_linux_test.go
+++ b/internal/cgroup/gen_kernel_linux_test.go
@@ -13,6 +13,7 @@ import (
"path/filepath"
"strconv"
"strings"
+ "sync"
"syscall"
"testing"
"time"
@@ -174,17 +175,75 @@ func oomKills(t *testing.T, dir string) int {
return 0
}
-func cgroupOf(t *testing.T, pid int) string {
- t.Helper()
+// cgroupOf returns pid's unified cgroup. It reports rather than fails, so
+// a caller holding a process that must not run on can kill it first.
+func cgroupOf(pid int) (string, error) {
raw, err := os.ReadFile(fmt.Sprintf("/proc/%d/cgroup", pid))
if err != nil {
- t.Fatal(err)
+ return "", err
}
rel, ok := ParseSelfCgroup(string(raw))
if !ok {
- t.Fatalf("pid %d has no unified cgroup", pid)
+ return "", fmt.Errorf("pid %d has no unified cgroup", pid)
}
- return rel
+ return rel, nil
+}
+
+// kernelKid is a synthetic child the test started. Its one waiter runs at
+// most once, so the test body and the cleanup can both wait on it.
+type kernelKid struct {
+ cmd *exec.Cmd
+ once sync.Once
+ done chan struct{}
+ err error
+}
+
+func (k *kernelKid) reaped() <-chan struct{} {
+ k.once.Do(func() {
+ go func() { k.err = k.cmd.Wait(); close(k.done) }()
+ })
+ return k.done
+}
+
+func (k *kernelKid) wait() error { <-k.reaped(); return k.err }
+
+// reapOnCleanup is registered right after a spawn, before anything that
+// can fail: whatever the test's outcome, the child ends. It tries the
+// cgroup's own kill first, on a fresh budget, then kills the process
+// directly, closes the pipes the test holds, and reaps on a bounded wait.
+// A child the spawn never started is left alone.
+func reapOnCleanup(t *testing.T, m *Manager, host, child string, cmd *exec.Cmd, pipes ...io.Closer) *kernelKid {
+ t.Helper()
+ k := &kernelKid{cmd: cmd, done: make(chan struct{})}
+ t.Cleanup(func() {
+ closePipes := func() {
+ for _, p := range pipes {
+ if p != nil {
+ _ = p.Close()
+ }
+ }
+ }
+ if cmd == nil || cmd.Process == nil {
+ closePipes()
+ return
+ }
+ if c, err := m.OpenCleanupChild(host, child); err == nil {
+ ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
+ if err := c.KillAndEmpty(ctx); err != nil {
+ t.Logf("cleanup: %s/%s: cgroup kill: %v (killing pid %d directly)", host, child, err, cmd.Process.Pid)
+ }
+ cancel()
+ _ = c.Close()
+ }
+ _ = cmd.Process.Kill()
+ closePipes()
+ select {
+ case <-k.reaped():
+ case <-time.After(5 * time.Second):
+ t.Errorf("cleanup: pid %d was not reaped within 5s", cmd.Process.Pid)
+ }
+ })
+ return k
}
// Spec §12, Containment: "a memory-hog successor that overruns its child
@@ -196,33 +255,34 @@ func TestGenOOMKernel(t *testing.T) {
const host = "oom.example"
small := Limit{MemoryMax: 64 << 20}
parent := m.hostDir(host)
+ // Registered first, so it runs last: after every child is reaped.
+ t.Cleanup(func() {
+ m.RemoveChild(host, GenChild(1))
+ m.RemoveChild(host, GenChild(2))
+ m.CleanupHost(host)
+ })
var echo *echoChild
- incumbent, err := m.StartChild(host, GenChild(1), small, small, func() *exec.Cmd {
+ incumbentCmd, err := m.StartChild(host, GenChild(1), small, small, func() *exec.Cmd {
cmd := genKernelCmd("echo")
echo = echoPipes(t, cmd)
return cmd
})
+ var echoIn io.Closer
+ if echo != nil {
+ echoIn = echo.in
+ }
+ incumbent := reapOnCleanup(t, m, host, GenChild(1), incumbentCmd, echoIn)
if err != nil {
t.Fatal(err)
}
- t.Cleanup(func() {
- if c, err := m.OpenCleanupChild(host, GenChild(1)); err == nil {
- ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
- _ = c.KillAndEmpty(ctx)
- cancel()
- _ = c.Close()
- }
- _ = incumbent.Wait()
- m.RemoveChild(host, GenChild(1))
- m.CleanupHost(host)
- })
echo.answer(t, "", "ready")
expectFile(t, filepath.Join(parent, "memory.max"), "67108864")
expectFile(t, filepath.Join(parent, "memory.oom.group"), "0")
expectFile(t, filepath.Join(parent, GenChild(1), "memory.oom.group"), "1")
- if got, want := cgroupOf(t, incumbent.Process.Pid), filepath.Join(strings.TrimPrefix(m.Root(), "/sys/fs/cgroup/"), ExecName(host), GenChild(1)); got != want {
- t.Fatalf("incumbent runs in %s, want %s", got, want)
+ got1, err := cgroupOf(incumbent.cmd.Process.Pid)
+ if want := filepath.Join(strings.TrimPrefix(m.Root(), "/sys/fs/cgroup/"), ExecName(host), GenChild(1)); err != nil || got1 != want {
+ t.Fatalf("incumbent runs in %s (%v), want %s", got1, err, want)
}
// The raise happens before the successor spawns.
@@ -232,39 +292,37 @@ func TestGenOOMKernel(t *testing.T) {
expectFile(t, filepath.Join(parent, "memory.max"), "134217728")
var hogIn io.WriteCloser
- hog, err := m.StartChild(host, GenChild(2), small, small, func() *exec.Cmd {
+ var hogPipeErr error
+ hogCmd, err := m.StartChild(host, GenChild(2), small, small, func() *exec.Cmd {
cmd := genKernelCmd("hog")
- hogIn, _ = cmd.StdinPipe()
+ hogIn, hogPipeErr = cmd.StdinPipe()
return cmd
})
+ hog := reapOnCleanup(t, m, host, GenChild(2), hogCmd, hogIn)
if err != nil {
t.Fatal(err)
}
- // StartChild is best-effort: a refused limit write starts the child
- // uncapped. The hog waits on stdin, so prove it is confined first.
- confined := func(ok bool, format string, args ...any) {
- t.Helper()
- if !ok {
- _ = hog.Process.Kill()
- _ = hog.Wait()
- t.Fatalf(format, args...)
- }
+ if hogPipeErr != nil {
+ t.Fatalf("hog stdin: %v", hogPipeErr)
}
+ // StartChild is best-effort: a refused limit write starts the child
+ // uncapped. The hog waits on stdin, so prove it is confined first. Any
+ // failure from here kills it: the cleanup registered above runs.
g2 := filepath.Join(parent, GenChild(2))
wantG2 := filepath.Join(strings.TrimPrefix(m.Root(), "/sys/fs/cgroup/"), ExecName(host), GenChild(2))
- got, err := os.ReadFile(filepath.Join(g2, "memory.max"))
- confined(err == nil && strings.TrimSpace(string(got)) == "67108864", "g2 memory.max = %q, %v; want 67108864 before the hog runs", got, err)
- gotCg := cgroupOf(t, hog.Process.Pid)
- confined(gotCg == wantG2, "hog runs in %s, want %s", gotCg, wantG2)
+ if got, err := os.ReadFile(filepath.Join(g2, "memory.max")); err != nil || strings.TrimSpace(string(got)) != "67108864" {
+ t.Fatalf("g2 memory.max = %q, %v; want 67108864 before the hog runs", got, err)
+ }
+ if gotCg, err := cgroupOf(hog.cmd.Process.Pid); err != nil || gotCg != wantG2 {
+ t.Fatalf("hog runs in %s (%v), want %s", gotCg, err, wantG2)
+ }
if _, err := fmt.Fprintln(hogIn, "go"); err != nil {
- confined(false, "hog go-ahead: %v", err)
+ t.Fatalf("hog go-ahead: %v", err)
}
- waited := make(chan error, 1)
- go func() { waited <- hog.Wait() }()
select {
- case err = <-waited:
+ case <-hog.reaped():
+ err = hog.err
case <-time.After(60 * time.Second):
- _ = hog.Process.Kill()
t.Fatal("the hog was not OOM-killed within 60s")
}
var exitErr *exec.ExitError
@@ -305,56 +363,77 @@ func TestGenStrictKernel(t *testing.T) {
if err != nil {
t.Fatal(err)
}
+ // Registered first, so it runs last: after every child is reaped. The
+ // handles close before the parent's, then the directories go.
+ var handles []*Prepared
+ t.Cleanup(func() {
+ for i := len(handles) - 1; i >= 0; i-- {
+ _ = handles[i].Close()
+ }
+ _ = parent.Close()
+ for n := uint64(1); n <= 3; n++ {
+ m.RemoveChild(host, GenChild(n))
+ }
+ m.CleanupHost(host)
+ })
expectFile(t, filepath.Join(dir, "memory.oom.group"), "0")
expectFile(t, filepath.Join(dir, "memory.max"), "536870912")
if b, err := os.ReadFile(filepath.Join(dir, "cgroup.subtree_control")); err != nil || !strings.Contains(string(b), "memory") {
t.Fatalf("parent subtree_control = %q, %v", b, err)
}
- if err := parent.Start(genKernelCmd("echo")); !errors.Is(err, ErrStrictState) {
+ refused := genKernelCmd("echo")
+ err = parent.Start(refused)
+ reapOnCleanup(t, m, host, "", refused)
+ if !errors.Is(err, ErrStrictState) {
t.Fatalf("a generation parent accepted a process: %v", err)
}
- start := func(g *Prepared) (*exec.Cmd, *echoChild) {
+ prepareGen := func(n uint64) *Prepared {
+ t.Helper()
+ g, err := parent.PrepareGen(n, TierDefault)
+ if err != nil {
+ t.Fatal(err)
+ }
+ handles = append(handles, g)
+ return g
+ }
+ start := func(g *Prepared, n uint64) (*kernelKid, *echoChild) {
t.Helper()
cmd := genKernelCmd("echo")
e := echoPipes(t, cmd)
- if err := g.Start(cmd); err != nil {
+ err := g.Start(cmd)
+ k := reapOnCleanup(t, m, host, GenChild(n), cmd, e.in)
+ if err != nil {
t.Fatal(err)
}
e.answer(t, "", "ready")
- return cmd, e
+ return k, e
}
- g1, err := parent.PrepareGen(1, TierDefault)
- if err != nil {
- t.Fatal(err)
- }
- cmd1, echo1 := start(g1)
+ g1 := prepareGen(1)
+ kid1, echo1 := start(g1, 1)
expectFile(t, filepath.Join(dir, GenChild(1), "memory.oom.group"), "1")
if err := parent.SetParentLimit(OverlapLimit(TierDefault, TierDefault)); err != nil {
t.Fatal(err)
}
expectFile(t, filepath.Join(dir, "memory.max"), "1073741824")
- g2, err := parent.PrepareGen(2, TierDefault)
- if err != nil {
- t.Fatal(err)
- }
- cmd2, echo2 := start(g2)
+ g2 := prepareGen(2)
+ kid2, echo2 := start(g2, 2)
base := filepath.Join(strings.TrimPrefix(m.Root(), "/sys/fs/cgroup/"), ExecName(host))
- if got := cgroupOf(t, cmd1.Process.Pid); got != filepath.Join(base, GenChild(1)) {
- t.Fatalf("g1 process in %s", got)
+ if got, err := cgroupOf(kid1.cmd.Process.Pid); err != nil || got != filepath.Join(base, GenChild(1)) {
+ t.Fatalf("g1 process in %s (%v)", got, err)
}
- if got := cgroupOf(t, cmd2.Process.Pid); got != filepath.Join(base, GenChild(2)) {
- t.Fatalf("g2 process in %s", got)
+ if got, err := cgroupOf(kid2.cmd.Process.Pid); err != nil || got != filepath.Join(base, GenChild(2)) {
+ t.Fatalf("g2 process in %s (%v)", got, err)
}
echo1.ping(t)
// Retire g1 alone.
- ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second)
- defer cancel()
- if err := g1.KillAndEmpty(ctx); err != nil {
+ ctx1, cancel1 := context.WithTimeout(t.Context(), 3*time.Second)
+ defer cancel1()
+ if err := g1.KillAndEmpty(ctx1); err != nil {
t.Fatal(err)
}
- _ = cmd1.Wait()
+ _ = kid1.wait()
echo2.ping(t)
if err := g1.CloseRemove(); err != nil {
t.Fatal(err)
@@ -371,22 +450,25 @@ func TestGenStrictKernel(t *testing.T) {
if err := os.WriteFile(filepath.Join(dir, "memory.max"), []byte("268435456"), 0o644); err != nil {
t.Fatal(err)
}
- g3, err := parent.PrepareGen(3, TierDefault)
- if err != nil {
- t.Fatal(err)
- }
- if err := g3.Start(genKernelCmd("echo")); !errors.Is(err, ErrStrictRefused) {
+ g3 := prepareGen(3)
+ refused3 := genKernelCmd("echo")
+ err = g3.Start(refused3)
+ reapOnCleanup(t, m, host, GenChild(3), refused3)
+ if !errors.Is(err, ErrStrictRefused) {
t.Fatalf("spawn under a foreign parent limit: %v", err)
}
if err := g3.CloseRemove(); err != nil {
t.Fatal(err)
}
- // A terminal stop: the parent's kill takes every generation.
- if err := parent.KillAndEmpty(ctx); err != nil {
+ // A terminal stop: the parent's kill takes every generation. Its own
+ // budget, not the one g1's retirement spent.
+ ctx2, cancel2 := context.WithTimeout(t.Context(), 3*time.Second)
+ defer cancel2()
+ if err := parent.KillAndEmpty(ctx2); err != nil {
t.Fatal(err)
}
- _ = cmd2.Wait()
+ _ = kid2.wait()
if err := g2.CloseRemove(); err != nil {
t.Fatal(err)
}