diff --git a/cmd/ateom-microvm/internal/ch/merge.go b/cmd/ateom-microvm/internal/ch/merge.go index 746a3bb11..e20f18ce7 100644 --- a/cmd/ateom-microvm/internal/ch/merge.go +++ b/cmd/ateom-microvm/internal/ch/merge.go @@ -24,6 +24,7 @@ import ( "os" "os/exec" + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/reaper" "golang.org/x/sys/unix" ) @@ -49,7 +50,7 @@ func MergeSparseOverlay(ctx context.Context, baseFile, deltaFile, outFile string // outFile := sparse copy of baseFile (preserves holes so it stays sparse). tmp := outFile + ".merge.tmp" _ = os.Remove(tmp) - if o, err := exec.CommandContext(ctx, "cp", "--sparse=always", baseFile, tmp).CombinedOutput(); err != nil { + if o, err := reaper.RunCombined(exec.CommandContext(ctx, "cp", "--sparse=always", baseFile, tmp)); err != nil { return fmt.Errorf("cp base->tmp: %w: %s", err, o) } diff --git a/cmd/ateom-microvm/internal/kata/overlay_linux.go b/cmd/ateom-microvm/internal/kata/overlay_linux.go index 708dfc450..4a7864519 100644 --- a/cmd/ateom-microvm/internal/kata/overlay_linux.go +++ b/cmd/ateom-microvm/internal/kata/overlay_linux.go @@ -31,6 +31,7 @@ import ( "strings" "time" + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/reaper" "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/third_party/kata/agentpb" specs "github.com/opencontainers/runtime-spec/specs-go" ) @@ -134,8 +135,8 @@ func ReconstructSharedDirFromImage(ctx context.Context, bundleRootfs, restoreID, dst := filepath.Join(SharedDir(restoreID), cid, "rootfs") // Drop any stale bind first (lazy if busy), then ensure a clean mountpoint. Not // RemoveAll: that would chase a live bind into bundleRootfs. - if err := exec.Command("umount", dst).Run(); err != nil { - _ = exec.Command("umount", "-l", dst).Run() + if err := reaper.Run(exec.Command("umount", dst)); err != nil { + _ = reaper.Run(exec.Command("umount", "-l", dst)) } if err := os.MkdirAll(dst, 0o755); err != nil { return fmt.Errorf("creating shared dir %q: %w", dst, err) @@ -143,7 +144,7 @@ func ReconstructSharedDirFromImage(ctx context.Context, bundleRootfs, restoreID, cmd := exec.CommandContext(ctx, "mount", "--bind", bundleRootfs, dst) var stderr strings.Builder cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { + if err := reaper.Run(cmd); err != nil { return fmt.Errorf("bind-mounting image rootfs %q -> %q: %w (%s)", bundleRootfs, dst, err, strings.TrimSpace(stderr.String())) } // Ensure the standard OCI mountpoints exist even for minimal images: the container @@ -157,7 +158,7 @@ func ReconstructSharedDirFromImage(ctx context.Context, bundleRootfs, restoreID, ro := exec.CommandContext(ctx, "mount", "-o", "remount,bind,ro", dst) var roErr strings.Builder ro.Stderr = &roErr - if err := ro.Run(); err != nil { + if err := reaper.Run(ro); err != nil { return fmt.Errorf("remounting overlay lower read-only %q: %w (%s)", dst, err, strings.TrimSpace(roErr.String())) } return nil diff --git a/cmd/ateom-microvm/internal/reaper/reaper.go b/cmd/ateom-microvm/internal/reaper/reaper.go new file mode 100644 index 000000000..0c99e9509 --- /dev/null +++ b/cmd/ateom-microvm/internal/reaper/reaper.go @@ -0,0 +1,66 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package reaper centralizes ateom-microvm's child-process reaping. +// +// ateom-microvm spawns long-lived, detached helpers (the cloud-hypervisor VMM, +// virtiofsd) that get reparented to it, so it runs a SIGCHLD reaper (Start) to +// collect them. That reaper calls wait4(-1), which will collect ANY child of the +// process — including one an exec.Cmd is about to wait for itself. When that +// happens the caller's own wait fails with "waitid: no child processes". +// +// Run and RunCombined are for the other kind of subprocess: short, synchronous +// helpers (mount, umount, cp, ...) whose exit status the caller needs. They hold a +// shared lock that is mutually exclusive with the reaper's collection pass, so the +// reaper cannot collect the child before the caller's wait completes. Every +// synchronous subprocess in ateom-microvm MUST go through Run/RunCombined. +// +// Detached daemons that must outlive the call (the VMM, virtiofsd) are deliberately +// NOT run through here: they are started with Cmd.Start and left for the reaper. +// +// gVisor's ateom (cmd/ateom-gvisor) holds the same kind of lock in shared mode around +// every runsc invocation; this package is the micro-VM equivalent, shared across the +// ateom-microvm subpackages that exec (internal/kata, internal/ch). +package reaper + +import ( + "os/exec" + "sync" +) + +// lock serializes deliberate fork+exec+wait against the child reaper. Synchronous +// callers hold it shared (RLock); the reaper started by Start holds it exclusively +// (Lock) while calling wait4(-1). A single process-global lock is the correct model: +// there is exactly one wait4(-1) collector per process. +var lock sync.RWMutex + +// Run runs cmd to completion (fork+exec+wait) with the reap lock held shared, so the +// child reaper cannot collect cmd's process before cmd.Wait does. Use it for every +// synchronous subprocess whose exit status matters. +// +// Do NOT use it for a daemon that must outlive the call (cloud-hypervisor, virtiofsd): +// those are started detached and intentionally left for the reaper. +func Run(cmd *exec.Cmd) error { + lock.RLock() + defer lock.RUnlock() + return cmd.Run() +} + +// RunCombined is Run for callers that need the command's merged stdout+stderr +// (cmd.CombinedOutput) rather than just its exit status. +func RunCombined(cmd *exec.Cmd) ([]byte, error) { + lock.RLock() + defer lock.RUnlock() + return cmd.CombinedOutput() +} diff --git a/cmd/ateom-microvm/internal/reaper/reaper_linux.go b/cmd/ateom-microvm/internal/reaper/reaper_linux.go new file mode 100644 index 000000000..84c10536e --- /dev/null +++ b/cmd/ateom-microvm/internal/reaper/reaper_linux.go @@ -0,0 +1,36 @@ +//go:build linux + +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package reaper + +import ( + "sync" + + "github.com/hashicorp/go-reap" +) + +var startOnce sync.Once + +// Start launches the SIGCHLD child reaper in a background goroutine, guarded by the +// package lock so the synchronous subprocesses run via Run/RunCombined are never +// collected out from under their own wait. It reaps the detached processes +// ateom-microvm spawns and that get reparented to it — the cloud-hypervisor VMM and +// virtiofsd. Start is idempotent; only the first call launches the reaper. +func Start() { + startOnce.Do(func() { + go reap.ReapChildren(nil, nil, nil, &lock) + }) +} diff --git a/cmd/ateom-microvm/internal/reaper/reaper_test.go b/cmd/ateom-microvm/internal/reaper/reaper_test.go new file mode 100644 index 000000000..bf99b0186 --- /dev/null +++ b/cmd/ateom-microvm/internal/reaper/reaper_test.go @@ -0,0 +1,78 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package reaper + +import ( + "io" + "os/exec" + "testing" + "time" +) + +func TestRunReturnsCommandResult(t *testing.T) { + if err := Run(exec.Command("true")); err != nil { + t.Fatalf("Run(true) = %v, want nil", err) + } + if err := Run(exec.Command("false")); err == nil { + t.Fatal("Run(false) = nil, want a non-nil exit error") + } +} + +func TestRunCombinedReturnsOutput(t *testing.T) { + out, err := RunCombined(exec.Command("sh", "-c", "echo hello")) + if err != nil { + t.Fatalf("RunCombined = %v, want nil", err) + } + if string(out) != "hello\n" { + t.Fatalf("RunCombined output = %q, want %q", out, "hello\n") + } +} + +// TestRunHoldsReapLock verifies the invariant that makes Run safe against the child +// reaper (#418): while a guarded command is executing, the exclusive lock the reaper +// takes before wait4(-1) cannot be acquired, so the reaper cannot collect the child +// out from under cmd.Wait (which would surface as "waitid: no child processes"). +func TestRunHoldsReapLock(t *testing.T) { + // cat blocks until its stdin closes, so we control exactly when the guarded + // command exits — no sleeps, no timing guesses. + pr, pw := io.Pipe() + cmd := exec.Command("cat") + cmd.Stdin = pr + + done := make(chan error, 1) + go func() { done <- Run(cmd) }() + + // Spin until Run has taken the shared lock (and thus forked the child): while it + // holds RLock, the exclusive TryLock the reaper would use must fail. + deadline := time.Now().Add(5 * time.Second) + for lock.TryLock() { + lock.Unlock() + if time.Now().After(deadline) { + t.Fatal("Run never acquired the reap lock") + } + time.Sleep(time.Millisecond) + } + + // Release the child and confirm Run succeeds and frees the lock afterward. + _ = pw.Close() + if err := <-done; err != nil { + t.Fatalf("guarded command failed: %v", err) + } + if !lock.TryLock() { + t.Fatal("reap lock still held after the guarded command returned") + } + lock.Unlock() + _ = pr.Close() +} diff --git a/cmd/ateom-microvm/main.go b/cmd/ateom-microvm/main.go index c1998da20..f6fb4e4a0 100644 --- a/cmd/ateom-microvm/main.go +++ b/cmd/ateom-microvm/main.go @@ -33,13 +33,13 @@ import ( "sync" "cloud.google.com/go/compute/metadata" + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/reaper" "github.com/agent-substrate/substrate/internal/actorlog" "github.com/agent-substrate/substrate/internal/ateinterceptors" "github.com/agent-substrate/substrate/internal/ateompath" "github.com/agent-substrate/substrate/internal/proto/ateompb" "github.com/agent-substrate/substrate/internal/serverboot" "github.com/agent-substrate/substrate/internal/version" - "github.com/hashicorp/go-reap" "github.com/vishvananda/netns" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" sdktrace "go.opentelemetry.io/otel/sdk/trace" @@ -54,10 +54,6 @@ var ( kataConfig = flag.String("kata-config", "", "Path to a kata configuration.toml (passed to the shim as KATA_CONF_FILE). Empty uses kata's default. atelet generates one pointing at runtime-fetched assets.") kataDebug = flag.Bool("kata-debug", false, "Verbose kata-agent debugging: raise the guest agent log level and forward the guest console (incl. agent logs) into the pod logs.") showVersion = flag.Bool("version", false, "Print version and exit.") - - // reapLock guards subprocess exec against the child reaper: ateom-microvm - // spawns the cloud-hypervisor process under it. - reapLock sync.RWMutex ) func main() { @@ -100,9 +96,11 @@ func do(ctx context.Context) error { return fmt.Errorf("in os.MkdirAll(%q): %w", ateomDir, err) } - // Reap children reparented to us (cloud-hypervisor), guarded so our own - // exec.Cmd calls can take the wait. - go reap.ReapChildren(nil, nil, nil, &reapLock) + // Reap children reparented to us: the detached cloud-hypervisor VMM and + // virtiofsd. Synchronous subprocesses (mount, umount, cp, ...) instead go + // through reaper.Run/RunCombined so this reaper cannot collect them out from + // under their own wait (which would surface as "waitid: no child processes"). + reaper.Start() slog.InfoContext(ctx, "Child process reaper launched") // kata's virtio-fs sharing depends on mount propagation: it slave-binds