amadan
cgroup: a start whose context ended during placement answers ctx.Err() (#272)
Whole-branch review round 1, finding 3. When exec.Cmd's Cancel killed a
shim mid-placement, the generation start returned "the start's context
ended before exec" around the shim's protocol error: no context sentinel
(errors.Is(err, context.Canceled) was false), and unreviewed text that
could reach a member through the seed's wake failure.
startIn now answers what exec.Cmd.Start answers for an ended context: the
context's own error, bare (Canceled or DeadlineExceeded as it ended), for
flat and generation starts alike, with no fallback (its Start would answer
the same at once). The placement's detail goes to one operator log line.
exec.Cmd keeps its context unexported, so cancelWatch keeps a copy of the
command taken before the start and, once Cancel has run, asks the copy,
stripped of files, attributes and Cancel and pointed at /dev/null, to
start: Start answers ctx.Err() before it opens or forks anything.
No new member text: the seed's wake failure reads as a direct start's
ended context always read ("wake failed: seeding <host>: context
canceled"). Tests: the classification at the source (both ways in, both
context errors), the probe running nothing, the kernel test asking for
context.Canceled itself, and in the activator the classifier and the
member rendering of the error as the seed wraps it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 files changed,
+210
−37
internal/activator/logsink_test.go+38 −0internal/cgroup/cgroup.go+67 −18internal/cgroup/gen_kernel_linux_test.go+5 −0internal/cgroup/gen_test.go+100 −19
diff --git a/internal/activator/logsink_test.go b/internal/activator/logsink_test.go| index 767035f86..e0ef72ae2 100644 |
| --- a/internal/activator/logsink_test.go |
| +++ b/internal/activator/logsink_test.go |
| @@ -240,3 +240,41 @@ func TestMemberSafeWakeReasonHidesALimitNotAppliedRefusal(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// Whole-branch review round 1 of zdd readiness 3, finding 3. A start whose |
| +// exec.CommandContext context ends while the cgroup manager's shim is being |
| +// placed answers the context's own error, bare, as exec.Cmd.Start answers |
| +// an ended context (cgroup's startIn; its unit and kernel tests pin that |
| +// shape). The only spawn built with a context is the seed, so this is that |
| +// error as the seed wraps it and the wake event renders it: the classifier |
| +// reads a cancellation as the caller's own end, and the member reads what a |
| +// direct start's ended context always read, with no placement, shim or |
| +// cgroup words (those stay in the agent's log line) and no new text. |
| +func TestCancelledPlacementReadsAsTheCallersOwnEnd(t *testing.T) { |
| + const host = "h.example.com" |
| + cancelled := fmt.Errorf("seeding %s: %w", host, context.Canceled) |
| + timedOut := fmt.Errorf("seeding %s: %w", host, fmt.Errorf("timed out after %s and was killed: %w", seedTimeout, context.DeadlineExceeded)) |
| + if got := classifyStep(nil, cancelled); got != stepCancelled { |
| + t.Errorf("classifyStep(cancelled placement) = %v, want stepCancelled", got) |
| + } |
| + if got, want := classifyStep(nil, timedOut), classifyStep(nil, context.DeadlineExceeded); got != want { |
| + t.Errorf("classifyStep(timed-out placement) = %v, want %v, as for a direct start's deadline", got, want) |
| + } |
| + for err, want := range map[error]string{ |
| + cancelled: "wake failed: seeding h.example.com: context canceled", |
| + timedOut: "wake failed: seeding h.example.com: timed out after " + seedTimeout.String() + " and was killed: context deadline exceeded", |
| + } { |
| + got := memberSafeWakeReason(err) |
| + if got != want { |
| + t.Errorf("member reason = %q, want %q", got, want) |
| + } |
| + for _, word := range []string{"placement", "shim", "cgroup"} { |
| + if strings.Contains(got, word) { |
| + t.Errorf("member reason %q carries %q", got, word) |
| + } |
| + } |
| + if startMayHaveRun(nil, err) { |
| + t.Errorf("a start whose context ended during placement counted as one that may have run: %v", err) |
| + } |
| + } |
| +} |
diff --git a/internal/cgroup/cgroup.go b/internal/cgroup/cgroup.go| index 43f449ba1..db5faf516 100644 |
| --- a/internal/cgroup/cgroup.go |
| +++ b/internal/cgroup/cgroup.go |
| @@ -485,7 +485,13 @@ func startPlain(build func() *exec.Cmd, before func() error) (*exec.Cmd, error) |
| // this spawn made is removed. A start whose exec.CommandContext context |
| // ends during the placement is the caller's own condition too, never the |
| // box's: the shim reports it as a placement failure (it could not start, |
| -// or was killed before placement), so startIn tells it apart (startEnded). |
| +// or was killed before placement), so startIn tells it apart (cancelWatch) |
| +// and, required or not, answers what exec.Cmd.Start answers for a command |
| +// whose context has ended: the context's own error (context.Canceled or |
| +// context.DeadlineExceeded), bare. Nothing ran and nothing is retried. The |
| +// placement's detail goes to the operator log only, so a member reading |
| +// the wake's failure sees what a direct start's cancellation always showed |
| +// (whole-branch review, finding 3). |
| func (m *Manager) startIn(host, dir string, limit Limit, required bool, build func() *exec.Cmd, before func() error) (*exec.Cmd, error) { |
| if required && !shimAvailable() { |
| m.logf("cgroup: %s: the placement shim is not available in this process, and a generation is never started then moved — generation start refused", host) |
| @@ -509,7 +515,7 @@ func (m *Manager) startIn(host, dir string, limit Limit, required bool, build fu |
| return cmd, err |
| } |
| cmd := build() |
| - ended := watchCancel(cmd) |
| + watch := watchCancel(cmd) |
| refused, err := placeStart(m, dir, cmd, before) |
| switch { |
| case refused: |
| @@ -525,45 +531,88 @@ func (m *Manager) startIn(host, dir string, limit Limit, required bool, build fu |
| // same way. |
| _ = os.Remove(dir) |
| return nil, err |
| - case required && startEnded(err, ended): |
| + } |
| + if ctxErr, ended := watch.endedErr(err); ended { |
| // The caller's context ended: nothing ran, and it says nothing |
| - // about the box. |
| + // about the box. No fallback either: its Start would answer the |
| + // same error at once. |
| _ = os.Remove(dir) |
| - return nil, fmt.Errorf("cgroup: %s: the start's context ended before exec: %w", host, err) |
| - case required: |
| + m.logf("cgroup: %s: the start's context ended during placement (%v); nothing ran", host, err) |
| + return nil, ctxErr |
| + } |
| + if required { |
| _ = os.Remove(dir) |
| m.logf("cgroup: %s: placement before exec failed (%v) — generation start refused", host, err) |
| return nil, fmt.Errorf("%w: %s: %w", ErrLimitNotApplied, host, err) |
| } |
| // The flat start alone, after a placement failure the shim proved |
| - // happened before exec. A cancelled context lands here too (the shim |
| - // could not start), and the retry's Start then answers ctx's error at once. |
| + // happened before exec. |
| m.logf("cgroup: %s: placement before exec failed (%v) — using the PID-move fallback for this spawn", host, err) |
| cmd, _, err = m.startAndMoveIn(host, dir, build, before) |
| return cmd, err |
| } |
| -// watchCancel records whether cmd's exec.CommandContext context ended |
| +// cancelWatch records whether cmd's exec.CommandContext context ended |
| // while it was being started: exec.Cmd calls Cancel (by default, a kill) |
| // when the context ends after Start, which during a placement kills the |
| // shim. A cmd without a context has no Cancel, and nothing is recorded. |
| -func watchCancel(cmd *exec.Cmd) *atomic.Bool { |
| - var ended atomic.Bool |
| +type cancelWatch struct { |
| + ended atomic.Bool |
| + // probe is a copy of cmd taken before its start, for the context |
| + // exec.Cmd keeps unexported (contextErr). Nil without a Cancel. |
| + probe *exec.Cmd |
| +} |
| + |
| +func watchCancel(cmd *exec.Cmd) *cancelWatch { |
| + w := &cancelWatch{} |
| if cmd.Cancel != nil { |
| + probe := *cmd |
| + w.probe = &probe |
| cancel := cmd.Cancel |
| cmd.Cancel = func() error { |
| - ended.Store(true) |
| + w.ended.Store(true) |
| return cancel() |
| } |
| } |
| - return &ended |
| + return w |
| } |
| -// startEnded reports whether a failed placement was the start's context |
| -// ending: before the shim could start (Start answers the context's error, |
| -// which the placement error wraps), or during it (Cancel ran). |
| -func startEnded(err error, ended *atomic.Bool) bool { |
| - return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || ended.Load() |
| +// endedErr reports whether a failed placement was the start's context |
| +// ending, and that context's error: before the shim could start (Start |
| +// answered the context's error, which the placement error wraps), or |
| +// during it (Cancel ran, and the shim's answer names no context). |
| +func (w *cancelWatch) endedErr(err error) (error, bool) { |
| + switch { |
| + case errors.Is(err, context.Canceled): |
| + return context.Canceled, true |
| + case errors.Is(err, context.DeadlineExceeded): |
| + return context.DeadlineExceeded, true |
| + case w.ended.Load(): |
| + return w.contextErr(), true |
| + } |
| + return nil, false |
| +} |
| + |
| +// contextErr is the error of the command's context, once Cancel has run, |
| +// which exec.Cmd does only after that context is done. exec.Cmd keeps the |
| +// context unexported, but its Start answers ctx.Err() for a context that |
| +// has ended, before it opens a descriptor or forks. So the copy taken |
| +// before the start is asked to start, stripped of everything it could |
| +// otherwise act on: no files, no attributes, no Cancel, and /dev/null as |
| +// its path, which no exec would run. Should the answer ever be something |
| +// else, the context still ended, and Canceled is the caller's own end. |
| +// The unit tests pin both Canceled and DeadlineExceeded through it. |
| +func (w *cancelWatch) contextErr() error { |
| + if w.probe != nil { |
| + p := w.probe |
| + p.Path, p.Args, p.Env, p.Dir = "/dev/null", nil, []string{}, "" |
| + p.Stdin, p.Stdout, p.Stderr, p.ExtraFiles = nil, nil, nil, nil |
| + p.SysProcAttr, p.Cancel, p.WaitDelay = nil, nil, 0 |
| + if err := p.Start(); errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { |
| + return err |
| + } |
| + } |
| + return context.Canceled |
| } |
| // applyLimit writes limit into dir. Best-effort in the package's usual |
diff --git a/internal/cgroup/gen_kernel_linux_test.go b/internal/cgroup/gen_kernel_linux_test.go| index 063f3836b..6425ea1de 100644 |
| --- a/internal/cgroup/gen_kernel_linux_test.go |
| +++ b/internal/cgroup/gen_kernel_linux_test.go |
| @@ -750,6 +750,11 @@ func TestGenKernelCancelledGenerationStartIsNotABoxCondition(t *testing.T) { |
| if err == nil || cmd != nil || errors.Is(err, ErrLimitNotApplied) { |
| t.Fatalf("StartChild = %v, %v: want a failure that is not ErrLimitNotApplied", cmd, err) |
| } |
| + // The context's own error, bare, as exec.Cmd.Start answers an ended |
| + // context (whole-branch review, finding 3). |
| + if err != context.Canceled { |
| + t.Fatalf("StartChild = %v: want context.Canceled itself", err) |
| + } |
| t.Logf("cancelled generation start: %v", err) |
| assertReaped(t, built) |
| assertNoMarker(t, marker) |
diff --git a/internal/cgroup/gen_test.go b/internal/cgroup/gen_test.go| index b9e37ce18..9228131af 100644 |
| --- a/internal/cgroup/gen_test.go |
| +++ b/internal/cgroup/gen_test.go |
| @@ -4,6 +4,7 @@ import ( |
| "context" |
| "errors" |
| "fmt" |
| + "io" |
| "os" |
| "os/exec" |
| "path/filepath" |
| @@ -867,41 +868,121 @@ func TestEnsureParentCountsAPlacementAtTheParent(t *testing.T) { |
| // the context had ended before the shim could start (Start answers its |
| // error, which the placement error wraps), and it ends while the shim is |
| // being placed (exec.Cmd's Cancel kills the shim, whose answer names no |
| -// context). |
| +// context). Either way the start answers the context's own error, bare, |
| +// as exec.Cmd.Start does for an ended context: context.Canceled or |
| +// context.DeadlineExceeded as the context ended, with no placement text, |
| +// which goes to the operator log (whole-branch review, finding 3). |
| func TestGenerationCancelledStartIsNotABoxCondition(t *testing.T) { |
| const host = "h.example.com" |
| + ended := map[string]func(t *testing.T) context.Context{ |
| + "cancelled": func(t *testing.T) context.Context { |
| + ctx, cancel := context.WithCancel(t.Context()) |
| + cancel() |
| + return ctx |
| + }, |
| + "past its deadline": func(t *testing.T) context.Context { |
| + ctx, cancel := context.WithDeadline(t.Context(), time.Unix(1, 0)) |
| + t.Cleanup(cancel) |
| + return ctx |
| + }, |
| + } |
| for name, place := range map[string]func(*Manager, string, *exec.Cmd, func() error) (bool, error){ |
| - "ended before the shim started": func(*Manager, string, *exec.Cmd, func() error) (bool, error) { |
| - return false, &placementError{fmt.Errorf("start the shim: %w", context.Canceled)} |
| + "ended before the shim started": func(_ *Manager, _ string, cmd *exec.Cmd, _ func() error) (bool, error) { |
| + return false, &placementError{fmt.Errorf("start the shim: %w", cmd.Start())} |
| }, |
| "ended during the placement": func(_ *Manager, _ string, cmd *exec.Cmd, _ func() error) (bool, error) { |
| _ = cmd.Cancel() // what exec.Cmd's context watcher does |
| return false, &placementError{errors.New("the shim exited before placement")} |
| }, |
| } { |
| - t.Run(name, func(t *testing.T) { |
| - m, parent, lines := legacyParent(t, host, "") |
| - stubPlacement(t, place) |
| + for ctxName, mkCtx := range ended { |
| + t.Run(name+", "+ctxName, func(t *testing.T) { |
| + m, parent, lines := legacyParent(t, host, "") |
| + stubPlacement(t, place) |
| + ctx := mkCtx(t) |
| + cmd, err := m.StartChild(host, GenChild(1), TierDefault, Limit{}, func() *exec.Cmd { |
| + c := exec.CommandContext(ctx, "/bin/true") |
| + c.Cancel = func() error { return nil } // no process to kill here |
| + return c |
| + }) |
| + if cmd != nil || err != ctx.Err() { |
| + t.Fatalf("StartChild = %v, %v: want the context's own error, %v, bare", cmd, err, ctx.Err()) |
| + } |
| + if errors.Is(err, ErrLimitNotApplied) || isPlacementError(err) { |
| + t.Fatalf("StartChild = %v: a cancelled start read as the box's condition", err) |
| + } |
| + if _, serr := os.Stat(filepath.Join(parent, "g1")); !os.IsNotExist(serr) { |
| + t.Fatalf("the empty child survived: %v", serr) |
| + } |
| + logged := false |
| + for _, l := range *lines { |
| + if strings.Contains(l, "generation start refused") { |
| + t.Errorf("a cancelled start logged a box condition: %q", l) |
| + } |
| + if strings.Contains(l, "the start's context ended during placement") && strings.Contains(l, "placement before exec") { |
| + logged = true |
| + } |
| + } |
| + if !logged { |
| + t.Errorf("log lines = %q, want the placement's detail in the operator line", *lines) |
| + } |
| + }) |
| + } |
| + } |
| +} |
| + |
| +// contextErr asks a copy of the command, taken before its start, for the |
| +// context's error. The copy must never run anything, even with the |
| +// original's files and attributes set, and must leave the original as it |
| +// was. |
| +func TestCancelWatchContextErrRunsNothing(t *testing.T) { |
| + for _, tc := range []struct { |
| + name string |
| + ctx func(t *testing.T) context.Context |
| + want error |
| + }{ |
| + {"cancelled", func(t *testing.T) context.Context { |
| ctx, cancel := context.WithCancel(t.Context()) |
| cancel() |
| - cmd, err := m.StartChild(host, GenChild(1), TierDefault, Limit{}, func() *exec.Cmd { |
| - c := exec.CommandContext(ctx, "/bin/true") |
| - c.Cancel = func() error { return nil } // no process to kill here |
| - return c |
| - }) |
| - if err == nil || cmd != nil || errors.Is(err, ErrLimitNotApplied) { |
| - t.Fatalf("StartChild = %v, %v: want a failure that is not ErrLimitNotApplied", cmd, err) |
| + return ctx |
| + }, context.Canceled}, |
| + {"past its deadline", func(t *testing.T) context.Context { |
| + ctx, cancel := context.WithDeadline(t.Context(), time.Unix(1, 0)) |
| + t.Cleanup(cancel) |
| + return ctx |
| + }, context.DeadlineExceeded}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + marker := filepath.Join(t.TempDir(), "ran") |
| + r, w, err := os.Pipe() |
| + if err != nil { |
| + t.Fatal(err) |
| } |
| - if _, serr := os.Stat(filepath.Join(parent, "g1")); !os.IsNotExist(serr) { |
| - t.Fatalf("the empty child survived: %v", serr) |
| + defer r.Close() |
| + defer w.Close() |
| + ctx := tc.ctx(t) |
| + cmd := exec.CommandContext(ctx, "/bin/sh", "-c", `touch "$0"`, marker) |
| + cmd.ExtraFiles = []*os.File{w} |
| + cmd.Stdout = io.Discard |
| + cmd.Cancel = func() error { return nil } // no process to kill here |
| + watch := watchCancel(cmd) |
| + _ = cmd.Cancel() // the context watcher's call |
| + got, ended := watch.endedErr(&placementError{errors.New("the shim exited before placement")}) |
| + if !ended || got != tc.want { |
| + t.Fatalf("endedErr = %v, %v: want %v", got, ended, tc.want) |
| } |
| - for _, l := range *lines { |
| - if strings.Contains(l, "generation start refused") { |
| - t.Errorf("a cancelled start logged a box condition: %q", l) |
| - } |
| + if _, err := os.Stat(marker); err == nil { |
| + t.Fatal("the probe ran the command") |
| + } |
| + if cmd.Path != "/bin/sh" || len(cmd.ExtraFiles) != 1 || cmd.Stdout != io.Discard || cmd.Process != nil { |
| + t.Fatalf("the probe changed the original command: %+v", cmd) |
| } |
| }) |
| } |
| + // Without a context nothing is watched and nothing ended. |
| + if _, ended := watchCancel(exec.Command("/bin/true")).endedErr(&placementError{errors.New("x")}); ended { |
| + t.Fatal("a command without a context read as ended") |
| + } |
| } |
| // Whole-branch review, finding 1, at ensureParent: a sibling generation in |