From 828d8ee6f34d448b136abb462e7a1d6f85544c25 Mon Sep 17 00:00:00 2001 From: preslavmihaylov Date: Mon, 5 Oct 2026 00:35:04 +0300 Subject: [PATCH 1/4] SHARD-622+623: a verb cut after its vmm came up still ends the vmm, and an OOM attach whose release breaks off lets its stream go A create, start, fork or restore that failed after attach called p.end on the verb's own ctx. When the failure was the verb's cut, awaitGone returned on that ctx before settle, so the machine stayed in p.machines with its pin and control stream while the record that names it was gone. endAnyway ends it on its own clock. An attach that reads an OOM in the guest state releases a machine no map holds yet; a release that broke off before its settle never closed it, so it closes the machine itself. --- services/provider/firecracker/checkpoint.go | 4 +- services/provider/firecracker/export_test.go | 9 ++ .../provider/firecracker/failguest_test.go | 109 +++++++++++++++++- services/provider/firecracker/fakevmm_test.go | 4 + .../provider/firecracker/firecracker_test.go | 86 ++++++++++++++ services/provider/firecracker/lifecycle.go | 9 +- services/provider/firecracker/machine.go | 9 +- 7 files changed, 221 insertions(+), 9 deletions(-) diff --git a/services/provider/firecracker/checkpoint.go b/services/provider/firecracker/checkpoint.go index 389f6558e..849d97a78 100644 --- a/services/provider/firecracker/checkpoint.go +++ b/services/provider/firecracker/checkpoint.go @@ -373,7 +373,7 @@ func (p *Provider) restore(ctx context.Context, id, stateDir string, r record, d return nil, err } if err := m.reseed(ctx); err != nil { - return nil, errors.Join(err, p.end(ctx, m)) + return nil, errors.Join(err, p.endAnyway(ctx, m)) } // Only a running sandbox is ever paused, so what a checkpoint brings back is running and Status says so. @@ -475,7 +475,7 @@ func (p *Provider) forkCheckpoint(ctx context.Context, dir string, spec models.S } // The restored guest still answers to the source's address and MAC, which the readdress replaces in place. if err := m.readdress(ctx, r); err != nil { - return errors.Join(err, p.end(ctx, m), os.Remove(filepath.Join(spec.StateDir, recordFile))) + return errors.Join(err, p.endAnyway(ctx, m), os.Remove(filepath.Join(spec.StateDir, recordFile))) } return nil diff --git a/services/provider/firecracker/export_test.go b/services/provider/firecracker/export_test.go index a0c16956d..586b157a7 100644 --- a/services/provider/firecracker/export_test.go +++ b/services/provider/firecracker/export_test.go @@ -88,3 +88,12 @@ func SetRedialGrace(grace time.Duration) (restore func()) { return func() { redialGrace = was } } + +// Holds says whether the provider keeps a machine for id, which a verb that failed must not leave behind. +func (p *Provider) Holds(id string) bool { + p.mu.Lock() + defer p.mu.Unlock() + _, held := p.machines[id] + + return held +} diff --git a/services/provider/firecracker/failguest_test.go b/services/provider/firecracker/failguest_test.go index 62aefd064..cca16b6db 100644 --- a/services/provider/firecracker/failguest_test.go +++ b/services/provider/firecracker/failguest_test.go @@ -3,11 +3,14 @@ package firecracker_test import ( "bufio" "context" + "errors" "fmt" "io" "net" "os" "path/filepath" + "runtime/debug" + "strconv" "strings" "syscall" "testing" @@ -46,10 +49,20 @@ func failingGuest(dir string) error { } }() - conn, err := control.Accept() - if err != nil { - return fmt.Errorf("accept control: %w", err) + // A restore's refreeze speaks first and hangs up, so every host connection is answered in turn. + for { + conn, err := control.Accept() + if err != nil { + return fmt.Errorf("accept control: %w", err) + } + if err := answerUntilStop(conn); !errors.Is(err, io.EOF) { + return err + } } +} + +// answerUntilStop sends the state, answers every request, and reports the death on the stop. +func answerUntilStop(conn net.Conn) error { if err := supervisor.WriteMessage(conn, supervisor.Message{Kind: supervisor.KindState, Logs: supervisor.LogsVersion}); err != nil { return fmt.Errorf("send the state: %w", err) } @@ -287,3 +300,93 @@ func TestABootFailureAnswersTheStartWithItsReason(t *testing.T) { t.Fatalf("Wait = %+v, %v, want the supervisor's %d", exit, err, models.SupervisorFailedExitCode) } } + +// oomGuestEnv makes the test binary a guest whose state carries a kill by its memory bound no host heard, and whose halt never comes. +const oomGuestEnv = "FIRECRACKER_FAKE_OOM_GUEST" + +// oomGuest opens with the kill in its state, answers the stop it brings, and keeps the VM up, so it returns only on a failure. +func oomGuest(dir string) error { + control, err := net.Listen("unix", filepath.Join(dir, fmt.Sprintf("%d.sock", supervisor.ControlPort))) + if err != nil { + return fmt.Errorf("listen for control: %w", err) + } + conn, err := control.Accept() + if err != nil { + return fmt.Errorf("accept control: %w", err) + } + if err := supervisor.WriteMessage(conn, supervisor.Message{Kind: supervisor.KindState, OOM: true, Logs: supervisor.LogsVersion}); err != nil { + return fmt.Errorf("send the state: %w", err) + } + var stop supervisor.Message + if err := supervisor.ReadMessage(bufio.NewReader(conn), &stop); err != nil { + return fmt.Errorf("read the stop: %w", err) + } + if err := supervisor.WriteMessage(conn, supervisor.Message{Kind: supervisor.KindDone, ID: stop.ID}); err != nil { + return fmt.Errorf("answer the stop: %w", err) + } + for { + time.Sleep(time.Hour) + } +} + +// An attach that replays a kill and whose release breaks off lets the control stream go, since no map holds the machine for a later verb to (SHARD-623). +func TestAnOOMAttachWhoseReleaseBreaksOffKeepsNoStream(t *testing.T) { + h := newHarness(t) + spec := h.newSpec(t, "/bin/sh", "-c", "while true; do sleep 1; done") + self, err := os.Executable() + if err != nil { + t.Fatal(err) + } + t.Setenv(fakeInitEnv, self) + t.Setenv(oomGuestEnv, "1") + // A finalizer closes a leaked stream at the next GC, which would hide the leak. + defer debug.SetGCPercent(debug.SetGCPercent(-1)) + before := openSockets(t) + + // The marker is down before the stop goes, so the cut lands on the release's wait for the halt. + ctx := newCutCtx(func() bool { + _, err := os.Stat(filepath.Join(spec.StateDir, "oom")) + + return err == nil + }) + if err := h.provider.Create(ctx, spec); !errors.Is(err, context.Canceled) { + t.Fatalf("Create = %v, want the release cut", err) + } + // The client lets its idle connections to the killed vmm go on their own goroutine. + for deadline := time.Now().Add(5 * time.Second); openSockets(t) != before; time.Sleep(50 * time.Millisecond) { + if time.Now().After(deadline) { + t.Fatalf("open sockets %d -> %d, want the control stream of the released machine closed", before, openSockets(t)) + } + } +} + +// openSockets counts the sockets this process holds open. +func openSockets(t *testing.T) int { + t.Helper() + + entries, err := os.ReadDir("/dev/fd") + if err != nil { + t.Fatal(err) + } + n := 0 + for _, entry := range entries { + fd, err := strconv.Atoi(entry.Name()) + if err != nil { + t.Fatal(err) + } + var st syscall.Stat_t + err = syscall.Fstat(fd, &st) + // The listing read through an fd of its own, closed by now. + if errors.Is(err, syscall.EBADF) { + continue + } + if err != nil { + t.Fatal(err) + } + if st.Mode&syscall.S_IFMT == syscall.S_IFSOCK { + n++ + } + } + + return n +} diff --git a/services/provider/firecracker/fakevmm_test.go b/services/provider/firecracker/fakevmm_test.go index 2251f249d..b56982911 100644 --- a/services/provider/firecracker/fakevmm_test.go +++ b/services/provider/firecracker/fakevmm_test.go @@ -94,6 +94,10 @@ func TestMain(m *testing.M) { } os.Exit(models.SupervisorFailedExitCode) } + if os.Getenv(oomGuestEnv) == "1" && len(os.Args) == 3 && os.Args[1] == "-transport" { + fmt.Fprintln(os.Stderr, "oom guest:", oomGuest(strings.TrimPrefix(os.Args[2], "unix:"))) + os.Exit(1) + } if os.Getenv(fakeVMMEnv) == "1" && slices.Contains(os.Args[1:], "--exec-file") { if err := fakeJailer(); err != nil { fmt.Fprintln(os.Stderr, "fake jailer:", err) diff --git a/services/provider/firecracker/firecracker_test.go b/services/provider/firecracker/firecracker_test.go index 19ba2e625..0e3c93aa8 100644 --- a/services/provider/firecracker/firecracker_test.go +++ b/services/provider/firecracker/firecracker_test.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io/fs" + "net/netip" "os" "os/exec" "path/filepath" @@ -2150,6 +2151,91 @@ func TestAnUnloadedVMMLeftByACutForkIsEnded(t *testing.T) { requireUnloadedEnded(t, h.reopen(t), spec, exited, h.jail(spec.ID)) } +// A verb cut once its vmm came up ends the vmm on its own clock, so no machine outlives the record the failed verb drops (SHARD-622). +func TestAVerbCutAfterItsAttachLeavesNoMachine(t *testing.T) { + address := models.NetworkSpec{Address: netip.MustParsePrefix("10.87.0.9/16"), Gateway: netip.MustParseAddr("10.87.0.1")} + t.Run("a create at its readdress", func(t *testing.T) { + h := newHarness(t) + spec := h.newSpec(t, "/bin/sh", "-c", "while true; do sleep 1; done") + spec.Network = address + answerEveryRequest(t) + requireCutLeavesNoMachine(t, h, spec, false, func(ctx context.Context) error { return h.provider.Create(ctx, spec) }) + }) + for _, readdress := range []bool{false, true} { + t.Run(fmt.Sprintf("a fork, at its readdress %v", readdress), func(t *testing.T) { + h := newHarness(t) + source, _ := h.runLong(t) + fork := h.forkSpec(t) + if readdress { + fork.Network = address + } + answerEveryRequest(t) + requireCutLeavesNoMachine(t, h, fork, readdress, func(ctx context.Context) error { return h.provider.Fork(ctx, source.ID, fork) }) + }) + } +} + +// answerEveryRequest makes the next guest the failing one, which answers a readdress without touching any interface; no stop reaches it here. +func answerEveryRequest(t *testing.T) { + t.Helper() + + self, err := os.Executable() + if err != nil { + t.Fatal(err) + } + t.Setenv(fakeInitEnv, self) + t.Setenv(failingGuestEnv, "1") +} + +// requireCutLeavesNoMachine cuts verb once the attach opened the log, or once the reseed after it is in too, and requires the provider to hold nothing for spec. +func requireCutLeavesNoMachine(t *testing.T, h *harness, spec models.SandboxSpec, reseeded bool, verb func(context.Context) error) { + t.Helper() + + ctx := newCutCtx(func() bool { + if _, err := os.Stat(filepath.Join(spec.StateDir, "output.log")); err != nil { + return false + } + _, err := os.Stat(filepath.Join(spec.StateDir, firecracker.ReseedFile)) + + return !reseeded || errors.Is(err, fs.ErrNotExist) + }) + if err := verb(ctx); !errors.Is(err, context.Canceled) { + t.Fatalf("the verb = %v, want the cut", err) + } + if h.provider.Holds(spec.ID) { + t.Fatal("the provider still holds a machine for the cut verb, which no record names and no verb reaches") + } +} + +// cutCtx is a context cut the moment its condition holds, read on every check so the cut lands at an exact step. +type cutCtx struct { + context.Context + cut func() bool + once sync.Once + done chan struct{} +} + +func newCutCtx(cut func() bool) *cutCtx { + return &cutCtx{Context: context.Background(), cut: cut, done: make(chan struct{})} +} + +func (c *cutCtx) Done() <-chan struct{} { + if c.cut() { + c.once.Do(func() { close(c.done) }) + } + + return c.done +} + +func (c *cutCtx) Err() error { + select { + case <-c.Done(): + return context.Canceled + default: + return nil + } +} + // A vmm a daemon before the jail spawned answers in the state directory, and the next daemon still finds it there and ends it (SHARD-306). func TestAVMMFromBeforeTheJailIsFoundAtItsOldSocket(t *testing.T) { h := newHarness(t) diff --git a/services/provider/firecracker/lifecycle.go b/services/provider/firecracker/lifecycle.go index 9d4b7d568..2d54900f9 100644 --- a/services/provider/firecracker/lifecycle.go +++ b/services/provider/firecracker/lifecycle.go @@ -69,7 +69,7 @@ func (p *Provider) launch(ctx context.Context, id, dir string, r record, run boo return errors.Join(err, os.Remove(filepath.Join(dir, recordFile))) } if err := m.readdress(ctx, r); err != nil { - return errors.Join(err, p.end(ctx, m), os.Remove(filepath.Join(dir, recordFile))) + return errors.Join(err, p.endAnyway(ctx, m), os.Remove(filepath.Join(dir, recordFile))) } if !run { return nil @@ -220,7 +220,7 @@ func (p *Provider) Start(ctx context.Context, id string) error { return err } if err := m.readdress(ctx, r); err != nil { - return errors.Join(err, p.end(ctx, m)) + return errors.Join(err, p.endAnyway(ctx, m)) } return p.run(ctx, m, r) @@ -350,6 +350,11 @@ func (p *Provider) end(ctx context.Context, m *machine) error { return p.settle(ctx, m) } +// endAnyway ends the vmm of a verb that failed after it came up, on its own clock: the failure may be the verb's cut, and the vmm must go either way (SHARD-622). +func (p *Provider) endAnyway(ctx context.Context, m *machine) error { + return p.end(context.WithoutCancel(ctx), m) +} + // endSilent kills a silent vmm through the pin its attach or adopt took, never by a pid or the socket, either of which may name a vmm begun since. func (p *Provider) endSilent(ctx context.Context, m *machine) error { if err := m.pinned.Kill(); err != nil { diff --git a/services/provider/firecracker/machine.go b/services/provider/firecracker/machine.go index 1ddd4bb8f..5b34ed10e 100644 --- a/services/provider/firecracker/machine.go +++ b/services/provider/firecracker/machine.go @@ -171,7 +171,7 @@ func (p *Provider) lookup(ctx context.Context, id, dir string, r record) (*machi } // A daemon cut between a restore's attach and its reseed left the guest on the checkpoint's key, and no other step gives it one. if err := m.reseed(ctx); err != nil { - return nil, errors.Join(err, p.end(ctx, m)) + return nil, errors.Join(err, p.endAnyway(ctx, m)) } // The attach thawed a guest the cut capture left frozen, so the source runs again and the marker is spent. if err := os.Remove(filepath.Join(dir, captureFile)); err != nil && !errors.Is(err, fs.ErrNotExist) { @@ -618,7 +618,12 @@ func (p *Provider) attach(ctx context.Context, id, dir, jail string, client *fca } if state.OOM { // The guest kept a kill no host heard; the marker is on disk and it is going, so there is nothing to follow. - return nil, p.release(ctx, m) + if err := p.release(ctx, m); err != nil { + // No map holds this machine yet, so a release that broke off before its settle is the last chance to let the pin and the stream go (SHARD-623). + return nil, errors.Join(err, m.close()) + } + + return nil, nil } // A checkpoint holds the guest frozen, so it runs nothing on the saved crng key until the reseed is in and the thaw follows (SHARD-409). if state.Frozen { From 4ede2b46cca457e8e413658a08d30a5039b57436 Mon Sep 17 00:00:00 2001 From: preslavmihaylov Date: Mon, 5 Oct 2026 00:56:16 +0300 Subject: [PATCH 2/4] SHARD-622: drop the 623 OOM attach hunk, which has no box trigger --- .../provider/firecracker/failguest_test.go | 92 ------------------- services/provider/firecracker/fakevmm_test.go | 4 - services/provider/firecracker/machine.go | 7 +- 3 files changed, 1 insertion(+), 102 deletions(-) diff --git a/services/provider/firecracker/failguest_test.go b/services/provider/firecracker/failguest_test.go index cca16b6db..5a1af6eff 100644 --- a/services/provider/firecracker/failguest_test.go +++ b/services/provider/firecracker/failguest_test.go @@ -9,8 +9,6 @@ import ( "net" "os" "path/filepath" - "runtime/debug" - "strconv" "strings" "syscall" "testing" @@ -300,93 +298,3 @@ func TestABootFailureAnswersTheStartWithItsReason(t *testing.T) { t.Fatalf("Wait = %+v, %v, want the supervisor's %d", exit, err, models.SupervisorFailedExitCode) } } - -// oomGuestEnv makes the test binary a guest whose state carries a kill by its memory bound no host heard, and whose halt never comes. -const oomGuestEnv = "FIRECRACKER_FAKE_OOM_GUEST" - -// oomGuest opens with the kill in its state, answers the stop it brings, and keeps the VM up, so it returns only on a failure. -func oomGuest(dir string) error { - control, err := net.Listen("unix", filepath.Join(dir, fmt.Sprintf("%d.sock", supervisor.ControlPort))) - if err != nil { - return fmt.Errorf("listen for control: %w", err) - } - conn, err := control.Accept() - if err != nil { - return fmt.Errorf("accept control: %w", err) - } - if err := supervisor.WriteMessage(conn, supervisor.Message{Kind: supervisor.KindState, OOM: true, Logs: supervisor.LogsVersion}); err != nil { - return fmt.Errorf("send the state: %w", err) - } - var stop supervisor.Message - if err := supervisor.ReadMessage(bufio.NewReader(conn), &stop); err != nil { - return fmt.Errorf("read the stop: %w", err) - } - if err := supervisor.WriteMessage(conn, supervisor.Message{Kind: supervisor.KindDone, ID: stop.ID}); err != nil { - return fmt.Errorf("answer the stop: %w", err) - } - for { - time.Sleep(time.Hour) - } -} - -// An attach that replays a kill and whose release breaks off lets the control stream go, since no map holds the machine for a later verb to (SHARD-623). -func TestAnOOMAttachWhoseReleaseBreaksOffKeepsNoStream(t *testing.T) { - h := newHarness(t) - spec := h.newSpec(t, "/bin/sh", "-c", "while true; do sleep 1; done") - self, err := os.Executable() - if err != nil { - t.Fatal(err) - } - t.Setenv(fakeInitEnv, self) - t.Setenv(oomGuestEnv, "1") - // A finalizer closes a leaked stream at the next GC, which would hide the leak. - defer debug.SetGCPercent(debug.SetGCPercent(-1)) - before := openSockets(t) - - // The marker is down before the stop goes, so the cut lands on the release's wait for the halt. - ctx := newCutCtx(func() bool { - _, err := os.Stat(filepath.Join(spec.StateDir, "oom")) - - return err == nil - }) - if err := h.provider.Create(ctx, spec); !errors.Is(err, context.Canceled) { - t.Fatalf("Create = %v, want the release cut", err) - } - // The client lets its idle connections to the killed vmm go on their own goroutine. - for deadline := time.Now().Add(5 * time.Second); openSockets(t) != before; time.Sleep(50 * time.Millisecond) { - if time.Now().After(deadline) { - t.Fatalf("open sockets %d -> %d, want the control stream of the released machine closed", before, openSockets(t)) - } - } -} - -// openSockets counts the sockets this process holds open. -func openSockets(t *testing.T) int { - t.Helper() - - entries, err := os.ReadDir("/dev/fd") - if err != nil { - t.Fatal(err) - } - n := 0 - for _, entry := range entries { - fd, err := strconv.Atoi(entry.Name()) - if err != nil { - t.Fatal(err) - } - var st syscall.Stat_t - err = syscall.Fstat(fd, &st) - // The listing read through an fd of its own, closed by now. - if errors.Is(err, syscall.EBADF) { - continue - } - if err != nil { - t.Fatal(err) - } - if st.Mode&syscall.S_IFMT == syscall.S_IFSOCK { - n++ - } - } - - return n -} diff --git a/services/provider/firecracker/fakevmm_test.go b/services/provider/firecracker/fakevmm_test.go index b56982911..2251f249d 100644 --- a/services/provider/firecracker/fakevmm_test.go +++ b/services/provider/firecracker/fakevmm_test.go @@ -94,10 +94,6 @@ func TestMain(m *testing.M) { } os.Exit(models.SupervisorFailedExitCode) } - if os.Getenv(oomGuestEnv) == "1" && len(os.Args) == 3 && os.Args[1] == "-transport" { - fmt.Fprintln(os.Stderr, "oom guest:", oomGuest(strings.TrimPrefix(os.Args[2], "unix:"))) - os.Exit(1) - } if os.Getenv(fakeVMMEnv) == "1" && slices.Contains(os.Args[1:], "--exec-file") { if err := fakeJailer(); err != nil { fmt.Fprintln(os.Stderr, "fake jailer:", err) diff --git a/services/provider/firecracker/machine.go b/services/provider/firecracker/machine.go index 5b34ed10e..0761c639b 100644 --- a/services/provider/firecracker/machine.go +++ b/services/provider/firecracker/machine.go @@ -618,12 +618,7 @@ func (p *Provider) attach(ctx context.Context, id, dir, jail string, client *fca } if state.OOM { // The guest kept a kill no host heard; the marker is on disk and it is going, so there is nothing to follow. - if err := p.release(ctx, m); err != nil { - // No map holds this machine yet, so a release that broke off before its settle is the last chance to let the pin and the stream go (SHARD-623). - return nil, errors.Join(err, m.close()) - } - - return nil, nil + return nil, p.release(ctx, m) } // A checkpoint holds the guest frozen, so it runs nothing on the saved crng key until the reseed is in and the thaw follows (SHARD-409). if state.Frozen { From 3655a1645b11b1f93a4502b333381bcda465b31b Mon Sep 17 00:00:00 2001 From: preslavmihaylov Date: Mon, 5 Oct 2026 01:20:20 +0300 Subject: [PATCH 3/4] SHARD-633: build a format a held tag lacks from its cached layers, never the registry --- services/image/image.go | 19 ++++- services/image/image_disk_test.go | 4 +- services/image/image_erofs_test.go | 20 ++++-- services/image/image_format_test.go | 108 ++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 10 deletions(-) create mode 100644 services/image/image_format_test.go diff --git a/services/image/image.go b/services/image/image.go index 1c5977526..167650e28 100644 --- a/services/image/image.go +++ b/services/image/image.go @@ -173,6 +173,19 @@ func (s *Service) pullLocked(ctx context.Context, ref string) (Image, error) { return Image{}, err } + // A held tag is never resolved again, so the format this provider lacks comes from its layers, and a failed build leaves it held. + held, err := s.store.Get(ref) + if err == nil { + if err := s.unpack(ctx, held); err != nil { + return Image{}, err + } + + return s.finish(progress, held) + } + if !errors.Is(err, registry.ErrNotCached) { + return Image{}, err + } + pulled, err := s.store.Pull(ctx, ref, pullReport{p: progress}) if err != nil { return Image{}, errors.Join(err, s.reclaim()) @@ -183,7 +196,11 @@ func (s *Service) pullLocked(ctx context.Context, ref string) (Image, error) { return Image{}, errors.Join(err, s.store.Remove(ref)) } - img, err = s.describe(pulled) + return s.finish(progress, pulled) +} + +func (s *Service) finish(progress *Progress, unpacked registry.Image) (Image, error) { + img, err := s.describe(unpacked) if err != nil { return Image{}, err } diff --git a/services/image/image_disk_test.go b/services/image/image_disk_test.go index 837f4831f..46cf162f1 100644 --- a/services/image/image_disk_test.go +++ b/services/image/image_disk_test.go @@ -44,8 +44,8 @@ func TestPullWithDisksBuildsOneDiskPerDigest(t *testing.T) { t.Fatalf("second Pull: %v", err) } again.Close() - // The tree is already there, so the rebuild says building alone and never unpacking (SHARD-385). - want := []string{image.StatusPulling, image.StatusLayer, image.StatusBuilding, image.StatusPulled} + // The tree and the tag are already held, so the rebuild never unpacks (SHARD-385) and never asks the registry. + want := []string{image.StatusBuilding, image.StatusPulled} if got := statuses(events(t, again)); !slices.Equal(got, want) { t.Errorf("the rebuild of the disk said %v, want %v", got, want) } diff --git a/services/image/image_erofs_test.go b/services/image/image_erofs_test.go index ebe0fbfff..d9f6b362f 100644 --- a/services/image/image_erofs_test.go +++ b/services/image/image_erofs_test.go @@ -25,6 +25,16 @@ func fakeMkfsErofs(t *testing.T) string { return log } +// failingMkfsErofs puts a mkfs.erofs stand-in first on PATH that fails the way a full disk does. +func failingMkfsErofs(t *testing.T) { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "mkfs.erofs"), []byte("#!/bin/sh\necho 'no space' >&2; exit 1\n"), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + func TestPullWithErofsBuildsOneImagePerDigestFromTheTree(t *testing.T) { log := fakeMkfsErofs(t) server, ref := servedImage(t, "app:1.0", map[string]string{"etc/hostname": "box"}) @@ -70,8 +80,8 @@ func TestPullWithErofsBuildsOneImagePerDigestFromTheTree(t *testing.T) { t.Fatalf("second Pull: %v", err) } again.Close() - // The tree is already there, so the rebuild says building alone and never unpacking (SHARD-385). - want := []string{image.StatusPulling, image.StatusLayer, image.StatusBuilding, image.StatusPulled} + // The tree and the tag are already held, so the rebuild never unpacks (SHARD-385) and never asks the registry. + want := []string{image.StatusBuilding, image.StatusPulled} if got := statuses(events(t, again)); !slices.Equal(got, want) { t.Errorf("the rebuild of the image said %v, want %v", got, want) } @@ -81,11 +91,7 @@ func TestPullWithErofsBuildsOneImagePerDigestFromTheTree(t *testing.T) { } func TestPullWithErofsFailsWhenTheToolDoes(t *testing.T) { - dir := t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "mkfs.erofs"), []byte("#!/bin/sh\necho 'no space' >&2; exit 1\n"), 0o755); err != nil { - t.Fatal(err) - } - t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + failingMkfsErofs(t) server, ref := servedImage(t, "app:1.0", map[string]string{"etc/hostname": "box"}) root := t.TempDir() svc := newServiceAt(t, root, server, image.WithErofs()) diff --git a/services/image/image_format_test.go b/services/image/image_format_test.go new file mode 100644 index 000000000..2cfa06449 --- /dev/null +++ b/services/image/image_format_test.go @@ -0,0 +1,108 @@ +package image_test + +import ( + "os" + "path/filepath" + "slices" + "testing" + + "github.com/presmihaylov/shard/services/image" +) + +// formats are the files a VM provider boots from, which a container provider never built for the tags it pulled. +var formats = []struct { + name string + opt image.Option + path func(image.Image) string +}{ + {name: "disk", opt: image.WithDisks(), path: func(img image.Image) string { return img.Disk }}, + {name: "erofs", opt: image.WithErofs(), path: func(img image.Image) string { return img.Erofs }}, +} + +func TestANewFormatKeepsTheDigestTheTagHeld(t *testing.T) { + for _, format := range formats { + t.Run(format.name, func(t *testing.T) { + fakeMkfsErofs(t) + server, ref := servedImage(t, "app:1.0", map[string]string{"etc/hostname": "first"}) + root := t.TempDir() + held, err := newServiceAt(t, root, server).Pull(t.Context(), ref) + if err != nil { + t.Fatalf("Pull: %v", err) + } + pushImage(t, server, "app:1.0", map[string]string{"etc/hostname": "second"}) + + progress := image.NewProgress() + img, err := newServiceAt(t, root, server, format.opt).Pull(image.WithProgress(t.Context(), progress), ref) + if err != nil { + t.Fatalf("Pull with the %s: %v", format.name, err) + } + progress.Close() + if img.Digest != held.Digest { + t.Errorf("the %s pull resolved the tag again: digest %s, want the held %s", format.name, img.Digest, held.Digest) + } + hostname, err := os.ReadFile(filepath.Join(img.RootFS, "etc", "hostname")) + if err != nil || string(hostname) != "first" { + t.Errorf("the tree holds %q (%v), want the held first", hostname, err) + } + if _, err := os.Stat(format.path(img)); err != nil { + t.Errorf("the %s is not there: %v", format.name, err) + } + want := []string{image.StatusBuilding, image.StatusPulled} + if got := statuses(events(t, progress)); !slices.Equal(got, want) { + t.Errorf("the %s pull said %v, want %v", format.name, got, want) + } + }) + } +} + +func TestANewFormatBuildsWithTheRegistryGone(t *testing.T) { + for _, format := range formats { + t.Run(format.name, func(t *testing.T) { + fakeMkfsErofs(t) + server, ref := servedImage(t, "app:1.0", map[string]string{"etc/hostname": "box"}) + root := t.TempDir() + held, err := newServiceAt(t, root, server).Pull(t.Context(), ref) + if err != nil { + t.Fatalf("Pull: %v", err) + } + server.Close() + + img, err := newServiceAt(t, root, server, format.opt).Pull(t.Context(), ref) + if err != nil { + t.Fatalf("Pull with the %s and no registry: %v", format.name, err) + } + if img.Digest != held.Digest { + t.Errorf("digest %s, want the held %s", img.Digest, held.Digest) + } + if _, err := os.Stat(format.path(img)); err != nil { + t.Errorf("the %s is not there: %v", format.name, err) + } + }) + } +} + +func TestAFailedBuildOfANewFormatKeepsTheHeldTag(t *testing.T) { + server, ref := servedImage(t, "app:1.0", map[string]string{"etc/hostname": "box"}) + root := t.TempDir() + held, err := newServiceAt(t, root, server).Pull(t.Context(), ref) + if err != nil { + t.Fatalf("Pull: %v", err) + } + failingMkfsErofs(t) + + svc := newServiceAt(t, root, server, image.WithErofs()) + if _, err := svc.Pull(t.Context(), ref); err == nil { + t.Fatal("Pull with a failing mkfs.erofs returned no error") + } + + images, err := svc.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(images) != 1 || images[0].Digest != held.Digest { + t.Errorf("the store lists %+v, want the held %s", images, held.Digest) + } + if _, err := os.Stat(held.RootFS); err != nil { + t.Errorf("the held tree went: %v", err) + } +} From 2e1864f7457418cf2ffc6930e2b5ae8b20fd9dcd Mon Sep 17 00:00:00 2001 From: preslavmihaylov Date: Mon, 5 Oct 2026 01:30:07 +0300 Subject: [PATCH 4/4] SHARD-637: the cgroup walk skips a child the guest removes mid-walk, so its ENOENT or ENODEV never fails stop or reads as cgroup v1 --- pkg/cgroup/cgroup.go | 39 ++++++++++++----- pkg/cgroup/procs_test.go | 90 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 11 deletions(-) diff --git a/pkg/cgroup/cgroup.go b/pkg/cgroup/cgroup.go index c45fb12cc..712f395b1 100644 --- a/pkg/cgroup/cgroup.go +++ b/pkg/cgroup/cgroup.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strconv" "strings" + "syscall" ) // Root is where a Linux host mounts the cgroup v2 hierarchy. @@ -154,13 +155,17 @@ func Populated(dir string) (bool, error) { return false, nil } -// Procs lists the processes in a cgroup and in every cgroup under it; a cgroup that is gone answers ErrNotFound. +// Procs lists the processes in a cgroup and in every cgroup under it; a cgroup that is gone answers ErrNotFound, and a child that goes mid-walk is skipped. func Procs(dir string) ([]int, error) { var pids []int err := filepath.WalkDir(dir, func(path string, entry fs.DirEntry, err error) error { if err != nil && path == dir && errors.Is(err, fs.ErrNotExist) { return ErrNotFound } + // A guest may remove its own child cgroups at any time, so their ENOENT never reads as a cgroup v1 host (SHARD-637). + if path != dir && vanished(err) { + return fs.SkipDir + } if err != nil { return err } @@ -168,11 +173,14 @@ func Procs(dir string) ([]int, error) { return nil } - raw, err := read(path, "cgroup.procs") + raw, err := os.ReadFile(filepath.Join(path, "cgroup.procs")) + if path != dir && vanished(err) { + return fs.SkipDir + } if err != nil { - return err + return readFailed(path, "cgroup.procs", err) } - for field := range strings.FieldsSeq(raw) { + for field := range strings.FieldsSeq(string(raw)) { pid, err := strconv.Atoi(field) if err != nil { return fmt.Errorf("read %s: %q is not a pid", filepath.Join(path, "cgroup.procs"), field) @@ -244,19 +252,28 @@ func readBound(dir, file string) (int64, error) { } func read(dir, file string) (string, error) { - path := filepath.Join(dir, file) - - raw, err := os.ReadFile(path) - if errors.Is(err, fs.ErrNotExist) { - return "", fmt.Errorf("read %s: %w", path, missing(dir)) - } + raw, err := os.ReadFile(filepath.Join(dir, file)) if err != nil { - return "", fmt.Errorf("read %s: %w", path, err) + return "", readFailed(dir, file, err) } return strings.TrimSpace(string(raw)), nil } +func readFailed(dir, file string, err error) error { + path := filepath.Join(dir, file) + if errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("read %s: %w", path, missing(dir)) + } + + return fmt.Errorf("read %s: %w", path, err) +} + +// vanished is how a cgroup removed under a reader fails: ENOENT once it is gone, ENODEV while the kernel tears its files down. +func vanished(err error) bool { + return errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENODEV) +} + // missing tells the two ways a control file goes missing apart, because they need opposite answers: // a cgroup that is gone is the caller's problem, and a controller that is gone is the host's. func missing(dir string) error { diff --git a/pkg/cgroup/procs_test.go b/pkg/cgroup/procs_test.go index 2aa55605f..727401946 100644 --- a/pkg/cgroup/procs_test.go +++ b/pkg/cgroup/procs_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "slices" "strings" + "syscall" "testing" "github.com/presmihaylov/shard/pkg/cgroup" @@ -65,3 +66,92 @@ func TestProcsRefusesAnEntryThatIsNotAPid(t *testing.T) { t.Fatalf("Procs = %v, want the entry named", err) } } + +// removedMidRead makes dir's cgroup.procs a fifo that answers only once remove is gone, so the walk meets the removal at a known point. +func removedMidRead(t *testing.T, dir, contents, remove string) <-chan error { + t.Helper() + + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("make the cgroup: %v", err) + } + fifo := filepath.Join(dir, "cgroup.procs") + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Fatalf("make the fifo: %v", err) + } + + done := make(chan error, 1) + go func() { + w, err := os.OpenFile(fifo, os.O_WRONLY, 0) + if err != nil { + done <- err + return + } + if err := os.RemoveAll(remove); err != nil { + done <- errors.Join(err, w.Close()) + return + } + _, err = w.WriteString(contents) + done <- errors.Join(err, w.Close()) + }() + + return done +} + +func TestProcsSkipsAChildTheGuestRemovesBeforeItIsRead(t *testing.T) { + dir := t.TempDir() + procs(t, dir, "1101\n") + procs(t, filepath.Join(dir, "c2"), "1103\n") + done := removedMidRead(t, filepath.Join(dir, "c1"), "1102\n", filepath.Join(dir, "c2")) + + got, err := cgroup.Procs(dir) + if err != nil { + t.Fatalf("Procs with a child gone mid-walk: %v", err) + } + if err := <-done; err != nil { + t.Fatalf("remove the child: %v", err) + } + if want := []int{1101, 1102}; !slices.Equal(got, want) { + t.Fatalf("Procs = %v, want %v", got, want) + } +} + +func TestProcsSkipsAChildTheGuestRemovesAfterItsProcsAreRead(t *testing.T) { + dir := t.TempDir() + procs(t, dir, "1101\n") + child := filepath.Join(dir, "c1") + done := removedMidRead(t, child, "1102\n", child) + + got, err := cgroup.Procs(dir) + if err != nil { + t.Fatalf("Procs with a child gone before its own children were listed: %v", err) + } + if err := <-done; err != nil { + t.Fatalf("remove the child: %v", err) + } + if want := []int{1101, 1102}; !slices.Equal(got, want) { + t.Fatalf("Procs = %v, want %v", got, want) + } +} + +func TestProcsNeverReadsAChildMadeAgainAsCgroupV1(t *testing.T) { + dir := t.TempDir() + procs(t, dir, "1101\n") + if err := os.Mkdir(filepath.Join(dir, "c1"), 0o755); err != nil { + t.Fatalf("make the child: %v", err) + } + + got, err := cgroup.Procs(dir) + if err != nil { + t.Fatalf("Procs with a child that has no cgroup.procs yet: %v", err) + } + if want := []int{1101}; !slices.Equal(got, want) { + t.Fatalf("Procs = %v, want %v", got, want) + } +} + +func TestProcsOfACgroupWithoutItsProcsIsNoController(t *testing.T) { + _, err := cgroup.Procs(t.TempDir()) + if !errors.Is(err, cgroup.ErrNoController) { + t.Fatalf("Procs on a cgroup with no cgroup.procs = %v, want ErrNoController", err) + } +}